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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Calvin Wan <calvinwan@google.com>
Cc: git@vger.kernel.org, chooglen@google.com, newren@gmail.com,
	jonathantanmy@google.com, phillip.wood123@gmail.com
Subject: Re: [PATCH v8 0/6] submodule: parallelize diff
Date: Thu, 09 Feb 2023 02:42:15 +0100	[thread overview]
Message-ID: <230209.867cwrzk1l.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <20230209000212.1892457-1-calvinwan@google.com>

On Thu, Feb 09 2023, Calvin Wan wrote:

> 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)
>      +		*defer_submodule_status = defer;
>     @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)
>       				       changed, istate, ce))

I think you dropped the 7/8 per my suggestion in [1]. I think this 6/6
is actually worse than the v6.

I.e. it seems you dropped the previous refactoring commit by squashing
the refactoring+functional change together.

What I was pointing out in [1] was that you don't need the refactoring,
and that both the change itself and the end-state is much easier to look
at and reason about as a result

I.e. I think the diff in your 6/6 should just be what's after "it
becomes" in [1] (maybe with some pre-refactoring, e.g. we could add the
braces first or whatever).

But in case you strongly prefer the current end-state I think having
your previous refactoring prep would be better, because it would at
least split off some of the refactoring & functional change.

I haven't looked as deeply at this v8 as v7 for the rest, but from
skimming the range-diff it all looked good otherwise.

1. https://lore.kernel.org/git/230208.861qn01s4g.gmgdl@evledraar.gmail.com/