[PATCH 00/13] submodule: convert the rest of 'update' to C

STALE1684d

Revision v1 of 3 in this series.

122 messages, 5 authors, 2022-02-17 · page 2 of 2 · open the first message on its own page

[PATCH v6 03/16] submodule--helper: allow setting superprefix for init_submodule()

From: Glen Choo <hidden>
Date: 2022-02-08 08:40:23

From: Atharva Raykar <redacted>

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 [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Signed-off-by: Atharva Raykar <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 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 5efceb9d46..09cda67c1e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -606,18 +606,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);
 
@@ -691,7 +695,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)
-- 
2.33.GIT

[PATCH v6 05/16] builtin/submodule--helper.c: reformat designated initializers

From: Glen Choo <hidden>
Date: 2022-02-08 08:40:23

From: Atharva Raykar <redacted>

The second hunk here will make a subsequent commit's diff smaller, and
let's do the first and third hunks while we're at it so that we
consistently format all of these.

Signed-off-by: Atharva Raykar <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 builtin/submodule--helper.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index db71e6f4ec..9f79bdf4d5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1654,7 +1654,10 @@ struct module_clone_data {
 	unsigned int require_init: 1;
 	int single_branch;
 };
-#define MODULE_CLONE_DATA_INIT { .reference = STRING_LIST_INIT_NODUP, .single_branch = -1 }
+#define MODULE_CLONE_DATA_INIT { \
+	.reference = STRING_LIST_INIT_NODUP, \
+	.single_branch = -1, \
+}
 
 struct submodule_alternate_setup {
 	const char *submodule_name;
@@ -2047,7 +2050,9 @@ struct update_data {
 	unsigned int nofetch: 1;
 	unsigned int just_cloned: 1;
 };
-#define UPDATE_DATA_INIT { .update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT }
+#define UPDATE_DATA_INIT { \
+	.update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT, \
+}
 
 static void next_submodule_warn_missing(struct submodule_update_clone *suc,
 		struct strbuf *out, const char *displaypath)
@@ -3013,7 +3018,9 @@ struct add_data {
 	unsigned int progress: 1;
 	unsigned int dissociate: 1;
 };
-#define ADD_DATA_INIT { .depth = -1 }
+#define ADD_DATA_INIT { \
+	.depth = -1, \
+}
 
 static void append_fetch_remotes(struct strbuf *msg, const char *git_dir_path)
 {
-- 
2.33.GIT

[PATCH v6 07/16] submodule--helper: don't use bitfield indirection for parse_options()

From: Glen Choo <hidden>
Date: 2022-02-08 08:40:25

From: Ævar Arnfjörð Bjarmason <redacted>

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.

This will also make a subsequent large commit's diff smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 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 c2d4fd0347..4a0890954e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2045,10 +2045,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 opt = 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(&opt.quiet,
+			   N_("suppress output for update by rebase or merge")),
+		OPT__FORCE(&opt.force, N_("force checkout updates"),
+			   0),
+		OPT_BOOL('N', "no-fetch", &opt.nofetch,
 			 N_("don't fetch new objects from the remote site")),
-		OPT_BOOL(0, "just-cloned", &just_cloned,
+		OPT_BOOL(0, "just-cloned", &opt.just_cloned,
 			 N_("overrides update mode in case the repository is a fresh clone")),
 		OPT_INTEGER(0, "depth", &opt.depth, N_("depth for shallow fetch")),
 		OPT_STRING(0, "prefix", &prefix,
@@ -2618,10 +2619,6 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
 	if (argc != 1)
 		usage_with_options(usage, options);
 
-	opt.force = !!force;
-	opt.quiet = !!quiet;
-	opt.nofetch = !!nofetch;
-	opt.just_cloned = !!just_cloned;
 	opt.sm_path = argv[0];
 
 	if (opt.recursive_prefix)
-- 
2.33.GIT

[PATCH v6 08/16] submodule tests: test for init and update failure output

From: Glen Choo <hidden>
Date: 2022-02-08 08:40:27

From: Ævar Arnfjörð Bjarmason <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 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.raw >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.raw >actual &&
+		test_cmp expect actual
 	)
 '
 
-- 
2.33.GIT

[PATCH v6 09/16] submodule--helper: remove update-module-mode

From: Glen Choo <hidden>
Date: 2022-02-08 08:40:30

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 <redacted>
---
 builtin/submodule--helper.c | 24 ------------------------
 1 file changed, 24 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4a0890954e..e0cc1c1b79 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1965,29 +1965,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> <path> [<update>]");
-
-	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;
@@ -3388,7 +3365,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},
-- 
2.33.GIT

[PATCH v6 06/16] builtin/submodule--helper.c: rename option variables to "opt"

From: Glen Choo <hidden>
Date: 2022-02-08 08:40:33

From: Ævar Arnfjörð Bjarmason <redacted>

Rename the "suc" variable in update_clone() to "opt", and do the same
for the "update_data" variable in run_update_procedure().

The only reason for this change is to make the subsequent commit's
diff smaller, by doing this rename we can "anchor" the diff better, as
it "borrow" most of the options declared here as-is as far as the diff
rename detection is concerned.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 builtin/submodule--helper.c | 74 ++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9f79bdf4d5..c2d4fd0347 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2517,36 +2517,36 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 {
 	const char *update = NULL;
 	struct pathspec pathspec;
-	struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;
+	struct submodule_update_clone opt = SUBMODULE_UPDATE_CLONE_INIT;
 
 	struct option module_update_clone_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
-		OPT_STRING(0, "recursive-prefix", &suc.recursive_prefix,
+		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
 			   N_("path"),
 			   N_("path into the working tree, across nested "
 			      "submodule boundaries")),
 		OPT_STRING(0, "update", &update,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
-		OPT_STRING_LIST(0, "reference", &suc.references, N_("repo"),
+		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
 			   N_("reference repository")),
-		OPT_BOOL(0, "dissociate", &suc.dissociate,
+		OPT_BOOL(0, "dissociate", &opt.dissociate,
 			   N_("use --reference only while cloning")),
-		OPT_STRING(0, "depth", &suc.depth, "<depth>",
+		OPT_STRING(0, "depth", &opt.depth, "<depth>",
 			   N_("create a shallow clone truncated to the "
 			      "specified number of revisions")),
-		OPT_INTEGER('j', "jobs", &suc.max_jobs,
+		OPT_INTEGER('j', "jobs", &opt.max_jobs,
 			    N_("parallel jobs")),
-		OPT_BOOL(0, "recommend-shallow", &suc.recommend_shallow,
+		OPT_BOOL(0, "recommend-shallow", &opt.recommend_shallow,
 			    N_("whether the initial clone should follow the shallow recommendation")),
-		OPT__QUIET(&suc.quiet, N_("don't print cloning progress")),
-		OPT_BOOL(0, "progress", &suc.progress,
+		OPT__QUIET(&opt.quiet, N_("don't print cloning progress")),
+		OPT_BOOL(0, "progress", &opt.progress,
 			    N_("force cloning progress")),
-		OPT_BOOL(0, "require-init", &suc.require_init,
+		OPT_BOOL(0, "require-init", &opt.require_init,
 			   N_("disallow cloning into non-empty directory")),
-		OPT_BOOL(0, "single-branch", &suc.single_branch,
+		OPT_BOOL(0, "single-branch", &opt.single_branch,
 			 N_("clone only one branch, HEAD or --branch")),
 		OPT_END()
 	};
@@ -2555,32 +2555,32 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 		N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"),
 		NULL
 	};
-	suc.prefix = prefix;
+	opt.prefix = prefix;
 
-	update_clone_config_from_gitmodules(&suc.max_jobs);
-	git_config(git_update_clone_config, &suc.max_jobs);
+	update_clone_config_from_gitmodules(&opt.max_jobs);
+	git_config(git_update_clone_config, &opt.max_jobs);
 
 	argc = parse_options(argc, argv, prefix, module_update_clone_options,
 			     git_submodule_helper_usage, 0);
 
 	if (update)
-		if (parse_submodule_update_strategy(update, &suc.update) < 0)
+		if (parse_submodule_update_strategy(update, &opt.update) < 0)
 			die(_("bad value for update parameter"));
 
-	if (module_list_compute(argc, argv, prefix, &pathspec, &suc.list) < 0)
+	if (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0)
 		return 1;
 
 	if (pathspec.nr)
-		suc.warn_if_uninitialized = 1;
+		opt.warn_if_uninitialized = 1;
 
-	return update_submodules(&suc);
+	return update_submodules(&opt);
 }
 
 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 update_data opt = UPDATE_DATA_INIT;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("suppress output for update by rebase or merge")),
@@ -2589,20 +2589,20 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
 			 N_("don't fetch new objects from the remote site")),
 		OPT_BOOL(0, "just-cloned", &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_INTEGER(0, "depth", &opt.depth, N_("depth for shallow fetch")),
 		OPT_STRING(0, "prefix", &prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
 		OPT_STRING(0, "update", &update,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
-		OPT_STRING(0, "recursive-prefix", &update_data.recursive_prefix, N_("path"),
+		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix, N_("path"),
 			   N_("path into the working tree, across nested "
 			      "submodule boundaries")),
-		OPT_CALLBACK_F(0, "oid", &update_data.oid, N_("sha1"),
+		OPT_CALLBACK_F(0, "oid", &opt.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"),
+		OPT_CALLBACK_F(0, "suboid", &opt.suboid, N_("subsha1"),
 			       N_("SHA1 of submodule's HEAD"), PARSE_OPT_NONEG,
 			       parse_opt_object_id),
 		OPT_END()
@@ -2618,27 +2618,27 @@ 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];
+	opt.force = !!force;
+	opt.quiet = !!quiet;
+	opt.nofetch = !!nofetch;
+	opt.just_cloned = !!just_cloned;
+	opt.sm_path = argv[0];
 
-	if (update_data.recursive_prefix)
-		prefixed_path = xstrfmt("%s%s", update_data.recursive_prefix, update_data.sm_path);
+	if (opt.recursive_prefix)
+		prefixed_path = xstrfmt("%s%s", opt.recursive_prefix, opt.sm_path);
 	else
-		prefixed_path = xstrdup(update_data.sm_path);
+		prefixed_path = xstrdup(opt.sm_path);
 
-	update_data.displaypath = get_submodule_displaypath(prefixed_path, prefix);
+	opt.displaypath = get_submodule_displaypath(prefixed_path, prefix);
 
-	determine_submodule_update_strategy(the_repository, update_data.just_cloned,
-					    update_data.sm_path, update,
-					    &update_data.update_strategy);
+	determine_submodule_update_strategy(the_repository, opt.just_cloned,
+					    opt.sm_path, update,
+					    &opt.update_strategy);
 
 	free(prefixed_path);
 
-	if (!oideq(&update_data.oid, &update_data.suboid) || update_data.force)
-		return do_run_update_procedure(&update_data);
+	if (!oideq(&opt.oid, &opt.suboid) || opt.force)
+		return do_run_update_procedure(&opt);
 
 	return 3;
 }
-- 
2.33.GIT

[PATCH v6 10/16] submodule--helper: reorganize code for sh to C conversion

From: Glen Choo <hidden>
Date: 2022-02-08 08:40:35

Introduce a function, update_submodule2(), that performs an update for
one submodule. This function will implement the functionality of
run-update-procedure and its surrounding shell code in submodule.sh.

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e0cc1c1b79..0b5120734a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2553,6 +2553,11 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	return update_submodules(&opt);
 }
 
+/*
+ * NEEDSWORK: Use a forward declaration to avoid moving
+ * run_update_procedure() (which will be removed soon).
+ */
+static int update_submodule2(struct update_data *update_data);
 static int run_update_procedure(int argc, const char **argv, const char *prefix)
 {
 	char *prefixed_path, *update = NULL;
@@ -2610,11 +2615,7 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
 					    &opt.update_strategy);
 
 	free(prefixed_path);
-
-	if (!oideq(&opt.oid, &opt.suboid) || opt.force)
-		return do_run_update_procedure(&opt);
-
-	return 3;
+	return update_submodule2(&opt);
 }
 
 static int resolve_relative_path(int argc, const char **argv, const char *prefix)
@@ -2978,6 +2979,15 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 	return !!ret;
 }
 
+/* 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;
-- 
2.33.GIT

[PATCH v6 12/16] submodule--helper run-update-procedure: learn --remote

From: Glen Choo <hidden>
Date: 2022-02-08 08:40:39

Teach run-update-procedure to handle --remote instead of parsing
--remote in git-submodule.sh. As a result, "git submodule--helper
print-default-remote" has no more callers, so remove it.

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 39 ++++++++++++++++++++++---------------
 git-submodule.sh            | 30 +---------------------------
 2 files changed, 24 insertions(+), 45 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a26477ce04..15ae986692 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -71,21 +71,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]);
@@ -2026,6 +2011,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, \
@@ -2585,6 +2571,8 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F(0, "oid", &opt.oid, N_("sha1"),
 			       N_("SHA1 expected by superproject"), PARSE_OPT_NONEG,
 			       parse_opt_object_id),
+		OPT_BOOL(0, "remote", &opt.remote,
+			 N_("use SHA-1 of submodule's remote tracking branch")),
 		OPT_END()
 	};
 
@@ -2988,6 +2976,25 @@ static int update_submodule2(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);
+
+		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);
 
@@ -3389,10 +3396,10 @@ 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},
+	/* NEEDSWORK: remote-branch is also obsolete */
 	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"push-check", push_check, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
diff --git a/git-submodule.sh b/git-submodule.sh
index d48c314f01..29fd69250d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -246,20 +246,6 @@ cmd_deinit()
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- "$@"
 }
 
-# usage: fetch_in_submodule <module_path> [<depth>] [<sha1>]
-# Because arguments are positional, use an empty string to omit <depth>
-# but include <sha1>.
-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
 #
@@ -396,21 +382,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} \
@@ -421,6 +392,7 @@ cmd_update()
 			  ${update:+--update "$update"} \
 			  ${prefix:+--recursive-prefix "$prefix"} \
 			  ${sha1:+--oid "$sha1"} \
+			  ${remote:+--remote} \
 			  "--" \
 			  "$sm_path")
 
-- 
2.33.GIT

[PATCH v6 13/16] submodule--helper: remove ensure-core-worktree

From: Glen Choo <hidden>
Date: 2022-02-08 08:41:02

Move the logic of "git submodule--helper ensure-core-worktree" into
run-update-procedure. Since the ensure-core-worktree command is
obsolete, remove it.

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 13 +++----------
 git-submodule.sh            |  2 --
 2 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 15ae986692..a05aea5cd6 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2746,17 +2746,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>");
-
-	path = argv[1];
-
 	if (repo_submodule_init(&subrepo, the_repository, path, null_oid()))
 		die(_("could not get a repository handle for submodule '%s'"), path);
 
@@ -2776,8 +2770,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)
@@ -2967,6 +2959,8 @@ static int module_set_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);
+
 	/* NEEDSWORK: fix the style issues e.g. braces */
 	if (update_data->just_cloned) {
 		oidcpy(&update_data->suboid, null_oid());
@@ -3390,7 +3384,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 29fd69250d..aa9c898e1c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -373,8 +373,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
-- 
2.33.GIT

[PATCH v6 11/16] submodule--helper run-update-procedure: remove --suboid

From: Glen Choo <hidden>
Date: 2022-02-08 08:41:02

Teach run-update-procedure to determine the oid of the submodule's HEAD
instead of doing it in git-subomdule.sh.

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 12 +++++++++---
 git-submodule.sh            |  8 +-------
 2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 0b5120734a..a26477ce04 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2585,9 +2585,6 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F(0, "oid", &opt.oid, N_("sha1"),
 			       N_("SHA1 expected by superproject"), PARSE_OPT_NONEG,
 			       parse_opt_object_id),
-		OPT_CALLBACK_F(0, "suboid", &opt.suboid, N_("subsha1"),
-			       N_("SHA1 of submodule's HEAD"), PARSE_OPT_NONEG,
-			       parse_opt_object_id),
 		OPT_END()
 	};
 
@@ -2982,6 +2979,15 @@ static int module_set_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)
 {
+	/* NEEDSWORK: fix the style issues e.g. braces */
+	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 652861aa66..d48c314f01 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -391,14 +391,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"
@@ -426,7 +421,6 @@ cmd_update()
 			  ${update:+--update "$update"} \
 			  ${prefix:+--recursive-prefix "$prefix"} \
 			  ${sha1:+--oid "$sha1"} \
-			  ${subsha1:+--suboid "$subsha1"} \
 			  "--" \
 			  "$sm_path")
 
-- 
2.33.GIT

[PATCH v6 14/16] submodule--helper update-clone: learn --init

From: Glen Choo <hidden>
Date: 2022-02-08 08:41:07

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 <redacted>
---
 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 a05aea5cd6..5635f0c48b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1989,6 +1989,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, \
