On Wed, 3 Feb 2021 at 10:35, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > > return command == TODO_FIXUP && > > > (flag == TODO_REPLACE_FIXUP_MSG || > > > flag == TODO_EDIT_FIXUP_MSG); > > > > I admit it resulted in a bit of confusion. Here, its true that flag is always > > going to be specific enum item( as command can be merge -c, fixup -c, or > > fixup -C ) and I combined the bag of bits to denote > > the specific enum item. So, maybe we can go with the first method? > > Sounds fine. It would clarify the intent. > (Apology for confusion) After, looking again at the source code, as we are using the flag element of the structure todo_item of in sequencer.h. So, I think right way is to let it be in binary only and change type from 'enum todo_item_flag' to 'unsigned' , as you suggested below (better than first method) : Otherwise, if `flag` will actually be a bag of bits, then the argument should be declared as such: static int check_fixup_flag(enum todo_command command, unsigned flag) > > Agree, here it's checking if the command is fixup and the flag value ( > > which implies either user has given command fixup -c or fixup -C ) > > So, I wonder if we can write is_fixup_flag() ? > > Reasonable. > [...] > > I agree, this [tolower(bol[1]) == 'c'] is actually doing all the > > magic, but I am not > > sure if we should change it or not ? As in the source code just after > > this code we > > are checking in a similar way for the 'merge' command. So, maybe implementing > > in a similar way is easier to read ? > > Keeping it similar to nearby code makes sense. Thanks for confirming! Thanks and Regards, Charvi