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

From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Date: Aug 9, 2023, 16:13 UTC
Message-ID: <ZNO7IVphPf8KOC3Q@ugly>
In-Reply-To: <xmqqv8dqd2bh.fsf@gitster.g>

On Mon, Aug 07, 2023 at 01:28:50PM -0700, Junio C Hamano wrote:

Show 20 quoted lines

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

Show 7 quoted lines

>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