@@ -2483,6 +2484,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	struct submodule_update_clone opt = SUBMODULE_UPDATE_CLONE_INIT;
 
 	struct option module_update_clone_options[] = {
+		OPT_BOOL(0, "init", &opt.init,
+			 N_("initialize uninitialized submodules before update")),
 		OPT_STRING(0, "prefix", &prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
@@ -2536,6 +2539,29 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	if (pathspec.nr)
 		opt.warn_if_uninitialized = 1;
 
+	if (opt.init) {
+		struct module_list list = MODULE_LIST_INIT;
+		struct init_cb info = INIT_CB_INIT;
+
+		if (module_list_compute(argc, argv, opt.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 = opt.prefix;
+		info.superprefix = opt.recursive_prefix;
+		if (opt.quiet)
+			info.flags |= OPT_QUIET;
+
+		for_each_listed_submodule(&list, init_submodule_cb, &info);
+	}
+
 	return update_submodules(&opt);
 }
 
diff --git a/git-submodule.sh b/git-submodule.sh
index aa9c898e1c..3ccf2388bf 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -347,14 +347,11 @@ cmd_update()
 		shift
 	done
 
-	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"} \
-- 
2.33.GIT

[PATCH v6 15/16] submodule--helper: move functions around

From: Glen Choo <hidden>
Date: 2022-02-08 08:41:09

The next commit will change the internals of several functions and
arrange them in a more logical manner. Move these functions to their
final positions so that the diff is smaller.

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 228 ++++++++++++++++++------------------
 1 file changed, 114 insertions(+), 114 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5635f0c48b..0ab8f9d49f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2451,120 +2451,6 @@ static void update_submodule(struct update_clone_data *ucd)
 		ucd->sub->path);
 }
 
-static int update_submodules(struct submodule_update_clone *suc)
-{
-	int i;
-
-	run_processes_parallel_tr2(suc->max_jobs, update_clone_get_next_task,
-				   update_clone_start_failure,
-				   update_clone_task_finished, suc, "submodule",
-				   "parallel/update");
-
-	/*
-	 * We saved the output and put it out all at once now.
-	 * That means:
-	 * - the listener does not have to interleave their (checkout)
-	 *   work with our fetching.  The writes involved in a
-	 *   checkout involve more straightforward sequential I/O.
-	 * - the listener can avoid doing any work if fetching failed.
-	 */
-	if (suc->quickstop)
-		return 1;
-
-	for (i = 0; i < suc->update_clone_nr; i++)
-		update_submodule(&suc->update_clone[i]);
-
-	return 0;
-}
-
-static int update_clone(int argc, const char **argv, const char *prefix)
-{
-	const char *update = NULL;
-	struct pathspec pathspec;
-	struct submodule_update_clone opt = SUBMODULE_UPDATE_CLONE_INIT;
-
-	struct option module_update_clone_options[] = {
-		OPT_BOOL(0, "init", &opt.init,
-			 N_("initialize uninitialized submodules before update")),
-		OPT_STRING(0, "prefix", &prefix,
-			   N_("path"),
-			   N_("path into the working tree")),
-		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
-			   N_("path"),
-			   N_("path into the working tree, across nested "
-			      "submodule boundaries")),
-		OPT_STRING(0, "update", &update,
-			   N_("string"),
-			   N_("rebase, merge, checkout or none")),
-		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
-			   N_("reference repository")),
-		OPT_BOOL(0, "dissociate", &opt.dissociate,
-			   N_("use --reference only while cloning")),
-		OPT_STRING(0, "depth", &opt.depth, "<depth>",
-			   N_("create a shallow clone truncated to the "
-			      "specified number of revisions")),
-		OPT_INTEGER('j', "jobs", &opt.max_jobs,
-			    N_("parallel jobs")),
-		OPT_BOOL(0, "recommend-shallow", &opt.recommend_shallow,
-			    N_("whether the initial clone should follow the shallow recommendation")),
-		OPT__QUIET(&opt.quiet, N_("don't print cloning progress")),
-		OPT_BOOL(0, "progress", &opt.progress,
-			    N_("force cloning progress")),
-		OPT_BOOL(0, "require-init", &opt.require_init,
-			   N_("disallow cloning into non-empty directory")),
-		OPT_BOOL(0, "single-branch", &opt.single_branch,
-			 N_("clone only one branch, HEAD or --branch")),
-		OPT_END()
-	};
-
-	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"),
-		NULL
-	};
-	opt.prefix = prefix;
-
-	update_clone_config_from_gitmodules(&opt.max_jobs);
-	git_config(git_update_clone_config, &opt.max_jobs);
-
-	argc = parse_options(argc, argv, prefix, module_update_clone_options,
-			     git_submodule_helper_usage, 0);
-
-	if (update)
-		if (parse_submodule_update_strategy(update, &opt.update) < 0)
-			die(_("bad value for update parameter"));
-
-	if (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0)
-		return 1;
-
-	if (pathspec.nr)
-		opt.warn_if_uninitialized = 1;
-
-	if (opt.init) {
-		struct module_list list = MODULE_LIST_INIT;
-		struct init_cb info = INIT_CB_INIT;
-
-		if (module_list_compute(argc, argv, opt.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 = opt.prefix;
-		info.superprefix = opt.recursive_prefix;
-		if (opt.quiet)
-			info.flags |= OPT_QUIET;
-
-		for_each_listed_submodule(&list, init_submodule_cb, &info);
-	}
-
-	return update_submodules(&opt);
-}
-
 /*
  * NEEDSWORK: Use a forward declaration to avoid moving
  * run_update_procedure() (which will be removed soon).
@@ -3021,6 +2907,120 @@ static int update_submodule2(struct update_data *update_data)
 	return 3;
 }
 
+static int update_submodules(struct submodule_update_clone *suc)
+{
+	int i;
+
+	run_processes_parallel_tr2(suc->max_jobs, update_clone_get_next_task,
+				   update_clone_start_failure,
+				   update_clone_task_finished, suc, "submodule",
+				   "parallel/update");
+
+	/*
+	 * We saved the output and put it out all at once now.
+	 * That means:
+	 * - the listener does not have to interleave their (checkout)
+	 *   work with our fetching.  The writes involved in a
+	 *   checkout involve more straightforward sequential I/O.
+	 * - the listener can avoid doing any work if fetching failed.
+	 */
+	if (suc->quickstop)
+		return 1;
+
+	for (i = 0; i < suc->update_clone_nr; i++)
+		update_submodule(&suc->update_clone[i]);
+
+	return 0;
+}
+
+static int update_clone(int argc, const char **argv, const char *prefix)
+{
+	const char *update = NULL;
+	struct pathspec pathspec;
+	struct submodule_update_clone opt = SUBMODULE_UPDATE_CLONE_INIT;
+
+	struct option module_update_clone_options[] = {
+		OPT_BOOL(0, "init", &opt.init,
+			 N_("initialize uninitialized submodules before update")),
+		OPT_STRING(0, "prefix", &prefix,
+			   N_("path"),
+			   N_("path into the working tree")),
+		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
+			   N_("path"),
+			   N_("path into the working tree, across nested "
+			      "submodule boundaries")),
+		OPT_STRING(0, "update", &update,
+			   N_("string"),
+			   N_("rebase, merge, checkout or none")),
+		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
+			   N_("reference repository")),
+		OPT_BOOL(0, "dissociate", &opt.dissociate,
+			   N_("use --reference only while cloning")),
+		OPT_STRING(0, "depth", &opt.depth, "<depth>",
+			   N_("create a shallow clone truncated to the "
+			      "specified number of revisions")),
+		OPT_INTEGER('j', "jobs", &opt.max_jobs,
+			    N_("parallel jobs")),
+		OPT_BOOL(0, "recommend-shallow", &opt.recommend_shallow,
+			    N_("whether the initial clone should follow the shallow recommendation")),
+		OPT__QUIET(&opt.quiet, N_("don't print cloning progress")),
+		OPT_BOOL(0, "progress", &opt.progress,
+			    N_("force cloning progress")),
+		OPT_BOOL(0, "require-init", &opt.require_init,
+			   N_("disallow cloning into non-empty directory")),
+		OPT_BOOL(0, "single-branch", &opt.single_branch,
+			 N_("clone only one branch, HEAD or --branch")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"),
+		NULL
+	};
+	opt.prefix = prefix;
+
+	update_clone_config_from_gitmodules(&opt.max_jobs);
+	git_config(git_update_clone_config, &opt.max_jobs);
+
+	argc = parse_options(argc, argv, prefix, module_update_clone_options,
+			     git_submodule_helper_usage, 0);
+
+	if (update)
+		if (parse_submodule_update_strategy(update, &opt.update) < 0)
+			die(_("bad value for update parameter"));
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0)
+		return 1;
+
+	if (pathspec.nr)
+		opt.warn_if_uninitialized = 1;
+
+	if (opt.init) {
+		struct module_list list = MODULE_LIST_INIT;
+		struct init_cb info = INIT_CB_INIT;
+
+		if (module_list_compute(argc, argv, opt.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 = opt.prefix;
+		info.superprefix = opt.recursive_prefix;
+		if (opt.quiet)
+			info.flags |= OPT_QUIET;
+
+		for_each_listed_submodule(&list, init_submodule_cb, &info);
+	}
+
+	return update_submodules(&opt);
+}
+
 struct add_data {
 	const char *prefix;
 	const char *branch;
-- 
2.33.GIT

[PATCH v6 16/16] submodule: move core cmd_update() logic to C

From: Glen Choo <hidden>
Date: 2022-02-08 08:41:11

From: Atharva Raykar <redacted>

This patch completes the conversion past the flag parsing of
`submodule update` by introducing a helper subcommand called
`submodule--helper update`. The behaviour of `submodule update` should
remain the same after this patch.

We add more fields to the `struct update_data` that are required by
`struct submodule_update_clone` to be able to perform a clone, when that
is needed to be done.

Recursing on a submodule is done by calling a subprocess that launches
`submodule--helper update`, with a modified `--recursive-prefix` and
`--prefix` parameter.

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Signed-off-by: Atharva Raykar <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 360 ++++++++++++++++++++++--------------
 git-submodule.sh            | 102 +---------
 2 files changed, 227 insertions(+), 235 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 0ab8f9d49f..5085b28a2d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1977,7 +1977,6 @@ struct submodule_update_clone {
 	const char *prefix;
 	int single_branch;
 
-	/* to be consumed by git-submodule.sh */
 	struct update_clone_data *update_clone;
 	int update_clone_nr; int update_clone_alloc;
 
@@ -1989,7 +1988,6 @@ 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, \
@@ -2001,6 +1999,7 @@ struct submodule_update_clone {
 }
 
 struct update_data {
+	const char *prefix;
 	const char *recursive_prefix;
 	const char *sm_path;
 	const char *displaypath;
@@ -2008,14 +2007,54 @@ struct update_data {
 	struct object_id suboid;
 	struct submodule_update_strategy update_strategy;
 	int depth;
+	int recommend_shallow;
+	int single_branch;
+	int max_jobs;
+	unsigned int init;
 	unsigned int force;
 	unsigned int quiet;
 	unsigned int nofetch;
 	unsigned int just_cloned;
 	unsigned int remote;
+	unsigned int recursive;
+	unsigned int progress;
+	unsigned int dissociate;
+	unsigned int require_init;
+	unsigned warn_if_uninitialized ;
+	struct string_list references;
+	struct module_list list;
 };
 #define UPDATE_DATA_INIT { \
+	.list = MODULE_LIST_INIT, \
 	.update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT, \
+	.recommend_shallow = -1, \
+	.references = STRING_LIST_INIT_DUP, \
+	.single_branch = -1, \
+	.max_jobs = 1, \
+}
+
+static void update_clone_from_update_data(struct submodule_update_clone *suc,
+					  struct update_data *update_data)
+{
+	suc->prefix = update_data->prefix;
+	suc->recursive_prefix = update_data->recursive_prefix;
+	suc->max_jobs = update_data->max_jobs;
+	suc->progress = update_data->progress;
+	suc->quiet = update_data->quiet;
+	suc->dissociate = update_data->dissociate;
+	suc->require_init = update_data->require_init;
+	suc->single_branch = update_data->single_branch;
+	suc->warn_if_uninitialized = update_data->warn_if_uninitialized;
+	suc->list = update_data->list;
+	suc->update = update_data->update_strategy;
+	suc->recommend_shallow = update_data->recommend_shallow;
+	if (update_data->depth)
+		suc->depth = xstrfmt("--depth=%d", update_data->depth);
+	if (update_data->references.nr) {
+		struct string_list_item *item;
+		for_each_string_list_item(item, &update_data->references)
+			string_list_append(&suc->references, item->string);
+	}
 }
 
 static void next_submodule_warn_missing(struct submodule_update_clone *suc,
@@ -2316,8 +2355,15 @@ static int run_update_command(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;
+	struct submodule_update_strategy strategy = SUBMODULE_UPDATE_STRATEGY_INIT;
+
+	if (ud->update_strategy.type == SM_UPDATE_UNSPECIFIED || ud->just_cloned)
+		determine_submodule_update_strategy(the_repository, ud->just_cloned,
+						    ud->sm_path, NULL, &strategy);
+	else
+		strategy = ud->update_strategy;
 
-	switch (ud->update_strategy.type) {
+	switch (strategy.type) {
 	case SM_UPDATE_CHECKOUT:
 		cp.git_cmd = 1;
 		strvec_pushl(&cp.args, "checkout", "-q", NULL);
@@ -2340,55 +2386,54 @@ static int run_update_command(struct update_data *ud, int subforce)
 		break;
 	case SM_UPDATE_COMMAND:
 		cp.use_shell = 1;
-		strvec_push(&cp.args, ud->update_strategy.command);
+		strvec_push(&cp.args, strategy.command);
 		must_die_on_failure = 1;
 		break;
 	default:
 		BUG("unexpected update strategy type: %s",
-		    submodule_strategy_to_string(&ud->update_strategy));
+		    submodule_strategy_to_string(&strategy));
 	}
 	strvec_push(&cp.args, oid);
 
 	cp.dir = xstrdup(ud->sm_path);
 	prepare_submodule_repo_env(&cp.env_array);
 	if (run_command(&cp)) {
-		switch (ud->update_strategy.type) {
+		switch (strategy.type) {
 		case SM_UPDATE_CHECKOUT:
-			printf(_("Unable to checkout '%s' in submodule path '%s'"),
-			       oid, ud->displaypath);
+			die_message(_("Unable to checkout '%s' in submodule path '%s'"),
+				    oid, ud->displaypath);
 			break;
 		case SM_UPDATE_REBASE:
-			printf(_("Unable to rebase '%s' in submodule path '%s'"),
-			       oid, ud->displaypath);
+			if (!must_die_on_failure)
+				break;
+			die(_("Unable to rebase '%s' in submodule path '%s'"),
+			    oid, ud->displaypath);
 			break;
 		case SM_UPDATE_MERGE:
-			printf(_("Unable to merge '%s' in submodule path '%s'"),
-			       oid, ud->displaypath);
+			if (!must_die_on_failure)
+				break;
+			die(_("Unable to merge '%s' in submodule path '%s'"),
+			    oid, ud->displaypath);
 			break;
 		case SM_UPDATE_COMMAND:
-			printf(_("Execution of '%s %s' failed in submodule path '%s'"),
-			       ud->update_strategy.command, oid, ud->displaypath);
+			if (!must_die_on_failure)
+				break;
+			die(_("Execution of '%s %s' failed in submodule path '%s'"),
+			    strategy.command, oid, ud->displaypath);
 			break;
 		default:
 			BUG("unexpected update strategy type: %s",
-			    submodule_strategy_to_string(&ud->update_strategy));
+			    submodule_strategy_to_string(&strategy));
 		}
-		/*
-		 * NEEDSWORK: We are currently printing to stdout with error
-		 * return so that the shell caller handles the error output
-		 * properly. Once we start handling the error messages within
-		 * C, we should use die() instead.
-		 */
-		if (must_die_on_failure)
-			return 2;
-		/*
-		 * This signifies to the caller in shell that the command
-		 * failed without dying
-		 */
+
+		/* the command failed, but update must continue */
 		return 1;
 	}
 
-	switch (ud->update_strategy.type) {
+	if (ud->quiet)
+		return 0;
+
+	switch (strategy.type) {
 	case SM_UPDATE_CHECKOUT:
 		printf(_("Submodule path '%s': checked out '%s'\n"),
 		       ud->displaypath, oid);
@@ -2403,17 +2448,17 @@ static int run_update_command(struct update_data *ud, int subforce)
 		break;
 	case SM_UPDATE_COMMAND:
 		printf(_("Submodule path '%s': '%s %s'\n"),
-		       ud->displaypath, ud->update_strategy.command, oid);
+		       ud->displaypath, strategy.command, oid);
 		break;
 	default:
 		BUG("unexpected update strategy type: %s",
-		    submodule_strategy_to_string(&ud->update_strategy));
+		    submodule_strategy_to_string(&strategy));
 	}
 
 	return 0;
 }
 
-static int do_run_update_procedure(struct update_data *ud)
+static int run_update_procedure(struct update_data *ud)
 {
 	int subforce = is_null_oid(&ud->suboid) || ud->force;
 
@@ -2443,89 +2488,6 @@ static int do_run_update_procedure(struct update_data *ud)
 	return run_update_command(ud, subforce);
 }
 
-static void update_submodule(struct update_clone_data *ucd)
-{
-	fprintf(stdout, "dummy %s %d\t%s\n",
-		oid_to_hex(&ucd->oid),
-		ucd->just_cloned,
-		ucd->sub->path);
-}
-
-/*
- * NEEDSWORK: Use a forward declaration to avoid moving
- * run_update_procedure() (which will be removed soon).
- */
-static int update_submodule2(struct update_data *update_data);
-static int run_update_procedure(int argc, const char **argv, const char *prefix)
-{
-	char *prefixed_path, *update = NULL;
-	struct update_data opt = UPDATE_DATA_INIT;
-
-	struct option options[] = {
-		OPT__QUIET(&opt.quiet,
-			   N_("suppress output for update by rebase or merge")),
-		OPT__FORCE(&opt.force, N_("force checkout updates"),
-			   0),
-		OPT_BOOL('N', "no-fetch", &opt.nofetch,
-			 N_("don't fetch new objects from the remote site")),
-		OPT_BOOL(0, "just-cloned", &opt.just_cloned,
-			 N_("overrides update mode in case the repository is a fresh clone")),
-		OPT_INTEGER(0, "depth", &opt.depth, N_("depth for shallow fetch")),
-		OPT_STRING(0, "prefix", &prefix,
-			   N_("path"),
-			   N_("path into the working tree")),
-		OPT_STRING(0, "update", &update,
-			   N_("string"),
-			   N_("rebase, merge, checkout or none")),
-		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix, N_("path"),
-			   N_("path into the working tree, across nested "
-			      "submodule boundaries")),
-		OPT_CALLBACK_F(0, "oid", &opt.oid, N_("sha1"),
-			       N_("SHA1 expected by superproject"), PARSE_OPT_NONEG,
-			       parse_opt_object_id),
-		OPT_BOOL(0, "remote", &opt.remote,
-			 N_("use SHA-1 of submodule's remote tracking branch")),
-		OPT_END()
-	};
-
-	const char *const usage[] = {
-		N_("git submodule--helper run-update-procedure [<options>] <path>"),
-		NULL
-	};
-
-	argc = parse_options(argc, argv, prefix, options, usage, 0);
-
-	if (argc != 1)
-		usage_with_options(usage, options);
-
-	opt.sm_path = argv[0];
-
-	if (opt.recursive_prefix)
-		prefixed_path = xstrfmt("%s%s", opt.recursive_prefix, opt.sm_path);
-	else
-		prefixed_path = xstrdup(opt.sm_path);
-
-	opt.displaypath = get_submodule_displaypath(prefixed_path, prefix);
-
-	determine_submodule_update_strategy(the_repository, opt.just_cloned,
-					    opt.sm_path, update,
-					    &opt.update_strategy);
-
-	free(prefixed_path);
-	return update_submodule2(&opt);
-}
-
-static int resolve_relative_path(int argc, const char **argv, const char *prefix)
-{
-	struct strbuf sb = STRBUF_INIT;
-	if (argc != 3)
-		die("submodule--helper relative-path takes exactly 2 arguments, got %d", argc);
-
-	printf("%s", relative_path(argv[1], argv[2], &sb));
-	strbuf_release(&sb);
-	return 0;
-}
-
 static const char *remote_submodule_branch(const char *path)
 {
 	const struct submodule *sub;
@@ -2868,11 +2830,66 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 	return !!ret;
 }
 
-/* NEEDSWORK: this is a temporary name until we delete update_submodule() */
-static int update_submodule2(struct update_data *update_data)
+static void update_data_to_args(struct update_data *update_data, struct strvec *args)
+{
+	const char *update = submodule_strategy_to_string(&update_data->update_strategy);
+
+	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
+	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
+	if (update_data->prefix)
+		strvec_pushl(args, "--prefix", update_data->prefix, NULL);
+	if (update_data->recursive_prefix)
+		strvec_pushl(args, "--recursive-prefix",
+			     update_data->recursive_prefix, NULL);
+	if (update_data->quiet)
+		strvec_push(args, "--quiet");
+	if (update_data->force)
+		strvec_push(args, "--force");
+	if (update_data->init)
+		strvec_push(args, "--init");
+	if (update_data->remote)
+		strvec_push(args, "--remote");
+	if (update_data->nofetch)
+		strvec_push(args, "--no-fetch");
+	if (update_data->dissociate)
+		strvec_push(args, "--dissociate");
+	if (update_data->progress)
+		strvec_push(args, "--progress");
+	if (update_data->require_init)
+		strvec_push(args, "--require-init");
+	if (update_data->depth)
+		strvec_pushf(args, "--depth=%d", update_data->depth);
+	if (update)
+		strvec_pushl(args, "--update", update, NULL);
+	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);
+	}
+	if (update_data->recommend_shallow == 0)
+		strvec_push(args, "--no-recommend-shallow");
+	else if (update_data->recommend_shallow == 1)
+		strvec_push(args, "--recommend-shallow");
+	if (update_data->single_branch >= 0)
+		strvec_push(args, "--single-branch");
+}
+
+static int update_submodule(struct update_data *update_data)
 {
+	char *prefixed_path;
+
 	ensure_core_worktree(update_data->sm_path);
 
+	if (update_data->recursive_prefix)
+		prefixed_path = xstrfmt("%s%s", update_data->recursive_prefix,
+					update_data->sm_path);
+	else
+		prefixed_path = xstrdup(update_data->sm_path);
+
+	update_data->displaypath = get_submodule_displaypath(prefixed_path,
+							     update_data->prefix);
+	free(prefixed_path);
+
 	/* NEEDSWORK: fix the style issues e.g. braces */
 	if (update_data->just_cloned) {
 		oidcpy(&update_data->suboid, null_oid());
@@ -2902,18 +2919,55 @@ 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);
+		if (run_update_procedure(update_data))
+			return 1;
 
-	return 3;
+	if (update_data->recursive) {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		struct update_data next = *update_data;
+		int res;
+
+		if (update_data->recursive_prefix)
+			prefixed_path = xstrfmt("%s%s/", update_data->recursive_prefix,
+						update_data->sm_path);
+		else
+			prefixed_path = xstrfmt("%s/", update_data->sm_path);
+
+		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
+								  update_data->prefix);
+		next.prefix = NULL;
+		oidcpy(&next.oid, null_oid());
+		oidcpy(&next.suboid, null_oid());
+
+		cp.dir = update_data->sm_path;
+		cp.git_cmd = 1;
+		prepare_submodule_repo_env(&cp.env_array);
+		update_data_to_args(&next, &cp.args);
+
+		/* die() if child process die()'d */
+		res = run_command(&cp);
+		if (!res)
+			return 0;
+		die_message(_("Failed to recurse into submodule path '%s'"),
+			    update_data->displaypath);
+		if (res == 128)
+			exit(res);
+		else if (res)
+			return 1;
+	}
+
+	return 0;
 }
 
