Re: [PATCH 3/5] run-command: Elaborate execvp error checking

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 24, 2012, 23:22 UTC
Message-ID: <20120124232239.GG8222@burratino>
In-Reply-To: <1327444346-6243-4-git-send-email-fransklaver@gmail.com>

Klaver wrote:

> The interpretation of errors from execvp was rather terse. For user
> convenience communication of the nature of the error can be improved.

Could you give an example?

[...]

Show 15 quoted lines

> --- a/run-command.c
> +++ b/run-command.c
> @@ -2,6 +2,7 @@
>  #include "run-command.h"
>  #include "exec_cmd.h"
>  #include "argv-array.h"
> +#include "dir.h"
>  
>  static inline void close_pair(int fd[2])
>  {
> @@ -134,6 +135,140 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
>  	return code;
>  }
>  
> +#ifndef WIN32

Not related to this patch, but I wonder if there should be a separate run-command-unix.c file so these ifdefs would no longer be necessary.

What happens on Windows?

> +static void die_file_error(const char *file, int err)
> +{
> +	die("cannot exec '%s': %s", file, strerror(err));
> +}

I suspect it might be clearer to use die() inline in the two call sites so the reader does not have to figure out the calling convention.

Show 13 quoted lines

> +
> +static char *get_interpreter(const char *first_line)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	size_t start = strspn(first_line + 2, " \t") + 2;
> +	size_t end = strcspn(first_line + start, " \t\r\n") + start;
> +
> +	if (start >= end)
> +		return NULL;
> +
> +	strbuf_add(&sb, first_line + start, end - start);
> +	return strbuf_detach(&sb, NULL);
> +}

What does this function do? What happens if first_line doesn't start with "#!"? What should happen when there is a newline instead of a command name? How about commands with quoting characters like " and backslash --- are the semantics portable in these cases?

No need to use a strbuf here: xmemdupz would take care of the allocation and copy more simply.

Show 17 quoted lines

> +static void inspect_failure(const char *argv0, int silent_exec_failure)
> +{
> +	int err = errno;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	/* errors not related to path */
> +	if (errno == E2BIG || errno == ENOMEM)
> +		die_file_error(argv0, err);
> +
> +	if (strchr(argv0, '/')) {
> +		if (file_exists(argv0)) {
> +			strbuf_add(&sb, argv0, strlen(argv0));
> +			inspect_file(&sb, err, argv0);
> +		}
> +	} else {
> +		char *path, *next;
> +		path = getenv("PATH");

I wonder if it's possible to rearrange this code to avoid deep nesting. What does the function do, anyway? (If the reader has to ask, it needs a comment or to be renamed.)

I guess the idea is to diagnose after the fact why execvp failed. Might be simplest like this:

	To diagnose execvp failure:
		if filename does not contain a '/':
			if we can't find it on the search path:
				That's the problem, dummy!
			replace filename with full path
		if file does not exist:
			just report strerror(errno)
		if not executable:
			...
		if interpreter does not exist:
			...
		if interpreter not executable:
			...
		otherwise, just report strerror(errno)

with a separate function to find a command on the PATH, complaining when it encounters an unsearchable entry.

Thanks for a fun read.

Jonathan