Re: [PATCH v4 6/9] rebase -i: add fixup [-C | -c] command

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux