submodule update: parse all options in C

From: Glen Choo <[email protected]>

When the cmd_*() functions in git-submodule.sh parse options, they sometimes set an intermediate variable to "1", check if the variable is set, and if so, pass a string option to "git submodule--helper".

In many cases, this parsing and unparsing is unnecessary because the "git submodule--helper" command already understands the literal options passed to "git submodule", e.g. when we see "--init", we don't need to set "init=1" only to pass "${init:+--init}" again.

In cmd_update(), remove unnecessary "${variable+option}" expressions by passing the literal option to "git submodule--helper update". Do this by appending the string option into a catch-all "opts", so that the intermediate variable becomes unused and can be removed.

This slightly changes the options we pass to "git submodule--helper update" because we now pass every instance of the option instead of only passing it once. This is probably a good thing though, because it's more consistent to handle this in C instead of custom shell code.

---
 git-submodule.sh | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

index fd0b4a2c947..f4679e0db80 100755
@@ -38,17 +38,13 @@ recursive=
 init=
 require_init=
 files=
-remote=
-nofetch=
 update=
 prefix=
 custom_name=
 depth=
 progress=
 dissociate=
-single_branch=
 jobs=
-recommend_shallow=
 filter=
 
 isnumber()

@@ -246,6 +242,7 @@ cmd_deinit()
 #
 cmd_update()
 {
+	opts=
 	# parse $args after "submodule ... update".
 	while test $# -ne 0
 	do

@@ -257,7 +254,7 @@ cmd_update()
 			unset GIT_QUIET
 			;;
 		--progress)
-			progress=1
+			opts="$opts $1"
 			;;
 		-i|--init)
 			init=1

@@ -267,13 +264,13 @@ cmd_update()
 			require_init=1
 			;;
 		--remote)
-			remote=1
+			opts="$opts $1"
 			;;
 		-N|--no-fetch)
-			nofetch=1
+			opts="$opts $1"
 			;;
 		-f|--force)
-			force=$1
+			opts="$opts $1"
 			;;
 		-r|--rebase)
 			update="rebase"

@@ -287,13 +284,13 @@ cmd_update()
 			reference="$1"
 			;;
 		--dissociate)
-			dissociate=1
+			opts="$opts $1"
 			;;
 		-m|--merge)
 			update="merge"
 			;;
 		--recursive)
-			recursive=1
+			opts="$opts $1"
 			;;
 		--checkout)
 			update="checkout"

@@ -350,24 +347,18 @@ cmd_update()
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update \
 		${GIT_QUIET:+--quiet} \
-		${force:+--force} \
-		${progress:+"--progress"} \
-		${remote:+--remote} \
-		${recursive:+--recursive} \
 		${init:+--init} \
-		${nofetch:+--no-fetch} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
 		${prefix:+--recursive-prefix "$prefix"} \
 		${update:+--update "$update"} \
 		${reference:+"$reference"} \
-		${dissociate:+"--dissociate"} \
 		${depth:+"$depth"} \
 		${require_init:+--require-init} \
-		${dissociate:+"--dissociate"} \
 		$single_branch \
 		$recommend_shallow \
 		$jobs \
 		$filter \
+		$opts \
 		-- \
 		"$@"
 }

gitgitgadget

As a follow up to ar/submodule-update [1] and its successors gc/submodule-update-part* [2] [3], this series converts the last remaining piece of "git submodule update" into C, namely, the option parsing in git-submodule.sh.

As a result, git-submodule.sh::cmd_update() is now an (almost) one-liner:

cmd_update() { git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update ${wt_prefix:+--prefix "$wt_prefix"}
"$@" }

and best of all, "git submodule update" now shows a usage string for its own subcommand instead of a giant usage string for all of "git submodule" :)

Given how many options "git submodule update" accepts, this series takes a gradual approach:

1. Create a variable opts, which holds the literal options we want to pass to "git submodule--helper update". Then, for each option...
2. If "git submodule--helper update" already understands the string option, append it to opts and remove any special handling (1-3/8).
3. Otherwise, if the option makes sense, teach "git submodule--helper update" to understand the option. Goto 2. (4-5/8).
4. Otherwise, if the option makes no sense, drop it (6/8).
5. When we've processed all options, delete all the option parsing code

    (7/8) and clean up (8/8).

I read over the other "git submodule" subcommands very briefly to see how much work this would be if we were to repeat the process for all subcommands. It should be relatively simple - most "git submodule--helper" commands already understand every CLI option that they need to, so we can skip step 3 (i.e. the tedious one).

I'm sending this as an RFC because even though this series is pretty low-risk, the previous topics [1] [2] [3] had their fair share of regressions (even when the conversions seemed 'obviously correct') and showed that the test coverage of "git submodule "update" is quite poor. I'd like to get feedback on the general approach before I invest more time into improving the test coverage and/or checking the before/after output for regressions [4].

[1] https://lore.kernel.org/git/[email protected]/
[2] Part 1
https://lore.kernel.org/git/[email protected]/ [3] Part 2
https://lore.kernel.org/git/[email protected]/ [4] Ævar suggested a way to do this using the test suite and "--tee" in
https://lore.kernel.org/git/[email protected]/

Glen Choo (8):
submodule update: remove intermediate parsing submodule update: pass options containing "[no-]" submodule update: pass options with stuck forms submodule update: pass --require-init and --init submodule--helper update: use one param per type submodule update: remove -v, pass --quiet submodule update: stop parsing options in .sh submodule update: remove never-used expansion

 builtin/submodule--helper.c |  41 +++++++----
 git-submodule.sh            | 131 ------------------------------------
 t/t7406-submodule-update.sh |   2 +-
 3 files changed, 29 insertions(+), 145 deletions(-)

base-commit: 1e59178e3f65880188caedb965e70db5ceeb2d64
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1275%2Fchooglen%2Fsubmodule%2Fparse-update-opts-in-c-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1275/chooglen/submodule/parse-update-opts-in-c-v1
Pull-Request: https://github.com/git/git/pull/1275

gitgitgadget

From: Glen Choo <[email protected]>

"git submodule--helper update" understands these options, so append them to opts and remove their intermediate variables.

As a result, we now pass every instance of the option and its "no-" variant to "git submodule--helper update" instead of only passing the last instance. As we noted in the previous commit, this is good because this makes our options parsing consistent.

---
 git-submodule.sh | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

index f4679e0db80..8c46eac22e8 100755
@@ -296,10 +296,10 @@ cmd_update()
 			update="checkout"
 			;;
 		--recommend-shallow)
-			recommend_shallow="--recommend-shallow"
+			opts="$opts $1"
 			;;
 		--no-recommend-shallow)
-			recommend_shallow="--no-recommend-shallow"
+			opts="$opts $1"
 			;;
 		--depth)
 			case "$2" in '') usage ;; esac

@@ -318,10 +318,10 @@ cmd_update()
 			jobs=$1
 			;;
 		--single-branch)
-			single_branch="--single-branch"
+			opts="$opts $1"
 			;;
 		--no-single-branch)
-			single_branch="--no-single-branch"
+			opts="$opts $1"
 			;;
 		--filter)
 			case "$2" in '') usage ;; esac

@@ -354,8 +354,6 @@ cmd_update()
 		${reference:+"$reference"} \
 		${depth:+"$depth"} \
 		${require_init:+--require-init} \
-		$single_branch \
-		$recommend_shallow \
 		$jobs \
 		$filter \
 		$opts \

gitgitgadget

From: Glen Choo <[email protected]>

git-submodule.sh::cmd_update() converts 'unstuck' forms of options into their 'stuck' forms before passing them to "git submodule--helper update". Remove this special handling of 'unstuck' options and append the options to opts.

---
 git-submodule.sh | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

index 8c46eac22e8..7418fd5bf51 100755
@@ -32,7 +32,6 @@ export GIT_PROTOCOL_FROM_USER
 command=
 branch=
 force=
-reference=
 cached=
 recursive=
 init=

@@ -44,8 +43,6 @@ custom_name=
 depth=
 progress=
 dissociate=
-jobs=
-filter=
 
 isnumber()
 {

@@ -277,11 +274,11 @@ cmd_update()
 			;;
 		--reference)
 			case "$2" in '') usage ;; esac
-			reference="--reference=$2"
+			opts="$opts $1 $2"
 			shift
 			;;
 		--reference=*)
-			reference="$1"
+			opts="$opts $1"
 			;;
 		--dissociate)
 			opts="$opts $1"

@@ -303,19 +300,19 @@ cmd_update()
 			;;
 		--depth)
 			case "$2" in '') usage ;; esac
-			depth="--depth=$2"
+			opts="$opts $1 $2"
 			shift
 			;;
 		--depth=*)
-			depth=$1
+			opts="$opts $1"
 			;;
 		-j|--jobs)
 			case "$2" in '') usage ;; esac
-			jobs="--jobs=$2"
+			opts="$opts $1 $2"
 			shift
 			;;
 		--jobs=*)
-			jobs=$1
+			opts="$opts $1"
 			;;
 		--single-branch)
 			opts="$opts $1"

@@ -325,11 +322,11 @@ cmd_update()
 			;;
 		--filter)
 			case "$2" in '') usage ;; esac
-			filter="--filter=$2"
+			opts="$opts $1 $2"
 			shift
 			;;
 		--filter=*)
-			filter="$1"
+			opts="$opts $1"
 			;;
 		--)
 			shift

@@ -351,11 +348,7 @@ cmd_update()
 		${wt_prefix:+--prefix "$wt_prefix"} \
 		${prefix:+--recursive-prefix "$prefix"} \
 		${update:+--update "$update"} \
-		${reference:+"$reference"} \
-		${depth:+"$depth"} \
 		${require_init:+--require-init} \
-		$jobs \
-		$filter \
 		$opts \
 		-- \
 		"$@"

gitgitgadget

From: Glen Choo <[email protected]>

git-submodule.sh::cmd_update() makes "--require-init" imply "--init". Move this logic into "git submodule--helper update" instead, and append both options to opts.

---
 builtin/submodule--helper.c | 4 ++++
 git-submodule.sh            | 9 ++-------
 2 files changed, 6 insertions(+), 7 deletions(-)

index c6c2ba1b6dc..a40f9f143e8 100644
@@ -2642,6 +2642,10 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_update_options,
 			     git_submodule_helper_usage, 0);
 
+	if (opt.require_init) {
+		opt.init = 1;
+	}
+
 	if (filter_options.choice && !opt.init) {
 		usage_with_options(git_submodule_helper_usage,
 				   module_update_options);

index 7418fd5bf51..f61d12ccd21 100755
@@ -34,8 +34,6 @@ branch=
 force=
 cached=
 recursive=
-init=
-require_init=
 files=
 update=
 prefix=

@@ -254,11 +252,10 @@ cmd_update()
 			opts="$opts $1"
 			;;
 		-i|--init)
-			init=1
+			opts="$opts $1"
 			;;
 		--require-init)
-			init=1
-			require_init=1
+			opts="$opts $1"
 			;;
 		--remote)
 			opts="$opts $1"

@@ -344,11 +341,9 @@ cmd_update()
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update \
 		${GIT_QUIET:+--quiet} \
-		${init:+--init} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
 		${prefix:+--recursive-prefix "$prefix"} \
 		${update:+--update "$update"} \
-		${require_init:+--require-init} \
 		$opts \
 		-- \
 		"$@"

gitgitgadget

From: Glen Choo <[email protected]>

git-submodule.sh accepts literal 'update types' as options, but parses them into a "--update" option for "git submodule--helper update", e.g. "--checkout" becomes "--update=checkout".

Teach the 'update type' options to "git submodule--helper update" so that we no longer have to do this parsing in git-submodule.sh, and append them to opts.

---
 builtin/submodule--helper.c | 37 ++++++++++++++++++++++++-------------
 git-submodule.sh            |  8 +++-----
 2 files changed, 27 insertions(+), 18 deletions(-)

index a40f9f143e8..09510ebc026 100644
@@ -1818,7 +1818,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 static void determine_submodule_update_strategy(struct repository *r,
 						int just_cloned,
 						const char *path,
-						const char *update,
+						enum submodule_update_type update,
 						struct submodule_update_strategy *out)
 {
 	const struct submodule *sub = submodule_from_path(r, null_oid(), path);

@@ -1828,9 +1828,7 @@ static void determine_submodule_update_strategy(struct repository *r,
 	key = xstrfmt("submodule.%s.update", sub->name);
 
 	if (update) {
-		if (parse_submodule_update_strategy(update, out) < 0)
-			die(_("Invalid update mode '%s' for submodule path '%s'"),
-				update, path);
+		out->type = update;
 	} else if (!repo_config_get_string_tmp(r, key, &val)) {
 		if (parse_submodule_update_strategy(val, out) < 0)
 			die(_("Invalid update mode '%s' configured for submodule path '%s'"),

@@ -1882,7 +1880,6 @@ struct update_data {
 	const char *prefix;
 	const char *recursive_prefix;
 	const char *displaypath;
-	const char *update_default;
 	struct object_id suboid;
 	struct string_list references;
 	struct submodule_update_strategy update_strategy;

@@ -1892,6 +1889,7 @@ struct update_data {
 	int max_jobs;
 	int single_branch;
 	int recommend_shallow;
+	enum submodule_update_type update_default;
 	unsigned int require_init;
 	unsigned int force;
 	unsigned int quiet;

@@ -2429,8 +2427,20 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 		strvec_push(args, "--require-init");
 	if (update_data->depth)
 		strvec_pushf(args, "--depth=%d", update_data->depth);
-	if (update_data->update_default)
-		strvec_pushl(args, "--update", update_data->update_default, NULL);
+	switch (update_data->update_default) {
+	case SM_UPDATE_MERGE:
+		strvec_push(args, "--merge");
+		break;
+	case SM_UPDATE_CHECKOUT:
+		strvec_push(args, "--checkout");
+		break;
+	case SM_UPDATE_REBASE:
+		strvec_push(args, "--rebase");
+		break;
+	default:
+		break;
+	}
+
 	if (update_data->references.nr) {
 		struct string_list_item *item;
 		for_each_string_list_item(item, &update_data->references)

@@ -2600,9 +2610,12 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			   N_("path"),
 			   N_("path into the working tree, across nested "
 			      "submodule boundaries")),
-		OPT_STRING(0, "update", &opt.update_default,
-			   N_("string"),
-			   N_("rebase, merge, checkout or none")),
+		OPT_SET_INT(0, "checkout", &opt.update_default,
+			    N_("update using checkout"), SM_UPDATE_CHECKOUT),
+		OPT_SET_INT('r', "rebase", &opt.update_default,
+			    N_("update using rebase"), SM_UPDATE_REBASE),
+		OPT_SET_INT('m', "merge", &opt.update_default,
+			    N_("update using merge"), SM_UPDATE_MERGE),
 		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
 			   N_("reference repository")),
 		OPT_BOOL(0, "dissociate", &opt.dissociate,

@@ -2654,9 +2667,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	opt.filter_options = &filter_options;
 
 	if (opt.update_default)
-		if (parse_submodule_update_strategy(opt.update_default,
-						    &opt.update_strategy) < 0)
-			die(_("bad value for update parameter"));
+		opt.update_strategy.type = opt.update_default;
 
 	if (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0) {
 		list_objects_filter_release(&filter_options);

index f61d12ccd21..d93b6dfbbd7 100755
@@ -35,7 +35,6 @@ force=
 cached=
 recursive=
 files=
-update=
 prefix=
 custom_name=
 depth=

@@ -267,7 +266,7 @@ cmd_update()
 			opts="$opts $1"
 			;;
 		-r|--rebase)
-			update="rebase"
+			opts="$opts $1"
 			;;
 		--reference)
 			case "$2" in '') usage ;; esac

@@ -281,13 +280,13 @@ cmd_update()
 			opts="$opts $1"
 			;;
 		-m|--merge)
-			update="merge"
+			opts="$opts $1"
 			;;
 		--recursive)
 			opts="$opts $1"
 			;;
 		--checkout)
-			update="checkout"
+			opts="$opts $1"
 			;;
 		--recommend-shallow)
 			opts="$opts $1"

@@ -343,7 +342,6 @@ cmd_update()
 		${GIT_QUIET:+--quiet} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
 		${prefix:+--recursive-prefix "$prefix"} \
-		${update:+--update "$update"} \
 		$opts \
 		-- \
 		"$@"

gitgitgadget

From: Glen Choo <[email protected]>

cmd_update()'s option parsing loop is now obsolete - it only appends options to opts without doing additional parsing. Remove it.

---
 git-submodule.sh | 101 -----------------------------------------------
 1 file changed, 101 deletions(-)

