[PATCH 0/2] clone: update submodule.recurse in config when using --recurse-submodule

[PATCH 0/2] clone: update submodule.recurse in config when using --recurse-submodule

[Mahi Kolla via GitGitGadget]

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

[PATCH 1/2] clone: update submodule.recurse in config when using --recurse-submodule

[Mahi Kolla via GitGitGadget]

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(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index 66fe66679c8..f41fd1afb66 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1130,6 +1130,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 					   strbuf_detach(&sb, NULL));
 		}
 
+                string_list_append(&option_config, "submodule.recurse=true");
 		if (option_required_reference.nr &&
 		    option_optional_reference.nr)
 			die(_("clone --recursive is not compatible with "
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 3a595c0f82c..3daef8c941f 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -16,6 +16,13 @@ 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 
+
+'
+
 test_expect_success 'clone -o' '
 
 	git clone -o foo parent clone-o &&
-- 
gitgitgadget

[PATCH 2/2] clone: update submodule.recurse in config when using --recurse-submodule

[Mahi Kolla via GitGitGadget]

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

diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 3daef8c941f..69c4bacf52f 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -19,7 +19,7 @@ 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 config submodule.recurse true
 
 '
 
-- 
gitgitgadget

[PATCH v2 0/3] clone: update submodule.recurse in config when using --recurse-submodule

[Mahi Kolla via GitGitGadget]

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

-- 
gitgitgadget

[PATCH v2 1/3] clone: update submodule.recurse in config when using --recurse-submodule

[Mahi Kolla via GitGitGadget]

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(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index 66fe66679c8..f41fd1afb66 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1130,6 +1130,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 					   strbuf_detach(&sb, NULL));
 		}
 
+                string_list_append(&option_config, "submodule.recurse=true");
 		if (option_required_reference.nr &&
 		    option_optional_reference.nr)
 			die(_("clone --recursive is not compatible with "
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 3a595c0f82c..3daef8c941f 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -16,6 +16,13 @@ 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 
+
+'
+
 test_expect_success 'clone -o' '
 
 	git clone -o foo parent clone-o &&
-- 
gitgitgadget

[PATCH v2 2/3] clone: update submodule.recurse in config when using --recurse-submodule

[Mahi Kolla via GitGitGadget]

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

diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 3daef8c941f..69c4bacf52f 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -19,7 +19,7 @@ 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 config submodule.recurse true
 
 '
 
-- 
gitgitgadget

[PATCH v2 3/3] clone test: update whitespace according to style guide

[Mahi Kolla via GitGitGadget]

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

diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 69c4bacf52f..1a3f1e9ab18 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -18,8 +18,8 @@ test_expect_success 'setup' '
 
 test_expect_success 'clone --recurse-submodules sets submodule.recurse=true' '
 
-        git clone --recurse-submodules parent clone-rec-submodule &&
-        git config submodule.recurse true
+	git clone --recurse-submodules parent clone-rec-submodule &&
+	git config submodule.recurse true
 
 '
 
-- 
gitgitgadget

[PATCH v3 0/4] clone: update submodule.recurse in config when using --recurse-submodule

[Mahi Kolla via GitGitGadget]

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

-- 
gitgitgadget

[PATCH v3 1/4] clone: update submodule.recurse in config when using --recurse-submodule

[Mahi Kolla via GitGitGadget]

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(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index 66fe66679c8..f41fd1afb66 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1130,6 +1130,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 					   strbuf_detach(&sb, NULL));
 		}
 
+                string_list_append(&option_config, "submodule.recurse=true");
 		if (option_required_reference.nr &&
 		    option_optional_reference.nr)
 			die(_("clone --recursive is not compatible with "
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 3a595c0f82c..3daef8c941f 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -16,6 +16,13 @@ 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 
+
+'
+
 test_expect_success 'clone -o' '
 
 	git clone -o foo parent clone-o &&
-- 
gitgitgadget

Re: [PATCH v3 1/4] clone: update submodule.recurse in config when using --recurse-submodule

[Philippe Blain]

Le 2021-08-02 à 19:23, Mahi Kolla via GitGitGadget a écrit :
> 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.

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

> 
> Signed-off-by: Mahi Kolla <mahikolla@google.com>
> ---
>   builtin/clone.c          | 1 +
>   t/t5606-clone-options.sh | 7 +++++++
>   2 files changed, 8 insertions(+)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 66fe66679c8..f41fd1afb66 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1130,6 +1130,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>   					   strbuf_detach(&sb, NULL));
>   		}
>   
> +                string_list_append(&option_config, "submodule.recurse=true");
>   		if (option_required_reference.nr &&
>   		    option_optional_reference.nr)
>   			die(_("clone --recursive is not compatible with "

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.

> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index 3a595c0f82c..3daef8c941f 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -16,6 +16,13 @@ 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

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.

Re: [PATCH v3 1/4] clone: update submodule.recurse in config when using --recurse-submodule

[Mahi Kolla]

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:
>
>
>
> Le 2021-08-02 à 19:23, Mahi Kolla via GitGitGadget a écrit :
> > 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.
>
> missing space after period here. Also, maybe I would say "some developers"
> or "developers might expect", or something like that.
>

That makes sense, will update.

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

Got it, will update.

> >
> > Signed-off-by: Mahi Kolla <mahikolla@google.com>
> > ---
> >   builtin/clone.c          | 1 +
> >   t/t5606-clone-options.sh | 7 +++++++
> >   2 files changed, 8 insertions(+)
> >
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index 66fe66679c8..f41fd1afb66 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -1130,6 +1130,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >                                          strbuf_detach(&sb, NULL));
> >               }
> >
> > +                string_list_append(&option_config, "submodule.recurse=true");
> >               if (option_required_reference.nr &&
> >                   option_optional_reference.nr)
> >                       die(_("clone --recursive is not compatible with "
>
> 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.
>

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?

> > diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> > index 3a595c0f82c..3daef8c941f 100755
> > --- a/t/t5606-clone-options.sh
> > +++ b/t/t5606-clone-options.sh
> > @@ -16,6 +16,13 @@ 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
>
> 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.

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!

[PATCH v3 2/4] clone: update submodule.recurse in config when using --recurse-submodule

[Mahi Kolla via GitGitGadget]

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

diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 3daef8c941f..69c4bacf52f 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -19,7 +19,7 @@ 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 config submodule.recurse true
 
 '
 
-- 
gitgitgadget

[PATCH v3 3/4] clone test: update whitespace according to style guide

[Mahi Kolla via GitGitGadget]

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

diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 69c4bacf52f..1a3f1e9ab18 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -18,8 +18,8 @@ test_expect_success 'setup' '
 
 test_expect_success 'clone --recurse-submodules sets submodule.recurse=true' '
 
-        git clone --recurse-submodules parent clone-rec-submodule &&
-        git config submodule.recurse true
+	git clone --recurse-submodules parent clone-rec-submodule &&
+	git config submodule.recurse true
 
 '
 
-- 
gitgitgadget

[PATCH v3 4/4] clone: update whitespace according to style guide

[Mahi Kolla via GitGitGadget]

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

diff --git a/builtin/clone.c b/builtin/clone.c
index f41fd1afb66..c6bb38d2fde 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1130,7 +1130,7 @@ 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 "
-- 
gitgitgadget

Re: [PATCH v3 0/4] clone: update submodule.recurse in config when using --recurse-submodule

[Philippe Blain]

Hi Mahi,

Le 2021-08-02 à 19:23, Mahi Kolla via GitGitGadget a écrit :
> 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


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.

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

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!

Re: [PATCH v3 0/4] clone: update submodule.recurse in config when using --recurse-submodule

[Junio C Hamano]

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

> When running 'git clone --recurse-submodules', developers expect various
> other commands such as 'pull' and 'checkout' to also run recursively into
> submodules.

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?

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

[Mahi Kolla via GitGitGadget]

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

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 3fe3810f1ce..1d6aeb9e367 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -276,7 +276,7 @@ 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
diff --git a/builtin/clone.c b/builtin/clone.c
index 66fe66679c8..c6bb38d2fde 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1130,6 +1130,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 					   strbuf_detach(&sb, NULL));
 		}
 
+		string_list_append(&option_config, "submodule.recurse=true");
 		if (option_required_reference.nr &&
 		    option_optional_reference.nr)
 			die(_("clone --recursive is not compatible with "
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 3a595c0f82c..055b74069b3 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -16,6 +16,13 @@ test_expect_success 'setup' '
 
 '
 
+test_expect_success 'clone --recurse-submodules sets submodule.recurse=true' '
+
+	git clone --recurse-submodules parent clone-rec-submodule &&
+	test_cmp_config -C clone-rec-submodule true submodule.recurse
+
+'
+
 test_expect_success 'clone -o' '
 
 	git clone -o foo parent clone-o &&

base-commit: 66262451ec94d30ac4b80eb3123549cf7a788afd
-- 
gitgitgadget

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

[Junio C Hamano]

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

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

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?

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

[Mahi Kolla]

Hi Junio,

Thank you for your feedback!

On Mon, Aug 9, 2021 at 2:15 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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'.
>
> Please wrap overlong lines in your proposed log message to say 70 or
> so columns.
>

Ah, my bad, will do so going forward.

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

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.


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

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

[Junio C Hamano]

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

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

[Philippe Blain]

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/

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

[Mahi Kolla]

Hi Phillippe and Junio,

On Tue, Aug 10, 2021 at 4:04 PM Philippe Blain
<levraiphilippeblain@gmail.com> wrote:
>
> 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'.
>

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

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

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

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

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

[Junio C Hamano]

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

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

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.

[PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Mahi Kolla via GitGitGadget]

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(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index 66fe66679c8..24f242b4a60 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -986,6 +986,7 @@ 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;
@@ -1130,6 +1131,11 @@ 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) {
+		    string_list_append(&option_config, "submodule.recurse=true");
+		}
+
 		if (option_required_reference.nr &&
 		    option_optional_reference.nr)
 			die(_("clone --recursive is not compatible with "
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 3a595c0f82c..f20cdfa6fca 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -16,6 +16,18 @@ test_expect_success 'setup' '
 
 '
 
+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 &&
+
+	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
+
+'
+
 test_expect_success 'clone -o' '
 
 	git clone -o foo parent clone-o &&

base-commit: 66262451ec94d30ac4b80eb3123549cf7a788afd
-- 
gitgitgadget

Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Junio C Hamano]

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

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

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

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

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.

> Signed-off-by: Mahi Kolla <mahikolla@google.com>
> ---
>     clone: set submodule.recurse=true if feature.experimental flag enabled

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

Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Emily Shaffer]

On Wed, Aug 11, 2021 at 09:20:58PM -0700, Junio C Hamano wrote:
> 
> "Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > 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.
> 
> Please wrap overlong lines in your proposed log message to say 70 or
> so columns.
> 
> >
> > Since V1: Made this an opt in feature under the experimental flag. Updated tests to reflect this design change. Also updated commit message.
> 
> 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.
> 
> > Signed-off-by: Mahi Kolla <mahikolla@google.com>
> > ---
> >     clone: set submodule.recurse=true if feature.experimental flag enabled
> 
> 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.

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.

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

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

Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Philippe Blain]

Hi Emily,

Le 2021-08-12 à 19:54, Emily Shaffer a écrit :
> On Wed, Aug 11, 2021 at 09:20:58PM -0700, Junio C Hamano wrote:
>>
>> "Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> 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.
>>
>> Please wrap overlong lines in your proposed log message to say 70 or
>> so columns.
>>
>>>
>>> Since V1: Made this an opt in feature under the experimental flag. Updated tests to reflect this design change. Also updated commit message.
>>
>> 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.

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

>>
>>> Signed-off-by: Mahi Kolla <mahikolla@google.com>
>>> ---
>>>      clone: set submodule.recurse=true if feature.experimental flag enabled
>>
>> 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.
> 
> 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.

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

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

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.

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

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

Cheers,

Philippe.

Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Mahi Kolla]

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.

> >>
> >>>
> >>> Since V1: Made this an opt in feature under the experimental flag. Updated tests to reflect this design change. Also updated commit message.
> >>
> >> 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.
>
> 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).
>

Got it, thank you!

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

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.

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

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

Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Junio C Hamano]

