Re: [PATCH v2 1/2] builtin/merge-base: UNLEAK commit lists

[Junio C Hamano <gitster@pobox.com>]

Martin Ågren <martin.agren@gmail.com> writes:

> In several functions, we iterate through a commit list by assigning
> `result = result->next`. As a consequence, we lose the original pointer
> and eventually leak the list.
>
> These are immediate helpers to `cmd_merge_base()` which is just about to
> return, so we can use UNLEAK. For example, we could `UNLEAK(result)`
> before we start iterating. That would be a one-liner change per
> function. Instead, leave the lists alone and iterate using a dedicated
> pointer. Then UNLEAK immediately before returning.

Hmm, I cannot shake this feeling that this goes somewhat opposite to
the spirit of UNLEAK(), which I view as "It is too cumbersome and
makes the resulting code ugly if we try to make everything properly
freed, so mark what we know we will leak upfront".  The result of
this patch feels more like "Even though we took pains to restructure
the code so that we could call free_commit_list() to properly free
things, we use UNLEAK() and do not actually bother to free."

Havin said that, I do not think the resulting code has become uglier
or the conversion process (both writing and reviewing) were too
cumbersome for this particular case, and marking where we could call
free_commit_list() with UNLEAK() like this patch does may make
sense.  If somebody someday wants to call some of these helpers in
other contexts repeatedly, they may have an easier time.