[PATCH 0/8] submodule: tests, cleanup to prepare for built-in

[PATCH 0/8] submodule: tests, cleanup to prepare for built-in

[Ævar Arnfjörð Bjarmason]

I have a topic on-list to remove git-submodule.sh and create a
builtin/submodule.c, i.e. promoting "git submodule--helper" to the
"real thing"[1].

Glen gave me a bunch of good feedback on it, including (but not
limited to) pointing out that we have outstanding dead code in
[2][3].

Once I started pulling at that thread things became a lot simpler for
the re-roll of [1], e.g. the migration of git-submodule.sh's commands
can squash in the "update" step, as it's no longer a special-case.

But that also made the series larger, and it's conflicted with other
outstanding patches. First René's strvec() cleanup in submodule.c, and
currently with Glen's in-flight submodule topic.

So here's "just the prep" part of that split-out. See also [4] and [5]
for previous "prep" topics, we're getting closer...

This only adds missing test coverage, and deletes dead code that we'd
otherwise have to account for. Then 8/8 converts submodule--helper to
use the OPT_SUBCOMMAND() API in 8/8.

CI & branch at: https://github.com/avar/git/tree/avar/submodule-builtin-final-prep

For a peek at the WIP re-roll of [1] that'll come after this:
https://github.com/avar/git/compare/avar/submodule-builtin-final-prep...avar/submodule-sh-dispatch-to-helper-directly-3

1. https://lore.kernel.org/git/cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com/
2. https://lore.kernel.org/git/kl6lpmemxg8p.fsf@chooglen-macbookpro.roam.corp.google.com/
3. https://lore.kernel.org/git/kl6lv8oexiyy.fsf@chooglen-macbookpro.roam.corp.google.com/
4. 361cbe6d6d2 (Merge branch 'ab/submodule-cleanup', 2022-07-14)
5. f322e9f51b5 (Merge branch 'ab/submodule-helper-prep', 2022-09-13)

Ævar Arnfjörð Bjarmason (8):
  submodule--helper: move "config" to a test-tool
  submodule tests: add tests for top-level flag output
  submodule tests: test for a "foreach" blind-spot
  submodule.c: refactor recursive block out of absorb function
  submodule API & "absorbgitdirs": remove "----recursive" option
  submodule--helper: remove --prefix from "absorbgitdirs"
  submodule--helper: drop "update --prefix <pfx>" for "-C <pfx> update"
  submodule--helper: use OPT_SUBCOMMAND() API

 builtin/rm.c                           |   3 +-
 builtin/submodule--helper.c            | 139 ++++++--------------
 git-submodule.sh                       |   3 +-
 git.c                                  |   2 +-
 submodule.c                            |  41 +++---
 submodule.h                            |   4 +-
 t/helper/test-submodule.c              |  84 ++++++++++++
 t/t7400-submodule-basic.sh             |  10 ++
 t/t7407-submodule-foreach.sh           |   5 +
 t/t7411-submodule-config.sh            |  28 ++--
 t/t7418-submodule-sparse-gitmodules.sh |   4 +-
 t/t7422-submodule-output.sh            | 169 +++++++++++++++++++++++++
 12 files changed, 349 insertions(+), 143 deletions(-)
 create mode 100755 t/t7422-submodule-output.sh

-- 
2.38.0.1280.g8136eb6fab2

[PATCH 1/8] submodule--helper: move "config" to a test-tool

[Ævar Arnfjörð Bjarmason]

As with other moves to "test-tool" in f322e9f51b5 (Merge branch
'ab/submodule-helper-prep', 2022-09-13) the "config" sub-command was
only used by our own tests.

Let's move it over, and while doing so make it easier to reason about
by splitting up the various uses for it into separate sub-commands, so
that we don't need to count arguments to see what it does.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/submodule--helper.c            | 46 --------------
 t/helper/test-submodule.c              | 84 ++++++++++++++++++++++++++
 t/t7411-submodule-config.sh            | 28 ++++-----
 t/t7418-submodule-sparse-gitmodules.sh |  4 +-
 4 files changed, 100 insertions(+), 62 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a7683d35299..6250b95a6f7 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2861,51 +2861,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
-static int module_config(int argc, const char **argv, const char *prefix)
-{
-	enum {
-		CHECK_WRITEABLE = 1,
-		DO_UNSET = 2
-	} command = 0;
-	struct option module_config_options[] = {
-		OPT_CMDMODE(0, "check-writeable", &command,
-			    N_("check if it is safe to write to the .gitmodules file"),
-			    CHECK_WRITEABLE),
-		OPT_CMDMODE(0, "unset", &command,
-			    N_("unset the config in the .gitmodules file"),
-			    DO_UNSET),
-		OPT_END()
-	};
-	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper config <name> [<value>]"),
-		N_("git submodule--helper config --unset <name>"),
-		"git submodule--helper config --check-writeable",
-		NULL
-	};
-
-	argc = parse_options(argc, argv, prefix, module_config_options,
-			     git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
-
-	if (argc == 1 && command == CHECK_WRITEABLE)
-		return is_writing_gitmodules_ok() ? 0 : -1;
-
-	/* Equivalent to ACTION_GET in builtin/config.c */
-	if (argc == 2 && command != DO_UNSET)
-		return print_config_from_gitmodules(the_repository, argv[1]);
-
-	/* Equivalent to ACTION_SET in builtin/config.c */
-	if (argc == 3 || (argc == 2 && command == DO_UNSET)) {
-		const char *value = (argc == 3) ? argv[2] : NULL;
-
-		if (!is_writing_gitmodules_ok())
-			die(_("please make sure that the .gitmodules file is in the working tree"));
-
-		return config_set_in_gitmodules_file_gently(argv[1], value);
-	}
-
-	usage_with_options(git_submodule_helper_usage, module_config_options);
-}
-
 static int module_set_url(int argc, const char **argv, const char *prefix)
 {
 	int quiet = 0;
@@ -3424,7 +3379,6 @@ static struct cmd_struct commands[] = {
 	{"summary", module_summary, 0},
 	{"push-check", push_check, 0},
 	{"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
-	{"config", module_config, 0},
 	{"set-url", module_set_url, 0},
 	{"set-branch", module_set_branch, 0},
 	{"create-branch", module_create_branch, 0},
diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c
index b7d117cd557..c7cb463a583 100644
--- a/t/helper/test-submodule.c
+++ b/t/helper/test-submodule.c
@@ -111,10 +111,94 @@ static int cmd__submodule_resolve_relative_url(int argc, const char **argv)
 	return 0;
 }
 
+static int cmd__submodule_config_list(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+	const char *const usage[] = {
+		"test-tool submodule config-list <key>",
+		NULL
+	};
+	argc = parse_options(argc, argv, "test-tools", options, usage,
+			     PARSE_OPT_KEEP_ARGV0);
+
+	setup_git_directory();
+
+	if (argc == 2)
+		return print_config_from_gitmodules(the_repository, argv[1]);
+	usage_with_options(usage, options);
+}
+
+static int cmd__submodule_config_set(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+	const char *const usage[] = {
+		"test-tool submodule config-set <key> <value>",
+		NULL
+	};
+	argc = parse_options(argc, argv, "test-tools", options, usage,
+			     PARSE_OPT_KEEP_ARGV0);
+
+	setup_git_directory();
+
+	/* Equivalent to ACTION_SET in builtin/config.c */
+	if (argc == 3) {
+		if (!is_writing_gitmodules_ok())
+			die(_("please make sure that the .gitmodules file is in the working tree"));
+
+		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+	}
+	usage_with_options(usage, options);
+}
+
+static int cmd__submodule_config_unset(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+	const char *const usage[] = {
+		"test-tool submodule config-unset <key>",
+		NULL
+	};
+
+	setup_git_directory();
+
+	if (argc == 2) {
+		if (!is_writing_gitmodules_ok())
+			die(_("please make sure that the .gitmodules file is in the working tree"));
+		return config_set_in_gitmodules_file_gently(argv[1], NULL);
+	}
+	usage_with_options(usage, options);
+}
+
+static int cmd__submodule_config_writeable(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+	const char *const usage[] = {
+		"test-tool submodule config-writeable",
+		NULL
+	};
+	setup_git_directory();
+
+	if (argc == 1)
+		return is_writing_gitmodules_ok() ? 0 : -1;
+
+	usage_with_options(usage, options);
+}
+
 static struct test_cmd cmds[] = {
 	{ "check-name", cmd__submodule_check_name },
 	{ "is-active", cmd__submodule_is_active },
 	{ "resolve-relative-url", cmd__submodule_resolve_relative_url},
+	{ "config-list", cmd__submodule_config_list },
+	{ "config-set", cmd__submodule_config_set },
+	{ "config-unset", cmd__submodule_config_unset },
+	{ "config-writeable", cmd__submodule_config_writeable },
 };
 
 int cmd__submodule(int argc, const char **argv)
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index c583c4e373a..2f57ecd177a 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -140,15 +140,15 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 test_expect_success 'reading submodules config from the working tree with "submodule--helper config"' '
 	(cd super &&
 		echo "../submodule" >expect &&
-		git submodule--helper config submodule.submodule.url >actual &&
+		test-tool submodule config-list submodule.submodule.url >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'unsetting submodules config from the working tree with "submodule--helper config --unset"' '
 	(cd super &&
-		git submodule--helper config --unset submodule.submodule.url &&
-		git submodule--helper config submodule.submodule.url >actual &&
+		test-tool submodule config-unset submodule.submodule.url &&
+		test-tool submodule config-list submodule.submodule.url >actual &&
 		test_must_be_empty actual
 	)
 '
@@ -157,8 +157,8 @@ test_expect_success 'unsetting submodules config from the working tree with "sub
 test_expect_success 'writing submodules config with "submodule--helper config"' '
 	(cd super &&
 		echo "new_url" >expect &&
-		git submodule--helper config submodule.submodule.url "new_url" &&
-		git submodule--helper config submodule.submodule.url >actual &&
+		test-tool submodule config-set submodule.submodule.url "new_url" &&
+		test-tool submodule config-list submodule.submodule.url >actual &&
 		test_cmp expect actual
 	)
 '
@@ -167,14 +167,14 @@ test_expect_success 'overwriting unstaged submodules config with "submodule--hel
 	test_when_finished "git -C super checkout .gitmodules" &&
 	(cd super &&
 		echo "newer_url" >expect &&
-		git submodule--helper config submodule.submodule.url "newer_url" &&
-		git submodule--helper config submodule.submodule.url >actual &&
+		test-tool submodule config-set submodule.submodule.url "newer_url" &&
+		test-tool submodule config-list submodule.submodule.url >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'writeable .gitmodules when it is in the working tree' '
-	git -C super submodule--helper config --check-writeable
+	test-tool -C super submodule config-writeable
 '
 
 test_expect_success 'writeable .gitmodules when it is nowhere in the repository' '
@@ -183,7 +183,7 @@ test_expect_success 'writeable .gitmodules when it is nowhere in the repository'
 	(cd super &&
 		git rm .gitmodules &&
 		git commit -m "remove .gitmodules from the current branch" &&
-		git submodule--helper config --check-writeable
+		test-tool submodule config-writeable
 	)
 '
 
@@ -191,7 +191,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the index but not i
 	test_when_finished "git -C super checkout .gitmodules" &&
 	(cd super &&
 		rm -f .gitmodules &&
-		test_must_fail git submodule--helper config --check-writeable
+		test_must_fail test-tool submodule config-writeable
 	)
 '
 
@@ -200,7 +200,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the current branch
 	test_when_finished "git -C super reset --hard $ORIG" &&
 	(cd super &&
 		git rm .gitmodules &&
-		test_must_fail git submodule--helper config --check-writeable
+		test_must_fail test-tool submodule config-writeable
 	)
 '
 
@@ -208,11 +208,11 @@ test_expect_success 'reading submodules config from the index when .gitmodules i
 	ORIG=$(git -C super rev-parse HEAD) &&
 	test_when_finished "git -C super reset --hard $ORIG" &&
 	(cd super &&
-		git submodule--helper config submodule.submodule.url "staged_url" &&
+		test-tool submodule config-set submodule.submodule.url "staged_url" &&
 		git add .gitmodules &&
 		rm -f .gitmodules &&
 		echo "staged_url" >expect &&
-		git submodule--helper config submodule.submodule.url >actual &&
+		test-tool submodule config-list submodule.submodule.url >actual &&
 		test_cmp expect actual
 	)
 '
@@ -223,7 +223,7 @@ test_expect_success 'reading submodules config from the current branch when .git
 	(cd super &&
 		git rm .gitmodules &&
 		echo "../submodule" >expect &&
-		git submodule--helper config submodule.submodule.url >actual &&
+		test-tool submodule config-list submodule.submodule.url >actual &&
 		test_cmp expect actual
 	)
 '
diff --git a/t/t7418-submodule-sparse-gitmodules.sh b/t/t7418-submodule-sparse-gitmodules.sh
index d5874200fdc..dde11ecce80 100755
--- a/t/t7418-submodule-sparse-gitmodules.sh
+++ b/t/t7418-submodule-sparse-gitmodules.sh
@@ -50,12 +50,12 @@ test_expect_success 'sparse checkout setup which hides .gitmodules' '
 
 test_expect_success 'reading gitmodules config file when it is not checked out' '
 	echo "../submodule" >expect &&
-	git -C super submodule--helper config submodule.submodule.url >actual &&
+	test-tool -C super submodule config-list submodule.submodule.url >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'not writing gitmodules config file when it is not checked out' '
-	test_must_fail git -C super submodule--helper config submodule.submodule.url newurl &&
+	test_must_fail test-tool -C super submodule config-set submodule.submodule.url newurl &&
 	test_path_is_missing super/.gitmodules
 '
 
-- 
2.38.0.1280.g8136eb6fab2

[PATCH 2/8] submodule tests: add tests for top-level flag output

[Ævar Arnfjörð Bjarmason]

Exhaustively test for how combining various "mixed-level" "git
submodule" option works. "Mixed-level" here means options that are
accepted by a mixture of the top-level "submodule" command, and
e.g. the "status" sub-command.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7400-submodule-basic.sh  |  10 +++
 t/t7422-submodule-output.sh | 169 ++++++++++++++++++++++++++++++++++++
 2 files changed, 179 insertions(+)
 create mode 100755 t/t7422-submodule-output.sh

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index a989aafaf57..eae6a46ef3d 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -579,6 +579,16 @@ test_expect_success 'status should be "modified" after submodule commit' '
 	grep "^+$rev2" list
 '
 
+test_expect_success '"submodule --cached" command forms should be identical' '
+	git submodule status --cached >expect &&
+
+	git submodule --cached >actual &&
+	test_cmp expect actual &&
+
+	git submodule --cached status >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'the --cached sha1 should be rev1' '
 	git submodule --cached status >list &&
 	grep "^+$rev1" list
diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh
new file mode 100755
index 00000000000..3ac56c23f72
--- /dev/null
+++ b/t/t7422-submodule-output.sh
@@ -0,0 +1,169 @@
+#!/bin/sh
+
+test_description='submodule --cached, --quiet etc. output'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-t3100.sh
+
+setup_sub () {
+	local d="$1" &&
+	shift &&
+	git $@ clone . "$d" &&
+	git $@ submodule add ./"$d"
+}
+
+normalize_status () {
+	sed -e 's/-g[0-9a-f]*/-gHASH/'
+}
+
+test_expect_success 'setup' '
+	test_commit A &&
+	test_commit B &&
+	setup_sub S  &&
+	setup_sub S.D &&
+	setup_sub S.C &&
+	setup_sub S.C.D &&
+	setup_sub X &&
+	git add S* &&
+	test_commit C &&
+
+	# recursive in X/
+	git -C X pull &&
+	GIT_ALLOW_PROTOCOL=file git -C X submodule update --init &&
+
+	# dirty p
+	for d in S.D X/S.D
+	do
+		echo dirty >S.D/A.t || return 1
+	done &&
+
+	# commit (for --cached)
+	for d in S.C* X/S.C*
+	do
+		git -C "$d" reset --hard A || return 1
+	done &&
+
+	# dirty
+	for d in S*.D X/S*.D
+	do
+		echo dirty >"$d/C2.t" || return 1
+	done &&
+
+	for ref in A B C
+	do
+		# Not different with SHA-1 and SHA-256, just (ab)usign
+		# test_oid_cache as a variable bag to avoid using
+		# $(git rev-parse ...).
+		oid=$(git rev-parse $ref) &&
+		test_oid_cache <<-EOF || return 1
+		$ref sha1:$oid
+		$ref sha256:$oid
+		EOF
+	done
+'
+
+for opts in "" "status"
+do
+	test_expect_success "git submodule $opts" '
+		sed -e "s/^>//" >expect <<-EOF &&
+		> $(test_oid B) S (B)
+		>+$(test_oid A) S.C (A)
+		>+$(test_oid A) S.C.D (A)
+		> $(test_oid B) S.D (B)
+		>+$(test_oid C) X (C)
+		EOF
+		git submodule $opts >actual.raw &&
+		normalize_status <actual.raw >actual &&
+		test_cmp expect actual
+	'
+done
+
+for opts in \
+	"status --recursive"
+do
+	test_expect_success "git submodule $opts" '
+		sed -e "s/^>//" >expect <<-EOF &&
+		> $(test_oid B) S (B)
+		>+$(test_oid A) S.C (A)
+		>+$(test_oid A) S.C.D (A)
+		> $(test_oid B) S.D (B)
+		>+$(test_oid C) X (C)
+		> $(test_oid B) X/S (B)
+		>+$(test_oid A) X/S.C (A)
+		>+$(test_oid A) X/S.C.D (A)
+		> $(test_oid B) X/S.D (B)
+		> $(test_oid B) X/X (B)
+		EOF
+		git submodule $opts >actual.raw &&
+		normalize_status <actual.raw >actual &&
+		test_cmp expect actual
+	'
+done
+
+for opts in \
+	"--quiet" \
+	"--quiet status" \
+	"status --quiet"
+do
+	test_expect_success "git submodule $opts" '
+		git submodule $opts >out &&
+		test_must_be_empty out
+	'
+done
+
+for opts in \
+	"--cached" \
+	"--cached status" \
+	"status --cached"
+do
+	test_expect_success "git submodule $opts" '
+		sed -e "s/^>//" >expect <<-EOF &&
+		> $(test_oid B) S (B)
+		>+$(test_oid B) S.C (B)
+		>+$(test_oid B) S.C.D (B)
+		> $(test_oid B) S.D (B)
+		>+$(test_oid B) X (B)
+		EOF
+		git submodule $opts >actual.raw &&
+		normalize_status <actual.raw >actual &&
+		test_cmp expect actual
+	'
+done
+
+for opts in \
+	"--cached --quiet" \
+	"--cached --quiet status" \
+	"--cached status --quiet" \
+	"--quiet status --cached" \
+	"status --cached --quiet"
+do
+	test_expect_success "git submodule $opts" '
+		git submodule $opts >out &&
+		test_must_be_empty out
+	'
+done
+
+for opts in \
+	"status --cached --recursive" \
+	"--cached status --recursive"
+do
+	test_expect_success "git submodule $opts" '
+		sed -e "s/^>//" >expect <<-EOF &&
+		> $(test_oid B) S (B)
+		>+$(test_oid B) S.C (B)
+		>+$(test_oid B) S.C.D (B)
+		> $(test_oid B) S.D (B)
+		>+$(test_oid B) X (B)
+		> $(test_oid B) X/S (B)
+		>+$(test_oid B) X/S.C (B)
+		>+$(test_oid B) X/S.C.D (B)
+		> $(test_oid B) X/S.D (B)
+		> $(test_oid B) X/X (B)
+		EOF
+		git submodule $opts >actual.raw &&
+		normalize_status <actual.raw >actual &&
+		test_cmp expect actual
+	'
+done
+
+test_done
-- 
2.38.0.1280.g8136eb6fab2

[PATCH 3/8] submodule tests: test for a "foreach" blind-spot

[Ævar Arnfjörð Bjarmason]

We tested for "--" followed by command names, but not for "--"
followed by an argument that looks like an option, let's do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7407-submodule-foreach.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 59bd1501667..8d7b234beb8 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -154,6 +154,11 @@ test_expect_success 'use "submodule foreach" to checkout 2nd level submodule' '
 	)
 '
 
+test_expect_success 'usage: foreach -- --not-an-option' '
+	test_expect_code 1 git submodule foreach -- --not-an-option &&
+	test_expect_code 1 git -C clone2 submodule foreach -- --not-an-option
+'
+
 test_expect_success 'use "foreach --recursive" to checkout all submodules' '
 	(
 		cd clone2 &&
-- 
2.38.0.1280.g8136eb6fab2

[PATCH 4/8] submodule.c: refactor recursive block out of absorb function

[Ævar Arnfjörð Bjarmason]

A move and indentation-only change to move the
ABSORB_GITDIR_RECURSE_SUBMODULES case into its own function, which as
we'll see makes the subsequent commit changing this code much smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 submodule.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/submodule.c b/submodule.c
index b958162d286..fe1e3f03905 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2310,6 +2310,23 @@ static void relocate_single_git_dir_into_superproject(const char *path)
 	strbuf_release(&new_gitdir);
 }
 
+static void absorb_git_dir_into_superproject_recurse(const char *path)
+{
+
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	cp.dir = path;
+	cp.git_cmd = 1;
+	cp.no_stdin = 1;
+	strvec_pushf(&cp.args, "--super-prefix=%s%s/",
+		     get_super_prefix_or_empty(), path);
+	strvec_pushl(&cp.args, "submodule--helper",
+		     "absorbgitdirs", NULL);
+	prepare_submodule_repo_env(&cp.env);
+	if (run_command(&cp))
+		die(_("could not recurse into submodule '%s'"), path);
+}
+
 /*
  * Migrate the git directory of the submodule given by path from
  * having its git directory within the working tree to the git dir nested
@@ -2366,21 +2383,10 @@ void absorb_git_dir_into_superproject(const char *path,
 	strbuf_release(&gitdir);
 
 	if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
-		struct child_process cp = CHILD_PROCESS_INIT;
-
 		if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)
 			BUG("we don't know how to pass the flags down?");
 
-		cp.dir = path;
-		cp.git_cmd = 1;
-		cp.no_stdin = 1;
-		strvec_pushf(&cp.args, "--super-prefix=%s%s/",
-			     get_super_prefix_or_empty(), path);
-		strvec_pushl(&cp.args, "submodule--helper",
-			     "absorbgitdirs", NULL);
-		prepare_submodule_repo_env(&cp.env);
-		if (run_command(&cp))
-			die(_("could not recurse into submodule '%s'"), path);
+		absorb_git_dir_into_superproject_recurse(path);
 	}
 }
 
-- 
2.38.0.1280.g8136eb6fab2

[PATCH 5/8] submodule API & "absorbgitdirs": remove "----recursive" option

[Ævar Arnfjörð Bjarmason]

Remove the "----recursive" option to "git submodule--helper
absorbgitdirs" (yes, with 4 dashes, not 2).

This option and all the "else" when "flags &
ABSORB_GITDIR_RECURSE_SUBMODULES" is false has never been used since
it was added in f6f85861400 (submodule: add absorb-git-dir function,
2016-12-12), which we'd have had to do as "----recursive", a
"--recursive" would have errored out.

It would be nice to follow-up with an optbug() assertion to
parse-options.c for such funnily named options, I manually validated
that this was the only long option whose name started with "-", but
let's skip adding such an assertion for now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rm.c                |  3 +--
 builtin/submodule--helper.c |  8 ++------
 submodule.c                 | 13 +++----------
 submodule.h                 |  4 +---
 4 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index f0d025a4e23..05bfe20a469 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -86,8 +86,7 @@ static void submodules_absorb_gitdir_if_needed(void)
 			continue;
 
 		if (!submodule_uses_gitfile(name))
-			absorb_git_dir_into_superproject(name,
-				ABSORB_GITDIR_RECURSE_SUBMODULES);
+			absorb_git_dir_into_superproject(name);
 	}
 }
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6250b95a6f7..8b4af8430dc 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1378,8 +1378,7 @@ static void deinit_submodule(const char *path, const char *prefix,
 					  ".git file by using absorbgitdirs."),
 					displaypath);
 
-			absorb_git_dir_into_superproject(path,
-							 ABSORB_GITDIR_RECURSE_SUBMODULES);
+			absorb_git_dir_into_superproject(path);
 
 		}
 
@@ -2830,13 +2829,10 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	int i;
 	struct pathspec pathspec = { 0 };
 	struct module_list list = MODULE_LIST_INIT;
-	unsigned flags = ABSORB_GITDIR_RECURSE_SUBMODULES;
 	struct option embed_gitdir_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
-		OPT_BIT(0, "--recursive", &flags, N_("recurse into submodules"),
-			ABSORB_GITDIR_RECURSE_SUBMODULES),
 		OPT_END()
 	};
 	const char *const git_submodule_helper_usage[] = {
@@ -2852,7 +2848,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 		goto cleanup;
 
 	for (i = 0; i < list.nr; i++)
-		absorb_git_dir_into_superproject(list.entries[i]->name, flags);
+		absorb_git_dir_into_superproject(list.entries[i]->name);
 
 	ret = 0;
 cleanup:
diff --git a/submodule.c b/submodule.c
index fe1e3f03905..8fa2ad457b2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2139,8 +2139,7 @@ int submodule_move_head(const char *path,
 	if (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {
 		if (old_head) {
 			if (!submodule_uses_gitfile(path))
-				absorb_git_dir_into_superproject(path,
-					ABSORB_GITDIR_RECURSE_SUBMODULES);
+				absorb_git_dir_into_superproject(path);
 		} else {
 			struct strbuf gitdir = STRBUF_INIT;
 			submodule_name_to_gitdir(&gitdir, the_repository,
@@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path)
  * having its git directory within the working tree to the git dir nested
  * in its superprojects git dir under modules/.
  */
-void absorb_git_dir_into_superproject(const char *path,
-				      unsigned flags)
+void absorb_git_dir_into_superproject(const char *path)
 {
 	int err_code;
 	const char *sub_git_dir;
@@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path,
 	}
 	strbuf_release(&gitdir);
 
-	if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
-		if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)
-			BUG("we don't know how to pass the flags down?");
-
-		absorb_git_dir_into_superproject_recurse(path);
-	}
+	absorb_git_dir_into_superproject_recurse(path);
 }
 
 int get_superproject_working_tree(struct strbuf *buf)
