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

[Jens Lehmann <Jens.Lehmann@web.de>]

Am 24.02.2011 00:07, schrieb Jonathan Nieder:
> 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?

Yup, AFAIK I have no means right now to tell "git fetch" to only fetch
certain commits. But we were talking at the GitTogether that this might
be useful and with a bit of tweaking the code would support that too.

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

Nope, as this only sets the default. "--recurse-submodules" overrides
anything configured, which is not what we want here. This option should
only set the default.

>> +++ 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,
> [...]
>> +		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?

Not yet ;-)

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

I'm not sure I understand what you mean by this, right now this can
only work for populated submodules. I hope this will change soon, but
I'm not quite there yet ;-)

>> +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;
> 
> ?

Fine by me!

>> +
>> +	init_revisions(&rev, NULL);
>> +	argv[1] = xstrdup(sha1_to_hex(new_sha1));
> 
> Maybe:
> 
> 	char *new_rev;
> 	...
> 	argv[1] = new_rev = xstrdup(...);
> 	...
> 	free(new_rev);

I'm not sure I get what the extra variable gains us ...

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

Yes, that makes sense!

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

Thanks!