[v3,00/32] submodule--helper: add tests, rm dead code, refactor & leak prep

Message

Ævar Arnfjörð Bjarmason Aug. 21, 2022, 1:57 p.m. UTC

This series is a "prep" series for the meaty leak fixes for
submodule--helper, see [1] (which will be re-rolled on top of this
v3). The v6 re-roll will be at [2].

Changes since v2:

 * Clarified 1/* commit message.
 * 22/*: keep the "to string" API function in submodule.[ch], as
   suggested by Glen.
 * 24/*: Minor change to existing comment obsoleted by the code change.
 * 26-31/*: Do the full libification suggested by Glen, instead of
   leaving it for later. As a result we don't need to pass up the
   "must_die_on_failure", and some parts of the soon-to-be-rerolled
   "leak" series[1] will be better as a result.

1. https://lore.kernel.org/git/[e-mail]/
2. https://lore.kernel.org/git/[e-mail]

Glen Choo (2):
  submodule--helper: add "const" to copy of "update_data"
  submodule--helper: refactor "errmsg_str" to be a "struct strbuf"

Ævar Arnfjörð Bjarmason (30):
  submodule tests: test usage behavior
  submodule tests: test for "add <repository> <abs-path>"
  submodule--helper: remove unused "name" helper
  submodule--helper: remove unused "list" helper
  test-tool submodule-config: remove unused "--url" handling
  submodule--helper: move "is-active" to a test-tool
  submodule--helper: move "check-name" to a test-tool
  submodule--helper: move "resolve-relative-url-test" to a test-tool
  submodule--helper style: don't separate declared variables with \n\n
  submodule--helper style: add \n\n after variable declarations
  submodule--helper: replace memset() with { 0 }-initialization
  submodule--helper: use xstrfmt() in clone_submodule()
  submodule--helper: move "sb" in clone_submodule() to its own scope
  submodule--helper: add "const" to passed "module_clone_data"
  submodule--helper: don't redundantly check "else if (res)"
  submodule--helper: rename "int res" to "int ret"
  submodule--helper: return "ret", not "1" from update_submodule()
  submodule--helper: add missing braces to "else" arm
  submodule--helper: don't call submodule_strategy_to_string() in BUG()
  submodule API: don't handle SM_..{UNSPECIFIED,COMMAND} in to_string()
  submodule--helper: use "code" in run_update_command()
  submodule--helper: don't exit() on failure, return
  submodule--helper: libify determine_submodule_update_strategy()
  submodule--helper: libify "must_die_on_failure" code paths
  submodule--helper update: don't override 'checkout' exit code
  submodule--helper: libify "must_die_on_failure" code paths (for die)
  submodule--helper: check repo{_submodule,}_init() return values
  submodule--helper: libify more "die" paths for module_update()
  submodule--helper: libify even more "die" paths for module_update()
  submodule--helper: fix bad config API usage

 Makefile                         |   1 +
 builtin/submodule--helper.c      | 534 ++++++++++++++-----------------
 git-compat-util.h                |   3 +
 repository.h                     |   3 +
 submodule.c                      |  12 +-
 submodule.h                      |   2 +-
 t/helper/test-submodule-config.c |  11 +-
 t/helper/test-submodule.c        | 146 +++++++++
 t/helper/test-tool-utils.h       |   9 +
 t/helper/test-tool.c             |   7 +-
 t/helper/test-tool.h             |   1 +
 t/t0060-path-utils.sh            |   2 +-
 t/t7400-submodule-basic.sh       |  56 ++--
 t/t7406-submodule-update.sh      |   2 +-
 t/t7413-submodule-is-active.sh   |  35 +-
 t/t7450-bad-git-dotfiles.sh      |   2 +-
 16 files changed, 460 insertions(+), 366 deletions(-)
 create mode 100644 t/helper/test-submodule.c
 create mode 100644 t/helper/test-tool-utils.h

Range-diff against v2:
 1:  daa5d3f9962 !  1:  77586985ab3 submodule tests: test usage behavior
    @@ Commit message
         subsequent eventual behavior change will become clear.
     
         For "--" this follows up on 68cabbfda36 (submodule: document default
    -    behavior, 2019-02-15) and tests that "status" doesn't don't support
    +    behavior, 2019-02-15) and tests that "status" doesn't support
         the "--" delimiter. There's no intrinsically good reason not to
         support that. We behave this way due to edge cases in
         git-submodule.sh's implementation, but as with "-h" let's assert our
    @@ Commit message
         doesn't seem to be a reason not to support it alongside "--quiet" and
         "--cached", but let's likewise assert our existing behavior for now.
     
    +    I.e. as long as "status" is optional it would make sense to support
    +    all of its options when it's omitted, but we only do that with
    +    "--quiet" and "--cached", and curiously omit "--recursive".
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## t/t7400-submodule-basic.sh ##
 2:  9d920326df3 =  2:  7425f0025da submodule tests: test for "add <repository> <abs-path>"
 3:  d981db49fa1 =  3:  1be48c0b22f submodule--helper: remove unused "name" helper
 4:  6f78f9c9274 =  4:  10189ba3da7 submodule--helper: remove unused "list" helper
 5:  43902201701 =  5:  ef66dfcd45f test-tool submodule-config: remove unused "--url" handling
 6:  e2a8bb0a28e =  6:  4727fbb4b64 submodule--helper: move "is-active" to a test-tool
 7:  b209532eb17 =  7:  9c644460b1d submodule--helper: move "check-name" to a test-tool
 8:  de49f31dab0 =  8:  03c8383b8e7 submodule--helper: move "resolve-relative-url-test" to a test-tool
 9:  b0238f699ce =  9:  b1eaa6a796b submodule--helper style: don't separate declared variables with \n\n
10:  5f5e68a868b = 10:  fd7fbe08536 submodule--helper style: add \n\n after variable declarations
11:  72dcf19e1c4 = 11:  356f07db436 submodule--helper: replace memset() with { 0 }-initialization
12:  e5e267dccd5 = 12:  241ac5c7eee submodule--helper: use xstrfmt() in clone_submodule()
13:  91558745e2e = 13:  f2f412f50c1 submodule--helper: move "sb" in clone_submodule() to its own scope
14:  866b8397a59 ! 14:  ad7848067a9 submodule--helper: pass a "const struct module_clone_data" to clone_submodule()
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## Commit message ##
    -    submodule--helper: pass a "const struct module_clone_data" to clone_submodule()
    +    submodule--helper: add "const" to passed "module_clone_data"
     
         Add "const" to the "struct module_clone_data" that we pass to
         clone_submodule(), which makes the ownership clear, and stops us from
15:  1ff380ec7b5 = 15:  ab283479b92 submodule--helper: add "const" to copy of "update_data"
16:  d3a7e646adc = 16:  ab0fd2c60f0 submodule--helper: refactor "errmsg_str" to be a "struct strbuf"
17:  23eb07176d9 = 17:  fa2417c7a17 submodule--helper: don't redundantly check "else if (res)"
18:  78f74df6d5e = 18:  be1ffbf2e26 submodule--helper: rename "int res" to "int ret"
19:  f0258e37ebe = 19:  92e17c37839 submodule--helper: return "ret", not "1" from update_submodule()
20:  70f030cca4e = 20:  55e3ea5f9dd submodule--helper: add missing braces to "else" arm
21:  bce1a014a2f = 21:  2bb45302392 submodule--helper: don't call submodule_strategy_to_string() in BUG()
22:  98c3e562c82 ! 22:  0131c197427 submodule--helper: move submodule_strategy_to_string() to only user
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## Commit message ##
    -    submodule--helper: move submodule_strategy_to_string() to only user
    +    submodule API: don't handle SM_..{UNSPECIFIED,COMMAND} in to_string()
     
    -    Move the submodule_strategy_to_string() function added in
    -    3604242f080 (submodule: port init from shell to C, 2016-04-15) to its
    -    only user.
    +    Change the submodule_strategy_to_string() function added in
    +    3604242f080 (submodule: port init from shell to C, 2016-04-15) to
    +    really return a "const char *". In the "SM_UPDATE_COMMAND" case it
    +    would return a strbuf_detach().
     
    -    This function would return NULL on SM_UPDATE_UNSPECIFIED, so it wasn't
    -    safe to xstrdup() its return value in the general case, or to use it
    -    in a sprintf() format as the code removed in the preceding commit did.
    +    Furthermore, this function would return NULL on SM_UPDATE_UNSPECIFIED,
    +    so it wasn't safe to xstrdup() its return value in the general case,
    +    or to use it in a sprintf() format as the code removed in the
    +    preceding commit did.
     
         But its callers would never call it with either SM_UPDATE_UNSPECIFIED
    -    or SM_UPDATE_COMMAND. Let's move it to a "static" helper, and have its
    -    functionality reflect how it's used, and BUG() out on the rest.
    +    or SM_UPDATE_COMMAND. Let's have its behavior reflect how its only
    +    user expects it to behave, and BUG() out on the rest.
     
         By doing this we can also stop needlessly xstrdup()-ing and free()-ing
         the memory for the config we're setting. We can instead always use
         constant strings. We can also use the *_tmp() variant of
         git_config_get_string().
     
    +    Let's also rename this submodule_strategy_to_string() function to
    +    submodule_update_type_to_string(). Now that it's only tasked with
    +    returning a string version of the "enum submodule_update_type type".
    +    Before it would look at the "command" field in "struct
    +    submodule_update_strategy".
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## builtin/submodule--helper.c ##
    -@@ builtin/submodule--helper.c: struct init_cb {
    - };
    - #define INIT_CB_INIT { 0 }
    - 
    -+static const char *submodule_strategy_to_string(enum submodule_update_type type)
    -+{
    -+	switch (type) {
    -+	case SM_UPDATE_CHECKOUT:
    -+		return "checkout";
    -+	case SM_UPDATE_MERGE:
    -+		return "merge";
    -+	case SM_UPDATE_REBASE:
    -+		return "rebase";
    -+	case SM_UPDATE_NONE:
    -+		return "none";
    -+	case SM_UPDATE_UNSPECIFIED:
    -+	case SM_UPDATE_COMMAND:
    -+		BUG("init_submodule() should handle type %d", type);
    -+	default:
    -+		BUG("unexpected update strategy type: %d", type);
    -+	}
    -+}
    -+
    - static void init_submodule(const char *path, const char *prefix,
    - 			   unsigned int flags)
    +@@ builtin/submodule--helper.c: static void init_submodule(const char *path, const char *prefix,
      {
      	const struct submodule *sub;
      	struct strbuf sb = STRBUF_INIT;
    @@ builtin/submodule--helper.c: static void init_submodule(const char *path, const
     +			upd = "none";
      		} else {
     -			upd = xstrdup(submodule_strategy_to_string(&sub->update_strategy));
    -+			upd = submodule_strategy_to_string(sub->update_strategy.type);
    ++			upd = submodule_update_type_to_string(sub->update_strategy.type);
      		}
      
      		if (git_config_set_gently(sb.buf, upd))
    @@ submodule.c: int parse_submodule_update_strategy(const char *value,
      }
      
     -const char *submodule_strategy_to_string(const struct submodule_update_strategy *s)
    --{
    ++const char *submodule_update_type_to_string(enum submodule_update_type type)
    + {
     -	struct strbuf sb = STRBUF_INIT;
     -	switch (s->type) {
    --	case SM_UPDATE_CHECKOUT:
    --		return "checkout";
    --	case SM_UPDATE_MERGE:
    --		return "merge";
    --	case SM_UPDATE_REBASE:
    --		return "rebase";
    --	case SM_UPDATE_NONE:
    --		return "none";
    --	case SM_UPDATE_UNSPECIFIED:
    ++	switch (type) {
    + 	case SM_UPDATE_CHECKOUT:
    + 		return "checkout";
    + 	case SM_UPDATE_MERGE:
    +@@ submodule.c: const char *submodule_strategy_to_string(const struct submodule_update_strategy
    + 	case SM_UPDATE_NONE:
    + 		return "none";
    + 	case SM_UPDATE_UNSPECIFIED:
     -		return NULL;
    --	case SM_UPDATE_COMMAND:
    + 	case SM_UPDATE_COMMAND:
     -		strbuf_addf(&sb, "!%s", s->command);
     -		return strbuf_detach(&sb, NULL);
    --	}
    ++		BUG("init_submodule() should handle type %d", type);
    ++	default:
    ++		BUG("unexpected update strategy type: %d", type);
    + 	}
     -	return NULL;
    --}
    --
    + }
    + 
      void handle_ignore_submodules_arg(struct diff_options *diffopt,
    - 				  const char *arg)
    - {
     
      ## submodule.h ##
     @@ submodule.h: void die_path_inside_submodule(struct index_state *istate,
    @@ submodule.h: void die_path_inside_submodule(struct index_state *istate,
      int parse_submodule_update_strategy(const char *value,
      				    struct submodule_update_strategy *dst);
     -const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
    ++const char *submodule_update_type_to_string(enum submodule_update_type type);
      void handle_ignore_submodules_arg(struct diff_options *, const char *);
      void show_submodule_diff_summary(struct diff_options *o, const char *path,
      			    struct object_id *one, struct object_id *two,
23:  db2d2fb5a21 = 23:  6cac6cb2fa6 submodule--helper: use "code" in run_update_command()
24:  d33260487bd ! 24:  6d56f671c7a submodule--helper: don't exit() on failure, return
    @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *upd
      	if (update_data->recursive) {
      		struct child_process cp = CHILD_PROCESS_INIT;
     @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)
    + 		prepare_submodule_repo_env(&cp.env);
    + 		update_data_to_args(&next, &cp.args);
    + 
    +-		/* die() if child process die()'d */
    + 		ret = run_command(&cp);
    + 		if (!ret)
    + 			return 0;
      		die_message(_("Failed to recurse into submodule path '%s'"),
      			    update_data->displaypath);
      		if (ret == 128)
