Re: [PATCH v7 6/7] diff-lib: refactor match_stat_with_submodule
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Feb 8, 2023, 17:07 UTC
- Message-ID
- <66bf75bd-b14f-2658-5f6f-8a368c1d429b@dunelm.org.uk>
- In-Reply-To
- <230208.861qn01s4g.gmgdl@evledraar.gmail.com>
On 08/02/2023 08:18, Ævar Arnfjörð Bjarmason wrote:
> > On Tue, Feb 07 2023, Calvin Wan wrote:
> Anyway, having looked at this closely I think this patch should be > dropped entirely. I don't understand how this refactoring is meant to > make the end result easier to read, reason about, or how it helps the > subsequent patch.
That's my feeling too c.f. <19f91fea-a2a9-7dc6-d940-cc10f384fe76@dunelm.org.uk>. This patch has improved since that comment on v4 but I still think we'd be better off without it.
Best Wishes
Phillip
Show 103 quoted lines
> In addition to the above diff in 7/7 you do (and that's the change this
> is meant to help):
>
> static int match_stat_with_submodule(struct diff_options *diffopt,
> const struct cache_entry *ce,
> struct stat *st, unsigned ce_option,
> - unsigned *dirty_submodule)
> + unsigned *dirty_submodule, int *defer_submodule_status,
> + unsigned *ignore_untracked)
> {
> int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
> struct diff_flags orig_flags;
> + int defer = 0;
>
> if (!S_ISGITLINK(ce->ce_mode))
> - return changed;
> + goto ret;
>
> orig_flags = diffopt->flags;
> if (!diffopt->flags.override_submodule_config)
> @@ -86,11 +92,20 @@ 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,
> + (!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);
> + }
> + }
> cleanup:
> diffopt->flags = orig_flags;
> +ret:
> + if (defer_submodule_status)
> + *defer_submodule_status = defer;
> return changed;
> }
>
> But if I rebase out this 6/7 patch and solve the conflict for 7/7 it
> becomes:
>
> @@ -65,14 +66,20 @@ static int check_removed(const struct index_state *istate, const struct cache_en
> * Return 1 when changes are detected, 0 otherwise. If the DIRTY_SUBMODULES
> * option is set, the caller does not only want to know if a submodule is
> * modified at all but wants to know all the conditions that are met (new
> - * commits, untracked content and/or modified content).
> + * commits, untracked content and/or modified content). If
> + * defer_submodule_status bit is set, dirty_submodule will be left to the
> + * caller to set. defer_submodule_status can also be set to 0 in this
> + * function if there is no need to check if the submodule is modified.
> */
> static int match_stat_with_submodule(struct diff_options *diffopt,
> const struct cache_entry *ce,
> struct stat *st, unsigned ce_option,
> - unsigned *dirty_submodule)
> + unsigned *dirty_submodule, int *defer_submodule_status,
> + unsigned *ignore_untracked)
> {
> int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
> + int defer = 0;
> +
> if (S_ISGITLINK(ce->ce_mode)) {
> struct diff_flags orig_flags = diffopt->flags;
> if (!diffopt->flags.override_submodule_config)
> @@ -80,11 +87,20 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
> if (diffopt->flags.ignore_submodules)
> changed = 0;
> else if (!diffopt->flags.ignore_dirty_submodules &&
> - (!changed || diffopt->flags.dirty_submodules))
> - *dirty_submodule = is_submodule_modified(ce->name,
> - diffopt->flags.ignore_untracked_in_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);
> + }
> + }
> diffopt->flags = orig_flags;
> }
> +
> + if (defer_submodule_status)
> + *defer_submodule_status = defer;
> return changed;
> }
>
>
> I can see how there's some room for *a* refactoring to reduce the
> subsequent diff, but not by mutch.
>
> But this commit didn't help at all. This whole "goto ret", and "goto
> cleanup" is just working around the fact that you pulled "orig_flags"
> out of the "if" scope. Normally the de-indentation would be worth it,
> but here it's not. The control flow becomes more complex to reason about
> as a result.
>