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

Hi Phillippe and Junio,

On Tue, Aug 10, 2021 at 4:04 PM Philippe Blain
[off-list ref] wrote:

Hi Junio,

Le 2021-08-10 à 14:36, Junio C Hamano a écrit :

quoted

Mahi Kolla [off-list ref] writes:

quoted

quoted

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

Because of the optional pathspec argument, adding a `=sticky` argument
to the option may be hard to implement. That was my initial hesitation
to the opt in design.

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

quoted

- git clone

...

quoted

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.

I am part of the team and am implementing that exact feature from the
roadmap :)

quoted

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.

I completely agree with this! These are a lot of the reasons why the
feature was initially suggested. An alternative path forward the team
discussed was testing `submodule.recurse=true` under
`feature.experimental`. This way we can collect feedback from
developers before making this the default config value.

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/ (local)

I agree that adding an advice message when a user runs `git clone
--recurse-submodules` would at least alert users of their options,
giving them the choice to set `submodule.recurse` accordingly.

Thanks!

Best,
Mahi Kolla