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

From: Frans Klaver <fransklaver@gmail.com>
Date: Jan 25, 2012, 07:09 UTC
Message-ID: <op.v8motzak0aolir@keputer>
In-Reply-To: <20120124232239.GG8222@burratino>

On Wed, 25 Jan 2012 00:22:39 +0100, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 6 quoted lines

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

The case that triggered me to work on this. I had an incorrect entry in my PATH and some aliasing tests failed. The generated command output was something like

fatal: script: Access Denied

while I was certain it didn't exist because I was expecting an alias to be executed. Also, access denied on a file that doesn't exist? That didn't make sense (it really doesn't if you don't know the execvp workings). Basically git takes no effort in actually pointing the user to the source of the problem. During the previous two versions I came to realize that this goes for all error codes execvp produces.

Show 21 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.

Might be useful indeed.

> What happens on Windows?

I haven't changed anything on the windows side, so that probably sticks to the old behavior.

Show 8 quoted lines

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

Fair enough. I originally expected it to be used more than twice in the code.

Show 19 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?

This gets the interpreter from a #! line, assuming that it actually has a #!. You might call it naive, which would be fair enough. I didn't see much point in doing the same check twice, but inlining this code in its callsite felt messy.

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

Right.

Show 23 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.

Correct guess.

Show 19 quoted lines

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

Well, this approach basically does the same thing, but this may have gotten a bit tunnel-visioned. I'll have a look at your approach and see what it gives us.

>
> Thanks for a fun read.

Thanks for an insightful review.