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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 16, 2022, 16:14 UTC
Message-ID: <xmqqy1uji9dz.fsf@gitster.g>
In-Reply-To: <87wna3jwx8.fsf@osv.gnss.ru>

Sergey Organov <sorganov@gmail.com> writes:

Show 14 quoted lines

> 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) { ... }".

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

Yup, see above for the reason why I would choose else-if cascade if I had to but I do not care too deeply either way in this particular case ;-)