Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.

From: Junio C Hamano <junkio@cox.net>
Date: May 30, 2007, 17:51 UTC
Message-ID: <7vd50i2o9c.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <87myzmr152.fsf@rho.meyering.net>

Jim Meyering <jim@meyering.net> writes:

Show 27 quoted lines

> Junio C Hamano <junkio@cox.net> wrote:
>> Jim Meyering <jim@meyering.net> writes:
>>
>>> Of course error messages are annoying when your short-pipe-read is
>>> _deliberate_ (tho, most real uses of git tools will actually get no
>>> message to be annoyed about[*]), but what if there really *is* a mistake?
>>> Try this:
>>>
>>>     # You want to force git to ignore the error.
>>>     $ trap '' PIPE; git-rev-list HEAD | sync
>>>     $
>>
>> It is perfectly valid (although it is stupid) for a Porcelain
>> script to do this:
>>
>>     latest_by_jim=$(git log --pretty=oneline --author='Jim' | head -n 1)
>>     case "$latest_by_jim" in
>>     '') echo "No commit by Jim" ;;
>>     *)  # do something interesting on the commit
>>         ;;;
>>     esac
>
> Hi Junio,
>
> The above snippet (prepending a single #!/bin/bash line) doesn't provoke
> an EPIPE diagnostic from my patched git.  In fact, even if you're using
> an old, unpatched version of bash, it provokes *no* diagnostic at all.

Show 7 quoted lines

> To provoke a diagnostic (from bash, not git), using old unpatched bash,
> you need a script doing output from a subshell, e.g.:
>
>     #!/tmp/bash-3.0/bash
>     for x in 1; do
>       git-log
>     done | head -1

I haven't thought it through, but isn't the above example only talking about the "Broken pipe" message? Surely, you would get that message from older Bash if you have a shell loop on the upstream side of the pipe no matter what we (the command that is run by the shell loop) do, and trap is needed to squelch it.

But I do not see how this pipeline, where git-rev-list produces more than what fits in the in-kernel pipe buffer:

	"git-rev-list a lot of data | head -n 1"

would not catch EPIPE and say "Broken Pipe" with your patch. Especially if the downstream is sufficiently slow (say, replace it with "(sleep 10 && head -n 1)", perhaps), wouldn't the upstream produce enough without being read, gets stuck on write, and when the downstream exits, it would notice its write(2) failed with EPIPE, wouldn't it?

Maybe you are talking about your updated patch?

Show 7 quoted lines

> ...[patch to make git/EPIPE exit nonzero, but with no diagnostic]
>
> Thank you for taking the time to reply and to come up with a compromise.
> At first I thought this would be a step in the right direction, but,
> now that I understand how infrequently EPIPE actually comes into play,
> I think it'd be better to avoid a half-measure fix, since that would
> just perpetuate the idea that EPIPE is worth handling specially.

After having read what Linus said about how the "fixed" one would behave differently, depending on the amount of data we produce before the consumer says "I've seen enough" and depending on the amount of data that would fit in the in-kernel pipe buffer, I no longer think the compromise patch you mention above is improvement anymore.