run-command: report exec failure
diff mbox series

Message ID xmqqd0q8liow.fsf@gitster-ct.c.googlers.com
State New
Headers show
Series
  • run-command: report exec failure
Related show

Commit Message

Junio C Hamano Dec. 11, 2018, 5:46 a.m. UTC
In 321fd823 ("run-command: mark path lookup errors with ENOENT",
2018-10-24), we rewrote the logic to execute a command by looking
in the directories on $PATH; as a side effect, a request to run a
command that is not found on $PATH is noticed even before a child
process is forked to execute it.

We however stopped to report an exec failure in such a case by
mistake.  Add a logic to report the error unless silent-exec-failure
is requested, to match the original code.

Reported-by: John Passaro <john.a.passaro@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Strictly speaking, the failure that is diagnosed by the spawned
   child is reported with die() and prefixed with "failure:"; I am
   adding error_errno(), so this will be reported with "error:"
   prefix, which is a slight change in behaviour, but I am guessing
   that this should be OK.

 run-command.c          | 2 ++
 t/t0061-run-command.sh | 9 ++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

Comments

Jeff King Dec. 11, 2018, 10:23 a.m. UTC | #1
On Tue, Dec 11, 2018 at 02:46:07PM +0900, Junio C Hamano wrote:

> In 321fd823 ("run-command: mark path lookup errors with ENOENT",
> 2018-10-24), we rewrote the logic to execute a command by looking
> in the directories on $PATH; as a side effect, a request to run a
> command that is not found on $PATH is noticed even before a child
> process is forked to execute it.
> 
> We however stopped to report an exec failure in such a case by
> mistake.  Add a logic to report the error unless silent-exec-failure
> is requested, to match the original code.
> 
> Reported-by: John Passaro <john.a.passaro@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ah, thanks, I didn't see this before writing my other message. The
commit message and fix look good to me.

>  * Strictly speaking, the failure that is diagnosed by the spawned
>    child is reported with die() and prefixed with "failure:"; I am
>    adding error_errno(), so this will be reported with "error:"
>    prefix, which is a slight change in behaviour, but I am guessing
>    that this should be OK.

Yes, IMHO that's fine. Arguably the in-child version should say
"error:", too, as the fact that there is a second process is purely an
implementation detail (and not even true on Windows, or if we were to
start using posix_spawn).

> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index cf932c8514..9c83d44d9c 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -13,11 +13,13 @@ cat >hello-script <<-EOF
>  EOF
>  
>  test_expect_success 'start_command reports ENOENT (slash)' '
> -	test-tool run-command start-command-ENOENT ./does-not-exist
> +	test-tool run-command start-command-ENOENT ./does-not-exist 2>err &&
> +	test_i18ngrep "cannot run" err
>  '

This one is already correct before the patch, but I agree it's a good
idea to test it. Here (and in the others), grepping for "does-not-exist"
would be slightly more robust against us later changing the error
message, but it's probably not a big deal in practice.

Thanks again for a quick fix for my bug.

-Peff
Johannes Schindelin Dec. 11, 2018, 12:31 p.m. UTC | #2
Hi Junio,

On Tue, 11 Dec 2018, Junio C Hamano wrote:

> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index cf932c8514..9c83d44d9c 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -13,11 +13,13 @@ cat >hello-script <<-EOF
>  EOF
>  
>  test_expect_success 'start_command reports ENOENT (slash)' '
> -	test-tool run-command start-command-ENOENT ./does-not-exist
> +	test-tool run-command start-command-ENOENT ./does-not-exist 2>err &&
> +	test_i18ngrep "cannot run" err

