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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 11, 2021, 06:25 UTC
Message-ID: <CAPig+cRxZFV7DDtnYOxRqEYU4PwXK8gb2HSehDyGorqdgsmoNQ@mail.gmail.com>
In-Reply-To: <20210310194306.32565-3-charvi077@gmail.com>

On Wed, Mar 10, 2021 at 2:44 PM Charvi Mendiratta <charvi077@gmail.com> wrote:

Show 17 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.
>
> 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.
> [...]
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
> diff --git a/builtin/commit.c b/builtin/commit.c
> @@ -681,6 +683,23 @@ static void adjust_comment_line_char(const struct strbuf *sb)
> +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,
> +                               struct pretty_print_context *ctx) {
> +
> +       const char *buffer, *subject, *fmt;

Two style nits:

* opening curly brace of function goes on its own line

* we don't normally have a blank line at the top of the function body
preceding the declarations

So:

    static int prepare_amend_commit(...)
    {
        const  char *buffer, *subject, *fmt;

Show 11 quoted lines

> +       buffer = get_commit_buffer(commit, NULL);
> +       find_commit_subject(buffer, &subject);
> +       /*
> +        * If we amend the 'amend!' commit then we don't want to
> +        * duplicate the subject line.
> +        */
> +       fmt = starts_with(subject, "amend!") ? "%b" : "%B";
> +       format_commit_message(commit, fmt, sb, ctx);
> +       unuse_commit_buffer(commit, buffer);
> +       return 0;
> +}

What is the significance of this function's return value? At least in this patch, the single caller of this function ignores the return value, which suggests that the function need not return any value. Will a later patch add other possible return values to indicate an error or something?

Show 8 quoted lines

> @@ -745,15 +764,32 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> +               char *fmt = xstrfmt("%s! %%s\n\n", fixup_prefix);
> +               commit = lookup_commit_reference_by_name(fixup_commit);
>                 if (!commit)
> +                       die(_("could not lookup commit %s"), fixup_commit);
>                 ctx.output_encoding = get_commit_output_encoding();
> +               format_commit_message(commit, fmt, &sb, &ctx);
> +               free(fmt);

Nit: it would reduce the cognitive load slightly if `fmt` is prepared
just before it is used rather than being prepared at the top of the
block:

    fmt = xstrfmt("%s! %%s\n\n", fixup_prefix);
    format_commit_message(commit, fmt, &sb, &ctx);
    free(fmt);

Subjective and not at all worth a re-roll.

Show 11 quoted lines

> @@ -1227,6 +1269,34 @@ static int parse_and_validate_options(int argc, const char *argv[],
> +       if (fixup_message) {
> +               /*
> +                * To check if fixup_message that contains ':' is a commit
> +                * reference for example: --fixup="HEAD^{/^area: string}" or
> +                * a suboption of `--fixup`.
> +                *
> +                * As `amend` suboption contains only alpha character.
> +                * So check if first non alpha character in fixup_message
> +                * is ':'.
> +                */

I have a tough time figuring out what this comment is trying to say, and I don't think I would have understood it if Junio had not already explained earlier in this thread why this code is as complex as it is (rather than using, say, skip_prefix()). Perhaps the entire comment can be replaced with this:

    Extract <option> (i.e. `amend`) from `--fixup=<option>:<commit>`,
    if present. To avoid being fooled by a legitimate ":" in <commit>
    (i.e. `--fixup="HEAD^{/^area: string}"`), <option> must be
    composed of only alphabetic characters.

Not necessarily worth a re-roll.

> +               size_t len = get_alpha_len(fixup_message);
> +               if (len && fixup_message[len] == ':') {
> +                       fixup_message[len++] = '\0';
> +                       fixup_commit = fixup_message + len;

An alternate -- just about as compact and perhaps more idiomatic -- way to write all this without introducing the new get_alpha_len() function:

    char *p = fixup_mesage;
    while (isalpha(*p))
        p++;
    if (p > fixup_message && *p == ':') {
        *p = '\0';
        fixup_commit = p + 1;

Subjective and not at all worth a re-roll.

Show 12 quoted lines

> +                       if (!strcmp("amend", fixup_message)) {
> +                               fixup_prefix = "amend";
> +                               allow_empty = 1;
> +                       } else {
> +                               die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
> +                       }
> +               } else {
> +                       fixup_commit = fixup_message;
> +                       fixup_prefix = "fixup";
> +                       use_editor = 0;
> +               }
> +       }