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

From: Sergey Organov <sorganov@gmail.com>
Date: Sep 16, 2022, 16:28 UTC
Message-ID: <875yhnjnbo.fsf@osv.gnss.ru>
In-Reply-To: <xmqqy1uji9dz.fsf@gitster.g>

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

Show 28 quoted lines

> Sergey Organov <sorganov@gmail.com> writes:
>
>> 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.
>
> This is a static helper function about a single "optarg" string that
> wanted to say "switch(optarg) { case "off": ... }" but couldn't in
> C, and I happen to view if strcmp else if strcmp ... sequence on the
> same string a poor-man's substitute for such a construct.  So my
> take of it is to call the second "if" not being "else if" a problem,
> not the rest of it.  If we add a new condition on a different input,
> making it "if (x) ...; switch(optarg) { ... }" that talks about
> something other than optarg, then writing it all with "if" without
> "else if" would make it harder to see the pattern, but I do not care
> too deeply either way, because this is unlikely to gain any logic
> more involved than "switch(optarg) { ... }".

Well, I even tend to write this switch(optarg) as:

if (0) ;
else if(...) {
  ...
}
else if(...) { 
  ...
}

to make the first actual "case" look the same way as the rest, and then through suitable defines it finally looks like:

IF_SWITCH
IF_CASE(...) {
  ...
}
IF_CASE(...) {
  ...
}

making the intent explicit.

Just saying.

Thanks, -- Sergey Organov