Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()

[Oswald Buddenhagen <oswald.buddenhagen@gmx.de>]

On Mon, Aug 07, 2023 at 01:28:50PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>>  			if (item->command == TODO_FIXUP) {
>>  				if (item->flags & TODO_EDIT_FIXUP_MSG)
>>  					strbuf_addstr(buf, " -c");
>> -				else if (item->flags & TODO_REPLACE_FIXUP_MSG) {
>> +				else if (item->flags & TODO_REPLACE_FIXUP_MSG)
>>  					strbuf_addstr(buf, " -C");
>> -				}
>> -			}
>> -
>> -			if (item->command == TODO_MERGE) {
>> +			} else if (item->command == TODO_MERGE) {
>>  				if (item->flags & TODO_EDIT_MERGE_MSG)
>>  					strbuf_addstr(buf, " -c");
>>  				else
>
>This patch as it stands is a strict Meh at least to me, as we know
>item->command is not something we will mess with in the loop,
>
the "we know" is actually something the reader needs to establish in 
their mind. it's simply unnecessary cognitive load.

>so
>turning two if() into if/elseif does not add all that much value in
>readability.
>
but it adds *some* value, and i don't think it's very constructive to 
fight that. in fact, i find the whole thread rather demotivating, and 
it's ironic that felipe's response was the most reasonable one.

>Having said that.
>
>The code makes casual readers curious about other things.
>
> * Are FIXUP and MERGE the only two commands that need to be treated
>   differently here?
>
yes, and it's obvious why. i don't think that explaining it in prose 
would make the answer any more accessible.

> * Can item->commit be some other TODO_* command?
>
the fact that it's an else-if implies that much. the definite yes is 
clear from the bigger context.

>What is the reason why they can be no-op?
>
i have no clue what you're referring to.

> * When one wants to invent a new kind of TODO_* command, what is
>   the right way to deal with it in this if/else cascade?
>
i think that someone who actually wants to modify the code can be 
expected to come up with an answer themselves, as this is a much rarer 
occurrence than just reading the code.

>And that leads me to wonder if this is better rewritten with
>
>	switch (item->command) {
>
as the commit message was meant to imply, my answer to that is no.

regards