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