Re: [PATCH v7 2/2] run-command: don't try to execute directories

[Jonathan Nieder <jrnieder@gmail.com>]

Brandon Williams wrote:

> Subject: run-command: don't try to execute directories

nit: this is also about non-executable files, now.  That would mean
something like

 run-command: don't try to execute directories or non-executable files

or

 run-command: restrict PATH search to files we can execute

[...]
> Reported-by: Brian Hatfield <bhatfield@google.com>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  run-command.c          |  2 +-
>  t/t0061-run-command.sh | 23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/run-command.c b/run-command.c
> index a97d7bf9f..ec08e0951 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -137,7 +137,7 @@ static char *locate_in_PATH(const char *file)
>  		}
>  		strbuf_addstr(&buf, file);
>  
> -		if (!access(buf.buf, F_OK))
> +		if (is_executable(buf.buf))
>  			return strbuf_detach(&buf, NULL);

It's probably worth a docstring for this function to explain
that this is not a complete emulation of execvp on Windows, since
it doesn't look for .com and .exe files.


>  
>  		if (!*end)
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index 98c09dd98..fd5e43766 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -37,6 +37,29 @@ 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 bin1 bin2 bin3" &&
> +	mkdir -p bin1/greet bin2 bin3 &&
> +	write_script bin2/greet <<-\EOF &&
> +	cat bin2/greet
> +	EOF
> +	chmod -x bin2/greet &&

This probably implies that the test needs a POSIXPERM dependency.
Should it be a separate test_expect_success case so that the other
part can still run on Windows?

The rest looks good.  Thanks for your patient work.

With whatever subset of the changes described makes sense,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

diff --git i/run-command.c w/run-command.c
index ec08e09518..dbbaec932e 100644
--- i/run-command.c
+++ w/run-command.c
@@ -117,6 +117,21 @@ static inline void close_pair(int fd[2])
 	close(fd[1]);
 }
 
+/*
+ * Search $PATH for a command.  This emulates the path search that
+ * execvp would perform, without actually executing the command so it
+ * can be used before fork() to prepare to run a command using
+ * execve() or after execvp() to diagnose why it failed.
+ *
+ * The caller should ensure that file contains no directory
+ * separators.
+ *
+ * Returns NULL if the command could not be found.
+ *
+ * This should not be used on Windows, where the $PATH search rules
+ * are more complicated (e.g., a search for "foo" should find
+ * "foo.exe").
+ */
 static char *locate_in_PATH(const char *file)
 {
 	const char *p = getenv("PATH");
diff --git i/t/t0061-run-command.sh w/t/t0061-run-command.sh
index fd5e43766a..e48a207fae 100755
--- i/t/t0061-run-command.sh
+++ w/t/t0061-run-command.sh
@@ -37,26 +37,33 @@ 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_expect_success 'run_command does not try to execute a directory' '
 	test_when_finished "rm -rf bin1 bin2" &&
-	mkdir -p bin1/greet bin2 bin3 &&
+	mkdir -p bin1/greet bin2 &&
 	write_script bin2/greet <<-\EOF &&
 	cat bin2/greet
 	EOF
-	chmod -x bin2/greet &&
-	write_script bin3/greet <<-\EOF &&
-	cat bin3/greet
+
+	PATH=$PWD/bin1:$PWD/bin2:$PATH \
+		test-run-command run-command greet >actual 2>err &&
+	test_cmp bin2/greet actual &&
+	test_cmp empty err
+'
+
+test_expect_success POSIXPERM 'run_command passes over non-executable file' '
+	test_when_finished "rm -rf bin1 bin2" &&
+	mkdir -p bin1 bin2 &&
+	write_script bin1/greet <<-\EOF &&
+	cat bin1/greet
+	EOF
+	chmod -x bin1/greet &&
+	write_script bin2/greet <<-\EOF &&
+	cat bin2/greet
 	EOF
 
-	# Test that run-command does not try to execute the "greet" directory in
-	# "bin1", or the non-executable file "greet" in "bin2", but rather
-	# correcty executes the "greet" script located in bin3.
-	(
-		PATH=$PWD/bin1:$PWD/bin2:$PWD/bin3:$PATH &&
-		export PATH &&
-		test-run-command run-command greet >actual 2>err
-	) &&
-	test_cmp bin3/greet actual &&
+	PATH=$PWD/bin1:$PWD/bin2:$PATH \
+		test-run-command run-command greet >actual 2>err &&
+	test_cmp bin2/greet actual &&
 	test_cmp empty err
 '