index 6b3f161dc53..e9d93e91d21 100755
@@ -236,110 +236,9 @@ cmd_deinit()
 #
 cmd_update()
 {
-	opts=
-	# parse $args after "submodule ... update".
-	while test $# -ne 0
-	do
-		case "$1" in
-		-q|--quiet)
-			opts="$opts $1"
-			;;
-		--progress)
-			opts="$opts $1"
-			;;
-		-i|--init)
-			opts="$opts $1"
-			;;
-		--require-init)
-			opts="$opts $1"
-			;;
-		--remote)
-			opts="$opts $1"
-			;;
-		-N|--no-fetch)
-			opts="$opts $1"
-			;;
-		-f|--force)
-			opts="$opts $1"
-			;;
-		-r|--rebase)
-			opts="$opts $1"
-			;;
-		--reference)
-			case "$2" in '') usage ;; esac
-			opts="$opts $1 $2"
-			shift
-			;;
-		--reference=*)
-			opts="$opts $1"
-			;;
-		--dissociate)
-			opts="$opts $1"
-			;;
-		-m|--merge)
-			opts="$opts $1"
-			;;
-		--recursive)
-			opts="$opts $1"
-			;;
-		--checkout)
-			opts="$opts $1"
-			;;
-		--recommend-shallow)
-			opts="$opts $1"
-			;;
-		--no-recommend-shallow)
-			opts="$opts $1"
-			;;
-		--depth)
-			case "$2" in '') usage ;; esac
-			opts="$opts $1 $2"
-			shift
-			;;
-		--depth=*)
-			opts="$opts $1"
-			;;
-		-j|--jobs)
-			case "$2" in '') usage ;; esac
-			opts="$opts $1 $2"
-			shift
-			;;
-		--jobs=*)
-			opts="$opts $1"
-			;;
-		--single-branch)
-			opts="$opts $1"
-			;;
-		--no-single-branch)
-			opts="$opts $1"
-			;;
-		--filter)
-			case "$2" in '') usage ;; esac
-			opts="$opts $1 $2"
-			shift
-			;;
-		--filter=*)
-			opts="$opts $1"
-			;;
-		--)
-			shift
-			break
-			;;
-		-*)
-			usage
-			;;
-		*)
-			break
-			;;
-		esac
-		shift
-	done
-
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update \
 		${wt_prefix:+--prefix "$wt_prefix"} \
 		${prefix:+--recursive-prefix "$prefix"} \
-		$opts \
-		-- \
 		"$@"
 }
 

gitgitgadget

From: Glen Choo <[email protected]>

The prefix variable is never set, so this conditional expansion is always '' (which does nothing).

---
 git-submodule.sh | 1 -
 1 file changed, 1 deletion(-)

index e9d93e91d21..2a6ffa8ac34 100755
@@ -238,7 +238,6 @@ cmd_update()
 {
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update \
 		${wt_prefix:+--prefix "$wt_prefix"} \
-		${prefix:+--recursive-prefix "$prefix"} \
 		"$@"
 }
 

gitgitgadget

From: Glen Choo <[email protected]>

In cmd_update(), "-v" unsets GIT_QUIET, but this does nothing because GIT_QUIET is only set when "-q|--quiet" is passed.

Remove "-v", and since "git submodule--helper update" already understands 'quiet' options, append them to opts. This makes GIT_QUIET obsolete, so remove it.

---
 git-submodule.sh            | 6 +-----
 t/t7406-submodule-update.sh | 2 +-
 2 files changed, 2 insertions(+), 6 deletions(-)

index d93b6dfbbd7..6b3f161dc53 100755
@@ -242,10 +242,7 @@ cmd_update()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
-			;;
-		-v)
-			unset GIT_QUIET
+			opts="$opts $1"
 			;;
 		--progress)
 			opts="$opts $1"

@@ -339,7 +336,6 @@ cmd_update()
 	done
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update \
-		${GIT_QUIET:+--quiet} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
 		${prefix:+--recursive-prefix "$prefix"} \
 		$opts \

index 43f779d751c..06d804e2131 100755
@@ -1074,7 +1074,7 @@ test_expect_success 'submodule update --quiet passes quietness to merge/rebase'
 	 git submodule update --rebase --quiet >out 2>err &&
 	 test_must_be_empty out &&
 	 test_must_be_empty err &&
-	 git submodule update --rebase -v >out 2>err &&
+	 git submodule update --rebase >out 2>err &&
 	 test_file_not_empty out &&
 	 test_must_be_empty err
 	)

gitgitgadget

The sanitize_submodule_env() function was last used in b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15), let's remove it.

This also allows us to remove clear_local_git_env() from git-sh-setup.sh. That function hasn't been documented in Documentation/git-sh-setup.sh, and since 14111fc4927 (git: submodule honor -c credential.* from command line, 2016-02-29) it had only been used in the sanitize_submodule_env() function being removed here.

---
 git-sh-setup.sh  |  7 -------
 git-submodule.sh | 11 -----------
 2 files changed, 18 deletions(-)

index d92df37e992..ecb60d9e3cb 100644
@@ -285,13 +285,6 @@ get_author_ident_from_commit () {
 	parse_ident_from_commit author AUTHOR
 }
 
-# Clear repo-local GIT_* environment variables. Useful when switching to
-# another repository (e.g. when entering a submodule). See also the env
-# list in git_connect()
-clear_local_git_env() {
-	unset $(git rev-parse --local-env-vars)
-}
-
 # Generate a virtual base file for a two-file merge. Uses git apply to
 # remove lines from $1 that are not in $2, leaving only common lines.
 create_virtual_base() {

index fd0b4a2c947..bc436c4ca47 100755
@@ -56,17 +56,6 @@ isnumber()
 	n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
 }
 
-# Sanitize the local git environment for use within a submodule. We
-# can't simply use clear_local_git_env since we want to preserve some
-# of the settings from GIT_CONFIG_PARAMETERS.
-sanitize_submodule_env()
-{
-	save_config=$GIT_CONFIG_PARAMETERS
-	clear_local_git_env
-	GIT_CONFIG_PARAMETERS=$save_config
-	export GIT_CONFIG_PARAMETERS
-}
-
 #
 # Add a new submodule to the working tree, .gitmodules and the index
 #

2.36.1.1178.gb5b1747c546

Aside at the end at [2].

That's quite the timing coincidence. I hacked this up yesterday, thinking that the submodule topic had been too quiet for a while, and wondering how hard it was to convert the rest of git-submodule.sh.

It's more than 2x the length of yours, but gets to the point where we can "git rm git-submodule.sh".

Some brief comparison/commentary:

Yeah, this is the alternate approach I considered and ended up discarding. I.e. to make forward progress with migrating things away from the cmd_*() functions you either have to prepare things in advance and then sweep the rug from under them in one go.

Or, as you're doing here teaching them about the options they're not-really-parsing anymore, but must know about because they're in a loop that ends with a "if unknown option, usage".

Almost the same as my 12/20.

Same as my 13/20, but I ended up doing it in a more narrow/smaller way. I tried your way and ran into some bug, then figured I'd do it more narrowly instead of debugging it.

Hrm, so we don't need it at all then. Well, that's a bit simpler than my 1[45]/20 and 17/20 :)

So yeah, definitely RFC-quality, but I ran into that one test that used -v, and then saw the missing docs etc. But no cheating, so I've left it in :)

I do wonder if we should leave it in anyway, we never documented -v, but we *did* understand it, and if you look at:

git log -p -Gsay -- git-submodule.sh

We used to have a lot more code impacted by it, but looking at this again now it would have only been for users of command-lines like:

    git submodule --quiet update -v [...]

I.e. where we already set the flag to the non-default quiet, and then used -v to flip it.

I think at this point I've talked myself into "let's just remove it", but maybe...

Same effect as my 16/20, but it's the last one I converted, the cmd_update() case being the trickiest.

Same as my 02/20, but as seen there I think you missed several "prefix" non-uses.

Brief commentary on my patches, details in commit messages:

Ævar Arnfjörð Bjarmason (20):
git-submodule.sh: remove unused sanitize_submodule_env() git-submodule.sh: remove unused $prefix variable git-submodule.sh: remove unused --super-prefix logic

I removed a bit more dead code here than yours.

  git-submodule.sh: normalize parsing of "--branch"
  git-submodule.sh: normalize parsing of --cached

This & various other prep commits (hereafter "easy prep") make subsequent one-time conversions of whole cmd_*() easier.

submodule--helper: rename "absorb-git-dirs" to "absorbgitdirs" git-submodule.sh: create a "case" dispatch statement

easy prep

submodule--helper: pretend to be "git submodule" in "-h" output

easy prep & bug fix for existing (on master) output bugs.

git-submodule.sh: dispatch "sync" to helper
git-submodule.sh: dispatch directly to helper
git-submodule.sh: dispatch "foreach" to helper

These are easy conversions as the options 1=1 map after the above prep.

  submodule--helper: have --require-init imply --init
  submodule--helper: understand --checkout, --merge and --rebase
    synonyms
  git-submodule doc: document the -v" option to "update"
  submodule--helper: understand -v option for "update"

not-so-easy prep for "cmd_update()"

git-submodule.sh: dispatch "update" to helper

Full cmd_update() migration in one go.

git-submodule.sh: use "$quiet", not "$GIT_QUIET"

"easy prep", but this one is less overall churn if done at the end, but as noted above could/should maybe be dropped entirely.

git-submodule.sh: simplify parsing loop

Not really needed, but I wanted to get the code as close to minimal for the next step, to eyeball the resulting sh v.s. C version.

submodule: make it a built-in, remove git-submodule.sh

We now have a builtin/submodule.c *and* the current builtin/submodule--helper.c, and we even dispatch to "git submodule--helper" via run_command()!

The idea is to be as close as possible to a bug-for-bug implementation of the shellscript, and that reviewers should be confident in being able to trace what commands we invoked before/after, we're invoking the same "git submodule--helper" commands.

Of course we eventually want to get to some full union of builtin/submodule{,--helper}.c, but that can wait.

submodule: add a subprocess-less submodule.useBuiltin setting

Wait, a useBuiltin setting to switch between two built-ins? Yeah, maybe it makes little sense, but here we get rid of the run_command() overhead, and could generally use the built-in to experiment with deeper integration between the two.

Performance is around ~2x faster with the "real" built-in than the run_command() version, whic hin turn is more than 6x as fast on basic overhead than the shellscript version, to the extent that anyone cares about "git submodule" overhead. See [1] at the end for a benchmark.

That last change adds a CI target for GIT_TEST_SUBMODULE_USE_BUILTIN=true, full CI run here:
https://github.com/avar/git/actions/runs/2472131257

 Documentation/config/submodule.txt |   4 +
 Documentation/git-submodule.txt    |   8 +-
 Makefile                           |   2 +-
 builtin.h                          |   1 +
 builtin/submodule--helper.c        | 118 +++---
 builtin/submodule.c                | 169 ++++++++
 ci/run-build-and-tests.sh          |   1 +
 git-sh-setup.sh                    |   7 -
 git-submodule.sh                   | 637 -----------------------------
 git.c                              |   1 +
 submodule.c                        |   2 +-
 t/README                           |   4 +
 12 files changed, 255 insertions(+), 699 deletions(-)
 create mode 100644 builtin/submodule.c
 delete mode 100755 git-submodule.sh

1. GIT_TEST_SUBMODULE_USE_BUILTIN=true git hyperfine -L rev origin/master,HEAD~0 -L v false,true -s 'make CFLAGS=-O3' 'GIT_TEST_SUBMODULE_USE_BUILTIN={v} ./git --exec-path=$PWD submodule status' -r 20 Benchmark 1: GIT_TEST_SUBMODULE_USE_BUILTIN=false ./git --exec-path=$PWD submodule status' in 'origin/master

  Time (mean ± σ):      40.9 ms ±   0.3 ms    [User: 33.3 ms, System: 9.7 ms]
  Range (min … max):    40.2 ms …  41.5 ms    20 runs

Benchmark 2: GIT_TEST_SUBMODULE_USE_BUILTIN=false ./git --exec-path=$PWD submodule status' in 'HEAD~0

  Time (mean ± σ):      12.4 ms ±   0.1 ms    [User: 9.9 ms, System: 2.5 ms]
  Range (min … max):    12.2 ms …  12.7 ms    20 runs

Benchmark 3: GIT_TEST_SUBMODULE_USE_BUILTIN=true ./git --exec-path=$PWD submodule status' in 'origin/master

  Time (mean ± σ):      40.9 ms ±   0.5 ms    [User: 35.6 ms, System: 7.2 ms]
  Range (min … max):    40.1 ms …  41.8 ms    20 runs

Benchmark 4: GIT_TEST_SUBMODULE_USE_BUILTIN=true ./git --exec-path=$PWD submodule status' in 'HEAD~0
Time (mean ± σ): 6.4 ms ± 0.1 ms [User: 3.9 ms, System: 2.5 ms]
Range (min … max): 6.3 ms … 6.6 ms 20 runs

Summary
'GIT_TEST_SUBMODULE_USE_BUILTIN=true ./git --exec-path=$PWD submodule status' in 'HEAD~0' ran

    1.94 ± 0.03 times faster than 'GIT_TEST_SUBMODULE_USE_BUILTIN=false ./git --exec-path=$PWD submodule status' in 'HEAD~0'
    6.40 ± 0.11 times faster than 'GIT_TEST_SUBMODULE_USE_BUILTIN=true ./git --exec-path=$PWD submodule status' in 'origin/master'
    6.40 ± 0.10 times faster than 'GIT_TEST_SUBMODULE_USE_BUILTIN=false ./git --exec-path=$PWD submodule status' in 'origin/master'

2. Aside: I don't think these ever made it on-list but Atharva's version of what we're trying to do here is at:
https://github.com/tfidfwastaken/git/tree/submodule-make-builtin-2

I'd looked those over at some distant point in the past, and skimmed them again yesterday, but thought they were too much all-at-once to be confident in testing it myself, hence coming up with this alternate & smaller approach.

2.36.1.1178.gb5b1747c546

Remove the $prefix variable which is never set, and never resulted in an option being passed to our "git submodule--helper". The --recursive-prefix option to "git submodule--helper" is still used, but only when it invokes itself.

---
 git-submodule.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index bc436c4ca47..53847bbf6e2 100755
@@ -41,7 +41,6 @@ files=
 remote=
 nofetch=
 update=
-prefix=
 custom_name=
 depth=
 progress=

@@ -127,7 +126,7 @@ cmd_add()
 		usage
 	fi
 
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
 }
 
 #

@@ -189,7 +188,7 @@ cmd_init()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
 }
 
 #

@@ -346,7 +345,6 @@ cmd_update()
 		${init:+--init} \
 		${nofetch:+--no-fetch} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
-		${prefix:+--recursive-prefix "$prefix"} \
 		${update:+--update "$update"} \
 		${reference:+"$reference"} \
 		${dissociate:+"--dissociate"} \

2.36.1.1178.gb5b1747c546

In 5c08dbbdf1a (git-submodule: fix subcommand parser, 2008-01-15) the "--branch" option was supported as an option to "git submodule" itself, i.e. "git submodule --branch" as a side-effect of its implementation.

Then in b57e8119e6e (submodule: teach set-branch subcommand, 2019-02-08) when the "set-branch" subcommand was added the assertion that we shouldn't have "--branch" anywhere except as an argument to "add" and "set-branch" was copy/pasted from the adjacent check for "--cache" added (or rather modified) in 496eeeb19b9 (git-submodule.sh:
avoid "test <cond> -a/-o <cond>", 2014-06-10).

But there's been a logic error in that check, this looked like it should be supporting:

    git submodule --branch <branch> (add | set-branch) [<options>]

But due to "||" in the condition (as opposed to "&&" for "--cache") if we have "--branch" here already we'll emit usage, even for "add" and "set-branch".

Since nobody's complained about "--branch <branch>" not being supported as argument to "git submodule" itself, i.e. we want to support:

    git submodule (add | set-branch) --branch <branch>  [<options>]

But not the first form noted above. Let's just remove this code, we've never documented "--branch" as a top-level option (unlike "--quiet"), so this looks like it was an accident of the implementation, which we broke v2.22.0, so we also know it must not have been important to anyone.

---
 git-submodule.sh | 14 --------------
 1 file changed, 14 deletions(-)

index 53847bbf6e2..1b0a31b8383 100755
@@ -574,14 +574,6 @@ do
 	-q|--quiet)
 		GIT_QUIET=1
 		;;
-	-b|--branch)
-		case "$2" in
-		'')
-			usage
-			;;
-		esac
-		branch="$2"; shift
-		;;
 	--cached)
 		cached="$1"
 		;;

@@ -609,12 +601,6 @@ then
     fi
 fi
 
-# "-b branch" is accepted only by "add" and "set-branch"
-if test -n "$branch" && (test "$command" != add || test "$command" != set-branch)
-then
-	usage
-fi
-
 # "--cached" is accepted only by "status" and "summary"
 if test -n "$cached" && test "$command" != status && test "$command" != summary
 then

2.36.1.1178.gb5b1747c546

The "$prefix" variable has not been set since b3c5f5cb048 (submodule:
move core cmd_update() logic to C, 2022-03-15), so we'd never pass the --super-prefix option to "git submodule--helper", and can therefore remove the handling of it from builtin/submodule--helper.c as well.

---
 builtin/submodule--helper.c | 53 +++++++++++++++----------------------
 1 file changed, 22 insertions(+), 31 deletions(-)

