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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html