Re: [PATCH v3 1/4] clone: update submodule.recurse in config when using --recurse-submodule
- From
Philippe Blain <levraiphilippeblain@gmail.com>
- Date
- Aug 3, 2021, 03:20 UTC
- Message-ID
- <e1bec4f1-d68f-2342-f759-8d0dc0de5eb4@gmail.com>
- In-Reply-To
- <fea3d6d72b63c06138d8eeb61e45edb30abbd79d.1627946590.git.gitgitgadget@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'.
Show 19 quoted lines
>
> 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.
Show 12 quoted lines
> 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.