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

Junio C Hamano [off-list ref] wrote:

quoted

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

This makes the final write failure trump the breakage p->fn()
already diagnosed, doesn't it?

Yes.  Are there circumstances in which a nonzero status from
some cmd_* function would mean something so grave that you
wouldn't also want to know that standard output is incomplete
or corrupt (and possibly use a different exit status)?
So far, after a quick and incomplete survey, I haven't found any.

However, if some git command is documented to exit with
status N for some listed values of N, e.g.,
    1 A happened
    2 B happened
    3 any other failure
then the above choice of dying with "die" would be wrong.

E.g. git-diff's --exit-code comes close:

       --exit-code
           Make the program exit with codes similar to diff(1). That is, it
           exits with 1 if there were differences and 0 means no differences.

but doesn't say how it handles errors.

[ OT: Perhaps that documentation should be changed to look more like diff's,
  so that it says there is a different exit code for the third
  case (some failure):

    $ diff --help|tail -3|head -1
    Exit status is 0 if inputs are the same, 1 if different, 2 if trouble.
]

Maybe if (fcntrl(...) >=0 )
should read if (!status && fcntrl(...) >= 0).

No, because then something like git-diff's --exit-code could hide
a write error.

If you want to preserve the exit status, then it should be enough
to call set_die_routine with a function that will work just like
"die" but exit with a specified (status) value.