On 2 November 2017 at 04:11, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Martin Ågren <martin.agren@xxxxxxxxx> 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.
