patch, 6 parts[Outreachy] commit: Implementation of "amend!" commit

> `git commit --fixup=amend:<commit>` will create an "amend!" commit.
> The resulting commit message subject will be "amend! ..." where
> "..." is the subject line of <commit> and the initial message
> body will be <commit>'s message. -m can be used to override the
> message body.
>
> The "amend!" commit when rebased with --autosquash will fixup the
> contents and replace the commit message of <commit> with the
> "amend!" commit's message body.
>
> Inorder to prevent rebase from creating commits with an empty
> message we refuse to create an "amend!" commit if commit message
> body is empty.
>
> Example usage:
> $ git commit --fixup=amend:HEAD
> $ git commit --fixup=amend:HEAD -m "clever commit message"

> +	if (fixup_message) {
> +		/*
> +		 * check if ':' occurs before '^' or '@', otherwise
> +		 * fixup_message is a commit reference.
> +		 */

> The second one, even with s|HEAD|HEAD~3| is even less clear.
> Without the "-m", the resulting commit will have the subject that
> begins with !amend but the log message body is taken from the given
> commit, but with "-m", what happens?  Does a single-liner 'clever
> commit message' _replace_ the log message of the named commit,
> resulting in an !amend commit that has no message from the original?
> Or does 'clever commit message' get _appended_ the log message?
>

> > +     if (fixup_message) {
> > +             /*
> > +              * check if ':' occurs before '^' or '@', otherwise
> > +              * fixup_message is a commit reference.
> > +              */
>
> Isn't it that you only intend to parse:
>
>     --fixup
>     --fixup=amend:<any string that names a commit>
>     --fixup=<any string that names a commit>
>
> and later extend it to allow keywords other than "amend"?
>

> I can understand that you are trying to avoid getting fooled by
> things like
>
>         --fixup='HEAD^{/commit message with a colon : in it}'
>
> but why special case only ^ and @?  This feels brittle (note that I
> said "things like", exactly because I do not know if any string that
> can name a commit must have "@" or "^" appear before ":" if it is to
> have ":" in anywhere, which is what this code assumes).
>

> Instead, you can find the first colon, check for known keywords (or
> a string that consists only of alnums to accomodate for future
> enhancement), and treat any garbage that happens to have a colon
> without the "keyword" as fixup_commit.  I.e.  something along this
> line...
>
>                 const char alphas[] = "abcde...xyz";
>                 size_t kwd_len;
>
>                 kwd_len = strspn(fixup_message, alphas);
>                 if (kwd_len && fixup_message[kwd_len] == ':') {
>                         /* found keyword? */
>                         fixup_message[kwd_len] = '\0';
>                         if (!strcmp("amend", fixup_message)) {
>                                 ... do the amend:<commit> thing ...
> #if in-next-step-when-you-add-support-for-reword
>                         } else if (!strcmp("reword", fixup_message)) {
>                                 ... do the reword:<commit> thing ...
> #endif
>                         } else {
>                                 die(_("unknown --fixup=%s:<commit>",
>                                         fixup_message));
>                         }
>                 } else {
>                         /* the entire fixup_message is the commit */
>                 }
>

> Hi Junio,
>
> On Thu, 18 Feb 2021 at 01:20, Junio C Hamano <gitster@pobox.com> wrote:
> [...]
>> The second one, even with s|HEAD|HEAD~3| is even less clear.
>> Without the "-m", the resulting commit will have the subject that
>> begins with !amend but the log message body is taken from the given
>> commit, but with "-m", what happens?  Does a single-liner 'clever
>> commit message' _replace_ the log message of the named commit,
>> resulting in an !amend commit that has no message from the original?
>> Or does 'clever commit message' get _appended_ the log message?
>>
>
> Yes, here it gets _appended_ the log message.  I agree this seems a bit
> confusing.

>> ...Thanks, for pointing this out. Also, in the above method for
>> alnum I think we can initialize an array of alnum[] instead of
>> alphas[]. Or otherwise I was thinking to instead check:
>>            if (!isalnum(*c) && *c == ':')
>
> Sure a loop is fine, or alnum[] is fine, or just alpha[] is OK, I
> would think.  Do you foresee you'd need --fixup=chomp124:<commit>?
> I somehow doubt it.

>
> Junio C Hamano <gitster@pobox.com> writes:
>
> >> ...Thanks, for pointing this out. Also, in the above method for
> >> alnum I think we can initialize an array of alnum[] instead of
> >> alphas[]. Or otherwise I was thinking to instead check:
> >>            if (!isalnum(*c) && *c == ':')
> >
> > Sure a loop is fine, or alnum[] is fine, or just alpha[] is OK, I
> > would think.  Do you foresee you'd need --fixup=chomp124:<commit>?
> > I somehow doubt it.
>
> Having said that, we may regret if we did not include some
> punctuation to allow for a multi-word keyword.  IOW, "alpha plus
> dash" might be a reasonable minimum.
>
> But what keyword --fixup=<keyword>:<commit> can take is entirely
> under our control, so it is not all that unreasonable if we just
> forced our developers some discipline to pick a single-word keyword
> for any of their future enhancements.  It's not like we are opening
> up extensibility to the end-users, who may complain that the way
> they can spell their new <keyword> is too limited.  So if we already
> have alpha[] and/or a helper function that does strspn(alpha) that
> we can reuse elsewhere, I do not think it is worth to try supporting
> punctuation.
>

> In what situation would a user use "-m 'appended pieces of text'"
> option, together with "--fixup=amend:<commit>"?  I am wondering if
> we want such a "append to" feature, or is it easier to understand
> for end-users if "-m", "-F", "-C" and "-c" (the common trait of
> these options is that they contribute to the log message text)
> are made incompatible with --fixup=amend:<commit>.
>

> > ...Thanks, for pointing this out. Also, in the above method for
> > alnum I think we can initialize an array of alnum[] instead of
> > alphas[]. Or otherwise I was thinking to instead check:
> >            if (!isalnum(*c) && *c == ':')
>
> Sure a loop is fine, or alnum[] is fine, or just alpha[] is OK, I
> would think.  Do you foresee you'd need --fixup=chomp124:<commit>?
> I somehow doubt it.

>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> > For end-users "-m" or "-F" will make it easier to prepare an "amend!"
> > commit. Because using the "-m" reduces the cost of opening an editor
> > to prepare "amend!" commit and it can be done with command line only.
>
> Hmph.  That is not very convicing to me.  The user is motivated
> enough to fix a wrong commit log message and replace it with an
> improved one by using the "--fixup:amend" feature---why would that
> improved message can sufficiently be written with just an "-m
> message", which by definition would be much less capable of
> preparing a well-thought-out message than with an editor?
>
> Yes, with "-m", you can add some short string easily at the end of
> the existing commit message without opening an editor.  But I am
> trying to see why it is a good thing to be able to do so in the
> first place.  It is very easy to make typoes while doing so and it
> would be hard to fix these typoes, exactly because you are doing so
> without being in an editor.  And the whole point of --fixup=amend is
> about improving the message (as opposed to --fixup that is to record
> the contents that have already been improved in the index).
>
> This is why I kept asking what the use case would look like.  I am
> trying to find out if you have a good end-user story in mind that we
> can use in the documentation to sell the feature to end-users, but I
> am not seeing one.
>

> Is the combination of "--fixup=amend" and "-m <msg>" meant to be
> used primarily "to leave a note to myself" when the user runs
> --fixup=amend:<commit>, to just record the points to be elaborated
> when the message is written for real much later?  E.g.
>
>     ... hack hack hack ...
>     ... good stopping point, but cannot afford time to write
>     ... a good log message now
>     $ git commit --fixup=amend:HEAD~2 -m 'talk about X, Y and Z' -a
>     ... hack hack hack ...
>     ... possibly doing something entirely different ...
>     ... finally comes back to finish cleaning up the branch for real ...
>     $ git rebase --autosquash -i origin
>
> And one of the insn created in the todo sheet would be amend!, whose
> commit message has the message taken from the original HEAD~2 PLUS the
> reminder to the user that s/he needs to talk about X, Y and Z?  And
> the user at that point writes more comprehensive message about these
> three things?
>

> That is a made-up example of what "appending some short strings
> possibly full of typoes without having to open an editor" could be
> useful for.  I am not sure if it is truly useful, or if it is just a
> hand wavy excuse not to mark -m/-F incompatible with --fixup=amend
> without a real merit [*].
>
>     Side note: one reason why I do not find it realistic is that it
>     is unlikely that the user would use --fixup=amend to slurp in
>     the original log message when recording "good stopping point,
>     but cannot afford time" fixup.  "--squash HEAD~2 -m <msg>" would
>     be much faster to record the "note to myself" reminder, and when
>     the user finally comes back to clean things up, the amount of
>     work to edit the original's message while looking at the "note
>     to myself" appended at the end would not be any different in
>     either workflow.
>

> Thanks a lot for explaining each perspective in detail. So, now if we
> use `-m` as an option to write side-note then `--squash` is already
> available and for fixing the commit message opening the editor is a
> must expected option. So shall we remove the `-m` option compatibility
> if `amend`/ `reword` suboptions are passed to `git commit --fixup` ?

>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> > Thanks a lot for explaining each perspective in detail. So, now if we
> > use `-m` as an option to write side-note then `--squash` is already
> > available and for fixing the commit message opening the editor is a
> > must expected option. So shall we remove the `-m` option compatibility
> > if `amend`/ `reword` suboptions are passed to `git commit --fixup` ?
>
> FWIW, I do not have a strong opposition against -m/-F that is not
> rejected when --fixup=amend is in use.  It is OK for Git to accept
> useless combinations of options that do not help end-users.  A
> combination that is not useful will be left unused, which is not a
> great loss.

> I was just hoping that letting "-m <message>" and "--fixup=amend"
> used at the same time would support a great use case, and because I
> didn't think of any, I asked the person who allowed the combination
> (that is you) what the intended use cases are.  Actively supporting
> a combination because it gives users great value is more satisfying
> than just leaving useless combination to exist only because it does
> not actively hurt.  When users say "The command can take these two
> options at the same time, but I cannot figure out what good the
> combination is for. It feels utterly useless to use them together"
> it also is much easier to answer them if we can say "because by
> using these two together, you can do this great thing" instead of
> having to say "we do not think it makes much sense to use these two
> options together."
>

>>  So, if it is more work to make the code notice when
>> these options are given in useless combinations and stop with an
>> error message
>
> Sorry I didn't get this and would like to once confirm here that, are
> you pointing to output an error message when the `-m`/`-F` option is
> passed with `git --fixup=amend/reword` ? Because I think we can do
> this also. Otherwise ....

>>than just accepting and doing useless thing, I am OK
>> if we left them as they are.
>
> ....If we allow both `m` and `F` to work with `git commit
> --fixup=amend/reword` with the same working as it is doing now i.e to
> use `m` to write new commit message, upon `--autosquash`, If it is
> okay? then I also agree to update the documentation more precisely and
> include the uses when passed with `m` /`F`(not yet added) option.

> If we were to make -m/-F incompatible with these new features, then
> sure, we'd notice the combination, show an error message and abort.
>
> >>than just accepting and doing useless thing, I am OK
> >> if we left them as they are.
> >
> > ....If we allow both `m` and `F` to work with `git commit
> > --fixup=amend/reword` with the same working as it is doing now i.e to
> > use `m` to write new commit message, upon `--autosquash`, If it is
> > okay? then I also agree to update the documentation more precisely and
> > include the uses when passed with `m` /`F`(not yet added) option.
>
> What would that more precise documentation would say, though?
>
> "'-m message' gets appended to the message taken from the original
> commit"?  Saying that alone, without explaining why doing such an
> appending is useful, would puzzle users and makes them ask "but why
> such a useless feature exist?" and that was why I was trying to
> figure out what it is useful for with you, which I think we have
> failed to do so far.
>
> My preference at this point is to error out the combination that we
> cannot figure out how it could be useful at this moment, so that
> users who find how it would be useful to come to us and present a
> hopefully good case for using -m <msg> with --fixup=amend:<commit>.
> I am assuming that allowing the combination at that point is easy,
> and the user request will give us a good use case we can use in the
> documentation to explain for what purpose a user may want to use -m
> <msg> to append a short string at the end.  The end users' use case
> we see at that point might even suggest that it would be more useful
> to prepend (as opposed to append) the message we get from -m <msg>
> to the original log message, and such a change will not be possible
> if we just choose to append without thinking through the use case we
> intend to support and release "we do not know what good it would do
> to append with -m <msg>, but that is what the code happens to do now"
> version to the users, as people will depend on the behaviour of any
> released versions.

> `git commit --fixup=reword:<commit>` creates an empty "amend!" commit
> that will reword <commit> without changing its contents when it is
> rebased with --autosquash.
>
> Apart from ignoring staged changes it works similarly to
> `--fixup=amend:<commit>`.
>
> Example usage:
> $ git commit --fixup=reword:HEAD~3
> $ git commit --fixup=reword:HEAD~3 -m "new commit message"

> +static void check_fixup_reword_options(void) {
> +	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."));
> +	}
> +	if (all)
> +		die(_("cannot combine reword option of --fixup with --all"));
> +	if (also)
> +		die(_("cannot combine reword option of --fixup with --include"));
> +	if (only)
> +		die(_("cannot combine reword option of --fixup with --only"));
> +}

> > +static void check_fixup_reword_options(void) {
> > +     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."));
> > +     }
> > +     if (all)
> > +             die(_("cannot combine reword option of --fixup with --all"));
> > +     if (also)
> > +             die(_("cannot combine reword option of --fixup with --include"));
> > +     if (only)
> > +             die(_("cannot combine reword option of --fixup with --only"));
> > +}
>
> Not just these options, wouldn't it be an error to ask to commit
> anything but an empty commit?  E.g. shouldn't this sequence
>
>         edit builtin/commit.c
>         git commit --fixup=reword:HEAD~3 -- builtin/commit.c
>
> trigger an error, as we will *not* be taking any change made to the
> working tree file?
>
> Or is that implicitly covered by some other code?
>

