Re: [PATCH] run-command: prettify -D_FORTIFY_SOURCE workaround

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 16, 2011, 05:37 UTC
Message-ID: <7v7hbzaan9.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110316035135.GA30348@elie>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 5 quoted lines

> Instead, use an "if" statement with an empty body to make the intent
> clear.
>
> 	if (write(...))
> 		; /* yes, yes, there was an error. */

Yuck --- and that is not meant against your workaround, but against the compiler bogosity. The above is reasonable (for some definition of the word) and the comment makes the yuckiness tolerable by being somewhat amusing.

But your comment in the actual patch is not amusing at all.

It certainly is _not_ "ok" to see errors from write(2); we are _ignoring_ the error because at that point in the codepath there isn't any better alternative. The unusual "if ()" whose condition is solely for its side effect, with an empty body, is a strong enough sign to any reader that there is something fishy going on, and it would be helpful to the reader to hint _why_ such an unusual construct is there. It would be much better for the longer term maintainability to say at least "gcc" in the comment, i.e.

	if (write(...))
        	; /* we know we are ignoring the error, mr gcc! */

or something.

Thanks for another amusing patch.