index c6c2ba1b6dc..70693205764 100644
@@ -3364,35 +3364,32 @@ static int module_add(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-#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[] = {
-	{"list", module_list, 0},
-	{"name", module_name, 0},
-	{"clone", module_clone, 0},
-	{"add", module_add, SUPPORT_SUPER_PREFIX},
-	{"update", module_update, 0},
-	{"resolve-relative-url-test", resolve_relative_url_test, 0},
-	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
-	{"init", module_init, SUPPORT_SUPER_PREFIX},
-	{"status", module_status, SUPPORT_SUPER_PREFIX},
-	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
-	{"deinit", module_deinit, 0},
-	{"summary", module_summary, SUPPORT_SUPER_PREFIX},
-	{"push-check", push_check, 0},
-	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
-	{"is-active", is_active, 0},
-	{"check-name", check_name, 0},
-	{"config", module_config, 0},
-	{"set-url", module_set_url, 0},
-	{"set-branch", module_set_branch, 0},
-	{"create-branch", module_create_branch, 0},
+	{"list", module_list},
+	{"name", module_name},
+	{"clone", module_clone},
+	{"add", module_add},
+	{"update", module_update},
+	{"resolve-relative-url-test", resolve_relative_url_test},
+	{"foreach", module_foreach},
+	{"init", module_init},
+	{"status", module_status},
+	{"sync", module_sync},
+	{"deinit", module_deinit},
+	{"summary", module_summary},
+	{"push-check", push_check},
+	{"absorb-git-dirs", absorb_git_dirs},
+	{"is-active", is_active},
+	{"check-name", check_name},
+	{"config", module_config},
+	{"set-url", module_set_url},
+	{"set-branch", module_set_branch},
+	{"create-branch", module_create_branch},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)

@@ -3401,15 +3398,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 	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);
+	for (i = 0; i < ARRAY_SIZE(commands); i++)
+		if (!strcmp(argv[1], commands[i].cmd))
 			return commands[i].fn(argc - 1, argv + 1, prefix);
-		}
-	}
 
 	die(_("'%s' is not a valid submodule--helper "
 	      "subcommand"), argv[1]);

2.36.1.1178.gb5b1747c546

Remove the assignment of "$1" to the "$cached" variable. As seen in the initial implementation in 70c7ac22de6 (Add git-submodule command, 2007-05-26) we only need to keep track of if we've seen the --cached option. In 28f9af5d25e (git-submodule summary: code framework, 2008-03-11) "$1" was assigned to it, but there was no reason to do so. Let's undo that.

---
 git-submodule.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 1b0a31b8383..20fc1b620fa 100755
@@ -446,7 +446,7 @@ cmd_summary() {
 	do
 		case "$1" in
 		--cached)
-			cached="$1"
+			cached=1
 			;;
 		--files)
 			files="$1"

@@ -575,7 +575,7 @@ do
 		GIT_QUIET=1
 		;;
 	--cached)
-		cached="$1"
+		cached=1
 		;;
 	--)
 		break

2.36.1.1178.gb5b1747c546

Rename the "absorb-git-dirs" subcommand to "absorbgitdirs", which is what the "git submodule" command itself has called it since the subcommand was implemented in f6f85861400 (submodule: add absorb-git-dir function, 2016-12-12).

Having these two be different will make it more tedious to dispatch to "git submodule--helper" directly, so let's get rid of this needless inconsistency.

---
 builtin/submodule--helper.c | 4 ++--
 git-submodule.sh            | 2 +-
 submodule.c                 | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

index 70693205764..8df1541baf8 100644
@@ -2784,7 +2784,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper absorb-git-dirs [<options>] [<path>...]"),
+		N_("git submodule--helper absorbgitdirs [<options>] [<path>...]"),
 		NULL
 	};
 

@@ -3383,7 +3383,7 @@ static struct cmd_struct commands[] = {
 	{"deinit", module_deinit},
 	{"summary", module_summary},
 	{"push-check", push_check},
-	{"absorb-git-dirs", absorb_git_dirs},
+	{"absorbgitdirs", absorb_git_dirs},
 	{"is-active", is_active},
 	{"check-name", check_name},
 	{"config", module_config},

index 20fc1b620fa..70258eaf38e 100755
@@ -556,7 +556,7 @@ cmd_sync()
 
 cmd_absorbgitdirs()
 {
-	git submodule--helper absorb-git-dirs --prefix "$wt_prefix" "$@"
+	git submodule--helper absorbgitdirs --prefix "$wt_prefix" "$@"
 }
 
 # This loop parses the command line arguments to find the

index 7923e951e1c..9129cd564c8 100644
@@ -2374,7 +2374,7 @@ void absorb_git_dir_into_superproject(const char *path,
 		cp.no_stdin = 1;
 		strvec_pushl(&cp.args, "--super-prefix", sb.buf,
 			     "submodule--helper",
-			     "absorb-git-dirs", NULL);
+			     "absorbgitdirs", NULL);
 		prepare_submodule_repo_env(&cp.env_array);
 		if (run_command(&cp))
 			die(_("could not recurse into submodule '%s'"), path);

2.36.1.1178.gb5b1747c546

Create a "case/esac" dispatch statement at the end of git-submodule.sh and move the contents of the trivial cmd_absorbgitdirs() function to it.

This template will be expanded on in subsequent commits, but for now we're moving the trivial "git submodule absorb-git-dirs" to it.

---
 git-submodule.sh | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

index 70258eaf38e..fb13f818c63 100755
@@ -554,11 +554,6 @@ cmd_sync()
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@"
 }
 
-cmd_absorbgitdirs()
-{
-	git submodule--helper absorbgitdirs --prefix "$wt_prefix" "$@"
-}
-
 # This loop parses the command line arguments to find the
 # subcommand name to dispatch.  Parsing of the subcommand specific
 # options are primarily done by the subcommand implementations.

@@ -607,4 +602,11 @@ then
 	usage
 fi
 
-"cmd_$(echo $command | sed -e s/-/_/g)" "$@"
+case "$command" in
+absorbgitdirs)
+	git submodule--helper "$command" --prefix "$wt_prefix" "$@"
+	;;
+*)
+	"cmd_$(echo $command | sed -e s/-/_/g)" "$@"
+	;;
+esac

2.36.1.1178.gb5b1747c546

Remove the cmd_sync() wrapper for "git submodule--helper sync" in favor of dispatching the raw command-line directly to the helper.

At this point we've already parsed out the optional "--quiet" flag that we need to support for "git submodule --quiet" (as opposed to "git submodule <subcommand> --quiet").

This changes the output we'll display on invalid usage for the better, before this we'd emit e.g.:

	$ git submodule sync --blah
	usage: git submodule [--quiet] [--cached]
	   or: [...many lines of "or" usage omitted...]

But now we'll emit the much more useful:

	$ git submodule sync --blah
	error: unknown option `blah'
	usage: git submodule sync [--quiet] [--recursive] [<path>]

-q, --quiet suppress output of synchronizing submodule url
--recursive recurse into nested submodules

This is because we'll now get as far as module_sync()'s failing call to parse_options() when we have invalid usage.

---
 git-submodule.sh | 37 ++++---------------------------------
 1 file changed, 4 insertions(+), 33 deletions(-)

index fb13f818c63..ddc2e4e4032 100755
@@ -520,39 +520,6 @@ cmd_status()
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- "$@"
 }
-#
-# Sync remote urls for submodules
-# This makes the value for remote.$remote.url match the value
-# specified in .gitmodules.
-#
-cmd_sync()
-{
-	while test $# -ne 0
-	do
-		case "$1" in
-		-q|--quiet)
-			GIT_QUIET=1
-			shift
-			;;
-		--recursive)
-			recursive=1
-			shift
-			;;
-		--)
-			shift
-			break
-			;;
-		-*)
-			usage
-			;;
-		*)
-			break
-			;;
-		esac
-	done
-
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@"
-}
 
 # This loop parses the command line arguments to find the
 # subcommand name to dispatch.  Parsing of the subcommand specific

@@ -606,6 +573,10 @@ case "$command" in
 absorbgitdirs)
 	git submodule--helper "$command" --prefix "$wt_prefix" "$@"
 	;;
+sync)
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper "$command" \
+		${GIT_QUIET:+--quiet} "$@"
+	;;
 *)
 	"cmd_$(echo $command | sed -e s/-/_/g)" "$@"
 	;;

2.36.1.1178.gb5b1747c546

Change the usage output for "git submodule--helper" so that it will pretend to be named "git submodule". This will allow us to dispatch further into "git submodule--helper" from git-submodule.sh while emitting the correct usage output.

This fixes a bug that's been there ever since "git submodule absorbgitdirs" was added in f6f85861400 (submodule: add absorb-git-dir function, 2016-12-12). Before this we'd emit e.g. this on invalid usage:

	$ git submodule absorbgitdirs --blah
	error: unknown option `blah'
	usage: git submodule--helper absorbgitdirs [<options>] [<path>...]

Now we'll emit the same, except that the usage line is now referring to the correct command run by the user:

	usage: git submodule absorbgitdirs [<options>] [<path>...]
	[.. same usage output ...]

Note that in the case of "status", "deinit" and "update" we were already referring to ourselves as "git submodule" in builtin/submodule--helper.c.

---
 builtin/submodule--helper.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

index 8df1541baf8..3173295ed8e 100644
@@ -286,7 +286,7 @@ static int module_list(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper list [--prefix=<path>] [<path>...]"),
+		N_("git submodule list [--prefix=<path>] [<path>...]"),
 		NULL
 	};
 

@@ -444,7 +444,7 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper foreach [--quiet] [--recursive] [--] <command>"),
+		N_("git submodule foreach [--quiet] [--recursive] [--] <command>"),
 		NULL
 	};
 

@@ -582,7 +582,7 @@ static int module_init(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper init [<options>] [<path>]"),
+		N_("git submodule init [<options>] [<path>]"),
 		NULL
 	};
 

@@ -786,7 +786,7 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	const struct submodule *sub;
 
 	if (argc != 2)
-		usage(_("git submodule--helper name <path>"));
+		usage(_("git submodule name <path>"));
 
 	sub = submodule_from_path(the_repository, null_oid(), argv[1]);
 

@@ -1185,7 +1185,7 @@ static int module_summary(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper summary [<options>] [<commit>] [--] [<path>]"),
+		N_("git submodule summary [<options>] [<commit>] [--] [<path>]"),
 		NULL
 	};
 

@@ -1349,7 +1349,7 @@ static int module_sync(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper sync [--quiet] [--recursive] [<path>]"),
+		N_("git submodule sync [--quiet] [--recursive] [<path>]"),
 		NULL
 	};
 

@@ -1789,7 +1789,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
+		N_("git submodule clone [--prefix=<path>] [--quiet] "
 		   "[--reference <repository>] [--name <name>] [--depth <depth>] "
 		   "[--single-branch] [--filter <filter-spec>] "
 		   "--url <url> --path <path>"),

@@ -2784,7 +2784,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper absorbgitdirs [<options>] [<path>...]"),
+		N_("git submodule absorbgitdirs [<options>] [<path>...]"),
 		NULL
 	};
 

@@ -2848,9 +2848,9 @@ static int module_config(int argc, const char **argv, const char *prefix)
 		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",
+		N_("git submodule config <name> [<value>]"),
+		N_("git submodule config --unset <name>"),
+		"git submodule config --check-writeable",
 		NULL
 	};
 

@@ -2889,7 +2889,7 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 	const char *const usage[] = {
-		N_("git submodule--helper set-url [--quiet] <path> <newurl>"),
+		N_("git submodule set-url [--quiet] <path> <newurl>"),
 		NULL
 	};
 

@@ -2928,8 +2928,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 	const char *const usage[] = {
-		N_("git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>"),
-		N_("git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
+		N_("git submodule set-branch [-q|--quiet] (-d|--default) <path>"),
+		N_("git submodule set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
 		NULL
 	};
 

@@ -2970,7 +2970,7 @@ static int module_create_branch(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 	const char *const usage[] = {
-		N_("git submodule--helper create-branch [-f|--force] [--create-reflog] [-q|--quiet] [-t|--track] [-n|--dry-run] <name> <start-oid> <start-name>"),
+		N_("git submodule create-branch [-f|--force] [--create-reflog] [-q|--quiet] [-t|--track] [-n|--dry-run] <name> <start-oid> <start-name>"),
 		NULL
 	};
 

@@ -3273,7 +3273,7 @@ static int module_add(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const usage[] = {
-		N_("git submodule--helper add [<options>] [--] <repository> [<path>]"),
+		N_("git submodule add [<options>] [--] <repository> [<path>]"),
 		NULL
 	};
 

2.36.1.1178.gb5b1747c546

Change the dispatching logic in "git-submodule.sh" for the "add", "init", "deinit", "set-branch", "set-url", "summary" and "status" sub-commands to do away with the argument parsing in git-submodule.sh, and instead dispatch directly to "git submodule--helper".

As in a preceding commit the only functional change here should be that on invalid options we'll now emit more targeted "-h" output.

The isnumber() helper function was only used in the now-removed cmd_summary(), the same goes for the $files variable. The $custom_name and $branch variables were only used in cmd_add().

Since there are no dashed commands anymore in git-submodule.sh we can get rid of the "$(echo | sed ...)" one-liner to change e.g. "set-url" to "set_url".

---
 git-submodule.sh | 321 +----------------------------------------------
 1 file changed, 5 insertions(+), 316 deletions(-)

index ddc2e4e4032..61e4f17b805 100755
@@ -30,18 +30,15 @@ GIT_PROTOCOL_FROM_USER=0
 export GIT_PROTOCOL_FROM_USER
 
 command=
-branch=
 force=
 reference=
 cached=
 recursive=
 init=
 require_init=
-files=
 remote=
 nofetch=
 update=
-custom_name=
 depth=
 progress=
 dissociate=

@@ -50,86 +47,7 @@ jobs=
 recommend_shallow=
 filter=
 
-isnumber()
-{
-	n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
-}
-
-#
-# Add a new submodule to the working tree, .gitmodules and the index
-#
-# $@ = repo path
-#
-# optional branch is stored in global branch variable
-#
-cmd_add()
-{
-	# parse $args after "submodule ... add".
-	reference_path=
-	while test $# -ne 0
-	do
-		case "$1" in
-		-b | --branch)
-			case "$2" in '') usage ;; esac
-			branch=$2
-			shift
-			;;
-		-f | --force)
-			force=$1
-			;;
-		-q|--quiet)
-			GIT_QUIET=1
-			;;
-		--progress)
-			progress=1
-			;;
-		--reference)
-			case "$2" in '') usage ;; esac
-			reference_path=$2
-			shift
-			;;
-		--reference=*)
-			reference_path="${1#--reference=}"
-			;;
-		--dissociate)
-			dissociate=1
-			;;
-		--name)
-			case "$2" in '') usage ;; esac
-			custom_name=$2
-			shift
-			;;
-		--depth)
-			case "$2" in '') usage ;; esac
-			depth="--depth=$2"
-			shift
-			;;
-		--depth=*)
-			depth=$1
-			;;
-		--)
-			shift
-			break
-			;;
-		-*)
-			usage
-			;;
-		*)
-			break
-			;;
-		esac
-		shift
-	done
-
-	if test -z "$1"
-	then
-		usage
-	fi
-
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
-}
 
-#
 # Execute an arbitrary command sequence in each checked out
 # submodule
 #

@@ -160,73 +78,6 @@ cmd_foreach()
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@"
 }
 
-#
-# Register submodules in .git/config
-#
-# $@ = requested paths (default to all)
-#
-cmd_init()
-{
-	# parse $args after "submodule ... init".
-	while test $# -ne 0
-	do
-		case "$1" in
-		-q|--quiet)
-			GIT_QUIET=1
-			;;
-		--)
-			shift
-			break
-			;;
-		-*)
-			usage
-			;;
-		*)
-			break
-			;;
-		esac
-		shift
-	done
-
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
-}
-
-#
-# Unregister submodules from .git/config and remove their work tree
-#
-cmd_deinit()
-{
-	# parse $args after "submodule ... deinit".
-	deinit_all=
-	while test $# -ne 0
-	do
-		case "$1" in
-		-f|--force)
-			force=$1
-			;;
-		-q|--quiet)
-			GIT_QUIET=1
-			;;
-		--all)
-			deinit_all=t
-			;;
-		--)
-			shift
-			break
-			;;
-		-*)
-			usage
-			;;
-		*)
-			break
-			;;
-		esac
-		shift
-	done
-
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- "$@"
-}
-
 #
 # Update each submodule path to correct revision, using clone and checkout as needed
 #

@@ -359,168 +210,6 @@ cmd_update()
 		"$@"
 }
 
