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

[Philippe Blain <levraiphilippeblain@gmail.com>]

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.