Re: [PATCH 3/8] submodule: make static functions read submodules from commits

[Glen Choo <chooglen@google.com>]

Hm, after reading your feedback, I'm starting to question whether this
patch makes sense in its current form.

Jonathan Tan <jonathantanmy@google.com> writes:

> Glen Choo <chooglen@google.com> writes:
>> As a result, "git fetch" now reads changed submodules using the
>> `.gitmodules` and path from super_oid's tree (which is where "git fetch"
>> actually noticed the changed submodule) instead of the filesystem.
>
> Could we have a test showing what has changed?

Looking at this closer, I don't think a test is feasible, but even more
importantly, I don't think this behavior change is even desirable at
all..

I was confused when I wrote the commit message. Prior to this patch,
"git fetch" already records the names of changed submodules by correctly
reading .gitmodules from the given commit. From
collect_changed_submodules_cb(): 

		submodule = submodule_from_path(me->repo,
						commit_oid, p->two->path);
    [...]
		item = string_list_insert(changed, name);
		if (!item->util)
			item->util = xcalloc(1, sizeof(struct changed_submodule_data));
		cs_data = item->util;
		oid_array_append(&cs_data->new_commits, &p->two->oid);

The only behavior that actually _does_ change is that we plumb super_oid
through submodule_has_commits(). "git fetch" invokes this function to
figure out if it already has all of the needed commits, and if so, skip
the fetch.

Before plumbing super_oid, we could not check for commits in an
unpopulated submodule, but now we do. We will need this when we fetch in
unpopulated submodules, but as of this patch, we never fetch in
unpopulated submodules anyway, so this check is just wasted effort.

And because we never fetch anyway, we can't test any meaningful
behavior. We could check whether or not we check the submodule odb, but
that's a lot of effort to spend on something we don't need.

So we probably don't want this behavior change. I can preserve the
existing behavior by passing null_oid() instead, and pass super_oid in
the actual "fetch unpopulated submodules" patch.

>> @@ -1476,7 +1493,7 @@ static int get_next_submodule(struct child_process *cp,
>>  		if (!S_ISGITLINK(ce->ce_mode))
>>  			continue;
>>  
>> -		task = fetch_task_create(spf->r, ce->name);
>> +		task = fetch_task_create(spf->r, ce->name, null_oid());
>
> Hmm...is the plumbing incomplete? This code is about fetching, but we're
> not passing any superproject commit OID here. If this will be fixed in a
> future commit, maybe the distribution of what goes into each commit
> needs to be revised.
>
>> @@ -1499,7 +1516,7 @@ static int get_next_submodule(struct child_process *cp,
>>  			continue;
>>  		}
>>  
>> -		task->repo = get_submodule_repo_for(spf->r, task->sub->path);
>> +		task->repo = get_submodule_repo_for(spf->r, task->sub->path, null_oid());
>
> Same comment here.

This is intentional (but I admit that I also got confused rereading
this). This should be null_oid() (i.e. read from the filesystem) because
we are iterating through the index to get submodule paths. So we should
pass null_oid() so that we read .gitmodules from the filesystem, not a
possibly out-of-sync superproject commit.