-#
-# Configures a submodule's default branch
-#
-# $@ = requested path
-#
-cmd_set_branch() {
-	default=
-	branch=
-
-	while test $# -ne 0
-	do
-		case "$1" in
-		-q|--quiet)
-			# we don't do anything with this but we need to accept it
-			;;
-		-d|--default)
-			default=1
-			;;
-		-b|--branch)
-			case "$2" in '') usage ;; esac
-			branch=$2
-			shift
-			;;
-		--)
-			shift
-			break
-			;;
-		-*)
-			usage
-			;;
-		*)
-			break
-			;;
-		esac
-		shift
-	done
-
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch "$branch"} ${default:+--default} -- "$@"
-}
-
-#
-# Configures a submodule's remote url
-#
-# $@ = requested path, requested url
-#
-cmd_set_url() {
-	while test $# -ne 0
-	do
-		case "$1" in
-		-q|--quiet)
-			GIT_QUIET=1
-			;;
-		--)
-			shift
-			break
-			;;
-		-*)
-			usage
-			;;
-		*)
-			break
-			;;
-		esac
-		shift
-	done
-
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- "$@"
-}
-
-#
-# Show commit summary for submodules in index or working tree
-#
-# If '--cached' is given, show summary between index and given commit,
-# or between working tree and given commit
-#
-# $@ = [commit (default 'HEAD'),] requested paths (default all)
-#
-cmd_summary() {
-	summary_limit=-1
-	for_status=
-	diff_cmd=diff-index
-
-	# parse $args after "submodule ... summary".
-	while test $# -ne 0
-	do
-		case "$1" in
-		--cached)
-			cached=1
-			;;
-		--files)
-			files="$1"
-			;;
-		--for-status)
-			for_status="$1"
-			;;
-		-n|--summary-limit)
-			summary_limit="$2"
-			isnumber "$summary_limit" || usage
-			shift
-			;;
-		--summary-limit=*)
-			summary_limit="${1#--summary-limit=}"
-			isnumber "$summary_limit" || usage
-			;;
-		--)
-			shift
-			break
-			;;
-		-*)
-			usage
-			;;
-		*)
-			break
-			;;
-		esac
-		shift
-	done
-
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper summary ${files:+--files} ${cached:+--cached} ${for_status:+--for-status} ${summary_limit:+-n $summary_limit} -- "$@"
-}
-#
-# List all submodules, prefixed with:
-#  - submodule not initialized
-#  + different revision checked out
-#
-# If --cached was specified the revision in the index will be printed
-# instead of the currently checked out revision.
-#
-# $@ = requested paths (default to all)
-#
-cmd_status()
-{
-	# parse $args after "submodule ... status".
-	while test $# -ne 0
-	do
-		case "$1" in
-		-q|--quiet)
-			GIT_QUIET=1
-			;;
-		--cached)
-			cached=1
-			;;
-		--recursive)
-			recursive=1
-			;;
-		--)
-			shift
-			break
-			;;
-		-*)
-			usage
-			;;
-		*)
-			break
-			;;
-		esac
-		shift
-	done
-
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- "$@"
-}
-
 # This loop parses the command line arguments to find the
 # subcommand name to dispatch.  Parsing of the subcommand specific
 # options are primarily done by the subcommand implementations.

@@ -573,11 +262,11 @@ case "$command" in
 absorbgitdirs)
 	git submodule--helper "$command" --prefix "$wt_prefix" "$@"
 	;;
-sync)
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper "$command" \
-		${GIT_QUIET:+--quiet} "$@"
+foreach | update)
+	"cmd_$command" "$@"
 	;;
-*)
-	"cmd_$(echo $command | sed -e s/-/_/g)" "$@"
+add | init | deinit | set-branch | set-url | status | summary | sync)
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper "$command" \
+		${GIT_QUIET:+--quiet} ${cached:+--cached} "$@"
 	;;
 esac

2.36.1.1178.gb5b1747c546

Adjust code added in 0060fd1511b (clone --recurse-submodules: prevent name squatting on Windows, 2019-09-12) to have the internal --require-init option imply --init, rather than having "git-submodule.sh" add it implicitly.

---
 builtin/submodule--helper.c | 5 ++++-
 git-submodule.sh            | 1 -
 2 files changed, 4 insertions(+), 2 deletions(-)

index 459260efc65..f1f5c6b84c6 100644
@@ -2619,7 +2619,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "progress", &opt.progress,
 			    N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &opt.require_init,
-			   N_("disallow cloning into non-empty directory")),
+			   N_("disallow cloning into non-empty directory, implies --init")),
 		OPT_BOOL(0, "single-branch", &opt.single_branch,
 			 N_("clone only one branch, HEAD or --branch")),
 		OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),

@@ -2643,6 +2643,9 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_update_options,
 			     git_submodule_helper_usage, 0);
 
+	if (opt.require_init)
+		opt.init = 1;
+
 	if (filter_options.choice && !opt.init) {
 		usage_with_options(git_submodule_helper_usage,
 				   module_update_options);

index de6c71d6bd5..43fe3bcb00b 100755
@@ -71,7 +71,6 @@ cmd_update()
 			init=1
 			;;
 		--require-init)
-			init=1
 			require_init=1
 			;;
 		--remote)

2.36.1.1178.gb5b1747c546

Dispatch the "git submodule foreach" command directly to "git submodule--helper foreach". This case requires the addition of the PARSE_OPT_STOP_AT_NON_OPTION flag, since the shellscript was unconditionally adding "--" to the "git submodule--helper" command-line.

---
 builtin/submodule--helper.c |  3 ++-
 git-submodule.sh            | 37 +++----------------------------------
 2 files changed, 5 insertions(+), 35 deletions(-)

index 3173295ed8e..459260efc65 100644
@@ -449,7 +449,8 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
 	};
 
 	argc = parse_options(argc, argv, prefix, module_foreach_options,
-			     git_submodule_helper_usage, 0);
+			     git_submodule_helper_usage,
+			     PARSE_OPT_STOP_AT_NON_OPTION);
 
 	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
 		return 1;

index 61e4f17b805..de6c71d6bd5 100755
@@ -47,37 +47,6 @@ jobs=
 recommend_shallow=
 filter=
 
-
-# Execute an arbitrary command sequence in each checked out
-# submodule
-#
-# $@ = command to execute
-#
-cmd_foreach()
-{
-	# parse $args after "submodule ... foreach".
-	while test $# -ne 0
-	do
-		case "$1" in
-		-q|--quiet)
-			GIT_QUIET=1
-			;;
-		--recursive)
-			recursive=1
-			;;
-		-*)
-			usage
-			;;
-		*)
-			break
-			;;
-		esac
-		shift
-	done
-
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@"
-}
-
 #
 # Update each submodule path to correct revision, using clone and checkout as needed
 #

@@ -262,10 +231,10 @@ case "$command" in
 absorbgitdirs)
 	git submodule--helper "$command" --prefix "$wt_prefix" "$@"
 	;;
-foreach | update)
-	"cmd_$command" "$@"
+update)
+	cmd_update "$@"
 	;;
-add | init | deinit | set-branch | set-url | status | summary | sync)
+add | foreach | init | deinit | set-branch | set-url | status | summary | sync)
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper "$command" \
 		${GIT_QUIET:+--quiet} ${cached:+--cached} "$@"
 	;;

2.36.1.1178.gb5b1747c546

Change the "OPT__QUIET()" in module_update() to use OPT__VERBOSITY() instead. This is in preparation for passing the command in "git-submodule.sh" to "git submodule update" directly.

Note that this implementation supports the "--verbose" synonym for "-v", but "git-submodule.sh" does not. Once we make the switch to calling "git submodule--helper update" directly we should update the documentation to reflect that.

---
 builtin/submodule--helper.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

index 64d7c5d7ba4..94126bcca07 100644
@@ -2584,6 +2584,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	struct list_objects_filter_options filter_options;
 	int ret;
 	enum submodule_update_type update_type = SM_UPDATE_UNSPECIFIED;
+	int verbosity = 0;
 
 	struct option module_update_options[] = {
 		OPT__FORCE(&opt.force, N_("force checkout updates"), 0),

@@ -2625,7 +2626,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			    N_("parallel jobs")),
 		OPT_BOOL(0, "recommend-shallow", &opt.recommend_shallow,
 			    N_("whether the initial clone should follow the shallow recommendation")),
-		OPT__QUIET(&opt.quiet, N_("don't print cloning progress")),
+		OPT__VERBOSITY(&verbosity),
 		OPT_BOOL(0, "progress", &opt.progress,
 			    N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &opt.require_init,

@@ -2637,7 +2638,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule [--quiet] update"
+		N_("git submodule [--quiet] update [-v | --verbose]"
 		" [--init [--filter=<filter-spec>]] [--remote]"
 		" [-N|--no-fetch] [-f|--force]"
 		" [--checkout|--merge|--rebase]"

@@ -2656,6 +2657,9 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	if (opt.require_init)
 		opt.init = 1;
 
+	if (verbosity < 0)
+		opt.quiet = 1;
+
 	if (filter_options.choice && !opt.init) {
 		usage_with_options(git_submodule_helper_usage,
 				   module_update_options);

2.36.1.1178.gb5b1747c546

In e84c3cf3dc3 (git-submodule.sh: accept verbose flag in cmd_update to be non-quiet, 2018-08-14) the "git submodule update" sub-command was made to understand "-v" for "don't be quiet" (but not "--verbose"). Let's update the documentation to reflect this.

---
 Documentation/git-submodule.txt | 7 +++++--
 git-submodule.sh                | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

index 4d3ab6b9f92..5f1c552110e 100644
@@ -14,7 +14,7 @@ SYNOPSIS
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
 'git submodule' [--quiet] init [--] [<path>...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] <path>...)
-'git submodule' [--quiet] update [<options>] [--] [<path>...]
+'git submodule' [--quiet] update [-v] [<options>] [--] [<path>...]
 'git submodule' [--quiet] set-branch [<options>] [--] <path>
 'git submodule' [--quiet] set-url [--] <path> <newurl>
 'git submodule' [--quiet] summary [<options>] [--] [<path>...]

@@ -133,7 +133,7 @@ If you really want to remove a submodule from the repository and commit
 that use linkgit:git-rm[1] instead. See linkgit:gitsubmodules[7] for removal
 options.
 
-update [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference <repository>] [--depth <depth>] [--recursive] [--jobs <n>] [--[no-]single-branch] [--filter <filter spec>] [--] [<path>...]::
+update [-v] [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference <repository>] [--depth <depth>] [--recursive] [--jobs <n>] [--[no-]single-branch] [--filter <filter spec>] [--] [<path>...]::
 +
 --
 Update the registered submodules to match what the superproject

@@ -270,6 +270,9 @@ OPTIONS
 --quiet::
 	Only print error messages.
 
+-v::
+	Don't be quiet. This option is only valid for the update command.
+
 --progress::
 	This option is only valid for add and update commands.
 	Progress status is reported on the standard error stream

index d8abdd8e8ad..1929c34ae8b 100755
@@ -10,7 +10,7 @@ USAGE="[--quiet] [--cached]
    or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] init [--] [<path>...]
    or: $dashless [--quiet] deinit [-f|--force] (--all| [--] <path>...)
-   or: $dashless [--quiet] update [--init [--filter=<filter-spec>]] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--[no-]single-branch] [--] [<path>...]
+   or: $dashless [--quiet] update [-v] [--init [--filter=<filter-spec>]] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--[no-]single-branch] [--] [<path>...]
    or: $dashless [--quiet] set-branch (--default|--branch <branch>) [--] <path>
    or: $dashless [--quiet] set-url [--] <path> <newurl>
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]

2.36.1.1178.gb5b1747c546

Simplify the parsing loop so that we don't try to find out the "$command" when looping over arguments initially, we'll only look for the --quiet and --cached options.

Then if we have no more arguments we default to "status", otherwise we emit our usage info. Most importantly we don't need to give --cache to only "status" and "summary", instead we trust that "submodule--helper" is checking those arguments, if some subcommands don't them they can emit their own errors.

---
 git-submodule.sh | 44 +++++++++++++++++---------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

index cad9948c047..0274b253297 100755
@@ -29,50 +29,40 @@ cd_to_toplevel
 GIT_PROTOCOL_FROM_USER=0
 export GIT_PROTOCOL_FROM_USER
 
-command=
 quiet=
 cached=
 
-while test $# != 0 && test -z "$command"
+while test $# != 0
 do
 	case "$1" in
-	add | foreach | init | deinit | update | set-branch | set-url | status | summary | sync | absorbgitdirs)
-		command=$1
-		;;
 	-q|--quiet)
-		quiet=1
+		quiet=1 &&
+		shift
 		;;
 	--cached)
-		cached=1
-		;;
-	--)
-		break
-		;;
-	-*)
-		usage
+		cached=1 &&
+		shift
 		;;
 	*)
 		break
 		;;
 	esac
-	shift
 done
 
 # No command word defaults to "status"
-if test -z "$command"
+command=
+if test $# = 0
 then
-    if test $# = 0
-    then
 	command=status
-    else
-	usage
-    fi
-fi
-
-# "--cached" is accepted only by "status" and "summary"
-if test -n "$cached" && test "$command" != status && test "$command" != summary
-then
-	usage
+else
+	case "$1" in
+	add | foreach | init | deinit | update | set-branch | set-url | status | summary | sync | absorbgitdirs)
+		command=$1 &&
+		shift
+		;;
+	*)
+		usage
+	esac
 fi
 
 case "$command" in

@@ -83,7 +73,7 @@ update)
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper "$command" \
 		${quiet:+--quiet} ${wt_prefix:+--prefix "$wt_prefix"} "$@"
 	;;
-add | foreach | init | deinit | set-branch | set-url | status | summary | sync)
+*)
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper "$command" \
 		${quiet:+--quiet} ${cached:+--cached} "$@"
 	;;

2.36.1.1178.gb5b1747c546

Add an experimental setting to avoid the subprocess invocation of "git submodule--helper", instead we'll call cmd_submodule__helper() directly.

---
 Documentation/config/submodule.txt |  4 ++++
 builtin/submodule.c                | 35 ++++++++++++++++++++++++++++--
 ci/run-build-and-tests.sh          |  1 +
 t/README                           |  4 ++++
 4 files changed, 42 insertions(+), 2 deletions(-)

index 6490527b45b..aa761f331c9 100644
@@ -79,6 +79,10 @@ setting.
 * `branch` is supported only if `submodule.propagateBranches` is
 enabled
 
+submodule.useBuiltin::
+	Set to `true` to use a faster but possibly less stable subprocess-less
+	implementation of linkgit:git-submodule[1]. Is `false` by default.
+
 submodule.propagateBranches::
 	[EXPERIMENTAL] A boolean that enables branching support when
 	using `--recurse-submodules` or `submodule.recurse=true`.

index b777787169e..9bb731ce87a 100644
@@ -6,6 +6,7 @@
 #include "parse-options.h"
 #include "run-command.h"
 #include "strvec.h"
+#include "config.h"
 
 #define BUILTIN_SUBMODULE_USAGE \
 	"git submodule [--quiet] [--cached]"

@@ -105,17 +106,33 @@ static void setup_helper_args(int argc, const char **argv, const char *prefix,
 	strvec_pushv(args, argv);
 }
 
+static int get_use_builtin(void)
+{
+	int v;
+
+	if (git_env_bool("GIT_TEST_SUBMODULE_USE_BUILTIN", 0))
+		v = 1;
+	else if (!git_config_get_bool("submodule.usebuiltin", &v))
+		;
+	else if (!git_config_get_bool("feature.experimental", &v))
+	      ;
+
+	return v;
+}
+
 int cmd_submodule(int argc, const char **argv, const char *prefix)
 {
 	int opt_quiet = 0;
 	int opt_cached = 0;
 	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strvec args = STRVEC_INIT;
 	struct option options[] = {
 		OPT__QUIET(&opt_quiet, N_("be quiet")),
 		OPT_BOOL(0, "cached", &opt_cached,
 			 N_("print the OID of submodules")),
 		OPT_END()
 	};
+	const int use_builtin = get_use_builtin();
 	int ret;
 
 	argc = parse_options(argc, argv, prefix, options, git_submodule_usage,

@@ -125,14 +142,28 @@ int cmd_submodule(int argc, const char **argv, const char *prefix)
 	 * Tell the rest of git that any URLs we get don't come
 	 * directly from the user, so it can apply policy as appropriate.
 	 */
-	strvec_push(&cp.env_array, "GIT_PROTOCOL_FROM_USER=0");
+	if (use_builtin)
+		xsetenv("GIT_PROTOCOL_FROM_USER", "0", 1);
+	else
+		strvec_push(&cp.env_array, "GIT_PROTOCOL_FROM_USER=0");
+
 	setup_helper_args(argc, argv, prefix, opt_quiet, opt_cached,
-			  &cp.args);
+			  use_builtin ? &args : &cp.args);
+
+	if (use_builtin) {
+		ret = cmd_submodule__helper(args.nr, args.v, prefix);
+		goto cleanup;
+	}
 
 	cp.git_cmd = 1;
 	cp.no_stdin = 0; /* for git submodule foreach */
 	cp.dir = startup_info->original_cwd;
 	ret = run_command(&cp);
 
+cleanup:
+	if (!use_builtin)
+		/* TODO: Double free? */
+		strvec_clear(&args);
+
 	return ret;
 }

index 8ebff425967..06d40fc94aa 100755
@@ -27,6 +27,7 @@ linux-TEST-vars)
 	export GIT_TEST_MULTI_PACK_INDEX=1
 	export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1
 	export GIT_TEST_ADD_I_USE_BUILTIN=0
+	export GIT_TEST_SUBMODULE_USE_BUILTIN=1
 	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
 	export GIT_TEST_WRITE_REV_INDEX=1
 	export GIT_TEST_CHECKOUT_WORKERS=2

index 309a31133c6..20c2e74723d 100644
@@ -423,6 +423,10 @@ GIT_TEST_ADD_I_USE_BUILTIN=<boolean>, when false, disables the
 built-in version of git add -i. See 'add.interactive.useBuiltin' in
 git-config(1).
 
