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

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> `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.

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

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

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