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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 6, 2017, 01:03 UTC
Message-ID: <xmqqa800kyks.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <b06f593d3f8b0ad53754eeb394f77e7c3ee916bd.1509908607.git.martin.agren@gmail.com>

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

Show 9 quoted lines

> 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.