[PATCH v3 0/6][Outreachy] commit: Implementation of "amend!" commit

[PATCH v3 0/6][Outreachy] commit: Implementation of "amend!" commit

[Charvi Mendiratta]

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>

** This work is rebased on the top of cm/rebase-i-updates.

Changes from v2 :

* doc/git-commit.c: reword and simplify the `--fixup=[amend|reword]`
* error out "foo" in the output message when `--fixup=reword:` combined
  with pathspec i.e
  $ git commit --fixup=reword:HEAD~ -- foo
  (I am bit unsure about this as I have directly used the "argv" to get the
  first foo (file) passed. I am doubtful and not sure about, if we can
  use the `pathspec_from_file` in (builtin/commit.c) ? )
* fixes the TRANSLATOR, commit message and other minor cleanup (v2 2/6)

Charvi Mendiratta (6):
  sequencer: export subject_length()
  commit: add amend suboption to --fixup to create amend! commit
  commit: add a reword suboption to --fixup
  t7500: add tests for --fixup=[amend|reword] options
  t3437: use --fixup with options to create amend! commit
  doc/git-commit: add documentation for fixup=[amend|reword] options

 Documentation/git-commit.txt              |  40 ++++++-
 Documentation/git-rebase.txt              |  21 ++--
 builtin/commit.c                          | 125 ++++++++++++++++++--
 commit.c                                  |  14 +++
 commit.h                                  |   3 +
 sequencer.c                               |  14 ---
 t/t3437-rebase-fixup-options.sh           |  30 +----
 t/t7500-commit-template-squash-signoff.sh | 135 ++++++++++++++++++++++
 8 files changed, 316 insertions(+), 66 deletions(-)

--
2.29.0.rc1

[PATCH v3 1/6] sequencer: export subject_length()

[Charvi Mendiratta]

This function can be used in other parts of git. Let's move the
function to commit.c.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 commit.c    | 14 ++++++++++++++
 commit.h    |  3 +++
 sequencer.c | 14 --------------
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/commit.c b/commit.c
index bab8d5ab07..41cc72c4de 100644
--- a/commit.c
+++ b/commit.c
@@ -535,6 +535,20 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
 	return eol - p;
 }
 
+size_t subject_length(const char *body)
+{
+	const char *p = body;
+	while (*p) {
+		const char *next = skip_blank_lines(p);
+		if (next != p)
+			break;
+		p = strchrnul(p, '\n');
+		if (*p)
+			p++;
+	}
+	return p - body;
+}
+
 struct commit_list *commit_list_insert(struct commit *item, struct commit_list **list_p)
 {
 	struct commit_list *new_list = xmalloc(sizeof(struct commit_list));
diff --git a/commit.h b/commit.h
index f4e7b0158e..12ff6bc641 100644
--- a/commit.h
+++ b/commit.h
@@ -165,6 +165,9 @@ const void *detach_commit_buffer(struct commit *, unsigned long *sizep);
 /* Find beginning and length of commit subject. */
 int find_commit_subject(const char *commit_buffer, const char **subject);
 
+/* Return length of the commit subject from commit log message. */
+size_t subject_length(const char *body);
+
 struct commit_list *commit_list_insert(struct commit *item,
 					struct commit_list **list);
 int commit_list_contains(struct commit *item,
diff --git a/sequencer.c b/sequencer.c
index abc6d5cdfd..3fac4ba62f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1724,20 +1724,6 @@ enum todo_item_flags {
 	TODO_EDIT_FIXUP_MSG    = (1 << 2),
 };
 
-static size_t subject_length(const char *body)
-{
-	const char *p = body;
-	while (*p) {
-		const char *next = skip_blank_lines(p);
-		if (next != p)
-			break;
-		p = strchrnul(p, '\n');
-		if (*p)
-			p++;
-	}
-	return p - body;
-}
-
 static const char first_commit_msg_str[] = N_("This is the 1st commit message:");
 static const char nth_commit_msg_fmt[] = N_("This is the commit message #%d:");
 static const char skip_first_commit_msg_str[] = N_("The 1st commit message will be skipped:");
-- 
2.29.0.rc1

Re: [PATCH v3 1/6] sequencer: export subject_length()

[Eric Sunshine]

On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> This function can be used in other parts of git. Let's move the
> function to commit.c.
>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
> diff --git a/commit.h b/commit.h
> @@ -165,6 +165,9 @@ const void *detach_commit_buffer(struct commit *, unsigned long *sizep);
> +/* Return length of the commit subject from commit log message. */
> +size_t subject_length(const char *body);

Now that this function is public, is the name too generic? Most other
functions in this header have "commit" in the name. So,
commit_subject_length() might be one possibility (assuming the current
name is too generic).

Re: [PATCH v3 1/6] sequencer: export subject_length()

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
>> This function can be used in other parts of git. Let's move the
>> function to commit.c.
>>
>> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
>> ---
>> diff --git a/commit.h b/commit.h
>> @@ -165,6 +165,9 @@ const void *detach_commit_buffer(struct commit *, unsigned long *sizep);
>> +/* Return length of the commit subject from commit log message. */
>> +size_t subject_length(const char *body);
>
> Now that this function is public, is the name too generic? Most other
> functions in this header have "commit" in the name. So,
> commit_subject_length() might be one possibility (assuming the current
> name is too generic).

Thanks for being a careful reviewer, as always.

Re: [PATCH v3 1/6] sequencer: export subject_length()

[Charvi Mendiratta]

On Tue, 2 Mar 2021 at 01:55, Eric Sunshine <sunshine@sunshineco.com> wrote:

> Now that this function is public, is the name too generic? Most other
> functions in this header have "commit" in the name. So,
> commit_subject_length() might be one possibility (assuming the current
> name is too generic).

Agree, I will change it and update the patch. Thanks for this fix.

[PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

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

In order to prevent rebase from creating commits with an empty
message we refuse to create an "amend!" commit if commit message
body is empty.

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 | 97 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 88 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 505fe60956..200ef83cc0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -105,7 +105,8 @@ static const char *template_file;
  */
 static const char *author_message, *author_message_buffer;
 static char *edit_message, *use_message;
-static char *fixup_message, *squash_message;
+static char *fixup_message, *fixup_commit, *squash_message;
+static const char *fixup_prefix;
 static int all, also, interactive, patch_interactive, only, amend, signoff;
 static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
@@ -681,6 +682,21 @@ static void adjust_comment_line_char(const struct strbuf *sb)
 	comment_line_char = *p;
 }

+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, format, sb, ctx);
+	return 0;
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct commit *current_head,
 			     struct wt_status *s,
@@ -745,15 +761,33 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	} else if (fixup_message) {
 		struct pretty_print_context ctx = {0};
 		struct commit *commit;
-		commit = lookup_commit_reference_by_name(fixup_message);
+		char *fmt = xstrfmt("%s! %%s\n\n", fixup_prefix);
+		commit = lookup_commit_reference_by_name(fixup_commit);
 		if (!commit)
-			die(_("could not lookup commit %s"), fixup_message);
+			die(_("could not lookup commit %s"), fixup_commit);
 		ctx.output_encoding = get_commit_output_encoding();
-		format_commit_message(commit, "fixup! %s\n\n",
-				      &sb, &ctx);
-		if (have_option_m)
-			strbuf_addbuf(&sb, &message);
+		format_commit_message(commit, fmt, &sb, &ctx);
+		free(fmt);
 		hook_arg1 = "message";
+
+		/*
+		 * Only `-m` commit message option is checked here, as
+		 * it supports `--fixup` to append the commit message.
+		 *
+		 * The other commit message options `-c`/`-C`/`-F` are
+		 * incompatible with all the forms of `--fixup` and
+		 * have already errored out while parsing the `git commit`
+		 * options.
+		 */
+		if (have_option_m && !strcmp(fixup_prefix, "fixup"))
+			strbuf_addbuf(&sb, &message);
+
+		if (!strcmp(fixup_prefix, "amend")) {
+			if (have_option_m)
+				die(_("cannot combine -m with --fixup:%s"), fixup_message);
+			else
+				prepare_amend_commit(commit, &sb, &ctx);
+		}
 	} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
 		size_t merge_msg_start;

@@ -1152,6 +1186,12 @@ static void finalize_deferred_config(struct wt_status *s)
 		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
 }

+/* returns the length of intial segment of alpha characters only */
+static size_t get_alpha_len(char *fixup_message) {
+	const char alphas[] = "abcdefghijklmnopqrstuvwxyz";
+	return strspn(fixup_message, alphas);
+}
+
 static int parse_and_validate_options(int argc, const char *argv[],
 				      const struct option *options,
 				      const char * const usage[],
@@ -1170,7 +1210,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (force_author && renew_authorship)
 		die(_("Using both --reset-author and --author does not make sense"));

-	if (logfile || have_option_m || use_message || fixup_message)
+	if (logfile || have_option_m || use_message)
 		use_editor = 0;
 	if (0 <= edit_flag)
 		use_editor = edit_flag;
@@ -1227,6 +1267,28 @@ static int parse_and_validate_options(int argc, const char *argv[],

 	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;
+		}
+	}
+
 	cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);

 	handle_untracked_files_arg(s);
@@ -1504,7 +1566,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK('m', "message", &message, N_("message"), N_("commit message"), opt_parse_m),
 		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")),
-		OPT_STRING(0, "fixup", &fixup_message, N_("commit"), N_("use autosquash formatted message to fixup specified commit")),
+		/*
+		 * TRANSLATORS: Leave "[amend:]" as-is, and
+		 * only translate <commit>.
+		 */
+		OPT_STRING(0, "fixup", &fixup_message, N_("[amend:]commit"), N_("use autosquash formatted message to fixup or amend 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")),
@@ -1663,6 +1729,19 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		exit(1);
 	}

+	if (fixup_message && starts_with(sb.buf, "amend! ") &&
+		!allow_empty_message) {
+		struct strbuf body = STRBUF_INIT;
+		size_t len = subject_length(sb.buf);
+		strbuf_addstr(&body, sb.buf + len);
+		if (message_is_empty(&body, cleanup_mode)) {
+			rollback_index_files();
+			fprintf(stderr, _("Aborting commit due to empty commit message body.\n"));
+			exit(1);
+		}
+		strbuf_release(&body);
+	}
+
 	if (amend) {
 		const char *exclude_gpgsig[3] = { "gpgsig", "gpgsig-sha256", NULL };
 		extra = read_commit_extra_headers(current_head, exclude_gpgsig);
--
2.29.0.rc1

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

> +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,
> +								 struct pretty_print_context *ctx) {

Why does this need to be overly indented?  Are you using some funny
tab width settings?  In this project, a tab stop is 8-spaces wide.

> +		/*
> +		 * Only `-m` commit message option is checked here, as
> +		 * it supports `--fixup` to append the commit message.

As it is OK to use "-m" with the plain vanilla "--fixup", an earlier
check did not reject the combination, but now we look at what kind
of fixup it is, and error out if it is "--fixup=amend:".  OK.

> +		 * The other commit message options `-c`/`-C`/`-F` are
> +		 * incompatible with all the forms of `--fixup` and
> +		 * have already errored out while parsing the `git commit`
> +		 * options.
> +		 */

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

On Tue, 2 Mar 2021 at 00:04, Junio C Hamano <gitster@pobox.com> wrote:
>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> > +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,
> > +                                                              struct pretty_print_context *ctx) {
>
> Why does this need to be overly indented?  Are you using some funny
> tab width settings?  In this project, a tab stop is 8-spaces wide.
>

Oops, I myself didn't expect it after cross-check in the vscode
editor. I will fix this.

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Eric Sunshine]

