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

[Junio C Hamano <gitster@pobox.com>]

Brandon Williams <bmwill@google.com> writes:

> This is due to only checking 'access()' when locating an executable in
> PATH, which doesn't distinguish between files and directories.  Instead
> use 'stat()' and check that the path is to a regular file.  Now
> run-command won't try to execute the directory 'git-remote-blah':
>
> 	$ git ls-remote blah://blah
> 	fatal: Unable to find remote helper for 'blah'

The above is not a very interesting example.  

More important is that $PATH may have a directory with
git-remote-blah directory (your setup above) and then another
directory with the git-remote-blah executable that the user wanted
to use.  Without this change, we won't get to the real one, and that
makes this change truly valuable.

The added test demostrates the "uninteresting" behaviour.  Even
though it is correct and technically sufficient, it would make it
more relevant to do something like this:

	mkdir -p bin/blah bin2 &&
	write_script bin2/blah <<-\EOF &&
	echo We found blah in bin2
	EOF
	PATH=bin:$PATH test_must_fail ... what you have
	...
	PATH=bin:bin2:$PATH test-run-command run-command blah >actual &&
	bin2/blah >expect &&
	test_cmp expect actual

as the point of locate_in_PATH() is to successfully find one,
without getting confused by an earlier unusable one.

Thanks.

> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  run-command.c          | 3 ++-
>  t/t0061-run-command.sh | 7 +++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index a97d7bf9f..ece0bf342 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -127,6 +127,7 @@ static char *locate_in_PATH(const char *file)
>  
>  	while (1) {
>  		const char *end = strchrnul(p, ':');
> +		struct stat st;
>  
>  		strbuf_reset(&buf);
>  
> @@ -137,7 +138,7 @@ static char *locate_in_PATH(const char *file)
>  		}
>  		strbuf_addstr(&buf, file);
>  
> -		if (!access(buf.buf, F_OK))
> +		if (!stat(buf.buf, &st) && S_ISREG(st.st_mode))
>  			return strbuf_detach(&buf, NULL);
>  
>  		if (!*end)
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index 98c09dd98..30c4ad75f 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -37,6 +37,13 @@ test_expect_success !MINGW 'run_command can run a script without a #! line' '
>  	test_cmp empty err
>  '
>  
> +test_expect_success 'run_command should not try to execute a directory' '
> +	test_when_finished "rm -rf bin/blah" &&
> +	mkdir -p bin/blah &&
> +	PATH=bin:$PATH test_must_fail test-run-command run-command blah 2>err &&
> +	test_i18ngrep "No such file or directory" err
> +'
> +
>  test_expect_success POSIXPERM 'run_command reports EACCES' '
>  	cat hello-script >hello.sh &&
>  	chmod -x hello.sh &&