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

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.

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