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

From: Jim Meyering <jim@meyering.net>
Date: May 30, 2007, 19:43 UTC
Message-ID: <87abvmp05o.fsf@rho.meyering.net>
In-Reply-To: <7vd50i2o9c.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:

Show 39 quoted lines

> Jim Meyering <jim@meyering.net> writes:
>> 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.
>
>> 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

I'm not sure which "Broken pipe" message you mean.

There are two types of "Broken pipe" messages. There's the old, verbose one from bash that includes script line-number, pid, and killed command name. Old, unpatched versions of bash print that message whenever bash kills a process with SIGPIPE.

Then there's the application (EPIPE) one, that can be printed by a writing application like git,cat,seq,etc. only when SIGPIPE stops a write but doesn't kill the writer. In that case, the write syscall fails with errno==EPIPE, and if it's diagnosed by the application, you get e.g.,

  seq: write error: Broken pipe

Since the script above is not ignoring SIGPIPE, the git-log process is killed by bash-3.0, and that old version of bash announces the killing with the verbose message:

  /t/bp-demo: line 2: 14474 Broken pipe             git-log

Any other patched or newer version of bash will print the single requested line on stdout and nothing on stderr.

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

Right. That's why no one is using such broken shells anymore. And why no porcelain tools incur the penalty of ignoring SIGQUIT anymore either.

Show 6 quoted lines

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

I haven't debugged the old bash to see why that first one fails to provoke a broken pipe message (from bash). Unless you add a line like "trap '' PIPE" before it, bash kills the writer with SIGPIPE, and so my patch is irrelevant, because the failing write syscall never returns.

Show 5 quoted lines

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

Are you presuming SIGPIPE is ignored?

> Maybe you are talking about your updated patch?

I was talking about the initial patch, or the one that also removed the errno == EPIPE tests.