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

On Fri, Jan 27, 2012 at 10:41 AM, Jonathan Nieder [off-list ref] wrote:

Frans Klaver wrote:

quoted

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.

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.