Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 2, 2021, 06:39 UTC
Message-ID: <CAPig+cRvwvT7QrO0-aLZX-2vsBPJSq6WO-O7g5A0OjDMNAYmCQ@mail.gmail.com>
In-Reply-To: <20210301084512.27170-7-charvi077@gmail.com>

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

Show 8 quoted lines

> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> @@ -9,7 +9,7 @@ SYNOPSIS
> +          [--dry-run] [(-c | -C | --squash) <commit> | --fixup [(amend|reword):]<commit>)]
> @@ -86,11 +86,39 @@ OPTIONS
> ---fixup=<commit>::
> +--fixup=[(amend|reword):]<commit>::

Although technically correct, I can't help but wonder if we can be more friendly to readers by rephrasing this as:

    --fixup=<commit>::
    --fixup=amend:<commit>::
    --fixup=reword:<commit>::

which is probably a lot easier to take in and understand at a glance. Same comment applies to the synopsis.

Not necessarily worth a re-roll.

Show 5 quoted lines

> +       Without `amend:` or `reword:`, create a `fixup!` commit where
> +       the commit message will be the subject line from the specified
> +       commit with a prefix of "fixup!'". The resulting "fixup!" commit
> +       is further used with `git rebase --autosquash` to fixup the
> +       content of the specified commit.

I think it becomes important at this point to make it more clear that _only_ the content of <commit> gets changed by the "fixup!" commit, and that the log message of <commit> is untouched.

Show 12 quoted lines

> +The `--fixup=amend:<commit>` form creates an "amend!" commit to
> +fixup both the content and the commit log message of the specified
> +commit. The resulting "amend!" commit's commit message subject
> +will be the subject line from the specified commit with a prefix of
> +"amend!'" and the message body will be commit log message of the
> +specified commit. It also invokes an editor seeded with the log
> +message of the "amend!" commit to allow to edit further. And it
> +refuses to create "amend!" commit if it's commit message body is
> +empty unless used with the `--allow-empty-message` option. "amend!"
> +commit when rebased with `--autosquash` will fixup the contents and
> +replace the commit message of the specified commit with the "amend!"
> +commit's message body.

I had to read this several times to understand what it is trying to say. I believe that part of the problem is that the bulk of the description goes into great detail describing bits and behaviors which make no sense without understanding what an "amend!" commit actually does, which isn't explained until the very last sentence. So, I think the entire description needs to be flipped on its head. In particular, it should start by saying "create a new commit which both fixes up the content of <commit> and replaces <commit>'s log message", and only then dive into the details.

In fact, what I just wrote suggests a larger problem with the description of `--fixup` overall. There is no high-level explanation of what a "fixup" (or "amend" or "reword") is; it just dives right into the minutiae without providing the reader with sufficient context to understand any of it. Only a reader who is already familiar with interactive rebase is likely to grok what is being said here. So, extending the thought I expressed above, it would be helpful for the description of `--fixup=[amend:|reword:]` to start by first explaining what a "fixup" is, followed by simple descriptions of "amend" and "reword" (building upon "fixup"), and followed finally by details of each. Very roughly, something like this:

    Creates a new commit which "fixes up" <commit> when applied with
    `git rebase --autosquash`.

    A "fixup" commit changes the content of <commit> but leaves its
    log message untouched.

    An "amend" commit is like "fixup" but also replaces the log
    message of <commit> with the log message of the "amend" commit.

    A "reword" commit replaces the log message of <commit> with its
    own log message but makes no changes to the content.

And then dive into the details of each variation.

> +The `--fixup=amend:` and `--fixup=reword:` forms cannot be used with
> +other options to add to the commit log message i.e it is incompatible
> +with `-m`/`-F`/`-c`/`-C` options.

I suppose it doesn't hurt, but I wonder if it's really necessary to document this considering that the user will learn soon enough upon trying invalid combinations.

> +Also, after fixing the commit using `--fixup`, with or without option
> +and rebased with `--autosquash`, the authorship of the original commit
> +remains unchanged. See linkgit:git-rebase[1] for details.

Good.

Show 6 quoted lines

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> @@ -593,16 +593,17 @@ See also INCOMPATIBLE OPTIONS below.
>  --autosquash::
>  --no-autosquash::
> +       When the commit log message begins with "squash! ..." (or "fixup! ..."
> +       or "amend! ..."), and there is already a commit in the todo list that

Should this also be mentioning `reword!`?

> +       matches the same `...`, automatically modify the todo list of
> +       `rebase -i`, so that the commit marked for squashing comes right after
> +       the commit to be modified, and change the action of the moved commit
> +       from `pick` to `squash` (or `fixup` or `fixup -C`) respectively. A commit

It's becoming difficult to know which of the "foo!" prefixes get transformed into which sequencer command since there is no longer a one-to-one correspondence between "foo!" prefixes and sequencer commands as there was when only "squash!" and "fixup!" existed. The reader should be told what sequencer command(s) "amend!" and "reword!" become.

Show 5 quoted lines

> +       matches the `...` if the commit subject matches, or if the `...` refers
> +       to the commit's hash. As a fall-back, partial matches of the commit
> +       subject work, too. The recommended way to create fixup/squash/amend
> +       commits is by using the `--fixup=[amend|reword]`/`--squash` options of
> +       linkgit:git-commit[1].

At this point, it may be beneficial to write these out long-form to make it easier on the reader; something along the lines of:

    ... the `--fixup`, `--fixup:amend:`, `--fixup:reword:`, and
    `--squash` options of ...