Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 1, 2021, 22:15 UTC
Message-ID: <CAPig+cQAG3p6rhGHzBNGPUxMXQbbLZgiw-uUpizNiEWY9wm-3A@mail.gmail.com>
In-Reply-To: <20210301084512.27170-3-charvi077@gmail.com>

On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta <charvi077@gmail.com> wrote:

Show 26 quoted lines

> `git commit --fixup=amend:<commit>` will create an "amend!" commit.
> The resulting commit message subject will be "amend! ..." where
> "..." is the subject line of <commit> and the initial message
> body will be <commit>'s message. -m can be used to override the
> message body.
>
> The "amend!" commit when rebased with --autosquash will fixup the
> contents and replace the commit message of <commit> with the
> "amend!" commit's message body.
>
> In order to prevent rebase from creating commits with an empty
> message we refuse to create an "amend!" commit if commit message
> body is empty.
>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
> diff --git a/builtin/commit.c b/builtin/commit.c
> @@ -105,7 +105,8 @@ static const char *template_file;
> +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,
> +                                                                struct pretty_print_context *ctx) {
> +       /*
> +        * If we amend the 'amend!' commit then we don't want to
> +        * duplicate the subject line.
> +        */
> +       const char *format = NULL;
> +       if (starts_with(sb->buf, "amend! amend!"))

Is the content of the incoming strbuf created mechanically so that we know that there will only ever be one space between the two "amend!" literals? If not, then this starts_with() check feels fragile. (Compare with the code in sequencer.c which checks for this sort of duplication but is tolerant of one or more spaces, not just a single space.)

> +               format = "%b";
> +       else
> +               format = "%B";

It's subjective and minor, but this could be expressed more compactly as:

    const char *fmt = starts_with(...) ? "%b" : "%B";

Also, no need to initialize `format` to NULL since it gets assigned in all code paths.

Not worth a re-roll.

Show 7 quoted lines

> @@ -745,15 +761,33 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> +               if (!strcmp(fixup_prefix, "amend")) {
> +                       if (have_option_m)
> +                               die(_("cannot combine -m with --fixup:%s"), fixup_message);
> +                       else
> +                               prepare_amend_commit(commit, &sb, &ctx);
> +               }

This is minor, but the way this is written, the error case and the normal case appear to have the same significance, whereas, if you write it like this:

    if (!strcmp(...)) {
        if (have_option_m)
            die(...);
        prepare_amend_commit(...);
    }

then it's easier to see that you're checking for and getting an error case out of the way early, which allows the reader to concentrate without distraction on the normal case. As a minor benefit, you also get to eliminate an indentation level for the normal case, which could be important if more code is added to that case.

Not worth a re-roll.

Show 15 quoted lines

> @@ -1227,6 +1267,28 @@ static int parse_and_validate_options(int argc, const char *argv[],
> +       if (fixup_message) {
> +               /*
> +                * As `amend` suboption contains only alpha
> +                * character. So check if first non alpha
> +                * character in fixup_message is ':'.
> +                */
> +               size_t len = get_alpha_len(fixup_message);
> +               if (len && fixup_message[len] == ':') {
> +                       fixup_message[len++] = '\0';
> +                       fixup_commit = fixup_message + len;
> +                       if (starts_with("amend", fixup_message))
> +                               fixup_prefix = "amend";
> +                       else
> +                               die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);

I haven't read ahead in the series yet, but I presume you're making this code extra generic because you plan to support additional `fixup` options (such as `reword`), but I wonder if the cognitive overhead is warranted or you could get by with something simpler, such as:

    if (skip_prefix(msg, "amend:", &arg) ||
        skip_prefix(msg, "reword:", &arg)) {
        ...
    }

Also, am I misreading when I think that the use of starts_with() could be replaced with a simple strcmp() since you've already inserted a '\0' immediately after the final alphabetic character?

Not necessarily worth a re-roll.

Show 5 quoted lines

> @@ -1663,6 +1729,19 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> +               if (message_is_empty(&body, cleanup_mode)) {
> +                       rollback_index_files();
> +                       fprintf(stderr, _("Aborting commit due to empty commit message body.\n"));
> +                       exit(1);

I was wondering why you are capitalizing the error message (these days we don't) and using exit() instead of die(), but I see that you're mirroring existing practice in this function. Okay.