Re: [PATCH] reduce_heads: fix memory leaks

["Martin Ågren" <martin.agren@gmail.com>]

On 2 November 2017 at 04:11, Junio C Hamano <gitster@pobox.com> wrote:
> Martin Ågren <martin.agren@gmail.com> writes:
>
>> diff --git a/builtin/merge-base.c b/builtin/merge-base.c
>> index 6dbd167d3..b1b7590c4 100644
>> --- a/builtin/merge-base.c
>> +++ b/builtin/merge-base.c
>> @@ -59,6 +59,8 @@ static int handle_independent(int count, const char **args)
>>               commit_list_insert(get_commit_reference(args[i]), &revs);
>>
>>       result = reduce_heads(revs);
>> +     free_commit_list(revs);
>> +
>>       if (!result)
>>               return 1;
>
> The post-context of this hunk continues like so:
>
>         while (result) {
>                 printf("%s\n", oid_to_hex(&result->item->object.oid));
>                 result = result->next;
>         }
>         return 0;
> }
>
> and we end up leaking "result".  This function is directly called from
> cmd_merge_base() and its value is returned to main(), so leaking it
> is not that a grave offence, but that excuse applies equally well to
> revs.

Good catch. I even have a patch to address the leak of `result`, except
I seem to have sorted it into another pile. For this series I just
grepped for "reduce_heads" and didn't stop to think about using UNLEAK,
nor about the leaking of `result`.

> I can see you are shooting for minimum change in this patch, but if
> we were writing this code in a codebase where reduce_heads_replace()
> is already available, I would imagine that we wouldn't use two separate
> variables, perhaps?

The way my other patch addresses the leaking of `result` is that it
rewrites the loop to avoid losing the original value of `result`, so
that it can be UNLEAK-ed at the very end. (That makes it obvious where
the leak happens, compared to adding an UNLEAK a few lines up.) If I do
`reduce_heads_replace(&revs)`, I'll need to touch the loop anyway, and
then I could probably just as well UNLEAK a little while at it.

I'll get to this within the next couple of days, then I'll see what it
looks like.

Thanks for your feedback.