Mahi Kolla <mahikolla@google.com> writes:

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

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.

Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

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

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.

Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

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

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

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

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

> 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

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.

Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Emily Shaffer]

On Thu, Aug 12, 2021 at 09:34:47PM -0700, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > 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.
> 
> 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).
> 
> > 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.
> 
> 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")?
> 
> > 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
> 
> 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.

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

Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

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

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

Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Emily Shaffer]

On Fri, Aug 13, 2021 at 01:30:22PM -0700, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > 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. 
> 
> Not that one---it was a 40% tongue-in-cheek comment, and does not
> deserve to be called a suggestion.  

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

Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Mahi Kolla]

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

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

Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

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

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.

RE: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

[Felipe Contreras]

Mahi Kolla via GitGitGadget wrote:
> From: Mahi Kolla <mahikolla@google.com>

> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -986,6 +986,7 @@ 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;
> @@ -1130,6 +1131,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  					   strbuf_detach(&sb, NULL));
>  		}
>  
> +		if(!git_config_get_bool("feature.experimental", &experimental_flag) &&

You are missing a space after the 'if'.

> +		    experimental_flag) {
> +		    string_list_append(&option_config, "submodule.recurse=true");
> +		}
> +

-- 
Felipe Contreras

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

[Mahi Kolla via GitGitGadget]

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(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index 66fe66679c8..a08d9012243 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -986,6 +986,7 @@ 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 sticky_recursive_clone;
 
 	struct transport_ls_refs_options transport_ls_refs_options =
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
@@ -1130,6 +1131,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 					   strbuf_detach(&sb, NULL));
 		}
 
+		if (!git_config_get_bool("submodule.stickyRecursiveClone", &sticky_recursive_clone)
+		    && sticky_recursive_clone) {
+		    string_list_append(&option_config, "submodule.recurse=true");
+		}
+
 		if (option_required_reference.nr &&
 		    option_optional_reference.nr)
 			die(_("clone --recursive is not compatible with "
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 3a595c0f82c..d822153e4d2 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -16,6 +16,18 @@ test_expect_success 'setup' '
 
 '
 
+test_expect_success 'submodule.stickyRecursiveClone flag manipulates submodule.recurse value' '
+
+	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 submodule.stickyRecursiveClone false &&
+	git clone --recurse-submodules parent clone_recurse_false &&
+	test_expect_code 1 git -C clone_recurse_false config --get submodule.recurse
+
+'
+
 test_expect_success 'clone -o' '
 
 	git clone -o foo parent clone-o &&

base-commit: 66262451ec94d30ac4b80eb3123549cf7a788afd
-- 
gitgitgadget

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

[Junio C Hamano]

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

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 66fe66679c8..a08d9012243 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -986,6 +986,7 @@ 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 sticky_recursive_clone;

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


>  	struct transport_ls_refs_options transport_ls_refs_options =
>  		TRANSPORT_LS_REFS_OPTIONS_INIT;
> @@ -1130,6 +1131,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  					   strbuf_detach(&sb, NULL));
>  		}

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.

