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

[Junio C Hamano <gitster@pobox.com>]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:

> Previously transport-helper exec failure error reporting was pretty
> much useless as it didn't report errors from execve, only from pipe
> and fork. Now that run-command passes errno from exec, use the
> improved support to actually print useful errors if execution fails.
>
> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>

Looks pretty straightforward, except that I have this nagging feeling that
we should *not* be married to the idea of "'proc->git_cmd = 1' is merely a
way to save you from typing 'git-' prefix in start_command(proc)".

> diff --git a/transport-helper.c b/transport-helper.c
> index 5078c71..0965c9b 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -31,13 +31,19 @@ static struct child_process *get_helper(struct transport *transport)
>  	helper->out = -1;
>  	helper->err = 0;
>  	helper->argv = xcalloc(4, sizeof(*helper->argv));
> -	strbuf_addf(&buf, "remote-%s", data->name);
> +	strbuf_addf(&buf, "git-remote-%s", data->name);
>  	helper->argv[0] = strbuf_detach(&buf, NULL);
>  	helper->argv[1] = transport->remote->name;
>  	helper->argv[2] = transport->url;
> -	helper->git_cmd = 1;
> -	if (start_command(helper))
> -		die("Unable to run helper: git %s", helper->argv[0]);
> +	helper->git_cmd = 0;
> +	if (start_command(helper)) {

For example, we might later want to use different $PATH only when running
a git subcommand, and telling run_command() explicitly that we are running
a git thing would help if you don't add "git-" to the command line and
drop "proc->git_cmd = 1" in the caller like your patch does.

> +		if (errno == ENOENT)
> +			die("Unable to find remote helper for \"%s\"",
> +				data->name);
> +		else
> +			die("Unable to run helper %s: %s", helper->argv[0],
> +				strerror(errno));
> +	}
>  	data->helper = helper;
>  
>  	write_str_in_full(helper->in, "capabilities\n");
> -- 
> 1.6.6.3.gaa2e1