Re: [PATCH] run-command.c: Accept EACCES as command not found

From: Frans Klaver <fransklaver@gmail.com>
Date: Nov 21, 2011, 23:06 UTC
Message-ID: <op.v5bjtk1r0aolir@keputer>
In-Reply-To: <7vbos5f7ix.fsf@alter.siamese.dyndns.org>

On Mon, 21 Nov 2011 23:13:58 +0100, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> Frans Klaver <fransklaver@gmail.com> writes:
>
>> execvp returns ENOENT if a command was not found after searching PATH.
>> If path contains a directory that current user has insufficient
>> privileges to, EACCES is returned. This may still mean the program
>> wasn't found.
>>
>> If the latter case is encountered, git errors out without giving aliases
>> a try,...
>
> Isn't that a *good* thing in general, though, so that the user can
> diagnose the breakage in the $PATH and fix it?

Actually I went through diagnosing and fixing it. After tracking it down, I did wonder about this question myself and I didn't come to a definitive conclusion on it. On one hand I do agree that it may be an incentive for the user to fix his path. On the other hand I found it an obscure one to track down; git's behavior doesn't match bash behavior:

$ git config --global alias.aliasedinit init && mkdir searchpath && chmod 400 searchpath && PATH=$(pwd)/searchpath:$PATH && export PATH && mkdir someproject && cd someproject && git aliasedinit fatal: cannot exec 'git-aliasedinit': Permission denied

$ git-aliasedinit bash: git-aliasedinit: command not found

This isn't very intuitive to track down an incorrect PATH with, imo. You have to dig into git core code, learn about how git handles commands, learn about debugging forked processes, and find out that execvp uses EACCES for more than just "permission denied" _just_ to find out you've got a wrong environment variable lying about. That's a full day of work gone for a newbie. If bash would also tell me in natural language that permission was denied, I wouldn't even have considered doing this patch.

For my part the root of the problem lies with the use of EACCES by execvp here, not so much with how git uses it. For this particular case, EACCES doesn't just mean "it exists, but you cannot execute this", it may also mean "not found, but one of the paths could not be accessed". If git were to provide a really helpful message, we'd have to detect which paths got denied. Once we know that, we can even on the spot decide to error out or not. In other words, we'd have to figure out which meaning of EACCES is actually used. Based on that, git can error out, warn or ignore at will.

In any case, I thought it best to have other developers have a look at it. I can put a bit more of that information in the commit message, but I'd be just as happy to drop the patch and keep the exercise.

Frans