[PATCH v5 00/17] submodule--helper: fix memory leaks

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

This series fixes all of the memory leaks in
builtin/submodule--helper.c that our test suite spots, except for
those where the leak is downstream of cmd_submodule__helper() and
caused by code that's not in builtin/submodule--helper.c (e.g. leaks
in the object or config APIs, and in submodule.c).

This re-roll is on top of the just-now re-rolled v2 of the base
topic[1]. The only changes are to rebase the changes here on top of
those changes.

Changes:

* The base topic renamed another "res" variable (one was left behind
  before).

* The "goto cleanup" pattern in update_submodule() is gone now, which
  makes 16/17 much easier.

For the CI run & pushed branch see [2].

1. https://lore.kernel.org/git/cover-v2-00.28-00000000000-20220802T154036Z-avarab@gmail.com/
2. https://github.com/avar/git/tree/avar/submodule--helper-memory-leaks-5

Ævar Arnfjörð Bjarmason (17):
  submodule--helper: fix a leak in "clone_submodule"
  submodule--helper: fix trivial get_default_remote_submodule() leak
  submodule--helper: fix most "struct pathspec" memory leaks
  submodule--helper: "struct pathspec" memory leak in module_update()
  submodule--helper: don't leak {run,capture}_command() cp.dir argument
  submodule--helper: add and use *_release() functions
  submodule--helper: fix "errmsg_str" memory leak
  submodule--helper: fix "sm_path" and other "module_cb_list" leaks
  submodule--helper: fix a leak with repo_clear()
  submodule--helper: fix a memory leak in get_default_remote_submodule()
  submodule--helper: fix "reference" leak
  submodule--helper: fix obscure leak in module_add()
  submodule--helper: fix a leak in module_add()
  submodule--helper: fix a memory leak in print_status()
  submodule--helper: free some "displaypath" in "struct update_data"
  submodule--helper: free rest of "displaypath" in "struct update_data"
  submodule--helper: fix a configure_added_submodule() leak

 builtin/submodule--helper.c        | 239 +++++++++++++++++++++--------
 t/t1500-rev-parse.sh               |   1 +
 t/t2403-worktree-move.sh           |   1 +
 t/t6008-rev-list-submodule.sh      |   1 +
 t/t6134-pathspec-in-submodule.sh   |   1 +
 t/t7412-submodule-absorbgitdirs.sh |   1 +
 t/t7413-submodule-is-active.sh     |   1 +
 t/t7414-submodule-mistakes.sh      |   2 +
 t/t7506-status-submodule.sh        |   1 +
 t/t7507-commit-verbose.sh          |   2 +
 10 files changed, 185 insertions(+), 65 deletions(-)