+GIT_TEST_SUBMODULE_USE_BUILTIN=<boolean>, when true, enables the
+built-in subprocess-less invocation of "git submodule--helper".
+See 'submodule.useBuiltin' in git-config(1).
+
 GIT_TEST_INDEX_THREADS=<n> enables exercising the multi-threaded loading
 of the index for the whole test suite by bypassing the default number of
 cache entries and thread minimums. Setting this to 1 will make the

2.36.1.1178.gb5b1747c546

Replace the now-trivial git-submodule.sh script with a built-in builtin/submodule.c. For now this new command is only a dumb dispatcher that uses run-command.c to invoke "git submodule--helper", just as "git-submodule.sh" used to do.

This is obviously not ideal, and we should follow-up and merge the builtin/submodule--helper.c code into builtin/submodule.c, but doing it this way makes it easy to review that this new C implementation isn't doing anything more clever than the old shellscript implementation.

The "define BUILTIN_" macros will help with that, i.e. the usage information we emit can be merged with what builtin/submodule--helper.c is now emitting. See 8757b35d443 (commit-graph: define common usage with a macro, 2021-08-23) and 1e91d3faf6c (reflog: move "usage" variables and use macros, 2022-03-17) for prior art using this pattern.

The "(argc < 2 || !strcmp(argv[1], "-h"))" path at the top of cmd_submodule__helper() could now be a "(argc < 2)" if not for t0012-help.sh (which invokes all built-ins manually with "-h"). Let's leave it for now, eventually we'll consolidate the two.

---
 Makefile            |   2 +-
 builtin.h           |   1 +
 builtin/submodule.c | 138 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh    |  80 -------------------------
 git.c               |   1 +
 5 files changed, 141 insertions(+), 81 deletions(-)
 create mode 100644 builtin/submodule.c
 delete mode 100755 git-submodule.sh

index 790382bd1e3..635ab791a8a 100644
@@ -622,7 +622,6 @@ SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
 SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-request-pull.sh
-SCRIPT_SH += git-submodule.sh
 SCRIPT_SH += git-web--browse.sh
 
 SCRIPT_LIB += git-mergetool--lib

@@ -1213,6 +1212,7 @@ BUILTIN_OBJS += builtin/show-ref.o
 BUILTIN_OBJS += builtin/sparse-checkout.o
 BUILTIN_OBJS += builtin/stash.o
 BUILTIN_OBJS += builtin/stripspace.o
+BUILTIN_OBJS += builtin/submodule.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
 BUILTIN_OBJS += builtin/tag.o

index 40e9ecc8485..c8cf4a2ae3b 100644
@@ -223,6 +223,7 @@ 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_switch(int argc, const char **argv, const char *prefix);
 int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);

index 5ff4f3e25b7..047fc262cd2 100644
@@ -609,6 +609,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", cmd_submodule, RUN_SETUP | NEED_WORK_TREE },
 	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },
 	{ "switch", cmd_switch, RUN_SETUP | NEED_WORK_TREE },
 	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },

2.36.1.1178.gb5b1747c546

Remove the use of the "$GIT_QUIET" variable in favor of our own "$quiet", ever since b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15) we have not used the "say" function in git-sh-setup.sh, which is the only thing that's affected by using "GIT_QUIET".

We still want to support --quiet for our own use though, but let's use our own variable for that.

This change could have come before the preceding commits which removed cmd_*() functions from this file, but that would have been a much more verbose change. Since we were removing most of the uses of "$GIT_QUIET" this change comes after those.

---
 git-submodule.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

index e245b7fbaca..cad9948c047 100755
@@ -30,6 +30,7 @@ GIT_PROTOCOL_FROM_USER=0
 export GIT_PROTOCOL_FROM_USER
 
 command=
+quiet=
 cached=
 
 while test $# != 0 && test -z "$command"

@@ -39,7 +40,7 @@ do
 		command=$1
 		;;
 	-q|--quiet)
-		GIT_QUIET=1
+		quiet=1
 		;;
 	--cached)
 		cached=1

@@ -80,10 +81,10 @@ absorbgitdirs)
 	;;
 update)
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper "$command" \
-		${GIT_QUIET:+--quiet} ${wt_prefix:+--prefix "$wt_prefix"} "$@"
+		${quiet:+--quiet} ${wt_prefix:+--prefix "$wt_prefix"} "$@"
 	;;
 add | foreach | init | deinit | set-branch | set-url | status | summary | sync)
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper "$command" \
-		${GIT_QUIET:+--quiet} ${cached:+--cached} "$@"
+		${quiet:+--quiet} ${cached:+--cached} "$@"
 	;;
 esac

2.36.1.1178.gb5b1747c546

Understand --checkout, --merge and --rebase synonyms for --update={checkout,merge,rebase}, as well as the short options that 'git submodule' itself understands.

This removes a difference between the CLI API of "git submodule" and "git submodule--helper", making it easier to make the latter an alias for the former. See 48308681b07 (git submodule update: have a dedicated helper for cloning, 2016-02-29) for the initial addition of --update.

---
 builtin/submodule--helper.c | 17 +++++++++++++++++
 git-submodule.sh            | 14 +++++++++-----
 2 files changed, 26 insertions(+), 5 deletions(-)

index f1f5c6b84c6..64d7c5d7ba4 100644
@@ -2583,6 +2583,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	struct update_data opt = UPDATE_DATA_INIT;
 	struct list_objects_filter_options filter_options;
 	int ret;
+	enum submodule_update_type update_type = SM_UPDATE_UNSPECIFIED;
 
 	struct option module_update_options[] = {
 		OPT__FORCE(&opt.force, N_("force checkout updates"), 0),

@@ -2604,6 +2605,15 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "update", &opt.update_default,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
+		OPT_SET_INT(0, "checkout", &update_type,
+			N_("use the 'checkout' update strategy (default)"),
+			SM_UPDATE_CHECKOUT),
+		OPT_SET_INT('m', "merge", &update_type,
+			N_("use the 'merge' update strategy"),
+			SM_UPDATE_MERGE),
+		OPT_SET_INT('r', "rebase", &update_type,
+			N_("use the 'rebase' update strategy"),
+			SM_UPDATE_MERGE),
 		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
 			   N_("reference repository")),
 		OPT_BOOL(0, "dissociate", &opt.dissociate,

@@ -2653,6 +2663,13 @@ static int module_update(int argc, const char **argv, const char *prefix)
 
 	opt.filter_options = &filter_options;
 
+	if (update_type == SM_UPDATE_CHECKOUT)
+		opt.update_default = "checkout";
+	else if (update_type == SM_UPDATE_MERGE)
+		opt.update_default = "merge";
+	else if (update_type == SM_UPDATE_REBASE)
+		opt.update_default = "rebase";
+
 	if (opt.update_default)
 		if (parse_submodule_update_strategy(opt.update_default,
 						    &opt.update_strategy) < 0)

index 43fe3bcb00b..d8abdd8e8ad 100755
@@ -38,7 +38,9 @@ init=
 require_init=
 remote=
 nofetch=
-update=
+rebase=
+merge=
+checkout=
 depth=
 progress=
 dissociate=

@@ -83,7 +85,7 @@ cmd_update()
 			force=$1
 			;;
 		-r|--rebase)
-			update="rebase"
+			rebase=1
 			;;
 		--reference)
 			case "$2" in '') usage ;; esac

@@ -97,13 +99,13 @@ cmd_update()
 			dissociate=1
 			;;
 		-m|--merge)
-			update="merge"
+			merge=1
 			;;
 		--recursive)
 			recursive=1
 			;;
 		--checkout)
-			update="checkout"
+			checkout=1
 			;;
 		--recommend-shallow)
 			recommend_shallow="--recommend-shallow"

@@ -164,7 +166,9 @@ cmd_update()
 		${init:+--init} \
 		${nofetch:+--no-fetch} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
-		${update:+--update "$update"} \
+		${rebase:+--rebase} \
+		${merge:+--merge} \
+		${checkout:+--checkout} \
 		${reference:+"$reference"} \
 		${dissociate:+"--dissociate"} \
 		${depth:+"$depth"} \

2.36.1.1178.gb5b1747c546

Dispatch the "update" command directly to "git submodule--helper update", rather than doing our own argument parsing in git-submodule.sh. As this is the last cmd_*() function to be removed from git-submodule.sh we can do some larger cleanup of that file as a result.

As noted in a preceding commit the only behavior change here should be the desirable change of better "-h" output, and that this implementation understands the "--verbose" synonym for "-v". Let's update the documentation to reflect the new "--verbose" synonym.

---
 Documentation/git-submodule.txt |   5 +-
 git-submodule.sh                | 159 +-------------------------------
 2 files changed, 5 insertions(+), 159 deletions(-)

index 5f1c552110e..95685ac4f1c 100644
@@ -14,7 +14,7 @@ SYNOPSIS
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
 'git submodule' [--quiet] init [--] [<path>...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] <path>...)
-'git submodule' [--quiet] update [-v] [<options>] [--] [<path>...]
+'git submodule' [--quiet] update [-v | --verbose] [<options>] [--] [<path>...]
 'git submodule' [--quiet] set-branch [<options>] [--] <path>
 'git submodule' [--quiet] set-url [--] <path> <newurl>
 'git submodule' [--quiet] summary [<options>] [--] [<path>...]

@@ -133,7 +133,7 @@ If you really want to remove a submodule from the repository and commit
 that use linkgit:git-rm[1] instead. See linkgit:gitsubmodules[7] for removal
 options.
 
-update [-v] [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference <repository>] [--depth <depth>] [--recursive] [--jobs <n>] [--[no-]single-branch] [--filter <filter spec>] [--] [<path>...]::
+update [-v | --verbose] [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference <repository>] [--depth <depth>] [--recursive] [--jobs <n>] [--[no-]single-branch] [--filter <filter spec>] [--] [<path>...]::
 +
 --
 Update the registered submodules to match what the superproject

@@ -271,6 +271,7 @@ OPTIONS
 	Only print error messages.
 
 -v::
+--verbose::
 	Don't be quiet. This option is only valid for the update command.
 
 --progress::

2.36.1.1178.gb5b1747c546

Very cool. I've skimmed through all of the patches, which mostly look good except for ~1-2 things.

Your series shows that there isn't any prohibitively difficult work left to finish the conversion, which is great! The real problem IMO is the potential for mechanical errors given how many lines this touches.

Here's a way of breaking apart the work that makes sense to me:

- Reuse the patches that prepare git-submodule.sh for the conversion, particularly 1-7/20 (create a "case" dispatch statement and its preceding patches).
- Keep my series that prepares "update", since that's the most tedious one to convert. If I don't dispatch to the "case" statement, I don't think it will even conflict with the preparatory series.

Some of your patches make more sense than mine, and I'll incorporate them as necessary :)
- Dispatch subcommands using the "case" dispatch, including "update". We might have to do this slowly if we want things to be easy to eyeball.
- "git rm git-submodule.sh"!

Yes, if you took as many steps as I did, your series would be way too long :P

To convert "update", I don't think this many steps is necessary; I prepared it this way primarily to make it easier for everyone to spot how the options changed so that they can give feedback. Some of these can be squashed in my reroll

Yeah your approach is easier to eyeball, so I'll do this instead.

On hindsight, what I did is definitely cheating ;)

My series also breaks the way we'd handle --quiet in "git submodule", i.e.

   git submodule --quiet update

should be quiet, but isn't.

Your approach actually handles --quiet as per the original shell script, which is a good enough reason to do it your way. We can think about removing it later.

This would all make sense in a preparatory series, with the exception of 3/20 git-submodule.sh: remove unused --super-prefix logic.

We have several instances where we invoke submodule--helper directly with --super-prefix, e.g. inside sync_submodule():

    if (flags & OPT_RECURSIVE) {
      struct child_process cpr = CHILD_PROCESS_INIT;

      cpr.git_cmd = 1;
      cpr.dir = path;
      prepare_submodule_repo_env(&cpr.env_array);

      strvec_push(&cpr.args, "--super-prefix"); /* Here */

I even have a (as of now private) patch that replaces "update"'s --recursive-prefix with --super-prefix.

This probably wasn't caught in the tests because this only affects how we calculate the submodule 'displayname'.

Yes, these are pretty easy. I'm worried about the number of lines changed and the potential for mechanical errors, but we can roll these more slowly if necessary.

Yeah, and since it's not-so-easy, it probably makes sense to continue to keep my series around. I'll borrow some of these patches if that's ok :)

Interesting approach. It looks ok to me, but if we break up this series, maybe this will be stale by the time we integrate the rest of the changes?

The sanitize_submodule_env() function was last used before b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15), let's remove it.

This also allows us to remove clear_local_git_env() from git-sh-setup.sh. That function hasn't been documented in Documentation/git-sh-setup.sh, and since 14111fc4927 (git: submodule honor -c credential.* from command line, 2016-02-29) it had only been used in the sanitize_submodule_env() function being removed here.

---
 git-sh-setup.sh  |  7 -------
 git-submodule.sh | 11 -----------
 2 files changed, 18 deletions(-)

index d92df37e992..ecb60d9e3cb 100644
@@ -285,13 +285,6 @@ get_author_ident_from_commit () {
 	parse_ident_from_commit author AUTHOR
 }
 
-# Clear repo-local GIT_* environment variables. Useful when switching to
-# another repository (e.g. when entering a submodule). See also the env
-# list in git_connect()
-clear_local_git_env() {
-	unset $(git rev-parse --local-env-vars)
-}
-
 # Generate a virtual base file for a two-file merge. Uses git apply to
 # remove lines from $1 that are not in $2, leaving only common lines.
 create_virtual_base() {

index fd0b4a2c947..bc436c4ca47 100755
@@ -56,17 +56,6 @@ isnumber()
 	n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
 }
 
-# Sanitize the local git environment for use within a submodule. We
-# can't simply use clear_local_git_env since we want to preserve some
-# of the settings from GIT_CONFIG_PARAMETERS.
-sanitize_submodule_env()
-{
-	save_config=$GIT_CONFIG_PARAMETERS
-	clear_local_git_env
-	GIT_CONFIG_PARAMETERS=$save_config
-	export GIT_CONFIG_PARAMETERS
-}
-
 #
 # Add a new submodule to the working tree, .gitmodules and the index
 #

2.36.1.1239.gfba91521d90

Remove the $prefix variable which isn't used anymore, and hasn't been since b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15).

Before that we'd use it to invoke "git submodule--helper" with the "--recursive-prefix" option, but since b3c5f5cb048 that "git submodule--helper" option is only used when it invokes itself.

Since we haven't used it since then we haven't been passing the --super-prefix option to "git submodule--helper", and can therefore remove the handling of it from builtin/submodule--helper.c as well.

Note also that the still-existing code in builtin/submodule--helper.c to invoke other "git submodule--helper" processes with "--super-prefix" is not passing the option to "cmd_submodule__helper()", rather it's an argument to "git" itself.

---
 builtin/submodule--helper.c | 53 +++++++++++++++----------------------
 git-submodule.sh            |  6 ++---
 2 files changed, 24 insertions(+), 35 deletions(-)

index 5c77dfcffee..9d3642290f3 100644
@@ -3364,35 +3364,32 @@ static int module_add(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-#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[] = {
-	{"list", module_list, 0},
-	{"name", module_name, 0},
-	{"clone", module_clone, 0},
-	{"add", module_add, SUPPORT_SUPER_PREFIX},
-	{"update", module_update, 0},
-	{"resolve-relative-url-test", resolve_relative_url_test, 0},
-	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
-	{"init", module_init, SUPPORT_SUPER_PREFIX},
-	{"status", module_status, SUPPORT_SUPER_PREFIX},
-	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
-	{"deinit", module_deinit, 0},
-	{"summary", module_summary, SUPPORT_SUPER_PREFIX},
-	{"push-check", push_check, 0},
-	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
-	{"is-active", is_active, 0},
-	{"check-name", check_name, 0},
-	{"config", module_config, 0},
-	{"set-url", module_set_url, 0},
-	{"set-branch", module_set_branch, 0},
-	{"create-branch", module_create_branch, 0},
+	{"list", module_list},
+	{"name", module_name},
+	{"clone", module_clone},
+	{"add", module_add},
+	{"update", module_update},
+	{"resolve-relative-url-test", resolve_relative_url_test},
+	{"foreach", module_foreach},
+	{"init", module_init},
+	{"status", module_status},
+	{"sync", module_sync},
+	{"deinit", module_deinit},
+	{"summary", module_summary},
+	{"push-check", push_check},
+	{"absorb-git-dirs", absorb_git_dirs},
+	{"is-active", is_active},
+	{"check-name", check_name},
+	{"config", module_config},
+	{"set-url", module_set_url},
+	{"set-branch", module_set_branch},
+	{"create-branch", module_create_branch},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)

@@ -3401,15 +3398,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 	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);
+	for (i = 0; i < ARRAY_SIZE(commands); i++)
+		if (!strcmp(argv[1], commands[i].cmd))
 			return commands[i].fn(argc - 1, argv + 1, prefix);
-		}
-	}
 
 	die(_("'%s' is not a valid submodule--helper "
 	      "subcommand"), argv[1]);

index bc436c4ca47..53847bbf6e2 100755
@@ -41,7 +41,6 @@ files=
 remote=
 nofetch=
 update=
