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

[Oswald Buddenhagen <oswald.buddenhagen@xxxxxx>]

On Mon, Aug 07, 2023 at 01:28:50PM -0700, Junio C Hamano wrote:
> Oswald Buddenhagen <oswald.buddenhagen@xxxxxx> 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