Re: [REGRESSION] git-wrapper to run-commands codepath regression

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 18, 2011, 22:17 UTC
Message-ID: <20110418221720.GA16977@elie>
In-Reply-To: <7v4o5v8dlp.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 5 quoted lines

>     	if (write(...))
>     		; /* yes, yes, there was an error. */
>
> No, a non-zero return is not an error from the write(2) system call.
> I cannot believe both of us didn't spot it.  What were we smoking?

Yagh.

	if (write(child_err, "fatal: ", 7) ||
	    write(child_err, msg, len) ||
	    write(child_err, "\n", 1))
		; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */

There are two unusual conditions in which this could fail:

 - it doesn't write anything at all, in which case the return value
   is -1.
 - a partial write, for example if writing to an almost-full pipe.

I suppose in a calmer time, a better fix will look like

	if (write_in_full(child_err, "fatal: ", 7) != 7 ||
	    write_in_full(child_err, msg, len) != len ||
	    write_in_full(child_err, "\n", 1) != 1)
		/* yes, yes, ...

and gcc will have told us something potentially useful.

Thanks for catching it.