> +test_fixup_reword_opt () {
> +	test_expect_success C_LOCALE_OUTPUT "--fixup=reword: incompatible with $1" "
> +		echo 'fatal: cannot combine reword option of --fixup with $1' >expect &&
> +		test_must_fail git commit --fixup=reword:HEAD~ $1 2>actual &&
> +		test_cmp expect actual
> +	"
> +}
> +
> +for opt in --all --include --only
> +do
> +	test_fixup_reword_opt $opt
> +done

> +test_expect_success '--fixup=reword: -F give error message' '
> +	echo "fatal: Only one of -c/-C/-F/--fixup can be used." >expect &&
> +	test_must_fail git commit --fixup=reword:HEAD~ -F msg  2>actual &&
> +	test_cmp expect actual
> +'

> > +for opt in --all --include --only
> > +do
> > +     test_fixup_reword_opt $opt
> > +done
>
> As I suspected earlier, a pathspec is not tested here, but it should
> be.
>

> > +test_expect_success '--fixup=reword: -F give error message' '
> > +     echo "fatal: Only one of -c/-C/-F/--fixup can be used." >expect &&
> > +     test_must_fail git commit --fixup=reword:HEAD~ -F msg  2>actual &&
> > +     test_cmp expect actual
> > +'
>
> Why?  If you can use -m msg, you should be able to use -F msgfile,
> too, no?

