Re: [PATCH v2 7/9] fetch: fetch unpopulated, changed submodules

> Glen Choo <chooglen@google.com> writes:
>> Teach "git fetch" to fetch cloned, changed submodules regardless of
>> whether they are populated (this is in addition to the current behavior
>> of fetching populated submodules).
>
> I think add a note that the current behavior is regardless of what is
> being fetched. So, maybe something like:
>
>   Teach "git fetch" to fetch cloned, changed submodules regardless of
>   whether they are populated. This is in addition to the current behavior
>   of fetching populated submodules (which happens regardless of what was
>   fetched in the superproject, or even if nothing was fetched in the
>   superproject).

>> As I mentioned in the cover letter, I'm not entirely happy with the
>> name repo_has_absorbed_submodules() - it's not a standardized term AFAIK
>> and it's a little clunky.
>> 
>> "absorbed submodule" is just a stand-in for "submodule in .git/modules",
>> so if we have a better term for "submodule in .git/modules", let's use
>> that instead.
>
> I think that this is OK if the doc comment is updated. I'll make the
> suggestion in the appropriate place below.
>> +/*
>> + * Returns 1 if the repo has absorbed submodule gitdirs, and 0
>> + * otherwise. Like submodule_name_to_gitdir(), this checks
>> + * $GIT_DIR/modules, not $GIT_COMMON_DIR.
>> + */
>
> I think that if you replace the doc comment with something like:
>
>   Returns 1 if there is at least one submodule gitdir in
>   $GIT_DIR/modules, and 0 otherwise. (End users can move submodule
>   gitdirs into $GIT_DIR/modules by running "git submodule
>   absorbgitdirs".) Like submodule_name_to_gitdir()...
>
> then it would be fine.

>> @@ -1338,6 +1366,7 @@ struct submodule_parallel_fetch {
>>  	int result;
>>  
>>  	struct string_list changed_submodule_names;
>> +	struct string_list seen_submodule_names;
>
> Could we unify the 2 lists instead of having 2 separate ones?

>
>> @@ -1529,12 +1569,77 @@ get_fetch_task(struct submodule_parallel_fetch *spf,
>>  	return NULL;
>>  }
>>  
>> +static struct fetch_task *
>> +get_fetch_task_from_changed(struct submodule_parallel_fetch *spf,
>> +			    const char **default_argv, struct strbuf *err)
>> +{
>> +	for (; spf->changed_count < spf->changed_submodule_names.nr;
>> +	     spf->changed_count++) {
>> +		struct string_list_item item =
>> +			spf->changed_submodule_names.items[spf->changed_count];
>> +		struct changed_submodule_data *cs_data = item.util;
>> +		struct fetch_task *task;
>> +
>> +		/*
>> +		 * We might have already considered this submodule
>> +		 * because we saw it in the index.
>> +		 */
>> +		if (string_list_lookup(&spf->seen_submodule_names, item.string))
>> +			continue;
>> +
>> +		task = fetch_task_create(spf->r, cs_data->path,
>> +					 cs_data->super_oid);
>> +		if (!task)
>> +			continue;
>> +
>> +		switch (get_fetch_recurse_config(task->sub, spf)) {
>> +		default:
>> +		case RECURSE_SUBMODULES_DEFAULT:
>> +		case RECURSE_SUBMODULES_ON_DEMAND:
>> +			*default_argv = "on-demand";
>> +			break;
>> +		case RECURSE_SUBMODULES_ON:
>> +			*default_argv = "yes";
>> +			break;
>> +		case RECURSE_SUBMODULES_OFF:
>> +			continue;
>> +		}
>> +
>> +		task->repo = get_submodule_repo_for(spf->r, task->sub->path,
>> +						    cs_data->super_oid);
>> +		if (!task->repo) {
>> +			fetch_task_release(task);
>> +			free(task);
>> +
>> +			strbuf_addf(err, _("Could not access submodule '%s'\n"),
>> +				    cs_data->path);
>> +			continue;
>> +		}
>> +		if (!is_tree_submodule_active(spf->r, cs_data->super_oid,
>> +					      task->sub->path))
>> +			continue;
>> +
>> +		if (!spf->quiet)
>> +			strbuf_addf(err,
>> +				    _("Fetching submodule %s%s at commit %s\n"),
>> +				    spf->prefix, task->sub->path,
>> +				    find_unique_abbrev(cs_data->super_oid,
>> +						       DEFAULT_ABBREV));
>> +		spf->changed_count++;
>> +		return task;
>> +	}
>> +	return NULL;
>> +}
>
> This is very similar to get_fetch_task_from_index(). Both:
>  1. loop over something
>  2. exit early if the submodule name is seen
>  3. create the fetch task
>  4. set the "recurse config"
>  5. get the submodule repo
>  6. if success, increment a counter
>  7. if failure, check for some conditions and maybe append to err
>
> Could a function be refactored that does 2-5?

>
>> +test_expect_success "'--recurse-submodules=on-demand' should fetch submodule commits if the submodule is changed but the index has no submodules" '
>> +	git -C downstream fetch --recurse-submodules &&
>
> First of all, thanks for updating the test - it is much easier to
> understand now.
>
> About this line, we shouldn't use the code being tested to set up (we're
> testing "fetch --recurse-submodules", so we shouldn't use the same
> command to set up). Also, if we don't have confidence in the starting
> state, it may be a sign to write it out more explicitly instead of
> relying on a complicated command to do the right thing.

>> +	# Create new superproject commit with updated submodules
>> +	add_upstream_commit &&
>> +	(
>> +		cd submodule &&
>> +		(
>> +			cd subdir/deepsubmodule &&
>> +			git fetch &&
>> +			git checkout -q FETCH_HEAD
>> +		) &&
>> +		git add subdir/deepsubmodule &&
>> +		git commit -m "new deep submodule"
>> +	) &&
>> +	git add submodule &&
>> +	git commit -m "new submodule" &&
>
> I thought add_upstream_commit() would do this, but apparently it just
> adds commits to the submodules (which works for the earlier tests that
> just tested that the submodules were fetched, but not for this one). I
> think that all this should go into its own function.
>
> Also, I understand that "git fetch" is there to pick up the commit we
> made in add_upstream_commit, but this indirection is unnecessary in a
> test, I think. Can we not use add_upstream_commit and just create our
> own in subdir/deepsubmodule? (This might conflict with subsequent tests
> that use the old scheme, but I think that it should be fine.)

>> +# In downstream, init "submodule2", but do not check it out while
>> +# fetching. This lets us assert that unpopulated submodules can be
>> +# fetched.
>
> Firstly, "In upstream", I think? You want to fetch from it, so it has to
> be upstream.

>> +test_expect_success "'--recurse-submodules' should fetch submodule commits in changed submodules and the index" '
>
> [snip]
>
>> +	git checkout super &&
>> +	(
>> +		cd downstream &&
>> +		git fetch --recurse-submodules >../actual.out 2>../actual.err
>> +	) &&
>> +	test_must_be_empty actual.out &&
>> +
>> +	# Assert that the submodules in the super branch are fetched
>> +	git rev-parse --short HEAD >superhead &&
>> +	git -C submodule rev-parse --short HEAD >subhead &&
>> +	git -C deepsubmodule rev-parse --short HEAD >deephead &&
>> +	verify_fetch_result actual.err &&
>> +	# grep for the exact line to check that the submodule is read
>> +	# from the index, not from a commit
>> +	grep "^Fetching submodule submodule\$" actual.err &&
>
> Instead of a grep, I think this should be done by precisely specifying
> what to fetch in the "git fetch" invocation, and then checking that the
> submodule has commits that it didn't have before.

> In addition, I think the following cases also need to be tested:
>  - two fetched commits have submodules of the same name but different
>    URL
>  - a fetched commit and a commit in the index have submodules of the
>    same name but different URL