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

Marco Roeland [off-list ref] wrote:

The if statement with 'fcntl' is missing a brace, it should be:

+		if ((fcntl(fileno (stdout), F_GETFD) >= 0)
+		    && (ferror(stdout) || fclose(stdout)))

Thank you.
That was a stale patch (from before I compiled/tested, obviously).
I intended this:

		if (fcntl(fileno (stdout), F_GETFD) >= 0
		    && (ferror(stdout) || fclose(stdout)))

Here's the correct one:

Subject: [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 and every 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.

Also, to be consistent, don't ignore EPIPE write failures.

Signed-off-by: Jim Meyering <redacted>
---
 git.c             |   11 ++++++++++-
 merge-recursive.c |    3 ---
 write_or_die.c    |    4 ----
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/git.c b/git.c
index 29b55a1..7b45a73 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 (fcntl(fileno (stdout), F_GETFD) >= 0
+		    && (ferror(stdout) || fclose(stdout)))
+			die("write failure on standard output: %s",
+			    strerror(errno));
+
+		exit(status);
 	}
 }

diff --git a/merge-recursive.c b/merge-recursive.c
index 8f72b2c..bfa4335 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -523,9 +523,6 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)
 	while (size > 0) {
 		long ret = write_in_full(fd, buf, size);
 		if (ret < 0) {
-			/* Ignore epipe */
-			if (errno == EPIPE)
-				break;
 			die("merge-recursive: %s", strerror(errno));
 		} else if (!ret) {
 			die("merge-recursive: disk full?");

diff --git a/write_or_die.c b/write_or_die.c
index 5c4bc85..fadfcaa 100644
--- a/write_or_die.c
+++ b/write_or_die.c

@@ -41,8 +41,6 @@ int write_in_full(int fd, const void *buf, size_t count)
 void write_or_die(int fd, const void *buf, size_t count)
 {
 	if (write_in_full(fd, buf, count) < 0) {
-		if (errno == EPIPE)
-			exit(0);
 		die("write error (%s)", strerror(errno));
 	}
 }

@@ -50,8 +48,6 @@ void write_or_die(int fd, const void *buf, size_t count)
 int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
 {
 	if (write_in_full(fd, buf, count) < 0) {
-		if (errno == EPIPE)
-			exit(0);
 		fprintf(stderr, "%s: write error (%s)\n",
 			msg, strerror(errno));
 		return 0;

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.

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
Debian version of the bash shell. In Debian, as earlier in many other
distributions, the annoying EPIPE error was "fixed" in version 2.0.3-3
from 19 dec 1999.

========================================================================
* Define DONT_REPORT_SIGPIPE: We don't want to see `Broken pipe' messages
  when a job like `cat jobs.c | exit 1' is executed. Fixes part of #7047,
  #10259, #10433 and #10494. Comment from the upstream author: "The default
  bash behavior with respect to the exit status of a pipeline will not
  change.  Changing it as suggested in the discussion of #10494 would render
  bash incompatible with every other shell out there.". Closed these reports.

  -- Matthias Klose [off-list ref]  Sun, 19 Dec 1999 15:58:43 +0100
========================================================================

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.

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