25:  9981a75d7e8 = 25:  dfd5c8bcd61 submodule--helper: libify determine_submodule_update_strategy()
26:  b48705c6cc0 ! 26:  da1a07afd25 submodule--helper: libify "must_die_on_failure" code paths
    @@ Commit message
         after having gone through the various codepaths that we were only
         returning 128 if we meant to early abort.
     
    +    In update_submodules() we'll for now set any non-zero non-128 exit
    +    codes to "1", but will start ferrying up the exit code as-is in a
    +    subsequent commit.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## builtin/submodule--helper.c ##
    @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data
      {
      	int ret;
      
    +@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data,
    + 						  update_data->sm_path,
    + 						  update_data->update_default,
    + 						  &update_data->update_strategy);
    +-	if (ret) {
    +-		*must_die_on_failure = 1;
    ++	if (ret)
    + 		return ret;
    +-	}
    + 
    + 	if (update_data->just_cloned)
    + 		oidcpy(&update_data->suboid, null_oid());
     @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data,
      	}
      
    @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *upd
      
      	if (update_data->recursive) {
     @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data,
    + 		update_data_to_args(&next, &cp.args);
      
    - 		/* die() if child process die()'d */
      		ret = run_command(&cp);
     -		if (!ret)
     -			return 0;
    @@ builtin/submodule--helper.c: static int update_submodules(struct update_data *up
      		update_data->sm_path = ucd.sub->path;
      
     -		code = update_submodule(update_data, &must_die_on_failure);
    -+		code = update_submodule(update_data);
    - 		if (code)
    - 			ret = code;
    +-		if (code)
    +-			ret = code;
     -		if (must_die_on_failure)
    -+		if (code == 128)
    ++		code = update_submodule(update_data);
    ++		if (!code)
    ++			continue;
    ++		ret = code;
    ++		if (ret == 128)
      			goto cleanup;
    - 		else if (code)
    - 			ret = 1;
    +-		else if (code)
    +-			ret = 1;
    ++		ret = 1;
    + 	}
    + 
    + cleanup:
 -:  ----------- > 27:  2795a3738c8 submodule--helper update: don't override 'checkout' exit code
27:  93cd1ccde54 ! 28:  6d9bccb34c3 submodule--helper: libify "must_die_on_failure" code paths (for die)
    @@ Commit message
                 const char *branch = remote_submodule_branch(update_data->sm_path);
     
         But as that code is used by other callers than the "update" code let's
    -    leave converting it for now.
    +    leave converting it for a subsequent commit.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
    @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *upd
      	update_data->displaypath = get_submodule_displaypath(
      		update_data->sm_path, update_data->prefix);
     @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)
    - 						  update_data->sm_path,
    - 						  update_data->update_default,
    - 						  &update_data->update_strategy);
    --	if (ret) {
    --		*must_die_on_failure = 1;
    -+	if (ret)
    - 		return ret;
    --	}
    - 
      	if (update_data->just_cloned)
      		oidcpy(&update_data->suboid, null_oid());
      	else if (resolve_gitlink_ref(update_data->sm_path, "HEAD", &update_data->suboid))
 -:  ----------- > 29:  d4b55f07a30 submodule--helper: check repo{_submodule,}_init() return values
 -:  ----------- > 30:  15c2490a978 submodule--helper: libify more "die" paths for module_update()
 -:  ----------- > 31:  1694ccfe882 submodule--helper: libify even more "die" paths for module_update()
28:  6160a1ab250 = 32:  d133402462f submodule--helper: fix bad config API usage