public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Sergey Organov <sorganov@gmail•com>
To: Junio C Hamano <gitster@pobox•com>
Cc: Elijah Newren <newren@gmail•com>, git@vger•kernel.org
Subject: Re: [PATCH 1/3] diff-merges: cleanup func_by_opt()
Date: Fri, 16 Sep 2022 16:01:07 +0300	[thread overview]
Message-ID: <87wna3jwx8.fsf@osv.gnss.ru> (raw)
In-Reply-To: <xmqqfsgsmq4j.fsf@gitster.g> (Junio C. Hamano's message of "Thu, 15 Sep 2022 11:47:24 -0700")

Junio C Hamano <gitster@pobox•com> writes:

> 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 agree the "else" vs "no else" is subjective, but the problem in fact
is that the first "if", unlike the rest of them, already had no "else",
making the code inconsistent. So the fix should either be adding one
"else" to the first "if", or remove all of the "else". I chose the
latter, to end up with less noisy code.

>
> 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.

I believe each helper function does exactly what its name suggests, so
no comments are needed. I hate to add comments that actually just say
the same as function name, so I'd rathe consider to rename some
functions instead should somebody point out to problematic ones.

That said, it seems Elijah Newren had not much trouble adding his
--remerge-diff capability to the diff-merges code, so the code must be
not that hard to follow even in its current state.

Thanks,
-- Sergey Organov.

next prev parent reply	other threads:[~2022-09-16 13:01 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
2022-09-16 13:01     ` Sergey Organov [this message]
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=87wna3jwx8.fsf@osv.gnss.ru \
    --to=sorganov@gmail$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=gitster@pobox$(echo .)com \
    --cc=newren@gmail$(echo .)com \
    /path/to/YOUR_REPLY

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox