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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 15, 2020, 18:32 UTC
Message-ID: <xmqqblr4k1t0.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20200113183313.GA2087@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 18 quoted lines

> 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).

I am not sure what we are aiming for. Are we making sure the command behaves well in the hands of end users, who may write a script that consumes only early parts of the input that is needed for its use and stops reading, or are we just aiming to claim "all our tests pass"? I was hoping that we would be doing the former, and I would understand if the suggestion were "sed 1q" for that exact reason.

IOW, shouldn't we be fixing the part that drives the external process, so that the test "passes" even with such a "broken" filter?

>> 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.

Yes, and I was hoping that we do not have to touch the test if we did the latter.

Show 9 quoted lines

> 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).

Yeah, I share that temptation.