-prefix=
 custom_name=
 depth=
 progress=

@@ -127,7 +126,7 @@ cmd_add()
 		usage
 	fi
 
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
 }
 
 #

@@ -189,7 +188,7 @@ cmd_init()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
 }
 
 #

@@ -346,7 +345,6 @@ cmd_update()
 		${init:+--init} \
 		${nofetch:+--no-fetch} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
-		${prefix:+--recursive-prefix "$prefix"} \
 		${update:+--update "$update"} \
 		${reference:+"$reference"} \
 		${dissociate:+"--dissociate"} \

2.36.1.1239.gfba91521d90

This series:

* Removes dead code from git-submodule.sh, or dead code where it was the last user.

* Simplifies CLI parsing in git-submodule.sh, where it was doing something overly complex for no reason.

* Brings "git submodule--helper" in line with the CLI interface of "git-submodule.sh", for a follow-up series to remove the latter, as we'll be able to make a new "git submodule" in C dispatch directly to our new "git submodule--helper" code.

* Removes the "-v" option to "git submodule", which has been supported, but was a) never documented b) never did anything anyway, except as a way to negate an earlier "--quiet" option, as "verbose" was always the default.

* The last couple of patches are cleanup that isn't strictly neccesary for the end-goal of "git submodule" in C, but cleans up some more shellscript code.

The "say" function is removed from "git-sh-setup.sh", now that "git-submodule.sh" doesn't use it anymore (which happened before this series) we can replace the couple of remaining uses with "echo", and by having "git-subtree.sh" own the code that used to live in "git-sh-setup.sh".

The approach here was to combine bits of the two RFC series' Glen and I sent out:

- https://lore.kernel.org/git/[email protected]/
- https://lore.kernel.org/git/[email protected]/

My branch for this (and passing CI) are at:
https://github.com/avar/git/tree/avar/submodule-sh-dispatch-to-helper-directly-prep

The rest here is a reply to
https://lore.kernel.org/git/[email protected]/:

Yes, and as I've discovered in some further poking at the follow-up work (not included here) we again lack quite a bit of testing.

Hopefully there's no stepping on toes here, but I thought I'd send this out now (I went back to the laptop) to avoid the duplicate work, since I'd already attempted combining the two, and this is the result.

Obviously you may have different ideas about it, and I wouldn't at all mind a v3 that goes in a different direction if you think it's needed.

As noted in the CL above I think this is more palatable for an initial iteration, i.e. it's not doing any of the migration work yet, but merely doing small changes to bring git-submodule.sh and "git submodule--helper" in line with one another.

Hopefully you'll like the change I made here, I kept my commit which narrowlry changes it so that we pass --checkout, --merge and --rebase from git-submodule.sh.

Then after that comes a commit extracted from your change where we make "git submodule--helper" use the same interface for passing options to itself.

Both are good changes to make, but I think it makes sense to split them up. One is needed for "git submodule" in C, the other is just janitorial work in the "git submodule--helper" code.

I think it makes sense to do both, but splitting up the concerns makes it a lot easier to review them IMO.

Your removal of "-v" is in this series. I rewrote the commit message to note exactly what we're changing, why it's thought to be OK etc.

We have some of these unfortunate "top-level" CLI options in other places, e.g. "git commit-graph", it's probably not worth it to remove them, as they're easy enough to support, and have been documented for a long time.

This is still in this series as 02/12. I think you've misunderstood that code, it *is* invoking "git submodule--helper" with "--super-prefix", but the option is passed as:

    git --super-prefix <path> submodule--helper

And not as:

    git submodule--helper --super-prefix <path>

This is thus handled by other code before builtin/submodule--helper.c, and it doesn't need to handle it.

But anyway, this is confusing, so I updated the commit message (seen in the range-diff below)>

All of this is left out here for now. FWIW I don't think it's hard to review due to the line count, yes e.g. the 2nd patch there is ~300 lines changed, but it's just 5 lines removed/added if we omit the mechanical deletion of the now-unused functions.

Maybe it would be better to omit that and do just one "git rm" at the end? FWIW I thought doing it atomically so that we were left with no dead code after each step made it easier to review.

The proposal in *this series* is to leave this aside for now, but generally I wonder what part of it you find not-so-easy.

Personally I find it much harder to carefully review the way you proposed to do it, i.e. to "buffer up" options that we "don't handle", but actually need to sort-of handle, as we'd still like to die if we have unknown options.

Particularly since shellscript quoting etc. is a pain with that sort of thing, as it doesn't have any real list or key-value datastructures.

Whereas getting it to the point where we're clearly just passing options as-is through beforehand, and then simply dropping the wrapper is, I think, much easier to review. You only need to trust or check that e.g. "git submodule--helper update" also supports a "--progress" option or whatever, and/or that we've got coverage for it.

Yes, I think it's probably best to drop this idea of a useBuiltin setting in this case entirely.

The reason we did it for "stash", "git add --interactive" etc. is that we were (or are) dispatching to drastically different code, i.e. we had or have an entire all-at-once re-implementation of the command or feature.

Whereas for "git submodule" we've been doing it incrementally already, and complexity-wise I think previous rounds to migrate to "git submodule--helper" were much more likely have subtle breakages than the relatively straightforward migration we've got left.

B.t.w. here is my WIP re-roll of the full thing, on top of these patches:
https://github.com/avar/git/tree/avar/submodule-sh-dispatch-to-helper-directly-2

As covered in amended commits & tests there the v1 RFC (but not the parts re-rolled here, just the later stuff) had some subtle behavior changes. See the new "submodule tests: test usage behavior" commit there & follow-ups to that.

I haven't found any bad bugs yet, but just obscure edge cases in e.g. how we handle "git submodule --" before/after.

Glen Choo (2):
submodule update: remove "-v" option
submodule--helper: eliminate internal "--update" option

Ævar Arnfjörð Bjarmason (10):
git-submodule.sh: remove unused sanitize_submodule_env() git-submodule.sh: remove unused $prefix var and --super-prefix git-submodule.sh: make "$cached" variable a boolean git-submodule.sh: remove unused top-level "--branch" argument
submodule--helper: have --require-init imply --init
submodule--helper: rename "absorb-git-dirs" to "absorbgitdirs"
submodule--helper: report "submodule" as our name in "-h" output
submodule--helper: understand --checkout, --merge and --rebase
synonyms
git-submodule.sh: use "$quiet", not "$GIT_QUIET" git-sh-setup.sh: remove "say" function, change last users

 builtin/submodule--helper.c    | 127 +++++++++++++++++----------------
 contrib/subtree/git-subtree.sh |  15 +++-
 git-instaweb.sh                |   2 +-
 git-sh-setup.sh                |  16 -----
 git-submodule.sh               |  88 ++++++++---------------
 submodule.c                    |   2 +-
 t/t7406-submodule-update.sh    |   2 +-
 7 files changed, 112 insertions(+), 140 deletions(-)

Range-diff against v1:
1: 0e9f13822ef ! 1: c5afc72e075 git-submodule.sh: remove unused sanitize_submodule_env()
@@ Metadata
## Commit message ##
git-submodule.sh: remove unused sanitize_submodule_env()

- The sanitize_submodule_env() function was last used in
+ The sanitize_submodule_env() function was last used before
b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15), let's remove it.

 2:  8fcd832e58f <  -:  ----------- git-submodule.sh: remove unused $prefix variable
 3:  6c774505ac5 !  2:  e127803d59d git-submodule.sh: remove unused --super-prefix logic

@@ Metadata
Author: Ævar Arnfjörð Bjarmason <[email protected]>

## Commit message ##
- git-submodule.sh: remove unused --super-prefix logic
+ git-submodule.sh: remove unused $prefix var and --super-prefix

- The "$prefix" variable has not been set since b3c5f5cb048 (submodule:
- move core cmd_update() logic to C, 2022-03-15), so we'd never pass the
+ Remove the $prefix variable which isn't used anymore, and hasn't been
+ since b3c5f5cb048 (submodule: move core cmd_update() logic to C,
+ 2022-03-15).
+
+ Before that we'd use it to invoke "git submodule--helper" with the
+ "--recursive-prefix" option, but since b3c5f5cb048 that "git
+ submodule--helper" option is only used when it invokes itself.
+
+ Since we haven't used it since then we haven't been passing the
--super-prefix option to "git submodule--helper", and can therefore remove the handling of it from builtin/submodule--helper.c as well.

+ Note also that the still-existing code in builtin/submodule--helper.c
+ to invoke other "git submodule--helper" processes with
+ "--super-prefix" is not passing the option to
+ "cmd_submodule__helper()", rather it's an argument to "git" itself.
+

## builtin/submodule--helper.c ##
@@ builtin/submodule--helper.c: int cmd_submodule__helper(int argc, const char **ar

die(_("'%s' is not a valid submodule--helper "
"subcommand"), argv[1]);
+
+ ## git-submodule.sh ##
+@@ git-submodule.sh: files=
+ remote=
+ nofetch=
+ update=
+-prefix=
+ custom_name=
+ depth=
+ progress=
+@@ git-submodule.sh: cmd_add()
+ usage
+ fi
+
+- git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
++ git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
+ }
+
+ #
+@@ git-submodule.sh: cmd_init()
+ shift
+ done
+
+- git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
++ git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
+ }
+
+ #
+@@ git-submodule.sh: cmd_update()
+ ${init:+--init} \
+ ${nofetch:+--no-fetch} \
+ ${wt_prefix:+--prefix "$wt_prefix"} \
+- ${prefix:+--recursive-prefix "$prefix"} \
+ ${update:+--update "$update"} \
+ ${reference:+"$reference"} \
+ ${dissociate:+"--dissociate"} \
5: 124c062e3a1 ! 3: d5c8613c654 git-submodule.sh: normalize parsing of --cached
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <[email protected]>

## Commit message ##
- git-submodule.sh: normalize parsing of --cached
+ git-submodule.sh: make "$cached" variable a boolean

Remove the assignment of "$1" to the "$cached" variable. As seen in the initial implementation in 70c7ac22de6 (Add git-submodule command, 2007-05-26) we only need to keep track of if we've seen the --cached
- option. In 28f9af5d25e (git-submodule summary: code framework,
- 2008-03-11) "$1" was assigned to it, but there was no reason to do
- so. Let's undo that.
+ option, not save the "--cached" string for later use.
+
+ In 28f9af5d25e (git-submodule summary: code framework, 2008-03-11)
+ "$1" was assigned to it, but since there was no reason to do so let's
+ stop doing it. This trivial change will make it easier to reason about
+ an eventual change that'll remove the cmd_summary() function in favor
+ of dispatching to "git submodule--helper summary" directly.

@@ git-submodule.sh: cmd_summary() {
--files)
files="$1"
@@ git-submodule.sh: do
- GIT_QUIET=1
+ branch="$2"; shift
;;
--cached)
- cached="$1"
4: f27723aa0a2 ! 4: fd328329b68 git-submodule.sh: normalize parsing of "--branch"
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <[email protected]>

## Commit message ##
- git-submodule.sh: normalize parsing of "--branch"
+ git-submodule.sh: remove unused top-level "--branch" argument

In 5c08dbbdf1a (git-submodule: fix subcommand parser, 2008-01-15) the "--branch" option was supported as an option to "git submodule"
@@ Commit message
"--cache" added (or rather modified) in 496eeeb19b9 (git-submodule.sh:
avoid "test <cond> -a/-o <cond>", 2014-06-10).

- But there's been a logic error in that check, this looked like it
- should be supporting:
+ But there's been a logic error in that check, which at a glance looked
+ like it should be supporting:

git submodule --branch <branch> (add | set-branch) [<options>]

@@ Commit message
we have "--branch" here already we'll emit usage, even for "add" and "set-branch".

- Since nobody's complained about "--branch <branch>" not being
- supported as argument to "git submodule" itself, i.e. we want to
- support:
+ So in addition to never having documented this form it hasn't worked
+ since b57e8119e6e was released with v2.22.0 it's safe to remove this
+ code. I.e. we don't want to support the form noted above, but only:

- git submodule (add | set-branch) --branch <branch> [<options>]
-
- But not the first form noted above. Let's just remove this code, we've
- never documented "--branch" as a top-level option (unlike "--quiet"),
- so this looks like it was an accident of the implementation, which we
- broke v2.22.0, so we also know it must not have been important to
- anyone.
+ git submodule (add | set-branch) --branch <branch> [<options>]

@@ git-submodule.sh: do
- branch="$2"; shift
- ;;
--cached)
- cached="$1"
+ cached=1
;;
@@ git-submodule.sh: then
fi
12: 57b9df29ea6 ! 5: ef3265a588e submodule--helper: have --require-init imply --init
@@ Commit message
--require-init option imply --init, rather than having "git-submodule.sh" add it implicitly.

+ This change doesn't make any difference now, but eliminates another
+ special-case where "git submodule--helper update"'s behavior was
+ different from "git submodule update". This will make it easier to
+ eventually replace the cmd_update() function in git-submodule.sh.
+
+ We'll still need to keep the distinction between "--init" and
+ "--require-init" in git-submodule.sh. Once cmd_update() gets
+ re-implemented in C we'll be able to variables and other code related
+ to that, but not yet.
+

## builtin/submodule--helper.c ##

 -:  ----------- >  6:  025694644fc submodule update: remove "-v" option
 6:  b1ca1183885 !  7:  63155ab5026 submodule--helper: rename "absorb-git-dirs" to "absorbgitdirs"

@@ Commit message
absorb-git-dir function, 2016-12-12).

Having these two be different will make it more tedious to dispatch to
- "git submodule--helper" directly, so let's get rid of this needless
+ eventually dispatch "git submodule--helper" directly, as we'd need to
+ retain this name mapping. So let's get rid of this needless
inconsistency.

@@ submodule.c: void absorb_git_dir_into_superproject(const char *path,
"submodule--helper",
- "absorb-git-dirs", NULL);
+ "absorbgitdirs", NULL);
- prepare_submodule_repo_env(&cp.env_array);
+ prepare_submodule_repo_env(&cp.env);
if (run_command(&cp))
die(_("could not recurse into submodule '%s'"), path);

 7:  9f5cfbb864a <  -:  ----------- git-submodule.sh: create a "case" dispatch statement
 8:  0c1a5063653 !  8:  99ac52d810f submodule--helper: pretend to be "git submodule" in "-h" output

@@ Metadata
Author: Ævar Arnfjörð Bjarmason <[email protected]>

## Commit message ##
- submodule--helper: pretend to be "git submodule" in "-h" output
+ submodule--helper: report "submodule" as our name in "-h" output

- Change the usage output for "git submodule--helper" so that it will
- pretend to be named "git submodule". This will allow us to dispatch
- further into "git submodule--helper" from git-submodule.sh while
- emitting the correct usage output.
+ Change the rest of the usage output for "git submodule--helper" so
+ that it will pretend to be named "git submodule", which it already did
+ e.g. in the case of "status", "deinit" and "update".
+
+ This will allow us to eventually dispatch further into "git
+ submodule--helper" from git-submodule.sh, while emitting the correct
+ usage output.
+
+ This does make the usage output of "git submodule--helper" itself
+ "incorrect", but since nothing user-facing invokes it directly it
+ makes sense to continue to elide that difference entirely. A user
+ isn't served by us exposing this internal implementation detail.

This fixes a bug that's been there ever since "git submodule absorbgitdirs" was added in f6f85861400 (submodule: add absorb-git-dir
@@ Commit message
usage: git submodule absorbgitdirs [<options>] [<path>...] [.. same usage output ...]

- Note that in the case of "status", "deinit" and "update" we were
- already referring to ourselves as "git submodule" in
- builtin/submodule--helper.c.
-

## builtin/submodule--helper.c ##

 9:  bd0e4a4f8b8 <  -:  ----------- git-submodule.sh: dispatch "sync" to helper
10:  498a1fd275b <  -:  ----------- git-submodule.sh: dispatch directly to helper
11:  625320e13b9 <  -:  ----------- git-submodule.sh: dispatch "foreach" to helper
13:  20db979a094 !  9:  01c5eee67c7 submodule--helper: understand --checkout, --merge and --rebase synonyms
    @@ builtin/submodule--helper.c: static int module_update(int argc, const char **arg
     +			SM_UPDATE_MERGE),
     +		OPT_SET_INT('r', "rebase", &update_type,
     +			N_("use the 'rebase' update strategy"),
    -+			SM_UPDATE_MERGE),
    ++			SM_UPDATE_REBASE),
      		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
      			   N_("reference repository")),
      		OPT_BOOL(0, "dissociate", &opt.dissociate,
    @@ builtin/submodule--helper.c: static int module_update(int argc, const char **arg
      						    &opt.update_strategy) < 0)
     
      ## git-submodule.sh ##
    -@@ git-submodule.sh: init=
    - require_init=
    +@@ git-submodule.sh: require_init=
    + files=
      remote=
      nofetch=
     -update=
     +rebase=
     +merge=
     +checkout=
    + custom_name=
      depth=
      progress=
    - dissociate=
     @@ git-submodule.sh: cmd_update()
      			force=$1
      			;;
