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

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

Am 23.02.2011 23:56, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> diff --git a/submodule.c b/submodule.c
>> index 6f1c107..c8c3a99 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -152,6 +153,20 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt,
>> ...
>> +int parse_fetch_recurse_submodules_arg(const char *arg)
>> +{
>> +	switch (git_config_maybe_bool("", arg)) {
> 
> It's a bit unusual to see "" as the variable name; doesn't config-maybe-bool
> barf when arg is not what it likes, with the name as part of its message?

git_config_*maybe*_bool() itself calls git_config_maybe_bool_text() and
git_parse_long() which all don't die() on anything (while git_config_bool()
can do that in git_config_int() via git_config_bool_or_int()). But you are
right, using something more descriptive than "" is much better here.

>> @@ -248,11 +263,73 @@ void set_config_fetch_recurse_submodules(int value)
>> ...
>> +static void submodule_collect_changed_cb(struct diff_queue_struct *q,
>> +					 struct diff_options *options,
>> +					 void *data)
>> +{
>> +	int i;
>> +	for (i = 0; i < q->nr; i++) {
>> +		struct diff_filepair *p = q->queue[i];
>> +		if (!S_ISGITLINK(p->two->mode))
>> +			continue;
>> +
>> +		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. */
>> +			struct string_list_item *path;
>> +			path = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);
>> +			if (!path)
>> +				string_list_append(&changed_submodule_paths, xstrdup(p->two->path));
> 
> I wondered why there is no mention of "data" in the implementation of this
> function; changed_submodule_paths global is used instead here.
> 
> I do not see anywhere the global string_list is initialized, freed, nor
> re-initialized for reuse.  Are you guaranteeing that the caller only calls
> the check_for_new_submodule_commits() function once, and if so how?  The
> function is called from update_local_ref() in builtin/fetch.c, which in
> turn gets called number of times during a fetch.  IOW, does the code do
> the right thing when you are fetching more than one refs?

I assume that a string_list initialized with 0 is initialized properly.
The idea is to let check_for_new_submodule_commits() be called as often
as needed and for each ref to collect all submodule changes recorded in
any ref and afterwards only once call fetch_populated_submodules() to
collect all submodules touched by any commits on any ref. But maybe
fetch_populated_submodules() should empty the string_list it just
worked through?