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/cover-v5-00.17-00000000000-20220802T155002Z-avarab@xxxxxxxxx/ 2. https://lore.kernel.org/git/cover-v6-00.17-00000000000-20220821T130415Z-avarab@xxxxxxxxx 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 <avarab@xxxxxxxxx> ## 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 <avarab@xxxxxxxxx> ## 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 <avarab@xxxxxxxxx> ## 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 <avarab@xxxxxxxxx> ## 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 <avarab@xxxxxxxxx> ## 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 <avarab@xxxxxxxxx> @@ 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 -- 2.37.2.1279.g64dec4e13cf
