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

[Jonathan Nieder <jrnieder@gmail.com>]

Jens Lehmann wrote:
> Am 24.02.2011 00:07, schrieb Jonathan Nieder:

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

Ah.  So --recurse-submodules-default means "like --recurse-submodules,
but with lower precedence than the configuration".  Sensible.  (Maybe
it could be documented in --help-all that way?)

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

What I mean is the following: to make life easier for people and
scripts using --recurse-submodules today, it might be nice to
document how stable or unstable its meaning is.

In this case, there is a plan to make --recurse-submodules=on-demand
do more in the future than it does now;

 - a note in BUGS could explain that --recurse-submodules's current
   behavior is considered an infelicity and likely to change;

 - unfortunately not all users will necessarily see it that way (c.f.
   aforementioned use case), so it might be better to plan on yet
   another choice in the list of options provided by
   --recurse-submodules.

>> Maybe:
>>
>> 	char *new_rev;
>> 	...
>> 	argv[1] = new_rev = xstrdup(...);
>> 	...
>> 	free(new_rev);
>
> I'm not sure I get what the extra variable gains us ...

A reminder to free that memory in all code paths exiting the function.
But it might not be worth the noise.

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

Sorry for the confusion.  That suggestion refers to the loop after it
rather than what comes before it.

>> 	/*
>> 	 * Collect checked out submodules that have changed upstream
>> 	 * in "changed_submodule_paths".
>> 	 */
>> 
>>> +	while ((commit = get_revision(&rev))) {
[...]
> Thanks!

Thanks for deciphering the gibberish I sent. :)
Jonathan