Re: [PATCH 3/8] submodule: make static functions read submodules from commits
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Feb 10, 2022, 19:15 UTC
- Message-ID
- <20220210191533.659570-1-jonathantanmy@google.com>
- In-Reply-To
- <20220210044152.78352-4-chooglen@google.com>
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?
Show 6 quoted lines
> @@ -1029,7 +1044,7 @@ static int submodule_needs_pushing(struct repository *r,
> const char *path,
> struct oid_array *commits)
> {
> - if (!submodule_has_commits(r, path, commits))
> + if (!submodule_has_commits(r, path, null_oid(), commits))This confused me at first, but I see that this code is not for fetching, but for pushing. This patch set concerns itself with fetching, so passing null_oid() to preserve existing behavior is good.
Show 13 quoted lines
> @@ -1414,12 +1429,13 @@ static const struct submodule *get_non_gitmodules_submodule(const char *path)
> }
>
> static struct fetch_task *fetch_task_create(struct repository *r,
> - const char *path)
> + const char *path,
> + const struct object_id *treeish_name)
> {
> struct fetch_task *task = xmalloc(sizeof(*task));
> memset(task, 0, sizeof(*task));
>
> - task->sub = submodule_from_path(r, null_oid(), path);
> + task->sub = submodule_from_path(r, treeish_name, path);If there is not a good reason to have "path" before "treeish_name", probably best to maintain the same parameter order as submodule_from_path().
Show 6 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.
Show 6 quoted lines
> @@ -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.