On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> `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.
>
> In order to prevent rebase from creating commits with an empty
> message we refuse to create an "amend!" commit if commit message
> body is empty.
>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
> diff --git a/builtin/commit.c b/builtin/commit.c
> @@ -105,7 +105,8 @@ static const char *template_file;
> +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!"))

Is the content of the incoming strbuf created mechanically so that we
know that there will only ever be one space between the two "amend!"
literals? If not, then this starts_with() check feels fragile.
(Compare with the code in sequencer.c which checks for this sort of
duplication but is tolerant of one or more spaces, not just a single
space.)

> +               format = "%b";
> +       else
> +               format = "%B";

It's subjective and minor, but this could be expressed more compactly as:

    const char *fmt = starts_with(...) ? "%b" : "%B";

Also, no need to initialize `format` to NULL since it gets assigned in
all code paths.

Not worth a re-roll.

> @@ -745,15 +761,33 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> +               if (!strcmp(fixup_prefix, "amend")) {
> +                       if (have_option_m)
> +                               die(_("cannot combine -m with --fixup:%s"), fixup_message);
> +                       else
> +                               prepare_amend_commit(commit, &sb, &ctx);
> +               }

This is minor, but the way this is written, the error case and the
normal case appear to have the same significance, whereas, if you
write it like this:

    if (!strcmp(...)) {
        if (have_option_m)
            die(...);
        prepare_amend_commit(...);
    }

then it's easier to see that you're checking for and getting an error
case out of the way early, which allows the reader to concentrate
without distraction on the normal case. As a minor benefit, you also
get to eliminate an indentation level for the normal case, which could
be important if more code is added to that case.

Not worth a re-roll.

> @@ -1227,6 +1267,28 @@ 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 ':'.
> +                */
> +               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);

I haven't read ahead in the series yet, but I presume you're making
this code extra generic because you plan to support additional `fixup`
options (such as `reword`), but I wonder if the cognitive overhead is
warranted or you could get by with something simpler, such as:

    if (skip_prefix(msg, "amend:", &arg) ||
        skip_prefix(msg, "reword:", &arg)) {
        ...
    }

Also, am I misreading when I think that the use of starts_with() could
be replaced with a simple strcmp() since you've already inserted a
'\0' immediately after the final alphabetic character?

Not necessarily worth a re-roll.

> @@ -1663,6 +1729,19 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> +               if (message_is_empty(&body, cleanup_mode)) {
> +                       rollback_index_files();
> +                       fprintf(stderr, _("Aborting commit due to empty commit message body.\n"));
> +                       exit(1);

I was wondering why you are capitalizing the error message (these days
we don't) and using exit() instead of die(), but I see that you're
mirroring existing practice in this function. Okay.

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +               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);
>
> I haven't read ahead in the series yet, but I presume you're making
> this code extra generic because you plan to support additional `fixup`
> options (such as `reword`), but I wonder if the cognitive overhead is
> warranted or you could get by with something simpler, such as:
>
>     if (skip_prefix(msg, "amend:", &arg) ||
>         skip_prefix(msg, "reword:", &arg)) {
>         ...
>     }

You still need to compute "len" because you'd want to tell between
--fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter
would want to say "no such variant 'bogus' for --fixup", but the
colon in the former is not the end of the name of variant.

So, skip_prefix() would not buy us much, I guess.

But the use of starts_with() in the original patch is bogus, I
think.  fixup_message[] by the time the comparison is made is
NULL terminated at where the colon was originally, so we should be
doing !strcmp() to reject "--fixup=amendo:HEAD~2" with "no, 'amendo'
is not a valid variant name for --fixup option".

> Also, am I misreading when I think that the use of starts_with() could
> be replaced with a simple strcmp() since you've already inserted a
> '\0' immediately after the final alphabetic character?

Correct.

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Eric Sunshine]

On Mon, Mar 1, 2021 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> >     if (skip_prefix(msg, "amend:", &arg) ||
> >         skip_prefix(msg, "reword:", &arg)) {
> >         ...
> >     }
>
> You still need to compute "len" because you'd want to tell between
> --fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter
> would want to say "no such variant 'bogus' for --fixup", but the
> colon in the former is not the end of the name of variant.

I see what you mean. I vaguely recall quickly scanning over that
earlier discussion about ":" being otherwise legitimate when embedded
in the argument as you demonstrate, but didn't think about it when
reading this code. Perhaps the comment which this code adds:

    * As `amend` suboption contains only alpha
    * character. So check if first non alpha
    * character in fixup_message is ':'.

could be extended a bit to mention that briefly since, without it, the
significance of "alpha-only characters followed by colon" is not
immediately obvious.

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

On Tue, 2 Mar 2021 at 04:17, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Mon, Mar 1, 2021 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > >     if (skip_prefix(msg, "amend:", &arg) ||
> > >         skip_prefix(msg, "reword:", &arg)) {
> > >         ...
> > >     }
> >
> > You still need to compute "len" because you'd want to tell between
> > --fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter
> > would want to say "no such variant 'bogus' for --fixup", but the
> > colon in the former is not the end of the name of variant.
>
> I see what you mean. I vaguely recall quickly scanning over that
> earlier discussion about ":" being otherwise legitimate when embedded
> in the argument as you demonstrate, but didn't think about it when
> reading this code. Perhaps the comment which this code adds:
>
>     * As `amend` suboption contains only alpha
>     * character. So check if first non alpha
>     * character in fixup_message is ':'.
>
> could be extended a bit to mention that briefly since, without it, the
> significance of "alpha-only characters followed by colon" is not
> immediately obvious.

Okay, I will reword it and make it more clear regarding the main
reason for choosing this way.

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

On Tue, 2 Mar 2021 at 04:02, Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> >> +               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);
> >
> > I haven't read ahead in the series yet, but I presume you're making
> > this code extra generic because you plan to support additional `fixup`
> > options (such as `reword`), but I wonder if the cognitive overhead is
> > warranted or you could get by with something simpler, such as:
> >
> >     if (skip_prefix(msg, "amend:", &arg) ||
> >         skip_prefix(msg, "reword:", &arg)) {
> >         ...
> >     }
>
> You still need to compute "len" because you'd want to tell between
> --fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter
> would want to say "no such variant 'bogus' for --fixup", but the
> colon in the former is not the end of the name of variant.
>
> So, skip_prefix() would not buy us much, I guess.
>

Yes, I also agree.

> But the use of starts_with() in the original patch is bogus, I
> think.  fixup_message[] by the time the comparison is made is
> NULL terminated at where the colon was originally, so we should be
> doing !strcmp() to reject "--fixup=amendo:HEAD~2" with "no, 'amendo'
> is not a valid variant name for --fixup option".
>

I am not sure about this because we used the starts_with() so that it can
support the _any_ prefix of `amend` or `reword` i.e to make all below
like combinations possible :
--fixup=a:HEAD~2
--fixup=am:HEAD~2

So, I am not sure if we need to replace it with !strcmp and work for
the specified prefix only ?

> > Also, am I misreading when I think that the use of starts_with() could
> > be replaced with a simple strcmp() since you've already inserted a
> > '\0' immediately after the final alphabetic character?
>
> Correct.
>

Same reason for using the starts_with() applies for this part also
after inserting the '\0'.  So, I think we can keep starts_with() ?

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Eric Sunshine]

On Wed, Mar 3, 2021 at 2:41 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> On Tue, 2 Mar 2021 at 04:02, Junio C Hamano <gitster@pobox.com> wrote:
> > But the use of starts_with() in the original patch is bogus, I
> > think.  fixup_message[] by the time the comparison is made is
> > NULL terminated at where the colon was originally, so we should be
> > doing !strcmp() to reject "--fixup=amendo:HEAD~2" with "no, 'amendo'
> > is not a valid variant name for --fixup option".
>
> I am not sure about this because we used the starts_with() so that it can
> support the _any_ prefix of `amend` or `reword` i.e to make all below
> like combinations possible :
> --fixup=a:HEAD~2
> --fixup=am:HEAD~2
>
> So, I am not sure if we need to replace it with !strcmp and work for
> the specified prefix only ?

Hmm, I see. I didn't follow whatever discussion led to the decision to
use this sort of prefix matching, but I have to wonder if it is a good
idea. Was the idea that it behave similarly to sequencer commands in
`git rebase --interactive` which are often abbreviated to a single
letter? I personally would feel much more comfortable requiring a
full-word match for `amend` and `reword` at initial implementation.
That leaves the door open to later loosening it to do prefix-matching
if enough people request such a feature, whereas starting with
prefix-matching closes that door since we can never later tighten it
to require full words.

Anyhow, if the decision is to keep this behavior, then it almost
certainly deserves an in-code comment explaining the sort of
prefix-matching it's doing since it's otherwise too easy for readers
to be fooled as Junio and I were by not noticing that you had reversed
the arguments to starts_with().

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

On Wed, 3 Mar 2021 at 13:27, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
[...]
> > I am not sure about this because we used the starts_with() so that it can
> > support the _any_ prefix of `amend` or `reword` i.e to make all below
> > like combinations possible :
> > --fixup=a:HEAD~2
> > --fixup=am:HEAD~2
> >
> > So, I am not sure if we need to replace it with !strcmp and work for
> > the specified prefix only ?
>
> Hmm, I see. I didn't follow whatever discussion led to the decision to
> use this sort of prefix matching, but I have to wonder if it is a good
> idea. Was the idea that it behave similarly to sequencer commands in
> `git rebase --interactive` which are often abbreviated to a single
> letter?

Yes, this is also true. Also, same is discussed as here:
https://lore.kernel.org/git/CAPSFM5cEnex1xaBy5ia_xNFDNzt5_Y=W-6TB9d9yW_AiPAKxDg@mail.gmail.com/

>I personally would feel much more comfortable requiring a
> full-word match for `amend` and `reword` at initial implementation.
> That leaves the door open to later loosening it to do prefix-matching
> if enough people request such a feature, whereas starting with
> prefix-matching closes that door since we can never later tighten it
> to require full words.
>
> Anyhow, if the decision is to keep this behavior, then it almost
> certainly deserves an in-code comment explaining the sort of
> prefix-matching it's doing since it's otherwise too easy for readers
> to be fooled as Junio and I were by not noticing that you had reversed
> the arguments to starts_with().

Okay, I will add the comments to it .

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Hmm, I see. I didn't follow whatever discussion led to the decision to
> use this sort of prefix matching, but I have to wonder if it is a good
> idea.

Meaning --fixup=a:<commit> and --fixup=amend:<commit> do the same
thing, until somebody invents --fixup=another:<commit> and makes the
prefix 'a' no longer unique?  I tend to agree that, especially with
command line completion support with modern shells, such a prefix
matching would not be necessary. 

> Was the idea that it behave similarly to sequencer commands in
> `git rebase --interactive` which are often abbreviated to a single
> letter? I personally would feel much more comfortable requiring a
> full-word match for `amend` and `reword` at initial implementation.

Me too.

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

On Thu, 4 Mar 2021 at 06:28, Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > Hmm, I see. I didn't follow whatever discussion led to the decision to
> > use this sort of prefix matching, but I have to wonder if it is a good
> > idea.
>
> Meaning --fixup=a:<commit> and --fixup=amend:<commit> do the same
> thing, until somebody invents --fixup=another:<commit> and makes the
> prefix 'a' no longer unique?  I tend to agree that, especially with
> command line completion support with modern shells, such a prefix
> matching would not be necessary.
>

Okay, so for now I think (as suggested), let's remove it and directly
use "!strcmp()" only instead of "starts_with()". I agree that if we
keep prefix matching or not, it will not matter much.

Thanks and Regards,
Charvi

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

On Tue, 2 Mar 2021 at 03:45, Eric Sunshine <sunshine@sunshineco.com> wrote:

> > @@ -105,7 +105,8 @@ static const char *template_file;
> > +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!"))
>
> Is the content of the incoming strbuf created mechanically so that we
> know that there will only ever be one space between the two "amend!"
> literals? If not, then this starts_with() check feels fragile.
> (Compare with the code in sequencer.c which checks for this sort of
> duplication but is tolerant of one or more spaces, not just a single
> space.)
>

Yes, so for preparing each "amend!" commit we add prefix "amend! '' to
the subject of the specific commit. And further if we amend the
"amend!" commit then this above code is checked before creating a
"amend! amend!" commit for the user. So I think maybe we don't need to
check for multiple spaces ?

> > +               format = "%b";
> > +       else
> > +               format = "%B";
>
> It's subjective and minor, but this could be expressed more compactly as:
>
>     const char *fmt = starts_with(...) ? "%b" : "%B";
>
> Also, no need to initialize `format` to NULL since it gets assigned in
> all code paths.
>
> Not worth a re-roll.
>

I agree, I will fix it.

> > @@ -745,15 +761,33 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> > +               if (!strcmp(fixup_prefix, "amend")) {
> > +                       if (have_option_m)
> > +                               die(_("cannot combine -m with --fixup:%s"), fixup_message);
> > +                       else
> > +                               prepare_amend_commit(commit, &sb, &ctx);
> > +               }
>
> This is minor, but the way this is written, the error case and the
> normal case appear to have the same significance, whereas, if you
> write it like this:
>
>     if (!strcmp(...)) {
>         if (have_option_m)
>             die(...);
>         prepare_amend_commit(...);
>     }
>
> then it's easier to see that you're checking for and getting an error
> case out of the way early, which allows the reader to concentrate
> without distraction on the normal case. As a minor benefit, you also
> get to eliminate an indentation level for the normal case, which could
> be important if more code is added to that case.
>
> Not worth a re-roll.
>

Thanks for pointing this out, I will remove the unnecessary `else`.

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Eric Sunshine]

On Wed, Mar 3, 2021 at 2:37 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> On Tue, 2 Mar 2021 at 03:45, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > +       if (starts_with(sb->buf, "amend! amend!"))
> >
> > Is the content of the incoming strbuf created mechanically so that we
> > know that there will only ever be one space between the two "amend!"
> > literals? If not, then this starts_with() check feels fragile.
>
> Yes, so for preparing each "amend!" commit we add prefix "amend! '' to
> the subject of the specific commit. And further if we amend the
> "amend!" commit then this above code is checked before creating a
> "amend! amend!" commit for the user. So I think maybe we don't need to
> check for multiple spaces ?

Okay, if this is guaranteed to be created mechanically, then what you
have should work, though it may be a good idea to add an in-code
comment stating the reason it is okay to expect just the single space.

The alternative would be to avoid having "amend! amend!" in the first
place. I didn't trace through the code carefully so I don't know if it
is possible, but would it make sense for the caller(s) to check before
adding a second "amend!", thus eliminating the need to do so here?
(Perhaps I'm misunderstanding, but the above code almost feels like a
case of "whoops, we did something undesirable, so let's undo it.".)

Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

Hi Eric,

On Wed, 3 Mar 2021 at 13:16, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Wed, Mar 3, 2021 at 2:37 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> > On Tue, 2 Mar 2021 at 03:45, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > > +       if (starts_with(sb->buf, "amend! amend!"))
> > >
> > > Is the content of the incoming strbuf created mechanically so that we
> > > know that there will only ever be one space between the two "amend!"
> > > literals? If not, then this starts_with() check feels fragile.
> >
> > Yes, so for preparing each "amend!" commit we add prefix "amend! '' to
> > the subject of the specific commit. And further if we amend the
> > "amend!" commit then this above code is checked before creating a
> > "amend! amend!" commit for the user. So I think maybe we don't need to
> > check for multiple spaces ?
>
> Okay, if this is guaranteed to be created mechanically, then what you
> have should work, though it may be a good idea to add an in-code
> comment stating the reason it is okay to expect just the single space.
>
> The alternative would be to avoid having "amend! amend!" in the first
> place.

Agree. I think we can do this...

> I didn't trace through the code carefully so I don't know if it
> is possible, but would it make sense for the caller(s) to check before
> adding a second "amend!", thus eliminating the need to do so here?
> (Perhaps I'm misunderstanding, but the above code almost feels like a
> case of "whoops, we did something undesirable, so let's undo it.".)

I looked into it and got another alternative, to extend the same
prepare_amend_commit() function and replace the check condition of
if (starts_with(sb->buf, "amend! amend!")) with the code as below :

const char *buffer = get_commit_buffer(commit, NULL);
const char *subject;
find_commit_subject(buffer, &subject);
if (starts_with(subject, "amend!"))
const char *fmt = starts_with(subject, "amend!") ? "%b" : "%B";
format_commit_message(commit, fmt, sb, ctx);
unuse_commit_buffer(commit, buffer);

So, now it checks the commit subject here only. Otherwise as you have
suggested above to check before adding a second "amend!", I think that
can result in confusion as currently both "fixup!" and "amend!"
commits (commit's subject) are prepared by same code and further for
"amend!" commit as we write a commit message body also so we used
prepare_amend_commit() to do that stuff.

[PATCH v3 3/6] commit: add a reword suboption to --fixup

[Charvi Mendiratta]

`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 | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 200ef83cc0..a4d18d96df 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1186,6 +1186,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, const char *argv[]) {
+	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 path %s"), *argv);
+	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";
@@ -1270,18 +1291,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') {
+					check_fixup_reword_options(argc, argv);
+					allow_empty = 1;
+					only = 1;
+				}
+			} else {
 				die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
+			}
 		} else {
 			fixup_commit = fixup_message;
 			fixup_prefix = "fixup";
@@ -1567,10 +1595,10 @@ 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: Leave "[amend:]" as-is, and
-		 * only translate <commit>.
+		 * TRANSLATORS: Leave "[(amend|reword):]" as-is,
+		 * and only translate <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")),
--
2.29.0.rc1

Re: [PATCH v3 3/6] commit: add a reword suboption to --fixup

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

> `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 | 44 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 200ef83cc0..a4d18d96df 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1186,6 +1186,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, const char *argv[]) {
> +	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 path %s"), *argv);

I think our convention is to quote '%s' with a single-quote pair.
See other error messages.

commit.c:			die_errno(_("could not read '%s'"), templat...
commit.c:		die_errno(_("could not open '%s'"), git_path_commit...
commit.c:	die(_("--author '%s' is not 'Name <email>' and matches no e...
commit.c:		die(_("Invalid ignored mode '%s'"), ignored_arg);
commit.c:		die(_("Invalid untracked files mode '%s'"), untrack...
...

Re: [PATCH v3 3/6] commit: add a reword suboption to --fixup

[Charvi Mendiratta]

On Tue, 2 Mar 2021 at 00:11, Junio C Hamano <gitster@pobox.com> wrote:
>
[...]
> > +     if (argc)
> > +             die(_("cannot combine reword option of --fixup with path %s"), *argv);
>
> I think our convention is to quote '%s' with a single-quote pair.
> See other error messages.
>
> commit.c:                       die_errno(_("could not read '%s'"), templat...
> commit.c:               die_errno(_("could not open '%s'"), git_path_commit...
> commit.c:       die(_("--author '%s' is not 'Name <email>' and matches no e...
> commit.c:               die(_("Invalid ignored mode '%s'"), ignored_arg);
> commit.c:               die(_("Invalid untracked files mode '%s'"), untrack...
> ...

I admit I forgot to add it. I will fix it.

Re: [PATCH v3 3/6] commit: add a reword suboption to --fixup

[Eric Sunshine]

On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> `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>`.
>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
> diff --git a/builtin/commit.c b/builtin/commit.c
> @@ -1186,6 +1186,27 @@ static void finalize_deferred_config(struct wt_status *s)
> +static void check_fixup_reword_options(int argc, const char *argv[]) {
> +       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 path %s"), *argv);
> +       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"));
> +}

Or, more concisely:

    if (argc)
        die(_("--fixup mutually exclusive with path '%s'), ...);
    if (patch_interactive || interactive || all || also || only)
        die(_("--fixup mutually exclusive with
--patch/--interactive/--all/--include/--only);

The mix of two different error message styles (capitalized with
full-stop vs. lowercase no-full-stop) is a bit jarring, but minor.

Not worth re-roll.

Re: [PATCH v3 3/6] commit: add a reword suboption to --fixup

[Charvi Mendiratta]

On Tue, 2 Mar 2021 at 04:06, Eric Sunshine <sunshine@sunshineco.com> wrote:
>

> > +       if (argc)
> > +               die(_("cannot combine reword option of --fixup with path %s"), *argv);
> > +       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"));
> > +}
>
> Or, more concisely:
>
>     if (argc)
>         die(_("--fixup mutually exclusive with path '%s'), ...);
>     if (patch_interactive || interactive || all || also || only)
>         die(_("--fixup mutually exclusive with
> --patch/--interactive/--all/--include/--only);
>

Got it ! Its seems more clear, I will fix it.

> The mix of two different error message styles (capitalized with
> full-stop vs. lowercase no-full-stop) is a bit jarring, but minor.
>

Okay, I will fix it.

[PATCH v3 4/6] t7500: add tests for --fixup=[amend|reword] options

[Charvi Mendiratta]

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t7500-commit-template-squash-signoff.sh | 135 ++++++++++++++++++++++
 1 file changed, 135 insertions(+)

diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 6d19ece05d..e62b7fbc1a 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -9,6 +9,8 @@ Tests for template, signoff, squash and -F functions.'
 
 . ./test-lib.sh
 
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
 commit_msg_is () {
 	expect=commit_msg_is.expect
 	actual=commit_msg_is.actual
@@ -279,6 +281,139 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
 
 extra"
 '
+get_commit_msg () {
+	rev="$1" &&
+	git log -1 --pretty=format:"%B" "$rev"
+}
+
+test_expect_success 'commit --fixup=amend: creates amend! commit' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited" \
+			git commit --fixup=amend:HEAD~
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--fixup=reword: does not commit staged changes' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited" \
+			git commit --fixup=reword:HEAD~
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual &&
+	test_cmp_rev HEAD@{1}^{tree} HEAD^{tree} &&
+	test_cmp_rev HEAD@{1} HEAD^ &&
+	test_expect_code 1 git diff --cached --exit-code &&
+	git cat-file blob :foo >actual &&
+	test_cmp foo actual
+'
+
+test_expect_success '--fixup=reword: error out with -m option' '
+	commit_for_rebase_autosquash_setup &&
+	echo "fatal: cannot combine -m with --fixup:reword" >expect &&
+	test_must_fail git commit --fixup=reword:HEAD~ -m "reword commit message" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--fixup=amend: error out with -m option' '
+	commit_for_rebase_autosquash_setup &&
+	echo "fatal: cannot combine -m with --fixup:amend" >expect &&
+	test_must_fail git commit --fixup=amend:HEAD~ -m "amend commit message" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'consecutive amend! commits remove amend! line from commit msg body' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited 1
+
+	edited 2
+	EOF
+	echo "reword new commit message" >actual &&
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited 1" \
+			git commit --fixup=reword:HEAD~ &&
+		FAKE_COMMIT_AMEND="edited 2" \
+			git commit --fixup=reword:HEAD
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'deny to create amend! commit if its commit msg body is empty' '
+	commit_for_rebase_autosquash_setup &&
+	echo "Aborting commit due to empty commit message body." >expected &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_COMMIT_MESSAGE="amend! target message subject line" \
+			git commit --fixup=amend:HEAD~ 2>actual
+	) &&
+	test_cmp expected actual
+'
+
+test_expect_success 'amend! commit allows empty commit msg body with --allow-empty-message' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_MESSAGE="amend! target message subject line" \
+			git commit --fixup=amend:HEAD~ --allow-empty-message &&
+		get_commit_msg HEAD >actual
+	) &&
+	test_cmp expected actual
+'
+
+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 --interactive --patch
+do
+	test_fixup_reword_opt $opt
+done
+
+test_expect_success '--fixup=reword: give error with pathsec' '
+	commit_for_rebase_autosquash_setup &&
+	echo "fatal: cannot combine reword option of --fixup with path foo" >expect &&
+	test_must_fail git commit --fixup=reword:HEAD~ -- foo 2>actual &&
+	test_cmp expect actual
+'
+
+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
+'
 
 test_expect_success 'commit --squash works with -F' '
 	commit_for_rebase_autosquash_setup &&
-- 
2.29.0.rc1

Re: [PATCH v3 4/6] t7500: add tests for --fixup=[amend|reword] options

[Eric Sunshine]

On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> t7500: add tests for --fixup=[amend|reword] options

It's usually preferable for tests and documentation updates to be
bundled along with the patch which makes a particular change[1] rather
than waiting until the very end of the series and adding tests and
documentation covering all the changes made by patches earlier in the
series. As a reviewer, it is much harder to tell if the late-added
tests and documentation updates are comprehensive since it's difficult
to keep in mind all the changes made by earlier patches.

When reading earlier patches in this series, I questioned whether or
not certain features of each patch were going to be covered by tests
or documentation updates, but I couldn't tell because those updates
weren't made at the same time as the change about which I was reading.
For instance, when reading the implementation of `--fixup:reword`, I
was wondering if the documentation was going to be updated to mention
that it would ignore changes staged in the index and leave the index
untouched, and I wondered if and hoped that tests would be added to
verify that the index was indeed left untouched. Over the course of
many patches, it can be difficult to keep track of all the accumulated
questions, which makes it onerous to review the final patches adding
the tests and documentation updates enmasse.

I'm not necessarily suggesting that you re-roll merely to incorporate
the tests and documentation updates into the patches to which they
belong, but it's something to keep in mind for future submissions.

FOOTNOTES

[1]: Once in a while a patch introducing a change is so large on its
own that it may make sense to split tests and documentation updates
out to their own patches which immediately follow the patch to which
they apply, but that's different from delaying _all_ tests and
documentation updates and plopping them at the end of the series all
crammed together.

Re: [PATCH v3 4/6] t7500: add tests for --fixup=[amend|reword] options

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
>> t7500: add tests for --fixup=[amend|reword] options
>
> It's usually preferable for tests and documentation updates to be
> bundled along with the patch which makes a particular change[1] rather
> than waiting until the very end of the series and adding tests and
> documentation covering all the changes made by patches earlier in the
> series. As a reviewer, it is much harder to tell if the late-added
> tests and documentation updates are comprehensive since it's difficult
> to keep in mind all the changes made by earlier patches.
>
> When reading earlier patches in this series, I questioned whether or
> not certain features of each patch were going to be covered by tests
> or documentation updates, but I couldn't tell because those updates
> weren't made at the same time as the change about which I was reading.
> For instance, when reading the implementation of `--fixup:reword`, I
> was wondering if the documentation was going to be updated to mention
> that it would ignore changes staged in the index and leave the index
> untouched, and I wondered if and hoped that tests would be added to
> verify that the index was indeed left untouched. Over the course of
> many patches, it can be difficult to keep track of all the accumulated
> questions, which makes it onerous to review the final patches adding
> the tests and documentation updates enmasse.
>
> I'm not necessarily suggesting that you re-roll merely to incorporate
> the tests and documentation updates into the patches to which they
> belong, but it's something to keep in mind for future submissions.
>
> FOOTNOTES
>
> [1]: Once in a while a patch introducing a change is so large on its
> own that it may make sense to split tests and documentation updates
> out to their own patches which immediately follow the patch to which
> they apply, but that's different from delaying _all_ tests and
> documentation updates and plopping them at the end of the series all
> crammed together.

A good piece of advice.  Thanks.

Re: [PATCH v3 4/6] t7500: add tests for --fixup=[amend|reword] options

[Charvi Mendiratta]

On Tue, 2 Mar 2021 at 11:13, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> > t7500: add tests for --fixup=[amend|reword] options
>
> It's usually preferable for tests and documentation updates to be
> bundled along with the patch which makes a particular change[1] rather
> than waiting until the very end of the series and adding tests and
> documentation covering all the changes made by patches earlier in the
> series. As a reviewer, it is much harder to tell if the late-added
> tests and documentation updates are comprehensive since it's difficult
> to keep in mind all the changes made by earlier patches.
>
> When reading earlier patches in this series, I questioned whether or
> not certain features of each patch were going to be covered by tests
> or documentation updates, but I couldn't tell because those updates
> weren't made at the same time as the change about which I was reading.
> For instance, when reading the implementation of `--fixup:reword`, I
> was wondering if the documentation was going to be updated to mention
> that it would ignore changes staged in the index and leave the index
> untouched, and I wondered if and hoped that tests would be added to
> verify that the index was indeed left untouched. Over the course of
> many patches, it can be difficult to keep track of all the accumulated
> questions, which makes it onerous to review the final patches adding
> the tests and documentation updates enmasse.
>
> I'm not necessarily suggesting that you re-roll merely to incorporate
> the tests and documentation updates into the patches to which they
> belong, but it's something to keep in mind for future submissions.
>
> FOOTNOTES
>
> [1]: Once in a while a patch introducing a change is so large on its
> own that it may make sense to split tests and documentation updates
> out to their own patches which immediately follow the patch to which
> they apply, but that's different from delaying _all_ tests and
> documentation updates and plopping them at the end of the series all
> crammed together.

Noted. Thanks for guiding, I will take care of it.

[PATCH v3 5/6] t3437: use --fixup with options to create amend! commit

[Charvi Mendiratta]

We taught `git commit --fixup` to create "amend!" commit. Let's also
update the tests and use it to setup the rebase tests.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t3437-rebase-fixup-options.sh | 30 +++---------------------------
 1 file changed, 3 insertions(+), 27 deletions(-)

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index a5a20354e3..d0bdc7ed02 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -72,40 +72,16 @@ test_expect_success 'setup' '
 	git commit --fixup=HEAD -a &&
 	git tag B1 &&
 	test_tick &&
-	git commit --allow-empty -F - <<-EOF &&
-	amend! B
-	$EMPTY
-	B
-	$EMPTY
-	edited 1
-	EOF
+	FAKE_COMMIT_AMEND="edited 1" git commit --fixup=reword:B &&
 	test_tick &&
-	git commit --allow-empty -F - <<-EOF &&
-	amend! amend! B
-	$EMPTY
-	B
-	$EMPTY
-	edited 1
-	$EMPTY
-	edited 2
-	EOF
+	FAKE_COMMIT_AMEND="edited 2" git commit --fixup=reword:HEAD &&
 	echo B2 >B &&
 	test_tick &&
 	FAKE_COMMIT_AMEND="edited squash" git commit --squash=HEAD -a &&
 	git tag B2 &&
 	echo B3 >B &&
 	test_tick &&
-	git commit -a -F - <<-EOF &&
-	amend! amend! amend! B
-	$EMPTY
-	B
-	$EMPTY
-	edited 1
-	$EMPTY
-	edited 2
-	$EMPTY
-	edited 3
-	EOF
+	FAKE_COMMIT_AMEND="edited 3" git commit -a --fixup=amend:HEAD^ &&
 	git tag B3 &&
 
 	GIT_AUTHOR_NAME="Rebase Author" &&
-- 
2.29.0.rc1

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

[Charvi Mendiratta]

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>
---
 Documentation/git-commit.txt | 40 ++++++++++++++++++++++++++++++------
 Documentation/git-rebase.txt | 21 ++++++++++---------
 2 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 17150fa7ea..722f8f0a6d 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,39 @@ 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>::
+	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.
++
+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.
++
+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 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`.
++
+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.
++
+Also, after fixing the commit using `--fixup`, with or without option
+and rebased with `--autosquash`, the authorship of the original commit
+remains unchanged. See linkgit:git-rebase[1] for details.
 
 --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
-- 
2.29.0.rc1

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

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

> +--fixup=[(amend|reword):]<commit>::
> +	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

What's the single quote in "fixup!'"???  We also have an "amend!'"
below.

> +	is further used with `git rebase --autosquash` to fixup the
> +	content of the specified commit.
> ++
> +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
> ...
> +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

"i.e."

> +with `-m`/`-F`/`-c`/`-C` options.
> ++

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

[Charvi Mendiratta]

On Tue, 2 Mar 2021 at 00:14, Junio C Hamano <gitster@pobox.com> wrote:
>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> > +--fixup=[(amend|reword):]<commit>::
> > +     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
>
> What's the single quote in "fixup!'"???  We also have an "amend!'"
> below.

I think I mistook it from the previous discussion and used a single
quote instead of space, as earlier it was "fixup " and "amend " which
was technically correct. But I will fix it and replace it with
"fixup!"  and "amend!".

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

[Eric Sunshine]

On Mon, Mar 1, 2021 at 3:52 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> @@ -9,7 +9,7 @@ SYNOPSIS
> +          [--dry-run] [(-c | -C | --squash) <commit> | --fixup [(amend|reword):]<commit>)]
> @@ -86,11 +86,39 @@ OPTIONS
> ---fixup=<commit>::
> +--fixup=[(amend|reword):]<commit>::

Although technically correct, I can't help but wonder if we can be
more friendly to readers by rephrasing this as:

    --fixup=<commit>::
    --fixup=amend:<commit>::
    --fixup=reword:<commit>::

which is probably a lot easier to take in and understand at a glance.
Same comment applies to the synopsis.

Not necessarily worth a re-roll.

> +       Without `amend:` or `reword:`, create a `fixup!` commit where
> +       the commit message will be the subject line from the specified
> +       commit with a prefix of "fixup!'". The resulting "fixup!" commit
> +       is further used with `git rebase --autosquash` to fixup the
> +       content of the specified commit.

I think it becomes important at this point to make it more clear that
_only_ the content of <commit> gets changed by the "fixup!" commit,
and that the log message of <commit> is untouched.

> +The `--fixup=amend:<commit>` form creates an "amend!" commit to
> +fixup both the content and the commit log message of the specified
> +commit. The resulting "amend!" commit's commit message subject
> +will be the subject line from the specified commit with a prefix of
> +"amend!'" and the message body will be commit log message of the
> +specified commit. It also invokes an editor seeded with the log
> +message of the "amend!" commit to allow to edit further. And it
> +refuses to create "amend!" commit if it's commit message body is
> +empty unless used with the `--allow-empty-message` option. "amend!"
> +commit when rebased with `--autosquash` will fixup the contents and
> +replace the commit message of the specified commit with the "amend!"
> +commit's message body.

I had to read this several times to understand what it is trying to
say. I believe that part of the problem is that the bulk of the
description goes into great detail describing bits and behaviors which
make no sense without understanding what an "amend!" commit actually
does, which isn't explained until the very last sentence. So, I think
the entire description needs to be flipped on its head. In particular,
it should start by saying "create a new commit which both fixes up the
content of <commit> and replaces <commit>'s log message", and only
then dive into the details.

