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

From

ILIlari Liusvaara <ilari.liusvaara@elisanet.fi>

Date

Dec 31, 2009, 18:24 UTC

Message-ID

<20091231182436.GA1326@Knoppix>

In-Reply-To

<4B3CE3D2.5010502@kdbg.org>

On Thu, Dec 31, 2009 at 06:48:02PM +0100, Johannes Sixt wrote:

> Ilari Liusvaara schrieb:
> >On Thu, Dec 31, 2009 at 04:44:37PM +0100, Johannes Sixt wrote:
> >>Ilari Liusvaara schrieb:

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

The transport helpers are special: they shouldn't be built-in.

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

Won't work. The error in git command case would be noted in another memory
image. And passing that back would be nasty to say the least.
 

> 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]);
> 

-Ilari