[PATCH v3 1/2] builtin/merge-base: free commit lists
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- Nov 7, 2017, 20:39 UTC
- Message-ID
- <a5f5a259f4cbe3661eb1960e83e9bcce5080b580.1510083859.git.martin.agren@gmail.com>
- In-Reply-To
- <20171106110549.dkv725bjikcms3aj@sigill.intra.peff.net>
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:
1) Use `UNLEAK(result)` before the loop. Simple change, but not very pretty. These would definitely be new lows among our usages of UNLEAK. 2) Use `pop_commit()` when looping. Slightly less simple change, but it feels slightly preferable to first display the list, then free it. 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.
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.
Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- v3: Like v2 but s/UNLEAK/free_commit_list/ and rebased onto maint.
> Like Junio, though, I kind of wonder if just calling free_commit_list() > would be the most readable thing.
Thanks Junio and Peff for insightful considerations around UNLEAK vs free. I'd rather miss one or two opportunities too UNLEAK than use it too often. I've got a couple of patches in the pipeline that will be better thanks to your comments. Thanks.
builtin/merge-base.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/builtin/merge-base.c b/builtin/merge-base.c index 6dbd167d3b..e17835fabb 100644 --- a/builtin/merge-base.c +++ b/builtin/merge-base.c @@ -9,20 +9,20 @@ static int show_merge_base(struct commit **rev, int rev_nr, int show_all) { - struct commit_list *result; + struct commit_list *result, *r; result = get_merge_bases_many_dirty(rev[0], rev_nr - 1, rev + 1); if (!result) return 1; - while (result) { - printf("%s\n", oid_to_hex(&result->item->object.oid)); + for (r = result; r; r = r->next) { + printf("%s\n", oid_to_hex(&r->item->object.oid)); if (!show_all) - return 0; - result = result->next; + break; } + free_commit_list(result); return 0; } @@ -51,28 +51,28 @@ static struct commit *get_commit_reference(const char *arg) static int handle_independent(int count, const char **args) { - struct commit_list *revs = NULL; - struct commit_list *result; + struct commit_list *revs = NULL, *rev; int i; for (i = count - 1; i >= 0; i--) commit_list_insert(get_commit_reference(args[i]), &revs); - result = reduce_heads(revs); - if (!result) + revs = reduce_heads(revs); + + if (!revs) return 1; - while (result) { - printf("%s\n", oid_to_hex(&result->item->object.oid)); - result = result->next; - } + for (rev = revs; rev; rev = rev->next) + printf("%s\n", oid_to_hex(&rev->item->object.oid)); + + free_commit_list(revs); return 0; } static int handle_octopus(int count, const char **args, int show_all) { struct commit_list *revs = NULL; - struct commit_list *result; + struct commit_list *result, *rev; int i; for (i = count - 1; i >= 0; i--) @@ -83,13 +83,13 @@ static int handle_octopus(int count, const char **args, int show_all) if (!result) return 1; - while (result) { - printf("%s\n", oid_to_hex(&result->item->object.oid)); + for (rev = result; rev; rev = rev->next) { + printf("%s\n", oid_to_hex(&rev->item->object.oid)); if (!show_all) - return 0; - result = result->next; + break; } + free_commit_list(result); return 0; }
-- 2.15.0.415.gac1375d7e