In fact, what I just wrote suggests a larger problem with the
description of `--fixup` overall. There is no high-level explanation
of what a "fixup" (or "amend" or "reword") is; it just dives right
into the minutiae without providing the reader with sufficient context
to understand any of it. Only a reader who is already familiar with
interactive rebase is likely to grok what is being said here. So,
extending the thought I expressed above, it would be helpful for the
description of `--fixup=[amend:|reword:]` to start by first explaining
what a "fixup" is, followed by simple descriptions of "amend" and
"reword" (building upon "fixup"), and followed finally by details of
each. Very roughly, something like this:

    Creates a new commit which "fixes up" <commit> when applied with
    `git rebase --autosquash`.

    A "fixup" commit changes the content of <commit> but leaves its
    log message untouched.

    An "amend" commit is like "fixup" but also replaces the log
    message of <commit> with the log message of the "amend" commit.

    A "reword" commit replaces the log message of <commit> with its
    own log message but makes no changes to the content.

And then dive into the details of each variation.

> +The `--fixup=amend:` and `--fixup=reword:` forms cannot be used with
> +other options to add to the commit log message i.e it is incompatible
> +with `-m`/`-F`/`-c`/`-C` options.

I suppose it doesn't hurt, but I wonder if it's really necessary to
document this considering that the user will learn soon enough upon
trying invalid combinations.

> +Also, after fixing the commit using `--fixup`, with or without option
> +and rebased with `--autosquash`, the authorship of the original commit
> +remains unchanged. See linkgit:git-rebase[1] for details.

Good.

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> @@ -593,16 +593,17 @@ See also INCOMPATIBLE OPTIONS below.
>  --autosquash::
>  --no-autosquash::
> +       When the commit log message begins with "squash! ..." (or "fixup! ..."
> +       or "amend! ..."), and there is already a commit in the todo list that

Should this also be mentioning `reword!`?

> +       matches the same `...`, automatically modify the todo list of
> +       `rebase -i`, so that the commit marked for squashing comes right after
> +       the commit to be modified, and change the action of the moved commit
> +       from `pick` to `squash` (or `fixup` or `fixup -C`) respectively. A commit

It's becoming difficult to know which of the "foo!" prefixes get
transformed into which sequencer command since there is no longer a
one-to-one correspondence between "foo!" prefixes and sequencer
commands as there was when only "squash!" and "fixup!" existed. The
reader should be told what sequencer command(s) "amend!" and "reword!"
become.

> +       matches the `...` if the commit subject matches, or if the `...` refers
> +       to the commit's hash. As a fall-back, partial matches of the commit
> +       subject work, too. The recommended way to create fixup/squash/amend
> +       commits is by using the `--fixup=[amend|reword]`/`--squash` options of
> +       linkgit:git-commit[1].

At this point, it may be beneficial to write these out long-form to
make it easier on the reader; something along the lines of:

    ... the `--fixup`, `--fixup:amend:`, `--fixup:reword:`, and
    `--squash` options of ...

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

[Charvi Mendiratta]

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

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

[Eric Sunshine]

On Wed, Mar 3, 2021 at 2:44 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> 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>
> > > +       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.

By the way, now that you explained in the other thread that "short"
prefix-matching of "amend" and "reword" are allowed, I realize that
the documentation doesn't mention it (or at least I don't remember
reading it).

(Nevertheless, I still feel uncomfortable about supporting short
prefix-matching in the initial implementation without any evidence
that users will demand it, since we can't change that decision once
it's in the hands of users.)

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

Hmm, I see. So "reword!" is really just an "amend!" with only commit
message but no patch content. That makes perfect sense from an
implementation standpoint, but it makes me wonder if it would be
easier for users to understand if it created a "reword!" commit which
would be recognized as an alias of "amend!". (But maybe that's getting
too confusing, and my musing should be ignored.)

This also answers an unasked question I had regarding the duplicate
"amend! amend!" check. I was wondering why it wasn't also checking for
"reword! reword!".

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

[Charvi Mendiratta]

On Wed, 3 Mar 2021 at 13:48, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
[...]
> By the way, now that you explained in the other thread that "short"
> prefix-matching of "amend" and "reword" are allowed, I realize that
> the documentation doesn't mention it (or at least I don't remember
> reading it).
>

Yes, I admit it was not included. I will add it too.

> (Nevertheless, I still feel uncomfortable about supporting short
> prefix-matching in the initial implementation without any evidence
> that users will demand it, since we can't change that decision once
> it's in the hands of users.)
>

I am not sure about strong evidence but I tried to keep the major
points discussed earlier, as mentioned in the previous thread. Also I
think otherwise the short prefix will ease out / shorten the command
to prepare the "amend!" and also mirrors the commands in interactive
rebase.

> > > > +       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.
>
> Hmm, I see. So "reword!" is really just an "amend!" with only commit
> message but no patch content. That makes perfect sense from an
> implementation standpoint, but it makes me wonder if it would be
> easier for users to understand if it created a "reword!" commit which
> would be recognized as an alias of "amend!". (But maybe that's getting
> too confusing, and my musing should be ignored.)
>

Yes, we didn't choose to make "reword!" commit because if we do so
then again it would be expected to implicitly change 'pick' command to
'reword' in sequencer/ rebase to-do list when combined with 'git
rebase -- autosquash'. But here we are changing 'pick' to ' fixup -C'
to fulfill the working. So, we decided to create a variant of
'--fixup' and serve it as "amend!" commit.

> This also answers an unasked question I had regarding the duplicate
> "amend! amend!" check. I was wondering why it wasn't also checking for
> "reword! reword!".

Yes, it's true.

Thanks for the reveiws, I will add the above mentioned changes too.

Thanks and Regards,
Charvi

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> 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.
>
> Hmm, I see. So "reword!" is really just an "amend!" with only commit
> message but no patch content. That makes perfect sense from an
> implementation standpoint, but it makes me wonder if it would be
> easier for users to understand if it created a "reword!" commit which
> would be recognized as an alias of "amend!". (But maybe that's getting
> too confusing, and my musing should be ignored.)

Perhaps related, perhaps not, but I wonder if we really need --fixup=amend
and --fixup=reword to begin with.  The "amend" variant,

    $ git commit --fixup=amend:<original> ... other args ...

is about shaping the index with "other args" and recording the
resulting tree with the log message taken from <original>, marked
with the "amend!" prefix.  The --fixup=reword:<original> variant
is a mere special case of it where the recorded tree is made of the
index in the same way as a partial commit with pathspec that matches
no paths, i.e.  If you have --fixup=amend, you can do

    $ git commit --fixup=amend:<original> --only

and you do not need --fixup=reword:<original> at all, no?

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

[Charvi Mendiratta]

On Thu, 4 Mar 2021 at 06:35, Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > Hmm, I see. So "reword!" is really just an "amend!" with only commit
> > message but no patch content. That makes perfect sense from an
> > implementation standpoint, but it makes me wonder if it would be
> > easier for users to understand if it created a "reword!" commit which
> > would be recognized as an alias of "amend!". (But maybe that's getting
> > too confusing, and my musing should be ignored.)
>
> Perhaps related, perhaps not, but I wonder if we really need --fixup=amend
> and --fixup=reword to begin with.  The "amend" variant,
>
>     $ git commit --fixup=amend:<original> ... other args ...
>
> is about shaping the index with "other args" and recording the
> resulting tree with the log message taken from <original>, marked
> with the "amend!" prefix.  The --fixup=reword:<original> variant
> is a mere special case of it where the recorded tree is made of the
> index in the same way as a partial commit with pathspec that matches
> no paths, i.e.  If you have --fixup=amend, you can do
>
>     $ git commit --fixup=amend:<original> --only
>
> and you do not need --fixup=reword:<original> at all, no?
>

Maybe as an alternative User interface, we can remove the
`--fixup=reword:<original>`.

But for this patch, as we have kept separate suboption
`--fixup=reword:<original>` , so if now we do
`--fixup=amend:<original> --only` then it will return the error as
below :
fatal: No paths with --include/--only does not make sense.
So, `amend` works only if staged changes are present otherwise to
change only the commit message `reword` option is there.

I agree we can change to the above UI but still I wonder which one is
more friendly ? Also, I think we need to add complete `--allow-empty
--only` to mirror the working of `reword` so this may result in a lot
of typing and hard to remember.

Thanks and Regards,
Charvi

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

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

>> no paths, i.e.  If you have --fixup=amend, you can do
>>
>>     $ git commit --fixup=amend:<original> --only
>>
>> and you do not need --fixup=reword:<original> at all, no?
>>
>
> Maybe as an alternative User interface, we can remove the
> `--fixup=reword:<original>`.
>
> But for this patch, as we have kept separate suboption
> `--fixup=reword:<original>` , so if now we do
> `--fixup=amend:<original> --only` then it will return the error as
> below :
> fatal: No paths with --include/--only does not make sense.

Yes, but it is something we can easily fix, just like we made
"--only" without any pathname to work with "--amend" (or with
"--allow-empty").

The reason I brought it up was not because "--fixup=reword" is not
needed as a short-hand for "--only --fixup=amend" (but thinking
about it again, I do not think it is so bad), but primarily in
response to "would it be easier for users if we had reword! insn in
addition to amend! verb in the todo file?" that was raised earlier
in the thread.  If we position "--fixup=reword" as a short-hand
and/or a syntax sugar for "--fixup=amend" and advertise it as such
sufficiently to educate users, it would be easier for users to
understand why they both result in "amend!".

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

[Charvi Mendiratta]

On Fri, 5 Mar 2021 at 03:48, Junio C Hamano <gitster@pobox.com> wrote:
>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> >> no paths, i.e.  If you have --fixup=amend, you can do
> >>
> >>     $ git commit --fixup=amend:<original> --only
> >>
> >> and you do not need --fixup=reword:<original> at all, no?
> >>
> >
> > Maybe as an alternative User interface, we can remove the
> > `--fixup=reword:<original>`.
> >
> > But for this patch, as we have kept separate suboption
> > `--fixup=reword:<original>` , so if now we do
> > `--fixup=amend:<original> --only` then it will return the error as
> > below :
> > fatal: No paths with --include/--only does not make sense.
>
> Yes, but it is something we can easily fix, just like we made
> "--only" without any pathname to work with "--amend" (or with
> "--allow-empty").
>

Agree.

> The reason I brought it up was not because "--fixup=reword" is not
> needed as a short-hand for "--only --fixup=amend" (but thinking
> about it again, I do not think it is so bad), but primarily in
> response to "would it be easier for users if we had reword! insn in
> addition to amend! verb in the todo file?" that was raised earlier
> in the thread.  If we position "--fixup=reword" as a short-hand
> and/or a syntax sugar for "--fixup=amend" and advertise it as such
> sufficiently to educate users, it would be easier for users to
> understand why they both result in "amend!".

Okay, so now if it's Ok to keep the short-hand "--fixup=reword" ? then
I think making the documentation more clear would be sufficient to
serve it to the users ?
Otherwise if the votes are more that the user will get confused as
both results in "amend!", then should we change this patch to "--only
--fixup=amend" ?

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

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

>> The reason I brought it up was not because "--fixup=reword" is not
>> needed as a short-hand for "--only --fixup=amend" (but thinking
>> about it again, I do not think it is so bad), but primarily in
>> response to "would it be easier for users if we had reword! insn in
>> addition to amend! verb in the todo file?" that was raised earlier
>> in the thread.  If we position "--fixup=reword" as a short-hand
>> and/or a syntax sugar for "--fixup=amend" and advertise it as such
>> sufficiently to educate users, it would be easier for users to
>> understand why they both result in "amend!".
>
> Okay, so now if it's Ok to keep the short-hand "--fixup=reword" ? then
> I think making the documentation more clear would be sufficient to
> serve it to the users ?

It would be good 

 (1) to keep "--fixup=reword:<commit>"

 (2) to keep "amend!" but not introduce "reword!" insn

 (3) document "--fixup=reword:<commit>" can be thought of as a mere
     special-case short-hand for "--fixup=amend:<commit> --only",
     and

 (4) make sure "fixup=amend:<commit> --only" is usable as a
     replacement for "--fixup=reword:<commit>".

but if we are not doing (3) and (4), then it would also be OK to

 (1) to keep "--fixup=reword:<commit>"

 (2) to keep "amend!" and introduce "reword!" insn

I would think.

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

[Charvi Mendiratta]

On Fri, 5 Mar 2021 at 23:55, Junio C Hamano <gitster@pobox.com> wrote:
>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> >> The reason I brought it up was not because "--fixup=reword" is not
> >> needed as a short-hand for "--only --fixup=amend" (but thinking
> >> about it again, I do not think it is so bad), but primarily in
> >> response to "would it be easier for users if we had reword! insn in
> >> addition to amend! verb in the todo file?" that was raised earlier
> >> in the thread.  If we position "--fixup=reword" as a short-hand
> >> and/or a syntax sugar for "--fixup=amend" and advertise it as such
> >> sufficiently to educate users, it would be easier for users to
> >> understand why they both result in "amend!".
> >
> > Okay, so now if it's Ok to keep the short-hand "--fixup=reword" ? then
> > I think making the documentation more clear would be sufficient to
> > serve it to the users ?
>
> It would be good
>
>  (1) to keep "--fixup=reword:<commit>"
>
>  (2) to keep "amend!" but not introduce "reword!" insn
>
>  (3) document "--fixup=reword:<commit>" can be thought of as a mere
>      special-case short-hand for "--fixup=amend:<commit> --only",
>      and
>
>  (4) make sure "fixup=amend:<commit> --only" is usable as a
>      replacement for "--fixup=reword:<commit>".
>

Okay, I agree that this method is more clear ...

> but if we are not doing (3) and (4), then it would also be OK to
>
>  (1) to keep "--fixup=reword:<commit>"
>
>  (2) to keep "amend!" and introduce "reword!" insn
>

... than this one and will update the patch in the above (former) suggested way.


Thanks for suggestions and detailed explanation.

Thanks and Regards,
Charvi

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

[Eric Sunshine]

On Fri, Mar 5, 2021 at 11:13 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> On Fri, 5 Mar 2021 at 23:55, Junio C Hamano <gitster@pobox.com> wrote:
> >  (1) to keep "--fixup=reword:<commit>"
> >
> >  (2) to keep "amend!" but not introduce "reword!" insn
> >
> >  (3) document "--fixup=reword:<commit>" can be thought of as a mere
> >      special-case short-hand for "--fixup=amend:<commit> --only",
> >      and
> >
> >  (4) make sure "fixup=amend:<commit> --only" is usable as a
> >      replacement for "--fixup=reword:<commit>".
>
> Okay, I agree that this method is more clear ...

This works for me too, especially the bit about improving the
documentation to be more clear that --fixup=reword: is a special-case
(or syntactic sugar) for --fixup=amend:.

My confusion all along was thinking that --fixup=amend: and
--fixup=reword: resulted in distinct "amend!" and "reword!" prefixes.
I don't know whether that confusion was due to me not reading the
commit messages or documentation carefully enough, or because the
behavior wasn't clearly documented or easily understood. (I did have
to re-read the documentation patch multiple times in an attempt to
understand what it was saying, so perhaps I can blame that. ;-) At any
rate, it will be good if we can get it clearly documented.

> > but if we are not doing (3) and (4), then it would also be OK to
> >
> >  (1) to keep "--fixup=reword:<commit>"
> >
> >  (2) to keep "amend!" and introduce "reword!" insn
>
> ... than this one and will update the patch in the above (former) suggested way.

