This new series based on "master" splits out the non-leak fixes from v3 of the submodule leaks series [1]. Sorry about the churn, but with e.g. the thread at [2] the "leaks" series was accumulating a lot of non-leaks related fixes, just so that we could get to the point of fixing various leaks. The immediate reason I split this off was because of the new "remove..helper" and "test-tool" patches here. I.e. some things that the leaks series was changing was test-only code, or code that was simply unused (or we had no business using anymore). The [1] series will then be re-rolled on top of this series, which will hopefully make it a lot easier to digest, as it'll now *just* focus on leak fixes. A passing CI run for this can be found at [3]. 1. https://lore.kernel.org/git/cover-v3-00.26-00000000000-20220721T191249Z-avarab@xxxxxxxxx/ 2. https://lore.kernel.org/git/220722.86y1wlqmqr.gmgdl@xxxxxxxxxxxxxxxxxxx/ 3. https://github.com/avar/git/tree/avar/submodule--helper-cleanup-and-tests 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 (18): 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: convert a strbuf_detach() to xstrfmt() submodule--helper: stop conflating "sb" in clone_submodule() submodule--helper: pass a "const struct module_clone_data" to clone_submodule() submodule--helper: rename "int res" to "int ret" submodule--helper: add skeleton "goto cleanup" to update_submodule() submodule--helper: don't exit() on failure, return submodule--helper: fix bad config API usage Makefile | 1 + builtin/submodule--helper.c | 338 ++++++++++++------------------- 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/t7413-submodule-is-active.sh | 32 +-- t/t7450-bad-git-dotfiles.sh | 2 +- 11 files changed, 335 insertions(+), 270 deletions(-) create mode 100644 t/helper/test-submodule.c create mode 100644 t/helper/test-tool-utils.h Range-diff: -: ----------- > 1: 84b05bda016 submodule tests: test usage behavior -: ----------- > 2: 9671169e348 submodule tests: test for "add <repository> <abs-path>" -: ----------- > 3: 7aa9c14251b submodule--helper: remove unused "name" helper -: ----------- > 4: 27df2efe718 submodule--helper: remove unused "list" helper -: ----------- > 5: 9fe8deb165a test-tool submodule-config: remove unused "--url" handling -: ----------- > 6: 8e4d2b09d56 submodule--helper: move "is-active" to a test-tool -: ----------- > 7: a2f3d812815 submodule--helper: move "check-name" to a test-tool -: ----------- > 8: 8188657cdfa submodule--helper: move "resolve-relative-url-test" to a test-tool -: ----------- > 9: 79a47f9b94d submodule--helper style: don't separate declared variables with \n\n -: ----------- > 10: 5e13080f894 submodule--helper style: add \n\n after variable declarations 1: 3c7ae3e0222 ! 11: dc5ac924496 submodule--helper: replace memset() with { 0 }-initialization @@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT; - struct list_objects_filter_options filter_options; + struct list_objects_filter_options filter_options = { 0 }; - struct option module_clone_options[] = { OPT_STRING(0, "prefix", &clone_data.prefix, + N_("path"), @@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv, const char *prefix) NULL }; @@ builtin/submodule--helper.c: static int module_update(int argc, const char **arg - struct list_objects_filter_options filter_options; + struct list_objects_filter_options filter_options = { 0 }; int ret; - struct option module_update_options[] = { + OPT__FORCE(&opt.force, N_("force checkout updates"), 0), @@ builtin/submodule--helper.c: static int module_update(int argc, const char **argv, const char *prefix) update_clone_config_from_gitmodules(&opt.max_jobs); git_config(git_update_clone_config, &opt.max_jobs); -: ----------- > 12: 08cc9ce2e24 submodule--helper: convert a strbuf_detach() to xstrfmt() 2: 32e4ae7ead5 ! 13: afe34d8b73b submodule--helper: stop conflating "sb" in clone_submodule() @@ Metadata ## Commit message ## submodule--helper: stop conflating "sb" in clone_submodule() - Refactor the three uses of a "struct strbuf sb" such that each of them + 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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> @@ builtin/submodule--helper.c: static void prepare_possible_alternates(const char + char *sm_alternate = NULL, *error_strategy = NULL; + struct child_process cp = CHILD_PROCESS_INIT; - if (!is_absolute_path(clone_data->path)) { -+ struct strbuf sb = STRBUF_INIT; -+ - strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path); - clone_data->path = strbuf_detach(&sb, NULL); - } else { + 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); 3: eee6ca28c6f ! 14: b364f3200d8 submodule--helper: pass a "const struct module_clone_data" to clone_submodule() @@ Commit message clobbering the "clone_data->path". We still need to add to the "reference" member, which is a "struct - string_list". We could do this by having clone_submodule() create its - own, and copy the contents over, but let's instead pass it as a - separate parameter. The main point of doing this is to make it clear - that e.g. "clone_data->path" always comes from the "argv", there's no - ambiguity about whether we can eventually free() the "struct - string_list". + string_list". Let's do this by having clone_submodule() create its + own, and copy the contents over, allowing us to pass it as a + separate parameter. + + 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. + + 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 + "goto cleanup" let's declare it at the start of the function. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## builtin/submodule--helper.c ## +@@ builtin/submodule--helper.c: struct module_clone_data { + const char *url; + const char *depth; + struct list_objects_filter_options *filter_options; +- struct string_list reference; + unsigned int quiet: 1; + unsigned int progress: 1; + unsigned int dissociate: 1; +@@ builtin/submodule--helper.c: struct module_clone_data { + int single_branch; + }; + #define MODULE_CLONE_DATA_INIT { \ +- .reference = STRING_LIST_INIT_NODUP, \ + .single_branch = -1, \ + } + @@ builtin/submodule--helper.c: static char *clone_submodule_sm_gitdir(const char *name) return sm_gitdir; } @@ builtin/submodule--helper.c: static char *clone_submodule_sm_gitdir(const char * struct child_process cp = CHILD_PROCESS_INIT; + const char *clone_data_path; - if (!is_absolute_path(clone_data->path)) { - struct strbuf sb = STRBUF_INIT; - - strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path); -- clone_data->path = strbuf_detach(&sb, NULL); -+ clone_data_path = strbuf_detach(&sb, NULL); - } else { + if (!is_absolute_path(clone_data->path)) +- clone_data->path = xstrfmt("%s/%s", get_git_work_tree(), +- clone_data->path); ++ clone_data_path = xstrfmt("%s/%s", get_git_work_tree(), ++ clone_data->path); + else - clone_data->path = xstrdup(clone_data->path); -+ clone_data_path = xstrdup(clone_data_path); - } ++ clone_data_path = xstrdup(clone_data->path); 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) if (safe_create_leading_directories_const(sm_gitdir) < 0) die(_("could not create directory '%s'"), sm_gitdir); @@ builtin/submodule--helper.c: static int clone_submodule(struct module_clone_data - if (clone_data->reference.nr) { + if (reference->nr) { struct string_list_item *item; + - for_each_string_list_item(item, &clone_data->reference) + for_each_string_list_item(item, reference) strvec_pushl(&cp.args, "--reference", @@ builtin/submodule--helper.c: static int clone_submodule(struct module_clone_data /* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */ git_config_get_string("submodule.alternateLocation", &sm_alternate); +@@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv, const char *prefix) + int dissociate = 0, quiet = 0, progress = 0, require_init = 0; + struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT; + struct list_objects_filter_options filter_options = { 0 }; ++ struct string_list reference = STRING_LIST_INIT_NODUP; + struct option module_clone_options[] = { + OPT_STRING(0, "prefix", &clone_data.prefix, + N_("path"), +@@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv, const char *prefix) + OPT_STRING(0, "url", &clone_data.url, + N_("string"), + N_("url where to clone the submodule from")), +- OPT_STRING_LIST(0, "reference", &clone_data.reference, ++ OPT_STRING_LIST(0, "reference", &reference, + N_("repo"), + N_("reference repository")), + OPT_BOOL(0, "dissociate", &dissociate, @@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv, const char *prefix) usage_with_options(git_submodule_helper_usage, module_clone_options); - clone_submodule(&clone_data); -+ clone_submodule(&clone_data, &clone_data.reference); ++ clone_submodule(&clone_data, &reference); list_objects_filter_release(&filter_options); return 0; } @@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add_data) + { + char *submod_gitdir_path; + struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT; ++ struct string_list reference = STRING_LIST_INIT_NODUP; + + /* perhaps the path already exists and is already a git repo, else clone it */ + if (is_directory(add_data->sm_path)) { +@@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add_data) + free(submod_gitdir_path); + } else { + struct child_process cp = CHILD_PROCESS_INIT; ++ + submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name); + + if (is_directory(submod_gitdir_path)) { +@@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add_data) + clone_data.quiet = add_data->quiet; + clone_data.progress = add_data->progress; + if (add_data->reference_path) +- string_list_append(&clone_data.reference, ++ string_list_append(&reference, + xstrdup(add_data->reference_path)); + clone_data.dissociate = add_data->dissociate; if (add_data->depth >= 0) clone_data.depth = xstrfmt("%d", add_data->depth); - if (clone_submodule(&clone_data)) -+ if (clone_submodule(&clone_data, &clone_data.reference)) ++ if (clone_submodule(&clone_data, &reference)) return -1; prepare_submodule_repo_env(&cp.env); 4: 26f0caf0386 < -: ----------- submodule--helper: fix a leak in "clone_submodule" 5: 75775bf4f6c < -: ----------- submodule--helper: fix trivial get_default_remote_submodule() leak 6: 7672ef1305f < -: ----------- submodule--helper: fix most "struct pathspec" memory leaks 7: 325aa1521e2 < -: ----------- submodule--helper: "struct pathspec" memory leak in module_update() 8: 424b24961b5 < -: ----------- submodule--helper: don't leak {run,capture}_command() cp.dir argument 9: abf5c4754a4 = 15: e319f4edc54 submodule--helper: add "const" to copy of "update_data" 10: 6ab5aabae35 < -: ----------- submodule--helper: add and use *_release() functions 11: feec1f20bf9 = 16: 68202100d9a submodule--helper: refactor "errmsg_str" to be a "struct strbuf" 12: d368db73de7 < -: ----------- submodule--helper: fix "errmsg_str" memory leak 13: 5be941b3d1b < -: ----------- submodule--helper: fix "sm_path" and other "module_cb_list" leaks 14: b8560e8c111 < -: ----------- submodule--helper: fix a leak with repo_clear() 15: abfd61f9f05 < -: ----------- submodule--helper: fix a memory leak in get_default_remote_submodule() 16: b8df96a9cf0 < -: ----------- submodule--helper: fix "reference" leak is "module_clone_data" 17: 7811bdbf149 < -: ----------- submodule--helper: fix obscure leak in module_add() 18: 01566d63926 < -: ----------- submodule--helper: fix a leak in module_add() 19: 7ef89abed86 < -: ----------- submodule--helper: fix a memory leak in print_status() 20: b8d47fc7d70 < -: ----------- submodule--helper: free some "displaypath" in "struct update_data" 21: 4fb17f0dff1 ! 17: e8aacb55a18 submodule--helper: rename "int res" to "int ret" @@ Commit message Rename the "res" variable added in b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15) to "ret", which is the convention - in the rest of this file. Subsequent commits will change this code to - a "goto cleanup" pattern, let's have the post image look consistent - with the rest. + 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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 22: af83925046b ! 18: 41fb22d8549 submodule--helper: add skeleton "goto cleanup" to update_submodule() @@ Commit message submodule--helper: add skeleton "goto cleanup" to update_submodule() Add a skeleton "goto cleanup" pattern to update_submodule(), rather - than having branches in it "return". This is in preparation for doing - something useful with the "cleanup" label, but for now we're using it - as the equivalent of a "done" label. + than having branches in it "return". This is in preparation for + eventually freeing data with the "cleanup" label, but for now we're + using it as the equivalent of a "done" label. The "exit()" branch is not handled yet, and neither is the exit() that run_update_procedure() might invoke. That'll be handled in a 23: 4c60784d281 ! 19: 5a26c9428be submodule--helper: don't exit() on failure, return @@ Commit message to the point where we could avoid calling exit() here. This introduces no functional changes, but makes it easier to both - call these routines as a library in the future, and to avoid leaking - memory. + call these routines as a library in the future, and to eventually + avoid leaking memory. + + This and similar control flow in submodule--helper.c could be made + simpler by properly "libifying" it, i.e. to have it consistently + return -1 on failures, and to early return on any non-success. + + But let's leave that larger project for now, and (mostly) emulate what + were doing with the "exit(128)" before this change. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 24: 7551af195ad < -: ----------- submodule--helper: free rest of "displaypath" in "struct update_data" 25: f650716cd7e = 20: 579560cb9a8 submodule--helper: fix bad config API usage 26: 581ce0872c0 < -: ----------- submodule--helper: fix a configure_added_submodule() leak -- 2.37.1.1167.g38fda70d8c4
