Re: [PATCH v6 5/6] diff-lib: parallelize run_diff_files for submodules
- From
Glen Choo <chooglen@google.com>
- Date
- Jan 26, 2023, 09:09 UTC
- Message-ID
- <kl6llelpvf0i.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <20230117193041.708692-6-calvinwan@google.com>
As Jonathan mentioned in [1], I think we should refactor functions out from the serial implementation in a preparatory patch, then use those functions to implement the parallel version in this patch. In its current form, there is a fair amount of duplicated code, which makes it tricky to review because of the additional overhead of checking what the duplicated code does and whether we've copied it correcly.
For cleanliness, I'll only point out the duplicated code in this email; I'll comment on other things I spotted in another one.
[1] https://lore.kernel.org/git/20221128210125.2751300-1-jonathantanmy@google.com/
Calvin Wan <calvinwan@google.com> writes:
Show 22 quoted lines
> + for (size_t i = 0; i < submodules.nr; i++) {
> + struct submodule_status_util *util = submodules.items[i].util;
> + struct cache_entry *ce = util->ce;
> + unsigned int oldmode;
> + const struct object_id *old_oid, *new_oid;
> +
> + if (!util->changed && !util->dirty_submodule) {
> + ce_mark_uptodate(ce);
> + mark_fsmonitor_valid(istate, ce);
> + if (!revs->diffopt.flags.find_copies_harder)
> + continue;
> + }
> + oldmode = ce->ce_mode;
> + old_oid = &ce->oid;
> + new_oid = util->changed ? null_oid() : &ce->oid;
> + diff_change(&revs->diffopt, oldmode, util->newmode,
> + old_oid, new_oid,
> + !is_null_oid(old_oid),
> + !is_null_oid(new_oid),
> + ce->name, 0, util->dirty_submodule);
> + }
> + }The lines from "if (!util->changed && !util->dirty_submodule)" onwards are copied from earlier in run_diff_files(). This might be refactored into something like diff_submodule_change().
Show 12 quoted lines
> +static struct status_task *
> +get_status_task_from_index(struct submodule_parallel_status *sps,
> + struct strbuf *err)
> +{
> + for (; sps->index_count < sps->submodule_names->nr; sps->index_count++) {
> + struct submodule_status_util *util = sps->submodule_names->items[sps->index_count].util;
> + struct status_task *task;
> + struct strbuf buf = STRBUF_INIT;
> + const char *git_dir;
> +
> + strbuf_addf(&buf, "%s/.git", util->path);
> + git_dir = read_gitfile(buf.buf);This...
Show 20 quoted lines
> +static int get_next_submodule_status(struct child_process *cp,
> + struct strbuf *err, void *data,
> + void **task_cb)
> +{
> + struct submodule_parallel_status *sps = data;
> + struct status_task *task = get_status_task_from_index(sps, err);
> +
> + if (!task)
> + return 0;
> +
> + child_process_init(cp);
> + prepare_submodule_repo_env_in_gitdir(&cp->env);
> +
> + strvec_init(&cp->args);
> + strvec_pushl(&cp->args, "status", "--porcelain=2", NULL);
> + if (task->ignore_untracked)
> + strvec_push(&cp->args, "-uno");
> +
> + prepare_submodule_repo_env(&cp->env);
> + cp->git_cmd = 1;this...
Show 12 quoted lines
> +static int status_start_failure(struct strbuf *err,
> + void *cb, void *task_cb)
> +{
> + struct submodule_parallel_status *sps = cb;
> + struct status_task *task = task_cb;
> +
> + sps->result = 1;
> + strbuf_addf(err,
> + _("could not run 'git status --porcelain=2' in submodule %s"),
> + task->path);
> + return 0;
> +}this...
Show 15 quoted lines
> +static int status_finish(int retvalue, struct strbuf *err,
> + void *cb, void *task_cb)
> +{
> + struct submodule_parallel_status *sps = cb;
> + struct status_task *task = task_cb;
> + struct string_list_item *it =
> + string_list_lookup(sps->submodule_names, task->path);
> + struct submodule_status_util *util = it->util;
> +
> + if (retvalue) {
> + sps->result = 1;
> + strbuf_addf(err,
> + _("'git status --porcelain=2' failed in submodule %s"),
> + task->path);
> + }and this are all copied from different parts of is_submodule_modified(). To refactor them out, I think we could combine the first two into "setup_submodule_status()". The last one could be moved into "process_submodule_status_result()" or perhaps we could find a way to combine it into parse_status_porcelain().