diff --git a/submodule.h b/submodule.h
index 6a9fec6de11..b52a4ff1e73 100644
--- a/submodule.h
+++ b/submodule.h
@@ -164,9 +164,7 @@ void submodule_unset_core_worktree(const struct submodule *sub);
  */
 void prepare_submodule_repo_env(struct strvec *env);
 
-#define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)
-void absorb_git_dir_into_superproject(const char *path,
-				      unsigned flags);
+void absorb_git_dir_into_superproject(const char *path);
 
 /*
  * Return the absolute path of the working tree of the superproject, which this
-- 
2.38.0.1280.g8136eb6fab2

[PATCH 6/8] submodule--helper: remove --prefix from "absorbgitdirs"

[Ævar Arnfjörð Bjarmason]

Let's pass the "-C <prefix>" option instead to "absorbgitdirs" from
its only caller.

When it was added in f6f85861400 (submodule: add absorb-git-dir
function, 2016-12-12) there were other "submodule--helper" subcommands
that were invoked with "-C <prefix>", so we could have done this all
along.

Suggested-by: Glen Choo <chooglen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/submodule--helper.c | 3 ---
 git-submodule.sh            | 2 +-
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8b4af8430dc..6bbefd34374 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2830,9 +2830,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	struct pathspec pathspec = { 0 };
 	struct module_list list = MODULE_LIST_INIT;
 	struct option embed_gitdir_options[] = {
-		OPT_STRING(0, "prefix", &prefix,
-			   N_("path"),
-			   N_("path into the working tree")),
 		OPT_END()
 	};
 	const char *const git_submodule_helper_usage[] = {
diff --git a/git-submodule.sh b/git-submodule.sh
index 5e5d21c010f..d359f171379 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -557,7 +557,7 @@ cmd_sync()
 
 cmd_absorbgitdirs()
 {
-	git submodule--helper absorbgitdirs --prefix "$wt_prefix" "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper absorbgitdirs "$@"
 }
 
 # This loop parses the command line arguments to find the
-- 
2.38.0.1280.g8136eb6fab2

[PATCH 7/8] submodule--helper: drop "update --prefix <pfx>" for "-C <pfx> update"

[Ævar Arnfjörð Bjarmason]

Since 29a5e9e1ffe (submodule--helper update-clone: learn --init,
2022-03-04) we've been passing "-C <prefix>" from "git-submodule.sh"
whenever we pass "--prefix <prefix>", so the latter is redundant to
the former. Let's drop the "--prefix" option.

Suggested-by: Glen Choo <chooglen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/submodule--helper.c | 4 +---
 git-submodule.sh            | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6bbefd34374..2012ad31d7f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2642,9 +2642,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			 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_SET_INT(0, "checkout", &opt.update_default,
 			N_("use the 'checkout' update strategy (default)"),
 			SM_UPDATE_CHECKOUT),
@@ -2700,6 +2697,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	}
 
 	opt.filter_options = &filter_options;
+	opt.prefix = prefix;
 
 	if (opt.update_default)
 		opt.update_strategy.type = opt.update_default;
diff --git a/git-submodule.sh b/git-submodule.sh
index d359f171379..9a50f2e9124 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -343,7 +343,6 @@ cmd_update()
 		${recursive:+--recursive} \
 		${init:+--init} \
 		${nofetch:+--no-fetch} \
-		${wt_prefix:+--prefix "$wt_prefix"} \
 		${rebase:+--rebase} \
 		${merge:+--merge} \
 		${checkout:+--checkout} \
-- 
2.38.0.1280.g8136eb6fab2

[PATCH 8/8] submodule--helper: use OPT_SUBCOMMAND() API

[Ævar Arnfjörð Bjarmason]

Have the cmd_submodule__helper() use the OPT_SUBCOMMAND() API
introduced in fa83cc834da (parse-options: add support for parsing
subcommands, 2022-08-19).

This is only a marginal reduction in line count, but once we start
unifying this with a yet-to-be-added "builtin/submodule.c" it'll be
much easier to reason about those changes, as they'll both use
OPT_SUBCOMMAND().

We don't need to worry about "argv[0]" being NULL in the die() because
we'd have errored out in parse_options() as we're not using
"PARSE_OPT_SUBCOMMAND_OPTIONAL".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/submodule--helper.c | 78 ++++++++++++++++++-------------------
 git.c                       |  2 +-
 2 files changed, 39 insertions(+), 41 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 2012ad31d7f..0bc25dcf5ae 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -3350,47 +3350,45 @@ static int module_add(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
-#define SUPPORT_SUPER_PREFIX (1<<0)
-
-struct cmd_struct {
-	const char *cmd;
-	int (*fn)(int, const char **, const char *);
-	unsigned option;
-};
-
-static struct cmd_struct commands[] = {
-	{"clone", module_clone, SUPPORT_SUPER_PREFIX},
-	{"add", module_add, 0},
-	{"update", module_update, SUPPORT_SUPER_PREFIX},
-	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
-	{"init", module_init, 0},
-	{"status", module_status, SUPPORT_SUPER_PREFIX},
-	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
-	{"deinit", module_deinit, 0},
-	{"summary", module_summary, 0},
-	{"push-check", push_check, 0},
-	{"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
-	{"set-url", module_set_url, 0},
-	{"set-branch", module_set_branch, 0},
-	{"create-branch", module_create_branch, 0},
-};
-
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 {
-	int i;
-	if (argc < 2 || !strcmp(argv[1], "-h"))
-		usage("git submodule--helper <command>");
-
-	for (i = 0; i < ARRAY_SIZE(commands); i++) {
-		if (!strcmp(argv[1], commands[i].cmd)) {
-			if (get_super_prefix() &&
-			    !(commands[i].option & SUPPORT_SUPER_PREFIX))
-				die(_("%s doesn't support --super-prefix"),
-				    commands[i].cmd);
-			return commands[i].fn(argc - 1, argv + 1, prefix);
-		}
-	}
+	const char *cmd = argv[0];
+	const char *subcmd;
+	parse_opt_subcommand_fn *fn = NULL;
+	const char *const usage[] = {
+		N_("git submodule--helper <command>"),
+		NULL
+	};
+	struct option options[] = {
+		OPT_SUBCOMMAND("clone", &fn, module_clone),
+		OPT_SUBCOMMAND("add", &fn, module_add),
+		OPT_SUBCOMMAND("update", &fn, module_update),
+		OPT_SUBCOMMAND("foreach", &fn, module_foreach),
+		OPT_SUBCOMMAND("init", &fn, module_init),
+		OPT_SUBCOMMAND("status", &fn, module_status),
+		OPT_SUBCOMMAND("sync", &fn, module_sync),
+		OPT_SUBCOMMAND("deinit", &fn, module_deinit),
+		OPT_SUBCOMMAND("summary", &fn, module_summary),
+		OPT_SUBCOMMAND("push-check", &fn, push_check),
+		OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs),
+		OPT_SUBCOMMAND("set-url", &fn, module_set_url),
+		OPT_SUBCOMMAND("set-branch", &fn, module_set_branch),
+		OPT_SUBCOMMAND("create-branch", &fn, module_create_branch),
+		OPT_END()
+	};
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+	subcmd = argv[0];
+
+	if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") &&
+	    strcmp(subcmd, "foreach") && strcmp(subcmd, "status") &&
+	    strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") &&
+	    get_super_prefix())
+		/*
+		 * xstrfmt() rather than "%s %s" to keep the translated
+		 * string identical to git.c's.
+		 */
+		die(_("%s doesn't support --super-prefix"),
+		    xstrfmt("'%s %s'", cmd, subcmd));
 
-	die(_("'%s' is not a valid submodule--helper "
-	      "subcommand"), argv[1]);
+	return fn(argc, argv, prefix);
 }
diff --git a/git.c b/git.c
index ee7758dcb0e..fb69e605912 100644
--- a/git.c
+++ b/git.c
@@ -610,7 +610,7 @@ static struct cmd_struct commands[] = {
 	{ "stash", cmd_stash, RUN_SETUP | NEED_WORK_TREE },
 	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
 	{ "stripspace", cmd_stripspace },
-	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },
+	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX },
 	{ "switch", cmd_switch, RUN_SETUP | NEED_WORK_TREE },
 	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
 	{ "tag", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },
-- 
2.38.0.1280.g8136eb6fab2

Re: [PATCH 1/8] submodule--helper: move "config" to a test-tool

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> As with other moves to "test-tool" in f322e9f51b5 (Merge branch
> 'ab/submodule-helper-prep', 2022-09-13) the "config" sub-command was
> only used by our own tests.
>
> Let's move it over, and while doing so make it easier to reason about
> by splitting up the various uses for it into separate sub-commands, so
> that we don't need to count arguments to see what it does.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/submodule--helper.c            | 46 --------------
>  t/helper/test-submodule.c              | 84 ++++++++++++++++++++++++++
>  t/t7411-submodule-config.sh            | 28 ++++-----
>  t/t7418-submodule-sparse-gitmodules.sh |  4 +-
>  4 files changed, 100 insertions(+), 62 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a7683d35299..6250b95a6f7 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2861,51 +2861,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
>  	return ret;
>  }
>  
> -static int module_config(int argc, const char **argv, const char *prefix)
> -{
> -	enum {
> -		CHECK_WRITEABLE = 1,
> -		DO_UNSET = 2
> -	} command = 0;
> -	struct option module_config_options[] = {
> -		OPT_CMDMODE(0, "check-writeable", &command,
> -			    N_("check if it is safe to write to the .gitmodules file"),
> -			    CHECK_WRITEABLE),
> -		OPT_CMDMODE(0, "unset", &command,
> -			    N_("unset the config in the .gitmodules file"),
> -			    DO_UNSET),
> -		OPT_END()
> -	};
> -	const char *const git_submodule_helper_usage[] = {
> -		N_("git submodule--helper config <name> [<value>]"),
> -		N_("git submodule--helper config --unset <name>"),
> -		"git submodule--helper config --check-writeable",
> -		NULL
> -	};
> -
> -	argc = parse_options(argc, argv, prefix, module_config_options,
> -			     git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
> -
> -	if (argc == 1 && command == CHECK_WRITEABLE)
> -		return is_writing_gitmodules_ok() ? 0 : -1;
> -
> -	/* Equivalent to ACTION_GET in builtin/config.c */
> -	if (argc == 2 && command != DO_UNSET)
> -		return print_config_from_gitmodules(the_repository, argv[1]);
> -
> -	/* Equivalent to ACTION_SET in builtin/config.c */
> -	if (argc == 3 || (argc == 2 && command == DO_UNSET)) {
> -		const char *value = (argc == 3) ? argv[2] : NULL;
> -
> -		if (!is_writing_gitmodules_ok())
> -			die(_("please make sure that the .gitmodules file is in the working tree"));
> -
> -		return config_set_in_gitmodules_file_gently(argv[1], value);
> -	}
> -
> -	usage_with_options(git_submodule_helper_usage, module_config_options);
> -}
> -
>  static int module_set_url(int argc, const char **argv, const char *prefix)
>  {
>  	int quiet = 0;

"git submodule--helper config" isn't familiar to me, so I did a bit of
digging. It looks like it falls into the category of submodule--helper
subcommmands that exist to expose functionality that's easy to do
in-process, but otherwise inaccessible to git-submodule.sh. 

Its last use (I think) was in a6226fd772 (submodule--helper: convert the
bulk of cmd_add() to C, 2021-08-10).

> @@ -3424,7 +3379,6 @@ static struct cmd_struct commands[] = {
>  	{"summary", module_summary, 0},
>  	{"push-check", push_check, 0},
>  	{"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
> -	{"config", module_config, 0},
>  	{"set-url", module_set_url, 0},
>  	{"set-branch", module_set_branch, 0},
>  	{"create-branch", module_create_branch, 0},
> diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c
> index b7d117cd557..c7cb463a583 100644
> --- a/t/helper/test-submodule.c
> +++ b/t/helper/test-submodule.c
> @@ -111,10 +111,94 @@ static int cmd__submodule_resolve_relative_url(int argc, const char **argv)
>  	return 0;
>  }
>  
> +static int cmd__submodule_config_list(int argc, const char **argv)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		"test-tool submodule config-list <key>",
> +		NULL
> +	};
> +	argc = parse_options(argc, argv, "test-tools", options, usage,
> +			     PARSE_OPT_KEEP_ARGV0);
> +
> +	setup_git_directory();
> +
> +	if (argc == 2)
> +		return print_config_from_gitmodules(the_repository, argv[1]);
> +	usage_with_options(usage, options);
> +}
> +
> +static int cmd__submodule_config_set(int argc, const char **argv)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		"test-tool submodule config-set <key> <value>",
> +		NULL
> +	};
> +	argc = parse_options(argc, argv, "test-tools", options, usage,
> +			     PARSE_OPT_KEEP_ARGV0);
> +
> +	setup_git_directory();
> +
> +	/* Equivalent to ACTION_SET in builtin/config.c */
> +	if (argc == 3) {
> +		if (!is_writing_gitmodules_ok())
> +			die(_("please make sure that the .gitmodules file is in the working tree"));
> +
> +		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
> +	}
> +	usage_with_options(usage, options);
> +}
> +
> +static int cmd__submodule_config_unset(int argc, const char **argv)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		"test-tool submodule config-unset <key>",
> +		NULL
> +	};
> +
> +	setup_git_directory();
> +
> +	if (argc == 2) {
> +		if (!is_writing_gitmodules_ok())
> +			die(_("please make sure that the .gitmodules file is in the working tree"));
> +		return config_set_in_gitmodules_file_gently(argv[1], NULL);
> +	}
> +	usage_with_options(usage, options);
> +}
> +
> +static int cmd__submodule_config_writeable(int argc, const char **argv)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		"test-tool submodule config-writeable",
> +		NULL
> +	};
> +	setup_git_directory();
> +
> +	if (argc == 1)
> +		return is_writing_gitmodules_ok() ? 0 : -1;
> +
> +	usage_with_options(usage, options);
> +}
> +
>  static struct test_cmd cmds[] = {
>  	{ "check-name", cmd__submodule_check_name },
>  	{ "is-active", cmd__submodule_is_active },
>  	{ "resolve-relative-url", cmd__submodule_resolve_relative_url},
> +	{ "config-list", cmd__submodule_config_list },
> +	{ "config-set", cmd__submodule_config_set },
> +	{ "config-unset", cmd__submodule_config_unset },
> +	{ "config-writeable", cmd__submodule_config_writeable },
>  };
>  
>  int cmd__submodule(int argc, const char **argv)
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index c583c4e373a..2f57ecd177a 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -140,15 +140,15 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
>  test_expect_success 'reading submodules config from the working tree with "submodule--helper config"' '
>  	(cd super &&
>  		echo "../submodule" >expect &&
> -		git submodule--helper config submodule.submodule.url >actual &&
> +		test-tool submodule config-list submodule.submodule.url >actual &&
>  		test_cmp expect actual
>  	)
>  '
>  
>  test_expect_success 'unsetting submodules config from the working tree with "submodule--helper config --unset"' '
>  	(cd super &&
> -		git submodule--helper config --unset submodule.submodule.url &&
> -		git submodule--helper config submodule.submodule.url >actual &&
> +		test-tool submodule config-unset submodule.submodule.url &&
> +		test-tool submodule config-list submodule.submodule.url >actual &&
>  		test_must_be_empty actual
>  	)
>  '

