Re: [PATCH v3 01/10] built-in add -i/-p: treat SIGPIPE as EOF

From: Jeff King <peff@peff.net>
Date: Jan 13, 2020, 18:33 UTC
Message-ID: <20200113183313.GA2087@coredump.intra.peff.net>
In-Reply-To: <20200113170417.GK32750@szeder.dev>

On Mon, Jan 13, 2020 at 06:04:17PM +0100, SZEDER Gábor wrote:

Show 12 quoted lines

> After looking into it, the issue seems to be sending data to the
> broken diffFilter process.  So in that test the diff is "filtered"
> through 'echo too-short', which exits real fast, and doesn't read its
> standard input at all (well, apart from e.g. the usual kernel
> buffering that might happen on a pipe between the two processes).
> Making sure that the diffFilter process reads all the data before
> exiting, i.e. changing it to:
> 
>   test_config interactive.diffFilter "cat >/dev/null ; echo too-short" &&
> 
> made the test reliable, with over 2000 --stress repetitions, and that
> with only a single "y" on 'git add's stdin.

Yeah, I agree the test should be changed. What you wrote above was my first thought, too, but I think "sed 1d" is actually a more realistic test (and is shorter and one fewer process).

> Now, merely tweaking the test is clearly insufficient, because we not
> only want the test to be realiable, but we want 'git add' to die
> gracefully when users out there mess up their configuration.

I also agree that it would be nice to deal with this for real-world cases. I suspect it's not something that would come up a lot, though.

Show 6 quoted lines

> Ignoring SIGPIPE can surely accomplish that, but I'm not sure about
> the scope.  I mean your patch seems to ignore SIGPIPE basically for
> almost the whole 'git add -(i|p)' process, but perhaps it should be
> limited only to the surroundings of the pipe_command() call running
> the diffFilter, and be done as part of the next patch adding the 'if
> (diff_filter)' block.

The scope there is probably OK in practice. In my opinion SIGPIPE is usually _not_ what the behavior we want. If we're carefully checking our write() return values, then we'd get EPIPE in such an instance and behave appropriately. And if we're not checking our write() return values, that's generally a bug that ought to be fixed.

The big exception is when we are writing copious output to stdout (or the pager) via printf() or similar, and want to die rather than continue writing output nobody will see. But I don't think git-add really counts as generating a lot of output, where EPIPE could prevent us from doing useless work (unlike, say, git-log).

> Furthermore, I'm worried that by simply ignoring SIGPIPE we might just
> ignore a more fundamental issue in pipe_command(): shouldn't that
> function be smart enough not to write() to a fd that has no one on the
> other side to read it in the first place?!

Maybe. As you noted below, checking for POLLERR is racy. Seeing that we "can" write to an fd and doing it to discover what write() returns (whether error or not) doesn't seem like the worst strategy. If the caller cares about pipe death, then it needs to be handling SIGPIPE anyway.

I really wish there was a way to set a handler for SIGPIPE that tells _which_ descriptor caused it. Because I think logic like "die if it was fd 1, ignore and let write() return EPIPE otherwise" is the behavior we'd like. But I don't think there's a portable way to do so.

I've been tempted to say that we should just ignore SIGPIPE everywhere, and convert even copious-output programs like git-log to just check for errors (they could probably even just check ferror(stdout) for each commit we output, if we didn't want to touch every printf call).

-Peff