patch, 2 partsclone: update submodule.recurse in config when using --recurse-submodule

When running 'git clone --recurse-submodules', developers expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules.The submitted code updates the 'submodule.recurse' config value to true when 'git clone' is run with the '--recurse-submodules' option.

Signed-off-by: Mahi Kolla mahikolla@google.com

Mahi Kolla (2):
  clone: update submodule.recurse in config when using
    --recurse-submodule
  clone: update submodule.recurse in config when using
    --recurse-submodule

 builtin/clone.c          | 1 +
 t/t5606-clone-options.sh | 7 +++++++
 2 files changed, 8 insertions(+)

base-commit: 940fe202adcbf9fa1825c648d97cbe1b90d26aec
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1006%2F24mahik%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1006/24mahik/master-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1006

From: Mahi Kolla <mahikolla@google.com>

When running 'git clone --recurse-submodules', developers expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules.The submitted code updates the 'submodule.recurse' config value to true when 'git clone' is run with the '--recurse-submodules' option.

Signed-off-by: Mahi Kolla <mahikolla@google.com>
---
 builtin/clone.c          | 1 +
 t/t5606-clone-options.sh | 7 +++++++
 2 files changed, 8 insertions(+)

From: Mahi Kolla <mahikolla@google.com>

When running 'git clone --recurse-submodules', developers expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules.The submitted code updates the 'submodule.recurse' config value to true when 'git clone' is run with the '--recurse-submodules' option.

Signed-off-by: Mahi Kolla <mahikolla@google.com>
---
 t/t5606-clone-options.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

When running 'git clone --recurse-submodules', developers expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules.The submitted code updates the 'submodule.recurse' config value to true when 'git clone' is run with the '--recurse-submodules' option.

Signed-off-by: Mahi Kolla mahikolla@google.com

Mahi Kolla (3):
  clone: update submodule.recurse in config when using
    --recurse-submodule
  clone: update submodule.recurse in config when using
    --recurse-submodule
  clone test: update whitespace according to style guide

 builtin/clone.c          | 1 +
 t/t5606-clone-options.sh | 7 +++++++
 2 files changed, 8 insertions(+)

base-commit: 940fe202adcbf9fa1825c648d97cbe1b90d26aec
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1006%2F24mahik%2Fmaster-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1006/24mahik/master-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1006

Range-diff vs v1:

 1:  fea3d6d72b6 = 1:  fea3d6d72b6 clone: update submodule.recurse in config when using --recurse-submodule
 2:  dd13a65ef0f = 2:  dd13a65ef0f clone: update submodule.recurse in config when using --recurse-submodule
 -:  ----------- > 3:  020eaa2c819 clone test: update whitespace according to style guide

From: Mahi Kolla <mahikolla@google.com>

When running 'git clone --recurse-submodules', developers expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules.The submitted code updates the 'submodule.recurse' config value to true when 'git clone' is run with the '--recurse-submodules' option.

Signed-off-by: Mahi Kolla <mahikolla@google.com>
---
 builtin/clone.c          | 1 +
 t/t5606-clone-options.sh | 7 +++++++
 2 files changed, 8 insertions(+)

From: Mahi Kolla <mahikolla@google.com>

When running 'git clone --recurse-submodules', developers expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules.The submitted code updates the 'submodule.recurse' config value to true when 'git clone' is run with the '--recurse-submodules' option.

Signed-off-by: Mahi Kolla <mahikolla@google.com>
---
 t/t5606-clone-options.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

From: Mahi Kolla <mahikolla@google.com>

Previously, the code used spaces to appropriately format. The spaces have been replaced with tabs to follow style guide standards.

Signed-off-by: Mahi Kolla <mahikolla@google.com>
---
 t/t5606-clone-options.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

When running 'git clone --recurse-submodules', developers expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules.The submitted code updates the 'submodule.recurse' config value to true when 'git clone' is run with the '--recurse-submodules' option.

Signed-off-by: Mahi Kolla mahikolla@google.com