This option would likely be less desirable since it could confuse
people into thinking that "reword!" would become "reword" in the
sequencer instruction sheet -- which isn't the case at all -- it
becomes "fixup -c" (or -C, I can't remember).

Re: [PATCH v3 0/6][Outreachy] commit: Implementation of "amend!" commit

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

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

I only had a few minor nits and I think with them fixed the series
becomes as good as it would get in the shorter term to be ready for
'next'.  We're almost there.

Thanks.

Re: [PATCH v3 0/6][Outreachy] commit: Implementation of "amend!" commit

[Charvi Mendiratta]

> I only had a few minor nits and I think with them fixed the series
> becomes as good as it would get in the shorter term to be ready for
> 'next'.  We're almost there.
>

Thanks for pointing out the mistakes, I will fix them and update in
the next version.

Thanks and Regards,
Charvi

[PATCH v4 0/6][Outreachy] commit: Implementation of "amend!" commit

[Charvi Mendiratta]

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. `--fixup=reword:<commit>` is a short-hand of
`--fixup=amend:<commit> --only`.
Example usuage:
git commit --fixup=reword:<commit>

** This work is rebased on the top of cm/rebase-i-updates.

Changes from v3 :
(Thanks to Junio C Hamano and Eric Sunshine for the reviews and suggesting the
improvements required in the previous patches )

* Now `--fixup=reword:` aliases `--fixup=amend: --only`. Added the respective
test and updated documentation.
* Modified prepared_amend_commit() in commit.c.
* Renamed the subject_length() to commit_subject_length() [v3 - 1/6]
* Modified the --fixup=[amend|reword] documentation
* Also, updated the commit messages and some other minor cleanup.

Charvi Mendiratta (6):
  sequencer: export and rename subject_length()
  commit: add amend suboption to --fixup to create amend! commit
  commit: add a reword suboption to --fixup
  t7500: add tests for --fixup=[amend|reword] options
  t3437: use --fixup with options to create amend! commit
  doc/git-commit: add documentation for fixup=[amend|reword] options

 Documentation/git-commit.txt              |  40 ++++++-
 Documentation/git-rebase.txt              |  21 ++--
 builtin/commit.c                          | 118 +++++++++++++++++--
 commit.c                                  |  14 +++
 commit.h                                  |   3 +
 sequencer.c                               |  16 +--
 t/t3437-rebase-fixup-options.sh           |  30 +----
 t/t7500-commit-template-squash-signoff.sh | 135 ++++++++++++++++++++++
 8 files changed, 309 insertions(+), 68 deletions(-)

--
2.29.0.rc1

Re: [PATCH v4 0/6][Outreachy] commit: Implementation of "amend!" commit

[Eric Sunshine]

On Wed, Mar 10, 2021 at 2:43 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> Changes from v3 :
> (Thanks to Junio C Hamano and Eric Sunshine for the reviews and suggesting the
> improvements required in the previous patches )
>
> * Now `--fixup=reword:` aliases `--fixup=amend: --only`. Added the respective
> test and updated documentation.
> * Modified prepared_amend_commit() in commit.c.
> * Renamed the subject_length() to commit_subject_length() [v3 - 1/6]
> * Modified the --fixup=[amend|reword] documentation
> * Also, updated the commit messages and some other minor cleanup.

Thanks for re-rolling. I left some relatively minor comments on patch
[2/6] and some substantial documentation-related comments on [6/6].
(If you take my suggested documentation rewrites verbatim, you can
have my Signed-off-by: along with them.)

One final request: Please consider using the --range-diff or
--interdiff options or both when preparing your series with
git-format-patch. Doing so can really help reviewers focus on the
changes made from re-roll to re-roll without having to painstakingly
re-read each patch in the series -- even if a patch didn't change --
thus saving time.

Re: [PATCH v4 0/6][Outreachy] commit: Implementation of "amend!" commit

[Charvi Mendiratta]

On Thu, 11 Mar 2021 at 13:36, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
[...]
> Thanks for re-rolling. I left some relatively minor comments on patch
> [2/6] and some substantial documentation-related comments on [6/6].
> (If you take my suggested documentation rewrites verbatim, you can
> have my Signed-off-by: along with them.)
>

Sure, I will add the Signed-off-by.

Thanks for all the suggestions, I will fix them and update in the next version.

> One final request: Please consider using the --range-diff or
> --interdiff options or both when preparing your series with
> git-format-patch. Doing so can really help reviewers focus on the
> changes made from re-roll to re-roll without having to painstakingly
> re-read each patch in the series -- even if a patch didn't change --
> thus saving time.

Okay, I will add it next time.

Thanks and Regards,
Charvi

[PATCH v5 0/6][Outreachy] commit: Implementation of "amend!" commit

[Charvi Mendiratta]

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. `--fixup=reword:<commit>` is a short-hand of
`--fixup=amend:<commit> --only`.
Example usuage:
git commit --fixup=reword:<commit>

** This work is rebased on the top of cm/rebase-i-updates.

Charvi Mendiratta (6):
  sequencer: export and rename subject_length()
  commit: add amend suboption to --fixup to create amend! commit
  commit: add a reword suboption to --fixup
  t7500: add tests for --fixup=[amend|reword] options
  t3437: use --fixup with options to create amend! commit
  doc/git-commit: add documentation for fixup=[amend|reword] options

 Documentation/git-commit.txt              |  45 +++++-
 Documentation/git-rebase.txt              |  21 +--
 builtin/commit.c                          | 126 +++++++++++++++--
 commit.c                                  |  14 ++
 commit.h                                  |   3 +
 sequencer.c                               |  16 +--
 t/t3437-rebase-fixup-options.sh           |  30 +---
 t/t7500-commit-template-squash-signoff.sh | 159 ++++++++++++++++++++++
 8 files changed, 346 insertions(+), 68 deletions(-)

Range-diff against v4:
-:  ---------- > 1:  a2e89540ec sequencer: export and rename subject_length()
1:  492ab8ec08 ! 2:  be2808a255 commit: add amend suboption to --fixup to create amend! commit
    @@ builtin/commit.c: static void adjust_comment_line_char(const struct strbuf *sb)
      	comment_line_char = *p;
      }

    -+static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,
    -+				struct pretty_print_context *ctx) {
    -+
    ++static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
    ++				struct pretty_print_context *ctx)
    ++{
     +	const char *buffer, *subject, *fmt;
     +
     +	buffer = get_commit_buffer(commit, NULL);
    @@ builtin/commit.c: static void adjust_comment_line_char(const struct strbuf *sb)
     +	fmt = starts_with(subject, "amend!") ? "%b" : "%B";
     +	format_commit_message(commit, fmt, sb, ctx);
     +	unuse_commit_buffer(commit, buffer);
    -+	return 0;
     +}
     +
      static int prepare_to_commit(const char *index_file, const char *prefix,
    @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const cha
      		struct pretty_print_context ctx = {0};
      		struct commit *commit;
     -		commit = lookup_commit_reference_by_name(fixup_message);
    -+		char *fmt = xstrfmt("%s! %%s\n\n", fixup_prefix);
    ++		char *fmt;
     +		commit = lookup_commit_reference_by_name(fixup_commit);
      		if (!commit)
     -			die(_("could not lookup commit %s"), fixup_message);
    @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const cha
     -				      &sb, &ctx);
     -		if (have_option_m)
     -			strbuf_addbuf(&sb, &message);
    ++		fmt = xstrfmt("%s! %%s\n\n", fixup_prefix);
     +		format_commit_message(commit, fmt, &sb, &ctx);
     +		free(fmt);
      		hook_arg1 = "message";
    @@ builtin/commit.c: static void finalize_deferred_config(struct wt_status *s)
      }

     +/* returns the length of intial segment of alpha characters only */
    -+static size_t get_alpha_len(char *fixup_message) {
    ++static size_t skip_suboption(char *fixup_message) {
     +	const char alphas[] = "abcdefghijklmnopqrstuvwxyz";
     +	return strspn(fixup_message, alphas);
     +}
    @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar
     +
     +	if (fixup_message) {
     +		/*
    -+		 * To check if fixup_message that contains ':' is a commit
    -+		 * reference for example: --fixup="HEAD^{/^area: string}" or
    -+		 * a suboption of `--fixup`.
    ++		 * We limit --fixup's suboptions to only alpha characters.
    ++		 * If the first character after a run of alpha is colon,
    ++		 * then the part before the colon may be a known suboption
    ++		 * name `amend` or a misspelt suboption name. In this case,
    ++		 * we treat it as --fixup=<suboption>:<arg>.
     +		 *
    -+		 * As `amend` suboption contains only alpha character.
    -+		 * So check if first non alpha character in fixup_message
    -+		 * is ':'.
    ++		 * Otherwise, we are dealing with --fixup=<commit>.
     +		 */
    -+		size_t len = get_alpha_len(fixup_message);
    ++		size_t len = skip_suboption(fixup_message);
     +		if (len && fixup_message[len] == ':') {
     +			fixup_message[len++] = '\0';
     +			fixup_commit = fixup_message + len;
2:  a20b85c3a1 ! 3:  f6217338c1 commit: add a reword suboption to --fixup
    @@ builtin/commit.c: static void finalize_deferred_config(struct wt_status *s)
     +}
     +
      /* returns the length of intial segment of alpha characters only */
    - static size_t get_alpha_len(char *fixup_message) {
    + static size_t skip_suboption(char *fixup_message) {
      	const char alphas[] = "abcdefghijklmnopqrstuvwxyz";
     @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
    - 		 * reference for example: --fixup="HEAD^{/^area: string}" or
    - 		 * a suboption of `--fixup`.
    + 		 * We limit --fixup's suboptions to only alpha characters.
    + 		 * If the first character after a run of alpha is colon,
    + 		 * then the part before the colon may be a known suboption
    +-		 * name `amend` or a misspelt suboption name. In this case,
    +-		 * we treat it as --fixup=<suboption>:<arg>.
    ++		 * name like `amend` or `reword`, or a misspelt suboption
    ++		 * name. In either case, we treat it as
    ++		 * --fixup=<suboption>:<arg>.
      		 *
    --		 * 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 ':'.
    + 		 * Otherwise, we are dealing with --fixup=<commit>.
      		 */
    - 		size_t len = get_alpha_len(fixup_message);
    +@@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
      		if (len && fixup_message[len] == ':') {
      			fixup_message[len++] = '\0';
      			fixup_commit = fixup_message + len;
3:  41297d7e8e = 4:  1a127dc0b3 t7500: add tests for --fixup=[amend|reword] options
4:  7ae75dc27d = 5:  be6f4fa0d1 t3437: use --fixup with options to create amend! commit
5:  533fed36ca ! 6:  79c098df2c doc/git-commit: add documentation for fixup=[amend|reword] options
    @@ Commit message
         Mentored-by: Christian Couder <chriscool@tuxfamily.org>
         Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
         Helped-by: Junio C Hamano <gitster@pobox.com>
    -    Helped-by: Eric Sunshine <sunshine@sunshineco.com>
    +    Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
         Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>

      ## Documentation/git-commit.txt ##
    @@ Documentation/git-commit.txt: OPTIONS
     -	commit with a prefix of "fixup! ".  See linkgit:git-rebase[1]
     -	for details.
     +--fixup=[(amend|reword):]<commit>::
    -+	Construct a new commit for use with `rebase --autosquash`,
    -+	which fixes the specified commit. The plain form
    -+	`--fixup=<commit>` creates a "fixup!" commit, that allows
    -+	to fixup only the content of the specified commit and leave
    -+	it's commit log message untouched. When used with `amend:`
    -+	or `reword:`, it creates "amend!" commit that is like "fixup!"
    -+	commit but it allows to fixup both the content and the commit
    -+	log message of the specified commit. The commit log message of
    -+	the specified commit is fixed implicitly by replacing it with
    -+	the "amend!" commit's message body upon `rebase --autosquash`.
    ++	Create a new commit which "fixes up" `<commit>` when applied with
    ++	`git rebase --autosquash`. Plain `--fixup=<commit>` creates a
    ++	"fixup!" commit which changes the content of `<commit>` but leaves
    ++	its log message untouched. `--fixup=amend:<commit>` is similar but
    ++	creates an "amend!" commit which also replaces the log message of
    ++	`<commit>` with the log message of the "amend!" commit.
    ++	`--fixup=reword:<commit>` creates an "amend!" commit which
    ++	replaces the log message of `<commit>` with its own log message
    ++	but makes no changes to the content of `<commit>`.
     ++
    -+The resulting "fixup!" commit message will be the subject line
    -+from the specified commit with a prefix of "fixup!". Can be used
    -+with additional commit message option `-m`.
    ++The commit created by plain `--fixup=<commit>` has a subject
    ++composed of "fixup!" followed by the subject line from <commit>,
    ++and is recognized specially by `git rebase --autosquash`. The `-m`
    ++option may be used to supplement the log message of the created
    ++commit, but the additional commentary will be thrown away once the
    ++"fixup!" commit is squashed into `<commit>` by
    ++`git rebase --autosquash`.
     ++
    -+The `--fixup=amend:<commit>` form creates an "amend!" commit where
    -+its 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. It refuses to create "amend!" commit if it's
    -+commit message body is empty unless used with the
    -+`--allow-empty-message` option.
    ++The commit created by `--fixup=amend:<commit>` is similar but its
    ++subject is instead prefixed with "amend!". The log message of
    ++<commit> is copied into the log message of the "amend!" commit and
    ++opened in an editor so it can be refined. When `git rebase
    ++--autosquash` squashes the "amend!" commit into `<commit>`, the
    ++log message of `<commit>` is replaced by the refined log message
    ++from the "amend!" commit. It is an error for the "amend!" commit's
    ++log message to be empty unless `--allow-empty-message` is
    ++specified.
     ++
    -+The `--fixup=reword:<commit>` aliases `--fixup=amend:<commit> --only`
    -+and it also creates an "amend!" commit, but here it records the same
    -+tree as `HEAD`, 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=reword:<commit>` is shorthand for `--fixup=amend:<commit>
    ++--only`. It creates an "amend!" commit with only a log message
    ++(ignoring any changes staged in the index). When squashed by `git
    ++rebase --autosquash`, it replaces the log message of `<commit>`
    ++without making any other changes.
     ++
    -+Also, after fixing the commit using `--fixup`, with or without option
    -+and rebased with `--autosquash`, the authorship of the original commit
    -+remains unchanged. See linkgit:git-rebase[1] for details.
    ++Neither "fixup!" nor "amend!" commits change authorship of
    ++`<commit>` when applied by `git rebase --autosquash`.
    ++See linkgit:git-rebase[1] for details.

      --squash=<commit>::
      	Construct a commit message for use with `rebase --autosquash`.
--
2.29.0.rc1

[PATCH v5 1/6] sequencer: export and rename subject_length()

[Charvi Mendiratta]

This function can be used in other parts of git. Let's move the
function to commit.c and also rename it to make the name of the
function more generic.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 commit.c    | 14 ++++++++++++++
 commit.h    |  3 +++
 sequencer.c | 16 +---------------
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/commit.c b/commit.c
index bab8d5ab07..199c7e274c 100644
--- a/commit.c
+++ b/commit.c
@@ -535,6 +535,20 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
 	return eol - p;
 }
 
+size_t commit_subject_length(const char *body)
+{
+	const char *p = body;
+	while (*p) {
+		const char *next = skip_blank_lines(p);
+		if (next != p)
+			break;
+		p = strchrnul(p, '\n');
+		if (*p)
+			p++;
+	}
+	return p - body;
+}
+
 struct commit_list *commit_list_insert(struct commit *item, struct commit_list **list_p)
 {
 	struct commit_list *new_list = xmalloc(sizeof(struct commit_list));
diff --git a/commit.h b/commit.h
index f4e7b0158e..e65d9b3e75 100644
--- a/commit.h
+++ b/commit.h
@@ -165,6 +165,9 @@ const void *detach_commit_buffer(struct commit *, unsigned long *sizep);
 /* Find beginning and length of commit subject. */
 int find_commit_subject(const char *commit_buffer, const char **subject);
 
+/* Return length of the commit subject from commit log message. */
+size_t commit_subject_length(const char *body);
+
 struct commit_list *commit_list_insert(struct commit *item,
 					struct commit_list **list);
 int commit_list_contains(struct commit *item,
diff --git a/sequencer.c b/sequencer.c
index abc6d5cdfd..da7d54c7ce 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1724,20 +1724,6 @@ enum todo_item_flags {
 	TODO_EDIT_FIXUP_MSG    = (1 << 2),
 };
 
-static size_t subject_length(const char *body)
-{
-	const char *p = body;
-	while (*p) {
-		const char *next = skip_blank_lines(p);
-		if (next != p)
-			break;
-		p = strchrnul(p, '\n');
-		if (*p)
-			p++;
-	}
-	return p - body;
-}
-
 static const char first_commit_msg_str[] = N_("This is the 1st commit message:");
 static const char nth_commit_msg_fmt[] = N_("This is the commit message #%d:");
 static const char skip_first_commit_msg_str[] = N_("The 1st commit message will be skipped:");
@@ -1861,7 +1847,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,
 	if (starts_with(body, "amend!") ||
 	    ((command == TODO_SQUASH || seen_squash(opts)) &&
 	     (starts_with(body, "squash!") || starts_with(body, "fixup!"))))
-		commented_len = subject_length(body);
+		commented_len = commit_subject_length(body);
 
 	strbuf_addf(buf, "\n%c ", comment_line_char);
 	strbuf_addf(buf, _(nth_commit_msg_fmt),
-- 
2.29.0.rc1

[PATCH v5 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

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

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.

In order to prevent rebase from creating commits with an empty
message we refuse to create an "amend!" commit if commit message
body is empty.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 builtin/commit.c | 107 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 97 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 505fe60956..55712fdc7b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -105,7 +105,8 @@ static const char *template_file;
  */
 static const char *author_message, *author_message_buffer;
 static char *edit_message, *use_message;
-static char *fixup_message, *squash_message;
+static char *fixup_message, *fixup_commit, *squash_message;
+static const char *fixup_prefix;
 static int all, also, interactive, patch_interactive, only, amend, signoff;
 static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
@@ -357,7 +358,8 @@ static const char *prepare_index(const char **argv, const char *prefix,
 		die(_("--pathspec-file-nul requires --pathspec-from-file"));
 	}
 
-	if (!pathspec.nr && (also || (only && !amend && !allow_empty)))
+	if (!pathspec.nr && (also || (only && !allow_empty &&
+	    (!amend || (fixup_message && strcmp(fixup_prefix, "amend"))))))
 		die(_("No paths with --include/--only does not make sense."));
 
 	if (read_cache_preload(&pathspec) < 0)
@@ -681,6 +683,22 @@ static void adjust_comment_line_char(const struct strbuf *sb)
 	comment_line_char = *p;
 }
 
+static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
+				struct pretty_print_context *ctx)
+{
+	const char *buffer, *subject, *fmt;
+
+	buffer = get_commit_buffer(commit, NULL);
+	find_commit_subject(buffer, &subject);
+	/*
+	 * If we amend the 'amend!' commit then we don't want to
+	 * duplicate the subject line.
+	 */
+	fmt = starts_with(subject, "amend!") ? "%b" : "%B";
+	format_commit_message(commit, fmt, sb, ctx);
+	unuse_commit_buffer(commit, buffer);
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct commit *current_head,
 			     struct wt_status *s,
@@ -745,15 +763,33 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	} else if (fixup_message) {
 		struct pretty_print_context ctx = {0};
 		struct commit *commit;
-		commit = lookup_commit_reference_by_name(fixup_message);
+		char *fmt;
+		commit = lookup_commit_reference_by_name(fixup_commit);
 		if (!commit)
-			die(_("could not lookup commit %s"), fixup_message);
+			die(_("could not lookup commit %s"), fixup_commit);
 		ctx.output_encoding = get_commit_output_encoding();
-		format_commit_message(commit, "fixup! %s\n\n",
-				      &sb, &ctx);
-		if (have_option_m)
-			strbuf_addbuf(&sb, &message);
+		fmt = xstrfmt("%s! %%s\n\n", fixup_prefix);
+		format_commit_message(commit, fmt, &sb, &ctx);
+		free(fmt);
 		hook_arg1 = "message";
+
+		/*
+		 * Only `-m` commit message option is checked here, as
+		 * it supports `--fixup` to append the commit message.
+		 *
+		 * The other commit message options `-c`/`-C`/`-F` are
+		 * incompatible with all the forms of `--fixup` and
+		 * have already errored out while parsing the `git commit`
+		 * options.
+		 */
+		if (have_option_m && !strcmp(fixup_prefix, "fixup"))
+			strbuf_addbuf(&sb, &message);
+
+		if (!strcmp(fixup_prefix, "amend")) {
+			if (have_option_m)
+				die(_("cannot combine -m with --fixup:%s"), fixup_message);
+			prepare_amend_commit(commit, &sb, &ctx);
+		}
 	} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
 		size_t merge_msg_start;
 
@@ -1152,6 +1188,12 @@ static void finalize_deferred_config(struct wt_status *s)
 		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
 }
 
+/* returns the length of intial segment of alpha characters only */
+static size_t skip_suboption(char *fixup_message) {
+	const char alphas[] = "abcdefghijklmnopqrstuvwxyz";
+	return strspn(fixup_message, alphas);
+}
+
 static int parse_and_validate_options(int argc, const char *argv[],
 				      const struct option *options,
 				      const char * const usage[],
@@ -1170,7 +1212,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (force_author && renew_authorship)
 		die(_("Using both --reset-author and --author does not make sense"));
 
-	if (logfile || have_option_m || use_message || fixup_message)
+	if (logfile || have_option_m || use_message)
 		use_editor = 0;
 	if (0 <= edit_flag)
 		use_editor = edit_flag;
@@ -1227,6 +1269,34 @@ static int parse_and_validate_options(int argc, const char *argv[],
 
 	if (also + only + all + interactive > 1)
 		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
+
+	if (fixup_message) {
+		/*
+		 * We limit --fixup's suboptions to only alpha characters.
+		 * If the first character after a run of alpha is colon,
+		 * then the part before the colon may be a known suboption
+		 * name `amend` or a misspelt suboption name. In this case,
+		 * we treat it as --fixup=<suboption>:<arg>.
+		 *
+		 * Otherwise, we are dealing with --fixup=<commit>.
+		 */
+		size_t len = skip_suboption(fixup_message);
+		if (len && fixup_message[len] == ':') {
+			fixup_message[len++] = '\0';
+			fixup_commit = fixup_message + len;
+			if (!strcmp("amend", fixup_message)) {
+				fixup_prefix = "amend";
+				allow_empty = 1;
+			} else {
+				die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
+			}
+		} else {
+			fixup_commit = fixup_message;
+			fixup_prefix = "fixup";
+			use_editor = 0;
+		}
+	}
+
 	cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
 
 	handle_untracked_files_arg(s);
@@ -1504,7 +1574,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK('m', "message", &message, N_("message"), N_("commit message"), opt_parse_m),
 		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")),
-		OPT_STRING(0, "fixup", &fixup_message, N_("commit"), N_("use autosquash formatted message to fixup specified commit")),
+		/*
+		 * TRANSLATORS: Leave "[amend:]" as-is, and
+		 * only translate <commit>.
+		 */
+		OPT_STRING(0, "fixup", &fixup_message, N_("[amend:]commit"), N_("use autosquash formatted message to fixup or amend 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")),
@@ -1663,6 +1737,19 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		exit(1);
 	}
 
+	if (fixup_message && starts_with(sb.buf, "amend! ") &&
+	    !allow_empty_message) {
+		struct strbuf body = STRBUF_INIT;
+		size_t len = commit_subject_length(sb.buf);
+		strbuf_addstr(&body, sb.buf + len);
+		if (message_is_empty(&body, cleanup_mode)) {
+			rollback_index_files();
+			fprintf(stderr, _("Aborting commit due to empty commit message body.\n"));
+			exit(1);
+		}
+		strbuf_release(&body);
+	}
+
 	if (amend) {
 		const char *exclude_gpgsig[3] = { "gpgsig", "gpgsig-sha256", NULL };
 		extra = read_commit_extra_headers(current_head, exclude_gpgsig);
-- 
2.29.0.rc1

Re: [PATCH v5 2/6] commit: add amend suboption to --fixup to create amend! commit

[Eric Sunshine]

On Sat, Mar 13, 2021 at 8:42 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> `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.
>
> 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.
>
> In order to prevent rebase from creating commits with an empty
> message we refuse to create an "amend!" commit if commit message
> body is empty.
>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
> diff --git a/builtin/commit.c b/builtin/commit.c
> @@ -1152,6 +1188,12 @@ static void finalize_deferred_config(struct wt_status *s)
> +/* returns the length of intial segment of alpha characters only */
> +static size_t skip_suboption(char *fixup_message) {
> +       const char alphas[] = "abcdefghijklmnopqrstuvwxyz";
> +       return strspn(fixup_message, alphas);
> +}

With the function name change, the comment above the function has
become outdated. Instead, it should instead explain the function's
purpose at a high-level (not at the low-level of skipping over
alpha-only characters -- especially since the alpha-only restriction
may change in the future). By high-level, I mean talking about
skipping past a token which is likely to be a --fixup suboption, and
giving the reason why the set of characters which comprise the token
is limited (perhaps citing the example Junio had given earlier in
which the code should not mistakenly scan too far to a legitimate ":"
in <commit>).

> @@ -1227,6 +1269,34 @@ static int parse_and_validate_options(int argc, const char *argv[],
> +       if (fixup_message) {
> +               /*
> +                * We limit --fixup's suboptions to only alpha characters.
> +                * If the first character after a run of alpha is colon,
> +                * then the part before the colon may be a known suboption
> +                * name `amend` or a misspelt suboption name. In this case,
> +                * we treat it as --fixup=<suboption>:<arg>.
> +                *
> +                * Otherwise, we are dealing with --fixup=<commit>.
> +                */

This comment is also now out of date following the function name
change. It no longer makes sense for this comment to talk about
skipping alpha-only characters; it should be written at a semantically
higher level, talking instead about skipping the suboption prefix (or
something) since that's what the function call is all about.

> +               size_t len = skip_suboption(fixup_message);
> +               if (len && fixup_message[len] == ':') {
> +                       fixup_message[len++] = '\0';
> +                       fixup_commit = fixup_message + len;
> +                       if (!strcmp("amend", fixup_message)) {
> +                               fixup_prefix = "amend";
> +                               allow_empty = 1;
> +                       } else {
> +                               die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
> +                       }

These are quite minor issues, not necessarily worth a re-roll. (It
might be perfectly fine to send a patch later on which addresses these
issues.)

[PATCH v5 3/6] commit: add a reword suboption to --fixup

[Charvi Mendiratta]

`git commit --fixup=reword:<commit>` aliases
`--fixup=amend:<commit> --only`, where it creates an empty "amend!"
commit that will reword <commit> without changing its contents when
it is rebased with `--autosquash`.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 builtin/commit.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 55712fdc7b..749d32205a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1188,6 +1188,19 @@ static void finalize_deferred_config(struct wt_status *s)
 		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
 }
 
+static void check_fixup_reword_options(int argc, const char *argv[]) {
+	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 path '%s'"), *argv);
+	if (patch_interactive || interactive || all || also || only)
+		die(_("reword option of --fixup is mutually exclusive with --patch/--interactive/--all/--include/--only"));
+}
+
 /* returns the length of intial segment of alpha characters only */
 static size_t skip_suboption(char *fixup_message) {
 	const char alphas[] = "abcdefghijklmnopqrstuvwxyz";
@@ -1275,8 +1288,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		 * We limit --fixup's suboptions to only alpha characters.
 		 * If the first character after a run of alpha is colon,
 		 * then the part before the colon may be a known suboption
-		 * name `amend` or a misspelt suboption name. In this case,
-		 * we treat it as --fixup=<suboption>:<arg>.
+		 * name like `amend` or `reword`, or a misspelt suboption
+		 * name. In either case, we treat it as
+		 * --fixup=<suboption>:<arg>.
 		 *
 		 * Otherwise, we are dealing with --fixup=<commit>.
 		 */
@@ -1284,9 +1298,14 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		if (len && fixup_message[len] == ':') {
 			fixup_message[len++] = '\0';
 			fixup_commit = fixup_message + len;
-			if (!strcmp("amend", fixup_message)) {
+			if (!strcmp("amend", fixup_message) ||
+			    !strcmp("reword", fixup_message)) {
 				fixup_prefix = "amend";
 				allow_empty = 1;
+				if (*fixup_message == 'r') {
+					check_fixup_reword_options(argc, argv);
+					only = 1;
+				}
 			} else {
 				die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
 			}
@@ -1575,10 +1594,10 @@ 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: Leave "[amend:]" as-is, and
-		 * only translate <commit>.
+		 * TRANSLATORS: Leave "[(amend|reword):]" as-is,
+		 * and only translate <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")),
-- 
2.29.0.rc1

[PATCH v5 4/6] t7500: add tests for --fixup=[amend|reword] options

[Charvi Mendiratta]

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t7500-commit-template-squash-signoff.sh | 159 ++++++++++++++++++++++
 1 file changed, 159 insertions(+)

diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 6d19ece05d..05e43a31d3 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -9,6 +9,8 @@ Tests for template, signoff, squash and -F functions.'
 
 . ./test-lib.sh
 
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
 commit_msg_is () {
 	expect=commit_msg_is.expect
 	actual=commit_msg_is.actual
@@ -279,6 +281,163 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
 
 extra"
 '
+get_commit_msg () {
+	rev="$1" &&
+	git log -1 --pretty=format:"%B" "$rev"
+}
+
+test_expect_success 'commit --fixup=amend: creates amend! commit' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited" \
+			git commit --fixup=amend:HEAD~
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--fixup=amend: --only ignores staged changes' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited" \
+			git commit --fixup=amend:HEAD~ --only
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual &&
+	test_cmp_rev HEAD@{1}^{tree} HEAD^{tree} &&
+	test_cmp_rev HEAD@{1} HEAD^ &&
+	test_expect_code 1 git diff --cached --exit-code &&
+	git cat-file blob :foo >actual &&
+	test_cmp foo actual
+'
+
+test_expect_success '--fixup=reword: ignores staged changes' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited" \
+			git commit --fixup=reword:HEAD~
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual &&
+	test_cmp_rev HEAD@{1}^{tree} HEAD^{tree} &&
+	test_cmp_rev HEAD@{1} HEAD^ &&
+	test_expect_code 1 git diff --cached --exit-code &&
+	git cat-file blob :foo >actual &&
+	test_cmp foo actual
+'
+
+test_expect_success '--fixup=reword: error out with -m option' '
+	commit_for_rebase_autosquash_setup &&
+	echo "fatal: cannot combine -m with --fixup:reword" >expect &&
+	test_must_fail git commit --fixup=reword:HEAD~ -m "reword commit message" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--fixup=amend: error out with -m option' '
+	commit_for_rebase_autosquash_setup &&
+	echo "fatal: cannot combine -m with --fixup:amend" >expect &&
+	test_must_fail git commit --fixup=amend:HEAD~ -m "amend commit message" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'consecutive amend! commits remove amend! line from commit msg body' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited 1
+
+	edited 2
+	EOF
+	echo "reword new commit message" >actual &&
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited 1" \
+			git commit --fixup=reword:HEAD~ &&
+		FAKE_COMMIT_AMEND="edited 2" \
+			git commit --fixup=reword:HEAD
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'deny to create amend! commit if its commit msg body is empty' '
+	commit_for_rebase_autosquash_setup &&
+	echo "Aborting commit due to empty commit message body." >expected &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_COMMIT_MESSAGE="amend! target message subject line" \
+			git commit --fixup=amend:HEAD~ 2>actual
+	) &&
+	test_cmp expected actual
+'
+
+test_expect_success 'amend! commit allows empty commit msg body with --allow-empty-message' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_MESSAGE="amend! target message subject line" \
+			git commit --fixup=amend:HEAD~ --allow-empty-message &&
+		get_commit_msg HEAD >actual
+	) &&
+	test_cmp expected actual
+'
+
+test_fixup_reword_opt () {
+	test_expect_success C_LOCALE_OUTPUT "--fixup=reword: incompatible with $1" "
+		echo 'fatal: reword option of --fixup is mutually exclusive with'\
+			'--patch/--interactive/--all/--include/--only' >expect &&
+		test_must_fail git commit --fixup=reword:HEAD~ $1 2>actual &&
+		test_cmp expect actual
+	"
+}
+
+for opt in --all --include --only --interactive --patch
+do
+	test_fixup_reword_opt $opt
+done
+
+test_expect_success '--fixup=reword: give error with pathsec' '
+	commit_for_rebase_autosquash_setup &&
+	echo "fatal: cannot combine reword option of --fixup with path '\''foo'\''" >expect &&
+	test_must_fail git commit --fixup=reword:HEAD~ -- foo 2>actual &&
+	test_cmp expect actual
+'
+
+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
+'
 
 test_expect_success 'commit --squash works with -F' '
 	commit_for_rebase_autosquash_setup &&
-- 
2.29.0.rc1

[PATCH v5 5/6] t3437: use --fixup with options to create amend! commit

[Charvi Mendiratta]

We taught `git commit --fixup` to create "amend!" commit. Let's also
update the tests and use it to setup the rebase tests.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t3437-rebase-fixup-options.sh | 30 +++---------------------------
 1 file changed, 3 insertions(+), 27 deletions(-)

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index a5a20354e3..d0bdc7ed02 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -72,40 +72,16 @@ test_expect_success 'setup' '
 	git commit --fixup=HEAD -a &&
 	git tag B1 &&
 	test_tick &&
-	git commit --allow-empty -F - <<-EOF &&
-	amend! B
-	$EMPTY
-	B
-	$EMPTY
-	edited 1
-	EOF
+	FAKE_COMMIT_AMEND="edited 1" git commit --fixup=reword:B &&
 	test_tick &&
-	git commit --allow-empty -F - <<-EOF &&
-	amend! amend! B
-	$EMPTY
-	B
-	$EMPTY
-	edited 1
-	$EMPTY
-	edited 2
-	EOF
+	FAKE_COMMIT_AMEND="edited 2" git commit --fixup=reword:HEAD &&
 	echo B2 >B &&
 	test_tick &&
 	FAKE_COMMIT_AMEND="edited squash" git commit --squash=HEAD -a &&
 	git tag B2 &&
 	echo B3 >B &&
 	test_tick &&
-	git commit -a -F - <<-EOF &&
-	amend! amend! amend! B
-	$EMPTY
-	B
-	$EMPTY
-	edited 1
-	$EMPTY
-	edited 2
-	$EMPTY
-	edited 3
-	EOF
+	FAKE_COMMIT_AMEND="edited 3" git commit -a --fixup=amend:HEAD^ &&
 	git tag B3 &&
 
 	GIT_AUTHOR_NAME="Rebase Author" &&
-- 
2.29.0.rc1

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

[Charvi Mendiratta]

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: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 Documentation/git-commit.txt | 45 +++++++++++++++++++++++++++++++-----
 Documentation/git-rebase.txt | 21 +++++++++--------
 2 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 17150fa7ea..3c69f461c9 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,44 @@ 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>::
+	Create a new commit which "fixes up" `<commit>` when applied with
+	`git rebase --autosquash`. Plain `--fixup=<commit>` creates a
+	"fixup!" commit which changes the content of `<commit>` but leaves
+	its log message untouched. `--fixup=amend:<commit>` is similar but
+	creates an "amend!" commit which also replaces the log message of
+	`<commit>` with the log message of the "amend!" commit.
+	`--fixup=reword:<commit>` creates an "amend!" commit which
+	replaces the log message of `<commit>` with its own log message
+	but makes no changes to the content of `<commit>`.
++
+The commit created by plain `--fixup=<commit>` has a subject
+composed of "fixup!" followed by the subject line from <commit>,
+and is recognized specially by `git rebase --autosquash`. The `-m`
+option may be used to supplement the log message of the created
+commit, but the additional commentary will be thrown away once the
+"fixup!" commit is squashed into `<commit>` by
+`git rebase --autosquash`.
++
+The commit created by `--fixup=amend:<commit>` is similar but its
+subject is instead prefixed with "amend!". The log message of
+<commit> is copied into the log message of the "amend!" commit and
+opened in an editor so it can be refined. When `git rebase
+--autosquash` squashes the "amend!" commit into `<commit>`, the
+log message of `<commit>` is replaced by the refined log message
+from the "amend!" commit. It is an error for the "amend!" commit's
+log message to be empty unless `--allow-empty-message` is
+specified.
++
+`--fixup=reword:<commit>` is shorthand for `--fixup=amend:<commit>
+--only`. It creates an "amend!" commit with only a log message
+(ignoring any changes staged in the index). When squashed by `git
+rebase --autosquash`, it replaces the log message of `<commit>`
+without making any other changes.
++
+Neither "fixup!" nor "amend!" commits change authorship of
+`<commit>` when applied by `git rebase --autosquash`.
+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..f08ae27e2a 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/amend/squash
+	commits is by using the `--fixup`, `--fixup=amend:` or `--fixup=reword:`
+	and `--squash` options respectively of linkgit:git-commit[1].
 +
 If the `--autosquash` option is enabled by default using the
 configuration variable `rebase.autoSquash`, this option can be
-- 
2.29.0.rc1

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

[Eric Sunshine]

On Sat, Mar 13, 2021 at 8:43 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>

It may have been more accurate to retain the Helped-by: with my name
while adding the Signed-off-by: (which I offered in case you
incorporated my significant rewrites), rather than replacing the
Helped-by: altogether. Not worth a re-roll, though.

> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> @@ -86,11 +86,44 @@ OPTIONS
> +--fixup=[(amend|reword):]<commit>::
> +       Create a new commit which "fixes up" `<commit>` when applied with
> +       `git rebase --autosquash`. Plain `--fixup=<commit>` creates a
> +       "fixup!" commit which changes the content of `<commit>` but leaves
> +       its log message untouched. `--fixup=amend:<commit>` is similar but
> +       creates an "amend!" commit which also replaces the log message of
> +       `<commit>` with the log message of the "amend!" commit.
> +       `--fixup=reword:<commit>` creates an "amend!" commit which
> +       replaces the log message of `<commit>` with its own log message
> +       but makes no changes to the content of `<commit>`.
> ++
> +The commit created by plain `--fixup=<commit>` has a subject
> +composed of "fixup!" followed by the subject line from <commit>,
> +and is recognized specially by `git rebase --autosquash`. The `-m`
> +option may be used to supplement the log message of the created
> +commit, but the additional commentary will be thrown away once the
> +"fixup!" commit is squashed into `<commit>` by
> +`git rebase --autosquash`.
> ++
> +The commit created by `--fixup=amend:<commit>` is similar but its
> +subject is instead prefixed with "amend!". The log message of
> +<commit> is copied into the log message of the "amend!" commit and
> +opened in an editor so it can be refined. When `git rebase
> +--autosquash` squashes the "amend!" commit into `<commit>`, the
> +log message of `<commit>` is replaced by the refined log message
> +from the "amend!" commit. It is an error for the "amend!" commit's
> +log message to be empty unless `--allow-empty-message` is
> +specified.
> ++
> +`--fixup=reword:<commit>` is shorthand for `--fixup=amend:<commit>
> +--only`. It creates an "amend!" commit with only a log message
> +(ignoring any changes staged in the index). When squashed by `git
> +rebase --autosquash`, it replaces the log message of `<commit>`
> +without making any other changes.
> ++
> +Neither "fixup!" nor "amend!" commits change authorship of
> +`<commit>` when applied by `git rebase --autosquash`.
> +See linkgit:git-rebase[1] for details.

I see that you took my entire rewrite verbatim. That's fine. My bias
is probably showing, but I do now find this documentation patch easier
to understand.

Thanks.

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

[Charvi Mendiratta]

On Sun, 14 Mar 2021 at 06:40, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sat, Mar 13, 2021 at 8:43 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> > Helped-by: Junio C Hamano <gitster@pobox.com>
> > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
>
> It may have been more accurate to retain the Helped-by: with my name
> while adding the Signed-off-by: (which I offered in case you
> incorporated my significant rewrites), rather than replacing the
> Helped-by: altogether. Not worth a re-roll, though.
>

Okay I will add that too, thanks.

[PATCH v6 0/6][Outreachy] commit: Implementation of "amend!" commit

[Charvi Mendiratta]

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. `--fixup=reword:<commit>` is a short-hand of
`--fixup=amend:<commit> --only`.
Example usuage:
git commit --fixup=reword:<commit>

** This work is rebased on the top of cm/rebase-i-updates.

Charvi Mendiratta (6):
  sequencer: export and rename subject_length()
  commit: add amend suboption to --fixup to create amend! commit
  commit: add a reword suboption to --fixup
  t7500: add tests for --fixup=[amend|reword] options
  t3437: use --fixup with options to create amend! commit
  doc/git-commit: add documentation for fixup=[amend|reword] options

 Documentation/git-commit.txt              |  45 +++++-
 Documentation/git-rebase.txt              |  21 +--
 builtin/commit.c                          | 122 +++++++++++++++--
 commit.c                                  |  14 ++
 commit.h                                  |   3 +
 sequencer.c                               |  16 +--
 t/t3437-rebase-fixup-options.sh           |  30 +---
 t/t7500-commit-template-squash-signoff.sh | 159 ++++++++++++++++++++++
 8 files changed, 342 insertions(+), 68 deletions(-)

Range-diff against v5:
-:  ---------- > 1:  a2e89540ec sequencer: export and rename subject_length()
1:  be2808a255 ! 2:  f3cdb3eb1e commit: add amend suboption to --fixup to create amend! commit
    @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const cha
      	} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
      		size_t merge_msg_start;

    -@@ builtin/commit.c: static void finalize_deferred_config(struct wt_status *s)
    - 		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
    - }
    -
    -+/* returns the length of intial segment of alpha characters only */
    -+static size_t skip_suboption(char *fixup_message) {
    -+	const char alphas[] = "abcdefghijklmnopqrstuvwxyz";
    -+	return strspn(fixup_message, alphas);
    -+}
    -+
    - static int parse_and_validate_options(int argc, const char *argv[],
    - 				      const struct option *options,
    - 				      const char * const usage[],
     @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
      	if (force_author && renew_authorship)
      		die(_("Using both --reset-author and --author does not make sense"));
    @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar
     +		 *
     +		 * Otherwise, we are dealing with --fixup=<commit>.
     +		 */
    -+		size_t len = skip_suboption(fixup_message);
    -+		if (len && fixup_message[len] == ':') {
    -+			fixup_message[len++] = '\0';
    -+			fixup_commit = fixup_message + len;
    ++		char *p = fixup_message;
    ++		while (isalpha(*p))
    ++			p++;
    ++		if (p > fixup_message && *p == ':') {
    ++			*p = '\0';
    ++			fixup_commit = p + 1;
     +			if (!strcmp("amend", fixup_message)) {
     +				fixup_prefix = "amend";
     +				allow_empty = 1;
2:  f6217338c1 ! 3:  ded339706f commit: add a reword suboption to --fixup
    @@ builtin/commit.c: static void finalize_deferred_config(struct wt_status *s)
     +		die(_("reword option of --fixup is mutually exclusive with --patch/--interactive/--all/--include/--only"));
     +}
     +
    - /* returns the length of intial segment of alpha characters only */
    - static size_t skip_suboption(char *fixup_message) {
    - 	const char alphas[] = "abcdefghijklmnopqrstuvwxyz";
    + static int parse_and_validate_options(int argc, const char *argv[],
    + 				      const struct option *options,
    + 				      const char * const usage[],
     @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
      		 * We limit --fixup's suboptions to only alpha characters.
      		 * If the first character after a run of alpha is colon,
    @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar
      		 * Otherwise, we are dealing with --fixup=<commit>.
      		 */
     @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
    - 		if (len && fixup_message[len] == ':') {
    - 			fixup_message[len++] = '\0';
    - 			fixup_commit = fixup_message + len;
    + 		if (p > fixup_message && *p == ':') {
    + 			*p = '\0';
    + 			fixup_commit = p + 1;
     -			if (!strcmp("amend", fixup_message)) {
     +			if (!strcmp("amend", fixup_message) ||
     +			    !strcmp("reword", fixup_message)) {
3:  1a127dc0b3 = 4:  ec6f3d5d7d t7500: add tests for --fixup=[amend|reword] options
4:  be6f4fa0d1 = 5:  148087b133 t3437: use --fixup with options to create amend! commit
5:  79c098df2c ! 6:  2b750d305e doc/git-commit: add documentation for fixup=[amend|reword] options
    @@ Commit message

         Mentored-by: Christian Couder <chriscool@tuxfamily.org>
         Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
    +    Helped-by: Eric Sunshine <sunshine@sunshineco.com>
         Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
         Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
--
2.29.0.rc1

Re: [PATCH v6 0/6][Outreachy] commit: Implementation of "amend!" commit

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

> 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. `--fixup=reword:<commit>` is a short-hand of
> `--fixup=amend:<commit> --only`.
> Example usuage:
> git commit --fixup=reword:<commit>
>
> ** This work is rebased on the top of cm/rebase-i-updates.

Are people happy with this round?  I gave an extra round of
eyeballing and didn't see anything suspicious, so I am planning to
merge it to 'next' soonish once the dust settles post release.

Thanks.

Re: [PATCH v6 0/6][Outreachy] commit: Implementation of "amend!" commit

[Eric Sunshine]

On Thu, Mar 18, 2021 at 8:53 PM Junio C Hamano <gitster@pobox.com> wrote:
> Charvi Mendiratta <charvi077@gmail.com> writes:
> > Example usuage:
> > git commit --fixup=amend:<commit>
>
> Are people happy with this round?  I gave an extra round of
> eyeballing and didn't see anything suspicious, so I am planning to
> merge it to 'next' soonish once the dust settles post release.

I gave the series a final read-through when v6 was posted and didn't
find anything about which to comment. I think all my review comments
on earlier versions have been addressed, so I have no objection to
seeing it merged to 'next'.

Re: [PATCH v6 0/6][Outreachy] commit: Implementation of "amend!" commit

[Charvi Mendiratta]

On Fri, 19 Mar 2021 at 08:46, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Thu, Mar 18, 2021 at 8:53 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Charvi Mendiratta <charvi077@gmail.com> writes:
> > > Example usuage:
> > > git commit --fixup=amend:<commit>
> >
> > Are people happy with this round?  I gave an extra round of
> > eyeballing and didn't see anything suspicious, so I am planning to
> > merge it to 'next' soonish once the dust settles post release.
>
> I gave the series a final read-through when v6 was posted and didn't
> find anything about which to comment. I think all my review comments
> on earlier versions have been addressed, so I have no objection to
> seeing it merged to 'next'.

I too agree, thanks for confirming.

Thanks and Regards,
Charvi

[PATCH v6 1/6] sequencer: export and rename subject_length()

[Charvi Mendiratta]

This function can be used in other parts of git. Let's move the
function to commit.c and also rename it to make the name of the
function more generic.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 commit.c    | 14 ++++++++++++++
 commit.h    |  3 +++
 sequencer.c | 16 +---------------
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/commit.c b/commit.c
index bab8d5ab07..199c7e274c 100644
--- a/commit.c
+++ b/commit.c
@@ -535,6 +535,20 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
 	return eol - p;
 }
 
+size_t commit_subject_length(const char *body)
+{
+	const char *p = body;
+	while (*p) {
+		const char *next = skip_blank_lines(p);
+		if (next != p)
+			break;
+		p = strchrnul(p, '\n');
+		if (*p)
+			p++;
+	}
+	return p - body;
+}
+
 struct commit_list *commit_list_insert(struct commit *item, struct commit_list **list_p)
 {
 	struct commit_list *new_list = xmalloc(sizeof(struct commit_list));
diff --git a/commit.h b/commit.h
index f4e7b0158e..e65d9b3e75 100644
--- a/commit.h
+++ b/commit.h
@@ -165,6 +165,9 @@ const void *detach_commit_buffer(struct commit *, unsigned long *sizep);
 /* Find beginning and length of commit subject. */
 int find_commit_subject(const char *commit_buffer, const char **subject);
 
+/* Return length of the commit subject from commit log message. */
+size_t commit_subject_length(const char *body);
+
 struct commit_list *commit_list_insert(struct commit *item,
 					struct commit_list **list);
 int commit_list_contains(struct commit *item,
diff --git a/sequencer.c b/sequencer.c
index abc6d5cdfd..da7d54c7ce 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1724,20 +1724,6 @@ enum todo_item_flags {
 	TODO_EDIT_FIXUP_MSG    = (1 << 2),
 };
 
-static size_t subject_length(const char *body)
-{
-	const char *p = body;
-	while (*p) {
-		const char *next = skip_blank_lines(p);
-		if (next != p)
-			break;
-		p = strchrnul(p, '\n');
-		if (*p)
-			p++;
-	}
-	return p - body;
-}
-
 static const char first_commit_msg_str[] = N_("This is the 1st commit message:");
 static const char nth_commit_msg_fmt[] = N_("This is the commit message #%d:");
 static const char skip_first_commit_msg_str[] = N_("The 1st commit message will be skipped:");
@@ -1861,7 +1847,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,
 	if (starts_with(body, "amend!") ||
 	    ((command == TODO_SQUASH || seen_squash(opts)) &&
 	     (starts_with(body, "squash!") || starts_with(body, "fixup!"))))
-		commented_len = subject_length(body);
+		commented_len = commit_subject_length(body);
 
 	strbuf_addf(buf, "\n%c ", comment_line_char);
 	strbuf_addf(buf, _(nth_commit_msg_fmt),
-- 
2.29.0.rc1

[PATCH v6 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

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

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.

In order to prevent rebase from creating commits with an empty
message we refuse to create an "amend!" commit if commit message
body is empty.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 builtin/commit.c | 103 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 93 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 505fe60956..9697bba10b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -105,7 +105,8 @@ static const char *template_file;
  */
 static const char *author_message, *author_message_buffer;
 static char *edit_message, *use_message;
-static char *fixup_message, *squash_message;
+static char *fixup_message, *fixup_commit, *squash_message;
+static const char *fixup_prefix;
 static int all, also, interactive, patch_interactive, only, amend, signoff;
 static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
@@ -357,7 +358,8 @@ static const char *prepare_index(const char **argv, const char *prefix,
 		die(_("--pathspec-file-nul requires --pathspec-from-file"));
 	}
 
-	if (!pathspec.nr && (also || (only && !amend && !allow_empty)))
+	if (!pathspec.nr && (also || (only && !allow_empty &&
+	    (!amend || (fixup_message && strcmp(fixup_prefix, "amend"))))))
 		die(_("No paths with --include/--only does not make sense."));
 
 	if (read_cache_preload(&pathspec) < 0)
@@ -681,6 +683,22 @@ static void adjust_comment_line_char(const struct strbuf *sb)
 	comment_line_char = *p;
 }
 
+static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
+				struct pretty_print_context *ctx)
+{
+	const char *buffer, *subject, *fmt;
+
+	buffer = get_commit_buffer(commit, NULL);
+	find_commit_subject(buffer, &subject);
+	/*
+	 * If we amend the 'amend!' commit then we don't want to
+	 * duplicate the subject line.
+	 */
+	fmt = starts_with(subject, "amend!") ? "%b" : "%B";
+	format_commit_message(commit, fmt, sb, ctx);
+	unuse_commit_buffer(commit, buffer);
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct commit *current_head,
 			     struct wt_status *s,
@@ -745,15 +763,33 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	} else if (fixup_message) {
 		struct pretty_print_context ctx = {0};
 		struct commit *commit;
-		commit = lookup_commit_reference_by_name(fixup_message);
+		char *fmt;
+		commit = lookup_commit_reference_by_name(fixup_commit);
 		if (!commit)
-			die(_("could not lookup commit %s"), fixup_message);
+			die(_("could not lookup commit %s"), fixup_commit);
 		ctx.output_encoding = get_commit_output_encoding();
-		format_commit_message(commit, "fixup! %s\n\n",
-				      &sb, &ctx);
-		if (have_option_m)
-			strbuf_addbuf(&sb, &message);
+		fmt = xstrfmt("%s! %%s\n\n", fixup_prefix);
+		format_commit_message(commit, fmt, &sb, &ctx);
+		free(fmt);
 		hook_arg1 = "message";
+
+		/*
+		 * Only `-m` commit message option is checked here, as
+		 * it supports `--fixup` to append the commit message.
+		 *
+		 * The other commit message options `-c`/`-C`/`-F` are
+		 * incompatible with all the forms of `--fixup` and
+		 * have already errored out while parsing the `git commit`
+		 * options.
+		 */
+		if (have_option_m && !strcmp(fixup_prefix, "fixup"))
+			strbuf_addbuf(&sb, &message);
+
+		if (!strcmp(fixup_prefix, "amend")) {
+			if (have_option_m)
+				die(_("cannot combine -m with --fixup:%s"), fixup_message);
+			prepare_amend_commit(commit, &sb, &ctx);
+		}
 	} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
 		size_t merge_msg_start;
 
@@ -1170,7 +1206,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (force_author && renew_authorship)
 		die(_("Using both --reset-author and --author does not make sense"));
 
-	if (logfile || have_option_m || use_message || fixup_message)
+	if (logfile || have_option_m || use_message)
 		use_editor = 0;
 	if (0 <= edit_flag)
 		use_editor = edit_flag;
@@ -1227,6 +1263,36 @@ static int parse_and_validate_options(int argc, const char *argv[],
 
 	if (also + only + all + interactive > 1)
 		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
+
+	if (fixup_message) {
+		/*
+		 * We limit --fixup's suboptions to only alpha characters.
+		 * If the first character after a run of alpha is colon,
+		 * then the part before the colon may be a known suboption
+		 * name `amend` or a misspelt suboption name. In this case,
+		 * we treat it as --fixup=<suboption>:<arg>.
+		 *
+		 * Otherwise, we are dealing with --fixup=<commit>.
+		 */
+		char *p = fixup_message;
+		while (isalpha(*p))
+			p++;
+		if (p > fixup_message && *p == ':') {
+			*p = '\0';
+			fixup_commit = p + 1;
+			if (!strcmp("amend", fixup_message)) {
+				fixup_prefix = "amend";
+				allow_empty = 1;
+			} else {
+				die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
+			}
+		} else {
+			fixup_commit = fixup_message;
+			fixup_prefix = "fixup";
+			use_editor = 0;
+		}
+	}
+
 	cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
 
 	handle_untracked_files_arg(s);
@@ -1504,7 +1570,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK('m', "message", &message, N_("message"), N_("commit message"), opt_parse_m),
 		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")),
-		OPT_STRING(0, "fixup", &fixup_message, N_("commit"), N_("use autosquash formatted message to fixup specified commit")),
+		/*
+		 * TRANSLATORS: Leave "[amend:]" as-is, and
+		 * only translate <commit>.
+		 */
+		OPT_STRING(0, "fixup", &fixup_message, N_("[amend:]commit"), N_("use autosquash formatted message to fixup or amend 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")),
@@ -1663,6 +1733,19 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		exit(1);
 	}
 
+	if (fixup_message && starts_with(sb.buf, "amend! ") &&
+	    !allow_empty_message) {
+		struct strbuf body = STRBUF_INIT;
+		size_t len = commit_subject_length(sb.buf);
+		strbuf_addstr(&body, sb.buf + len);
+		if (message_is_empty(&body, cleanup_mode)) {
+			rollback_index_files();
+			fprintf(stderr, _("Aborting commit due to empty commit message body.\n"));
+			exit(1);
+		}
+		strbuf_release(&body);
+	}
+
 	if (amend) {
 		const char *exclude_gpgsig[3] = { "gpgsig", "gpgsig-sha256", NULL };
 		extra = read_commit_extra_headers(current_head, exclude_gpgsig);
-- 
2.29.0.rc1

[PATCH v6 3/6] commit: add a reword suboption to --fixup

[Charvi Mendiratta]

`git commit --fixup=reword:<commit>` aliases
`--fixup=amend:<commit> --only`, where it creates an empty "amend!"
commit that will reword <commit> without changing its contents when
it is rebased with `--autosquash`.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 builtin/commit.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 9697bba10b..b17e6e32f8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1188,6 +1188,19 @@ static void finalize_deferred_config(struct wt_status *s)
 		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
 }
 
+static void check_fixup_reword_options(int argc, const char *argv[]) {
+	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 path '%s'"), *argv);
+	if (patch_interactive || interactive || all || also || only)
+		die(_("reword option of --fixup is mutually exclusive with --patch/--interactive/--all/--include/--only"));
+}
+
 static int parse_and_validate_options(int argc, const char *argv[],
 				      const struct option *options,
 				      const char * const usage[],
@@ -1269,8 +1282,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		 * We limit --fixup's suboptions to only alpha characters.
 		 * If the first character after a run of alpha is colon,
 		 * then the part before the colon may be a known suboption
-		 * name `amend` or a misspelt suboption name. In this case,
-		 * we treat it as --fixup=<suboption>:<arg>.
+		 * name like `amend` or `reword`, or a misspelt suboption
+		 * name. In either case, we treat it as
+		 * --fixup=<suboption>:<arg>.
 		 *
 		 * Otherwise, we are dealing with --fixup=<commit>.
 		 */
@@ -1280,9 +1294,14 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		if (p > fixup_message && *p == ':') {
 			*p = '\0';
 			fixup_commit = p + 1;
-			if (!strcmp("amend", fixup_message)) {
+			if (!strcmp("amend", fixup_message) ||
+			    !strcmp("reword", fixup_message)) {
 				fixup_prefix = "amend";
 				allow_empty = 1;
+				if (*fixup_message == 'r') {
+					check_fixup_reword_options(argc, argv);
+					only = 1;
+				}
 			} else {
 				die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
 			}
@@ -1571,10 +1590,10 @@ 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: Leave "[amend:]" as-is, and
-		 * only translate <commit>.
+		 * TRANSLATORS: Leave "[(amend|reword):]" as-is,
+		 * and only translate <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")),
-- 
2.29.0.rc1

[PATCH v6 4/6] t7500: add tests for --fixup=[amend|reword] options

[Charvi Mendiratta]

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t7500-commit-template-squash-signoff.sh | 159 ++++++++++++++++++++++
 1 file changed, 159 insertions(+)

diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 6d19ece05d..05e43a31d3 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -9,6 +9,8 @@ Tests for template, signoff, squash and -F functions.'
 
 . ./test-lib.sh
 
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
 commit_msg_is () {
 	expect=commit_msg_is.expect
 	actual=commit_msg_is.actual
@@ -279,6 +281,163 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
 
 extra"
 '
+get_commit_msg () {
+	rev="$1" &&
+	git log -1 --pretty=format:"%B" "$rev"
+}
+
+test_expect_success 'commit --fixup=amend: creates amend! commit' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited" \
+			git commit --fixup=amend:HEAD~
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--fixup=amend: --only ignores staged changes' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited" \
+			git commit --fixup=amend:HEAD~ --only
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual &&
+	test_cmp_rev HEAD@{1}^{tree} HEAD^{tree} &&
+	test_cmp_rev HEAD@{1} HEAD^ &&
+	test_expect_code 1 git diff --cached --exit-code &&
+	git cat-file blob :foo >actual &&
+	test_cmp foo actual
+'
+
+test_expect_success '--fixup=reword: ignores staged changes' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited" \
+			git commit --fixup=reword:HEAD~
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual &&
+	test_cmp_rev HEAD@{1}^{tree} HEAD^{tree} &&
+	test_cmp_rev HEAD@{1} HEAD^ &&
+	test_expect_code 1 git diff --cached --exit-code &&
+	git cat-file blob :foo >actual &&
+	test_cmp foo actual
+'
+
+test_expect_success '--fixup=reword: error out with -m option' '
+	commit_for_rebase_autosquash_setup &&
+	echo "fatal: cannot combine -m with --fixup:reword" >expect &&
+	test_must_fail git commit --fixup=reword:HEAD~ -m "reword commit message" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--fixup=amend: error out with -m option' '
+	commit_for_rebase_autosquash_setup &&
+	echo "fatal: cannot combine -m with --fixup:amend" >expect &&
+	test_must_fail git commit --fixup=amend:HEAD~ -m "amend commit message" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'consecutive amend! commits remove amend! line from commit msg body' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited 1
+
+	edited 2
+	EOF
+	echo "reword new commit message" >actual &&
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited 1" \
+			git commit --fixup=reword:HEAD~ &&
+		FAKE_COMMIT_AMEND="edited 2" \
+			git commit --fixup=reword:HEAD
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'deny to create amend! commit if its commit msg body is empty' '
+	commit_for_rebase_autosquash_setup &&
+	echo "Aborting commit due to empty commit message body." >expected &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_COMMIT_MESSAGE="amend! target message subject line" \
+			git commit --fixup=amend:HEAD~ 2>actual
+	) &&
+	test_cmp expected actual
+'
+
+test_expect_success 'amend! commit allows empty commit msg body with --allow-empty-message' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_MESSAGE="amend! target message subject line" \
+			git commit --fixup=amend:HEAD~ --allow-empty-message &&
+		get_commit_msg HEAD >actual
+	) &&
+	test_cmp expected actual
+'
+
+test_fixup_reword_opt () {
+	test_expect_success C_LOCALE_OUTPUT "--fixup=reword: incompatible with $1" "
+		echo 'fatal: reword option of --fixup is mutually exclusive with'\
+			'--patch/--interactive/--all/--include/--only' >expect &&
+		test_must_fail git commit --fixup=reword:HEAD~ $1 2>actual &&
+		test_cmp expect actual
+	"
+}
+
+for opt in --all --include --only --interactive --patch
+do
+	test_fixup_reword_opt $opt
+done
+
+test_expect_success '--fixup=reword: give error with pathsec' '
+	commit_for_rebase_autosquash_setup &&
+	echo "fatal: cannot combine reword option of --fixup with path '\''foo'\''" >expect &&
+	test_must_fail git commit --fixup=reword:HEAD~ -- foo 2>actual &&
+	test_cmp expect actual
+'
+
+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
+'
 
 test_expect_success 'commit --squash works with -F' '
 	commit_for_rebase_autosquash_setup &&
-- 
2.29.0.rc1

[PATCH v6 5/6] t3437: use --fixup with options to create amend! commit

[Charvi Mendiratta]

We taught `git commit --fixup` to create "amend!" commit. Let's also
update the tests and use it to setup the rebase tests.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t3437-rebase-fixup-options.sh | 30 +++---------------------------
 1 file changed, 3 insertions(+), 27 deletions(-)

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index a5a20354e3..d0bdc7ed02 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -72,40 +72,16 @@ test_expect_success 'setup' '
 	git commit --fixup=HEAD -a &&
 	git tag B1 &&
 	test_tick &&
-	git commit --allow-empty -F - <<-EOF &&
-	amend! B
-	$EMPTY
-	B
-	$EMPTY
-	edited 1
-	EOF
+	FAKE_COMMIT_AMEND="edited 1" git commit --fixup=reword:B &&
 	test_tick &&
-	git commit --allow-empty -F - <<-EOF &&
-	amend! amend! B
-	$EMPTY
-	B
-	$EMPTY
-	edited 1
-	$EMPTY
-	edited 2
-	EOF
+	FAKE_COMMIT_AMEND="edited 2" git commit --fixup=reword:HEAD &&
 	echo B2 >B &&
 	test_tick &&
 	FAKE_COMMIT_AMEND="edited squash" git commit --squash=HEAD -a &&
 	git tag B2 &&
 	echo B3 >B &&
 	test_tick &&
-	git commit -a -F - <<-EOF &&
-	amend! amend! amend! B
-	$EMPTY
-	B
-	$EMPTY
-	edited 1
-	$EMPTY
-	edited 2
-	$EMPTY
-	edited 3
-	EOF
+	FAKE_COMMIT_AMEND="edited 3" git commit -a --fixup=amend:HEAD^ &&
 	git tag B3 &&
 
 	GIT_AUTHOR_NAME="Rebase Author" &&
-- 
2.29.0.rc1

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

[Charvi Mendiratta]

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 Documentation/git-commit.txt | 45 +++++++++++++++++++++++++++++++-----
 Documentation/git-rebase.txt | 21 +++++++++--------
 2 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 17150fa7ea..3c69f461c9 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,44 @@ 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>::
+	Create a new commit which "fixes up" `<commit>` when applied with
+	`git rebase --autosquash`. Plain `--fixup=<commit>` creates a
+	"fixup!" commit which changes the content of `<commit>` but leaves
+	its log message untouched. `--fixup=amend:<commit>` is similar but
+	creates an "amend!" commit which also replaces the log message of
+	`<commit>` with the log message of the "amend!" commit.
+	`--fixup=reword:<commit>` creates an "amend!" commit which
+	replaces the log message of `<commit>` with its own log message
+	but makes no changes to the content of `<commit>`.
++
+The commit created by plain `--fixup=<commit>` has a subject
+composed of "fixup!" followed by the subject line from <commit>,
+and is recognized specially by `git rebase --autosquash`. The `-m`
+option may be used to supplement the log message of the created
+commit, but the additional commentary will be thrown away once the
+"fixup!" commit is squashed into `<commit>` by
+`git rebase --autosquash`.
++
+The commit created by `--fixup=amend:<commit>` is similar but its
+subject is instead prefixed with "amend!". The log message of
+<commit> is copied into the log message of the "amend!" commit and
+opened in an editor so it can be refined. When `git rebase
+--autosquash` squashes the "amend!" commit into `<commit>`, the
+log message of `<commit>` is replaced by the refined log message
+from the "amend!" commit. It is an error for the "amend!" commit's
+log message to be empty unless `--allow-empty-message` is
+specified.
++
+`--fixup=reword:<commit>` is shorthand for `--fixup=amend:<commit>
+--only`. It creates an "amend!" commit with only a log message
+(ignoring any changes staged in the index). When squashed by `git
+rebase --autosquash`, it replaces the log message of `<commit>`
+without making any other changes.
++
+Neither "fixup!" nor "amend!" commits change authorship of
+`<commit>` when applied by `git rebase --autosquash`.
+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..f08ae27e2a 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/amend/squash
+	commits is by using the `--fixup`, `--fixup=amend:` or `--fixup=reword:`
+	and `--squash` options respectively of linkgit:git-commit[1].
 +
 If the `--autosquash` option is enabled by default using the
 configuration variable `rebase.autoSquash`, this option can be
-- 
2.29.0.rc1

[PATCH v4 1/6] sequencer: export and rename subject_length()

[Charvi Mendiratta]

This function can be used in other parts of git. Let's move the
function to commit.c and also rename it to make the name of the
function more generic.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 commit.c    | 14 ++++++++++++++
 commit.h    |  3 +++
 sequencer.c | 16 +---------------
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/commit.c b/commit.c
index bab8d5ab07..199c7e274c 100644
--- a/commit.c
+++ b/commit.c
@@ -535,6 +535,20 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
 	return eol - p;
 }
 
+size_t commit_subject_length(const char *body)
+{
+	const char *p = body;
+	while (*p) {
+		const char *next = skip_blank_lines(p);
+		if (next != p)
+			break;
+		p = strchrnul(p, '\n');
+		if (*p)
+			p++;
+	}
+	return p - body;
+}
+
 struct commit_list *commit_list_insert(struct commit *item, struct commit_list **list_p)
 {
 	struct commit_list *new_list = xmalloc(sizeof(struct commit_list));
diff --git a/commit.h b/commit.h
index f4e7b0158e..e65d9b3e75 100644
--- a/commit.h
+++ b/commit.h
@@ -165,6 +165,9 @@ const void *detach_commit_buffer(struct commit *, unsigned long *sizep);
 /* Find beginning and length of commit subject. */
 int find_commit_subject(const char *commit_buffer, const char **subject);
 
+/* Return length of the commit subject from commit log message. */
+size_t commit_subject_length(const char *body);
+
 struct commit_list *commit_list_insert(struct commit *item,
 					struct commit_list **list);
 int commit_list_contains(struct commit *item,
diff --git a/sequencer.c b/sequencer.c
index abc6d5cdfd..da7d54c7ce 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1724,20 +1724,6 @@ enum todo_item_flags {
 	TODO_EDIT_FIXUP_MSG    = (1 << 2),
 };
 
-static size_t subject_length(const char *body)
-{
-	const char *p = body;
-	while (*p) {
-		const char *next = skip_blank_lines(p);
-		if (next != p)
-			break;
-		p = strchrnul(p, '\n');
-		if (*p)
-			p++;
-	}
-	return p - body;
-}
-
 static const char first_commit_msg_str[] = N_("This is the 1st commit message:");
 static const char nth_commit_msg_fmt[] = N_("This is the commit message #%d:");
 static const char skip_first_commit_msg_str[] = N_("The 1st commit message will be skipped:");
@@ -1861,7 +1847,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,
 	if (starts_with(body, "amend!") ||
 	    ((command == TODO_SQUASH || seen_squash(opts)) &&
 	     (starts_with(body, "squash!") || starts_with(body, "fixup!"))))
-		commented_len = subject_length(body);
+		commented_len = commit_subject_length(body);
 
 	strbuf_addf(buf, "\n%c ", comment_line_char);
 	strbuf_addf(buf, _(nth_commit_msg_fmt),
-- 
2.29.0.rc1

[PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

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

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.

In order to prevent rebase from creating commits with an empty
message we refuse to create an "amend!" commit if commit message
body is empty.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 builtin/commit.c | 107 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 97 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 505fe60956..05594fa8ab 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -105,7 +105,8 @@ static const char *template_file;
  */
 static const char *author_message, *author_message_buffer;
 static char *edit_message, *use_message;
-static char *fixup_message, *squash_message;
+static char *fixup_message, *fixup_commit, *squash_message;
+static const char *fixup_prefix;
 static int all, also, interactive, patch_interactive, only, amend, signoff;
 static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
@@ -357,7 +358,8 @@ static const char *prepare_index(const char **argv, const char *prefix,
 		die(_("--pathspec-file-nul requires --pathspec-from-file"));
 	}
 
-	if (!pathspec.nr && (also || (only && !amend && !allow_empty)))
+	if (!pathspec.nr && (also || (only && !allow_empty &&
+	    (!amend || (fixup_message && strcmp(fixup_prefix, "amend"))))))
 		die(_("No paths with --include/--only does not make sense."));
 
 	if (read_cache_preload(&pathspec) < 0)
@@ -681,6 +683,23 @@ static void adjust_comment_line_char(const struct strbuf *sb)
 	comment_line_char = *p;
 }
 
+static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,
+				struct pretty_print_context *ctx) {
+
+	const char *buffer, *subject, *fmt;
+
+	buffer = get_commit_buffer(commit, NULL);
+	find_commit_subject(buffer, &subject);
+	/*
+	 * If we amend the 'amend!' commit then we don't want to
+	 * duplicate the subject line.
+	 */
+	fmt = starts_with(subject, "amend!") ? "%b" : "%B";
+	format_commit_message(commit, fmt, sb, ctx);
+	unuse_commit_buffer(commit, buffer);
+	return 0;
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct commit *current_head,
 			     struct wt_status *s,
@@ -745,15 +764,32 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	} else if (fixup_message) {
 		struct pretty_print_context ctx = {0};
 		struct commit *commit;
-		commit = lookup_commit_reference_by_name(fixup_message);
+		char *fmt = xstrfmt("%s! %%s\n\n", fixup_prefix);
+		commit = lookup_commit_reference_by_name(fixup_commit);
 		if (!commit)
-			die(_("could not lookup commit %s"), fixup_message);
+			die(_("could not lookup commit %s"), fixup_commit);
 		ctx.output_encoding = get_commit_output_encoding();
-		format_commit_message(commit, "fixup! %s\n\n",
-				      &sb, &ctx);
-		if (have_option_m)
-			strbuf_addbuf(&sb, &message);
+		format_commit_message(commit, fmt, &sb, &ctx);
+		free(fmt);
 		hook_arg1 = "message";
+
+		/*
+		 * Only `-m` commit message option is checked here, as
+		 * it supports `--fixup` to append the commit message.
+		 *
+		 * The other commit message options `-c`/`-C`/`-F` are
+		 * incompatible with all the forms of `--fixup` and
+		 * have already errored out while parsing the `git commit`
+		 * options.
+		 */
+		if (have_option_m && !strcmp(fixup_prefix, "fixup"))
+			strbuf_addbuf(&sb, &message);
+
+		if (!strcmp(fixup_prefix, "amend")) {
+			if (have_option_m)
+				die(_("cannot combine -m with --fixup:%s"), fixup_message);
+			prepare_amend_commit(commit, &sb, &ctx);
+		}
 	} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
 		size_t merge_msg_start;
 
@@ -1152,6 +1188,12 @@ static void finalize_deferred_config(struct wt_status *s)
 		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
 }
 
+/* returns the length of intial segment of alpha characters only */
+static size_t get_alpha_len(char *fixup_message) {
+	const char alphas[] = "abcdefghijklmnopqrstuvwxyz";
+	return strspn(fixup_message, alphas);
+}
+
 static int parse_and_validate_options(int argc, const char *argv[],
 				      const struct option *options,
 				      const char * const usage[],
@@ -1170,7 +1212,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (force_author && renew_authorship)
 		die(_("Using both --reset-author and --author does not make sense"));
 
-	if (logfile || have_option_m || use_message || fixup_message)
+	if (logfile || have_option_m || use_message)
 		use_editor = 0;
 	if (0 <= edit_flag)
 		use_editor = edit_flag;
@@ -1227,6 +1269,34 @@ static int parse_and_validate_options(int argc, const char *argv[],
 
 	if (also + only + all + interactive > 1)
 		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
+
+	if (fixup_message) {
+		/*
+		 * To check if fixup_message that contains ':' is a commit
+		 * reference for example: --fixup="HEAD^{/^area: string}" or
+		 * a suboption of `--fixup`.
+		 *
+		 * 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 (!strcmp("amend", fixup_message)) {
+				fixup_prefix = "amend";
+				allow_empty = 1;
+			} else {
+				die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
+			}
+		} else {
+			fixup_commit = fixup_message;
+			fixup_prefix = "fixup";
+			use_editor = 0;
+		}
+	}
+
 	cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
 
 	handle_untracked_files_arg(s);
@@ -1504,7 +1574,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK('m', "message", &message, N_("message"), N_("commit message"), opt_parse_m),
 		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")),
-		OPT_STRING(0, "fixup", &fixup_message, N_("commit"), N_("use autosquash formatted message to fixup specified commit")),
+		/*
+		 * TRANSLATORS: Leave "[amend:]" as-is, and
+		 * only translate <commit>.
+		 */
+		OPT_STRING(0, "fixup", &fixup_message, N_("[amend:]commit"), N_("use autosquash formatted message to fixup or amend 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")),
@@ -1663,6 +1737,19 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		exit(1);
 	}
 
+	if (fixup_message && starts_with(sb.buf, "amend! ") &&
+	    !allow_empty_message) {
+		struct strbuf body = STRBUF_INIT;
+		size_t len = commit_subject_length(sb.buf);
+		strbuf_addstr(&body, sb.buf + len);
+		if (message_is_empty(&body, cleanup_mode)) {
+			rollback_index_files();
+			fprintf(stderr, _("Aborting commit due to empty commit message body.\n"));
+			exit(1);
+		}
+		strbuf_release(&body);
+	}
+
 	if (amend) {
 		const char *exclude_gpgsig[3] = { "gpgsig", "gpgsig-sha256", NULL };
 		extra = read_commit_extra_headers(current_head, exclude_gpgsig);
-- 
2.29.0.rc1

Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit

[Eric Sunshine]

On Wed, Mar 10, 2021 at 2:44 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> `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.
>
> 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.
> [...]
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
> diff --git a/builtin/commit.c b/builtin/commit.c
> @@ -681,6 +683,23 @@ static void adjust_comment_line_char(const struct strbuf *sb)
> +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,
> +                               struct pretty_print_context *ctx) {
> +
> +       const char *buffer, *subject, *fmt;

Two style nits:

* opening curly brace of function goes on its own line

* we don't normally have a blank line at the top of the function body
preceding the declarations

So:

    static int prepare_amend_commit(...)
    {
        const  char *buffer, *subject, *fmt;

> +       buffer = get_commit_buffer(commit, NULL);
> +       find_commit_subject(buffer, &subject);
> +       /*
> +        * If we amend the 'amend!' commit then we don't want to
> +        * duplicate the subject line.
> +        */
> +       fmt = starts_with(subject, "amend!") ? "%b" : "%B";
> +       format_commit_message(commit, fmt, sb, ctx);
> +       unuse_commit_buffer(commit, buffer);
> +       return 0;
> +}

What is the significance of this function's return value? At least in
this patch, the single caller of this function ignores the return
value, which suggests that the function need not return any value.
Will a later patch add other possible return values to indicate an
error or something?

> @@ -745,15 +764,32 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> +               char *fmt = xstrfmt("%s! %%s\n\n", fixup_prefix);
> +               commit = lookup_commit_reference_by_name(fixup_commit);
>                 if (!commit)
> +                       die(_("could not lookup commit %s"), fixup_commit);
>                 ctx.output_encoding = get_commit_output_encoding();
> +               format_commit_message(commit, fmt, &sb, &ctx);
> +               free(fmt);

Nit: it would reduce the cognitive load slightly if `fmt` is prepared
just before it is used rather than being prepared at the top of the
block:

    fmt = xstrfmt("%s! %%s\n\n", fixup_prefix);
    format_commit_message(commit, fmt, &sb, &ctx);
    free(fmt);

Subjective and not at all worth a re-roll.

> @@ -1227,6 +1269,34 @@ static int parse_and_validate_options(int argc, const char *argv[],
> +       if (fixup_message) {
> +               /*
> +                * To check if fixup_message that contains ':' is a commit
> +                * reference for example: --fixup="HEAD^{/^area: string}" or
> +                * a suboption of `--fixup`.
> +                *
> +                * As `amend` suboption contains only alpha character.
> +                * So check if first non alpha character in fixup_message
> +                * is ':'.
> +                */

I have a tough time figuring out what this comment is trying to say,
and I don't think I would have understood it if Junio had not already
explained earlier in this thread why this code is as complex as it is
(rather than using, say, skip_prefix()). Perhaps the entire comment
can be replaced with this:

    Extract <option> (i.e. `amend`) from `--fixup=<option>:<commit>`,
    if present. To avoid being fooled by a legitimate ":" in <commit>
    (i.e. `--fixup="HEAD^{/^area: string}"`), <option> must be
    composed of only alphabetic characters.

Not necessarily worth a re-roll.

> +               size_t len = get_alpha_len(fixup_message);
> +               if (len && fixup_message[len] == ':') {
> +                       fixup_message[len++] = '\0';
> +                       fixup_commit = fixup_message + len;

An alternate -- just about as compact and perhaps more idiomatic --
way to write all this without introducing the new get_alpha_len()
function:

    char *p = fixup_mesage;
    while (isalpha(*p))
        p++;
    if (p > fixup_message && *p == ':') {
        *p = '\0';
        fixup_commit = p + 1;

Subjective and not at all worth a re-roll.

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

Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

On Thu, 11 Mar 2021 at 11:55, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
[...]
> Two style nits:
>
> * opening curly brace of function goes on its own line
>
> * we don't normally have a blank line at the top of the function body
> preceding the declarations
>
> So:
>
>     static int prepare_amend_commit(...)
>     {
>         const  char *buffer, *subject, *fmt;
>

Okay, I will fix it.

> > +       buffer = get_commit_buffer(commit, NULL);
> > +       find_commit_subject(buffer, &subject);
> > +       /*
> > +        * If we amend the 'amend!' commit then we don't want to
> > +        * duplicate the subject line.
> > +        */
> > +       fmt = starts_with(subject, "amend!") ? "%b" : "%B";
> > +       format_commit_message(commit, fmt, sb, ctx);
> > +       unuse_commit_buffer(commit, buffer);
> > +       return 0;
> > +}
>
> What is the significance of this function's return value? At least in
> this patch, the single caller of this function ignores the return
> value, which suggests that the function need not return any value.
> Will a later patch add other possible return values to indicate an
> error or something?
>

No, it will not return another value later. I will remove it from here.

> > @@ -745,15 +764,32 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> > +               char *fmt = xstrfmt("%s! %%s\n\n", fixup_prefix);
> > +               commit = lookup_commit_reference_by_name(fixup_commit);
> >                 if (!commit)
> > +                       die(_("could not lookup commit %s"), fixup_commit);
> >                 ctx.output_encoding = get_commit_output_encoding();
> > +               format_commit_message(commit, fmt, &sb, &ctx);
> > +               free(fmt);
>
> Nit: it would reduce the cognitive load slightly if `fmt` is prepared
> just before it is used rather than being prepared at the top of the
> block:
>
>     fmt = xstrfmt("%s! %%s\n\n", fixup_prefix);
>     format_commit_message(commit, fmt, &sb, &ctx);
>     free(fmt);
>
> Subjective and not at all worth a re-roll.
>

Agree, will fix it.

> > @@ -1227,6 +1269,34 @@ static int parse_and_validate_options(int argc, const char *argv[],
> > +       if (fixup_message) {
> > +               /*
> > +                * To check if fixup_message that contains ':' is a commit
> > +                * reference for example: --fixup="HEAD^{/^area: string}" or
> > +                * a suboption of `--fixup`.
> > +                *
> > +                * As `amend` suboption contains only alpha character.
> > +                * So check if first non alpha character in fixup_message
> > +                * is ':'.
> > +                */
>
> I have a tough time figuring out what this comment is trying to say,
> and I don't think I would have understood it if Junio had not already
> explained earlier in this thread why this code is as complex as it is
> (rather than using, say, skip_prefix()). Perhaps the entire comment
> can be replaced with this:
>

I admit, this comment seems confusing...

>     Extract <option> (i.e. `amend`) from `--fixup=<option>:<commit>`,
>     if present. To avoid being fooled by a legitimate ":" in <commit>
>     (i.e. `--fixup="HEAD^{/^area: string}"`), <option> must be
>     composed of only alphabetic characters.
>
> Not necessarily worth a re-roll.
>

.. and I think we can reword it as suggested by Junio in patch[v4
3/6], as it seems more clear.

> > +               size_t len = get_alpha_len(fixup_message);
> > +               if (len && fixup_message[len] == ':') {
> > +                       fixup_message[len++] = '\0';
> > +                       fixup_commit = fixup_message + len;
>
> An alternate -- just about as compact and perhaps more idiomatic --
> way to write all this without introducing the new get_alpha_len()
> function:
>
>     char *p = fixup_mesage;
>     while (isalpha(*p))
>         p++;
>     if (p > fixup_message && *p == ':') {
>         *p = '\0';
>         fixup_commit = p + 1;
>
> Subjective and not at all worth a re-roll.
>

Earlier we had discussed[1] keeping a separate helper function, so
that it may re-use it later. But I agree above is easier to get and
compact so I think maybe it will be ok, for this patch series to
replace it with the above and remove the function.

[1] https://lore.kernel.org/git/xmqqpn0xdse8.fsf@gitster.g/

Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit

[Eric Sunshine]

On Thu, Mar 11, 2021 at 10:24 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> On Thu, 11 Mar 2021 at 11:55, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > +               size_t len = get_alpha_len(fixup_message);
> > > +               if (len && fixup_message[len] == ':') {
> > > +                       fixup_message[len++] = '\0';
> > > +                       fixup_commit = fixup_message + len;
> >
> > An alternate -- just about as compact and perhaps more idiomatic --
> > way to write all this without introducing the new get_alpha_len()
> > function:
> >
> >     char *p = fixup_mesage;
> >     while (isalpha(*p))
> >         p++;
> >     if (p > fixup_message && *p == ':') {
> >         *p = '\0';
> >         fixup_commit = p + 1;
>
> Earlier we had discussed[1] keeping a separate helper function, so
> that it may re-use it later. But I agree above is easier to get and
> compact so I think maybe it will be ok, for this patch series to
> replace it with the above and remove the function.

I don't have strong feelings one way or the other whether you should
use a function or inline it as I showed above, and since our aim is to
land this series rather than endlessly re-rolling it, let's not spend
a lot of cycles worrying about it.

The one thing that does bother me, however, is the name of the
function, get_alpha_len(), which tells you (somewhat) literally what
it does but doesn't convey to the reader its actual purpose (which is
something we should strive for when naming functions and variables).
In that previous discussion you referenced, Junio mentioned that a
future sub-option might want to have punctuation in its name. If that
ever comes about, then the name get_alpha_len() no longer makes sense
and needs to be renamed so it doesn't become a lie. Giving the
function a better name up front would not only help readers now to
understand what is going on, but would also help down the road when or
if punctuation (or numbers or whatnot) become valid in a sub-option
name. suboption_length() is one possibility. With a slight semantic
change, skip_suboption() or latch_suboption() are other possibilities.
Or, if you were to open-code the loop as I did above, then you might
have a function named is_suboption_char() and use that in place of
isalpha().

So, if you do re-roll, I wouldn't mind seeing a better name for the
function; but the rest is subjective and not worth spending time
refining unless you actually feel that one style has a clear advantage
over others.

Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

On Thu, 11 Mar 2021 at 22:38, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Thu, Mar 11, 2021 at 10:24 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> > On Thu, 11 Mar 2021 at 11:55, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > > +               size_t len = get_alpha_len(fixup_message);
> > > > +               if (len && fixup_message[len] == ':') {
> > > > +                       fixup_message[len++] = '\0';
> > > > +                       fixup_commit = fixup_message + len;
> > >
> > > An alternate -- just about as compact and perhaps more idiomatic --
> > > way to write all this without introducing the new get_alpha_len()
> > > function:
> > >
> > >     char *p = fixup_mesage;
> > >     while (isalpha(*p))
> > >         p++;
> > >     if (p > fixup_message && *p == ':') {
> > >         *p = '\0';
> > >         fixup_commit = p + 1;
> >
> > Earlier we had discussed[1] keeping a separate helper function, so
> > that it may re-use it later. But I agree above is easier to get and
> > compact so I think maybe it will be ok, for this patch series to
> > replace it with the above and remove the function.
>
> I don't have strong feelings one way or the other whether you should
> use a function or inline it as I showed above, and since our aim is to
> land this series rather than endlessly re-rolling it, let's not spend
> a lot of cycles worrying about it.
>
> The one thing that does bother me, however, is the name of the
> function, get_alpha_len(), which tells you (somewhat) literally what
> it does but doesn't convey to the reader its actual purpose (which is
> something we should strive for when naming functions and variables).
> In that previous discussion you referenced, Junio mentioned that a
> future sub-option might want to have punctuation in its name. If that
> ever comes about, then the name get_alpha_len() no longer makes sense
> and needs to be renamed so it doesn't become a lie. Giving the
> function a better name up front would not only help readers now to
> understand what is going on, but would also help down the road when or
> if punctuation (or numbers or whatnot) become valid in a sub-option
> name. suboption_length() is one possibility. With a slight semantic
> change, skip_suboption() or latch_suboption() are other possibilities.
> Or, if you were to open-code the loop as I did above, then you might
> have a function named is_suboption_char() and use that in place of
> isalpha().
>
> So, if you do re-roll, I wouldn't mind seeing a better name for the
> function; but the rest is subjective and not worth spending time
> refining unless you actually feel that one style has a clear advantage
> over others.

Okay, I will rename the get_alpha_len() to skip_suboption().

Thanks and Regards,
Charvi

Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> The one thing that does bother me, however, is the name of the
> function, get_alpha_len(), which tells you (somewhat) literally what
> it does but doesn't convey to the reader its actual purpose (which is
> something we should strive for when naming functions and variables).

I actually think the helper function that is used as a building
block the "subcommand parser" uses should be named more directly
to represent what it does (i.e. look for a run of alphas) than
what it means (i.e. look for a run of letters allowed in a
subcommand name).  IOW

	char *end = skip_alphas(ptr);
	if (*end == ':' && ptr != end) {
		/* 
		 * ptr..end could be a subcommand in 
	         * "--fixup=<subcommand>:"; see if it is a known one
		 */
		*end = '\0';
		if (!strcmp(ptr, "amend"))
			... do the amend thing ...
		else if (!strcmp(ptr, "reword"))
			... do the reword thing ...
		else
			... we do not know such a subcommand yet ...
	} else {
		/* assume it is --fixup=<command> form */
		...
	}

conveys more information to readers than a variant where you replace
"skip_alphas" with "skip_subcommand_chars" without losing any
information.

Yes, in different contexts, where a helpers are designed to be used
by multiple callers that may not even be aware of each other, we do
encourage naming them after what they do _means_.  But in this
codepath, I do not think it applies.

Thanks.

Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

On Sun, 14 Mar 2021 at 07:55, Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > The one thing that does bother me, however, is the name of the
> > function, get_alpha_len(), which tells you (somewhat) literally what
> > it does but doesn't convey to the reader its actual purpose (which is
> > something we should strive for when naming functions and variables).
>
> I actually think the helper function that is used as a building
> block the "subcommand parser" uses should be named more directly
> to represent what it does (i.e. look for a run of alphas) than
> what it means (i.e. look for a run of letters allowed in a
> subcommand name).  IOW
>
>         char *end = skip_alphas(ptr);
>         if (*end == ':' && ptr != end) {
>                 /*
>                  * ptr..end could be a subcommand in
>                  * "--fixup=<subcommand>:"; see if it is a known one
>                  */
>                 *end = '\0';
>                 if (!strcmp(ptr, "amend"))
>                         ... do the amend thing ...
>                 else if (!strcmp(ptr, "reword"))
>                         ... do the reword thing ...
>                 else
>                         ... we do not know such a subcommand yet ...
>         } else {
>                 /* assume it is --fixup=<command> form */
>                 ...
>         }
>
> conveys more information to readers than a variant where you replace
> "skip_alphas" with "skip_subcommand_chars" without losing any
> information.
>

I thought to just rename get_alpha_len() to skip_alpha() that returns
alpha length. But even removing the "len" variable and implementing as
suggested above seems a better and clear alternative. I also agree to
update it.

Thanks for the suggestions.

Thanks and Regards,
Charvi

Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

> On Sun, 14 Mar 2021 at 07:55, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>> > The one thing that does bother me, however, is the name of the
>> > function, get_alpha_len(), which tells you (somewhat) literally what
>> > it does but doesn't convey to the reader its actual purpose (which is
>> > something we should strive for when naming functions and variables).
>>
>> I actually think the helper function that is used as a building
>> block the "subcommand parser" uses should be named more directly
>> to represent what it does (i.e. look for a run of alphas) than
>> what it means (i.e. look for a run of letters allowed in a
>> subcommand name).  IOW
>>
>>         char *end = skip_alphas(ptr);
>>         if (*end == ':' && ptr != end) {
>>                 /*
>>                  * ptr..end could be a subcommand in
>>                  * "--fixup=<subcommand>:"; see if it is a known one
>>                  */
>>                 *end = '\0';
>>                 if (!strcmp(ptr, "amend"))
>>                         ... do the amend thing ...
>>                 else if (!strcmp(ptr, "reword"))
>>                         ... do the reword thing ...
>>                 else
>>                         ... we do not know such a subcommand yet ...
>>         } else {
>>                 /* assume it is --fixup=<command> form */
>>                 ...
>>         }
>>
>> conveys more information to readers than a variant where you replace
>> "skip_alphas" with "skip_subcommand_chars" without losing any
>> information.
>>
>
> I thought to just rename get_alpha_len() to skip_alpha() that returns
> alpha length. But even removing the "len" variable and implementing as
> suggested above seems a better and clear alternative. I also agree to
> update it.
>
> Thanks for the suggestions.

FWIW I am also fine with Eric's simpler "open code it right there"
suggestion in this case.  Just like the "skip alphas" suggestion, it
makes the logic to parse subcommand name out isolated to a single
place without asking readers to refer to the implementation of a
helper, and it would be short enough.

Thanks.

Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit

[Eric Sunshine]

On Sun, Mar 14, 2021 at 6:43 PM Junio C Hamano <gitster@pobox.com> wrote:
> FWIW I am also fine with Eric's simpler "open code it right there"
> suggestion in this case.  Just like the "skip alphas" suggestion, it
> makes the logic to parse subcommand name out isolated to a single
> place without asking readers to refer to the implementation of a
> helper, and it would be short enough.

Likewise. If you're going to re-roll anyhow, the open-coded:

    char *p = fixup_mesage;
    while (isalpha(*p))
        p++;
    if (p > fixup_message && *p == ':') {
        *p = '\0';
        fixup_commit = p + 1;

would be perfectly fine with me too (or any simple variation on that
theme). Whether or not it's worth re-rolling again, I leave up to you
and Junio.

Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

On Mon, 15 Mar 2021 at 04:37, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Mar 14, 2021 at 6:43 PM Junio C Hamano <gitster@pobox.com> wrote:
> > FWIW I am also fine with Eric's simpler "open code it right there"
> > suggestion in this case.  Just like the "skip alphas" suggestion, it
> > makes the logic to parse subcommand name out isolated to a single
> > place without asking readers to refer to the implementation of a
> > helper, and it would be short enough.
>
> Likewise. If you're going to re-roll anyhow, the open-coded:
>
>     char *p = fixup_mesage;
>     while (isalpha(*p))
>         p++;
>     if (p > fixup_message && *p == ':') {
>         *p = '\0';
>         fixup_commit = p + 1;
>
> would be perfectly fine with me too (or any simple variation on that
> theme). Whether or not it's worth re-rolling again, I leave up to you
> and Junio.

okay, I agree too. I have updated it in the re-roll.

Thanks for all the detailed reviews and suggestions.

Thanks and Regards,
Charvi

Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit

[Eric Sunshine]

On Mon, Mar 15, 2021 at 3:59 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> On Mon, 15 Mar 2021 at 04:37, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > Likewise. If you're going to re-roll anyhow, the open-coded:
> > would be perfectly fine with me too (or any simple variation on that
> > theme). Whether or not it's worth re-rolling again, I leave up to you
> > and Junio.
>
> okay, I agree too. I have updated it in the re-roll.
>
> Thanks for all the detailed reviews and suggestions.

Thanks for patiently putting up with reviewers who sometimes have
opposing or contradictory opinions and recommendations.

Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta]

On Mon, 15 Mar 2021 at 13:46, Eric Sunshine <sunshine@sunshineco.com> wrote:
[..]
> Thanks for patiently putting up with reviewers who sometimes have
> opposing or contradictory opinions and recommendations.

It was more a learning path and helpful for me to proceed. I really
appreciate all the guidance and suggestions received. Glad to get this
merge!

Thanks and regards,
Charvi

[PATCH v4 3/6] commit: add a reword suboption to --fixup

[Charvi Mendiratta]

`git commit --fixup=reword:<commit>` aliases
`--fixup=amend:<commit> --only`, where it creates an empty "amend!"
commit that will reword <commit> without changing its contents when
it is rebased with `--autosquash`.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 builtin/commit.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 05594fa8ab..d8cec75888 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1188,6 +1188,19 @@ static void finalize_deferred_config(struct wt_status *s)
 		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
 }
 
+static void check_fixup_reword_options(int argc, const char *argv[]) {
+	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 path '%s'"), *argv);
+	if (patch_interactive || interactive || all || also || only)
+		die(_("reword option of --fixup is mutually exclusive with --patch/--interactive/--all/--include/--only"));
+}
+
 /* returns the length of intial segment of alpha characters only */
 static size_t get_alpha_len(char *fixup_message) {
 	const char alphas[] = "abcdefghijklmnopqrstuvwxyz";
@@ -1276,17 +1289,22 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		 * reference for example: --fixup="HEAD^{/^area: string}" or
 		 * a suboption of `--fixup`.
 		 *
-		 * 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 (!strcmp("amend", fixup_message)) {
+			if (!strcmp("amend", fixup_message) ||
+			    !strcmp("reword", fixup_message)) {
 				fixup_prefix = "amend";
 				allow_empty = 1;
+				if (*fixup_message == 'r') {
+					check_fixup_reword_options(argc, argv);
+					only = 1;
+				}
 			} else {
 				die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
 			}
@@ -1575,10 +1593,10 @@ 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: Leave "[amend:]" as-is, and
-		 * only translate <commit>.
+		 * TRANSLATORS: Leave "[(amend|reword):]" as-is,
+		 * and only translate <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")),
-- 
2.29.0.rc1

Re: [PATCH v4 3/6] commit: add a reword suboption to --fixup

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

>  		 * reference for example: --fixup="HEAD^{/^area: string}" or
>  		 * a suboption of `--fixup`.
>  		 *
> -		 * 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 ':'.

Sorry, but I cannot quite follow the logic.  

	We limit --fixup's suboptions to only alpha characters.  If
	the first character after a len of alpha is colon, then the
	part before the colon may be a known suboption name like
	`amend` or `reword`, or a misspelt suboption name.

	Otherwise, we are dealing with --fixup=<commit> that happens
	to have a colon in <commit> object name.

perhaps?

Re: [PATCH v4 3/6] commit: add a reword suboption to --fixup

[Charvi Mendiratta]

On Thu, 11 Mar 2021 at 06:01, Junio C Hamano <gitster@pobox.com> wrote:
>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> >                * reference for example: --fixup="HEAD^{/^area: string}" or
> >                * a suboption of `--fixup`.
> >                *
> > -              * 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 ':'.
>
> Sorry, but I cannot quite follow the logic.
>
>         We limit --fixup's suboptions to only alpha characters.  If
>         the first character after a len of alpha is colon, then the
>         part before the colon may be a known suboption name like
>         `amend` or `reword`, or a misspelt suboption name.
>
>         Otherwise, we are dealing with --fixup=<commit> that happens
>         to have a colon in <commit> object name.
>
> perhaps?

Yes, Agree. Here I just intend to mention the special case
"--fixup=HEAD^{/^area: string}" because of which we chose the method
to check if first non alpha char is ':' instead of directly checking
the suboption like (skip_prefix(msg, "amend:", &arg). So maybe we can
reword it like

- To check if fixup_message that contains ':' is a commit
- reference for example: --fixup="HEAD^{/^area: string}" or
- a suboption of `--fixup`.
+ fixup_message could be a commit reference for example:
+ --fixup="HEAD^{/^area:string}" or a suboption of `--fixup`.
+
+ As `amend` ...

Thanks and Regards,
Charvi

Re: [PATCH v4 3/6] commit: add a reword suboption to --fixup

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

> On Thu, 11 Mar 2021 at 06:01, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Charvi Mendiratta <charvi077@gmail.com> writes:
>>
>> >                * reference for example: --fixup="HEAD^{/^area: string}" or
>> >                * a suboption of `--fixup`.
>> >                *
>> > -              * 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 ':'.
>>
>> Sorry, but I cannot quite follow the logic.
>>
>>         We limit --fixup's suboptions to only alpha characters.  If
>>         the first character after a len of alpha is colon, then the
>>         part before the colon may be a known suboption name like
>>         `amend` or `reword`, or a misspelt suboption name.
>>
>>         Otherwise, we are dealing with --fixup=<commit> that happens
>>         to have a colon in <commit> object name.
>>
>> perhaps?
>
> Yes, Agree. Here I just intend to mention the special case
> "--fixup=HEAD^{/^area: string}" because of which we chose the method
> to check if first non alpha char is ':' instead of directly checking
> the suboption like (skip_prefix(msg, "amend:", &arg). So maybe we can
> reword it like
>
> - To check if fixup_message that contains ':' is a commit
> - reference for example: --fixup="HEAD^{/^area: string}" or
> - a suboption of `--fixup`.
> + fixup_message could be a commit reference for example:
> + --fixup="HEAD^{/^area:string}" or a suboption of `--fixup`.
> +
> + As `amend` ...

My suggestion primarily started a reaction to that "As `amend`..."
which was not gramatically complete sentence, and I ended up
rewriting everything after "As `amend`..."

But re-reading what is in the paragraph before, I tend to think that
it places too much stress on 'colon' and should be removed.

The comment is about what is being parsed, so

         We limit --fixup's suboptions to only alpha characters.  If
         the first character after a run of alpha is colon, then the
         part before the colon may be a known suboption name like
         `amend` or `reword`, or a misspelt suboption name.  In
         either case, we treat it as --fixup=<suboption>:<arg>

         Otherwise, we are dealing with --fixup=<commit>.

would be good.  The code, when it decides it is not in the
--fixup=<suboption>:<arg>  form but it is --fixup=<commit>, does not
even care about a colon, so there is no need to mention colon in the
"Otherwise" part.

Re: [PATCH v4 3/6] commit: add a reword suboption to --fixup

[Eric Sunshine]

On Thu, Mar 11, 2021 at 12:37 AM Junio C Hamano <gitster@pobox.com> wrote:
> My suggestion primarily started a reaction to that "As `amend`..."
> which was not gramatically complete sentence, and I ended up
> rewriting everything after "As `amend`..."
>
> But re-reading what is in the paragraph before, I tend to think that
> it places too much stress on 'colon' and should be removed.
>
> The comment is about what is being parsed, so
>
>          We limit --fixup's suboptions to only alpha characters.  If
>          the first character after a run of alpha is colon, then the
>          part before the colon may be a known suboption name like
>          `amend` or `reword`, or a misspelt suboption name.  In
>          either case, we treat it as --fixup=<suboption>:<arg>
>
>          Otherwise, we are dealing with --fixup=<commit>.

I suggested a different way to rewrite this comment[1] in my review of
patch [2/6] before I was aware that you two were discussing rewrites
here in [3/6].

[1]: https://lore.kernel.org/git/CAPig+cRxZFV7DDtnYOxRqEYU4PwXK8gb2HSehDyGorqdgsmoNQ@mail.gmail.com/

Re: [PATCH v4 3/6] commit: add a reword suboption to --fixup

[Charvi Mendiratta]

On Thu, 11 Mar 2021 at 11:07, Junio C Hamano <gitster@pobox.com> wrote:
>
[...]
> > - To check if fixup_message that contains ':' is a commit
> > - reference for example: --fixup="HEAD^{/^area: string}" or
> > - a suboption of `--fixup`.
> > + fixup_message could be a commit reference for example:
> > + --fixup="HEAD^{/^area:string}" or a suboption of `--fixup`.
> > +
> > + As `amend` ...
>
> My suggestion primarily started a reaction to that "As `amend`..."
> which was not gramatically complete sentence, and I ended up
> rewriting everything after "As `amend`..."
>

Oops, I must have written a complete comment. I will take care of it.

> But re-reading what is in the paragraph before, I tend to think that
> it places too much stress on 'colon' and should be removed.
>
> The comment is about what is being parsed, so
>
>          We limit --fixup's suboptions to only alpha characters.  If
>          the first character after a run of alpha is colon, then the
>          part before the colon may be a known suboption name like
>          `amend` or `reword`, or a misspelt suboption name.  In
>          either case, we treat it as --fixup=<suboption>:<arg>
>
>          Otherwise, we are dealing with --fixup=<commit>.
>
> would be good.  The code, when it decides it is not in the
> --fixup=<suboption>:<arg>  form but it is --fixup=<commit>, does not
> even care about a colon, so there is no need to mention colon in the
> "Otherwise" part.

Okay, I will replace it.

Thanks for helping out and guiding.

Thanks and Regards,
Charvi

[PATCH v4 4/6] t7500: add tests for --fixup=[amend|reword] options

[Charvi Mendiratta]

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t7500-commit-template-squash-signoff.sh | 159 ++++++++++++++++++++++
 1 file changed, 159 insertions(+)

diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 6d19ece05d..05e43a31d3 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -9,6 +9,8 @@ Tests for template, signoff, squash and -F functions.'
 
 . ./test-lib.sh
 
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
 commit_msg_is () {
 	expect=commit_msg_is.expect
 	actual=commit_msg_is.actual
@@ -279,6 +281,163 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
 
 extra"
 '
+get_commit_msg () {
+	rev="$1" &&
+	git log -1 --pretty=format:"%B" "$rev"
+}
+
+test_expect_success 'commit --fixup=amend: creates amend! commit' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited" \
+			git commit --fixup=amend:HEAD~
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--fixup=amend: --only ignores staged changes' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited" \
+			git commit --fixup=amend:HEAD~ --only
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual &&
+	test_cmp_rev HEAD@{1}^{tree} HEAD^{tree} &&
+	test_cmp_rev HEAD@{1} HEAD^ &&
+	test_expect_code 1 git diff --cached --exit-code &&
+	git cat-file blob :foo >actual &&
+	test_cmp foo actual
+'
+
+test_expect_success '--fixup=reword: ignores staged changes' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited" \
+			git commit --fixup=reword:HEAD~
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual &&
+	test_cmp_rev HEAD@{1}^{tree} HEAD^{tree} &&
+	test_cmp_rev HEAD@{1} HEAD^ &&
+	test_expect_code 1 git diff --cached --exit-code &&
+	git cat-file blob :foo >actual &&
+	test_cmp foo actual
+'
+
+test_expect_success '--fixup=reword: error out with -m option' '
+	commit_for_rebase_autosquash_setup &&
+	echo "fatal: cannot combine -m with --fixup:reword" >expect &&
+	test_must_fail git commit --fixup=reword:HEAD~ -m "reword commit message" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--fixup=amend: error out with -m option' '
+	commit_for_rebase_autosquash_setup &&
+	echo "fatal: cannot combine -m with --fixup:amend" >expect &&
+	test_must_fail git commit --fixup=amend:HEAD~ -m "amend commit message" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'consecutive amend! commits remove amend! line from commit msg body' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! amend! $(git log -1 --format=%s HEAD~)
+
+	$(get_commit_msg HEAD~)
+
+	edited 1
+
+	edited 2
+	EOF
+	echo "reword new commit message" >actual &&
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_AMEND="edited 1" \
+			git commit --fixup=reword:HEAD~ &&
+		FAKE_COMMIT_AMEND="edited 2" \
+			git commit --fixup=reword:HEAD
+	) &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'deny to create amend! commit if its commit msg body is empty' '
+	commit_for_rebase_autosquash_setup &&
+	echo "Aborting commit due to empty commit message body." >expected &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_COMMIT_MESSAGE="amend! target message subject line" \
+			git commit --fixup=amend:HEAD~ 2>actual
+	) &&
+	test_cmp expected actual
+'
+
+test_expect_success 'amend! commit allows empty commit msg body with --allow-empty-message' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! $(git log -1 --format=%s HEAD~)
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_COMMIT_MESSAGE="amend! target message subject line" \
+			git commit --fixup=amend:HEAD~ --allow-empty-message &&
+		get_commit_msg HEAD >actual
+	) &&
+	test_cmp expected actual
+'
+
+test_fixup_reword_opt () {
+	test_expect_success C_LOCALE_OUTPUT "--fixup=reword: incompatible with $1" "
+		echo 'fatal: reword option of --fixup is mutually exclusive with'\
+			'--patch/--interactive/--all/--include/--only' >expect &&
+		test_must_fail git commit --fixup=reword:HEAD~ $1 2>actual &&
+		test_cmp expect actual
+	"
+}
+
+for opt in --all --include --only --interactive --patch
+do
+	test_fixup_reword_opt $opt
+done
+
+test_expect_success '--fixup=reword: give error with pathsec' '
+	commit_for_rebase_autosquash_setup &&
+	echo "fatal: cannot combine reword option of --fixup with path '\''foo'\''" >expect &&
+	test_must_fail git commit --fixup=reword:HEAD~ -- foo 2>actual &&
+	test_cmp expect actual
+'
+
+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
+'
 
 test_expect_success 'commit --squash works with -F' '
 	commit_for_rebase_autosquash_setup &&
-- 
2.29.0.rc1

[PATCH v4 5/6] t3437: use --fixup with options to create amend! commit

[Charvi Mendiratta]

We taught `git commit --fixup` to create "amend!" commit. Let's also
update the tests and use it to setup the rebase tests.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t3437-rebase-fixup-options.sh | 30 +++---------------------------
 1 file changed, 3 insertions(+), 27 deletions(-)

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index a5a20354e3..d0bdc7ed02 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -72,40 +72,16 @@ test_expect_success 'setup' '
 	git commit --fixup=HEAD -a &&
 	git tag B1 &&
 	test_tick &&
-	git commit --allow-empty -F - <<-EOF &&
-	amend! B
-	$EMPTY
-	B
-	$EMPTY
-	edited 1
-	EOF
+	FAKE_COMMIT_AMEND="edited 1" git commit --fixup=reword:B &&
 	test_tick &&
-	git commit --allow-empty -F - <<-EOF &&
-	amend! amend! B
-	$EMPTY
-	B
-	$EMPTY
-	edited 1
-	$EMPTY
-	edited 2
-	EOF
+	FAKE_COMMIT_AMEND="edited 2" git commit --fixup=reword:HEAD &&
 	echo B2 >B &&
 	test_tick &&
 	FAKE_COMMIT_AMEND="edited squash" git commit --squash=HEAD -a &&
 	git tag B2 &&
 	echo B3 >B &&
 	test_tick &&
-	git commit -a -F - <<-EOF &&
-	amend! amend! amend! B
-	$EMPTY
-	B
-	$EMPTY
-	edited 1
-	$EMPTY
-	edited 2
-	$EMPTY
-	edited 3
-	EOF
+	FAKE_COMMIT_AMEND="edited 3" git commit -a --fixup=amend:HEAD^ &&
 	git tag B3 &&
 
 	GIT_AUTHOR_NAME="Rebase Author" &&
-- 
2.29.0.rc1

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

[Charvi Mendiratta]

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 Documentation/git-commit.txt | 41 ++++++++++++++++++++++++++++++------
 Documentation/git-rebase.txt | 21 +++++++++---------
 2 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 17150fa7ea..2de8255bdb 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,40 @@ 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>::
+	Construct a new commit for use with `rebase --autosquash`,
+	which fixes the specified commit. The plain form
+	`--fixup=<commit>` creates a "fixup!" commit, that allows
+	to fixup only the content of the specified commit and leave
+	it's commit log message untouched. When used with `amend:`
+	or `reword:`, it creates "amend!" commit that is like "fixup!"
+	commit but it allows to fixup both the content and the commit
+	log message of the specified commit. The commit log message of
+	the specified commit is fixed implicitly by replacing it with
+	the "amend!" commit's message body upon `rebase --autosquash`.
++
+The resulting "fixup!" commit message will be the subject line
+from the specified commit with a prefix of "fixup!". Can be used
+with additional commit message option `-m`.
++
+The `--fixup=amend:<commit>` form creates an "amend!" commit where
+its 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. It refuses to create "amend!" commit if it's
+commit message body is empty unless used with the
+`--allow-empty-message` option.
++
+The `--fixup=reword:<commit>` aliases `--fixup=amend:<commit> --only`
+and it also creates an "amend!" commit, but here it records the same
+tree as `HEAD`, 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`.
++
+Also, after fixing the commit using `--fixup`, with or without option
+and rebased with `--autosquash`, the authorship of the original commit
+remains unchanged. See linkgit:git-rebase[1] for details.
 
 --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..f08ae27e2a 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/amend/squash
+	commits is by using the `--fixup`, `--fixup=amend:` or `--fixup=reword:`
+	and `--squash` options respectively of linkgit:git-commit[1].
 +
 If the `--autosquash` option is enabled by default using the
 configuration variable `rebase.autoSquash`, this option can be
-- 
2.29.0.rc1

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

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

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

Overall, the text in this round reads much better than the previous
round.

> +--fixup=[(amend|reword):]<commit>::
> +	Construct a new commit for use with `rebase --autosquash`,
> +	which fixes the specified commit. The plain form
> +	`--fixup=<commit>` creates a "fixup!" commit, that allows
> +	to fixup only the content of the specified commit and leave
> +	it's commit log message untouched. When used with `amend:`
> +	or `reword:`, it creates "amend!" commit that is like "fixup!"
> +	commit but it allows to fixup both the content and the commit
> +	log message of the specified commit. The commit log message of
> +	the specified commit is fixed implicitly by replacing it with
> +	the "amend!" commit's message body upon `rebase --autosquash`.

Rewriting

    "is fixed implicitly by replacing it with" -> "is replaced by"

may make it easier to follow.

Thanks.

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

[Charvi Mendiratta]

On Thu, 11 Mar 2021 at 06:01, Junio C Hamano <gitster@pobox.com> wrote:

> > +--fixup=[(amend|reword):]<commit>::
> > +     Construct a new commit for use with `rebase --autosquash`,
> > +     which fixes the specified commit. The plain form
> > +     `--fixup=<commit>` creates a "fixup!" commit, that allows
> > +     to fixup only the content of the specified commit and leave
> > +     it's commit log message untouched. When used with `amend:`
> > +     or `reword:`, it creates "amend!" commit that is like "fixup!"
> > +     commit but it allows to fixup both the content and the commit
> > +     log message of the specified commit. The commit log message of
> > +     the specified commit is fixed implicitly by replacing it with
> > +     the "amend!" commit's message body upon `rebase --autosquash`.
>
> Rewriting
>
>     "is fixed implicitly by replacing it with" -> "is replaced by"
>
> may make it easier to follow.
>

Okay I will replace it.

Thanks and Regards,
Charvi

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

[Eric Sunshine]

On Wed, Mar 10, 2021 at 2:45 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> ---
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> @@ -86,11 +86,40 @@ OPTIONS
> +--fixup=[(amend|reword):]<commit>::
> +       Construct a new commit for use with `rebase --autosquash`,
> +       which fixes the specified commit. The plain form
> +       `--fixup=<commit>` creates a "fixup!" commit, that allows
> +       to fixup only the content of the specified commit and leave
> +       it's commit log message untouched. When used with `amend:`

s/it's/its/

> +       or `reword:`, it creates "amend!" commit that is like "fixup!"
> +       commit but it allows to fixup both the content and the commit
> +       log message of the specified commit. The commit log message of
> +       the specified commit is fixed implicitly by replacing it with
> +       the "amend!" commit's message body upon `rebase --autosquash`.

The first half of this description is clear. The second half gets
bogged down and difficult to decipher. It also seems to claim that
"reword:" can change the content of <commit>, which isn't accurate at
the UI level (even if it happens to reflect the underlying
implementation). I might have written the above description like this:

    Create a new commit which "fixes up" `<commit>` when applied with
    `git rebase --autosquash`. Plain `--fixup=<commit>` creates a
    "fixup!" commit which changes the content of `<commit>` but leaves
    its log message untouched. `--fixup=amend:<commit>` is similar but
    creates an "amend!" commit which also replaces the log message of
    `<commit>` with the log message of the "amend!" commit.
    `--fixup=reword:<commit>` creates an "amend!" commit which
    replaces the log message of `<commit>` with its own log message
    but makes no changes to the content of `<commit>`.

> +The resulting "fixup!" commit message will be the subject line
> +from the specified commit with a prefix of "fixup!". Can be used
> +with additional commit message option `-m`.

This gives details without providing meaning. If I didn't already know
how this all works, I think I'd probably be mystified about what it is
trying to say. Providing context by mentioning `git rebase
--autosquash` would help explain the significance of "fixup!".
Similarly, it's not clear on the surface why this mentions `-m` at
all. I might have written it like this:

    The commit created by plain `--fixup=<commit>` has a subject
    composed of "fixup!" followed by the subject line from <commit>,
    and is recognized specially by `git rebase --autosquash`. The `-m`
    option may be used to supplement the log message of the created
    commit, but the additional commentary will be thrown away once the
    "fixup!" commit is squashed into `<commit>` by `git rebase
    --autosquash`.

> +The `--fixup=amend:<commit>` form creates an "amend!" commit where
> +its 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. It refuses to create "amend!" commit if it's
> +commit message body is empty unless used with the
> +`--allow-empty-message` option.

This is reasonable, but does get into the weeds somewhat and uses
potentially unusual terms such as "seeded". It can be tightened up a
bit by building upon what was explained earlier for plain
`--fixup=<commit>`. To really round it out and give proper context for
understanding the purpose, it would also be helpful to explain how an
"amend!" commit is handled by `git rebase --autosquash`. I might have
written it like this:

    The commit created by `--fixup=amend:<commit>` is similar but its
    subject is instead prefixed with "amend!". The log message of
    <commit> is copied into the log message of the "amend!" commit and
    opened in an editor so it can be refined. When `git rebase
    --autosquash` squashes the "amend!" commit into `<commit>`, the
    log message of `<commit>` is replaced by the refined log message
    from the "amend!" commit. It is an error for the "amend!" commit's
    log message to be empty unless `--allow-empty-message` is
    specified.

> +The `--fixup=reword:<commit>` aliases `--fixup=amend:<commit> --only`
> +and it also creates an "amend!" commit, but here it records the same
> +tree as `HEAD`, 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`.

This gets too deep into the techno-speak by talking about "tree" and
`HEAD`. You can convey the same concept more simply by saying merely
that it creates an empty commit. I might have written it like this:

    `--fixup=reword:<commit>` is shorthand for `--fixup=amend:<commit>
    --only`. It creates an "amend!" commit with only a log message
    (ignoring any changes staged in the index). When squashed by `git
    rebase --autosquash`, it replaces the log message of `<commit>`
    without making any other changes.

> +Also, after fixing the commit using `--fixup`, with or without option
> +and rebased with `--autosquash`, the authorship of the original commit
> +remains unchanged. See linkgit:git-rebase[1] for details.

It sounds odd to start this sentence with "also". Perhaps:

    Neither "fixup!" nor "amend!" commits change authorship of
    `<commit>` when applied by `git rebase --autosquash`.

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

[Charvi Mendiratta]

I agree, below reworded documentation is easier to understand. I
realized I am quite poor at documenting the things (Apology for
mistakes, but I learned a lot and will try my best from next time)

I really appreciate all the suggestions and guidance.

Thanks and Regards,
Charvi


On Thu, 11 Mar 2021 at 13:18, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Wed, Mar 10, 2021 at 2:45 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> > ---
> > diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> > @@ -86,11 +86,40 @@ OPTIONS
> > +--fixup=[(amend|reword):]<commit>::
> > +       Construct a new commit for use with `rebase --autosquash`,
> > +       which fixes the specified commit. The plain form
> > +       `--fixup=<commit>` creates a "fixup!" commit, that allows
> > +       to fixup only the content of the specified commit and leave
> > +       it's commit log message untouched. When used with `amend:`
>
> s/it's/its/
>
> > +       or `reword:`, it creates "amend!" commit that is like "fixup!"
> > +       commit but it allows to fixup both the content and the commit
> > +       log message of the specified commit. The commit log message of
> > +       the specified commit is fixed implicitly by replacing it with
> > +       the "amend!" commit's message body upon `rebase --autosquash`.
>
> The first half of this description is clear. The second half gets
> bogged down and difficult to decipher. It also seems to claim that
> "reword:" can change the content of <commit>, which isn't accurate at
> the UI level (even if it happens to reflect the underlying
> implementation). I might have written the above description like this:
>
>     Create a new commit which "fixes up" `<commit>` when applied with
>     `git rebase --autosquash`. Plain `--fixup=<commit>` creates a
>     "fixup!" commit which changes the content of `<commit>` but leaves
>     its log message untouched. `--fixup=amend:<commit>` is similar but
>     creates an "amend!" commit which also replaces the log message of
>     `<commit>` with the log message of the "amend!" commit.
>     `--fixup=reword:<commit>` creates an "amend!" commit which
>     replaces the log message of `<commit>` with its own log message
>     but makes no changes to the content of `<commit>`.
>
> > +The resulting "fixup!" commit message will be the subject line
> > +from the specified commit with a prefix of "fixup!". Can be used
> > +with additional commit message option `-m`.
>
> This gives details without providing meaning. If I didn't already know
> how this all works, I think I'd probably be mystified about what it is
> trying to say. Providing context by mentioning `git rebase
> --autosquash` would help explain the significance of "fixup!".
> Similarly, it's not clear on the surface why this mentions `-m` at
> all. I might have written it like this:
>
>     The commit created by plain `--fixup=<commit>` has a subject
>     composed of "fixup!" followed by the subject line from <commit>,
>     and is recognized specially by `git rebase --autosquash`. The `-m`
>     option may be used to supplement the log message of the created
>     commit, but the additional commentary will be thrown away once the
>     "fixup!" commit is squashed into `<commit>` by `git rebase
>     --autosquash`.
>
> > +The `--fixup=amend:<commit>` form creates an "amend!" commit where
> > +its 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. It refuses to create "amend!" commit if it's
> > +commit message body is empty unless used with the
> > +`--allow-empty-message` option.
>
> This is reasonable, but does get into the weeds somewhat and uses
> potentially unusual terms such as "seeded". It can be tightened up a
> bit by building upon what was explained earlier for plain
> `--fixup=<commit>`. To really round it out and give proper context for
> understanding the purpose, it would also be helpful to explain how an
> "amend!" commit is handled by `git rebase --autosquash`. I might have
> written it like this:
>
>     The commit created by `--fixup=amend:<commit>` is similar but its
>     subject is instead prefixed with "amend!". The log message of
>     <commit> is copied into the log message of the "amend!" commit and
>     opened in an editor so it can be refined. When `git rebase
>     --autosquash` squashes the "amend!" commit into `<commit>`, the
>     log message of `<commit>` is replaced by the refined log message
>     from the "amend!" commit. It is an error for the "amend!" commit's
>     log message to be empty unless `--allow-empty-message` is
>     specified.
>
> > +The `--fixup=reword:<commit>` aliases `--fixup=amend:<commit> --only`
> > +and it also creates an "amend!" commit, but here it records the same
> > +tree as `HEAD`, 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`.
>
> This gets too deep into the techno-speak by talking about "tree" and
> `HEAD`. You can convey the same concept more simply by saying merely
> that it creates an empty commit. I might have written it like this:
>
>     `--fixup=reword:<commit>` is shorthand for `--fixup=amend:<commit>
>     --only`. It creates an "amend!" commit with only a log message
>     (ignoring any changes staged in the index). When squashed by `git
>     rebase --autosquash`, it replaces the log message of `<commit>`
>     without making any other changes.
>
> > +Also, after fixing the commit using `--fixup`, with or without option
> > +and rebased with `--autosquash`, the authorship of the original commit
> > +remains unchanged. See linkgit:git-rebase[1] for details.
>
> It sounds odd to start this sentence with "also". Perhaps:
>
>     Neither "fixup!" nor "amend!" commits change authorship of
>     `<commit>` when applied by `git rebase --autosquash`.