-static int update_submodules(struct submodule_update_clone *suc)
+static int update_submodules(struct update_data *update_data)
 {
-	int i;
+	int i, res = 0;
+	struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;
 
-	run_processes_parallel_tr2(suc->max_jobs, update_clone_get_next_task,
+	update_clone_from_update_data(&suc, update_data);
+	run_processes_parallel_tr2(suc.max_jobs, update_clone_get_next_task,
 				   update_clone_start_failure,
-				   update_clone_task_finished, suc, "submodule",
+				   update_clone_task_finished, &suc, "submodule",
 				   "parallel/update");
 
 	/*
@@ -2924,25 +2978,45 @@ static int update_submodules(struct submodule_update_clone *suc)
 	 *   checkout involve more straightforward sequential I/O.
 	 * - the listener can avoid doing any work if fetching failed.
 	 */
-	if (suc->quickstop)
-		return 1;
+	if (suc.quickstop) {
+		res = 1;
+		goto cleanup;
+	}
 
-	for (i = 0; i < suc->update_clone_nr; i++)
-		update_submodule(&suc->update_clone[i]);
+	for (i = 0; i < suc.update_clone_nr; i++) {
+		struct update_clone_data ucd = suc.update_clone[i];
 
-	return 0;
+		oidcpy(&update_data->oid, &ucd.oid);
+		update_data->just_cloned = ucd.just_cloned;
+		update_data->sm_path = ucd.sub->path;
+
+		if (update_submodule(update_data))
+			res = 1;
+	}
+
+cleanup:
+	string_list_clear(&update_data->references, 0);
+	return res;
 }
 
-static int update_clone(int argc, const char **argv, const char *prefix)
+static int module_update(int argc, const char **argv, const char *prefix)
 {
 	const char *update = NULL;
 	struct pathspec pathspec;
-	struct submodule_update_clone opt = SUBMODULE_UPDATE_CLONE_INIT;
+	struct update_data opt = UPDATE_DATA_INIT;
 
+	/* NEEDSWORK: update names and strings */
 	struct option module_update_clone_options[] = {
+		OPT__FORCE(&opt.force, N_("force checkout updates"), 0),
 		OPT_BOOL(0, "init", &opt.init,
 			 N_("initialize uninitialized submodules before update")),
-		OPT_STRING(0, "prefix", &prefix,
+		OPT_BOOL(0, "remote", &opt.remote,
+			 N_("use SHA-1 of submodule's remote tracking branch")),
+		OPT_BOOL(0, "recursive", &opt.recursive,
+			 N_("traverse submodules recursively")),
+		OPT_BOOL('N', "no-fetch", &opt.nofetch,
+			 N_("don't fetch new objects from the remote site")),
+		OPT_STRING(0, "prefix", &opt.prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
 		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
@@ -2953,21 +3027,21 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
 		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
-			   N_("reference repository")),
+				N_("reference repository")),
 		OPT_BOOL(0, "dissociate", &opt.dissociate,
-			   N_("use --reference only while cloning")),
-		OPT_STRING(0, "depth", &opt.depth, "<depth>",
-			   N_("create a shallow clone truncated to the "
-			      "specified number of revisions")),
+			 N_("use --reference only while cloning")),
+		OPT_INTEGER(0, "depth", &opt.depth,
+			    N_("create a shallow clone truncated to the "
+			       "specified number of revisions")),
 		OPT_INTEGER('j', "jobs", &opt.max_jobs,
 			    N_("parallel jobs")),
 		OPT_BOOL(0, "recommend-shallow", &opt.recommend_shallow,
-			    N_("whether the initial clone should follow the shallow recommendation")),
+			 N_("whether the initial clone should follow the shallow recommendation")),
 		OPT__QUIET(&opt.quiet, N_("don't print cloning progress")),
 		OPT_BOOL(0, "progress", &opt.progress,
-			    N_("force cloning progress")),
+			 N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &opt.require_init,
-			   N_("disallow cloning into non-empty directory")),
+			 N_("disallow cloning into non-empty directory")),
 		OPT_BOOL(0, "single-branch", &opt.single_branch,
 			 N_("clone only one branch, HEAD or --branch")),
 		OPT_END()
@@ -2977,16 +3051,18 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 		N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"),
 		NULL
 	};
-	opt.prefix = prefix;
 
 	update_clone_config_from_gitmodules(&opt.max_jobs);
 	git_config(git_update_clone_config, &opt.max_jobs);
 
 	argc = parse_options(argc, argv, prefix, module_update_clone_options,
 			     git_submodule_helper_usage, 0);
+	oidcpy(&opt.oid, null_oid());
+	oidcpy(&opt.suboid, null_oid());
 
 	if (update)
-		if (parse_submodule_update_strategy(update, &opt.update) < 0)
+		if (parse_submodule_update_strategy(update,
+						    &opt.update_strategy) < 0)
 			die(_("bad value for update parameter"));
 
 	if (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0)
@@ -3408,9 +3484,7 @@ static struct cmd_struct commands[] = {
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
 	{"add", module_add, SUPPORT_SUPER_PREFIX},
-	{"update-clone", update_clone, 0},
-	{"run-update-procedure", run_update_procedure, 0},
-	{"relative-path", resolve_relative_path, 0},
+	{"update", module_update, 0},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},
diff --git a/git-submodule.sh b/git-submodule.sh
index 3ccf2388bf..bcd8b92aab 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -347,108 +347,26 @@ cmd_update()
 		shift
 	done
 
-	{
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update-clone \
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper update \
 		${GIT_QUIET:+--quiet} \
-		${progress:+"--progress"} \
+		${force:+--force} \
+		${progress:+--progress} \
+		${dissociate:+--dissociate} \
+		${remote:+--remote} \
+		${recursive:+--recursive} \
 		${init:+--init} \
+		${require_init:+--require-init} \
+		${nofetch:+--no-fetch} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
 		${prefix:+--recursive-prefix "$prefix"} \
 		${update:+--update "$update"} \
 		${reference:+"$reference"} \
-		${dissociate:+"--dissociate"} \
-		${depth:+--depth "$depth"} \
-		${require_init:+--require-init} \
+		${depth:+"$depth"} \
 		$single_branch \
 		$recommend_shallow \
 		$jobs \
 		-- \
-		"$@" || echo "#unmatched" $?
-	} | {
-	err=
-	while read -r quickabort sha1 just_cloned sm_path
-	do
-		die_if_unmatched "$quickabort" "$sha1"
-
-		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-
-		if test $just_cloned -eq 0
-		then
-			just_cloned=
-		fi
-
-		out=$(git submodule--helper run-update-procedure \
-			  ${wt_prefix:+--prefix "$wt_prefix"} \
-			  ${GIT_QUIET:+--quiet} \
-			  ${force:+--force} \
-			  ${just_cloned:+--just-cloned} \
-			  ${nofetch:+--no-fetch} \
-			  ${depth:+"$depth"} \
-			  ${update:+--update "$update"} \
-			  ${prefix:+--recursive-prefix "$prefix"} \
-			  ${sha1:+--oid "$sha1"} \
-			  ${remote:+--remote} \
-			  "--" \
-			  "$sm_path")
-
-		# exit codes for run-update-procedure:
-		# 0: update was successful, say command output
-		# 1: update procedure failed, but should not die
-		# 2 or 128: subcommand died during execution
-		# 3: no update procedure was run
-		res="$?"
-		case $res in
-		0)
-			say "$out"
-			;;
-		1)
-			err="${err};fatal: $out"
-			continue
-			;;
-		2|128)
-			die_with_status $res "fatal: $out"
-			;;
-		esac
-
-		if test -n "$recursive"
-		then
-			(
-				prefix=$(git submodule--helper relative-path "$prefix$sm_path/" "$wt_prefix")
-				wt_prefix=
-				sanitize_submodule_env
-				cd "$sm_path" &&
-				eval cmd_update
-			)
-			res=$?
-			if test $res -gt 0
-			then
-				die_msg="fatal: $(eval_gettext "Failed to recurse into submodule path '\$displaypath'")"
-				if test $res -ne 2
-				then
-					err="${err};$die_msg"
-					continue
-				else
-					die_with_status $res "$die_msg"
-				fi
-			fi
-		fi
-	done
-
-	if test -n "$err"
-	then
-		OIFS=$IFS
-		IFS=';'
-		for e in $err
-		do
-			if test -n "$e"
-			then
-				echo >&2 "$e"
-			fi
-		done
-		IFS=$OIFS
-		exit 1
-	fi
-	}
+		"$@"
 }
 
 #
-- 
2.33.GIT

[PATCH v7 00/20] submodule: convert the rest of 'update' to C

From: Glen Choo <hidden>
Date: 2022-02-10 09:28:51

This reroll contains another 'easy' preparatory patch and the fixups I
alluded to in v6 [1]. This isn't the split-up I described in the
footnote of v6, but it gets the big patch (patch 17) to what I think is
a reviewable state.

The diff between v7 and v5 is no longer just NEEDSWORK comments, but I
think it is easier to reason about. Patch 17 resembles v5 the most (I
will include a diff in a reply to that patch); everything after patch 17
is fixups (I did not squash them in because they would grow the diff
even more).

I will also leave a review on patch 17 since the changes were not
originally authored by me.

[1] https://lore.kernel.org/git/20220208083952.35036-1-chooglen@google.com

Changes in v7:
- Split the last patch of v6 (the big one) into patches 16-17.
- Patch 16 moves logic out of run_update_procedure() (because the
  command is going away), removing some noise from patch 17. This makes
  the update_strategy parsing easier to reason about, but at the cost of
  growing the diff vis-a-vis v5
- Patches 18-20 are fixups that address NEEDSWORK comments from earlier
  patches. Once maintaining a small diff vis-a-vis v5 stops making
  sense, I will squash them in.

Atharva Raykar (6):
  submodule--helper: get remote names from any repository
  submodule--helper: refactor get_submodule_displaypath()
  submodule--helper: allow setting superprefix for init_submodule()
  submodule--helper: run update using child process struct
  builtin/submodule--helper.c: reformat designated initializers
  submodule: move core cmd_update() logic to C

Glen Choo (11):
  submodule--helper: remove update-module-mode
  submodule--helper: reorganize code for sh to C conversion
  submodule--helper run-update-procedure: remove --suboid
  submodule--helper run-update-procedure: learn --remote
  submodule--helper: remove ensure-core-worktree
  submodule--helper update-clone: learn --init
  submodule--helper: move functions around
  submodule--helper: reduce logic in run_update_procedure()
  fixup! submodule--helper run-update-procedure: remove --suboid
  fixup! submodule--helper run-update-procedure: learn --remote
  fixup! submodule: move core cmd_update() logic to C

Ævar Arnfjörð Bjarmason (3):
  builtin/submodule--helper.c: rename option variables to "opt"
  submodule--helper: don't use bitfield indirection for parse_options()
  submodule tests: test for init and update failure output

 builtin/submodule--helper.c    | 695 ++++++++++++++++++---------------
 git-submodule.sh               | 153 +-------
 t/t7406-submodule-update.sh    |  14 +-
 t/t7408-submodule-reference.sh |  14 +-
 4 files changed, 423 insertions(+), 453 deletions(-)

Range-diff against v6:
 1:  79c522cf56 =  1:  86ffb53742 submodule--helper: get remote names from any repository
 2:  0b97034d89 =  2:  2a40266b7a submodule--helper: refactor get_submodule_displaypath()
 3:  fedbed87a3 =  3:  cd851c8eb5 submodule--helper: allow setting superprefix for init_submodule()
 4:  8e7868c5d1 =  4:  bfe5cad136 submodule--helper: run update using child process struct
 5:  3dee4b9b15 =  5:  72c257fdbf builtin/submodule--helper.c: reformat designated initializers
 6:  e2cc866e6b =  6:  4b5f703fde builtin/submodule--helper.c: rename option variables to "opt"
 7:  4831695dc6 =  7:  f1d21f5b1c submodule--helper: don't use bitfield indirection for parse_options()
 8:  c2dadfffb2 =  8:  9d32a73fc3 submodule tests: test for init and update failure output
 9:  79ceb88dee =  9:  087bf43aba submodule--helper: remove update-module-mode
10:  6fe25e24da = 10:  4eb2893a19 submodule--helper: reorganize code for sh to C conversion
11:  52c997f97b = 11:  c08e7781e3 submodule--helper run-update-procedure: remove --suboid
12:  61a5b02472 = 12:  2419c37184 submodule--helper run-update-procedure: learn --remote
13:  f76627a078 = 13:  6691fd3648 submodule--helper: remove ensure-core-worktree
14:  2d93c4232b = 14:  d2c9c356e9 submodule--helper update-clone: learn --init
15:  d4899c5635 ! 15:  c8945fcc6f submodule--helper: move functions around
    @@ Metadata
      ## Commit message ##
         submodule--helper: move functions around
     
    -    The next commit will change the internals of several functions and
    +    A subsequent commit will change the internals of several functions and
         arrange them in a more logical manner. Move these functions to their
         final positions so that the diff is smaller.
     
 -:  ---------- > 16:  10af533ae0 submodule--helper: reduce logic in run_update_procedure()
