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@xxxxxx>
---
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
