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

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Mar 10, 2021 at 2:44 PM 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.
>
> 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;

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

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

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

> +                       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;
> +               }
> +       }