16:  edf752da8d ! 17:  19143f6009 submodule: move core cmd_update() logic to C
    @@ builtin/submodule--helper.c: struct submodule_update_clone {
      };
      #define SUBMODULE_UPDATE_CLONE_INIT { \
      	.list = MODULE_LIST_INIT, \
    -@@ builtin/submodule--helper.c: struct submodule_update_clone {
    - }
    - 
    - struct update_data {
    -+	const char *prefix;
    - 	const char *recursive_prefix;
    - 	const char *sm_path;
    - 	const char *displaypath;
     @@ builtin/submodule--helper.c: struct update_data {
      	struct object_id suboid;
      	struct submodule_update_strategy update_strategy;
    @@ builtin/submodule--helper.c: struct update_data {
      
      static void next_submodule_warn_missing(struct submodule_update_clone *suc,
     @@ builtin/submodule--helper.c: static int run_update_command(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;
    -+	struct submodule_update_strategy strategy = SUBMODULE_UPDATE_STRATEGY_INIT;
    -+
    -+	if (ud->update_strategy.type == SM_UPDATE_UNSPECIFIED || ud->just_cloned)
    -+		determine_submodule_update_strategy(the_repository, ud->just_cloned,
    -+						    ud->sm_path, NULL, &strategy);
    -+	else
    -+		strategy = ud->update_strategy;
    - 
    --	switch (ud->update_strategy.type) {
    -+	switch (strategy.type) {
    - 	case SM_UPDATE_CHECKOUT:
    - 		cp.git_cmd = 1;
    - 		strvec_pushl(&cp.args, "checkout", "-q", NULL);
    -@@ builtin/submodule--helper.c: static int run_update_command(struct update_data *ud, int subforce)
    - 		break;
    - 	case SM_UPDATE_COMMAND:
    - 		cp.use_shell = 1;
    --		strvec_push(&cp.args, ud->update_strategy.command);
    -+		strvec_push(&cp.args, strategy.command);
    - 		must_die_on_failure = 1;
    - 		break;
    - 	default:
    - 		BUG("unexpected update strategy type: %s",
    --		    submodule_strategy_to_string(&ud->update_strategy));
    -+		    submodule_strategy_to_string(&strategy));
    - 	}
    - 	strvec_push(&cp.args, oid);
    - 
    - 	cp.dir = xstrdup(ud->sm_path);
    - 	prepare_submodule_repo_env(&cp.env_array);
      	if (run_command(&cp)) {
    --		switch (ud->update_strategy.type) {
    -+		switch (strategy.type) {
    + 		switch (ud->update_strategy.type) {
      		case SM_UPDATE_CHECKOUT:
     -			printf(_("Unable to checkout '%s' in submodule path '%s'"),
     -			       oid, ud->displaypath);
    @@ builtin/submodule--helper.c: static int run_update_command(struct update_data *u
      		case SM_UPDATE_REBASE:
     -			printf(_("Unable to rebase '%s' in submodule path '%s'"),
     -			       oid, ud->displaypath);
    -+			if (!must_die_on_failure)
    -+				break;
    -+			die(_("Unable to rebase '%s' in submodule path '%s'"),
    ++			die_message(_("Unable to rebase '%s' in submodule path '%s'"),
     +			    oid, ud->displaypath);
      			break;
      		case SM_UPDATE_MERGE:
     -			printf(_("Unable to merge '%s' in submodule path '%s'"),
     -			       oid, ud->displaypath);
    -+			if (!must_die_on_failure)
    -+				break;
    -+			die(_("Unable to merge '%s' in submodule path '%s'"),
    ++			die_message(_("Unable to merge '%s' in submodule path '%s'"),
     +			    oid, ud->displaypath);
      			break;
      		case SM_UPDATE_COMMAND:
     -			printf(_("Execution of '%s %s' failed in submodule path '%s'"),
     -			       ud->update_strategy.command, oid, ud->displaypath);
    -+			if (!must_die_on_failure)
    -+				break;
    -+			die(_("Execution of '%s %s' failed in submodule path '%s'"),
    -+			    strategy.command, oid, ud->displaypath);
    ++			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: %s",
    --			    submodule_strategy_to_string(&ud->update_strategy));
    -+			    submodule_strategy_to_string(&strategy));
    + 			    submodule_strategy_to_string(&ud->update_strategy));
      		}
     -		/*
     -		 * NEEDSWORK: We are currently printing to stdout with error
    @@ builtin/submodule--helper.c: static int run_update_command(struct update_data *u
     -		 * properly. Once we start handling the error messages within
     -		 * C, we should use die() instead.
     -		 */
    --		if (must_die_on_failure)
    + 		if (must_die_on_failure)
     -			return 2;
     -		/*
     -		 * This signifies to the caller in shell that the command
     -		 * failed without dying
     -		 */
    ++			exit(128);
     +
     +		/* the command failed, but update must continue */
      		return 1;
      	}
      
    --	switch (ud->update_strategy.type) {
     +	if (ud->quiet)
     +		return 0;
     +
    -+	switch (strategy.type) {
    + 	switch (ud->update_strategy.type) {
      	case SM_UPDATE_CHECKOUT:
      		printf(_("Submodule path '%s': checked out '%s'\n"),
    - 		       ud->displaypath, oid);
     @@ builtin/submodule--helper.c: static int run_update_command(struct update_data *ud, int subforce)
    - 		break;
    - 	case SM_UPDATE_COMMAND:
    - 		printf(_("Submodule path '%s': '%s %s'\n"),
    --		       ud->displaypath, ud->update_strategy.command, oid);
    -+		       ud->displaypath, strategy.command, oid);
    - 		break;
    - 	default:
    - 		BUG("unexpected update strategy type: %s",
    --		    submodule_strategy_to_string(&ud->update_strategy));
    -+		    submodule_strategy_to_string(&strategy));
    - 	}
    - 
      	return 0;
      }
      
    @@ builtin/submodule--helper.c: static int do_run_update_procedure(struct update_da
     -static int update_submodule2(struct update_data *update_data);
     -static int run_update_procedure(int argc, const char **argv, const char *prefix)
     -{
    --	char *prefixed_path, *update = NULL;
     -	struct update_data opt = UPDATE_DATA_INIT;
     -
     -	struct option options[] = {
    @@ builtin/submodule--helper.c: static int do_run_update_procedure(struct update_da
     -		OPT_BOOL(0, "just-cloned", &opt.just_cloned,
     -			 N_("overrides update mode in case the repository is a fresh clone")),
     -		OPT_INTEGER(0, "depth", &opt.depth, N_("depth for shallow fetch")),
    --		OPT_STRING(0, "prefix", &prefix,
    +-		OPT_STRING(0, "prefix", &opt.prefix,
     -			   N_("path"),
     -			   N_("path into the working tree")),
    --		OPT_STRING(0, "update", &update,
    +-		OPT_STRING(0, "update", &opt.update_default,
     -			   N_("string"),
     -			   N_("rebase, merge, checkout or none")),
     -		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix, N_("path"),
    @@ builtin/submodule--helper.c: static int do_run_update_procedure(struct update_da
     -
     -	opt.sm_path = argv[0];
     -
    --	if (opt.recursive_prefix)
    --		prefixed_path = xstrfmt("%s%s", opt.recursive_prefix, opt.sm_path);
    --	else
    --		prefixed_path = xstrdup(opt.sm_path);
    --
    --	opt.displaypath = get_submodule_displaypath(prefixed_path, prefix);
    --
    --	determine_submodule_update_strategy(the_repository, opt.just_cloned,
    --					    opt.sm_path, update,
    --					    &opt.update_strategy);
    --
    --	free(prefixed_path);
     -	return update_submodule2(&opt);
     -}
     -
    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *
     -static int update_submodule2(struct update_data *update_data)
     +static void update_data_to_args(struct update_data *update_data, struct strvec *args)
     +{
    -+	const char *update = submodule_strategy_to_string(&update_data->update_strategy);
    -+
     +	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
     +	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
    ++	/*
    ++	 * NEEDSWORK: the equivalent code in git-submodule.sh does not
    ++	 * pass --prefix, so this shouldn't either
    ++	*/
     +	if (update_data->prefix)
     +		strvec_pushl(args, "--prefix", update_data->prefix, NULL);
     +	if (update_data->recursive_prefix)
    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *
     +		strvec_push(args, "--require-init");
     +	if (update_data->depth)
     +		strvec_pushf(args, "--depth=%d", update_data->depth);
    -+	if (update)
    -+		strvec_pushl(args, "--update", update, NULL);
    ++	if (update_data->update_default)
    ++		strvec_pushl(args, "--update", update_data->update_default, NULL);
     +	if (update_data->references.nr) {
     +		struct string_list_item *item;
     +		for_each_string_list_item(item, &update_data->references)
    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *
     +
     +static int update_submodule(struct update_data *update_data)
      {
    -+	char *prefixed_path;
    -+
    - 	ensure_core_worktree(update_data->sm_path);
    + 	char *prefixed_path;
      
    -+	if (update_data->recursive_prefix)
    -+		prefixed_path = xstrfmt("%s%s", update_data->recursive_prefix,
    -+					update_data->sm_path);
    -+	else
    -+		prefixed_path = xstrdup(update_data->sm_path);
    -+
    -+	update_data->displaypath = get_submodule_displaypath(prefixed_path,
    -+							     update_data->prefix);
    -+	free(prefixed_path);
    -+
    - 	/* NEEDSWORK: fix the style issues e.g. braces */
    - 	if (update_data->just_cloned) {
    - 		oidcpy(&update_data->suboid, null_oid());
     @@ builtin/submodule--helper.c: static int update_submodule2(struct update_data *update_data)
      	}
      
    @@ builtin/submodule--helper.c: static int update_submodule2(struct update_data *up
     -		return do_run_update_procedure(update_data);
     +		if (run_update_procedure(update_data))
     +			return 1;
    - 
    --	return 3;
    ++
     +	if (update_data->recursive) {
     +		struct child_process cp = CHILD_PROCESS_INIT;
     +		struct update_data next = *update_data;
    @@ builtin/submodule--helper.c: static int update_submodule2(struct update_data *up
     +		cp.git_cmd = 1;
     +		prepare_submodule_repo_env(&cp.env_array);
     +		update_data_to_args(&next, &cp.args);
    -+
    + 
    +-	return 3;
     +		/* die() if child process die()'d */
     +		res = run_command(&cp);
     +		if (!res)
    @@ builtin/submodule--helper.c: static int update_submodules(struct submodule_updat
     -static int update_clone(int argc, const char **argv, const char *prefix)
     +static int module_update(int argc, const char **argv, const char *prefix)
      {
    - 	const char *update = NULL;
    +-	const char *update = NULL;
      	struct pathspec pathspec;
     -	struct submodule_update_clone opt = SUBMODULE_UPDATE_CLONE_INIT;
     +	struct update_data opt = UPDATE_DATA_INIT;
    @@ builtin/submodule--helper.c: static int update_submodules(struct submodule_updat
      			   N_("path"),
      			   N_("path into the working tree")),
      		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
    -@@ builtin/submodule--helper.c: static int update_clone(int argc, const char **argv, const char *prefix)
    + 			   N_("path"),
    + 			   N_("path into the working tree, across nested "
    + 			      "submodule boundaries")),
    +-		OPT_STRING(0, "update", &update,
    ++		OPT_STRING(0, "update", &opt.update_default,
      			   N_("string"),
      			   N_("rebase, merge, checkout or none")),
      		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
    @@ builtin/submodule--helper.c: static int update_clone(int argc, const char **argv
     +	oidcpy(&opt.oid, null_oid());
     +	oidcpy(&opt.suboid, null_oid());
      
    - 	if (update)
    +-	if (update)
     -		if (parse_submodule_update_strategy(update, &opt.update) < 0)
    -+		if (parse_submodule_update_strategy(update,
    ++	if (opt.update_default)
    ++		if (parse_submodule_update_strategy(opt.update_default,
     +						    &opt.update_strategy) < 0)
      			die(_("bad value for update parameter"));
      
    @@ builtin/submodule--helper.c: static struct cmd_struct commands[] = {
      	{"init", module_init, SUPPORT_SUPER_PREFIX},
     
      ## git-submodule.sh ##
    +@@ git-submodule.sh: single_branch=
    + jobs=
    + recommend_shallow=
    + 
    ++# NEEDSWORK this is now unused
    + die_if_unmatched ()
    + {
    + 	if test "$1" = "#unmatched"
     @@ git-submodule.sh: cmd_update()
      		shift
      	done
      
     -	{
     -	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update-clone \
    ++	# NEEDSWORK --super-prefix isn't actually supported by this
    ++	# command - we just pass the $prefix to --recursive-prefix.
     +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper update \
      		${GIT_QUIET:+--quiet} \
     -		${progress:+"--progress"} \
 -:  ---------- > 18:  9d0afc60ec fixup! submodule--helper run-update-procedure: remove --suboid
 -:  ---------- > 19:  e700861239 fixup! submodule--helper run-update-procedure: learn --remote
 -:  ---------- > 20:  aef1a03b44 fixup! submodule: move core cmd_update() logic to C

base-commit: b23dac905bde28da47543484320db16312c87551
-- 
2.33.GIT

[PATCH v7 01/20] submodule--helper: get remote names from any repository

From: Glen Choo <hidden>
Date: 2022-02-10 09:28:56

From: Atharva Raykar <redacted>

`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 the next patch, which will require this
functionality.

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Helped-by: Glen Choo [off-list ref]
Signed-off-by: Atharva Raykar <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 builtin/submodule--helper.c | 39 +++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c5d3fc3817..4c7c1e1432 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -29,11 +29,14 @@
 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);
+	int ignore_errno;
+	const char *refname = refs_resolve_ref_unsafe(store, "HEAD", 0, NULL,
+						      NULL, &ignore_errno);
 
 	if (!refname)
 		die(_("No such ref: %s"), "HEAD");
@@ -46,7 +49,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;
@@ -55,6 +58,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;
@@ -1341,9 +1357,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))
@@ -1382,21 +1397,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");
 
-- 
2.33.GIT

[PATCH v7 02/20] submodule--helper: refactor get_submodule_displaypath()

From: Glen Choo <hidden>
Date: 2022-02-10 09:28:58

From: Atharva Raykar <redacted>

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 [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Signed-off-by: Atharva Raykar <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 builtin/submodule--helper.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4c7c1e1432..5efceb9d46 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -261,11 +261,8 @@ 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);
@@ -281,6 +278,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;
-- 
2.33.GIT

[PATCH v7 03/20] submodule--helper: allow setting superprefix for init_submodule()

From: Glen Choo <hidden>
Date: 2022-02-10 09:28:59

From: Atharva Raykar <redacted>

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 [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Signed-off-by: Atharva Raykar <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 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 5efceb9d46..09cda67c1e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -606,18 +606,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);
 
@@ -691,7 +695,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)
-- 
2.33.GIT

[PATCH v7 04/20] submodule--helper: run update using child process struct

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:01

From: Atharva Raykar <redacted>

We switch to using the run-command API function that takes a
'struct child process', since we are using a lot of the options. This
will also make it simple to switch over to using 'capture_command()'
when we start handling the output of the command completely in C.

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Signed-off-by: Atharva Raykar <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 builtin/submodule--helper.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 09cda67c1e..db71e6f4ec 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2344,47 +2344,45 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str
 
 static int run_update_command(struct update_data *ud, int subforce)
 {
-	struct strvec args = STRVEC_INIT;
-	struct strvec child_env = STRVEC_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
 	char *oid = oid_to_hex(&ud->oid);
 	int must_die_on_failure = 0;
-	int git_cmd;
 
 	switch (ud->update_strategy.type) {
 	case SM_UPDATE_CHECKOUT:
-		git_cmd = 1;
-		strvec_pushl(&args, "checkout", "-q", NULL);
+		cp.git_cmd = 1;
+		strvec_pushl(&cp.args, "checkout", "-q", NULL);
 		if (subforce)
-			strvec_push(&args, "-f");
+			strvec_push(&cp.args, "-f");
 		break;
 	case SM_UPDATE_REBASE:
-		git_cmd = 1;
-		strvec_push(&args, "rebase");
+		cp.git_cmd = 1;
+		strvec_push(&cp.args, "rebase");
 		if (ud->quiet)
-			strvec_push(&args, "--quiet");
+			strvec_push(&cp.args, "--quiet");
 		must_die_on_failure = 1;
 		break;
 	case SM_UPDATE_MERGE:
-		git_cmd = 1;
-		strvec_push(&args, "merge");
+		cp.git_cmd = 1;
+		strvec_push(&cp.args, "merge");
 		if (ud->quiet)
-			strvec_push(&args, "--quiet");
+			strvec_push(&cp.args, "--quiet");
 		must_die_on_failure = 1;
 		break;
 	case SM_UPDATE_COMMAND:
-		git_cmd = 0;
-		strvec_push(&args, ud->update_strategy.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: %s",
 		    submodule_strategy_to_string(&ud->update_strategy));
 	}
-	strvec_push(&args, oid);
+	strvec_push(&cp.args, oid);
 
-	prepare_submodule_repo_env(&child_env);
-	if (run_command_v_opt_cd_env(args.v, git_cmd ? RUN_GIT_CMD : RUN_USING_SHELL,
-				     ud->sm_path, child_env.v)) {
+	cp.dir = xstrdup(ud->sm_path);
+	prepare_submodule_repo_env(&cp.env_array);
+	if (run_command(&cp)) {
 		switch (ud->update_strategy.type) {
 		case SM_UPDATE_CHECKOUT:
 			printf(_("Unable to checkout '%s' in submodule path '%s'"),
-- 
2.33.GIT

[PATCH v7 05/20] builtin/submodule--helper.c: reformat designated initializers

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:07

From: Atharva Raykar <redacted>

The second hunk here will make a subsequent commit's diff smaller, and
let's do the first and third hunks while we're at it so that we
consistently format all of these.

Signed-off-by: Atharva Raykar <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 builtin/submodule--helper.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index db71e6f4ec..9f79bdf4d5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1654,7 +1654,10 @@ struct module_clone_data {
 	unsigned int require_init: 1;
 	int single_branch;
 };
-#define MODULE_CLONE_DATA_INIT { .reference = STRING_LIST_INIT_NODUP, .single_branch = -1 }
+#define MODULE_CLONE_DATA_INIT { \
+	.reference = STRING_LIST_INIT_NODUP, \
+	.single_branch = -1, \
+}
 
 struct submodule_alternate_setup {
 	const char *submodule_name;
@@ -2047,7 +2050,9 @@ struct update_data {
 	unsigned int nofetch: 1;
 	unsigned int just_cloned: 1;
 };
-#define UPDATE_DATA_INIT { .update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT }
+#define UPDATE_DATA_INIT { \
+	.update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT, \
+}
 
 static void next_submodule_warn_missing(struct submodule_update_clone *suc,
 		struct strbuf *out, const char *displaypath)
@@ -3013,7 +3018,9 @@ struct add_data {
 	unsigned int progress: 1;
 	unsigned int dissociate: 1;
 };
-#define ADD_DATA_INIT { .depth = -1 }
+#define ADD_DATA_INIT { \
+	.depth = -1, \
+}
 
 static void append_fetch_remotes(struct strbuf *msg, const char *git_dir_path)
 {
-- 
2.33.GIT

[PATCH v7 06/20] builtin/submodule--helper.c: rename option variables to "opt"

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:09

From: Ævar Arnfjörð Bjarmason <redacted>

Rename the "suc" variable in update_clone() to "opt", and do the same
for the "update_data" variable in run_update_procedure().

The only reason for this change is to make the subsequent commit's
diff smaller, by doing this rename we can "anchor" the diff better, as
it "borrow" most of the options declared here as-is as far as the diff
rename detection is concerned.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 builtin/submodule--helper.c | 74 ++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9f79bdf4d5..c2d4fd0347 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2517,36 +2517,36 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 {
 	const char *update = NULL;
 	struct pathspec pathspec;
-	struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;
+	struct submodule_update_clone opt = SUBMODULE_UPDATE_CLONE_INIT;
 
 	struct option module_update_clone_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
-		OPT_STRING(0, "recursive-prefix", &suc.recursive_prefix,
+		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
 			   N_("path"),
 			   N_("path into the working tree, across nested "
 			      "submodule boundaries")),
 		OPT_STRING(0, "update", &update,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
-		OPT_STRING_LIST(0, "reference", &suc.references, N_("repo"),
+		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
 			   N_("reference repository")),
-		OPT_BOOL(0, "dissociate", &suc.dissociate,
+		OPT_BOOL(0, "dissociate", &opt.dissociate,
 			   N_("use --reference only while cloning")),
-		OPT_STRING(0, "depth", &suc.depth, "<depth>",
+		OPT_STRING(0, "depth", &opt.depth, "<depth>",
 			   N_("create a shallow clone truncated to the "
 			      "specified number of revisions")),
-		OPT_INTEGER('j', "jobs", &suc.max_jobs,
+		OPT_INTEGER('j', "jobs", &opt.max_jobs,
 			    N_("parallel jobs")),
-		OPT_BOOL(0, "recommend-shallow", &suc.recommend_shallow,
+		OPT_BOOL(0, "recommend-shallow", &opt.recommend_shallow,
 			    N_("whether the initial clone should follow the shallow recommendation")),
-		OPT__QUIET(&suc.quiet, N_("don't print cloning progress")),
-		OPT_BOOL(0, "progress", &suc.progress,
+		OPT__QUIET(&opt.quiet, N_("don't print cloning progress")),
+		OPT_BOOL(0, "progress", &opt.progress,
 			    N_("force cloning progress")),
-		OPT_BOOL(0, "require-init", &suc.require_init,
+		OPT_BOOL(0, "require-init", &opt.require_init,
 			   N_("disallow cloning into non-empty directory")),
-		OPT_BOOL(0, "single-branch", &suc.single_branch,
+		OPT_BOOL(0, "single-branch", &opt.single_branch,
 			 N_("clone only one branch, HEAD or --branch")),
 		OPT_END()
 	};
@@ -2555,32 +2555,32 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 		N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"),
 		NULL
 	};
-	suc.prefix = prefix;
+	opt.prefix = prefix;
 
-	update_clone_config_from_gitmodules(&suc.max_jobs);
-	git_config(git_update_clone_config, &suc.max_jobs);
+	update_clone_config_from_gitmodules(&opt.max_jobs);
+	git_config(git_update_clone_config, &opt.max_jobs);
 
 	argc = parse_options(argc, argv, prefix, module_update_clone_options,
 			     git_submodule_helper_usage, 0);
 
 	if (update)
-		if (parse_submodule_update_strategy(update, &suc.update) < 0)
+		if (parse_submodule_update_strategy(update, &opt.update) < 0)
 			die(_("bad value for update parameter"));
 
-	if (module_list_compute(argc, argv, prefix, &pathspec, &suc.list) < 0)
+	if (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0)
 		return 1;
 
 	if (pathspec.nr)
-		suc.warn_if_uninitialized = 1;
+		opt.warn_if_uninitialized = 1;
 
-	return update_submodules(&suc);
+	return update_submodules(&opt);
 }
 
 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 update_data opt = UPDATE_DATA_INIT;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("suppress output for update by rebase or merge")),
@@ -2589,20 +2589,20 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
 			 N_("don't fetch new objects from the remote site")),
 		OPT_BOOL(0, "just-cloned", &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_INTEGER(0, "depth", &opt.depth, N_("depth for shallow fetch")),
 		OPT_STRING(0, "prefix", &prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
 		OPT_STRING(0, "update", &update,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
-		OPT_STRING(0, "recursive-prefix", &update_data.recursive_prefix, N_("path"),
+		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix, N_("path"),
 			   N_("path into the working tree, across nested "
 			      "submodule boundaries")),
-		OPT_CALLBACK_F(0, "oid", &update_data.oid, N_("sha1"),
+		OPT_CALLBACK_F(0, "oid", &opt.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"),
+		OPT_CALLBACK_F(0, "suboid", &opt.suboid, N_("subsha1"),
 			       N_("SHA1 of submodule's HEAD"), PARSE_OPT_NONEG,
 			       parse_opt_object_id),
 		OPT_END()
@@ -2618,27 +2618,27 @@ 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];
+	opt.force = !!force;
+	opt.quiet = !!quiet;
+	opt.nofetch = !!nofetch;
+	opt.just_cloned = !!just_cloned;
+	opt.sm_path = argv[0];
 
-	if (update_data.recursive_prefix)
-		prefixed_path = xstrfmt("%s%s", update_data.recursive_prefix, update_data.sm_path);
+	if (opt.recursive_prefix)
+		prefixed_path = xstrfmt("%s%s", opt.recursive_prefix, opt.sm_path);
 	else
-		prefixed_path = xstrdup(update_data.sm_path);
+		prefixed_path = xstrdup(opt.sm_path);
 
-	update_data.displaypath = get_submodule_displaypath(prefixed_path, prefix);
+	opt.displaypath = get_submodule_displaypath(prefixed_path, prefix);
 
-	determine_submodule_update_strategy(the_repository, update_data.just_cloned,
-					    update_data.sm_path, update,
-					    &update_data.update_strategy);
+	determine_submodule_update_strategy(the_repository, opt.just_cloned,
+					    opt.sm_path, update,
+					    &opt.update_strategy);
 
 	free(prefixed_path);
 
-	if (!oideq(&update_data.oid, &update_data.suboid) || update_data.force)
-		return do_run_update_procedure(&update_data);
+	if (!oideq(&opt.oid, &opt.suboid) || opt.force)
+		return do_run_update_procedure(&opt);
 
 	return 3;
 }
-- 
2.33.GIT

[PATCH v7 07/20] submodule--helper: don't use bitfield indirection for parse_options()

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:10

From: Ævar Arnfjörð Bjarmason <redacted>

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.

This will also make a subsequent large commit's diff smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 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 c2d4fd0347..4a0890954e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2045,10 +2045,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 opt = 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(&opt.quiet,
+			   N_("suppress output for update by rebase or merge")),
+		OPT__FORCE(&opt.force, N_("force checkout updates"),
+			   0),
+		OPT_BOOL('N', "no-fetch", &opt.nofetch,
 			 N_("don't fetch new objects from the remote site")),
-		OPT_BOOL(0, "just-cloned", &just_cloned,
+		OPT_BOOL(0, "just-cloned", &opt.just_cloned,
 			 N_("overrides update mode in case the repository is a fresh clone")),
 		OPT_INTEGER(0, "depth", &opt.depth, N_("depth for shallow fetch")),
 		OPT_STRING(0, "prefix", &prefix,
@@ -2618,10 +2619,6 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
 	if (argc != 1)
 		usage_with_options(usage, options);
 
-	opt.force = !!force;
-	opt.quiet = !!quiet;
-	opt.nofetch = !!nofetch;
-	opt.just_cloned = !!just_cloned;
 	opt.sm_path = argv[0];
 
 	if (opt.recursive_prefix)
-- 
2.33.GIT

[PATCH v7 08/20] submodule tests: test for init and update failure output

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:12

From: Ævar Arnfjörð Bjarmason <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 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.raw >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.raw >actual &&
+		test_cmp expect actual
 	)
 '
 
-- 
2.33.GIT

[PATCH v7 09/20] submodule--helper: remove update-module-mode

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:23

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 <redacted>
---
 builtin/submodule--helper.c | 24 ------------------------
 1 file changed, 24 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4a0890954e..e0cc1c1b79 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1965,29 +1965,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> <path> [<update>]");
-
-	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;
@@ -3388,7 +3365,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},
-- 
2.33.GIT

[PATCH v7 11/20] submodule--helper run-update-procedure: remove --suboid

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:26

Teach run-update-procedure to determine the oid of the submodule's HEAD
instead of doing it in git-subomdule.sh.

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 12 +++++++++---
 git-submodule.sh            |  8 +-------
 2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 0b5120734a..a26477ce04 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2585,9 +2585,6 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F(0, "oid", &opt.oid, N_("sha1"),
 			       N_("SHA1 expected by superproject"), PARSE_OPT_NONEG,
 			       parse_opt_object_id),
-		OPT_CALLBACK_F(0, "suboid", &opt.suboid, N_("subsha1"),
-			       N_("SHA1 of submodule's HEAD"), PARSE_OPT_NONEG,
-			       parse_opt_object_id),
 		OPT_END()
 	};
 
@@ -2982,6 +2979,15 @@ static int module_set_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)
 {
+	/* NEEDSWORK: fix the style issues e.g. braces */
+	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 652861aa66..d48c314f01 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -391,14 +391,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"
@@ -426,7 +421,6 @@ cmd_update()
 			  ${update:+--update "$update"} \
 			  ${prefix:+--recursive-prefix "$prefix"} \
 			  ${sha1:+--oid "$sha1"} \
-			  ${subsha1:+--suboid "$subsha1"} \
 			  "--" \
 			  "$sm_path")
 
-- 
2.33.GIT

[PATCH v7 10/20] submodule--helper: reorganize code for sh to C conversion

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:28

Introduce a function, update_submodule2(), that performs an update for
one submodule. This function will implement the functionality of
run-update-procedure and its surrounding shell code in submodule.sh.

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e0cc1c1b79..0b5120734a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2553,6 +2553,11 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	return update_submodules(&opt);
 }
 
+/*
+ * NEEDSWORK: Use a forward declaration to avoid moving
+ * run_update_procedure() (which will be removed soon).
+ */
+static int update_submodule2(struct update_data *update_data);
 static int run_update_procedure(int argc, const char **argv, const char *prefix)
 {
 	char *prefixed_path, *update = NULL;
@@ -2610,11 +2615,7 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
 					    &opt.update_strategy);
 
 	free(prefixed_path);
-
-	if (!oideq(&opt.oid, &opt.suboid) || opt.force)
-		return do_run_update_procedure(&opt);
-
-	return 3;
+	return update_submodule2(&opt);
 }
 
 static int resolve_relative_path(int argc, const char **argv, const char *prefix)
@@ -2978,6 +2979,15 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 	return !!ret;
 }
 
+/* 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;
-- 
2.33.GIT

[PATCH v7 12/20] submodule--helper run-update-procedure: learn --remote

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:35

Teach run-update-procedure to handle --remote instead of parsing
--remote in git-submodule.sh. As a result, "git submodule--helper
print-default-remote" has no more callers, so remove it.

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 39 ++++++++++++++++++++++---------------
 git-submodule.sh            | 30 +---------------------------
 2 files changed, 24 insertions(+), 45 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a26477ce04..15ae986692 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -71,21 +71,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]);
@@ -2026,6 +2011,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, \
@@ -2585,6 +2571,8 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F(0, "oid", &opt.oid, N_("sha1"),
 			       N_("SHA1 expected by superproject"), PARSE_OPT_NONEG,
 			       parse_opt_object_id),
+		OPT_BOOL(0, "remote", &opt.remote,
+			 N_("use SHA-1 of submodule's remote tracking branch")),
 		OPT_END()
 	};
 
@@ -2988,6 +2976,25 @@ static int update_submodule2(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);
+
+		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);
 
@@ -3389,10 +3396,10 @@ 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},
+	/* NEEDSWORK: remote-branch is also obsolete */
 	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"push-check", push_check, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
diff --git a/git-submodule.sh b/git-submodule.sh
index d48c314f01..29fd69250d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -246,20 +246,6 @@ cmd_deinit()
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- "$@"
 }
 
-# usage: fetch_in_submodule <module_path> [<depth>] [<sha1>]
-# Because arguments are positional, use an empty string to omit <depth>
-# but include <sha1>.
-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
 #
@@ -396,21 +382,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} \
@@ -421,6 +392,7 @@ cmd_update()
 			  ${update:+--update "$update"} \
 			  ${prefix:+--recursive-prefix "$prefix"} \
 			  ${sha1:+--oid "$sha1"} \
+			  ${remote:+--remote} \
 			  "--" \
 			  "$sm_path")
 
-- 
2.33.GIT

[PATCH v7 13/20] submodule--helper: remove ensure-core-worktree

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:38

Move the logic of "git submodule--helper ensure-core-worktree" into
run-update-procedure. Since the ensure-core-worktree command is
obsolete, remove it.

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 13 +++----------
 git-submodule.sh            |  2 --
 2 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 15ae986692..a05aea5cd6 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2746,17 +2746,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>");
-
-	path = argv[1];
-
 	if (repo_submodule_init(&subrepo, the_repository, path, null_oid()))
 		die(_("could not get a repository handle for submodule '%s'"), path);
 
@@ -2776,8 +2770,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)
@@ -2967,6 +2959,8 @@ static int module_set_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);
+
 	/* NEEDSWORK: fix the style issues e.g. braces */
 	if (update_data->just_cloned) {
 		oidcpy(&update_data->suboid, null_oid());
@@ -3390,7 +3384,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 29fd69250d..aa9c898e1c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -373,8 +373,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
-- 
2.33.GIT

[PATCH v7 14/20] submodule--helper update-clone: learn --init

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:40

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 <redacted>
---
 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 a05aea5cd6..5635f0c48b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1989,6 +1989,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, \
@@ -2483,6 +2484,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	struct submodule_update_clone opt = SUBMODULE_UPDATE_CLONE_INIT;
 
 	struct option module_update_clone_options[] = {
+		OPT_BOOL(0, "init", &opt.init,
+			 N_("initialize uninitialized submodules before update")),
 		OPT_STRING(0, "prefix", &prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
@@ -2536,6 +2539,29 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	if (pathspec.nr)
 		opt.warn_if_uninitialized = 1;
 
+	if (opt.init) {
+		struct module_list list = MODULE_LIST_INIT;
+		struct init_cb info = INIT_CB_INIT;
+
+		if (module_list_compute(argc, argv, opt.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 = opt.prefix;
+		info.superprefix = opt.recursive_prefix;
+		if (opt.quiet)
+			info.flags |= OPT_QUIET;
+
+		for_each_listed_submodule(&list, init_submodule_cb, &info);
+	}
+
 	return update_submodules(&opt);
 }
 
diff --git a/git-submodule.sh b/git-submodule.sh
index aa9c898e1c..3ccf2388bf 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -347,14 +347,11 @@ cmd_update()
 		shift
 	done
 
-	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"} \
-- 
2.33.GIT

[PATCH v7 15/20] submodule--helper: move functions around

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:49

A subsequent commit will change the internals of several functions and
arrange them in a more logical manner. Move these functions to their
final positions so that the diff is smaller.

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 228 ++++++++++++++++++------------------
 1 file changed, 114 insertions(+), 114 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5635f0c48b..0ab8f9d49f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2451,120 +2451,6 @@ static void update_submodule(struct update_clone_data *ucd)
 		ucd->sub->path);
 }
 
-static int update_submodules(struct submodule_update_clone *suc)
-{
-	int i;
-
-	run_processes_parallel_tr2(suc->max_jobs, update_clone_get_next_task,
-				   update_clone_start_failure,
-				   update_clone_task_finished, suc, "submodule",
-				   "parallel/update");
-
-	/*
-	 * We saved the output and put it out all at once now.
-	 * That means:
-	 * - the listener does not have to interleave their (checkout)
-	 *   work with our fetching.  The writes involved in a
-	 *   checkout involve more straightforward sequential I/O.
-	 * - the listener can avoid doing any work if fetching failed.
-	 */
-	if (suc->quickstop)
-		return 1;
-
-	for (i = 0; i < suc->update_clone_nr; i++)
-		update_submodule(&suc->update_clone[i]);
-
-	return 0;
-}
-
-static int update_clone(int argc, const char **argv, const char *prefix)
-{
-	const char *update = NULL;
-	struct pathspec pathspec;
-	struct submodule_update_clone opt = SUBMODULE_UPDATE_CLONE_INIT;
-
-	struct option module_update_clone_options[] = {
-		OPT_BOOL(0, "init", &opt.init,
-			 N_("initialize uninitialized submodules before update")),
-		OPT_STRING(0, "prefix", &prefix,
-			   N_("path"),
-			   N_("path into the working tree")),
-		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
-			   N_("path"),
-			   N_("path into the working tree, across nested "
-			      "submodule boundaries")),
-		OPT_STRING(0, "update", &update,
-			   N_("string"),
-			   N_("rebase, merge, checkout or none")),
-		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
-			   N_("reference repository")),
-		OPT_BOOL(0, "dissociate", &opt.dissociate,
-			   N_("use --reference only while cloning")),
-		OPT_STRING(0, "depth", &opt.depth, "<depth>",
-			   N_("create a shallow clone truncated to the "
-			      "specified number of revisions")),
-		OPT_INTEGER('j', "jobs", &opt.max_jobs,
-			    N_("parallel jobs")),
-		OPT_BOOL(0, "recommend-shallow", &opt.recommend_shallow,
-			    N_("whether the initial clone should follow the shallow recommendation")),
-		OPT__QUIET(&opt.quiet, N_("don't print cloning progress")),
-		OPT_BOOL(0, "progress", &opt.progress,
-			    N_("force cloning progress")),
-		OPT_BOOL(0, "require-init", &opt.require_init,
-			   N_("disallow cloning into non-empty directory")),
-		OPT_BOOL(0, "single-branch", &opt.single_branch,
-			 N_("clone only one branch, HEAD or --branch")),
-		OPT_END()
-	};
-
-	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"),
-		NULL
-	};
-	opt.prefix = prefix;
-
-	update_clone_config_from_gitmodules(&opt.max_jobs);
-	git_config(git_update_clone_config, &opt.max_jobs);
-
-	argc = parse_options(argc, argv, prefix, module_update_clone_options,
-			     git_submodule_helper_usage, 0);
-
-	if (update)
-		if (parse_submodule_update_strategy(update, &opt.update) < 0)
-			die(_("bad value for update parameter"));
-
-	if (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0)
-		return 1;
-
-	if (pathspec.nr)
-		opt.warn_if_uninitialized = 1;
-
-	if (opt.init) {
-		struct module_list list = MODULE_LIST_INIT;
-		struct init_cb info = INIT_CB_INIT;
-
-		if (module_list_compute(argc, argv, opt.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 = opt.prefix;
-		info.superprefix = opt.recursive_prefix;
-		if (opt.quiet)
-			info.flags |= OPT_QUIET;
-
-		for_each_listed_submodule(&list, init_submodule_cb, &info);
-	}
-
-	return update_submodules(&opt);
-}
-
 /*
  * NEEDSWORK: Use a forward declaration to avoid moving
  * run_update_procedure() (which will be removed soon).
@@ -3021,6 +2907,120 @@ static int update_submodule2(struct update_data *update_data)
 	return 3;
 }
 
+static int update_submodules(struct submodule_update_clone *suc)
+{
+	int i;
+
+	run_processes_parallel_tr2(suc->max_jobs, update_clone_get_next_task,
+				   update_clone_start_failure,
+				   update_clone_task_finished, suc, "submodule",
+				   "parallel/update");
+
+	/*
+	 * We saved the output and put it out all at once now.
+	 * That means:
+	 * - the listener does not have to interleave their (checkout)
+	 *   work with our fetching.  The writes involved in a
+	 *   checkout involve more straightforward sequential I/O.
+	 * - the listener can avoid doing any work if fetching failed.
+	 */
+	if (suc->quickstop)
+		return 1;
+
+	for (i = 0; i < suc->update_clone_nr; i++)
+		update_submodule(&suc->update_clone[i]);
+
+	return 0;
+}
+
+static int update_clone(int argc, const char **argv, const char *prefix)
+{
+	const char *update = NULL;
+	struct pathspec pathspec;
+	struct submodule_update_clone opt = SUBMODULE_UPDATE_CLONE_INIT;
+
+	struct option module_update_clone_options[] = {
+		OPT_BOOL(0, "init", &opt.init,
+			 N_("initialize uninitialized submodules before update")),
+		OPT_STRING(0, "prefix", &prefix,
+			   N_("path"),
+			   N_("path into the working tree")),
+		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
+			   N_("path"),
+			   N_("path into the working tree, across nested "
+			      "submodule boundaries")),
+		OPT_STRING(0, "update", &update,
+			   N_("string"),
+			   N_("rebase, merge, checkout or none")),
+		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
+			   N_("reference repository")),
+		OPT_BOOL(0, "dissociate", &opt.dissociate,
+			   N_("use --reference only while cloning")),
+		OPT_STRING(0, "depth", &opt.depth, "<depth>",
+			   N_("create a shallow clone truncated to the "
+			      "specified number of revisions")),
+		OPT_INTEGER('j', "jobs", &opt.max_jobs,
+			    N_("parallel jobs")),
+		OPT_BOOL(0, "recommend-shallow", &opt.recommend_shallow,
+			    N_("whether the initial clone should follow the shallow recommendation")),
+		OPT__QUIET(&opt.quiet, N_("don't print cloning progress")),
+		OPT_BOOL(0, "progress", &opt.progress,
+			    N_("force cloning progress")),
+		OPT_BOOL(0, "require-init", &opt.require_init,
+			   N_("disallow cloning into non-empty directory")),
+		OPT_BOOL(0, "single-branch", &opt.single_branch,
+			 N_("clone only one branch, HEAD or --branch")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"),
+		NULL
+	};
+	opt.prefix = prefix;
+
+	update_clone_config_from_gitmodules(&opt.max_jobs);
+	git_config(git_update_clone_config, &opt.max_jobs);
+
+	argc = parse_options(argc, argv, prefix, module_update_clone_options,
+			     git_submodule_helper_usage, 0);
+
+	if (update)
+		if (parse_submodule_update_strategy(update, &opt.update) < 0)
+			die(_("bad value for update parameter"));
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0)
+		return 1;
+
+	if (pathspec.nr)
+		opt.warn_if_uninitialized = 1;
+
+	if (opt.init) {
+		struct module_list list = MODULE_LIST_INIT;
+		struct init_cb info = INIT_CB_INIT;
+
+		if (module_list_compute(argc, argv, opt.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 = opt.prefix;
+		info.superprefix = opt.recursive_prefix;
+		if (opt.quiet)
+			info.flags |= OPT_QUIET;
+
+		for_each_listed_submodule(&list, init_submodule_cb, &info);
+	}
+
+	return update_submodules(&opt);
+}
+
 struct add_data {
 	const char *prefix;
 	const char *branch;
-- 
2.33.GIT

[PATCH v7 16/20] submodule--helper: reduce logic in run_update_procedure()

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:51

A later commit will combine the "update-clone" and
"run-update-procedure" commands, so run_update_procedure() will be
removed. Prepare for this by moving as much logic as possible out of
run_update_procedure() and into update_submodule2().

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 0ab8f9d49f..ff7ee73e1a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2001,9 +2001,11 @@ struct submodule_update_clone {
 }
 
 struct update_data {
+	const char *prefix;
 	const char *recursive_prefix;
 	const char *sm_path;
 	const char *displaypath;
+	const char *update_default;
 	struct object_id oid;
 	struct object_id suboid;
 	struct submodule_update_strategy update_strategy;
@@ -2458,7 +2460,6 @@ static void update_submodule(struct update_clone_data *ucd)
 static int update_submodule2(struct update_data *update_data);
 static int run_update_procedure(int argc, const char **argv, const char *prefix)
 {
-	char *prefixed_path, *update = NULL;
 	struct update_data opt = UPDATE_DATA_INIT;
 
 	struct option options[] = {
@@ -2471,10 +2472,10 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "just-cloned", &opt.just_cloned,
 			 N_("overrides update mode in case the repository is a fresh clone")),
 		OPT_INTEGER(0, "depth", &opt.depth, N_("depth for shallow fetch")),
-		OPT_STRING(0, "prefix", &prefix,
+		OPT_STRING(0, "prefix", &opt.prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
-		OPT_STRING(0, "update", &update,
+		OPT_STRING(0, "update", &opt.update_default,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
 		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix, N_("path"),
@@ -2500,18 +2501,6 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
 
 	opt.sm_path = argv[0];
 
-	if (opt.recursive_prefix)
-		prefixed_path = xstrfmt("%s%s", opt.recursive_prefix, opt.sm_path);
-	else
-		prefixed_path = xstrdup(opt.sm_path);
-
-	opt.displaypath = get_submodule_displaypath(prefixed_path, prefix);
-
-	determine_submodule_update_strategy(the_repository, opt.just_cloned,
-					    opt.sm_path, update,
-					    &opt.update_strategy);
-
-	free(prefixed_path);
 	return update_submodule2(&opt);
 }
 
@@ -2871,8 +2860,24 @@ static int module_set_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)
 {
+	char *prefixed_path;
+
 	ensure_core_worktree(update_data->sm_path);
 
+	if (update_data->recursive_prefix)
+		prefixed_path = xstrfmt("%s%s", update_data->recursive_prefix,
+					update_data->sm_path);
+	else
+		prefixed_path = xstrdup(update_data->sm_path);
+
+	update_data->displaypath = get_submodule_displaypath(prefixed_path,
+							     update_data->prefix);
+	free(prefixed_path);
+
+	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
+					    update_data->sm_path, update_data->update_default,
+					    &update_data->update_strategy);
+
 	/* NEEDSWORK: fix the style issues e.g. braces */
 	if (update_data->just_cloned) {
 		oidcpy(&update_data->suboid, null_oid());
-- 
2.33.GIT

[PATCH v7 17/20] submodule: move core cmd_update() logic to C

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:53

From: Atharva Raykar <redacted>

This patch completes the conversion past the flag parsing of
`submodule update` by introducing a helper subcommand called
`submodule--helper update`. The behaviour of `submodule update` should
remain the same after this patch.

We add more fields to the `struct update_data` that are required by
`struct submodule_update_clone` to be able to perform a clone, when that
is needed to be done.

Recursing on a submodule is done by calling a subprocess that launches
`submodule--helper update`, with a modified `--recursive-prefix` and
`--prefix` parameter.

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Signed-off-by: Atharva Raykar <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 312 ++++++++++++++++++++++--------------
 git-submodule.sh            | 105 ++----------
 2 files changed, 201 insertions(+), 216 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ff7ee73e1a..b895e88a64 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1977,7 +1977,6 @@ struct submodule_update_clone {
 	const char *prefix;
 	int single_branch;
 
-	/* to be consumed by git-submodule.sh */
 	struct update_clone_data *update_clone;
 	int update_clone_nr; int update_clone_alloc;
 
@@ -1989,7 +1988,6 @@ 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, \
@@ -2010,14 +2008,54 @@ struct update_data {
 	struct object_id suboid;
 	struct submodule_update_strategy update_strategy;
 	int depth;
+	int recommend_shallow;
+	int single_branch;
+	int max_jobs;
+	unsigned int init;
 	unsigned int force;
 	unsigned int quiet;
 	unsigned int nofetch;
 	unsigned int just_cloned;
 	unsigned int remote;
+	unsigned int recursive;
+	unsigned int progress;
+	unsigned int dissociate;
+	unsigned int require_init;
+	unsigned warn_if_uninitialized ;
+	struct string_list references;
+	struct module_list list;
 };
 #define UPDATE_DATA_INIT { \
+	.list = MODULE_LIST_INIT, \
 	.update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT, \
+	.recommend_shallow = -1, \
+	.references = STRING_LIST_INIT_DUP, \
+	.single_branch = -1, \
+	.max_jobs = 1, \
+}
+
+static void update_clone_from_update_data(struct submodule_update_clone *suc,
+					  struct update_data *update_data)
+{
+	suc->prefix = update_data->prefix;
+	suc->recursive_prefix = update_data->recursive_prefix;
+	suc->max_jobs = update_data->max_jobs;
+	suc->progress = update_data->progress;
+	suc->quiet = update_data->quiet;
+	suc->dissociate = update_data->dissociate;
+	suc->require_init = update_data->require_init;
+	suc->single_branch = update_data->single_branch;
+	suc->warn_if_uninitialized = update_data->warn_if_uninitialized;
+	suc->list = update_data->list;
+	suc->update = update_data->update_strategy;
+	suc->recommend_shallow = update_data->recommend_shallow;
+	if (update_data->depth)
+		suc->depth = xstrfmt("--depth=%d", update_data->depth);
+	if (update_data->references.nr) {
+		struct string_list_item *item;
+		for_each_string_list_item(item, &update_data->references)
+			string_list_append(&suc->references, item->string);
+	}
 }
 
 static void next_submodule_warn_missing(struct submodule_update_clone *suc,
@@ -2356,40 +2394,35 @@ static int run_update_command(struct update_data *ud, int subforce)
 	if (run_command(&cp)) {
 		switch (ud->update_strategy.type) {
 		case SM_UPDATE_CHECKOUT:
-			printf(_("Unable to checkout '%s' in submodule path '%s'"),
-			       oid, ud->displaypath);
+			die_message(_("Unable to checkout '%s' in submodule path '%s'"),
+				    oid, ud->displaypath);
 			break;
 		case SM_UPDATE_REBASE:
-			printf(_("Unable to rebase '%s' in submodule path '%s'"),
-			       oid, ud->displaypath);
+			die_message(_("Unable to rebase '%s' in submodule path '%s'"),
+			    oid, ud->displaypath);
 			break;
 		case SM_UPDATE_MERGE:
-			printf(_("Unable to merge '%s' in submodule path '%s'"),
-			       oid, ud->displaypath);
+			die_message(_("Unable to merge '%s' in submodule path '%s'"),
+			    oid, ud->displaypath);
 			break;
 		case SM_UPDATE_COMMAND:
-			printf(_("Execution of '%s %s' failed in submodule path '%s'"),
-			       ud->update_strategy.command, oid, ud->displaypath);
+			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: %s",
 			    submodule_strategy_to_string(&ud->update_strategy));
 		}
-		/*
-		 * NEEDSWORK: We are currently printing to stdout with error
-		 * return so that the shell caller handles the error output
-		 * properly. Once we start handling the error messages within
-		 * C, we should use die() instead.
-		 */
 		if (must_die_on_failure)
-			return 2;
-		/*
-		 * This signifies to the caller in shell that the command
-		 * failed without dying
-		 */
+			exit(128);
+
+		/* the command failed, but update must continue */
 		return 1;
 	}
 
+	if (ud->quiet)
+		return 0;
+
 	switch (ud->update_strategy.type) {
 	case SM_UPDATE_CHECKOUT:
 		printf(_("Submodule path '%s': checked out '%s'\n"),
@@ -2415,7 +2448,7 @@ static int run_update_command(struct update_data *ud, int subforce)
 	return 0;
 }
 
-static int do_run_update_procedure(struct update_data *ud)
+static int run_update_procedure(struct update_data *ud)
 {
 	int subforce = is_null_oid(&ud->suboid) || ud->force;
 
@@ -2445,76 +2478,6 @@ static int do_run_update_procedure(struct update_data *ud)
 	return run_update_command(ud, subforce);
 }
 
-static void update_submodule(struct update_clone_data *ucd)
-{
-	fprintf(stdout, "dummy %s %d\t%s\n",
-		oid_to_hex(&ucd->oid),
-		ucd->just_cloned,
-		ucd->sub->path);
-}
-
-/*
- * NEEDSWORK: Use a forward declaration to avoid moving
- * run_update_procedure() (which will be removed soon).
- */
-static int update_submodule2(struct update_data *update_data);
-static int run_update_procedure(int argc, const char **argv, const char *prefix)
-{
-	struct update_data opt = UPDATE_DATA_INIT;
-
-	struct option options[] = {
-		OPT__QUIET(&opt.quiet,
-			   N_("suppress output for update by rebase or merge")),
-		OPT__FORCE(&opt.force, N_("force checkout updates"),
-			   0),
-		OPT_BOOL('N', "no-fetch", &opt.nofetch,
-			 N_("don't fetch new objects from the remote site")),
-		OPT_BOOL(0, "just-cloned", &opt.just_cloned,
-			 N_("overrides update mode in case the repository is a fresh clone")),
-		OPT_INTEGER(0, "depth", &opt.depth, N_("depth for shallow fetch")),
-		OPT_STRING(0, "prefix", &opt.prefix,
-			   N_("path"),
-			   N_("path into the working tree")),
-		OPT_STRING(0, "update", &opt.update_default,
-			   N_("string"),
-			   N_("rebase, merge, checkout or none")),
-		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix, N_("path"),
-			   N_("path into the working tree, across nested "
-			      "submodule boundaries")),
-		OPT_CALLBACK_F(0, "oid", &opt.oid, N_("sha1"),
-			       N_("SHA1 expected by superproject"), PARSE_OPT_NONEG,
-			       parse_opt_object_id),
-		OPT_BOOL(0, "remote", &opt.remote,
-			 N_("use SHA-1 of submodule's remote tracking branch")),
-		OPT_END()
-	};
-
-	const char *const usage[] = {
-		N_("git submodule--helper run-update-procedure [<options>] <path>"),
-		NULL
-	};
-
-	argc = parse_options(argc, argv, prefix, options, usage, 0);
-
-	if (argc != 1)
-		usage_with_options(usage, options);
-
-	opt.sm_path = argv[0];
-
-	return update_submodule2(&opt);
-}
-
-static int resolve_relative_path(int argc, const char **argv, const char *prefix)
-{
-	struct strbuf sb = STRBUF_INIT;
-	if (argc != 3)
-		die("submodule--helper relative-path takes exactly 2 arguments, got %d", argc);
-
-	printf("%s", relative_path(argv[1], argv[2], &sb));
-	strbuf_release(&sb);
-	return 0;
-}
-
 static const char *remote_submodule_branch(const char *path)
 {
 	const struct submodule *sub;
@@ -2857,8 +2820,53 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 	return !!ret;
 }
 
-/* NEEDSWORK: this is a temporary name until we delete update_submodule() */
-static int update_submodule2(struct update_data *update_data)
+static void update_data_to_args(struct update_data *update_data, struct strvec *args)
+{
+	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
+	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
+	/*
+	 * NEEDSWORK: the equivalent code in git-submodule.sh does not
+	 * pass --prefix, so this shouldn't either
+	*/
+	if (update_data->prefix)
+		strvec_pushl(args, "--prefix", update_data->prefix, NULL);
+	if (update_data->recursive_prefix)
+		strvec_pushl(args, "--recursive-prefix",
+			     update_data->recursive_prefix, NULL);
+	if (update_data->quiet)
+		strvec_push(args, "--quiet");
+	if (update_data->force)
+		strvec_push(args, "--force");
+	if (update_data->init)
+		strvec_push(args, "--init");
+	if (update_data->remote)
+		strvec_push(args, "--remote");
+	if (update_data->nofetch)
+		strvec_push(args, "--no-fetch");
+	if (update_data->dissociate)
+		strvec_push(args, "--dissociate");
+	if (update_data->progress)
+		strvec_push(args, "--progress");
+	if (update_data->require_init)
+		strvec_push(args, "--require-init");
+	if (update_data->depth)
+		strvec_pushf(args, "--depth=%d", update_data->depth);
+	if (update_data->update_default)
+		strvec_pushl(args, "--update", update_data->update_default, NULL);
+	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);
+	}
+	if (update_data->recommend_shallow == 0)
+		strvec_push(args, "--no-recommend-shallow");
+	else if (update_data->recommend_shallow == 1)
+		strvec_push(args, "--recommend-shallow");
+	if (update_data->single_branch >= 0)
+		strvec_push(args, "--single-branch");
+}
+
+static int update_submodule(struct update_data *update_data)
 {
 	char *prefixed_path;
 
@@ -2907,18 +2915,55 @@ 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);
+		if (run_update_procedure(update_data))
+			return 1;
+
+	if (update_data->recursive) {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		struct update_data next = *update_data;
+		int res;
+
+		if (update_data->recursive_prefix)
+			prefixed_path = xstrfmt("%s%s/", update_data->recursive_prefix,
+						update_data->sm_path);
+		else
+			prefixed_path = xstrfmt("%s/", update_data->sm_path);
+
+		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
+								  update_data->prefix);
+		next.prefix = NULL;
+		oidcpy(&next.oid, null_oid());
+		oidcpy(&next.suboid, null_oid());
+
+		cp.dir = update_data->sm_path;
+		cp.git_cmd = 1;
+		prepare_submodule_repo_env(&cp.env_array);
+		update_data_to_args(&next, &cp.args);
 
-	return 3;
+		/* die() if child process die()'d */
+		res = run_command(&cp);
+		if (!res)
+			return 0;
+		die_message(_("Failed to recurse into submodule path '%s'"),
+			    update_data->displaypath);
+		if (res == 128)
+			exit(res);
+		else if (res)
+			return 1;
+	}
+
+	return 0;
 }
 
-static int update_submodules(struct submodule_update_clone *suc)
+static int update_submodules(struct update_data *update_data)
 {
-	int i;
+	int i, res = 0;
+	struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;
 
-	run_processes_parallel_tr2(suc->max_jobs, update_clone_get_next_task,
+	update_clone_from_update_data(&suc, update_data);
+	run_processes_parallel_tr2(suc.max_jobs, update_clone_get_next_task,
 				   update_clone_start_failure,
-				   update_clone_task_finished, suc, "submodule",
+				   update_clone_task_finished, &suc, "submodule",
 				   "parallel/update");
 
 	/*
@@ -2929,50 +2974,69 @@ static int update_submodules(struct submodule_update_clone *suc)
 	 *   checkout involve more straightforward sequential I/O.
 	 * - the listener can avoid doing any work if fetching failed.
 	 */
-	if (suc->quickstop)
-		return 1;
+	if (suc.quickstop) {
+		res = 1;
+		goto cleanup;
+	}
 
-	for (i = 0; i < suc->update_clone_nr; i++)
-		update_submodule(&suc->update_clone[i]);
+	for (i = 0; i < suc.update_clone_nr; i++) {
+		struct update_clone_data ucd = suc.update_clone[i];
 
-	return 0;
+		oidcpy(&update_data->oid, &ucd.oid);
+		update_data->just_cloned = ucd.just_cloned;
+		update_data->sm_path = ucd.sub->path;
+
+		if (update_submodule(update_data))
+			res = 1;
+	}
+
+cleanup:
+	string_list_clear(&update_data->references, 0);
+	return res;
 }
 
-static int update_clone(int argc, const char **argv, const char *prefix)
+static int module_update(int argc, const char **argv, const char *prefix)
 {
-	const char *update = NULL;
 	struct pathspec pathspec;
-	struct submodule_update_clone opt = SUBMODULE_UPDATE_CLONE_INIT;
+	struct update_data opt = UPDATE_DATA_INIT;
 
+	/* NEEDSWORK: update names and strings */
 	struct option module_update_clone_options[] = {
+		OPT__FORCE(&opt.force, N_("force checkout updates"), 0),
 		OPT_BOOL(0, "init", &opt.init,
 			 N_("initialize uninitialized submodules before update")),
-		OPT_STRING(0, "prefix", &prefix,
+		OPT_BOOL(0, "remote", &opt.remote,
+			 N_("use SHA-1 of submodule's remote tracking branch")),
+		OPT_BOOL(0, "recursive", &opt.recursive,
+			 N_("traverse submodules recursively")),
+		OPT_BOOL('N', "no-fetch", &opt.nofetch,
+			 N_("don't fetch new objects from the remote site")),
+		OPT_STRING(0, "prefix", &opt.prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
 		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
 			   N_("path"),
 			   N_("path into the working tree, across nested "
 			      "submodule boundaries")),
-		OPT_STRING(0, "update", &update,
+		OPT_STRING(0, "update", &opt.update_default,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
 		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
-			   N_("reference repository")),
+				N_("reference repository")),
 		OPT_BOOL(0, "dissociate", &opt.dissociate,
-			   N_("use --reference only while cloning")),
-		OPT_STRING(0, "depth", &opt.depth, "<depth>",
-			   N_("create a shallow clone truncated to the "
-			      "specified number of revisions")),
+			 N_("use --reference only while cloning")),
+		OPT_INTEGER(0, "depth", &opt.depth,
+			    N_("create a shallow clone truncated to the "
+			       "specified number of revisions")),
 		OPT_INTEGER('j', "jobs", &opt.max_jobs,
 			    N_("parallel jobs")),
 		OPT_BOOL(0, "recommend-shallow", &opt.recommend_shallow,
-			    N_("whether the initial clone should follow the shallow recommendation")),
+			 N_("whether the initial clone should follow the shallow recommendation")),
 		OPT__QUIET(&opt.quiet, N_("don't print cloning progress")),
 		OPT_BOOL(0, "progress", &opt.progress,
-			    N_("force cloning progress")),
+			 N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &opt.require_init,
-			   N_("disallow cloning into non-empty directory")),
+			 N_("disallow cloning into non-empty directory")),
 		OPT_BOOL(0, "single-branch", &opt.single_branch,
 			 N_("clone only one branch, HEAD or --branch")),
 		OPT_END()
@@ -2982,16 +3046,18 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 		N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"),
 		NULL
 	};
-	opt.prefix = prefix;
 
 	update_clone_config_from_gitmodules(&opt.max_jobs);
 	git_config(git_update_clone_config, &opt.max_jobs);
 
 	argc = parse_options(argc, argv, prefix, module_update_clone_options,
 			     git_submodule_helper_usage, 0);
+	oidcpy(&opt.oid, null_oid());
+	oidcpy(&opt.suboid, null_oid());
 
-	if (update)
-		if (parse_submodule_update_strategy(update, &opt.update) < 0)
+	if (opt.update_default)
+		if (parse_submodule_update_strategy(opt.update_default,
+						    &opt.update_strategy) < 0)
 			die(_("bad value for update parameter"));
 
 	if (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0)
@@ -3413,9 +3479,7 @@ static struct cmd_struct commands[] = {
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
 	{"add", module_add, SUPPORT_SUPER_PREFIX},
-	{"update-clone", update_clone, 0},
-	{"run-update-procedure", run_update_procedure, 0},
-	{"relative-path", resolve_relative_path, 0},
+	{"update", module_update, 0},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},
diff --git a/git-submodule.sh b/git-submodule.sh
index 3ccf2388bf..d176469fb1 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -50,6 +50,7 @@ single_branch=
 jobs=
 recommend_shallow=
 
+# NEEDSWORK this is now unused
 die_if_unmatched ()
 {
 	if test "$1" = "#unmatched"
@@ -347,108 +348,28 @@ cmd_update()
 		shift
 	done
 
-	{
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update-clone \
+	# NEEDSWORK --super-prefix isn't actually supported by this
+	# command - we just pass the $prefix to --recursive-prefix.
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper update \
 		${GIT_QUIET:+--quiet} \
-		${progress:+"--progress"} \
+		${force:+--force} \
+		${progress:+--progress} \
+		${dissociate:+--dissociate} \
+		${remote:+--remote} \
+		${recursive:+--recursive} \
 		${init:+--init} \
+		${require_init:+--require-init} \
+		${nofetch:+--no-fetch} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
 		${prefix:+--recursive-prefix "$prefix"} \
 		${update:+--update "$update"} \
 		${reference:+"$reference"} \
-		${dissociate:+"--dissociate"} \
-		${depth:+--depth "$depth"} \
-		${require_init:+--require-init} \
+		${depth:+"$depth"} \
 		$single_branch \
 		$recommend_shallow \
 		$jobs \
 		-- \
-		"$@" || echo "#unmatched" $?
-	} | {
-	err=
-	while read -r quickabort sha1 just_cloned sm_path
-	do
-		die_if_unmatched "$quickabort" "$sha1"
-
-		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-
-		if test $just_cloned -eq 0
-		then
-			just_cloned=
-		fi
-
-		out=$(git submodule--helper run-update-procedure \
-			  ${wt_prefix:+--prefix "$wt_prefix"} \
-			  ${GIT_QUIET:+--quiet} \
-			  ${force:+--force} \
-			  ${just_cloned:+--just-cloned} \
-			  ${nofetch:+--no-fetch} \
-			  ${depth:+"$depth"} \
-			  ${update:+--update "$update"} \
-			  ${prefix:+--recursive-prefix "$prefix"} \
-			  ${sha1:+--oid "$sha1"} \
-			  ${remote:+--remote} \
-			  "--" \
-			  "$sm_path")
-
-		# exit codes for run-update-procedure:
-		# 0: update was successful, say command output
-		# 1: update procedure failed, but should not die
-		# 2 or 128: subcommand died during execution
-		# 3: no update procedure was run
-		res="$?"
-		case $res in
-		0)
-			say "$out"
-			;;
-		1)
-			err="${err};fatal: $out"
-			continue
-			;;
-		2|128)
-			die_with_status $res "fatal: $out"
-			;;
-		esac
-
-		if test -n "$recursive"
-		then
-			(
-				prefix=$(git submodule--helper relative-path "$prefix$sm_path/" "$wt_prefix")
-				wt_prefix=
-				sanitize_submodule_env
-				cd "$sm_path" &&
-				eval cmd_update
-			)
-			res=$?
-			if test $res -gt 0
-			then
-				die_msg="fatal: $(eval_gettext "Failed to recurse into submodule path '\$displaypath'")"
-				if test $res -ne 2
-				then
-					err="${err};$die_msg"
-					continue
-				else
-					die_with_status $res "$die_msg"
-				fi
-			fi
-		fi
-	done
-
-	if test -n "$err"
-	then
-		OIFS=$IFS
-		IFS=';'
-		for e in $err
-		do
-			if test -n "$e"
-			then
-				echo >&2 "$e"
-			fi
-		done
-		IFS=$OIFS
-		exit 1
-	fi
-	}
+		"$@"
 }
 
 #
-- 
2.33.GIT

[PATCH v7 18/20] fixup! submodule--helper run-update-procedure: remove --suboid

From: Glen Choo <hidden>
Date: 2022-02-10 09:29:56

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b895e88a64..98d8910930 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2886,14 +2886,11 @@ static int update_submodule(struct update_data *update_data)
 					    update_data->sm_path, update_data->update_default,
 					    &update_data->update_strategy);
 
-	/* NEEDSWORK: fix the style issues e.g. braces */
-	if (update_data->just_cloned) {
+	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);
-	}
+	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 (update_data->remote) {
 		char *remote_name = get_default_remote_submodule(update_data->sm_path);
-- 
2.33.GIT

[PATCH v7 19/20] fixup! submodule--helper run-update-procedure: learn --remote

From: Glen Choo <hidden>
Date: 2022-02-10 09:30:06

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 19 -------------------
 1 file changed, 19 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 98d8910930..5d5302b50b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2516,23 +2516,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;
@@ -3484,8 +3467,6 @@ static struct cmd_struct commands[] = {
 	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
 	{"deinit", module_deinit, 0},
 	{"summary", module_summary, SUPPORT_SUPER_PREFIX},
-	/* NEEDSWORK: remote-branch is also obsolete */
-	{"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},
-- 
2.33.GIT

[PATCH v7 20/20] fixup! submodule: move core cmd_update() logic to C

From: Glen Choo <hidden>
Date: 2022-02-10 09:30:09

Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 13 +++----------
 git-submodule.sh            | 13 +------------
 2 files changed, 4 insertions(+), 22 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5d5302b50b..3367997973 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2807,12 +2807,6 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 {
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
 	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
-	/*
-	 * NEEDSWORK: the equivalent code in git-submodule.sh does not
-	 * pass --prefix, so this shouldn't either
-	*/
-	if (update_data->prefix)
-		strvec_pushl(args, "--prefix", update_data->prefix, NULL);
 	if (update_data->recursive_prefix)
 		strvec_pushl(args, "--recursive-prefix",
 			     update_data->recursive_prefix, NULL);
@@ -2980,8 +2974,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	struct pathspec pathspec;
 	struct update_data opt = UPDATE_DATA_INIT;
 
-	/* NEEDSWORK: update names and strings */
-	struct option module_update_clone_options[] = {
+	struct option module_update_options[] = {
 		OPT__FORCE(&opt.force, N_("force checkout updates"), 0),
 		OPT_BOOL(0, "init", &opt.init,
 			 N_("initialize uninitialized submodules before update")),
@@ -3023,14 +3016,14 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"),
+		N_("git submodule--helper update [--prefix=<path>] [<path>...]"),
 		NULL
 	};
 
 	update_clone_config_from_gitmodules(&opt.max_jobs);
 	git_config(git_update_clone_config, &opt.max_jobs);
 
-	argc = parse_options(argc, argv, prefix, module_update_clone_options,
+	argc = parse_options(argc, argv, prefix, module_update_options,
 			     git_submodule_helper_usage, 0);
 	oidcpy(&opt.oid, null_oid());
 	oidcpy(&opt.suboid, null_oid());
diff --git a/git-submodule.sh b/git-submodule.sh
index d176469fb1..89f5cf393e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -50,15 +50,6 @@ single_branch=
 jobs=
 recommend_shallow=
 
-# NEEDSWORK this is now unused
-die_if_unmatched ()
-{
-	if test "$1" = "#unmatched"
-	then
-		exit ${2:-1}
-	fi
-}
-
 isnumber()
 {
 	n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
@@ -348,9 +339,7 @@ cmd_update()
 		shift
 	done
 
-	# NEEDSWORK --super-prefix isn't actually supported by this
-	# command - we just pass the $prefix to --recursive-prefix.
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper update \
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update \
 		${GIT_QUIET:+--quiet} \
 		${force:+--force} \
 		${progress:+--progress} \
-- 
2.33.GIT

Re: [PATCH v7 17/20] submodule: move core cmd_update() logic to C

From: Glen Choo <hidden>
Date: 2022-02-10 09:34:58

Here is the diff between this tree (+) and that of v5's [1] (-).

[1] https://lore.kernel.org/git/patch-v5-9.9-e8e57606ee9-20220128T125206Z-avarab@gmail.com

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1c28b6f479..b895e88a64 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2003,6 +2003,7 @@ struct update_data {
 	const char *recursive_prefix;
 	const char *sm_path;
 	const char *displaypath;
+	const char *update_default;
 	struct object_id oid;
 	struct object_id suboid;
 	struct submodule_update_strategy update_strategy;
@@ -2355,15 +2356,8 @@ static int run_update_command(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;
-	struct submodule_update_strategy strategy = SUBMODULE_UPDATE_STRATEGY_INIT;
 
-	if (ud->update_strategy.type == SM_UPDATE_UNSPECIFIED || ud->just_cloned)
-		determine_submodule_update_strategy(the_repository, ud->just_cloned,
-						    ud->sm_path, NULL, &strategy);
-	else
-		strategy = ud->update_strategy;
-
-	switch (strategy.type) {
+	switch (ud->update_strategy.type) {
 	case SM_UPDATE_CHECKOUT:
 		cp.git_cmd = 1;
 		strvec_pushl(&cp.args, "checkout", "-q", NULL);
@@ -2386,45 +2380,41 @@ static int run_update_command(struct update_data *ud, int subforce)
 		break;
 	case SM_UPDATE_COMMAND:
 		cp.use_shell = 1;
-		strvec_push(&cp.args, strategy.command);
+		strvec_push(&cp.args, ud->update_strategy.command);
 		must_die_on_failure = 1;
 		break;
 	default:
 		BUG("unexpected update strategy type: %s",
-		    submodule_strategy_to_string(&strategy));
+		    submodule_strategy_to_string(&ud->update_strategy));
 	}
 	strvec_push(&cp.args, oid);
 
 	cp.dir = xstrdup(ud->sm_path);
 	prepare_submodule_repo_env(&cp.env_array);
 	if (run_command(&cp)) {
-		switch (strategy.type) {
+		switch (ud->update_strategy.type) {
 		case SM_UPDATE_CHECKOUT:
 			die_message(_("Unable to checkout '%s' in submodule path '%s'"),
 				    oid, ud->displaypath);
 			break;
 		case SM_UPDATE_REBASE:
-			if (!must_die_on_failure)
-				break;
-			die(_("Unable to rebase '%s' in submodule path '%s'"),
+			die_message(_("Unable to rebase '%s' in submodule path '%s'"),
 			    oid, ud->displaypath);
 			break;
 		case SM_UPDATE_MERGE:
-			if (!must_die_on_failure)
-				break;
-			die(_("Unable to merge '%s' in submodule path '%s'"),
+			die_message(_("Unable to merge '%s' in submodule path '%s'"),
 			    oid, ud->displaypath);
 			break;
 		case SM_UPDATE_COMMAND:
-			if (!must_die_on_failure)
-				break;
-			die(_("Execution of '%s %s' failed in submodule path '%s'"),
-			    strategy.command, oid, ud->displaypath);
+			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: %s",
-			    submodule_strategy_to_string(&strategy));
+			    submodule_strategy_to_string(&ud->update_strategy));
 		}
+		if (must_die_on_failure)
+			exit(128);
 
 		/* the command failed, but update must continue */
 		return 1;
@@ -2433,7 +2423,7 @@ static int run_update_command(struct update_data *ud, int subforce)
 	if (ud->quiet)
 		return 0;
 
-	switch (strategy.type) {
+	switch (ud->update_strategy.type) {
 	case SM_UPDATE_CHECKOUT:
 		printf(_("Submodule path '%s': checked out '%s'\n"),
 		       ud->displaypath, oid);
@@ -2448,11 +2438,11 @@ static int run_update_command(struct update_data *ud, int subforce)
 		break;
 	case SM_UPDATE_COMMAND:
 		printf(_("Submodule path '%s': '%s %s'\n"),
-		       ud->displaypath, strategy.command, oid);
+		       ud->displaypath, ud->update_strategy.command, oid);
 		break;
 	default:
 		BUG("unexpected update strategy type: %s",
-		    submodule_strategy_to_string(&strategy));
+		    submodule_strategy_to_string(&ud->update_strategy));
 	}
 
 	return 0;
@@ -2832,10 +2822,12 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 
 static void update_data_to_args(struct update_data *update_data, struct strvec *args)
 {
-	const char *update = submodule_strategy_to_string(&update_data->update_strategy);
-
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
 	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
+	/*
+	 * NEEDSWORK: the equivalent code in git-submodule.sh does not
+	 * pass --prefix, so this shouldn't either
+	*/
 	if (update_data->prefix)
 		strvec_pushl(args, "--prefix", update_data->prefix, NULL);
 	if (update_data->recursive_prefix)
@@ -2859,8 +2851,8 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 		strvec_push(args, "--require-init");
 	if (update_data->depth)
 		strvec_pushf(args, "--depth=%d", update_data->depth);
-	if (update)
-		strvec_pushl(args, "--update", update, NULL);
+	if (update_data->update_default)
+		strvec_pushl(args, "--update", update_data->update_default, NULL);
 	if (update_data->references.nr) {
 		struct string_list_item *item;
 		for_each_string_list_item(item, &update_data->references)
@@ -2890,6 +2882,11 @@ static int update_submodule(struct update_data *update_data)
 							     update_data->prefix);
 	free(prefixed_path);
 
+	determine_submodule_update_strategy(the_repository, update_data->just_cloned,
+					    update_data->sm_path, update_data->update_default,
+					    &update_data->update_strategy);
+
+	/* NEEDSWORK: fix the style issues e.g. braces */
 	if (update_data->just_cloned) {
 		oidcpy(&update_data->suboid, null_oid());
 	} else {
@@ -3000,10 +2997,10 @@ static int update_submodules(struct update_data *update_data)
 
 static int module_update(int argc, const char **argv, const char *prefix)
 {
-	const char *update = NULL;
 	struct pathspec pathspec;
 	struct update_data opt = UPDATE_DATA_INIT;
 
+	/* NEEDSWORK: update names and strings */
 	struct option module_update_clone_options[] = {
 		OPT__FORCE(&opt.force, N_("force checkout updates"), 0),
 		OPT_BOOL(0, "init", &opt.init,
@@ -3021,7 +3018,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			   N_("path"),
 			   N_("path into the working tree, across nested "
 			      "submodule boundaries")),
-		OPT_STRING(0, "update", &update,
+		OPT_STRING(0, "update", &opt.update_default,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
 		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
@@ -3058,8 +3055,8 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	oidcpy(&opt.oid, null_oid());
 	oidcpy(&opt.suboid, null_oid());
 
-	if (update)
-		if (parse_submodule_update_strategy(update,
+	if (opt.update_default)
+		if (parse_submodule_update_strategy(opt.update_default,
 						    &opt.update_strategy) < 0)
 			die(_("bad value for update parameter"));
 
@@ -3490,6 +3487,7 @@ static struct cmd_struct commands[] = {
 	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
 	{"deinit", module_deinit, 0},
 	{"summary", module_summary, SUPPORT_SUPER_PREFIX},
+	/* NEEDSWORK: remote-branch is also obsolete */
 	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"push-check", push_check, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
diff --git a/git-submodule.sh b/git-submodule.sh
index bcd8b92aab..d176469fb1 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -50,6 +50,7 @@ single_branch=
 jobs=
 recommend_shallow=
 
+# NEEDSWORK this is now unused
 die_if_unmatched ()
 {
 	if test "$1" = "#unmatched"
@@ -347,6 +348,8 @@ cmd_update()
 		shift
 	done
 
+	# NEEDSWORK --super-prefix isn't actually supported by this
+	# command - we just pass the $prefix to --recursive-prefix.
 	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper update \
 		${GIT_QUIET:+--quiet} \
 		${force:+--force} \

Re: [PATCH v7 02/20] submodule--helper: refactor get_submodule_displaypath()

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-02-12 14:24:45

On Thu, Feb 10 2022, Glen Choo wrote:
quoted hunk
From: Atharva Raykar <redacted>

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 [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Signed-off-by: Atharva Raykar <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 builtin/submodule--helper.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4c7c1e1432..5efceb9d46 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -261,11 +261,8 @@ 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)
Nit: overly long line (also in my v5, but since we're applying some
final polishing touches...)

Re: [PATCH v7 03/20] submodule--helper: allow setting superprefix for init_submodule()

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-02-12 14:32:01

On Thu, Feb 10 2022, Glen Choo wrote:
quoted hunk
From: Atharva Raykar <redacted>

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 [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Signed-off-by: Atharva Raykar <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 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 5efceb9d46..09cda67c1e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -606,18 +606,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);
 
@@ -691,7 +695,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)
Note/nit on existing (pre this series) code, I wonder why we ended up
with this init_submodule() v.s. init_submodule_cb() indirection,
v.s. just doing:
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 09cda67c1ea..aa82abeb37a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -611,9 +611,14 @@ struct init_cb {
 };
 #define INIT_CB_INIT { 0 }
 
-static void init_submodule(const char *path, const char *prefix,
-			   const char *superprefix, unsigned int flags)
+static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)
 {
+	const char *path = list_item->name;
+	struct init_cb *info = cb_data;
+	const char *prefix = info->prefix;
+	const char *superprefix = info->superprefix ? info->superprefix :
+		get_super_prefix();
+	unsigned int flags = info->flags;
 	const struct submodule *sub;
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
@@ -692,12 +697,6 @@ static void init_submodule(const char *path, const char *prefix,
 	free(upd);
 }
 
-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->superprefix, info->flags);
-}
-
 static int module_init(int argc, const char **argv, const char *prefix)
 {
 	struct init_cb info = INIT_CB_INIT;
Maybe it's worth it to declare the variables in the argument list
v.s. the function.

Re: [PATCH v7 04/20] submodule--helper: run update using child process struct

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-02-12 14:34:13

On Thu, Feb 10 2022, Glen Choo wrote:
quoted hunk
From: Atharva Raykar <redacted>

We switch to using the run-command API function that takes a
'struct child process', since we are using a lot of the options. This
will also make it simple to switch over to using 'capture_command()'
when we start handling the output of the command completely in C.

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Signed-off-by: Atharva Raykar <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 builtin/submodule--helper.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 09cda67c1e..db71e6f4ec 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2344,47 +2344,45 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str
 
 static int run_update_command(struct update_data *ud, int subforce)
 {
-	struct strvec args = STRVEC_INIT;
-	struct strvec child_env = STRVEC_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
 	char *oid = oid_to_hex(&ud->oid);
 	int must_die_on_failure = 0;
-	int git_cmd;
 
 	switch (ud->update_strategy.type) {
 	case SM_UPDATE_CHECKOUT:
-		git_cmd = 1;
-		strvec_pushl(&args, "checkout", "-q", NULL);
+		cp.git_cmd = 1;
+		strvec_pushl(&cp.args, "checkout", "-q", NULL);
 		if (subforce)
-			strvec_push(&args, "-f");
+			strvec_push(&cp.args, "-f");
 		break;
 	case SM_UPDATE_REBASE:
-		git_cmd = 1;
-		strvec_push(&args, "rebase");
+		cp.git_cmd = 1;
+		strvec_push(&cp.args, "rebase");
 		if (ud->quiet)
-			strvec_push(&args, "--quiet");
+			strvec_push(&cp.args, "--quiet");
 		must_die_on_failure = 1;
 		break;
 	case SM_UPDATE_MERGE:
-		git_cmd = 1;
-		strvec_push(&args, "merge");
+		cp.git_cmd = 1;
+		strvec_push(&cp.args, "merge");
 		if (ud->quiet)
-			strvec_push(&args, "--quiet");
+			strvec_push(&cp.args, "--quiet");
 		must_die_on_failure = 1;
 		break;
 	case SM_UPDATE_COMMAND:
-		git_cmd = 0;
-		strvec_push(&args, ud->update_strategy.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: %s",
 		    submodule_strategy_to_string(&ud->update_strategy));
 	}
-	strvec_push(&args, oid);
+	strvec_push(&cp.args, oid);
 
-	prepare_submodule_repo_env(&child_env);
-	if (run_command_v_opt_cd_env(args.v, git_cmd ? RUN_GIT_CMD : RUN_USING_SHELL,
-				     ud->sm_path, child_env.v)) {
+	cp.dir = xstrdup(ud->sm_path);
+	prepare_submodule_repo_env(&cp.env_array);
+	if (run_command(&cp)) {
 		switch (ud->update_strategy.type) {
 		case SM_UPDATE_CHECKOUT:
 			printf(_("Unable to checkout '%s' in submodule path '%s'"),
If this series is re-arranged so that this comes first, this compiles
just fine & passes all tests.

So I wonder if we can peel this and perhaps other such easy to review
prep changes off into its own series, since this one has grown to 20
patches.

We could then hopefully fast-track those easy-to-review prep changes,
which would make the "real" series to follow smaller and easier to
review/grok.

Re: [PATCH v7 09/20] submodule--helper: remove update-module-mode

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-02-12 14:36:15

On Thu, Feb 10 2022, Glen Choo wrote:
quoted hunk
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 <redacted>
---
 builtin/submodule--helper.c | 24 ------------------------
 1 file changed, 24 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4a0890954e..e0cc1c1b79 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1965,29 +1965,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> <path> [<update>]");
-
-	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;
@@ -3388,7 +3365,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},
Nice catch!

Re my comment on 04/20 in [off-list ref]
at least 04..09/20 could be split into such a "trivial refactors for
later changes" series, and it would make sense to lead with this (and
any other deletions of already-dead code).

Re: [PATCH v7 12/20] submodule--helper run-update-procedure: learn --remote

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-02-12 14:39:58

On Thu, Feb 10 2022, Glen Choo wrote:
quoted hunk
 #define UPDATE_DATA_INIT { \
 	.update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT, \
@@ -2585,6 +2571,8 @@ static int run_update_procedure(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F(0, "oid", &opt.oid, N_("sha1"),
 			       N_("SHA1 expected by superproject"), PARSE_OPT_NONEG,
 			       parse_opt_object_id),
+		OPT_BOOL(0, "remote", &opt.remote,
+			 N_("use SHA-1 of submodule's remote tracking branch")),
For some things the references to SHA-1 are faithfully copying existing
test, but for new things let's use "object" instead.

Re: [PATCH v7 15/20] submodule--helper: move functions around

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-02-12 14:41:31

On Thu, Feb 10 2022, Glen Choo wrote:
A subsequent commit will change the internals of several functions and
arrange them in a more logical manner. Move these functions to their
final positions so that the diff is smaller.
Shouldn't we do this earlier & avoid the FIXME comment in 10/20? (maybe
a subsequent fixup addresses it, haven't checked...)

Re: [PATCH v7 18/20] fixup! submodule--helper run-update-procedure: remove --suboid

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-02-12 14:43:19

On Thu, Feb 10 2022, Glen Choo wrote:
quoted hunk
Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b895e88a64..98d8910930 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2886,14 +2886,11 @@ static int update_submodule(struct update_data *update_data)
 					    update_data->sm_path, update_data->update_default,
 					    &update_data->update_strategy);
 
-	/* NEEDSWORK: fix the style issues e.g. braces */
-	if (update_data->just_cloned) {
+	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);
-	}
+	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 (update_data->remote) {
 		char *remote_name = get_default_remote_submodule(update_data->sm_path);
This fixup looks good, let's apply this fix-up to the relevant preceding
commit.

Re: [PATCH v7 19/20] fixup! submodule--helper run-update-procedure: learn --remote

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-02-12 14:43:43

On Thu, Feb 10 2022, Glen Choo wrote:
quoted hunk
Signed-off-by: Glen Choo <redacted>
---
 builtin/submodule--helper.c | 19 -------------------
 1 file changed, 19 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 98d8910930..5d5302b50b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2516,23 +2516,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;
@@ -3484,8 +3467,6 @@ static struct cmd_struct commands[] = {
 	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
 	{"deinit", module_deinit, 0},
 	{"summary", module_summary, SUPPORT_SUPER_PREFIX},
-	/* NEEDSWORK: remote-branch is also obsolete */
-	{"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},
Ditto good fix-up, should squash this into the relevant commit.

Re: [PATCH v7 00/20] submodule: convert the rest of 'update' to C

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-02-12 14:49:55

On Thu, Feb 10 2022, Glen Choo wrote:
This reroll contains another 'easy' preparatory patch and the fixups I
alluded to in v6 [1]. This isn't the split-up I described in the
footnote of v6, but it gets the big patch (patch 17) to what I think is
a reviewable state.

The diff between v7 and v5 is no longer just NEEDSWORK comments, but I
think it is easier to reason about. Patch 17 resembles v5 the most (I
will include a diff in a reply to that patch); everything after patch 17
is fixups (I did not squash them in because they would grow the diff
even more).

I will also leave a review on patch 17 since the changes were not
originally authored by me.

[1] https://lore.kernel.org/git/20220208083952.35036-1-chooglen@google.com

Changes in v7:
- Split the last patch of v6 (the big one) into patches 16-17.
- Patch 16 moves logic out of run_update_procedure() (because the
  command is going away), removing some noise from patch 17. This makes
  the update_strategy parsing easier to reason about, but at the cost of
  growing the diff vis-a-vis v5
- Patches 18-20 are fixups that address NEEDSWORK comments from earlier
  patches. Once maintaining a small diff vis-a-vis v5 stops making
  sense, I will squash them in.

Atharva Raykar (6):
  submodule--helper: get remote names from any repository
  submodule--helper: refactor get_submodule_displaypath()
  submodule--helper: allow setting superprefix for init_submodule()
  submodule--helper: run update using child process struct
  builtin/submodule--helper.c: reformat designated initializers
  submodule: move core cmd_update() logic to C

Glen Choo (11):
  submodule--helper: remove update-module-mode
  submodule--helper: reorganize code for sh to C conversion
  submodule--helper run-update-procedure: remove --suboid
  submodule--helper run-update-procedure: learn --remote
  submodule--helper: remove ensure-core-worktree
  submodule--helper update-clone: learn --init
  submodule--helper: move functions around
  submodule--helper: reduce logic in run_update_procedure()
  fixup! submodule--helper run-update-procedure: remove --suboid
  fixup! submodule--helper run-update-procedure: learn --remote
  fixup! submodule: move core cmd_update() logic to C

Ævar Arnfjörð Bjarmason (3):
  builtin/submodule--helper.c: rename option variables to "opt"
  submodule--helper: don't use bitfield indirection for parse_options()
  submodule tests: test for init and update failure output
Thanks a lot for picking this up! This split-up is much easier to read
than my v5, particularly with the end diff-stat of the "main" patch
being:

 2 files changed, 201 insertions(+), 216 deletions(-)

Instead of:

 2 files changed, 356 insertions(+), 388 deletions(-)

I think sending a version of this with the fixups squashed in as a v8
would be good, and perhaps addressing some of my comments.

I don't know if my suggested split-up of "prep fixes" into another
series would be a good thing to pursue overall, perhaps Junio will chime
in on how he'd be most comfortable in merging this down. I'd think
splitting such trivial fixes into their own series be easier to review,
but perhaps not.

For the Signed-off-by question on v6, I think you should add your SOB to
all the patches you submit. See this in SubmittingPatches:
    
    Notice that you can place your own `Signed-off-by` trailer when
    forwarding somebody else's patch with the above rules for
    D-C-O.  Indeed you are encouraged to do so.

Just running "git rebase -i -x 'git commit --amend --no-edit -s'" should
do it.
    

Re: [PATCH v7 00/20] submodule: convert the rest of 'update' to C

From: Glen Choo <hidden>
Date: 2022-02-17 05:44:13

Ævar Arnfjörð Bjarmason [off-list ref] writes:
On Thu, Feb 10 2022, Glen Choo wrote:
quoted
Atharva Raykar (6):
  submodule--helper: get remote names from any repository
  submodule--helper: refactor get_submodule_displaypath()
  submodule--helper: allow setting superprefix for init_submodule()
  submodule--helper: run update using child process struct
  builtin/submodule--helper.c: reformat designated initializers
  submodule: move core cmd_update() logic to C

Glen Choo (11):
  submodule--helper: remove update-module-mode
  submodule--helper: reorganize code for sh to C conversion
  submodule--helper run-update-procedure: remove --suboid
  submodule--helper run-update-procedure: learn --remote
  submodule--helper: remove ensure-core-worktree
  submodule--helper update-clone: learn --init
  submodule--helper: move functions around
  submodule--helper: reduce logic in run_update_procedure()
  fixup! submodule--helper run-update-procedure: remove --suboid
  fixup! submodule--helper run-update-procedure: learn --remote
  fixup! submodule: move core cmd_update() logic to C

Ævar Arnfjörð Bjarmason (3):
  builtin/submodule--helper.c: rename option variables to "opt"
  submodule--helper: don't use bitfield indirection for parse_options()
  submodule tests: test for init and update failure output
I think sending a version of this with the fixups squashed in as a v8
would be good, and perhaps addressing some of my comments.

I don't know if my suggested split-up of "prep fixes" into another
series would be a good thing to pursue overall, perhaps Junio will chime
in on how he'd be most comfortable in merging this down. I'd think
splitting such trivial fixes into their own series be easier to review,
but perhaps not.
Combing through the patches again, I couldn't really convince myself
that the patch 4..9 prep fixes make sense as obvious standalone fixes,
except maybe:

- patch 4 submodule--helper: run update using child process struct
- patch 8 submodule tests: test for init and update failure output
- patch 9:  087bf43aba submodule--helper: remove update-module-mode

But, since the 'final' patch (ignoring the fixup!-s) is consuming a huge
chunk of the work anyway, here's an alternative patch organization with
the fixup!-s squashed:

= Move 'easy' and 'obviously correct' code from sh->C
- patches 8-9   Cleanup and introduce tests
- patches 1-4   Refactor existing functions, which enables..
- patches 10-14 Move 'obviously correct' pieces of logic from sh-> C

= Finalize move from sh->C
i.e. combine "run-update-procedure" and "update-clone" into "update"
- patches 5,7     Cleanup and prep
- patches 6,15-16 Shrinking the diff
- patch 17        Implement "git submodule--helper update" 

I'll send this if there are no objections :)

Re: [PATCH v7 00/20] submodule: convert the rest of 'update' to C

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-02-17 09:18:46

On Thu, Feb 17 2022, Glen Choo wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
On Thu, Feb 10 2022, Glen Choo wrote:
quoted
Atharva Raykar (6):
  submodule--helper: get remote names from any repository
  submodule--helper: refactor get_submodule_displaypath()
  submodule--helper: allow setting superprefix for init_submodule()
  submodule--helper: run update using child process struct
  builtin/submodule--helper.c: reformat designated initializers
  submodule: move core cmd_update() logic to C

Glen Choo (11):
  submodule--helper: remove update-module-mode
  submodule--helper: reorganize code for sh to C conversion
  submodule--helper run-update-procedure: remove --suboid
  submodule--helper run-update-procedure: learn --remote
  submodule--helper: remove ensure-core-worktree
  submodule--helper update-clone: learn --init
  submodule--helper: move functions around
  submodule--helper: reduce logic in run_update_procedure()
  fixup! submodule--helper run-update-procedure: remove --suboid
  fixup! submodule--helper run-update-procedure: learn --remote
  fixup! submodule: move core cmd_update() logic to C

Ævar Arnfjörð Bjarmason (3):
  builtin/submodule--helper.c: rename option variables to "opt"
  submodule--helper: don't use bitfield indirection for parse_options()
  submodule tests: test for init and update failure output
I think sending a version of this with the fixups squashed in as a v8
would be good, and perhaps addressing some of my comments.

I don't know if my suggested split-up of "prep fixes" into another
series would be a good thing to pursue overall, perhaps Junio will chime
in on how he'd be most comfortable in merging this down. I'd think
splitting such trivial fixes into their own series be easier to review,
but perhaps not.
Combing through the patches again, I couldn't really convince myself
that the patch 4..9 prep fixes make sense as obvious standalone fixes,
except maybe:

- patch 4 submodule--helper: run update using child process struct
- patch 8 submodule tests: test for init and update failure output
- patch 9:  087bf43aba submodule--helper: remove update-module-mode

But, since the 'final' patch (ignoring the fixup!-s) is consuming a huge
chunk of the work anyway, here's an alternative patch organization with
the fixup!-s squashed:

= Move 'easy' and 'obviously correct' code from sh->C
- patches 8-9   Cleanup and introduce tests
- patches 1-4   Refactor existing functions, which enables..
- patches 10-14 Move 'obviously correct' pieces of logic from sh-> C

= Finalize move from sh->C
i.e. combine "run-update-procedure" and "update-clone" into "update"
- patches 5,7     Cleanup and prep
- patches 6,15-16 Shrinking the diff
- patch 17        Implement "git submodule--helper update" 

I'll send this if there are no objections :)
Yes that sounds good, or rather, I haven't re-looked at that in detail,
but I think if you think it makes sense we should go for it.

Or rather, we should really be aiming to produce a patch series that
makes sense in its current iteration, as opposed to optimizing for a
diff against some ad-hoc re-roll I produced a few versions ago :)

Thanks again for working on this & picking this up. It's great to see
progress in this area!

Re: [PATCH v7 00/20] submodule: convert the rest of 'update' to C

From: Glen Choo <hidden>
Date: 2022-02-17 16:14:26

Ævar Arnfjörð Bjarmason [off-list ref] writes:
On Thu, Feb 17 2022, Glen Choo wrote:
quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
On Thu, Feb 10 2022, Glen Choo wrote:
quoted
Atharva Raykar (6):
  submodule--helper: get remote names from any repository
  submodule--helper: refactor get_submodule_displaypath()
  submodule--helper: allow setting superprefix for init_submodule()
  submodule--helper: run update using child process struct
  builtin/submodule--helper.c: reformat designated initializers
  submodule: move core cmd_update() logic to C

Glen Choo (11):
  submodule--helper: remove update-module-mode
  submodule--helper: reorganize code for sh to C conversion
  submodule--helper run-update-procedure: remove --suboid
  submodule--helper run-update-procedure: learn --remote
  submodule--helper: remove ensure-core-worktree
  submodule--helper update-clone: learn --init
  submodule--helper: move functions around
  submodule--helper: reduce logic in run_update_procedure()
  fixup! submodule--helper run-update-procedure: remove --suboid
  fixup! submodule--helper run-update-procedure: learn --remote
  fixup! submodule: move core cmd_update() logic to C

Ævar Arnfjörð Bjarmason (3):
  builtin/submodule--helper.c: rename option variables to "opt"
  submodule--helper: don't use bitfield indirection for parse_options()
  submodule tests: test for init and update failure output
I think sending a version of this with the fixups squashed in as a v8
would be good, and perhaps addressing some of my comments.

I don't know if my suggested split-up of "prep fixes" into another
series would be a good thing to pursue overall, perhaps Junio will chime
in on how he'd be most comfortable in merging this down. I'd think
splitting such trivial fixes into their own series be easier to review,
but perhaps not.
Combing through the patches again, I couldn't really convince myself
that the patch 4..9 prep fixes make sense as obvious standalone fixes,
except maybe:

- patch 4 submodule--helper: run update using child process struct
- patch 8 submodule tests: test for init and update failure output
- patch 9:  087bf43aba submodule--helper: remove update-module-mode

But, since the 'final' patch (ignoring the fixup!-s) is consuming a huge
chunk of the work anyway, here's an alternative patch organization with
the fixup!-s squashed:

= Move 'easy' and 'obviously correct' code from sh->C
- patches 8-9   Cleanup and introduce tests
- patches 1-4   Refactor existing functions, which enables..
- patches 10-14 Move 'obviously correct' pieces of logic from sh-> C

= Finalize move from sh->C
i.e. combine "run-update-procedure" and "update-clone" into "update"
- patches 5,7     Cleanup and prep
- patches 6,15-16 Shrinking the diff
- patch 17        Implement "git submodule--helper update" 

I'll send this if there are no objections :)
Yes that sounds good, or rather, I haven't re-looked at that in detail,
but I think if you think it makes sense we should go for it.

Or rather, we should really be aiming to produce a patch series that
makes sense in its current iteration, as opposed to optimizing for a
diff against some ad-hoc re-roll I produced a few versions ago :)
Agreed, makes sense.
Thanks again for working on this & picking this up. It's great to see
progress in this area!
Thanks to you too for getting the ball rolling and lending me your
thoughts :)

Previous page

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help