Mahi Kolla (4):
  clone: update submodule.recurse in config when using
    --recurse-submodule
  clone: update submodule.recurse in config when using
    --recurse-submodule
  clone test: update whitespace according to style guide
  clone: update whitespace according to style guide

 builtin/clone.c          | 1 +
 t/t5606-clone-options.sh | 7 +++++++
 2 files changed, 8 insertions(+)

base-commit: 940fe202adcbf9fa1825c648d97cbe1b90d26aec
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1006%2F24mahik%2Fmaster-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1006/24mahik/master-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1006

Range-diff vs v2:

 1:  fea3d6d72b6 = 1:  fea3d6d72b6 clone: update submodule.recurse in config when using --recurse-submodule
 2:  dd13a65ef0f = 2:  dd13a65ef0f clone: update submodule.recurse in config when using --recurse-submodule
 3:  020eaa2c819 = 3:  020eaa2c819 clone test: update whitespace according to style guide
 -:  ----------- > 4:  f3ddb344b49 clone: update whitespace according to style guide

From: Mahi Kolla <mahikolla@google.com>

When running 'git clone --recurse-submodules', developers expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules.The submitted code updates the 'submodule.recurse' config value to true when 'git clone' is run with the '--recurse-submodules' option.

Signed-off-by: Mahi Kolla <mahikolla@google.com>
---
 builtin/clone.c          | 1 +
 t/t5606-clone-options.sh | 7 +++++++
 2 files changed, 8 insertions(+)

Le 2021-08-02 à 19:23, Mahi Kolla via GitGitGadget a écrit :

missing space after period here. Also, maybe I would say "some developers" or "developers might expect", or something like that.

The submitted code updates the 'submodule.recurse' config value to true when 'git clone' is run with the '--recurse-submodules' option.

We usually use the imperative form for commit messages, like if giving an order to the code base to improve itself. So you might word this simply as:

Set 'submodule.recurse' to true when 'git clone' is run with '--recurse-submodules'.

I think that this change is a big enough behaviour change to warrant that an "advice" message be shown to the user, informing them that 'submodule.recurse' has been set to true, and maybe point users to 'gitsubmodules(7) to learn what this configuration entails.

This should be 'test_cmp_config', which checks that a specific config variable has the expected value. 'test_config' is used to set a config for the duration of the test, which is not what you want to do here. I see you've changed it in a following patch, but not to the right thing. As I wrote in my response to your cover letter, this series should be as a single patch.

Thanks for working on this!

Philippe.

Thanks Philippe for the feedback! As a first time contributor, I definitely struggled a bit with understanding how to submit patches and how to amend commits versus stacking changes across commits. Thank you for your patience! :)

On Mon, Aug 2, 2021 at 8:20 PM Philippe Blain <levraiphilippeblain@gmail.com> wrote:

That makes sense, will update.

Got it, will update.

I will definitely update the 'git-clone' man page to include some documentation on this change. Should the "advice" to the user show up in the output message once the clone command is run?

Ah, thank you for pointing me to the right testing function. Definitely, agree with the single patch. I'll update the corresponding files and submit them all as one V2 patch.

Thank you so much!

From: Mahi Kolla <mahikolla@google.com>

When running 'git clone --recurse-submodules', developers expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules.The submitted code updates the 'submodule.recurse' config value to true when 'git clone' is run with the '--recurse-submodules' option.

Signed-off-by: Mahi Kolla <mahikolla@google.com>
---
 t/t5606-clone-options.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

From: Mahi Kolla <mahikolla@google.com>

Previously, the code used spaces to appropriately format. The spaces have been replaced with tabs to follow style guide standards.

Signed-off-by: Mahi Kolla <mahikolla@google.com>
---
 t/t5606-clone-options.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

From: Mahi Kolla <mahikolla@google.com>

Previously, the code used spaces to appropriately format. The spaces have been replaced with tabs to follow style guide standards.

Signed-off-by: Mahi Kolla <mahikolla@google.com>
---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Mahi,

Le 2021-08-02 à 19:23, Mahi Kolla via GitGitGadget a écrit :

This could all be done in a single patch. As long as the patches are not merged to the 'next' branch, you can simply force-push and re '/submit' on Gitgitgadget.

I think this change would require a mention in the 'git-clone(1)' man page.

