Re: [Updated PATCH 2/2] Improve transport helper exec failure reporting

[Johannes Sixt <j6t@kdbg.org>]

Ilari Liusvaara schrieb:
> On Thu, Dec 31, 2009 at 04:44:37PM +0100, Johannes Sixt wrote:
>> Ilari Liusvaara schrieb:
>>> @@ -31,13 +31,19 @@ static struct child_process *get_helper(struct transport *transport)
>> You should set helper->silent_exec_failure = 1 when you give your
>> own error message for the ENOENT case.
> 
> Ah yeah, might matter for Win32.

Actually, no. I forgot to mention that your modified start_command should 
treat ENOENT differently by looking at cmd->silent_exec_failure. But see 
below.

>> BTW, which error message do you see without your change in this
>> case? You only say "pretty much useless", but do not give an
>> example.
> 
> git: 'remote-foo' is not a git-command. See 'git --help'.
> 
> And as first line of output, such thing is utterly confusing.

And you change this by treating the helper command not as a git command, 
but as a normal command that happens to start with 'git-'. Whether this 
interpretation is suitable for the transport layer, I do not want to 
decide and I will certainly not object. :-)

An alternative solution would be to forward the silent_exec_failure flag 
to exec_git_cmd() to unify the treatment of the error condition with the 
non-git-command error path.

>>> +		else
>>> +			die("Unable to run helper %s: %s", helper->argv[0],
>>> +				strerror(errno));
>> You shouldn't write an error message here because start_command has
>> already reported the error.
> 
> Its not printed on Unix.

I see.

Documentation/technical/api-run-command.txt documents the error behavior. 
There are three error cases:

1. system call failures
2. exec failure due to ENOENT
3. non-zero exit of the child and death through signal

Your patch makes all exec failures into category 1, but IMO, these are 
actually category 3 (except for the ENOENT case).

In case 3, it is expected that the child process prints a suitable error 
message. Therefore, you should start with merely replacing the unconditional

	exit(127);
by
	if (errno == ENOENT)
		exit(127);
	else
		die_errno("Cannot exec %s", cmd->argv[0]);

And then you can think about how you support the ENOENT case better. My 
proposal for this was to do the PATH lookup manually before the fork(), 
and then the above conditional would melt down to simply:

	die_errno("Cannot exec %s", cmd->argv[0]);

-- Hannes