[PATCH v2 0/6] revisions API: fix more memory leaks

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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@gmail.com/

Æ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@gmail.com>
     
      ## 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