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

Currently, when git-diff writes to a full device or gets an I/O error,
it fails to detect the write error:

    $ git-diff |wc -c
    3984
    $ git-diff > /dev/full && echo ignored write failure
    ignored write failure

git-log does the same thing:

    $ git-log -n1 > /dev/full && echo ignored write failure
    ignored write failure

Each git command should report such a failure.
Some already do, but with the patch below, they all do, and we
won't have to rely on code in each command's implementation to
perform the right incantation.

    $ ./git-log -n1 > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]
    $ ./git-diff > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]

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.

Signed-off-by: Jim Meyering <redacted>
---
 git.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/git.c b/git.c
index 29b55a1..a7d6515 100644
--- a/git.c
+++ b/git.c

@@ -308,6 +308,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 	for (i = 0; i < ARRAY_SIZE(commands); i++) {
 		struct cmd_struct *p = commands+i;
 		const char *prefix;
+		int status;
 		if (strcmp(p->cmd, cmd))
 			continue;

@@ -321,7 +322,15 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 			die("%s must be run in a work tree", cmd);
 		trace_argv_printf(argv, argc, "trace: built-in: git");

-		exit(p->fn(argc, argv, prefix));
+		status = p->fn(argc, argv, prefix);
+
+		/* Close stdout if necessary, and diagnose any failure.  */
+		if (0 <= fcntl(fileno (stdout), F_GETFD)
+		    && (ferror(stdout) || fclose(stdout)))
+			die("write failure on standard output: %s",
+			    strerror(errno));
+
+		exit(status);
 	}
 }

--

1.5.2.73.g18bece

On Sat, 26 May 2007, Jim Meyering wrote:

Each git command should report such a failure.
Some already do, but with the patch below, they all do, and we
won't have to rely on code in each command's implementation to
perform the right incantation.

The patch is wrong.

Some write errors are expected and GOOD.

For example, EPIPE should not be reported. It's normal. The user got 
bored. It might be hidden by the SIGPIPE killing us, but regardless, 
reporting it for the normal log/diff thing is just not correct. EPIPE 
isn't an error, it's a "ok, nobody is listening any more".

Also, PLEASE don't do this:

+		if (0 <= fcntl(fileno (stdout), F_GETFD)

That's totally unreadable to any normal human.

You don't say "if zero is smaller or equal to X". You say "if X is larger 
than or equal to zero". Stop messing with peoples minds, dammit!

Anybody who thinks that code like this causes fewer errors is just fooling 
himself. It causes *more* bugs, because people have a harder time reading 
it.

Maybe you and Junio have taught yourself bad manners, but you're a tiny 
tiny part of humanity or the development community. Junio can do it just 
because while he's just a single person, he's a big part of the git coding 
base, but anybody else who does it should just be shot.

			Linus