In a few of the test names, we still say 'do X with "submodule--helper
config [...]"'. I think just we can drop 'with "submodule--helper config
[...]"', e.g.

     test_expect_sucess 'unsetting submodules config from the working tree'

> @@ -157,8 +157,8 @@ test_expect_success 'unsetting submodules config from the working tree with "sub
>  test_expect_success 'writing submodules config with "submodule--helper config"' '
>  	(cd super &&
>  		echo "new_url" >expect &&
> -		git submodule--helper config submodule.submodule.url "new_url" &&
> -		git submodule--helper config submodule.submodule.url >actual &&
> +		test-tool submodule config-set submodule.submodule.url "new_url" &&
> +		test-tool submodule config-list submodule.submodule.url >actual &&
>  		test_cmp expect actual
>  	)
>  '
> @@ -167,14 +167,14 @@ test_expect_success 'overwriting unstaged submodules config with "submodule--hel
>  	test_when_finished "git -C super checkout .gitmodules" &&
>  	(cd super &&
>  		echo "newer_url" >expect &&
> -		git submodule--helper config submodule.submodule.url "newer_url" &&
> -		git submodule--helper config submodule.submodule.url >actual &&
> +		test-tool submodule config-set submodule.submodule.url "newer_url" &&
> +		test-tool submodule config-list submodule.submodule.url >actual &&
>  		test_cmp expect actual
>  	)
>  '
>  
>  test_expect_success 'writeable .gitmodules when it is in the working tree' '
> -	git -C super submodule--helper config --check-writeable
> +	test-tool -C super submodule config-writeable
>  '
>  
>  test_expect_success 'writeable .gitmodules when it is nowhere in the repository' '
> @@ -183,7 +183,7 @@ test_expect_success 'writeable .gitmodules when it is nowhere in the repository'
>  	(cd super &&
>  		git rm .gitmodules &&
>  		git commit -m "remove .gitmodules from the current branch" &&
> -		git submodule--helper config --check-writeable
> +		test-tool submodule config-writeable
>  	)
>  '

The value of the writeable/unwriteable tests feels quite low to me, but
I am ok to keep them just to be safe.

>  
> @@ -191,7 +191,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the index but not i
>  	test_when_finished "git -C super checkout .gitmodules" &&
>  	(cd super &&
>  		rm -f .gitmodules &&
> -		test_must_fail git submodule--helper config --check-writeable
> +		test_must_fail test-tool submodule config-writeable
>  	)
>  '
>  
> @@ -200,7 +200,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the current branch
>  	test_when_finished "git -C super reset --hard $ORIG" &&
>  	(cd super &&
>  		git rm .gitmodules &&
> -		test_must_fail git submodule--helper config --check-writeable
> +		test_must_fail test-tool submodule config-writeable
>  	)
>  '
>  
> @@ -208,11 +208,11 @@ test_expect_success 'reading submodules config from the index when .gitmodules i
>  	ORIG=$(git -C super rev-parse HEAD) &&
>  	test_when_finished "git -C super reset --hard $ORIG" &&
>  	(cd super &&
> -		git submodule--helper config submodule.submodule.url "staged_url" &&
> +		test-tool submodule config-set submodule.submodule.url "staged_url" &&
>  		git add .gitmodules &&
>  		rm -f .gitmodules &&
>  		echo "staged_url" >expect &&
> -		git submodule--helper config submodule.submodule.url >actual &&
> +		test-tool submodule config-list submodule.submodule.url >actual &&
>  		test_cmp expect actual
>  	)
>  '
> @@ -223,7 +223,7 @@ test_expect_success 'reading submodules config from the current branch when .git
>  	(cd super &&
>  		git rm .gitmodules &&
>  		echo "../submodule" >expect &&
> -		git submodule--helper config submodule.submodule.url >actual &&
> +		test-tool submodule config-list submodule.submodule.url >actual &&
>  		test_cmp expect actual
>  	)
>  '
> diff --git a/t/t7418-submodule-sparse-gitmodules.sh b/t/t7418-submodule-sparse-gitmodules.sh
> index d5874200fdc..dde11ecce80 100755
> --- a/t/t7418-submodule-sparse-gitmodules.sh
> +++ b/t/t7418-submodule-sparse-gitmodules.sh
> @@ -50,12 +50,12 @@ test_expect_success 'sparse checkout setup which hides .gitmodules' '
>  
>  test_expect_success 'reading gitmodules config file when it is not checked out' '
>  	echo "../submodule" >expect &&
> -	git -C super submodule--helper config submodule.submodule.url >actual &&
> +	test-tool -C super submodule config-list submodule.submodule.url >actual &&
>  	test_cmp expect actual
>  '
>  
>  test_expect_success 'not writing gitmodules config file when it is not checked out' '
> -	test_must_fail git -C super submodule--helper config submodule.submodule.url newurl &&
> +	test_must_fail test-tool -C super submodule config-set submodule.submodule.url newurl &&
>  	test_path_is_missing super/.gitmodules
>  '
>  
> -- 
> 2.38.0.1280.g8136eb6fab2

Re: [PATCH 2/8] submodule tests: add tests for top-level flag output

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Exhaustively test for how combining various "mixed-level" "git
> submodule" option works. "Mixed-level" here means options that are
> accepted by a mixture of the top-level "submodule" command, and
> e.g. the "status" sub-command.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t7400-submodule-basic.sh  |  10 +++
>  t/t7422-submodule-output.sh | 169 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 179 insertions(+)
>  create mode 100755 t/t7422-submodule-output.sh
>
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index a989aafaf57..eae6a46ef3d 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -579,6 +579,16 @@ test_expect_success 'status should be "modified" after submodule commit' '
>  	grep "^+$rev2" list
>  '
>  
> +test_expect_success '"submodule --cached" command forms should be identical' '
> +	git submodule status --cached >expect &&
> +
> +	git submodule --cached >actual &&
> +	test_cmp expect actual &&
> +
> +	git submodule --cached status >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'the --cached sha1 should be rev1' '
>  	git submodule --cached status >list &&
>  	grep "^+$rev1" list
> diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh
> new file mode 100755
> index 00000000000..3ac56c23f72
> --- /dev/null
> +++ b/t/t7422-submodule-output.sh
> @@ -0,0 +1,169 @@
> +#!/bin/sh
> +
> +test_description='submodule --cached, --quiet etc. output'
> +
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-t3100.sh
> +
> +setup_sub () {
> +	local d="$1" &&
> +	shift &&
> +	git $@ clone . "$d" &&
> +	git $@ submodule add ./"$d"
> +}
> +
> +normalize_status () {
> +	sed -e 's/-g[0-9a-f]*/-gHASH/'
> +}
> +
> +test_expect_success 'setup' '
> +	test_commit A &&
> +	test_commit B &&
> +	setup_sub S  &&
> +	setup_sub S.D &&
> +	setup_sub S.C &&
> +	setup_sub S.C.D &&
> +	setup_sub X &&
> +	git add S* &&
> +	test_commit C &&
> +
> +	# recursive in X/
> +	git -C X pull &&
> +	GIT_ALLOW_PROTOCOL=file git -C X submodule update --init &&
> +
> +	# dirty p
> +	for d in S.D X/S.D
> +	do
> +		echo dirty >S.D/A.t || return 1
> +	done &&

I think this was meant to be replaced by the loop labelled # dirty ?
This doesn't look like it will work, e.g. $d isn't used.

> +
> +	# commit (for --cached)
> +	for d in S.C* X/S.C*
> +	do
> +		git -C "$d" reset --hard A || return 1
> +	done &&
> +
> +	# dirty
> +	for d in S*.D X/S*.D
> +	do
> +		echo dirty >"$d/C2.t" || return 1
> +	done &&
> +
> +	for ref in A B C
> +	do
> +		# Not different with SHA-1 and SHA-256, just (ab)usign
> +		# test_oid_cache as a variable bag to avoid using
> +		# $(git rev-parse ...).
> +		oid=$(git rev-parse $ref) &&
> +		test_oid_cache <<-EOF || return 1
> +		$ref sha1:$oid
> +		$ref sha256:$oid
> +		EOF
> +	done
> +'
> +
> +for opts in "" "status"
> +do
> +	test_expect_success "git submodule $opts" '
> +		sed -e "s/^>//" >expect <<-EOF &&
> +		> $(test_oid B) S (B)
> +		>+$(test_oid A) S.C (A)
> +		>+$(test_oid A) S.C.D (A)
> +		> $(test_oid B) S.D (B)
> +		>+$(test_oid C) X (C)
> +		EOF
> +		git submodule $opts >actual.raw &&
> +		normalize_status <actual.raw >actual &&
> +		test_cmp expect actual
> +	'
> +done
> +
> +for opts in \
> +	"status --recursive"

We don't test "--recursive status" or "--recursive" because we don't
parse the top level "--recursive" flag, right?

> +do
> +	test_expect_success "git submodule $opts" '
> +		sed -e "s/^>//" >expect <<-EOF &&
> +		> $(test_oid B) S (B)
> +		>+$(test_oid A) S.C (A)
> +		>+$(test_oid A) S.C.D (A)
> +		> $(test_oid B) S.D (B)
> +		>+$(test_oid C) X (C)
> +		> $(test_oid B) X/S (B)
> +		>+$(test_oid A) X/S.C (A)
> +		>+$(test_oid A) X/S.C.D (A)
> +		> $(test_oid B) X/S.D (B)
> +		> $(test_oid B) X/X (B)
> +		EOF
> +		git submodule $opts >actual.raw &&
> +		normalize_status <actual.raw >actual &&
> +		test_cmp expect actual
> +	'
> +done
> +
> +for opts in \
> +	"--quiet" \
> +	"--quiet status" \
> +	"status --quiet"
> +do
> +	test_expect_success "git submodule $opts" '
> +		git submodule $opts >out &&
> +		test_must_be_empty out
> +	'
> +done
> +
> +for opts in \
> +	"--cached" \
> +	"--cached status" \
> +	"status --cached"
> +do
> +	test_expect_success "git submodule $opts" '
> +		sed -e "s/^>//" >expect <<-EOF &&
> +		> $(test_oid B) S (B)
> +		>+$(test_oid B) S.C (B)
> +		>+$(test_oid B) S.C.D (B)
> +		> $(test_oid B) S.D (B)
> +		>+$(test_oid B) X (B)
> +		EOF
> +		git submodule $opts >actual.raw &&
> +		normalize_status <actual.raw >actual &&
> +		test_cmp expect actual
> +	'
> +done
> +
> +for opts in \
> +	"--cached --quiet" \
> +	"--cached --quiet status" \
> +	"--cached status --quiet" \
> +	"--quiet status --cached" \
> +	"status --cached --quiet"
> +do
> +	test_expect_success "git submodule $opts" '
> +		git submodule $opts >out &&
> +		test_must_be_empty out
> +	'
> +done
> +
> +for opts in \
> +	"status --cached --recursive" \
> +	"--cached status --recursive"
> +do
> +	test_expect_success "git submodule $opts" '
> +		sed -e "s/^>//" >expect <<-EOF &&
> +		> $(test_oid B) S (B)
> +		>+$(test_oid B) S.C (B)
> +		>+$(test_oid B) S.C.D (B)
> +		> $(test_oid B) S.D (B)
> +		>+$(test_oid B) X (B)
> +		> $(test_oid B) X/S (B)
> +		>+$(test_oid B) X/S.C (B)
> +		>+$(test_oid B) X/S.C.D (B)
> +		> $(test_oid B) X/S.D (B)
> +		> $(test_oid B) X/X (B)
> +		EOF
> +		git submodule $opts >actual.raw &&
> +		normalize_status <actual.raw >actual &&
> +		test_cmp expect actual
> +	'
> +done
> +
> +test_done
> -- 
> 2.38.0.1280.g8136eb6fab2

Re: [PATCH 5/8] submodule API & "absorbgitdirs": remove "----recursive" option

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Remove the "----recursive" option to "git submodule--helper
> absorbgitdirs" (yes, with 4 dashes, not 2).

o.O

At least this makes it pretty easy to grep for usage, and it makes sense
that we've never used it (otherwise this would've been caught).

> diff --git a/submodule.c b/submodule.c
> index fe1e3f03905..8fa2ad457b2 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path)
>   * having its git directory within the working tree to the git dir nested
>   * in its superprojects git dir under modules/.
>   */
> -void absorb_git_dir_into_superproject(const char *path,
> -				      unsigned flags)
> +void absorb_git_dir_into_superproject(const char *path)
>  {
>  	int err_code;
>  	const char *sub_git_dir;
> @@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path,
>  	}
>  	strbuf_release(&gitdir);
>  
> -	if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
> -		if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)
> -			BUG("we don't know how to pass the flags down?");
> -
> -		absorb_git_dir_into_superproject_recurse(path);
> -	}
> +	absorb_git_dir_into_superproject_recurse(path);
>  }

Maybe I'm misreading, but I don't follow this change.

Before, we recursed into the submodule only if the
ABSORB_GITDIR_RECURSE_SUBMODULES flag is set (which we now know is
never), but now we unconditionally recurse into the submodule.

Wouldn't it be more accurate to get rid of this recursing behavior
altogether? i.e. drop the previous patch and delete that code when we
delete this conditional.

Re: [PATCH 8/8] submodule--helper: use OPT_SUBCOMMAND() API

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Have the cmd_submodule__helper() use the OPT_SUBCOMMAND() API
> introduced in fa83cc834da (parse-options: add support for parsing
> subcommands, 2022-08-19).
>
> This is only a marginal reduction in line count, but once we start
> unifying this with a yet-to-be-added "builtin/submodule.c" it'll be
> much easier to reason about those changes, as they'll both use
> OPT_SUBCOMMAND().

Even if nothing else, this is a nice standardization change :)

> We don't need to worry about "argv[0]" being NULL in the die() because
> we'd have errored out in parse_options() as we're not using
> "PARSE_OPT_SUBCOMMAND_OPTIONAL".
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/submodule--helper.c | 78 ++++++++++++++++++-------------------
>  git.c                       |  2 +-
>  2 files changed, 39 insertions(+), 41 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 2012ad31d7f..0bc25dcf5ae 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -3350,47 +3350,45 @@ static int module_add(int argc, const char **argv, const char *prefix)
>  	return ret;
>  }
>  
> -#define SUPPORT_SUPER_PREFIX (1<<0)
> -
> -struct cmd_struct {
> -	const char *cmd;
> -	int (*fn)(int, const char **, const char *);
> -	unsigned option;
> -};
> -
> -static struct cmd_struct commands[] = {
> -	{"clone", module_clone, SUPPORT_SUPER_PREFIX},
> -	{"add", module_add, 0},
> -	{"update", module_update, SUPPORT_SUPER_PREFIX},
> -	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
> -	{"init", module_init, 0},
> -	{"status", module_status, SUPPORT_SUPER_PREFIX},
> -	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
> -	{"deinit", module_deinit, 0},
> -	{"summary", module_summary, 0},
> -	{"push-check", push_check, 0},
> -	{"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
> -	{"set-url", module_set_url, 0},
> -	{"set-branch", module_set_branch, 0},
> -	{"create-branch", module_create_branch, 0},
> -};
> -
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>  {
> -	int i;
> -	if (argc < 2 || !strcmp(argv[1], "-h"))
> -		usage("git submodule--helper <command>");
> -
> -	for (i = 0; i < ARRAY_SIZE(commands); i++) {
> -		if (!strcmp(argv[1], commands[i].cmd)) {
> -			if (get_super_prefix() &&
> -			    !(commands[i].option & SUPPORT_SUPER_PREFIX))
> -				die(_("%s doesn't support --super-prefix"),
> -				    commands[i].cmd);
> -			return commands[i].fn(argc - 1, argv + 1, prefix);
> -		}
> -	}
> +	const char *cmd = argv[0];
> +	const char *subcmd;
> +	parse_opt_subcommand_fn *fn = NULL;
> +	const char *const usage[] = {
> +		N_("git submodule--helper <command>"),
> +		NULL
> +	};
> +	struct option options[] = {
> +		OPT_SUBCOMMAND("clone", &fn, module_clone),
> +		OPT_SUBCOMMAND("add", &fn, module_add),
> +		OPT_SUBCOMMAND("update", &fn, module_update),
> +		OPT_SUBCOMMAND("foreach", &fn, module_foreach),
> +		OPT_SUBCOMMAND("init", &fn, module_init),
> +		OPT_SUBCOMMAND("status", &fn, module_status),
> +		OPT_SUBCOMMAND("sync", &fn, module_sync),
> +		OPT_SUBCOMMAND("deinit", &fn, module_deinit),
> +		OPT_SUBCOMMAND("summary", &fn, module_summary),
> +		OPT_SUBCOMMAND("push-check", &fn, push_check),
> +		OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs),
> +		OPT_SUBCOMMAND("set-url", &fn, module_set_url),
> +		OPT_SUBCOMMAND("set-branch", &fn, module_set_branch),
> +		OPT_SUBCOMMAND("create-branch", &fn, module_create_branch),
> +		OPT_END()
> +	};
> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
> +	subcmd = argv[0];
> +
> +	if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") &&
> +	    strcmp(subcmd, "foreach") && strcmp(subcmd, "status") &&
> +	    strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") &&
> +	    get_super_prefix())
> +		/*
> +		 * xstrfmt() rather than "%s %s" to keep the translated
> +		 * string identical to git.c's.
> +		 */
> +		die(_("%s doesn't support --super-prefix"),
> +		    xstrfmt("'%s %s'", cmd, subcmd));

FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks
in both the top level and in submodule--helper (i.e. revisiting my
complaints from [1]), but I haven't sent it out yet because I haven't
found the right way to motivate that change. Feel free to chime in on
that series when it comes out.

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

> -	die(_("'%s' is not a valid submodule--helper "
> -	      "subcommand"), argv[1]);
> +	return fn(argc, argv, prefix);
>  }
> diff --git a/git.c b/git.c
> index ee7758dcb0e..fb69e605912 100644
> --- a/git.c
> +++ b/git.c
> @@ -610,7 +610,7 @@ static struct cmd_struct commands[] = {
>  	{ "stash", cmd_stash, RUN_SETUP | NEED_WORK_TREE },
>  	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
>  	{ "stripspace", cmd_stripspace },
> -	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },
> +	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX },
>  	{ "switch", cmd_switch, RUN_SETUP | NEED_WORK_TREE },
>  	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
>  	{ "tag", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },
> -- 
> 2.38.0.1280.g8136eb6fab2

Re: [PATCH 8/8] submodule--helper: use OPT_SUBCOMMAND() API

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 03 2022, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> [...]
>> +	struct option options[] = {
>> +		OPT_SUBCOMMAND("clone", &fn, module_clone),
>> +		OPT_SUBCOMMAND("add", &fn, module_add),
>> +		OPT_SUBCOMMAND("update", &fn, module_update),
>> +		OPT_SUBCOMMAND("foreach", &fn, module_foreach),
>> +		OPT_SUBCOMMAND("init", &fn, module_init),
>> +		OPT_SUBCOMMAND("status", &fn, module_status),
>> +		OPT_SUBCOMMAND("sync", &fn, module_sync),
>> +		OPT_SUBCOMMAND("deinit", &fn, module_deinit),
>> +		OPT_SUBCOMMAND("summary", &fn, module_summary),
>> +		OPT_SUBCOMMAND("push-check", &fn, push_check),
>> +		OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs),
>> +		OPT_SUBCOMMAND("set-url", &fn, module_set_url),
>> +		OPT_SUBCOMMAND("set-branch", &fn, module_set_branch),
>> +		OPT_SUBCOMMAND("create-branch", &fn, module_create_branch),
>> +		OPT_END()
>> +	};
>> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
>> +	subcmd = argv[0];
>> +
>> +	if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") &&
>> +	    strcmp(subcmd, "foreach") && strcmp(subcmd, "status") &&
>> +	    strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") &&
>> +	    get_super_prefix())
>> +		/*
>> +		 * xstrfmt() rather than "%s %s" to keep the translated
>> +		 * string identical to git.c's.
>> +		 */
>> +		die(_("%s doesn't support --super-prefix"),
>> +		    xstrfmt("'%s %s'", cmd, subcmd));
>
> FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks
> in both the top level and in submodule--helper (i.e. revisiting my
> complaints from [1]), but I haven't sent it out yet because I haven't
> found the right way to motivate that change. Feel free to chime in on
> that series when it comes out.
>
> [1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/

Maybe you have different plans, but keep at the WIP re-roll of what I
have after this, I've also removed all of that.

Basically, ending up with:
	
	--- a/builtin.h
	+++ b/builtin.h
	@@ -224,7 +224,14 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix);
	 int cmd_status(int argc, const char **argv, const char *prefix);
	 int cmd_stash(int argc, const char **argv, const char *prefix);
	 int cmd_stripspace(int argc, const char **argv, const char *prefix);
	+int cmd_submodule(int argc, const char **argv, const char *prefix);
	 int cmd_submodule__helper(int argc, const char **argv, const char *prefix);
	+int cmd_submodule__helper_clone(int argc, const char **argv, const char *prefix);
	+int cmd_submodule_absorbgitdirs(int argc, const char **argv, const char *prefix);
	+int cmd_submodule_foreach(int argc, const char **argv, const char *prefix);
	+int cmd_submodule_status(int argc, const char **argv, const char *prefix);
	+int cmd_submodule_sync(int argc, const char **argv, const char *prefix);
	+int cmd_submodule_update(int argc, const char **argv, const char *prefix);
	 int cmd_switch(int argc, const char **argv, const char *prefix);
	 int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
	 int cmd_tag(int argc, const char **argv, const char *prefix);

And changes like:
	
	@@ -366,7 +365,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
	 
	                strvec_pushl(&cpr.args, "--super-prefix", NULL);
	                strvec_pushf(&cpr.args, "%s/", displaypath);
	-               strvec_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive",
	+               strvec_pushl(&cpr.args, "submodule--helper-foreach", "--recursive",
	                             NULL);
	 
	                if (info->quiet)

I.e. when we call "foreach" we dispatch to cmd_submodule_foreach(), but
when "foreach" needs to recurse it doesn't invoke a "git submodule
foreach", instead it invokes "git submodule--helper-foreach".

The reason for doing that is that we can't promote the helper to a
built-in without also leaking implementation detail that we support the
now internal-only --super-prefix in the command itself.

So this code becomes:
	
	@@ -3352,43 +3304,17 @@ static int module_add(int argc, const char **argv, const char *prefix)
	 
	 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
	 {
	-       const char *cmd = argv[0];
	-       const char *subcmd;
	        parse_opt_subcommand_fn *fn = NULL;
	        const char *const usage[] = {
	                N_("git submodule--helper <command>"),
	                NULL
	        };
	        struct option options[] = {
	-               OPT_SUBCOMMAND("clone", &fn, module_clone),
	-               OPT_SUBCOMMAND("add", &fn, module_add),
	-               OPT_SUBCOMMAND("update", &fn, module_update),
	-               OPT_SUBCOMMAND("foreach", &fn, module_foreach),
	-               OPT_SUBCOMMAND("init", &fn, module_init),
	-               OPT_SUBCOMMAND("status", &fn, module_status),
	-               OPT_SUBCOMMAND("sync", &fn, module_sync),
	-               OPT_SUBCOMMAND("deinit", &fn, module_deinit),
	-               OPT_SUBCOMMAND("summary", &fn, module_summary),
	-               OPT_SUBCOMMAND("push-check", &fn, push_check),
	-               OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs),
	-               OPT_SUBCOMMAND("set-url", &fn, module_set_url),
	-               OPT_SUBCOMMAND("set-branch", &fn, module_set_branch),
	-               OPT_SUBCOMMAND("create-branch", &fn, module_create_branch),
	+               OPT_SUBCOMMAND("push-check", &fn, cmd_submodule_push_check),
	+               OPT_SUBCOMMAND("create-branch", &fn, cmd_submodule_create_branch),
	                OPT_END()
	        };
	        argc = parse_options(argc, argv, prefix, options, usage, 0);
	-       subcmd = argv[0];
	-
	-       if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") &&
	-           strcmp(subcmd, "foreach") && strcmp(subcmd, "status") &&
	-           strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") &&
	-           get_super_prefix())
	-               /*
	-                * xstrfmt() rather than "%s %s" to keep the translated
	-                * string identical to git.c's.
	-                */
	-               die(_("%s doesn't support --super-prefix"),
	-                   xstrfmt("'%s %s'", cmd, subcmd));
	 
	        return fn(argc, argv, prefix);
	 }

I.e. for all the "super-prefix" ones we dispatch directly (and maybe I
should just do that too for those two stragglers).

It's ugly, but it's only ugly on the inside, but if you can think of a
better way to do it...

Re: [PATCH 5/8] submodule API & "absorbgitdirs": remove "----recursive" option

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 03 2022, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Remove the "----recursive" option to "git submodule--helper
>> absorbgitdirs" (yes, with 4 dashes, not 2).
>
> o.O
>
> At least this makes it pretty easy to grep for usage, and it makes sense
> that we've never used it (otherwise this would've been caught).
>
>> diff --git a/submodule.c b/submodule.c
>> index fe1e3f03905..8fa2ad457b2 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path)
>>   * having its git directory within the working tree to the git dir nested
>>   * in its superprojects git dir under modules/.
>>   */
>> -void absorb_git_dir_into_superproject(const char *path,
>> -				      unsigned flags)
>> +void absorb_git_dir_into_superproject(const char *path)
>>  {
>>  	int err_code;
>>  	const char *sub_git_dir;
>> @@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path,
>>  	}
>>  	strbuf_release(&gitdir);
>>  
>> -	if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
>> -		if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)
>> -			BUG("we don't know how to pass the flags down?");
>> -
>> -		absorb_git_dir_into_superproject_recurse(path);
>> -	}
>> +	absorb_git_dir_into_superproject_recurse(path);
>>  }
>
> Maybe I'm misreading, but I don't follow this change.
>
> Before, we recursed into the submodule only if the
> ABSORB_GITDIR_RECURSE_SUBMODULES flag is set (which we now know is
> never), but now we unconditionally recurse into the submodule.

No, it's always set. I.e. ----recursive did nothing, but the default was
to always set ABSORB_GITDIR_RECURSE_SUBMODULES, so it was never not-set
(and there was no --no---recursive user).

So we should be unconditionally going on this recursive path.

FW: [PATCH 5/8] submodule API & "absorbgitdirs": remove "----recursive" option

[Simpson, Phyllis]

Hello.

I do not wish to get these messages any longer.

Thank you.

Phyllis 

P Simpson  |  Application Systems Analyst Programmer - Int
Office of Technology & Cybersecurity
office: 317-233-8477
psimpson@health.in.gov
health.in.gov
   





-----Original Message-----
From: Ævar Arnfjörð Bjarmason <avarab@gmail.com> 
Sent: Thursday, November 3, 2022 9:43 PM
To: Glen Choo <chooglen@google.com>
Cc: git@vger.kernel.org; Junio C Hamano <gitster@pobox.com>; Emily Shaffer <emilyshaffer@google.com>
Subject: Re: [PATCH 5/8] submodule API & "absorbgitdirs": remove "----recursive" option

**** This is an EXTERNAL email. Exercise caution. DO NOT open attachments or click links from unknown senders or unexpected email. **** ________________________________

On Thu, Nov 03 2022, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Remove the "----recursive" option to "git submodule--helper 
>> absorbgitdirs" (yes, with 4 dashes, not 2).
>
> o.O
>
> At least this makes it pretty easy to grep for usage, and it makes 
> sense that we've never used it (otherwise this would've been caught).
>
>> diff --git a/submodule.c b/submodule.c index fe1e3f03905..8fa2ad457b2 
>> 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path)
>>   * having its git directory within the working tree to the git dir nested
>>   * in its superprojects git dir under modules/.
>>   */
>> -void absorb_git_dir_into_superproject(const char *path,
>> -                                  unsigned flags)
>> +void absorb_git_dir_into_superproject(const char *path)
>>  {
>>      int err_code;
>>      const char *sub_git_dir;
>> @@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path,
>>      }
>>      strbuf_release(&gitdir);
>>
>> -    if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
>> -            if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)
>> -                    BUG("we don't know how to pass the flags down?");
>> -
>> -            absorb_git_dir_into_superproject_recurse(path);
>> -    }
>> +    absorb_git_dir_into_superproject_recurse(path);
>>  }
>
> Maybe I'm misreading, but I don't follow this change.
>
> Before, we recursed into the submodule only if the 
> ABSORB_GITDIR_RECURSE_SUBMODULES flag is set (which we now know is 
> never), but now we unconditionally recurse into the submodule.

No, it's always set. I.e. ----recursive did nothing, but the default was to always set ABSORB_GITDIR_RECURSE_SUBMODULES, so it was never not-set (and there was no --no---recursive user).

So we should be unconditionally going on this recursive path.

Re: [PATCH 8/8] submodule--helper: use OPT_SUBCOMMAND() API

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Thu, Nov 03 2022, Glen Choo wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> [...]
>>> +	struct option options[] = {
>>> +		OPT_SUBCOMMAND("clone", &fn, module_clone),
>>> +		OPT_SUBCOMMAND("add", &fn, module_add),
>>> +		OPT_SUBCOMMAND("update", &fn, module_update),
>>> +		OPT_SUBCOMMAND("foreach", &fn, module_foreach),
>>> +		OPT_SUBCOMMAND("init", &fn, module_init),
>>> +		OPT_SUBCOMMAND("status", &fn, module_status),
>>> +		OPT_SUBCOMMAND("sync", &fn, module_sync),
>>> +		OPT_SUBCOMMAND("deinit", &fn, module_deinit),
>>> +		OPT_SUBCOMMAND("summary", &fn, module_summary),
>>> +		OPT_SUBCOMMAND("push-check", &fn, push_check),
>>> +		OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs),
>>> +		OPT_SUBCOMMAND("set-url", &fn, module_set_url),
>>> +		OPT_SUBCOMMAND("set-branch", &fn, module_set_branch),
>>> +		OPT_SUBCOMMAND("create-branch", &fn, module_create_branch),
>>> +		OPT_END()
>>> +	};
>>> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
>>> +	subcmd = argv[0];
>>> +
>>> +	if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") &&
>>> +	    strcmp(subcmd, "foreach") && strcmp(subcmd, "status") &&
>>> +	    strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") &&
>>> +	    get_super_prefix())
>>> +		/*
>>> +		 * xstrfmt() rather than "%s %s" to keep the translated
>>> +		 * string identical to git.c's.
>>> +		 */
>>> +		die(_("%s doesn't support --super-prefix"),
>>> +		    xstrfmt("'%s %s'", cmd, subcmd));
>>
>> FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks
>> in both the top level and in submodule--helper (i.e. revisiting my
>> complaints from [1]), but I haven't sent it out yet because I haven't
>> found the right way to motivate that change. Feel free to chime in on
>> that series when it comes out.
>>
>> [1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/
>
> Maybe you have different plans, but keep at the WIP re-roll of what I
> have after this, I've also removed all of that.
>
> Basically, ending up with:
> 	
> 	--- a/builtin.h
> 	+++ b/builtin.h
> 	@@ -224,7 +224,14 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix);
> 	 int cmd_status(int argc, const char **argv, const char *prefix);
> 	 int cmd_stash(int argc, const char **argv, const char *prefix);
> 	 int cmd_stripspace(int argc, const char **argv, const char *prefix);
> 	+int cmd_submodule(int argc, const char **argv, const char *prefix);
> 	 int cmd_submodule__helper(int argc, const char **argv, const char *prefix);
> 	+int cmd_submodule__helper_clone(int argc, const char **argv, const char *prefix);
> 	+int cmd_submodule_absorbgitdirs(int argc, const char **argv, const char *prefix);
> 	+int cmd_submodule_foreach(int argc, const char **argv, const char *prefix);
> 	+int cmd_submodule_status(int argc, const char **argv, const char *prefix);
> 	+int cmd_submodule_sync(int argc, const char **argv, const char *prefix);
> 	+int cmd_submodule_update(int argc, const char **argv, const char *prefix);
> 	 int cmd_switch(int argc, const char **argv, const char *prefix);
> 	 int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
> 	 int cmd_tag(int argc, const char **argv, const char *prefix);
>
> And changes like:
> 	
> 	@@ -366,7 +365,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
> 	 
> 	                strvec_pushl(&cpr.args, "--super-prefix", NULL);
> 	                strvec_pushf(&cpr.args, "%s/", displaypath);
> 	-               strvec_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive",
> 	+               strvec_pushl(&cpr.args, "submodule--helper-foreach", "--recursive",
> 	                             NULL);
> 	 
> 	                if (info->quiet)
>
> I.e. when we call "foreach" we dispatch to cmd_submodule_foreach(), but
> when "foreach" needs to recurse it doesn't invoke a "git submodule
> foreach", instead it invokes "git submodule--helper-foreach".
>
> The reason for doing that is that we can't promote the helper to a
> built-in without also leaking implementation detail that we support the
> now internal-only --super-prefix in the command itself.
>
> So this code becomes:
> 	
> 	@@ -3352,43 +3304,17 @@ static int module_add(int argc, const char **argv, const char *prefix)
> 	 
> 	 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> 	 {
> 	-       const char *cmd = argv[0];
> 	-       const char *subcmd;
> 	        parse_opt_subcommand_fn *fn = NULL;
> 	        const char *const usage[] = {
> 	                N_("git submodule--helper <command>"),
> 	                NULL
> 	        };
> 	        struct option options[] = {
> 	-               OPT_SUBCOMMAND("clone", &fn, module_clone),
> 	-               OPT_SUBCOMMAND("add", &fn, module_add),
> 	-               OPT_SUBCOMMAND("update", &fn, module_update),
> 	-               OPT_SUBCOMMAND("foreach", &fn, module_foreach),
> 	-               OPT_SUBCOMMAND("init", &fn, module_init),
> 	-               OPT_SUBCOMMAND("status", &fn, module_status),
> 	-               OPT_SUBCOMMAND("sync", &fn, module_sync),
> 	-               OPT_SUBCOMMAND("deinit", &fn, module_deinit),
> 	-               OPT_SUBCOMMAND("summary", &fn, module_summary),
> 	-               OPT_SUBCOMMAND("push-check", &fn, push_check),
> 	-               OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs),
> 	-               OPT_SUBCOMMAND("set-url", &fn, module_set_url),
> 	-               OPT_SUBCOMMAND("set-branch", &fn, module_set_branch),
> 	-               OPT_SUBCOMMAND("create-branch", &fn, module_create_branch),
> 	+               OPT_SUBCOMMAND("push-check", &fn, cmd_submodule_push_check),
> 	+               OPT_SUBCOMMAND("create-branch", &fn, cmd_submodule_create_branch),
> 	                OPT_END()
> 	        };
> 	        argc = parse_options(argc, argv, prefix, options, usage, 0);
> 	-       subcmd = argv[0];
> 	-
> 	-       if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") &&
> 	-           strcmp(subcmd, "foreach") && strcmp(subcmd, "status") &&
> 	-           strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") &&
> 	-           get_super_prefix())
> 	-               /*
> 	-                * xstrfmt() rather than "%s %s" to keep the translated
> 	-                * string identical to git.c's.
> 	-                */
> 	-               die(_("%s doesn't support --super-prefix"),
> 	-                   xstrfmt("'%s %s'", cmd, subcmd));
> 	 
> 	        return fn(argc, argv, prefix);
> 	 }
>
> I.e. for all the "super-prefix" ones we dispatch directly (and maybe I
> should just do that too for those two stragglers).
>
> It's ugly, but it's only ugly on the inside, but if you can think of a
> better way to do it...

If we _really_ wanted to keep the check, an alternative might be to
teach the subcommand parser about SUPPORT_SUPER_PREFIX. But frankly, I
think this SUPPORT_SUPER_PREFIX check is far more trouble than it's
worth, and I wouldn't want you to spend your time trying to find ways to
keep it only for it to be dropped altogether.

Here's a snippet from the cover letter I'm working on, which will
hopefully convince you that we don't need to worry about this any more.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----

  When we introduced the internal-only "--super-prefix" in 74866d7579 (git: make
  super-prefix option, 2016-10-07), we specified that commands must prefix paths
  by it, and pathspecs must be parsed relative to it. That commit also includes
  safeguards to ensure that "--super-prefix" is used correctly, namely:

  - Only commands marked SUPPORT_SUPER_PREFIX can be invoked with "--super-prefix".
  - The super prefix is propagated via the GIT_INTERNAL_SUPER_PREFIX env var, so a
    non-SUPPORT_SUPER_PREFIX command cannot be invoked by a SUPPORT_SUPER_PREFIX
    one.

  However, its use is inconsistent, which is a strong reason to consider using
  better-scoped flags instead. For example..

    - Of the 4 commands that are SUPPORT_SUPER_PREFIX, only "read-tree" and
      "submodule--helper" do anything useful with it. "fsmonitor--daemon" has it
      to avoid the SUPPORT_SUPER_PREFIX warning [1]. "checkout--worker" doesn't have
      a documented reason, but since it can be invoked by "read-tree", it's
      presumably also a workaround.
    - "read-tree" and "submodule--helper" use different values for the super prefix;
      "read-tree" passes the path from the root of the superproject's tree to the
      submodule's gitlink, while "submodule--helper" passes the relative path of the
      original CWD to the submodule.
    - "submodule--helper" doesn't use "--super-prefix" to parse pathspecs, only to
      display paths.

  This series fixes replaces "--super-prefix" with such better-scoped flags, and
  fixes some bugs resulting from the SUPPORT_SUPER_PREFIX check.

[1] 53fcfbc84f (fsmonitor--daemon: allow --super-prefix argument, 2022-05-26)

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----

I figured out all the details, but it would look something like:

- Add an internal-only "--tree-super-prefix" flag to "git read-tree",
  which sets a global variable that is read from unpack-trees.c.
- Add an internal-only "--wt-super-prefix" flag to "git
  submodule--helper", which sets a global variable that is read from
  submodule.c. Unlike "--super-prefix", this won't be gated behind a
  SUPPORT_SUPER_PREFIX for each subcommand, since AFAICT, every
  subcommand is using this "--wt-super-prefix" correctly, so we can just
  make sure that new subcommands do too.
- Remove the global "--super-prefix", the corresponding env var and
  SUPPORT_SUPER_PREFIX.

Re: [PATCH 5/8] submodule API & "absorbgitdirs": remove "----recursive" option

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Thu, Nov 03 2022, Glen Choo wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> Remove the "----recursive" option to "git submodule--helper
>>> absorbgitdirs" (yes, with 4 dashes, not 2).
>>
>> o.O
>>
>> At least this makes it pretty easy to grep for usage, and it makes sense
>> that we've never used it (otherwise this would've been caught).
>>
>>> diff --git a/submodule.c b/submodule.c
>>> index fe1e3f03905..8fa2ad457b2 100644
>>> --- a/submodule.c
>>> +++ b/submodule.c
>>> @@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path)
>>>   * having its git directory within the working tree to the git dir nested
>>>   * in its superprojects git dir under modules/.
>>>   */
>>> -void absorb_git_dir_into_superproject(const char *path,
>>> -				      unsigned flags)
>>> +void absorb_git_dir_into_superproject(const char *path)
>>>  {
>>>  	int err_code;
>>>  	const char *sub_git_dir;
>>> @@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path,
>>>  	}
>>>  	strbuf_release(&gitdir);
>>>  
>>> -	if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
>>> -		if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)
>>> -			BUG("we don't know how to pass the flags down?");
>>> -
>>> -		absorb_git_dir_into_superproject_recurse(path);
>>> -	}
>>> +	absorb_git_dir_into_superproject_recurse(path);
>>>  }
>>
>> Maybe I'm misreading, but I don't follow this change.
>>
>> Before, we recursed into the submodule only if the
>> ABSORB_GITDIR_RECURSE_SUBMODULES flag is set (which we now know is
>> never), but now we unconditionally recurse into the submodule.
>
> No, it's always set. I.e. ----recursive did nothing, but the default was
> to always set ABSORB_GITDIR_RECURSE_SUBMODULES, so it was never not-set
> (and there was no --no---recursive user).
>
> So we should be unconditionally going on this recursive path.

Ah, because we initialize flags to ABSORB_GITDIR_RECURSE_SUBMODULES. I
see that this is also covered by t/t7412-submodule-absorbgitdirs.sh,
which has a few nested submodules tests. Thanks!

Since it's clear that recursing should be unconditional, I think we
don't need the previous patch, but I'm fine either way.

Re: [PATCH 0/8] submodule: tests, cleanup to prepare for built-in

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I have a topic on-list to remove git-submodule.sh and create a
> builtin/submodule.c, i.e. promoting "git submodule--helper" to the
> "real thing"[1].
>
> Glen gave me a bunch of good feedback on it, including (but not
> limited to) pointing out that we have outstanding dead code in
> [2][3].
>
> Once I started pulling at that thread things became a lot simpler for
> the re-roll of [1], e.g. the migration of git-submodule.sh's commands
> can squash in the "update" step, as it's no longer a special-case.
>
> But that also made the series larger, and it's conflicted with other
> outstanding patches. First René's strvec() cleanup in submodule.c, and
> currently with Glen's in-flight submodule topic.
>
> So here's "just the prep" part of that split-out. See also [4] and [5]
> for previous "prep" topics, we're getting closer...
>
> This only adds missing test coverage, and deletes dead code that we'd
> otherwise have to account for. Then 8/8 converts submodule--helper to
> use the OPT_SUBCOMMAND() API in 8/8.

Thanks for working at this so patiently :) I can't wait to see
git-submodule.sh gone.

The only issue I saw was the out-of-date test names in [1]. Otherwise, I
think this is ready to merge.

[1] https://lore.kernel.org/git/kl6lfsezofk9.fsf@chooglen-macbookpro.roam.corp.google.com

Re: [PATCH 0/8] submodule: tests, cleanup to prepare for built-in

[Taylor Blau]

On Fri, Nov 04, 2022 at 10:10:29AM -0700, Glen Choo wrote:
> Thanks for working at this so patiently :) I can't wait to see
> git-submodule.sh gone.
>
> The only issue I saw was the out-of-date test names in [1]. Otherwise, I
> think this is ready to merge.
>
> [1] https://lore.kernel.org/git/kl6lfsezofk9.fsf@chooglen-macbookpro.roam.corp.google.com

Thanks for the review. I'll wait for a reroll on this topic and then we
can start merging it down to 'next' and so forth.

Thanks,
Taylor

Re: [PATCH 8/8] submodule--helper: use OPT_SUBCOMMAND() API

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 04 2022, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Thu, Nov 03 2022, Glen Choo wrote:
>>
>>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>> [...]
>>>> +	struct option options[] = {
>>>> +		OPT_SUBCOMMAND("clone", &fn, module_clone),
>>>> +		OPT_SUBCOMMAND("add", &fn, module_add),
>>>> +		OPT_SUBCOMMAND("update", &fn, module_update),
>>>> +		OPT_SUBCOMMAND("foreach", &fn, module_foreach),
>>>> +		OPT_SUBCOMMAND("init", &fn, module_init),
>>>> +		OPT_SUBCOMMAND("status", &fn, module_status),
>>>> +		OPT_SUBCOMMAND("sync", &fn, module_sync),
>>>> +		OPT_SUBCOMMAND("deinit", &fn, module_deinit),
>>>> +		OPT_SUBCOMMAND("summary", &fn, module_summary),
>>>> +		OPT_SUBCOMMAND("push-check", &fn, push_check),
>>>> +		OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs),
>>>> +		OPT_SUBCOMMAND("set-url", &fn, module_set_url),
>>>> +		OPT_SUBCOMMAND("set-branch", &fn, module_set_branch),
>>>> +		OPT_SUBCOMMAND("create-branch", &fn, module_create_branch),
>>>> +		OPT_END()
>>>> +	};
>>>> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
>>>> +	subcmd = argv[0];
>>>> +
>>>> +	if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") &&
>>>> +	    strcmp(subcmd, "foreach") && strcmp(subcmd, "status") &&
>>>> +	    strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") &&
>>>> +	    get_super_prefix())
>>>> +		/*
>>>> +		 * xstrfmt() rather than "%s %s" to keep the translated
>>>> +		 * string identical to git.c's.
>>>> +		 */
>>>> +		die(_("%s doesn't support --super-prefix"),
>>>> +		    xstrfmt("'%s %s'", cmd, subcmd));
>>>
>>> FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks
>>> in both the top level and in submodule--helper (i.e. revisiting my
>>> complaints from [1]), but I haven't sent it out yet because I haven't
>>> found the right way to motivate that change. Feel free to chime in on
>>> that series when it comes out.
>>>
>>> [1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/
>>
>> Maybe you have different plans, but keep at the WIP re-roll of what I
>> have after this, I've also removed all of that.
>>
>> Basically, ending up with:
>> 	
>> 	--- a/builtin.h
>> 	+++ b/builtin.h
>> 	@@ -224,7 +224,14 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix);
>> 	 int cmd_status(int argc, const char **argv, const char *prefix);
>> 	 int cmd_stash(int argc, const char **argv, const char *prefix);
>> 	 int cmd_stripspace(int argc, const char **argv, const char *prefix);
>> 	+int cmd_submodule(int argc, const char **argv, const char *prefix);
>> 	 int cmd_submodule__helper(int argc, const char **argv, const char *prefix);
>> 	+int cmd_submodule__helper_clone(int argc, const char **argv, const char *prefix);
>> 	+int cmd_submodule_absorbgitdirs(int argc, const char **argv, const char *prefix);
>> 	+int cmd_submodule_foreach(int argc, const char **argv, const char *prefix);
>> 	+int cmd_submodule_status(int argc, const char **argv, const char *prefix);
>> 	+int cmd_submodule_sync(int argc, const char **argv, const char *prefix);
>> 	+int cmd_submodule_update(int argc, const char **argv, const char *prefix);
>> 	 int cmd_switch(int argc, const char **argv, const char *prefix);
>> 	 int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
>> 	 int cmd_tag(int argc, const char **argv, const char *prefix);
>>
>> And changes like:
>> 	
>> 	@@ -366,7 +365,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
>> 	 
>> 	                strvec_pushl(&cpr.args, "--super-prefix", NULL);
>> 	                strvec_pushf(&cpr.args, "%s/", displaypath);
>> 	-               strvec_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive",
>> 	+               strvec_pushl(&cpr.args, "submodule--helper-foreach", "--recursive",
>> 	                             NULL);
>> 	 
>> 	                if (info->quiet)
>>
>> I.e. when we call "foreach" we dispatch to cmd_submodule_foreach(), but
>> when "foreach" needs to recurse it doesn't invoke a "git submodule
>> foreach", instead it invokes "git submodule--helper-foreach".
>>
>> The reason for doing that is that we can't promote the helper to a
>> built-in without also leaking implementation detail that we support the
>> now internal-only --super-prefix in the command itself.
>>
>> So this code becomes:
>> 	
>> 	@@ -3352,43 +3304,17 @@ static int module_add(int argc, const char **argv, const char *prefix)
>> 	 
>> 	 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>> 	 {
>> 	-       const char *cmd = argv[0];
>> 	-       const char *subcmd;
>> 	        parse_opt_subcommand_fn *fn = NULL;
>> 	        const char *const usage[] = {
>> 	                N_("git submodule--helper <command>"),
>> 	                NULL
>> 	        };
>> 	        struct option options[] = {
>> 	-               OPT_SUBCOMMAND("clone", &fn, module_clone),
>> 	-               OPT_SUBCOMMAND("add", &fn, module_add),
>> 	-               OPT_SUBCOMMAND("update", &fn, module_update),
>> 	-               OPT_SUBCOMMAND("foreach", &fn, module_foreach),
>> 	-               OPT_SUBCOMMAND("init", &fn, module_init),
>> 	-               OPT_SUBCOMMAND("status", &fn, module_status),
>> 	-               OPT_SUBCOMMAND("sync", &fn, module_sync),
>> 	-               OPT_SUBCOMMAND("deinit", &fn, module_deinit),
>> 	-               OPT_SUBCOMMAND("summary", &fn, module_summary),
>> 	-               OPT_SUBCOMMAND("push-check", &fn, push_check),
>> 	-               OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs),
>> 	-               OPT_SUBCOMMAND("set-url", &fn, module_set_url),
>> 	-               OPT_SUBCOMMAND("set-branch", &fn, module_set_branch),
>> 	-               OPT_SUBCOMMAND("create-branch", &fn, module_create_branch),
>> 	+               OPT_SUBCOMMAND("push-check", &fn, cmd_submodule_push_check),
>> 	+               OPT_SUBCOMMAND("create-branch", &fn, cmd_submodule_create_branch),
>> 	                OPT_END()
>> 	        };
>> 	        argc = parse_options(argc, argv, prefix, options, usage, 0);
>> 	-       subcmd = argv[0];
>> 	-
>> 	-       if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") &&
>> 	-           strcmp(subcmd, "foreach") && strcmp(subcmd, "status") &&
>> 	-           strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") &&
>> 	-           get_super_prefix())
>> 	-               /*
>> 	-                * xstrfmt() rather than "%s %s" to keep the translated
>> 	-                * string identical to git.c's.
>> 	-                */
>> 	-               die(_("%s doesn't support --super-prefix"),
>> 	-                   xstrfmt("'%s %s'", cmd, subcmd));
>> 	 
>> 	        return fn(argc, argv, prefix);
>> 	 }
>>
>> I.e. for all the "super-prefix" ones we dispatch directly (and maybe I
>> should just do that too for those two stragglers).
>>
>> It's ugly, but it's only ugly on the inside, but if you can think of a
>> better way to do it...
>
> If we _really_ wanted to keep the check, an alternative might be to
> teach the subcommand parser about SUPPORT_SUPER_PREFIX. But frankly, I
> think this SUPPORT_SUPER_PREFIX check is far more trouble than it's
> worth, and I wouldn't want you to spend your time trying to find ways to
> keep it only for it to be dropped altogether.
>
> Here's a snippet from the cover letter I'm working on, which will
> hopefully convince you that we don't need to worry about this any more.
>
> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
>
>   When we introduced the internal-only "--super-prefix" in 74866d7579 (git: make
>   super-prefix option, 2016-10-07), we specified that commands must prefix paths
>   by it, and pathspecs must be parsed relative to it. That commit also includes
>   safeguards to ensure that "--super-prefix" is used correctly, namely:
>
>   - Only commands marked SUPPORT_SUPER_PREFIX can be invoked with "--super-prefix".
>   - The super prefix is propagated via the GIT_INTERNAL_SUPER_PREFIX env var, so a
>     non-SUPPORT_SUPER_PREFIX command cannot be invoked by a SUPPORT_SUPER_PREFIX
>     one.
>
>   However, its use is inconsistent, which is a strong reason to consider using
>   better-scoped flags instead. For example..
>
>     - Of the 4 commands that are SUPPORT_SUPER_PREFIX, only "read-tree" and
>       "submodule--helper" do anything useful with it. "fsmonitor--daemon" has it
>       to avoid the SUPPORT_SUPER_PREFIX warning [1]. "checkout--worker" doesn't have
>       a documented reason, but since it can be invoked by "read-tree", it's
>       presumably also a workaround.
>     - "read-tree" and "submodule--helper" use different values for the super prefix;
>       "read-tree" passes the path from the root of the superproject's tree to the
>       submodule's gitlink, while "submodule--helper" passes the relative path of the
>       original CWD to the submodule.
>     - "submodule--helper" doesn't use "--super-prefix" to parse pathspecs, only to
>       display paths.
>
>   This series fixes replaces "--super-prefix" with such better-scoped flags, and
>   fixes some bugs resulting from the SUPPORT_SUPER_PREFIX check.
>
> [1] 53fcfbc84f (fsmonitor--daemon: allow --super-prefix argument, 2022-05-26)
>
> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
>
> I figured out all the details, but it would look something like:
>
> - Add an internal-only "--tree-super-prefix" flag to "git read-tree",
>   which sets a global variable that is read from unpack-trees.c.
> - Add an internal-only "--wt-super-prefix" flag to "git
>   submodule--helper", which sets a global variable that is read from
>   submodule.c. Unlike "--super-prefix", this won't be gated behind a
>   SUPPORT_SUPER_PREFIX for each subcommand, since AFAICT, every
>   subcommand is using this "--wt-super-prefix" correctly, so we can just
>   make sure that new subcommands do too.
> - Remove the global "--super-prefix", the corresponding env var and
>   SUPPORT_SUPER_PREFIX.

Yes, I'm very much on the same page with removing it eventully. But as
you note above that'll take some doing.

So for the "submodule as a built-in" I didn't want to take it hostage to
that, but just to ensure that I:

 * Got it working
 * Did not implicitly leak "this needs a --super-prefix" from the
   current set of commands to any other commands, or to e.g. public
   "submodule foreach" just because we need it for the internal-only
   helper.

Are you proposing that you'll submit another clean-up topic that the
submodule-as-a-builtin will need to be queued on top of, in addition to
this topic?

If so I'd really prefer we defer those cleanups, which should also be
much easier once submodule's fully in C, unless they're much more
trivial than I'm currently suspecting they are...

