Glen Choo <chooglen@xxxxxxxxxx> 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.
