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

From: Junio C Hamano <junkio@cox.net>
Date: May 28, 2007, 20:44 UTC
Message-ID: <7v4plwd6f0.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <87646cx13d.fsf@rho.meyering.net>

Jim Meyering <jim@meyering.net> writes:

Show 18 quoted lines

> Subject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
> ...
> You can demonstrate this with git's own --version output, too:
> (but git --help detects the failure without this patch)
>
>     $ ./git --version > /dev/full
>     fatal: write failure on standard output: No space left on device
>     [Exit 128]
>
> Note that the fcntl test (for whether the fileno may be closed) is
> required in order to avoid EBADF upon closing an already-closed stdout,
> as would happen for each git command that already closes stdout; I think
> update-index was the one I noticed in the failure of t5400, before I
> added that test.
>
> Also, to be consistent, don't ignore EPIPE write failures.
>
> Signed-off-by: Jim Meyering <jim@meyering.net>

I do not think anybody has much objection about the change to handle_internal_command() in git.c you made. Earlier we relied on exit(3) to close still open filehandles (while ignoring errors), and you made the close explicit in order to detect errors.

But "to be consistent" above is not a very good justification.

In the presense of shells that give "annoying" behaviour (we have to remember that not everybody have enough privilege, expertise, or motivation to update /bin/sh or install their own copy in $HOME/bin/sh), "EPIPE is different from other errors" is a more practical point of view, I'd have to say. IOW, it is not clear if it is a good thing "to be consistent" to begin with.

I would have appreciated if this were two separate patches. I think the EPIPE one is an independent issue. We could even make it a configuration option to ignore EPIPE ;-)