Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@xxxxxxxxx> writes:
>> Until we switched from using execvp to execve, the symptom was very
>> subtle: it only affected the error message when a program could not be
>> found, instead of affecting functionality more substantially.
>
> Hmph, what if you had bin/ssh/ directory and bin2/ssh executable and
> had bin:bin2 listed in this order in your $PATH? Without this change
> you'll get an error and that's the end of it. With this change,
> you'd be able to execute bin2/ssh executable, no? So I am not sure
> if I agree with the "this is just an error message subtlety".
I think you misunderstood what I meant. execvp() does not have this
bug. In current master, run_command() (within function sane_execvp())
double-checks execvp()'s work when it sees EACCES to decide whether
to convert it into a more user-friendly ENOENT. Because of this bug,
if you have a bin/ssh/ directory and no bin2/ssh executable, instead
of reporting this condition as a user-friendly ENOENT, it would leave
it as EACCES.
> What does execvp() do when bin/ssh/ directory, bin2/ssh
> non-executable regular file, and bin3/ssh executable file exist and
> you have bin:bin2:bin3 on your $PATH? That is what locate_in_PATH()
> should emulate, I would think.
Good catch.
$ mkdir -p $HOME/bin1/greet
$ mkdir $HOME/bin2
$ printf '%s\n' 'echo bin2' >$HOME/bin2/greet
$ mkdir $HOME/bin3
$ printf '%s\n' '#!/bin/sh' 'echo bin3' >$HOME/bin3/greet
$ chmod +x $HOME/bin3/greet
$ PATH=$HOME/bin1:$HOME/bin2:$HOME/bin3:$PATH perl -e 'exec("greet")'
bin3
It needs to skip over non-executable files.
I think this means we'd want to reuse something like is_executable
from help.c.
[...]
>>> + if (!stat(buf.buf, &st) && S_ISREG(st.st_mode))
>>> return strbuf_detach(&buf, NULL);
>>
>> Should this share code with help.c's is_executable()?
>>
>> I suppose not, since that would have trouble finding scripts without
>> the executable bit set.
I confused myself about the script special-case: they are supposed to
have the executable bit set, too --- the special-casing is just about
lacking #!/bin/sh at the top (and hence not being directly executable
with execve).
>> I was momentarily nervous about what happens if this gets run on
>> Windows. This is just looking for a file's existence, not
>> executability, so it should be fine.
>
> When we are looking for "ssh" with locate_in_PATH(), shouldn't we
> look for "ssh.exe" on Windows, though?
Fortunately this is in a #if !defined(GIT_WINDOWS_NATIVE) block. It's
probably worth adding a comment so people know not to rely on it
matching Windows path search behavior.
Thanks for looking it over,
Jonathan