Re: [PATCH 5/5] run-command: Error out if interpreter not found

From: Frans Klaver <fransklaver@gmail.com>
Date: Jan 27, 2012, 11:46 UTC
Message-ID: <CAH6sp9O0WG++mqq-YvOo3BUZioNWkKhCjGo4ySvSTaNFaY9afQ@mail.gmail.com>
In-Reply-To: <20120127094145.GA2611@burratino>

On Fri, Jan 27, 2012 at 10:41 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 8 quoted lines

> Frans Klaver wrote:
>
>> Just for my understanding: before a command is executed, a pager
>> (less/more or so) is started? We want to avoid starting the pager if
>> we won't be able to execute the command?
>
> See [1] for an example of a recent patch touching the relevant
> code path.

I'll have a look at that.

Show 6 quoted lines

> For example: if I run "git --paginate foo", foo is an alias for bar,
> and the "[pager] bar" configuration is set to point to "otherpager",
> then without this safety git launches the default pager in preparation
> for running git-foo, receives ENOENT from execvp("git-foo"), and then
> the pager has already been launched and it is too late to launch
> otherpager instead.

Something to be looked into then.

> Well, as I said, I don't know. :)  And I don't want to give false
> hopes --- it's perfectly possible and not even unlikely that this is a
> dead end and any patch in this direction will turn out not to be a
> good idea and not applied.

Hm don't worry about false hopes. I don't insist on having some of my work actually in if there's no point in including it. Contributing to the research is good enough for me if we can come to a conclusion that we can present to people running into similar issues.

> That's part of why I was really grateful to Hannes for the reminder to
> take a step back for a moment and consider whether it's worth it.
> Maybe there's another way or a more targetted way to take care of the
> motivational original confusing scenario that leads to execvp errors.

I wonder.

> (By the way, can you remind me which one that was?)

I'll even summarize my thinking and motivation about this.

I was executing the test suite on my PC. Some test for aliases failed -- git said EACCES, while for aliases one would expect ENOENT. For users expecting an alias to be executed, "cannot execute git-frotz: Access Denied" will be rather confusing. "Huh? Access denied? The file doesn't even exist?!". It took me quite some debugging in git to track this down to an inaccessible PATH entry. As someone who didn't know anything of the git internal code it took quite a bit of learning as well just to find out where we'd end up in the first place. It bothered me (and still does) that execve uses EACCES for at least four different errors:

...
    EACCES Search  permission is denied on a component of the path
prefix of filename or the
              name of a script interpreter.  (See also path_resolution(7).)
    EACCES The file or a script interpreter is not a regular file.
    EACCES Execute permission is denied for the file or a script or
ELF interpreter.
    EACCES The file system is mounted noexec.
...

Anyway, reading through that man page later on I found that a lot of errors are only mentioned once, but do contain 'or' in the problem description, like the first one of the EACCES items. ENOENT does that as well:

    ENOENT The file filename or a script or ELF interpreter does  not
exist,  or  a  shared
              library needed for file or interpreter cannot be found.

I then additionally figured that always silently passing ENOENT is a bad thing to do, because it simply can mean more than just "The file you asked for cannot be found". It means "something required cannot be found". My resulting view on this is basically that the execvp error handling git currently has, is lacking a nuance that is necessary for effective debugging. As I said earlier, it's when things go wrong people get annoyed. Even more so if you don't provide them with pointers to what might be wrong.

It also bothers me that to effectively debug program execution errors, you have to know that git uses execvp, and you have to know how it behaves. I would label that "implementation details" and as a user I really don't want to be bothered by that, especially not as a new user. For that reason I would have liked to end up with something like bash does. I would rather see "Hey dummy, can't access /some/path" or "Hey dummy, you ask for an interpreter that I have no acces to" than "Well we got EACCES, so check the following things: Do we have search permission on.... Is it a regular file? mounted noexec?..." or "We got EACCES, check man execve(2) for possible reasons", although I'd agree that even that would be slightly better than "We got EACCES".

So take of your git-guru hat and put on your new-git-user one and let it simmer for a while.