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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Eff

Thanks for reporting this

On 09/08/2023 18:17, Jeff King wrote:
> 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.

I think this is the easiest option, I'll send a patch later today

>    2. If the diff command completed as expected, it should be safe to
>       "wait" for each of them to confirm that they successfully wrote
>       everything. I'm not sure it buys us much over testing the output
>       from "diff", though.

If the diff output is OK that's I think that's all we really care about.

> I still don't see where the hang comes from, though. It might be
> unrelated. I'll try to examine more next time I catch it in the act.

I had a look at the tests again and nothing jumped out at me. If you do 
manage to catch it hanging we should at least we should be able to tell 
which test is causing the problem.

Thanks again,

Phillip