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

[Eric Sunshine <sunshine@sunshineco.com>]

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

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

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

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

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