Re: [PATCH v6 5/6] diff-lib: parallelize run_diff_files for submodules

[Glen Choo <chooglen@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:

> +		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().

> +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...

> +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...

> +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...

> +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().