Re: [PATCH 2/3] sequencer.c: free author variable when merging fails
On Thu, May 31, 2018 at 5:04 AM, Johannes Schindelin
wrote:
> Hi Stefan,
>
> On Wed, 30 May 2018, Stefan Beller wrote:
>
>> Signed-off-by: Stefan Beller
>> ---
>>
>> 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 ;-)
ok, I am not just bad at writing commit messages, but
also bad at reading other peoples commit messages. ;)
"As this patch is already complex enough, we
leave that change for a later patch." is what lead me to
believe it was deliberate.
>> - 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()`?
I think so, but wasn't sure. I wrote these patches before
my usual morning routine. I'll change that.
Thanks,
Stefan
Re: [PATCH 2/3] sequencer.c: free author variable when merging fails
Hi Stefan,
On Wed, 30 May 2018, Stefan Beller wrote:
> Signed-off-by: Stefan Beller
> ---
>
> 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 ;-)
> 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,
>, , 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
> + }
> res |= write_message(msgbuf.buf, msgbuf.len,
>git_path_merge_msg(), 0);
> } else {
> --
> 2.17.1.1185.g55be947832-goog
>
>
