Re: [PATCH 1/6] fetch/pull: recurse into submodules when necessary
- From
Jens Lehmann <jens.lehmann@web.de>
- Date
- Feb 23, 2011, 23:28 UTC
- Message-ID
- <4D659818.4070107@web.de>
- In-Reply-To
- <7vipwa5phh.fsf@alter.siamese.dyndns.org>
Am 23.02.2011 23:56, schrieb Junio C Hamano:
Show 14 quoted lines
> 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.
Show 31 quoted lines
>> @@ -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?