Re: [PATCH v2 4/4] diff --no-index: support reading from named pipes

> On Wed, Jul 05, 2023 at 08:49:30PM +0100, Phillip Wood wrote:
> 
>> +test_expect_success PIPE 'diff --no-index refuses to diff a named pipe and a directory' '
>> +	test_when_finished "rm -f pipe" &&
>> +	mkfifo pipe &&
>> +	{
>> +		(>pipe) &
>> +	} &&
>> +	test_when_finished "kill $!" &&
>> +	test_must_fail git diff --no-index -- pipe a 2>err &&
>> +	grep "fatal: cannot compare a named pipe to a directory" err
>> +'
>> +
>> +test_expect_success PIPE,SYMLINKS 'diff --no-index reads from pipes' '
>> +	test_when_finished "rm -f old new new-link" &&
>> +	mkfifo old &&
>> +	mkfifo new &&
>> +	ln -s new new-link &&
>> +	{
>> +		(test_write_lines a b c >old) &
>> +	} &&
>> +	test_when_finished "! kill $!" &&
>> +	{
>> +		(test_write_lines a x c >new) &
>> +	} &&
>> +	test_when_finished "! kill $!" &&
>> +
>> +	cat >expect <<-EOF &&
>> +	diff --git a/old b/new-link
>> +	--- a/old
>> +	+++ b/new-link
>> +	@@ -1,3 +1,3 @@
>> +	 a
>> +	-b
>> +	+x
>> +	 c
>> +	EOF
>> +
>> +	test_expect_code 1 git diff --no-index old new-link >actual &&
>> +	test_cmp expect actual
>> +'
> 
> I've had t4053 hang for me once or twice in the last few days. I haven't
> managed to pinpoint the problem, but I did notice that running it with
> --stress seems to occasionally fail in thie "reads from pipes" test.
> 
> The problem is that the "kill" is racy. Even after we've read all of the
> input from those sub-processes, they might still be hanging around
> waiting to exit when our test_when_finished runs. And then kill will
> return "0". So I think we need to either:
> 
>    1. Soften the when_finished to "kill $! || true" so that we are OK if
>       they are still there.