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 v2). Change since v1: * This hopefully addresses all of Glen's comments on v1. * The "libification" of "update" suggested in [2] is (almost) complete here. See 27/28 for the "almost". This is why the series grew from 20 to 28 patches. There's a lot of incremental "showing work" here. * The "goto cleanup" in update_submodule() change is gone, it's needed for the current v4 of the follow-up, but I'll submit a re-roll which solves that problem in a cleaner way. For this series it means we can end up with just a "return die_message(...)" rather than "ret = die_message(...); goto cleanup" in that code, which is a lot less verbose. * Various other small changes, see the range-diff below. A CI run for this & branch is at [3]. 1. https://lore.kernel.org/git/cover-v4-00.17-00000000000-20220728T162442Z-avarab@xxxxxxxxx/ 2. https://lore.kernel.org/git/kl6lilnfr22i.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ 3. https://github.com/avar/git/tree/avar/submodule--helper-cleanup-and-tests-2 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 (26): 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: pass a "const struct module_clone_data" to clone_submodule() 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--helper: move submodule_strategy_to_string() to only user 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: libify "must_die_on_failure" code paths (for die) submodule--helper: fix bad config API usage Makefile | 1 + builtin/submodule--helper.c | 451 +++++++++++++------------------ submodule.c | 21 -- submodule.h | 1 - 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 +- 14 files changed, 403 insertions(+), 342 deletions(-) create mode 100644 t/helper/test-submodule.c create mode 100644 t/helper/test-tool-utils.h Range-diff against v1: 1: 84b05bda016 ! 1: daa5d3f9962 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 when "status" isn't supplied we - don't support the "--" delimiter. There's no intrinsically good reason - not to support that. We behave this way due to edge cases in + behavior, 2019-02-15) and tests that "status" doesn't don'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 current long-standing behavior for now. 2: 9671169e348 = 2: 9d920326df3 submodule tests: test for "add <repository> <abs-path>" 3: 7aa9c14251b = 3: d981db49fa1 submodule--helper: remove unused "name" helper 4: 27df2efe718 = 4: 6f78f9c9274 submodule--helper: remove unused "list" helper 5: 9fe8deb165a = 5: 43902201701 test-tool submodule-config: remove unused "--url" handling 6: 8e4d2b09d56 ! 6: e2a8bb0a28e submodule--helper: move "is-active" to a test-tool @@ t/t7413-submodule-is-active.sh -This test verifies that `git submodue--helper is-active` correctly identifies +This test verifies that `test-tool submodule is-active` correctly identifies submodules which are "active" and interesting to the user. ++ ++This is a unit test of the submodule.c is_submodule_active() function, ++which is also indirectly tested elsewhere. ' + . ./test-lib.sh @@ t/t7413-submodule-is-active.sh: test_expect_success 'setup' ' ' 7: a2f3d812815 ! 7: b209532eb17 submodule--helper: move "check-name" to a test-tool @@ Commit message a6226fd772b (submodule--helper: convert the bulk of cmd_add() to C, 2021-08-10) it has only been used by this test, not git-submodule.sh. + As noted with its introduction in 0383bbb9015 (submodule-config: + verify submodule names as paths, 2018-04-30) the intent of + t7450-bad-git-dotfiles.sh has always been to unit test the + check_submodule_name() function. + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## builtin/submodule--helper.c ## 8: 8188657cdfa ! 8: de49f31dab0 submodule--helper: move "resolve-relative-url-test" to a test-tool @@ Commit message resolve_relative_url from shell to C, 2016-04-15) for its original addition. + Perhaps it would make sense to drop this code entirely, as we feel + that we've got enough indirect test coverage, but let's leave that + question to a possible follow-up change. For now let's keep the test + coverage this gives us. + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## builtin/submodule--helper.c ## 9: 79a47f9b94d = 9: b0238f699ce submodule--helper style: don't separate declared variables with \n\n 10: 5e13080f894 = 10: 5f5e68a868b submodule--helper style: add \n\n after variable declarations 11: dc5ac924496 = 11: 72dcf19e1c4 submodule--helper: replace memset() with { 0 }-initialization 12: 08cc9ce2e24 ! 12: e5e267dccd5 submodule--helper: convert a strbuf_detach() to xstrfmt() @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## Commit message ## - submodule--helper: convert a strbuf_detach() to xstrfmt() + submodule--helper: use xstrfmt() in clone_submodule() - Convert a case where we had no reason to use a "struct strbuf" to use - an xstrfmt(). This code was added along with other uses of "struct + Use xstrfmt() in clone_submodule() instead of a "struct strbuf" in two + cases where we weren't getting anything out of using the "struct + strbuf". + + This changes code that was was added along with other uses of "struct strbuf" in this function in ee8838d1577 (submodule: rewrite `module_clone` shell function in C, 2015-09-08). @@ builtin/submodule--helper.c: static int clone_submodule(struct module_clone_data if (validate_submodule_git_dir(sm_gitdir, clone_data->name) < 0) die(_("refusing to create/use '%s' in another submodule's " +@@ builtin/submodule--helper.c: static int clone_submodule(struct module_clone_data *clone_data) + die(_("clone of '%s' into submodule path '%s' failed"), + clone_data->url, clone_data->path); + } else { ++ char *path; ++ + if (clone_data->require_init && !access(clone_data->path, X_OK) && + !is_empty_dir(clone_data->path)) + die(_("directory not empty: '%s'"), clone_data->path); + if (safe_create_leading_directories_const(clone_data->path) < 0) + die(_("could not create directory '%s'"), clone_data->path); +- strbuf_addf(&sb, "%s/index", sm_gitdir); +- unlink_or_warn(sb.buf); +- strbuf_reset(&sb); ++ path = xstrfmt("%s/index", sm_gitdir); ++ unlink_or_warn(path); ++ free(path); + } + + connect_work_tree_and_git_dir(clone_data->path, sm_gitdir, 0); 13: afe34d8b73b ! 13: 91558745e2e submodule--helper: stop conflating "sb" in clone_submodule() @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## Commit message ## - submodule--helper: stop conflating "sb" in clone_submodule() + submodule--helper: move "sb" in clone_submodule() to its own scope - Refactor the two uses of a "struct strbuf sb" such that each of them - exists in its own scope. This makes the control flow clearer. + Refactor the only remaining use of a "struct strbuf sb" in + clone_submodule() to live in its own scope. This makes the code + clearer by limiting its lifetime. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> @@ builtin/submodule--helper.c: static void prepare_possible_alternates(const char if (!is_absolute_path(clone_data->path)) clone_data->path = xstrfmt("%s/%s", get_git_work_tree(), -@@ builtin/submodule--helper.c: static int clone_submodule(struct module_clone_data *clone_data) - die(_("clone of '%s' into submodule path '%s' failed"), - clone_data->url, clone_data->path); - } else { -+ struct strbuf sb = STRBUF_INIT; -+ - if (clone_data->require_init && !access(clone_data->path, X_OK) && - !is_empty_dir(clone_data->path)) - die(_("directory not empty: '%s'"), clone_data->path); -@@ builtin/submodule--helper.c: static int clone_submodule(struct module_clone_data *clone_data) - die(_("could not create directory '%s'"), clone_data->path); - strbuf_addf(&sb, "%s/index", sm_gitdir); - unlink_or_warn(sb.buf); -- strbuf_reset(&sb); -+ strbuf_release(&sb); - } - - connect_work_tree_and_git_dir(clone_data->path, sm_gitdir, 0); @@ builtin/submodule--helper.c: static int clone_submodule(struct module_clone_data *clone_data) free(sm_alternate); free(error_strategy); 14: b364f3200d8 ! 14: 866b8397a59 submodule--helper: pass a "const struct module_clone_data" to clone_submodule() @@ Commit message This new "struct string_list" still leaks memory, just as the "struct module_clone_data" did before. let's not fix that for now, to fix that we'll need to add some "goto cleanup" to the relevant code. That will - be done in a follow-up commits, at that point it'll be easier to fix - the memory leak. + eventually be done in follow-up commits, this change makes it easier + to fix the memory leak. The scope of the new "reference" variable in add_submodule() could be narrowed to the "else" block, but as we'll eventually free it with a @@ builtin/submodule--helper.c: static int clone_submodule(struct module_clone_data - clone_data->url, clone_data->path); + clone_data->url, clone_data_path); } else { - struct strbuf sb = STRBUF_INIT; + char *path; - if (clone_data->require_init && !access(clone_data->path, X_OK) && - !is_empty_dir(clone_data->path)) @@ builtin/submodule--helper.c: static int clone_submodule(struct module_clone_data + die(_("directory not empty: '%s'"), clone_data_path); + if (safe_create_leading_directories_const(clone_data_path) < 0) + die(_("could not create directory '%s'"), clone_data_path); - strbuf_addf(&sb, "%s/index", sm_gitdir); - unlink_or_warn(sb.buf); - strbuf_release(&sb); + path = xstrfmt("%s/index", sm_gitdir); + unlink_or_warn(path); + free(path); } - connect_work_tree_and_git_dir(clone_data->path, sm_gitdir, 0); 15: e319f4edc54 = 15: 1ff380ec7b5 submodule--helper: add "const" to copy of "update_data" 16: 68202100d9a = 16: d3a7e646adc submodule--helper: refactor "errmsg_str" to be a "struct strbuf" -: ----------- > 17: 23eb07176d9 submodule--helper: don't redundantly check "else if (res)" 17: e8aacb55a18 ! 18: 78f74df6d5e submodule--helper: rename "int res" to "int ret" @@ Commit message cmd_update() logic to C, 2022-03-15) to "ret", which is the convention in the rest of this file. - Eventual follow-up commits will change this code to a "goto cleanup" - pattern, let's have the post image look consistent with the rest. + Eventual follow-up commits will change the code in update_submodule() + to a "goto cleanup" pattern, let's have the post image look consistent + with the rest. For update_submodules() let's also use a "ret" for + consistency, that use was also added in b3c5f5cb048. We'll be + modifying that codepath in subsequent commits. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *upd update_data->displaypath); - if (res == 128) - exit(res); -- else if (res) + if (ret == 128) + exit(ret); -+ else if (ret) - return 1; + return 1; } +@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data) + + static int update_submodules(struct update_data *update_data) + { +- int i, res = 0; ++ int i, ret = 0; + struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT; + + suc.update_data = update_data; +@@ builtin/submodule--helper.c: static int update_submodules(struct update_data *update_data) + * - the listener can avoid doing any work if fetching failed. + */ + if (suc.quickstop) { +- res = 1; ++ ret = 1; + goto cleanup; + } + +@@ builtin/submodule--helper.c: static int update_submodules(struct update_data *update_data) + update_data->sm_path = ucd.sub->path; + + if (update_submodule(update_data)) +- res = 1; ++ ret = 1; + } + + cleanup: + string_list_clear(&update_data->references, 0); +- return res; ++ return ret; + } + + static int module_update(int argc, const char **argv, const char *prefix) 18: 41fb22d8549 < -: ----------- submodule--helper: add skeleton "goto cleanup" to update_submodule() -: ----------- > 19: f0258e37ebe submodule--helper: return "ret", not "1" from update_submodule() -: ----------- > 20: 70f030cca4e submodule--helper: add missing braces to "else" arm -: ----------- > 21: bce1a014a2f submodule--helper: don't call submodule_strategy_to_string() in BUG() -: ----------- > 22: 98c3e562c82 submodule--helper: move submodule_strategy_to_string() to only user -: ----------- > 23: db2d2fb5a21 submodule--helper: use "code" in run_update_command() 19: 5a26c9428be ! 24: d33260487bd submodule--helper: don't exit() on failure, return @@ builtin/submodule--helper.c: static int fetch_in_submodule(const char *module_pa -static int run_update_command(struct update_data *ud, int subforce) +static int run_update_command(struct update_data *ud, int subforce, -+ int *must_die_on_failurep) ++ int *must_die_on_failure) { struct child_process cp = CHILD_PROCESS_INIT; char *oid = oid_to_hex(&ud->oid); @@ builtin/submodule--helper.c: static int run_update_command(struct update_data *ud, int subforce) - BUG("unexpected update strategy type: %s", - submodule_strategy_to_string(&ud->update_strategy)); } -- if (must_die_on_failure) -- exit(128); -+ if (must_die_on_failure) { -+ *must_die_on_failurep = 1; -+ return 128; -+ } - - /* the command failed, but update must continue */ - return 1; + + if (ret == 128) +- exit(ret); ++ *must_die_on_failure = 1; + return ret; + } + @@ builtin/submodule--helper.c: static int run_update_command(struct update_data *ud, int subforce) return 0; } @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data +static int update_submodule(struct update_data *update_data, + int *must_die_on_failure) { - int ret = 1; + ensure_core_worktree(update_data->sm_path); @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data) + free(remote_ref); } - if (!oideq(&update_data->oid, &update_data->suboid) || update_data->force) { +- if (!oideq(&update_data->oid, &update_data->suboid) || update_data->force) - if (run_update_procedure(update_data)) ++ if (!oideq(&update_data->oid, &update_data->suboid) || update_data->force) { ++ int ret; ++ + ret = run_update_procedure(update_data, must_die_on_failure); -+ if (ret && *must_die_on_failure) { -+ goto cleanup; -+ } else if (ret) { -+ ret = 1; - goto cleanup; -+ } - } ++ if (*must_die_on_failure) ++ return ret; ++ if (ret) + return 1; ++ } if (update_data->recursive) { + struct child_process cp = CHILD_PROCESS_INIT; @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data) die_message(_("Failed to recurse into submodule path '%s'"), update_data->displaypath); - if (ret == 128) { + if (ret == 128) - exit(ret); + *must_die_on_failure = 1; -+ goto cleanup; - } else if (ret) { - ret = 1; - goto cleanup; + return ret; + } + @@ builtin/submodule--helper.c: static int update_submodules(struct update_data *update_data) for (i = 0; i < suc.update_clone_nr; i++) { struct update_clone_data ucd = suc.update_clone[i]; -+ int code; + int must_die_on_failure = 0; ++ int code; oidcpy(&update_data->oid, &ucd.oid); update_data->just_cloned = ucd.just_cloned; update_data->sm_path = ucd.sub->path; - if (update_submodule(update_data)) -- res = 1; + code = update_submodule(update_data, &must_die_on_failure); + if (code) -+ res = code; ++ ret = code; + if (must_die_on_failure) + goto cleanup; ++ else if (code) + ret = 1; } - cleanup: -: ----------- > 25: 9981a75d7e8 submodule--helper: libify determine_submodule_update_strategy() -: ----------- > 26: b48705c6cc0 submodule--helper: libify "must_die_on_failure" code paths -: ----------- > 27: 93cd1ccde54 submodule--helper: libify "must_die_on_failure" code paths (for die) 20: 579560cb9a8 = 28: 6160a1ab250 submodule--helper: fix bad config API usage -- 2.37.1.1233.ge8b09efaedc
