Re: [PATCH v6] clone: set submodule.recurse=true if submodule.stickyRecursiveClone enabled

[Philippe Blain <levraiphilippeblain@gmail.com>]

Hi Mahi, Emily and Junio,

Le 2021-08-13 à 21:09, Mahi Kolla via GitGitGadget a écrit :
> From: Mahi Kolla <mkolla2@illinois.edu>
> 
> Based on current experience, when running git clone --recurse-submodules,
> developers do not expect other commands such as pull or checkout to run
> recursively into active submodules. However, setting submodule.recurse=true
> at this step could make for a simpler workflow by eliminating the need for
> the --recurse-submodules option in subsequent commands. To collect more
> data on developers' preference in regards to making submodule.recurse=true
> a default config value in the future, deploy this feature under the opt in
> submodule.stickyRecursiveClone flag.
> 

As I mentioned upthread [1], I'm really not sure that we need a new config
variable. If people want to have "--recurse-submodules" the default behaviour
for all commands that know this flag, they can set 'submodule.recurse' in their
~/.gitconfig, which enables the behaviour for all those commands (except clone
and ls-files). And orgs shipping Git to their devs can set it in their system
gitconfig. I've been using this setup for over two years, with almost zero
adverse effect on repositories that do not contain submodules. The *only* bug that
I encountered that affects git commands when *no* submodules are at play was:

c56c48dd07 (grep: ignore --recurse-submodules if --no-index is given, 2020-01-30)

I understand that once submodule.recurse is set in the global or system config file,
then Git will start recursing in repos that were cloned prior to
that config being enabled, as Emily mentions in [2]. Personnally I think it's a positive
point. That *might* be a deal-breaker for other people,
but in any case it would be good that this alternative - just using 'submodule.recurse'
- is mentioned in the commit message and that it mentions that caveat, i.e. why we need
a separate config.

Le 2021-08-13 à 16:38, Emily Shaffer a écrit :
>  and apparently
> 'submodule.recurse=true' has some weird edge cases for commands which
> are happy to run out-of-repo.

It would be nice if those known edge cases were documented somewhere (on the list,
on Gitgitgadget's issues list or at https://bugs.chromium.org/p/git). Apart from
the 'grep --no-index' glitch that I mentioned above, I did not encounter any
myself.

> Signed-off-by: Mahi Kolla <mkolla2@illinois.edu>
> ---
>      clone: set submodule.recurse=true if submodule.stickyRecursiveClone
>      enabled
>      
>      Based on current experience, when running git clone
>      --recurse-submodules, developers do not expect other commands such as
>      pull or checkout to run recursively into active submodules. However,
>      setting submodule.recurse=true at this step could make for a simpler
>      workflow by eliminating the need for the --recurse-submodules option in
>      subsequent commands. To collect more data on developers' behavior and
>      preferences when making submodule.recurse=true a default, deploy this
>      feature under the opt in submodule.stickyRecursiveClone flag.
>      
>      Signed-off-by: Mahi Kolla mkolla2@illinois.edu

Mahi, you can keep just the "Since v1" part in the GGG PR description (and the
automatically added CC's). No need to also repeat the commit message.

>      
>      Since V1: Made this an opt in feature under a custom
>      submodule.stickyRecursiveClone flag as opposed to feature.experimental.
>      Updated tests to reflect this design change. Also updated commit
>      message. Additionally, I will be contributing from my personal email
>      going forward as opposed to my @google email.
>      
>      cc: Philippe Blain levraiphilippeblain@gmail.com

Small nit: since you used '/submit' several times on Gitgitgadget, the previous version
you sent was actually sent as v5, and this here version is v6. So for next round, you could
write "Changes since v6" :)

> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1006%2F24mahik%2Fmaster-v6
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1006/24mahik/master-v6
> Pull-Request: https://github.com/gitgitgadget/git/pull/1006
> 
> Range-diff vs v5:
> 
>  
>         
>       -+test_expect_success 'feature.experimental flag manipulates submodule.recurse value' '
>       ++test_expect_success 'submodule.stickyRecursiveClone flag manipulates submodule.recurse value' '
>        +
>       -+	test_config_global feature.experimental true &&
>       ++	test_config_global submodule.stickyRecursiveClone true &&
>        +	git clone --recurse-submodules parent clone_recurse_true &&
>        +	test_cmp_config -C clone_recurse_true true submodule.recurse &&
>        +
>       -+	test_config_global feature.experimental false &&
>       ++	test_config_global submodule.stickyRecursiveClone false &&
>        +	git clone --recurse-submodules parent clone_recurse_false &&
>        +	test_expect_code 1 git -C clone_recurse_false config --get submodule.recurse
>        +

OK, so the expectation with 'submodule.stickyRecursiveClone' is that :
-  if it's true, then 'submodule.recurse' is set to true in the clone's local config file.
    That makes sense.
- if it's false, then 'submodule.recurse' is not set in the clone. This means that
   if 'submodule.recurse' is already set in ~/.gitconfig (or the system config file)
   then the clone will respect the configured value. That also makes sense, I think.

> 
>   builtin/clone.c          |  6 ++++++
>   t/t5606-clone-options.sh | 12 ++++++++++++
>   2 files changed, 18 insertions(+)

The new config variable should be documented in Documentation/config/clone.txt (which
gets added to text of the git-config(1) man page).


Cheers,

Philippe.

[1] https://lore.kernel.org/git/xmqqa6le5x1f.fsf_-_@gitster.g/T/#m1c2e522368ec4c9d458fcf6d83e75afab1632306
[2] https://lore.kernel.org/git/YRbYWR+X8vSq8CYe@google.com/