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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 2, 2021, 00:47 UTC
Message-ID: <CAPig+cQeBE7m8wf1e_soVrpvL3==u50MPyb90NwWLnFiUz1Byw@mail.gmail.com>
In-Reply-To: <20210129182050.26143-7-charvi077@gmail.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).

Show 9 quoted lines

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

Show 7 quoted lines

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

Show 13 quoted lines

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

Show 9 quoted lines

> +       /* 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?

Show 12 quoted lines

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