Re: [PATCH v3 1/2] builtin/merge-base: free commit lists

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 8, 2017, 02:40 UTC
Message-ID: <xmqqzi7xlcha.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <a5f5a259f4cbe3661eb1960e83e9bcce5080b580.1510083859.git.martin.agren@gmail.com>

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

Show 6 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.
>
> Rewrite the loops so that we keep the original pointers, then call
> `free_commit_list()`. Various alternatives were considered:

So, the reader expects to see a list of alternatives that we considered but did not use in this solution to follow.

>
> 1) Use `UNLEAK(result)` before the loop. Simple change, but not very
> pretty. These would definitely be new lows among our usages of UNLEAK.

I am not sure if I agree with the judgment, but we did reject it, so describing it as a candidate is good.

> 2) Use `pop_commit()` when looping. Slightly less simple change, but it
> feels slightly preferable to first display the list, then free it.

OK.

> 3) As in this patch, but with `UNLEAK()` instead of freeing. We'd still
> go through all the trouble of refactoring the loop, and because it's not
> super-obvious that we're about to exit, let's just free the lists -- it
> probably doesn't affect the runtime much.

That does include what we did, too. A rejected alternative is only the first 3/4 of what this says.

    3) As in this patch, but with `UNLEAK()` instead of freeing. We'd
       still go through all the trouble of refactoring the loop, and the
       use of UNLEAK() is left questionable because it's not very obvious
       that we're about to exit.

and then you can begin a separate paragraph, after the above lists, e.g.

    Let's just free the lists -- it probably doesn't affect the
    runtime much.

> In `handle_independent()` we can drop `result` while we're here and
> reuse the `revs`-variable instead. That matches several other users of
> `reduce_heads()`. The memory-leak that this hides will be addressed in
> the next commit.

The patch text looks agreeable.

Thanks.