Re: [PATCH v2 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests

On Fri, Jul 10, 2026 at 05:30:55PM +0000, Michael Montalbo via GitGitGadget wrote:
> diff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh
> index b1682944e2..adb9cec528 100644
> --- a/t/lib-httpd/apply-one-time-script.sh
> +++ b/t/lib-httpd/apply-one-time-script.sh
> @@ -6,21 +6,37 @@
>  #
>  # This can be used to simulate the effects of the repository changing in
>  # between HTTP request-response pairs.
> -if test -f one-time-script
> -then
> -	LC_ALL=C
> -	export LC_ALL
> +#
> +# Apache can run this CGI for concurrent requests (for example a partial fetch
> +# that lazily fetches a missing object while the first response is still in
> +# flight), so the helper claims the marker atomically with a rename, and only
> +# once it has decided to modify the response. A request that loses the race
> +# finds the marker already gone and serves its response unchanged; no request
> +# is left emitting an empty body, which the server would report as HTTP 500.
> +# Scratch files are per-request ($$) so concurrent requests do not clobber each
> +# other.
> +#
> +# The script may run more than once: the marker is consumed when the response
> +# actually changes (the rename after "cmp"), not when the script runs, so a
> +# request whose response is not the targeted one runs the script, sees no
> +# change, and leaves the marker for a later request. That is safe because the
> +# scripts are stateless filters over the captured response.
>  
> -	"$GIT_EXEC_PATH/git-http-backend" >out
> -	./one-time-script out >out_modified
> +test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
>  
> -	if cmp -s out out_modified
> -	then
> -		cat out
> -	else
> -		cat out_modified
> -		rm one-time-script
> -	fi
> +LC_ALL=C
> +export LC_ALL
> +
> +out=out.$$
> +modified=out-modified.$$
> +"$GIT_EXEC_PATH/git-http-backend" >"$out"
> +
> +if ./one-time-script "$out" 2>/dev/null >"$modified" &&

Is it intentional that we swallow stderr of this script now? We didn't
before. I assume that this is to swallow the error in case the script
got removed by the concurrent request?

Patrick