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

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).

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);

 * The only 'git-frotz' found in the directories listed on $PATH were not
   marked with executable bit, and we got "Permission denied" (EACCES); or

 * No 'git-frotz' was found in the directories listed on $PATH, but one of
   the directories were unreadable, and we got EACCES.

The first one is easy to understand and to rectify.  Most likely, the user
made a typo, either on the command line, or when creating the custom
subcommand.  However, the latter two cases are harder to notice, as 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.

Add an internal function sane_execvp() that emulates execvp(3), skipping
ENOENT and EACCES while remembering a path that resulted in EACCES while
trying later directories on $PATH.  When failing the request at the end,
report the path that we had trouble with, and use it when reporting the
error.

Signed-off-by: Junio C Hamano <redacted>
---

  Junio C Hamano [off-list ref] writes:

  >> The following is a tangent that was brought up at $work.
  > ...
  > We would need to emulate what execvp() does ourselves (i.e. split $PATH,
  > prefix each component and try execv(), ignoring ENOENT or EACCES while
  > trying next component in $PATH), plus note the first path that got EACCES
  > so that we can report which script (including its leading directories) had
  > trouble executing.  Perhaps a simple enough task for beginners.

 run-command.c |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/run-command.c b/run-command.c
index f91e446..4c95f50 100644
--- a/run-command.c
+++ b/run-command.c

@@ -135,6 +135,52 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
 	return code;
 }
 
+static const char *sane_execvp(const char *arg0, const char **argv)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf failed_path = STRBUF_INIT;
+	char *path = getenv("PATH");
+	char *next;
+
+	if (!path)
+		path = "";
+
+	for (;;) {
+		next = strchrnul(path, ':');
+		if (path < next)
+			strbuf_add(&sb, path, next - path);
+		else
+			strbuf_addch(&sb, '.');
+		if (sb.len && sb.buf[sb.len - 1] != '/')
+			strbuf_addch(&sb, '/');
+		strbuf_addstr(&sb, arg0);
+		execv(sb.buf, (char * const*) argv);
+
+		/*
+		 * execvp() skips EACCES and ENOENT and goes on to try
+		 * the next entry in the $PATH, but sets errno to EACCES
+		 * when it fails at the end.
+		 */
+		if (errno == EACCES && !failed_path.len)
+			strbuf_add(&failed_path, sb.buf, sb.len);
+		if (errno != ENOENT) {
+			strbuf_release(&failed_path);
+			return strbuf_detach(&sb, NULL);
+		}
+		strbuf_release(&sb);
+		if (!*next)
+			break;
+		path = next + 1;
+	}
+	if (failed_path.len) {
+		errno = EACCES;
+		return strbuf_detach(&failed_path, NULL);
+	}
+	strbuf_release(&sb);
+	strbuf_release(&failed_path);
+	return arg0;
+}
+
 int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;

@@ -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

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

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.

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

Am 4/20/2011 6:01, schrieb Junio C Hamano:

Add an internal function sane_execvp() that emulates execvp(3), skipping
ENOENT and EACCES while remembering a path that resulted in EACCES while
trying later directories on $PATH.  When failing the request at the end,
report the path that we had trouble with, and use it when reporting the
error.

I don't think this is worth the trouble. In which way is git different
from other tools that execvp other programs?

And how do you help when the script is executable, but the interpreter is not:

$ chmod -x git	# use git itself just for exposition
$ echo '#!'"$(pwd)/git" > git-frotz
$ chmod +x git-frotz
$ git --exec-path=. frotz
fatal: cannot exec 'git-frotz': Permission denied
$ # WTF, git-frotz *is* executable and readable!?!?

IOW, when you get strange behavior, you still have to dig around to know
what went wrong.

-- Hannes

Hi,

Junio C Hamano wrote:

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).

Three plausible scenarios that the execvp(3) would fail for us are:

[...]

The first one is easy to understand and to rectify.  Most likely, the user
made a typo, either on the command line, or when creating the custom
subcommand.  However, the latter two cases are harder to notice, as 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.

My first reaction was the same as Hannes's.  I suppose I would be
happier about something like an optional dependency on something
generic like libexplain[1] (though I'm not thrilled about the style of
its error messages).  If we are to implement it ourselves, using
standard execvp and then trying to track down a guess for the cause
after it fails might be okay.

[1] http://libexplain.sourceforge.net/

I was also reminded that anyone writing scripts following the advice
of POSIX (meaning no #!) would find their custom git commands broken.
Luckily that is easily fixed by using execvp with absolute path.

A part of this is tempting: as Jeff mentioned, it would be nice to
avoid commit_pager_choice when checking for a dashed external before
executing an alias to an internal command that doesn't want a pager
(see v1.7.2~16^2, git --paginate: paginate external commands again,
2010-07-14).  Hm.

Jeff King [off-list ref] writes:

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

quoted

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"?

No, one/frotz is the first one found along $PATH, and we report that we
cannot exec 'one/frotz'.  We are trying to imitate the semantics of the
usual command search done by execvp() and by the shell.  Three possible
user reactions are (1) Huh? I wanted to see two/frotz be used.  My $PATH
is wrong, and I'll fix it by reordering elements on $PATH; (2) Huh? I
wanted to see two/frotz be used. I have a stale one in one/frotz, and I'll
fix it by removing it; and (3) Yuck, I forgot to chmod +x one/frotz.

As J6t mentioned, the user needs to examine what went wrong anyway.  The
point of the change is to make it easier by giving more information than
what execvp(3) gives us (especially there may be hidden GIT_EXEC_PATH that
the user of a third-party scripted Porcelain may not be aware of, which is
tacked before the usual $PATH).

quoted

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?
...
I have always found that behavior slightly insane, but it is
well-established, and your sane_execvp breaks anybody who is depending
on it.

We can choose to add that on top of the sane_execvp() patch.