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

From: Jim Meyering <jim@meyering.net>
Date: May 30, 2007, 07:12 UTC
Message-ID: <874pluss22.fsf@rho.meyering.net>
In-Reply-To: <7v646b5gw5.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:

Show 18 quoted lines

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