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:

quoted

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