> +		if (!git_config_get_bool("submodule.stickyRecursiveClone", &sticky_recursive_clone)
> +		    && sticky_recursive_clone) {
> +		    string_list_append(&option_config, "submodule.recurse=true");
> +		}

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.

diff --git c/builtin/clone.c w/builtin/clone.c
index 66fe66679c..c4e02d2f78 100644
--- c/builtin/clone.c
+++ w/builtin/clone.c
@@ -1114,6 +1114,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_recurse_submodules.nr > 0) {
 		struct string_list_item *item;
 		struct strbuf sb = STRBUF_INIT;
+		int val;
 
 		/* remove duplicates */
 		string_list_sort(&option_recurse_submodules);
@@ -1130,6 +1131,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 					   strbuf_detach(&sb, NULL));
 		}
 
+		if (!git_config_get_bool("submodule.stickyRecursiveClone", &val) &&
+		    val)
+		    string_list_append(&option_config, "submodule.recurse=true");
+
 		if (option_required_reference.nr &&
 		    option_optional_reference.nr)
 			die(_("clone --recursive is not compatible with "

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

[Emily Shaffer]

On Sat, Aug 14, 2021 at 11:05:41AM -0700, Junio C Hamano wrote:
> 
> "Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index 66fe66679c8..a08d9012243 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -986,6 +986,7 @@ 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 sticky_recursive_clone;
> 
> This variable does not have to be in such a wider scope, I think.
> 
> 
> >  	struct transport_ls_refs_options transport_ls_refs_options =
> >  		TRANSPORT_LS_REFS_OPTIONS_INIT;
> > @@ -1130,6 +1131,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >  					   strbuf_detach(&sb, NULL));
> >  		}
> 
> 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.
> 
> > +		if (!git_config_get_bool("submodule.stickyRecursiveClone", &sticky_recursive_clone)
> > +		    && sticky_recursive_clone) {
> > +		    string_list_append(&option_config, "submodule.recurse=true");
> > +		}
> 
> 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.
> 
> diff --git c/builtin/clone.c w/builtin/clone.c
> index 66fe66679c..c4e02d2f78 100644
> --- c/builtin/clone.c
> +++ w/builtin/clone.c
> @@ -1114,6 +1114,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	if (option_recurse_submodules.nr > 0) {
>  		struct string_list_item *item;
>  		struct strbuf sb = STRBUF_INIT;
> +		int val;
>  
>  		/* remove duplicates */
>  		string_list_sort(&option_recurse_submodules);
> @@ -1130,6 +1131,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  					   strbuf_detach(&sb, NULL));
>  		}
>  
> +		if (!git_config_get_bool("submodule.stickyRecursiveClone", &val) &&
> +		    val)
> +		    string_list_append(&option_config, "submodule.recurse=true");
> +
>  		if (option_required_reference.nr &&
>  		    option_optional_reference.nr)
>  			die(_("clone --recursive is not compatible with "

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

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

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

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

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.

[PATCH] fixup! clone: set submodule.recurse=true if submodule.stickyRecursiveClone enabled

[Junio C Hamano]

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

diff --git a/builtin/clone.c b/builtin/clone.c
index a08d901224..9c0c68a8ef 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -986,7 +986,6 @@ 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 sticky_recursive_clone;
 
 	struct transport_ls_refs_options transport_ls_refs_options =
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
@@ -1115,6 +1114,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_recurse_submodules.nr > 0) {
 		struct string_list_item *item;
 		struct strbuf sb = STRBUF_INIT;
+		int val;
 
 		/* remove duplicates */
 		string_list_sort(&option_recurse_submodules);
@@ -1131,10 +1131,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 					   strbuf_detach(&sb, NULL));
 		}
 
