Re: [PATCH 1/6] fetch/pull: recurse into submodules when necessary
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 23, 2011, 22:56 UTC
- Message-ID
- <7vipwa5phh.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <4D656F4D.6080401@web.de>
Jens Lehmann <Jens.Lehmann@web.de> writes:
Show 9 quoted lines
> 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?
Show 21 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?