14:  1cb40a5f42e <  -:  ----------- git-submodule doc: document the -v" option to "update"
15:  0c388eed1d1 <  -:  ----------- submodule--helper: understand -v option for "update"
16:  08abadda7c3 <  -:  ----------- git-submodule.sh: dispatch "update" to helper
17:  59a72296967 <  -:  ----------- git-submodule.sh: use "$quiet", not "$GIT_QUIET"
18:  c5796878f0b <  -:  ----------- git-submodule.sh: simplify parsing loop
19:  1423950de08 <  -:  ----------- submodule: make it a built-in, remove git-submodule.sh
20:  b2aaad5c008 <  -:  ----------- submodule: add a subprocess-less submodule.useBuiltin setting
 -:  ----------- > 10:  ac00a9599de submodule--helper: eliminate internal "--update" option
 -:  ----------- > 11:  875acae8550 git-submodule.sh: use "$quiet", not "$GIT_QUIET"
 -:  ----------- > 12:  31517345ae8 git-sh-setup.sh: remove "say" function, change last users

2.36.1.1239.gfba91521d90

Remove the assignment of "$1" to the "$cached" variable. As seen in the initial implementation in 70c7ac22de6 (Add git-submodule command, 2007-05-26) we only need to keep track of if we've seen the --cached option, not save the "--cached" string for later use.

In 28f9af5d25e (git-submodule summary: code framework, 2008-03-11) "$1" was assigned to it, but since there was no reason to do so let's stop doing it. This trivial change will make it easier to reason about an eventual change that'll remove the cmd_summary() function in favor of dispatching to "git submodule--helper summary" directly.

---
 git-submodule.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 53847bbf6e2..b99a00d9f84 100755
@@ -446,7 +446,7 @@ cmd_summary() {
 	do
 		case "$1" in
 		--cached)
-			cached="$1"
+			cached=1
 			;;
 		--files)
 			files="$1"

@@ -583,7 +583,7 @@ do
 		branch="$2"; shift
 		;;
 	--cached)
-		cached="$1"
+		cached=1
 		;;
 	--)
 		break

2.36.1.1239.gfba91521d90

In 5c08dbbdf1a (git-submodule: fix subcommand parser, 2008-01-15) the "--branch" option was supported as an option to "git submodule" itself, i.e. "git submodule --branch" as a side-effect of its implementation.

Then in b57e8119e6e (submodule: teach set-branch subcommand, 2019-02-08) when the "set-branch" subcommand was added the assertion that we shouldn't have "--branch" anywhere except as an argument to "add" and "set-branch" was copy/pasted from the adjacent check for "--cache" added (or rather modified) in 496eeeb19b9 (git-submodule.sh:
avoid "test <cond> -a/-o <cond>", 2014-06-10).

But there's been a logic error in that check, which at a glance looked like it should be supporting:

    git submodule --branch <branch> (add | set-branch) [<options>]

But due to "||" in the condition (as opposed to "&&" for "--cache") if we have "--branch" here already we'll emit usage, even for "add" and "set-branch".

So in addition to never having documented this form it hasn't worked since b57e8119e6e was released with v2.22.0 it's safe to remove this code. I.e. we don't want to support the form noted above, but only:

    git submodule (add | set-branch) --branch <branch> [<options>]

---
 git-submodule.sh | 14 --------------
 1 file changed, 14 deletions(-)

index b99a00d9f84..20fc1b620fa 100755
@@ -574,14 +574,6 @@ do
 	-q|--quiet)
 		GIT_QUIET=1
 		;;
-	-b|--branch)
-		case "$2" in
-		'')
-			usage
-			;;
-		esac
-		branch="$2"; shift
-		;;
 	--cached)
 		cached=1
 		;;

@@ -609,12 +601,6 @@ then
     fi
 fi
 
-# "-b branch" is accepted only by "add" and "set-branch"
-if test -n "$branch" && (test "$command" != add || test "$command" != set-branch)
-then
-	usage
-fi
-
 # "--cached" is accepted only by "status" and "summary"
 if test -n "$cached" && test "$command" != status && test "$command" != summary
 then

2.36.1.1239.gfba91521d90

Adjust code added in 0060fd1511b (clone --recurse-submodules: prevent name squatting on Windows, 2019-09-12) to have the internal --require-init option imply --init, rather than having "git-submodule.sh" add it implicitly.

This change doesn't make any difference now, but eliminates another special-case where "git submodule--helper update"'s behavior was different from "git submodule update". This will make it easier to eventually replace the cmd_update() function in git-submodule.sh.

We'll still need to keep the distinction between "--init" and "--require-init" in git-submodule.sh. Once cmd_update() gets re-implemented in C we'll be able to variables and other code related to that, but not yet.

---
 builtin/submodule--helper.c | 5 ++++-
 git-submodule.sh            | 1 -
 2 files changed, 4 insertions(+), 2 deletions(-)

index 9d3642290f3..7bcb76cea6a 100644
@@ -2618,7 +2618,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "progress", &opt.progress,
 			    N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &opt.require_init,
-			   N_("disallow cloning into non-empty directory")),
+			   N_("disallow cloning into non-empty directory, implies --init")),
 		OPT_BOOL(0, "single-branch", &opt.single_branch,
 			 N_("clone only one branch, HEAD or --branch")),
 		OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),

@@ -2642,6 +2642,9 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_update_options,
 			     git_submodule_helper_usage, 0);
 
+	if (opt.require_init)
+		opt.init = 1;
+
 	if (filter_options.choice && !opt.init) {
 		usage_with_options(git_submodule_helper_usage,
 				   module_update_options);

index 20fc1b620fa..5b9683bf766 100755
@@ -251,7 +251,6 @@ cmd_update()
 			init=1
 			;;
 		--require-init)
-			init=1
 			require_init=1
 			;;
 		--remote)

2.36.1.1239.gfba91521d90

From: Glen Choo <[email protected]>

In e84c3cf3dc3 (git-submodule.sh: accept verbose flag in cmd_update to be non-quiet, 2018-08-14) the "git submodule update" sub-command was made to understand "-v", but the option was never documented.

The only in-tree user has been this test added in 3ad0401e9e6 (submodule update: silence underlying merge/rebase with "--quiet", 2020-09-30), it wasn't per-se testing --quiet, but fixing a bug in e84c3cf3dc3: It used to set "GIT_QUIET=0" instead of unsetting it on "-v", and thus we'd end up passing "--quiet" to "git submodule--helper" on "-v", since the "--quiet" option was passed sing the ${parameter:+word} construct.

Furthermore, even if someone had used the "-v" option they'd only be getting the default output. Our default in both git-submodule.sh and "git submodule--helper" has been to be "verbose", so the only way this option could have matter is if it were used as e.g.:

    git submodule --quiet update -v [...]

I.e. to undo the effect of a previous "--quiet" on the command-line.

---
 git-submodule.sh            | 3 ---
 t/t7406-submodule-update.sh | 2 +-
 2 files changed, 1 insertion(+), 4 deletions(-)

index 5b9683bf766..0df6b0fc974 100755
@@ -241,9 +241,6 @@ cmd_update()
 		-q|--quiet)
 			GIT_QUIET=1
 			;;
-		-v)
-			unset GIT_QUIET
-			;;
 		--progress)
 			progress=1
 			;;

index 43f779d751c..06d804e2131 100755
@@ -1074,7 +1074,7 @@ test_expect_success 'submodule update --quiet passes quietness to merge/rebase'
 	 git submodule update --rebase --quiet >out 2>err &&
 	 test_must_be_empty out &&
 	 test_must_be_empty err &&
-	 git submodule update --rebase -v >out 2>err &&
+	 git submodule update --rebase >out 2>err &&
 	 test_file_not_empty out &&
 	 test_must_be_empty err
 	)

2.36.1.1239.gfba91521d90

Understand --checkout, --merge and --rebase synonyms for --update={checkout,merge,rebase}, as well as the short options that 'git submodule' itself understands.

This removes a difference between the CLI API of "git submodule" and "git submodule--helper", making it easier to make the latter an alias for the former. See 48308681b07 (git submodule update: have a dedicated helper for cloning, 2016-02-29) for the initial addition of --update.

---
 builtin/submodule--helper.c | 17 +++++++++++++++++
 git-submodule.sh            | 14 +++++++++-----
 2 files changed, 26 insertions(+), 5 deletions(-)

index c2f55779cb1..57f0237af23 100644
@@ -2582,6 +2582,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	struct update_data opt = UPDATE_DATA_INIT;
 	struct list_objects_filter_options filter_options;
 	int ret;
+	enum submodule_update_type update_type = SM_UPDATE_UNSPECIFIED;
 
 	struct option module_update_options[] = {
 		OPT__FORCE(&opt.force, N_("force checkout updates"), 0),

@@ -2603,6 +2604,15 @@ static int module_update(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "update", &opt.update_default,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
+		OPT_SET_INT(0, "checkout", &update_type,
+			N_("use the 'checkout' update strategy (default)"),
+			SM_UPDATE_CHECKOUT),
+		OPT_SET_INT('m', "merge", &update_type,
+			N_("use the 'merge' update strategy"),
+			SM_UPDATE_MERGE),
+		OPT_SET_INT('r', "rebase", &update_type,
+			N_("use the 'rebase' update strategy"),
+			SM_UPDATE_REBASE),
 		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
 			   N_("reference repository")),
 		OPT_BOOL(0, "dissociate", &opt.dissociate,

@@ -2652,6 +2662,13 @@ static int module_update(int argc, const char **argv, const char *prefix)
 
 	opt.filter_options = &filter_options;
 
+	if (update_type == SM_UPDATE_CHECKOUT)
+		opt.update_default = "checkout";
+	else if (update_type == SM_UPDATE_MERGE)
+		opt.update_default = "merge";
+	else if (update_type == SM_UPDATE_REBASE)
+		opt.update_default = "rebase";
+
 	if (opt.update_default)
 		if (parse_submodule_update_strategy(opt.update_default,
 						    &opt.update_strategy) < 0)

index 1c1dc320922..7fc7119fb21 100755
@@ -40,7 +40,9 @@ require_init=
 files=
 remote=
 nofetch=
-update=
+rebase=
+merge=
+checkout=
 custom_name=
 depth=
 progress=

@@ -260,7 +262,7 @@ cmd_update()
 			force=$1
 			;;
 		-r|--rebase)
-			update="rebase"
+			rebase=1
 			;;
 		--reference)
 			case "$2" in '') usage ;; esac

@@ -274,13 +276,13 @@ cmd_update()
 			dissociate=1
 			;;
 		-m|--merge)
-			update="merge"
+			merge=1
 			;;
 		--recursive)
 			recursive=1
 			;;
 		--checkout)
-			update="checkout"
+			checkout=1
 			;;
 		--recommend-shallow)
 			recommend_shallow="--recommend-shallow"

@@ -341,7 +343,9 @@ cmd_update()
 		${init:+--init} \
 		${nofetch:+--no-fetch} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
-		${update:+--update "$update"} \
+		${rebase:+--rebase} \
+		${merge:+--merge} \
+		${checkout:+--checkout} \
 		${reference:+"$reference"} \
 		${dissociate:+"--dissociate"} \
 		${depth:+"$depth"} \

2.36.1.1239.gfba91521d90

Change the rest of the usage output for "git submodule--helper" so that it will pretend to be named "git submodule", which it already did e.g. in the case of "status", "deinit" and "update".

This will allow us to eventually dispatch further into "git submodule--helper" from git-submodule.sh, while emitting the correct usage output.

This does make the usage output of "git submodule--helper" itself "incorrect", but since nothing user-facing invokes it directly it makes sense to continue to elide that difference entirely. A user isn't served by us exposing this internal implementation detail.

