[3/3] commit: add an option the reword HEAD

Commit Message

Jean-Noël Avila via GitGitGadget Sept. 21, 2020, 1:30 p.m. UTC

From: Phillip Wood <[e-mail]>

If one notices a typo in the last commit after starting to stage
changes for the next commit it is useful to be able to reword the last
commit without changing its contents. Currently the way to do that is
by specifying --amend --only with no pathspec which is not that
obvious to new users (so much so that before beb635ca9c ("commit:
remove 'Clever' message for --only --amend", 2016-12-09) commit
printed a message to congratulate the user on figuring out how to do
it). If the last commit is empty one has to pass --allow-empty as well
even though the contents are not being changed. This commits adds a
--reword option for commit that rewords the last commit without
changing its contents.

Signed-off-by: Phillip Wood <[e-mail]>
---
 Documentation/git-commit.txt          | 14 ++++++-
 builtin/commit.c                      | 46 +++++++++++++++++++++-
 t/t7501-commit-basic-functionality.sh | 56 +++++++++++++++++++++++++++
 3 files changed, 113 insertions(+), 3 deletions(-)

Comments

Eric Sunshine Sept. 21, 2020, 3:43 p.m. UTC | #1

On Mon, Sep 21, 2020 at 9:31 AM Phillip Wood via GitGitGadget
<[e-mail]> wrote:
> +--reword::
> +       Reword the commit message of the tip of the current branch by
> +       replacing it with a new commit. The commit contents will be
> +       unchanged even if there are staged changes. This is equivalent
> +       to specifying `--amend --only --allow-empty` with no paths.
> diff --git a/builtin/commit.c b/builtin/commit.c
> @@ -1152,6 +1153,41 @@ static void finalize_deferred_config(struct wt_status *s)
> +static void validate_reword_options(int argc, struct commit *current_head)
> +{
> +       if (amend)
> +               die(_("cannot combine --reword with --amend"));
> +       if (only)
> +               die(_("cannot combine --reword with --only"));

Nit: It feels a bit odd (though not outright wrong) to disallow
--reword in combination with --amend and --only after the
documentation states that --reword is equivalent to using those
options.

> diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
> @@ -713,4 +713,60 @@ test_expect_success '--dry-run --short' '
> +test_reword_opt () {
> +       test_expect_success C_LOCALE_OUTPUT "--reword incompatible with $1" "
> +               echo 'fatal: cannot combine --reword with $1' >expect &&
> +               test_must_fail git commit --reword $1 2>actual &&
> +               test_cmp expect actual
> +       "
> +}

These error messages are subject to localization, so you'd want to use
test_i18ncmp() here, I think.

Same comment for other new tests.

Christian Couder Sept. 21, 2020, 5:04 p.m. UTC | #2

On Mon, Sep 21, 2020 at 3:33 PM Phillip Wood via GitGitGadget
<[e-mail]> wrote:

> If one notices a typo in the last commit ...

In the title "commit: add an option the reword HEAD", I would suggest
s/the/to/ :-)

Phillip Wood Sept. 21, 2020, 6:01 p.m. UTC | #3

On 21/09/2020 18:04, Christian Couder wrote:
> On Mon, Sep 21, 2020 at 3:33 PM Phillip Wood via GitGitGadget
> <[e-mail]> wrote:
> 
>> If one notices a typo in the last commit ...
> 
> In the title "commit: add an option the reword HEAD", I would suggest
> s/the/to/ :-)

Well spotted Christian, Thanks

Phillip

Phillip Wood Sept. 21, 2020, 6:05 p.m. UTC | #4

Hi Eric

