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

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