This fixes a bug that's been there ever since "git submodule absorbgitdirs" was added in f6f85861400 (submodule: add absorb-git-dir function, 2016-12-12). Before this we'd emit e.g. this on invalid usage:

	$ git submodule absorbgitdirs --blah
	error: unknown option `blah'
	usage: git submodule--helper absorbgitdirs [<options>] [<path>...]

Now we'll emit the same, except that the usage line is now referring to the correct command run by the user:

	usage: git submodule absorbgitdirs [<options>] [<path>...]
	[.. same usage output ...]

---
 builtin/submodule--helper.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

index 574d6e0a79b..c2f55779cb1 100644
@@ -286,7 +286,7 @@ static int module_list(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper list [--prefix=<path>] [<path>...]"),
+		N_("git submodule list [--prefix=<path>] [<path>...]"),
 		NULL
 	};
 

@@ -444,7 +444,7 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper foreach [--quiet] [--recursive] [--] <command>"),
+		N_("git submodule foreach [--quiet] [--recursive] [--] <command>"),
 		NULL
 	};
 

@@ -582,7 +582,7 @@ static int module_init(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper init [<options>] [<path>]"),
+		N_("git submodule init [<options>] [<path>]"),
 		NULL
 	};
 

@@ -786,7 +786,7 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	const struct submodule *sub;
 
 	if (argc != 2)
-		usage(_("git submodule--helper name <path>"));
+		usage(_("git submodule name <path>"));
 
 	sub = submodule_from_path(the_repository, null_oid(), argv[1]);
 

@@ -1185,7 +1185,7 @@ static int module_summary(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper summary [<options>] [<commit>] [--] [<path>]"),
+		N_("git submodule summary [<options>] [<commit>] [--] [<path>]"),
 		NULL
 	};
 

@@ -1349,7 +1349,7 @@ static int module_sync(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper sync [--quiet] [--recursive] [<path>]"),
+		N_("git submodule sync [--quiet] [--recursive] [<path>]"),
 		NULL
 	};
 

@@ -1789,7 +1789,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
+		N_("git submodule clone [--prefix=<path>] [--quiet] "
 		   "[--reference <repository>] [--name <name>] [--depth <depth>] "
 		   "[--single-branch] [--filter <filter-spec>] "
 		   "--url <url> --path <path>"),

@@ -2787,7 +2787,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper absorbgitdirs [<options>] [<path>...]"),
+		N_("git submodule absorbgitdirs [<options>] [<path>...]"),
 		NULL
 	};
 

@@ -2851,9 +2851,9 @@ static int module_config(int argc, const char **argv, const char *prefix)
 		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",
+		N_("git submodule config <name> [<value>]"),
+		N_("git submodule config --unset <name>"),
+		"git submodule config --check-writeable",
 		NULL
 	};
 

@@ -2892,7 +2892,7 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 	const char *const usage[] = {
-		N_("git submodule--helper set-url [--quiet] <path> <newurl>"),
+		N_("git submodule set-url [--quiet] <path> <newurl>"),
 		NULL
 	};
 

@@ -2931,8 +2931,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 	const char *const usage[] = {
-		N_("git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>"),
-		N_("git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
+		N_("git submodule set-branch [-q|--quiet] (-d|--default) <path>"),
+		N_("git submodule set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
 		NULL
 	};
 

@@ -2973,7 +2973,7 @@ static int module_create_branch(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 	const char *const usage[] = {
-		N_("git submodule--helper create-branch [-f|--force] [--create-reflog] [-q|--quiet] [-t|--track] [-n|--dry-run] <name> <start-oid> <start-name>"),
+		N_("git submodule create-branch [-f|--force] [--create-reflog] [-q|--quiet] [-t|--track] [-n|--dry-run] <name> <start-oid> <start-name>"),
 		NULL
 	};
 

@@ -3276,7 +3276,7 @@ static int module_add(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const usage[] = {
-		N_("git submodule--helper add [<options>] [--] <repository> [<path>]"),
+		N_("git submodule add [<options>] [--] <repository> [<path>]"),
 		NULL
 	};
 

2.36.1.1239.gfba91521d90

Rename the "absorb-git-dirs" subcommand to "absorbgitdirs", which is what the "git submodule" command itself has called it since the subcommand was implemented in f6f85861400 (submodule: add absorb-git-dir function, 2016-12-12).

Having these two be different will make it more tedious to dispatch to eventually dispatch "git submodule--helper" directly, as we'd need to retain this name mapping. So let's get rid of this needless inconsistency.

---
 builtin/submodule--helper.c | 4 ++--
 git-submodule.sh            | 2 +-
 submodule.c                 | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

index 7bcb76cea6a..574d6e0a79b 100644
@@ -2787,7 +2787,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper absorb-git-dirs [<options>] [<path>...]"),
+		N_("git submodule--helper absorbgitdirs [<options>] [<path>...]"),
 		NULL
 	};
 

@@ -3386,7 +3386,7 @@ static struct cmd_struct commands[] = {
 	{"deinit", module_deinit},
 	{"summary", module_summary},
 	{"push-check", push_check},
-	{"absorb-git-dirs", absorb_git_dirs},
+	{"absorbgitdirs", absorb_git_dirs},
 	{"is-active", is_active},
 	{"check-name", check_name},
 	{"config", module_config},

index 0df6b0fc974..1c1dc320922 100755
@@ -552,7 +552,7 @@ cmd_sync()
 
 cmd_absorbgitdirs()
 {
-	git submodule--helper absorb-git-dirs --prefix "$wt_prefix" "$@"
+	git submodule--helper absorbgitdirs --prefix "$wt_prefix" "$@"
 }
 
 # This loop parses the command line arguments to find the

index 4e299f578f9..2af16c647d5 100644
@@ -2374,7 +2374,7 @@ void absorb_git_dir_into_superproject(const char *path,
 		cp.no_stdin = 1;
 		strvec_pushl(&cp.args, "--super-prefix", sb.buf,
 			     "submodule--helper",
-			     "absorb-git-dirs", NULL);
+			     "absorbgitdirs", NULL);
 		prepare_submodule_repo_env(&cp.env);
 		if (run_command(&cp))
 			die(_("could not recurse into submodule '%s'"), path);

2.36.1.1239.gfba91521d90

Remove the use of the "$GIT_QUIET" variable in favor of our own "$quiet", ever since b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15) we have not used the "say" function in git-sh-setup.sh, which is the only thing that's affected by using "GIT_QUIET".

We still want to support --quiet for our own use though, but let's use our own variable for that, but now it's obvious that we only care about passing "--quiet" to "git submodule--helper", and not to change the output of any "say" invocation.

---
 git-submodule.sh | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

index 7fc7119fb21..5e5d21c010f 100755
@@ -30,6 +30,7 @@ GIT_PROTOCOL_FROM_USER=0
 export GIT_PROTOCOL_FROM_USER
 
 command=
+quiet=
 branch=
 force=
 reference=

@@ -80,7 +81,7 @@ cmd_add()
 			force=$1
 			;;
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--progress)
 			progress=1

@@ -128,7 +129,7 @@ cmd_add()
 		usage
 	fi
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper add ${quiet:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
 }
 
 #

@@ -144,7 +145,7 @@ cmd_foreach()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--recursive)
 			recursive=1

@@ -159,7 +160,7 @@ cmd_foreach()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foreach ${quiet:+--quiet} ${recursive:+--recursive} -- "$@"
 }
 
 #

@@ -174,7 +175,7 @@ cmd_init()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--)
 			shift

@@ -190,7 +191,7 @@ cmd_init()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${quiet:+--quiet} -- "$@"
 }
 
 #

@@ -207,7 +208,7 @@ cmd_deinit()
 			force=$1
 			;;
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--all)
 			deinit_all=t

@@ -226,7 +227,7 @@ cmd_deinit()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${quiet:+--quiet} ${force:+--force} ${deinit_all:+--all} -- "$@"
 }
 
 #

@@ -241,7 +242,7 @@ cmd_update()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--progress)
 			progress=1

@@ -335,7 +336,7 @@ cmd_update()
 	done
 
 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update \
-		${GIT_QUIET:+--quiet} \
+		${quiet:+--quiet} \
 		${force:+--force} \
 		${progress:+"--progress"} \
 		${remote:+--remote} \

@@ -396,7 +397,7 @@ cmd_set_branch() {
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch "$branch"} ${default:+--default} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-branch ${quiet:+--quiet} ${branch:+--branch "$branch"} ${default:+--default} -- "$@"
 }
 
 #

@@ -409,7 +410,7 @@ cmd_set_url() {
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--)
 			shift

@@ -425,7 +426,7 @@ cmd_set_url() {
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper set-url ${quiet:+--quiet} -- "$@"
 }
 
 #

@@ -496,7 +497,7 @@ cmd_status()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			;;
 		--cached)
 			cached=1

@@ -518,7 +519,7 @@ cmd_status()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status ${quiet:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- "$@"
 }
 #
 # Sync remote urls for submodules

@@ -531,7 +532,7 @@ cmd_sync()
 	do
 		case "$1" in
 		-q|--quiet)
-			GIT_QUIET=1
+			quiet=1
 			shift
 			;;
 		--recursive)

@@ -551,7 +552,7 @@ cmd_sync()
 		esac
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper sync ${quiet:+--quiet} ${recursive:+--recursive} -- "$@"
 }
 
 cmd_absorbgitdirs()

@@ -572,7 +573,7 @@ do
 		command=$1
 		;;
 	-q|--quiet)
-		GIT_QUIET=1
+		quiet=1
 		;;
 	--cached)
 		cached=1

2.36.1.1239.gfba91521d90

Remove the "say" function, with various rewrites of the remaining git-*.sh code to C and the preceding change to have git-submodule.sh stop using the GIT_QUIET variable there were only four uses in git-subtree.sh. Let's have it use an "arg_quiet" variable instead, and move the "say" function over to it.

The only other use was a trivial message in git-instaweb.sh, since it has never supported the --quiet option (or similar) that code added in 0b624b4ceee (instaweb: restart server if already running, 2009-11-22) can simply use "echo" instead.

The remaining in-tree hits from "say" are all for the sibling function defined in t/test-lib.sh. It's safe to remove this function since it has never been documented in Documentation/git-sh-setup.txt.

---
 contrib/subtree/git-subtree.sh | 15 ++++++++++++---
 git-instaweb.sh                |  2 +-
 git-sh-setup.sh                |  9 ---------
 3 files changed, 13 insertions(+), 13 deletions(-)

index 1af1d9653e9..7562a395c24 100755
@@ -50,6 +50,14 @@ m,message=    use the given message as the commit message for the merge commit
 
 indent=0
 
+# Usage: say [MSG...]
+say () {
+	if test -z "$arg_quiet"
+	then
+		printf '%s\n' "$*"
+	fi
+}
+
 # Usage: debug [MSG...]
 debug () {
 	if test -n "$arg_debug"

@@ -60,7 +68,7 @@ debug () {
 
 # Usage: progress [MSG...]
 progress () {
-	if test -z "$GIT_QUIET"
+	if test -z "$arg_quiet"
 	then
 		if test -z "$arg_debug"
 		then

@@ -146,6 +154,7 @@ main () {
 	eval "$set_args"
 
 	# Begin "real" flag parsing.
+	arg_quiet=
 	arg_debug=
 	arg_prefix=
 	arg_split_branch=

@@ -161,7 +170,7 @@ main () {
 
 		case "$opt" in
 		-q)
-			GIT_QUIET=1
+			arg_quiet=1
 			;;
 		-d)
 			arg_debug=1

@@ -252,7 +261,7 @@ main () {
 	dir="$(dirname "$arg_prefix/.")"
 
 	debug "command: {$arg_command}"
-	debug "quiet: {$GIT_QUIET}"
+	debug "quiet: {$arg_quiet}"
 	debug "dir: {$dir}"
 	debug "opts: {$*}"
 	debug

index 4349566c891..c68f49454cd 100755
@@ -102,7 +102,7 @@ resolve_full_httpd () {
 
 start_httpd () {
 	if test -f "$fqgitdir/pid"; then
-		say "Instance already running. Restarting..."
+		echo "Instance already running. Restarting..."
 		stop_httpd
 	fi
 

index ecb60d9e3cb..ce273fe0e48 100644
@@ -57,15 +57,6 @@ die_with_status () {
 	exit "$status"
 }
 
-GIT_QUIET=
-
-say () {
-	if test -z "$GIT_QUIET"
-	then
-		printf '%s\n' "$*"
-	fi
-}
-
 if test -n "$OPTIONS_SPEC"; then
 	usage() {
 		"$0" -h

2.36.1.1239.gfba91521d90

From: Glen Choo <[email protected]>

Follow-up on the preceding commit which taught "git submodule--helper update" to understand "--merge", "--checkout" and "--rebase" and use those options instead of "--update=(rebase|merge|checkout|none)" when the command invokes itself.

Unlike the preceding change this isn't strictly necessary to eventually change "git-submodule.sh" so that it invokes "git submodule--helper update" directly, but let's remove this inconsistency in the command-line interface. We shouldn't need to carry special synonyms for existing options in "git submodule--helper" when that command can use the primary documented names instead.

But, as seen in the post-image this makes the control flow within "builtin/submodule--helper.c" simpler, we can now write directly to the "update_default" member of "struct update_data" when parsing the options in "module_update()".

---
 builtin/submodule--helper.c | 42 ++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

index 57f0237af23..65cf4b915df 100644
@@ -1818,7 +1818,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 static void determine_submodule_update_strategy(struct repository *r,
 						int just_cloned,
 						const char *path,
-						const char *update,
+						enum submodule_update_type update,
 						struct submodule_update_strategy *out)
 {
 	const struct submodule *sub = submodule_from_path(r, null_oid(), path);

@@ -1828,9 +1828,7 @@ static void determine_submodule_update_strategy(struct repository *r,
 	key = xstrfmt("submodule.%s.update", sub->name);
 
 	if (update) {
-		if (parse_submodule_update_strategy(update, out) < 0)
-			die(_("Invalid update mode '%s' for submodule path '%s'"),
-				update, path);
+		out->type = update;
 	} else if (!repo_config_get_string_tmp(r, key, &val)) {
 		if (parse_submodule_update_strategy(val, out) < 0)
 			die(_("Invalid update mode '%s' configured for submodule path '%s'"),

@@ -1882,7 +1880,7 @@ struct update_data {
 	const char *prefix;
 	const char *recursive_prefix;
 	const char *displaypath;
-	const char *update_default;
+	enum submodule_update_type update_default;
 	struct object_id suboid;
 	struct string_list references;
 	struct submodule_update_strategy update_strategy;

@@ -2406,6 +2404,8 @@ static void ensure_core_worktree(const char *path)
 
 static void update_data_to_args(struct update_data *update_data, struct strvec *args)
 {
+	enum submodule_update_type ud = update_data->update_default;
+
 	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
 	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
 	if (update_data->recursive_prefix)

@@ -2429,8 +2429,15 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
 		strvec_push(args, "--require-init");
 	if (update_data->depth)
 		strvec_pushf(args, "--depth=%d", update_data->depth);
-	if (update_data->update_default)
-		strvec_pushl(args, "--update", update_data->update_default, NULL);
+	if (ud == SM_UPDATE_MERGE)
+		strvec_push(args, "--merge");
+	else if (ud == SM_UPDATE_CHECKOUT)
+		strvec_push(args, "--checkout");
+	else if (ud == SM_UPDATE_REBASE)
+		strvec_push(args, "--rebase");
+	else if (ud != SM_UPDATE_UNSPECIFIED)
+		BUG("cannot convert update_default=%d to args", ud);
+
 	if (update_data->references.nr) {
 		struct string_list_item *item;
 		for_each_string_list_item(item, &update_data->references)

@@ -2582,7 +2589,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	struct update_data opt = UPDATE_DATA_INIT;
 	struct list_objects_filter_options filter_options;
 	int ret;
-	enum submodule_update_type update_type = SM_UPDATE_UNSPECIFIED;
 
 	struct option module_update_options[] = {
 		OPT__FORCE(&opt.force, N_("force checkout updates"), 0),

@@ -2601,16 +2607,13 @@ static int module_update(int argc, const char **argv, const char *prefix)
 			   N_("path"),
 			   N_("path into the working tree, across nested "
 			      "submodule boundaries")),
-		OPT_STRING(0, "update", &opt.update_default,
-			   N_("string"),
-			   N_("rebase, merge, checkout or none")),
-		OPT_SET_INT(0, "checkout", &update_type,
+		OPT_SET_INT(0, "checkout", &opt.update_default,
 			N_("use the 'checkout' update strategy (default)"),
 			SM_UPDATE_CHECKOUT),
-		OPT_SET_INT('m', "merge", &update_type,
+		OPT_SET_INT('m', "merge", &opt.update_default,
 			N_("use the 'merge' update strategy"),
 			SM_UPDATE_MERGE),
-		OPT_SET_INT('r', "rebase", &update_type,
+		OPT_SET_INT('r', "rebase", &opt.update_default,
 			N_("use the 'rebase' update strategy"),
 			SM_UPDATE_REBASE),
 		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),

@@ -2662,17 +2665,8 @@ static int module_update(int argc, const char **argv, const char *prefix)
 
 	opt.filter_options = &filter_options;
 
-	if (update_type == SM_UPDATE_CHECKOUT)
-		opt.update_default = "checkout";
-	else if (update_type == SM_UPDATE_MERGE)
-		opt.update_default = "merge";
-	else if (update_type == SM_UPDATE_REBASE)
-		opt.update_default = "rebase";
-
 	if (opt.update_default)
-		if (parse_submodule_update_strategy(opt.update_default,
-						    &opt.update_strategy) < 0)
-			die(_("bad value for update parameter"));
+		opt.update_strategy.type = opt.update_default;
 
 	if (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0) {
 		list_objects_filter_release(&filter_options);

2.36.1.1239.gfba91521d90

Thanks! I'm happy to see this happen regardless of whose patches we use :)

Reading the cover letter, I think it probably makes sense for this to supersede gc/submodule-update. I haven't really looked at the changes yet though, but I will soon.

Fortunately I hadn't resumed work on this yet, so it works out :)

Ah that's right, I forgot that we have to pass it to "git" directly. Thanks.

I wonder why we ever needed this. 89c8626557 (submodule helper: support super prefix, 2016-12-08) doesn't really explain it, so it looks like I'll have to dig around the ML.

Makes sense. I suppose we don't have to overthink the conversion because we will have to make the leap of faith to C at some point.

I didn't set out to keep "my side", but just to see if we could come up with something non-RFC that could be queued/integrated sooner than later, and removing dead code & the smallish changes to unify the interfaces for a subsequent conversion seemed like it would be more palatable.

Thus dropping 1-3/8 of yours, it's the "real" migration (or steps towards it). Then your 4/8 was tangled up in that migration so I took my smaller 12/20 over it.

I then took your 5/8, but combined with my 13/20 as discussed in the CL. I.e. splitting up the external from internal interface change, which we can do in two steps.

Your 6/8 is then taken over my 14-15/20, 7/8 is another thing I dropped as the "real" migration to leave for later, and as discussed (see below) your 8/8 is much smaller than the dead code removal we can do here in 2/12, i.e. we can drop --super-prefix, and a lot of yarn falls out once we pull on that thread...

Most of the patches I dropped were my own :) I.e. the "actual conversion" bits, and then I picked up e.g. the s/absorb-git-dirs/absorbgitdirs/g etc., and other patches in my series that unified the interfaces.

I hadn't noticed that Junio had picked your RFC patches up (I didn't check, figuring since it was RFC they wouldn't be...).

Anyway, I'm fine with whatever gets us to the end goal most efficiently.

Hope it helped :)

It was needed, but not after b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15) as my 02/12 discusses.

As a quick test try to check out b3c5f5cb048 and apply this change:

-#define SUPPORT_SUPER_PREFIX (1<<0)
+#define SUPPORT_SUPER_PREFIX 0

You'll find that t7406-submodule-update.sh passes, but check out its parent (which is a commit of yours) and it'll fail, as we'll then emit output like:

    -Submodule path '../super': checked out 'e1c658656b91df52a4634fbffeaa739807ce3521'
    +Submodule path 'super': checked out 'e1c658656b91df52a4634fbffeaa739807ce3521'

So this is just one of the things that were overly complex in git-submodule--helper because parts of it had to bridge the gap between *.sh and *.c land, but once we moved more parts to C we ended up getting that for free.

Anyway, that's something we can leave aside for now. I.e. maybe your way of doing it is better, maybe mine is. That's for later.

For now what do you think about focusing on initial smaller changes to get the two command-line interfaces as close to 1=1 as possible, and to remove dead code etc.

I.e. to aim for small changes now to make reviewing the eventual *.sh v.s. *.c code as easy as possible, as the CLI for "submodule" and "submodule--helper" will behave the same at that point, there won't be any dead code etc.

Ah yes you are right, as of b3c5f5cb048 we never pass --super-prefix to "git submodule" any more. Great!

I also remember that when I was reading the patch that became b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15), I noted (internally) that we used prefix to set "--recursive-prefix", which was unusual because prefix is usually mapped to "--super-prefix".

I have a patch to replace "--recursive-prefix" with "--super-prefix", but it doesn't make sense for that patch to be included in this series (it's preparation for something else entirely, and it won't make this series any cleaner.)

Hm, that's suprising. But I guess the good news is that if it's been broken for so long, nobody probably needs this.

Small typo maybe?

re-implemented in C we'll be able to *change* variables and other code related to that, but not yet.

Ah, that explains why we have a test for something that seemed so unnecessary. Note to self to dig a little more in the future.

The additional motivation looks great, and I think it's changed sufficiently that you should be the author instead of me.

Everything up to here makes sense. I agree that "git submodule--helper" should have been pretending to be "git submodule" wherever possible.

Before now, it probably didn't matter much because it's quite hard to get "git submodule--helper" to print usage without being first intercepted by "git submodule", but we'll need it now. Which brings us to..

We actually used to output the internal-only name of the submodule--helper subcommand, i.e.

git submodule--helper absorb-git-dirs

with the dashes.

I initially wondered why "absorbgitdirs" deserved a special shoutout when we are changing several other subcommands in the same patch, but it turns out that this usage string bug probably is "absorbgitdirs"-specific.

"absorbgitdirs" is the only subcommand that doesn't have an options parsing loop that catches invalid options, it's just:

  cmd_absorbgitdirs()
  {
    git submodule--helper absorb-git-dirs --prefix "$wt_prefix" "$@"
  }

whereas in other subcommands, invalid usage would be caught by by the options parsing loop and we never invoke "git submodule--helper blah". As a result, "git submodule--helper absorb-git-dirs" is probably the only subcommand that prints its usage string (instead of the generic "git submodule" usage string.)

It might be worth highlighting how absorbgitdirs is special in the commit message, but I don't feel strongly either way.