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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jun 22 2022, Glen Choo wrote:

> Æ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/

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.