Re: [PATCH 3/8] submodule: make static functions read submodules from commits
- From
Glen Choo <chooglen@google.com>
- Date
- Feb 11, 2022, 10:07 UTC
- Message-ID
- <kl6l7da1yawq.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <20220210191533.659570-1-jonathantanmy@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:
Show 6 quoted lines
> 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.
Show 20 quoted lines
>> @@ -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.