git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Sergey Organov <sorganov@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/3] diff-merges: cleanup func_by_opt()
Date: Thu, 15 Sep 2022 11:47:24 -0700	[thread overview]
Message-ID: <xmqqfsgsmq4j.fsf@gitster.g> (raw)
In-Reply-To: <20220914193102.5275-2-sorganov@gmail.com> (Sergey Organov's message of "Wed, 14 Sep 2022 22:31:00 +0300")

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.

> 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;
>  }

  reply	other threads:[~2022-09-15 18:47 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-14 19:30 [PATCH 0/3] diff-merges: minor cleanups Sergey Organov
2022-09-14 19:31 ` [PATCH 1/3] diff-merges: cleanup func_by_opt() Sergey Organov
2022-09-15 18:47   ` Junio C Hamano [this message]
2022-09-16 13:01     ` Sergey Organov
2022-09-16 16:14       ` Junio C Hamano
2022-09-16 16:28         ` Sergey Organov
2022-09-14 19:31 ` [PATCH 2/3] diff-merges: cleanup set_diff_merges() Sergey Organov
2022-09-15 20:41   ` Junio C Hamano
2022-09-16 13:38     ` Sergey Organov
2022-09-14 19:31 ` [PATCH 3/3] diff-merges: clarify log.diffMerges documentation Sergey Organov
2022-09-15 18:43   ` Junio C Hamano
2022-09-16 13:46     ` Sergey Organov
2022-09-16 16:21       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqfsgsmq4j.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=sorganov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).