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

[Jeff King <peff@peff.net>]

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.

  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.

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.

-Peff