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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Feb 2, 2021 at 10:29 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> On Tue, 2 Feb 2021 at 06:17, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > When `-c` says "edit the commit message" it's not clear what will be
> > edited. The original's commit message? The replacement's message? A
> > combination of the two? If you can come up with a succinct way to word
> > it such that it states more precisely what exactly will be edited, it
> > would be nice, but not necessarily worth a re-roll.
>
> Here the editor shows the commented out commit message of original commit and
> the replacement commit message (of fixup -c commit) is not commented out.
>
> So maybe s/edit the commit message/edit this commit message  is better.

Yes, that would be clearer and is just as succinct.

> > This code adds to the confusion. In the function argument list, `flag`
> > has been declared as a single enum item, yet this code is treating
> > `flag` as if it is a combination of bits. So, it's not clear what the
> > intention is here. Is `flag` always going to be a specific enum item
> > in this context or is it going to be a combination of bits? If it is
> > only ever going to be a distinct enum item, then one would expect this
> > code to be written like this:
> >
> >     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.

> > By the way, the function name check_fixup_flag() doesn't necessarily
> > do a good job conveying the purpose of this function. Based upon the
> > implementation, I gather that it is checking whether the command is a
> > "fixup" command, so perhaps the name could reflect that. Perhaps
> > is_fixup() or something?
>
> 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 was wondering if the above could be rephrased like this to avoid the
> > repetition:
> > [...]
> > but perhaps it's too magical and ugly.
>
> 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.