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
