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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 18, 2011, 21:16 UTC
Message-ID: <7v4o5v8dlp.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7v8vv78eld.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 38 quoted lines

> There appears to be a regression in the codepath between git wrapper and
> run_commands API.
>
> 	$ T=/var/tmp/test-commands
> 	$ mkdir $T
> 	$ cat >$T/git-hello <<\-EOF
> 	#!/bin/sh
> 	echo hello
> 	EOF
> 	$ chmod +x $T/git-hello
> 	$ oPATH=$PATH
> 	$ PATH=$T:$PATH
> 	$ export PATH
> 	$ git hello
> 	hello
>
> So far, I added a "hello" subcommand to "git", and it runs correctly.
>
> Now, when I make the script non-executable, this is what I get from
> 'maint':
>
> 	$ chmod a-x $T/git-hello
> 	$ git hello
> 	fatal: cannot exec 'git-hello': Permission denied
>
> But with 'master', we get a disturbing output:
>
> 	$ git hello
>         fatal: $
>
> Note that we can observe the same regression if you instead make $T
> unreadable with:
>
> 	$ chmod 755 $T/git-hello ;# make it executable again
> 	$ chmod a-rwx $T ;# but that directory cannot be read
>         $ git hello
>
> So that is the "regression" part.

This bisects down to ebec842 (run-command: prettify -D_FORTIFY_SOURCE workaround, 2011-03-16).

And we should really have been more careful.  Look at what the patch does:

    Sometimes when there is an output error, especially right before exit,
    there really is nothing to be done.  The obvious solution, adopted in
    v1.7.0.3~20^2 (run-command.c: fix build warnings on Ubuntu,
    2010-01-30), is to save the return value to a dummy variable:
    
    	ssize_t dummy;
    	dummy = write(...);
    
    But that (1) is ugly and (2) triggers -Wunused-but-set-variable
    warnings with gcc-4.6 -Wall, so we are not much better off than when
    we started.

    Instead, use an "if" statement with an empty body to make the intent
    clear.
    
    	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?

I'm reverting it for now, but am open to a submission of a proper fix after 1.7.5.