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

From: Frans Klaver <fransklaver@gmail.com>
Date: Feb 4, 2012, 21:31 UTC
Message-ID: <op.v86bepmh0aolir@keputer>
In-Reply-To: <20120127094145.GA2611@burratino>

On Fri, 27 Jan 2012 10:41:45 +0100, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 15 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.
>
> 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.

Show 9 quoted lines

>> On Fri, Jan 27, 2012 at 9:48 AM, Jonathan Nieder <jrnieder@gmail.com>  
>> wrote:
>
>>> 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