Re: [PATCH 1/6] fetch/pull: recurse into submodules when necessary

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 23, 2011, 23:07 UTC
Message-ID: <20110223230713.GB6819@elie>
In-Reply-To: <4D656F4D.6080401@web.de>

Jens Lehmann wrote:

> To be able to access all commits of populated submodules referenced by the
> superproject it is sufficient to only then let "git fetch" recurse into a
> submodule when the new commits fetched in the superproject record new
> commits for it.

Exciting stuff. This would use the default refspec rather than fetching the bare minimum of commits in submodules, right?

> There is currently no infrastructure for storing deleted and new
> submodules in the .git directory of the superproject. Thats why fetch and
> pull for now only fetch submodules that are already checked out and are
> not renamed.

Ah.

Show 14 quoted lines

> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -73,6 +73,14 @@ ifndef::git-pull[]
>  	Prepend <path> to paths printed in informative messages
>  	such as "Fetching submodule foo".  This option is used
>  	internally when recursing over submodules.
> +
> +--submodule-default=[yes|on-demand]::
> +	This option is used internally to set the submodule recursion default
> +	to either a boolean configuration value representing "true" (for
> +	unconditonal recursion) or to "on-demand" (when only those submodules
> +	should be fetched of which new commits have been fetched in its
> +	superproject).
> +	Using the "--[no-]recurse-submodules" option ignores this setting.

Could this be combined with the --recurse-submodules, with a "last instance of the option wins" rule? Something like:

 --recurse-submodules[=(yes | no | changed)]::
 --no-recurse-submodules::

> +++ b/submodule.c

[...]

> @@ -248,11 +263,73 @@ void set_config_fetch_recurse_submodules(int value)

[...]

> +static void submodule_collect_changed_cb(struct diff_queue_struct *q,

[...]

Show 5 quoted lines

> +		if (S_ISGITLINK(p->one->mode)) {
> +			/* NEEDSWORK: We should honor the name configured in
> +			 * the .gitmodules file of the commit we are examining
> +			 * here to be able to correctly follow submodules
> +			 * being moved around. */

Hmm, a sort of variant on rename detection. Does "git submodule update" / "git checkout --recurse-submodules" use .gitmodules to move pre-populated subrepositories?

Show 7 quoted lines

> +			/* Submodule is new or was moved here */
> +			/* NEEDSWORK: When the .git directories of submodules
> +			 * live inside the superprojects .git directory some
> +			 * day we should fetch new submodules directly into
> +			 * that location too when config or options request
> +			 * that so they can be checked out from there. */
> +			continue;

Maybe this can be mentioned in a BUGS section on the git-fetch(1) manpage to give readers a warning and clue about the intended meaning of --recurse-submodules?

I'm afraid a certain kind of person might get used to the "don't fetch new submodules" behavior (e.g., if upstream is including unneeded convenience copies of libraries right and left in addition to some useful submodules).

Show 6 quoted lines

> +void check_for_new_submodule_commits(unsigned char new_sha1[20])
> +{
> +	struct rev_info rev;
> +	struct commit *commit;
> +	int argc = 5;
> +	const char *argv[] = {NULL, NULL, "--not", "--branches", "--remotes", NULL};

Nit: maybe

	const char *argv[] = ...;
	int argc = ARRAY_SIZE(argv) - 1;

> +
> +	init_revisions(&rev, NULL);
> +	argv[1] = xstrdup(sha1_to_hex(new_sha1));

Maybe:

	char *new_rev;
	...
	argv[1] = new_rev = xstrdup(...);
	...
	free(new_rev);

> +	setup_revisions(argc, argv, &rev, NULL);
> +	if (prepare_revision_walk(&rev))
> +		die("revision walk setup failed");
> +

Maybe a comment so the reader doesn't have to delve deeper?

	/*
	 * Collect checked out submodules that have changed upstream
	 * in "changed_submodule_paths".
	 */

> +	while ((commit = get_revision(&rev))) {

[...]

> +++ b/t/t5526-fetch-submodules.sh
> @@ -192,4 +192,113 @@ test_expect_success "--no-recurse-submodules overrides config setting" '

[...]

> +	echo "Fetching submodule submodule" > expect.out.sub &&
> +	echo "From $pwd/." > expect.err.sub &&
> +	echo "   $head1..$head2  master     -> origin/master" >> expect.err.sub

I wonder if we should be testing the output in this much detail.

Wouldn't it be nicer to check the remote-tracking refs to make sure the lasting effects were correct, rather than the detailed output format?

So: aside from the option name, nothing but nits.  Thanks; that was
fun.