-		if (!git_config_get_bool("submodule.stickyRecursiveClone", &sticky_recursive_clone)
-		    && sticky_recursive_clone) {
-		    string_list_append(&option_config, "submodule.recurse=true");
-		}
+		if (!git_config_get_bool("submodule.stickyRecursiveClone", &val) &&
+		    val)
+			string_list_append(&option_config, "submodule.recurse=true");
 
 		if (option_required_reference.nr &&
 		    option_optional_reference.nr)
-- 
2.33.0-204-gff69670db4

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

[Emily Shaffer]

On Wed, Aug 18, 2021 at 01:15:56PM -0700, Junio C Hamano wrote:
> 
> 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>
Acked-by: Emily Shaffer <emilyshaffer@google.com>

Thanks.

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

[Emily Shaffer]

On Thu, Aug 19, 2021 at 10:41:03AM -0700, Emily Shaffer wrote:
> 
> On Wed, Aug 18, 2021 at 01:15:56PM -0700, Junio C Hamano wrote:
> > 
> > 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>
> Acked-by: Emily Shaffer <emilyshaffer@google.com>
> 
> Thanks.

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

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

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> On Thu, Aug 19, 2021 at 10:41:03AM -0700, Emily Shaffer wrote:
>> 
>> On Wed, Aug 18, 2021 at 01:15:56PM -0700, Junio C Hamano wrote:
>> > 
>> > 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>
>> Acked-by: Emily Shaffer <emilyshaffer@google.com>
>> 
>> Thanks.
>
> 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.

Thanks for a nudge.

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

[Philippe Blain]

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/