Re: [PATCH 8/8] submodule--helper: use OPT_SUBCOMMAND() API

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Fri, Nov 04 2022, Glen Choo wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> On Thu, Nov 03 2022, Glen Choo wrote:
>>>
>>>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>>> [...]
>>>>> +	struct option options[] = {
>>>>> +		OPT_SUBCOMMAND("clone", &fn, module_clone),
>>>>> +		OPT_SUBCOMMAND("add", &fn, module_add),
>>>>> +		OPT_SUBCOMMAND("update", &fn, module_update),
>>>>> +		OPT_SUBCOMMAND("foreach", &fn, module_foreach),
>>>>> +		OPT_SUBCOMMAND("init", &fn, module_init),
>>>>> +		OPT_SUBCOMMAND("status", &fn, module_status),
>>>>> +		OPT_SUBCOMMAND("sync", &fn, module_sync),
>>>>> +		OPT_SUBCOMMAND("deinit", &fn, module_deinit),
>>>>> +		OPT_SUBCOMMAND("summary", &fn, module_summary),
>>>>> +		OPT_SUBCOMMAND("push-check", &fn, push_check),
>>>>> +		OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs),
>>>>> +		OPT_SUBCOMMAND("set-url", &fn, module_set_url),
>>>>> +		OPT_SUBCOMMAND("set-branch", &fn, module_set_branch),
>>>>> +		OPT_SUBCOMMAND("create-branch", &fn, module_create_branch),
>>>>> +		OPT_END()
>>>>> +	};
>>>>> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
>>>>> +	subcmd = argv[0];
>>>>> +
>>>>> +	if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") &&
>>>>> +	    strcmp(subcmd, "foreach") && strcmp(subcmd, "status") &&
>>>>> +	    strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") &&
>>>>> +	    get_super_prefix())
>>>>> +		/*
>>>>> +		 * xstrfmt() rather than "%s %s" to keep the translated
>>>>> +		 * string identical to git.c's.
>>>>> +		 */
>>>>> +		die(_("%s doesn't support --super-prefix"),
>>>>> +		    xstrfmt("'%s %s'", cmd, subcmd));
>>>>
>>>> FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks
>>>> in both the top level and in submodule--helper (i.e. revisiting my
>>>> complaints from [1]), but I haven't sent it out yet because I haven't
>>>> found the right way to motivate that change. Feel free to chime in on
>>>> that series when it comes out.
>>>>
>>>> [1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/
>>>
>>> Maybe you have different plans, but keep at the WIP re-roll of what I
>>> have after this, I've also removed all of that.
>>>
>>> Basically, ending up with:
>>> 	
>>> 	--- a/builtin.h
>>> 	+++ b/builtin.h
>>> 	@@ -224,7 +224,14 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix);
>>> 	 int cmd_status(int argc, const char **argv, const char *prefix);
>>> 	 int cmd_stash(int argc, const char **argv, const char *prefix);
>>> 	 int cmd_stripspace(int argc, const char **argv, const char *prefix);
>>> 	+int cmd_submodule(int argc, const char **argv, const char *prefix);
>>> 	 int cmd_submodule__helper(int argc, const char **argv, const char *prefix);
>>> 	+int cmd_submodule__helper_clone(int argc, const char **argv, const char *prefix);
>>> 	+int cmd_submodule_absorbgitdirs(int argc, const char **argv, const char *prefix);
>>> 	+int cmd_submodule_foreach(int argc, const char **argv, const char *prefix);
>>> 	+int cmd_submodule_status(int argc, const char **argv, const char *prefix);
>>> 	+int cmd_submodule_sync(int argc, const char **argv, const char *prefix);
>>> 	+int cmd_submodule_update(int argc, const char **argv, const char *prefix);
>>> 	 int cmd_switch(int argc, const char **argv, const char *prefix);
>>> 	 int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
>>> 	 int cmd_tag(int argc, const char **argv, const char *prefix);
>>>
>>> And changes like:
>>> 	
>>> 	@@ -366,7 +365,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
>>> 	 
>>> 	                strvec_pushl(&cpr.args, "--super-prefix", NULL);
>>> 	                strvec_pushf(&cpr.args, "%s/", displaypath);
>>> 	-               strvec_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive",
>>> 	+               strvec_pushl(&cpr.args, "submodule--helper-foreach", "--recursive",
>>> 	                             NULL);
>>> 	 
>>> 	                if (info->quiet)
>>>
>>> I.e. when we call "foreach" we dispatch to cmd_submodule_foreach(), but
>>> when "foreach" needs to recurse it doesn't invoke a "git submodule
>>> foreach", instead it invokes "git submodule--helper-foreach".
>>>
>>> The reason for doing that is that we can't promote the helper to a
>>> built-in without also leaking implementation detail that we support the
>>> now internal-only --super-prefix in the command itself.
>>>
>>> So this code becomes:
>>> 	
>>> 	@@ -3352,43 +3304,17 @@ static int module_add(int argc, const char **argv, const char *prefix)
>>> 	 
>>> 	 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>>> 	 {
>>> 	-       const char *cmd = argv[0];
>>> 	-       const char *subcmd;
>>> 	        parse_opt_subcommand_fn *fn = NULL;
>>> 	        const char *const usage[] = {
>>> 	                N_("git submodule--helper <command>"),
>>> 	                NULL
>>> 	        };
>>> 	        struct option options[] = {
>>> 	-               OPT_SUBCOMMAND("clone", &fn, module_clone),
>>> 	-               OPT_SUBCOMMAND("add", &fn, module_add),
>>> 	-               OPT_SUBCOMMAND("update", &fn, module_update),
>>> 	-               OPT_SUBCOMMAND("foreach", &fn, module_foreach),
>>> 	-               OPT_SUBCOMMAND("init", &fn, module_init),
>>> 	-               OPT_SUBCOMMAND("status", &fn, module_status),
>>> 	-               OPT_SUBCOMMAND("sync", &fn, module_sync),
>>> 	-               OPT_SUBCOMMAND("deinit", &fn, module_deinit),
>>> 	-               OPT_SUBCOMMAND("summary", &fn, module_summary),
>>> 	-               OPT_SUBCOMMAND("push-check", &fn, push_check),
>>> 	-               OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs),
>>> 	-               OPT_SUBCOMMAND("set-url", &fn, module_set_url),
>>> 	-               OPT_SUBCOMMAND("set-branch", &fn, module_set_branch),
>>> 	-               OPT_SUBCOMMAND("create-branch", &fn, module_create_branch),
>>> 	+               OPT_SUBCOMMAND("push-check", &fn, cmd_submodule_push_check),
>>> 	+               OPT_SUBCOMMAND("create-branch", &fn, cmd_submodule_create_branch),
>>> 	                OPT_END()
>>> 	        };
>>> 	        argc = parse_options(argc, argv, prefix, options, usage, 0);
>>> 	-       subcmd = argv[0];
>>> 	-
>>> 	-       if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") &&
>>> 	-           strcmp(subcmd, "foreach") && strcmp(subcmd, "status") &&
>>> 	-           strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") &&
>>> 	-           get_super_prefix())
>>> 	-               /*
>>> 	-                * xstrfmt() rather than "%s %s" to keep the translated
>>> 	-                * string identical to git.c's.
>>> 	-                */
>>> 	-               die(_("%s doesn't support --super-prefix"),
>>> 	-                   xstrfmt("'%s %s'", cmd, subcmd));
>>> 	 
>>> 	        return fn(argc, argv, prefix);
>>> 	 }
>>>
>>> I.e. for all the "super-prefix" ones we dispatch directly (and maybe I
>>> should just do that too for those two stragglers).
>>>
>>> It's ugly, but it's only ugly on the inside, but if you can think of a
>>> better way to do it...
>>
>> If we _really_ wanted to keep the check, an alternative might be to
>> teach the subcommand parser about SUPPORT_SUPER_PREFIX. But frankly, I
>> think this SUPPORT_SUPER_PREFIX check is far more trouble than it's
>> worth, and I wouldn't want you to spend your time trying to find ways to
>> keep it only for it to be dropped altogether.
>>
>> Here's a snippet from the cover letter I'm working on, which will
>> hopefully convince you that we don't need to worry about this any more.
>>
>> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
>>
>>   When we introduced the internal-only "--super-prefix" in 74866d7579 (git: make
>>   super-prefix option, 2016-10-07), we specified that commands must prefix paths
>>   by it, and pathspecs must be parsed relative to it. That commit also includes
>>   safeguards to ensure that "--super-prefix" is used correctly, namely:
>>
>>   - Only commands marked SUPPORT_SUPER_PREFIX can be invoked with "--super-prefix".
>>   - The super prefix is propagated via the GIT_INTERNAL_SUPER_PREFIX env var, so a
>>     non-SUPPORT_SUPER_PREFIX command cannot be invoked by a SUPPORT_SUPER_PREFIX
>>     one.
>>
>>   However, its use is inconsistent, which is a strong reason to consider using
>>   better-scoped flags instead. For example..
>>
>>     - Of the 4 commands that are SUPPORT_SUPER_PREFIX, only "read-tree" and
>>       "submodule--helper" do anything useful with it. "fsmonitor--daemon" has it
>>       to avoid the SUPPORT_SUPER_PREFIX warning [1]. "checkout--worker" doesn't have
>>       a documented reason, but since it can be invoked by "read-tree", it's
>>       presumably also a workaround.
>>     - "read-tree" and "submodule--helper" use different values for the super prefix;
>>       "read-tree" passes the path from the root of the superproject's tree to the
>>       submodule's gitlink, while "submodule--helper" passes the relative path of the
>>       original CWD to the submodule.
>>     - "submodule--helper" doesn't use "--super-prefix" to parse pathspecs, only to
>>       display paths.
>>
>>   This series fixes replaces "--super-prefix" with such better-scoped flags, and
>>   fixes some bugs resulting from the SUPPORT_SUPER_PREFIX check.
>>
>> [1] 53fcfbc84f (fsmonitor--daemon: allow --super-prefix argument, 2022-05-26)
>>
>> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
>>
>> I figured out all the details, but it would look something like:
>>
>> - Add an internal-only "--tree-super-prefix" flag to "git read-tree",
>>   which sets a global variable that is read from unpack-trees.c.
>> - Add an internal-only "--wt-super-prefix" flag to "git
>>   submodule--helper", which sets a global variable that is read from
>>   submodule.c. Unlike "--super-prefix", this won't be gated behind a
>>   SUPPORT_SUPER_PREFIX for each subcommand, since AFAICT, every
>>   subcommand is using this "--wt-super-prefix" correctly, so we can just
>>   make sure that new subcommands do too.
>> - Remove the global "--super-prefix", the corresponding env var and
>>   SUPPORT_SUPER_PREFIX.
> So for the "submodule as a built-in" I didn't want to take it hostage to
> that

That's a reasonable concern.

> Are you proposing that you'll submit another clean-up topic that the
> submodule-as-a-builtin will need to be queued on top of, in addition to
> this topic?
>
> If so I'd really prefer we defer those cleanups, which should also be
> much easier once submodule's fully in C, unless they're much more
> trivial than I'm currently suspecting they are...

Yes, that is what I'm proposing (unfortunately). I don't like having
either of us blocked on the other's work, but it seems like it really
does work out better this way, e.g. this series makes it much easier to
replace "git --super-prefix" with "git submodule--helper
--something-else" because we now use the options parser, and removing
the "--super-prefix" check makes the eventual conversion simpler because
that's one less behavior to worry about keeping.

We can see if my proposal makes sense when I send something out
(possibly as RFC). I'll do that today/tomorrow.

[PATCH v2 0/9] submodule: tests, cleanup to prepare for built-in

[Ævar Arnfjörð Bjarmason]

