| Message ID | 20220210044152.78352-7-chooglen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch --recurse-submodules: fetch unpopulated submodules | expand |
Glen Choo <chooglen@google.com> writes: > Extract the index iterating code into an iterator function, > get_fetch_task(), so that get_next_submodule() is agnostic of how > to find submodules. This prepares for a subsequent commit will teach the > fetch machinery to also iterate through the list of changed > submodules (in addition to the index). The transformation looks correct, but there are several things that would have made it much easier to review. > @@ -1507,41 +1505,17 @@ static int get_next_submodule(struct child_process *cp, [snip] > - if (task->repo) { > - struct strbuf submodule_prefix = STRBUF_INIT; > - child_process_init(cp); > - cp->dir = task->repo->gitdir; > - prepare_submodule_repo_env_in_gitdir(&cp->env_array); > - cp->git_cmd = 1; > - if (!spf->quiet) > - strbuf_addf(err, _("Fetching submodule %s%s\n"), > - spf->prefix, ce->name); > - strvec_init(&cp->args); > - strvec_pushv(&cp->args, spf->args.v); > - strvec_push(&cp->args, default_argv); > - strvec_push(&cp->args, "--submodule-prefix"); > - > - strbuf_addf(&submodule_prefix, "%s%s/", > - spf->prefix, > - task->sub->path); > - strvec_push(&cp->args, submodule_prefix.buf); > - > - spf->count++; > - *task_cb = task; > - > - strbuf_release(&submodule_prefix); > - return 1; > - } else { > + if (!task->repo) { > struct strbuf empty_submodule_path = STRBUF_INIT; > > fetch_task_release(task); > @@ -1562,7 +1536,44 @@ static int get_next_submodule(struct child_process *cp, > ce->name); > } > strbuf_release(&empty_submodule_path); > + continue; > } > + if (!spf->quiet) > + strbuf_addf(err, _("Fetching submodule %s%s\n"), > + spf->prefix, ce->name); > + > + spf->count++; > + return task; > + } > + return NULL; > +} You could have retained the "if (task->repo) { } else { }" structure instead of adding a "continue;". Also, the "if (!spf->quiet)" could be moved into get_next_submodule(), but I see why it's there (it needs ce->name, which we otherwise don't need), so leaving it where it is in this patch is fine too. > + strbuf_addf(&submodule_prefix, "%s%s/", spf->prefix, > + task->sub->path); It would have been clearer if this line wasn't rewrapped. As a reviewer, sometimes it's hard to make the tradeoff between asking the author to make formatting changes versus leaving it alone because the reviewer has already inspected the changes and decided that any errors are only in formatting, not in logic. In this case, though, because there is only one more patch in the series and the formatting change I'm suggesting here won't really affect it that much, I think it's better if you make the formatting change for the benefit of other reviewers who are currently reviewing this patch set, and anyone looking at this commit in the future.
diff --git a/submodule.c b/submodule.c index d4227ac22d..d695dcadf4 100644 --- a/submodule.c +++ b/submodule.c @@ -1480,14 +1480,12 @@ static struct repository *get_submodule_repo_for(struct repository *r, return ret; } -static int get_next_submodule(struct child_process *cp, - struct strbuf *err, void *data, void **task_cb) +static struct fetch_task * +get_fetch_task(struct submodule_parallel_fetch *spf, + const char **default_argv, struct strbuf *err) { - struct submodule_parallel_fetch *spf = data; - for (; spf->count < spf->r->index->cache_nr; spf->count++) { const struct cache_entry *ce = spf->r->index->cache[spf->count]; - const char *default_argv; struct fetch_task *task; if (!S_ISGITLINK(ce->ce_mode)) @@ -1507,41 +1505,17 @@ static int get_next_submodule(struct child_process *cp, &spf->changed_submodule_names, task->sub->name)) continue; - default_argv = "on-demand"; + *default_argv = "on-demand"; break; case RECURSE_SUBMODULES_ON: - default_argv = "yes"; + *default_argv = "yes"; break; case RECURSE_SUBMODULES_OFF: continue; } 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); - cp->dir = task->repo->gitdir; - prepare_submodule_repo_env_in_gitdir(&cp->env_array); - cp->git_cmd = 1; - if (!spf->quiet) - strbuf_addf(err, _("Fetching submodule %s%s\n"), - spf->prefix, ce->name); - strvec_init(&cp->args); - strvec_pushv(&cp->args, spf->args.v); - strvec_push(&cp->args, default_argv); - strvec_push(&cp->args, "--submodule-prefix"); - - strbuf_addf(&submodule_prefix, "%s%s/", - spf->prefix, - task->sub->path); - strvec_push(&cp->args, submodule_prefix.buf); - - spf->count++; - *task_cb = task; - - strbuf_release(&submodule_prefix); - return 1; - } else { + if (!task->repo) { struct strbuf empty_submodule_path = STRBUF_INIT; fetch_task_release(task); @@ -1562,7 +1536,44 @@ static int get_next_submodule(struct child_process *cp, ce->name); } strbuf_release(&empty_submodule_path); + continue; } + if (!spf->quiet) + strbuf_addf(err, _("Fetching submodule %s%s\n"), + spf->prefix, ce->name); + + spf->count++; + return task; + } + return NULL; +} + +static int get_next_submodule(struct child_process *cp, struct strbuf *err, + void *data, void **task_cb) +{ + struct submodule_parallel_fetch *spf = data; + const char *default_argv = NULL; + struct fetch_task *task = get_fetch_task(spf, &default_argv, err); + + if (task) { + struct strbuf submodule_prefix = STRBUF_INIT; + + child_process_init(cp); + cp->dir = task->repo->gitdir; + prepare_submodule_repo_env_in_gitdir(&cp->env_array); + cp->git_cmd = 1; + strvec_init(&cp->args); + strvec_pushv(&cp->args, spf->args.v); + strvec_push(&cp->args, default_argv); + strvec_push(&cp->args, "--submodule-prefix"); + + strbuf_addf(&submodule_prefix, "%s%s/", spf->prefix, + task->sub->path); + strvec_push(&cp->args, submodule_prefix.buf); + *task_cb = task; + + strbuf_release(&submodule_prefix); + return 1; } if (spf->oid_fetch_tasks_nr) {
get_next_submodule() configures the parallel submodule fetch by performing two functions: * iterate the index to find submodules * configure the child processes to fetch the submodules found in the previous step Extract the index iterating code into an iterator function, get_fetch_task(), so that get_next_submodule() is agnostic of how to find submodules. This prepares for a subsequent commit will teach the fetch machinery to also iterate through the list of changed submodules (in addition to the index). Signed-off-by: Glen Choo <chooglen@google.com> --- submodule.c | 75 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 43 insertions(+), 32 deletions(-)