Re: [PATCH 6/8] submodule: extract get_fetch_task()
From: Jonathan Tan <hidden>
Date: 2022-02-10 19:34:05
Glen Choo [off-list ref] 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.
quoted hunk ↗ jump to hunk
@@ -1507,41 +1505,17 @@ static int get_next_submodule(struct child_process *cp,
[snip]
quoted hunk ↗ jump to hunk
- 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.