From 89bc7b5c0104bc7e98a3e2392598e90e6f49fcd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:43 +0200 Subject: [PATCH 01/33] submodule tests: test usage behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test what exit code and output we emit on "git submodule -h", how we handle "--" when no subcommand is specified, and how the top-level "--recursive" option is handled. For "-h" this doesn't make sense, but let's test for it so that any 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 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. For "--recursive" the exclusion of it from the top-level appears to have been an omission in 15fc56a8536 (git submodule foreach: Add --recursive to recurse into nested submodules, 2009-08-19), there 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 Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- t/t7400-submodule-basic.sh | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index e7cec2e457..b858871a95 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -14,6 +14,32 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +test_expect_success 'submodule usage: -h' ' + git submodule -h >out 2>err && + grep "^usage: git submodule" out && + test_must_be_empty err +' + +test_expect_success 'submodule usage: --recursive' ' + test_expect_code 1 git submodule --recursive >out 2>err && + grep "^usage: git submodule" err && + test_must_be_empty out +' + +test_expect_success 'submodule usage: status --' ' + test_expect_code 1 git submodule -- && + test_expect_code 1 git submodule --end-of-options +' + +for opt in '--quiet' '--cached' +do + test_expect_success "submodule usage: status $opt" ' + git submodule $opt && + git submodule status $opt && + git submodule $opt status + ' +done + test_expect_success 'submodule deinit works on empty repository' ' git submodule deinit --all ' From 59378e33553e1a7e00e315e61f4ae8d8550d49c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:44 +0200 Subject: [PATCH 02/33] submodule tests: test for "add " MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a missing test for ""add " where "" is an absolute path. This tests code added in [1] and later turned into an "else" branch in clone_submodule() in [2] that's never been tested. This needs to be skipped on WINDOWS because all of $PWD, $(pwd) and the "$(pwd -P)" we get via "$submodurl" would fail in CI with e.g.: fatal: could not create directory 'D:/a/git/git/t/trash directory.t7400-submodule-basic/.git/modules/D:/a/git/git/t/trash directory.t7400-submodule-basic/add-abs' I.e. we can't handle these sorts of paths in this context on that platform. I'm not sure where we run into the edges of "$PWD" behavior on Windows (see [1] for a previous loose end on the topic), but for the purposes of this test it's sufficient that we test this on other platforms. 1. ee8838d1577 (submodule: rewrite `module_clone` shell function in C, 2015-09-08) 2. f8eaa0ba98b (submodule--helper, module_clone: always operate on absolute paths, 2016-03-31) 1. https://lore.kernel.org/git/220630.86edz6c75c.gmgdl@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- t/t7400-submodule-basic.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index b858871a95..6a77d817a8 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -178,6 +178,11 @@ test_expect_success 'submodule add' ' test_must_be_empty untracked ' +test_expect_success !WINDOWS 'submodule add (absolute path)' ' + test_when_finished "git reset --hard" && + git submodule add "$submodurl" "$submodurl/add-abs" +' + test_expect_success 'setup parent and one repository' ' test_create_repo parent && test_commit -C parent one From 76d63ddc467c28263d7c5f48baf3894354056743 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:45 +0200 Subject: [PATCH 03/33] submodule--helper: remove unused "name" helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "name" helper has not been used since e83e3333b57 (submodule: port submodule subcommand 'summary' from shell to C, 2020-08-13). Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index fac52ade5e..ac2553ba9d 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -771,24 +771,6 @@ static int module_status(int argc, const char **argv, const char *prefix) return 0; } -static int module_name(int argc, const char **argv, const char *prefix) -{ - const struct submodule *sub; - - if (argc != 2) - usage(_("git submodule--helper name ")); - - sub = submodule_from_path(the_repository, null_oid(), argv[1]); - - if (!sub) - die(_("no submodule mapping found in .gitmodules for path '%s'"), - argv[1]); - - printf("%s\n", sub->name); - - return 0; -} - struct module_cb { unsigned int mod_src; unsigned int mod_dst; @@ -3359,7 +3341,6 @@ struct cmd_struct { static struct cmd_struct commands[] = { {"list", module_list, 0}, - {"name", module_name, 0}, {"clone", module_clone, SUPPORT_SUPER_PREFIX}, {"add", module_add, 0}, {"update", module_update, SUPPORT_SUPER_PREFIX}, From 31955475d1c283120d5d84247eb3fd55d9f5fdd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:46 +0200 Subject: [PATCH 04/33] submodule--helper: remove unused "list" helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the "submodule--helper list" sub-command, which hasn't been used by git-submodule.sh since 2964d6e5e1e (submodule: port subcommand 'set-branch' from shell to C, 2020-06-02). There was a test added in 2b56bb7a87a (submodule helper list: respect correct path prefix, 2016-02-24) which relied on it, but the right thing to do here is to delete that test as well. That test was regression testing the "list" subcommand itself. We're not getting anything useful from the "list | cut -f2" invocation that we couldn't get from "foreach 'echo $sm_path'". Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 40 ------------------------------------- t/t7400-submodule-basic.sh | 25 ----------------------- 2 files changed, 65 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index ac2553ba9d..47ed24c6a6 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -266,45 +266,6 @@ static char *get_up_path(const char *path) return strbuf_detach(&sb, NULL); } -static int module_list(int argc, const char **argv, const char *prefix) -{ - int i; - struct pathspec pathspec; - struct module_list list = MODULE_LIST_INIT; - - struct option module_list_options[] = { - OPT_STRING(0, "prefix", &prefix, - N_("path"), - N_("alternative anchor for relative paths")), - OPT_END() - }; - - const char *const git_submodule_helper_usage[] = { - N_("git submodule--helper list [--prefix=] [...]"), - NULL - }; - - argc = parse_options(argc, argv, prefix, module_list_options, - git_submodule_helper_usage, 0); - - if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) - return 1; - - for (i = 0; i < list.nr; i++) { - const struct cache_entry *ce = list.entries[i]; - - if (ce_stage(ce)) - printf("%06o %s U\t", ce->ce_mode, - oid_to_hex(null_oid())); - else - printf("%06o %s %d\t", ce->ce_mode, - oid_to_hex(&ce->oid), ce_stage(ce)); - - fprintf(stdout, "%s\n", ce->name); - } - return 0; -} - static void for_each_listed_submodule(const struct module_list *list, each_submodule_fn fn, void *cb_data) { @@ -3340,7 +3301,6 @@ struct cmd_struct { }; static struct cmd_struct commands[] = { - {"list", module_list, 0}, {"clone", module_clone, SUPPORT_SUPER_PREFIX}, {"add", module_add, 0}, {"update", module_update, SUPPORT_SUPER_PREFIX}, diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 6a77d817a8..b50db3f103 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -1255,31 +1255,6 @@ test_expect_success 'submodule add clone shallow submodule' ' ) ' -test_expect_success 'submodule helper list is not confused by common prefixes' ' - mkdir -p dir1/b && - ( - cd dir1/b && - git init && - echo hi >testfile2 && - git add . && - git commit -m "test1" - ) && - mkdir -p dir2/b && - ( - cd dir2/b && - git init && - echo hello >testfile1 && - git add . && - git commit -m "test2" - ) && - git submodule add /dir1/b dir1/b && - git submodule add /dir2/b dir2/b && - git commit -m "first submodule commit" && - git submodule--helper list dir1/b | cut -f 2 >actual && - echo "dir1/b" >expect && - test_cmp expect actual -' - test_expect_success 'setup superproject with submodules' ' git init sub1 && test_commit -C sub1 test && From 255a1ae5dab68ba1e310077b20c69bd92ad899d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:47 +0200 Subject: [PATCH 05/33] test-tool submodule-config: remove unused "--url" handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No test has used this "--url" parameter since the test code that made use of it was removed in 32bc548329d (submodule-config: remove support for overlaying repository config, 2017-08-03). Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- t/helper/test-submodule-config.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c index e2692746df..22a41c4092 100644 --- a/t/helper/test-submodule-config.c +++ b/t/helper/test-submodule-config.c @@ -15,14 +15,11 @@ int cmd__submodule_config(int argc, const char **argv) { const char **arg = argv; int my_argc = argc; - int output_url = 0; int lookup_name = 0; arg++; my_argc--; while (arg[0] && starts_with(arg[0], "--")) { - if (!strcmp(arg[0], "--url")) - output_url = 1; if (!strcmp(arg[0], "--name")) lookup_name = 1; arg++; @@ -57,12 +54,8 @@ int cmd__submodule_config(int argc, const char **argv) if (!submodule) die_usage(argc, argv, "Submodule not found."); - if (output_url) - printf("Submodule url: '%s' for path '%s'\n", - submodule->url, submodule->path); - else - printf("Submodule name: '%s' for path '%s'\n", - submodule->name, submodule->path); + printf("Submodule name: '%s' for path '%s'\n", submodule->name, + submodule->path); arg += 2; } From 9fb2a970e9ffe5e37637f25b700e8bc09789fbf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:48 +0200 Subject: [PATCH 06/33] submodule--helper: move "is-active" to a test-tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Create a new "test-tool submodule" and move the "is-active" subcommand over to it. It was added in 5c2bd8b77ae (submodule--helper: add is-active subcommand, 2017-03-16), since a452128a36c (submodule--helper: introduce add-config subcommand, 2021-08-06) it hasn't been used by git-submodule.sh. Since we're creating a command dispatch similar to test-tool.c itself let's split out the "struct test_cmd" into a new test-tool-utils.h, which both this new code and test-tool.c itself can use. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- Makefile | 1 + builtin/submodule--helper.c | 9 ------ t/helper/test-submodule.c | 58 ++++++++++++++++++++++++++++++++++ t/helper/test-tool-utils.h | 9 ++++++ t/helper/test-tool.c | 7 ++-- t/helper/test-tool.h | 1 + t/t7413-submodule-is-active.sh | 35 ++++++++++---------- 7 files changed, 90 insertions(+), 30 deletions(-) create mode 100644 t/helper/test-submodule.c create mode 100644 t/helper/test-tool-utils.h diff --git a/Makefile b/Makefile index 1624471bad..ad7fbd3688 100644 --- a/Makefile +++ b/Makefile @@ -785,6 +785,7 @@ TEST_BUILTINS_OBJS += test-strcmp-offset.o TEST_BUILTINS_OBJS += test-string-list.o TEST_BUILTINS_OBJS += test-submodule-config.o TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o +TEST_BUILTINS_OBJS += test-submodule.o TEST_BUILTINS_OBJS += test-subprocess.o TEST_BUILTINS_OBJS += test-trace2.o TEST_BUILTINS_OBJS += test-urlmatch-normalization.o diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 47ed24c6a6..b2fc732b5d 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2728,14 +2728,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix) return 0; } -static int is_active(int argc, const char **argv, const char *prefix) -{ - if (argc != 2) - die("submodule--helper is-active takes exactly 1 argument"); - - return !is_submodule_active(the_repository, argv[1]); -} - /* * Exit non-zero if any of the submodule names given on the command line is * invalid. If no names are given, filter stdin to print only valid names @@ -3313,7 +3305,6 @@ static struct cmd_struct commands[] = { {"summary", module_summary, 0}, {"push-check", push_check, 0}, {"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX}, - {"is-active", is_active, 0}, {"check-name", check_name, 0}, {"config", module_config, 0}, {"set-url", module_set_url, 0}, diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c new file mode 100644 index 0000000000..494c6558d9 --- /dev/null +++ b/t/helper/test-submodule.c @@ -0,0 +1,58 @@ +#include "test-tool.h" +#include "test-tool-utils.h" +#include "cache.h" +#include "parse-options.h" +#include "submodule.h" + +#define TEST_TOOL_IS_ACTIVE_USAGE \ + "test-tool submodule is-active " +static const char *submodule_is_active_usage[] = { + TEST_TOOL_IS_ACTIVE_USAGE, + NULL +}; + +static const char *submodule_usage[] = { + TEST_TOOL_IS_ACTIVE_USAGE, + NULL +}; + +static int cmd__submodule_is_active(int argc, const char **argv) +{ + struct option options[] = { + OPT_END() + }; + argc = parse_options(argc, argv, "test-tools", options, + submodule_is_active_usage, 0); + if (argc != 1) + usage_with_options(submodule_is_active_usage, options); + + setup_git_directory(); + + return !is_submodule_active(the_repository, argv[0]); +} + +static struct test_cmd cmds[] = { + { "is-active", cmd__submodule_is_active }, +}; + +int cmd__submodule(int argc, const char **argv) +{ + struct option options[] = { + OPT_END() + }; + size_t i; + + argc = parse_options(argc, argv, "test-tools", options, submodule_usage, + PARSE_OPT_STOP_AT_NON_OPTION); + if (argc < 1) + usage_with_options(submodule_usage, options); + + for (i = 0; i < ARRAY_SIZE(cmds); i++) + if (!strcmp(cmds[i].name, argv[0])) + return cmds[i].fn(argc, argv); + + usage_msg_optf("unknown subcommand '%s'", submodule_usage, options, + argv[0]); + + return 0; +} diff --git a/t/helper/test-tool-utils.h b/t/helper/test-tool-utils.h new file mode 100644 index 0000000000..6a0e5e0074 --- /dev/null +++ b/t/helper/test-tool-utils.h @@ -0,0 +1,9 @@ +#ifndef TEST_TOOL_UTILS_H +#define TEST_TOOL_UTILS_H + +struct test_cmd { + const char *name; + int (*fn)(int argc, const char **argv); +}; + +#endif diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 318fdbab0c..7a6a8b88a9 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -1,5 +1,6 @@ #include "git-compat-util.h" #include "test-tool.h" +#include "test-tool-utils.h" #include "trace2.h" #include "parse-options.h" @@ -8,11 +9,6 @@ static const char * const test_tool_usage[] = { NULL }; -struct test_cmd { - const char *name; - int (*fn)(int argc, const char **argv); -}; - static struct test_cmd cmds[] = { { "advise", cmd__advise_if_enabled }, { "bitmap", cmd__bitmap }, @@ -78,6 +74,7 @@ static struct test_cmd cmds[] = { { "simple-ipc", cmd__simple_ipc }, { "strcmp-offset", cmd__strcmp_offset }, { "string-list", cmd__string_list }, + { "submodule", cmd__submodule }, { "submodule-config", cmd__submodule_config }, { "submodule-nested-repo-config", cmd__submodule_nested_repo_config }, { "subprocess", cmd__subprocess }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index bb79927163..5f4f69dee8 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -68,6 +68,7 @@ int cmd__sigchain(int argc, const char **argv); int cmd__simple_ipc(int argc, const char **argv); int cmd__strcmp_offset(int argc, const char **argv); int cmd__string_list(int argc, const char **argv); +int cmd__submodule(int argc, const char **argv); int cmd__submodule_config(int argc, const char **argv); int cmd__submodule_nested_repo_config(int argc, const char **argv); int cmd__subprocess(int argc, const char **argv); diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh index c8e7e98331..ede6f02dbd 100755 --- a/t/t7413-submodule-is-active.sh +++ b/t/t7413-submodule-is-active.sh @@ -1,9 +1,12 @@ #!/bin/sh -test_description='Test submodule--helper is-active +test_description='Test with test-tool submodule is-active -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 @@ -25,13 +28,13 @@ test_expect_success 'setup' ' ' test_expect_success 'is-active works with urls' ' - git -C super submodule--helper is-active sub1 && - git -C super submodule--helper is-active sub2 && + test-tool -C super submodule is-active sub1 && + test-tool -C super submodule is-active sub2 && git -C super config --unset submodule.sub1.URL && - test_must_fail git -C super submodule--helper is-active sub1 && + test_must_fail test-tool -C super submodule is-active sub1 && git -C super config submodule.sub1.URL ../sub && - git -C super submodule--helper is-active sub1 + test-tool -C super submodule is-active sub1 ' test_expect_success 'is-active works with submodule..active config' ' @@ -39,11 +42,11 @@ test_expect_success 'is-active works with submodule..active config' ' test_when_finished "git -C super config submodule.sub1.URL ../sub" && git -C super config --bool submodule.sub1.active "false" && - test_must_fail git -C super submodule--helper is-active sub1 && + test_must_fail test-tool -C super submodule is-active sub1 && git -C super config --bool submodule.sub1.active "true" && git -C super config --unset submodule.sub1.URL && - git -C super submodule--helper is-active sub1 + test-tool -C super submodule is-active sub1 ' test_expect_success 'is-active works with basic submodule.active config' ' @@ -53,17 +56,17 @@ test_expect_success 'is-active works with basic submodule.active config' ' git -C super config --add submodule.active "." && git -C super config --unset submodule.sub1.URL && - git -C super submodule--helper is-active sub1 && - git -C super submodule--helper is-active sub2 + test-tool -C super submodule is-active sub1 && + test-tool -C super submodule is-active sub2 ' test_expect_success 'is-active correctly works with paths that are not submodules' ' test_when_finished "git -C super config --unset-all submodule.active" && - test_must_fail git -C super submodule--helper is-active not-a-submodule && + test_must_fail test-tool -C super submodule is-active not-a-submodule && git -C super config --add submodule.active "." && - test_must_fail git -C super submodule--helper is-active not-a-submodule + test_must_fail test-tool -C super submodule is-active not-a-submodule ' test_expect_success 'is-active works with exclusions in submodule.active config' ' @@ -72,8 +75,8 @@ test_expect_success 'is-active works with exclusions in submodule.active config' git -C super config --add submodule.active "." && git -C super config --add submodule.active ":(exclude)sub1" && - test_must_fail git -C super submodule--helper is-active sub1 && - git -C super submodule--helper is-active sub2 + test_must_fail test-tool -C super submodule is-active sub1 && + test-tool -C super submodule is-active sub2 ' test_expect_success 'is-active with submodule.active and submodule..active' ' @@ -85,8 +88,8 @@ test_expect_success 'is-active with submodule.active and submodule..active git -C super config --bool submodule.sub1.active "false" && git -C super config --bool submodule.sub2.active "true" && - test_must_fail git -C super submodule--helper is-active sub1 && - git -C super submodule--helper is-active sub2 + test_must_fail test-tool -C super submodule is-active sub1 && + test-tool -C super submodule is-active sub2 ' test_expect_success 'is-active, submodule.active and submodule add' ' From 85321a346b5889f5602603a300d643493901ba44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:49 +0200 Subject: [PATCH 07/33] submodule--helper: move "check-name" to a test-tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the "check-name" helper to a test-tool, since 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 Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 24 ------------------- t/helper/test-submodule.c | 46 +++++++++++++++++++++++++++++++++++++ t/t7450-bad-git-dotfiles.sh | 2 +- 3 files changed, 47 insertions(+), 25 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index b2fc732b5d..0630788608 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2728,29 +2728,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix) return 0; } -/* - * Exit non-zero if any of the submodule names given on the command line is - * invalid. If no names are given, filter stdin to print only valid names - * (which is primarily intended for testing). - */ -static int check_name(int argc, const char **argv, const char *prefix) -{ - if (argc > 1) { - while (*++argv) { - if (check_submodule_name(*argv) < 0) - return 1; - } - } else { - struct strbuf buf = STRBUF_INIT; - while (strbuf_getline(&buf, stdin) != EOF) { - if (!check_submodule_name(buf.buf)) - printf("%s\n", buf.buf); - } - strbuf_release(&buf); - } - return 0; -} - static int module_config(int argc, const char **argv, const char *prefix) { enum { @@ -3305,7 +3282,6 @@ static struct cmd_struct commands[] = { {"summary", module_summary, 0}, {"push-check", push_check, 0}, {"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX}, - {"check-name", check_name, 0}, {"config", module_config, 0}, {"set-url", module_set_url, 0}, {"set-branch", module_set_branch, 0}, diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c index 494c6558d9..9f0eb44019 100644 --- a/t/helper/test-submodule.c +++ b/t/helper/test-submodule.c @@ -2,8 +2,16 @@ #include "test-tool-utils.h" #include "cache.h" #include "parse-options.h" +#include "submodule-config.h" #include "submodule.h" +#define TEST_TOOL_CHECK_NAME_USAGE \ + "test-tool submodule check-name " +static const char *submodule_check_name_usage[] = { + TEST_TOOL_CHECK_NAME_USAGE, + NULL +}; + #define TEST_TOOL_IS_ACTIVE_USAGE \ "test-tool submodule is-active " static const char *submodule_is_active_usage[] = { @@ -12,10 +20,47 @@ static const char *submodule_is_active_usage[] = { }; static const char *submodule_usage[] = { + TEST_TOOL_CHECK_NAME_USAGE, TEST_TOOL_IS_ACTIVE_USAGE, NULL }; +/* + * Exit non-zero if any of the submodule names given on the command line is + * invalid. If no names are given, filter stdin to print only valid names + * (which is primarily intended for testing). + */ +static int check_name(int argc, const char **argv) +{ + if (argc > 1) { + while (*++argv) { + if (check_submodule_name(*argv) < 0) + return 1; + } + } else { + struct strbuf buf = STRBUF_INIT; + while (strbuf_getline(&buf, stdin) != EOF) { + if (!check_submodule_name(buf.buf)) + printf("%s\n", buf.buf); + } + strbuf_release(&buf); + } + return 0; +} + +static int cmd__submodule_check_name(int argc, const char **argv) +{ + struct option options[] = { + OPT_END() + }; + argc = parse_options(argc, argv, "test-tools", options, + submodule_check_name_usage, 0); + if (argc) + usage_with_options(submodule_check_name_usage, options); + + return check_name(argc, argv); +} + static int cmd__submodule_is_active(int argc, const char **argv) { struct option options[] = { @@ -32,6 +77,7 @@ static int cmd__submodule_is_active(int argc, const char **argv) } static struct test_cmd cmds[] = { + { "check-name", cmd__submodule_check_name }, { "is-active", cmd__submodule_is_active }, }; diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh index 41706c1c9f..2c24f120da 100755 --- a/t/t7450-bad-git-dotfiles.sh +++ b/t/t7450-bad-git-dotfiles.sh @@ -21,7 +21,7 @@ test_expect_success 'check names' ' valid/with/paths EOF - git submodule--helper check-name >actual <<-\EOF && + test-tool submodule check-name >actual <<-\EOF && valid valid/with/paths From 96a28a9bc655e1804a516de864d5aada13260f50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:50 +0200 Subject: [PATCH 08/33] submodule--helper: move "resolve-relative-url-test" to a test-tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As its name suggests the "resolve-relative-url-test" has never been used outside of the test suite, see 63e95beb085 (submodule: port 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 Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 23 -------------------- t/helper/test-submodule.c | 42 +++++++++++++++++++++++++++++++++++++ t/t0060-path-utils.sh | 2 +- 3 files changed, 43 insertions(+), 24 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 0630788608..246457ec2e 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -96,28 +96,6 @@ static char *resolve_relative_url(const char *rel_url, const char *up_path, int return resolved_url; } -static int resolve_relative_url_test(int argc, const char **argv, const char *prefix) -{ - char *remoteurl, *res; - const char *up_path, *url; - - if (argc != 4) - die("resolve-relative-url-test only accepts three arguments: "); - - up_path = argv[1]; - remoteurl = xstrdup(argv[2]); - url = argv[3]; - - if (!strcmp(up_path, "(null)")) - up_path = NULL; - - res = relative_url(remoteurl, url, up_path); - puts(res); - free(res); - free(remoteurl); - return 0; -} - /* the result should be freed by the caller. */ static char *get_submodule_displaypath(const char *path, const char *prefix) { @@ -3273,7 +3251,6 @@ static struct cmd_struct commands[] = { {"clone", module_clone, SUPPORT_SUPER_PREFIX}, {"add", module_add, 0}, {"update", module_update, SUPPORT_SUPER_PREFIX}, - {"resolve-relative-url-test", resolve_relative_url_test, 0}, {"foreach", module_foreach, SUPPORT_SUPER_PREFIX}, {"init", module_init, 0}, {"status", module_status, SUPPORT_SUPER_PREFIX}, diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c index 9f0eb44019..e0e0c53d38 100644 --- a/t/helper/test-submodule.c +++ b/t/helper/test-submodule.c @@ -2,6 +2,7 @@ #include "test-tool-utils.h" #include "cache.h" #include "parse-options.h" +#include "remote.h" #include "submodule-config.h" #include "submodule.h" @@ -19,9 +20,17 @@ static const char *submodule_is_active_usage[] = { NULL }; +#define TEST_TOOL_RESOLVE_RELATIVE_URL_USAGE \ + "test-tool submodule resolve-relative-url " +static const char *submodule_resolve_relative_url_usage[] = { + TEST_TOOL_RESOLVE_RELATIVE_URL_USAGE, + NULL, +}; + static const char *submodule_usage[] = { TEST_TOOL_CHECK_NAME_USAGE, TEST_TOOL_IS_ACTIVE_USAGE, + TEST_TOOL_RESOLVE_RELATIVE_URL_USAGE, NULL }; @@ -76,9 +85,42 @@ static int cmd__submodule_is_active(int argc, const char **argv) return !is_submodule_active(the_repository, argv[0]); } +static int resolve_relative_url(int argc, const char **argv) +{ + char *remoteurl, *res; + const char *up_path, *url; + + up_path = argv[0]; + remoteurl = xstrdup(argv[1]); + url = argv[2]; + + if (!strcmp(up_path, "(null)")) + up_path = NULL; + + res = relative_url(remoteurl, url, up_path); + puts(res); + free(res); + free(remoteurl); + return 0; +} + +static int cmd__submodule_resolve_relative_url(int argc, const char **argv) +{ + struct option options[] = { + OPT_END() + }; + argc = parse_options(argc, argv, "test-tools", options, + submodule_resolve_relative_url_usage, 0); + if (argc != 3) + usage_with_options(submodule_resolve_relative_url_usage, options); + + return resolve_relative_url(argc, argv); +} + static struct test_cmd cmds[] = { { "check-name", cmd__submodule_check_name }, { "is-active", cmd__submodule_is_active }, + { "resolve-relative-url", cmd__submodule_resolve_relative_url}, }; int cmd__submodule(int argc, const char **argv) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 1f2007e62b..68e29c904a 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -22,7 +22,7 @@ relative_path() { test_submodule_relative_url() { test_expect_success "test_submodule_relative_url: $1 $2 $3 => $4" " - actual=\$(git submodule--helper resolve-relative-url-test '$1' '$2' '$3') && + actual=\$(test-tool submodule resolve-relative-url '$1' '$2' '$3') && test \"\$actual\" = '$4' " } From e2d5c886bfb9ac21368786d9330a3e395f21ac5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:51 +0200 Subject: [PATCH 09/33] submodule--helper style: don't separate declared variables with \n\n MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The usual style in the codebase is to separate declared variables with a single newline, not two, let's adjust this code to conform to that. This makes the eventual addition of various "int ret" variables more consistent. In doing this the comment added in 2964d6e5e1e (submodule: port subcommand 'set-branch' from shell to C, 2020-06-02) might become ambiguous to some, although it should be clear what it's referring to, let's move it above the 'OPT_NOOP_NOARG('q', "quiet")' to make that clearer. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 33 +++++---------------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 246457ec2e..53b9e14767 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -368,14 +368,12 @@ static int module_foreach(int argc, const char **argv, const char *prefix) struct foreach_cb info = FOREACH_CB_INIT; struct pathspec pathspec; struct module_list list = MODULE_LIST_INIT; - struct option module_foreach_options[] = { OPT__QUIET(&info.quiet, N_("suppress output of entering each submodule command")), OPT_BOOL(0, "recursive", &info.recursive, N_("recurse into nested submodules")), OPT_END() }; - const char *const git_submodule_helper_usage[] = { N_("git submodule foreach [--quiet] [--recursive] [--] "), NULL @@ -504,12 +502,10 @@ static int module_init(int argc, const char **argv, const char *prefix) struct pathspec pathspec; struct module_list list = MODULE_LIST_INIT; int quiet = 0; - struct option module_init_options[] = { OPT__QUIET(&quiet, N_("suppress output for initializing a submodule")), OPT_END() }; - const char *const git_submodule_helper_usage[] = { N_("git submodule init [] []"), NULL @@ -682,14 +678,12 @@ static int module_status(int argc, const char **argv, const char *prefix) struct pathspec pathspec; struct module_list list = MODULE_LIST_INIT; int quiet = 0; - struct option module_status_options[] = { OPT__QUIET(&quiet, N_("suppress submodule status output")), OPT_BIT(0, "cached", &info.flags, N_("use commit stored in the index instead of the one stored in the submodule HEAD"), OPT_CACHED), OPT_BIT(0, "recursive", &info.flags, N_("recurse into nested submodules"), OPT_RECURSIVE), OPT_END() }; - const char *const git_submodule_helper_usage[] = { N_("git submodule status [--quiet] [--cached] [--recursive] [...]"), NULL @@ -1082,7 +1076,6 @@ static int module_summary(int argc, const char **argv, const char *prefix) enum diff_cmd diff_cmd = DIFF_INDEX; struct object_id head_oid; int ret; - struct option module_summary_options[] = { OPT_BOOL(0, "cached", &cached, N_("use the commit stored in the index instead of the submodule HEAD")), @@ -1094,7 +1087,6 @@ static int module_summary(int argc, const char **argv, const char *prefix) N_("limit the summary size")), OPT_END() }; - const char *const git_submodule_helper_usage[] = { N_("git submodule summary [] [] [--] []"), NULL @@ -1251,14 +1243,12 @@ static int module_sync(int argc, const char **argv, const char *prefix) struct module_list list = MODULE_LIST_INIT; int quiet = 0; int recursive = 0; - struct option module_sync_options[] = { OPT__QUIET(&quiet, N_("suppress output of synchronizing submodule url")), OPT_BOOL(0, "recursive", &recursive, N_("recurse into nested submodules")), OPT_END() }; - const char *const git_submodule_helper_usage[] = { N_("git submodule sync [--quiet] [--recursive] []"), NULL @@ -1390,14 +1380,12 @@ static int module_deinit(int argc, const char **argv, const char *prefix) int quiet = 0; int force = 0; int all = 0; - struct option module_deinit_options[] = { OPT__QUIET(&quiet, N_("suppress submodule status output")), OPT__FORCE(&force, N_("remove submodule working trees even if they contain local changes"), 0), OPT_BOOL(0, "all", &all, N_("unregister all submodules")), OPT_END() }; - const char *const git_submodule_helper_usage[] = { N_("git submodule deinit [--quiet] [-f | --force] [--all | [--] [...]]"), NULL @@ -1666,7 +1654,6 @@ 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; - struct option module_clone_options[] = { OPT_STRING(0, "prefix", &clone_data.prefix, N_("path"), @@ -1698,7 +1685,6 @@ static int module_clone(int argc, const char **argv, const char *prefix) OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options), OPT_END() }; - const char *const git_submodule_helper_usage[] = { N_("git submodule--helper clone [--prefix=] [--quiet] " "[--reference ] [--name ] [--depth ] " @@ -2486,7 +2472,6 @@ static int module_update(int argc, const char **argv, const char *prefix) struct update_data opt = UPDATE_DATA_INIT; struct list_objects_filter_options filter_options; int ret; - struct option module_update_options[] = { OPT__FORCE(&opt.force, N_("force checkout updates"), 0), OPT_BOOL(0, "init", &opt.init, @@ -2530,7 +2515,6 @@ static int module_update(int argc, const char **argv, const char *prefix) OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options), OPT_END() }; - const char *const git_submodule_helper_usage[] = { N_("git submodule [--quiet] update" " [--init [--filter=]] [--remote]" @@ -2679,7 +2663,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix) struct pathspec pathspec; struct module_list list = MODULE_LIST_INIT; unsigned flags = ABSORB_GITDIR_RECURSE_SUBMODULES; - struct option embed_gitdir_options[] = { OPT_STRING(0, "prefix", &prefix, N_("path"), @@ -2688,7 +2671,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix) ABSORB_GITDIR_RECURSE_SUBMODULES), OPT_END() }; - const char *const git_submodule_helper_usage[] = { N_("git submodule absorbgitdirs [] [...]"), NULL @@ -2712,7 +2694,6 @@ static int module_config(int argc, const char **argv, const char *prefix) CHECK_WRITEABLE = 1, DO_UNSET = 2 } command = 0; - struct option module_config_options[] = { OPT_CMDMODE(0, "check-writeable", &command, N_("check if it is safe to write to the .gitmodules file"), @@ -2758,7 +2739,6 @@ static int module_set_url(int argc, const char **argv, const char *prefix) const char *newurl; const char *path; char *config_name; - struct option options[] = { OPT__QUIET(&quiet, N_("suppress output for setting url of a submodule")), OPT_END() @@ -2789,13 +2769,13 @@ static int module_set_branch(int argc, const char **argv, const char *prefix) const char *opt_branch = NULL; const char *path; char *config_name; - - /* - * We accept the `quiet` option for uniformity across subcommands, - * though there is nothing to make less verbose in this subcommand. - */ struct option options[] = { + /* + * We accept the `quiet` option for uniformity across subcommands, + * though there is nothing to make less verbose in this subcommand. + */ OPT_NOOP_NOARG('q', "quiet"), + OPT_BOOL('d', "default", &opt_default, N_("set the default tracking branch to master")), OPT_STRING('b', "branch", &opt_branch, N_("branch"), @@ -2830,7 +2810,6 @@ static int module_create_branch(int argc, const char **argv, const char *prefix) { enum branch_track track; int quiet = 0, force = 0, reflog = 0, dry_run = 0; - struct option options[] = { OPT__QUIET(&quiet, N_("print only error messages")), OPT__FORCE(&force, N_("force creation"), 0), @@ -3129,7 +3108,6 @@ static int module_add(int argc, const char **argv, const char *prefix) int force = 0, quiet = 0, progress = 0, dissociate = 0; struct add_data add_data = ADD_DATA_INIT; char *to_free = NULL; - struct option options[] = { OPT_STRING('b', "branch", &add_data.branch, N_("branch"), N_("branch of repository to add as submodule")), @@ -3146,7 +3124,6 @@ static int module_add(int argc, const char **argv, const char *prefix) OPT_INTEGER(0, "depth", &add_data.depth, N_("depth for shallow clones")), OPT_END() }; - const char *const usage[] = { N_("git submodule add [] [--] []"), NULL From 0b83b2b03a322a7948ddb453ab8766594ca239de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:52 +0200 Subject: [PATCH 10/33] submodule--helper style: add \n\n after variable declarations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the preceding commit fixed style issues with \n\n among the declared variables let's fix the minor stylistic issues with those variables not being consistently followed by a \n\n. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 53b9e14767..15bf89910f 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -167,6 +167,7 @@ static int module_list_compute(int argc, const char **argv, { int i, result = 0; char *ps_matched = NULL; + parse_pathspec(pathspec, 0, PATHSPEC_PREFER_FULL, prefix, argv); @@ -248,6 +249,7 @@ static void for_each_listed_submodule(const struct module_list *list, each_submodule_fn fn, void *cb_data) { int i; + for (i = 0; i < list->nr; i++) fn(list->entries[i], cb_data); } @@ -267,7 +269,6 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item, struct foreach_cb *info = cb_data; const char *path = list_item->name; const struct object_id *ce_oid = &list_item->oid; - const struct submodule *sub; struct child_process cp = CHILD_PROCESS_INIT; char *displaypath; @@ -456,6 +457,7 @@ static void init_submodule(const char *path, const char *prefix, if (starts_with_dot_dot_slash(url) || starts_with_dot_slash(url)) { char *oldurl = url; + url = resolve_relative_url(oldurl, NULL, 0); free(oldurl); } @@ -493,6 +495,7 @@ static void init_submodule(const char *path, const char *prefix, static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data) { struct init_cb *info = cb_data; + init_submodule(list_item->name, info->prefix, info->flags); } @@ -562,6 +565,7 @@ static int handle_submodule_head_ref(const char *refname, void *cb_data) { struct object_id *output = cb_data; + if (oid) oidcpy(output, oid); @@ -668,6 +672,7 @@ static void status_submodule_cb(const struct cache_entry *list_item, void *cb_data) { struct status_cb *info = cb_data; + status_submodule(list_item->name, &list_item->oid, list_item->ce_flags, info->prefix, info->flags); } @@ -820,6 +825,7 @@ static void generate_submodule_summary(struct summary_cb *info, if (!info->cached && oideq(&p->oid_dst, null_oid())) { if (S_ISGITLINK(p->mod_dst)) { struct ref_store *refs = get_submodule_ref_store(p->sm_path); + if (refs) refs_head_ref(refs, handle_submodule_head_ref, &p->oid_dst); } else if (S_ISLNK(p->mod_dst) || S_ISREG(p->mod_dst)) { @@ -1158,6 +1164,7 @@ static void sync_submodule(const char *path, const char *prefix, if (starts_with_dot_dot_slash(sub->url) || starts_with_dot_slash(sub->url)) { char *up_path = get_up_path(path); + sub_origin_url = resolve_relative_url(sub->url, up_path, 1); super_config_url = resolve_relative_url(sub->url, NULL, 1); free(up_path); @@ -1233,6 +1240,7 @@ cleanup: static void sync_submodule_cb(const struct cache_entry *list_item, void *cb_data) { struct sync_cb *info = cb_data; + sync_submodule(list_item->name, info->prefix, info->flags); } @@ -1312,6 +1320,7 @@ static void deinit_submodule(const char *path, const char *prefix, if (!(flags & OPT_FORCE)) { struct child_process cp_rm = CHILD_PROCESS_INIT; + cp_rm.git_cmd = 1; strvec_pushl(&cp_rm.args, "rm", "-qn", path, NULL); @@ -1348,6 +1357,7 @@ static void deinit_submodule(const char *path, const char *prefix, /* remove the .git/config entries (unless the user already did it) */ if (!capture_command(&cp_config, &sb_config, 0) && sb_config.len) { char *sub_key = xstrfmt("submodule.%s", sub->name); + /* * remove the whole section so we have a clean state when * the user later decides to init this submodule again @@ -1585,6 +1595,7 @@ static int clone_submodule(struct module_clone_data *clone_data) strvec_pushl(&cp.args, "--depth", clone_data->depth, NULL); if (clone_data->reference.nr) { struct string_list_item *item; + for_each_string_list_item(item, &clone_data->reference) strvec_pushl(&cp.args, "--reference", item->string, NULL); @@ -1935,6 +1946,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, strvec_pushl(&child->args, "--url", url, NULL); if (suc->update_data->references.nr) { struct string_list_item *item; + for_each_string_list_item(item, &suc->update_data->references) strvec_pushl(&child->args, "--reference", item->string, NULL); } @@ -1967,6 +1979,7 @@ static int update_clone_get_next_task(struct child_process *child, ce = suc->update_data->list.entries[suc->current]; if (prepare_to_clone_next_submodule(ce, child, suc, err)) { int *p = xmalloc(sizeof(*p)); + *p = suc->current; *idx_task_cb = p; suc->current++; @@ -1982,6 +1995,7 @@ static int update_clone_get_next_task(struct child_process *child, index = suc->current - suc->update_data->list.nr; if (index < suc->failed_clones_nr) { int *p; + ce = suc->failed_clones[index]; if (!prepare_to_clone_next_submodule(ce, child, suc, err)) { suc->current ++; @@ -2005,6 +2019,7 @@ static int update_clone_start_failure(struct strbuf *err, void *idx_task_cb) { struct submodule_update_clone *suc = suc_cb; + suc->quickstop = 1; return 1; } @@ -2016,9 +2031,9 @@ static int update_clone_task_finished(int result, { const struct cache_entry *ce; struct submodule_update_clone *suc = suc_cb; - int *idxP = idx_task_cb; int idx = *idxP; + free(idxP); if (!result) @@ -2051,6 +2066,7 @@ static int git_update_clone_config(const char *var, const char *value, void *cb) { int *max_jobs = cb; + if (!strcmp(var, "submodule.fetchjobs")) *max_jobs = parse_submodule_fetchjobs(var, value); return 0; @@ -2091,6 +2107,7 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str if (oid) { char *hex = oid_to_hex(oid); char *remote = get_default_remote(); + strvec_pushl(&cp.args, remote, hex, NULL); free(remote); } @@ -2340,6 +2357,7 @@ static void update_data_to_args(struct update_data *update_data, struct strvec * if (update_data->references.nr) { struct string_list_item *item; + for_each_string_list_item(item, &update_data->references) strvec_pushl(args, "--reference", item->string, NULL); } @@ -2872,8 +2890,10 @@ static void append_fetch_remotes(struct strbuf *msg, const char *git_dir_path) if (!capture_command(&cp_remote, &sb_remote_out, 0)) { char *next_line; char *line = sb_remote_out.buf; + while ((next_line = strchr(line, '\n')) != NULL) { size_t len = next_line - line; + if (strip_suffix_mem(line, &len, " (fetch)")) strbuf_addf(msg, " %.*s\n", (int)len, line); line = next_line + 1; @@ -3180,6 +3200,7 @@ static int module_add(int argc, const char **argv, const char *prefix) int exit_code = -1; struct strbuf sb = STRBUF_INIT; struct child_process cp = CHILD_PROCESS_INIT; + cp.git_cmd = 1; cp.no_stdout = 1; strvec_pushl(&cp.args, "add", "--dry-run", "--ignore-missing", From 4b82d75b51eec202dc3d2b64e8de2ad9cb52d638 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:53 +0200 Subject: [PATCH 11/33] submodule--helper: replace memset() with { 0 }-initialization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use the less verbose { 0 }-initialization syntax rather than memset() in builtin/submodule--helper.c, this doesn't make a difference in terms of behavior, but as we're about to modify adjacent code makes this more consistent, and lets us avoid worrying about when the memset() happens v.s. a "goto cleanup". Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 15bf89910f..60165a848a 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1664,7 +1664,7 @@ 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; + struct list_objects_filter_options filter_options = { 0 }; struct option module_clone_options[] = { OPT_STRING(0, "prefix", &clone_data.prefix, N_("path"), @@ -1704,7 +1704,6 @@ static int module_clone(int argc, const char **argv, const char *prefix) NULL }; - memset(&filter_options, 0, sizeof(filter_options)); argc = parse_options(argc, argv, prefix, module_clone_options, git_submodule_helper_usage, 0); @@ -2488,7 +2487,7 @@ static int module_update(int argc, const char **argv, const char *prefix) { struct pathspec pathspec; struct update_data opt = UPDATE_DATA_INIT; - 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), @@ -2546,7 +2545,6 @@ 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); - memset(&filter_options, 0, sizeof(filter_options)); argc = parse_options(argc, argv, prefix, module_update_options, git_submodule_helper_usage, 0); From 21496b4c60b2327417c63fc9762096037a185be3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:54 +0200 Subject: [PATCH 12/33] submodule--helper: use xstrfmt() in clone_submodule() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 60165a848a..63008970f1 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1568,12 +1568,11 @@ static int clone_submodule(struct module_clone_data *clone_data) sm_gitdir = absolute_pathdup(sb.buf); strbuf_reset(&sb); - if (!is_absolute_path(clone_data->path)) { - 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(), + clone_data->path); + else 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 " @@ -1625,14 +1624,16 @@ 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); From 9bdf5277d5989d128469847b7b88ff20077e6d43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:55 +0200 Subject: [PATCH 13/33] submodule--helper: move "sb" in clone_submodule() to its own scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 63008970f1..fe32abd45e 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1557,17 +1557,25 @@ static void prepare_possible_alternates(const char *sm_name, free(error_strategy); } +static char *clone_submodule_sm_gitdir(const char *name) +{ + struct strbuf sb = STRBUF_INIT; + char *sm_gitdir; + + submodule_name_to_gitdir(&sb, the_repository, name); + sm_gitdir = absolute_pathdup(sb.buf); + strbuf_release(&sb); + + return sm_gitdir; +} + static int clone_submodule(struct module_clone_data *clone_data) { - char *p, *sm_gitdir; + char *p; + char *sm_gitdir = clone_submodule_sm_gitdir(clone_data->name); char *sm_alternate = NULL, *error_strategy = NULL; - struct strbuf sb = STRBUF_INIT; struct child_process cp = CHILD_PROCESS_INIT; - submodule_name_to_gitdir(&sb, the_repository, clone_data->name); - sm_gitdir = absolute_pathdup(sb.buf); - strbuf_reset(&sb); - if (!is_absolute_path(clone_data->path)) clone_data->path = xstrfmt("%s/%s", get_git_work_tree(), clone_data->path); @@ -1655,7 +1663,6 @@ static int clone_submodule(struct module_clone_data *clone_data) free(sm_alternate); free(error_strategy); - strbuf_release(&sb); free(sm_gitdir); free(p); return 0; From 6fac5b2f352efc8c246d6d5be63a66b7b0fc0209 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:56 +0200 Subject: [PATCH 14/33] submodule--helper: add "const" to passed "module_clone_data" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add "const" to the "struct module_clone_data" that we pass to clone_submodule(), which makes the ownership clear, and stops us from clobbering the "clone_data->path". We still need to add to the "reference" member, which is a "struct 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 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 "goto cleanup" let's declare it at the start of the function. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 49 ++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index fe32abd45e..6b4ee8a31b 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1434,7 +1434,6 @@ 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; @@ -1442,7 +1441,6 @@ struct module_clone_data { int single_branch; }; #define MODULE_CLONE_DATA_INIT { \ - .reference = STRING_LIST_INIT_NODUP, \ .single_branch = -1, \ } @@ -1569,18 +1567,20 @@ static char *clone_submodule_sm_gitdir(const char *name) return sm_gitdir; } -static int clone_submodule(struct module_clone_data *clone_data) +static int clone_submodule(const struct module_clone_data *clone_data, + struct string_list *reference) { char *p; char *sm_gitdir = clone_submodule_sm_gitdir(clone_data->name); char *sm_alternate = NULL, *error_strategy = NULL; struct child_process cp = CHILD_PROCESS_INIT; + const char *clone_data_path; 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); if (validate_submodule_git_dir(sm_gitdir, clone_data->name) < 0) die(_("refusing to create/use '%s' in another submodule's " @@ -1590,7 +1590,7 @@ 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); - prepare_possible_alternates(clone_data->name, &clone_data->reference); + prepare_possible_alternates(clone_data->name, reference); strvec_push(&cp.args, "clone"); strvec_push(&cp.args, "--no-checkout"); @@ -1600,10 +1600,10 @@ static int clone_submodule(struct module_clone_data *clone_data) strvec_push(&cp.args, "--progress"); if (clone_data->depth && *(clone_data->depth)) strvec_pushl(&cp.args, "--depth", clone_data->depth, NULL); - 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", item->string, NULL); } @@ -1622,7 +1622,7 @@ static int clone_submodule(struct module_clone_data *clone_data) strvec_push(&cp.args, "--"); strvec_push(&cp.args, clone_data->url); - strvec_push(&cp.args, clone_data->path); + strvec_push(&cp.args, clone_data_path); cp.git_cmd = 1; prepare_submodule_repo_env(&cp.env); @@ -1630,25 +1630,25 @@ static int clone_submodule(struct module_clone_data *clone_data) if(run_command(&cp)) die(_("clone of '%s' into submodule path '%s' failed"), - clone_data->url, clone_data->path); + 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); + 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); path = xstrfmt("%s/index", sm_gitdir); unlink_or_warn(path); free(path); } - connect_work_tree_and_git_dir(clone_data->path, sm_gitdir, 0); + connect_work_tree_and_git_dir(clone_data_path, sm_gitdir, 0); - p = git_pathdup_submodule(clone_data->path, "config"); + p = git_pathdup_submodule(clone_data_path, "config"); if (!p) - die(_("could not get submodule directory for '%s'"), clone_data->path); + die(_("could not get submodule directory for '%s'"), clone_data_path); /* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */ git_config_get_string("submodule.alternateLocation", &sm_alternate); @@ -1673,6 +1673,7 @@ 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"), @@ -1686,7 +1687,7 @@ 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, @@ -1725,7 +1726,7 @@ 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, &reference); list_objects_filter_release(&filter_options); return 0; } @@ -2913,6 +2914,7 @@ 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)) { @@ -2929,6 +2931,7 @@ 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)) { @@ -2968,13 +2971,13 @@ 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, &reference)) return -1; prepare_submodule_repo_env(&cp.env); From 1da635b84d438d46f1a87ee49bea416513193785 Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Thu, 1 Sep 2022 01:17:57 +0200 Subject: [PATCH 15/33] submodule--helper: add "const" to copy of "update_data" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a "const" to the copy of "struct update_data" that's tracked by the "struct submodule_update_clone", as it neither owns nor modifies it. Signed-off-by: Glen Choo Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 6b4ee8a31b..1391316cde 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1777,7 +1777,7 @@ struct submodule_update_clone { int current; /* configuration parameters which are passed on to the children */ - struct update_data *update_data; + const struct update_data *update_data; /* to be consumed by update_submodule() */ struct update_clone_data *update_clone; @@ -1862,7 +1862,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, const char *update_string; enum submodule_update_type update_type; char *key; - struct update_data *ud = suc->update_data; + const struct update_data *ud = suc->update_data; char *displaypath = get_submodule_displaypath(ce->name, ud->prefix); struct strbuf sb = STRBUF_INIT; int needs_cloning = 0; From a253be682f96bcb66ea427b9f7421f16dc063676 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:17:58 +0200 Subject: [PATCH 16/33] submodule--helper: add "const" to passed "struct update_data" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a "const" to the "struct update_data" passed to run_update_procedure(), which it in turn passes along (peeled) to is_tip_reachable() and fetch_in_submodule()). Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 1391316cde..7e70f49547 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2080,7 +2080,7 @@ static int git_update_clone_config(const char *var, const char *value, return 0; } -static int is_tip_reachable(const char *path, struct object_id *oid) +static int is_tip_reachable(const char *path, const struct object_id *oid) { struct child_process cp = CHILD_PROCESS_INIT; struct strbuf rev = STRBUF_INIT; @@ -2099,7 +2099,8 @@ static int is_tip_reachable(const char *path, struct object_id *oid) return 1; } -static int fetch_in_submodule(const char *module_path, int depth, int quiet, struct object_id *oid) +static int fetch_in_submodule(const char *module_path, int depth, int quiet, + const struct object_id *oid) { struct child_process cp = CHILD_PROCESS_INIT; @@ -2123,7 +2124,7 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str return run_command(&cp); } -static int run_update_command(struct update_data *ud, int subforce) +static int run_update_command(const struct update_data *ud, int subforce) { struct child_process cp = CHILD_PROCESS_INIT; char *oid = oid_to_hex(&ud->oid); @@ -2220,7 +2221,7 @@ static int run_update_command(struct update_data *ud, int subforce) return 0; } -static int run_update_procedure(struct update_data *ud) +static int run_update_procedure(const struct update_data *ud) { int subforce = is_null_oid(&ud->suboid) || ud->force; @@ -2331,7 +2332,8 @@ static const char *submodule_update_type_to_label(enum submodule_update_type typ BUG("unreachable with type %d", type); } -static void update_data_to_args(struct update_data *update_data, struct strvec *args) +static void update_data_to_args(const struct update_data *update_data, + struct strvec *args) { enum submodule_update_type update_type = update_data->update_default; From 9d02f9499ffe7af9ce7e449e482820d437f84026 Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Thu, 1 Sep 2022 01:17:59 +0200 Subject: [PATCH 17/33] submodule--helper: refactor "errmsg_str" to be a "struct strbuf" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactor code added in e83e3333b57 (submodule: port submodule subcommand 'summary' from shell to C, 2020-08-13) so that "errmsg" and "errmsg_str" are folded into one. The distinction between the empty string and NULL is something that's tested for by e.g. "t/t7401-submodule-summary.sh". This is in preparation for fixing a memory leak the "struct strbuf" in the pre-image. Let's also pass a "const char *" to print_submodule_summary(), as it should not be modifying the "errmsg". Signed-off-by: Glen Choo Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 7e70f49547..befcd2d0d9 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -761,7 +761,7 @@ static char *verify_submodule_committish(const char *sm_path, return strbuf_detach(&result, NULL); } -static void print_submodule_summary(struct summary_cb *info, char *errmsg, +static void print_submodule_summary(struct summary_cb *info, const char *errmsg, int total_commits, const char *displaypath, const char *src_abbrev, const char *dst_abbrev, struct module_cb *p) @@ -819,7 +819,7 @@ static void generate_submodule_summary(struct summary_cb *info, { char *displaypath, *src_abbrev = NULL, *dst_abbrev; int missing_src = 0, missing_dst = 0; - char *errmsg = NULL; + struct strbuf errmsg = STRBUF_INIT; int total_commits = -1; if (!info->cached && oideq(&p->oid_dst, null_oid())) { @@ -920,23 +920,21 @@ static void generate_submodule_summary(struct summary_cb *info, * submodule, i.e., deleted or changed to blob */ if (S_ISGITLINK(p->mod_dst)) { - struct strbuf errmsg_str = STRBUF_INIT; if (missing_src && missing_dst) { - strbuf_addf(&errmsg_str, " Warn: %s doesn't contain commits %s and %s\n", + strbuf_addf(&errmsg, " Warn: %s doesn't contain commits %s and %s\n", displaypath, oid_to_hex(&p->oid_src), oid_to_hex(&p->oid_dst)); } else { - strbuf_addf(&errmsg_str, " Warn: %s doesn't contain commit %s\n", + strbuf_addf(&errmsg, " Warn: %s doesn't contain commit %s\n", displaypath, missing_src ? oid_to_hex(&p->oid_src) : oid_to_hex(&p->oid_dst)); } - errmsg = strbuf_detach(&errmsg_str, NULL); } } - print_submodule_summary(info, errmsg, total_commits, - displaypath, src_abbrev, + print_submodule_summary(info, errmsg.len ? errmsg.buf : NULL, + total_commits, displaypath, src_abbrev, dst_abbrev, p); free(displaypath); From b0bff0be5463ebfb75a76ff8577b90ade439a437 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:00 +0200 Subject: [PATCH 18/33] submodule--helper: don't redundantly check "else if (res)" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "res" variable must be true at this point in update_submodule(), as just a few lines above this we've unconditionally: if (!res) return 0; So we don't need to guard the "return 1" with an "else if (res)", we can return unconditionally at this point. See b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15) for the initial introduction of this code, this check of "res" has always been redundant. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index befcd2d0d9..557834fa73 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2445,8 +2445,7 @@ static int update_submodule(struct update_data *update_data) update_data->displaypath); if (res == 128) exit(res); - else if (res) - return 1; + return 1; } return 0; From addda284cbcde90b8b541caf94f198ed2f6bd936 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:01 +0200 Subject: [PATCH 19/33] submodule--helper: rename "int res" to "int ret" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. 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 Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 557834fa73..d76ee0323e 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2426,7 +2426,7 @@ static int update_submodule(struct update_data *update_data) if (update_data->recursive) { struct child_process cp = CHILD_PROCESS_INIT; struct update_data next = *update_data; - int res; + int ret; next.prefix = NULL; oidcpy(&next.oid, null_oid()); @@ -2438,13 +2438,13 @@ static int update_submodule(struct update_data *update_data) update_data_to_args(&next, &cp.args); /* die() if child process die()'d */ - res = run_command(&cp); - if (!res) + ret = run_command(&cp); + if (!ret) return 0; die_message(_("Failed to recurse into submodule path '%s'"), update_data->displaypath); - if (res == 128) - exit(res); + if (ret == 128) + exit(ret); return 1; } @@ -2453,7 +2453,7 @@ 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; @@ -2471,7 +2471,7 @@ 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; } @@ -2483,12 +2483,12 @@ 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) From 0b917a9f5cb21ca1655daf549108361602e912f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:02 +0200 Subject: [PATCH 20/33] submodule--helper: return "ret", not "1" from update_submodule() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Amend the update_submodule() function to return the failing "ret" on error, instead of overriding it with "1". This code was added in b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15), and this change ends up not making a difference as this function is only called in update_submodules(). If we return non-zero here we'll always in turn return "1" in module_update(). But if we didn't do that and returned any other non-zero exit code in update_submodules() we'd fail the test that's being amended here. We're still testing the status quo here. This change makes subsequent refactoring of update_submodule() easier, as we'll no longer need to worry about clobbering the "ret" we get from the run_command(). Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 2 +- t/t7406-submodule-update.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index d76ee0323e..d6a827366d 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2445,7 +2445,7 @@ static int update_submodule(struct update_data *update_data) update_data->displaypath); if (ret == 128) exit(ret); - return 1; + return ret; } return 0; diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index 6cc07460dd..c5f5dbe55e 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -769,7 +769,7 @@ test_expect_success 'submodule update continues after recursive checkout error' echo "" > file ) ) && - test_must_fail git submodule update --recursive && + test_expect_code 1 git submodule update --recursive && (cd submodule2 && git rev-parse --verify HEAD >../actual ) && From 96a907376b10c42879dafc7f6b4842f8dbc04597 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:03 +0200 Subject: [PATCH 21/33] submodule--helper: add missing braces to "else" arm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add missing braces to an "else" arm in init_submodule(), this stylistic change makes this code conform to the CodingGuidelines, and makes a subsequent commit smaller. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index d6a827366d..f97923ff50 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -480,8 +480,9 @@ static void init_submodule(const char *path, const char *prefix, fprintf(stderr, _("warning: command update mode suggested for submodule '%s'\n"), sub->name); upd = xstrdup("none"); - } else + } else { upd = xstrdup(submodule_strategy_to_string(&sub->update_strategy)); + } if (git_config_set_gently(sb.buf, upd)) die(_("Failed to register update mode for submodule path '%s'"), displaypath); From 08c2e778d6b16296aebfccd609ad09db9d40764a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:04 +0200 Subject: [PATCH 22/33] submodule--helper: don't call submodule_strategy_to_string() in BUG() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Don't call submodule_strategy_to_string() in a BUG() message. These calls added in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24) don't need the extra information submodule_strategy_to_string() gives us, as we'll never reach the SM_UPDATE_COMMAND case here. That case is the only one where we'd get any information beyond the straightforward number-to-string mapping. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index f97923ff50..44c6dfdfbe 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2156,8 +2156,8 @@ static int run_update_command(const struct update_data *ud, int subforce) must_die_on_failure = 1; break; default: - BUG("unexpected update strategy type: %s", - submodule_strategy_to_string(&ud->update_strategy)); + BUG("unexpected update strategy type: %d", + ud->update_strategy.type); } strvec_push(&cp.args, oid); @@ -2182,8 +2182,8 @@ static int run_update_command(const struct update_data *ud, int subforce) ud->update_strategy.command, oid, ud->displaypath); break; default: - BUG("unexpected update strategy type: %s", - submodule_strategy_to_string(&ud->update_strategy)); + BUG("unexpected update strategy type: %d", + ud->update_strategy.type); } if (must_die_on_failure) exit(128); @@ -2213,8 +2213,8 @@ static int run_update_command(const struct update_data *ud, int subforce) ud->displaypath, ud->update_strategy.command, oid); break; default: - BUG("unexpected update strategy type: %s", - submodule_strategy_to_string(&ud->update_strategy)); + BUG("unexpected update strategy type: %d", + ud->update_strategy.type); } return 0; From b9dd63ffe2842fe676f54d4d5f8f06bc04c6dd8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:05 +0200 Subject: [PATCH 23/33] submodule API: don't handle SM_..{UNSPECIFIED,COMMAND} in to_string() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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(). 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 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 Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 10 +++++----- submodule.c | 12 +++++------- submodule.h | 2 +- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 44c6dfdfbe..a0505212cb 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -418,7 +418,8 @@ static void init_submodule(const char *path, const char *prefix, { const struct submodule *sub; struct strbuf sb = STRBUF_INIT; - char *upd = NULL, *url = NULL, *displaypath; + const char *upd; + char *url = NULL, *displaypath; displaypath = get_submodule_displaypath(path, prefix); @@ -474,14 +475,14 @@ static void init_submodule(const char *path, const char *prefix, /* Copy "update" setting when it is not set yet */ strbuf_addf(&sb, "submodule.%s.update", sub->name); - if (git_config_get_string(sb.buf, &upd) && + if (git_config_get_string_tmp(sb.buf, &upd) && sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) { if (sub->update_strategy.type == SM_UPDATE_COMMAND) { fprintf(stderr, _("warning: command update mode suggested for submodule '%s'\n"), sub->name); - upd = xstrdup("none"); + upd = "none"; } else { - upd = xstrdup(submodule_strategy_to_string(&sub->update_strategy)); + upd = submodule_update_type_to_string(sub->update_strategy.type); } if (git_config_set_gently(sb.buf, upd)) @@ -490,7 +491,6 @@ static void init_submodule(const char *path, const char *prefix, strbuf_release(&sb); free(displaypath); free(url); - free(upd); } static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data) diff --git a/submodule.c b/submodule.c index 3fa5db3ecd..1ebda30c50 100644 --- a/submodule.c +++ b/submodule.c @@ -415,10 +415,9 @@ int parse_submodule_update_strategy(const char *value, return 0; } -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) { + switch (type) { case SM_UPDATE_CHECKOUT: return "checkout"; case SM_UPDATE_MERGE: @@ -428,12 +427,11 @@ 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: - 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, diff --git a/submodule.h b/submodule.h index bfaa9da186..6a9fec6de1 100644 --- a/submodule.h +++ b/submodule.h @@ -72,7 +72,7 @@ void die_path_inside_submodule(struct index_state *istate, enum submodule_update_type parse_submodule_update_type(const char *value); 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, From 6870cdc32aaa2b5786b77898522d932cf592a7f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:06 +0200 Subject: [PATCH 24/33] submodule--helper: use "code" in run_update_command() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply some DRY principles in run_update_command() and don't have two "switch" statements over "ud->update_strategy.type" determine the same thing. First we were setting "must_die_on_failure = 1" in all cases except "SM_UPDATE_CHECKOUT" (and we'd BUG(...) out on the rest). This code was added in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24). Then we'd duplicate same "switch" logic when we were using the "must_die_on_failure" variable. Let's instead have the "case" branches in that inner "switch" determine whether or not the "update must continue" by picking an exit code. This also mostly avoids hardcoding the "128" exit code, instead we can make use of the return value of the die_message() function, which we've been calling here since 55b3f12cb54 (submodule update: use die_message(), 2022-03-15). We're still hardcoding it to determine if we "exit()", but subsequent commit(s) will address that. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index a0505212cb..35abb993d8 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2127,7 +2127,6 @@ static int run_update_command(const struct update_data *ud, int subforce) { struct child_process cp = CHILD_PROCESS_INIT; char *oid = oid_to_hex(&ud->oid); - int must_die_on_failure = 0; switch (ud->update_strategy.type) { case SM_UPDATE_CHECKOUT: @@ -2141,19 +2140,16 @@ static int run_update_command(const struct update_data *ud, int subforce) strvec_push(&cp.args, "rebase"); if (ud->quiet) strvec_push(&cp.args, "--quiet"); - must_die_on_failure = 1; break; case SM_UPDATE_MERGE: cp.git_cmd = 1; strvec_push(&cp.args, "merge"); if (ud->quiet) strvec_push(&cp.args, "--quiet"); - must_die_on_failure = 1; break; case SM_UPDATE_COMMAND: cp.use_shell = 1; strvec_push(&cp.args, ud->update_strategy.command); - must_die_on_failure = 1; break; default: BUG("unexpected update strategy type: %d", @@ -2164,32 +2160,35 @@ static int run_update_command(const struct update_data *ud, int subforce) cp.dir = xstrdup(ud->sm_path); prepare_submodule_repo_env(&cp.env); if (run_command(&cp)) { + int ret; + switch (ud->update_strategy.type) { case SM_UPDATE_CHECKOUT: die_message(_("Unable to checkout '%s' in submodule path '%s'"), oid, ud->displaypath); + /* the command failed, but update must continue */ + ret = 1; break; case SM_UPDATE_REBASE: - die_message(_("Unable to rebase '%s' in submodule path '%s'"), - oid, ud->displaypath); + ret = die_message(_("Unable to rebase '%s' in submodule path '%s'"), + oid, ud->displaypath); break; case SM_UPDATE_MERGE: - die_message(_("Unable to merge '%s' in submodule path '%s'"), - oid, ud->displaypath); + ret = die_message(_("Unable to merge '%s' in submodule path '%s'"), + oid, ud->displaypath); break; case SM_UPDATE_COMMAND: - die_message(_("Execution of '%s %s' failed in submodule path '%s'"), - ud->update_strategy.command, oid, ud->displaypath); + ret = die_message(_("Execution of '%s %s' failed in submodule path '%s'"), + ud->update_strategy.command, oid, ud->displaypath); break; default: BUG("unexpected update strategy type: %d", ud->update_strategy.type); } - if (must_die_on_failure) - exit(128); - /* the command failed, but update must continue */ - return 1; + if (ret == 128) + exit(ret); + return ret; } if (ud->quiet) From 2cb9294b99f62bb7439c99ba22898508ba4ff621 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:07 +0200 Subject: [PATCH 25/33] submodule--helper: don't exit() on failure, return MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change code downstream of module_update() to short-circuit and return to the top-level on failure, rather than calling exit(). To do so we need to diligently check whether we "must_die_on_failure", which is a pattern started in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24), but which hadn't been completed 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 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 Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 35abb993d8..1646caf82a 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2123,7 +2123,8 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, return run_command(&cp); } -static int run_update_command(const struct update_data *ud, int subforce) +static int run_update_command(const struct update_data *ud, int subforce, + int *must_die_on_failure) { struct child_process cp = CHILD_PROCESS_INIT; char *oid = oid_to_hex(&ud->oid); @@ -2187,7 +2188,7 @@ static int run_update_command(const struct update_data *ud, int subforce) } if (ret == 128) - exit(ret); + *must_die_on_failure = 1; return ret; } @@ -2219,7 +2220,8 @@ static int run_update_command(const struct update_data *ud, int subforce) return 0; } -static int run_update_procedure(const struct update_data *ud) +static int run_update_procedure(const struct update_data *ud, + int *must_die_on_failure) { int subforce = is_null_oid(&ud->suboid) || ud->force; @@ -2246,7 +2248,7 @@ static int run_update_procedure(const struct update_data *ud) ud->displaypath, oid_to_hex(&ud->oid)); } - return run_update_command(ud, subforce); + return run_update_command(ud, subforce, must_die_on_failure); } static const char *remote_submodule_branch(const char *path) @@ -2383,7 +2385,8 @@ static void update_data_to_args(const struct update_data *update_data, "--no-single-branch"); } -static int update_submodule(struct update_data *update_data) +static int update_submodule(struct update_data *update_data, + int *must_die_on_failure) { ensure_core_worktree(update_data->sm_path); @@ -2419,9 +2422,15 @@ static int update_submodule(struct update_data *update_data) free(remote_ref); } - 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 (*must_die_on_failure) + return ret; + if (ret) return 1; + } if (update_data->recursive) { struct child_process cp = CHILD_PROCESS_INIT; @@ -2437,14 +2446,13 @@ 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) - exit(ret); + *must_die_on_failure = 1; return ret; } @@ -2477,12 +2485,19 @@ 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 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)) + code = update_submodule(update_data, &must_die_on_failure); + if (code) + ret = code; + if (must_die_on_failure) + goto cleanup; + else if (code) ret = 1; } From 484f9150e6eda45ba3ebdb1e64568bb294f791c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:08 +0200 Subject: [PATCH 26/33] submodule--helper: libify determine_submodule_update_strategy() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Libify the determine_submodule_update_strategy() by having it invoke die_message() rather than die(), and returning the code die_message() returns on failure. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 39 ++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 1646caf82a..78614743ed 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1730,24 +1730,27 @@ static int module_clone(int argc, const char **argv, const char *prefix) return 0; } -static void determine_submodule_update_strategy(struct repository *r, - int just_cloned, - const char *path, - enum submodule_update_type update, - struct submodule_update_strategy *out) +static int determine_submodule_update_strategy(struct repository *r, + int just_cloned, + const char *path, + enum submodule_update_type update, + struct submodule_update_strategy *out) { const struct submodule *sub = submodule_from_path(r, null_oid(), path); char *key; const char *val; + int ret; key = xstrfmt("submodule.%s.update", sub->name); if (update) { out->type = update; } else if (!repo_config_get_string_tmp(r, key, &val)) { - if (parse_submodule_update_strategy(val, out) < 0) - die(_("Invalid update mode '%s' configured for submodule path '%s'"), - val, path); + if (parse_submodule_update_strategy(val, out) < 0) { + ret = die_message(_("Invalid update mode '%s' configured for submodule path '%s'"), + val, path); + goto cleanup; + } } else if (sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) { if (sub->update_strategy.type == SM_UPDATE_COMMAND) BUG("how did we read update = !command from .gitmodules?"); @@ -1762,7 +1765,10 @@ static void determine_submodule_update_strategy(struct repository *r, out->type == SM_UPDATE_NONE)) out->type = SM_UPDATE_CHECKOUT; + ret = 0; +cleanup: free(key); + return ret; } struct update_clone_data { @@ -2388,14 +2394,22 @@ static void update_data_to_args(const struct update_data *update_data, static int update_submodule(struct update_data *update_data, int *must_die_on_failure) { + int ret; + ensure_core_worktree(update_data->sm_path); update_data->displaypath = get_submodule_displaypath( update_data->sm_path, update_data->prefix); - determine_submodule_update_strategy(the_repository, update_data->just_cloned, - update_data->sm_path, update_data->update_default, - &update_data->update_strategy); + ret = determine_submodule_update_strategy(the_repository, + update_data->just_cloned, + update_data->sm_path, + update_data->update_default, + &update_data->update_strategy); + if (ret) { + *must_die_on_failure = 1; + return ret; + } if (update_data->just_cloned) oidcpy(&update_data->suboid, null_oid()); @@ -2423,8 +2437,6 @@ static int update_submodule(struct update_data *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 (*must_die_on_failure) return ret; @@ -2435,7 +2447,6 @@ static int update_submodule(struct update_data *update_data, if (update_data->recursive) { struct child_process cp = CHILD_PROCESS_INIT; struct update_data next = *update_data; - int ret; next.prefix = NULL; oidcpy(&next.oid, null_oid()); From d905d4432ff2d027847f57bf963df4c441def4ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:09 +0200 Subject: [PATCH 27/33] submodule--helper: libify "must_die_on_failure" code paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In preceding commits the codepaths around update_submodules() were changed from using exit() or die() to ferrying up a "must_die_on_failure" in the cases where we'd exit(), and in most cases where we'd die(). We needed to do this this to ensure that we'd early exit or otherwise abort the update_submodules() processing before it was completed. Now that those preceding changes have shown that we've converted those paths, we can remove the remaining "ret == 128" special-cases, leaving the only such special-case in update_submodules(). I.e. we now know 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 Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 45 +++++++++++++------------------------ 1 file changed, 16 insertions(+), 29 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 78614743ed..a39b93f274 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2129,8 +2129,7 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, return run_command(&cp); } -static int run_update_command(const struct update_data *ud, int subforce, - int *must_die_on_failure) +static int run_update_command(const struct update_data *ud, int subforce) { struct child_process cp = CHILD_PROCESS_INIT; char *oid = oid_to_hex(&ud->oid); @@ -2193,8 +2192,6 @@ static int run_update_command(const struct update_data *ud, int subforce, ud->update_strategy.type); } - if (ret == 128) - *must_die_on_failure = 1; return ret; } @@ -2226,8 +2223,7 @@ static int run_update_command(const struct update_data *ud, int subforce, return 0; } -static int run_update_procedure(const struct update_data *ud, - int *must_die_on_failure) +static int run_update_procedure(const struct update_data *ud) { int subforce = is_null_oid(&ud->suboid) || ud->force; @@ -2254,7 +2250,7 @@ static int run_update_procedure(const struct update_data *ud, ud->displaypath, oid_to_hex(&ud->oid)); } - return run_update_command(ud, subforce, must_die_on_failure); + return run_update_command(ud, subforce); } static const char *remote_submodule_branch(const char *path) @@ -2391,8 +2387,7 @@ static void update_data_to_args(const struct update_data *update_data, "--no-single-branch"); } -static int update_submodule(struct update_data *update_data, - int *must_die_on_failure) +static int update_submodule(struct update_data *update_data) { int ret; @@ -2406,10 +2401,8 @@ 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()); @@ -2437,11 +2430,9 @@ static int update_submodule(struct update_data *update_data, } if (!oideq(&update_data->oid, &update_data->suboid) || update_data->force) { - ret = run_update_procedure(update_data, must_die_on_failure); - if (*must_die_on_failure) - return ret; + ret = run_update_procedure(update_data); if (ret) - return 1; + return ret; } if (update_data->recursive) { @@ -2458,12 +2449,9 @@ static int update_submodule(struct update_data *update_data, update_data_to_args(&next, &cp.args); ret = run_command(&cp); - if (!ret) - return 0; - die_message(_("Failed to recurse into submodule path '%s'"), - update_data->displaypath); - if (ret == 128) - *must_die_on_failure = 1; + if (ret) + die_message(_("Failed to recurse into submodule path '%s'"), + update_data->displaypath); return ret; } @@ -2496,20 +2484,19 @@ 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 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; - code = update_submodule(update_data, &must_die_on_failure); - if (code) - ret = code; - if (must_die_on_failure) + code = update_submodule(update_data); + if (!code) + continue; + ret = code; + if (ret == 128) goto cleanup; - else if (code) - ret = 1; + ret = 1; } cleanup: From a03c01de2fbb1e83138d83095ddacd1417f49b66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:10 +0200 Subject: [PATCH 28/33] submodule--helper update: don't override 'checkout' exit code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When "git submodule update" runs it might call "checkout", "merge", "rebase", or a custom command. Ever since run_update_command() was added in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24) we'd either exit immediately if the "submodule..update" method failed, or in the case of "checkout" continue trying to update other submodules. This code used to use the magical "2" return code, but in 55b3f12cb54 (submodule update: use die_message(), 2022-03-15) it was made to exit(128), which in preceding commits has been changed to return that 128 code to the top-level. Let's "libify" this code even more by not having it arbitrarily override the return code. In practice this doesn't change anything as the code "git checkout" would return on any normal failure is "1", but we'll now in principle properly abort the operation if "git checkout" were to exit with 128. It would make sense to follow-up this change with a change to allow the "submodule..update = !..." (SM_UPDATE_COMMAND) method the same liberties as "checkout", and perhaps to do the same with a failed "merge" or "rebase". But let's leave that for now. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index a39b93f274..66c81c1e58 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2133,6 +2133,7 @@ static int run_update_command(const struct update_data *ud, int subforce) { struct child_process cp = CHILD_PROCESS_INIT; char *oid = oid_to_hex(&ud->oid); + int ret; switch (ud->update_strategy.type) { case SM_UPDATE_CHECKOUT: @@ -2165,15 +2166,12 @@ static int run_update_command(const struct update_data *ud, int subforce) cp.dir = xstrdup(ud->sm_path); prepare_submodule_repo_env(&cp.env); - if (run_command(&cp)) { - int ret; - + if ((ret = run_command(&cp))) { switch (ud->update_strategy.type) { case SM_UPDATE_CHECKOUT: die_message(_("Unable to checkout '%s' in submodule path '%s'"), oid, ud->displaypath); - /* the command failed, but update must continue */ - ret = 1; + /* No "ret" assignment, use "git checkout"'s */ break; case SM_UPDATE_REBASE: ret = die_message(_("Unable to rebase '%s' in submodule path '%s'"), @@ -2496,7 +2494,6 @@ static int update_submodules(struct update_data *update_data) ret = code; if (ret == 128) goto cleanup; - ret = 1; } cleanup: From ac350155de34767e132b383fb1b819ff74e23842 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:11 +0200 Subject: [PATCH 29/33] submodule--helper: libify "must_die_on_failure" code paths (for die) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Continue the libification of codepaths that previously relied on "must_die_on_failure". In these cases we've always been early aborting by calling die(), but as we know that these codepaths will properly handle return codes of 128 to mean an early abort let's have them use die_message() instead. This still isn't a complete migration away from die() for these codepaths, in particular this code in update_submodule() will still call die() in some cases: char *remote_name = get_default_remote_submodule(update_data->sm_path); 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 a subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 66c81c1e58..bfcc8e2c99 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2243,9 +2243,9 @@ static int run_update_procedure(const struct update_data *ud) */ if (!is_tip_reachable(ud->sm_path, &ud->oid) && fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, &ud->oid)) - die(_("Fetched in submodule path '%s', but it did not " - "contain %s. Direct fetching of that commit failed."), - ud->displaypath, oid_to_hex(&ud->oid)); + return die_message(_("Fetched in submodule path '%s', but it did not " + "contain %s. Direct fetching of that commit failed."), + ud->displaypath, oid_to_hex(&ud->oid)); } return run_update_command(ud, subforce); @@ -2289,13 +2289,14 @@ static const char *remote_submodule_branch(const char *path) return branch; } -static void ensure_core_worktree(const char *path) +static int ensure_core_worktree(const char *path) { const char *cw; struct repository subrepo; if (repo_submodule_init(&subrepo, the_repository, path, null_oid())) - die(_("could not get a repository handle for submodule '%s'"), path); + return die_message(_("could not get a repository handle for submodule '%s'"), + path); if (!repo_config_get_string_tmp(&subrepo, "core.worktree", &cw)) { char *cfg_file, *abs_path; @@ -2313,6 +2314,8 @@ static void ensure_core_worktree(const char *path) free(abs_path); strbuf_release(&sb); } + + return 0; } static const char *submodule_update_type_to_label(enum submodule_update_type type) @@ -2389,7 +2392,9 @@ static int update_submodule(struct update_data *update_data) { int ret; - ensure_core_worktree(update_data->sm_path); + ret = ensure_core_worktree(update_data->sm_path); + if (ret) + return ret; update_data->displaypath = get_submodule_displaypath( update_data->sm_path, update_data->prefix); @@ -2405,8 +2410,8 @@ static int update_submodule(struct update_data *update_data) if (update_data->just_cloned) oidcpy(&update_data->suboid, null_oid()); else if (resolve_gitlink_ref(update_data->sm_path, "HEAD", &update_data->suboid)) - die(_("Unable to find current revision in submodule path '%s'"), - update_data->displaypath); + return die_message(_("Unable to find current revision in submodule path '%s'"), + update_data->displaypath); if (update_data->remote) { char *remote_name = get_default_remote_submodule(update_data->sm_path); @@ -2416,13 +2421,13 @@ static int update_submodule(struct update_data *update_data) if (!update_data->nofetch) { if (fetch_in_submodule(update_data->sm_path, update_data->depth, 0, NULL)) - die(_("Unable to fetch in submodule path '%s'"), - update_data->sm_path); + return die_message(_("Unable to fetch in submodule path '%s'"), + update_data->sm_path); } if (resolve_gitlink_ref(update_data->sm_path, remote_ref, &update_data->oid)) - die(_("Unable to find %s revision in submodule path '%s'"), - remote_ref, update_data->sm_path); + return die_message(_("Unable to find %s revision in submodule path '%s'"), + remote_ref, update_data->sm_path); free(remote_ref); } From 1e8697b5c4ed7c2762a0858251efa8feb8126038 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:12 +0200 Subject: [PATCH 30/33] submodule--helper: check repo{_submodule,}_init() return values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix code added in ce125d431aa (submodule: extract path to submodule gitdir func, 2021-09-15) and a77c3fcb5ec (submodule--helper: get remote names from any repository, 2022-03-04) which failed to check the return values of repo_init() and repo_submodule_init(). If we failed to initialize the repository or submodule we could segfault when trying to access the invalid repository structs. Let's also check that these were the only such logic errors in the codebase by making use of the "warn_unused_result" attribute. This is valid as of GCC 3.4.0 (and clang will catch it via its faking of __GNUC__ ). As the comment being added to git-compat-util.h we're piggy-backing on the LAST_ARG_MUST_BE_NULL version check out of lazyness. See 9fe3edc47f1 (Add the LAST_ARG_MUST_BE_NULL macro, 2013-07-18) for its addition. The marginal benefit of covering gcc 3.4.0..4.0.0 is near-zero (or zero) at this point. It mostly matters that we catch this somewhere. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 9 +++++++-- git-compat-util.h | 3 +++ repository.h | 3 +++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index bfcc8e2c99..7f0a39286f 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -63,7 +63,10 @@ static char *get_default_remote_submodule(const char *module_path) { struct repository subrepo; - repo_submodule_init(&subrepo, the_repository, module_path, null_oid()); + if (repo_submodule_init(&subrepo, the_repository, module_path, + null_oid()) < 0) + die(_("could not get a repository handle for submodule '%s'"), + module_path); return repo_get_default_remote(&subrepo); } @@ -1480,7 +1483,9 @@ static int add_possible_reference_from_superproject( struct strbuf err = STRBUF_INIT; strbuf_add(&sb, odb->path, len); - repo_init(&alternate, sb.buf, NULL); + if (repo_init(&alternate, sb.buf, NULL) < 0) + die(_("could not get a repository handle for gitdir '%s'"), + sb.buf); /* * We need to end the new path with '/' to mark it as a dir, diff --git a/git-compat-util.h b/git-compat-util.h index 58d7708296..3eb7785bdd 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -565,8 +565,11 @@ static inline int git_has_dir_sep(const char *path) /* The sentinel attribute is valid from gcc version 4.0 */ #if defined(__GNUC__) && (__GNUC__ >= 4) #define LAST_ARG_MUST_BE_NULL __attribute__((sentinel)) +/* warn_unused_result exists as of gcc 3.4.0, but be lazy and check 4.0 */ +#define RESULT_MUST_BE_USED __attribute__ ((warn_unused_result)) #else #define LAST_ARG_MUST_BE_NULL +#define RESULT_MUST_BE_USED #endif #define MAYBE_UNUSED __attribute__((__unused__)) diff --git a/repository.h b/repository.h index 6cc661e5a4..17c45ae096 100644 --- a/repository.h +++ b/repository.h @@ -1,6 +1,7 @@ #ifndef REPOSITORY_H #define REPOSITORY_H +#include "git-compat-util.h" #include "path.h" struct config_set; @@ -185,6 +186,7 @@ void repo_set_gitdir(struct repository *repo, const char *root, void repo_set_worktree(struct repository *repo, const char *path); void repo_set_hash_algo(struct repository *repo, int algo); void initialize_the_repository(void); +RESULT_MUST_BE_USED int repo_init(struct repository *r, const char *gitdir, const char *worktree); /* @@ -196,6 +198,7 @@ int repo_init(struct repository *r, const char *gitdir, const char *worktree); * Return 0 upon success and a non-zero value upon failure. */ struct object_id; +RESULT_MUST_BE_USED int repo_submodule_init(struct repository *subrepo, struct repository *superproject, const char *path, From f5373deabd537daae3d7c34f6dfa4b62ed45be51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:13 +0200 Subject: [PATCH 31/33] submodule--helper: libify more "die" paths for module_update() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As noted in a preceding commit the get_default_remote_submodule() and remote_submodule_branch() functions would invoke die(), and thus leave update_submodule() only partially lib-ified. Let's address the former of those cases. Change the functions to return an int exit code (non-zero on failure), while leaving the get_default_remote() function for the callers that still want the die() semantics. This change addresses 1/2 of the "die" issue in these two lines in update_submodule(): char *remote_name = get_default_remote_submodule(update_data->sm_path); const char *branch = remote_submodule_branch(update_data->sm_path); We can safely remove the "!default_remote" case from sync_submodule(), because our get_default_remote_submodule() function now returns a die_message() on failure, so we can have it and other callers check if the exit code should be non-zero instead. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 58 +++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 7f0a39286f..1445b76c29 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -31,48 +31,57 @@ typedef void (*each_submodule_fn)(const struct cache_entry *list_item, void *cb_data); -static char *repo_get_default_remote(struct repository *repo) +static int repo_get_default_remote(struct repository *repo, char **default_remote) { - char *dest = NULL, *ret; + char *dest = NULL; struct strbuf sb = STRBUF_INIT; struct ref_store *store = get_main_ref_store(repo); const char *refname = refs_resolve_ref_unsafe(store, "HEAD", 0, NULL, NULL); if (!refname) - die(_("No such ref: %s"), "HEAD"); + return die_message(_("No such ref: %s"), "HEAD"); /* detached HEAD */ - if (!strcmp(refname, "HEAD")) - return xstrdup("origin"); + if (!strcmp(refname, "HEAD")) { + *default_remote = xstrdup("origin"); + return 0; + } if (!skip_prefix(refname, "refs/heads/", &refname)) - die(_("Expecting a full ref name, got %s"), refname); + return die_message(_("Expecting a full ref name, got %s"), + refname); strbuf_addf(&sb, "branch.%s.remote", refname); if (repo_config_get_string(repo, sb.buf, &dest)) - ret = xstrdup("origin"); + *default_remote = xstrdup("origin"); else - ret = dest; + *default_remote = dest; strbuf_release(&sb); - return ret; + return 0; } -static char *get_default_remote_submodule(const char *module_path) +static int get_default_remote_submodule(const char *module_path, char **default_remote) { struct repository subrepo; if (repo_submodule_init(&subrepo, the_repository, module_path, null_oid()) < 0) - die(_("could not get a repository handle for submodule '%s'"), - module_path); - return repo_get_default_remote(&subrepo); + return die_message(_("could not get a repository handle for submodule '%s'"), + module_path); + return repo_get_default_remote(&subrepo, default_remote); } static char *get_default_remote(void) { - return repo_get_default_remote(the_repository); + char *default_remote; + int code = repo_get_default_remote(the_repository, &default_remote); + + if (code) + exit(code); + + return default_remote; } static char *resolve_relative_url(const char *rel_url, const char *up_path, int quiet) @@ -1156,6 +1165,7 @@ static void sync_submodule(const char *path, const char *prefix, char *sub_origin_url, *super_config_url, *displaypath, *default_remote; struct strbuf sb = STRBUF_INIT; char *sub_config_path = NULL; + int code; if (!is_submodule_active(the_repository, path)) return; @@ -1195,10 +1205,9 @@ static void sync_submodule(const char *path, const char *prefix, goto cleanup; strbuf_reset(&sb); - default_remote = get_default_remote_submodule(path); - if (!default_remote) - die(_("failed to get the default remote for submodule '%s'"), - path); + code = get_default_remote_submodule(path, &default_remote); + if (code) + exit(code); remote_key = xstrfmt("remote.%s.url", default_remote); free(default_remote); @@ -2419,9 +2428,16 @@ static int update_submodule(struct update_data *update_data) update_data->displaypath); if (update_data->remote) { - char *remote_name = get_default_remote_submodule(update_data->sm_path); - const char *branch = remote_submodule_branch(update_data->sm_path); - char *remote_ref = xstrfmt("refs/remotes/%s/%s", remote_name, branch); + char *remote_name; + const char *branch; + char *remote_ref; + int code; + + code = get_default_remote_submodule(update_data->sm_path, &remote_name); + if (code) + return code; + branch = remote_submodule_branch(update_data->sm_path); + remote_ref = xstrfmt("refs/remotes/%s/%s", remote_name, branch); if (!update_data->nofetch) { if (fetch_in_submodule(update_data->sm_path, update_data->depth, From 86e16ed3a9fbf03bc8a5d4030177980193e30f57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:14 +0200 Subject: [PATCH 32/33] submodule--helper: libify even more "die" paths for module_update() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As noted in a preceding commit the get_default_remote_submodule() and remote_submodule_branch() functions would invoke die(), and thus leave update_submodule() only partially lib-ified. We've addressed the former of those in a preceding commit, let's now address the latter. In addition to lib-ifying the function this fixes a potential (but obscure) segfault introduced by a logic error in 1012a5cbc3f (submodule--helper run-update-procedure: learn --remote, 2022-03-04): We were assuming that remote_submodule_branch() would always return non-NULL, but if the submodule_from_path() call in that function fails we'll return NULL. See its introduction in 92bbe7ccf1f (submodule--helper: add remote-branch helper, 2016-08-03). I.e. we'd previously have segfaulted in the xstrfmt() call in update_submodule() seen in the context. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 41 ++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 1445b76c29..3afaddb9a5 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2265,42 +2265,49 @@ static int run_update_procedure(const struct update_data *ud) return run_update_command(ud, subforce); } -static const char *remote_submodule_branch(const char *path) +static int remote_submodule_branch(const char *path, const char **branch) { const struct submodule *sub; - const char *branch = NULL; char *key; + *branch = NULL; sub = submodule_from_path(the_repository, null_oid(), path); if (!sub) - return NULL; + return die_message(_("could not initialize submodule at path '%s'"), + path); key = xstrfmt("submodule.%s.branch", sub->name); - if (repo_config_get_string_tmp(the_repository, key, &branch)) - branch = sub->branch; + if (repo_config_get_string_tmp(the_repository, key, branch)) + *branch = sub->branch; free(key); - if (!branch) - return "HEAD"; + if (!*branch) { + *branch = "HEAD"; + return 0; + } - if (!strcmp(branch, ".")) { + if (!strcmp(*branch, ".")) { const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL); if (!refname) - die(_("No such ref: %s"), "HEAD"); + return die_message(_("No such ref: %s"), "HEAD"); /* detached HEAD */ if (!strcmp(refname, "HEAD")) - die(_("Submodule (%s) branch configured to inherit " - "branch from superproject, but the superproject " - "is not on any branch"), sub->name); + return die_message(_("Submodule (%s) branch configured to inherit " + "branch from superproject, but the superproject " + "is not on any branch"), sub->name); if (!skip_prefix(refname, "refs/heads/", &refname)) - die(_("Expecting a full ref name, got %s"), refname); - return refname; + return die_message(_("Expecting a full ref name, got %s"), + refname); + + *branch = refname; + return 0; } - return branch; + /* Our "branch" is coming from repo_config_get_string_tmp() */ + return 0; } static int ensure_core_worktree(const char *path) @@ -2436,7 +2443,9 @@ static int update_submodule(struct update_data *update_data) code = get_default_remote_submodule(update_data->sm_path, &remote_name); if (code) return code; - branch = remote_submodule_branch(update_data->sm_path); + code = remote_submodule_branch(update_data->sm_path, &branch); + if (code) + return code; remote_ref = xstrfmt("refs/remotes/%s/%s", remote_name, branch); if (!update_data->nofetch) { From d4a492f4ad0455710a7db8c589f4865810b0d0b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 1 Sep 2022 01:18:15 +0200 Subject: [PATCH 33/33] submodule--helper: fix bad config API usage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix bad config API usage added in a452128a36c (submodule--helper: introduce add-config subcommand, 2021-08-06). After git_config_get_string() returns successfully we know the "char **dest" will be non-NULL. A coccinelle patch that transforms this turns up a couple of other such issues, one in fetch-pack.c, and another in upload-pack.c: @@ identifier F =~ "^(repo|git)_config_get_string(_tmp)?$"; identifier V; @@ !F(..., &V) - && (V) But let's focus narrowly on submodule--helper for now, we can fix those some other time. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 3afaddb9a5..3a2a92da51 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -3107,7 +3107,7 @@ static void configure_added_submodule(struct add_data *add_data) * is_submodule_active(), since that function needs to find * out the value of "submodule.active" again anyway. */ - if (!git_config_get_string("submodule.active", &val) && val) { + if (!git_config_get_string("submodule.active", &val)) { /* * If the submodule being added isn't already covered by the * current configured pathspec, set the submodule's active flag