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

From: Mahi Kolla <mahikolla@google.com>
Date: Aug 7, 2021, 03:06 UTC
Message-ID: <CAN3QUFabO1PsW8te5zqwNRrHc99q4jO=6-pPc8cEv43Zevux4A@mail.gmail.com>
In-Reply-To: <e1bec4f1-d68f-2342-f759-8d0dc0de5eb4@gmail.com>

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:

Show 11 quoted lines

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

Show 7 quoted lines

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

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

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?

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

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!