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

[Charvi Mendiratta <charvi077@gmail.com>]

On Tue, 2 Mar 2021 at 12:09, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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.
>

Agree, thanks for pointing this out in detail. I will rewrite the doc
in the above suggested way and update in the next version.

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

Not necessary, but I thought that users must know that `-m` is
otherwise supported with plain `--fixup` and not with the `amend` and
`reword` suboptions. So, I think to reword it and add with the above.

> > 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!`?

No, as both `amend` and `reword` suboptions create "amend!" commit
only. I think it seems a bit confusing but I will try another attempt
to reword the document.

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

Okay, I will change it and explain it in more detail.

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

Agree, I will fix it.


Thanks for all the detailed reviews and suggestions. I will update all
the changes in the next revision.

Thanks and Regards,
Charvi