On Tue, 2 Mar 2021 at 12:09, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > On Mon, Mar 1, 2021 at 3:52 AM Charvi Mendiratta <charvi077@xxxxxxxxx> wrote: > > Signed-off-by: Charvi Mendiratta <charvi077@xxxxxxxxx> > > --- > > 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
