Re: [PATCH v8 0/6] submodule: parallelize diff
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Feb 9, 2023, 01:42 UTC
- Message-ID
- <230209.867cwrzk1l.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20230209000212.1892457-1-calvinwan@google.com>
On Thu, Feb 09 2023, Calvin Wan wrote:
Show 64 quoted lines
> 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/