[PATCH] run-command: report exec failure

[PATCH] run-command: report exec failure

[Junio C Hamano]

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(-)

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' '
-- 
2.20.0

Re: [PATCH] run-command: report exec failure

[Jeff King]

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

Re: [PATCH] run-command: report exec failure

[Johannes Schindelin]

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 #	

Re: [PATCH] run-command: report exec failure

[Junio C Hamano]

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.

Re: [PATCH] run-command: report exec failure

[John Passaro]

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.

Re: [PATCH] run-command: report exec failure

[Jeff King]

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 v2] run-command: report exec failure

[Junio C Hamano]

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

 * This time, tests look for the command name in the message, to
   avoid getting affected by the differences the error is reported
   by two codepaths (Windows codepath uses "spawn" while others say
   "run"), which was pointed out by Dscho.

   I am taking that https://travis-ci.org/git/git/jobs/466908193
   that succeeded on Windows as a sign that this is now OK there.

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

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 b9cfc03a53..8a484878ec 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -14,11 +14,13 @@ EOF
 >empty
 
 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 "\./does-not-exist" 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 "does-not-exist" err
 '
 
 test_expect_success 'run_command can run a command' '
@@ -34,7 +36,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 "should-not-run" err
 '
 
 test_expect_success !MINGW 'run_command can run a script without a #! line' '
-- 
2.20.0

Re: [PATCH v2] run-command: report exec failure

[Jeff King]

On Thu, Dec 13, 2018 at 03:36:53AM +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>

Thanks, this looks good to me.

>  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 "\./does-not-exist" err
>  '

I thought at first you could use "grep" here, since we know that the
name of the file would appear untranslated. But I think the way
GETTEXT_POISON works, it actually eats the whole string, including
placeholders (which IMHO is a failing of GETTEXT_POISON, since no real
translation would do that, but not worth caring too much about).

-Peff

Re: [PATCH v2] run-command: report exec failure

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Dec 13, 2018 at 03:36:53AM +0900, Junio C Hamano wrote:
>
>>  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 "\./does-not-exist" err
>>  '
>
> I thought at first you could use "grep" here, since we know that the
> name of the file would appear untranslated. But I think the way
> GETTEXT_POISON works, it actually eats the whole string, including
> placeholders (which IMHO is a failing of GETTEXT_POISON, since no real
> translation would do that, but not worth caring too much about).

When Ævar's dynamic gettext poison topic was discussed, there was an
idea or two floated for a possible follow-up to introduce a true
"fake translation", replacing e with é, n with ñ, etc., while
keeping the printf formaters intact.  When that comes, we should be
able to use grep and that would make the result more robust than the
current test_i18ngrep that always pretends to have seen a match, but
that hasn't happened yet.

Re: [PATCH v2] run-command: report exec failure

[Johannes Schindelin]

Hi Junio,

On Thu, 13 Dec 2018, Junio C Hamano wrote:

>    I am taking that https://travis-ci.org/git/git/jobs/466908193
>    that succeeded on Windows as a sign that this is now OK there.

I concur,
Dscho