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

From: Jim Meyering <jim@meyering.net>
Date: May 28, 2007, 20:23 UTC
Message-ID: <87veecvgsn.fsf@rho.meyering.net>
In-Reply-To: <20070528190529.GA10656@fiberbit.xs4all.nl>

Marco Roeland <marco.roeland@xs4all.nl> wrote:

Show 6 quoted lines

> On monday May 28th 2007 at 20:19 Jim Meyering wrote:
>> Also, to be consistent, don't ignore EPIPE write failures.
>
> In practice I agree with someone else on this thread that EPIPE _is_
> different. In a way the responsibility doesn't lie with the writer but
> with the reader.

Do you think it's ok for git-rev-list _not_ to diagnose an erroneous command like this (i.e., to exit(0)):

    git-rev-list HEAD | sync

where "sync" could be any command that exits successfully without reading any input?

Is it ok that it is currently *impossible* to diagnose that failure by looking at exit codes?

> But just out of curiosity is there an easy way to test the EPIPE
> behaviour? I cite a piece of the "changelog.Debian" file from the

There are some examples here: http://thread.gmane.org/gmane.comp.version-control.git/48469/focus=48617

...

Show 6 quoted lines

> The mentioned "test-case" as used in "git log -n1 | exit 1" doesn't
> produce an error in my Debian 'sid' bash, either with or without your
> patch, so it doesn't seem to have any effect there? Whereas probably in
> a "default" bash (don't know if upstream has changed it's mind already!)
> with your patch (i.e. the EPIPE special casing removal) it will again
> probably introduce these annoying (for interactive use) errors.

As I just said in reply to Linus, the EPIPE handling difference is independent of what version of bash you use.

> Thanks for your patch anyway, the "fcntl" diagnosis is a really useful
> technique to know, and IMVHO also useful for git; although perhaps not
> very portable for all platforms.

It appears to be portable enough. fcntl/F_GETFD support is required by POSIX, and has been around for ages. FWIW, it's also used in git's daemon.c and sha1_file.c.