I'm very much in favor of a change like this. It is a pretty big change in behaviour, but I really believe this is a good way forward. Thanks for getting the ball rolling on this!

"Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes:

Some developers might, but "developers expect" as if we speak for everybody is a bold statement to make that needs to be substantiated, I would think. Is this something easy to make opt-in, e.g. "git clone --recurse-submodules=sticky" or something?

From: Mahi Kolla <mahikolla@google.com>

When running 'git clone --recurse-submodules', developers might expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules. Set 'submodule.recurse' to true when 'git clone' is run with '--recurse-submodules'.

Since V1: Updated test and 'git clone' man page. Also updated commit message.

Signed-off-by: Mahi Kolla <mahikolla@google.com>
---
    clone: update submodule.recurse in config when using --recurse-submodule
    
    When running 'git clone --recurse-submodules', developers might expect
    various other commands such as 'pull' and 'checkout' to also run
    recursively into submodules. Set 'submodule.recurse' to true when 'git
    clone' is run with '--recurse-submodules'.
    
    Since V1: Updated test and 'git clone' man page. Also updated commit
    message.
    
    Signed-off-by: Mahi Kolla mahikolla@google.com
    
    cc: Philippe Blain levraiphilippeblain@gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1006%2F24mahik%2Fmaster-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1006/24mahik/master-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1006

