Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

Jonathan Nieder [off-list ref] writes:

Jeff King wrote:

quoted

On Wed, Mar 28, 2012 at 10:42:26AM -0700, Junio C Hamano wrote:

quoted

quoted

I am leaning to think that it would be the least surprising if we treat as
if /bin/ls does not even exist if /bin is not searchable.  If /bin/ls is
unreadable or unexecutable but /bin is searchable, then we _know_ it
exists, and we follow the usual exec*p() rule to ignore it

[...]

quoted

That sounds sensible to me. I think it involves writing our own
execvp, though, right?

If I understood Junio correctly, then checking for ENOENT and EACCES
should be enough.

Example: when I try

 :; mkdir $HOME/cannotread
 :; chmod -x $HOME/cannotread
 :; echo nonsense >$HOME/bin/cat
 :; chmod -x $HOME/bin/cat
 :; PATH=$HOME/cannotread:$HOME/bin/cat:/usr/local/bin:/usr/bin:/bin
 :; cat /etc/fstab

the shell uses /bin/cat without complaint.

Yeah, but I think that the case Peff is worried about is:

        $ >~/bin/nosuch
        $ nosuch
        nosuch: Permission denied

On Wed, Mar 28, 2012 at 11:31:10AM -0700, Junio C Hamano wrote:

Jonathan Nieder [off-list ref] writes:

quoted

Example: when I try

 :; mkdir $HOME/cannotread
 :; chmod -x $HOME/cannotread
 :; echo nonsense >$HOME/bin/cat
 :; chmod -x $HOME/bin/cat
 :; PATH=$HOME/cannotread:$HOME/bin:/usr/local/bin:/usr/bin:/bin
 :; cat /etc/fstab

the shell uses /bin/cat without complaint.

Yeah, but I think that the case Peff is worried about is:

        $ >~/bin/nosuch
        $ nosuch
        nosuch: Permission denied

Just remembering the EACCES and reporting it when no alias exists
would take care of that, no?  In other words, this seems analogous
to the example of a non-executable "cat" that is reported if no
other cat exists but does not prevent /bin/cat from being run.

On Wed, Mar 28, 2012 at 11:31:10AM -0700, Junio C Hamano wrote:

quoted

If I understood Junio correctly, then checking for ENOENT and EACCES
should be enough.

Example: when I try

 :; mkdir $HOME/cannotread
 :; chmod -x $HOME/cannotread
 :; echo nonsense >$HOME/bin/cat
 :; chmod -x $HOME/bin/cat
 :; PATH=$HOME/cannotread:$HOME/bin/cat:/usr/local/bin:/usr/bin:/bin
 :; cat /etc/fstab

the shell uses /bin/cat without complaint.

Yeah, but I think that the case Peff is worried about is:

        $ >~/bin/nosuch
        $ nosuch
        nosuch: Permission denied

Right. My reading of your suggestion was that we would differentiate
those two cases, which one cannot do simply from the return value and
errno after execvp. The former case (inaccessible directory) is common
and probably harmless. The latter (non-executable file) is rare and
probably an actual error we should point out.

I'd also be OK with saying that the latter is too rare to worry about,
and simply accept it as collateral damage (or we could even flag it with
test_expect_failure and leave it for somebody else to work on later if
they care).

-Peff

On Wed, Mar 28, 2012 at 01:40:14PM -0500, Jonathan Nieder wrote:

On Wed, Mar 28, 2012 at 11:31:10AM -0700, Junio C Hamano wrote:

quoted

Jonathan Nieder [off-list ref] writes:

quoted

quoted

Example: when I try

 :; mkdir $HOME/cannotread
 :; chmod -x $HOME/cannotread
 :; echo nonsense >$HOME/bin/cat
 :; chmod -x $HOME/bin/cat
 :; PATH=$HOME/cannotread:$HOME/bin:/usr/local/bin:/usr/bin:/bin
 :; cat /etc/fstab

the shell uses /bin/cat without complaint.

Yeah, but I think that the case Peff is worried about is:

        $ >~/bin/nosuch
        $ nosuch
        nosuch: Permission denied

Just remembering the EACCES and reporting it when no alias exists
would take care of that, no?  In other words, this seems analogous
to the example of a non-executable "cat" that is reported if no
other cat exists but does not prevent /bin/cat from being run.

That's what the patch I posted earlier does. But it means we _also_
report "permission denied" for inaccessible directories, which is
needlessly confusing (and much more common, I would think).

-Peff

Jeff King wrote:

That's what the patch I posted earlier does. But it means we _also_
report "permission denied" for inaccessible directories, which is
needlessly confusing (and much more common, I would think).

So the message could say

	$ nosuch
	nosuch: Permission denied
	hint: A permissions problem was encountered searching for or
	hint: executing that command on the $PATH.
	hint: Check your PATH setting and permissions.

or even

	$ nosuch
	nosuch: No such file or directory or permission denied

On Wed, Mar 28, 2012 at 02:45:16PM -0500, Jonathan Nieder wrote:

Jeff King wrote:

quoted

That's what the patch I posted earlier does. But it means we _also_
report "permission denied" for inaccessible directories, which is
needlessly confusing (and much more common, I would think).

So the message could say

	$ nosuch
	nosuch: Permission denied
	hint: A permissions problem was encountered searching for or
	hint: executing that command on the $PATH.
	hint: Check your PATH setting and permissions.

or even

	$ nosuch
	nosuch: No such file or directory or permission denied

That is slightly better than the current behavior, but for people in
James's situation, it's still quite ugly. How about this patch, which
just treats the inaccessible directory case as ENOENT. This matches
bash's behavior. And we don't need any other patches. In James's
situation, the problem just goes away, and we still get an error on a
nonexecutable file.

It won't continue trying aliases in the latter case, but we could put my
other patches on top if we want to. It's less compelling to do so,
though, because having "git-foo" in your path and not executable
probably _is_ a configuration error that you should deal with.

-Peff

---

diff --git a/cache.h b/cache.h
index e5e1aa4..59e1c44 100644
--- a/cache.h
+++ b/cache.h

@@ -1276,4 +1276,6 @@ extern struct startup_info *startup_info;
 /* builtin/merge.c */
 int checkout_fast_forward(const unsigned char *from, const unsigned char *to);
 
+int sane_execvp(const char *file, char *const argv[]);
+
 #endif /* CACHE_H */

diff --git a/exec_cmd.c b/exec_cmd.c
index 171e841..125fa6f 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c

@@ -134,7 +134,7 @@ int execv_git_cmd(const char **argv) {
 	trace_argv_printf(nargv, "trace: exec:");
 
 	/* execvp() can only ever return if it fails */
-	execvp("git", (char **)nargv);
+	sane_execvp("git", (char **)nargv);
 
 	trace_printf("trace: exec failed: %s\n", strerror(errno));
 

diff --git a/run-command.c b/run-command.c
index 1db8abf..4bdbea8 100644
--- a/run-command.c
+++ b/run-command.c

@@ -76,6 +76,39 @@ static inline void dup_devnull(int to)
 }
 #endif
 
+static int file_in_path_is_nonexecutable(const char *file)
+{
+	const char *p = getenv("PATH");
+
+	if (!p)
+		return 0;
+
+	while (1) {
+		const char *end = strchrnul(p, ':');
+		const char *path;
+		struct stat st;
+
+		path = mkpath("%.*s/%s", (int)(end - p), p, file);
+		if (!stat(path, &st) && access(path, X_OK) < 0)
+			return 1;
+
+		if (!*end)
+			break;
+
+		p = end + 1;
+	}
+
+	return 0;
+}
+
+int sane_execvp(const char *file, char * const argv[])
+{
+	int ret = execvp(file, argv);
+	if (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))
+		errno = ENOENT;
+	return ret;
+}
+
 static const char **prepare_shell_cmd(const char **argv)
 {
 	int argc, nargc = 0;

@@ -114,7 +147,7 @@ static int execv_shell_cmd(const char **argv)
 {
 	const char **nargv = prepare_shell_cmd(argv);
 	trace_argv_printf(nargv, "trace: exec:");
-	execvp(nargv[0], (char **)nargv);
+	sane_execvp(nargv[0], (char **)nargv);
 	free(nargv);
 	return -1;
 }

@@ -339,7 +372,7 @@ fail_pipe:
 		} else if (cmd->use_shell) {
 			execv_shell_cmd(cmd->argv);
 		} else {
-			execvp(cmd->argv[0], (char *const*) cmd->argv);
+			sane_execvp(cmd->argv[0], (char *const*) cmd->argv);
 		}
 		if (errno == ENOENT) {
 			if (!cmd->silent_exec_failure)

On Wed, Mar 28, 2012 at 04:18:51PM -0400, Jeff King wrote:

+int sane_execvp(const char *file, char * const argv[])
+{
+	int ret = execvp(file, argv);
+	if (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))
+		errno = ENOENT;
+	return ret;
+}

Hmm, this should check for (*file == '/') to handle absolute paths
properly. If you have an absolute path, I would tend to think that we
should never rewrite it into ENOENT (so if you have "/foo/bar", even if
"foo" is inaccessible, ENOENT is still the right response).

-Peff

(cc-ing Frans who had a related itch if I remember correctly[1])
Hi again,

Jeff King wrote:

Nice.

Nitpicks:

 - (end - p) is not guaranteed to fit inside an int.  What should happen
   when my PATH is very long?

 - the existence check would be simpler spelled as access(path, F_OK).

 - the above checks if there is _any_ nonexecutable instance of "file"
   in the directories listed in $PATH, but isn't what we want to check
   whether _all_ of them are nonexecutable?

+
+int sane_execvp(const char *file, char * const argv[])
+{
+	int ret = execvp(file, argv);
+	if (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))
+		errno = ENOENT;
+	return ret;
+}

Makes sense.  No objections from me.

	if (!execvp(file, argv))
		return 0;
	/*
	 * When a command can't be found because one of the directories
	 * listed in $PATH is unsearchable, execvp reports EACCES, but
	 * careful usability testing (read: analysis of occasional bug
	 * reports) reveals that "No such file or directory" is more
	 * intuitive.
	 */
	if (errno == EACCES && cannot_find_in_PATH(file))
		errno = ENOENT;
	return -1;

Thanks,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/189077/focus=189913

On Wed, Mar 28, 2012 at 03:42:21PM -0500, Jonathan Nieder wrote:

quoted

+		path = mkpath("%.*s/%s", (int)(end - p), p, file);

[...]
 - (end - p) is not guaranteed to fit inside an int.  What should happen
   when my PATH is very long?

That is the cost of using the mkpath convenience function (otherwise,
the compiler will complain that ".*" expects an int). We can do it
manually, but in practice, do you really expect your PATH environment
variable to overflow an int?

 - the existence check would be simpler spelled as access(path, F_OK).

Yeah, I think that is nicer. I went with !stat() because that is our
usual file_exists test, and I was wondering if there were any
portability issues with access(..., F_OK). However, we seem to use it
already in other places, so it should be fine.

 - the above checks if there is _any_ nonexecutable instance of "file"
   in the directories listed in $PATH, but isn't what we want to check
   whether _all_ of them are nonexecutable?

If there is one that is executable, then execvp would not have returned.
So if there is any entry that is non-executable, then they all are. And
we don't care about the actual number; we only care whether there is one
(in which case it is no ENOENT).

quoted

+int sane_execvp(const char *file, char * const argv[])
+{
+	int ret = execvp(file, argv);
+	if (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))
+		errno = ENOENT;
+	return ret;
+}

Makes sense.  No objections from me.

	if (!execvp(file, argv))
		return 0;
[...]
	return -1;

That is nicer; I have a general avoidance of rewriting return codes, but I
think it is safe to translate a non-zero execvp result into -1.

	/*
	 * When a command can't be found because one of the directories
	 * listed in $PATH is unsearchable, execvp reports EACCES, but
	 * careful usability testing (read: analysis of occasional bug
	 * reports) reveals that "No such file or directory" is more
	 * intuitive.
	 */
	if (errno == EACCES && cannot_find_in_PATH(file))
		errno = ENOENT;

I think we can even simplify cannot_find to "!exists_in_PATH" to make
it even simpler. If it exists and execvp did not execute it, then it
must be non-executable (or there is a race condition :) ).

-Peff

Jeff King wrote:

On Wed, Mar 28, 2012 at 04:18:51PM -0400, Jeff King wrote:

quoted

+int sane_execvp(const char *file, char * const argv[])
+{
+	int ret = execvp(file, argv);
+	if (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))
+		errno = ENOENT;
+	return ret;
+}

Hmm, this should check for (*file == '/') to handle absolute paths
properly.

Or rather for "strchr(file, '/')", because "path/to/cmd" does not mean to
append that string to each term of $PATH.

On Wed, Mar 28, 2012 at 03:51:33PM -0500, Jonathan Nieder wrote:

quoted

Hmm, this should check for (*file == '/') to handle absolute paths
properly.

Or rather for "strchr(file, '/')", because "path/to/cmd" does not mean to
append that string to each term of $PATH.

Yes, thanks for a sanity check.

-Peff

Jeff King wrote:

That is the cost of using the mkpath convenience function (otherwise,
the compiler will complain that ".*" expects an int). We can do it
manually, but in practice, do you really expect your PATH environment
variable to overflow an int?

I'd think a check like

	if (end - p > INT_MAX)
		die("holy cow your PATH is big");

would be good enough.  Or even

	assert(end - p <= INT_MAX);

if there is some environment limit I forgot about that makes that
always true.

quoted

	/*
	 * When a command can't be found because one of the directories
	 * listed in $PATH is unsearchable, execvp reports EACCES, but
	 * careful usability testing (read: analysis of occasional bug
	 * reports) reveals that "No such file or directory" is more
	 * intuitive.
	 */
	if (errno == EACCES && cannot_find_in_PATH(file))
		errno = ENOENT;

I think we can even simplify cannot_find to "!exists_in_PATH" to make
it even simpler. If it exists and execvp did not execute it, then it
must be non-executable (or there is a race condition :) ).

Yeah, sounds good.  With Junio's caveat, that makes:

	if (errno == EACCES && !strchr(file, '/'))
		errno = exists_in_PATH(file) ? EACCES : ENOENT;

On Wed, Mar 28, 2012 at 04:01:45PM -0500, Jonathan Nieder wrote:

Jeff King wrote:

quoted

That is the cost of using the mkpath convenience function (otherwise,
the compiler will complain that ".*" expects an int). We can do it
manually, but in practice, do you really expect your PATH environment
variable to overflow an int?

I'd think a check like

	if (end - p > INT_MAX)
		die("holy cow your PATH is big");

would be good enough.  Or even

	assert(end - p <= INT_MAX);

if there is some environment limit I forgot about that makes that
always true.

You can generally only pass a limited amount through execve. In theory
we could putenv() an arbitrarily large string, but I'm not sure we need
to worry about that. The execve limitation ranges from a few pages to a
few dozen pages by default. On recent versions of linux, it is based on
the stack rlimit. But my reading of execve(2) says that individual items
are still capped at 32 pages.

However, you have a much bigger problem with giant PATH elements, which
is that the whole thing is generally going to get stuck in a PATH_MAX
buffer and truncated. I would expect ENAMETOOLONG or EINVAL from execvp
in that case. That's what dietlibc will do. But glibc being glibc, it's
dynamically allocated there.

-Peff

On Wed, 28 Mar 2012 22:42:21 +0200, Jonathan Nieder [off-list ref]  
wrote:

(cc-ing Frans who had a related itch if I remember correctly[1])

Thanks.

[1]  
http://thread.gmane.org/gmane.comp.version-control.git/189077/focus=189913

Reminds me that I need to get me some time to work on that again.

Frans

Here's a rework of the patch based on the comments so far.

It handles empty path elements properly, and it handles the munging of
errno properly.  It uses a strbuf to avoid any path limitations (in
practice, I don't expect this to be much of an issue, but it matches
what glibc does. And this is the slow error-path anyway, so it's not a
big deal). And it has miscellaneous style fixes and comments.

No tests yet. I'll post some output on that in a minute.

---

diff --git a/cache.h b/cache.h
index e5e1aa4..59e1c44 100644
--- a/cache.h
+++ b/cache.h

@@ -1276,4 +1276,6 @@ extern struct startup_info *startup_info;
 /* builtin/merge.c */
 int checkout_fast_forward(const unsigned char *from, const unsigned char *to);
 
+int sane_execvp(const char *file, char *const argv[]);
+
 #endif /* CACHE_H */

diff --git a/exec_cmd.c b/exec_cmd.c
index 171e841..125fa6f 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c

@@ -134,7 +134,7 @@ int execv_git_cmd(const char **argv) {
 	trace_argv_printf(nargv, "trace: exec:");
 
 	/* execvp() can only ever return if it fails */
-	execvp("git", (char **)nargv);
+	sane_execvp("git", (char **)nargv);
 
 	trace_printf("trace: exec failed: %s\n", strerror(errno));
 

diff --git a/run-command.c b/run-command.c
index 1db8abf..2b0c311 100644
--- a/run-command.c
+++ b/run-command.c

@@ -76,6 +76,63 @@ static inline void dup_devnull(int to)
 }
 #endif
 
