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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Taylor & Oswald

On 23/03/2023 20:32, Taylor Blau wrote:
> On Thu, Mar 23, 2023 at 05:22:35PM +0100, Oswald Buddenhagen wrote:
>> Make it obvious that the two conditional branches are mutually
>> exclusive.
>>
>> As a drive-by, remove a pair of unnecessary braces.
>>
>> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>> ---
>>   sequencer.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index 3be23d7ca2..9169876441 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
>>   			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) {
> 
> I dunno. I think seeing adjacent
> 
>      if (item->command == TODO_ABC)
> 
> and
> 
>      if (item->command == TODO_XYZ)
> 
> makes it clear that these two are mutually exclusive, since TODO_ABC !=
> TODO_XYZ.

I agree, it is easy to see that they are testing different conditions 
and item->command is not mutated in between

> So I don't mind the unnecessary brace cleanup, but I don't think that
> this adds additional clarity around these two if-statements.
> 
> Specifically: why not combine these two with if-statement that proceeds
> it? That might look something like:

I think you're looking at parse_insn_line() here rather than 
todo_list_to_strbuf() but your analysis of this patch still stands.

Best Wishes

Phillip

> 
>      if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
>          item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
>        ...
>      } else if (item->command == TODO_FIXUP) {
>        ...
>      } else if (item->command == TODO_MERGE) {
>        ...
>      }
> 
> but at that point, you might consider something like:
> 
>      switch (item->command) {
>      case TODO_EXEC:
>      case TODO_LABEL:
>      case TODO_RESET:
>      case TODO_UPDATE_REF:
>        ...
>        break;
>      case TODO_FIXUP:
>        ...
>        break;
>      case TODO_MERGE:
>        ...
>        break;
>      }
> 
> which is arguably clearer, but I have a hard time justifying as
> worthwhile. TBH, it feels like churn to me, but others may disagree and
> see it differently.
> 
> Thanks,
> Taylor