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, so
turning two if() into if/elseif does not add all that much value in
readability.
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?
* Can item->commit be some other TODO_* command? What is the
reason why they can be no-op?
* 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?
And that leads me to wonder if this is better rewritten with
switch (item->command) {
case TODO_FIXUP:
...
break;
case TODO_MERGE:
...
break;
default:
/*
* all other cases:
* we can have a brief explanation on why
* they do not need anything done here if we want
*/
break;
}
