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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 19, 2011, 00:07 UTC
Message-ID: <7vipub6r3s.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7v8vv78eld.fsf@alter.siamese.dyndns.org>

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

Show 29 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.
>
> ...
> 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.
>
> The following is a tangent that was brought up at $work.

And that is the topic of this follow-up.

Show 16 quoted lines

> Some people might argue that we should skip $T/git-hello in the last case
> and try to find git-hello in a later directory listed in $PATH, but I do
> not personally think that is a right thing to do.  It would make the
> problem harder to diagnose, and more importantly, the fact that the user
> listed $T earlier in the $PATH is a strong indication that the user wants
> the scripts in $T override the scripts with the same name in directories
> that appear later in the $PATH, and we should report when that is not
> happening, either
>
>  (1) when $T/git-hello was found but was not executable; or
>
>  (2) when we cannot read $T and we cannot even tell $T/git-hello exists or
>      not.
>
> So I think it is Ok to be silent only when we see ENOENT like the current
> code does.

Unfortunately, as we are giving "git-hello" to underlying execvp(3), instead of splitting $PATH, prefixing each element of it in front of "git-hello" and trying execv() in a loop, when the call fails, we do not know which component of $PATH caused execvp() to fail, and end up saying "fatal: cannot exec 'git-hello': Permission denied". So in that sense, we are not helping the end user by making it easier to diagnose the problem.

We would need to emulate what execvp() does ourselves (i.e. split $PATH, prefix each component and try execv(), ignoring ENOENT or EACCES while trying next component in $PATH), plus note the first path that got EACCES so that we can report which script (including its leading directories) had trouble executing. Perhaps a simple enough task for beginners.