A late re-roll of this set of revisions API and API user memory leak fixes. I think the much simpler code here in 4/6 should address the points Jeff King brought up in the v1 review. Other than that I renamed the variable in 3/6 s/cp/pending/g, per Jeff's suggestion (FWIW "cp" = "copy"). 1. https://lore.kernel.org/git/cover-0.6-00000000000-20220713T130511Z-avarab@xxxxxxxxx/ Ævar Arnfjörð Bjarmason (6): bisect.c: add missing "goto" for release_revisions() test-fast-rebase helper: use release_revisions() (again) log: make the intent of cmd_show()'s "rev.pending" juggling clearer log: fix common "rev.pending" memory leak in "git show" bisect.c: partially fix bisect_rev_setup() memory leak revisions API: don't leak memory on argv elements that need free()-ing bisect.c | 28 +++++++++++-------- builtin/log.c | 21 +++++++------- builtin/submodule--helper.c | 5 +++- remote.c | 5 +++- revision.c | 2 ++ revision.h | 3 +- t/helper/test-fast-rebase.c | 2 -- t/t0203-gettext-setlocale-sanity.sh | 1 + t/t1020-subdirectory.sh | 1 + t/t2020-checkout-detach.sh | 1 + t/t3307-notes-man.sh | 1 + t/t3920-crlf-messages.sh | 2 ++ t/t4069-remerge-diff.sh | 1 + t/t7007-show.sh | 1 + ...3-pre-commit-and-pre-merge-commit-hooks.sh | 1 + t/t9122-git-svn-author.sh | 1 - t/t9162-git-svn-dcommit-interactive.sh | 1 - 17 files changed, 49 insertions(+), 28 deletions(-) Range-diff against v1: 1: 6624eb315a5 = 1: 12a4a20c59f bisect.c: add missing "goto" for release_revisions() 2: 0da0acba8b0 = 2: bbd3a7e5ecc test-fast-rebase helper: use release_revisions() (again) 3: f7cc2f6a381 ! 3: 1629299f883 log: make the intent of cmd_show()'s "rev.pending" juggling clearer @@ builtin/log.c: static void show_setup_revisions_tweak(struct rev_info *rev, struct rev_info rev; - struct object_array_entry *objects; + struct object_array blank = OBJECT_ARRAY_INIT; -+ struct object_array cp = OBJECT_ARRAY_INIT; ++ struct object_array pending = OBJECT_ARRAY_INIT; + unsigned int i; struct setup_revision_opt opt; struct pathspec match_all; @@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix) - count = rev.pending.nr; - objects = rev.pending.objects; -+ memcpy(&cp, &rev.pending, sizeof(rev.pending)); ++ memcpy(&pending, &rev.pending, sizeof(rev.pending)); rev.diffopt.no_free = 1; - for (i = 0; i < count && !ret; i++) { - struct object *o = objects[i].item; - const char *name = objects[i].name; -+ for (i = 0; i < cp.nr && !ret; i++) { -+ struct object *o = cp.objects[i].item; -+ const char *name = cp.objects[i].name; ++ for (i = 0; i < pending.nr && !ret; i++) { ++ struct object *o = pending.objects[i].item; ++ const char *name = pending.objects[i].name; switch (o->type) { case OBJ_BLOB: ret = show_blob_object(&o->oid, &rev, name); @@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix) ret = error(_("could not read object %s"), oid_to_hex(oid)); - objects[i].item = o; -+ cp.objects[i].item = o; ++ pending.objects[i].item = o; i--; break; } 4: 2c5b41d2b24 ! 4: 54b632c1124 log: fix common "rev.pending" memory leak in "git show" @@ Commit message In the preceding commit this code was made to use a more extendable pattern, which we can now complete. Once we've clobbered our "rev.pending" and invoked "cmd_log_walk_no_free()" we need to - "object_array_clear()" our newly created "rev.pending" to avoid - leaking the memory related to the one member array we've created. + "object_array_clear()" our old "rev.pending". - But more importantly we need to set "rev.pending" back to the original - we squirreled away in the "cp" variable, so that we'll make use of the + So in addition to the now-populated &pending array, to avoid leaking + the memory related to the one member array we've created. The + &rev.pending was already being object_array_clear()'d by the release_revisions() added in f6bfea0ad01 (revisions API users: use - release_revisions() in builtin/log.c, 2022-04-13). In f6bfea0ad01 this - memory leak was noted as an outstanding TODO, but it's now been fixed. + release_revisions() in builtin/log.c, 2022-04-13), now we'll also + correctly free the previous data (f6bfea0ad01 noted this memory leak + as an outstanding TODO). + + This way of doing it is making assumptions about the internals of + "struct rev_info", in particular that the "pending" member is + stand-alone, and that no other state is referring to it. But we've + been making those assumptions ever since 5d7eeee2ac6 (git-show: grok + blobs, trees and tags, too, 2006-12-14). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## builtin/log.c ## @@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix) - memcpy(&rev.pending, &blank, sizeof(rev.pending)); + return cmd_log_deinit(cmd_log_walk(&rev), &rev); + + memcpy(&pending, &rev.pending, sizeof(rev.pending)); ++ memcpy(&rev.pending, &blank, sizeof(rev.pending)); + rev.diffopt.no_free = 1; + for (i = 0; i < pending.nr && !ret; i++) { + struct object *o = pending.objects[i].item; +@@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix) + rev.shown_one = 1; + break; + case OBJ_COMMIT: +- memcpy(&rev.pending, &blank, sizeof(rev.pending)); add_object_array(o, name, &rev.pending); ret = cmd_log_walk_no_free(&rev); -+ object_array_clear(&rev.pending); -+ memcpy(&rev.pending, &cp, sizeof(rev.pending)); break; - default: +@@ builtin/log.c: int cmd_show(int argc, const char **argv, const char *prefix) ret = error(_("unknown type: %d"), o->type); + } + } ++ object_array_clear(&pending); + + rev.diffopt.no_free = 0; + diff_free(&rev.diffopt); ## t/t0203-gettext-setlocale-sanity.sh ## @@ 5: 525e3427396 = 5: 2af2bce2d17 bisect.c: partially fix bisect_rev_setup() memory leak 6: 291e660a2bc = 6: 3c57e126554 revisions API: don't leak memory on argv elements that need free()-ing -- 2.37.1.1196.g8af3636bc64
