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@xxxxxxxxx>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
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
