Re: [PATCH 1/3] diff-merges: cleanup func_by_opt()

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 15, 2022, 18:47 UTC
Message-ID: <xmqqfsgsmq4j.fsf@gitster.g>
In-Reply-To: <20220914193102.5275-2-sorganov@gmail.com>

Sergey Organov <sorganov@gmail.com> writes:

> Get rid of unneeded "else" statements in func_by_opt().

While it is true that loss of "else" will not change what the code means, this change feels subjective and I'd say it falls into "once committed, it is not worth the patch noise to go in and change" category, not a "clean-up we should do".

I haven't looked at diff-merges.c for quite a while, but I did. I notice that the code is barely commented on what each helper function is supposed to do, etc., and very hard to follow. The file needs cleaning up in that area a lot more, I would say.

Thanks.

Show 30 quoted lines

> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  diff-merges.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/diff-merges.c b/diff-merges.c
> index 7f64156b8bfe..780ed08fc87f 100644
> --- a/diff-merges.c
> +++ b/diff-merges.c
> @@ -60,15 +60,15 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)
>  		return suppress;
>  	if (!strcmp(optarg, "1") || !strcmp(optarg, "first-parent"))
>  		return set_first_parent;
> -	else if (!strcmp(optarg, "separate"))
> +	if (!strcmp(optarg, "separate"))
>  		return set_separate;
> -	else if (!strcmp(optarg, "c") || !strcmp(optarg, "combined"))
> +	if (!strcmp(optarg, "c") || !strcmp(optarg, "combined"))
>  		return set_combined;
> -	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
> +	if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
>  		return set_dense_combined;
> -	else if (!strcmp(optarg, "r") || !strcmp(optarg, "remerge"))
> +	if (!strcmp(optarg, "r") || !strcmp(optarg, "remerge"))
>  		return set_remerge_diff;
> -	else if (!strcmp(optarg, "m") || !strcmp(optarg, "on"))
> +	if (!strcmp(optarg, "m") || !strcmp(optarg, "on"))
>  		return set_to_default;
>  	return NULL;
>  }