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

From: Jeff King <peff@peff.net>
Date: Nov 6, 2017, 11:05 UTC
Message-ID: <20171106110549.dkv725bjikcms3aj@sigill.intra.peff.net>
In-Reply-To: <b06f593d3f8b0ad53754eeb394f77e7c3ee916bd.1509908607.git.martin.agren@gmail.com>

On Sun, Nov 05, 2017 at 09:26:30PM +0100, Martin Ågren wrote:

Show 14 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.
> 
> After this change, it is clearer that the leaks happen as we return, and
> not as we process the list. That is, we could just as well have used
> `free_commit_list()`. Also, leaving a "result" unchanged as we display
> it feels (marginally) better.

I think it would be OK to show that we are consuming the list as we go, like:

  while ((commit = pop_commit(&result))
	...do the thing ...

but like you I think I prefer the read-only iteration followed by a separate deallocation/leak phase.

Like Junio, though, I kind of wonder if just calling free_commit_list() would be the most readable thing.

The "other" benefit of UNLEAK() is that it has zero runtime cost. I'm not sure if that matters much here, so I'd tend to choose the thing that is most readable / understandable. These aren't cmd_* functions, so it's not _immediately_ obvious that it is OK for them to leak. But it's also pretty easy to see that they are called as the final element of cmd_merge_base(). So I'm on the fence as far as this being a good use of UNLEAK() or not.

-Peff