>> > +test_expect_success '--fixup=reword: -F give error message' '
>> > +     echo "fatal: Only one of -c/-C/-F/--fixup can be used." >expect &&
>> > +     test_must_fail git commit --fixup=reword:HEAD~ -F msg  2>actual &&
>> > +     test_cmp expect actual
>> > +'
>>
>> Why?  If you can use -m msg, you should be able to use -F msgfile,
>> too, no?
>
> Earlier I was thinking to let the `--fixup=amend:`  use the same options as of
> current `--fixup=` . But yes I agree that there should be  -F option
> also with `amend`
> and `reword`.

> Hmph, I was actually imagining the opposite---a context that does
> not want to take -c/-C/-F would not want to take -m, either.
>
> Why is -m so special, and a lot more importantly, what would a user
> want to achieve by using "-m more-text" combined with this
> "--fixup=reword:<commit>" or "--fixup=amend:<commit>" feature?

> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
>  Documentation/git-commit.txt | 39 ++++++++++++++++++++++++++++++------
>  Documentation/git-rebase.txt | 21 ++++++++++---------
>  2 files changed, 44 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index 17150fa7ea..9a60876845 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -9,7 +9,7 @@ SYNOPSIS
>  --------
>  [verse]
>  'git commit' [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]
> -	   [--dry-run] [(-c | -C | --fixup | --squash) <commit>]
> +	   [--dry-run] [(-c | -C | --squash) <commit> | --fixup [(amend|reword):]<commit>)]
>  	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
>  	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
>  	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
> @@ -86,11 +86,38 @@ OPTIONS
>  	Like '-C', but with `-c` the editor is invoked, so that
>  	the user can further edit the commit message.
>  
> ---fixup=<commit>::
> -	Construct a commit message for use with `rebase --autosquash`.
> -	The commit message will be the subject line from the specified
> -	commit with a prefix of "fixup! ".  See linkgit:git-rebase[1]
> -	for details.
> +--fixup=[(amend|reword):]<commit>::
> +	When used without options, lets's say `git commit --fixup=<commit>`,
> +	it creates 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.
> +
> +	When used with option `amend`, let's say
> +	`git commit --fixup=amend:<commit>`, it creates a "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
> +	"amend!" commit log message to allow to edit further. And it denies
> +	to create "amend!" commit if it's commit message body is empty unless
> +	used with `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.
> +
> +	When used with alternative option `reword`, let's say
> +	`git commit --fixup=reword:<commit>`, it works similar to `amend`
> +	option, but here it creates an empty "amend!" commit, i.e it does
> +	not take any staged changes and only allows to fixup the commit
> +	message of the specified commit. It will reword the specified
> +	commit when it is rebased with `--autosquash`.
> +
> +	`--fixup`, with or without option, can be used with additional
> +	commit message option `-m` but not with `-F`/`-c`/`-C`. See
> +	linkgit:git-rebase[1] for details.

>  --squash=<commit>::
>  	Construct a commit message for use with `rebase --autosquash`.
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 8bfa5a9272..ffea76e53b 100644
> --- 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! ..."), and there is already a commit in the todo list that
> -	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`).  A commit 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 commits is by using
> -	the `--fixup`/`--squash` options of linkgit:git-commit[1].
> +	When the commit log message begins with "squash! ..." (or "fixup! ..."
> +	or "amend! ..."), and there is already a commit in the todo list that
> +	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
> +	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].
>  +
>  If the `--autosquash` option is enabled by default using the
>  configuration variable `rebase.autoSquash`, this option can be

