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

"Markus Klein via GitGitGadget" [off-list ref] writes:

quoted

From: Markus Klein <redacted>

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.

quoted

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.

quoted

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

quoted

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.

quoted

+/**
+ * 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.

quoted

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

quoted

+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