On 21/09/2020 16:43, Eric Sunshine wrote:
> On Mon, Sep 21, 2020 at 9:31 AM Phillip Wood via GitGitGadget
> <[e-mail]> wrote:
>> +--reword::
>> +       Reword the commit message of the tip of the current branch by
>> +       replacing it with a new commit. The commit contents will be
>> +       unchanged even if there are staged changes. This is equivalent
>> +       to specifying `--amend --only --allow-empty` with no paths.
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> @@ -1152,6 +1153,41 @@ static void finalize_deferred_config(struct wt_status *s)
>> +static void validate_reword_options(int argc, struct commit *current_head)
>> +{
>> +       if (amend)
>> +               die(_("cannot combine --reword with --amend"));
>> +       if (only)
>> +               die(_("cannot combine --reword with --only"));
> 
> Nit: It feels a bit odd (though not outright wrong) to disallow
> --reword in combination with --amend and --only after the
> documentation states that --reword is equivalent to using those
> options.

Yeah I decided to be quite strict, I'm in two minds about the 
documentation, I think it might be better to remove that line.

>> diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
>> @@ -713,4 +713,60 @@ test_expect_success '--dry-run --short' '
>> +test_reword_opt () {
>> +       test_expect_success C_LOCALE_OUTPUT "--reword incompatible with $1" "
>> +               echo 'fatal: cannot combine --reword with $1' >expect &&
>> +               test_must_fail git commit --reword $1 2>actual &&
>> +               test_cmp expect actual
>> +       "
>> +}
> 
> These error messages are subject to localization, so you'd want to use
> test_i18ncmp() here, I think.
> 
> Same comment for other new tests.

I decided to use the C_LOCALE_OUTPUT prerequisite and test_cmp rather 
than grep so I could check the exact output. I'm slightly suspicious of 
tests that just grep for an error message when that is all we should be 
showing. I should probably check that nothing is printed to stdout in 
these tests

Best Wishes

Phillip

Eric Sunshine Sept. 21, 2020, 6:12 p.m. UTC | #5

On Mon, Sep 21, 2020 at 2:05 PM Phillip Wood <[e-mail]> wrote:
> On 21/09/2020 16:43, Eric Sunshine wrote:
> > Nit: It feels a bit odd (though not outright wrong) to disallow
> > --reword in combination with --amend and --only after the
> > documentation states that --reword is equivalent to using those
> > options.
>
> Yeah I decided to be quite strict, I'm in two minds about the
> documentation, I think it might be better to remove that line.

I rather like that the documentation explains the equivalency between
--reword and `--amend --only --allow-empty` since it makes --reword
less magical and provides the readers with pointers for further study.

> I decided to use the C_LOCALE_OUTPUT prerequisite and test_cmp rather
> than grep so I could check the exact output. [...]

I overlooked C_LOCALE_OUTPUT.

Junio C Hamano Sept. 21, 2020, 7:27 p.m. UTC | #6

Phillip Wood <[e-mail]> writes:

>>> diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
>>> @@ -713,4 +713,60 @@ test_expect_success '--dry-run --short' '
>>> +test_reword_opt () {
>>> +       test_expect_success C_LOCALE_OUTPUT "--reword incompatible with $1" "
>>> +               echo 'fatal: cannot combine --reword with $1' >expect &&
>>> +               test_must_fail git commit --reword $1 2>actual &&
>>> +               test_cmp expect actual
>>> +       "
>>> +}
>> These error messages are subject to localization, so you'd want to
>> use
>> test_i18ncmp() here, I think.
>> Same comment for other new tests.
>
> I decided to use the C_LOCALE_OUTPUT prerequisite and test_cmp rather
> than grep so I could check the exact output.

I do not think it is a good idea.  Dropping the C_LOCALE_OUTPUT
prerequisite and using test_i18ncmpw would be more appropriate.

A test run without GIT_TEST_GETTEXT_POISON will do the byte-for-byte
comparison like test_cmp.  It is only the poison test, whose purpose
is to catch commands that by mistake translated their messages, that
would want to mark a test that checks end-user facing messages like
this one as special with test_i18ncmp.

> ... I should probably check that nothing is printed to stdout in
> these tests

Perhaps, but that is not the point of "do we diagnose options thare
are incompatble with --reword?" test.

Phillip Wood Sept. 22, 2020, 1:38 p.m. UTC | #7

Hi Junio

On 21/09/2020 20:27, Junio C Hamano wrote:
> Phillip Wood <[e-mail]> writes:
> 
>>>> diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
>>>> @@ -713,4 +713,60 @@ test_expect_success '--dry-run --short' '
>>>> +test_reword_opt () {
>>>> +       test_expect_success C_LOCALE_OUTPUT "--reword incompatible with $1" "
>>>> +               echo 'fatal: cannot combine --reword with $1' >expect &&
>>>> +               test_must_fail git commit --reword $1 2>actual &&
>>>> +               test_cmp expect actual
>>>> +       "
>>>> +}
>>> These error messages are subject to localization, so you'd want to
>>> use
>>> test_i18ncmp() here, I think.
>>> Same comment for other new tests.
>>
>> I decided to use the C_LOCALE_OUTPUT prerequisite and test_cmp rather
>> than grep so I could check the exact output.
> 
> I do not think it is a good idea.  Dropping the C_LOCALE_OUTPUT
> prerequisite and using test_i18ncmpw would be more appropriate.
> 
> A test run without GIT_TEST_GETTEXT_POISON will do the byte-for-byte
> comparison like test_cmp.  It is only the poison test, whose purpose
> is to catch commands that by mistake translated their messages, that
> would want to mark a test that checks end-user facing messages like
> this one as special with test_i18ncmp.

Thanks I wasn't aware of test_i18ncmp

>> ... I should probably check that nothing is printed to stdout in
>> these tests
> 
> Perhaps, but that is not the point of "do we diagnose options thare
> are incompatble with --reword?" test.

I think it depends if one views the test as checking "do we diagnose 
options there are incompatible with --reword?" or "what do we show the 
user when there are options that are incompatible with --reword". For 
the former we just want to check that the correct error message is 
printed, for the latter we want to check that only what we expect to be 
printed is actually printed.

Best Wishes

Phillip

Junio C Hamano Sept. 22, 2020, 4:54 p.m. UTC | #8

Phillip Wood <[e-mail]> writes:

>>>>> +test_reword_opt () {
>>>>> ...
>>> ... I should probably check that nothing is printed to stdout in
>>> these tests
>> Perhaps, but that is not the point of "do we diagnose options thare
>> are incompatble with --reword?" test.
>
> I think it depends if one views the test as checking "do we diagnose
> options there are incompatible with --reword?" or "what do we show the 
> user when there are options that are incompatible with --reword". For
> the former we just want to check that the correct error message is 
> printed, for the latter we want to check that only what we expect to
> be printed is actually printed.

I dunno.  It is plausible that we will further give some advices
(not directly related to --reword option being incompatible with
other options) and output may evolve.  I do not think we want to be
updating each and every test that expects exact output.

In other words, the answer to "what do we show when --reword and
another incompatible option are given?" is "we want to point it out
that --reword and --amend are not to be used together" and not "and
we should not ever say anything else".

Just like back when we started making sure everybody sets user.name
configuration variable (and the way to do so was to give warnings
and advices at strategic places), output can be added to commands
where authors if each individual feature would not expect.  I would
want to see our tests prepared for such an occasion.  We cannot be
perfectly prepared, of course, but keeping the expectation focused
enough to check what really matters for the objective at hand would
help.

Thanks.

Johannes Schindelin Sept. 23, 2020, 10:22 a.m. UTC | #9

Hi Phillip,

On Mon, 21 Sep 2020, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <[e-mail]>
>
> If one notices a typo in the last commit after starting to stage
> changes for the next commit it is useful to be able to reword the last
> commit without changing its contents. Currently the way to do that is
> by specifying --amend --only with no pathspec which is not that
> obvious to new users (so much so that before beb635ca9c ("commit:
> remove 'Clever' message for --only --amend", 2016-12-09) commit
> printed a message to congratulate the user on figuring out how to do
> it). If the last commit is empty one has to pass --allow-empty as well
> even though the contents are not being changed. This commits adds a
> --reword option for commit that rewords the last commit without
> changing its contents.

I would like to explain the idea I tried to get across when I proposed to
implement support for `reword!` (and `--reword`) because I feel that it
will change the design of this patch in a rather big way.

First of all, let me explain the scenario in which I long for the
`--reword` option: I maintain several patch thickets, the most obvious one
being Git for Windows' patch thicket that is merge-rebased [*1*] onto
every new Git version.

At times, I need to adjust a commit message in that patch thicket. It
would be quite wasteful to perform a full merge-rebase, therefore I
typically call `git commit --squash <commit> -c <commit>`, copy the
oneline, paste it after the `squash!` line (surrounded by empty lines), and
then reword the commit message. When the next Git version comes out, I do
a merging-rebase, and when the editor pops up because of that `squash!`
oneline, I remove the now-obsolete version(s) of the commit message.

Obviously, I have to be careful to either also pass `--only` (which I
somehow managed to learn about only today) or I have to make sure that I
have no staged changes. In practice, I actually specify a bogus path,
which has the same effect as `--only`.

What I would actually rather have is the `--reword` option: `git commit
--reword <commit>`. In my mind, this would _add_ a new, "empty" commit,
letting me edit the commit message of the specified commit, and using that
as commit message, prefixed with the line `reword! <oneline>`.

This, in turn, would need to be accompanied by support in the interactive
rebase, to perform the desired reword (which is admittedly quite a bit
different from what the way the todo command `reword` works).

With that in mind, I would like to caution against the design of your
current patch, because it would slam the door shut on the way I would like
`--reword` to work.

Ciao,
Dscho

Footnote *1*: In Git for Windows, I want to not only rebase the patches
(so that they are as ready to be submitted to the Git mailing list as they
can be) but I also want the commit history to fast-forward. The strategy I
settled on is the "merging rebase": it is a rebase that starts with a fake
merge of the previous commit history, i.e. merging it in using `-s ours`
so that only the commit history comes in, but not the changes. This allows
contributors to pull without problems, but also provides the benefits of
having a rebased version of the patches. The price is a rather big commit
history on top of Git's main branch, as Git for Windows' main branch
contains not only the newest iteration of its patches, but _all_
iterations (at least since the first merging-rebase).

Phillip Wood Sept. 23, 2020, 6:23 p.m. UTC | #10

Hi Dscho

On 23/09/2020 11:22, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Mon, 21 Sep 2020, Phillip Wood via GitGitGadget wrote:
> 
>> From: Phillip Wood <[e-mail]>
>>
>> If one notices a typo in the last commit after starting to stage
>> changes for the next commit it is useful to be able to reword the last
>> commit without changing its contents. Currently the way to do that is
>> by specifying --amend --only with no pathspec which is not that
>> obvious to new users (so much so that before beb635ca9c ("commit:
>> remove 'Clever' message for --only --amend", 2016-12-09) commit
>> printed a message to congratulate the user on figuring out how to do
>> it). If the last commit is empty one has to pass --allow-empty as well
>> even though the contents are not being changed. This commits adds a
>> --reword option for commit that rewords the last commit without
>> changing its contents.
> 
> I would like to explain the idea I tried to get across when I proposed to
> implement support for `reword!` (and `--reword`) because I feel that it
> will change the design of this patch in a rather big way.
> 
> First of all, let me explain the scenario in which I long for the
> `--reword` option: I maintain several patch thickets, the most obvious one
> being Git for Windows' patch thicket that is merge-rebased [*1*] onto
> every new Git version.
> 
> At times, I need to adjust a commit message in that patch thicket. It
> would be quite wasteful to perform a full merge-rebase, therefore I
> typically call `git commit --squash <commit> -c <commit>`, copy the
> oneline, paste it after the `squash!` line (surrounded by empty lines), and
> then reword the commit message. When the next Git version comes out, I do
> a merging-rebase, and when the editor pops up because of that `squash!`
> oneline, I remove the now-obsolete version(s) of the commit message.
> 
> Obviously, I have to be careful to either also pass `--only` (which I
> somehow managed to learn about only today) or I have to make sure that I
> have no staged changes. In practice, I actually specify a bogus path,
> which has the same effect as `--only`.
> 
> What I would actually rather have is the `--reword` option: `git commit
> --reword <commit>`. In my mind, this would _add_ a new, "empty" commit,
> letting me edit the commit message of the specified commit, and using that
> as commit message, prefixed with the line `reword! <oneline>`.
> 
> This, in turn, would need to be accompanied by support in the interactive
> rebase, to perform the desired reword (which is admittedly quite a bit
> different from what the way the todo command `reword` works).
> 
> With that in mind, I would like to caution against the design of your
> current patch, because it would slam the door shut on the way I would like
> `--reword` to work.

I'm keen to have an easy way to reword HEAD and a way to implement your 
reword! idea.

I posted a comment on your gitgitgadget issue about reword! and drop![1] 
pointing to some patches[2] that implement the reword! idea as amend!. I 
think we want to be able to  fixup a commit and reword it at the same 
time which is way I chose the name amend! rather than reword! The 
implementation currently changes `git commit --amend` to take an 
optional commit which isn't ideal. I wonder if calling it revise! would 
be better then we could have `git commit --reword` to reword HEAD and 
`git commit --revise <commit>` to create a commit that will reword and 
fixup <commit> when the user runs `git rebase -i --autostash`. fold! is 
another possibility.

I don't think this patch series stops us implementing something for 
rebase but it would mean we couldn't use the name reword! unless we 
allow `git commit --reword` to take an optional commit which I'm not 
that keen on.

What do you think to an alternative name?

Best Wishes

Phillip

[1] https://github.com/gitgitgadget/git/issues/259
[2] https://github.com/phillipwood/git/commits/wip/rebase-amend

> Ciao,
> Dscho
> 
> Footnote *1*: In Git for Windows, I want to not only rebase the patches
> (so that they are as ready to be submitted to the Git mailing list as they
> can be) but I also want the commit history to fast-forward. The strategy I
> settled on is the "merging rebase": it is a rebase that starts with a fake
> merge of the previous commit history, i.e. merging it in using `-s ours`
> so that only the commit history comes in, but not the changes. This allows
> contributors to pull without problems, but also provides the benefits of
> having a rebased version of the patches. The price is a rather big commit
> history on top of Git's main branch, as Git for Windows' main branch
> contains not only the newest iteration of its patches, but _all_
> iterations (at least since the first merging-rebase).
>

Johannes Schindelin Sept. 23, 2020, 8:42 p.m. UTC | #11

Hi Phillip,

On Wed, 23 Sep 2020, Phillip Wood wrote:

> On 23/09/2020 11:22, Johannes Schindelin wrote:
> >
> > On Mon, 21 Sep 2020, Phillip Wood via GitGitGadget wrote:
> >
> > > From: Phillip Wood <[e-mail]>
> > >
> > > If one notices a typo in the last commit after starting to stage
> > > changes for the next commit it is useful to be able to reword the last
> > > commit without changing its contents. Currently the way to do that is
> > > by specifying --amend --only with no pathspec which is not that
> > > obvious to new users (so much so that before beb635ca9c ("commit:
> > > remove 'Clever' message for --only --amend", 2016-12-09) commit
> > > printed a message to congratulate the user on figuring out how to do
> > > it). If the last commit is empty one has to pass --allow-empty as well
> > > even though the contents are not being changed. This commits adds a
> > > --reword option for commit that rewords the last commit without
> > > changing its contents.
> >
> > I would like to explain the idea I tried to get across when I proposed to
> > implement support for `reword!` (and `--reword`) because I feel that it
> > will change the design of this patch in a rather big way.
> >
> > First of all, let me explain the scenario in which I long for the
> > `--reword` option: I maintain several patch thickets, the most obvious one
> > being Git for Windows' patch thicket that is merge-rebased [*1*] onto
> > every new Git version.
> >
> > At times, I need to adjust a commit message in that patch thicket. It
> > would be quite wasteful to perform a full merge-rebase, therefore I
> > typically call `git commit --squash <commit> -c <commit>`, copy the
> > oneline, paste it after the `squash!` line (surrounded by empty lines), and
> > then reword the commit message. When the next Git version comes out, I do
> > a merging-rebase, and when the editor pops up because of that `squash!`
> > oneline, I remove the now-obsolete version(s) of the commit message.
> >
> > Obviously, I have to be careful to either also pass `--only` (which I
> > somehow managed to learn about only today) or I have to make sure that I
> > have no staged changes. In practice, I actually specify a bogus path,
> > which has the same effect as `--only`.
> >
> > What I would actually rather have is the `--reword` option: `git commit
> > --reword <commit>`. In my mind, this would _add_ a new, "empty" commit,
> > letting me edit the commit message of the specified commit, and using that
> > as commit message, prefixed with the line `reword! <oneline>`.
> >
> > This, in turn, would need to be accompanied by support in the interactive
> > rebase, to perform the desired reword (which is admittedly quite a bit
> > different from what the way the todo command `reword` works).
> >
> > With that in mind, I would like to caution against the design of your
> > current patch, because it would slam the door shut on the way I would like
> > `--reword` to work.
>
> I'm keen to have an easy way to reword HEAD and a way to implement your
> reword! idea.
>
> I posted a comment on your gitgitgadget issue about reword! and drop![1]
> pointing to some patches[2] that implement the reword! idea as amend!. I think
> we want to be able to  fixup a commit and reword it at the same time which is
> way I chose the name amend! rather than reword! The implementation currently
> changes `git commit --amend` to take an optional commit which isn't ideal. I
> wonder if calling it revise! would be better then we could have `git commit
> --reword` to reword HEAD and `git commit --revise <commit>` to create a commit
> that will reword and fixup <commit> when the user runs `git rebase -i
> --autostash`. fold! is another possibility.
>
> I don't think this patch series stops us implementing something for rebase but
> it would mean we couldn't use the name reword! unless we allow `git commit
> --reword` to take an optional commit which I'm not that keen on.
>
> What do you think to an alternative name?

I am really worried that the proliferation of confusingly similar options
will increase Git's reputation for being awfully hard to use.

Ciao,
Dscho

Phillip Wood Sept. 24, 2020, 9:58 a.m. UTC | #12

Hi Dscho

On 23/09/2020 21:42, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Wed, 23 Sep 2020, Phillip Wood wrote:
> 
>> On 23/09/2020 11:22, Johannes Schindelin wrote:
>>>
>>> On Mon, 21 Sep 2020, Phillip Wood via GitGitGadget wrote:
>>>
>>>> From: Phillip Wood <[e-mail]>
>>>>
>>>> If one notices a typo in the last commit after starting to stage
>>>> changes for the next commit it is useful to be able to reword the last
>>>> commit without changing its contents. Currently the way to do that is
>>>> by specifying --amend --only with no pathspec which is not that
>>>> obvious to new users (so much so that before beb635ca9c ("commit:
>>>> remove 'Clever' message for --only --amend", 2016-12-09) commit
>>>> printed a message to congratulate the user on figuring out how to do
>>>> it). If the last commit is empty one has to pass --allow-empty as well
>>>> even though the contents are not being changed. This commits adds a
>>>> --reword option for commit that rewords the last commit without
>>>> changing its contents.
>>>
>>> I would like to explain the idea I tried to get across when I proposed to
>>> implement support for `reword!` (and `--reword`) because I feel that it
>>> will change the design of this patch in a rather big way.
>>>
>>> First of all, let me explain the scenario in which I long for the
>>> `--reword` option: I maintain several patch thickets, the most obvious one
>>> being Git for Windows' patch thicket that is merge-rebased [*1*] onto
>>> every new Git version.
>>>
>>> At times, I need to adjust a commit message in that patch thicket. It
>>> would be quite wasteful to perform a full merge-rebase, therefore I
>>> typically call `git commit --squash <commit> -c <commit>`, copy the
>>> oneline, paste it after the `squash!` line (surrounded by empty lines), and
>>> then reword the commit message. When the next Git version comes out, I do
>>> a merging-rebase, and when the editor pops up because of that `squash!`
>>> oneline, I remove the now-obsolete version(s) of the commit message.
>>>
>>> Obviously, I have to be careful to either also pass `--only` (which I
>>> somehow managed to learn about only today) or I have to make sure that I
>>> have no staged changes. In practice, I actually specify a bogus path,
>>> which has the same effect as `--only`.
>>>
>>> What I would actually rather have is the `--reword` option: `git commit
>>> --reword <commit>`. In my mind, this would _add_ a new, "empty" commit,
>>> letting me edit the commit message of the specified commit, and using that
>>> as commit message, prefixed with the line `reword! <oneline>`.
>>>
>>> This, in turn, would need to be accompanied by support in the interactive
>>> rebase, to perform the desired reword (which is admittedly quite a bit
>>> different from what the way the todo command `reword` works).
>>>
>>> With that in mind, I would like to caution against the design of your
>>> current patch, because it would slam the door shut on the way I would like
>>> `--reword` to work.
>>
>> I'm keen to have an easy way to reword HEAD and a way to implement your
>> reword! idea.
>>
>> I posted a comment on your gitgitgadget issue about reword! and drop![1]
>> pointing to some patches[2] that implement the reword! idea as amend!. I think
>> we want to be able to  fixup a commit and reword it at the same time which is
>> way I chose the name amend! rather than reword! The implementation currently
>> changes `git commit --amend` to take an optional commit which isn't ideal. I
>> wonder if calling it revise! would be better then we could have `git commit
>> --reword` to reword HEAD and `git commit --revise <commit>` to create a commit
>> that will reword and fixup <commit> when the user runs `git rebase -i
>> --autostash`. fold! is another possibility.
>>
>> I don't think this patch series stops us implementing something for rebase but
>> it would mean we couldn't use the name reword! unless we allow `git commit
>> --reword` to take an optional commit which I'm not that keen on.
>>
>> What do you think to an alternative name?
> 
> I am really worried that the proliferation of confusingly similar options
> will increase Git's reputation for being awfully hard to use.

That is certainly a consideration, but not having a way to easily reword 
the last commit without changing its contents does not to improve Git's 
user friendliness. If you only just discovered using --only for 
rewording it's a fair bet a lot of regular users are unaware of it.

The reason I'm not keen on having --amend or --reword take an optional 
commit is that I think it is confusing as it means sometimes that option 
creates a new commit and sometimes it modifies the last commit 
furthermore passing --reword=HEAD would not reword HEAD but creates a 
reword! commit.

Rewording the last commit and creating a reword! commit are two 
different operations so I'm not sure having different options for them 
is that bad. To me the real confusion is that we end up with 3 options 
that create different flavors of fixup commits. It would be much nicer 
if there was a single fixup type that reworded the message as well as 
fixing up the contents and users just passed `--no-edit` to avoid 
changing the message. I'd really like to somehow change the semantics of 
`git commit --fixup/--squash` and the rebase `fixup`/`squash` commands 
to actually reword the commit. I guess that would mean an opt-in config 
setting which isn't ideal.

As an aside I'd like to see a new `rewrite` command that wraps the 
functionality of `rebase -i` so the user does not have to deal with 
fixups and editing a todo list. `git rewrite amend <commit>` would be 
equivalent to starting a rebase and marking <commit> as `edit`, the user 
can then make their changes and run `git rewrite continue` which would 
finish the rebase or `git rewrite amend <another-commit>` which would 
either alter the todo list to mark <another-commit> as `edit` or create 
new entries to rewind the rebase if <another-commit> is not in the todo 
list and then run `git rebase --continue`. Additionally there would be 
`reword` and `drop` commands and support for blame so that the user can 
do all this from an editor which would run `git rewrite amend -L 
<current-line> <current-file>` to amend the commit that introduced the 
current line. I've got a nasty scheme prototype of this and I've found 
it really useful. For an individual developer developing a patch series 
it is much more convenient to just edit the history directly rather than 
creating fixup! commits (which often seem to have conflicts when they 
are applied). It would not address your use where you don't want to be 
rebasing all the time though.

Best Wishes

Phillip

> Ciao,
> Dscho
>

Junio C Hamano Sept. 24, 2020, 4:58 p.m. UTC | #13

Phillip Wood <[e-mail]> writes:

>>> I don't think this patch series stops us implementing something for rebase but
>>> it would mean we couldn't use the name reword! unless we allow `git commit
>>> --reword` to take an optional commit which I'm not that keen on.
>>>
>>> What do you think to an alternative name?
>> I am really worried that the proliferation of confusingly similar
>> options
>> will increase Git's reputation for being awfully hard to use.
> ...
> The reason I'm not keen on having --amend or --reword take an optional
> commit is that I think it is confusing as it means sometimes that
> option creates a new commit and sometimes it modifies the last commit 
> furthermore passing --reword=HEAD would not reword HEAD but creates a
> reword! commit.

Adding just another subjective view to the two already presented,
but I think --reword, as presented by Phillip, sits better next to
the existing --amend.

I wonder if we can extend the existing "--fixup <commit>" (and
perhaps "--squash <commit>") to make them work better with the
workflow Dscho envisions?  Explicit presence of the "-e" option
might be a way to tell the command to behave differently from the
current "--fixup" and to leave a mark that is different from
'fixup!" in the resulting commit to affect the later "rebase" step
as well, for example.

Patch

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 9de4dc5d66..8ec87ecb6b 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -8,7 +8,7 @@  git-commit - Record changes to the repository
 SYNOPSIS
 --------
 [verse]
-'git commit' [-a | --interactive | --patch] [--amend]
+'git commit' [-a | --interactive | --patch] [--amend | --reword]
 	   [(-c | -C | --fixup | --squash) <commit>] [-F <file> | -m <msg>]
 	   [--allow-empty] [--allow-empty-message] [--no-verify] [-e]
 	   [--reset-author] [--author=<author>] [--date=<date>]
@@ -99,7 +99,7 @@  OPTIONS
 	linkgit:git-rebase[1] for details.
 
 --reset-author::
-	When used with `-C`/`-c`/`--amend` options, or when committing
+	When used with `-C`/`-c`/`--amend`/`--reword` options, or when committing
 	after a conflicting cherry-pick, declare that the authorship of
 	the resulting commit now belongs to the committer. This also
 	renews the author timestamp.
@@ -229,6 +229,16 @@  variable (see linkgit:git-config[1]).
 	For example, `git commit --amend --no-edit` amends a commit
 	without changing its commit message.
 
+--reword::
+	Reword the commit message of the tip of the current branch by
+	replacing it with a new commit. The commit contents will be
+	unchanged even if there are staged changes. This is equivalent
+	to specifying `--amend --only --allow-empty` with no paths.
++
+You should understand the implications of rewriting history if you
+reword a commit that has already been published.  (See the "RECOVERING
+FROM UPSTREAM REBASE" section in linkgit:git-rebase[1].)
+
 --amend::
 	Replace the tip of the current branch by creating a new
 	commit. The recorded tree is prepared as usual (including
diff --git a/builtin/commit.c b/builtin/commit.c
index 5d91b13a5c..f7913f771a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -107,6 +107,7 @@  static const char *author_message, *author_message_buffer;
 static char *edit_message, *use_message;
 static char *fixup_message, *squash_message;
 static int all, also, interactive, patch_interactive, only, amend, signoff;
+static int reword;
 static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int config_commit_verbose = -1; /* unspecified */
@@ -1152,6 +1153,41 @@  static void finalize_deferred_config(struct wt_status *s)
 		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
 }
 
+static void validate_reword_options(int argc, struct commit *current_head)
+{
+	if (!current_head)
+		die(_("You have nothing to reword."));
+	if (whence != FROM_COMMIT) {
+		if (whence == FROM_MERGE)
+			die(_("You are in the middle of a merge -- cannot "
+			      "reword."));
+		else if (is_from_cherry_pick(whence))
+			die(_("You are in the middle of a cherry-pick -- cannot"
+			      " reword."));
+		else if (is_from_rebase(whence))
+			die(_("You are in the middle of a rebase -- cannot "
+			      "reword."));
+	}
+	if (amend)
+		die(_("cannot combine --reword with --amend"));
+	if (argc)
+		die(_("cannot combine --reword with paths"));
+	if (interactive)
+		die(_("cannot combine --reword with --interactive"));
+	if (patch_interactive)
+		die(_("cannot combine --reword with --patch"));
+	if (all)
+		die(_("cannot combine --reword with --all"));
+	if (also)
+		die(_("cannot combine --reword with --include"));
+	if (only)
+		die(_("cannot combine --reword with --only"));
+	if (!edit_flag && !force_author && !force_date && !renew_authorship &&
+	    !use_message && !edit_message && !fixup_message &&
+	    !squash_message && !logfile && !have_option_m && !signoff)
+		die(_("cannot combine --reword with --no-edit"));
+}
+
 static int parse_and_validate_options(int argc, const char *argv[],
 				      const struct option *options,
 				      const char * const usage[],
@@ -1186,6 +1222,12 @@  static int parse_and_validate_options(int argc, const char *argv[],
 		else if (whence == FROM_REBASE_PICK)
 			die(_("You are in the middle of a rebase -- cannot amend."));
 	}
+	if (reword) {
+		validate_reword_options(argc, current_head);
+		amend = 1;
+		only = 1;
+		allow_empty = 1;
+	}
 	if (fixup_message && squash_message)
 		die(_("Options --squash and --fixup cannot be used together"));
 	if (use_message)
@@ -1208,7 +1250,8 @@  static int parse_and_validate_options(int argc, const char *argv[],
 		use_message = "HEAD";
 	if (!use_message && !is_from_cherry_pick(whence) &&
 	    !is_from_rebase(whence) && renew_authorship)
-		die(_("--reset-author can be used only with -C, -c or --amend."));
+		die(_("--reset-author can be used only with -C, -c, --amend "
+		      "or --reword."));
 	if (use_message) {
 		use_message_buffer = read_commit_message(use_message);
 		if (!renew_authorship) {
@@ -1537,6 +1580,7 @@  int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('z', "null", &s.null_termination,
 			 N_("terminate entries with NUL")),
 		OPT_BOOL(0, "amend", &amend, N_("amend previous commit")),
+		OPT_BOOL(0, "reword", &reword, N_("reword the previous commit")),
 		OPT_BOOL(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")),
 		{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, N_("mode"), N_("show untracked files, optional modes: all, normal, no. (Default: all)"), PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
 		OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 110b4bf459..1ea65b426a 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -713,4 +713,60 @@  test_expect_success '--dry-run --short' '
 	git commit --dry-run --short
 '
 
+test_expect_success '--reword does not commit staged changes' '
+	echo changed >file &&
+	git add file &&
+	cat >expect <<-EOF &&
+	$(git log -1 --pretty=format:%B HEAD)
+
+	reworded
+	EOF
+	GIT_EDITOR="printf reworded >>" git commit --reword &&
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual &&
+	test_cmp_rev HEAD@{1}^{tree} HEAD^{tree} &&
+	test_cmp_rev HEAD@{1}^ HEAD^ &&
+	git cat-file blob :file >actual &&
+	test_cmp file actual
+'
+
+test_reword_opt () {
+	test_expect_success C_LOCALE_OUTPUT "--reword incompatible with $1" "
+		echo 'fatal: cannot combine --reword with $1' >expect &&
+		test_must_fail git commit --reword $1 2>actual &&
+		test_cmp expect actual
+	"
+}
+
+for opt in --all --amend --include --interactive --only --patch --no-edit
+do
+	test_reword_opt $opt
+done
+
+test_expect_success C_LOCALE_OUTPUT '--reword with paths' '
+	echo "fatal: cannot combine --reword with paths" >expect &&
+	test_must_fail git commit --reword file 2>actual &&
+	test_cmp expect actual
+'
+
+test_reword_no_edit () {
+	test_expect_success "--reword $@ --no-edit" '
+		git commit --reword '"$@"' --no-edit
+	'
+}
+
+for opt in -mmessage -CHEAD^ -cHEAD --reset-author \
+		"--author=\"Commit Author <[e-mail]>\"" \
+		--date=yesterday --fixup=HEAD^ --squash=HEAD^ --signoff
+do
+	test_reword_no_edit "$opt"
+done
+
+test_expect_success '--reword -F' '
+	echo reworded >msg &&
+	git commit --reword -F msg --no-edit &&
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp msg actual
+'
+
 test_done