> > +
> > +     When used with alternative option `reword`, let's say
> > +     `git commit --fixup=reword:<commit>`, it works similar to `amend`
> > +     option, but here it creates an empty "amend!" commit, i.e it does
> > +     not take any staged changes and only allows to fixup the commit
> > +     message of the specified commit. It will reword the specified
> > +     commit when it is rebased with `--autosquash`.
> > +
> > +     `--fixup`, with or without option, can be used with additional
> > +     commit message option `-m` but not with `-F`/`-c`/`-C`. See
> > +     linkgit:git-rebase[1] for details.
>
> You must dedent the second and the subsequent paragraphs and replace
> each of these blank lines that mark inter-paragraph breaks with a
> line with a single plus '+' sign on it.  See how "is mutually
> exclusive" explanation is appended to the description of "-m" as its
> second paragraph.
>
> Side note.  It probably needs "exclusive -> incompatible".
>
> The description in git-rebase.txt you touched is another good
> example.  Mimic the way its second paragraph "If the autosquash is
> enabled..."  is formatted.
>

> This patch series teaches `git commit --fixup` to create "amend!" commit
> as an alternative that works with `git rebase --autosquash`. It allows to
> fixup both the content and the commit message of the specified commit.
> Here we add two suboptions to the `--fixup`, first `amend` suboption that
> creates an "amend!" commit. It takes the staged changes and also allows to
> edit the commit message of the commit we are fixing.
> Example usuage:
> git commit --fixup=amend:<commit>
>
> Secondly, `reword` suboption that creates an empty "amend!" commit i.e it
> ignores the staged changes and only allows to reword/edit the commit message
> of the commit we are fixing.
> Example usuage:
> git commit --fixup=reword:<commit>

