Re: [PATCH v3 02/12] git-submodule.sh: remove unused $prefix var and --super-prefix

From: Glen Choo <chooglen@google.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Atharva Raykar" <raykar.ath@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v3 02/12] git-submodule.sh: remove unused $prefix var and --super-prefix
Date: Wed, 22 Jun 2022 16:43:58 -0700	[thread overview]
Message-ID: <kl6ltu8ci7r5.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <patch-v3-02.12-082e015781e-20220622T142012Z-avarab@gmail.com>

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

> 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...

> @@ -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/
[2] https://lore.kernel.org/git/1474930003-83750-2-git-send-email-bmwill@google.com/