From aca8568e2c342b1764837df29468ed498b5e6dcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 16:13:49 -0800 Subject: submodule tests: test for init and update failure output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Amend some submodule tests to test for the failure output of "git submodule [update|init]". The lack of such tests hid a regression in an earlier version of a subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- t/t7406-submodule-update.sh | 14 ++++++++++++-- t/t7408-submodule-reference.sh | 14 +++++++++++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index 11cccbb333..7764c1c3cb 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -205,8 +205,18 @@ test_expect_success 'submodule update should fail due to local changes' ' (cd submodule && compare_head ) && - test_must_fail git submodule update submodule - ) + test_must_fail git submodule update submodule 2>../actual.raw + ) && + sed "s/^> //" >expect <<-\EOF && + > error: Your local changes to the following files would be overwritten by checkout: + > file + > Please commit your changes or stash them before you switch branches. + > Aborting + > fatal: Unable to checkout OID in submodule path '\''submodule'\'' + EOF + sed -e "s/checkout $SQ[^$SQ]*$SQ/checkout OID/" actual && + test_cmp expect actual + ' test_expect_success 'submodule update should throw away changes with --force ' ' (cd super && diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh index a3892f494b..c3a4545510 100755 --- a/t/t7408-submodule-reference.sh +++ b/t/t7408-submodule-reference.sh @@ -193,7 +193,19 @@ test_expect_success 'missing nested submodule alternate fails clone and submodul cd supersuper-clone && check_that_two_of_three_alternates_are_used && # update of the submodule fails - test_must_fail git submodule update --init --recursive + cat >expect <<-\EOF && + fatal: submodule '\''sub'\'' cannot add alternate: path ... does not exist + Failed to clone '\''sub'\''. Retry scheduled + fatal: submodule '\''sub-dissociate'\'' cannot add alternate: path ... does not exist + Failed to clone '\''sub-dissociate'\''. Retry scheduled + fatal: submodule '\''sub'\'' cannot add alternate: path ... does not exist + Failed to clone '\''sub'\'' a second time, aborting + fatal: Failed to recurse into submodule path ... + EOF + test_must_fail git submodule update --init --recursive 2>err && + grep -e fatal: -e ^Failed err >actual.raw && + sed -e "s/path $SQ[^$SQ]*$SQ/path .../" actual && + test_cmp expect actual ) ' -- cgit v1.2.3 From f7bdb3291873f0c10ad63015b18d1caf1ec719d7 Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Fri, 4 Mar 2022 16:13:50 -0800 Subject: submodule--helper: remove update-module-mode This is dead code - it has not been used since c51f8f94e5 (submodule--helper: run update procedures from C, 2021-08-24). Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index eeacefcc38..c11ee1ea2b 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1957,29 +1957,6 @@ static void determine_submodule_update_strategy(struct repository *r, free(key); } -static int module_update_module_mode(int argc, const char **argv, const char *prefix) -{ - const char *path, *update = NULL; - int just_cloned; - struct submodule_update_strategy update_strategy = { .type = SM_UPDATE_CHECKOUT }; - - if (argc < 3 || argc > 4) - die("submodule--helper update-module-clone expects []"); - - just_cloned = git_config_int("just_cloned", argv[1]); - path = argv[2]; - - if (argc == 4) - update = argv[3]; - - determine_submodule_update_strategy(the_repository, - just_cloned, path, update, - &update_strategy); - fputs(submodule_strategy_to_string(&update_strategy), stdout); - - return 0; -} - struct update_clone_data { const struct submodule *sub; struct object_id oid; @@ -3430,7 +3407,6 @@ static struct cmd_struct commands[] = { {"name", module_name, 0}, {"clone", module_clone, 0}, {"add", module_add, SUPPORT_SUPER_PREFIX}, - {"update-module-mode", module_update_module_mode, 0}, {"update-clone", update_clone, 0}, {"run-update-procedure", run_update_procedure, 0}, {"ensure-core-worktree", ensure_core_worktree, 0}, -- cgit v1.2.3 From 1a0b78c9537571c18bafc4ac6e3fc16fe6b63c84 Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Fri, 4 Mar 2022 16:13:51 -0800 Subject: submodule--helper: reorganize code for sh to C conversion Introduce a function, update_submodule2(), that will implement the functionality of run-update-procedure and its surrounding shell code in submodule.sh. This name is temporary; it will replace update_submodule() when the sh to C conversion is complete. Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index c11ee1ea2b..1b67a3887c 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2452,6 +2452,16 @@ static int do_run_update_procedure(struct update_data *ud) return run_update_command(ud, subforce); } +/* + * NEEDSWORK: As we convert "git submodule update" to C, + * update_submodule2() will invoke more and more functions, making it + * difficult to preserve the function ordering without forward + * declarations. + * + * When the conversion is complete, this forward declaration will be + * unnecessary and should be removed. + */ +static int update_submodule2(struct update_data *update_data); static void update_submodule(struct update_clone_data *ucd) { fprintf(stdout, "dummy %s %d\t%s\n", @@ -2618,11 +2628,7 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix) &update_data.update_strategy); free(prefixed_path); - - if (!oideq(&update_data.oid, &update_data.suboid) || update_data.force) - return do_run_update_procedure(&update_data); - - return 3; + return update_submodule2(&update_data); } static int resolve_relative_path(int argc, const char **argv, const char *prefix) @@ -3022,6 +3028,16 @@ static int module_create_branch(int argc, const char **argv, const char *prefix) force, reflog, quiet, track, dry_run); return 0; } + +/* NEEDSWORK: this is a temporary name until we delete update_submodule() */ +static int update_submodule2(struct update_data *update_data) +{ + if (!oideq(&update_data->oid, &update_data->suboid) || update_data->force) + return do_run_update_procedure(update_data); + + return 3; +} + struct add_data { const char *prefix; const char *branch; -- cgit v1.2.3 From e44196659634fdc42672888f5a91e94303052284 Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Fri, 4 Mar 2022 16:13:52 -0800 Subject: submodule--helper run-update-procedure: remove --suboid Teach run-update-procedure to determine the oid of the submodule's HEAD instead of doing it in git-submodule.sh. Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 9 ++++++--- git-submodule.sh | 8 +------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 1b67a3887c..77ca4270f4 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2594,9 +2594,6 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix) OPT_CALLBACK_F(0, "oid", &update_data.oid, N_("sha1"), N_("SHA1 expected by superproject"), PARSE_OPT_NONEG, parse_opt_object_id), - OPT_CALLBACK_F(0, "suboid", &update_data.suboid, N_("subsha1"), - N_("SHA1 of submodule's HEAD"), PARSE_OPT_NONEG, - parse_opt_object_id), OPT_END() }; @@ -3032,6 +3029,12 @@ static int module_create_branch(int argc, const char **argv, const char *prefix) /* NEEDSWORK: this is a temporary name until we delete update_submodule() */ static int update_submodule2(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); + if (!oideq(&update_data->oid, &update_data->suboid) || update_data->force) return do_run_update_procedure(update_data); diff --git a/git-submodule.sh b/git-submodule.sh index 87772ac891..32a09302ab 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -406,14 +406,9 @@ cmd_update() displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix") - if test $just_cloned -eq 1 + if test $just_cloned -eq 0 then - subsha1= - else just_cloned= - subsha1=$(sanitize_submodule_env; cd "$sm_path" && - git rev-parse --verify HEAD) || - die "fatal: $(eval_gettext "Unable to find current revision in submodule path '\$displaypath'")" fi if test -n "$remote" @@ -441,7 +436,6 @@ cmd_update() ${update:+--update "$update"} \ ${prefix:+--recursive-prefix "$prefix"} \ ${sha1:+--oid "$sha1"} \ - ${subsha1:+--suboid "$subsha1"} \ "--" \ "$sm_path") -- cgit v1.2.3 From a77c3fcb5ec535507e1e71da4f642151f65b4b17 Mon Sep 17 00:00:00 2001 From: Atharva Raykar Date: Fri, 4 Mar 2022 16:13:53 -0800 Subject: submodule--helper: get remote names from any repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `get_default_remote()` retrieves the name of a remote by resolving the refs from of the current repository's ref store. Thus in order to use it for retrieving the remote name of a submodule, we have to start a new subprocess which runs from the submodule directory. Let's instead introduce a function called `repo_get_default_remote()` which takes any repository object and retrieves the remote accordingly. `get_default_remote()` is then defined as a call to `repo_get_default_remote()` with 'the_repository' passed to it. Now that we have `repo_get_default_remote()`, we no longer have to start a subprocess that called `submodule--helper get-default-remote` from within the submodule directory. So let's make a function called `get_default_remote_submodule()` which takes a submodule path, and returns the default remote for that submodule, all within the same process. We can now use this function to save an unnecessary subprocess spawn in `sync_submodule()`, and also in a subsequent patch, which will require this functionality. Mentored-by: Christian Couder Mentored-by: Shourya Shukla Helped-by: Glen Choo Signed-off-by: Atharva Raykar Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 77ca4270f4..b5a2d83029 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -31,11 +31,13 @@ typedef void (*each_submodule_fn)(const struct cache_entry *list_item, void *cb_data); -static char *get_default_remote(void) +static char *repo_get_default_remote(struct repository *repo) { char *dest = NULL, *ret; struct strbuf sb = STRBUF_INIT; - const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL); + 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"); @@ -48,7 +50,7 @@ static char *get_default_remote(void) die(_("Expecting a full ref name, got %s"), refname); strbuf_addf(&sb, "branch.%s.remote", refname); - if (git_config_get_string(sb.buf, &dest)) + if (repo_config_get_string(repo, sb.buf, &dest)) ret = xstrdup("origin"); else ret = dest; @@ -57,6 +59,19 @@ static char *get_default_remote(void) return ret; } +static char *get_default_remote_submodule(const char *module_path) +{ + struct repository subrepo; + + repo_submodule_init(&subrepo, the_repository, module_path, null_oid()); + return repo_get_default_remote(&subrepo); +} + +static char *get_default_remote(void) +{ + return repo_get_default_remote(the_repository); +} + static int print_default_remote(int argc, const char **argv, const char *prefix) { char *remote; @@ -1343,9 +1358,8 @@ static void sync_submodule(const char *path, const char *prefix, { const struct submodule *sub; char *remote_key = NULL; - char *sub_origin_url, *super_config_url, *displaypath; + char *sub_origin_url, *super_config_url, *displaypath, *default_remote; struct strbuf sb = STRBUF_INIT; - struct child_process cp = CHILD_PROCESS_INIT; char *sub_config_path = NULL; if (!is_submodule_active(the_repository, path)) @@ -1384,21 +1398,15 @@ static void sync_submodule(const char *path, const char *prefix, if (!is_submodule_populated_gently(path, NULL)) goto cleanup; - prepare_submodule_repo_env(&cp.env_array); - cp.git_cmd = 1; - cp.dir = path; - strvec_pushl(&cp.args, "submodule--helper", - "print-default-remote", NULL); - strbuf_reset(&sb); - if (capture_command(&cp, &sb, 0)) + default_remote = get_default_remote_submodule(path); + if (!default_remote) die(_("failed to get the default remote for submodule '%s'"), path); - strbuf_strip_suffix(&sb, "\n"); - remote_key = xstrfmt("remote.%s.url", sb.buf); + remote_key = xstrfmt("remote.%s.url", default_remote); + free(default_remote); - strbuf_reset(&sb); submodule_to_gitdir(&sb, path); strbuf_addstr(&sb, "/config"); -- cgit v1.2.3 From ed9c84853e99e2dbbb2b49898ca7badfe1686474 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 4 Mar 2022 16:13:54 -0800 Subject: submodule--helper: don't use bitfield indirection for parse_options() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Do away with the indirection of local variables added in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24). These were only needed because in C you can't get a pointer to a single bit, so we were using intermediate variables instead. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index b5a2d83029..3832dccae5 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2023,10 +2023,10 @@ struct update_data { struct object_id suboid; struct submodule_update_strategy update_strategy; int depth; - unsigned int force: 1; - unsigned int quiet: 1; - unsigned int nofetch: 1; - unsigned int just_cloned: 1; + unsigned int force; + unsigned int quiet; + unsigned int nofetch; + unsigned int just_cloned; }; #define UPDATE_DATA_INIT { .update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT } @@ -2578,16 +2578,17 @@ static int update_clone(int argc, const char **argv, const char *prefix) static int run_update_procedure(int argc, const char **argv, const char *prefix) { - int force = 0, quiet = 0, nofetch = 0, just_cloned = 0; char *prefixed_path, *update = NULL; struct update_data update_data = UPDATE_DATA_INIT; struct option options[] = { - OPT__QUIET(&quiet, N_("suppress output for update by rebase or merge")), - OPT__FORCE(&force, N_("force checkout updates"), 0), - OPT_BOOL('N', "no-fetch", &nofetch, + OPT__QUIET(&update_data.quiet, + N_("suppress output for update by rebase or merge")), + OPT__FORCE(&update_data.force, N_("force checkout updates"), + 0), + OPT_BOOL('N', "no-fetch", &update_data.nofetch, N_("don't fetch new objects from the remote site")), - OPT_BOOL(0, "just-cloned", &just_cloned, + OPT_BOOL(0, "just-cloned", &update_data.just_cloned, N_("overrides update mode in case the repository is a fresh clone")), OPT_INTEGER(0, "depth", &update_data.depth, N_("depth for shallow fetch")), OPT_STRING(0, "prefix", &prefix, @@ -2615,10 +2616,6 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix) if (argc != 1) usage_with_options(usage, options); - update_data.force = !!force; - update_data.quiet = !!quiet; - update_data.nofetch = !!nofetch; - update_data.just_cloned = !!just_cloned; update_data.sm_path = argv[0]; if (update_data.recursive_prefix) -- cgit v1.2.3 From 1012a5cbc3fe4bcfae278a8103e60054d2674784 Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Fri, 4 Mar 2022 16:13:55 -0800 Subject: submodule--helper run-update-procedure: learn --remote Teach run-update-procedure to handle --remote instead of parsing --remote in git-submodule.sh. As a result, "git submodule--helper [print-default-remote|remote-branch]" have no more callers, so remove them. Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 56 ++++++++++++++++++--------------------------- git-submodule.sh | 30 +----------------------- 2 files changed, 23 insertions(+), 63 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 3832dccae5..f673f7ebbf 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -72,21 +72,6 @@ static char *get_default_remote(void) return repo_get_default_remote(the_repository); } -static int print_default_remote(int argc, const char **argv, const char *prefix) -{ - char *remote; - - if (argc != 1) - die(_("submodule--helper print-default-remote takes no arguments")); - - remote = get_default_remote(); - if (remote) - printf("%s\n", remote); - - free(remote); - return 0; -} - static int starts_with_dot_slash(const char *str) { return str[0] == '.' && is_dir_sep(str[1]); @@ -2027,6 +2012,7 @@ struct update_data { unsigned int quiet; unsigned int nofetch; unsigned int just_cloned; + unsigned int remote; }; #define UPDATE_DATA_INIT { .update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT } @@ -2603,6 +2589,8 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix) OPT_CALLBACK_F(0, "oid", &update_data.oid, N_("sha1"), N_("SHA1 expected by superproject"), PARSE_OPT_NONEG, parse_opt_object_id), + OPT_BOOL(0, "remote", &update_data.remote, + N_("use SHA-1 of submodule's remote tracking branch")), OPT_END() }; @@ -2682,23 +2670,6 @@ static const char *remote_submodule_branch(const char *path) return branch; } -static int resolve_remote_submodule_branch(int argc, const char **argv, - const char *prefix) -{ - const char *ret; - struct strbuf sb = STRBUF_INIT; - if (argc != 2) - die("submodule--helper remote-branch takes exactly one arguments, got %d", argc); - - ret = remote_submodule_branch(argv[1]); - if (!ret) - die("submodule %s doesn't exist", argv[1]); - - printf("%s", ret); - strbuf_release(&sb); - return 0; -} - static int push_check(int argc, const char **argv, const char *prefix) { struct remote *remote; @@ -3040,6 +3011,25 @@ static int update_submodule2(struct update_data *update_data) die(_("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); + const char *branch = remote_submodule_branch(update_data->sm_path); + char *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, + 0, NULL)) + die(_("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); + + free(remote_ref); + } + if (!oideq(&update_data->oid, &update_data->suboid) || update_data->force) return do_run_update_procedure(update_data); @@ -3439,11 +3429,9 @@ static struct cmd_struct commands[] = { {"foreach", module_foreach, SUPPORT_SUPER_PREFIX}, {"init", module_init, SUPPORT_SUPER_PREFIX}, {"status", module_status, SUPPORT_SUPER_PREFIX}, - {"print-default-remote", print_default_remote, 0}, {"sync", module_sync, SUPPORT_SUPER_PREFIX}, {"deinit", module_deinit, 0}, {"summary", module_summary, SUPPORT_SUPER_PREFIX}, - {"remote-branch", resolve_remote_submodule_branch, 0}, {"push-check", push_check, 0}, {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX}, {"is-active", is_active, 0}, diff --git a/git-submodule.sh b/git-submodule.sh index 32a09302ab..882bf097d5 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -247,20 +247,6 @@ cmd_deinit() git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- "$@" } -# usage: fetch_in_submodule [] [] -# Because arguments are positional, use an empty string to omit -# but include . -fetch_in_submodule () ( - sanitize_submodule_env && - cd "$1" && - if test $# -eq 3 - then - echo "$3" | git fetch ${GIT_QUIET:+--quiet} --stdin ${2:+"$2"} - else - git fetch ${GIT_QUIET:+--quiet} ${2:+"$2"} - fi -) - # # Update each submodule path to correct revision, using clone and checkout as needed # @@ -411,21 +397,6 @@ cmd_update() just_cloned= fi - if test -n "$remote" - then - branch=$(git submodule--helper remote-branch "$sm_path") - if test -z "$nofetch" - then - # Fetch remote before determining tracking $sha1 - fetch_in_submodule "$sm_path" $depth || - die "fatal: $(eval_gettext "Unable to fetch in submodule path '\$sm_path'")" - fi - remote_name=$(sanitize_submodule_env; cd "$sm_path" && git submodule--helper print-default-remote) - sha1=$(sanitize_submodule_env; cd "$sm_path" && - git rev-parse --verify "${remote_name}/${branch}") || - die "fatal: $(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")" - fi - out=$(git submodule--helper run-update-procedure \ ${wt_prefix:+--prefix "$wt_prefix"} \ ${GIT_QUIET:+--quiet} \ @@ -436,6 +407,7 @@ cmd_update() ${update:+--update "$update"} \ ${prefix:+--recursive-prefix "$prefix"} \ ${sha1:+--oid "$sha1"} \ + ${remote:+--remote} \ "--" \ "$sm_path") -- cgit v1.2.3 From 5312a850b882d5e4b61b3590a151d41d9a1e8767 Mon Sep 17 00:00:00 2001 From: Atharva Raykar Date: Fri, 4 Mar 2022 16:13:56 -0800 Subject: submodule--helper: refactor get_submodule_displaypath() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We create a function called `do_get_submodule_displaypath()` that generates the display path required by several submodule functions, and takes a custom superprefix parameter, instead of reading it from the environment. We then redefine the existing `get_submodule_displaypath()` function as a call to this new function, where the superprefix is obtained from the environment. Mentored-by: Christian Couder Mentored-by: Shourya Shukla Signed-off-by: Atharva Raykar Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index f673f7ebbf..52d4f47ffc 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -247,11 +247,10 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr return 0; } -/* the result should be freed by the caller. */ -static char *get_submodule_displaypath(const char *path, const char *prefix) +static char *do_get_submodule_displaypath(const char *path, + const char *prefix, + const char *super_prefix) { - const char *super_prefix = get_super_prefix(); - if (prefix && super_prefix) { BUG("cannot have prefix '%s' and superprefix '%s'", prefix, super_prefix); @@ -267,6 +266,13 @@ static char *get_submodule_displaypath(const char *path, const char *prefix) } } +/* the result should be freed by the caller. */ +static char *get_submodule_displaypath(const char *path, const char *prefix) +{ + const char *super_prefix = get_super_prefix(); + return do_get_submodule_displaypath(path, prefix, super_prefix); +} + static char *compute_rev_name(const char *sub_path, const char* object_id) { struct strbuf sb = STRBUF_INIT; -- cgit v1.2.3 From 3ce52cba5b283af1f5dbebfe43aeea5d3421dac6 Mon Sep 17 00:00:00 2001 From: Atharva Raykar Date: Fri, 4 Mar 2022 16:13:57 -0800 Subject: submodule--helper: allow setting superprefix for init_submodule() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We allow callers of the `init_submodule()` function to optionally override the superprefix from the environment. We need to enable this option because in our conversion of the update command that will follow, the '--init' option will be handled through this API. We will need to change the superprefix at that time to ensure the display paths show correctly in the output messages. Mentored-by: Christian Couder Mentored-by: Shourya Shukla Signed-off-by: Atharva Raykar Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 52d4f47ffc..c6df64bf6a 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -594,18 +594,22 @@ static int module_foreach(int argc, const char **argv, const char *prefix) struct init_cb { const char *prefix; + const char *superprefix; unsigned int flags; }; #define INIT_CB_INIT { 0 } static void init_submodule(const char *path, const char *prefix, - unsigned int flags) + const char *superprefix, unsigned int flags) { const struct submodule *sub; struct strbuf sb = STRBUF_INIT; char *upd = NULL, *url = NULL, *displaypath; - displaypath = get_submodule_displaypath(path, prefix); + /* try superprefix from the environment, if it is not passed explicitly */ + if (!superprefix) + superprefix = get_super_prefix(); + displaypath = do_get_submodule_displaypath(path, prefix, superprefix); sub = submodule_from_path(the_repository, null_oid(), path); @@ -679,7 +683,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); + init_submodule(list_item->name, info->prefix, info->superprefix, info->flags); } static int module_init(int argc, const char **argv, const char *prefix) -- cgit v1.2.3 From 29a5e9e1ffeadb0555fdd2f366f1cb7259462ab9 Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Fri, 4 Mar 2022 16:13:58 -0800 Subject: submodule--helper update-clone: learn --init Teach "git submodule--helper update-clone" the --init flag and remove the corresponding shell code. When the `--init` flag is passed to the subcommand, we do not spawn a new subprocess and call `submodule--helper init` on the submodule paths, because the Git machinery is not able to pick up the configuration changes introduced by that init call. So we instead run the `init_submodule_cb()` callback over each submodule in the same process. [1] https://lore.kernel.org/git/CAP8UFD0NCQ5w_3GtT_xHr35i7h8BuLX4UcHNY6VHPGREmDVObA@mail.gmail.com/ Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 26 ++++++++++++++++++++++++++ git-submodule.sh | 9 +++------ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index c6df64bf6a..17dabf3d12 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2000,6 +2000,7 @@ struct submodule_update_clone { int failed_clones_nr, failed_clones_alloc; int max_jobs; + unsigned int init; }; #define SUBMODULE_UPDATE_CLONE_INIT { \ .list = MODULE_LIST_INIT, \ @@ -2509,6 +2510,8 @@ static int update_clone(int argc, const char **argv, const char *prefix) int ret; struct option module_update_clone_options[] = { + OPT_BOOL(0, "init", &suc.init, + N_("initialize uninitialized submodules before update")), OPT_STRING(0, "prefix", &prefix, N_("path"), N_("path into the working tree")), @@ -2567,6 +2570,29 @@ static int update_clone(int argc, const char **argv, const char *prefix) if (pathspec.nr) suc.warn_if_uninitialized = 1; + if (suc.init) { + struct module_list list = MODULE_LIST_INIT; + struct init_cb info = INIT_CB_INIT; + + if (module_list_compute(argc, argv, suc.prefix, + &pathspec, &list) < 0) + return 1; + + /* + * If there are no path args and submodule.active is set then, + * by default, only initialize 'active' modules. + */ + if (!argc && git_config_get_value_multi("submodule.active")) + module_list_active(&list); + + info.prefix = suc.prefix; + info.superprefix = suc.recursive_prefix; + if (suc.quiet) + info.flags |= OPT_QUIET; + + for_each_listed_submodule(&list, init_submodule_cb, &info); + } + ret = update_submodules(&suc); list_objects_filter_release(&filter_options); return ret; diff --git a/git-submodule.sh b/git-submodule.sh index 882bf097d5..16dea0ca59 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -361,14 +361,11 @@ cmd_update() usage fi - if test -n "$init" - then - cmd_init "--" "$@" || return - fi - { - git submodule--helper update-clone ${GIT_QUIET:+--quiet} \ + git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update-clone \ + ${GIT_QUIET:+--quiet} \ ${progress:+"--progress"} \ + ${init:+--init} \ ${wt_prefix:+--prefix "$wt_prefix"} \ ${prefix:+--recursive-prefix "$prefix"} \ ${update:+--update "$update"} \ -- cgit v1.2.3 From 97cb977c8243c3393a85d662456a1e161596f211 Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Fri, 4 Mar 2022 16:13:59 -0800 Subject: submodule--helper: remove ensure-core-worktree Move the logic of "git submodule--helper ensure-core-worktree" into run-update-procedure, and since this makes the ensure-core-worktree command obsolete, remove it. As a result, the order of two operations in git-submodule.sh is reversed: 'set the value of core.worktree' now happens after the call to "git submodule--helper relative-path". This is safe - "relative-path" does not depend on the value of core.worktree. Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 12 ++---------- git-submodule.sh | 2 -- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 17dabf3d12..296ab80bf2 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2783,17 +2783,11 @@ static int push_check(int argc, const char **argv, const char *prefix) return 0; } -static int ensure_core_worktree(int argc, const char **argv, const char *prefix) +static void ensure_core_worktree(const char *path) { - const char *path; const char *cw; struct repository subrepo; - if (argc != 2) - BUG("submodule--helper ensure-core-worktree "); - - path = argv[1]; - if (repo_submodule_init(&subrepo, the_repository, path, null_oid())) die(_("could not get a repository handle for submodule '%s'"), path); @@ -2813,8 +2807,6 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix) free(abs_path); strbuf_release(&sb); } - - return 0; } static int absorb_git_dirs(int argc, const char **argv, const char *prefix) @@ -3041,6 +3033,7 @@ static int module_create_branch(int argc, const char **argv, const char *prefix) /* NEEDSWORK: this is a temporary name until we delete update_submodule() */ static int update_submodule2(struct update_data *update_data) { + ensure_core_worktree(update_data->sm_path); if (update_data->just_cloned) oidcpy(&update_data->suboid, null_oid()); else if (resolve_gitlink_ref(update_data->sm_path, "HEAD", &update_data->suboid)) @@ -3459,7 +3452,6 @@ static struct cmd_struct commands[] = { {"add", module_add, SUPPORT_SUPER_PREFIX}, {"update-clone", update_clone, 0}, {"run-update-procedure", run_update_procedure, 0}, - {"ensure-core-worktree", ensure_core_worktree, 0}, {"relative-path", resolve_relative_path, 0}, {"resolve-relative-url-test", resolve_relative_url_test, 0}, {"foreach", module_foreach, SUPPORT_SUPER_PREFIX}, diff --git a/git-submodule.sh b/git-submodule.sh index 16dea0ca59..51be7c7f7e 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -385,8 +385,6 @@ cmd_update() do die_if_unmatched "$quickabort" "$sha1" - git submodule--helper ensure-core-worktree "$sm_path" || exit 1 - displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix") if test $just_cloned -eq 0 -- cgit v1.2.3 From 104744f91d9d04763ab2df06308bfa3c0b464f84 Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Fri, 4 Mar 2022 16:14:00 -0800 Subject: submodule update: add tests for --filter Test the "--filter" option to make sure we don't break anything while refactoring "git submodule update". Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- t/t7406-submodule-update.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index 7764c1c3cb..000e055811 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -1071,4 +1071,16 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s ) ' +test_expect_success 'submodule update --filter requires --init' ' + test_expect_code 129 git -C super submodule update --filter blob:none +' + +test_expect_success 'submodule update --filter sets partial clone settings' ' + test_when_finished "rm -rf super-filter" && + git clone cloned super-filter && + git -C super-filter submodule update --init --filter blob:none && + test_cmp_config -C super-filter/submodule true remote.origin.promisor && + test_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter +' + test_done -- cgit v1.2.3 From c9d256249375c7b8a1773139791448860b5789ff Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Fri, 4 Mar 2022 16:14:01 -0800 Subject: submodule--helper update-clone: check for --filter and --init "git submodule update --filter" also requires the "--init" option. Teach update-clone to do this usage check in C and remove the check from git-submodule.sh. In addition, change update-clone's usage string so that it teaches users about "git submodule update" instead of "git submodule--helper update-clone" (the string is copied from git-submodule.sh). This should be more helpful to users since they don't invoke update-clone directly. Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 20 +++++++++++++++++++- git-submodule.sh | 5 ----- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 296ab80bf2..bef9ab22d4 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2545,7 +2545,12 @@ static int update_clone(int argc, const char **argv, const char *prefix) }; const char *const git_submodule_helper_usage[] = { - N_("git submodule--helper update-clone [--prefix=] [...]"), + N_("git submodule [--quiet] update" + " [--init [--filter=]] [--remote]" + " [-N|--no-fetch] [-f|--force]" + " [--checkout|--merge|--rebase]" + " [--[no-]recommend-shallow] [--reference ]" + " [--recursive] [--[no-]single-branch] [--] [...]"), NULL }; suc.prefix = prefix; @@ -2556,6 +2561,19 @@ static int update_clone(int argc, const char **argv, const char *prefix) memset(&filter_options, 0, sizeof(filter_options)); argc = parse_options(argc, argv, prefix, module_update_clone_options, git_submodule_helper_usage, 0); + + if (filter_options.choice && !suc.init) { + /* + * NEEDSWORK: Don't use usage_with_options() because the + * usage string is for "git submodule update", but the + * options are for "git submodule--helper update-clone". + * + * This will no longer be an issue when "update-clone" + * is replaced by "git submodule--helper update". + */ + usage(git_submodule_helper_usage[0]); + } + suc.filter_options = &filter_options; if (update) diff --git a/git-submodule.sh b/git-submodule.sh index 51be7c7f7e..aa8bdfca9d 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -356,11 +356,6 @@ cmd_update() shift done - if test -n "$filter" && test "$init" != "1" - then - usage - fi - { git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update-clone \ ${GIT_QUIET:+--quiet} \ -- cgit v1.2.3