> We used to have only --fixup that was meant to squeeze in minor
> corrections to the contents recorded, and it kept the log message
> of the original commit intact.
>
> Now we have two other ways, --fixup=reword that is meant to correct
> only the log message while keeping the contents intact from the
> original, and --fixup=amend that is meant to allow users to do both.
> They are nice additions to our toolbox.
>
> While trying to use the --fixup=amend myself to "touch up" somebody
> else's work today, another thing that we did not discuss so far came
> to my mind (sorry, if this was discussed and resolved in your
> previous discussions with other reviewers).  What should we do to
> the authorship?
>

> For the original --fixup, it is reasonably obvious that the original
> authorship should be kept, as the intended use case is to make a
> small tweak that does not change the intention of the commit in any
> way (and that is why the log message from the original is kept), and
> with --fixup=reword, it would probably be the same (the contents
> were written by the original author alone, and the person fixing-up
> is not changing only the log message).  So these two have a
> reasonably good default model for the authorship information for the
> final outcome: the original authorship should be kept (of course,
> the user can easily take over the authorship later with "git commit
> --amend --reset-author" perhaps run as part of "rebase -i", if the
> contribution is significant enough to deserve the transfer of the
> authorship).
>
> But I am not sure what the default behaviour for the authorship when
> --fixup=amend:<commit> is used to update somebody else's commit.  I
> think it is OK to leave it to whatever the code happens to do right
> now (simply because I have no strong reason to vote for either way
> between keeping the original and letting the amending user take it
> over), but I think it should be documented what happens in each
> case.
>

