{"thread":{"id":"27128","subject":"[REGRESSION] git-wrapper to run-commands codepath regression","startedAt":"2011-04-18T20:54:54Z","lastAt":"2011-04-21T00:00:17Z","messageCount":20,"participants":["Junio C Hamano","Jeff King","Andreas Schwab","Jonathan Nieder","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"166043","messageId":"7v8vv78eld.fsf@alter.siamese.dyndns.org","threadId":"27128","inReplyTo":null,"subject":"[REGRESSION] git-wrapper to run-commands codepath regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-18T20:54:54Z","receivedAt":"2011-04-18T20:54:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"There appears to be a regression in the codepath between git wrapper and\nrun_commands API.\n\n\t$ T=/var/tmp/test-commands\n\t$ mkdir $T\n\t$ cat >$T/git-hello <<\\-EOF\n\t#!/bin/sh\n\techo hello\n\tEOF\n\t$ chmod +x $T/git-hello\n\t$ oPATH=$PATH\n\t$ PATH=$T:$PATH\n\t$ export PATH\n\t$ git hello\n\thello\n\nSo far, I added a \"hello\" subcommand to \"git\", and it runs correctly.\n\nNow, when I make the script non-executable, this is what I get from\n'maint':\n\n\t$ chmod a-x $T/git-hello\n\t$ git hello\n\tfatal: cannot exec 'git-hello': Permission denied\n\nBut with 'master', we get a disturbing output:\n\n\t$ git hello\n        fatal: $\n\nNote that we can observe the same regression if you instead make $T\nunreadable with:\n\n\t$ chmod 755 $T/git-hello ;# make it executable again\n\t$ chmod a-rwx $T ;# but that directory cannot be read\n        $ git hello\n\nSo that is the \"regression\" part.\n\nThe following is a tangent that was brought up at $work.\n\nSome people might argue that we should skip $T/git-hello in the last case\nand try to find git-hello in a later directory listed in $PATH, but I do\nnot personally think that is a right thing to do.  It would make the\nproblem harder to diagnose, and more importantly, the fact that the user\nlisted $T earlier in the $PATH is a strong indication that the user wants\nthe scripts in $T override the scripts with the same name in directories\nthat appear later in the $PATH, and we should report when that is not\nhappening, either\n\n (1) when $T/git-st was found but was not executable; or\n\n (2) when we cannot read $T and we cannot even tell $T/git-st exists or\n     not.\n\nSo I think it is Ok to be silent only when we see ENOENT like the current\ncode does.\n\nI am somewhat sympathetic to the case (2) above, but not sympathetic\nenough to suggest changing the current behaviour.  In fact, I would say\nif we treat EACCES the same way as we treat ENOENT, it would be a bug.\n\nWhen your $HOME is mounted over NFS on two different machines, it is\nperfectly fine to have a directory that exists on one machine but not on\nother machines in $PATH, and it is reasonable to expect such a directory\nto be skipped silently without complaints.\n\nThat situation, with a small stretch of imagination, can be extended to a\ncase where a directory early in your $PATH that you are using on one\nmachine for your private git-script correctly on one machine is owned by\nsomebody else, used for other purposes, and most importantly you have no\ncontrol on it on another machine, and you could argue that these two cases\nare similar.\n\nIt is _not_ quite similar, though.  Such an \"early path component is a\nrandom place I do not control\" arrangement is a total security risk, and\nwe shouldn't be bending backwards to support it.  Instead, we should be\nactively discouraging it.  That is why I said I am not sympathetic enough\nabove.\n"},{"id":"166045","messageId":"20110418211102.GA13566@sigill.intra.peff.net","threadId":"27128","inReplyTo":"7v8vv78eld.fsf@alter.siamese.dyndns.org","subject":"Re: [REGRESSION] git-wrapper to run-commands codepath regression","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-18T21:11:02Z","receivedAt":"2011-04-18T21:11:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 18, 2011 at 01:54:54PM -0700, Junio C Hamano wrote:\n\n> There appears to be a regression in the codepath between git wrapper and\n> run_commands API.\n> \n> \t$ T=/var/tmp/test-commands\n> \t$ mkdir $T\n> \t$ cat >$T/git-hello <<\\-EOF\n> \t#!/bin/sh\n> \techo hello\n> \tEOF\n> \t$ chmod +x $T/git-hello\n> \t$ oPATH=$PATH\n> \t$ PATH=$T:$PATH\n> \t$ export PATH\n> \t$ git hello\n> \thello\n> \n> So far, I added a \"hello\" subcommand to \"git\", and it runs correctly.\n> \n> Now, when I make the script non-executable, this is what I get from\n> 'maint':\n> \n> \t$ chmod a-x $T/git-hello\n> \t$ git hello\n> \tfatal: cannot exec 'git-hello': Permission denied\n> \n> But with 'master', we get a disturbing output:\n> \n> \t$ git hello\n>         fatal: $\n\nThe good news is that the bug is trivial. It bisects Jonathan's ebec842\n(run-command: prettify -D_FORTIFY_SOURCE workaround, 2011-03-16), which\nintroduces:\n\n-       unused = write(child_err, \"fatal: \", 7);\n-       unused = write(child_err, msg, len);\n-       unused = write(child_err, \"\\n\", 1);\n+       if (write(child_err, \"fatal: \", 7) ||\n+           write(child_err, msg, len) ||\n+           write(child_err, \"\\n\", 1))\n+               ; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */\n\nStare at that for a minute and see if you can guess what's wrong. :)\n\n-Peff\n"},{"id":"166046","messageId":"7v4o5v8dlp.fsf@alter.siamese.dyndns.org","threadId":"27128","inReplyTo":"7v8vv78eld.fsf@alter.siamese.dyndns.org","subject":"Re: [REGRESSION] git-wrapper to run-commands codepath regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-18T21:16:18Z","receivedAt":"2011-04-18T21:16:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> There appears to be a regression in the codepath between git wrapper and\n> run_commands API.\n>\n> \t$ T=/var/tmp/test-commands\n> \t$ mkdir $T\n> \t$ cat >$T/git-hello <<\\-EOF\n> \t#!/bin/sh\n> \techo hello\n> \tEOF\n> \t$ chmod +x $T/git-hello\n> \t$ oPATH=$PATH\n> \t$ PATH=$T:$PATH\n> \t$ export PATH\n> \t$ git hello\n> \thello\n>\n> So far, I added a \"hello\" subcommand to \"git\", and it runs correctly.\n>\n> Now, when I make the script non-executable, this is what I get from\n> 'maint':\n>\n> \t$ chmod a-x $T/git-hello\n> \t$ git hello\n> \tfatal: cannot exec 'git-hello': Permission denied\n>\n> But with 'master', we get a disturbing output:\n>\n> \t$ git hello\n>         fatal: $\n>\n> Note that we can observe the same regression if you instead make $T\n> unreadable with:\n>\n> \t$ chmod 755 $T/git-hello ;# make it executable again\n> \t$ chmod a-rwx $T ;# but that directory cannot be read\n>         $ git hello\n>\n> So that is the \"regression\" part.\n\nThis bisects down to ebec842 (run-command: prettify -D_FORTIFY_SOURCE\nworkaround, 2011-03-16).\n\nAnd we should really have been more careful.  Look at what the patch does:\n\n    Sometimes when there is an output error, especially right before exit,\n    there really is nothing to be done.  The obvious solution, adopted in\n    v1.7.0.3~20^2 (run-command.c: fix build warnings on Ubuntu,\n    2010-01-30), is to save the return value to a dummy variable:\n    \n    \tssize_t dummy;\n    \tdummy = write(...);\n    \n    But that (1) is ugly and (2) triggers -Wunused-but-set-variable\n    warnings with gcc-4.6 -Wall, so we are not much better off than when\n    we started.\n\n    Instead, use an \"if\" statement with an empty body to make the intent\n    clear.\n    \n    \tif (write(...))\n    \t\t; /* yes, yes, there was an error. */\n    \nNo, a non-zero return is not an error from the write(2) system call.\nI cannot believe both of us didn't spot it.  What were we smoking?\n\nI'm reverting it for now, but am open to a submission of a proper fix\nafter 1.7.5.\n"},{"id":"166047","messageId":"20110418211849.GB13566@sigill.intra.peff.net","threadId":"27128","inReplyTo":"20110418211102.GA13566@sigill.intra.peff.net","subject":"Re: [REGRESSION] git-wrapper to run-commands codepath regression","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-18T21:18:50Z","receivedAt":"2011-04-18T21:18:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 18, 2011 at 05:11:02PM -0400, Jeff King wrote:\n\n> The good news is that the bug is trivial. It bisects Jonathan's ebec842\n> (run-command: prettify -D_FORTIFY_SOURCE workaround, 2011-03-16), which\n> introduces:\n> \n> -       unused = write(child_err, \"fatal: \", 7);\n> -       unused = write(child_err, msg, len);\n> -       unused = write(child_err, \"\\n\", 1);\n> +       if (write(child_err, \"fatal: \", 7) ||\n> +           write(child_err, msg, len) ||\n> +           write(child_err, \"\\n\", 1))\n> +               ; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */\n> \n> Stare at that for a minute and see if you can guess what's wrong. :)\n\nAnd here's the fix.\n\n-- >8 --\nSubject: [PATCH] run-command: fix broken error messages from child\n\nAfter we fork, we try to exec the child; if exec fails, we\nwrite an error message and exit. We ignore the return value\nof write, since there's nothing we can do about it.\n\nCommit ebec842 turned this into a conditional to make\n-D_FORTIFY_SOURCE happy with the ignored return value, but\nbotched the change so that we never write more than\n\"fatal:\".\n\nWrite will return the number of bytes written, so the\nconditional as written will always appear as an error. Of\ncourse we don't actually do anything for the error, but\nthe short-circuit logic means we never execute the\nsubsequent write()s, giving us a truncated error message.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n run-command.c |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 8619c76..508a4c6 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -83,9 +83,9 @@ static NORETURN void die_child(const char *err, va_list params)\n \tif (len > sizeof(msg))\n \t\tlen = sizeof(msg);\n \n-\tif (write(child_err, \"fatal: \", 7) ||\n-\t    write(child_err, msg, len) ||\n-\t    write(child_err, \"\\n\", 1))\n+\tif (write(child_err, \"fatal: \", 7) < 0 ||\n+\t    write(child_err, msg, len) < 0 ||\n+\t    write(child_err, \"\\n\", 1) < 0)\n \t\t; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */\n \texit(128);\n }\n-- \n1.7.5.rc2.3.g728b2\n"},{"id":"166049","messageId":"7vvcyb6xww.fsf@alter.siamese.dyndns.org","threadId":"27128","inReplyTo":"20110418211849.GB13566@sigill.intra.peff.net","subject":"Re: [REGRESSION] git-wrapper to run-commands codepath regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-18T21:40:31Z","receivedAt":"2011-04-18T21:40:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> And here's the fix.\n\nI was tempted to suggest the change in your patch.\n\nWith ebec842 (run-command: prettify -D_FORTIFY_SOURCE workaround,\n2011-03-16) reverted, I still don't get complaints from -D_FORTIFY_SOURCE\nfor run-command.c (but I do get \"ignoring return value of 'fwrite' from\nmany places).  Perhaps the kinds of checks done by versions of gcc you,\nJonathan and I use are different.\n\nI'd rather revert it for now; I don't want to see patch ping-pong at this\nlate in the pre-release cycle.\n"},{"id":"166050","messageId":"20110418214337.GB25104@sigill.intra.peff.net","threadId":"27128","inReplyTo":"7vvcyb6xww.fsf@alter.siamese.dyndns.org","subject":"Re: [REGRESSION] git-wrapper to run-commands codepath regression","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-18T21:43:37Z","receivedAt":"2011-04-18T21:43:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 18, 2011 at 02:40:31PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > And here's the fix.\n> \n> I was tempted to suggest the change in your patch.\n> \n> With ebec842 (run-command: prettify -D_FORTIFY_SOURCE workaround,\n> 2011-03-16) reverted, I still don't get complaints from -D_FORTIFY_SOURCE\n> for run-command.c (but I do get \"ignoring return value of 'fwrite' from\n> many places).  Perhaps the kinds of checks done by versions of gcc you,\n> Jonathan and I use are different.\n\nI don't use _FORTIFY_SOURCE at all, so I have no clue. I just saw that\nthe code in ebec842 is obviously wrong, and the fix looked equally\nobvious.\n\n> I'd rather revert it for now; I don't want to see patch ping-pong at this\n> late in the pre-release cycle.\n\nThat's your call, but the fix seems dead simple to me. _FORTIFY_SOURCE\nlikes the conditional, according to Jonathan's patch. We don't remove\nthe conditional, just the wrong \"non-zero is an error\" assumption. So I\nwouldn't expect any ping-pong on it, but then again, it looked like a\npretty innocuous patch in the first place, and held a pretty nasty and\nsurprising bug. :)\n\n-Peff\n"},{"id":"166052","messageId":"7vmxjn6wjh.fsf@alter.siamese.dyndns.org","threadId":"27128","inReplyTo":"20110418214337.GB25104@sigill.intra.peff.net","subject":"Re: [REGRESSION] git-wrapper to run-commands codepath regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-18T22:10:10Z","receivedAt":"2011-04-18T22:10:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ..., but then again, it looked like a\n> pretty innocuous patch in the first place, and held a pretty nasty and\n> surprising bug. :)\n\nExactly.  It was quite embarrassing.\n"},{"id":"166053","messageId":"m2k4erb462.fsf@igel.home","threadId":"27128","inReplyTo":"7vvcyb6xww.fsf@alter.siamese.dyndns.org","subject":"Re: [REGRESSION] git-wrapper to run-commands codepath regression","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-04-18T22:11:49Z","receivedAt":"2011-04-18T22:11:49Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> With ebec842 (run-command: prettify -D_FORTIFY_SOURCE workaround,\n> 2011-03-16) reverted, I still don't get complaints from -D_FORTIFY_SOURCE\n> for run-command.c (but I do get \"ignoring return value of 'fwrite' from\n> many places).  Perhaps the kinds of checks done by versions of gcc you,\n> Jonathan and I use are different.\n\nIt's not about fortify warnings, but \"set but not used\" warnings.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"166054","messageId":"20110418221720.GA16977@elie","threadId":"27128","inReplyTo":"7v4o5v8dlp.fsf@alter.siamese.dyndns.org","subject":"Re: [REGRESSION] git-wrapper to run-commands codepath regression","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-18T22:17:20Z","receivedAt":"2011-04-18T22:17:20Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n>     \tif (write(...))\n>     \t\t; /* yes, yes, there was an error. */\n>\n> No, a non-zero return is not an error from the write(2) system call.\n> I cannot believe both of us didn't spot it.  What were we smoking?\n\nYagh.\n\n\tif (write(child_err, \"fatal: \", 7) ||\n\t    write(child_err, msg, len) ||\n\t    write(child_err, \"\\n\", 1))\n\t\t; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */\n\nThere are two unusual conditions in which this could fail:\n\n - it doesn't write anything at all, in which case the return value\n   is -1.\n - a partial write, for example if writing to an almost-full pipe.\n\nI suppose in a calmer time, a better fix will look like\n\n\tif (write_in_full(child_err, \"fatal: \", 7) != 7 ||\n\t    write_in_full(child_err, msg, len) != len ||\n\t    write_in_full(child_err, \"\\n\", 1) != 1)\n\t\t/* yes, yes, ...\n\nand gcc will have told us something potentially useful.\n\nThanks for catching it.\n"},{"id":"166055","messageId":"7vipub6r3s.fsf@alter.siamese.dyndns.org","threadId":"27128","inReplyTo":"7v8vv78eld.fsf@alter.siamese.dyndns.org","subject":"Re: [REGRESSION] git-wrapper to run-commands codepath regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-19T00:07:35Z","receivedAt":"2011-04-19T00:07:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> There appears to be a regression in the codepath between git wrapper and\n> run_commands API.\n>\n> \t$ T=/var/tmp/test-commands\n> \t$ mkdir $T\n> \t$ cat >$T/git-hello <<\\-EOF\n> \t#!/bin/sh\n> \techo hello\n> \tEOF\n> \t$ chmod +x $T/git-hello\n> \t$ oPATH=$PATH\n> \t$ PATH=$T:$PATH\n> \t$ export PATH\n> \t$ git hello\n> \thello\n>\n> So far, I added a \"hello\" subcommand to \"git\", and it runs correctly.\n>\n> ...\n> Note that we can observe the same regression if you instead make $T\n> unreadable with:\n>\n> \t$ chmod 755 $T/git-hello ;# make it executable again\n> \t$ chmod a-rwx $T ;# but that directory cannot be read\n> \t$ git hello\n>\n> So that is the \"regression\" part.\n>\n> The following is a tangent that was brought up at $work.\n\nAnd that is the topic of this follow-up.\n\n> Some people might argue that we should skip $T/git-hello in the last case\n> and try to find git-hello in a later directory listed in $PATH, but I do\n> not personally think that is a right thing to do.  It would make the\n> problem harder to diagnose, and more importantly, the fact that the user\n> listed $T earlier in the $PATH is a strong indication that the user wants\n> the scripts in $T override the scripts with the same name in directories\n> that appear later in the $PATH, and we should report when that is not\n> happening, either\n>\n>  (1) when $T/git-hello was found but was not executable; or\n>\n>  (2) when we cannot read $T and we cannot even tell $T/git-hello exists or\n>      not.\n>\n> So I think it is Ok to be silent only when we see ENOENT like the current\n> code does.\n\nUnfortunately, as we are giving \"git-hello\" to underlying execvp(3),\ninstead of splitting $PATH, prefixing each element of it in front of\n\"git-hello\" and trying execv() in a loop, when the call fails, we do not\nknow which component of $PATH caused execvp() to fail, and end up saying\n\"fatal: cannot exec 'git-hello': Permission denied\".  So in that sense, we\nare not helping the end user by making it easier to diagnose the problem.\n\nWe would need to emulate what execvp() does ourselves (i.e. split $PATH,\nprefix each component and try execv(), ignoring ENOENT or EACCES while\ntrying next component in $PATH), plus note the first path that got EACCES\nso that we can report which script (including its leading directories) had\ntrouble executing.  Perhaps a simple enough task for beginners.\n"},{"id":"166063","messageId":"20110419070510.GB28291@elie","threadId":"27128","inReplyTo":"7v4o5v8dlp.fsf@alter.siamese.dyndns.org","subject":"[PATCH] run-command: write full error message in die_child","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-19T07:05:10Z","receivedAt":"2011-04-19T07:05:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The run_command facility writes a truncated error message when the\ncommand is present but cannot be executed for some other reason.  For\nexample, if I add a 'hello' command to git:\n\n\t$ echo 'echo hello' >git-hello\n\t$ chmod +x git-hello\n\t$ PATH=.:$PATH git hello\n\thello\n\nand then make it non-executable, this is what I get from 'maint':\n\n\t$ chmod a-x git-hello\n\t$ git hello\n\tfatal: cannot exec 'git-hello': Permission denied\n\nBut with 'master', we get disturbing output:\n\n\t$ PATH=.:$PATH git hello\n\tfatal: $\n\nThat is a regression introduced by v1.7.5-rc0~29^2 (run-command:\nprettify -D_FORTIFY_SOURCE workaround, 2011-03-16), which uses the\nconstruct \"if (write(...) || write(...) || write(...))\" to perform\nsome writes in sequence, with the \"if\" body acknowledging errors from\nthem once.  write does not return 0 on success, so only the first\nwrite succeeds.  Oops.\n\nWhile fixing the above, let's actually pay attention to the return\nvalue and handle partial writes.  write_in_full has the desired\nsemantics --- it loops until the desired number of bytes have been\nwritten and on error it returns -1 to let us handle the error.\n\nThe \"if\" to appease warn_unused_result is no longer necessary after\nthis patch since xwrite and write_in_full check the return value from\nwrite(2), but we leave it in for clarity and for robustness against\nfuture static analyzers.\n\nReported-by: Junio C Hamano <gitster@pobox.com>\nAnalysis-by: Jeff King <peff@peff.net>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nJunio C Hamano wrote:\n\n> I'm reverting it for now, but am open to a submission of a proper fix\n> after 1.7.5.\n\nKnowing myself, I'm likely to forget to submit a fix later.  So here's\na patch to consider applying after 1.7.5.\n\nBased directly against ebec84277 (run-command: prettify\n-D_FORTIFY_SOURCE workaround, 2011-03-16).  The \"grep\" in the test\ncase should be test_i18ngrep if applying to a gettextized git.\n\nSorry for the breakage.\n\n run-command.c          |    8 ++++----\n t/t0061-run-command.sh |   24 ++++++++++++++++++++++++\n test-run-command.c     |    2 ++\n 3 files changed, 30 insertions(+), 4 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 8619c76..3e2ce2a 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -72,7 +72,7 @@ static void notify_parent(void)\n \t * know, so failures like ENOENT can be handled right away; but\n \t * otherwise, finish_command will still report the error.\n \t */\n-\tif (write(child_notifier, \"\", 1))\n+\tif (xwrite(child_notifier, \"\", 1) < 0)\n \t\t; /* yes, dear gcc -D_FORTIFY_SOURCE, there was an error. */\n }\n \n@@ -83,9 +83,9 @@ static NORETURN void die_child(const char *err, va_list params)\n \tif (len > sizeof(msg))\n \t\tlen = sizeof(msg);\n \n-\tif (write(child_err, \"fatal: \", 7) ||\n-\t    write(child_err, msg, len) ||\n-\t    write(child_err, \"\\n\", 1))\n+\tif (write_in_full(child_err, \"fatal: \", 7) < 0 ||\n+\t    write_in_full(child_err, msg, len) < 0 ||\n+\t    write_in_full(child_err, \"\\n\", 1) < 0)\n \t\t; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */\n \texit(128);\n }\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 10b26e4..be602fd 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -7,8 +7,32 @@ test_description='Test run command'\n \n . ./test-lib.sh\n \n+cat >hello-script <<-EOF\n+\t#!$SHELL_PATH\n+\techo hello\n+EOF\n+>empty\n+\n test_expect_success 'start_command reports ENOENT' '\n \ttest-run-command start-command-ENOENT ./does-not-exist\n '\n \n+test_expect_success 'run_command can run a command' '\n+\techo hello >expect &&\n+\tcat hello-script >hello.sh &&\n+\tchmod +x hello.sh &&\n+\ttest-run-command run-command ./hello.sh >actual 2>err &&\n+\n+\ttest_cmp expect actual &&\n+\ttest_cmp empty err\n+'\n+\n+test_expect_success POSIXPERM,SANITY 'run_command reports EACCES' '\n+\tcat hello-script >hello.sh &&\n+\tchmod -x hello.sh &&\n+\ttest_must_fail test-run-command run-command ./hello.sh 2>err &&\n+\n+\tgrep \"fatal: cannot exec.*hello.sh\" err\n+'\n+\n test_done\ndiff --git a/test-run-command.c b/test-run-command.c\nindex 0612bfa..37918e1 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -29,6 +29,8 @@ int main(int argc, char **argv)\n \t\tfprintf(stderr, \"FAIL %s\\n\", argv[1]);\n \t\treturn 1;\n \t}\n+\tif (!strcmp(argv[1], \"run-command\"))\n+\t\texit(run_command(&proc));\n \n \tfprintf(stderr, \"check usage\\n\");\n \treturn 1;\n-- \n1.7.5.rc2\n"},{"id":"166124","messageId":"7vaafl371q.fsf_-_@alter.siamese.dyndns.org","threadId":"27128","inReplyTo":"7vipub6r3s.fsf@alter.siamese.dyndns.org","subject":"[PATCH] report which $PATH entry had trouble running execvp(3)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-20T04:01:21Z","receivedAt":"2011-04-20T04:01:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"You can add your own custom subcommand 'frotz' to the system by adding\n'git-frotz' in a directory somewhere in your $PATH environment variable.\nWhen you ask \"git frotz\" from the command line, \"git-frotz\" is run via\nexecvp(3).\n\nThree plausible scenarios that the execvp(3) would fail for us are:\n\n * The first 'git-frotz' found in a directory on $PATH was not a proper\n   executable binary, and we got \"Exec format error\" (ENOEXEC);\n\n * The only 'git-frotz' found in the directories listed on $PATH were not\n   marked with executable bit, and we got \"Permission denied\" (EACCES); or\n\n * No 'git-frotz' was found in the directories listed on $PATH, but one of\n   the directories were unreadable, and we got EACCES.\n\nThe first one is easy to understand and to rectify.  Most likely, the user\nmade a typo, either on the command line, or when creating the custom\nsubcommand.  However, the latter two cases are harder to notice, as we do\nnot report 'git-frotz' in which directory we had trouble with.  We could\ndo better if we implemented the command search behaviour of execvp(3)\nourselves.\n\nAdd an internal function sane_execvp() that emulates execvp(3), skipping\nENOENT and EACCES while remembering a path that resulted in EACCES while\ntrying later directories on $PATH.  When failing the request at the end,\nreport the path that we had trouble with, and use it when reporting the\nerror.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n  Junio C Hamano <gitster@pobox.com> writes:\n\n  >> The following is a tangent that was brought up at $work.\n  > ...\n  > We would need to emulate what execvp() does ourselves (i.e. split $PATH,\n  > prefix each component and try execv(), ignoring ENOENT or EACCES while\n  > trying next component in $PATH), plus note the first path that got EACCES\n  > so that we can report which script (including its leading directories) had\n  > trouble executing.  Perhaps a simple enough task for beginners.\n\n run-command.c |   48 +++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 47 insertions(+), 1 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex f91e446..4c95f50 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -135,6 +135,52 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \treturn code;\n }\n \n+static const char *sane_execvp(const char *arg0, const char **argv)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf failed_path = STRBUF_INIT;\n+\tchar *path = getenv(\"PATH\");\n+\tchar *next;\n+\n+\tif (!path)\n+\t\tpath = \"\";\n+\n+\tfor (;;) {\n+\t\tnext = strchrnul(path, ':');\n+\t\tif (path < next)\n+\t\t\tstrbuf_add(&sb, path, next - path);\n+\t\telse\n+\t\t\tstrbuf_addch(&sb, '.');\n+\t\tif (sb.len && sb.buf[sb.len - 1] != '/')\n+\t\t\tstrbuf_addch(&sb, '/');\n+\t\tstrbuf_addstr(&sb, arg0);\n+\t\texecv(sb.buf, (char * const*) argv);\n+\n+\t\t/*\n+\t\t * execvp() skips EACCES and ENOENT and goes on to try\n+\t\t * the next entry in the $PATH, but sets errno to EACCES\n+\t\t * when it fails at the end.\n+\t\t */\n+\t\tif (errno == EACCES && !failed_path.len)\n+\t\t\tstrbuf_add(&failed_path, sb.buf, sb.len);\n+\t\tif (errno != ENOENT) {\n+\t\t\tstrbuf_release(&failed_path);\n+\t\t\treturn strbuf_detach(&sb, NULL);\n+\t\t}\n+\t\tstrbuf_release(&sb);\n+\t\tif (!*next)\n+\t\t\tbreak;\n+\t\tpath = next + 1;\n+\t}\n+\tif (failed_path.len) {\n+\t\terrno = EACCES;\n+\t\treturn strbuf_detach(&failed_path, NULL);\n+\t}\n+\tstrbuf_release(&sb);\n+\tstrbuf_release(&failed_path);\n+\treturn arg0;\n+}\n+\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n@@ -278,7 +324,7 @@ fail_pipe:\n \t\t} else if (cmd->use_shell) {\n \t\t\texecv_shell_cmd(cmd->argv);\n \t\t} else {\n-\t\t\texecvp(cmd->argv[0], (char *const*) cmd->argv);\n+\t\t\tcmd->argv[0] = sane_execvp(cmd->argv[0], cmd->argv);\n \t\t}\n \t\t/*\n \t\t * Do not check for cmd->silent_exec_failure; the parent\n"},{"id":"166126","messageId":"20110420055111.GB28597@sigill.intra.peff.net","threadId":"27128","inReplyTo":"7vaafl371q.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] report which $PATH entry had trouble running execvp(3)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-20T05:51:11Z","receivedAt":"2011-04-20T05:51:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 19, 2011 at 09:01:21PM -0700, Junio C Hamano wrote:\n\n> You can add your own custom subcommand 'frotz' to the system by adding\n> 'git-frotz' in a directory somewhere in your $PATH environment variable.\n> When you ask \"git frotz\" from the command line, \"git-frotz\" is run via\n> execvp(3).\n> [...]\n> we do not report 'git-frotz' in which directory we had trouble with.\n> We could do better if we implemented the command search behaviour of\n> execvp(3) ourselves.\n\nI like the idea of giving the user more information about which\ngit-frotz was the problem. Usually there is just one, and pointing them\nto it saves them time.\n\nBut what about the case of\n\n  mkdir one two\n  touch one/frotz two/frotz\n  PATH=one:two:$PATH\n\nWe would report two/frotz, but might it be even better to say \"we found\n2 frotzes, but neither of them were executable\"?\n\nI don't know if it is worth the effort for such a weird corner case.\n\n> Three plausible scenarios that the execvp(3) would fail for us are:\n> \n>  * The first 'git-frotz' found in a directory on $PATH was not a proper\n>    executable binary, and we got \"Exec format error\" (ENOEXEC);\n\nWhat about the magic \"unknown things get executed as shell scripts\"\nbehavior that is implemented by libc's execvp? Your patch has a\nregression for:\n\n  echo \"git log --with-some-options\" >local/bin/git-frotz\n  chmod +x local/bin/git-frotz\n  git frotz\n\nI have always found that behavior slightly insane, but it is\nwell-established, and your sane_execvp breaks anybody who is depending\non it.\n\n> @@ -278,7 +324,7 @@ fail_pipe:\n>  \t\t} else if (cmd->use_shell) {\n>  \t\t\texecv_shell_cmd(cmd->argv);\n>  \t\t} else {\n> -\t\t\texecvp(cmd->argv[0], (char *const*) cmd->argv);\n> +\t\t\tcmd->argv[0] = sane_execvp(cmd->argv[0], cmd->argv);\n>  \t\t}\n>  \t\t/*\n>  \t\t * Do not check for cmd->silent_exec_failure; the parent\n\nThis is inside \"#ifndef WIN32\". Presumably people on Windows want it,\ntoo.  In fact, they already have their own execvp in compat/mingw.c. It\nmight make sense to bring the implementations together. Or perhaps not.\nTheirs is quite different; it does a search of PATH itself, looking for\nexecutables (and magically appending \".exe\"), and then exec's the\nresult. On the other hand, doing that PATH lookup, deciding you have\nsomething, and _then_ exec'ing can be convenient. IIRC, there are a few\nwarts in the git wrapper that could be improved by doing that, but I\ndon't recall the specifics anymore (maybe something like handling the\npager between the momemnt when we decide a command exists and when we\nexec?).\n\n-Peff\n"},{"id":"166134","messageId":"4DAE8D3E.8000705@viscovery.net","threadId":"27128","inReplyTo":"7vaafl371q.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] report which $PATH entry had trouble running execvp(3)","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-04-20T07:37:34Z","receivedAt":"2011-04-20T07:37:34Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/20/2011 6:01, schrieb Junio C Hamano:\n> Add an internal function sane_execvp() that emulates execvp(3), skipping\n> ENOENT and EACCES while remembering a path that resulted in EACCES while\n> trying later directories on $PATH.  When failing the request at the end,\n> report the path that we had trouble with, and use it when reporting the\n> error.\n\nI don't think this is worth the trouble. In which way is git different\nfrom other tools that execvp other programs?\n\nAnd how do you help when the script is executable, but the interpreter is not:\n\n$ chmod -x git\t# use git itself just for exposition\n$ echo '#!'\"$(pwd)/git\" > git-frotz\n$ chmod +x git-frotz\n$ git --exec-path=. frotz\nfatal: cannot exec 'git-frotz': Permission denied\n$ # WTF, git-frotz *is* executable and readable!?!?\n\nIOW, when you get strange behavior, you still have to dig around to know\nwhat went wrong.\n\n-- Hannes\n"},{"id":"166137","messageId":"4DAE8E66.5060705@viscovery.net","threadId":"27128","inReplyTo":"20110419070510.GB28291@elie","subject":"Re: [PATCH] run-command: write full error message in die_child","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-04-20T07:42:30Z","receivedAt":"2011-04-20T07:42:30Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/19/2011 9:05, schrieb Jonathan Nieder:\n> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\n> index 10b26e4..be602fd 100755\n> --- a/t/t0061-run-command.sh\n> +++ b/t/t0061-run-command.sh\n> @@ -7,8 +7,32 @@ test_description='Test run command'\n>  \n>  . ./test-lib.sh\n>  \n> +cat >hello-script <<-EOF\n> +\t#!$SHELL_PATH\n> +\techo hello\n> +EOF\n> +>empty\n> +\n\nUnfortunately, on Windows, the bash spawnd by git converts LF to CRLF...\n\n>  test_expect_success 'start_command reports ENOENT' '\n>  \ttest-run-command start-command-ENOENT ./does-not-exist\n>  '\n>  \n> +test_expect_success 'run_command can run a command' '\n> +\techo hello >expect &&\n> +\tcat hello-script >hello.sh &&\n> +\tchmod +x hello.sh &&\n> +\ttest-run-command run-command ./hello.sh >actual 2>err &&\n> +\n> +\ttest_cmp expect actual &&\n\n... therefore, we fail here. Can we have this squashed in, because 'cat'\nleaves LFs alone?\n\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex be602fd..979b478 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -9,7 +9,7 @@ test_description='Test run command'\n \n cat >hello-script <<-EOF\n \t#!$SHELL_PATH\n-\techo hello\n+\tcat hello-script\n EOF\n >empty\n \n@@ -18,12 +18,11 @@ test_expect_success 'start_command reports ENOENT' '\n '\n \n test_expect_success 'run_command can run a command' '\n-\techo hello >expect &&\n \tcat hello-script >hello.sh &&\n \tchmod +x hello.sh &&\n \ttest-run-command run-command ./hello.sh >actual 2>err &&\n \n-\ttest_cmp expect actual &&\n+\ttest_cmp hello-script actual &&\n \ttest_cmp empty err\n '\n \n\n\n> +test_expect_success POSIXPERM,SANITY 'run_command reports EACCES' '\n\nThanks for this detail (POSIXPERM). It's required. I did not check whether\nSANITY is really needed; I trust you did.\n\n-- Hannes\n"},{"id":"166141","messageId":"20110420103319.GA6027@elie","threadId":"27128","inReplyTo":"4DAE8E66.5060705@viscovery.net","subject":"[PATCH v2 0/2] Re: run-command: write full error message in die_child","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-20T10:33:19Z","receivedAt":"2011-04-20T10:33:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Sixt wrote:\n\n> Unfortunately, on Windows, the bash spawnd by git converts LF to CRLF...\n[...]\n> ... therefore, we fail here. Can we have this squashed in, because 'cat'\n> leaves LFs alone?\n\nThanks for catching this...\n\n[...]\n> I did not check whether\n> SANITY is really needed; I trust you did.\n\n... and this.  No, SANITY is not needed.\n\nHere's a reroll, on top of v1.7.5-rc3~2 (Revert \"run-command: prettify\n-D_FORTIFY_SOURCE workaround\", 2011-04-18).  It even applies on maint\nthis way (not that anyone would need that :)).\n\nJonathan Nieder (2):\n  tests: check error message from run_command\n  run-command: handle short writes and EINTR in die_child\n\n run-command.c          |   15 +++++++++------\n t/t0061-run-command.sh |   23 +++++++++++++++++++++++\n test-run-command.c     |    2 ++\n 3 files changed, 34 insertions(+), 6 deletions(-)\n"},{"id":"166142","messageId":"20110420103508.GB6027@elie","threadId":"27128","inReplyTo":"20110420103319.GA6027@elie","subject":"[PATCH 1/2] tests: check error message from run_command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-20T10:35:08Z","receivedAt":"2011-04-20T10:35:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"In git versions starting at v1.7.5-rc0~29^2 until v1.7.5-rc3~2 (Revert\n\"run-command: prettify -D_FORTIFY_SOURCE workaround\", 2011-04-18)\nfixed it, the run_command facility would write a truncated error\nmessage when the command is present but cannot be executed for some\nother reason.  For example, if I add a 'hello' command to git:\n\n\t$ echo 'echo hello' >git-hello\n\t$ chmod +x git-hello\n\t$ PATH=.:$PATH git hello\n\thello\n\nand make it non-executable, this is what I normally get:\n\n\t$ chmod -x git-hello\n\t$ git hello\n\tfatal: cannot exec 'git-hello': Permission denied\n\nBut with the problematic versions, we get disturbing output:\n\n\t$ PATH=.:$PATH git hello\n\tfatal: $\n\nAdd some tests to make sure it doesn't happen again.\n\nThe hello-script used in these tests uses cat instead of echo because\non Windows the bash spawned by git converts LF to CRLF in text written\nby echo while the bash running tests does not, causing the test to\nfail if \"echo\" is used.  Thanks to Hannes for noticing.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\nImproved-by: Johannes Sixt <j6t@kdbg.org>\n---\n t/t0061-run-command.sh |   23 +++++++++++++++++++++++\n test-run-command.c     |    2 ++\n 2 files changed, 25 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 10b26e4..8d4938f 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -7,8 +7,31 @@ test_description='Test run command'\n \n . ./test-lib.sh\n \n+cat >hello-script <<-EOF\n+\t#!$SHELL_PATH\n+\tcat hello-script\n+EOF\n+>empty\n+\n test_expect_success 'start_command reports ENOENT' '\n \ttest-run-command start-command-ENOENT ./does-not-exist\n '\n \n+test_expect_success 'run_command can run a command' '\n+\tcat hello-script >hello.sh &&\n+\tchmod +x hello.sh &&\n+\ttest-run-command run-command ./hello.sh >actual 2>err &&\n+\n+\ttest_cmp hello-script actual &&\n+\ttest_cmp empty err\n+'\n+\n+test_expect_success POSIXPERM 'run_command reports EACCES' '\n+\tcat hello-script >hello.sh &&\n+\tchmod -x hello.sh &&\n+\ttest_must_fail test-run-command run-command ./hello.sh 2>err &&\n+\n+\tgrep \"fatal: cannot exec.*hello.sh\" err\n+'\n+\n test_done\ndiff --git a/test-run-command.c b/test-run-command.c\nindex 0612bfa..37918e1 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -29,6 +29,8 @@ int main(int argc, char **argv)\n \t\tfprintf(stderr, \"FAIL %s\\n\", argv[1]);\n \t\treturn 1;\n \t}\n+\tif (!strcmp(argv[1], \"run-command\"))\n+\t\texit(run_command(&proc));\n \n \tfprintf(stderr, \"check usage\\n\");\n \treturn 1;\n-- \n1.7.5.rc2\n"},{"id":"166143","messageId":"20110420104005.GC6027@elie","threadId":"27128","inReplyTo":"20110420103319.GA6027@elie","subject":"[PATCH 2/2] run-command: handle short writes and EINTR in die_child","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-20T10:40:05Z","receivedAt":"2011-04-20T10:40:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"If start_command fails after forking and before exec finishes, there\nis not much use in noticing an I/O error on top of that.\nfinish_command will notice that the child exited with nonzero status\nanyway.  So as noted in v1.7.0.3~20^2 (run-command.c: fix build\nwarnings on Ubuntu, 2010-01-30) and v1.7.5-rc0~29^2 (2011-03-16), it\nis safe to ignore errors from write in this codepath.\n\nEven so, the result from write contains useful information: it tells\nus if the write was cancelled by a signal (EINTR) or was only\npartially completed (e.g., when writing to an almost-full pipe).\nLet's use write_in_full to loop until the desired number of bytes have\nbeen written (still ignoring errors if that fails).\n\nAs a happy side effect, the assignment to a dummy variable to appease\ngcc -D_FORTIFY_SOURCE is no longer needed.  xwrite and write_in_full\ncheck the return value from write(2).\n\nNoticed with gcc -Wunused-but-set-variable.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nChanges from v1:\n\n - rewrite the commit message from the pov of a person who does not\n   care about this patch's origin as a brown paper bag\n\n - drop the \"if\"s.  If the fussy compiler/library combination was\n   right in some strange sense after all, then it does not make much\n   sense to take the opportunity to make another token effort to\n   appease it as a preventative step.\n\n run-command.c |   15 +++++++++------\n 1 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex f91e446..70e8a24 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -67,21 +67,24 @@ static int child_notifier = -1;\n \n static void notify_parent(void)\n {\n-\tssize_t unused;\n-\tunused = write(child_notifier, \"\", 1);\n+\t/*\n+\t * execvp failed.  If possible, we'd like to let start_command\n+\t * know, so failures like ENOENT can be handled right away; but\n+\t * otherwise, finish_command will still report the error.\n+\t */\n+\txwrite(child_notifier, \"\", 1);\n }\n \n static NORETURN void die_child(const char *err, va_list params)\n {\n \tchar msg[4096];\n-\tssize_t unused;\n \tint len = vsnprintf(msg, sizeof(msg), err, params);\n \tif (len > sizeof(msg))\n \t\tlen = sizeof(msg);\n \n-\tunused = write(child_err, \"fatal: \", 7);\n-\tunused = write(child_err, msg, len);\n-\tunused = write(child_err, \"\\n\", 1);\n+\twrite_in_full(child_err, \"fatal: \", 7);\n+\twrite_in_full(child_err, msg, len);\n+\twrite_in_full(child_err, \"\\n\", 1);\n \texit(128);\n }\n #endif\n-- \n1.7.5.rc2\n"},{"id":"166146","messageId":"20110420112122.GA6580@elie","threadId":"27128","inReplyTo":"7vaafl371q.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] report which $PATH entry had trouble running execvp(3)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-20T11:21:22Z","receivedAt":"2011-04-20T11:21:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n\n> You can add your own custom subcommand 'frotz' to the system by adding\n> 'git-frotz' in a directory somewhere in your $PATH environment variable.\n> When you ask \"git frotz\" from the command line, \"git-frotz\" is run via\n> execvp(3).\n>\n> Three plausible scenarios that the execvp(3) would fail for us are:\n[...]\n> The first one is easy to understand and to rectify.  Most likely, the user\n> made a typo, either on the command line, or when creating the custom\n> subcommand.  However, the latter two cases are harder to notice, as we do\n> not report 'git-frotz' in which directory we had trouble with.  We could\n> do better if we implemented the command search behaviour of execvp(3)\n> ourselves.\n\nMy first reaction was the same as Hannes's.  I suppose I would be\nhappier about something like an optional dependency on something\ngeneric like libexplain[1] (though I'm not thrilled about the style of\nits error messages).  If we are to implement it ourselves, using\nstandard execvp and then trying to track down a guess for the cause\nafter it fails might be okay.\n\n[1] http://libexplain.sourceforge.net/\n\nI was also reminded that anyone writing scripts following the advice\nof POSIX (meaning no #!) would find their custom git commands broken.\nLuckily that is easily fixed by using execvp with absolute path.\n\nA part of this is tempting: as Jeff mentioned, it would be nice to\navoid commit_pager_choice when checking for a dashed external before\nexecuting an alias to an internal command that doesn't want a pager\n(see v1.7.2~16^2, git --paginate: paginate external commands again,\n2010-07-14).  Hm.\n"},{"id":"166168","messageId":"7v1v0w1nji.fsf@alter.siamese.dyndns.org","threadId":"27128","inReplyTo":"20110420055111.GB28597@sigill.intra.peff.net","subject":"Re: [PATCH] report which $PATH entry had trouble running execvp(3)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-21T00:00:17Z","receivedAt":"2011-04-21T00:00:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Apr 19, 2011 at 09:01:21PM -0700, Junio C Hamano wrote:\n>\n>> You can add your own custom subcommand 'frotz' to the system by adding\n>> 'git-frotz' in a directory somewhere in your $PATH environment variable.\n>> When you ask \"git frotz\" from the command line, \"git-frotz\" is run via\n>> execvp(3).\n>> [...]\n>> we do not report 'git-frotz' in which directory we had trouble with.\n>> We could do better if we implemented the command search behaviour of\n>> execvp(3) ourselves.\n>\n> I like the idea of giving the user more information about which\n> git-frotz was the problem. Usually there is just one, and pointing them\n> to it saves them time.\n>\n> But what about the case of\n>\n>   mkdir one two\n>   touch one/frotz two/frotz\n>   PATH=one:two:$PATH\n>\n> We would report two/frotz, but might it be even better to say \"we found\n> 2 frotzes, but neither of them were executable\"?\n\nNo, one/frotz is the first one found along $PATH, and we report that we\ncannot exec 'one/frotz'.  We are trying to imitate the semantics of the\nusual command search done by execvp() and by the shell.  Three possible\nuser reactions are (1) Huh? I wanted to see two/frotz be used.  My $PATH\nis wrong, and I'll fix it by reordering elements on $PATH; (2) Huh? I\nwanted to see two/frotz be used. I have a stale one in one/frotz, and I'll\nfix it by removing it; and (3) Yuck, I forgot to chmod +x one/frotz.\n\nAs J6t mentioned, the user needs to examine what went wrong anyway.  The\npoint of the change is to make it easier by giving more information than\nwhat execvp(3) gives us (especially there may be hidden GIT_EXEC_PATH that\nthe user of a third-party scripted Porcelain may not be aware of, which is\ntacked before the usual $PATH).\n\n>> Three plausible scenarios that the execvp(3) would fail for us are:\n>> \n>>  * The first 'git-frotz' found in a directory on $PATH was not a proper\n>>    executable binary, and we got \"Exec format error\" (ENOEXEC);\n>\n> What about the magic \"unknown things get executed as shell scripts\"\n> behavior that is implemented by libc's execvp?\n> ...\n> I have always found that behavior slightly insane, but it is\n> well-established, and your sane_execvp breaks anybody who is depending\n> on it.\n\nWe can choose to add that on top of the sane_execvp() patch.\n"}]}