From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 30 Nov 2022 14:36:20 GMT
Subject: Re: [PATCH v4 4/5] diff-lib: refactor match_stat_with_submodule
Message-ID: <19f91fea-a2a9-7dc6-d940-cc10f384fe76@dunelm.org.uk>
In-Reply-To: <20221108184200.2813458-5-calvinwan@google.com>

Hi Calven

On 08/11/2022 18:41, 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.

Thanks for splitting this change out. I have to say I find the original 
quite a bit easier to read. If you're worried about the code added in 
the next commit being too indented perhaps you could move the body of 
the if statement into a separate function.

Best Wishes

Phillip

> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>   diff-lib.c | 29 ++++++++++++++++++-----------
>   1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/diff-lib.c b/diff-lib.c
> index 2edea41a23..f5257c0c71 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -73,18 +73,25 @@ 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))
> +		goto ret;
> +
> +	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))
> +		*dirty_submodule = is_submodule_modified(ce->name,
> +					 diffopt->flags.ignore_untracked_in_submodules);
> +cleanup:
> +	diffopt->flags = orig_flags;
> +ret:
>   	return changed;
>   }
>