[PATCHv7 8/8] clone: recursive and reference option triggers submodule alternates

> +submodule.alternateLocation::
> +	Specifies how the submodules obtain alternates when submodules are
> +	cloned. Possible values are `no`, `superproject`.
> +	By default `no` is assumed, which doesn't add references. When the
> +	value is set to `superproject` the submodule to be cloned computes
> +	its alternates location relative to the superprojects alternate.

> +submodule.alternateErrorStrategy
> +	Specifies how to treat errors with the alternates for a submodule
> +	as computed via `submodule.alternateLocation`. Possible values are
> +	`ignore`, `info`, `die`. Default is `die`.
> +
>  tag.forceSignAnnotated::
>  	A boolean to specify whether annotated tags created should be GPG signed.
>  	If `--annotate` is specified on the command line, it takes
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 0182665..404c5e8 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -947,6 +947,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		else
>  			fprintf(stderr, _("Cloning into '%s'...\n"), dir);
>  	}
> +
> +	if (option_recursive) {
> +		if (option_required_reference.nr &&
> +		    option_optional_reference.nr)
> +			die(_("clone --recursive is not compatible with "
> +			      "both --reference and --reference-if-able"));
> +		else if (option_required_reference.nr) {
> +			string_list_append(&option_config,
> +				"submodule.alternateLocation=superproject");
> +			string_list_append(&option_config,
> +				"submodule.alternateErrorStrategy=die");
> +		} else if (option_optional_reference.nr) {
> +			string_list_append(&option_config,
> +				"submodule.alternateLocation=superproject");
> +			string_list_append(&option_config,
> +				"submodule.alternateErrorStrategy=info");
> +		}
> +	}
> +
>  	init_db(option_template, INIT_DB_QUIET);
>  	write_config(&option_config);
>  
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 7096848..f8f35c1 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -472,6 +472,96 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
>  	return run_command(&cp);
>  }
>  
> +struct submodule_alternate_setup {
> +	const char *submodule_name;
> +	enum SUBMODULE_ALTERNATE_ERROR_MODE {
> +		SUBMODULE_ALTERNATE_ERROR_DIE,
> +		SUBMODULE_ALTERNATE_ERROR_INFO,
> +		SUBMODULE_ALTERNATE_ERROR_IGNORE
> +	} error_mode;
> +	struct string_list *reference;
> +};
> +#define SUBMODULE_ALTERNATE_SETUP_INIT { NULL, \
> +	SUBMODULE_ALTERNATE_ERROR_IGNORE, NULL }
> +
> +static int add_possible_reference_from_superproject(
> +		struct alternate_object_database *alt, void *sas_cb)
> +{
> +	struct submodule_alternate_setup *sas = sas_cb;
> +
> +	/* directory name, minus trailing slash */
> +	size_t namelen = alt->name - alt->base - 1;
> +	struct strbuf name = STRBUF_INIT;
> +	strbuf_add(&name, alt->base, namelen);
> +
> +	/*
> +	 * If the alternate object store is another repository, try the
> +	 * standard layout with .git/modules/<name>/objects
> +	 */
> +	if (ends_with(name.buf, ".git/objects")) {
> +		char *sm_alternate;
> +		struct strbuf sb = STRBUF_INIT;
> +		struct strbuf err = STRBUF_INIT;
> +		strbuf_add(&sb, name.buf, name.len - strlen("objects"));
> +		/*
> +		 * We need to end the new path with '/' to mark it as a dir,
> +		 * otherwise a submodule name containing '/' will be broken
> +		 * as the last part of a missing submodule reference would
> +		 * be taken as a file name.
> +		 */
> +		strbuf_addf(&sb, "modules/%s/", sas->submodule_name);
> +
> +		sm_alternate = compute_alternate_path(sb.buf, &err);
> +		if (sm_alternate) {
> +			string_list_append(sas->reference, xstrdup(sb.buf));
> +			free(sm_alternate);
> +		} else {
> +			switch (sas->error_mode) {
> +			case SUBMODULE_ALTERNATE_ERROR_DIE:
> +				die(_("submodule '%s' cannot add alternate: %s"),
> +				    sas->submodule_name, err.buf);
> +			case SUBMODULE_ALTERNATE_ERROR_INFO:
> +				fprintf(stderr, _("submodule '%s' cannot add alternate: %s"),
> +					sas->submodule_name, err.buf);
> +			case SUBMODULE_ALTERNATE_ERROR_IGNORE:
> +				; /* nothing */
> +			}
> +		}
> +		strbuf_release(&sb);
> +	}
> +
> +	strbuf_release(&name);
> +	return 0;
> +}
> +
> +static void prepare_possible_alternates(const char *sm_name,
> +		struct string_list *reference)
> +{
> +	char *sm_alternate = NULL, *error_strategy = NULL;
> +	struct submodule_alternate_setup sas = SUBMODULE_ALTERNATE_SETUP_INIT;
> +
> +	git_config_get_string("submodule.alternateLocation", &sm_alternate);
> +	if (!sm_alternate)
> +		return;
> +
> +	git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
> +
> +	if (!error_strategy)
> +		error_strategy = xstrdup("die");
> +
> +	sas.submodule_name = sm_name;
> +	sas.reference = reference;
> +	if (!strcmp(error_strategy, "die"))
> +		sas.error_mode = SUBMODULE_ALTERNATE_ERROR_DIE;
> +	if (!strcmp(error_strategy, "info"))
> +		sas.error_mode = SUBMODULE_ALTERNATE_ERROR_INFO;
> +	if (!strcmp(sm_alternate, "superproject"))
> +		foreach_alt_odb(add_possible_reference_from_superproject, &sas);
> +
> +	free(sm_alternate);
> +	free(error_strategy);
> +}
> +
>  static int module_clone(int argc, const char **argv, const char *prefix)
>  {
>  	const char *name = NULL, *url = NULL, *depth = NULL;
> @@ -532,6 +622,9 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>  	if (!file_exists(sm_gitdir)) {
>  		if (safe_create_leading_directories_const(sm_gitdir) < 0)
>  			die(_("could not create directory '%s'"), sm_gitdir);
> +
> +		prepare_possible_alternates(name, &reference);
> +
>  		if (clone_submodule(path, sm_gitdir, url, depth, &reference, quiet))
>  			die(_("clone of '%s' into submodule path '%s' failed"),
>  			    url, path);
> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
> index dff47af..1c1e289 100755
> --- a/t/t7408-submodule-reference.sh
> +++ b/t/t7408-submodule-reference.sh
> @@ -82,4 +82,47 @@ test_expect_success 'updating superproject keeps alternates' '
>  	test_alternate_is_used super-clone/.git/modules/sub/objects/info/alternates super-clone/sub
>  '
>  
> +test_expect_success 'submodules use alternates when cloning a superproject' '
> +	test_when_finished "rm -rf super-clone" &&
> +	git clone --reference super --recursive super super-clone &&
> +	(
> +		cd super-clone &&
> +		# test superproject has alternates setup correctly
> +		test_alternate_is_used .git/objects/info/alternates . &&
> +		# test submodule has correct setup
> +		test_alternate_is_used .git/modules/sub/objects/info/alternates sub
> +	)
> +'
> +
> +test_expect_success 'missing submodule alternate fails clone and submodule update' '
> +	test_when_finished "rm -rf super-clone" &&
> +	git clone super super2 &&
> +	test_must_fail git clone --recursive --reference super2 super2 super-clone &&
> +	(
> +		cd super-clone &&
> +		# test superproject has alternates setup correctly
> +		test_alternate_is_used .git/objects/info/alternates . &&
> +		# update of the submodule succeeds
> +		test_must_fail git submodule update --init &&
> +		# and we have no alternates:
> +		test_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&
> +		test_must_fail test_path_is_file sub/file1
> +	)
> +'
> +
> +test_expect_success 'ignoring missing submodule alternates passes clone and submodule update' '
> +	test_when_finished "rm -rf super-clone" &&
> +	git clone --reference-if-able super2 --recursive super2 super-clone &&
> +	(
> +		cd super-clone &&
> +		# test superproject has alternates setup correctly
> +		test_alternate_is_used .git/objects/info/alternates . &&
> +		# update of the submodule succeeds
> +		git submodule update --init &&
> +		# and we have no alternates:
> +		test_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&
> +		test_path_is_file sub/file1
> +	)
> +'
> +
>  test_done

> Stefan Beller <sbeller@google.com> writes:
>
>>  Added a default for alternateErrorStrategy and hence fixed the null pointer
>>  for error_strategy in prepare_possible_alternates(),
>
> Looks much better.
>
>> +submodule.alternateLocation::
>> +     Specifies how the submodules obtain alternates when submodules are
>> +     cloned. Possible values are `no`, `superproject`.
>> +     By default `no` is assumed, which doesn't add references. When the
>> +     value is set to `superproject` the submodule to be cloned computes
>> +     its alternates location relative to the superprojects alternate.
>
> I am not seeing a code that handles 'no' and any other string that
> is not 'superproject' differently, though.
>
> I can see that "clone" has codepath that writes 'superproject' to
> the variable, but the only thing that seems to care what value the
> variable is set to checks "no".  When somebody sets the variable to
> "yes", shouldn't we at least say "Sorry, I do not understand" and
> preferrably stop before spreading potential damage?  We'd surely end
> up doing something that the user who set the value to "yes" did not
> expect.
>
> There is something still missing?
>

>> +     sas.submodule_name = sm_name;
>> +     sas.reference = reference;
>> +     if (!strcmp(error_strategy, "die"))
>> +             sas.error_mode = SUBMODULE_ALTERNATE_ERROR_DIE;
>> +     if (!strcmp(error_strategy, "info"))
>> +             sas.error_mode = SUBMODULE_ALTERNATE_ERROR_INFO;