Ævar Arnfjörð Bjarmason [off-list ref] writes:

Replace the "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 eventually follow-up and
merge the "builtin/submodule--helper.c" code into
"builtin/submodule.c". 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.

This is a large win for performance, we're now more than 4x as fast as
before in terms of the fixed cost of invoking any "git submodule"
command[1]:

	$ git hyperfine -L rev HEAD~1,HEAD -s 'make CFLAGS=-O3' './git --exec-path=$PWD submodule foreach "echo \$name"'
	Benchmark 1: ./git --exec-path=$PWD submodule foreach "echo \$name"' in 'HEAD~1
	  Time (mean ± σ):      42.2 ms ±   0.4 ms    [User: 34.9 ms, System: 9.1 ms]
	  Range (min … max):    41.3 ms …  43.2 ms    70 runs

	Benchmark 2: ./git --exec-path=$PWD submodule foreach "echo \$name"' in 'HEAD
	  Time (mean ± σ):       9.7 ms ±   0.1 ms    [User: 7.6 ms, System: 2.2 ms]
	  Range (min … max):     9.5 ms …  10.3 ms    282 runs

	Summary
	  './git --exec-path=$PWD submodule foreach "echo \$name"' in 'HEAD' ran
	    4.33 ± 0.07 times faster than './git --exec-path=$PWD submodule foreach "echo \$name"' in 'HEAD~1'

We're taking pains here to faithfully reproduce existing
"git-submodule.sh" behavior, even when that behavior is stupid. Some
of it we'll fix in subsequent commits, but let's first faithfully
reproduce the behavior.

One exception is the change in the behavior of the exit code
stand-alone "-h" and "--" yield, see the altered tests. Returning 129
instead of 0 and 1 for "-h" and "--" respectively is a concession to
basic sanity.

Sounds reasonable.

I was surprised to see that these strings aren't deduped from the ones
in builtin/submodule--helper.c, and are, in fact, different. Is there a
reason for that?

+static const char * const git_submodule_usage[] = {
+	BUILTIN_SUBMODULE_USAGE,
+	BUILTIN_SUBMODULE_ADD_USAGE,
+	BUILTIN_SUBMODULE_STATUS_USAGE,
+	BUILTIN_SUBMODULE_INIT_USAGE,
+	BUILTIN_SUBMODULE_DEINIT_USAGE,
+	BUILTIN_SUBMODULE_UPDATE_USAGE,
+	BUILTIN_SUBMODULE_SET_BRANCH_USAGE,
+	BUILTIN_SUBMODULE_SET_URL_USAGE,
+	BUILTIN_SUBMODULE_SUMMARY_USAGE,
+	BUILTIN_SUBMODULE_FOREACH_USAGE,
+	BUILTIN_SUBMODULE_SYNC_USAGE,
+	BUILTIN_SUBMODULE_ABSORBGITDIRS_USAGE,
+	NULL,
+};
+
+static void setup_helper_args(int argc, const char **argv, const char *prefix,
+			      int quiet, int cached, struct strvec *args,
+			      const struct option *options)
+{
+	const char *cmd;
+	int do_quiet_cache = 1;
+	int do_prefix = 1;
+
+	strvec_push(args, "submodule--helper");
+
+	/* No command word defaults to "status" */
+	if (!argc) {
+		strvec_push(args, "status");
+		return;
+	}

We can't return without processing "--cached", e.g. removing the
explicit "status" subcommand like so fails.

  diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
  index d8f7d6ee29..5e61cef18e 100755
  --- a/t/t7400-submodule-basic.sh
  +++ b/t/t7400-submodule-basic.sh
  @@ -587,7 +587,7 @@ test_expect_success 'status should be "modified" after submodule commit' '
  '

  test_expect_success 'the --cached sha1 should be rev1' '
  -	git submodule status --cached >list &&
  +	git submodule --cached >list &&
    grep "^+$rev1" list
  '

+
+	/* Did we get --cached with a command? */
+	if (cached)
+		usage_msg_optf(_("'%s' option is only supported with explicit 'status'"),
+			       git_submodule_usage, options, "--cached");
+
+
+	/* Either a valid command, or submodule--helper will barf! */
+	cmd = argv[0];

submodule--helper does die, but the help message that it emits is
submodule--helper specific, instead of the "git submodule" usage string
from before.

+	strvec_push(args, cmd);
+	argv++;
+	argc--;
+
+	/*
+	  * This is stupid, but don't support "[--]" to
+	 * subcommand-less "git-submodule" for now.
+	 */
+	if (!strcmp(cmd, "--") || !strcmp(cmd, "--end-of-options"))
+		usage_msg_optf(_("need explicit sub-command name to delimit with '%s'"),
+			       git_submodule_usage, options, cmd);

If this is the only "stupid" behavior, maybe just call this out
specifically in the commit message.

+
+	/* Options that need to go before user-supplied options */
+	if (!strcmp(cmd, "absorbgitdirs"))
+		do_quiet_cache = 0;
+	else if (!strcmp(cmd, "update"))
+		;
+	else
+		do_prefix = 0;
+	if (do_quiet_cache) {
+		if (quiet)
+			strvec_push(args, "--quiet");
+		if (cached)
+			strvec_push(args, "--cached");
+
+		if (prefix && do_prefix)
+			strvec_pushl(args, "--prefix", prefix, NULL);
+	}

This looks like a good reason to get rid of "--prefix" from "git
submodule--helper update" like I mentioned upthread.

I didn't notice earlier that absorbgitdirs had the same problem, but
that's an even easier fixup:

  diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
  index d11e100301..1f7f142270 100644
  --- a/builtin/submodule--helper.c
  +++ b/builtin/submodule--helper.c
  @@ -2825,9 +2825,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
    struct module_list list = MODULE_LIST_INIT;
    unsigned flags = ABSORB_GITDIR_RECURSE_SUBMODULES;
    struct option embed_gitdir_options[] = {
  -		OPT_STRING(0, "prefix", &prefix,
  -			   N_("path"),
  -			   N_("path into the working tree")),
      OPT_BIT(0, "--recursive", &flags, N_("recurse into submodules"),
        ABSORB_GITDIR_RECURSE_SUBMODULES),
      OPT_END()