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

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

Link to the related discussions:
https://lore.kernel.org/git/CAPSFM5f+cm87N5TO3V+rJvWyrcazybNb_Zu_bJZ+sBH4N4iyow@mail.gmail.com/

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              |  39 +++++--
 Documentation/git-rebase.txt              |  21 ++--
 builtin/commit.c                          |  97 ++++++++++++++++--
 commit.c                                  |  14 +++
 commit.h                                  |   3 +
 sequencer.c                               |  14 ---
 t/t3437-rebase-fixup-options.sh           |  30 +-----
 t/t7500-commit-template-squash-signoff.sh | 118 ++++++++++++++++++++++
 8 files changed, 271 insertions(+), 65 deletions(-)

--
2.29.0.rc1

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

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

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

Example usage:
$ git commit --fixup=amend:HEAD
$ git commit --fixup=amend:HEAD -m "clever commit message"

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 builtin/commit.c | 76 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 68 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 505fe60956..f2c5ad2e62 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,18 @@ 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);
+		format_commit_message(commit, fmt, &sb, &ctx);
+		free(fmt);
+		hook_arg1 = "message";
 		if (have_option_m)
 			strbuf_addbuf(&sb, &message);
-		hook_arg1 = "message";
+		else if (!strcmp(fixup_prefix,"amend"))
+			prepare_amend_commit(commit, &sb, &ctx);
 	} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
 		size_t merge_msg_start;
 
