{"thread":{"id":"30068","subject":"Bug? Bad permissions in $PATH breaks Git aliases","startedAt":"2012-03-26T23:48:29Z","lastAt":"2012-03-29T17:23:12Z","messageCount":47,"participants":["James Pickens","Jeff King","Johannes Sixt","Junio C Hamano","Jonathan Nieder","Frans Klaver"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"187797","messageId":"CAJMEqRBmuBJuUmeoAU-_xf=s10ybD9pXhUJT+fn8aHNE2WJz6A@mail.gmail.com","threadId":"30068","inReplyTo":null,"subject":"Bug? Bad permissions in $PATH breaks Git aliases","fromName":"James Pickens","fromEmail":"jepicken@gmail.com","sentAt":"2012-03-26T23:48:29Z","receivedAt":"2012-03-26T23:48:29Z","isPatch":false,"sender":{"key":"jepicken@gmail.com","avatar":null},"body":"Hi,\n\nI'm not sure if this should be considered a bug or not, but I've noticed that\nwhen my $PATH contains an inaccessible directory, Git fails to execute aliases.\nFor example:\n\ngit config alias.l log\ngit l\n# works fine\nPATH=boguspath:$PATH\nmkdir boguspath\nchmod 000 boguspath\ngit l\n# fatal: cannot exec 'git-l': Permission denied\n\nI lean towards calling it a bug, since my shell doesn't seem to care if there\nare inaccessible directories in my $PATH. It just ignores them, and I think Git\nought to do the same.\n\nJames\n"},{"id":"187806","messageId":"20120327031953.GA17338@sigill.intra.peff.net","threadId":"30068","inReplyTo":"CAJMEqRBmuBJuUmeoAU-_xf=s10ybD9pXhUJT+fn8aHNE2WJz6A@mail.gmail.com","subject":"Re: Bug? Bad permissions in $PATH breaks Git aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-27T03:19:53Z","receivedAt":"2012-03-27T03:19:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 26, 2012 at 04:48:29PM -0700, James Pickens wrote:\n\n> I'm not sure if this should be considered a bug or not, but I've noticed that\n> when my $PATH contains an inaccessible directory, Git fails to execute aliases.\n> For example:\n> \n> git config alias.l log\n> git l\n> # works fine\n> PATH=boguspath:$PATH\n> mkdir boguspath\n> chmod 000 boguspath\n> git l\n> # fatal: cannot exec 'git-l': Permission denied\n\nThis seems to come up about once a year. The short of it is that execve\nwill return EACCESS whether the file exists is not actually executable\nby you, or if you have an inaccessible element in your PATH. execvp will\ncontinue the search if it sees EACCESS, but will return EACCESS if it\nfinds nothing. So git just sees the EACCESS and doesn't know if you\nhave bogus entries in your PATH or if there is a permissions problem\nwith your executable files.\n\nFor something like a shell, it's not that big a deal; either way, you\ncouldn't execute the command in question. For git, it matters more\nbecause we first try to exec an external command, and then fall back to\nan alias (because externals take precedence over aliases).\n\nSo basically our options are:\n\n 1. Start treating EACCESS silently as ENOENT. The downside is that we\n fail to report the proper error when the file really does have\n permissions problems (we would say \"command not found\", but that is\n misleading).\n\n 2. Implement our own execvp, differentiating between \"path not\n available for looking in\" and \"we found the command, but there was\n a permissions problem\". I think somebody was working on this a few\n months ago (search for \"add exeecvp failure diagnostics\") but it\n seems to have fizzled.\n\n 3. If we get an EACCESS, remember it, try to do the alias lookup, and\n then if that fails, report the \"Permission denied\" error (not\n \"command not found\"). That is following the spirit of what execvp\n does (it will find later entries in the PATH if they are there, but\n otherwise will remember the EACCESS error).\n\n>From what I can tell, dash uses stock execvp, and ends up closest to\n(3). Bash seems to have implemented their own path lookup, as it will\ndistinguish between the two cases as in (2):\n\n $ mkdir /tmp/foo\n $ chmod 0 /tmp/foo\n $ PATH=/tmp/foo:$PATH\n $ dash -c does-not-exist\n dash: 1: does-not-exist: Permission denied\n $ bash -c does-not-exist\n bash: does-not-exist: command not found\n\n $ chmod 755 /tmp/foo\n $ >/tmp/foo/does-not-exist\n $ chmod 0 /tmp/foo/does-not-exist\n $ dash -c does-not-exist\n dash: 1: does-not-exist: Permission denied\n $ bash -c does-not-exist\n bash: /tmp/foo/does-not-exist: Permission denied\n\nI think the general feeling last time this came up was \"why not just\nremove the cruft from your PATH?\" But I would personally be OK with\noption (3) above, and it is probably not that hard to implement.\n\n-Peff\n"},{"id":"187825","messageId":"4F715ABD.4080102@viscovery.net","threadId":"30068","inReplyTo":"CAJMEqRBmuBJuUmeoAU-_xf=s10ybD9pXhUJT+fn8aHNE2WJz6A@mail.gmail.com","subject":"Re: Bug? Bad permissions in $PATH breaks Git aliases","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-03-27T06:14:21Z","receivedAt":"2012-03-27T06:14:21Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/27/2012 1:48, schrieb James Pickens:\n> I'm not sure if this should be considered a bug or not, but I've noticed that\n> when my $PATH contains an inaccessible directory, Git fails to execute aliases.\n> ...\n> I lean towards calling it a bug, since my shell doesn't seem to care if there\n> are inaccessible directories in my $PATH. It just ignores them, and I think Git\n> ought to do the same.\n\nGit is not a shell. And I'm sure it is not the only program that has this\nissue. \"Don't do it, then.\"\n\nGit's implementation depends on the system's execvp behavior. As it\nstands, current implementations of execvp path lookup and of popular\nshells' path lookup differ in this respect. Bad luck. Don't check the\nsanity of your PATH by testing how your shell looks up executables.\n\n-- Hannes\n"},{"id":"187837","messageId":"CAJMEqRCMJ6pEi3=HHyHA5v_+UBgmi8Qa9-XiMasnXC64UtEoYw@mail.gmail.com","threadId":"30068","inReplyTo":"20120327031953.GA17338@sigill.intra.peff.net","subject":"Re: Bug? Bad permissions in $PATH breaks Git aliases","fromName":"James Pickens","fromEmail":"jepicken@gmail.com","sentAt":"2012-03-27T07:25:24Z","receivedAt":"2012-03-27T07:25:24Z","isPatch":false,"sender":{"key":"jepicken@gmail.com","avatar":null},"body":"On Mon, Mar 26, 2012 at 8:19 PM, Jeff King wrote:\n> This seems to come up about once a year. The short of it is that execve\n> will return EACCESS whether the file exists is not actually executable\n> by you, or if you have an inaccessible element in your PATH. execvp will\n> continue the search if it sees EACCESS, but will return EACCESS if it\n> finds nothing.  So git just sees the EACCESS and doesn't know if you\n> have bogus entries in your PATH or if there is a permissions problem\n> with your executable files.\n>\n> For something like a shell, it's not that big a deal; either way, you\n> couldn't execute the command in question. For git, it matters more\n> because we first try to exec an external command, and then fall back to\n> an alias (because externals take precedence over aliases).\n>\n> So basically our options are:\n>\n>  1. Start treating EACCESS silently as ENOENT. The downside is that we\n>     fail to report the proper error when the file really does have\n>     permissions problems (we would say \"command not found\", but that is\n>     misleading).\n>\n>  2. Implement our own execvp, differentiating between \"path not\n>     available for looking in\" and \"we found the command, but there was\n>     a permissions problem\". I think somebody was working on this a few\n>     months ago (search for \"add exeecvp failure diagnostics\") but it\n>     seems to have fizzled.\n>\n>  3. If we get an EACCESS, remember it, try to do the alias lookup, and\n>     then if that fails, report the \"Permission denied\" error (not\n>     \"command not found\"). That is following the spirit of what execvp\n>     does (it will find later entries in the PATH if they are there, but\n>     otherwise will remember the EACCESS error).\n\nThanks for the detailed explanation!\n\n> I think the general feeling last time this came up was \"why not just\n> remove the cruft from your PATH?\"\n\nBecause I didn't know there was cruft in my path. The cruft was put\nthere by a sloppily written project setup script that is not under my\ncontrol, and it may be difficult to get the owner of that script to\nfix it. Even after the script is fixed, we're certain to run into\nother sloppy scripts in the future.\n\nIt took me quite a while to figure out the real problem the first\ncouple of times this happened, and I'm sure many of my colleagues who\nuse the same project setup script would not have figured it out at\nall, so if there's anything Git can do to make it easier for them, I\nthink it's worth doing. BTW repairing the crufty PATH is not easy,\nbecause the project setup script will have added many unfamiliar\nthings to PATH, so you have to check them one by one to figure out\nwhich ones are bad.\n\n> But I would personally be OK with\n> option (3) above, and it is probably not that hard to implement.\n\nI would be happy with option (3). This is the part where I sheepishly\nconfess that I probably can't find time to work on this myself, but if\noption (3) is also acceptable to Junio, I may be able to find a\ncoworker to do it. So Junio, do you have any objection to (3)?\n\nJames\n"},{"id":"187839","messageId":"CAJMEqRAQZwaeMNai9wckmPE2mRVVpttzEobZrsn29fMAo+LRRQ@mail.gmail.com","threadId":"30068","inReplyTo":"4F715ABD.4080102@viscovery.net","subject":"Re: Bug? Bad permissions in $PATH breaks Git aliases","fromName":"James Pickens","fromEmail":"jepicken@gmail.com","sentAt":"2012-03-27T07:37:50Z","receivedAt":"2012-03-27T07:37:50Z","isPatch":false,"sender":{"key":"jepicken@gmail.com","avatar":null},"body":"On Mar 26, 2012 at 11:14 PM, Johannes Sixt wrote:\n> Git is not a shell. And I'm sure it is not the only program that has this\n> issue. \"Don't do it, then.\"\n\nI haven't found any other program that has this issue yet. It seems\nlike a pretty unique situation, since it's basically a side effect of\nGit having aliases with lower precedence than executables. Most\nprograms don't have that combination - they either don't have aliases\nat all, or their aliases have higher precedence than executables.\n\n> Don't check the\n> sanity of your PATH by testing how your shell looks up executables.\n\nI'm not claiming that it's sane to have a broken PATH, but as I\nmentioned in an earlier email, sometimes my PATH gets broken through\nno fault of my own, and it would be nice if Git could be more helpful\nin that case.\n\nJames\n"},{"id":"187856","messageId":"7vbonikrj4.fsf@alter.siamese.dyndns.org","threadId":"30068","inReplyTo":"20120327031953.GA17338@sigill.intra.peff.net","subject":"Re: Bug? Bad permissions in $PATH breaks Git aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-27T15:11:27Z","receivedAt":"2012-03-27T15:11:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This seems to come up about once a year....\n> ...\n> So basically our options are:\n>\n> 1. Start treating EACCESS silently as ENOENT. The downside is that we\n> fail to report the proper error when the file really does have\n> permissions problems (we would say \"command not found\", but that is\n> misleading).\n>\n> 2. Implement our own execvp, differentiating between \"path not\n> available for looking in\" and \"we found the command, but there was\n> a permissions problem\". I think somebody was working on this a few\n> months ago (search for \"add exeecvp failure diagnostics\") but it\n> seems to have fizzled.\n>\n> 3. If we get an EACCESS, remember it, try to do the alias lookup, and\n> then if that fails, report the \"Permission denied\" error (not\n> \"command not found\"). That is following the spirit of what execvp\n> does (it will find later entries in the PATH if they are there, but\n> otherwise will remember the EACCESS error).\n>\n> From what I can tell, dash uses stock execvp, and ends up closest to\n> (3). Bash seems to have implemented their own path lookup, as it will\n> distinguish between the two cases as in (2):\n> ...\n> I think the general feeling last time this came up was \"why not just\n> remove the cruft from your PATH?\" But I would personally be OK with\n> option (3) above, and it is probably not that hard to implement.\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/171755/focus=171838\nshows that it was almost exactly a year ago; we tried (2) and nobody liked\nit.\n\nI got an impression from the discussion in it that #3 may give confusing\nmessages to the end users, but I didn't think the issues through.\n"},{"id":"187857","messageId":"7v7gy6krei.fsf@alter.siamese.dyndns.org","threadId":"30068","inReplyTo":"CAJMEqRAQZwaeMNai9wckmPE2mRVVpttzEobZrsn29fMAo+LRRQ@mail.gmail.com","subject":"Re: Bug? Bad permissions in $PATH breaks Git aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-27T15:14:13Z","receivedAt":"2012-03-27T15:14:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James Pickens writes:\n\n> On Mar 26, 2012 at 11:14 PM, Johannes Sixt wrote:\n>> Git is not a shell. And I'm sure it is not the only program that has this\n>> ... \n>> Don't check the\n>> sanity of your PATH by testing how your shell looks up executables.\n>\n> I'm not claiming that it's sane to have a broken PATH, but as I\n> mentioned in an earlier email, sometimes my PATH gets broken through\n> no fault of my own, and it would be nice if Git could be more helpful\n> in that case.\n\nHrm, so which was more helpful in diagnosing the broken PATH? Git by\nletting you be aware that there is some problem, or your shell by keeping\nme oblivious of the issue?\n"},{"id":"187871","messageId":"CAJMEqRDodYQa_4vZ0+BZYS1+zL3e1iFXAMPgONbg8miEEs9wJQ@mail.gmail.com","threadId":"30068","inReplyTo":"7v7gy6krei.fsf@alter.siamese.dyndns.org","subject":"Re: Bug? Bad permissions in $PATH breaks Git aliases","fromName":"James Pickens","fromEmail":"jepicken@gmail.com","sentAt":"2012-03-27T17:48:31Z","receivedAt":"2012-03-27T17:48:31Z","isPatch":false,"sender":{"key":"jepicken@gmail.com","avatar":null},"body":"On Tue, Mar 27, 2012 at 8:14 AM, Junio C Hamano wrote:\n> James Pickens writes:\n>> I'm not claiming that it's sane to have a broken PATH, but as I\n>> mentioned in an earlier email, sometimes my PATH gets broken through\n>> no fault of my own, and it would be nice if Git could be more helpful\n>> in that case.\n>\n> Hrm, so which was more helpful in diagnosing the broken PATH?  Git by\n> letting you be aware that there is some problem, or your shell by keeping\n> me oblivious of the issue?\n\nIn this case the broken parts of my PATH were completely uninteresting\nto me - they didn't contain any executables that I would ever use. So\nif it didn't break my Git aliases, I could have continued working with\nthe broken PATH and never known or cared that it was broken.\n\nBut I get your point - sometimes it's more helpful to let the user\nknow something is amiss than try to guess what was intended. I just\ndon't think this is one of those cases, mainly because Git's behavior\nis inconsistent with other programs. Git's behavior is not even\nconsistent with itself - IMO, a PATH containing a directory that\ndoesn't exist is just as broken as a PATH containing an inaccessible\ndirectory, but Git only has a problem with the latter. That doesn't\nmake sense to me.\n\nJames\n"},{"id":"187873","messageId":"20120327175933.GA1716@sigill.intra.peff.net","threadId":"30068","inReplyTo":"7vbonikrj4.fsf@alter.siamese.dyndns.org","subject":"Re: Bug? Bad permissions in $PATH breaks Git aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-27T17:59:33Z","receivedAt":"2012-03-27T17:59:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 27, 2012 at 08:11:27AM -0700, Junio C Hamano wrote:\n\n> > I think the general feeling last time this came up was \"why not just\n> > remove the cruft from your PATH?\" But I would personally be OK with\n> > option (3) above, and it is probably not that hard to implement.\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/171755/focus=171838\n> shows that it was almost exactly a year ago; we tried (2) and nobody liked\n> it.\n\nI was actually thinking of:\n\n http://thread.gmane.org/gmane.comp.version-control.git/189077\n\n> I got an impression from the discussion in it that #3 may give confusing\n> messages to the end users, but I didn't think the issues through.\n\nThe implementation for #3 is straight-forward; I'll post the patches in\na moment. However, it still ends up being confusing, because git ends up\ntalking about permissions instead of offering its usual help.\n\nHere are a few cases with stock git and no broken entries in PATH:\n\n (1)\n $ git does-not-exist\n git: 'does-not-exist' is not a git command. See 'git --help'.\n\n (2)\n $ git cerry-pick\n git: 'cerry-pick' is not a git command. See 'git --help'.\n\n Did you mean this?\n cherry-pick\n\n (3)\n $ git config alias.broken does-not-exist\n $ git broken\n Expansion of alias 'broken' failed; 'does-not-exist' is not a git command\n\n (4)\n $ git config alias.ok '!echo ok'\n $ git ok\n ok\n\nHere are the same cases with a broken entry in PATH:\n\n $ mkdir foo; chmod 0 foo; PATH=$PWD/foo:$PATH\n\n (1)\n $ git does-not-exist\n fatal: cannot exec 'git-does-not-exist': Permission denied\n\n (2)\n $ git cerry-pick\n fatal: cannot exec 'git-cerry-pick': Permission denied\n\n (3)\n $ git broken\n fatal: cannot exec 'git-broken': Permission denied\n\n (4)\n $ git ok\n fatal: cannot exec 'git-ok': Permission denied\n\nCase (1) is OK; we report the differing error. But case (2) is worse, as\nwe don't offer suggestions any more. Cases (3) and (4) are both worse,\nbecause we don't even try to expand the alias (whether it would work or\nnot).\n\nHere are the same cases with my patches:\n\n (1)\n $ git does-not-exist\n Failed to run command 'does-not-exist': Permission denied\n\n (2)\n $ git cerry-pick\n Failed to run command 'cerry-pick': Permission denied\n\n (3)\n $ git broken\n Expansion of alias 'broken' failed; 'does-not-exist': Permission\n denied\n\n (4)\n $ git ok\n ok\n\nThis is somewhat improved. Case (4) now runs the alias. Case (3) has a\nbetter error message, which is that it tells you it was not \"broken\"\nwhich was a problem, but its subcommand. But the \"permission denied\"\nerror still ends up being somewhat confusing. And in case (2), you don't\nget a list of suggestions (nor should you, because we still don't know\nwhether \"cerry-pick\" exists and cannot be executed, or if there is a\nbroken directory in the PATH).\n\nSo we've made the situation better, but it's still way less nice than\nhaving a fixed PATH. Which makes me wonder if this half-way effort is\nworth it.\n\n-Peff\n"},{"id":"187874","messageId":"7vlimlj50g.fsf@alter.siamese.dyndns.org","threadId":"30068","inReplyTo":"CAJMEqRDodYQa_4vZ0+BZYS1+zL3e1iFXAMPgONbg8miEEs9wJQ@mail.gmail.com","subject":"Re: Bug? Bad permissions in $PATH breaks Git aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-27T18:03:11Z","receivedAt":"2012-03-27T18:03:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James Pickens writes:\n\n> On Tue, Mar 27, 2012 at 8:14 AM, Junio C Hamano wrote:\n>> James Pickens writes:\n>>> I'm not claiming that it's sane to have a broken PATH, but as I\n>>> mentioned in an earlier email, sometimes my PATH gets broken through\n>>> no fault of my own, and it would be nice if Git could be more helpful\n>>> in that case.\n>>\n>> Hrm, so which was more helpful in diagnosing the broken PATH? Git by\n>> letting you be aware that there is some problem, or your shell by keeping\n>> me oblivious of the issue?\n>\n> In this case the broken parts of my PATH were completely uninteresting\n> to me - they didn't contain any executables that I would ever use. So\n> if it didn't break my Git aliases, I could have continued working with\n> the broken PATH and never known or cared that it was broken.\n>\n> But I get your point - sometimes it's more helpful to let the user\n> know something is amiss than try to guess what was intended.\n\nThat was not the \"point\" of my question. In fact, there was no point. I\nmay be a mean person and may often throw rhetorical questions to embarrass\nothers, but I am not *that* mean to always ask only rhetorical questions ;-).\n\nJudging from your answer, it would have been better for you if Git didn't\neven tell you that there was an error due to an unreadable directory. And\nif that is the case, \"Git could be more helpful in that case\" will lead us\nin one direction (i.e. \"we simply ignore EACCESS and treat it as ENOENT\"),\nwhich is a quite different direction from what others discussed and\nsuggested in the thread (i.e. \"we give more detailed diagnosis, perhaps\nsaying \"your PATH has /usr/local/bin but it cannot be read, so we cannot\ntell git-frotz exists there or not\").\n\nI just wanted to see what was the desired behaviour you have in mind.\n"},{"id":"187875","messageId":"20120327180425.GA4659@sigill.intra.peff.net","threadId":"30068","inReplyTo":"20120327175933.GA1716@sigill.intra.peff.net","subject":"[PATCH 1/2] run-command: propagate EACCES errors to parent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-27T18:04:25Z","receivedAt":"2012-03-27T18:04:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The caller of run_command does not directly get access to\nthe errno from exec, because it happens in the forked child.\nHowever, knowing the specific reason for an exec failure can\nhelp the parent respond better or produce better error\nmessages.\n\nWe already propagate ENOENT to the parent via exit code 127.\nLet's do the same for EACCES with exit code 126, which is\nalready used by bash to indicate the same thing.\n\nSigned-off-by: Jeff King \n---\nActually, there is a slight bending of the truth in the commit message.\nbash implements its own execvp, and it will only return 126/EACCES if a\nfile is found via stat(), but is not executable. If there is an\ninaccessible directory in the PATH (meaning that stat() will fail), it\nwill silently convert that to 127/ENOENT.\n\n run-command.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 1db8abf..e303beb 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -185,6 +185,10 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \t\t\tcode = -1;\n \t\t\tfailed_errno = ENOENT;\n \t\t}\n+\t\telse if (code == 126) {\n+\t\t\tcode = -1;\n+\t\t\tfailed_errno = EACCES;\n+\t\t}\n \t} else {\n \t\terror(\"waitpid is confused (%s)\", argv0);\n \t}\n@@ -346,6 +350,11 @@ fail_pipe:\n \t\t\t\terror(\"cannot run %s: %s\", cmd->argv[0],\n \t\t\t\t\tstrerror(ENOENT));\n \t\t\texit(127);\n+\t\t} else if (errno == EACCES) {\n+\t\t\tif (!cmd->silent_exec_failure)\n+\t\t\t\terror(\"cannot run %s: %s\", cmd->argv[0],\n+\t\t\t\t\tstrerror(errno));\n+\t\t\texit(126);\n \t\t} else {\n \t\t\tdie_errno(\"cannot exec '%s'\", cmd->argv[0]);\n \t\t}\n-- \n1.7.9.5.5.g9b709b\n"},{"id":"187876","messageId":"20120327180503.GB4659@sigill.intra.peff.net","threadId":"30068","inReplyTo":"20120327175933.GA1716@sigill.intra.peff.net","subject":"[PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-27T18:05:03Z","receivedAt":"2012-03-27T18:05:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If git receives an EACCES error while trying to execute an\nexternal command, we currently give up and report the error.\nHowever, the EACCES may be caused by an inaccessible\ndirectory in the user's PATH.\n\nIn this case, execvp will skip over the inaccessible\ndirectory and keep searching the PATH. If it finds\nsomething, then that gets executed. Otherwise, the earlier\nEACCES is remembered and returned.\n\nHowever, git does not implement the same rule when looking\nup aliases. It will return immediately upon seeing EACCES\nfrom execvp, without trying aliases. This renders aliases\nunusable if there is an inaccessible directory in the PATH.\n\nThis patch implements a logical extension of execvp's lookup\nrules to aliases. We will try to find aliases even after\nexecvp returns EACCES. If there is an alias, then we expand\nit as usual. If ther eisn't, then we will remember and\nreport the EACCES error.\n\nSigned-off-by: Jeff King \n---\n git.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 3805616..917bc9e 100644\n--- a/git.c\n+++ b/git.c\n@@ -496,7 +496,7 @@ static void execv_dashed_external(const char **argv)\n \t * OK to return. Otherwise, we just pass along the status code.\n \t */\n \tstatus = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT);\n-\tif (status >= 0 || errno != ENOENT)\n+\tif (status >= 0 || (errno != ENOENT && errno != EACCES))\n \t\texit(status);\n \n \targv[0] = tmp;\n@@ -586,14 +586,16 @@ int main(int argc, const char **argv)\n \t\tstatic int done_help = 0;\n \t\tstatic int was_alias = 0;\n \t\twas_alias = run_argv(&argc, &argv);\n-\t\tif (errno != ENOENT)\n-\t\t\tbreak;\n-\t\tif (was_alias) {\n+\t\tif (was_alias && (errno == ENOENT || errno == EACCES)) {\n \t\t\tfprintf(stderr, \"Expansion of alias '%s' failed; \"\n-\t\t\t\t\"'%s' is not a git command\\n\",\n-\t\t\t\tcmd, argv[0]);\n+\t\t\t\t\"'%s'%s\\n\", cmd, argv[0],\n+\t\t\t\terrno == ENOENT ?\n+\t\t\t\t \" is not a git command\" :\n+\t\t\t\t \": Permission denied\");\n \t\t\texit(1);\n \t\t}\n+\t\tif (errno != ENOENT)\n+\t\t\tbreak;\n \t\tif (!done_help) {\n \t\t\tcmd = argv[0] = help_unknown_cmd(cmd);\n \t\t\tdone_help = 1;\n-- \n1.7.9.5.5.g9b709b\n"},{"id":"187877","messageId":"7vhax9j41p.fsf@alter.siamese.dyndns.org","threadId":"30068","inReplyTo":"20120327180425.GA4659@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] run-command: propagate EACCES errors to parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-27T18:24:02Z","receivedAt":"2012-03-27T18:24:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The caller of run_command does not directly get access to\n> the errno from exec, because it happens in the forked child.\n> However, knowing the specific reason for an exec failure can\n> help the parent respond better or produce better error\n> messages.\n>\n> We already propagate ENOENT to the parent via exit code 127.\n> Let's do the same for EACCES with exit code 126, which is\n> already used by bash to indicate the same thing.\n>\n> Signed-off-by: Jeff King \n> ---\n> Actually, there is a slight bending of the truth in the commit message.\n> bash implements its own execvp, and it will only return 126/EACCES if a\n> file is found via stat(), but is not executable. If there is an\n> inaccessible directory in the PATH (meaning that stat() will fail), it\n> will silently convert that to 127/ENOENT.\n\nI am wondering what would happen if we treated EACCESS and ENOENT exactly\nthe same way. Wouldn't the four breakage scenarios in the cover letter\nend up being even better? Case (3) will still say does-not-exist is not a\ngit command (instead of \"permission denied\", which this patch gives), but\nyour case (2) will see a much better diagnosis.\n\nTake the above with a grain of salt, though, as this is written soon after\nI wrote my response to James (the one with \"I may be a mean person\").\n\n> run-command.c | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 1db8abf..e303beb 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -185,6 +185,10 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n> \t\t\tcode = -1;\n> \t\t\tfailed_errno = ENOENT;\n> \t\t}\n> +\t\telse if (code == 126) {\n> +\t\t\tcode = -1;\n> +\t\t\tfailed_errno = EACCES;\n> +\t\t}\n> \t} else {\n> \t\terror(\"waitpid is confused (%s)\", argv0);\n> \t}\n> @@ -346,6 +350,11 @@ fail_pipe:\n> \t\t\t\terror(\"cannot run %s: %s\", cmd->argv[0],\n> \t\t\t\t\tstrerror(ENOENT));\n> \t\t\texit(127);\n> +\t\t} else if (errno == EACCES) {\n> +\t\t\tif (!cmd->silent_exec_failure)\n> +\t\t\t\terror(\"cannot run %s: %s\", cmd->argv[0],\n> +\t\t\t\t\tstrerror(errno));\n> +\t\t\texit(126);\n> \t\t} else {\n> \t\t\tdie_errno(\"cannot exec '%s'\", cmd->argv[0]);\n> \t\t}\n"},{"id":"187879","messageId":"20120327183322.GA8460@sigill.intra.peff.net","threadId":"30068","inReplyTo":"7vhax9j41p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] run-command: propagate EACCES errors to parent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-27T18:33:22Z","receivedAt":"2012-03-27T18:33:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 27, 2012 at 11:24:02AM -0700, Junio C Hamano wrote:\n\n> > Actually, there is a slight bending of the truth in the commit message.\n> > bash implements its own execvp, and it will only return 126/EACCES if a\n> > file is found via stat(), but is not executable. If there is an\n> > inaccessible directory in the PATH (meaning that stat() will fail), it\n> > will silently convert that to 127/ENOENT.\n> \n> I am wondering what would happen if we treated EACCESS and ENOENT exactly\n> the same way. Wouldn't the four breakage scenarios in the cover letter\n> end up being even better? Case (3) will still say does-not-exist is not a\n> git command (instead of \"permission denied\", which this patch gives), but\n> your case (2) will see a much better diagnosis.\n\nYes, after writing my last message detailing all of the cases, I am\ntempted to go that way. The downside is that it is more confusing if you\nhave a file in your PATH without the execute bit. IOW, we do not\ndifferentiate the common mistake of \"directory in PATH is not\naccessible\" from the uncommon \"we found /usr/bin/foo, but it is not\nexecutable by you\". While the latter case is much less common, it would\nbe nice to continue to report EACCES.\n\nWhich leads us to either implementing our own execvp, or tracing through\nthe PATH after execvp fails (which amounts to basically the same thing).\n\n-Peff\n"},{"id":"187885","messageId":"7v4nt9j1m3.fsf@alter.siamese.dyndns.org","threadId":"30068","inReplyTo":"20120327180503.GB4659@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-27T19:16:36Z","receivedAt":"2012-03-27T19:16:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If git receives an EACCES error while trying to execute an\n> external command, we currently give up and report the error.\n> However, the EACCES may be caused by an inaccessible\n> directory in the user's PATH.\n\nRegardless of EACCES/ENOENT change we discussed, the observable behaviour\nshould be testable. Something like this?\n\n t/t0061-run-command.sh | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 8d4938f..dbb1d9e 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -26,7 +26,7 @@ test_expect_success 'run_command can run a command' '\n \ttest_cmp empty err\n '\n \n-test_expect_success POSIXPERM 'run_command reports EACCES' '\n+test_expect_failure 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@@ -34,4 +34,17 @@ test_expect_success POSIXPERM 'run_command reports EACCES' '\n \tgrep \"fatal: cannot exec.*hello.sh\" err\n '\n \n+test_expect_success POSIXPERM 'unreadable directory in PATH' '\n+\tmkdir local-command &&\n+\ttest_when_finished \"chmod u+rwx local-command && rm -fr local-command\" &&\n+\tgit config alias.nitfol \"!echo frotz\" &&\n+\tchmod a-rx local-command &&\n+\t(\n+\t\tPATH=./local-command:$PATH &&\n+\t\tgit nitfol >actual\n+\t) &&\n+\techo frotz >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"187912","messageId":"20120328043058.GD30251@sigill.intra.peff.net","threadId":"30068","inReplyTo":"7v4nt9j1m3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T04:30:58Z","receivedAt":"2012-03-28T04:30:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 27, 2012 at 12:16:36PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > If git receives an EACCES error while trying to execute an\n> > external command, we currently give up and report the error.\n> > However, the EACCES may be caused by an inaccessible\n> > directory in the user's PATH.\n> \n> Regardless of EACCES/ENOENT change we discussed, the observable behaviour\n> should be testable. Something like this?\n\nYes, though I held back on writing tests, because I don't think we've\nquite decided what the behavior _should_ be. Should we be\ndifferentiating \"chmod -x /bin/ls\" from \"chmod -x /bin\"? Should we be\ncontinuing alias lookup on EACCES? Should we print edit-distance\nsuggestions on EACCES?\n\nI think the four cases from my previous email would be reasonable things\nto test, but I wasn't sure what the expected outcomes should look like.\n\n-Peff\n"},{"id":"187955","messageId":"7vaa30wrjx.fsf@alter.siamese.dyndns.org","threadId":"30068","inReplyTo":"20120328043058.GD30251@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-28T17:42:26Z","receivedAt":"2012-03-28T17:42:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Mar 27, 2012 at 12:16:36PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > If git receives an EACCES error while trying to execute an\n>> > external command, we currently give up and report the error.\n>> > However, the EACCES may be caused by an inaccessible\n>> > directory in the user's PATH.\n>> \n>> Regardless of EACCES/ENOENT change we discussed, the observable behaviour\n>> should be testable. Something like this?\n>\n> Yes, though I held back on writing tests, because I don't think we've\n> quite decided what the behavior _should_ be. Should we be\n> differentiating \"chmod -x /bin/ls\" from \"chmod -x /bin\"? Should we be\n> continuing alias lookup on EACCES? Should we print edit-distance\n> suggestions on EACCES?\n\nI am leaning to think that it would be the least surprising if we treat as\nif /bin/ls does not even exist if /bin is not searchable. If /bin/ls is\nunreadable or unexecutable but /bin is searchable, then we _know_ it\nexists, and we follow the usual exec*p() rule to ignore it so \"git ls\"\nwould try to find an alias and when all else fails will give the edit\ndistance suggestions but should exclude /bin/ls from candidates. If /bin\nitself is unsearchable, we do not even know what it contains, so it is\nneedless to say that /bin/ls will not be part of suggestion candidates.\n\nThat way, the only thing people _could_ complain about is \"I have a\ndirectory $HOME/sillybin in my $PATH but do not have an executable bit on\nit. When I try to run 'git stupid', 'git-stupid' in that diretory is not\nexecuted, and I do not even get an error message to point out that I am\nmissing the executable bit on $HOME/sillybin directory\". And you can say\n\"Ah, just like the shell. So make sure you have necessary permission bits\non things\". Very easy and straightforward to explain and understand.\n"},{"id":"187958","messageId":"20120328174841.GA27876@sigill.intra.peff.net","threadId":"30068","inReplyTo":"7vaa30wrjx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T17:48:42Z","receivedAt":"2012-03-28T17:48:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2012 at 10:42:26AM -0700, Junio C Hamano wrote:\n\n> > Yes, though I held back on writing tests, because I don't think we've\n> > quite decided what the behavior _should_ be. Should we be\n> > differentiating \"chmod -x /bin/ls\" from \"chmod -x /bin\"? Should we be\n> > continuing alias lookup on EACCES? Should we print edit-distance\n> > suggestions on EACCES?\n> \n> I am leaning to think that it would be the least surprising if we treat as\n> if /bin/ls does not even exist if /bin is not searchable. If /bin/ls is\n> unreadable or unexecutable but /bin is searchable, then we _know_ it\n> exists, and we follow the usual exec*p() rule to ignore it so \"git ls\"\n> would try to find an alias and when all else fails will give the edit\n> distance suggestions but should exclude /bin/ls from candidates. If /bin\n> itself is unsearchable, we do not even know what it contains, so it is\n> needless to say that /bin/ls will not be part of suggestion candidates.\n\nThat sounds sensible to me. I think it involves writing our own\nexecvp, though, right? If we use stock execvp, we can't tell the\ndifference between the two cases. OTOH, I think we already have an\nimplementation in compat/mingw.\n\n> That way, the only thing people _could_ complain about is \"I have a\n> directory $HOME/sillybin in my $PATH but do not have an executable bit on\n> it. When I try to run 'git stupid', 'git-stupid' in that diretory is not\n> executed, and I do not even get an error message to point out that I am\n> missing the executable bit on $HOME/sillybin directory\". And you can say\n> \"Ah, just like the shell. So make sure you have necessary permission bits\n> on things\". Very easy and straightforward to explain and understand.\n\nAgreed.\n\n-Peff\n"},{"id":"187960","messageId":"20120328180404.GA9052@burratino","threadId":"30068","inReplyTo":"20120328174841.GA27876@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-28T18:04:04Z","receivedAt":"2012-03-28T18:04:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Wed, Mar 28, 2012 at 10:42:26AM -0700, Junio C Hamano wrote:\n\n>> I am leaning to think that it would be the least surprising if we treat as\n>> if /bin/ls does not even exist if /bin is not searchable. If /bin/ls is\n>> unreadable or unexecutable but /bin is searchable, then we _know_ it\n>> exists, and we follow the usual exec*p() rule to ignore it\n[...]\n> That sounds sensible to me. I think it involves writing our own\n> execvp, though, right?\n\nIf I understood Junio correctly, then checking for ENOENT and EACCES\nshould be enough.\n\nExample: when I try\n\n :; mkdir $HOME/cannotread\n :; chmod -x $HOME/cannotread\n :; echo nonsense >$HOME/bin/cat\n :; chmod -x $HOME/bin/cat\n :; PATH=$HOME/cannotread:$HOME/bin/cat:/usr/local/bin:/usr/bin:/bin\n :; cat /etc/fstab\n\nthe shell uses /bin/cat without complaint.\n"},{"id":"187963","messageId":"7v62dowpdu.fsf@alter.siamese.dyndns.org","threadId":"30068","inReplyTo":"20120328174841.GA27876@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-28T18:29:17Z","receivedAt":"2012-03-28T18:29:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Mar 28, 2012 at 10:42:26AM -0700, Junio C Hamano wrote:\n>\n>> > Yes, though I held back on writing tests, because I don't think we've\n>> > quite decided what the behavior _should_ be. Should we be\n>> > differentiating \"chmod -x /bin/ls\" from \"chmod -x /bin\"? Should we be\n>> > continuing alias lookup on EACCES? Should we print edit-distance\n>> > suggestions on EACCES?\n>> \n>> I am leaning to think that it would be the least surprising if we treat as\n>> if /bin/ls does not even exist if /bin is not searchable. If /bin/ls is\n>> unreadable or unexecutable but /bin is searchable, then we _know_ it\n>> exists, and we follow the usual exec*p() rule to ignore it so \"git ls\"\n>> would try to find an alias and when all else fails will give the edit\n>> distance suggestions but should exclude /bin/ls from candidates. If /bin\n>> itself is unsearchable, we do not even know what it contains, so it is\n>> needless to say that /bin/ls will not be part of suggestion candidates.\n>\n> That sounds sensible to me. I think it involves writing our own\n> execvp, though, right? If we use stock execvp, we can't tell the\n> difference between the two cases.\n\nThe stock exec*p() will not hit \"/bin/ls\" in either case, so we will give\n\"'ls' is not a git command\", without having to differenciate it. That is\nwhat I meant by \"we follow the usual rule to ignore it\".\n\nWe already have the code necessary to enumerate the possible commands from\ncomponents of the PATH in order to give suggestion, so we can run it\nafter seeing exec*p() failure to see if we did not see any \"ls\", or we saw\n\"ls\" but it was not executable. No need to penalize the normal case, no?\n"},{"id":"187964","messageId":"7v1uocwpap.fsf@alter.siamese.dyndns.org","threadId":"30068","inReplyTo":"20120328180404.GA9052@burratino","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-28T18:31:10Z","receivedAt":"2012-03-28T18:31:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Jeff King wrote:\n>> On Wed, Mar 28, 2012 at 10:42:26AM -0700, Junio C Hamano wrote:\n>\n>>> I am leaning to think that it would be the least surprising if we treat as\n>>> if /bin/ls does not even exist if /bin is not searchable. If /bin/ls is\n>>> unreadable or unexecutable but /bin is searchable, then we _know_ it\n>>> exists, and we follow the usual exec*p() rule to ignore it\n> [...]\n>> That sounds sensible to me. I think it involves writing our own\n>> execvp, though, right?\n>\n> If I understood Junio correctly, then checking for ENOENT and EACCES\n> should be enough.\n>\n> Example: when I try\n>\n> :; mkdir $HOME/cannotread\n> :; chmod -x $HOME/cannotread\n> :; echo nonsense >$HOME/bin/cat\n> :; chmod -x $HOME/bin/cat\n> :; PATH=$HOME/cannotread:$HOME/bin/cat:/usr/local/bin:/usr/bin:/bin\n> :; cat /etc/fstab\n>\n> the shell uses /bin/cat without complaint.\n\nYeah, but I think that the case Peff is worried about is:\n\n $ >~/bin/nosuch\n $ nosuch\n nosuch: Permission denied\n"},{"id":"187967","messageId":"20120328184014.GA8982@burratino","threadId":"30068","inReplyTo":"7v1uocwpap.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-28T18:40:14Z","receivedAt":"2012-03-28T18:40:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"On Wed, Mar 28, 2012 at 11:31:10AM -0700, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> Example: when I try\n>>\n>> :; mkdir $HOME/cannotread\n>> :; chmod -x $HOME/cannotread\n>> :; echo nonsense >$HOME/bin/cat\n>> :; chmod -x $HOME/bin/cat\n>> :; PATH=$HOME/cannotread:$HOME/bin:/usr/local/bin:/usr/bin:/bin\n>> :; cat /etc/fstab\n>>\n>> the shell uses /bin/cat without complaint.\n>\n> Yeah, but I think that the case Peff is worried about is:\n>\n> $ >~/bin/nosuch\n> $ nosuch\n> nosuch: Permission denied\n\nJust remembering the EACCES and reporting it when no alias exists\nwould take care of that, no? In other words, this seems analogous\nto the example of a non-executable \"cat\" that is reported if no\nother cat exists but does not prevent /bin/cat from being run.\n"},{"id":"187977","messageId":"20120328193811.GA29019@sigill.intra.peff.net","threadId":"30068","inReplyTo":"7v1uocwpap.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T19:38:11Z","receivedAt":"2012-03-28T19:38:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2012 at 11:31:10AM -0700, Junio C Hamano wrote:\n\n> > If I understood Junio correctly, then checking for ENOENT and EACCES\n> > should be enough.\n> >\n> > Example: when I try\n> >\n> > :; mkdir $HOME/cannotread\n> > :; chmod -x $HOME/cannotread\n> > :; echo nonsense >$HOME/bin/cat\n> > :; chmod -x $HOME/bin/cat\n> > :; PATH=$HOME/cannotread:$HOME/bin/cat:/usr/local/bin:/usr/bin:/bin\n> > :; cat /etc/fstab\n> >\n> > the shell uses /bin/cat without complaint.\n> \n> Yeah, but I think that the case Peff is worried about is:\n> \n> $ >~/bin/nosuch\n> $ nosuch\n> nosuch: Permission denied\n\nRight. My reading of your suggestion was that we would differentiate\nthose two cases, which one cannot do simply from the return value and\nerrno after execvp. The former case (inaccessible directory) is common\nand probably harmless. The latter (non-executable file) is rare and\nprobably an actual error we should point out.\n\nI'd also be OK with saying that the latter is too rare to worry about,\nand simply accept it as collateral damage (or we could even flag it with\ntest_expect_failure and leave it for somebody else to work on later if\nthey care).\n\n-Peff\n"},{"id":"187979","messageId":"20120328193909.GB29019@sigill.intra.peff.net","threadId":"30068","inReplyTo":"20120328184014.GA8982@burratino","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T19:39:09Z","receivedAt":"2012-03-28T19:39:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2012 at 01:40:14PM -0500, Jonathan Nieder wrote:\n\n> On Wed, Mar 28, 2012 at 11:31:10AM -0700, Junio C Hamano wrote:\n> > Jonathan Nieder writes:\n> \n> >> Example: when I try\n> >>\n> >> :; mkdir $HOME/cannotread\n> >> :; chmod -x $HOME/cannotread\n> >> :; echo nonsense >$HOME/bin/cat\n> >> :; chmod -x $HOME/bin/cat\n> >> :; PATH=$HOME/cannotread:$HOME/bin:/usr/local/bin:/usr/bin:/bin\n> >> :; cat /etc/fstab\n> >>\n> >> the shell uses /bin/cat without complaint.\n> >\n> > Yeah, but I think that the case Peff is worried about is:\n> >\n> > $ >~/bin/nosuch\n> > $ nosuch\n> > nosuch: Permission denied\n> \n> Just remembering the EACCES and reporting it when no alias exists\n> would take care of that, no? In other words, this seems analogous\n> to the example of a non-executable \"cat\" that is reported if no\n> other cat exists but does not prevent /bin/cat from being run.\n\nThat's what the patch I posted earlier does. But it means we _also_\nreport \"permission denied\" for inaccessible directories, which is\nneedlessly confusing (and much more common, I would think).\n\n-Peff\n"},{"id":"187980","messageId":"20120328194045.GC29019@sigill.intra.peff.net","threadId":"30068","inReplyTo":"7v62dowpdu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T19:40:45Z","receivedAt":"2012-03-28T19:40:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2012 at 11:29:17AM -0700, Junio C Hamano wrote:\n\n> > That sounds sensible to me. I think it involves writing our own\n> > execvp, though, right? If we use stock execvp, we can't tell the\n> > difference between the two cases.\n> \n> The stock exec*p() will not hit \"/bin/ls\" in either case, so we will give\n> \"'ls' is not a git command\", without having to differenciate it. That is\n> what I meant by \"we follow the usual rule to ignore it\".\n> \n> We already have the code necessary to enumerate the possible commands from\n> components of the PATH in order to give suggestion, so we can run it\n> after seeing exec*p() failure to see if we did not see any \"ls\", or we saw\n> \"ls\" but it was not executable. No need to penalize the normal case, no?\n\nYes, we can differentiate after the fact. Though I think it ends up\nbeing almost the same code as just implementing execvp in the first\nplace.\n\n-Peff\n"},{"id":"187981","messageId":"20120328194516.GD8982@burratino","threadId":"30068","inReplyTo":"20120328193909.GB29019@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-28T19:45:16Z","receivedAt":"2012-03-28T19:45:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> That's what the patch I posted earlier does. But it means we _also_\n> report \"permission denied\" for inaccessible directories, which is\n> needlessly confusing (and much more common, I would think).\n\nSo the message could say\n\n\t$ nosuch\n\tnosuch: Permission denied\n\thint: A permissions problem was encountered searching for or\n\thint: executing that command on the $PATH.\n\thint: Check your PATH setting and permissions.\n\nor even\n\n\t$ nosuch\n\tnosuch: No such file or directory or permission denied\n"},{"id":"187992","messageId":"20120328201851.GA29315@sigill.intra.peff.net","threadId":"30068","inReplyTo":"20120328194516.GD8982@burratino","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T20:18:51Z","receivedAt":"2012-03-28T20:18:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2012 at 02:45:16PM -0500, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > That's what the patch I posted earlier does. But it means we _also_\n> > report \"permission denied\" for inaccessible directories, which is\n> > needlessly confusing (and much more common, I would think).\n> \n> So the message could say\n> \n> \t$ nosuch\n> \tnosuch: Permission denied\n> \thint: A permissions problem was encountered searching for or\n> \thint: executing that command on the $PATH.\n> \thint: Check your PATH setting and permissions.\n> \n> or even\n> \n> \t$ nosuch\n> \tnosuch: No such file or directory or permission denied\n\nThat is slightly better than the current behavior, but for people in\nJames's situation, it's still quite ugly. How about this patch, which\njust treats the inaccessible directory case as ENOENT. This matches\nbash's behavior. And we don't need any other patches. In James's\nsituation, the problem just goes away, and we still get an error on a\nnonexecutable file.\n\nIt won't continue trying aliases in the latter case, but we could put my\nother patches on top if we want to. It's less compelling to do so,\nthough, because having \"git-foo\" in your path and not executable\nprobably _is_ a configuration error that you should deal with.\n\n-Peff\n\n---\ndiff --git a/cache.h b/cache.h\nindex e5e1aa4..59e1c44 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1276,4 +1276,6 @@ extern struct startup_info *startup_info;\n /* builtin/merge.c */\n int checkout_fast_forward(const unsigned char *from, const unsigned char *to);\n \n+int sane_execvp(const char *file, char *const argv[]);\n+\n #endif /* CACHE_H */\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 171e841..125fa6f 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -134,7 +134,7 @@ int execv_git_cmd(const char **argv) {\n \ttrace_argv_printf(nargv, \"trace: exec:\");\n \n \t/* execvp() can only ever return if it fails */\n-\texecvp(\"git\", (char **)nargv);\n+\tsane_execvp(\"git\", (char **)nargv);\n \n \ttrace_printf(\"trace: exec failed: %s\\n\", strerror(errno));\n \ndiff --git a/run-command.c b/run-command.c\nindex 1db8abf..4bdbea8 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -76,6 +76,39 @@ static inline void dup_devnull(int to)\n }\n #endif\n \n+static int file_in_path_is_nonexecutable(const char *file)\n+{\n+\tconst char *p = getenv(\"PATH\");\n+\n+\tif (!p)\n+\t\treturn 0;\n+\n+\twhile (1) {\n+\t\tconst char *end = strchrnul(p, ':');\n+\t\tconst char *path;\n+\t\tstruct stat st;\n+\n+\t\tpath = mkpath(\"%.*s/%s\", (int)(end - p), p, file);\n+\t\tif (!stat(path, &st) && access(path, X_OK) < 0)\n+\t\t\treturn 1;\n+\n+\t\tif (!*end)\n+\t\t\tbreak;\n+\n+\t\tp = end + 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+int sane_execvp(const char *file, char * const argv[])\n+{\n+\tint ret = execvp(file, argv);\n+\tif (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))\n+\t\terrno = ENOENT;\n+\treturn ret;\n+}\n+\n static const char **prepare_shell_cmd(const char **argv)\n {\n \tint argc, nargc = 0;\n@@ -114,7 +147,7 @@ static int execv_shell_cmd(const char **argv)\n {\n \tconst char **nargv = prepare_shell_cmd(argv);\n \ttrace_argv_printf(nargv, \"trace: exec:\");\n-\texecvp(nargv[0], (char **)nargv);\n+\tsane_execvp(nargv[0], (char **)nargv);\n \tfree(nargv);\n \treturn -1;\n }\n@@ -339,7 +372,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\tsane_execvp(cmd->argv[0], (char *const*) cmd->argv);\n \t\t}\n \t\tif (errno == ENOENT) {\n \t\t\tif (!cmd->silent_exec_failure)\n"},{"id":"187996","messageId":"20120328203758.GA10104@sigill.intra.peff.net","threadId":"30068","inReplyTo":"20120328201851.GA29315@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T20:37:58Z","receivedAt":"2012-03-28T20:37:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2012 at 04:18:51PM -0400, Jeff King wrote:\n\n> +int sane_execvp(const char *file, char * const argv[])\n> +{\n> +\tint ret = execvp(file, argv);\n> +\tif (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))\n> +\t\terrno = ENOENT;\n> +\treturn ret;\n> +}\n\nHmm, this should check for (*file == '/') to handle absolute paths\nproperly. If you have an absolute path, I would tend to think that we\nshould never rewrite it into ENOENT (so if you have \"/foo/bar\", even if\n\"foo\" is inaccessible, ENOENT is still the right response).\n\n-Peff\n"},{"id":"187997","messageId":"20120328204221.GE8982@burratino","threadId":"30068","inReplyTo":"20120328201851.GA29315@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-28T20:42:21Z","receivedAt":"2012-03-28T20:42:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc-ing Frans who had a related itch if I remember correctly[1])\nHi again,\n\nJeff King wrote:\n\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -76,6 +76,39 @@ static inline void dup_devnull(int to)\n> }\n> #endif\n> \n> +static int file_in_path_is_nonexecutable(const char *file)\n> +{\n> +\tconst char *p = getenv(\"PATH\");\n> +\n> +\tif (!p)\n> +\t\treturn 0;\n> +\n> +\twhile (1) {\n> +\t\tconst char *end = strchrnul(p, ':');\n> +\t\tconst char *path;\n> +\t\tstruct stat st;\n> +\n> +\t\tpath = mkpath(\"%.*s/%s\", (int)(end - p), p, file);\n> +\t\tif (!stat(path, &st) && access(path, X_OK) < 0)\n> +\t\t\treturn 1;\n> +\n> +\t\tif (!*end)\n> +\t\t\tbreak;\n> +\n> +\t\tp = end + 1;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n\nNice.\n\nNitpicks:\n\n - (end - p) is not guaranteed to fit inside an int. What should happen\n when my PATH is very long?\n\n - the existence check would be simpler spelled as access(path, F_OK).\n\n - the above checks if there is _any_ nonexecutable instance of \"file\"\n in the directories listed in $PATH, but isn't what we want to check\n whether _all_ of them are nonexecutable?\n\n> +\n> +int sane_execvp(const char *file, char * const argv[])\n> +{\n> +\tint ret = execvp(file, argv);\n> +\tif (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))\n> +\t\terrno = ENOENT;\n> +\treturn ret;\n> +}\n\nMakes sense. No objections from me.\n\n\tif (!execvp(file, argv))\n\t\treturn 0;\n\t/*\n\t * When a command can't be found because one of the directories\n\t * listed in $PATH is unsearchable, execvp reports EACCES, but\n\t * careful usability testing (read: analysis of occasional bug\n\t * reports) reveals that \"No such file or directory\" is more\n\t * intuitive.\n\t */\n\tif (errno == EACCES && cannot_find_in_PATH(file))\n\t\terrno = ENOENT;\n\treturn -1;\n\nThanks,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/189077/focus=189913\n"},{"id":"187998","messageId":"7vzkb0tq10.fsf@alter.siamese.dyndns.org","threadId":"30068","inReplyTo":"20120328201851.GA29315@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-28T20:43:39Z","receivedAt":"2012-03-28T20:43:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +static int file_in_path_is_nonexecutable(const char *file)\n> +{\n> +\tconst char *p = getenv(\"PATH\");\n> +\n> +\tif (!p)\n> +\t\treturn 0;\n> +\n> +\twhile (1) {\n> +\t\tconst char *end = strchrnul(p, ':');\n> +\t\tconst char *path;\n> +\t\tstruct stat st;\n> +\n> +\t\tpath = mkpath(\"%.*s/%s\", (int)(end - p), p, file);\n\nGiven PATH=\":/usr/bin:/bin\" and file \"frotz\" (please call it \"cmd\" or\nsomething, by the way), end points at the first colon and path becomes\n\"/frotz\". Oops?\n\n> +\t\tif (!stat(path, &st) && access(path, X_OK) < 0)\n> +\t\t\treturn 1;\n> +\n> +\t\tif (!*end)\n> +\t\t\tbreak;\n> +\n> +\t\tp = end + 1;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int sane_execvp(const char *file, char * const argv[])\n> +{\n> +\tint ret = execvp(file, argv);\n> +\tif (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))\n> +\t\terrno = ENOENT;\n> +\treturn ret;\n\nDouble negation makes my head hurt, but unfortunately, we cannot rename it\nto \"executable_exists_on_path()\" and negate its return value.\n\nAnyway, the logic is to set errno to ENOENT when\n\n - We tried to exec, and got EACCES; and\n - There is a file on the PATH that lacks executable bit.\n\nIn such a case, the error from execvp() is not about the file it tried to\nexecute lacked executable bit, but there was nothing that match the name,\nbut it couldn't be certain because some directories were not readable.\n\nOK. I think I can follow that logic.\n\nIf there are more than one entry on PATH, and a system call made during\nfirst round of the loop fails but a later round finds a non-executable\nfile, i.e.\n\n\t$ PATH=/nosuch:/home/peff/bin; export PATH\n $ >/home/peff/bin/frotz; chmod -x /home/peff/bin/frotz\n git frotz\n\nwe would get EACCES from execvp(), the first round runs stat(\"/nosuch/frotz\")\nand sets errno to ENOTDIR, and the second round runs stat() and access()\non \"/home/peff/bin/frotz\" and returns 1 to say \"Yeah, there is a plain\nfile frotz that cannot be executed\".\n\nAnd sane_execvp() will return ENOTDIR?\n\nSo sane_execvp() would probably need to do a bit more (but not that much).\n\n\tif (ret < 0 && errno == EACCES)\n\t\terrno = file_in_path_is_nonexecutable(file) ? EACCES : ENOENT;\n\treturn ret;\n\nor something.\n"},{"id":"188001","messageId":"20120328205133.GF8982@burratino","threadId":"30068","inReplyTo":"20120328203758.GA10104@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-28T20:51:33Z","receivedAt":"2012-03-28T20:51:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Wed, Mar 28, 2012 at 04:18:51PM -0400, Jeff King wrote:\n\n>> +int sane_execvp(const char *file, char * const argv[])\n>> +{\n>> +\tint ret = execvp(file, argv);\n>> +\tif (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))\n>> +\t\terrno = ENOENT;\n>> +\treturn ret;\n>> +}\n>\n> Hmm, this should check for (*file == '/') to handle absolute paths\n> properly.\n\nOr rather for \"strchr(file, '/')\", because \"path/to/cmd\" does not mean to\nappend that string to each term of $PATH.\n"},{"id":"188000","messageId":"20120328205144.GA10174@sigill.intra.peff.net","threadId":"30068","inReplyTo":"20120328204221.GE8982@burratino","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T20:51:44Z","receivedAt":"2012-03-28T20:51:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2012 at 03:42:21PM -0500, Jonathan Nieder wrote:\n\n> > +\t\tpath = mkpath(\"%.*s/%s\", (int)(end - p), p, file);\n> [...]\n> - (end - p) is not guaranteed to fit inside an int. What should happen\n> when my PATH is very long?\n\nThat is the cost of using the mkpath convenience function (otherwise,\nthe compiler will complain that \".*\" expects an int). We can do it\nmanually, but in practice, do you really expect your PATH environment\nvariable to overflow an int?\n\n> - the existence check would be simpler spelled as access(path, F_OK).\n\nYeah, I think that is nicer. I went with !stat() because that is our\nusual file_exists test, and I was wondering if there were any\nportability issues with access(..., F_OK). However, we seem to use it\nalready in other places, so it should be fine.\n\n> - the above checks if there is _any_ nonexecutable instance of \"file\"\n> in the directories listed in $PATH, but isn't what we want to check\n> whether _all_ of them are nonexecutable?\n\nIf there is one that is executable, then execvp would not have returned.\nSo if there is any entry that is non-executable, then they all are. And\nwe don't care about the actual number; we only care whether there is one\n(in which case it is no ENOENT).\n\n> > +int sane_execvp(const char *file, char * const argv[])\n> > +{\n> > +\tint ret = execvp(file, argv);\n> > +\tif (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))\n> > +\t\terrno = ENOENT;\n> > +\treturn ret;\n> > +}\n> \n> Makes sense. No objections from me.\n> \n> \tif (!execvp(file, argv))\n> \t\treturn 0;\n> [...]\n> \treturn -1;\n\nThat is nicer; I have a general avoidance of rewriting return codes, but I\nthink it is safe to translate a non-zero execvp result into -1.\n\n> \t/*\n> \t * When a command can't be found because one of the directories\n> \t * listed in $PATH is unsearchable, execvp reports EACCES, but\n> \t * careful usability testing (read: analysis of occasional bug\n> \t * reports) reveals that \"No such file or directory\" is more\n> \t * intuitive.\n> \t */\n> \tif (errno == EACCES && cannot_find_in_PATH(file))\n> \t\terrno = ENOENT;\n\nI think we can even simplify cannot_find to \"!exists_in_PATH\" to make\nit even simpler. If it exists and execvp did not execute it, then it\nmust be non-executable (or there is a race condition :) ).\n\n-Peff\n"},{"id":"188002","messageId":"20120328205251.GB10174@sigill.intra.peff.net","threadId":"30068","inReplyTo":"20120328205133.GF8982@burratino","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T20:52:51Z","receivedAt":"2012-03-28T20:52:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2012 at 03:51:33PM -0500, Jonathan Nieder wrote:\n\n> > Hmm, this should check for (*file == '/') to handle absolute paths\n> > properly.\n> \n> Or rather for \"strchr(file, '/')\", because \"path/to/cmd\" does not mean to\n> append that string to each term of $PATH.\n\nYes, thanks for a sanity check.\n\n-Peff\n"},{"id":"188004","messageId":"20120328210145.GG8982@burratino","threadId":"30068","inReplyTo":"20120328205144.GA10174@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-28T21:01:45Z","receivedAt":"2012-03-28T21:01:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> That is the cost of using the mkpath convenience function (otherwise,\n> the compiler will complain that \".*\" expects an int). We can do it\n> manually, but in practice, do you really expect your PATH environment\n> variable to overflow an int?\n\nI'd think a check like\n\n\tif (end - p > INT_MAX)\n\t\tdie(\"holy cow your PATH is big\");\n\nwould be good enough. Or even\n\n\tassert(end - p <= INT_MAX);\n\nif there is some environment limit I forgot about that makes that\nalways true.\n\n>> \t/*\n>> \t * When a command can't be found because one of the directories\n>> \t * listed in $PATH is unsearchable, execvp reports EACCES, but\n>> \t * careful usability testing (read: analysis of occasional bug\n>> \t * reports) reveals that \"No such file or directory\" is more\n>> \t * intuitive.\n>> \t */\n>> \tif (errno == EACCES && cannot_find_in_PATH(file))\n>> \t\terrno = ENOENT;\n>\n> I think we can even simplify cannot_find to \"!exists_in_PATH\" to make\n> it even simpler. If it exists and execvp did not execute it, then it\n> must be non-executable (or there is a race condition :) ).\n\nYeah, sounds good. With Junio's caveat, that makes:\n\n\tif (errno == EACCES && !strchr(file, '/'))\n\t\terrno = exists_in_PATH(file) ? EACCES : ENOENT;\n"},{"id":"188006","messageId":"20120328210407.GC10174@sigill.intra.peff.net","threadId":"30068","inReplyTo":"7vzkb0tq10.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T21:04:07Z","receivedAt":"2012-03-28T21:04:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2012 at 01:43:39PM -0700, Junio C Hamano wrote:\n\n> > +\twhile (1) {\n> > +\t\tconst char *end = strchrnul(p, ':');\n> > +\t\tconst char *path;\n> > +\t\tstruct stat st;\n> > +\n> > +\t\tpath = mkpath(\"%.*s/%s\", (int)(end - p), p, file);\n> \n> Given PATH=\":/usr/bin:/bin\" and file \"frotz\" (please call it \"cmd\" or\n> something, by the way), end points at the first colon and path becomes\n> \"/frotz\". Oops?\n\nUgh, yeah. This is what I meant when I said \"checking afterwards\nbasically means re-implementing execvp\". :)\n\nRegarding the name, I pulled it from the linux-manpages execvp(3), since\nthis is supposed to be compatible. POSIX uses the even worse \"path\" as\nthe first element. But there is no reason we have to follow those naming\nconventions.\n\n> > +int sane_execvp(const char *file, char * const argv[])\n> > +{\n> > +\tint ret = execvp(file, argv);\n> > +\tif (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))\n> > +\t\terrno = ENOENT;\n> > +\treturn ret;\n> \n> Double negation makes my head hurt, but unfortunately, we cannot rename it\n> to \"executable_exists_on_path()\" and negate its return value.\n\nRight, technically it could exist in two places, and we care about\nfinding the first one.\n\nIn practice, I think we can just do exists_on_path() and not even worry\nabout the executable bit. If it exists, execvp did not run it, and we\ngot EACCES, then it is not executable. Any other error would trump\nEACCES (i.e., execvp would have returned immediately; with EACCES it\nwaits until it has processed all entries before returning EACCES).\n\nI actually think this would be easier to read if we simply\nre-implemented execvp.\n\n> Anyway, the logic is to set errno to ENOENT when\n> \n> - We tried to exec, and got EACCES; and\n> - There is a file on the PATH that lacks executable bit.\n> \n> In such a case, the error from execvp() is not about the file it tried to\n> execute lacked executable bit, but there was nothing that match the name,\n> but it couldn't be certain because some directories were not readable.\n> \n> OK. I think I can follow that logic.\n\nI think you are backwards. There is _no_ file on the PATH that lacks the\nexecutable bit, and therefore the error is about an inaccessible\ndirectory.\n\nYou could also search for an inaccessible directory, but that is not\nquite right. If you have an inaccessible directory _and_ a matching file\nwith no executable bit, then you would make the wrong assumption.\n\n> If there are more than one entry on PATH, and a system call made during\n> first round of the loop fails but a later round finds a non-executable\n> file, i.e.\n> \n> \t$ PATH=/nosuch:/home/peff/bin; export PATH\n> $ >/home/peff/bin/frotz; chmod -x /home/peff/bin/frotz\n> git frotz\n> \n> we would get EACCES from execvp(), the first round runs stat(\"/nosuch/frotz\")\n> and sets errno to ENOTDIR, and the second round runs stat() and access()\n> on \"/home/peff/bin/frotz\" and returns 1 to say \"Yeah, there is a plain\n> file frotz that cannot be executed\".\n> \n> And sane_execvp() will return ENOTDIR?\n>\n> So sane_execvp() would probably need to do a bit more (but not that much).\n> \n> \tif (ret < 0 && errno == EACCES)\n> \t\terrno = file_in_path_is_nonexecutable(file) ? EACCES : ENOENT;\n> \treturn ret;\n> \n> or something.\n\nGood point. We definitely need to save the EACCES errno across the\nsecond round lookup.\n\n-Peff\n"},{"id":"188007","messageId":"20120328212526.GA10795@sigill.intra.peff.net","threadId":"30068","inReplyTo":"20120328210145.GG8982@burratino","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T21:25:27Z","receivedAt":"2012-03-28T21:25:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2012 at 04:01:45PM -0500, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > That is the cost of using the mkpath convenience function (otherwise,\n> > the compiler will complain that \".*\" expects an int). We can do it\n> > manually, but in practice, do you really expect your PATH environment\n> > variable to overflow an int?\n> \n> I'd think a check like\n> \n> \tif (end - p > INT_MAX)\n> \t\tdie(\"holy cow your PATH is big\");\n> \n> would be good enough. Or even\n> \n> \tassert(end - p <= INT_MAX);\n> \n> if there is some environment limit I forgot about that makes that\n> always true.\n\nYou can generally only pass a limited amount through execve. In theory\nwe could putenv() an arbitrarily large string, but I'm not sure we need\nto worry about that. The execve limitation ranges from a few pages to a\nfew dozen pages by default. On recent versions of linux, it is based on\nthe stack rlimit. But my reading of execve(2) says that individual items\nare still capped at 32 pages.\n\nHowever, you have a much bigger problem with giant PATH elements, which\nis that the whole thing is generally going to get stuck in a PATH_MAX\nbuffer and truncated. I would expect ENAMETOOLONG or EINVAL from execvp\nin that case. That's what dietlibc will do. But glibc being glibc, it's\ndynamically allocated there.\n\n-Peff\n"},{"id":"188009","messageId":"op.wbwgpus00aolir@keputer","threadId":"30068","inReplyTo":"20120328204221.GE8982@burratino","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2012-03-28T21:30:56Z","receivedAt":"2012-03-28T21:30:56Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Wed, 28 Mar 2012 22:42:21 +0200, Jonathan Nieder \nwrote:\n\n> (cc-ing Frans who had a related itch if I remember correctly[1])\n\nThanks.\n\n> [1] \n> http://thread.gmane.org/gmane.comp.version-control.git/189077/focus=189913\n\nReminds me that I need to get me some time to work on that again.\n\nFrans\n"},{"id":"188011","messageId":"7vehsctn7h.fsf@alter.siamese.dyndns.org","threadId":"30068","inReplyTo":"20120328210407.GC10174@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-28T21:44:34Z","receivedAt":"2012-03-28T21:44:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Regarding the name, I pulled it from the linux-manpages execvp(3), since\n> this is supposed to be compatible. POSIX uses the even worse \"path\" as\n> the first element. But there is no reason we have to follow those naming\n> conventions.\n\nLet me take that back. We are checking if there is a non-command in a\ndirectory that is somewhere on the PATH, so calling it file like you did\nis a lot saner than calling it cmd.\n"},{"id":"188013","messageId":"20120328215704.GB10795@sigill.intra.peff.net","threadId":"30068","inReplyTo":"20120328201851.GA29315@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T21:57:04Z","receivedAt":"2012-03-28T21:57:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a rework of the patch based on the comments so far.\n\nIt handles empty path elements properly, and it handles the munging of\nerrno properly. It uses a strbuf to avoid any path limitations (in\npractice, I don't expect this to be much of an issue, but it matches\nwhat glibc does. And this is the slow error-path anyway, so it's not a\nbig deal). And it has miscellaneous style fixes and comments.\n\nNo tests yet. I'll post some output on that in a minute.\n\n---\ndiff --git a/cache.h b/cache.h\nindex e5e1aa4..59e1c44 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1276,4 +1276,6 @@ extern struct startup_info *startup_info;\n /* builtin/merge.c */\n int checkout_fast_forward(const unsigned char *from, const unsigned char *to);\n \n+int sane_execvp(const char *file, char *const argv[]);\n+\n #endif /* CACHE_H */\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 171e841..125fa6f 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -134,7 +134,7 @@ int execv_git_cmd(const char **argv) {\n \ttrace_argv_printf(nargv, \"trace: exec:\");\n \n \t/* execvp() can only ever return if it fails */\n-\texecvp(\"git\", (char **)nargv);\n+\tsane_execvp(\"git\", (char **)nargv);\n \n \ttrace_printf(\"trace: exec failed: %s\\n\", strerror(errno));\n \ndiff --git a/run-command.c b/run-command.c\nindex 1db8abf..2b0c311 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -76,6 +76,63 @@ static inline void dup_devnull(int to)\n }\n #endif\n \n+static int exists_in_PATH(const char *file)\n+{\n+\tconst char *p = getenv(\"PATH\");\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (!p || !*p)\n+\t\treturn 0;\n+\n+\twhile (1) {\n+\t\tconst char *end = strchrnul(p, ':');\n+\n+\t\tstrbuf_reset(&buf);\n+\n+\t\t/* POSIX specifies an empty entry as the current directory. */\n+\t\tif (end != p) {\n+\t\t\tstrbuf_add(&buf, p, end - p);\n+\t\t\tstrbuf_addch(&buf, '/');\n+\t\t}\n+\t\tstrbuf_addstr(&buf, file);\n+\n+\t\tif (!access(buf.buf, F_OK)) {\n+\t\t\tstrbuf_release(&buf);\n+\t\t\treturn 1;\n+\t\t}\n+\n+\t\tif (!*end)\n+\t\t\tbreak;\n+\t\tp = end + 1;\n+\t}\n+\n+\tstrbuf_release(&buf);\n+\treturn 0;\n+}\n+\n+int sane_execvp(const char *file, char * const argv[])\n+{\n+\tif (!execvp(file, argv))\n+\t\treturn 0;\n+\n+\t/*\n+\t * When a command can't be found because one of the directories\n+\t * listed in $PATH is unsearchable, execvp reports EACCES, but\n+\t * careful usability testing (read: analysis of occasional bug\n+\t * reports) reveals that \"No such file or directory\" is more\n+\t * intuitive.\n+\t *\n+\t * We avoid commands with \"/\", because execvp will not do $PATH\n+\t * lookups in that case.\n+\t *\n+\t * The reassignment of EACCES to errno looks like a no-op below,\n+\t * but we need to protect against exists_in_PATH overwriting errno.\n+\t */\n+\tif (errno == EACCES && !strchr(file, '/'))\n+\t\terrno = exists_in_PATH(file) ? EACCES : ENOENT;\n+\treturn -1;\n+}\n+\n static const char **prepare_shell_cmd(const char **argv)\n {\n \tint argc, nargc = 0;\n@@ -114,7 +171,7 @@ static int execv_shell_cmd(const char **argv)\n {\n \tconst char **nargv = prepare_shell_cmd(argv);\n \ttrace_argv_printf(nargv, \"trace: exec:\");\n-\texecvp(nargv[0], (char **)nargv);\n+\tsane_execvp(nargv[0], (char **)nargv);\n \tfree(nargv);\n \treturn -1;\n }\n@@ -339,7 +396,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\tsane_execvp(cmd->argv[0], (char *const*) cmd->argv);\n \t\t}\n \t\tif (errno == ENOENT) {\n \t\t\tif (!cmd->silent_exec_failure)\n"},{"id":"188015","messageId":"20120328220731.GC10795@sigill.intra.peff.net","threadId":"30068","inReplyTo":"20120328215704.GB10795@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T22:07:31Z","receivedAt":"2012-03-28T22:07:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2012 at 05:57:04PM -0400, Jeff King wrote:\n\n> +static int exists_in_PATH(const char *file)\n> +{\n> +\tconst char *p = getenv(\"PATH\");\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tif (!p || !*p)\n> +\t\treturn 0;\n\nOne thing to note: real execvp, when it sees a NULL $PATH, will fill in\nsome OS-dependent default path. My linux box has _PATH_DEFPATH, but I\ndon't know how portable that is (I can't find anything useful in POSIX).\n\n> No tests yet. I'll post some output on that in a minute.\n\nSo here is a quick test script to show the output for a couple different\ncases. Should this be a real test script? A lot of what is being tested\nis the actual stderr output in many cases, which we tend to try not to\ninclude in tests.\n\n-- >8 --\n#!/bin/sh\n\nrm -rf bin .git\n\n# bin/broken is a PATH directory that cannot be searched\n# bin/ok can be searched, but has a broken entry\nmkdir bin bin/broken bin/ok\nchmod -x bin/broken\n\n# The \"yes\" command lets us know when things are working.\ncat >bin/ok/git-yes <<\\EOF\n#!/bin/sh\necho yes\nEOF\nchmod +x bin/ok/git-yes\n\n# and the \"no\" command is broken, and should be reported as EACCES\n >bin/ok/git-no\n\ngit init -q\ngit config alias.alias-yes yes\ngit config alias.alias-no no\n\nPATH=$PWD/bin/broken:$PWD/bin/ok:$PATH\n\nset -x\ngit does-not-exist\ngit yes\ngit no\ngit alias-yes\ngit alias-no\n\n-- >8 --\n\nThe output I get is:\n\n# stock git\n+ git does-not-exist\nfatal: cannot exec 'git-does-not-exist': Permission denied\n+ git yes\nyes\n+ git no\nfatal: cannot exec 'git-no': Permission denied\n+ git alias-yes\nfatal: cannot exec 'git-alias-yes': Permission denied\n+ git alias-no\nfatal: cannot exec 'git-alias-no': Permission denied\n\n# my earlier patches to do alias lookup after EACCES\n+ git does-not-exist\nFailed to run command 'does-not-exist': Permission denied\n+ git yes\nyes\n+ git no\nFailed to run command 'no': Permission denied\n+ git alias-yes\nyes\n+ git alias-no\nExpansion of alias 'alias-no' failed; 'no': Permission denied\n\n# this patch\n+ git does-not-exist\ngit: 'does-not-exist' is not a git command. See 'git --help'.\n+ git yes\nyes\n+ git no\nfatal: cannot exec 'git-no': Permission denied\n+ git alias-yes\nyes\n+ git alias-no\nfatal: cannot exec 'git-no': Permission denied\n\n-Peff\n"},{"id":"188018","messageId":"7vwr64s72n.fsf@alter.siamese.dyndns.org","threadId":"30068","inReplyTo":"20120328220731.GC10795@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-28T22:18:24Z","receivedAt":"2012-03-28T22:18:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> # this patch\n> + git does-not-exist\n> git: 'does-not-exist' is not a git command. See 'git --help'.\n> + git yes\n> yes\n> + git no\n> fatal: cannot exec 'git-no': Permission denied\n> + git alias-yes\n> yes\n> + git alias-no\n> fatal: cannot exec 'git-no': Permission denied\n\nLooks sane and clean.\n"},{"id":"188052","messageId":"CAH6sp9Pw75x6YrmEyLmbsbvHrbs8r6xSp3YC2NP-jOed-zZ3+g@mail.gmail.com","threadId":"30068","inReplyTo":"20120328194045.GC29019@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2012-03-29T11:16:47Z","receivedAt":"2012-03-29T11:16:47Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"Hi,\n\nOn Wed, Mar 28, 2012 at 9:40 PM, Jeff King wrote:\n> On Wed, Mar 28, 2012 at 11:29:17AM -0700, Junio C Hamano wrote:\n>\n>> > That sounds sensible to me. I think it involves writing our own\n>> > execvp, though, right? If we use stock execvp, we can't tell the\n>> > difference between the two cases.\n>>\n>> The stock exec*p() will not hit \"/bin/ls\" in either case, so we will give\n>> \"'ls' is not a git command\", without having to differenciate it.  That is\n>> what I meant by \"we follow the usual rule to ignore it\".\n>>\n>> We already have the code necessary to enumerate the possible commands from\n>> components of the PATH in order to give suggestion, so we can run it\n>> after seeing exec*p() failure to see if we did not see any \"ls\", or we saw\n>> \"ls\" but it was not executable.  No need to penalize the normal case, no?\n>\n> Yes, we can differentiate after the fact. Though I think it ends up\n> being almost the same code as just implementing execvp in the first\n> place.\n\nIt will, but doesn't stock execv*() also provide access to shell\nbuiltins? If that's the case then I wouldn't be bothered by the extra\nbit of code we need to understand what execvp has been doing. I think\nit would be sane to keep sane_execvp a wrapper instead of a\nreimplementation.\n"},{"id":"188053","messageId":"CAH6sp9OcWUks_n1bD2n1KbePHeUX+FSY0+wLFu+zPik1Pwj3Aw@mail.gmail.com","threadId":"30068","inReplyTo":"20120328215704.GB10795@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2012-03-29T11:31:09Z","receivedAt":"2012-03-29T11:31:09Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Wed, Mar 28, 2012 at 11:57 PM, Jeff King wrote:\n\n> +static int exists_in_PATH(const char *file)\n> +{\n> +       const char *p = getenv(\"PATH\");\n> +       struct strbuf buf = STRBUF_INIT;\n> +\n> +       if (!p || !*p)\n> +               return 0;\n> +\n> +       while (1) {\n> +               const char *end = strchrnul(p, ':');\n> +\n> +               strbuf_reset(&buf);\n> +\n> +               /* POSIX specifies an empty entry as the current directory. */\n> +               if (end != p) {\n> +                       strbuf_add(&buf, p, end - p);\n> +                       strbuf_addch(&buf, '/');\n> +               }\n> +               strbuf_addstr(&buf, file);\n> +\n> +               if (!access(buf.buf, F_OK)) {\n> +                       strbuf_release(&buf);\n> +                       return 1;\n> +               }\n> +\n> +               if (!*end)\n> +                       break;\n> +               p = end + 1;\n> +       }\n> +\n> +       strbuf_release(&buf);\n> +       return 0;\n> +}\n\nI expect that if more post-mortem checking is done, this function is\ngoing to need a sibling that provides you with the first found entry\nin PATH, so you can do more checks on it.\n\n\n> +\n> +int sane_execvp(const char *file, char * const argv[])\n> +{\n> +       if (!execvp(file, argv))\n> +               return 0;\n\n> +       if (errno == EACCES && !strchr(file, '/'))\n> +               errno = exists_in_PATH(file) ? EACCES : ENOENT;\n> +       return -1;\n> +}\n\nOne of the things I ran into while working on [1] is that quite some\nerrors that are produced can also be caused by the interpreter. This\ndoes cover most of the itch I had earlier. I will still want to have\nthe interpreter check [2] in though; errno can for example also be set\nto ENOENT if the interpreter or a required library isn't available. In\nthat case you wouldn't want to continue to the aliases, right?\n"},{"id":"188075","messageId":"20120329171525.GB12318@sigill.intra.peff.net","threadId":"30068","inReplyTo":"CAH6sp9Pw75x6YrmEyLmbsbvHrbs8r6xSp3YC2NP-jOed-zZ3+g@mail.gmail.com","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-29T17:15:25Z","receivedAt":"2012-03-29T17:15:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 29, 2012 at 01:16:47PM +0200, Frans Klaver wrote:\n\n> > Yes, we can differentiate after the fact. Though I think it ends up\n> > being almost the same code as just implementing execvp in the first\n> > place.\n> \n> It will, but doesn't stock execv*() also provide access to shell\n> builtins? If that's the case then I wouldn't be bothered by the extra\n> bit of code we need to understand what execvp has been doing. I think\n> it would be sane to keep sane_execvp a wrapper instead of a\n> reimplementation.\n\nNo, definitely not. Handling builtins is the responsibility of the\nshell, not of execvp. It is responsible for falling back to \"/bin/sh\n$file\" if execve returns ENOEXEC.\n\nAnyway, I think the last round I posted is good enough. It is\napproaching execvp in complexity, but it is still a little bit simpler.\nAnd because it's on the error code path, if we are incompatible the\nworst thing we can screw up is the error message, not the actual exec.\n\n-Peff\n"},{"id":"188076","messageId":"20120329172033.GC12318@sigill.intra.peff.net","threadId":"30068","inReplyTo":"CAH6sp9OcWUks_n1bD2n1KbePHeUX+FSY0+wLFu+zPik1Pwj3Aw@mail.gmail.com","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-29T17:20:33Z","receivedAt":"2012-03-29T17:20:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 29, 2012 at 01:31:09PM +0200, Frans Klaver wrote:\n\n> On Wed, Mar 28, 2012 at 11:57 PM, Jeff King wrote:\n> \n> > +static int exists_in_PATH(const char *file)\n> [...]\n> \n> I expect that if more post-mortem checking is done, this function is\n> going to need a sibling that provides you with the first found entry\n> in PATH, so you can do more checks on it.\n\nIt should be easy to write it that way. I'm not personally planning on\nadding more checks, but I think it's worth considering future additions.\n\n> One of the things I ran into while working on [1] is that quite some\n> errors that are produced can also be caused by the interpreter.\n\nYeah, they can be confusing and hard to track down. I'll leave that\ntopic out of this round, and you can build on it if you like.\n\n-Peff\n"},{"id":"188077","messageId":"op.wbxzunjh0aolir@keputer","threadId":"30068","inReplyTo":"20120329171525.GB12318@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2012-03-29T17:21:49Z","receivedAt":"2012-03-29T17:21:49Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Thu, 29 Mar 2012 19:15:25 +0200, Jeff King wrote:\n\n> On Thu, Mar 29, 2012 at 01:16:47PM +0200, Frans Klaver wrote:\n>\n>> > Yes, we can differentiate after the fact. Though I think it ends up\n>> > being almost the same code as just implementing execvp in the first\n>> > place.\n>>\n>> It will, but doesn't stock execv*() also provide access to shell\n>> builtins? If that's the case then I wouldn't be bothered by the extra\n>> bit of code we need to understand what execvp has been doing. I think\n>> it would be sane to keep sane_execvp a wrapper instead of a\n>> reimplementation.\n>\n> No, definitely not. Handling builtins is the responsibility of the\n> shell, not of execvp. It is responsible for falling back to \"/bin/sh\n> $file\" if execve returns ENOEXEC.\n>\n> Anyway, I think the last round I posted is good enough. It is\n> approaching execvp in complexity, but it is still a little bit simpler.\n> And because it's on the error code path, if we are incompatible the\n> worst thing we can screw up is the error message, not the actual exec.\n\nGood. In that case I think this last looks good, indeed.\n\nFrans\n"},{"id":"188078","messageId":"op.wbxzwyre0aolir@keputer","threadId":"30068","inReplyTo":"20120329172033.GC12318@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git: continue alias lookup on EACCES errors","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2012-03-29T17:23:12Z","receivedAt":"2012-03-29T17:23:12Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Thu, 29 Mar 2012 19:20:33 +0200, Jeff King wrote:\n\n> On Thu, Mar 29, 2012 at 01:31:09PM +0200, Frans Klaver wrote:\n>\n>> On Wed, Mar 28, 2012 at 11:57 PM, Jeff King wrote:\n>>\n>> > +static int exists_in_PATH(const char *file)\n>> [...]\n>>\n>> I expect that if more post-mortem checking is done, this function is\n>> going to need a sibling that provides you with the first found entry\n>> in PATH, so you can do more checks on it.\n>\n> It should be easy to write it that way. I'm not personally planning on\n> adding more checks, but I think it's worth considering future additions.\n\nI have some similar code lying around. It shouldn't be too hard to rebase \nthat on top of this.\n\n\n>> One of the things I ran into while working on [1] is that quite some\n>> errors that are produced can also be caused by the interpreter.\n>\n> Yeah, they can be confusing and hard to track down. I'll leave that\n> topic out of this round, and you can build on it if you like.\n\nI was planning to do that, yea.\n"}]}