AW: [PATCH v2] clone: use submodules.recurse option for automatically clone submodules

[<masmiseim@gmx.de>]

> "Markus Klein via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > From: Markus Klein <masmiseim@gmx.de>
> >
> > Simplify cloning repositories with submodules when the option 
> > submodules.recurse is set by the user. This makes it transparent to 
> > the user if submodules are used. The user doesn’t have to know if he 
> > has to add an extra parameter to get the full project including the used submodules.
> > This makes clone behave identical to other commands like fetch, pull, 
> > checkout, ... which include the submodules automatically if this 
> > option is set.
> 
> I am not sure if it is even a good idea to make clone behave identically to fetch and pull. 
> We cannot escape from the fact that the initial cloning of the top-level superproject is a special
> event---we do not even have a place to put the configuration specific to that superproject
> (e.g. which submodules are good ones to clone by default) before that happens.

It behaves only identical if the option "submodule.recurse" is set in the global .gitconfig. So, it is optional for people who know what they do. For people which use submodules heavily, this is very useful.

For the case where you don't like to get all submodules but have this option set, you can disable it via --no-recurse-submodules

> 
> You misspelt "submodule.recurse" everywhere in the log message, by the way, even though the code seems
> to react to the right variable.
> 
> > It is implemented analog to the pull command by using an own config 
> > function instead of using just the default config.
> 
> I am not sure if this is worth saying, but it is not incorrect per-se.
> 
> > In contrast to the pull
> > command, the submodule.recurse state is saved as an array of strings 
> > as it can take an optionally pathspec argument which describes which 
> > submodules should be recursively initialized and cloned.
> 
> Sorry, but I do not think I get this part at all.  Your callback seems to add a fixed string "true"
> to option_recurse_submodules string list as many times as submodule.recurse variable is defined in
> various configuration files.  Does anybody count how many and react differently?  You mention "pathspec"
> here, but how does one specify a pathspec beforehand (remember, this is clone and there is no superproject
> repository or its per-repository configuration file yet before we clone it)?

I'm so sorry for the confusing with the true. This is definitely wrong. Johannes already pointed this out to me and I had already fixed it. Shame on me, as I had uploaded an old version :-(

> 
> > To recursively initialize and
> > clone all submodules a pathspec of "." has to be used.
> > The regression test is simplified compared to the test for "git clone 
> > --recursive" as the general functionality is already checked there.
> 
> Documentation/config/submodule.txt says submodule.recurse says
> 
>     Specifies if commands recurse into submodules by default. This
>     applies to all commands that have a `--recurse-submodules`
>     option, except `clone`.  Defaults to false.
> 
> so I take that the value must be a boolean.  So I am lost what pathspec you are talking about here.
> 
> > +/**
> > + * Read config variables.
> > + */
> 
> That's a fairly useless comment that does not say more than what the name of the function already tells us X-<.

True, this was copy'pasted from the pull implementation. So it should be useless there also.

> 
> > +static int git_clone_config(const char *var, const char *value, void 
> > +*cb) {
> > +	if (!strcmp(var, "submodule.recurse") && git_config_bool(var, value)) {
> > +		string_list_append(&option_recurse_submodules, "true");
> > +		return 0;
> 
> The breakage of this is not apparent, but this is misleading.  If submodule.recurse is set to a value
> that git_config_bool() would say "false", the if statement is skipped, and you end up calling
> git_default_config() with "submodule.recurse", even though you are supposed to have already dealt with
> the setting.
> 
> 	if (!strcmp(var, "submodule.recurse")) {
> 		if (git_config_bool(var, value))
> 			...
> 		return 0; /* done with the variable either way */
> 	}
> 
> is more appropriate.  I still do not know what this code is trying to do by appending "true" as many
> times as submodule.recurse appears in the configuration file(s), though.
> 
> When given from the command line, i.e.
> 
> 	git clone --no-recurse-submodules ...
> 	git clone --recurse-submodules    ...
> 	git clone --recurse-submodules=<something> ...
> 
> recurse_submodules_cb() reacts to them by
> 
>  (1) clearing what have been accumulated so far,
>  (2) appending the match-all "." pathspec, and
>  (3) appending the <something> string 
> 
> to option_recurse_submodules string list.  But given that submodule.recurse is not (and will not be without
> an involved transition plan) a pathspec but merely a boolean, I would think appending hardcoded string
> constant "true" makes little sense.
> After sorting the list, these values become values of the submodule.active configuration variable whose
> values are pathspec elements in cmd_clone(); see the part of the code before it makes a call to init_db().
> 
> So, I would sort-of understand if you pretend --recurse-submodules was given from the command line when
> submodule.recurse is set to true (which would mean that you'd append "." to the string list).
> But I do not understand why appending "true" is a good thing at all here.
> 
> Another thing I noticed.
> 
> If you have "[submodule] recurse" in your $HOME/.gitconfig, you'd want to be able to countermand
> from the command line with
> 
>     git clone --no-recurse-submodules ...
> 
> so that the clone would not go recursive.  And that should be tested.  
> 
> You'd also want the opposite, i.e. with "[submodule] recurse=no" in your $HOME/.gitconfig and running
> 
>     git clone --recurse-submodules ...
> 
> should countermand the configuration.

Thanks for the hint. I added this tests, and it was very helpful, as it pointed out, that the disabling via --no-recurse-submodules was not working.

> 
> Thanks.
> 
> > +test_expect_success 'use "git clone" with submodule.recurse=true to checkout all submodules' '
> > +	git clone -c submodule.recurse=true super clone7 &&
> > +	(
> > +		git -C clone7 rev-parse --resolve-git-dir .git --resolve-git-dir nested1/nested2/nested3/submodule/.git >actual &&
> > +		cat >expect <<-EOF &&
> > +		.git
> > +		$(pwd)/clone7/.git/modules/nested1/modules/nested2/modules/nested3/modules/submodule
> > +		EOF
> > +		test_cmp expect actual
> > +	)
> > +'

Thanks for the feedback