Re: [PATCH v4] clone: update submodule.recurse in config when using --recurse-submodule

[Philippe Blain <levraiphilippeblain@gmail.com>]

Hi Junio,

Le 2021-08-10 à 14:36, Junio C Hamano a écrit :
> Mahi Kolla <mahikolla@google.com> writes:
> 
>>> Is it possible to avoid changing the behaviour unconditionally and
>>> potentially breaking existing users by making it an opt-in feature,
>>> e.g. "git clone --recurse-submodules" would work as the current
>>> users would expect, while "git clone --recurse-submodules=sticky"
>>> would set submodule.recurse to true, or something?
>>
>> As mentioned, the `submodule.recurse=true` will only apply to active
>> submodules specified by the user. Setting this config value when the
>> user runs their initial `git clone` minimizes the number of times a
>> developer must use the `--recurse-submodule` option on other commands.
>>
>> However, this is a behavior change that may be surprising for
>> developers. To ensure a smooth rollout and easy adoption, I think
>> adding a message using an `advice.*` config setting would be useful.
> 
> It may be better than nothing, but that still is a unilateral
> behaviour change.  Can't we come up with a way to make it an opt-in
> feature?  I've already suggested to allow the "--recurse-submodules"
> option of "git clone" to take an optional parameter (e.g. "sticky")
> so that the user can request configuration variable to be set, but
> you seem to be ignoring or skirting it.  

The '--recures-submodule' option in 'git clone' already takes an optional
argument, which is a pathspec and if given, only submodules matching the given
pathspec will be initialized (as opposed to all submodules if the flag is given without
an argument). So, it does not seem to be possible to use this
flag as a way to also set 'submodule.recurse'.

When Emily (CC'ed) sent her roadmap for submodule enhancements in [1], the enhancement
that Mahi is suggesting was explicitely mentioned:

> - git clone 
...
> What doesn't already work:
>
>    * --recurse-submodules should turn on submodule.recurse=true

I don't know if Mahi is part of this effort or just came up with the same idea,
but in any case maybe Emily would be able to add more justification for this change.

> Even though I am not
> married to the "give optional parameter to --recurse-submodules"
> design, unconditionally setting the variable, with or without advice
> or warning, is a regression we'd want to avoid.
> 

In my opinion, it would not be a regression; it would a behaviour change that
would be a *vast improvement* for the majority of projects that use submodules, at
least those that use non-optional submodules (which, I believe, is the vast majority
of projects that use submodules, judging by what I've read on the web over the past 3
years of my interest in the subject.)

As soon as you use submodules in a non-optional way, you really *want* submodule.recurse=true,
because if not:

1. 'git checkout' does not recursively check out your submodules, which probably breaks your build.
    You have to remember to always run 'git checkout --recurse-submodules' or run 'git submdule update'
    after each checkout, and teach your team to do the same.
2. 'git pull' fetches submodules commits, but does not recursively check out your submodules,
    which also probably breaks your build. You have to remember to always run 'git pull --recurse-submodules',
    or run 'git submodule update' after each pull, and also teach your team to do so.
3. If you forget to do 1. or 2., and then use 'git commit -am "some message" (as a lot
    of Git beginners unfortunately do), you regress the submodule commit, creating a lot
    of problems down the line.

These are the main reasons that I think Git should recurse by default. Setting 'submodule.recurse'
also brings other niceties, like 'git grep' recursing into submodules.

If we can agree that the behaviour *should* change eventually, then at least
'git clone --recurse-submodules' could be changed *right now* to suggest setting
'submodule.recurse' using the advice API, and stating that this will be the default
some day.

Even if we don't agree that the behaviour should enventually change, I think
having this advice would be a strict improvement because
it would help user discover the setting, which would already go a long way.

Thanks,

Philippe.


[1] https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com/