On Wed, Jun 22 2022, Glen Choo wrote:

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

quoted

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.

One way to verify that this is indeed dead code is to try to check out
b3c5f5cb048^ and apply this change to a part of the code being removed
here:

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

Doing that will cause t7406-submodule-update.sh to fail with errors
such as:

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

I.e. the removal of the --super-prefix handling broke those cases, but
when doing the same ad-hoc test with b3c5f5cb048 all of our tests will
pass, since the "--super-prefix" will now be handled by earlier by
"git" itself.

Your finding is correct, but I just can't figure out why it is this way.
Neither b3c5f5cb048 nor b3c5f5cb048^ make any use of "--super-prefix"
(both use "--recursive-prefix"). And what's most puzzling to me is...

quoted

@@ -3402,15 +3399,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]);

that all we do here is die() if we see "--super-prefix" but it is not
supported. I wouldn't expect that the printed result is different; I'd
expect git to die(). This isn't even an issue with SUPPORT_SUPER_PREFIX
either - if we just had:

	if (get_super_prefix())
		die(_("%s doesn't support --super-prefix"),
		    commands[i].cmd);

we still see the same failure. At any rate, we don't seem to need
"--super-prefix" any more, so I didn't look deeper into it.

One thing that I noticed (while trying to replace "--recursive-prefix"
with "--super-prefix" is that since this check checks the environment
for the super prefix and not the CLI option, it will complain if we do
"git --super-prefix=foo submodule unsupported-command", and e.g. t7407
will fail if we add

  -	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
  +	{"foreach", module_foreach, 0},

I don't like this check but for another reason: the super prefix is set
in a GIT_* environment variable so it gets passed to all child
processes. So e.g. if we teach "git submodule update" to use
"--super-prefix", we must mark module_update with SUPPORT_SUPER_PREFIX.
But because that invokes "git submodule clone", "module_clone" must also
be marked SUPPORT_SUPER_PREFIX.

Frankly, I'm not sure why we need to check for SUPPORT_SUPER_PREFIX in
the "git submodule--helper" subcommand. I see that it was introduced in
89c8626557 (submodule helper: support super prefix, 2016-12-08) as part
of what eventually became absorbgitdirs, but I couldn't find any
discussion of why we need this check when it was first proposed [1].

I'm not 100% sure of why we need the top level check either, but as I
understand it, it's a way of saying whether a command "supports
submodules" or not [2]. If so, then checking whether a "git
submodule--helper" command can recurse into submodules sounds like a
pointless exercise.

I'm still all for deleting this because it really doesn't seem useful,
but I'd be lot more confident if someone knows why we have this to begin
with.

[1] https://lore.kernel.org/git/20161122192235.6055-1-sbeller@google.com/ (local)
[2] https://lore.kernel.org/git/1474930003-83750-2-git-send-email-bmwill@google.com/ (local)

I think I figured this out. I'm right about it being unnecessary, but
the explanation in the commit message is wrong.

What threw me off the trail is that the series that included b3c5f5cb048
(submodule: move core cmd_update() logic to C, 2022-03-15) has in
intra-series regression, which is what you're seeing here.

I.e. its parent 75df9df0f81 (submodule--helper: reduce logic in
run_update_procedure(), 2022-03-15) will fail with the above "../super"
error without that local change. You won't get the failure on its
parent, c9911c9358e (submodule--helper: teach update_data more options,
2022-03-15).

I.e. 75df9df0f81 & b3c5f5cb048 should have been squashed, anyway.

The actual point *for that test* at which we could have deleted that
"define" is 29a5e9e1ffe (submodule--helper update-clone: learn --init,
2022-03-04), but other tests fail.

The actual point is this commit in this series, I'll dig some more,
sorry about sending you down the wrong path. That digression was just
about chechking if we'd passed --super-prefix, which is changed in this
same commit...

I'll dig some more and re-roll.