Re: [PATCH] rebase: clarify conditionals in todo_list_to_strbuf()
From: Taylor Blau <hidden>
Date: 2023-03-23 20:32:46
On Thu, Mar 23, 2023 at 05:22:35PM +0100, Oswald Buddenhagen wrote:
quoted hunk ↗ jump to hunk
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.
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:
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