Re: [PATCH] update-ref: fix streaming of status updates

From: Jeff King <peff@peff.net>
Date: Sep 15, 2021, 17:13 UTC
Message-ID: <YUIptJNN5DxrkydV@coredump.intra.peff.net>
In-Reply-To: <1e9e62a2c5f74db91aaded83783076c28b757836.1630659922.git.ps@pks.im>

Show 28 quoted lines

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 4506cd435b..1e754e258f 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -1598,6 +1598,38 @@ test_expect_success 'transaction cannot restart ongoing transaction' '
>  	test_must_fail git show-ref --verify refs/heads/restart
>  '
>  
> +test_expect_success PIPE 'transaction flushes status updates' '
> +	mkfifo in out &&
> +	(git update-ref --stdin <in >out &) &&
> +
> +	exec 9>in &&
> +	test_when_finished "exec 9>&-" &&
> +
> +	echo "start" >&9 &&
> +	echo "start: ok" >expected &&
> +	read line <out &&
> +	echo "$line" >actual &&
> +	test_cmp expected actual &&
> +
> +	echo "create refs/heads/flush $A" >&9 &&
> +
> +	echo prepare >&9 &&
> +	echo "prepare: ok" >expected &&
> +	read line <out &&
> +	echo "$line" >actual &&
> +	test_cmp expected actual &&

I think this test may be racy. I saw a strange failure from it in CI:

  https://github.com/peff/git/runs/3605506649?check_suite_focus=true#step:5:6734

I can't reproduce the problem locally with "--stress", but the failure there is on macOS (and likewise, a nearby run failed with a timeout just for macOS, which could be caused by a racy deadlock).

I'm guessing the problem may be the continued opening and closing of "out" by the read calls from the shell. The update-ref process may get SIGPIPE as a result, depending on the write timing.

The solution is to use open a descriptor for "out" like we do for "in", and then read from that. See 4783e7ea83 (t0008: avoid SIGPIPE race condition on fifo, 2013-07-12) for some prior art.

-Peff