git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] run-command: report exec failure
@ 2018-12-11  5:46 Junio C Hamano
  2018-12-11 10:23 ` Jeff King
  2018-12-11 12:31 ` Johannes Schindelin
  0 siblings, 2 replies; 10+ messages in thread
From: Junio C Hamano @ 2018-12-11  5:46 UTC (permalink / raw)
  To: git; +Cc: Jeff King, John Passaro

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] run-command: report exec failure
  2018-12-11  5:46 [PATCH] run-command: report exec failure Junio C Hamano
@ 2018-12-11 10:23 ` Jeff King
  2018-12-11 12:31 ` Johannes Schindelin
  1 sibling, 0 replies; 10+ messages in thread
From: Jeff King @ 2018-12-11 10:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, John Passaro

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] run-command: report exec failure
  2018-12-11  5:46 [PATCH] run-command: report exec failure Junio C Hamano
  2018-12-11 10:23 ` Jeff King
@ 2018-12-11 12:31 ` Johannes Schindelin
  2018-12-11 12:50   ` Junio C Hamano
  1 sibling, 1 reply; 10+ messages in thread
From: Johannes Schindelin @ 2018-12-11 12:31 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, John Passaro

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 #	

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] run-command: report exec failure
  2018-12-11 12:31 ` Johannes Schindelin
@ 2018-12-11 12:50   ` Junio C Hamano
  2018-12-12 15:27     ` John Passaro
  2018-12-12 18:36     ` [PATCH v2] " Junio C Hamano
  0 siblings, 2 replies; 10+ messages in thread
From: Junio C Hamano @ 2018-12-11 12:50 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Jeff King, John Passaro

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] run-command: report exec failure
  2018-12-11 12:50   ` Junio C Hamano
@ 2018-12-12 15:27     ` John Passaro
  2018-12-13  8:10       ` Jeff King
  2018-12-12 18:36     ` [PATCH v2] " Junio C Hamano
  1 sibling, 1 reply; 10+ messages in thread
From: John Passaro @ 2018-12-12 15:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes.Schindelin, git, Jeff King

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] run-command: report exec failure
  2018-12-11 12:50   ` Junio C Hamano
  2018-12-12 15:27     ` John Passaro
@ 2018-12-12 18:36     ` Junio C Hamano
  2018-12-13  8:08       ` Jeff King
  2018-12-13 11:15       ` Johannes Schindelin
  1 sibling, 2 replies; 10+ messages in thread
From: Junio C Hamano @ 2018-12-12 18:36 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Johannes Schindelin

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] run-command: report exec failure
  2018-12-12 18:36     ` [PATCH v2] " Junio C Hamano
@ 2018-12-13  8:08       ` Jeff King
  2018-12-13  8:17         ` Junio C Hamano
  2018-12-13 11:15       ` Johannes Schindelin
  1 sibling, 1 reply; 10+ messages in thread
From: Jeff King @ 2018-12-13  8:08 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] run-command: report exec failure
  2018-12-12 15:27     ` John Passaro
@ 2018-12-13  8:10       ` Jeff King
  0 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2018-12-13  8:10 UTC (permalink / raw)
  To: John Passaro; +Cc: Junio C Hamano, Johannes.Schindelin, git

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] run-command: report exec failure
  2018-12-13  8:08       ` Jeff King
@ 2018-12-13  8:17         ` Junio C Hamano
  0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2018-12-13  8:17 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Johannes Schindelin

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.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] run-command: report exec failure
  2018-12-12 18:36     ` [PATCH v2] " Junio C Hamano
  2018-12-13  8:08       ` Jeff King
@ 2018-12-13 11:15       ` Johannes Schindelin
  1 sibling, 0 replies; 10+ messages in thread
From: Johannes Schindelin @ 2018-12-13 11:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-12-13 11:16 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-11  5:46 [PATCH] run-command: report exec failure Junio C Hamano
2018-12-11 10:23 ` Jeff King
2018-12-11 12:31 ` Johannes Schindelin
2018-12-11 12:50   ` Junio C Hamano
2018-12-12 15:27     ` John Passaro
2018-12-13  8:10       ` Jeff King
2018-12-12 18:36     ` [PATCH v2] " Junio C Hamano
2018-12-13  8:08       ` Jeff King
2018-12-13  8:17         ` Junio C Hamano
2018-12-13 11:15       ` Johannes Schindelin

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git