As noted in the v1[1] this is a prep series for getting us to a
built-in submodule.c, with a "git rm git-submodule.sh". As noted in
the v1 discussion (e.g. Taylor's [2]) this was mostly ready, but just
needed some final finishing touches.

Changes since v1:

* Noted the history of when "submodule--helper config" was last used,
  per Glen's comment.
* Re-titled the tests htat no longer use the now-removed
  "submodule--helper config", per Glen's comment.
* I removed _() from a string in what's left of "submodule--helper
  config", which is now a test helper. We should not be translating
  test helper strings.
* Fixed a buggy for-loop in the t7422-submodule-output.sh test I'm
  adding.
* Fixed a trivial typo in a comment in that test & in another nearby
  comment.

Passing CI run & branch for this available at[3].

1. https://lore.kernel.org/git/cover-0.8-00000000000-20221102T074148Z-avarab@gmail.com/
2. https://lore.kernel.org/git/Y2Vi95r+RqHPwbw8@nand.local/
3. https://github.com/avar/git/tree/avar/submodule-builtin-final-prep-2

Ævar Arnfjörð Bjarmason (9):
  submodule--helper: move "config" to a test-tool
  submodule tests: add tests for top-level flag output
  submodule--helper: fix  a memory leak in "status"
  submodule tests: test for a "foreach" blind-spot
  submodule.c: refactor recursive block out of absorb function
  submodule API & "absorbgitdirs": remove "----recursive" option
  submodule--helper: remove --prefix from "absorbgitdirs"
  submodule--helper: drop "update --prefix <pfx>" for "-C <pfx> update"
  submodule--helper: use OPT_SUBCOMMAND() API

 builtin/rm.c                           |   3 +-
 builtin/submodule--helper.c            | 146 +++++++--------------
 git-submodule.sh                       |   3 +-
 git.c                                  |   2 +-
 submodule.c                            |  41 +++---
 submodule.h                            |   4 +-
 t/helper/test-submodule.c              |  84 ++++++++++++
 t/t7400-submodule-basic.sh             |  10 ++
 t/t7407-submodule-foreach.sh           |   5 +
 t/t7411-submodule-config.sh            |  36 +++---
 t/t7418-submodule-sparse-gitmodules.sh |   4 +-
 t/t7422-submodule-output.sh            | 170 +++++++++++++++++++++++++
 12 files changed, 358 insertions(+), 150 deletions(-)
 create mode 100755 t/t7422-submodule-output.sh

Range-diff against v1:
 1:  b2649f96715 !  1:  ca8f8b4bf63 submodule--helper: move "config" to a test-tool
    @@ Commit message
         'ab/submodule-helper-prep', 2022-09-13) the "config" sub-command was
         only used by our own tests.
     
    +    It was last used by "git submodule" itself in code that went away with
    +    a6226fd772b (submodule--helper: convert the bulk of cmd_add() to C,
    +    2021-08-10).
    +
         Let's move it over, and while doing so make it easier to reason about
         by splitting up the various uses for it into separate sub-commands, so
         that we don't need to count arguments to see what it does.
     
    +    This also has the advantage that we stop wasting future translator
    +    time on this command, currently the usage information for this
    +    internal-only tool has been translated into several languages. The use
    +    of the "_" function has also been removed from the "please make
    +    sure..." message.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/submodule--helper.c ##
    @@ t/helper/test-submodule.c: static int cmd__submodule_resolve_relative_url(int ar
     +	/* Equivalent to ACTION_SET in builtin/config.c */
     +	if (argc == 3) {
     +		if (!is_writing_gitmodules_ok())
    -+			die(_("please make sure that the .gitmodules file is in the working tree"));
    ++			die("please make sure that the .gitmodules file is in the working tree");
     +
     +		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
     +	}
    @@ t/helper/test-submodule.c: static int cmd__submodule_resolve_relative_url(int ar
     +
     +	if (argc == 2) {
     +		if (!is_writing_gitmodules_ok())
    -+			die(_("please make sure that the .gitmodules file is in the working tree"));
    ++			die("please make sure that the .gitmodules file is in the working tree");
     +		return config_set_in_gitmodules_file_gently(argv[1], NULL);
     +	}
     +	usage_with_options(usage, options);
    @@ t/helper/test-submodule.c: static int cmd__submodule_resolve_relative_url(int ar
     
      ## t/t7411-submodule-config.sh ##
     @@ t/t7411-submodule-config.sh: test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
    - test_expect_success 'reading submodules config from the working tree with "submodule--helper config"' '
    + 	)
    + '
    + 
    +-test_expect_success 'reading submodules config from the working tree with "submodule--helper config"' '
    ++test_expect_success 'reading submodules config from the working tree' '
      	(cd super &&
      		echo "../submodule" >expect &&
     -		git submodule--helper config submodule.submodule.url >actual &&
    @@ t/t7411-submodule-config.sh: test_expect_success 'error in history in fetchrecur
      	)
      '
      
    - test_expect_success 'unsetting submodules config from the working tree with "submodule--helper config --unset"' '
    +-test_expect_success 'unsetting submodules config from the working tree with "submodule--helper config --unset"' '
    ++test_expect_success 'unsetting submodules config from the working tree' '
      	(cd super &&
     -		git submodule--helper config --unset submodule.submodule.url &&
     -		git submodule--helper config submodule.submodule.url >actual &&
    @@ t/t7411-submodule-config.sh: test_expect_success 'error in history in fetchrecur
      		test_must_be_empty actual
      	)
      '
    -@@ t/t7411-submodule-config.sh: test_expect_success 'unsetting submodules config from the working tree with "sub
    - test_expect_success 'writing submodules config with "submodule--helper config"' '
    + 
    + 
    +-test_expect_success 'writing submodules config with "submodule--helper config"' '
    ++test_expect_success 'writing submodules config' '
      	(cd super &&
      		echo "new_url" >expect &&
     -		git submodule--helper config submodule.submodule.url "new_url" &&
    @@ t/t7411-submodule-config.sh: test_expect_success 'unsetting submodules config fr
      		test_cmp expect actual
      	)
      '
    -@@ t/t7411-submodule-config.sh: test_expect_success 'overwriting unstaged submodules config with "submodule--hel
    + 
    +-test_expect_success 'overwriting unstaged submodules config with "submodule--helper config"' '
    ++test_expect_success 'overwriting unstaged submodules config' '
      	test_when_finished "git -C super checkout .gitmodules" &&
      	(cd super &&
      		echo "newer_url" >expect &&
 2:  cda36b5b6e0 !  2:  5508c27f653 submodule tests: add tests for top-level flag output
    @@ t/t7422-submodule-output.sh (new)
     +	git -C X pull &&
     +	GIT_ALLOW_PROTOCOL=file git -C X submodule update --init &&
     +
    -+	# dirty p
    ++	# dirty
     +	for d in S.D X/S.D
     +	do
    -+		echo dirty >S.D/A.t || return 1
    ++		echo dirty >"$d"/A.t || return 1
     +	done &&
     +
     +	# commit (for --cached)
    @@ t/t7422-submodule-output.sh (new)
     +
     +	for ref in A B C
     +	do
    -+		# Not different with SHA-1 and SHA-256, just (ab)usign
    ++		# Not different with SHA-1 and SHA-256, just (ab)using
     +		# test_oid_cache as a variable bag to avoid using
     +		# $(git rev-parse ...).
     +		oid=$(git rev-parse $ref) &&
 -:  ----------- >  3:  a3529d7f9e0 submodule--helper: fix  a memory leak in "status"
 3:  0ed1fc7fdf8 =  4:  c14cc0e14b8 submodule tests: test for a "foreach" blind-spot
 4:  f7adfbc13ae =  5:  459ea25125b submodule.c: refactor recursive block out of absorb function
 5:  2b8afd73b9b =  6:  322a02c30fc submodule API & "absorbgitdirs": remove "----recursive" option
 6:  91208241070 =  7:  d1f4ac20a4f submodule--helper: remove --prefix from "absorbgitdirs"
 7:  77d4d5a6c09 =  8:  ac9ff05ef68 submodule--helper: drop "update --prefix <pfx>" for "-C <pfx> update"
 8:  105853cd358 =  9:  cccd92a829c submodule--helper: use OPT_SUBCOMMAND() API
-- 
2.38.0.1464.gea6794aacbc

[PATCH v2 1/9] submodule--helper: move "config" to a test-tool

[Ævar Arnfjörð Bjarmason]

As with other moves to "test-tool" in f322e9f51b5 (Merge branch
'ab/submodule-helper-prep', 2022-09-13) the "config" sub-command was
only used by our own tests.

It was last used by "git submodule" itself in code that went away with
a6226fd772b (submodule--helper: convert the bulk of cmd_add() to C,
2021-08-10).

Let's move it over, and while doing so make it easier to reason about
by splitting up the various uses for it into separate sub-commands, so
that we don't need to count arguments to see what it does.

This also has the advantage that we stop wasting future translator
time on this command, currently the usage information for this
internal-only tool has been translated into several languages. The use
of the "_" function has also been removed from the "please make
sure..." message.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/submodule--helper.c            | 46 --------------
 t/helper/test-submodule.c              | 84 ++++++++++++++++++++++++++
 t/t7411-submodule-config.sh            | 36 +++++------
 t/t7418-submodule-sparse-gitmodules.sh |  4 +-
 4 files changed, 104 insertions(+), 66 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a7683d35299..6250b95a6f7 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2861,51 +2861,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
-static int module_config(int argc, const char **argv, const char *prefix)
-{
-	enum {
-		CHECK_WRITEABLE = 1,
-		DO_UNSET = 2
-	} command = 0;
-	struct option module_config_options[] = {
-		OPT_CMDMODE(0, "check-writeable", &command,
-			    N_("check if it is safe to write to the .gitmodules file"),
-			    CHECK_WRITEABLE),
-		OPT_CMDMODE(0, "unset", &command,
-			    N_("unset the config in the .gitmodules file"),
-			    DO_UNSET),
-		OPT_END()
-	};
-	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper config <name> [<value>]"),
-		N_("git submodule--helper config --unset <name>"),
-		"git submodule--helper config --check-writeable",
-		NULL
-	};
-
-	argc = parse_options(argc, argv, prefix, module_config_options,
-			     git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
-
-	if (argc == 1 && command == CHECK_WRITEABLE)
-		return is_writing_gitmodules_ok() ? 0 : -1;
-
-	/* Equivalent to ACTION_GET in builtin/config.c */
-	if (argc == 2 && command != DO_UNSET)
-		return print_config_from_gitmodules(the_repository, argv[1]);
-
-	/* Equivalent to ACTION_SET in builtin/config.c */
-	if (argc == 3 || (argc == 2 && command == DO_UNSET)) {
-		const char *value = (argc == 3) ? argv[2] : NULL;
-
-		if (!is_writing_gitmodules_ok())
-			die(_("please make sure that the .gitmodules file is in the working tree"));
-
-		return config_set_in_gitmodules_file_gently(argv[1], value);
-	}
-
-	usage_with_options(git_submodule_helper_usage, module_config_options);
-}
-
 static int module_set_url(int argc, const char **argv, const char *prefix)
 {
 	int quiet = 0;
@@ -3424,7 +3379,6 @@ static struct cmd_struct commands[] = {
 	{"summary", module_summary, 0},
 	{"push-check", push_check, 0},
 	{"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
-	{"config", module_config, 0},
 	{"set-url", module_set_url, 0},
 	{"set-branch", module_set_branch, 0},
 	{"create-branch", module_create_branch, 0},
diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c
index b7d117cd557..e060cc62268 100644
--- a/t/helper/test-submodule.c
+++ b/t/helper/test-submodule.c
@@ -111,10 +111,94 @@ static int cmd__submodule_resolve_relative_url(int argc, const char **argv)
 	return 0;
 }
 
+static int cmd__submodule_config_list(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+	const char *const usage[] = {
+		"test-tool submodule config-list <key>",
+		NULL
+	};
+	argc = parse_options(argc, argv, "test-tools", options, usage,
+			     PARSE_OPT_KEEP_ARGV0);
+
+	setup_git_directory();
+
+	if (argc == 2)
+		return print_config_from_gitmodules(the_repository, argv[1]);
+	usage_with_options(usage, options);
+}
+
+static int cmd__submodule_config_set(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+	const char *const usage[] = {
+		"test-tool submodule config-set <key> <value>",
+		NULL
+	};
+	argc = parse_options(argc, argv, "test-tools", options, usage,
+			     PARSE_OPT_KEEP_ARGV0);
+
+	setup_git_directory();
+
+	/* Equivalent to ACTION_SET in builtin/config.c */
+	if (argc == 3) {
+		if (!is_writing_gitmodules_ok())
+			die("please make sure that the .gitmodules file is in the working tree");
+
+		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+	}
+	usage_with_options(usage, options);
+}
+
+static int cmd__submodule_config_unset(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+	const char *const usage[] = {
+		"test-tool submodule config-unset <key>",
+		NULL
+	};
+
+	setup_git_directory();
+
+	if (argc == 2) {
+		if (!is_writing_gitmodules_ok())
+			die("please make sure that the .gitmodules file is in the working tree");
+		return config_set_in_gitmodules_file_gently(argv[1], NULL);
+	}
+	usage_with_options(usage, options);
+}
+
+static int cmd__submodule_config_writeable(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+	const char *const usage[] = {
+		"test-tool submodule config-writeable",
+		NULL
+	};
+	setup_git_directory();
+
+	if (argc == 1)
+		return is_writing_gitmodules_ok() ? 0 : -1;
+
+	usage_with_options(usage, options);
+}
+
 static struct test_cmd cmds[] = {
 	{ "check-name", cmd__submodule_check_name },
 	{ "is-active", cmd__submodule_is_active },
 	{ "resolve-relative-url", cmd__submodule_resolve_relative_url},
+	{ "config-list", cmd__submodule_config_list },
+	{ "config-set", cmd__submodule_config_set },
+	{ "config-unset", cmd__submodule_config_unset },
+	{ "config-writeable", cmd__submodule_config_writeable },
 };
 
 int cmd__submodule(int argc, const char **argv)
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index c583c4e373a..c0167944abd 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -137,44 +137,44 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 	)
 '
 
-test_expect_success 'reading submodules config from the working tree with "submodule--helper config"' '
+test_expect_success 'reading submodules config from the working tree' '
 	(cd super &&
 		echo "../submodule" >expect &&
-		git submodule--helper config submodule.submodule.url >actual &&
+		test-tool submodule config-list submodule.submodule.url >actual &&
 		test_cmp expect actual
 	)
 '
 
-test_expect_success 'unsetting submodules config from the working tree with "submodule--helper config --unset"' '
+test_expect_success 'unsetting submodules config from the working tree' '
 	(cd super &&
-		git submodule--helper config --unset submodule.submodule.url &&
-		git submodule--helper config submodule.submodule.url >actual &&
+		test-tool submodule config-unset submodule.submodule.url &&
+		test-tool submodule config-list submodule.submodule.url >actual &&
 		test_must_be_empty actual
 	)
 '
 
 
-test_expect_success 'writing submodules config with "submodule--helper config"' '
+test_expect_success 'writing submodules config' '
 	(cd super &&
 		echo "new_url" >expect &&
-		git submodule--helper config submodule.submodule.url "new_url" &&
-		git submodule--helper config submodule.submodule.url >actual &&
+		test-tool submodule config-set submodule.submodule.url "new_url" &&
+		test-tool submodule config-list submodule.submodule.url >actual &&
 		test_cmp expect actual
 	)
 '
 
-test_expect_success 'overwriting unstaged submodules config with "submodule--helper config"' '
+test_expect_success 'overwriting unstaged submodules config' '
 	test_when_finished "git -C super checkout .gitmodules" &&
 	(cd super &&
 		echo "newer_url" >expect &&
-		git submodule--helper config submodule.submodule.url "newer_url" &&
-		git submodule--helper config submodule.submodule.url >actual &&
+		test-tool submodule config-set submodule.submodule.url "newer_url" &&
+		test-tool submodule config-list submodule.submodule.url >actual &&
 		test_cmp expect actual
 	)
 '
 
 test_expect_success 'writeable .gitmodules when it is in the working tree' '
-	git -C super submodule--helper config --check-writeable
+	test-tool -C super submodule config-writeable
 '
 
 test_expect_success 'writeable .gitmodules when it is nowhere in the repository' '
@@ -183,7 +183,7 @@ test_expect_success 'writeable .gitmodules when it is nowhere in the repository'
 	(cd super &&
 		git rm .gitmodules &&
 		git commit -m "remove .gitmodules from the current branch" &&
-		git submodule--helper config --check-writeable
+		test-tool submodule config-writeable
 	)
 '
 
@@ -191,7 +191,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the index but not i
 	test_when_finished "git -C super checkout .gitmodules" &&
 	(cd super &&
 		rm -f .gitmodules &&
-		test_must_fail git submodule--helper config --check-writeable
+		test_must_fail test-tool submodule config-writeable
 	)
 '
 
@@ -200,7 +200,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the current branch
 	test_when_finished "git -C super reset --hard $ORIG" &&
 	(cd super &&
 		git rm .gitmodules &&
-		test_must_fail git submodule--helper config --check-writeable
+		test_must_fail test-tool submodule config-writeable
 	)
 '
 
@@ -208,11 +208,11 @@ test_expect_success 'reading submodules config from the index when .gitmodules i
 	ORIG=$(git -C super rev-parse HEAD) &&
 	test_when_finished "git -C super reset --hard $ORIG" &&
 	(cd super &&
-		git submodule--helper config submodule.submodule.url "staged_url" &&
+		test-tool submodule config-set submodule.submodule.url "staged_url" &&
 		git add .gitmodules &&
 		rm -f .gitmodules &&
 		echo "staged_url" >expect &&
-		git submodule--helper config submodule.submodule.url >actual &&
+		test-tool submodule config-list submodule.submodule.url >actual &&
 		test_cmp expect actual
 	)
 '
@@ -223,7 +223,7 @@ test_expect_success 'reading submodules config from the current branch when .git
 	(cd super &&
 		git rm .gitmodules &&
 		echo "../submodule" >expect &&
-		git submodule--helper config submodule.submodule.url >actual &&
+		test-tool submodule config-list submodule.submodule.url >actual &&
 		test_cmp expect actual
 	)
 '
diff --git a/t/t7418-submodule-sparse-gitmodules.sh b/t/t7418-submodule-sparse-gitmodules.sh
index d5874200fdc..dde11ecce80 100755
--- a/t/t7418-submodule-sparse-gitmodules.sh
+++ b/t/t7418-submodule-sparse-gitmodules.sh
@@ -50,12 +50,12 @@ test_expect_success 'sparse checkout setup which hides .gitmodules' '
 
 test_expect_success 'reading gitmodules config file when it is not checked out' '
 	echo "../submodule" >expect &&
-	git -C super submodule--helper config submodule.submodule.url >actual &&
+	test-tool -C super submodule config-list submodule.submodule.url >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'not writing gitmodules config file when it is not checked out' '
-	test_must_fail git -C super submodule--helper config submodule.submodule.url newurl &&
+	test_must_fail test-tool -C super submodule config-set submodule.submodule.url newurl &&
 	test_path_is_missing super/.gitmodules
 '
 
-- 
2.38.0.1464.gea6794aacbc

[PATCH v2 2/9] submodule tests: add tests for top-level flag output

[Ævar Arnfjörð Bjarmason]

Exhaustively test for how combining various "mixed-level" "git
submodule" option works. "Mixed-level" here means options that are
accepted by a mixture of the top-level "submodule" command, and
e.g. the "status" sub-command.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7400-submodule-basic.sh  |  10 +++
 t/t7422-submodule-output.sh | 169 ++++++++++++++++++++++++++++++++++++
 2 files changed, 179 insertions(+)
 create mode 100755 t/t7422-submodule-output.sh

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index a989aafaf57..eae6a46ef3d 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -579,6 +579,16 @@ test_expect_success 'status should be "modified" after submodule commit' '
 	grep "^+$rev2" list
 '
 
+test_expect_success '"submodule --cached" command forms should be identical' '
+	git submodule status --cached >expect &&
+
+	git submodule --cached >actual &&
+	test_cmp expect actual &&
+
+	git submodule --cached status >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'the --cached sha1 should be rev1' '
 	git submodule --cached status >list &&
 	grep "^+$rev1" list
diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh
new file mode 100755
index 00000000000..1e9cdf1a68e
--- /dev/null
+++ b/t/t7422-submodule-output.sh
@@ -0,0 +1,169 @@
+#!/bin/sh
+
+test_description='submodule --cached, --quiet etc. output'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-t3100.sh
+
+setup_sub () {
+	local d="$1" &&
+	shift &&
+	git $@ clone . "$d" &&
+	git $@ submodule add ./"$d"
+}
+
+normalize_status () {
+	sed -e 's/-g[0-9a-f]*/-gHASH/'
+}
+
+test_expect_success 'setup' '
+	test_commit A &&
+	test_commit B &&
+	setup_sub S  &&
+	setup_sub S.D &&
+	setup_sub S.C &&
+	setup_sub S.C.D &&
+	setup_sub X &&
+	git add S* &&
+	test_commit C &&
+
+	# recursive in X/
+	git -C X pull &&
+	GIT_ALLOW_PROTOCOL=file git -C X submodule update --init &&
+
+	# dirty
+	for d in S.D X/S.D
+	do
+		echo dirty >"$d"/A.t || return 1
+	done &&
+
+	# commit (for --cached)
+	for d in S.C* X/S.C*
+	do
+		git -C "$d" reset --hard A || return 1
+	done &&
+
+	# dirty
+	for d in S*.D X/S*.D
+	do
+		echo dirty >"$d/C2.t" || return 1
+	done &&
+
+	for ref in A B C
+	do
+		# Not different with SHA-1 and SHA-256, just (ab)using
+		# test_oid_cache as a variable bag to avoid using
+		# $(git rev-parse ...).
+		oid=$(git rev-parse $ref) &&
+		test_oid_cache <<-EOF || return 1
+		$ref sha1:$oid
+		$ref sha256:$oid
+		EOF
+	done
+'
+
+for opts in "" "status"
+do
+	test_expect_success "git submodule $opts" '
+		sed -e "s/^>//" >expect <<-EOF &&
+		> $(test_oid B) S (B)
+		>+$(test_oid A) S.C (A)
+		>+$(test_oid A) S.C.D (A)
+		> $(test_oid B) S.D (B)
+		>+$(test_oid C) X (C)
+		EOF
+		git submodule $opts >actual.raw &&
+		normalize_status <actual.raw >actual &&
+		test_cmp expect actual
+	'
+done
+
+for opts in \
+	"status --recursive"
+do
+	test_expect_success "git submodule $opts" '
+		sed -e "s/^>//" >expect <<-EOF &&
+		> $(test_oid B) S (B)
+		>+$(test_oid A) S.C (A)
+		>+$(test_oid A) S.C.D (A)
+		> $(test_oid B) S.D (B)
+		>+$(test_oid C) X (C)
+		> $(test_oid B) X/S (B)
+		>+$(test_oid A) X/S.C (A)
+		>+$(test_oid A) X/S.C.D (A)
+		> $(test_oid B) X/S.D (B)
+		> $(test_oid B) X/X (B)
+		EOF
+		git submodule $opts >actual.raw &&
+		normalize_status <actual.raw >actual &&
+		test_cmp expect actual
+	'
+done
+
+for opts in \
+	"--quiet" \
+	"--quiet status" \
+	"status --quiet"
+do
+	test_expect_success "git submodule $opts" '
+		git submodule $opts >out &&
+		test_must_be_empty out
+	'
+done
+
+for opts in \
+	"--cached" \
+	"--cached status" \
+	"status --cached"
+do
+	test_expect_success "git submodule $opts" '
+		sed -e "s/^>//" >expect <<-EOF &&
+		> $(test_oid B) S (B)
+		>+$(test_oid B) S.C (B)
+		>+$(test_oid B) S.C.D (B)
+		> $(test_oid B) S.D (B)
+		>+$(test_oid B) X (B)
+		EOF
+		git submodule $opts >actual.raw &&
+		normalize_status <actual.raw >actual &&
+		test_cmp expect actual
+	'
+done
+
+for opts in \
+	"--cached --quiet" \
+	"--cached --quiet status" \
+	"--cached status --quiet" \
+	"--quiet status --cached" \
+	"status --cached --quiet"
+do
+	test_expect_success "git submodule $opts" '
+		git submodule $opts >out &&
+		test_must_be_empty out
+	'
+done
+
+for opts in \
+	"status --cached --recursive" \
+	"--cached status --recursive"
+do
+	test_expect_success "git submodule $opts" '
+		sed -e "s/^>//" >expect <<-EOF &&
+		> $(test_oid B) S (B)
+		>+$(test_oid B) S.C (B)
+		>+$(test_oid B) S.C.D (B)
+		> $(test_oid B) S.D (B)
+		>+$(test_oid B) X (B)
+		> $(test_oid B) X/S (B)
+		>+$(test_oid B) X/S.C (B)
+		>+$(test_oid B) X/S.C.D (B)
+		> $(test_oid B) X/S.D (B)
+		> $(test_oid B) X/X (B)
+		EOF
+		git submodule $opts >actual.raw &&
+		normalize_status <actual.raw >actual &&
+		test_cmp expect actual
+	'
+done
+
+test_done
-- 
2.38.0.1464.gea6794aacbc

[PATCH v2 3/9] submodule--helper: fix a memory leak in "status"

[Ævar Arnfjörð Bjarmason]

The "status" sub-command was leaking the "struct strvec" it was
setting up for the reasons explained in f92dbdbc6a8 (revisions API:
don't leak memory on argv elements that need free()-ing, 2022-08-02),
so let's use the "free_removed_argv_elements" option to
setup_revisions() to fix the leak.

Even if we did that, clobbering the "diff_files_args.nr" with the
return value of setup_revisions() would leave leaks in place, but we
can just stop clobbering it.

Ever since that code was added in a9f8a37584a (submodule: port
submodule subcommand 'status' from shell to C, 2017-10-06) we've had
no reason to modify the "nr" member ("argc" at the time): The next use
of "diff_files_args" after this is the "strvec_clear()" at the end of
the function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/submodule--helper.c | 7 ++++---
 t/t7422-submodule-output.sh | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6250b95a6f7..ee6f2d34cba 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -616,6 +616,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 	int diff_files_result;
 	struct strbuf buf = STRBUF_INIT;
 	const char *git_dir;
+	struct setup_revision_opt opt = {
+		.free_removed_argv_elements = 1,
+	};
 
 	if (!submodule_from_path(the_repository, null_oid(), path))
 		die(_("no submodule mapping found in .gitmodules for path '%s'"),
@@ -649,9 +652,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 
 	repo_init_revisions(the_repository, &rev, NULL);
 	rev.abbrev = 0;
-	diff_files_args.nr = setup_revisions(diff_files_args.nr,
-					     diff_files_args.v,
-					     &rev, NULL);
+	setup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);
 	diff_files_result = run_diff_files(&rev, 0);
 
 	if (!diff_result_code(&rev.diffopt, diff_files_result)) {
diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh
index 1e9cdf1a68e..ab946ec9405 100755
--- a/t/t7422-submodule-output.sh
+++ b/t/t7422-submodule-output.sh
@@ -2,6 +2,7 @@
 
 test_description='submodule --cached, --quiet etc. output'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-t3100.sh
 
-- 
2.38.0.1464.gea6794aacbc

[PATCH v2 4/9] submodule tests: test for a "foreach" blind-spot

[Ævar Arnfjörð Bjarmason]

We tested for "--" followed by command names, but not for "--"
followed by an argument that looks like an option, let's do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7407-submodule-foreach.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 59bd1501667..8d7b234beb8 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -154,6 +154,11 @@ test_expect_success 'use "submodule foreach" to checkout 2nd level submodule' '
 	)
 '
 
+test_expect_success 'usage: foreach -- --not-an-option' '
+	test_expect_code 1 git submodule foreach -- --not-an-option &&
+	test_expect_code 1 git -C clone2 submodule foreach -- --not-an-option
+'
+
 test_expect_success 'use "foreach --recursive" to checkout all submodules' '
 	(
 		cd clone2 &&
-- 
2.38.0.1464.gea6794aacbc

[PATCH v2 5/9] submodule.c: refactor recursive block out of absorb function

[Ævar Arnfjörð Bjarmason]

A move and indentation-only change to move the
ABSORB_GITDIR_RECURSE_SUBMODULES case into its own function, which as
we'll see makes the subsequent commit changing this code much smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 submodule.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/submodule.c b/submodule.c
index b958162d286..fe1e3f03905 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2310,6 +2310,23 @@ static void relocate_single_git_dir_into_superproject(const char *path)
 	strbuf_release(&new_gitdir);
 }
 
+static void absorb_git_dir_into_superproject_recurse(const char *path)
+{
+
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	cp.dir = path;
+	cp.git_cmd = 1;
+	cp.no_stdin = 1;
+	strvec_pushf(&cp.args, "--super-prefix=%s%s/",
+		     get_super_prefix_or_empty(), path);
+	strvec_pushl(&cp.args, "submodule--helper",
+		     "absorbgitdirs", NULL);
+	prepare_submodule_repo_env(&cp.env);
+	if (run_command(&cp))
+		die(_("could not recurse into submodule '%s'"), path);
+}
+
 /*
  * Migrate the git directory of the submodule given by path from
  * having its git directory within the working tree to the git dir nested
@@ -2366,21 +2383,10 @@ void absorb_git_dir_into_superproject(const char *path,
 	strbuf_release(&gitdir);
 
 	if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
-		struct child_process cp = CHILD_PROCESS_INIT;
-
 		if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)
 			BUG("we don't know how to pass the flags down?");
 
-		cp.dir = path;
-		cp.git_cmd = 1;
-		cp.no_stdin = 1;
-		strvec_pushf(&cp.args, "--super-prefix=%s%s/",
-			     get_super_prefix_or_empty(), path);
-		strvec_pushl(&cp.args, "submodule--helper",
-			     "absorbgitdirs", NULL);
-		prepare_submodule_repo_env(&cp.env);
-		if (run_command(&cp))
-			die(_("could not recurse into submodule '%s'"), path);
+		absorb_git_dir_into_superproject_recurse(path);
 	}
 }
 
-- 
2.38.0.1464.gea6794aacbc

[PATCH v2 6/9] submodule API & "absorbgitdirs": remove "----recursive" option

[Ævar Arnfjörð Bjarmason]

Remove the "----recursive" option to "git submodule--helper
absorbgitdirs" (yes, with 4 dashes, not 2).

This option and all the "else" when "flags &
ABSORB_GITDIR_RECURSE_SUBMODULES" is false has never been used since
it was added in f6f85861400 (submodule: add absorb-git-dir function,
2016-12-12), which we'd have had to do as "----recursive", a
"--recursive" would have errored out.

It would be nice to follow-up with an optbug() assertion to
parse-options.c for such funnily named options, I manually validated
that this was the only long option whose name started with "-", but
let's skip adding such an assertion for now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rm.c                |  3 +--
 builtin/submodule--helper.c |  8 ++------
 submodule.c                 | 13 +++----------
 submodule.h                 |  4 +---
 4 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index f0d025a4e23..05bfe20a469 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -86,8 +86,7 @@ static void submodules_absorb_gitdir_if_needed(void)
 			continue;
 
 		if (!submodule_uses_gitfile(name))
-			absorb_git_dir_into_superproject(name,
-				ABSORB_GITDIR_RECURSE_SUBMODULES);
+			absorb_git_dir_into_superproject(name);
 	}
 }
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ee6f2d34cba..33f099dbc86 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1379,8 +1379,7 @@ static void deinit_submodule(const char *path, const char *prefix,
 					  ".git file by using absorbgitdirs."),
 					displaypath);
 
-			absorb_git_dir_into_superproject(path,
-							 ABSORB_GITDIR_RECURSE_SUBMODULES);
+			absorb_git_dir_into_superproject(path);
 
 		}
 
@@ -2831,13 +2830,10 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	int i;
 	struct pathspec pathspec = { 0 };
 	struct module_list list = MODULE_LIST_INIT;
-	unsigned flags = ABSORB_GITDIR_RECURSE_SUBMODULES;
 	struct option embed_gitdir_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
 			   N_("path"),
 			   N_("path into the working tree")),
-		OPT_BIT(0, "--recursive", &flags, N_("recurse into submodules"),
-			ABSORB_GITDIR_RECURSE_SUBMODULES),
 		OPT_END()
 	};
 	const char *const git_submodule_helper_usage[] = {
@@ -2853,7 +2849,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 		goto cleanup;
 
 	for (i = 0; i < list.nr; i++)
-		absorb_git_dir_into_superproject(list.entries[i]->name, flags);
+		absorb_git_dir_into_superproject(list.entries[i]->name);
 
 	ret = 0;
 cleanup:
diff --git a/submodule.c b/submodule.c
index fe1e3f03905..8fa2ad457b2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2139,8 +2139,7 @@ int submodule_move_head(const char *path,
 	if (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {
 		if (old_head) {
 			if (!submodule_uses_gitfile(path))
-				absorb_git_dir_into_superproject(path,
-					ABSORB_GITDIR_RECURSE_SUBMODULES);
+				absorb_git_dir_into_superproject(path);
 		} else {
 			struct strbuf gitdir = STRBUF_INIT;
 			submodule_name_to_gitdir(&gitdir, the_repository,
@@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path)
  * having its git directory within the working tree to the git dir nested
  * in its superprojects git dir under modules/.
  */
-void absorb_git_dir_into_superproject(const char *path,
-				      unsigned flags)
+void absorb_git_dir_into_superproject(const char *path)
 {
 	int err_code;
 	const char *sub_git_dir;
@@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path,
 	}
 	strbuf_release(&gitdir);
 
-	if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
-		if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)
-			BUG("we don't know how to pass the flags down?");
-
-		absorb_git_dir_into_superproject_recurse(path);
-	}
+	absorb_git_dir_into_superproject_recurse(path);
 }
 
 int get_superproject_working_tree(struct strbuf *buf)
diff --git a/submodule.h b/submodule.h
index 6a9fec6de11..b52a4ff1e73 100644
--- a/submodule.h
+++ b/submodule.h
@@ -164,9 +164,7 @@ void submodule_unset_core_worktree(const struct submodule *sub);
  */
 void prepare_submodule_repo_env(struct strvec *env);
 
-#define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)
-void absorb_git_dir_into_superproject(const char *path,
-				      unsigned flags);
+void absorb_git_dir_into_superproject(const char *path);
 
 /*
  * Return the absolute path of the working tree of the superproject, which this
-- 
2.38.0.1464.gea6794aacbc

[PATCH v2 7/9] submodule--helper: remove --prefix from "absorbgitdirs"

[Ævar Arnfjörð Bjarmason]

Let's pass the "-C <prefix>" option instead to "absorbgitdirs" from
its only caller.

When it was added in f6f85861400 (submodule: add absorb-git-dir
function, 2016-12-12) there were other "submodule--helper" subcommands
that were invoked with "-C <prefix>", so we could have done this all
along.

Suggested-by: Glen Choo <chooglen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/submodule--helper.c | 3 ---
 git-submodule.sh            | 2 +-
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 33f099dbc86..fefedcf8097 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2831,9 +2831,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	struct pathspec pathspec = { 0 };
 	struct module_list list = MODULE_LIST_INIT;
 	struct option embed_gitdir_options[] = {
-		OPT_STRING(0, "prefix", &prefix,
-			   N_("path"),
-			   N_("path into the working tree")),
 		OPT_END()
 	};
 	const char *const git_submodule_helper_usage[] = {
diff --git a/git-submodule.sh b/git-submodule.sh
index 5e5d21c010f..d359f171379 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -557,7 +557,7 @@ cmd_sync()
 
 cmd_absorbgitdirs()
 {
-	git submodule--helper absorbgitdirs --prefix "$wt_prefix" "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper absorbgitdirs "$@"
 }
 
 # This loop parses the command line arguments to find the
-- 
2.38.0.1464.gea6794aacbc

[PATCH v2 8/9] submodule--helper: drop "update --prefix <pfx>" for "-C <pfx> update"

[Ævar Arnfjörð Bjarmason]

Since 29a5e9e1ffe (submodule--helper update-clone: learn --init,
2022-03-04) we've been passing "-C <prefix>" from "git-submodule.sh"
whenever we pass "--prefix <prefix>", so the latter is redundant to
the former. Let's drop the "--prefix" option.

Suggested-by: Glen Choo <chooglen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/submodule--helper.c | 4 +---
 git-submodule.sh            | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fefedcf8097..03d1b58acaa 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2643,9 +2643,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			 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_SET_INT(0, "checkout", &opt.update_default,
 			N_("use the 'checkout' update strategy (default)"),
 			SM_UPDATE_CHECKOUT),
@@ -2701,6 +2698,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	}
 
 	opt.filter_options = &filter_options;
+	opt.prefix = prefix;
 
 	if (opt.update_default)
 		opt.update_strategy.type = opt.update_default;
diff --git a/git-submodule.sh b/git-submodule.sh
index d359f171379..9a50f2e9124 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -343,7 +343,6 @@ cmd_update()
 		${recursive:+--recursive} \
 		${init:+--init} \
 		${nofetch:+--no-fetch} \
-		${wt_prefix:+--prefix "$wt_prefix"} \
 		${rebase:+--rebase} \
 		${merge:+--merge} \
 		${checkout:+--checkout} \
-- 
2.38.0.1464.gea6794aacbc

[PATCH v2 9/9] submodule--helper: use OPT_SUBCOMMAND() API

[Ævar Arnfjörð Bjarmason]

Have the cmd_submodule__helper() use the OPT_SUBCOMMAND() API
introduced in fa83cc834da (parse-options: add support for parsing
subcommands, 2022-08-19).

This is only a marginal reduction in line count, but once we start
unifying this with a yet-to-be-added "builtin/submodule.c" it'll be
much easier to reason about those changes, as they'll both use
OPT_SUBCOMMAND().

We don't need to worry about "argv[0]" being NULL in the die() because
we'd have errored out in parse_options() as we're not using
"PARSE_OPT_SUBCOMMAND_OPTIONAL".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/submodule--helper.c | 78 ++++++++++++++++++-------------------
 git.c                       |  2 +-
 2 files changed, 39 insertions(+), 41 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 03d1b58acaa..c75e9e86b06 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -3351,47 +3351,45 @@ static int module_add(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
-#define SUPPORT_SUPER_PREFIX (1<<0)
-
-struct cmd_struct {
-	const char *cmd;
-	int (*fn)(int, const char **, const char *);
-	unsigned option;
-};
-
-static struct cmd_struct commands[] = {
-	{"clone", module_clone, SUPPORT_SUPER_PREFIX},
-	{"add", module_add, 0},
-	{"update", module_update, SUPPORT_SUPER_PREFIX},
-	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
-	{"init", module_init, 0},
-	{"status", module_status, SUPPORT_SUPER_PREFIX},
-	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
-	{"deinit", module_deinit, 0},
-	{"summary", module_summary, 0},
-	{"push-check", push_check, 0},
-	{"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
-	{"set-url", module_set_url, 0},
-	{"set-branch", module_set_branch, 0},
-	{"create-branch", module_create_branch, 0},
-};
-
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 {
-	int i;
-	if (argc < 2 || !strcmp(argv[1], "-h"))
-		usage("git submodule--helper <command>");
-
-	for (i = 0; i < ARRAY_SIZE(commands); i++) {
-		if (!strcmp(argv[1], commands[i].cmd)) {
-			if (get_super_prefix() &&
-			    !(commands[i].option & SUPPORT_SUPER_PREFIX))
-				die(_("%s doesn't support --super-prefix"),
-				    commands[i].cmd);
-			return commands[i].fn(argc - 1, argv + 1, prefix);
-		}
-	}
+	const char *cmd = argv[0];
+	const char *subcmd;
+	parse_opt_subcommand_fn *fn = NULL;
+	const char *const usage[] = {
+		N_("git submodule--helper <command>"),
+		NULL
+	};
+	struct option options[] = {
+		OPT_SUBCOMMAND("clone", &fn, module_clone),
+		OPT_SUBCOMMAND("add", &fn, module_add),
+		OPT_SUBCOMMAND("update", &fn, module_update),
+		OPT_SUBCOMMAND("foreach", &fn, module_foreach),
+		OPT_SUBCOMMAND("init", &fn, module_init),
+		OPT_SUBCOMMAND("status", &fn, module_status),
+		OPT_SUBCOMMAND("sync", &fn, module_sync),
+		OPT_SUBCOMMAND("deinit", &fn, module_deinit),
+		OPT_SUBCOMMAND("summary", &fn, module_summary),
+		OPT_SUBCOMMAND("push-check", &fn, push_check),
+		OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs),
+		OPT_SUBCOMMAND("set-url", &fn, module_set_url),
+		OPT_SUBCOMMAND("set-branch", &fn, module_set_branch),
+		OPT_SUBCOMMAND("create-branch", &fn, module_create_branch),
+		OPT_END()
+	};
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+	subcmd = argv[0];
+
+	if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") &&
+	    strcmp(subcmd, "foreach") && strcmp(subcmd, "status") &&
+	    strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") &&
+	    get_super_prefix())
+		/*
+		 * xstrfmt() rather than "%s %s" to keep the translated
+		 * string identical to git.c's.
+		 */
+		die(_("%s doesn't support --super-prefix"),
+		    xstrfmt("'%s %s'", cmd, subcmd));
 
-	die(_("'%s' is not a valid submodule--helper "
-	      "subcommand"), argv[1]);
+	return fn(argc, argv, prefix);
 }
diff --git a/git.c b/git.c
index ee7758dcb0e..fb69e605912 100644
--- a/git.c
+++ b/git.c
@@ -610,7 +610,7 @@ static struct cmd_struct commands[] = {
 	{ "stash", cmd_stash, RUN_SETUP | NEED_WORK_TREE },
 	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
 	{ "stripspace", cmd_stripspace },
-	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },
+	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX },
 	{ "switch", cmd_switch, RUN_SETUP | NEED_WORK_TREE },
 	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
 	{ "tag", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },
