Re: [PATCH 2/3] sequencer.c: free author variable when merging fails

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 31, 2018, 12:04 UTC
Message-ID: <nycvar.QRO.7.76.6.1805311402210.82@tvgsbejvaqbjf.bet>
In-Reply-To: <20180530170302.191176-2-sbeller@google.com>

Hi Stefan,

On Wed, 30 May 2018, Stefan Beller wrote:

Show 5 quoted lines

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> This was a deliberate oversight in f241ff0d0a9 (prepare the builtins for a
> libified merge_recursive(), 2016-07-26)

No, it was not deliberate. It was inadvertent, most likely ;-)

Show 15 quoted lines

>  sequencer.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 72b4d8ecae3..5c93586cc1c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1771,8 +1771,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  	else if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command == TODO_REVERT) {
>  		res = do_recursive_merge(base, next, base_label, next_label,
>  					 &head, &msgbuf, opts);
> -		if (res < 0)
> +		if (res < 0) {
> +			free(author);
>  			return res;

Why not `goto leave;` instead? I wonder what is happening to the commit message: can we be certain at this point that it was not set yet? And also: should we call `update_abort_safety_file()`?

Ciao, Dscho

Show 8 quoted lines

> +		}
>  		res |= write_message(msgbuf.buf, msgbuf.len,
>  				     git_path_merge_msg(), 0);
>  	} else {
> -- 
> 2.17.1.1185.g55be947832-goog
> 
>