Re: [PATCH v8 0/6] submodule: parallelize diff

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Calvin

On 09/02/2023 00:02, Calvin Wan wrote:
> Original cover letter for context:
> https://lore.kernel.org/git/20221011232604.839941-1-calvinwan@google.com/
> 
> This reroll contains stylistic changes suggested by Avar and Phillip,
> and includes a range-diff below.
> 
> Calvin Wan (6):
>    run-command: add duplicate_output_fn to run_processes_parallel_opts
>    submodule: strbuf variable rename
>    submodule: move status parsing into function
>    submodule: refactor is_submodule_modified()
>    diff-lib: refactor out diff_change logic
>    diff-lib: parallelize run_diff_files for submodules
> 
>   Documentation/config/submodule.txt |  12 ++
>   diff-lib.c                         | 133 +++++++++++----
>   run-command.c                      |  16 +-
>   run-command.h                      |  25 +++
>   submodule.c                        | 266 ++++++++++++++++++++++++-----
>   submodule.h                        |   9 +
>   t/helper/test-run-command.c        |  20 +++
>   t/t0061-run-command.sh             |  39 +++++
>   t/t4027-diff-submodule.sh          |  31 ++++
>   t/t7506-status-submodule.sh        |  25 +++
>   10 files changed, 497 insertions(+), 79 deletions(-)
> 
> Range-diff against v7:
> 6:  0d35fcc38d < -:  ---------- diff-lib: refactor match_stat_with_submodule
> 7:  fd1eec974d ! 6:  bb25dadbe5 diff-lib: parallelize run_diff_files for submodules
>      @@ diff-lib.c: static int check_removed(const struct index_state *istate, const str
>       +				     unsigned *ignore_untracked)
>        {
>        	int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
>      - 	struct diff_flags orig_flags;
>      +-	if (S_ISGITLINK(ce->ce_mode)) {
>      +-		struct diff_flags orig_flags = diffopt->flags;
>      +-		if (!diffopt->flags.override_submodule_config)
>      +-			set_diffopt_flags_from_submodule_config(diffopt, ce->name);
>      +-		if (diffopt->flags.ignore_submodules)
>      +-			changed = 0;
>      +-		else if (!diffopt->flags.ignore_dirty_submodules &&
>      +-			 (!changed || diffopt->flags.dirty_submodules))
>      ++	struct diff_flags orig_flags;
>       +	int defer = 0;
>      -
>      - 	if (!S_ISGITLINK(ce->ce_mode))
>      --		return changed;
>      ++
>      ++	if (!S_ISGITLINK(ce->ce_mode))
>       +		goto ret;
>      -
>      - 	orig_flags = diffopt->flags;
>      - 	if (!diffopt->flags.override_submodule_config)
>      -@@ diff-lib.c: static int match_stat_with_submodule(struct diff_options *diffopt,
>      - 		goto cleanup;
>      - 	}
>      - 	if (!diffopt->flags.ignore_dirty_submodules &&
>      --	    (!changed || diffopt->flags.dirty_submodules))
>      --		*dirty_submodule = is_submodule_modified(ce->name,
>      ++
>      ++	orig_flags = diffopt->flags;
>      ++	if (!diffopt->flags.override_submodule_config)
>      ++		set_diffopt_flags_from_submodule_config(diffopt, ce->name);
>      ++	if (diffopt->flags.ignore_submodules) {
>      ++		changed = 0;
>      ++		goto cleanup;
>      ++	}
>      ++	if (!diffopt->flags.ignore_dirty_submodules &&
>       +	    (!changed || diffopt->flags.dirty_submodules)) {
>       +		if (defer_submodule_status && *defer_submodule_status) {
>       +			defer = 1;
>       +			*ignore_untracked = diffopt->flags.ignore_untracked_in_submodules;
>       +		} else {
>      -+			*dirty_submodule = is_submodule_modified(ce->name,
>      - 					 diffopt->flags.ignore_untracked_in_submodules);
>      + 			*dirty_submodule = is_submodule_modified(ce->name,
>      +-								 diffopt->flags.ignore_untracked_in_submodules);
>      +-		diffopt->flags = orig_flags;
>      ++					 diffopt->flags.ignore_untracked_in_submodules);
>       +		}
>      -+	}
>      - cleanup:
>      - 	diffopt->flags = orig_flags;
>      + 	}
>      ++cleanup:
>      ++	diffopt->flags = orig_flags;
>       +ret:
>       +	if (defer_submodule_status)

The idea behind the suggestion to drop the previous patch from the last 
version was to stop refactoring the if block and get away from having 
these labels. Can't you just add the "if (defer_submodule_status && 
...)" into the existing code?

Best Wishes

Phillip

>       +		*defer_submodule_status = defer;
>      @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)
>        				       changed, istate, ce))
>        			continue;
>        	}
>      -+	if (submodules.nr > 0) {
>      -+		int parallel_jobs;
>      -+		if (git_config_get_int("submodule.diffjobs", &parallel_jobs))
>      ++	if (submodules.nr) {
>      ++		unsigned long parallel_jobs;
>      ++		struct string_list_item *item;
>      ++
>      ++		if (git_config_get_ulong("submodule.diffjobs", &parallel_jobs))
>       +			parallel_jobs = 1;
>       +		else if (!parallel_jobs)
>       +			parallel_jobs = online_cpus();
>      -+		else if (parallel_jobs < 0)
>      -+			die(_("submodule.diffjobs cannot be negative"));
>       +
>       +		if (get_submodules_status(&submodules, parallel_jobs))
>       +			die(_("submodule status failed"));
>      -+		for (size_t i = 0; i < submodules.nr; i++) {
>      -+			struct submodule_status_util *util = submodules.items[i].util;
>      ++		for_each_string_list_item(item, &submodules) {
>      ++			struct submodule_status_util *util = item->util;
>       +
>       +			if (diff_change_helper(&revs->diffopt, util->newmode,
>       +				       util->dirty_submodule, util->changed,
>      @@ submodule.c: int submodule_touches_in_range(struct repository *r,
>       +	int result;
>       +
>       +	struct string_list *submodule_names;
>      -+
>      -+	/* Pending statuses by OIDs */
>      -+	struct status_task **oid_status_tasks;
>      -+	int oid_status_tasks_nr, oid_status_tasks_alloc;
>       +};
>       +
>        struct submodule_parallel_fetch {
>      @@ submodule.c: unsigned is_submodule_modified(const char *path, int ignore_untrack
>       +	struct status_task *task = task_cb;
>       +
>       +	sps->result = 1;
>      -+	strbuf_addf(err,
>      -+	    _(status_porcelain_start_error),
>      -+	    task->path);
>      ++	strbuf_addf(err, _(STATUS_PORCELAIN_START_ERROR), task->path);
>       +	return 0;
>       +}
>       +
>      @@ submodule.c: unsigned is_submodule_modified(const char *path, int ignore_untrack
>       +
>       +	if (retvalue) {
>       +		sps->result = 1;
>      -+		strbuf_addf(err,
>      -+		    _(status_porcelain_fail_error),
>      -+		    task->path);
>      ++		strbuf_addf(err, _(STATUS_PORCELAIN_FAIL_ERROR), task->path);
>       +	}
>       +
>       +	parse_status_porcelain_strbuf(&task->out,