Re: [PATCH v6 12/11] run-command: don't try to execute directories

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 25, 2017, 01:58 UTC
Message-ID: <xmqqr30hxn72.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170425001724.GG28740@aiede.svl.corp.google.com>

Jonathan Nieder <jrnieder@gmail.com> 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".

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.

Show 11 quoted lines

>> +		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 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?