{"thread":{"id":"50003","subject":"[PATCH] run-command: report exec failure","startedAt":"2018-12-11T05:46:13Z","lastAt":"2018-12-13T11:16:25Z","messageCount":10,"participants":["Junio C Hamano","Jeff King","Johannes Schindelin","John Passaro"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"365037","messageId":"xmqqd0q8liow.fsf@gitster-ct.c.googlers.com","threadId":"50003","inReplyTo":null,"subject":"[PATCH] run-command: report exec failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-11T05:46:07Z","receivedAt":"2018-12-11T05:46:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In 321fd823 (\"run-command: mark path lookup errors with ENOENT\",\n2018-10-24), we rewrote the logic to execute a command by looking\nin the directories on $PATH; as a side effect, a request to run a\ncommand that is not found on $PATH is noticed even before a child\nprocess is forked to execute it.\n\nWe however stopped to report an exec failure in such a case by\nmistake. Add a logic to report the error unless silent-exec-failure\nis requested, to match the original code.\n\nReported-by: John Passaro \nSigned-off-by: Junio C Hamano \n---\n\n * Strictly speaking, the failure that is diagnosed by the spawned\n child is reported with die() and prefixed with \"failure:\"; I am\n adding error_errno(), so this will be reported with \"error:\"\n prefix, which is a slight change in behaviour, but I am guessing\n that this should be OK.\n\n run-command.c | 2 ++\n t/t0061-run-command.sh | 9 ++++++---\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex d679cc267c..e2bc18a083 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -728,6 +728,8 @@ int start_command(struct child_process *cmd)\n \tif (prepare_cmd(&argv, cmd) < 0) {\n \t\tfailed_errno = errno;\n \t\tcmd->pid = -1;\n+\t\tif (!cmd->silent_exec_failure)\n+\t\t\terror_errno(\"cannot run %s\", cmd->argv[0]);\n \t\tgoto end_of_spawn;\n \t}\n \ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex cf932c8514..9c83d44d9c 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -13,11 +13,13 @@ cat >hello-script <<-EOF\n EOF\n \n test_expect_success 'start_command reports ENOENT (slash)' '\n-\ttest-tool run-command start-command-ENOENT ./does-not-exist\n+\ttest-tool run-command start-command-ENOENT ./does-not-exist 2>err &&\n+\ttest_i18ngrep \"cannot run\" err\n '\n \n test_expect_success 'start_command reports ENOENT (no slash)' '\n-\ttest-tool run-command start-command-ENOENT does-not-exist\n+\ttest-tool run-command start-command-ENOENT does-not-exist 2>err &&\n+\ttest_i18ngrep \"cannot run\" err\n '\n \n test_expect_success 'run_command can run a command' '\n@@ -33,7 +35,8 @@ test_expect_success 'run_command is restricted to PATH' '\n \twrite_script should-not-run <<-\\EOF &&\n \techo yikes\n \tEOF\n-\ttest_must_fail test-tool run-command run-command should-not-run\n+\ttest_must_fail test-tool run-command run-command should-not-run 2>err &&\n+\ttest_i18ngrep \"cannot run\" err\n '\n \n test_expect_success !MINGW 'run_command can run a script without a #! line' '\n-- \n2.20.0\n\n"},{"id":"365055","messageId":"20181211102309.GF31588@sigill.intra.peff.net","threadId":"50003","inReplyTo":"xmqqd0q8liow.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] run-command: report exec failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-11T10:23:09Z","receivedAt":"2018-12-11T10:23:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 11, 2018 at 02:46:07PM +0900, Junio C Hamano wrote:\n\n> In 321fd823 (\"run-command: mark path lookup errors with ENOENT\",\n> 2018-10-24), we rewrote the logic to execute a command by looking\n> in the directories on $PATH; as a side effect, a request to run a\n> command that is not found on $PATH is noticed even before a child\n> process is forked to execute it.\n> \n> We however stopped to report an exec failure in such a case by\n> mistake. Add a logic to report the error unless silent-exec-failure\n> is requested, to match the original code.\n> \n> Reported-by: John Passaro \n> Signed-off-by: Junio C Hamano \n\nAh, thanks, I didn't see this before writing my other message. The\ncommit message and fix look good to me.\n\n> * Strictly speaking, the failure that is diagnosed by the spawned\n> child is reported with die() and prefixed with \"failure:\"; I am\n> adding error_errno(), so this will be reported with \"error:\"\n> prefix, which is a slight change in behaviour, but I am guessing\n> that this should be OK.\n\nYes, IMHO that's fine. Arguably the in-child version should say\n\"error:\", too, as the fact that there is a second process is purely an\nimplementation detail (and not even true on Windows, or if we were to\nstart using posix_spawn).\n\n> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\n> index cf932c8514..9c83d44d9c 100755\n> --- a/t/t0061-run-command.sh\n> +++ b/t/t0061-run-command.sh\n> @@ -13,11 +13,13 @@ cat >hello-script <<-EOF\n> EOF\n> \n> test_expect_success 'start_command reports ENOENT (slash)' '\n> -\ttest-tool run-command start-command-ENOENT ./does-not-exist\n> +\ttest-tool run-command start-command-ENOENT ./does-not-exist 2>err &&\n> +\ttest_i18ngrep \"cannot run\" err\n> '\n\nThis one is already correct before the patch, but I agree it's a good\nidea to test it. Here (and in the others), grepping for \"does-not-exist\"\nwould be slightly more robust against us later changing the error\nmessage, but it's probably not a big deal in practice.\n\nThanks again for a quick fix for my bug.\n\n-Peff\n"},{"id":"365068","messageId":"nycvar.QRO.7.76.6.1812111327460.43@tvgsbejvaqbjf.bet","threadId":"50003","inReplyTo":"xmqqd0q8liow.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] run-command: report exec failure","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-12-11T12:31:01Z","receivedAt":"2018-12-11T12:31:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 11 Dec 2018, Junio C Hamano wrote:\n\n> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\n> index cf932c8514..9c83d44d9c 100755\n> --- a/t/t0061-run-command.sh\n> +++ b/t/t0061-run-command.sh\n> @@ -13,11 +13,13 @@ cat >hello-script <<-EOF\n> EOF\n> \n> test_expect_success 'start_command reports ENOENT (slash)' '\n> -\ttest-tool run-command start-command-ENOENT ./does-not-exist\n> +\ttest-tool run-command start-command-ENOENT ./does-not-exist 2>err &&\n> +\ttest_i18ngrep \"cannot run\" err\n\nThis breaks on Windows (on Windows, the error message says \"cannot spawn\", see\nhttps://dev.azure.com/git-for-windows/git/_build/results?buildId=26917):\n\n-- snipsnap --\n[...]\n2018-12-11T11:13:59.9924183Z ++ grep 'cannot run' err\n2018-12-11T11:14:00.0092569Z ++ echo 'error: '\\''grep cannot run' 'err'\\'' didn'\\''t find a match in:'\n2018-12-11T11:14:00.0099500Z error: 'grep cannot run err' didn't find a match in:\n2018-12-11T11:14:00.0100663Z ++ test -s err\n2018-12-11T11:14:00.0101058Z ++ cat err\n2018-12-11T11:14:00.0239691Z error: cannot spawn ./does-not-exist: No such file or directory\n2018-12-11T11:14:00.0254289Z ++ return 1\n2018-12-11T11:14:00.0254489Z not ok 1 - start_command reports ENOENT (slash)\n2018-12-11T11:14:00.0258844Z error: last command exited with $?=1\n2018-12-11T11:14:00.0472195Z #\t\n2018-12-11T11:14:00.0473619Z #\t\ttest-tool run-command start-command-ENOENT ./does-not-exist 2>err &&\n2018-12-11T11:14:00.0473874Z #\t\ttest_i18ngrep \"cannot run\" err\n2018-12-11T11:14:00.0474098Z #\t\n"},{"id":"365074","messageId":"xmqqsgz4jkgl.fsf@gitster-ct.c.googlers.com","threadId":"50003","inReplyTo":"nycvar.QRO.7.76.6.1812111327460.43@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] run-command: report exec failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-11T12:50:50Z","receivedAt":"2018-12-11T12:50:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> This breaks on Windows (on Windows, the error message says \"cannot spawn\", see\n\nThanks for a quick feedback. Let's update to look for the pathname\nof the command, as Peff suggested earlier.\n"},{"id":"365178","messageId":"CAJdN7Khf+Y_jyyG2qoqiMHYPCHHSws15EDftOQni=gFJ7SoMXg@mail.gmail.com","threadId":"50003","inReplyTo":"xmqqsgz4jkgl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] run-command: report exec failure","fromName":"John Passaro","fromEmail":"john.a.passaro@gmail.com","sentAt":"2018-12-12T15:27:40Z","receivedAt":"2018-12-12T15:28:19Z","isPatch":true,"sender":{"key":"john.a.passaro@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6754005?v=4"},"body":"Thank you for this incredibly quick fix.\n\nI see the fix made it to pu as 6b206be3e5 (\"run-command: report exec\nfailure\" 2018-12-11). For what it's worth, it fixes the issue as far\nas I'm concerned and I'm very glad to see the behavior is covered by\ntests now.\n\nAs a procedural question: I'd like to reference this patch in one of\nmy own. Can I reference it as I typed it above? Or is there a chance\nof the SHA1 changing before it goes into some sort of a main history?\n\nJohn Passaro\n(917) 678-8293\nOn Tue, Dec 11, 2018 at 7:50 AM Junio C Hamano wrote:\n>\n> Johannes Schindelin writes:\n>\n> > This breaks on Windows (on Windows, the error message says \"cannot spawn\", see\n>\n> Thanks for a quick feedback. Let's update to look for the pathname\n> of the command, as Peff suggested earlier.\n"},{"id":"365190","messageId":"xmqqbm5qioca.fsf_-_@gitster-ct.c.googlers.com","threadId":"50003","inReplyTo":"xmqqsgz4jkgl.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2] run-command: report exec failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-12T18:36:53Z","receivedAt":"2018-12-12T18:36:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In 321fd823 (\"run-command: mark path lookup errors with ENOENT\",\n2018-10-24), we rewrote the logic to execute a command by looking\nin the directories on $PATH; as a side effect, a request to run a\ncommand that is not found on $PATH is noticed even before a child\nprocess is forked to execute it.\n\nWe however stopped to report an exec failure in such a case by\nmistake. Add a logic to report the error unless silent-exec-failure\nis requested, to match the original code.\n\nReported-by: John Passaro \nSigned-off-by: Junio C Hamano \n---\n\n * This time, tests look for the command name in the message, to\n avoid getting affected by the differences the error is reported\n by two codepaths (Windows codepath uses \"spawn\" while others say\n \"run\"), which was pointed out by Dscho.\n\n I am taking that https://travis-ci.org/git/git/jobs/466908193\n that succeeded on Windows as a sign that this is now OK there.\n\n run-command.c | 2 ++\n t/t0061-run-command.sh | 9 ++++++---\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex d679cc267c..e2bc18a083 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -728,6 +728,8 @@ int start_command(struct child_process *cmd)\n \tif (prepare_cmd(&argv, cmd) < 0) {\n \t\tfailed_errno = errno;\n \t\tcmd->pid = -1;\n+\t\tif (!cmd->silent_exec_failure)\n+\t\t\terror_errno(\"cannot run %s\", cmd->argv[0]);\n \t\tgoto end_of_spawn;\n \t}\n \ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex b9cfc03a53..8a484878ec 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -14,11 +14,13 @@ EOF\n >empty\n \n test_expect_success 'start_command reports ENOENT (slash)' '\n-\ttest-tool run-command start-command-ENOENT ./does-not-exist\n+\ttest-tool run-command start-command-ENOENT ./does-not-exist 2>err &&\n+\ttest_i18ngrep \"\\./does-not-exist\" err\n '\n \n test_expect_success 'start_command reports ENOENT (no slash)' '\n-\ttest-tool run-command start-command-ENOENT does-not-exist\n+\ttest-tool run-command start-command-ENOENT does-not-exist 2>err &&\n+\ttest_i18ngrep \"does-not-exist\" err\n '\n \n test_expect_success 'run_command can run a command' '\n@@ -34,7 +36,8 @@ test_expect_success 'run_command is restricted to PATH' '\n \twrite_script should-not-run <<-\\EOF &&\n \techo yikes\n \tEOF\n-\ttest_must_fail test-tool run-command run-command should-not-run\n+\ttest_must_fail test-tool run-command run-command should-not-run 2>err &&\n+\ttest_i18ngrep \"should-not-run\" err\n '\n \n test_expect_success !MINGW 'run_command can run a script without a #! line' '\n-- \n2.20.0\n\n"},{"id":"365232","messageId":"20181213080825.GB12132@sigill.intra.peff.net","threadId":"50003","inReplyTo":"xmqqbm5qioca.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] run-command: report exec failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-13T08:08:25Z","receivedAt":"2018-12-13T08:08:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 13, 2018 at 03:36:53AM +0900, Junio C Hamano wrote:\n\n> In 321fd823 (\"run-command: mark path lookup errors with ENOENT\",\n> 2018-10-24), we rewrote the logic to execute a command by looking\n> in the directories on $PATH; as a side effect, a request to run a\n> command that is not found on $PATH is noticed even before a child\n> process is forked to execute it.\n> \n> We however stopped to report an exec failure in such a case by\n> mistake. Add a logic to report the error unless silent-exec-failure\n> is requested, to match the original code.\n> \n> Reported-by: John Passaro \n> Signed-off-by: Junio C Hamano \n\nThanks, this looks good to me.\n\n> test_expect_success 'start_command reports ENOENT (slash)' '\n> -\ttest-tool run-command start-command-ENOENT ./does-not-exist\n> +\ttest-tool run-command start-command-ENOENT ./does-not-exist 2>err &&\n> +\ttest_i18ngrep \"\\./does-not-exist\" err\n> '\n\nI thought at first you could use \"grep\" here, since we know that the\nname of the file would appear untranslated. But I think the way\nGETTEXT_POISON works, it actually eats the whole string, including\nplaceholders (which IMHO is a failing of GETTEXT_POISON, since no real\ntranslation would do that, but not worth caring too much about).\n\n-Peff\n"},{"id":"365233","messageId":"20181213081057.GC12132@sigill.intra.peff.net","threadId":"50003","inReplyTo":"CAJdN7Khf+Y_jyyG2qoqiMHYPCHHSws15EDftOQni=gFJ7SoMXg@mail.gmail.com","subject":"Re: [PATCH] run-command: report exec failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-13T08:10:58Z","receivedAt":"2018-12-13T08:11:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 12, 2018 at 10:27:40AM -0500, John Passaro wrote:\n\n> Thank you for this incredibly quick fix.\n> \n> I see the fix made it to pu as 6b206be3e5 (\"run-command: report exec\n> failure\" 2018-12-11). For what it's worth, it fixes the issue as far\n> as I'm concerned and I'm very glad to see the behavior is covered by\n> tests now.\n> \n> As a procedural question: I'd like to reference this patch in one of\n> my own. Can I reference it as I typed it above? Or is there a chance\n> of the SHA1 changing before it goes into some sort of a main history?\n\nCommits in \"pu\" are still subject to change (and indeed, this one was\namended to e5a329a279 to fix the grep issue on Windows).\n\nOnce it hits \"next\" it is generally stable. That hasn't happened yet,\nbut I think what's there now is likely to get merged as-is (and will\nretain that commit id).\n\n-Peff\n"},{"id":"365234","messageId":"xmqqzht9et8h.fsf@gitster-ct.c.googlers.com","threadId":"50003","inReplyTo":"20181213080825.GB12132@sigill.intra.peff.net","subject":"Re: [PATCH v2] run-command: report exec failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-13T08:17:02Z","receivedAt":"2018-12-13T08:17:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Dec 13, 2018 at 03:36:53AM +0900, Junio C Hamano wrote:\n>\n>> test_expect_success 'start_command reports ENOENT (slash)' '\n>> -\ttest-tool run-command start-command-ENOENT ./does-not-exist\n>> +\ttest-tool run-command start-command-ENOENT ./does-not-exist 2>err &&\n>> +\ttest_i18ngrep \"\\./does-not-exist\" err\n>> '\n>\n> I thought at first you could use \"grep\" here, since we know that the\n> name of the file would appear untranslated. But I think the way\n> GETTEXT_POISON works, it actually eats the whole string, including\n> placeholders (which IMHO is a failing of GETTEXT_POISON, since no real\n> translation would do that, but not worth caring too much about).\n\nWhen Ævar's dynamic gettext poison topic was discussed, there was an\nidea or two floated for a possible follow-up to introduce a true\n\"fake translation\", replacing e with é, n with ñ, etc., while\nkeeping the printf formaters intact. When that comes, we should be\nable to use grep and that would make the result more robust than the\ncurrent test_i18ngrep that always pretends to have seen a match, but\nthat hasn't happened yet.\n\n"},{"id":"365238","messageId":"nycvar.QRO.7.76.6.1812131215440.43@tvgsbejvaqbjf.bet","threadId":"50003","inReplyTo":"xmqqbm5qioca.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] run-command: report exec failure","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-12-13T11:15:57Z","receivedAt":"2018-12-13T11:16:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 13 Dec 2018, Junio C Hamano wrote:\n\n> I am taking that https://travis-ci.org/git/git/jobs/466908193\n> that succeeded on Windows as a sign that this is now OK there.\n\nI concur,\nDscho\n"}]}