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

[Glen Choo <chooglen@google.com>]

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

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

Ah that might explain some things. I'm still not sure how removing
SUPPORT_SUPER_PREFIX gives the result we see here, but here are some
observations using the extra info I learned while converting
"--recursive-prefix" -> "--super-prefix":

- 29a5e9e1ffe and its parent teach init_submodule() how to use the
  "--recursive-prefix" flag as a substitute for "--super-prefix".
  "--recursive-prefix" is used exclusively by "git submodule update" and
  does exactly the same thing as "--super-prefix".

  Those commits pass "--recursive-prefix" to
  do_get_submodule_displaypath(), which is an escape hatch to
  get_submodule_displaypath() (which always uses "--super-prefix").

- 75df9df0f81 introduced a regression because we now pass
  update_data.prefix to get_submodule_displaypath() instead of prefix,
  and update_data.prefix was never set (oops, that's my mistake). This
  was rectified in its child (b3c5f5cb048), where we start to set
  update_data.prefix when the two subcommands were combined.

  As a sidenote, in 75df9df0f81's diff, we see that recursive_prefix is
  used in conjunction with get_submodule_displaypath(), which means that
  we could theoretically use *both* "--recursive-prefix" AND
  "--super-prefix" at the same time. We don't do that though, because
  "--super-prefix" was never used by "git submodule update".

  (This prefixed_path stuff is going away when I convert
  "--recursive-prefix" to "--super-prefix", so don't feel the need to
  clean it up).