-- 
2.38.0.1464.gea6794aacbc

Re: [PATCH v2 0/9] submodule: tests, cleanup to prepare for built-in

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Changes since v1:
>
> * Noted the history of when "submodule--helper config" was last used,
>   per Glen's comment.
> * Re-titled the tests htat no longer use the now-removed
>   "submodule--helper config", per Glen's comment.
> * I removed _() from a string in what's left of "submodule--helper
>   config", which is now a test helper. We should not be translating
>   test helper strings.
> * Fixed a buggy for-loop in the t7422-submodule-output.sh test I'm
>   adding.
> * Fixed a trivial typo in a comment in that test & in another nearby
>   comment.
>

[...]

> Range-diff against v1:
>  1:  b2649f96715 !  1:  ca8f8b4bf63 submodule--helper: move "config" to a test-tool
>     @@ Commit message
>          'ab/submodule-helper-prep', 2022-09-13) the "config" sub-command was
>          only used by our own tests.
>      
>     +    It was last used by "git submodule" itself in code that went away with
>     +    a6226fd772b (submodule--helper: convert the bulk of cmd_add() to C,
>     +    2021-08-10).
>     +
>          Let's move it over, and while doing so make it easier to reason about
>          by splitting up the various uses for it into separate sub-commands, so
>          that we don't need to count arguments to see what it does.
>      
>     +    This also has the advantage that we stop wasting future translator
>     +    time on this command, currently the usage information for this
>     +    internal-only tool has been translated into several languages. The use
>     +    of the "_" function has also been removed from the "please make
>     +    sure..." message.
>     +
>          Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>      
>       ## builtin/submodule--helper.c ##
>     @@ t/helper/test-submodule.c: static int cmd__submodule_resolve_relative_url(int ar
>      +	/* Equivalent to ACTION_SET in builtin/config.c */
>      +	if (argc == 3) {
>      +		if (!is_writing_gitmodules_ok())
>     -+			die(_("please make sure that the .gitmodules file is in the working tree"));
>     ++			die("please make sure that the .gitmodules file is in the working tree");
>      +
>      +		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
>      +	}
>     @@ t/helper/test-submodule.c: static int cmd__submodule_resolve_relative_url(int ar
>      +
>      +	if (argc == 2) {
>      +		if (!is_writing_gitmodules_ok())
>     -+			die(_("please make sure that the .gitmodules file is in the working tree"));
>     ++			die("please make sure that the .gitmodules file is in the working tree");
>      +		return config_set_in_gitmodules_file_gently(argv[1], NULL);
>      +	}
>      +	usage_with_options(usage, options);
>     @@ t/helper/test-submodule.c: static int cmd__submodule_resolve_relative_url(int ar
>      
>       ## t/t7411-submodule-config.sh ##
>      @@ t/t7411-submodule-config.sh: test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
>     - test_expect_success 'reading submodules config from the working tree with "submodule--helper config"' '
>     + 	)
>     + '
>     + 
>     +-test_expect_success 'reading submodules config from the working tree with "submodule--helper config"' '
>     ++test_expect_success 'reading submodules config from the working tree' '
>       	(cd super &&
>       		echo "../submodule" >expect &&
>      -		git submodule--helper config submodule.submodule.url >actual &&
>     @@ t/t7411-submodule-config.sh: test_expect_success 'error in history in fetchrecur
>       	)
>       '
>       
>     - test_expect_success 'unsetting submodules config from the working tree with "submodule--helper config --unset"' '
>     +-test_expect_success 'unsetting submodules config from the working tree with "submodule--helper config --unset"' '
>     ++test_expect_success 'unsetting submodules config from the working tree' '
>       	(cd super &&
>      -		git submodule--helper config --unset submodule.submodule.url &&
>      -		git submodule--helper config submodule.submodule.url >actual &&
>     @@ t/t7411-submodule-config.sh: test_expect_success 'error in history in fetchrecur
>       		test_must_be_empty actual
>       	)
>       '
>     -@@ t/t7411-submodule-config.sh: test_expect_success 'unsetting submodules config from the working tree with "sub
>     - test_expect_success 'writing submodules config with "submodule--helper config"' '
>     + 
>     + 
>     +-test_expect_success 'writing submodules config with "submodule--helper config"' '
>     ++test_expect_success 'writing submodules config' '
>       	(cd super &&
>       		echo "new_url" >expect &&
>      -		git submodule--helper config submodule.submodule.url "new_url" &&
>     @@ t/t7411-submodule-config.sh: test_expect_success 'unsetting submodules config fr
>       		test_cmp expect actual
>       	)
>       '
>     -@@ t/t7411-submodule-config.sh: test_expect_success 'overwriting unstaged submodules config with "submodule--hel
>     + 
>     +-test_expect_success 'overwriting unstaged submodules config with "submodule--helper config"' '
>     ++test_expect_success 'overwriting unstaged submodules config' '
>       	test_when_finished "git -C super checkout .gitmodules" &&
>       	(cd super &&
>       		echo "newer_url" >expect &&
>  2:  cda36b5b6e0 !  2:  5508c27f653 submodule tests: add tests for top-level flag output
>     @@ t/t7422-submodule-output.sh (new)
>      +	git -C X pull &&
>      +	GIT_ALLOW_PROTOCOL=file git -C X submodule update --init &&
>      +
>     -+	# dirty p
>     ++	# dirty
>      +	for d in S.D X/S.D
>      +	do
>     -+		echo dirty >S.D/A.t || return 1
>     ++		echo dirty >"$d"/A.t || return 1
>      +	done &&
>      +
>      +	# commit (for --cached)
>     @@ t/t7422-submodule-output.sh (new)
>      +
>      +	for ref in A B C
>      +	do
>     -+		# Not different with SHA-1 and SHA-256, just (ab)usign
>     ++		# Not different with SHA-1 and SHA-256, just (ab)using
>      +		# test_oid_cache as a variable bag to avoid using
>      +		# $(git rev-parse ...).
>      +		oid=$(git rev-parse $ref) &&
>  -:  ----------- >  3:  a3529d7f9e0 submodule--helper: fix  a memory leak in "status"
>  3:  0ed1fc7fdf8 =  4:  c14cc0e14b8 submodule tests: test for a "foreach" blind-spot
>  4:  f7adfbc13ae =  5:  459ea25125b submodule.c: refactor recursive block out of absorb function
>  5:  2b8afd73b9b =  6:  322a02c30fc submodule API & "absorbgitdirs": remove "----recursive" option
>  6:  91208241070 =  7:  d1f4ac20a4f submodule--helper: remove --prefix from "absorbgitdirs"
>  7:  77d4d5a6c09 =  8:  ac9ff05ef68 submodule--helper: drop "update --prefix <pfx>" for "-C <pfx> update"
>  8:  105853cd358 =  9:  cccd92a829c submodule--helper: use OPT_SUBCOMMAND() API

Thanks! This addresses all of my comments from v1, and I didn't 
spot any other issues during a quick 2nd pass.

  Reviewed-by: Glen Choo <chooglen@google.com>

Re: [PATCH v2 0/9] submodule: tests, cleanup to prepare for built-in

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 08 2022, Glen Choo wrote:

> [...]
> Thanks! This addresses all of my comments from v1, and I didn't 
> spot any other issues during a quick 2nd pass.
>
>   Reviewed-by: Glen Choo <chooglen@google.com>

Thanks for the quick review!

One thing I completely forgot in the v2 CL: The 3/9 here is new. Stictly
speaking we could skip it, but I wanted to be able to mark the new test
I added as as leak-free, and fixing the only leak it ran into was
trivial:
https://lore.kernel.org/git/patch-v2-3.9-a3529d7f9e0-20221108T140501Z-avarab@gmail.com/

So rather than having another "small leak fix for submodules" topic
depend on this topic, I think it's OK to just bundle that up with this
one.

Re: [PATCH v2 0/9] submodule: tests, cleanup to prepare for built-in

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, Nov 08 2022, Glen Choo wrote:
>
>> [...]
>> Thanks! This addresses all of my comments from v1, and I didn't 
>> spot any other issues during a quick 2nd pass.
>>
>>   Reviewed-by: Glen Choo <chooglen@google.com>
>
> Thanks for the quick review!
>
> One thing I completely forgot in the v2 CL: The 3/9 here is new. Stictly
> speaking we could skip it, but I wanted to be able to mark the new test
> I added as as leak-free, and fixing the only leak it ran into was
> trivial:
> https://lore.kernel.org/git/patch-v2-3.9-a3529d7f9e0-20221108T140501Z-avarab@gmail.com/
>
> So rather than having another "small leak fix for submodules" topic
> depend on this topic, I think it's OK to just bundle that up with this
> one.

Yes, it looked quite trivial. IMO bundling this was a more efficient use
of time than sending another series.