This breaks on Windows (on Windows, the error message says "cannot spawn", see
https://dev.azure.com/git-for-windows/git/_build/results?buildId=26917):

-- snipsnap --
[...]
2018-12-11T11:13:59.9924183Z ++ grep 'cannot run' err
2018-12-11T11:14:00.0092569Z ++ echo 'error: '\''grep cannot run' 'err'\'' didn'\''t find a match in:'
2018-12-11T11:14:00.0099500Z error: 'grep cannot run err' didn't find a match in:
2018-12-11T11:14:00.0100663Z ++ test -s err
2018-12-11T11:14:00.0101058Z ++ cat err
2018-12-11T11:14:00.0239691Z error: cannot spawn ./does-not-exist: No such file or directory
2018-12-11T11:14:00.0254289Z ++ return 1
2018-12-11T11:14:00.0254489Z not ok 1 - start_command reports ENOENT (slash)
2018-12-11T11:14:00.0258844Z error: last command exited with $?=1
2018-12-11T11:14:00.0472195Z #	
2018-12-11T11:14:00.0473619Z #		test-tool run-command start-command-ENOENT ./does-not-exist 2>err &&
2018-12-11T11:14:00.0473874Z #		test_i18ngrep "cannot run" err
2018-12-11T11:14:00.0474098Z #
Junio C Hamano Dec. 11, 2018, 12:50 p.m. UTC | #3
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> This breaks on Windows (on Windows, the error message says "cannot spawn", see

Thanks for a quick feedback.  Let's update to look for the pathname
of the command, as Peff suggested earlier.
John Passaro Dec. 12, 2018, 3:27 p.m. UTC | #4
Thank you for this incredibly quick fix.

I see the fix made it to pu as 6b206be3e5 ("run-command: report exec
failure" 2018-12-11).  For what it's worth, it fixes the issue as far
as I'm concerned and I'm very glad to see the behavior is covered by
tests now.

As a procedural question: I'd like to reference this patch in one of
my own. Can I reference it as I typed it above? Or is there a chance
of the SHA1 changing before it goes into some sort of a main history?

John Passaro
(917) 678-8293
On Tue, Dec 11, 2018 at 7:50 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > This breaks on Windows (on Windows, the error message says "cannot spawn", see
>
> Thanks for a quick feedback.  Let's update to look for the pathname
> of the command, as Peff suggested earlier.
Jeff King Dec. 13, 2018, 8:10 a.m. UTC | #5
On Wed, Dec 12, 2018 at 10:27:40AM -0500, John Passaro wrote:

> Thank you for this incredibly quick fix.
> 
> I see the fix made it to pu as 6b206be3e5 ("run-command: report exec
> failure" 2018-12-11).  For what it's worth, it fixes the issue as far
> as I'm concerned and I'm very glad to see the behavior is covered by
> tests now.
> 
> As a procedural question: I'd like to reference this patch in one of
> my own. Can I reference it as I typed it above? Or is there a chance
> of the SHA1 changing before it goes into some sort of a main history?

Commits in "pu" are still subject to change (and indeed, this one was
amended to e5a329a279 to fix the grep issue on Windows).

Once it hits "next" it is generally stable. That hasn't happened yet,
but I think what's there now is likely to get merged as-is (and will
retain that commit id).

-Peff

Patch
diff mbox series

diff --git a/run-command.c b/run-command.c
index d679cc267c..e2bc18a083 100644
--- a/run-command.c
+++ b/run-command.c
@@ -728,6 +728,8 @@  int start_command(struct child_process *cmd)
 	if (prepare_cmd(&argv, cmd) < 0) {
 		failed_errno = errno;
 		cmd->pid = -1;
+		if (!cmd->silent_exec_failure)
+			error_errno("cannot run %s", cmd->argv[0]);
 		goto end_of_spawn;
 	}
 
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index cf932c8514..9c83d44d9c 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -13,11 +13,13 @@  cat >hello-script <<-EOF
 EOF
 
 test_expect_success 'start_command reports ENOENT (slash)' '
-	test-tool run-command start-command-ENOENT ./does-not-exist
+	test-tool run-command start-command-ENOENT ./does-not-exist 2>err &&
+	test_i18ngrep "cannot run" err
 '
 
 test_expect_success 'start_command reports ENOENT (no slash)' '
-	test-tool run-command start-command-ENOENT does-not-exist
+	test-tool run-command start-command-ENOENT does-not-exist 2>err &&
+	test_i18ngrep "cannot run" err
 '
 
 test_expect_success 'run_command can run a command' '
@@ -33,7 +35,8 @@  test_expect_success 'run_command is restricted to PATH' '
 	write_script should-not-run <<-\EOF &&
 	echo yikes
 	EOF
-	test_must_fail test-tool run-command run-command should-not-run
+	test_must_fail test-tool run-command run-command should-not-run 2>err &&
+	test_i18ngrep "cannot run" err
 '
 
 test_expect_success !MINGW 'run_command can run a script without a #! line' '