Junio C Hamano <gitster@xxxxxxxxx> writes:
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html