> `git commit --fixup=amend:<commit>` will create an "amend!" commit.
> The resulting commit message subject will be "amend! ..." where
> "..." is the subject line of <commit> and the initial message
> body will be <commit>'s message. -m can be used to override the
> message body.
>
> The "amend!" commit when rebased with --autosquash will fixup the
> contents and replace the commit message of <commit> with the
> "amend!" commit's message body.
>
> Inorder to prevent rebase from creating commits with an empty

> message we refuse to create an "amend!" commit if commit message
> body is empty.
> ...
> +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,
> +								struct pretty_print_context *ctx) {

> +	/*
> +	 * If we amend the 'amend!' commit then we don't want to
> +	 * duplicate the subject line.
> +	 */
> +	const char *format = NULL;
> +	if (starts_with(sb->buf, "amend! amend!"))
> +		format = "%b";
> +	else
> +		format = "%B";

> ...
> +		format_commit_message(commit, fmt, &sb, &ctx);
> +		free(fmt);
>  		hook_arg1 = "message";
> +
> +		if ((have_option_m) && !strcmp(fixup_prefix,"fixup"))

>  	if (also + only + all + interactive > 1)
>  		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
> +
> +	if (fixup_message) {
> +		/*
> +		 * As `amend` suboption contains only alpha
> +		 * character. So check if first non alpha
> +		 * character in fixup_message is ':'.
> +		 */
> +		size_t len = get_alpha_len(fixup_message);
> +		if (len && fixup_message[len] == ':') {
> +			fixup_message[len] = '\0';
> +			fixup_commit = fixup_message + ++len;

> +			if (starts_with("amend", fixup_message))
> +				fixup_prefix = "amend";
> +			else
> +				die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
> +		} else {
> +			fixup_commit = fixup_message;
> +			fixup_prefix = "fixup";
> +			use_editor = 0;
> +		}
> +	}

> > Inorder to prevent rebase from creating commits with an empty
>
> In order to prevent ...
>
> > message we refuse to create an "amend!" commit if commit message
> > body is empty.
> > ...
> > +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,
> > +                                                             struct pretty_print_context *ctx) {
>
> Don't indent the second line unnecessarily too deep.
>

> > +     /*
> > +      * If we amend the 'amend!' commit then we don't want to
> > +      * duplicate the subject line.
> > +      */
> > +     const char *format = NULL;
> > +     if (starts_with(sb->buf, "amend! amend!"))
> > +             format = "%b";
> > +     else
> > +             format = "%B";
>
> I am not sure how well this strategy of special case only two amend!
> scales.  What would happen when we --fixup another "fixup!"commit?
>

> Shouldn't the caller, when it called format_commit_message() to
> prepare sb it passed to us, have stripped out existing prefix, if
> any, so that we can always use the same %B format, or something like
> that?
>

> > ...
> > +             format_commit_message(commit, fmt, &sb, &ctx);
> > +             free(fmt);
> >               hook_arg1 = "message";
> > +
> > +             if ((have_option_m) && !strcmp(fixup_prefix,"fixup"))
>
> Unnecessary () around have_option_m, and missing SP after ",".
>
> > +                     strbuf_addbuf(&sb, &message);
> > +
> > +             if (!strcmp(fixup_prefix,"amend")) {
>
> Missing SP after "," (I won't repeat---please re-check the whole
> patch series before rerolling).

>
> > +                     if (have_option_m)
> > +                             die(_("cannot combine -m with --fixup:%s"), fixup_message);
> > +                     else
> > +                             prepare_amend_commit(commit, &sb, &ctx);
>
> Hmph, why is -m so special?  Should we allow --fixup=amend:<cmd>
> with -F (or -c/-C for that matter), or are these other options
> caught at a lot higher layer already and we do not have to check
> them here?
>

> >       if (also + only + all + interactive > 1)
> >               die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
> > +
> > +     if (fixup_message) {
> > +             /*
> > +              * As `amend` suboption contains only alpha
> > +              * character. So check if first non alpha
> > +              * character in fixup_message is ':'.
> > +              */
> > +             size_t len = get_alpha_len(fixup_message);
> > +             if (len && fixup_message[len] == ':') {
> > +                     fixup_message[len] = '\0';
> > +                     fixup_commit = fixup_message + ++len;
>
> It would be easier to follow to write it this way, no?
>
>                         fixup_message[len++] = '\0';
>                         fixup_commit = fixup_message + len;
>

>> > +                     if (have_option_m)
>> > +                             die(_("cannot combine -m with --fixup:%s"), fixup_message);
>> > +                     else
>> > +                             prepare_amend_commit(commit, &sb, &ctx);
>>
>> Hmph, why is -m so special?  Should we allow --fixup=amend:<cmd>
>> with -F (or -c/-C for that matter), or are these other options
>> caught at a lot higher layer already and we do not have to check
>> them here?
>
> yes, those options are caught earlier and give the error as below:
> "Only one of -c/-C/-F/--fixup can be used."
> and only `-m` is checked over here.

> > yes, those options are caught earlier and give the error as below:
> > "Only one of -c/-C/-F/--fixup can be used."
> > and only `-m` is checked over here.
>
> And the reason why -m cannot be checked early is because we do not
> recognize which kind of "fixup" we are doing when "only one of
> -c/-C/-F/--fixup" check is made before this function is called?
>
> OK.  I wonder if we can tell which kind of fixup we are doing much
> earlier, though.  Then we could extend it to say "Only one of
> -c/-C/-F/-m/--fixup=amend:<commit> can be used", etc., and we do not
> have to have this "only -m is checked here, everything else is
> checked earlier" curiosity.  But I do not know if such a change is
> necessarily an improvement.  I guess a better "fix" would probably
> be to add a comment to this function where it only checks for "-m"
> and tell readers why -c/-C/-F do not have to be checked here.
>

> `git commit --fixup=reword:<commit>` creates an empty "amend!" commit
> that will reword <commit> without changing its contents when it is
> rebased with --autosquash.
>
> Apart from ignoring staged changes it works similarly to
> `--fixup=amend:<commit>`.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
>  builtin/commit.c | 46 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 56ae15a762..82e77aa61d 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1177,6 +1177,27 @@ static void finalize_deferred_config(struct wt_status *s)
>  		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
>  }
>  
> +static void check_fixup_reword_options(int argc) {
> +	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."));
> +	}
> +	if (argc)
> +		die(_("cannot combine reword option of --fixup with paths"));

> +	if (patch_interactive)
> +		die(_("cannot combine reword option of --fixup with --patch"));
> +	if (interactive)
> +		die(_("cannot combine reword option of --fixup with --interactive"));
> +	if (all)
> +		die(_("cannot combine reword option of --fixup with --all"));
> +	if (also)
> +		die(_("cannot combine reword option of --fixup with --include"));
> +	if (only)
> +		die(_("cannot combine reword option of --fixup with --only"));
> +}
> +
>  /* returns the length of intial segment of alpha characters only */
>  static size_t get_alpha_len(char *fixup_message) {
>  	const char alphas[] = "abcdefghijklmnopqrstuvwxyz";
> @@ -1261,18 +1282,25 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  
>  	if (fixup_message) {
>  		/*
> -		 * As `amend` suboption contains only alpha
> -		 * character. So check if first non alpha
> -		 * character in fixup_message is ':'.
> +		 * As `amend`/`reword` suboptions contains only alpha
> +		 * characters. So check if first non alpha character
> +		 * in fixup_message is ':'.
>  		 */
>  		size_t len = get_alpha_len(fixup_message);
>  		if (len && fixup_message[len] == ':') {
>  			fixup_message[len] = '\0';
>  			fixup_commit = fixup_message + ++len;
> -			if (starts_with("amend", fixup_message))
> +			if (starts_with("amend", fixup_message) ||
> +				starts_with("reword", fixup_message)) {
>  				fixup_prefix = "amend";
> -			else
> +				if (*fixup_message == 'r') {

> +				}
> +			} else {
>  				die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
> +			}
>  		} else {
>  			fixup_commit = fixup_message;
>  			fixup_prefix = "fixup";
> @@ -1558,11 +1586,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		OPT_STRING('c', "reedit-message", &edit_message, N_("commit"), N_("reuse and edit message from specified commit")),
>  		OPT_STRING('C', "reuse-message", &use_message, N_("commit"), N_("reuse message from specified commit")),
>  		/*
> -		 * TRANSLATORS: please do not translate [amend:]
> -		 * Here "amend" is an option to the --fixup command
> -		 * line flag, that creates amend! commit.
> +		 * TRANSLATORS: please do not translate [(amend|reword):]
> +		 * Here "amend" and "reword" are options to the --fixup
> +		 * command line flag, that creates amend! commit.

>  		 */
> -		OPT_STRING(0, "fixup", &fixup_message, N_("[amend:]commit"), N_("use autosquash formatted message to fixup or amend specified commit")),
> +		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
>  		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
>  		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
>  		OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),

> > +     if (argc)
> > +             die(_("cannot combine reword option of --fixup with paths"));
>
> It would be easier if the user is told "foo" in the message when
>
>     $ git commit --fixup=reword:HEAD~ -- foo
>
> (from your tests) is attempted, no?
>

>
> I am not sure this comment is all that helpful to the translaters.
> If they are not allowed to translate <amend|reword> part, telling
> them what that part means does not help them very much.
>
>         Leave "[(amend|reword):]" as-is, and only translate <commit>.
>
> would be more direct without distracting them with useless piece of
> information, no?

> +	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.
> ++
> +When used with option `amend`, let's say `git commit --fixup=amend:<commit>`,
> +it creates a "amend!" commit to fixup both the content and the commit log

> +contents and replace the commit message of the specified commit with the
> +"amend!" commit's message body.
> ++
> +When used with alternative option `reword`, let's say
> +`git commit --fixup=reword:<commit>`, it works similar to `amend` option, but
> +here it creates an empty "amend!" commit, i.e it does not take any staged

> +changes and only allows to fixup the commit message of the specified commit.
> +It will reword the specified commit when it is rebased with `--autosquash`.
> ++
> +Unlike `--fixup` without options, `--fixup=[amend/reword]:` is incompatible with
> +`-m` commit message option.

>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> > +--fixup=[(amend|reword):]<commit>::
> > +     When used without options, lets's say `git commit --fixup=<commit>`,
> > +     it creates a "fixup!" commit where the commit message will be
>
> Be careful to use word 'option' that refers to something other than
> what a casual reader would think of an `--option` in the description
> of the `--fixup` option.
>
>         Without `amend:` or `reword:`, create a `fixup!` commit where...
>
> would be sufficient, no?
>

> > +     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.
> > ++
> > +When used with option `amend`, let's say `git commit --fixup=amend:<commit>`,
> > +it creates a "amend!" commit to fixup both the content and the commit log
>
>         The `--fixup=amend:<commit>` form creates an "amend!" commit to...
>
> > +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
>
> While that SP inside the double-quote may be technically more
> correct (and it was inherited from the original), I think with 'a
> prefix of "amend!"' is still understandable and a lot easier to
> read, especially because you'd mention "amend!"  a few more times in
> the same paragraph below.
>
> The same comment applies to "fixup! " above.
>

> > +specified commit. It also invokes an editor seeded with the "amend!" commit
> > +log message to allow to edit further. And it denies to create "amend!" commit
>
> "amend!" commit log message -> log message of the "amend!" commit
>
> denies -> refuses
>
> > +if it's commit message body is empty unless used with `allow-empty-message`
> > +option. "amend!" commit when rebased with `--autosquash` will fixup the
>
> with the `--allow-empty-message` option.
>
> > +contents and replace the commit message of the specified commit with the
> > +"amend!" commit's message body.
> > ++
> > +When used with alternative option `reword`, let's say
> > +`git commit --fixup=reword:<commit>`, it works similar to `amend` option, but
> > +here it creates an empty "amend!" commit, i.e it does not take any staged
>
>         The `--fixup=reword:<commit>` form creates an `amend!`
>         commit similar to `--fixup=amend:<commit>` creates, but it
>         records the same tree as `HEAD`, i.e. it does not ...
>

> > +changes and only allows to fixup the commit message of the specified commit.
> > +It will reword the specified commit when it is rebased with `--autosquash`.
> > ++
> > +Unlike `--fixup` without options, `--fixup=[amend/reword]:` is incompatible with
> > +`-m` commit message option.
>
>         The `--fixup=amend:` and `--fixup=reword:` forms cannot be
>         used with other options to add to the commit log message,
>         e.g. `-m`.
>
> Again, why is `-m` so special?  Shouldn't -F/-c/-C also be
> incompatible?
>