Re: [PATCH v6] clone: set submodule.recurse=true if submodule.stickyRecursiveClone enabled
- From
Philippe Blain <levraiphilippeblain@gmail.com>
- Date
- Aug 18, 2021, 22:40 UTC
- Message-ID
- <4103806f-6d88-2756-8853-7e16e56ad425@gmail.com>
- In-Reply-To
- <pull.1006.v6.git.1628903396783.gitgitgadget@gmail.com>
Hi Mahi, Emily and Junio,
Le 2021-08-13 à 21:09, Mahi Kolla via GitGitGadget a écrit :
Show 11 quoted lines
> 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.
Show 15 quoted lines
> 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.
Show 8 quoted lines
> > 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" :)
Show 22 quoted lines
> > 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/