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

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Jan 29, 2021 at 1:24 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> Add options to `fixup` command to fixup both the commit contents and
> message. `fixup -C` command is used to replace the original commit
> message and `fixup -c`, additionally allows to edit the commit message.

In the cover letter for this series, you had this additional information:

    This convention is similar to the existing `merge` command in the
    interactive rebase, that also supports `-c` and `-C` options with
    similar meanings.

which helps to explain the choice of -c and -C. It might be nice to
include that explanation in this commit message, as well (but not
itself worth a re-roll).

> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> @@ -44,7 +44,9 @@ void append_todo_help(int command_count,
>  "s, squash <commit> = use commit, but meld into previous commit\n"
> -"f, fixup <commit> = like \"squash\", but discard this commit's log message\n"
> +"f, fixup [-C | -c] <commit> = like \"squash\", but discard this\n"
> +"                   commit's log message. Use -C to replace with this\n"
> +"                   commit message or -c to edit the commit message\n"

This change jarringly introduces the first and only use of a period
and capitalized word in the to-do help text. Perhaps instead say:

    ... like \"squash\", but discard this
    commit's log message; use -C to replace with this
    commit message or -c to edit the commit message

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.

> diff --git a/sequencer.c b/sequencer.c
> @@ -1718,6 +1718,12 @@ static int is_pick_or_similar(enum todo_command command)
> +enum todo_item_flags {
> +       TODO_EDIT_MERGE_MSG    = (1 << 0),
> +       TODO_REPLACE_FIXUP_MSG = (1 << 1),
> +       TODO_EDIT_FIXUP_MSG    = (1 << 2),
> +};

I'm confused. These enumeration items are defined as bit flags,
implying that they may be combined, however...

> @@ -1734,32 +1740,176 @@ static size_t subject_length(const char *body)
> +static int check_fixup_flag(enum todo_command command,
> +                           enum todo_item_flags flag)

...here and elsewhere, you declare the argument as a strict
`todo_item_flags` enum item rather than `unsigned` which is the
typical declaration when combining bit flag values. So, the picture
thus far is confusing. Are the `todo_item_flags` values distinct
unique values which will never be combined, or are they meant to be
combined?

> +{
> +       return command == TODO_FIXUP && ((flag & TODO_REPLACE_FIXUP_MSG) ||
> +                                        (flag & TODO_EDIT_FIXUP_MSG));
> +}

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

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)

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?

> +static int append_squash_message(struct strbuf *buf, const char *body,
> +                        enum todo_command command, struct replay_opts *opts,
> +                        enum todo_item_flags flag)
> +{
> +       /*
> +        * amend is non-interactive and not normally used with fixup!
> +        * or squash! commits, so only comment out those subjects when
> +        * squashing commit messages.
> +        */
> +       if (starts_with(body, "amend!") ||
> +           ((command == TODO_SQUASH || seen_squash(opts)) &&
> +            (starts_with(body, "squash!") || starts_with(body, "fixup!"))))
>                 commented_len = subject_length(body);

I understand from the cover letter that "amend!" is being added by
this series, however, no patch up to this point, nor this patch
itself, adds "amend!" functionality, so it's surprising to see it
being tested here. As a reader, I would expect code/comments related
to "amend!" to be added in the patch which actually introduces
"amend!" rather than doing it here.

> +       /* fixup -C after squash behaves like squash */
> +       if (check_fixup_flag(command, flag) && !seen_squash(opts)) {
> +               if (opts->signoff)
> +                       append_signoff(buf, 0, 0);
> +
> +               if ((command == TODO_FIXUP) &&
> +                   (flag & TODO_REPLACE_FIXUP_MSG) &&
> +                   (file_exists(rebase_path_fixup_msg()) ||
> +                    !file_exists(rebase_path_squash_msg()))) {

Is the expression `command == TODO_FIXUP` redundant here considering
that the only way we got this far is if check_fixup_flag() returned
true, in which case we know that command is TODO_FIXUP? Or am I
missing something?

> @@ -2281,6 +2436,18 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
> +       if (item->command == TODO_FIXUP) {
> +               if (skip_prefix(bol, "-C", &bol) &&
> +                  (*bol == ' ' || *bol == '\t')) {
> +                       bol += strspn(bol, " \t");
> +                       item->flags |= TODO_REPLACE_FIXUP_MSG;
> +               } else if (skip_prefix(bol, "-c", &bol) &&
> +                                 (*bol == ' ' || *bol == '\t')) {
> +                       bol += strspn(bol, " \t");
> +                       item->flags |= TODO_EDIT_FIXUP_MSG;
> +               }
> +       }

I was wondering if the above could be rephrased like this to avoid the
repetition:

    if (bol[0] == '-' && tolower(bol[1]) == 'c' &&
        (bol[2] == ' ' || bol[2] == '\t') {
        item->flags |= bol[1] == 'C' ?
            TODO_REPLACE_FIXUP_MSG :
            TODO_EDIT_FIXUP_MSG;
        bol += 2 + strspn(bol + 2, " \t");
    }

but perhaps it's too magical and ugly.