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

From: Charvi Mendiratta <charvi077@gmail.com>
Date: Feb 4, 2021, 00:00 UTC
Message-ID: <CAPSFM5fLi-U3zVcXFip_kgchHSXiEUF9nngO2nSf31kAEBkq1w@mail.gmail.com>
In-Reply-To: <CAPig+cRxmFr_Sbwdf4OFMr8Vp1q6O6J7AbgYAD5cgdD--hgDuw@mail.gmail.com>

On Wed, 3 Feb 2021 at 10:35, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 11 quoted lines

> > >     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)

Show 6 quoted lines

> > 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.
>

[...]

Show 8 quoted lines

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