Re: [PATCH] report which $PATH entry had trouble running execvp(3)

From: Jeff King <peff@peff.net>
Date: Apr 20, 2011, 05:51 UTC
Message-ID: <20110420055111.GB28597@sigill.intra.peff.net>
In-Reply-To: <7vaafl371q.fsf_-_@alter.siamese.dyndns.org>

On Tue, Apr 19, 2011 at 09:01:21PM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> You can add your own custom subcommand 'frotz' to the system by adding
> 'git-frotz' in a directory somewhere in your $PATH environment variable.
> When you ask "git frotz" from the command line, "git-frotz" is run via
> execvp(3).
> [...]
> we do not report 'git-frotz' in which directory we had trouble with.
> We could do better if we implemented the command search behaviour of
> execvp(3) ourselves.

I like the idea of giving the user more information about which git-frotz was the problem. Usually there is just one, and pointing them to it saves them time.

But what about the case of

  mkdir one two
  touch one/frotz two/frotz
  PATH=one:two:$PATH

We would report two/frotz, but might it be even better to say "we found 2 frotzes, but neither of them were executable"?

I don't know if it is worth the effort for such a weird corner case.

> Three plausible scenarios that the execvp(3) would fail for us are:
> 
>  * The first 'git-frotz' found in a directory on $PATH was not a proper
>    executable binary, and we got "Exec format error" (ENOEXEC);

What about the magic "unknown things get executed as shell scripts" behavior that is implemented by libc's execvp? Your patch has a regression for:

  echo "git log --with-some-options" >local/bin/git-frotz
  chmod +x local/bin/git-frotz
  git frotz

I have always found that behavior slightly insane, but it is well-established, and your sane_execvp breaks anybody who is depending on it.

Show 9 quoted lines

> @@ -278,7 +324,7 @@ fail_pipe:
>  		} else if (cmd->use_shell) {
>  			execv_shell_cmd(cmd->argv);
>  		} else {
> -			execvp(cmd->argv[0], (char *const*) cmd->argv);
> +			cmd->argv[0] = sane_execvp(cmd->argv[0], cmd->argv);
>  		}
>  		/*
>  		 * Do not check for cmd->silent_exec_failure; the parent

This is inside "#ifndef WIN32". Presumably people on Windows want it, too. In fact, they already have their own execvp in compat/mingw.c. It might make sense to bring the implementations together. Or perhaps not. Theirs is quite different; it does a search of PATH itself, looking for executables (and magically appending ".exe"), and then exec's the result. On the other hand, doing that PATH lookup, deciding you have something, and _then_ exec'ing can be convenient. IIRC, there are a few warts in the git wrapper that could be improved by doing that, but I don't recall the specifics anymore (maybe something like handling the pager between the momemnt when we decide a command exists and when we exec?).

-Peff