Range-diff vs v3:

 1:  fea3d6d72b6 ! 1:  73937d48a53 clone: update submodule.recurse in config when using --recurse-submodule
     @@ Metadata
       ## Commit message ##
          clone: update submodule.recurse in config when using --recurse-submodule
      
     -    When running 'git clone --recurse-submodules', developers expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules.The submitted code updates the 'submodule.recurse' config value to true when 'git clone' is run with the '--recurse-submodules' option.
     +    When running 'git clone --recurse-submodules', developers might expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules. Set 'submodule.recurse' to true when 'git clone' is run with '--recurse-submodules'.
     +
     +    Since V1: Updated test and 'git clone' man page. Also updated commit message.
      
          Signed-off-by: Mahi Kolla <mahikolla@google.com>
      
     + ## Documentation/git-clone.txt ##
     +@@ Documentation/git-clone.txt: branch of some repository for search indexing.
     + 	This option can be given multiple times for pathspecs consisting
     + 	of multiple entries.  The resulting clone has `submodule.active` set to
     + 	the provided pathspec, or "." (meaning all submodules) if no
     +-	pathspec is provided.
     ++	pathspec is provided. In addition, `submodule.recurse` is set to true.
     + +
     + Submodules are initialized and cloned using their default settings. This is
     + equivalent to running
     +
       ## builtin/clone.c ##
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       					   strbuf_detach(&sb, NULL));
       		}
       
     -+                string_list_append(&option_config, "submodule.recurse=true");
     ++		string_list_append(&option_config, "submodule.recurse=true");
       		if (option_required_reference.nr &&
       		    option_optional_reference.nr)
       			die(_("clone --recursive is not compatible with "
     @@ t/t5606-clone-options.sh: test_expect_success 'setup' '
       
      +test_expect_success 'clone --recurse-submodules sets submodule.recurse=true' '
      +
     -+        git clone --recurse-submodules parent clone-rec-submodule &&
     -+        test_config_global submodule.recurse true 
     ++	git clone --recurse-submodules parent clone-rec-submodule &&
     ++	test_cmp_config -C clone-rec-submodule true submodule.recurse
      +
      +'
      +
 2:  dd13a65ef0f < -:  ----------- clone: update submodule.recurse in config when using --recurse-submodule
 3:  020eaa2c819 < -:  ----------- clone test: update whitespace according to style guide
 4:  f3ddb344b49 < -:  ----------- clone: update whitespace according to style guide

 Documentation/git-clone.txt | 2 +-
 builtin/clone.c             | 1 +
 t/t5606-clone-options.sh    | 7 +++++++
 3 files changed, 9 insertions(+), 1 deletion(-)

"Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes:

Please wrap overlong lines in your proposed log message to say 70 or so columns.

Some developers might expect, but wouldn't some others want to see this not set to true, but want to recurse only into some but not all submodules?

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?

Hi Junio,

Thank you for your feedback!

On Mon, Aug 9, 2021 at 2:15 PM Junio C Hamano <gitster@pobox.com> wrote:

Ah, my bad, will do so going forward.

I definitely agree with this. Currently, the `--recurse-submodules` option takes in 1 parameter, a pathspec to the submodule they would like to initialize. `submodule.active` stores this path or `.` if no path is specified. Accordingly, `submodule.recurse=true` will only apply to the submodules specified by the user in `submodule.active`. This way users can ensure commands are run recursively only in submodules they have listed as active.

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. When a user runs `git clone --recurse-submodules` an advice message will pop up alerting them `submodule.recurse=true`, what this means for other commands' functionality, and how to change it back (`git -C dst-dir config submodule.recurse false`). This also gives the user the option to turn off the advice message if they don't need it.

I'll also update the appropriate documentation to better explain how setting submodule.recurse=true effects workflow.

Let me know what you think!

Best, Mahi Kolla

Mahi Kolla <mahikolla@google.com> writes:

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

Hi Junio,

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

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:

...

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.

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/

Hi Phillippe and Junio,

On Tue, Aug 10, 2021 at 4:04 PM Philippe Blain <levraiphilippeblain@gmail.com> wrote:

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.

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

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.

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

Junio C Hamano <gitster@pobox.com> writes:

Let me outline some general rules on changing the behaviour of the system used around here.

First of all, if a proposed change of behaviour is a bugfix, the following does not apply [*1*].

When a new behaviour is made available to those who want to use it, it starts as an opt-in feature.

 - Existing users will not be surprised by a familiar command
   suddenly changing its behaviour.  If users keep using the system
   the same way as they used it before, the system will behave the
   same way, without changing the behaviour.

 - Those who want to use the new behaviour need to do something to
   explicitly trigger it (with a command line option, configuration
   variable, a new command, etc.)

Over time, a behaviour that used to be a "new way" may just become "one of the two ways available", and it may even turn out to be a more desirable one between the two. At that point, we may propose to flip the default, with a migration plan that is carefully designed to avoid breaking existing users.

Even if it were an *improvement* to set the configuration variable, it is not an excuse to suddenly change the behaviour of the command for users who do not ask. It needs to start as an optional feature, and if we really like it and manage to convince majority users to also like the new way, we may even consider making it the default, but it is way too premature to do so.

Unless we can argue that the current behaviour *is* buggy, that is.

Thanks.

[Footnote]

*1* A change that we have to say "not all users may be happy with
    this new behaviour" or "developers would be surprised by the new
    behaviour" cannot be a bugfix.

From: Mahi Kolla <mahikolla@google.com>

Currently, 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' to true at this step could make for a simpler workflow by eliminating 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 feature.experimental flag.

Since V1: Made this an opt in feature under the experimental flag. Updated tests to reflect this design change. Also updated commit message.

Signed-off-by: Mahi Kolla <mahikolla@google.com>
---
    clone: set submodule.recurse=true if feature.experimental flag 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' preference in
    regards to making submodule.recurse=true a default config value in the
    future, deploy this feature under the opt in feature.experimental flag.
    
    Since V1: Made this an opt in feature under the experimental flag.
    Updated tests to reflect this design change. Also updated commit
    message.
    
    Signed-off-by: Mahi Kolla mahikolla@google.com
    
    cc: Philippe Blain levraiphilippeblain@gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1006%2F24mahik%2Fmaster-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1006/24mahik/master-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1006

Range-diff vs v4:

 1:  73937d48a53 ! 1:  2c6ffe00736 clone: update submodule.recurse in config when using --recurse-submodule
     @@ Metadata
      Author: Mahi Kolla <mahikolla@google.com>
      
       ## Commit message ##
     -    clone: update submodule.recurse in config when using --recurse-submodule
     +    clone: set submodule.recurse=true if user enables feature.experimental flag
      
     -    When running 'git clone --recurse-submodules', developers might expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules. Set 'submodule.recurse' to true when 'git clone' is run with '--recurse-submodules'.
     +    Currently, 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' to true at this step could make for a simpler workflow by eliminating 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 feature.experimental flag.
      
     -    Since V1: Updated test and 'git clone' man page. Also updated commit message.
     +    Since V1: Made this an opt in feature under the experimental flag. Updated tests to reflect this design change. Also updated commit message.
      
          Signed-off-by: Mahi Kolla <mahikolla@google.com>
      
     - ## Documentation/git-clone.txt ##
     -@@ Documentation/git-clone.txt: branch of some repository for search indexing.
     - 	This option can be given multiple times for pathspecs consisting
     - 	of multiple entries.  The resulting clone has `submodule.active` set to
     - 	the provided pathspec, or "." (meaning all submodules) if no
     --	pathspec is provided.
     -+	pathspec is provided. In addition, `submodule.recurse` is set to true.
     - +
     - Submodules are initialized and cloned using their default settings. This is
     - equivalent to running
     -
       ## builtin/clone.c ##
     +@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     + 	struct remote *remote;
     + 	int err = 0, complete_refs_before_fetch = 1;
     + 	int submodule_progress;
     ++	int experimental_flag;
     + 
     + 	struct transport_ls_refs_options transport_ls_refs_options =
     + 		TRANSPORT_LS_REFS_OPTIONS_INIT;
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       					   strbuf_detach(&sb, NULL));
       		}
       
     -+		string_list_append(&option_config, "submodule.recurse=true");
     ++		if(!git_config_get_bool("feature.experimental", &experimental_flag) &&
     ++		    experimental_flag) {
     ++		    string_list_append(&option_config, "submodule.recurse=true");
     ++		}
     ++
       		if (option_required_reference.nr &&
       		    option_optional_reference.nr)
       			die(_("clone --recursive is not compatible with "
     @@ t/t5606-clone-options.sh: test_expect_success 'setup' '
       
       '
       
     -+test_expect_success 'clone --recurse-submodules sets submodule.recurse=true' '
     ++test_expect_success 'feature.experimental flag manipulates submodule.recurse value' '
     ++
     ++	test_config_global feature.experimental true &&
     ++	git clone --recurse-submodules parent clone_recurse_true &&
     ++	test_cmp_config -C clone_recurse_true true submodule.recurse &&
      +
     -+	git clone --recurse-submodules parent clone-rec-submodule &&
     -+	test_cmp_config -C clone-rec-submodule true submodule.recurse
     ++	test_config_global feature.experimental false &&
     ++	git clone --recurse-submodules parent clone_recurse_false &&
     ++	test_expect_code 1 git -C clone_recurse_false config --get submodule.recurse
      +
      +'
      +

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

"Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes:

Please wrap overlong lines in your proposed log message to say 70 or so columns.

This does not belong to the commit log message proper. Noting the difference between the version being submitted and the pervious one this way is a way to help reviewers and is very much appreciated, but please do so below the three-dash line below your sign-off.

The proposed approach misuses feature.experimental flag, which was designed to turn on many new features at once. The features covered by the flag share one common trait: they all have gained consensus that in the longer term we would hopefully be able to make it on by default, and give early adopters an easy way to turn them all on.

I do not think setting submodule.recurse=true upon "clone --recurse" falls into that category just yet. If we were to make this opt-in, we'd want a separate flag, so that those early adopters who are dogfooding other features that have consensus that they are hopefully the way of the future won't have to be forced into this separate feature.

Perhaps a separate (and new) configuration variable (in ~/.gitconfig perhaps) can be used as that opt-in flag (I wonder if the existing submodule.recurse variable can be that opt-in flag, though).

On Wed, Aug 11, 2021 at 09:20:58PM -0700, Junio C Hamano wrote:

I'd like to open discussions to get said consensus :)

It seems surprising to me that a user would want to clone with all the submodules fetched *without* intending to then use superproject-plus-submodules together recursively. I would like to hear more about the use case you have in mind, Junio.

One scenario that did come to mind when I discussed this with Mahi is that a user may provide a pathspec to --recurse-submodules (that is, "yes, this repo has submodules a/ and b/, but I only care about the contents of submodule a/") - and in that case, --recurse-submodules seems to do the right thing with or without Mahi's change.

It seemed to me that trying out this change on feature.experimental flag was the right approach, because users with that flag have already volunteered to be testers for upcoming behavior changes; this seems like one such that is likely to be welcome. By contrast, turning the behavior on with a separate config variable reduces the pool of testers essentially to "users who know about this change" - or, to be more reductive, "a handful of users at Google who we Google Git contributors already know want this change". I recommended to Mahi that we stick this feature under 'feature.experimental' because I really wanted to hear from more users than just Googlers.

Do you mean something like "git config --global submodule.recurse TryTheNewThingPlease"? I guess it could work - repos that use a pathspec in that slot would still have the pathspec configured locally, repos that have submodule.recurse intentionally unset wouldn't know what to do with the junk string, and repos that have submodule.recurse intentionally set to true would still have that true override the global value.

Or else I misunderstood you...

 - Emily

Hi Emily,

Le 2021-08-12 à 19:54, Emily Shaffer a écrit :

Mahi, since you're using Gitgitgadget, you would put this "since v1" content in the PR description, and Gitgitgadget will append it under the three-dash line when you /submit :) (Do keep the CC's automatically added by GGG so that your next version is CC'ed to those that participated in earlier rounds).

I'm not sure what you mean by "the right thing" here. '--recurse-submodules=a' would set 'submodule.active' to 'a', which means "when command are asked to recurse into submodules, I only care about submodules a", but it does not do anything to 'submodule.recurse=true', which means "I do not ever want to type '--recurse-submodules', always use this behaviour for all commands that have that flag, except clone and ls-files. Unless I'm missing something :)

I agree that we would not want yet another config variable that users would have to set. If people know about submodule.recurse and want to always use this behaviour, they already have it in their ~/.gitconfig, so they do not need a new variable. If they do not know about submodule.recurse, then they probably won't learn about this new variable either ;) That's why I suggested to Mahi that in any case it would be a good thing that 'git clone --recurse-submodules' would at least inform users, using an advice, that they might want to set submodule.recurse.

Regarding feature.experimental, I do not have a strong opinion. I don't think the population of Git users that have this flag set is representative of the total population of Git users, unfortunately. But I agree it's better than nothing.

Here I think you are confusing submodule.active (which takes a pathspec) and submodule.recurse (which takes a boolean).

Cheers,

Philippe.

Hi all,

Thank you all for the great feedback! I'm learning a lot as a first-time contributor :) I will be wrapping my internship this week and will continue contributing externally.

Got it, thank you!

When discussing with the team, we revisited the feature.experimental design. As we have yet to gain strong consensus on making this a default config value, we've decided to ship it under a different config value: submodule.stickyRecursiveClone. Now, if the user sets submodule.stickyRecursiveClone=true, when they run git clone --recurse-submodules, we will set submodule.recurse=true. While this may mean a smaller dataset (only people who know of this flag), we can still collect valuable data.

As for the advice message, I agree that would be a really useful feature. I'll submit that as a different patch.

Unfortunately, the submodule.recurse variable can't be used as the opt-in flag because this would cause commands to run recursively even if developers don't have submodules in their project (aka don't run git clone --recurse-submodules). That's why the alternate config value seems a better choice at the moment.

Let me know what you guys think!

Best, Mahi Kolla

Mahi Kolla <mahikolla@google.com> writes:

If you cloned without recurse-submodules and lack submodules, wouldn't it be a no-op to have submodule.recurse set to true, so it would not hurt anyway?

IOW, that may already solve the original problem you wanted to solve---those who want their submodules recursively descended into by default can just set submodule.recurse to true (in ~/.gitconfig presumably) and after "git clone" with --recurse-submodules they will get what they want, no? Am I missing something obvious?

Thanks.

Philippe Blain <levraiphilippeblain@gmail.com> writes:

Sigh, but I have to agree with you.

I really hoped that in a recursive clone of a repository with two submodules A and B, when made with --recurse-submodules=A, "git grep ." would look for things in the superproject and submodule A as Emily seemed to have meant by her "the right thing", but you are correct. We only set .active but we do not set .recurse, so "git grep ." in the superproject does not descend into neither submodules without being told.

If it did "the right thing" as Emily said, it would have been much easier to justify the change being proposed as a simple fix for the bug that --recurse-submodules without pathspec does one thing (i.e. setting things up not to recurse for later "grep" etc.") and the same option with "everything matches" pathspec "." does a different thing (i.e. always to recurse).

The discrepancy would have given us an excuse, an argument for changing the behaviour for the former to match the latter. Some users may have deliberately built their workflow relying on the distinction and the result still may give them a regression, but at least it would have gave us a viable justification:

    A command run without pathspec means the entire tree and it is
    the same as running it with pathspec '.' in the rest of Git, but
    the way "git clone --recurse-submodules" handles its optional
    pathspec is inconsistent.  Treat "clone --recurse-submodules"
    without pathspec as if it came with pathspec '.' and give the
    same configuration.

But unfortunately it does not seem to be the case.

Emily Shaffer <emilyshaffer@google.com> writes:

You may need full forest of submodules with the superproject to build your ware (i.e. you'd probably want to clone and fetch-update them), but you may only be working on the sources in a small subset of submodules and do not need your recursive grep or diff to go outside that subset, for example. You'd need to ask the people who recursively clone and not set submodule.recurse to true (I am not among them).

Please be a bit more specific about "the right thing". Do you mean "the submodules that matched the pathspec gets recursed into by later operations"?

If so, "git clone --resurse-submodules=. $from_there" may perhaps be the "there is no way to we make this opt-in?" I have been asking about (not "asking for")?

Yes, if we already have a consensus that a proposed change is something we hope to be desirable, then feature.experimental is a good way to see if early adopters can find problems in their real world use, as these volunteers may include audiences with different use pattern from the original advocates of a particular feature, who might have dogfooded the new feature to gain consensus that it may want to become the default.

By the way, I am not fundamentally opposed to the feature being proposed. I would imagine that such a feature would be liked by those who want to keep things simpler. I however am hesitant to see it pushed too hastily without considering if it harms existing users with different preferences.

IOW, I was primarily reacting to the apparent wrong order in which things are being done, first throwing this into feature.experimental before we have gathered enough confidence that it may be a good thing to do by having it in shipped version as an opt-in feature.

Thanks.

On Thu, Aug 12, 2021 at 09:34:47PM -0700, Junio C Hamano wrote:

Yeah, since writing my reply I was very helpfully reinformed on the convention around 'feature.experimental' by Jonathan N off-list. Thanks for being patient with me.

I think the right move, then, is to explore whether your suggestion in
https://lore.kernel.org/git/xmqqeeaxw28z.fsf%40gitster.g is appropriate
- I have the sense that it is, but I want to make sure to think it
through before I say so for sure. 

 - Emily

Emily Shaffer <emilyshaffer@google.com> writes:

Not that one---it was a 40% tongue-in-cheek comment, and does not deserve to be called a suggestion.

On Fri, Aug 13, 2021 at 01:30:22PM -0700, Junio C Hamano wrote:

Ah well ;)

Anyway, I think it does not make sense, as behavior starts to change for people who already cloned expecting not to recurse (Jonathan N says this is the case for his Rust checkout, for example) - and apparently 'submodule.recurse=true' has some weird edge cases for commands which are happy to run out-of-repo.

Mahi mentioned wanting to rework her commit to use a config besides 'feature.experimental' for this same behavior, so hopefully we will see that change come through soon - but today is also the last day of her internship, so we may not be so lucky.

 - Emily

This change is pretty much good to go! I implemented it under `submodule.stickyRecursiveClone`. The commit is actually in the PR. Just wanted to hear more from you guys before submitting :)

Emily Shaffer <emilyshaffer@google.com> writes:

You may (or may not) have noticed it, but the entry for the topic in the last "What's cooking" report pretends as if we have already concensus that it is a good thing to do and be made default. As it seems to be valid use cases to have submodule.recurse not set to true even when you have submodules (a clone of Rust Jonathan N uses you mentioned in the other message, perhaps?), it probably does make sense to initially make this opt-in and also keep opt-in, not as a candidate for future default, but we'll see.

Thanks.

Mahi Kolla via GitGitGadget wrote:

You are missing a space after the 'if'.

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.

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

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:

 1:  2c6ffe00736 ! 1:  e668fa403cf clone: set submodule.recurse=true if user enables feature.experimental flag
     @@
       ## Metadata ##
     -Author: Mahi Kolla <mahikolla@google.com>
     +Author: Mahi Kolla <mkolla2@illinois.edu>
      
       ## Commit message ##
     -    clone: set submodule.recurse=true if user enables feature.experimental flag
     +    clone: set submodule.recurse=true if submodule.stickyRecursiveClone enabled
      
     -    Currently, 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' to true at this step could make for a simpler workflow by eliminating 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 feature.experimental flag.
     +    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.
      
     -    Since V1: Made this an opt in feature under the experimental flag. Updated tests to reflect this design change. Also updated commit message.
     -
     -    Signed-off-by: Mahi Kolla <mahikolla@google.com>
     +    Signed-off-by: Mahi Kolla <mkolla2@illinois.edu>
      
       ## builtin/clone.c ##
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       	struct remote *remote;
       	int err = 0, complete_refs_before_fetch = 1;
       	int submodule_progress;
     -+	int experimental_flag;
     ++	int sticky_recursive_clone;
       
       	struct transport_ls_refs_options transport_ls_refs_options =
       		TRANSPORT_LS_REFS_OPTIONS_INIT;
     @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       					   strbuf_detach(&sb, NULL));
       		}
       
     -+		if(!git_config_get_bool("feature.experimental", &experimental_flag) &&
     -+		    experimental_flag) {
     ++		if (!git_config_get_bool("submodule.stickyRecursiveClone", &sticky_recursive_clone)
     ++		    && sticky_recursive_clone) {
      +		    string_list_append(&option_config, "submodule.recurse=true");
      +		}
      +
     @@ t/t5606-clone-options.sh: test_expect_success 'setup' '
       
       '
       
     -+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
      +

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

"Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes:

This variable does not have to be in such a wider scope, I think.

Just in this scope, where "struct string_list_item *item" and "struct strbuf sb" are declared, is where an extra int variable, which receives the configuration value, needs to exist.

Also, for a variable that is used only to receive value from git_config_get_bool(), immediately to be used and then never used again, we do not need such a long and descriptive name.

We do not need {} around a single statement block.

Taken together, perhaps like the attached.

I'll queue the patch posted as-is for now.

Thanks.

On Sat, Aug 14, 2021 at 11:05:41AM -0700, Junio C Hamano wrote:

I like the changes you propose here. It also looks to me like you wanted to wait for Mahi to send the updates herself, which I appreciate.

Mahi's internship ended last week and I believe she told me she's planning to be very busy this week; so if we don't hear from her by this time next week, I'll send a reroll with these changes (unless you want to just take them yourself without the list churn).

 - Emily

Emily Shaffer <emilyshaffer@google.com> writes:

I'd make a "fixup!" out of the message you are responding to, and then queue it on top of what was posted by Mahi.

Then I'll send the fixup to the list as a reply to this message, so that later you and Mahi can ack (or further update) it.

After that, I can squash the fixup into the patch and merge it down (unless there are objections from other places, that is).

Thanks.

Narrow the scope of a temporary variable used only once and immediately die, and rename it to a shorter, throw-away name.

Also lose a {} around a single statement block.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * To be squashed into mk/clone-recurse-submodules topic 1a0e8231
   (clone: set submodule.recurse=true if
   submodule.stickyRecursiveClone enabled, 2021-08-14)

 builtin/clone.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

On Wed, Aug 18, 2021 at 01:15:56PM -0700, Junio C Hamano wrote:

Acked-by: Emily Shaffer <emilyshaffer@google.com>

Thanks.

On Thu, Aug 19, 2021 at 10:41:03AM -0700, Emily Shaffer wrote:

Having not heard from Mahi last week, if you are happy with the patch+squash, I think it would be OK to take this without waiting longer for her. Up to you of course.

 - Emily

Emily Shaffer <emilyshaffer@google.com> writes:

Thanks for a nudge.

Hi Mahi, Emily and Junio,

Le 2021-08-13 à 21:09, Mahi Kolla via GitGitGadget a écrit :

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 :

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.

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.

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" :)

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.

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/