Range-diff against v4:
 1:  aac987a414a =  1:  118e965d401 submodule--helper: fix a leak in "clone_submodule"
 2:  390c5174e17 !  2:  d885e1dd59a submodule--helper: fix trivial get_default_remote_submodule() leak
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/submodule--helper.c ##
    -@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data,
    +@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)
      		const char *branch = remote_submodule_branch(update_data->sm_path);
      		char *remote_ref = xstrfmt("refs/remotes/%s/%s", remote_name, branch);
      
 3:  529e10233d5 =  3:  1f90348d61f submodule--helper: fix most "struct pathspec" memory leaks
 4:  683d327752f =  4:  f768ff2e34d submodule--helper: "struct pathspec" memory leak in module_update()
 5:  4e8e9197539 !  5:  509133c37fa submodule--helper: don't leak {run,capture}_command() cp.dir argument
    @@ builtin/submodule--helper.c: static int fetch_in_submodule(const char *module_pa
      
      	strvec_push(&cp.args, "fetch");
      	if (quiet)
    -@@ builtin/submodule--helper.c: static int run_update_command(struct update_data *ud, int subforce,
    +@@ builtin/submodule--helper.c: static int run_update_command(struct update_data *ud, int subforce)
      	}
      	strvec_push(&cp.args, oid);
      
    @@ builtin/submodule--helper.c: static int run_update_command(struct update_data *u
     +	cp.dir = ud->sm_path;
      	prepare_submodule_repo_env(&cp.env);
      	if (run_command(&cp)) {
    - 		switch (ud->update_strategy.type) {
    + 		int ret;
 6:  575d3e8d2e2 !  6:  25377f1d06c submodule--helper: add and use *_release() functions
    @@ builtin/submodule--helper.c: static int update_submodules(struct update_data *up
      cleanup:
     +	submodule_update_clone_release(&suc);
      	string_list_clear(&update_data->references, 0);
    - 	return res;
    + 	return ret;
      }
     @@ builtin/submodule--helper.c: static int module_update(int argc, const char **argv, const char *prefix)
      
 7:  d4c81e04254 =  7:  1a85057cd0c submodule--helper: fix "errmsg_str" memory leak
 8:  ef9e29d5bfe =  8:  3c4f734e958 submodule--helper: fix "sm_path" and other "module_cb_list" leaks
 9:  0798a00c9ef !  9:  3aebff9f8e3 submodule--helper: fix a leak with repo_clear()
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/submodule--helper.c ##
    -@@ builtin/submodule--helper.c: static void ensure_core_worktree(const char *path)
    - 		free(abs_path);
    +@@ builtin/submodule--helper.c: static int ensure_core_worktree(const char *path)
      		strbuf_release(&sb);
      	}
    + 
     +	repo_clear(&subrepo);
    + 	return 0;
      }
      
    - static const char *submodule_update_type_to_label(enum submodule_update_type type)
10:  dae2a6f8e07 = 10:  38345ec76bc submodule--helper: fix a memory leak in get_default_remote_submodule()
11:  e7352bb8cfa = 11:  4b1e5ced969 submodule--helper: fix "reference" leak
12:  1adb7b66656 = 12:  e74c396073b submodule--helper: fix obscure leak in module_add()
13:  b27b665d287 = 13:  71a56c59864 submodule--helper: fix a leak in module_add()
14:  53ba1705eb6 = 14:  2429db2f1c7 submodule--helper: fix a memory leak in print_status()
15:  230e5f8ad14 = 15:  613d077c4ec submodule--helper: free some "displaypath" in "struct update_data"
16:  c0fba2f1c56 ! 16:  8f150a81507 submodule--helper: free rest of "displaypath" in "struct update_data"
    @@ Commit message
         update_submodule() function itself can use it, and for the
         run_update_procedure() called within this function.
     
    +    To make managing that clobbering easier let's wrap the
    +    update_submodule() in a new update_submodule_outer() function, which
    +    will do the clobbering and free(to_free) dance for us.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/submodule--helper.c ##
    -@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data,
    - 			    int *must_die_on_failure)
    +@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)
      {
    - 	int ret = 1;
    -+	char *to_free, *restore = update_data->displaypath;
    - 
    - 	ensure_core_worktree(update_data->sm_path);
    + 	int ret;
      
    +-	ret = ensure_core_worktree(update_data->sm_path);
    +-	if (ret)
    +-		return ret;
    +-
     -	update_data->displaypath = get_submodule_displaypath(
    -+	update_data->displaypath = to_free = get_submodule_displaypath(
    - 		update_data->sm_path, update_data->prefix);
    - 
    - 	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
    -@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data,
    +-		update_data->sm_path, update_data->prefix);
    +-
    + 	ret = determine_submodule_update_strategy(the_repository,
    + 						  update_data->just_cloned,
    + 						  update_data->sm_path,
    +@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)
    + 	return 0;
    + }
      
    - 	ret = 0;
    - cleanup:
    ++static int update_submodule_outer(struct update_data *update_data)
    ++{
    ++	char *to_free, *restore = update_data->displaypath;
    ++	int ret;
    ++
    ++	ensure_core_worktree(update_data->sm_path);
    ++
    ++	update_data->displaypath = to_free = get_submodule_displaypath(
    ++		update_data->sm_path, update_data->prefix);
    ++
    ++	ret = update_submodule(update_data);
    ++
     +	free(to_free);
     +	update_data->displaypath = restore;
     +
    - 	return ret;
    - }
    ++	return ret;
    ++}
    ++
    + static int update_submodules(struct update_data *update_data)
    + {
    + 	int i, ret = 0;
    +@@ builtin/submodule--helper.c: static int update_submodules(struct update_data *update_data)
    + 		update_data->just_cloned = ucd.just_cloned;
    + 		update_data->sm_path = ucd.sub->path;
      
    +-		code = update_submodule(update_data);
    ++		code = update_submodule_outer(update_data);
    + 		if (code)
    + 			ret = code;
    + 		if (code == 128)
17:  95f8b68bd41 ! 17:  17c77ceba01 submodule--helper: fix a configure_added_submodule() leak
    @@ builtin/submodule--helper.c: static void configure_added_submodule(struct add_da
      		 * current configured pathspec, set the submodule's active flag
     
      ## t/t7413-submodule-is-active.sh ##
    -@@ t/t7413-submodule-is-active.sh: This test verifies that `test-tool submodule is-active` correctly identifies
    - submodules which are "active" and interesting to the user.
    +@@ t/t7413-submodule-is-active.sh: This is a unit test of the submodule.c is_submodule_active() function,
    + which is also indirectly tested elsewhere.
      '
      
     +TEST_PASSES_SANITIZE_LEAK=true
-- 
2.37.1.1233.ge8b09efaedc