From: Johannes Sixt <j6t@kdbg.org>
Date: Tue, 20 Dec 2016 18:33:59 GMT
Subject: Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
Message-ID: <aad0af97-7588-632d-a113-5d8372b8b7a8@kdbg.org>
In-Reply-To: <20161219232828.5075-4-sbeller@google.com>

Am 20.12.2016 um 00:28 schrieb Stefan Beller:
> +static void report_and_die(struct child_process *cmd, const char *action)
> +{
> +	int i;
> +	struct strbuf err = STRBUF_INIT;
> +	if (cmd->git_cmd)
> +		strbuf_addstr(&err, "git ");
> +	for (i = 0; cmd->argv[i]; )
> +		strbuf_addf(&err, "'%s'", cmd->argv[i]);

Take note that cmd is accessed here.

> +	die(_("could not %s %s"), action, err.buf);

Should lego sentences not be avoided? They are not exactly translator 
friendly.

Given that a lot of effort is spent elsewhere to actually *avoid* dying 
in library code, this new die() is not very welcome, I must say. 
Granted, you just add convenience functions here, and callers have 
alternatives that do not die, but still...

> +}
> +
>  int start_command(struct child_process *cmd)
>  {
>  	int need_in, need_out, need_err;
> @@ -546,6 +557,12 @@ int start_command(struct child_process *cmd)
>  	return 0;
>  }
>
> +void start_command_or_die(struct child_process *cmd)
> +{
> +	if (start_command(cmd))
> +		report_and_die(cmd, "start");

But cmd has been cleaned up at this point of call of report_and_die. The 
access noted above goes to freed memory, I think.

> +}
> ...
> +void finish_command_or_die(struct child_process *cmd)
> +{
> +	if (finish_command(cmd))
> +		report_and_die(cmd, "finish");

Same here.

> +}
> ...
> +void run_command_or_die(struct child_process *cmd)
> +{
> +	if (finish_command(cmd))
> +		report_and_die(cmd, "run");

And here as well.

> +}

-- Hannes