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

[Jonathan Nieder <jrnieder@xxxxxxxxx>]

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

Jeff King wrote:

> --- 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;
> +}

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html