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

Hi Junio,

On Thu, 6 Feb 2020, Junio C Hamano wrote:

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

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.

Good catch, and I think you will have to do even more: in the "else" case,
it is possible that the user overrode a `submodule.recurse` from the
system config in their user-wide config, so we must _undo_ the
`string_list_append().

Further, it is probably not a good idea to append "true" _twice_ if
multiple configs in the chain specify `submodule.recurse = true`.

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

Indeed, I think I even pointed out that "true" is not an appropriate value
to use here: https://github.com/git/git/pull/695/#discussion_r367866708

Ciao,
Dscho