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

From: Johannes Sixt <j6t@kdbg.org>
Date: Dec 31, 2009, 17:48 UTC
Message-ID: <4B3CE3D2.5010502@kdbg.org>
In-Reply-To: <20091231165904.GA24243@Knoppix>

Ilari Liusvaara schrieb:

Show 7 quoted lines

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

Show 7 quoted lines

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

Show 7 quoted lines

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