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

Johannes Sixt [off-list ref] writes:

No thanks. IMHO, this is already too much code for too little gain.

Thanks for bringing a bit of sanity. You have already said it "In which
way is git different from other tools that execvp other programs?" earlier
(http://thread.gmane.org/gmane.comp.version-control.git/171755/focus=171848)

On Thu, Jan 26, 2012 at 8:32 PM, Junio C Hamano [off-list ref] wrote:

Johannes Sixt [off-list ref] writes:

quoted

No thanks. IMHO, this is already too much code for too little gain.

Thanks for bringing a bit of sanity. You have already said it "In which
way is git different from other tools that execvp other programs?" earlier
(http://thread.gmane.org/gmane.comp.version-control.git/171755/focus=171848)

Well, one tool that it differs from is bash (although bash uses execve
directly I think). Personally I think this whole thing essentially a
lack of information from execv*. Also, I do agree that the code
required for this is quite more than I would have liked, but it will
reduce confusion when things go wrong. It's when things go wrong that
people get annoyed. Annoyed people look for greener grass. If that bit
of annoyance could be reduced, why not go the extra mile for that
little bit of gain?

Being as it is, I'll stop working on this. If this was pretty much
going to be /dev/null'ed from the beginning, I'd rather have heard it
after my first patches.

In any case, it has been an education so far. Thanks for that. And if
there's any issue you think I could start tackling, please don't
hesitate to cc me.

Cheers,
Frans

(+cc: Jeff because mentioning a pagination side-issue [*])
Frans Klaver wrote:

                                           If this was pretty much
going to be /dev/null'ed from the beginning, I'd rather have heard it
after my first patches.

Almost always when a developer has an itch, it is _possible_ to
massage a patch that scratches it into something acceptable to others.
And whether it is worth the trouble in terms of time is something that
only that developer can decide.

So no, I would not say these patches were not doomed from the
beginning.  However, I certainly agree that in their current form they
are more complicated than the use case justifies.

There is a tension between requirements that leaves me oddly
uncomfortable with the series:

a. on one hand, it would be nice to preserve all the current features
   of execvp(), which makes the approach of only doing post-mortem
   analysis after a failed execvp appealing;

b. on the other hand, it would be nice [*] to avoid launching a pager
   only in order to call execvp for a command that does not exist when
   the fallback might be to an alias to a command that does not want a
   pager.  That would require figuring out in advance that execvp
   would fail with ENOENT and missing out on possible system extensions
   that allow execvp to run shell built-in commands not existing on
   the filesystem.

I want to like (b), but the downside seems unacceptable.  I honestly
don't know if something like (a) would be a good idea if well
executed, so I was happy to have the opportunity to try to help
massage these patches into a form that would make the answer more
obvious.

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

quoted

                                           If this was pretty much
going to be /dev/null'ed from the beginning, I'd rather have heard it
after my first patches.

Almost always when a developer has an itch, it is _possible_ to
massage a patch that scratches it into something acceptable to others.
And whether it is worth the trouble in terms of time is something that
only that developer can decide.

Hannes' reaction and Junio's to his didn't give me the impression they
even saw a possibility.

So no, I would not say these patches were not doomed from the
beginning.  However, I certainly agree that in their current form they
are more complicated than the use case justifies.

Good. That's something we can work on.

There is a tension between requirements that leaves me oddly
uncomfortable with the series:

a. on one hand, it would be nice to preserve all the current features
  of execvp(), which makes the approach of only doing post-mortem
  analysis after a failed execvp appealing;

b. on the other hand, it would be nice [*] to avoid launching a pager
  only in order to call execvp for a command that does not exist when
  the fallback might be to an alias to a command that does not want a
  pager.  That would require figuring out in advance that execvp
  would fail with ENOENT and missing out on possible system extensions
  that allow execvp to run shell built-in commands not existing on
  the filesystem.

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?

I want to like (b), but the downside seems unacceptable.

The downside being: having to figure out what execvp is going to do?
That would be tantamount to writing your own execvp.

I honestly
don't know if something like (a) would be a good idea if well
executed, so I was happy to have the opportunity to try to help
massage these patches into a form that would make the answer more
obvious.

Given the above information, I'm happy to work on this to see if we
can mould it into something usable. Since the impact seems to go
beyond figuring out why execvp failed, I'm probably going to need some
help.

For now, I'll go through your suggestions and see what it produces.
We'll go from there.

Thanks for the heads-up.

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.

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.

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

quoted

I want to like (b), but the downside seems unacceptable.

The downside being: having to figure out what execvp is going to do?
That would be tantamount to writing your own execvp.

Exactly.

quoted

I honestly
don't know if something like (a) would be a good idea if well
executed, so I was happy to have the opportunity to try to help
massage these patches into a form that would make the answer more
obvious.

Given the above information, I'm happy to work on this

I see.

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.

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.
(By the way, can you remind me which one that was?)

Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/179635

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.

On Fri, 27 Jan 2012 10:41:45 +0100, 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.

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.

Took me a while to catch your drift, but if I understand correctly, you're  
thinking using some of the code to find out if starting the pager is a  
good idea or not. If I factor out the part that finds a command in PATH,  
there's the helper that with a fair amount of certainty, will predict  
whether 'git foo' will fail with ENOENT or not. It would fix a possible  
problem that is currently there. Obviously the only case we can catch, is  
the command not actually existing. Although it is just one of the cases  
ENOENT can be returned for, I think it is the only one git actually cares  
about when checking for it.

quoted

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

quoted

quoted

I want to like (b), but the downside seems unacceptable.

The downside being: having to figure out what execvp is going to do?
That would be tantamount to writing your own execvp.

Exactly.

So as it seems, there are a few cases where we can fairly reliably predict  
whether a command is or isn't going to be found. Unless I'm mistaken,  
dashed externals are never shell built-ins and so we don't have to be able  
to check for their existence. Then assuming that silent_exec_failure  
really only cares about commands actually not existing, we can be fairly  
naive about it. See if we can find it somewhere in PATH and if we can't  
bail out. If we can, start the pager and everything execvp then returns  
will be regarded a fatal error. In this case it would be a choice between  
spawning the wrong pager, or having a quick browse through the file system.

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.

It may be a sensible reminder. I didn't understand that comment as such.  
Maybe it's Hannes' style, I don't know.

Maybe there's another way or a more targetted way to take care of the
motivational original confusing scenario that leads to execvp errors.
(By the way, can you remind me which one that was?)

Been thinking about it and I doubt it. To find out whether EACCES is  
returned due to a PATH issue, you have to go through all of those PATH  
entries. So while you're at it, there's a lot more you can check and most  
of those checks are fairly trivial to do.

I think I've worked through all your review comments. I'll address Hannes'  
comments, create an RFC series and see where we end up.

Junio, care to be CC'd in that?

Thanks,
Frans

[1] http://thread.gmane.org/gmane.comp.version-control.git/179635