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

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

`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 [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 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

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

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 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

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 [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 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

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 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

`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 [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 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

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 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

On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta [off-list ref] 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).

On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta [off-list ref] wrote:

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

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.

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.

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.

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.

On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta [off-list ref] wrote:

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.

On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta [off-list ref] 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.

On Mon, Mar 1, 2021 at 3:52 AM Charvi Mendiratta [off-list ref] wrote:

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.

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.

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.

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.

Good.

Should this also be mentioning `reword!`?

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.

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

On Tue, 2 Mar 2021 at 11:13, Eric Sunshine [off-list ref] wrote:

On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta [off-list ref] wrote:

quoted

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.

On Wed, Mar 3, 2021 at 2:37 AM Charvi Mendiratta [off-list ref] wrote:

On Tue, 2 Mar 2021 at 03:45, Eric Sunshine [off-list ref] wrote:

quoted

quoted

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

On Wed, Mar 3, 2021 at 2:44 AM Charvi Mendiratta [off-list ref] wrote:

On Tue, 2 Mar 2021 at 12:09, Eric Sunshine [off-list ref] wrote:

quoted

On Mon, Mar 1, 2021 at 3:52 AM Charvi Mendiratta [off-list ref] wrote:

quoted

Signed-off-by: Charvi Mendiratta <redacted>
+       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.)

quoted

quoted

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

On Tue, 2 Mar 2021 at 04:06, Eric Sunshine [off-list ref] wrote:

quoted

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

On Tue, 2 Mar 2021 at 01:55, Eric Sunshine [off-list ref] 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.

On Tue, 2 Mar 2021 at 03:45, Eric Sunshine [off-list ref] wrote:

quoted

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

quoted

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

quoted

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

On Tue, 2 Mar 2021 at 12:09, Eric Sunshine [off-list ref] wrote:

On Mon, Mar 1, 2021 at 3:52 AM Charvi Mendiratta [off-list ref] wrote:

quoted

Signed-off-by: Charvi Mendiratta <redacted>
---

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.

quoted

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

quoted

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

quoted

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

quoted

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.

quoted

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

quoted

+       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

Hi Eric,

On Wed, 3 Mar 2021 at 13:16, Eric Sunshine [off-list ref] wrote:

On Wed, Mar 3, 2021 at 2:37 AM Charvi Mendiratta [off-list ref] wrote:

quoted

On Tue, 2 Mar 2021 at 03:45, Eric Sunshine [off-list ref] wrote:

quoted

quoted

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

On Wed, 3 Mar 2021 at 13:48, Eric Sunshine [off-list ref] 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.

quoted

quoted

quoted

+       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

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

`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 [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 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

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 [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 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

`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 [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 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);
 			}