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

["Frans Klaver" <fransklaver@gmail.com>]

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

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


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



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



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


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

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