From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 23 Feb 2011 22:56:58 GMT
Subject: Re: [PATCH 1/6] fetch/pull: recurse into submodules when necessary
Message-ID: <7vipwa5phh.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4D656F4D.6080401@web.de>

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?

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