+static int exists_in_PATH(const char *file)
+{
+	const char *p = getenv("PATH");
+	struct strbuf buf = STRBUF_INIT;
+
+	if (!p || !*p)
+		return 0;
+
+	while (1) {
+		const char *end = strchrnul(p, ':');
+
+		strbuf_reset(&buf);
+
+		/* POSIX specifies an empty entry as the current directory. */
+		if (end != p) {
+			strbuf_add(&buf, p, end - p);
+			strbuf_addch(&buf, '/');
+		}
+		strbuf_addstr(&buf, file);
+
+		if (!access(buf.buf, F_OK)) {
+			strbuf_release(&buf);
+			return 1;
+		}
+
+		if (!*end)
+			break;
+		p = end + 1;
+	}
+
+	strbuf_release(&buf);
+	return 0;
+}
+
+int sane_execvp(const char *file, char * const argv[])
+{
+	if (!execvp(file, argv))
+		return 0;
+
+	/*
+	 * When a command can't be found because one of the directories
+	 * listed in $PATH is unsearchable, execvp reports EACCES, but
+	 * careful usability testing (read: analysis of occasional bug
+	 * reports) reveals that "No such file or directory" is more
+	 * intuitive.
+	 *
+	 * We avoid commands with "/", because execvp will not do $PATH
+	 * lookups in that case.
+	 *
+	 * The reassignment of EACCES to errno looks like a no-op below,
+	 * but we need to protect against exists_in_PATH overwriting errno.
+	 */
+	if (errno == EACCES && !strchr(file, '/'))
+		errno = exists_in_PATH(file) ? EACCES : ENOENT;
+	return -1;
+}
+
 static const char **prepare_shell_cmd(const char **argv)
 {
 	int argc, nargc = 0;

@@ -114,7 +171,7 @@ static int execv_shell_cmd(const char **argv)
 {
 	const char **nargv = prepare_shell_cmd(argv);
 	trace_argv_printf(nargv, "trace: exec:");
-	execvp(nargv[0], (char **)nargv);
+	sane_execvp(nargv[0], (char **)nargv);
 	free(nargv);
 	return -1;
 }

@@ -339,7 +396,7 @@ fail_pipe:
 		} else if (cmd->use_shell) {
 			execv_shell_cmd(cmd->argv);
 		} else {
-			execvp(cmd->argv[0], (char *const*) cmd->argv);
+			sane_execvp(cmd->argv[0], (char *const*) cmd->argv);
 		}
 		if (errno == ENOENT) {
 			if (!cmd->silent_exec_failure)

On Wed, Mar 28, 2012 at 05:57:04PM -0400, Jeff King wrote:

+static int exists_in_PATH(const char *file)
+{
+	const char *p = getenv("PATH");
+	struct strbuf buf = STRBUF_INIT;
+
+	if (!p || !*p)
+		return 0;

One thing to note: real execvp, when it sees a NULL $PATH, will fill in
some OS-dependent default path. My linux box has _PATH_DEFPATH, but I
don't know how portable that is (I can't find anything useful in POSIX).

No tests yet. I'll post some output on that in a minute.

So here is a quick test script to show the output for a couple different
cases. Should this be a real test script? A lot of what is being tested
is the actual stderr output in many cases, which we tend to try not to
include in tests.

-- >8 --
#!/bin/sh

rm -rf bin .git

# bin/broken is a PATH directory that cannot be searched
# bin/ok can be searched, but has a broken entry
mkdir bin bin/broken bin/ok
chmod -x bin/broken

# The "yes" command lets us know when things are working.
cat >bin/ok/git-yes <<\EOF
#!/bin/sh
echo yes
EOF
chmod +x bin/ok/git-yes

# and the "no" command is broken, and should be reported as EACCES
 >bin/ok/git-no

git init -q
git config alias.alias-yes yes
git config alias.alias-no no

PATH=$PWD/bin/broken:$PWD/bin/ok:$PATH

set -x
git does-not-exist
git yes
git no
git alias-yes
git alias-no

-- >8 --

The output I get is:

# stock git
+ git does-not-exist
fatal: cannot exec 'git-does-not-exist': Permission denied
+ git yes
yes
+ git no
fatal: cannot exec 'git-no': Permission denied
+ git alias-yes
fatal: cannot exec 'git-alias-yes': Permission denied
+ git alias-no
fatal: cannot exec 'git-alias-no': Permission denied

# my earlier patches to do alias lookup after EACCES
+ git does-not-exist
Failed to run command 'does-not-exist': Permission denied
+ git yes
yes
+ git no
Failed to run command 'no': Permission denied
+ git alias-yes
yes
+ git alias-no
Expansion of alias 'alias-no' failed; 'no': Permission denied

# this patch
+ git does-not-exist
git: 'does-not-exist' is not a git command. See 'git --help'.
+ git yes
yes
+ git no
fatal: cannot exec 'git-no': Permission denied
+ git alias-yes
yes
+ git alias-no
fatal: cannot exec 'git-no': Permission denied

-Peff

On Wed, Mar 28, 2012 at 11:57 PM, Jeff King [off-list ref] wrote:

+static int exists_in_PATH(const char *file)
+{
+       const char *p = getenv("PATH");
+       struct strbuf buf = STRBUF_INIT;
+
+       if (!p || !*p)
+               return 0;
+
+       while (1) {
+               const char *end = strchrnul(p, ':');
+
+               strbuf_reset(&buf);
+
+               /* POSIX specifies an empty entry as the current directory. */
+               if (end != p) {
+                       strbuf_add(&buf, p, end - p);
+                       strbuf_addch(&buf, '/');
+               }
+               strbuf_addstr(&buf, file);
+
+               if (!access(buf.buf, F_OK)) {
+                       strbuf_release(&buf);
+                       return 1;
+               }
+
+               if (!*end)
+                       break;
+               p = end + 1;
+       }
+
+       strbuf_release(&buf);
+       return 0;
+}

I expect that if more post-mortem checking is done, this function is
going to need a sibling that provides you with the first found entry
in PATH, so you can do more checks on it.

+
+int sane_execvp(const char *file, char * const argv[])
+{
+       if (!execvp(file, argv))
+               return 0;

+       if (errno == EACCES && !strchr(file, '/'))
+               errno = exists_in_PATH(file) ? EACCES : ENOENT;
+       return -1;
+}

One of the things I ran into while working on [1] is that quite some
errors that are produced can also be caused by the interpreter. This
does cover most of the itch I had earlier. I will still want to have
the interpreter check [2] in though; errno can for example also be set
to ENOENT if the interpreter or a required library isn't available. In
that case you wouldn't want to continue to the aliases, right?

On Thu, Mar 29, 2012 at 01:31:09PM +0200, Frans Klaver wrote:

On Wed, Mar 28, 2012 at 11:57 PM, Jeff King [off-list ref] wrote:

quoted

+static int exists_in_PATH(const char *file)

[...]

I expect that if more post-mortem checking is done, this function is
going to need a sibling that provides you with the first found entry
in PATH, so you can do more checks on it.

It should be easy to write it that way. I'm not personally planning on
adding more checks, but I think it's worth considering future additions.

One of the things I ran into while working on [1] is that quite some
errors that are produced can also be caused by the interpreter.

Yeah, they can be confusing and hard to track down. I'll leave that
topic out of this round, and you can build on it if you like.

-Peff

On Thu, 29 Mar 2012 19:20:33 +0200, Jeff King [off-list ref] wrote:

On Thu, Mar 29, 2012 at 01:31:09PM +0200, Frans Klaver wrote:

quoted

On Wed, Mar 28, 2012 at 11:57 PM, Jeff King [off-list ref] wrote:

quoted

+static int exists_in_PATH(const char *file)

[...]

I expect that if more post-mortem checking is done, this function is
going to need a sibling that provides you with the first found entry
in PATH, so you can do more checks on it.

It should be easy to write it that way. I'm not personally planning on
adding more checks, but I think it's worth considering future additions.

I have some similar code lying around. It shouldn't be too hard to rebase  
that on top of this.

quoted

One of the things I ran into while working on [1] is that quite some
errors that are produced can also be caused by the interpreter.

Yeah, they can be confusing and hard to track down. I'll leave that
topic out of this round, and you can build on it if you like.

I was planning to do that, yea.