| Message ID | 20220215172318.73533-4-chooglen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch --recurse-submodules: fetch unpopulated submodules | expand |
First of all, patches 1 and 2 look good since they are the same as in v1 and I have reviewed them. Moving on... Glen Choo <chooglen@google.com> writes: > The changed function signatures follow repo_submodule_init()'s argument > order, i.e. "path" then "treeish_name". Where needed, reorder the > arguments of functions that already take "path" and "treeish_name" to be > consistent with this convention. This paragraph made me nervous, but looking at the diff, you didn't actually reorder any arguments. Probably best to delete this paragraph. The fact that the additional functionality is not used also means that we can't tell for sure if all relevant functions are indeed changed, but perhaps we can determine this in a later patch.
On Wed, Feb 16 2022, Glen Choo wrote: > diff --git a/submodule.c b/submodule.c > index 5ace18a7d9..7032dcabb8 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -932,6 +932,7 @@ struct has_commit_data { > struct repository *repo; > int result; > const char *path; > + const struct object_id *super_oid; > }; ... > - struct has_commit_data has_commit = { r, 1, path }; > + struct has_commit_data has_commit = { r, 1, path, super_oid }; FWIW I wouldn't at all mind the tiny detour of just turning this into designated initializers while we're at it, instead of having to keep track of the positionals. I.e.: [...] = { .repo = r, .result = 1, .path = path, ,super_oid = super_oid };
Jonathan Tan <jonathantanmy@google.com> writes: > Glen Choo <chooglen@google.com> writes: >> The changed function signatures follow repo_submodule_init()'s argument >> order, i.e. "path" then "treeish_name". Where needed, reorder the >> arguments of functions that already take "path" and "treeish_name" to be >> consistent with this convention. > > This paragraph made me nervous, but looking at the diff, you didn't > actually reorder any arguments. Probably best to delete this paragraph. Oh you're right. I was sure that this paragraph used to be relevant, but I guess not. Thanks for the catch.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Wed, Feb 16 2022, Glen Choo wrote: > >> diff --git a/submodule.c b/submodule.c >> index 5ace18a7d9..7032dcabb8 100644 >> --- a/submodule.c >> +++ b/submodule.c >> @@ -932,6 +932,7 @@ struct has_commit_data { >> struct repository *repo; >> int result; >> const char *path; >> + const struct object_id *super_oid; >> }; > > ... > >> - struct has_commit_data has_commit = { r, 1, path }; >> + struct has_commit_data has_commit = { r, 1, path, super_oid }; > > FWIW I wouldn't at all mind the tiny detour of just turning this into > designated initializers while we're at it, instead of having to keep > track of the positionals. I.e.: > > [...] = { > .repo = r, > .result = 1, > .path = path, > ,super_oid = super_oid > }; Since I'm touching the line anyway, this seems like a reasonable change.
diff --git a/submodule.c b/submodule.c index 5ace18a7d9..7032dcabb8 100644 --- a/submodule.c +++ b/submodule.c @@ -932,6 +932,7 @@ struct has_commit_data { struct repository *repo; int result; const char *path; + const struct object_id *super_oid; }; static int check_has_commit(const struct object_id *oid, void *data) @@ -940,7 +941,7 @@ static int check_has_commit(const struct object_id *oid, void *data) struct repository subrepo; enum object_type type; - if (repo_submodule_init(&subrepo, cb->repo, cb->path, null_oid())) { + if (repo_submodule_init(&subrepo, cb->repo, cb->path, cb->super_oid)) { cb->result = 0; goto cleanup; } @@ -968,9 +969,10 @@ static int check_has_commit(const struct object_id *oid, void *data) static int submodule_has_commits(struct repository *r, const char *path, + const struct object_id *super_oid, struct oid_array *commits) { - struct has_commit_data has_commit = { r, 1, path }; + struct has_commit_data has_commit = { r, 1, path, super_oid }; /* * Perform a cheap, but incorrect check for the existence of 'commits'. @@ -1017,7 +1019,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)) /* * NOTE: We do consider it safe to return "no" here. The * correct answer would be "We do not know" instead of @@ -1277,7 +1279,7 @@ static void calculate_changed_submodule_paths(struct repository *r, if (!path) continue; - if (submodule_has_commits(r, path, commits)) { + if (submodule_has_commits(r, path, null_oid(), commits)) { oid_array_clear(commits); *name->string = '\0'; } @@ -1402,12 +1404,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 (!task->sub) { /* * No entry in .gitmodules? Technically not a submodule, @@ -1439,11 +1442,12 @@ static void fetch_task_release(struct fetch_task *p) } static struct repository *get_submodule_repo_for(struct repository *r, - const char *path) + const char *path, + const struct object_id *treeish_name) { struct repository *ret = xmalloc(sizeof(*ret)); - if (repo_submodule_init(ret, r, path, null_oid())) { + if (repo_submodule_init(ret, r, path, treeish_name)) { free(ret); return NULL; } @@ -1464,7 +1468,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()); if (!task) continue; @@ -1487,7 +1491,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()); if (task->repo) { struct strbuf submodule_prefix = STRBUF_INIT; child_process_init(cp);
A future commit will teach "fetch --recurse-submodules" to fetch unpopulated submodules. To prepare for this, teach the necessary static functions how to read submodules from superproject commits using a "treeish_name" argument (instead of always reading from the index and filesystem) but do not actually change where submodules are read from. Submodules will be read from commits when we fetch unpopulated submodules. The changed function signatures follow repo_submodule_init()'s argument order, i.e. "path" then "treeish_name". Where needed, reorder the arguments of functions that already take "path" and "treeish_name" to be consistent with this convention. Signed-off-by: Glen Choo <chooglen@google.com> --- submodule.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)