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

[Junio C Hamano <gitster@pobox.com>]

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

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)?

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

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

> +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
> +	)
> +'