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

Linus Torvalds <torvalds@linux-foundation.org> wrote:

What you should have said is that the patch is fine in principle, since it does fix a pretty serious bug (important tools ignoring ENOSPC), but you'd prefer that it continue to ignore EPIPE.

With a name like yours, being more positive would go a long way toward encouraging (or rather *not discouraging*) contributions.

I have to disagree. There may be precedent for hiding EPIPE errors, but that is not the norm among command line tools wrt piped stdout.

First of all, one has to work just to get such an error. These days, most people use a shell that doesn't ignore, handle or block SIGPIPE, so direct use of a program like git-log or git-diff gets the signal directly, and there's no EPIPE error. E.g., try "cat", and you see it gets SIGPIPE (141=128+SIGPIPE(13)):

    $ seq 90000|(cat; echo $? >&2) | head -1 > /dev/null
    141

However, you can tweak your shell to handle/ignore SIGPIPE. Also, some porcelain scripts do ignore SIGPIPE. Then, a program run in that environment does see EPIPE. Consider how a few other non-interactive programs work when one of their stdout-writing syscalls fails with EPIPE:

Try GNU diff and sed: [now, using shorter ":" in place of more realistic head -1]

    $ (trap '' PIPE; seq 90000|diff - /dev/null;echo $? >&2)| :
    diff: standard output: Broken pipe
    2
    $ (trap '' PIPE; seq 90000|sed s/a/b/; echo $? 1>&2)| :
    /bin/sed: couldn't write 5 items to stdout: Broken pipe
    seq: write error: Broken pipe
    4

Try tee (from GNU coreutils):

    $ (trap '' PIPE; seq 90000|tee /dev/null; echo $? >&2) | :
    tee: standard output: Broken pipe
    tee: write error
    1

sort, tac, cut, fold, od, head, tail, tr, uniq, etc. all work the same way, if you're using the coreutils. But perhaps that's not fair, since I maintain the coreutils. And there is some variance among how other- vendor versions of those tools work. E.g., Solaris 10's /bin/cat diagnoses the error, but neither /bin/sort nor /bin/diff do.

As for version control tools, monotone does what I'd expect in this situation: "mtn diff" reports the failure and exits nonzero.

svn and cvs also report the error, although they both exit successfully in spite of that. I tried both log and diff commands for each tool. cvs catches the signal, svn doesn't.

mercurial and darcs totally ignore the write error and SIGPIPE, so there is no way to determine from stderr or exit code whether their writes complete normally.

For any tool whose output might be piped to another, the pipe-writing tool should exit nonzero for any write error. Otherwise, its exit code ends up being a lie, pretending success but, in effect, covering up for a failure. In general, I've found that papering over syscall failures makes higher-level problems harder to diagnose.

Do you really want git-log to continue to do this?

    $ (trap '' PIPE; git-log; echo $? >&2 ) | :
    0

With my patch, it does this:

    $ (trap '' PIPE; ./git-log; echo $? >&2 ) | :
    fatal: write failure on standard output: Broken pipe
    128

Since Junio is making an effort to "conform", I too will make the effort when contributing to git.