From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 08 Feb 2023 14:22:33 GMT
Subject: Re: [PATCH v7 6/7] diff-lib: refactor match_stat_with_submodule
Message-ID: <c9b10e8b-641c-ff29-95a4-2ac3f3219c0d@dunelm.org.uk>
In-Reply-To: <20230207181706.363453-7-calvinwan@google.com>

Hi Calvin

On 07/02/2023 18:17, Calvin Wan wrote:
> Flatten out the if statements in match_stat_with_submodule so the
> logic is more readable and easier for future patches to add to.
> orig_flags didn't need to be set if the cache entry wasn't a
> GITLINK so defer setting it.
> 
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>   diff-lib.c | 28 +++++++++++++++++-----------
>   1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/diff-lib.c b/diff-lib.c
> index 7101cfda3f..e18c886a80 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -73,18 +73,24 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
>   				     unsigned *dirty_submodule)
>   {
>   	int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
> -	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))
> -			*dirty_submodule = is_submodule_modified(ce->name,
> -								 diffopt->flags.ignore_untracked_in_submodules);
> -		diffopt->flags = orig_flags;
> +	struct diff_flags orig_flags;
> +
> +	if (!S_ISGITLINK(ce->ce_mode))
> +		return changed;
> +
> +	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;

Looking ahead to patch 7 there are no new uses of the "cleanup" label so 
I think it would be simpler to leave the code as it was, rather than 
changing the "else if" below to "if" and adding the goto here.

Best Wishes

Phillip

>   	}
> +	if (!diffopt->flags.ignore_dirty_submodules &&
> +	    (!changed || diffopt->flags.dirty_submodules))
> +		*dirty_submodule = is_submodule_modified(ce->name,
> +					 diffopt->flags.ignore_untracked_in_submodules);
> +cleanup:
> +	diffopt->flags = orig_flags;
>   	return changed;
>   }
>