@@ -1170,7 +1189,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 +1246,29 @@ 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) {
+		/*
+		 * check if ':' occurs before '^' or '@', otherwise
+		 * fixup_message is a commit reference.
+		 */
+		fixup_commit = strpbrk(fixup_message, "^@:");
+		if (fixup_commit && *fixup_commit == ':' &&
+		    fixup_commit != fixup_message) {
+			*fixup_commit = '\0';
+			if (starts_with("amend", fixup_message)) {
+				fixup_prefix = "amend";
+				fixup_commit++;
+			} else {
+				die(_("only amend option can be used with --fixup"));
+			}
+		} 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 +1546,12 @@ 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: please do not translate [amend:]
+		 * Here "amend" is an option to the --fixup command
+		 * line flag, that creates amend! commit.
+		 */
+		OPT_STRING(0, "fixup", &fixup_message, N_("[amend:]commit"), N_("use autosquash formatted message to fixup or amend specified commit")),
 		OPT_STRING(0, "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 +1710,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

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

Example usage:
$ git commit --fixup=reword:HEAD~3
$ git commit --fixup=reword:HEAD~3 -m "new commit message"

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 builtin/commit.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index f2c5ad2e62..b5293f46d2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1171,6 +1171,21 @@ static void finalize_deferred_config(struct wt_status *s)
 		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
 }
 
+static void check_fixup_reword_options(void) {
+	if (whence != FROM_COMMIT) {
+		if (whence == FROM_MERGE)
+			die(_("You are in the middle of a merge -- cannot reword."));
+		else if (is_from_cherry_pick(whence))
+			die(_("You are in the middle of a cherry-pick -- cannot reword."));
+	}
+	if (all)
+		die(_("cannot combine reword option of --fixup with --all"));
+	if (also)
+		die(_("cannot combine reword option of --fixup with --include"));
+	if (only)
+		die(_("cannot combine reword option of --fixup with --only"));
+}
+
 static int parse_and_validate_options(int argc, const char *argv[],
 				      const struct option *options,
 				      const char * const usage[],
@@ -1256,11 +1271,17 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		if (fixup_commit && *fixup_commit == ':' &&
 		    fixup_commit != fixup_message) {
 			*fixup_commit = '\0';
-			if (starts_with("amend", fixup_message)) {
+			if (starts_with("amend", fixup_message) ||
+			    starts_with("reword", fixup_message)) {
 				fixup_prefix = "amend";
 				fixup_commit++;
+				if (*fixup_message == 'r') {
+					check_fixup_reword_options();
+					allow_empty = 1;
+					only = 1;
+				}
 			} else {
-				die(_("only amend option can be used with --fixup"));
+				die(_("only `amend` or `reword` options can be used with --fixup"));
 			}
 		} else {
 			fixup_commit = fixup_message;
@@ -1547,11 +1568,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_STRING('c', "reedit-message", &edit_message, N_("commit"), N_("reuse and edit message from specified commit")),
 		OPT_STRING('C', "reuse-message", &use_message, N_("commit"), N_("reuse message from specified commit")),
 		/*
-		 * TRANSLATORS: please do not translate [amend:]
-		 * Here "amend" is an option to the --fixup command
-		 * line flag, that creates amend! commit.
+		 * TRANSLATORS: please do not translate [(amend|reword):]
+		 * Here "amend" and "reword" are options to the --fixup
+		 * command line flag, that creates amend! commit.
 		 */
-		OPT_STRING(0, "fixup", &fixup_message, N_("[amend:]commit"), N_("use autosquash formatted message to fixup or amend specified commit")),
+		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
 		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
 		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
 		OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
-- 
2.29.0.rc1

[PATCH 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 | 118 ++++++++++++++++++++++
 1 file changed, 118 insertions(+)

diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 6d19ece05d..d2c34019c0 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,122 @@ 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: works with -m option' '
+	commit_for_rebase_autosquash_setup &&
+	cat >expected <<-EOF &&
+	amend! target message subject line
+
+	reword commit message
+	EOF
+	git commit --fixup=reword:HEAD~ -m "reword commit message" &&
+	get_commit_msg HEAD >actual &&
+	test_cmp expected 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 &&
+	test_must_fail git commit --fixup=amend:HEAD~ -m " " 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
+	git commit --fixup=amend:HEAD~ -m " " --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
+do
+	test_fixup_reword_opt $opt
+done
+
+test_expect_success '--fixup=reword: -F give error message' '
+	echo "fatal: Only one of -c/-C/-F/--fixup can be used." >expect &&
+	test_must_fail git commit --fixup=reword:HEAD~ -F msg  2>actual &&
+	test_cmp expect actual
+'
 
 test_expect_success 'commit --squash works with -F' '
 	commit_for_rebase_autosquash_setup &&
-- 
2.29.0.rc1

[PATCH 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 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>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 Documentation/git-commit.txt | 39 ++++++++++++++++++++++++++++++------
 Documentation/git-rebase.txt | 21 ++++++++++---------
 2 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 17150fa7ea..9a60876845 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git commit' [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]
-	   [--dry-run] [(-c | -C | --fixup | --squash) <commit>]
+	   [--dry-run] [(-c | -C | --squash) <commit> | --fixup [(amend|reword):]<commit>)]
 	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
 	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
 	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
@@ -86,11 +86,38 @@ OPTIONS
 	Like '-C', but with `-c` the editor is invoked, so that
 	the user can further edit the commit message.
 
---fixup=<commit>::
-	Construct a commit message for use with `rebase --autosquash`.
-	The commit message will be the subject line from the specified
-	commit with a prefix of "fixup! ".  See linkgit:git-rebase[1]
-	for details.
+--fixup=[(amend|reword):]<commit>::
+	When used without options, lets's say `git commit --fixup=<commit>`,
+	it creates a "fixup!" commit where the commit message will be
+	the subject line from the specified commit with a prefix of
+	"fixup! ". The resulting "fixup!" commit is further used with
+	`git rebase --autosquash` to fixup the content of the specified
+	commit.
+
+	When used with option `amend`, let's say
+	`git commit --fixup=amend:<commit>`, it creates a "amend!" commit
+	to fixup both the content and the commit log message of the
+	specified commit. The resulting "amend!" commit's commit message
+	subject will be the subject line from the specified commit with a
+	prefix of "amend! " and the message body will be commit log message
+	of the specified commit. It also invokes an editor seeded with the
+	"amend!" commit log message to allow to edit further. And it denies
+	to create "amend!" commit if it's commit message body is empty unless
+	used with `allow-empty-message` option. "amend!" commit when rebased
+	with `--autosquash` will fixup the contents and replace the commit
+	message of the specified commit with the "amend!" commit's message
+	body.
+
+	When used with alternative option `reword`, let's say
+	`git commit --fixup=reword:<commit>`, it works similar to `amend`
+	option, but here it creates an empty "amend!" commit, i.e it does
+	not take any staged changes and only allows to fixup the commit
+	message of the specified commit. It will reword the specified
+	commit when it is rebased with `--autosquash`.
+
+	`--fixup`, with or without option, can be used with additional
+	commit message option `-m` but not with `-F`/`-c`/`-C`. See
+	linkgit:git-rebase[1] for details.
 
 --squash=<commit>::
 	Construct a commit message for use with `rebase --autosquash`.
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 8bfa5a9272..ffea76e53b 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -593,16 +593,17 @@ See also INCOMPATIBLE OPTIONS below.
 
 --autosquash::
 --no-autosquash::
-	When the commit log message begins with "squash! ..." (or
-	"fixup! ..."), and there is already a commit in the todo list that
-	matches the same `...`, automatically modify the todo list of rebase
-	-i so that the commit marked for squashing comes right after the
-	commit to be modified, and change the action of the moved commit
-	from `pick` to `squash` (or `fixup`).  A commit matches the `...` if
-	the commit subject matches, or if the `...` refers to the commit's
-	hash. As a fall-back, partial matches of the commit subject work,
-	too.  The recommended way to create fixup/squash commits is by using
-	the `--fixup`/`--squash` options of linkgit:git-commit[1].
+	When the commit log message begins with "squash! ..." (or "fixup! ..."
+	or "amend! ..."), and there is already a commit in the todo list that
+	matches the same `...`, automatically modify the todo list of
+	`rebase -i`, so that the commit marked for squashing comes right after
+	the commit to be modified, and change the action of the moved commit
+	from `pick` to `squash` (or `fixup` or `fixup -C`) respectively. A commit
+	matches the `...` if the commit subject matches, or if the `...` refers
+	to the commit's hash. As a fall-back, partial matches of the commit
+	subject work, too. The recommended way to create fixup/squash/amend
+	commits is by using the `--fixup=[amend|reword]`/`--squash` options of
+	linkgit:git-commit[1].
 +
 If the `--autosquash` option is enabled by default using the
 configuration variable `rebase.autoSquash`, this option can be
-- 
2.29.0.rc1

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

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

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

Sorry, but it is not so clear what these examples are trying to
illustrate.

The first one is to add a new commit to later amend the tip commit
(It is a bit of mystery why the user does not do a more usual "git
commit --amend" right there, though, and such a mystery may distract
readers.  If the commit were not at the tip, e.g.  HEAD~3, it may be
less distracting).

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

I think we can just remove the "example" from here and explain the
feature well in the end-user facing documentation.

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

Isn't it that you only intend to parse:

    --fixup
    --fixup=amend:<any string that names a commit>
    --fixup=<any string that names a commit>

and later extend it to allow keywords other than "amend"?

I can understand that you are trying to avoid getting fooled by
things like

	--fixup='HEAD^{/commit message with a colon : in it}'

but why special case only ^ and @?  This feels brittle (note that I
said "things like", exactly because I do not know if any string that
can name a commit must have "@" or "^" appear before ":" if it is to
have ":" in anywhere, which is what this code assumes).

Instead, you can find the first colon, check for known keywords (or
a string that consists only of alnums to accomodate for future
enhancement), and treat any garbage that happens to have a colon
without the "keyword" as fixup_commit.  I.e.  something along this
line...

		const char alphas[] = "abcde...xyz";
		size_t kwd_len;

		kwd_len = strspn(fixup_message, alphas);
		if (kwd_len && fixup_message[kwd_len] == ':') {
			/* found keyword? */
			fixup_message[kwd_len] = '\0';
			if (!strcmp("amend", fixup_message)) {
				... do the amend:<commit> thing ...
#if in-next-step-when-you-add-support-for-reword
			} else if (!strcmp("reword", fixup_message)) {
				... do the reword:<commit> thing ...
#endif
			} else {
				die(_("unknown --fixup=%s:<commit>",
					fixup_message));
			}
		} else {
			/* the entire fixup_message is the commit */
		}

Re: [PATCH 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>`.
>
> Example usage:
> $ git commit --fixup=reword:HEAD~3
> $ git commit --fixup=reword:HEAD~3 -m "new commit message"

The same comment applies to the above as an earlier step.

> +static void check_fixup_reword_options(void) {
> +	if (whence != FROM_COMMIT) {
> +		if (whence == FROM_MERGE)
> +			die(_("You are in the middle of a merge -- cannot reword."));
> +		else if (is_from_cherry_pick(whence))
> +			die(_("You are in the middle of a cherry-pick -- cannot reword."));
> +	}
> +	if (all)
> +		die(_("cannot combine reword option of --fixup with --all"));
> +	if (also)
> +		die(_("cannot combine reword option of --fixup with --include"));
> +	if (only)
> +		die(_("cannot combine reword option of --fixup with --only"));
> +}

Not just these options, wouldn't it be an error to ask to commit
anything but an empty commit?  E.g. shouldn't this sequence

	edit builtin/commit.c
	git commit --fixup=reword:HEAD~3 -- builtin/commit.c

trigger an error, as we will *not* be taking any change made to the
working tree file?

Or is that implicitly covered by some other code?

In any case, we'd need a test for that (this is just a mental note
for myself---I haven't finished reading the series to the end, so
you may have one already).

Thanks.

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

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

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

Isn't an equal '=' sign missing somewhere?

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

As I suspected earlier, a pathspec is not tested here, but it should
be.

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

Why?  If you can use -m msg, you should be able to use -F msgfile,
too, no?

>  test_expect_success 'commit --squash works with -F' '
>  	commit_for_rebase_autosquash_setup &&

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

[Charvi Mendiratta]

Hi Junio,

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

Yes, here it gets _appended_ the log message.  I agree this seems a bit
confusing.

> I think we can just remove the "example" from here and explain the
> feature well in the end-user facing documentation.
>

Okay, I will remove it from here and add it in the documentation.

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

Agree.

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

Okay, I got this...

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

...Thanks, for pointing this out. Also, in the above method for
alnum I think we can initialize an array of alnum[] instead of
alphas[]. Or otherwise I was thinking to instead check:
           if (!isalnum(*c) && *c == ':')
i.e to check that first non alnum char in fixup_message is ':' and
returning it's position to extract both fixup_prefix and fixup_commit.

Will look into it and update in the next revision.

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

[Charvi Mendiratta]

On Thu, 18 Feb 2021 at 01:26, Junio C Hamano <gitster@pobox.com> wrote:
[...]
> The same comment applies to the above as an earlier step.
>

Okay, will remove it.

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

I admit this is a bug here. Thanks for pointing this out and will add the
check for pathspec.

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

[Charvi Mendiratta]

On Thu, 18 Feb 2021 at 01:29, Junio C Hamano <gitster@pobox.com> wrote:
>
[...]
> > +for opt in --all --include --only
> > +do
> > +     test_fixup_reword_opt $opt
> > +done
>
> As I suspected earlier, a pathspec is not tested here, but it should
> be.
>

Okay, I will add it here.

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

Earlier I was thinking to let the `--fixup=amend:`  use the same options as of
current `--fixup=` . But yes I agree that there should be  -F option
also with `amend`
and `reword`.

Thanks for the corrections, will do all the changes and update in the
next revision.

Thanks and Regards,
Charvi

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

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

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

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

> ...Thanks, for pointing this out. Also, in the above method for
> alnum I think we can initialize an array of alnum[] instead of
> alphas[]. Or otherwise I was thinking to instead check:
>            if (!isalnum(*c) && *c == ':')

Sure a loop is fine, or alnum[] is fine, or just alpha[] is OK, I
would think.  Do you foresee you'd need --fixup=chomp124:<commit>?
I somehow doubt it.

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

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

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

You must dedent the second and the subsequent paragraphs and replace
each of these blank lines that mark inter-paragraph breaks with a
line with a single plus '+' sign on it.  See how "is mutually
exclusive" explanation is appended to the description of "-m" as its
second paragraph.

Side note.  It probably needs "exclusive -> incompatible".

The description in git-rebase.txt you touched is another good
example.  Mimic the way its second paragraph "If the autosquash is
enabled..."  is formatted.

Thanks.

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

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

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

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

Hmph, I was actually imagining the opposite---a context that does
not want to take -c/-C/-F would not want to take -m, either.

Why is -m so special, and a lot more importantly, what would a user
want to achieve by using "-m more-text" combined with this
"--fixup=reword:<commit>" or "--fixup=amend:<commit>" feature?

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

Having said that, we may regret if we did not include some
punctuation to allow for a multi-word keyword.  IOW, "alpha plus
dash" might be a reasonable minimum.

But what keyword --fixup=<keyword>:<commit> can take is entirely
under our control, so it is not all that unreasonable if we just
forced our developers some discipline to pick a single-word keyword
for any of their future enhancements.  It's not like we are opening
up extensibility to the end-users, who may complain that the way
they can spell their new <keyword> is too limited.  So if we already
have alpha[] and/or a helper function that does strspn(alpha) that
we can reuse elsewhere, I do not think it is worth to try supporting
punctuation.

Thanks.

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

[Charvi Mendiratta]

Hi Junio,

On Fri, 19 Feb 2021 at 00:49, Junio C Hamano <gitster@pobox.com> wrote:
[...]
> In what situation would a user use "-m 'appended pieces of text'"
> option, together with "--fixup=amend:<commit>"?  I am wondering if
> we want such a "append to" feature, or is it easier to understand
> for end-users if "-m", "-F", "-C" and "-c" (the common trait of
> these options is that they contribute to the log message text)
> are made incompatible with --fixup=amend:<commit>.
>

For end-users "-m" or "-F" will make it easier to prepare an "amend!"
commit. Because using the "-m" reduces the cost of opening an editor
to prepare "amend!" commit and it can be done with command line only.
So, I think we can keep -m/-F options.

(Explained more about "-m" use in next thread)

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

For naming the suboptions, I don't see any use of alnum. Earlier, I
thought that it could be possible to add the option of _commit
name/ID_, although I am not sure about any particular use case of it
for future. So I thought of changing it to an alnum[] but I also agree
that we can use just alpha[].

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

[Charvi Mendiratta]

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

Got it. Thanks for pointing this out, I will correct it.

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

[Charvi Mendiratta]

On Fri, 19 Feb 2021 at 00:56, Junio C Hamano <gitster@pobox.com> wrote:
>
[...]
> Hmph, I was actually imagining the opposite---a context that does
> not want to take -c/-C/-F would not want to take -m, either.
>
> Why is -m so special, and a lot more importantly, what would a user
> want to achieve by using "-m more-text" combined with this
> "--fixup=reword:<commit>" or "--fixup=amend:<commit>" feature?

If we run without '-m' option like below:
$ git commit --fixup=reword:<commit>

Then it pops the editor with default "amend!" commit's message i.e:

amend! subject of <commit> we are fixing.

commit log message of <commit> we are fixing.

(Here the end-user is free to edit the above message body of "amend!" commit )

On the other hand, if used with -m option like below:
$ git commit --fixup=reword:<commit> -m "edited <commit> message"

Then it will not pop the editor and the prepared "amend!" commit is :

amend! subject of <commit> we are fixing.

edited <commit> message.

So, with the "-m" option users can do it with the command line only.
And in both the cases upon `git rebase --autosquash` the commit log
message of <commit> we are fixing, will automatically be replaced by
the commit message body of "amend!" commit.

Hope that explains the working and I also wonder if we can improve it
to make it more user friendly ?

Thanks and Regards,
Charvi

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

[Charvi Mendiratta]

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

Oh, yes punctuation is another point. Then for now maybe we can just make the
helper function to check for alpha[] only.

Thanks and Regards,
Charvi

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

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

> For end-users "-m" or "-F" will make it easier to prepare an "amend!"
> commit. Because using the "-m" reduces the cost of opening an editor
> to prepare "amend!" commit and it can be done with command line only.

Hmph.  That is not very convicing to me.  The user is motivated
enough to fix a wrong commit log message and replace it with an
improved one by using the "--fixup:amend" feature---why would that
improved message can sufficiently be written with just an "-m
message", which by definition would be much less capable of
preparing a well-thought-out message than with an editor?

Yes, with "-m", you can add some short string easily at the end of
the existing commit message without opening an editor.  But I am
trying to see why it is a good thing to be able to do so in the
first place.  It is very easy to make typoes while doing so and it
would be hard to fix these typoes, exactly because you are doing so
without being in an editor.  And the whole point of --fixup=amend is
about improving the message (as opposed to --fixup that is to record
the contents that have already been improved in the index).

This is why I kept asking what the use case would look like.  I am
trying to find out if you have a good end-user story in mind that we
can use in the documentation to sell the feature to end-users, but I
am not seeing one.

Is the combination of "--fixup=amend" and "-m <msg>" meant to be
used primarily "to leave a note to myself" when the user runs
--fixup=amend:<commit>, to just record the points to be elaborated
when the message is written for real much later?  E.g.

    ... hack hack hack ...
    ... good stopping point, but cannot afford time to write
    ... a good log message now
    $ git commit --fixup=amend:HEAD~2 -m 'talk about X, Y and Z' -a
    ... hack hack hack ...
    ... possibly doing something entirely different ...
    ... finally comes back to finish cleaning up the branch for real ...
    $ git rebase --autosquash -i origin

And one of the insn created in the todo sheet would be amend!, whose
commit message has the message taken from the original HEAD~2 PLUS the
reminder to the user that s/he needs to talk about X, Y and Z?  And
the user at that point writes more comprehensive message about these
three things?

That is a made-up example of what "appending some short strings
possibly full of typoes without having to open an editor" could be
useful for.  I am not sure if it is truly useful, or if it is just a
hand wavy excuse not to mark -m/-F incompatible with --fixup=amend
without a real merit [*].

    Side note: one reason why I do not find it realistic is that it
    is unlikely that the user would use --fixup=amend to slurp in
    the original log message when recording "good stopping point,
    but cannot afford time" fixup.  "--squash HEAD~2 -m <msg>" would
    be much faster to record the "note to myself" reminder, and when
    the user finally comes back to clean things up, the amount of
    work to edit the original's message while looking at the "note
    to myself" appended at the end would not be any different in
    either workflow.

In any case, that was the kind of answer(s) I was looking for when I
asked "what is this good for?" question.

Thanks.

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

[Charvi Mendiratta]

Hi,

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

I was in my mind that, as we can easily prepare the commit using `git
commit -m "commit message"`. Similarly, we can extend this working
with `git commit --fixup=amend:<commit> -m "new commit message"`. And
the difference is that in the later one the goal of changing the
commit message will be achieved after `git rebase --autosquash` i.e
the later command works with sequencer. This will easily allow us to
write a new message for the commit we are fixing up through the
command line only similar to `git commit -m` and if we need to fixup
the commit msg then it can be done without `-m` and the editor gets
open with a seeded commit message we want to fixup. Also the same
working applies to `reword` option also and may allow to reword the
commit msg from command line only.

But I am still doubtful, as I agree with above that the main concern
is to only fixup the wrong commit message and in that case the "-m"
option can't be that much productive or maybe confusing for users.

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

But here in this case, after `git rebase --autosquash -i origin`, it
will not open editor again and user is not allowed to write more
comprehensive message about these three things, unless the user
manually changes from automatically converted `pick` to `fixup -C`,
to `fixup -c` command in the rebase todo list that gets opened after
`git rebase --autosquash`. And for this case `git commit --squash` is
more easy to use.

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

Yes, I also thought the same and agree with it.

> In any case, that was the kind of answer(s) I was looking for when I
> asked "what is this good for?" question.
>

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

Thanks and Regards,
Charvi

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

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

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

FWIW, I do not have a strong opposition against -m/-F that is not
rejected when --fixup=amend is in use.  It is OK for Git to accept
useless combinations of options that do not help end-users.  A
combination that is not useful will be left unused, which is not a
great loss.  So, if it is more work to make the code notice when
these options are given in useless combinations and stop with an
error message than just accepting and doing useless thing, I am OK
if we left them as they are.

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

Thanks.

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

[Charvi Mendiratta]

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

>  So, if it is more work to make the code notice when
> these options are given in useless combinations and stop with an
> error message

Sorry I didn't get this and would like to once confirm here that, are
you pointing to output an error message when the `-m`/`-F` option is
passed with `git --fixup=amend/reword` ? Because I think we can do
this also. Otherwise ....

>than just accepting and doing useless thing, I am OK
> if we left them as they are.
>

> I was just hoping that letting "-m <message>" and "--fixup=amend"
> used at the same time would support a great use case, and because I
> didn't think of any, I asked the person who allowed the combination
> (that is you) what the intended use cases are.  Actively supporting
> a combination because it gives users great value is more satisfying
> than just leaving useless combination to exist only because it does
> not actively hurt.  When users say "The command can take these two
> options at the same time, but I cannot figure out what good the
> combination is for. It feels utterly useless to use them together"
> it also is much easier to answer them if we can say "because by
> using these two together, you can do this great thing" instead of
> having to say "we do not think it makes much sense to use these two
> options together."
>
....If we allow both `m` and `F` to work with `git commit
--fixup=amend/reword` with the same working as it is doing now i.e to
use `m` to write new commit message, upon `--autosquash`, If it is
okay? then I also agree to update the documentation more precisely and
include the uses when passed with `m` /`F`(not yet added) option.

(Please correct me if I am wrong)

Thanks and Regards,
Charvi

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

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

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

If we were to make -m/-F incompatible with these new features, then
sure, we'd notice the combination, show an error message and abort.

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

What would that more precise documentation would say, though?  

"'-m message' gets appended to the message taken from the original
commit"?  Saying that alone, without explaining why doing such an
appending is useful, would puzzle users and makes them ask "but why
such a useless feature exist?" and that was why I was trying to
figure out what it is useful for with you, which I think we have
failed to do so far.

My preference at this point is to error out the combination that we
cannot figure out how it could be useful at this moment, so that
users who find how it would be useful to come to us and present a
hopefully good case for using -m <msg> with --fixup=amend:<commit>.
I am assuming that allowing the combination at that point is easy,
and the user request will give us a good use case we can use in the
documentation to explain for what purpose a user may want to use -m
<msg> to append a short string at the end.  The end users' use case
we see at that point might even suggest that it would be more useful
to prepend (as opposed to append) the message we get from -m <msg>
to the original log message, and such a change will not be possible
if we just choose to append without thinking through the use case we
intend to support and release "we do not know what good it would do
to append with -m <msg>, but that is what the code happens to do now"
version to the users, as people will depend on the behaviour of any
released versions.

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

[Charvi Mendiratta]

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

Okay, I admit prepending the msg can be another way. Thanks a lot for
clarifying in detail, I completely agree with it and will error out
the combination for now.

Thanks and Regards,
Charvi

Re: [PATCH 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.
> Example usuage:
> git commit --fixup=reword:<commit>

We used to have only --fixup that was meant to squeeze in minor
corrections to the contents recorded, and it kept the log message
of the original commit intact.

Now we have two other ways, --fixup=reword that is meant to correct
only the log message while keeping the contents intact from the
original, and --fixup=amend that is meant to allow users to do both.
They are nice additions to our toolbox.

While trying to use the --fixup=amend myself to "touch up" somebody
else's work today, another thing that we did not discuss so far came
to my mind (sorry, if this was discussed and resolved in your
previous discussions with other reviewers).  What should we do to
the authorship?

For the original --fixup, it is reasonably obvious that the original
authorship should be kept, as the intended use case is to make a
small tweak that does not change the intention of the commit in any
way (and that is why the log message from the original is kept), and
with --fixup=reword, it would probably be the same (the contents
were written by the original author alone, and the person fixing-up
is not changing only the log message).  So these two have a
reasonably good default model for the authorship information for the
final outcome: the original authorship should be kept (of course,
the user can easily take over the authorship later with "git commit
--amend --reset-author" perhaps run as part of "rebase -i", if the
contribution is significant enough to deserve the transfer of the
authorship).

But I am not sure what the default behaviour for the authorship when
--fixup=amend:<commit> is used to update somebody else's commit.  I
think it is OK to leave it to whatever the code happens to do right
now (simply because I have no strong reason to vote for either way
between keeping the original and letting the amending user take it
over), but I think it should be documented what happens in each
case.

Thanks.

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

[Charvi Mendiratta]

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

Yes, for the authorship similar to `--fixup`, when used with suboptions
`amend` or `reword`, it keeps the original authorship.

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

Okay, I agree. We have included in the tests where we check both the
resulting commit message and the author details  but yes I will document
it as well.

Thanks and Regards,
Charvi

[PATCH v2 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 v1:
(Thanks Junio C Hamano for the reviews and pointing out the changes required in v1)

* (As `reword` and `amend` suboptions of `--fixup` uses apha
  characters only) add helper function that does strspn(alpha) to
  detect that suboptions are passed to `--fixup` instead of checking
  if ':' occurs before '^' or '@'.

* update the `check_fixup_reword_options()` function and disallow
  `reword` option with path(argc), interactive and patch_interactive.
  Also update the test script (t7500).

* make '-m' commit message option incompatible with `amend` and
  `reword` suboption, as it is more expected to edit the original
  commit message while preparing "amend!" commit and using `-m` for
  directly appending the commit message results in the confusion at
  the user end. So, error out when combined with `m` and update
  the respective tests.

* slight improvements in commit messages and update the documention
  (git-commit.txt) to include the above changes.

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              |  39 ++++++-
 Documentation/git-rebase.txt              |  21 ++--
 builtin/commit.c                          | 117 +++++++++++++++++--
 commit.c                                  |  14 +++
 commit.h                                  |   3 +
 sequencer.c                               |  14 ---
 t/t3437-rebase-fixup-options.sh           |  30 +----
 t/t7500-commit-template-squash-signoff.sh | 134 ++++++++++++++++++++++
 8 files changed, 306 insertions(+), 66 deletions(-)

--
2.29.0.rc1

[PATCH v2 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

[PATCH v2 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.

Inorder 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 | 89 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 80 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 505fe60956..56ae15a762 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,24 @@ 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";
+
+		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 +1177,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 +1201,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 +1258,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 +1557,12 @@ 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: please do not translate [amend:]
+		 * Here "amend" is an option to the --fixup command
+		 * line flag, that creates amend! commit.
+		 */
+		OPT_STRING(0, "fixup", &fixup_message, N_("[amend:]commit"), N_("use autosquash formatted message to fixup or amend specified commit")),
 		OPT_STRING(0, "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 +1721,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

[PATCH v2 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 | 46 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 56ae15a762..82e77aa61d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1177,6 +1177,27 @@ static void finalize_deferred_config(struct wt_status *s)
 		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
 }
 
+static void check_fixup_reword_options(int argc) {
+	if (whence != FROM_COMMIT) {
+		if (whence == FROM_MERGE)
+			die(_("You are in the middle of a merge -- cannot reword."));
+		else if (is_from_cherry_pick(whence))
+			die(_("You are in the middle of a cherry-pick -- cannot reword."));
+	}
+	if (argc)
+		die(_("cannot combine reword option of --fixup with paths"));
+	if (patch_interactive)
+		die(_("cannot combine reword option of --fixup with --patch"));
+	if (interactive)
+		die(_("cannot combine reword option of --fixup with --interactive"));
+	if (all)
+		die(_("cannot combine reword option of --fixup with --all"));
+	if (also)
+		die(_("cannot combine reword option of --fixup with --include"));
+	if (only)
+		die(_("cannot combine reword option of --fixup with --only"));
+}
+
 /* returns the length of intial segment of alpha characters only */
 static size_t get_alpha_len(char *fixup_message) {
 	const char alphas[] = "abcdefghijklmnopqrstuvwxyz";
@@ -1261,18 +1282,25 @@ static int parse_and_validate_options(int argc, const char *argv[],
 
 	if (fixup_message) {
 		/*
-		 * As `amend` suboption contains only alpha
-		 * character. So check if first non alpha
-		 * character in fixup_message is ':'.
+		 * As `amend`/`reword` suboptions contains only alpha
+		 * characters. So check if first non alpha character
+		 * in fixup_message is ':'.
 		 */
 		size_t len = get_alpha_len(fixup_message);
 		if (len && fixup_message[len] == ':') {
 			fixup_message[len] = '\0';
 			fixup_commit = fixup_message + ++len;
-			if (starts_with("amend", fixup_message))
+			if (starts_with("amend", fixup_message) ||
+				starts_with("reword", fixup_message)) {
 				fixup_prefix = "amend";
-			else
+				if (*fixup_message == 'r') {
+					check_fixup_reword_options(argc);
+					allow_empty = 1;
+					only = 1;
+				}
+			} else {
 				die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
+			}
 		} else {
 			fixup_commit = fixup_message;
 			fixup_prefix = "fixup";
@@ -1558,11 +1586,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_STRING('c', "reedit-message", &edit_message, N_("commit"), N_("reuse and edit message from specified commit")),
 		OPT_STRING('C', "reuse-message", &use_message, N_("commit"), N_("reuse message from specified commit")),
 		/*
-		 * TRANSLATORS: please do not translate [amend:]
-		 * Here "amend" is an option to the --fixup command
-		 * line flag, that creates amend! commit.
+		 * TRANSLATORS: please do not translate [(amend|reword):]
+		 * Here "amend" and "reword" are options to the --fixup
+		 * command line flag, that creates amend! commit.
 		 */
-		OPT_STRING(0, "fixup", &fixup_message, N_("[amend:]commit"), N_("use autosquash formatted message to fixup or amend specified commit")),
+		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
 		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
 		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
 		OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
-- 
2.29.0.rc1

[PATCH v2 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..3b3a39fcaf 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 paths" >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 v2 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 v2 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 | 39 ++++++++++++++++++++++++++++++------
 Documentation/git-rebase.txt | 21 ++++++++++---------
 2 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 17150fa7ea..bc50301b1c 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git commit' [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]
-	   [--dry-run] [(-c | -C | --fixup | --squash) <commit>]
+	   [--dry-run] [(-c | -C | --squash) <commit> | --fixup [(amend|reword):]<commit>)]
 	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
 	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
 	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
@@ -86,11 +86,38 @@ OPTIONS
 	Like '-C', but with `-c` the editor is invoked, so that
 	the user can further edit the commit message.
 
---fixup=<commit>::
-	Construct a commit message for use with `rebase --autosquash`.
-	The commit message will be the subject line from the specified
-	commit with a prefix of "fixup! ".  See linkgit:git-rebase[1]
-	for details.
+--fixup=[(amend|reword):]<commit>::
+	When used without options, lets's say `git commit --fixup=<commit>`,
+	it creates a "fixup!" commit where the commit message will be
+	the subject line from the specified commit with a prefix of
+	"fixup! ". The resulting "fixup!" commit is further used with
+	`git rebase --autosquash` to fixup the content of the specified
+	commit.
++
+When used with option `amend`, let's say `git commit --fixup=amend:<commit>`,
+it creates a "amend!" commit to fixup both the content and the commit log
+message of the specified commit. The resulting "amend!" commit's commit
+message subject will be the subject line from the specified commit with a
+prefix of "amend! " and the message body will be commit log message of the
+specified commit. It also invokes an editor seeded with the "amend!" commit
+log message to allow to edit further. And it denies to create "amend!" commit
+if it's commit message body is empty unless used with `allow-empty-message`
+option. "amend!" commit when rebased with `--autosquash` will fixup the
+contents and replace the commit message of the specified commit with the
+"amend!" commit's message body.
++
+When used with alternative option `reword`, let's say
+`git commit --fixup=reword:<commit>`, it works similar to `amend` option, but
+here it creates an empty "amend!" commit, i.e it does not take any staged
+changes and only allows to fixup the commit message of the specified commit.
+It will reword the specified commit when it is rebased with `--autosquash`.
++
+Unlike `--fixup` without options, `--fixup=[amend/reword]:` is incompatible with
+`-m` commit message option.
++
+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 v2 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 | 46 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 56ae15a762..82e77aa61d 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1177,6 +1177,27 @@ static void finalize_deferred_config(struct wt_status *s)
>  		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
>  }
>  
> +static void check_fixup_reword_options(int argc) {
> +	if (whence != FROM_COMMIT) {
> +		if (whence == FROM_MERGE)
> +			die(_("You are in the middle of a merge -- cannot reword."));
> +		else if (is_from_cherry_pick(whence))
> +			die(_("You are in the middle of a cherry-pick -- cannot reword."));
> +	}
> +	if (argc)
> +		die(_("cannot combine reword option of --fixup with paths"));

It would be easier if the user is told "foo" in the message when

    $ git commit --fixup=reword:HEAD~ -- foo

(from your tests) is attempted, no?

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

> +					check_fixup_reword_options(argc);
> +					allow_empty = 1;
> +					only = 1;

OK.  We make sure that there is no pathspec and then by giving
only==1, we ignore any difference that already may exist between
HEAD and the index.  IOW

	edit somefile
	git add somefile
	git commit --fixup=reword:<commit>

will still record the same tree as HEAD, without affected by the
addition of somefile to the index that is done before.

Good.

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

I am not sure this comment is all that helpful to the translaters.
If they are not allowed to translate <amend|reword> part, telling
them what that part means does not help them very much.

	Leave "[(amend|reword):]" as-is, and only translate <commit>.

would be more direct without distracting them with useless piece of
information, no?

>  		 */
> -		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")),

Re: [PATCH v2 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>::
> +	When used without options, lets's say `git commit --fixup=<commit>`,
> +	it creates a "fixup!" commit where the commit message will be

Be careful to use word 'option' that refers to something other than
what a casual reader would think of an `--option` in the description
of the `--fixup` option.

	Without `amend:` or `reword:`, create a `fixup!` commit where...

would be sufficient, no?

> +	the subject line from the specified commit with a prefix of
> +	"fixup! ". The resulting "fixup!" commit is further used with
> +	`git rebase --autosquash` to fixup the content of the specified
> +	commit.
> ++
> +When used with option `amend`, let's say `git commit --fixup=amend:<commit>`,
> +it creates a "amend!" commit to fixup both the content and the commit log

	The `--fixup=amend:<commit>` form creates an "amend!" commit to...

> +message of the specified commit. The resulting "amend!" commit's commit
> +message subject will be the subject line from the specified commit with a
> +prefix of "amend! " and the message body will be commit log message of the

While that SP inside the double-quote may be technically more
correct (and it was inherited from the original), I think with 'a
prefix of "amend!"' is still understandable and a lot easier to
read, especially because you'd mention "amend!"  a few more times in
the same paragraph below.

The same comment applies to "fixup! " above.

> +specified commit. It also invokes an editor seeded with the "amend!" commit
> +log message to allow to edit further. And it denies to create "amend!" commit

"amend!" commit log message -> log message of the "amend!" commit

denies -> refuses

> +if it's commit message body is empty unless used with `allow-empty-message`
> +option. "amend!" commit when rebased with `--autosquash` will fixup the

with the `--allow-empty-message` option.

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

	The `--fixup=reword:<commit>` form creates an `amend!`
	commit similar to `--fixup=amend:<commit>` creates, but it
	records the same tree as `HEAD`, i.e. it does not ...

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

	The `--fixup=amend:` and `--fixup=reword:` forms cannot be
	used with other options to add to the commit log message,
	e.g. `-m`.

Again, why is `-m` so special?  Shouldn't -F/-c/-C also be
incompatible?

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

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

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

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

In order to prevent ...

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

Don't indent the second line unnecessarily too deep.

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

I am not sure how well this strategy of special case only two amend!
scales.  What would happen when we --fixup another "fixup!"commit?

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

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

Unnecessary () around have_option_m, and missing SP after ",".

> +			strbuf_addbuf(&sb, &message);
> +
> +		if (!strcmp(fixup_prefix,"amend")) {

Missing SP after "," (I won't repeat---please re-check the whole
patch series before rerolling).

> +			if (have_option_m)
> +				die(_("cannot combine -m with --fixup:%s"), fixup_message);
> +			else
> +				prepare_amend_commit(commit, &sb, &ctx);

Hmph, why is -m so special?  Should we allow --fixup=amend:<cmd>
with -F (or -c/-C for that matter), or are these other options
caught at a lot higher layer already and we do not have to check
them here?

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

It would be easier to follow to write it this way, no?

			fixup_message[len++] = '\0';
			fixup_commit = fixup_message + len;

> +			if (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;
> +		}
> +	}

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

[Charvi Mendiratta]

Hi Junio,

On Fri, 26 Feb 2021 at 02:03, Junio C Hamano <gitster@pobox.com> wrote:
>
[...]
> > +     if (argc)
> > +             die(_("cannot combine reword option of --fixup with paths"));
>
> It would be easier if the user is told "foo" in the message when
>
>     $ git commit --fixup=reword:HEAD~ -- foo
>
> (from your tests) is attempted, no?
>

Okay, will print the passed argument in the message.

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

Yes, I agree and will correct it.

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

[Charvi Mendiratta]

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

Okay, I agree this is more clear.

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

Okay, I will fix it.


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

Thanks for pointing out these changes. I agree these are more clear
and will fix them all.

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

Yes, they are also incompatible. I thought to highlight -m because `--fixup`
allows the `-m` but I agree to reword it in above way with slight change :
s/e.g. `-m`/i.e it is incompatible with `-m`/`-F`/`-c`/`-C` options.

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

[Charvi Mendiratta]

On Fri, 26 Feb 2021 at 02:30, Junio C Hamano <gitster@pobox.com> wrote:
>
> Charvi Mendiratta <charvi077@gmail.com> writes:

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

Okay, I will fix the above changes.

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

This is applied for more than one "amend!" or in other words, "amend!"
commit chain. On the other hand we don't need to skip any subject if we
--fixup another fixup! commit because the resulting commit message is
just one liner. But yes if "fixup!" commit is combined with `--squash`
then it comments the complete "fixup!" commit line by finding its length
and increasing pointer.

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

I am not sure about this, because I think in the way you have suggested, we need
to strip off the complete subject line instead of prefix. I am saying
this because the
commit message body of "amend!" commit contains the complete commit message
of the commit we are fixing up.
for example :
$ git commit --fixup=amend:HEAD will create commit with log message :
amend! subject of head

subject of head
body of head

and again if we `--fixup:amend` the HEAD commit then :
$ git commit --fixup=amend:HEAD (by default) will create commit with
log message:
amend! amend! subject of head

amend! subject of head /* we need to comment this complete line */

subject of head
body of head

So, I am not sure about the other option to implement it ?

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

Apologies for this. I will take care of it.

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

yes, those options are caught earlier and give the error as below:
"Only one of -c/-C/-F/--fixup can be used."
and only `-m` is checked over here.

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

I agree and will update it .

Thanks a lot for the reviews. I will do the fixes and update in the
next revision.

Thanks and Regards,
Charvi

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

[Junio C Hamano]

> subject of head
> body of head
>
> So, I am not sure about the other option to implement it ?

Thaks, and OK.

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

And the reason why -m cannot be checked early is because we do not
recognize which kind of "fixup" we are doing when "only one of
-c/-C/-F/--fixup" check is made before this function is called?

OK.  I wonder if we can tell which kind of fixup we are doing much
earlier, though.  Then we could extend it to say "Only one of
-c/-C/-F/-m/--fixup=amend:<commit> can be used", etc., and we do not
have to have this "only -m is checked here, everything else is
checked earlier" curiosity.  But I do not know if such a change is
necessarily an improvement.  I guess a better "fix" would probably
be to add a comment to this function where it only checks for "-m"
and tell readers why -c/-C/-F do not have to be checked here.

Thanks.

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

[Charvi Mendiratta]

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

I agree, earlier I even asked myself why only `-m`, but this was the
only option that was allowing to write/append commit message in `
--fixup`. And I agree to add a comment, to make it more clear.

Thanks and Regards,
Charvi