| Message ID | xmqqd0q8liow.fsf@gitster-ct.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | run-command: report exec failure | expand |
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
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 #
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.
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.
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
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' '
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(-)