[PATCH 0/7][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase

[PATCH 0/7][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase

[Charvi Mendiratta]

This patch series is build on the top of "cm/rebase-i" in the 'next' branch and
improves it. It fixup the source code of 'fixup [-C | -c]' command in the
sequencer, do some fixes in rebase -i, improves the 'fixup_-C' like commands
in lib-rebase.sh, update the test-script 't3437' and fixes a typo in the
documentation.

(Thanks to Junio C Hamano, Eric Sunshine, Christian Couder and Phillip Wood
for the suggestions and guidance for this patch series)

Charvi Mendiratta (7):
  sequencer: fixup the datatype of the 'flag' argument
  sequencer: rename a few functions
  rebase -i: clarify and fix 'fixup -c' rebase-todo help
  t/lib-rebase: change the implementation of commands with options
  t3437: fix indendation of the here-doc
  t/t3437: update the tests
  doc/rebase -i: fix typo in the documentation of 'fixup' command

 Documentation/git-rebase.txt    |   2 +-
 rebase-interactive.c            |   6 +-
 sequencer.c                     |  23 +++---
 t/lib-rebase.sh                 |   8 +-
 t/t3437-rebase-fixup-options.sh | 140 +++++++++++++++++---------------
 5 files changed, 93 insertions(+), 86 deletions(-)

--
2.29.0.rc1

[PATCH 1/7] sequencer: fixup the datatype of the 'flag' argument

[Charvi Mendiratta]

As 'flag' is a combination of bits, so change its datatype from
'enum todo_item_flags' to 'unsigned'.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index d09ce446b6..f3928cf45c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1744,8 +1744,7 @@ static const char skip_first_commit_msg_str[] = N_("The 1st commit message will
 static const char skip_nth_commit_msg_fmt[] = N_("The commit message #%d will be skipped:");
 static const char combined_commit_msg_fmt[] = N_("This is a combination of %d commits.");
 
-static int check_fixup_flag(enum todo_command command,
-			    enum todo_item_flags flag)
+static int check_fixup_flag(enum todo_command command, unsigned flag)
 {
 	return command == TODO_FIXUP && ((flag & TODO_REPLACE_FIXUP_MSG) ||
 					 (flag & TODO_EDIT_FIXUP_MSG));
@@ -1850,7 +1849,7 @@ static void update_squash_message_for_fixup(struct strbuf *msg)
 
 static int append_squash_message(struct strbuf *buf, const char *body,
 			 enum todo_command command, struct replay_opts *opts,
-			 enum todo_item_flags flag)
+			 unsigned flag)
 {
 	const char *fixup_msg;
 	size_t commented_len = 0, fixup_off;
@@ -1906,7 +1905,7 @@ static int update_squash_messages(struct repository *r,
 				  enum todo_command command,
 				  struct commit *commit,
 				  struct replay_opts *opts,
-				  enum todo_item_flags flag)
+				  unsigned flag)
 {
 	struct strbuf buf = STRBUF_INIT;
 	int res = 0;
-- 
2.29.0.rc1

[PATCH 2/7] sequencer: rename a few functions

[Charvi Mendiratta]

Rename functions to make them more descriptive and while at it, remove
unnecessary 'inline' of the skip_fixupish() function.

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

diff --git a/sequencer.c b/sequencer.c
index f3928cf45c..abc6d5cdfd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1744,7 +1744,7 @@ static const char skip_first_commit_msg_str[] = N_("The 1st commit message will
 static const char skip_nth_commit_msg_fmt[] = N_("The commit message #%d will be skipped:");
 static const char combined_commit_msg_fmt[] = N_("This is a combination of %d commits.");
 
-static int check_fixup_flag(enum todo_command command, unsigned flag)
+static int is_fixup_flag(enum todo_command command, unsigned flag)
 {
 	return command == TODO_FIXUP && ((flag & TODO_REPLACE_FIXUP_MSG) ||
 					 (flag & TODO_EDIT_FIXUP_MSG));
@@ -1873,7 +1873,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,
 	strbuf_addstr(buf, body + commented_len);
 
 	/* fixup -C after squash behaves like squash */
-	if (check_fixup_flag(command, flag) && !seen_squash(opts)) {
+	if (is_fixup_flag(command, flag) && !seen_squash(opts)) {
 		/*
 		 * We're replacing the commit message so we need to
 		 * append the Signed-off-by: trailer if the user
@@ -1928,7 +1928,7 @@ static int update_squash_messages(struct repository *r,
 			    opts->current_fixup_count + 2);
 		strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
 		strbuf_release(&header);
-		if (check_fixup_flag(command, flag) && !seen_squash(opts))
+		if (is_fixup_flag(command, flag) && !seen_squash(opts))
 			update_squash_message_for_fixup(&buf);
 	} else {
 		struct object_id head;
@@ -1951,11 +1951,11 @@ static int update_squash_messages(struct repository *r,
 		strbuf_addf(&buf, "%c ", comment_line_char);
 		strbuf_addf(&buf, _(combined_commit_msg_fmt), 2);
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
-		strbuf_addstr(&buf, check_fixup_flag(command, flag) ?
+		strbuf_addstr(&buf, is_fixup_flag(command, flag) ?
 			      _(skip_first_commit_msg_str) :
 			      _(first_commit_msg_str));
 		strbuf_addstr(&buf, "\n\n");
-		if (check_fixup_flag(command, flag))
+		if (is_fixup_flag(command, flag))
 			strbuf_add_commented_lines(&buf, body, strlen(body));
 		else
 			strbuf_addstr(&buf, body);
@@ -1968,7 +1968,7 @@ static int update_squash_messages(struct repository *r,
 			     oid_to_hex(&commit->object.oid));
 	find_commit_subject(message, &body);
 
-	if (command == TODO_SQUASH || check_fixup_flag(command, flag)) {
+	if (command == TODO_SQUASH || is_fixup_flag(command, flag)) {
 		res = append_squash_message(&buf, body, command, opts, flag);
 	} else if (command == TODO_FIXUP) {
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
@@ -5661,7 +5661,7 @@ static int subject2item_cmp(const void *fndata,
 
 define_commit_slab(commit_todo_item, struct todo_item *);
 
-static inline int skip_fixup_amend_squash(const char *subject, const char **p) {
+static int skip_fixupish(const char *subject, const char **p) {
 	return skip_prefix(subject, "fixup! ", p) ||
 	       skip_prefix(subject, "amend! ", p) ||
 	       skip_prefix(subject, "squash! ", p);
@@ -5725,13 +5725,13 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 		format_subject(&buf, subject, " ");
 		subject = subjects[i] = strbuf_detach(&buf, &subject_len);
 		unuse_commit_buffer(item->commit, commit_buffer);
-		if (skip_fixup_amend_squash(subject, &p)) {
+		if (skip_fixupish(subject, &p)) {
 			struct commit *commit2;
 
 			for (;;) {
 				while (isspace(*p))
 					p++;
-				if (!skip_fixup_amend_squash(p, &p))
+				if (!skip_fixupish(p, &p))
 					break;
 			}
 
-- 
2.29.0.rc1

[PATCH 3/7] rebase -i: clarify and fix 'fixup -c' rebase-todo help

[Charvi Mendiratta]

When `-c` says "edit the commit message" it's not clear what will be
edited. The original's commit message or the replacement's message or a
combination of the two. Word it such that it states more precisely what
exactly will be edited and also remove the use of a period and
capitalized word in the to-do help text.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 rebase-interactive.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index c3bd02adee..e85994beb6 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -45,8 +45,8 @@ void append_todo_help(int command_count,
 "e, edit <commit> = use commit, but stop for amending\n"
 "s, squash <commit> = use commit, but meld into previous commit\n"
 "f, fixup [-C | -c] <commit> = like \"squash\", but discard this\n"
-"                   commit's log message. Use -C to replace with this\n"
-"                   commit message or -c to edit the commit message\n"
+"                   commit's log message; use -C to replace with this\n"
+"                   commit message or -c to edit this commit message\n"
 "x, exec <command> = run command (the rest of the line) using shell\n"
 "b, break = stop here (continue rebase later with 'git rebase --continue')\n"
 "d, drop <commit> = remove commit\n"
@@ -55,7 +55,7 @@ void append_todo_help(int command_count,
 "m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]\n"
 ".       create a merge commit using the original merge commit's\n"
 ".       message (or the oneline, if no original merge commit was\n"
-".       specified). Use -c <commit> to reword the commit message.\n"
+".       specified); use -c <commit> to reword the commit message\n"
 "\n"
 "These lines can be re-ordered; they are executed from top to bottom.\n");
 	unsigned edit_todo = !(shortrevisions && shortonto);
-- 
2.29.0.rc1

Re: [PATCH 3/7] rebase -i: clarify and fix 'fixup -c' rebase-todo help

[Eric Sunshine]

On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> When `-c` says "edit the commit message" it's not clear what will be
> edited. The original's commit message or the replacement's message or a
> combination of the two. Word it such that it states more precisely what
> exactly will be edited and also remove the use of a period and
> capitalized word in the to-do help text.

If you happen to re-roll for some reason, it might be a good idea to
explain why you are removing the period and capitalization since the
reason is not otherwise clear to the casual reader. So, perhaps:

    ... exactly will be edited. While at it, also drop the jarring
    period and capitalization, neither of which is otherwise present
    in the message.

or something like that.

> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>

Re: [PATCH 3/7] rebase -i: clarify and fix 'fixup -c' rebase-todo help

[Charvi Mendiratta]

On Mon, 8 Feb 2021 at 00:19, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> > When `-c` says "edit the commit message" it's not clear what will be
> > edited. The original's commit message or the replacement's message or a
> > combination of the two. Word it such that it states more precisely what
> > exactly will be edited and also remove the use of a period and
> > capitalized word in the to-do help text.
>
> If you happen to re-roll for some reason, it might be a good idea to
> explain why you are removing the period and capitalization since the
> reason is not otherwise clear to the casual reader. So, perhaps:
>
>     ... exactly will be edited. While at it, also drop the jarring
>     period and capitalization, neither of which is otherwise present
>     in the message.
>
> or something like that.

okay, I will change it.

[PATCH 4/7] t/lib-rebase: change the implementation of commands with options

[Charvi Mendiratta]

"fixup" and "merge" mirrors the implementation of FAKE_LINES handling of
"exec", but the cases are quite different. The argument to "exec" is
arbitrary and can have any number of spaces embedded in it, which
conflicts with the meaning of spaces in FAKE_LINES, which separate the
individual commands in FAKE_LINES. Consequently, "_" was chosen as a
placeholder in "exec" to mean "space".

However, "fixup" is very different from "exec". Its arguments are not
arbitrary at all, so there isn't a good reason to mirror the choice of
"_" to represent a space, which leads to rather unsightly tokens such
as "fixup_-C". Let's replace it with simpler tokens such as "fixup-C"
and "fixup-c".

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/lib-rebase.sh                 |  8 ++++----
 t/t3437-rebase-fixup-options.sh | 18 +++++++++---------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index e10e38060b..e6bd295c05 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -15,8 +15,8 @@
 #       specified line.
 #
 #   "<cmd> <lineno>" -- add a line with the specified command
-#       ("pick", "squash", "fixup", "edit", "reword" or "drop") and the
-#       SHA1 taken from the specified line.
+#      ("pick", "squash", "fixup"|"fixup-C"|"fixup-c", "edit", "reword" or "drop")
+#      and the SHA1 taken from the specified line.
 #
 #   "exec_cmd_with_args" -- add an "exec cmd with args" line.
 #
@@ -53,8 +53,8 @@ set_fake_editor () {
 			action="$line";;
 		exec_*|x_*|break|b)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
-		merge_*|fixup_*)
-			action=$(echo "$line" | sed 's/_/ /g');;
+		merge-*|fixup-*)
+			action=$(echo "$line" | sed 's/-/ -/');;
 		"#")
 			echo '# comment' >> "$1";;
 		">")
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 945df2555b..36dee15c4b 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -112,7 +112,7 @@ test_expect_success 'setup' '
 test_expect_success 'simple fixup -C works' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A2 &&
-	FAKE_LINES="1 fixup_-C 2" git rebase -i B &&
+	FAKE_LINES="1 fixup-C 2" git rebase -i B &&
 	test_cmp_rev HEAD^ B &&
 	test_cmp_rev HEAD^{tree} A2^{tree} &&
 	test_commit_message HEAD -m "A2"
@@ -123,7 +123,7 @@ test_expect_success 'simple fixup -c works' '
 	git checkout --detach A2 &&
 	git log -1 --pretty=format:%B >expected-fixup-message &&
 	test_write_lines "" "Modified A2" >>expected-fixup-message &&
-	FAKE_LINES="1 fixup_-c 2" \
+	FAKE_LINES="1 fixup-c 2" \
 		FAKE_COMMIT_AMEND="Modified A2" \
 		git rebase -i B &&
 	test_cmp_rev HEAD^ B &&
@@ -134,7 +134,7 @@ test_expect_success 'simple fixup -c works' '
 test_expect_success 'fixup -C removes amend! from message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A1 &&
-	FAKE_LINES="1 fixup_-C 2" git rebase -i A &&
+	FAKE_LINES="1 fixup-C 2" git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
 	test_cmp_rev HEAD^{tree} A1^{tree} &&
 	test_commit_message HEAD expected-message &&
@@ -145,7 +145,7 @@ test_expect_success 'fixup -C removes amend! from message' '
 test_expect_success 'fixup -C with conflicts gives correct message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A1 &&
-	test_must_fail env FAKE_LINES="1 fixup_-C 2" git rebase -i conflicts &&
+	test_must_fail env FAKE_LINES="1 fixup-C 2" git rebase -i conflicts &&
 	git checkout --theirs -- A &&
 	git add A &&
 	FAKE_COMMIT_AMEND=edited git rebase --continue &&
@@ -160,7 +160,7 @@ test_expect_success 'fixup -C with conflicts gives correct message' '
 test_expect_success 'skipping fixup -C after fixup gives correct message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A3 &&
-	test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A &&
+	test_must_fail env FAKE_LINES="1 fixup 2 fixup-C 4" git rebase -i A &&
 	git reset --hard &&
 	FAKE_COMMIT_AMEND=edited git rebase --continue &&
 	test_commit_message HEAD -m "B"
@@ -168,7 +168,7 @@ test_expect_success 'skipping fixup -C after fixup gives correct message' '
 
 test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' '
 	git checkout --detach branch &&
-	FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4 squash 5 fixup_-C 6" \
+	FAKE_LINES="1 fixup 2 fixup-C 3 fixup-C 4 squash 5 fixup-C 6" \
 		FAKE_COMMIT_AMEND=squashed \
 		FAKE_MESSAGE_COPY=actual-squash-message \
 		git -c commit.status=false rebase -ik --signoff A &&
@@ -182,7 +182,7 @@ test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fix
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout branch && git checkout --detach branch~2 &&
 	git log -1 --pretty=format:%b >expected-message &&
-	FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4" git rebase -i A &&
+	FAKE_LINES="1 fixup 2 fixup-C 3 fixup-C 4" git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
 	test_commit_message HEAD expected-message
 '
@@ -192,7 +192,7 @@ test_expect_success 'multiple fixup -c opens editor once' '
 	git checkout --detach A3 &&
 	base=$(git rev-parse HEAD~4) &&
 	FAKE_COMMIT_MESSAGE="Modified-A3" \
-		FAKE_LINES="1 fixup_-C 2 fixup_-c 3 fixup_-c 4" \
+		FAKE_LINES="1 fixup-C 2 fixup-c 3 fixup-c 4" \
 		EXPECT_HEADER_COUNT=4 \
 		git rebase -i $base &&
 	test_cmp_rev $base HEAD^ &&
@@ -202,7 +202,7 @@ test_expect_success 'multiple fixup -c opens editor once' '
 test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A3 &&
-	FAKE_LINES="1 squash 2 fixup 3 fixup_-c 4" \
+	FAKE_LINES="1 squash 2 fixup 3 fixup-c 4" \
 		FAKE_MESSAGE_COPY=actual-combined-message \
 		git -c commit.status=false rebase -i A &&
 	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-combined-message" \
-- 
2.29.0.rc1

[PATCH 5/7] t3437: fix indendation of the here-doc

[Charvi Mendiratta]

In the test scripts, the here-doc body and EOF are indented the same
amount as the command which opened the here-doc. Let's remove
one level of indendation.

Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t3437-rebase-fixup-options.sh | 62 ++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 36dee15c4b..3de899f68a 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -38,13 +38,13 @@ get_author () {
 
 test_expect_success 'setup' '
 	cat >message <<-EOF &&
-		amend! B
-		${EMPTY}
-		new subject
-		${EMPTY}
-		new
-		body
-		EOF
+	amend! B
+	${EMPTY}
+	new subject
+	${EMPTY}
+	new
+	body
+	EOF
 
 	sed "1,2d" message >expected-message &&
 
@@ -70,38 +70,38 @@ test_expect_success 'setup' '
 	git commit --fixup=HEAD -a &&
 	test_tick &&
 	git commit --allow-empty -F - <<-EOF &&
-		amend! B
-		${EMPTY}
-		B
-		${EMPTY}
-		edited 1
-		EOF
+	amend! B
+	${EMPTY}
+	B
+	${EMPTY}
+	edited 1
+	EOF
 	test_tick &&
 	git commit --allow-empty -F - <<-EOF &&
-		amend! amend! B
-		${EMPTY}
-		B
-		${EMPTY}
-		edited 1
-		${EMPTY}
-		edited 2
-		EOF
+	amend! amend! B
+	${EMPTY}
+	B
+	${EMPTY}
+	edited 1
+	${EMPTY}
+	edited 2
+	EOF
 	echo B2 >B &&
 	test_tick &&
 	FAKE_COMMIT_AMEND="edited squash" git commit --squash=HEAD -a &&
 	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
+	amend! amend! amend! B
+	${EMPTY}
+	B
+	${EMPTY}
+	edited 1
+	${EMPTY}
+	edited 2
+	${EMPTY}
+	edited 3
+	EOF
 
 	GIT_AUTHOR_NAME="Rebase Author" &&
 	GIT_AUTHOR_EMAIL="rebase.author@example.com" &&
-- 
2.29.0.rc1

Re: [PATCH 5/7] t3437: fix indendation of the here-doc

[Eric Sunshine]

On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> In the test scripts, the here-doc body and EOF are indented the same
> amount as the command which opened the here-doc. Let's remove
> one level of indendation.

s/indendation/indentation/

I found "In the test scripts" ambiguous. It isn't clear if you are
talking about all test scripts or the script(s) this patch is fixing
up. Sp, if you happen to re-roll for some reason, perhaps clarify by
saying something like:

    The most common way to format here-docs in Git test scripts is for
    the body and EOF to be indented the same amount as the command
    which opened the here-doc. Fix a few here-docs in this script to
    conform to that standard.

> Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>

I don't think this new patch is based upon Phillip's, so you can
probably drop this attribution.

> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>

Re: [PATCH 5/7] t3437: fix indendation of the here-doc

[Charvi Mendiratta]

On Mon, 8 Feb 2021 at 00:24, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> > In the test scripts, the here-doc body and EOF are indented the same
> > amount as the command which opened the here-doc. Let's remove
> > one level of indendation.
>
> s/indendation/indentation/
>

My spelling mistake, I will fix it.

> I found "In the test scripts" ambiguous. It isn't clear if you are
> talking about all test scripts or the script(s) this patch is fixing
> up. Sp, if you happen to re-roll for some reason, perhaps clarify by
> saying something like:
>
>     The most common way to format here-docs in Git test scripts is for
>     the body and EOF to be indented the same amount as the command
>     which opened the here-doc. Fix a few here-docs in this script to
>     conform to that standard.
>

Okay, will change in the above way.

> > Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> I don't think this new patch is based upon Phillip's, so you can
> probably drop this attribution.
>

Okay, thanks.

Re: [PATCH 5/7] t3437: fix indendation of the here-doc

[Phillip Wood]

Hi Chariv and Eric

On 07/02/2021 18:54, Eric Sunshine wrote:
> On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
>> In the test scripts, the here-doc body and EOF are indented the same
>> amount as the command which opened the here-doc. Let's remove
>> one level of indendation.
> 
> s/indendation/indentation/
> 
> I found "In the test scripts" ambiguous. It isn't clear if you are
> talking about all test scripts or the script(s) this patch is fixing
> up. Sp, if you happen to re-roll for some reason, perhaps clarify by
> saying something like:
> 
>      The most common way to format here-docs in Git test scripts is for
>      the body and EOF to be indented the same amount as the command
>      which opened the here-doc. Fix a few here-docs in this script to
>      conform to that standard.
> 
>> Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> I don't think this new patch is based upon Phillip's, so you can
> probably drop this attribution.

Good point - well spotted as ever Eric

Thanks

Phillip
>> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>

[PATCH 6/7] t/t3437: update the tests

[Charvi Mendiratta]

Let's do the changes listed below to make tests more easier to follow :

-Remove the dependency of 'expected-message' file from earlier tests to
make it easier to run tests selectively with '--run' or 'GIT_SKIP_TESTS'.

-Add author timestamp to check that the author date of fixed up commit
is unchanged.

-Simplify the test_commit_message() and add comments before the
function.

-Clarify the working of 'fixup -c' with "amend!" in the test-description.

-Remove unnecessary curly braces and use the named commits in the
tests so that they will still refer to the same commit if the setup
gets changed in the future whereas 'branch~2' will change which commit
it points to.

Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t3437-rebase-fixup-options.sh | 82 ++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 37 deletions(-)

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 3de899f68a..96f3a94831 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -8,8 +8,10 @@ test_description='git rebase interactive fixup options
 This test checks the "fixup [-C|-c]" command of rebase interactive.
 In addition to amending the contents of the commit, "fixup -C"
 replaces the original commit message with the message of the fixup
-commit. "fixup -c" also replaces the original message, but opens the
-editor to allow the user to edit the message before committing.
+commit and similar to "fixup" command that works with "fixup!", "fixup -C"
+works with "amend!" upon --autosquash. "fixup -c" also replaces the original
+message, but opens the editor to allow the user to edit the message before
+committing.
 '

 . ./test-lib.sh
@@ -18,36 +20,34 @@ editor to allow the user to edit the message before committing.

 EMPTY=""

+# test_commit_message <rev> -m <msg>
+# test_commit_message <rev> <path>
+# Verify that the commit message of <rev> matches
+# <msg> or the content of <path>.
 test_commit_message () {
-	rev="$1" && # commit or tag we want to test
-	file="$2" && # test against the content of a file
-	git show --no-patch --pretty=format:%B "$rev" >actual-message &&
-	if test "$2" = -m
-	then
-		str="$3" && # test against a string
-		printf "%s\n" "$str" >tmp-expected-message &&
-		file="tmp-expected-message"
-	fi
-	test_cmp "$file" actual-message
+	git show --no-patch --pretty=format:%B "$1" >actual &&
+    case "$2" in
+    -m) echo "$3" >expect &&
+	    test_cmp expect actual ;;
+    *) test_cmp "$2" actual ;;
+    esac
 }

 get_author () {
 	rev="$1" &&
-	git log -1 --pretty=format:"%an %ae" "$rev"
+	git log -1 --pretty=format:"%an %ae %at" "$rev"
 }

 test_expect_success 'setup' '
 	cat >message <<-EOF &&
 	amend! B
-	${EMPTY}
+	$EMPTY
 	new subject
-	${EMPTY}
+	$EMPTY
 	new
 	body
 	EOF

-	sed "1,2d" message >expected-message &&
-
 	test_commit A A &&
 	test_commit B B &&
 	get_author HEAD >expected-author &&
@@ -68,40 +68,43 @@ test_expect_success 'setup' '
 	echo B1 >B &&
 	test_tick &&
 	git commit --fixup=HEAD -a &&
+	git tag B1 &&
 	test_tick &&
 	git commit --allow-empty -F - <<-EOF &&
 	amend! B
-	${EMPTY}
+	$EMPTY
 	B
-	${EMPTY}
+	$EMPTY
 	edited 1
 	EOF
 	test_tick &&
 	git commit --allow-empty -F - <<-EOF &&
 	amend! amend! B
-	${EMPTY}
+	$EMPTY
 	B
-	${EMPTY}
+	$EMPTY
 	edited 1
-	${EMPTY}
+	$EMPTY
 	edited 2
 	EOF
 	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}
+	$EMPTY
 	B
-	${EMPTY}
+	$EMPTY
 	edited 1
-	${EMPTY}
+	$EMPTY
 	edited 2
-	${EMPTY}
+	$EMPTY
 	edited 3
 	EOF
+	git tag B3 &&

 	GIT_AUTHOR_NAME="Rebase Author" &&
 	GIT_AUTHOR_EMAIL="rebase.author@example.com" &&
@@ -134,6 +137,7 @@ test_expect_success 'simple fixup -c works' '
 test_expect_success 'fixup -C removes amend! from message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A1 &&
+	git log -1 --pretty=format:%b >expected-message &&
 	FAKE_LINES="1 fixup-C 2" git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
 	test_cmp_rev HEAD^{tree} A1^{tree} &&
@@ -145,13 +149,14 @@ test_expect_success 'fixup -C removes amend! from message' '
 test_expect_success 'fixup -C with conflicts gives correct message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A1 &&
+	git log -1 --pretty=format:%b >expected-message &&
+	test_write_lines "" "edited" >>expected-message &&
 	test_must_fail env FAKE_LINES="1 fixup-C 2" git rebase -i conflicts &&
 	git checkout --theirs -- A &&
 	git add A &&
 	FAKE_COMMIT_AMEND=edited git rebase --continue &&
 	test_cmp_rev HEAD^ conflicts &&
 	test_cmp_rev HEAD^{tree} A1^{tree} &&
-	test_write_lines "" edited >>expected-message &&
 	test_commit_message HEAD expected-message &&
 	get_author HEAD >actual-author &&
 	test_cmp expected-author actual-author
@@ -167,12 +172,12 @@ test_expect_success 'skipping fixup -C after fixup gives correct message' '
 '

 test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' '
-	git checkout --detach branch &&
+	git checkout --detach B3 &&
 	FAKE_LINES="1 fixup 2 fixup-C 3 fixup-C 4 squash 5 fixup-C 6" \
 		FAKE_COMMIT_AMEND=squashed \
 		FAKE_MESSAGE_COPY=actual-squash-message \
 		git -c commit.status=false rebase -ik --signoff A &&
-	git diff-tree --exit-code --patch HEAD branch -- &&
+	git diff-tree --exit-code --patch HEAD B3 -- &&
 	test_cmp_rev HEAD^ A &&
 	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
 		actual-squash-message
@@ -180,7 +185,7 @@ test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' '

 test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fixup -C' '
 	test_when_finished "test_might_fail git rebase --abort" &&
-	git checkout branch && git checkout --detach branch~2 &&
+	git checkout --detach B2~ &&
 	git log -1 --pretty=format:%b >expected-message &&
 	FAKE_LINES="1 fixup 2 fixup-C 3 fixup-C 4" git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
@@ -190,13 +195,16 @@ test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fix
 test_expect_success 'multiple fixup -c opens editor once' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A3 &&
-	base=$(git rev-parse HEAD~4) &&
-	FAKE_COMMIT_MESSAGE="Modified-A3" \
+	git log -1 --pretty=format:%B >expected-message &&
+	test_write_lines "" "Modified-A3" >>expected-message &&
+	FAKE_COMMIT_AMEND="Modified-A3" \
 		FAKE_LINES="1 fixup-C 2 fixup-c 3 fixup-c 4" \
 		EXPECT_HEADER_COUNT=4 \
-		git rebase -i $base &&
-	test_cmp_rev $base HEAD^ &&
-	test 1 = $(git show | grep Modified-A3 | wc -l)
+		git rebase -i A &&
+	test_cmp_rev HEAD^ A &&
+	get_author HEAD >actual-author &&
+	test_cmp expected-author actual-author &&
+	test_commit_message HEAD expected-message
 '

 test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '
@@ -211,12 +219,12 @@ test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '
 '

 test_expect_success 'fixup -C works upon --autosquash with amend!' '
-	git checkout --detach branch &&
+	git checkout --detach B3 &&
 	FAKE_COMMIT_AMEND=squashed \
 		FAKE_MESSAGE_COPY=actual-squash-message \
 		git -c commit.status=false rebase -ik --autosquash \
 						--signoff A &&
-	git diff-tree --exit-code --patch HEAD branch -- &&
+	git diff-tree --exit-code --patch HEAD B3 -- &&
 	test_cmp_rev HEAD^ A &&
 	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
 		actual-squash-message
--
2.29.0.rc1

Re: [PATCH 6/7] t/t3437: update the tests

[Eric Sunshine]

On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> Let's do the changes listed below to make tests more easier to follow :
>
> -Remove the dependency of 'expected-message' file from earlier tests to
> make it easier to run tests selectively with '--run' or 'GIT_SKIP_TESTS'.
>
> -Add author timestamp to check that the author date of fixed up commit
> is unchanged.
>
> -Simplify the test_commit_message() and add comments before the
> function.
>
> -Clarify the working of 'fixup -c' with "amend!" in the test-description.
>
> -Remove unnecessary curly braces and use the named commits in the
> tests so that they will still refer to the same commit if the setup
> gets changed in the future whereas 'branch~2' will change which commit
> it points to.

Typically, if you find yourself enumerating a list of distinct changes
like this in a commit message, it's a good indication that it should
be split into multiple patches, each taking care of one item from the
list. A good reason for splitting it up like this is that it's
difficult for reviewers to keep the entire list in mind while
reviewing the patch, however, it's easy to keep in mind a single
stated goal while reading the changes.

Having said that, I'm not sure it's worth a re-roll or the extra work
of actually splitting it up since you've already been dragged deeper
into this than planned, and these are relatively minor issues.

(Returning to this after reading the remainder of the patch, I did
find it reasonably confusing trying to figure out which changes
related to each other and to items from the list above. It would have
been easier to reason about the changes had they been done in separate
patches. Still, though, I'm not sure it's worth the time and effort to
split them up -- but I wouldn't complain if you did.)

More below...

> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
> diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
> @@ -8,8 +8,10 @@ test_description='git rebase interactive fixup options
>  This test checks the "fixup [-C|-c]" command of rebase interactive.
>  In addition to amending the contents of the commit, "fixup -C"
>  replaces the original commit message with the message of the fixup
> -commit. "fixup -c" also replaces the original message, but opens the
> -editor to allow the user to edit the message before committing.
> +commit and similar to "fixup" command that works with "fixup!", "fixup -C"
> +works with "amend!" upon --autosquash. "fixup -c" also replaces the original
> +message, but opens the editor to allow the user to edit the message before
> +committing.
>  '

I had trouble digesting this run-on sentence due, I think, to the
mixing of thoughts. It might be easier to understand if you first talk
only about the options to `fixup` (-c/-C), and then, as a separate
sentence, talk about how `amend!` is transformed into `fixup -C` (like
`fixup!` is transformed into `fixup`). However, as this is just minor
descriptive text in a test file, not user-facing documentation, I'm
not sure it matters enough to warrant a re-roll.

> @@ -18,36 +20,34 @@ editor to allow the user to edit the message before committing.
> +# test_commit_message <rev> -m <msg>
> +# test_commit_message <rev> <path>
> +# Verify that the commit message of <rev> matches
> +# <msg> or the content of <path>.

Good.

>  test_commit_message () {
> +       git show --no-patch --pretty=format:%B "$1" >actual &&
> +    case "$2" in
> +    -m) echo "$3" >expect &&
> +           test_cmp expect actual ;;
> +    *) test_cmp "$2" actual ;;
> +    esac
>  }

The funky indentation here is due to a mix of tabs and spaces. It
should use tabs exclusively.

Re: [PATCH 6/7] t/t3437: update the tests

[Charvi Mendiratta]

Hi Eric,

On Mon, 8 Feb 2021 at 00:13, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
[...]
> Typically, if you find yourself enumerating a list of distinct changes
> like this in a commit message, it's a good indication that it should
> be split into multiple patches, each taking care of one item from the
> list. A good reason for splitting it up like this is that it's
> difficult for reviewers to keep the entire list in mind while
> reviewing the patch, however, it's easy to keep in mind a single
> stated goal while reading the changes.
>
> Having said that, I'm not sure it's worth a re-roll or the extra work
> of actually splitting it up since you've already been dragged deeper
> into this than planned, and these are relatively minor issues.

> (Returning to this after reading the remainder of the patch, I did
> find it reasonably confusing trying to figure out which changes
> related to each other and to items from the list above. It would have
> been easier to reason about the changes had they been done in separate
> patches. Still, though, I'm not sure it's worth the time and effort to
> split them up -- but I wouldn't complain if you did.)
>

Agree, I will split this patch.

> More below...
>
> > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> > ---
> > diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
> > @@ -8,8 +8,10 @@ test_description='git rebase interactive fixup options
> >  This test checks the "fixup [-C|-c]" command of rebase interactive.
> >  In addition to amending the contents of the commit, "fixup -C"
> >  replaces the original commit message with the message of the fixup
> > -commit. "fixup -c" also replaces the original message, but opens the
> > -editor to allow the user to edit the message before committing.
> > +commit and similar to "fixup" command that works with "fixup!", "fixup -C"
> > +works with "amend!" upon --autosquash. "fixup -c" also replaces the original
> > +message, but opens the editor to allow the user to edit the message before
> > +committing.
> >  '
>
> I had trouble digesting this run-on sentence due, I think, to the
> mixing of thoughts. It might be easier to understand if you first talk
> only about the options to `fixup` (-c/-C), and then, as a separate
> sentence, talk about how `amend!` is transformed into `fixup -C` (like
> `fixup!` is transformed into `fixup`). However, as this is just minor
> descriptive text in a test file, not user-facing documentation, I'm
> not sure it matters enough to warrant a re-roll.
>

Okay, will change it.

> >  test_commit_message () {
> > +       git show --no-patch --pretty=format:%B "$1" >actual &&
> > +    case "$2" in
> > +    -m) echo "$3" >expect &&
> > +           test_cmp expect actual ;;
> > +    *) test_cmp "$2" actual ;;
> > +    esac
> >  }
>
> The funky indentation here is due to a mix of tabs and spaces. It
> should use tabs exclusively.

Oh, thanks I will correct it.

[PATCH 7/7] doc/rebase -i: fix typo in the documentation of 'fixup' command

[Charvi Mendiratta]

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 Documentation/git-rebase.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index a6903419c4..8bfa5a9272 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -894,7 +894,7 @@ is used.  In that case the suggested commit message is only the message
 of the "fixup -c" commit, and an editor is opened allowing you to edit
 the message.  The contents (patch) of the "fixup -c" commit are still
 incorporated into the folded commit. If there is more than one "fixup -c"
-commit, the message from the last last one is used.  You can also use
+commit, the message from the final one is used.  You can also use
 "fixup -C" to get the same behavior as "fixup -c" except without opening
 an editor.
 
-- 
2.29.0.rc1

Re: [PATCH 0/7][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase

[Eric Sunshine]

On Sun, Feb 7, 2021 at 1:18 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> This patch series is build on the top of "cm/rebase-i" in the 'next' branch and
> improves it. It fixup the source code of 'fixup [-C | -c]' command in the
> sequencer, do some fixes in rebase -i, improves the 'fixup_-C' like commands
> in lib-rebase.sh, update the test-script 't3437' and fixes a typo in the
> documentation.

Thanks for working on this. I looked over the entire series and left a
few minor comments. As mentioned in my [6/7] review, you might also
want to consider splitting that patch into several patches (though
it's not clear if the extra work of doing so is warranted). Anyhow,
aside from some botched indentation in [6/7], it all looked clean.

Re: [PATCH 0/7][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase

[Charvi Mendiratta]

On Mon, 8 Feb 2021 at 00:28, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Feb 7, 2021 at 1:18 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> > This patch series is build on the top of "cm/rebase-i" in the 'next' branch and
> > improves it. It fixup the source code of 'fixup [-C | -c]' command in the
> > sequencer, do some fixes in rebase -i, improves the 'fixup_-C' like commands
> > in lib-rebase.sh, update the test-script 't3437' and fixes a typo in the
> > documentation.
>
> Thanks for working on this. I looked over the entire series and left a
> few minor comments. As mentioned in my [6/7] review, you might also
> want to consider splitting that patch into several patches (though
> it's not clear if the extra work of doing so is warranted). Anyhow,
> aside from some botched indentation in [6/7], it all looked clean.

Thanks for the corrections. I admit there are few silly mistakes, will
fixup all and
also split [6/7] in the next version.

Thanks and Regards,
Charvi

[PATCH v2 00/11][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase

[Charvi Mendiratta]

This patch series is build on the top of "cm/rebase-i" in the 'next' branch and
improves it. It fixup the source code of 'fixup [-C | -c]' command in the
sequencer, do some fixes in rebase -i, improves the 'fixup_-C' like commands
in lib-rebase.sh, update the test-script 't3437' and fixes a typo in the
documentation.

Changes from v1 :
* Splits the patch 'update test-script', to make it more easy to follow.
* Modification in few commit messages as suggested by Eric.


Charvi Mendiratta (11):
  sequencer: fixup the datatype of the 'flag' argument
  sequencer: rename a few functions
  rebase -i: clarify and fix 'fixup -c' rebase-todo help
  t/lib-rebase: change the implementation of commands with options
  t/t3437: fix indentation of the here-doc
  t/t3437: remove the dependency of 'expected-message' file from tests
  t/t3437: check author date of the fixed up commit
  t/t3437: simplify and document the test helpers
  t/t3437: cleanup the 'setup' test and use named commits in the tests
  t/t3437: fixup the test 'multiple fixup -c opens editor once'
  doc/rebase -i: fix typo in the documentation of 'fixup' command

 Documentation/git-rebase.txt    |   2 +-
 rebase-interactive.c            |   6 +-
 sequencer.c                     |  23 +++---
 t/lib-rebase.sh                 |   8 +-
 t/t3437-rebase-fixup-options.sh | 140 +++++++++++++++++---------------
 5 files changed, 94 insertions(+), 85 deletions(-)

--
2.29.0.rc1

Re: [PATCH v2 00/11][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

> This patch series is build on the top of "cm/rebase-i" in the 'next' branch and
> improves it. It fixup the source code of 'fixup [-C | -c]' command in the
> sequencer, do some fixes in rebase -i, improves the 'fixup_-C' like commands
> in lib-rebase.sh, update the test-script 't3437' and fixes a typo in the
> documentation.

Thanks.  I saw a couple of minor nits, but overall it was a pleasant
read.

Re: [PATCH v2 00/11][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase

[Charvi Mendiratta]

On Tue, 9 Feb 2021 at 03:27, Junio C Hamano <gitster@pobox.com> wrote:
>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> > This patch series is build on the top of "cm/rebase-i" in the 'next' branch and
> > improves it. It fixup the source code of 'fixup [-C | -c]' command in the
> > sequencer, do some fixes in rebase -i, improves the 'fixup_-C' like commands
> > in lib-rebase.sh, update the test-script 't3437' and fixes a typo in the
> > documentation.
>
> Thanks.  I saw a couple of minor nits, but overall it was a pleasant
> read.
>

Thanks for feedback! I will fix those nits in the next version.

Thanks and Regards,
Charvi

[PATCH v2 01/11] sequencer: fixup the datatype of the 'flag' argument

[Charvi Mendiratta]

As 'flag' is a combination of bits, so change its datatype from
'enum todo_item_flags' to 'unsigned'.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index d09ce446b6..f3928cf45c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1744,8 +1744,7 @@ static const char skip_first_commit_msg_str[] = N_("The 1st commit message will
 static const char skip_nth_commit_msg_fmt[] = N_("The commit message #%d will be skipped:");
 static const char combined_commit_msg_fmt[] = N_("This is a combination of %d commits.");
 
-static int check_fixup_flag(enum todo_command command,
-			    enum todo_item_flags flag)
+static int check_fixup_flag(enum todo_command command, unsigned flag)
 {
 	return command == TODO_FIXUP && ((flag & TODO_REPLACE_FIXUP_MSG) ||
 					 (flag & TODO_EDIT_FIXUP_MSG));
@@ -1850,7 +1849,7 @@ static void update_squash_message_for_fixup(struct strbuf *msg)
 
 static int append_squash_message(struct strbuf *buf, const char *body,
 			 enum todo_command command, struct replay_opts *opts,
-			 enum todo_item_flags flag)
+			 unsigned flag)
 {
 	const char *fixup_msg;
 	size_t commented_len = 0, fixup_off;
@@ -1906,7 +1905,7 @@ static int update_squash_messages(struct repository *r,
 				  enum todo_command command,
 				  struct commit *commit,
 				  struct replay_opts *opts,
-				  enum todo_item_flags flag)
+				  unsigned flag)
 {
 	struct strbuf buf = STRBUF_INIT;
 	int res = 0;
-- 
2.29.0.rc1

[PATCH v2 02/11] sequencer: rename a few functions

[Charvi Mendiratta]

Rename functions to make them more descriptive and while at it, remove
unnecessary 'inline' of the skip_fixupish() function.

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

diff --git a/sequencer.c b/sequencer.c
index f3928cf45c..abc6d5cdfd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1744,7 +1744,7 @@ static const char skip_first_commit_msg_str[] = N_("The 1st commit message will
 static const char skip_nth_commit_msg_fmt[] = N_("The commit message #%d will be skipped:");
 static const char combined_commit_msg_fmt[] = N_("This is a combination of %d commits.");
 
-static int check_fixup_flag(enum todo_command command, unsigned flag)
+static int is_fixup_flag(enum todo_command command, unsigned flag)
 {
 	return command == TODO_FIXUP && ((flag & TODO_REPLACE_FIXUP_MSG) ||
 					 (flag & TODO_EDIT_FIXUP_MSG));
@@ -1873,7 +1873,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,
 	strbuf_addstr(buf, body + commented_len);
 
 	/* fixup -C after squash behaves like squash */
-	if (check_fixup_flag(command, flag) && !seen_squash(opts)) {
+	if (is_fixup_flag(command, flag) && !seen_squash(opts)) {
 		/*
 		 * We're replacing the commit message so we need to
 		 * append the Signed-off-by: trailer if the user
@@ -1928,7 +1928,7 @@ static int update_squash_messages(struct repository *r,
 			    opts->current_fixup_count + 2);
 		strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
 		strbuf_release(&header);
-		if (check_fixup_flag(command, flag) && !seen_squash(opts))
+		if (is_fixup_flag(command, flag) && !seen_squash(opts))
 			update_squash_message_for_fixup(&buf);
 	} else {
 		struct object_id head;
@@ -1951,11 +1951,11 @@ static int update_squash_messages(struct repository *r,
 		strbuf_addf(&buf, "%c ", comment_line_char);
 		strbuf_addf(&buf, _(combined_commit_msg_fmt), 2);
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
-		strbuf_addstr(&buf, check_fixup_flag(command, flag) ?
+		strbuf_addstr(&buf, is_fixup_flag(command, flag) ?
 			      _(skip_first_commit_msg_str) :
 			      _(first_commit_msg_str));
 		strbuf_addstr(&buf, "\n\n");
-		if (check_fixup_flag(command, flag))
+		if (is_fixup_flag(command, flag))
 			strbuf_add_commented_lines(&buf, body, strlen(body));
 		else
 			strbuf_addstr(&buf, body);
@@ -1968,7 +1968,7 @@ static int update_squash_messages(struct repository *r,
 			     oid_to_hex(&commit->object.oid));
 	find_commit_subject(message, &body);
 
-	if (command == TODO_SQUASH || check_fixup_flag(command, flag)) {
+	if (command == TODO_SQUASH || is_fixup_flag(command, flag)) {
 		res = append_squash_message(&buf, body, command, opts, flag);
 	} else if (command == TODO_FIXUP) {
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
@@ -5661,7 +5661,7 @@ static int subject2item_cmp(const void *fndata,
 
 define_commit_slab(commit_todo_item, struct todo_item *);
 
-static inline int skip_fixup_amend_squash(const char *subject, const char **p) {
+static int skip_fixupish(const char *subject, const char **p) {
 	return skip_prefix(subject, "fixup! ", p) ||
 	       skip_prefix(subject, "amend! ", p) ||
 	       skip_prefix(subject, "squash! ", p);
@@ -5725,13 +5725,13 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 		format_subject(&buf, subject, " ");
 		subject = subjects[i] = strbuf_detach(&buf, &subject_len);
 		unuse_commit_buffer(item->commit, commit_buffer);
-		if (skip_fixup_amend_squash(subject, &p)) {
+		if (skip_fixupish(subject, &p)) {
 			struct commit *commit2;
 
 			for (;;) {
 				while (isspace(*p))
 					p++;
-				if (!skip_fixup_amend_squash(p, &p))
+				if (!skip_fixupish(p, &p))
 					break;
 			}
 
-- 
2.29.0.rc1

[PATCH v2 03/11] rebase -i: clarify and fix 'fixup -c' rebase-todo help

[Charvi Mendiratta]

When `-c` says "edit the commit message" it's not clear what will be
edited. The original's commit message or the replacement's message or a
combination of the two. Word it such that it states more precisely what
exactly will be edited. While at it, also drop the jarring period and
capitalization, neither of which is otherwise present in the message.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 rebase-interactive.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index c3bd02adee..e85994beb6 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -45,8 +45,8 @@ void append_todo_help(int command_count,
 "e, edit <commit> = use commit, but stop for amending\n"
 "s, squash <commit> = use commit, but meld into previous commit\n"
 "f, fixup [-C | -c] <commit> = like \"squash\", but discard this\n"
-"                   commit's log message. Use -C to replace with this\n"
-"                   commit message or -c to edit the commit message\n"
+"                   commit's log message; use -C to replace with this\n"
+"                   commit message or -c to edit this commit message\n"
 "x, exec <command> = run command (the rest of the line) using shell\n"
 "b, break = stop here (continue rebase later with 'git rebase --continue')\n"
 "d, drop <commit> = remove commit\n"
@@ -55,7 +55,7 @@ void append_todo_help(int command_count,
 "m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]\n"
 ".       create a merge commit using the original merge commit's\n"
 ".       message (or the oneline, if no original merge commit was\n"
-".       specified). Use -c <commit> to reword the commit message.\n"
+".       specified); use -c <commit> to reword the commit message\n"
 "\n"
 "These lines can be re-ordered; they are executed from top to bottom.\n");
 	unsigned edit_todo = !(shortrevisions && shortonto);
-- 
2.29.0.rc1

Re: [PATCH v2 03/11] rebase -i: clarify and fix 'fixup -c' rebase-todo help

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

> When `-c` says "edit the commit message" it's not clear what will be
> edited. The original's commit message or the replacement's message or a
> combination of the two. Word it such that it states more precisely what
> exactly will be edited. While at it, also drop the jarring period and
> capitalization, neither of which is otherwise present in the message.

>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
>  rebase-interactive.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index c3bd02adee..e85994beb6 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -45,8 +45,8 @@ void append_todo_help(int command_count,
>  "e, edit <commit> = use commit, but stop for amending\n"
>  "s, squash <commit> = use commit, but meld into previous commit\n"
>  "f, fixup [-C | -c] <commit> = like \"squash\", but discard this\n"
> -"                   commit's log message. Use -C to replace with this\n"
> -"                   commit message or -c to edit the commit message\n"
> +"                   commit's log message; use -C to replace with this\n"
> +"                   commit message or -c to edit this commit message\n"

The goal is good, but I am not sure if this "the commit" -> "this commit"
is an effective enough way to fix the issue.  Here is my attempt but
I do not think it is not 10x better to be worth replacing yours X-<.

    use only the log message of the "fixup" commit, discarding the
    message from the previous commit.  While "-C" uses the message
    as-is, "-c" lets the user further edit it.

>  "x, exec <command> = run command (the rest of the line) using shell\n"
>  "b, break = stop here (continue rebase later with 'git rebase --continue')\n"
>  "d, drop <commit> = remove commit\n"
> @@ -55,7 +55,7 @@ void append_todo_help(int command_count,
>  "m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]\n"
>  ".       create a merge commit using the original merge commit's\n"
>  ".       message (or the oneline, if no original merge commit was\n"
> -".       specified). Use -c <commit> to reword the commit message.\n"
> +".       specified); use -c <commit> to reword the commit message\n"

This hunk fixes the formatting by dropping the full-stop.  Unlike
the description of "fixup -C/-c", I find it very easy to understand.

Thanks.

Re: [PATCH v2 03/11] rebase -i: clarify and fix 'fixup -c' rebase-todo help

[Charvi Mendiratta]

Hi Junio,

> >  "f, fixup [-C | -c] <commit> = like \"squash\", but discard this\n"
> > -"                   commit's log message. Use -C to replace with this\n"
> > -"                   commit message or -c to edit the commit message\n"
> > +"                   commit's log message; use -C to replace with this\n"
> > +"                   commit message or -c to edit this commit message\n"
>
> The goal is good, but I am not sure if this "the commit" -> "this commit"
> is an effective enough way to fix the issue.  Here is my attempt but
> I do not think it is not 10x better to be worth replacing yours X-<.
>
>     use only the log message of the "fixup" commit, discarding the
>     message from the previous commit.  While "-C" uses the message
>     as-is, "-c" lets the user further edit it.
>

Okay, but in this patch we are also removing period and capitalization from
rebase to-do help of commands. So, maybe we can replace it like :

f, fixup [-C | -c] <commit> = like \"squash\", but discard this\n"
 "                  commit's log message; use -C to use only the\n"
 "                  log message of the "fixup" commit, discarding the\n"
 "                  message from the previous commit; while -C uses \n"
 "                  the message as-is, -c allows to further edit it\n"

If it is okay ?

Thanks and Regards,
Charvi

Re: [PATCH v2 03/11] rebase -i: clarify and fix 'fixup -c' rebase-todo help

[Eric Sunshine]

On Tue, Feb 9, 2021 at 2:13 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> > The goal is good, but I am not sure if this "the commit" -> "this commit"
> > is an effective enough way to fix the issue.  Here is my attempt but
> > I do not think it is not 10x better to be worth replacing yours X-<.
> >
> >     use only the log message of the "fixup" commit, discarding the
> >     message from the previous commit.  While "-C" uses the message
> >     as-is, "-c" lets the user further edit it.
>
> Okay, but in this patch we are also removing period and capitalization from
> rebase to-do help of commands. So, maybe we can replace it like :
>
> f, fixup [-C | -c] <commit> = like \"squash\", but discard this\n"
>  "                  commit's log message; use -C to use only the\n"
>  "                  log message of the "fixup" commit, discarding the\n"
>  "                  message from the previous commit; while -C uses \n"
>  "                  the message as-is, -c allows to further edit it\n"

Here's another more concise attempt:

    like "squash" but keep only the previous commit's log message,
    unless -C is used, in which case keep only this commit's message;
    -c is same as -C but opens editor

Re: [PATCH v2 03/11] rebase -i: clarify and fix 'fixup -c' rebase-todo help

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Here's another more concise attempt:
>
>     like "squash" but keep only the previous commit's log message,
>     unless -C is used, in which case keep only this commit's message;
>     -c is same as -C but opens editor

Nice.

Re: [PATCH v2 03/11] rebase -i: clarify and fix 'fixup -c' rebase-todo help

[Eric Sunshine]

On Tue, Feb 9, 2021 at 2:08 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > Here's another more concise attempt:
> >
> >     like "squash" but keep only the previous commit's log message,
> >     unless -C is used, in which case keep only this commit's message;
> >     -c is same as -C but opens editor
>
> Nice.

For conciseness, I intentionally omitted "the", however, upon
reflection, it probably would be a good idea to insert "the" between
"opens" and "editor".

Re: [PATCH v2 03/11] rebase -i: clarify and fix 'fixup -c' rebase-todo help

[Charvi Mendiratta]

On Wed, 10 Feb 2021 at 00:43, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Tue, Feb 9, 2021 at 2:08 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > Here's another more concise attempt:
> > >
> > >     like "squash" but keep only the previous commit's log message,
> > >     unless -C is used, in which case keep only this commit's message;
> > >     -c is same as -C but opens editor
> >
> > Nice.
>
> For conciseness, I intentionally omitted "the", however, upon
> reflection, it probably would be a good idea to insert "the" between
> "opens" and "editor".

Okay, I agree this is also very easy to understand and will update it.

Thanks !

[PATCH v2 04/11] t/lib-rebase: change the implementation of commands with options

[Charvi Mendiratta]

"fixup" and "merge" mirrors the implementation of FAKE_LINES handling of
"exec", but the cases are quite different. The argument to "exec" is
arbitrary and can have any number of spaces embedded in it, which
conflicts with the meaning of spaces in FAKE_LINES, which separate the
individual commands in FAKE_LINES. Consequently, "_" was chosen as a
placeholder in "exec" to mean "space".

However, "fixup" is very different from "exec". Its arguments are not
arbitrary at all, so there isn't a good reason to mirror the choice of
"_" to represent a space, which leads to rather unsightly tokens such
as "fixup_-C". Let's replace it with simpler tokens such as "fixup-C"
and "fixup-c".

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/lib-rebase.sh                 |  8 ++++----
 t/t3437-rebase-fixup-options.sh | 18 +++++++++---------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index e10e38060b..e6bd295c05 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -15,8 +15,8 @@
 #       specified line.
 #
 #   "<cmd> <lineno>" -- add a line with the specified command
-#       ("pick", "squash", "fixup", "edit", "reword" or "drop") and the
-#       SHA1 taken from the specified line.
+#      ("pick", "squash", "fixup"|"fixup-C"|"fixup-c", "edit", "reword" or "drop")
+#      and the SHA1 taken from the specified line.
 #
 #   "exec_cmd_with_args" -- add an "exec cmd with args" line.
 #
@@ -53,8 +53,8 @@ set_fake_editor () {
 			action="$line";;
 		exec_*|x_*|break|b)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
-		merge_*|fixup_*)
-			action=$(echo "$line" | sed 's/_/ /g');;
+		merge-*|fixup-*)
+			action=$(echo "$line" | sed 's/-/ -/');;
 		"#")
 			echo '# comment' >> "$1";;
 		">")
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 945df2555b..36dee15c4b 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -112,7 +112,7 @@ test_expect_success 'setup' '
 test_expect_success 'simple fixup -C works' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A2 &&
-	FAKE_LINES="1 fixup_-C 2" git rebase -i B &&
+	FAKE_LINES="1 fixup-C 2" git rebase -i B &&
 	test_cmp_rev HEAD^ B &&
 	test_cmp_rev HEAD^{tree} A2^{tree} &&
 	test_commit_message HEAD -m "A2"
@@ -123,7 +123,7 @@ test_expect_success 'simple fixup -c works' '
 	git checkout --detach A2 &&
 	git log -1 --pretty=format:%B >expected-fixup-message &&
 	test_write_lines "" "Modified A2" >>expected-fixup-message &&
-	FAKE_LINES="1 fixup_-c 2" \
+	FAKE_LINES="1 fixup-c 2" \
 		FAKE_COMMIT_AMEND="Modified A2" \
 		git rebase -i B &&
 	test_cmp_rev HEAD^ B &&
@@ -134,7 +134,7 @@ test_expect_success 'simple fixup -c works' '
 test_expect_success 'fixup -C removes amend! from message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A1 &&
-	FAKE_LINES="1 fixup_-C 2" git rebase -i A &&
+	FAKE_LINES="1 fixup-C 2" git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
 	test_cmp_rev HEAD^{tree} A1^{tree} &&
 	test_commit_message HEAD expected-message &&
@@ -145,7 +145,7 @@ test_expect_success 'fixup -C removes amend! from message' '
 test_expect_success 'fixup -C with conflicts gives correct message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A1 &&
-	test_must_fail env FAKE_LINES="1 fixup_-C 2" git rebase -i conflicts &&
+	test_must_fail env FAKE_LINES="1 fixup-C 2" git rebase -i conflicts &&
 	git checkout --theirs -- A &&
 	git add A &&
 	FAKE_COMMIT_AMEND=edited git rebase --continue &&
@@ -160,7 +160,7 @@ test_expect_success 'fixup -C with conflicts gives correct message' '
 test_expect_success 'skipping fixup -C after fixup gives correct message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A3 &&
-	test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A &&
+	test_must_fail env FAKE_LINES="1 fixup 2 fixup-C 4" git rebase -i A &&
 	git reset --hard &&
 	FAKE_COMMIT_AMEND=edited git rebase --continue &&
 	test_commit_message HEAD -m "B"
@@ -168,7 +168,7 @@ test_expect_success 'skipping fixup -C after fixup gives correct message' '
 
 test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' '
 	git checkout --detach branch &&
-	FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4 squash 5 fixup_-C 6" \
+	FAKE_LINES="1 fixup 2 fixup-C 3 fixup-C 4 squash 5 fixup-C 6" \
 		FAKE_COMMIT_AMEND=squashed \
 		FAKE_MESSAGE_COPY=actual-squash-message \
 		git -c commit.status=false rebase -ik --signoff A &&
@@ -182,7 +182,7 @@ test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fix
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout branch && git checkout --detach branch~2 &&
 	git log -1 --pretty=format:%b >expected-message &&
-	FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4" git rebase -i A &&
+	FAKE_LINES="1 fixup 2 fixup-C 3 fixup-C 4" git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
 	test_commit_message HEAD expected-message
 '
@@ -192,7 +192,7 @@ test_expect_success 'multiple fixup -c opens editor once' '
 	git checkout --detach A3 &&
 	base=$(git rev-parse HEAD~4) &&
 	FAKE_COMMIT_MESSAGE="Modified-A3" \
-		FAKE_LINES="1 fixup_-C 2 fixup_-c 3 fixup_-c 4" \
+		FAKE_LINES="1 fixup-C 2 fixup-c 3 fixup-c 4" \
 		EXPECT_HEADER_COUNT=4 \
 		git rebase -i $base &&
 	test_cmp_rev $base HEAD^ &&
@@ -202,7 +202,7 @@ test_expect_success 'multiple fixup -c opens editor once' '
 test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A3 &&
-	FAKE_LINES="1 squash 2 fixup 3 fixup_-c 4" \
+	FAKE_LINES="1 squash 2 fixup 3 fixup-c 4" \
 		FAKE_MESSAGE_COPY=actual-combined-message \
 		git -c commit.status=false rebase -i A &&
 	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-combined-message" \
-- 
2.29.0.rc1

Re: [PATCH v2 04/11] t/lib-rebase: change the implementation of commands with options

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

> However, "fixup" is very different from "exec". Its arguments are not
> arbitrary at all, so there isn't a good reason to mirror the choice of
> "_" to represent a space, which leads to rather unsightly tokens such
> as "fixup_-C". Let's replace it with simpler tokens such as "fixup-C"
> and "fixup-c".

Sadly, I have to say that this change may be making the developer
experience worse.

To use the original, test writers only need to remember a single
rule: "when a single command needs to embed a SP, replace it with
underscore" regardless of which insn they are listing in FAKE_LINES.

Now they need to remember that rule only applies to exec, and merge
and fixup uses a different rule, namely, a SP immediately before a
dash must be removed.

So, if I didn't know you folks have invested enough hours in this
patch, I would have said not to do this, but it is such a small
change, its effect isolated to only those who would be writing tests
for "rebase -i", it may be OK to let them endure a bit additional
burden to remember an extra rule with this patch.  I dunno.

Re: [PATCH v2 04/11] t/lib-rebase: change the implementation of commands with options

[Christian Couder]

On Mon, Feb 8, 2021 at 10:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> > However, "fixup" is very different from "exec". Its arguments are not
> > arbitrary at all, so there isn't a good reason to mirror the choice of
> > "_" to represent a space, which leads to rather unsightly tokens such
> > as "fixup_-C". Let's replace it with simpler tokens such as "fixup-C"
> > and "fixup-c".
>
> Sadly, I have to say that this change may be making the developer
> experience worse.
>
> To use the original, test writers only need to remember a single
> rule: "when a single command needs to embed a SP, replace it with
> underscore" regardless of which insn they are listing in FAKE_LINES.
>
> Now they need to remember that rule only applies to exec, and merge
> and fixup uses a different rule, namely, a SP immediately before a
> dash must be removed.

I agree with that, and discussed it with Eric. See:

https://lore.kernel.org/git/CAPig+cSBVG0AdyqXH2mZp6Ohrcb8_ec1Mm_vGbQM4zWT_7yYxQ@mail.gmail.com/

The discussion was:

-----------------------

> > > However, "fixup" is a very different beast. Its arguments are not
> > > arbitrary at all, so there isn't a good reason to mirror the choice of
> > > "_" to represent a space, which leads to rather unsightly tokens such
> > > as "fixup_-C". It would work just as well to use simpler tokens such
> > > as "fixup-C" and "fixup-c", in which case t/lib-rebase.sh might parse
> > > them like this (note that I also dropped `g` from the `sed` action):
> > >
> > >     fixup-*)
> > >         action=$(echo "$line" | sed 's/-/ -/');;
> >
> > I agree that "fixup" arguments are not arbitrary at all, but I think
> > it makes things simpler to just use one way to encode spaces instead
> > of many different ways.
>
> Is that the intention here, though? Is the idea that some day `fixup`
> will accept arbitrary arguments thus needs to encode spaces? If not,
> then mirroring the treatment given to `exec` confuses readers into
> thinking that it will/should accept arbitrary arguments. I brought
> this up in my review specifically because it was confusing to a person
> (me) new to this topic and reading the patches for the first time. The
> more specific and exact the code can be, the less likely it will
> confuse readers in the future.

-----------------------

> So, if I didn't know you folks have invested enough hours in this
> patch, I would have said not to do this, but it is such a small
> change, its effect isolated to only those who would be writing tests
> for "rebase -i", it may be OK to let them endure a bit additional
> burden to remember an extra rule with this patch.  I dunno.

I would be ok with dropping this patch. It might be a good idea to
improve the documentation before the function though.

Re: [PATCH v2 04/11] t/lib-rebase: change the implementation of commands with options

[Charvi Mendiratta]

> I agree with that, and discussed it with Eric. See:
>
> https://lore.kernel.org/git/CAPig+cSBVG0AdyqXH2mZp6Ohrcb8_ec1Mm_vGbQM4zWT_7yYxQ@mail.gmail.com/
>
> The discussion was:
>
> -----------------------
>
> > > > However, "fixup" is a very different beast. Its arguments are not
> > > > arbitrary at all, so there isn't a good reason to mirror the choice of
> > > > "_" to represent a space, which leads to rather unsightly tokens such
> > > > as "fixup_-C". It would work just as well to use simpler tokens such
> > > > as "fixup-C" and "fixup-c", in which case t/lib-rebase.sh might parse
> > > > them like this (note that I also dropped `g` from the `sed` action):
> > > >
> > > >     fixup-*)
> > > >         action=$(echo "$line" | sed 's/-/ -/');;
> > >
> > > I agree that "fixup" arguments are not arbitrary at all, but I think
> > > it makes things simpler to just use one way to encode spaces instead
> > > of many different ways.
> >
> > Is that the intention here, though? Is the idea that some day `fixup`
> > will accept arbitrary arguments thus needs to encode spaces? If not,
> > then mirroring the treatment given to `exec` confuses readers into
> > thinking that it will/should accept arbitrary arguments. I brought
> > this up in my review specifically because it was confusing to a person
> > (me) new to this topic and reading the patches for the first time. The
> > more specific and exact the code can be, the less likely it will
> > confuse readers in the future.
>
> -----------------------
>
> > So, if I didn't know you folks have invested enough hours in this
> > patch, I would have said not to do this, but it is such a small
> > change, its effect isolated to only those who would be writing tests
> > for "rebase -i", it may be OK to let them endure a bit additional
> > burden to remember an extra rule with this patch.  I dunno.
>
> I would be ok with dropping this patch.

Earlier from the discussions I thought it would be ok to make separate rules for
command taking arbitrary arguments(exec) and the command taking single
option(fixup).

But I also agree we can make the same rules and will remove it.

> It might be a good idea to
> improve the documentation before the function though.

Okay, Maybe we can improve like below:

update the current comment:
# "exec_cmd_with_args" -- add an "exec cmd with args" line.

with:
# "_" -- add a space, like "fixup_-C" implies "fixup -C" and
#        "exec_cmd_with_args" add an "exec cmd with args" line.

Thanks and Regards,
Charvi.

[PATCH v2 05/11] t/t3437: fix indentation of the here-doc

[Charvi Mendiratta]

The most common way to format here-docs in Git test scripts is for the
body and EOF to be indented the same amount as the command which opened
the here-doc. Fix a few here-docs in this script to conform to that
standard.

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

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 36dee15c4b..3de899f68a 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -38,13 +38,13 @@ get_author () {
 
 test_expect_success 'setup' '
 	cat >message <<-EOF &&
-		amend! B
-		${EMPTY}
-		new subject
-		${EMPTY}
-		new
-		body
-		EOF
+	amend! B
+	${EMPTY}
+	new subject
+	${EMPTY}
+	new
+	body
+	EOF
 
 	sed "1,2d" message >expected-message &&
 
@@ -70,38 +70,38 @@ test_expect_success 'setup' '
 	git commit --fixup=HEAD -a &&
 	test_tick &&
 	git commit --allow-empty -F - <<-EOF &&
-		amend! B
-		${EMPTY}
-		B
-		${EMPTY}
-		edited 1
-		EOF
+	amend! B
+	${EMPTY}
+	B
+	${EMPTY}
+	edited 1
+	EOF
 	test_tick &&
 	git commit --allow-empty -F - <<-EOF &&
-		amend! amend! B
-		${EMPTY}
-		B
-		${EMPTY}
-		edited 1
-		${EMPTY}
-		edited 2
-		EOF
+	amend! amend! B
+	${EMPTY}
+	B
+	${EMPTY}
+	edited 1
+	${EMPTY}
+	edited 2
+	EOF
 	echo B2 >B &&
 	test_tick &&
 	FAKE_COMMIT_AMEND="edited squash" git commit --squash=HEAD -a &&
 	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
+	amend! amend! amend! B
+	${EMPTY}
+	B
+	${EMPTY}
+	edited 1
+	${EMPTY}
+	edited 2
+	${EMPTY}
+	edited 3
+	EOF
 
 	GIT_AUTHOR_NAME="Rebase Author" &&
 	GIT_AUTHOR_EMAIL="rebase.author@example.com" &&
-- 
2.29.0.rc1

[PATCH v2 06/11] t/t3437: remove the dependency of 'expected-message' file from tests

[Charvi Mendiratta]

As it is currently implemented, it's too difficult to follow along and
remember the value of "expected-message" from test to test. It also
makes it difficult to extend tests or add new tests in between existing
tests without negatively impacting other tests.

Let's set up "expected-message" to the precise content needed by the
test, so that both the problems go away and also makes easier to run
tests selectively with '--run' or 'GIT_SKIP_TESTS'

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

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 3de899f68a..242770a3ec 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -46,8 +46,6 @@ test_expect_success 'setup' '
 	body
 	EOF
 
-	sed "1,2d" message >expected-message &&
-
 	test_commit A A &&
 	test_commit B B &&
 	get_author HEAD >expected-author &&
@@ -134,6 +132,7 @@ test_expect_success 'simple fixup -c works' '
 test_expect_success 'fixup -C removes amend! from message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A1 &&
+	git log -1 --pretty=format:%b >expected-message &&
 	FAKE_LINES="1 fixup-C 2" git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
 	test_cmp_rev HEAD^{tree} A1^{tree} &&
@@ -145,13 +144,14 @@ test_expect_success 'fixup -C removes amend! from message' '
 test_expect_success 'fixup -C with conflicts gives correct message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A1 &&
+	git log -1 --pretty=format:%b >expected-message &&
+	test_write_lines "" "edited" >>expected-message &&
 	test_must_fail env FAKE_LINES="1 fixup-C 2" git rebase -i conflicts &&
 	git checkout --theirs -- A &&
 	git add A &&
 	FAKE_COMMIT_AMEND=edited git rebase --continue &&
 	test_cmp_rev HEAD^ conflicts &&
 	test_cmp_rev HEAD^{tree} A1^{tree} &&
-	test_write_lines "" edited >>expected-message &&
 	test_commit_message HEAD expected-message &&
 	get_author HEAD >actual-author &&
 	test_cmp expected-author actual-author
-- 
2.29.0.rc1

[PATCH v2 07/11] t/t3437: check author date of the fixed up commit

[Charvi Mendiratta]

Add '%at' format in the get_author() function and update the test to check
that the author date of the fixed up commit is unchanged.

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

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 242770a3ec..180fc50248 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -33,7 +33,7 @@ test_commit_message () {

 get_author () {
 	rev="$1" &&
-	git log -1 --pretty=format:"%an %ae" "$rev"
+	git log -1 --pretty=format:"%an %ae %at" "$rev"
 }

 test_expect_success 'setup' '
@@ -196,6 +196,8 @@ test_expect_success 'multiple fixup -c opens editor once' '
 		EXPECT_HEADER_COUNT=4 \
 		git rebase -i $base &&
 	test_cmp_rev $base HEAD^ &&
+	get_author HEAD >actual-author &&
+	test_cmp expected-author actual-author &&
 	test 1 = $(git show | grep Modified-A3 | wc -l)
 '

--
2.29.0.rc1

[PATCH v2 08/11] t/t3437: simplify and document the test helpers

[Charvi Mendiratta]

Let's simplify the test_commit_message() helper function and add
comments to the function.

This patch also document the working of 'fixup -C' with "amend!" in the
test-description.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t3437-rebase-fixup-options.sh | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 180fc50248..cc0ae9411a 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -9,7 +9,9 @@ This test checks the "fixup [-C|-c]" command of rebase interactive.
 In addition to amending the contents of the commit, "fixup -C"
 replaces the original commit message with the message of the fixup
 commit. "fixup -c" also replaces the original message, but opens the
-editor to allow the user to edit the message before committing.
+editor to allow the user to edit the message before committing. Similar
+to the "fixup" command that works with "fixup!", "fixup -C" works with
+"amend!" upon --autosquash.
 '
 
 . ./test-lib.sh
@@ -18,17 +20,19 @@ editor to allow the user to edit the message before committing.
 
 EMPTY=""
 
+# test_commit_message <rev> -m <msg>
+# test_commit_message <rev> <path>
+# Verify that the commit message of <rev> matches
+# <msg> or the content of <path>.
 test_commit_message () {
-	rev="$1" && # commit or tag we want to test
-	file="$2" && # test against the content of a file
-	git show --no-patch --pretty=format:%B "$rev" >actual-message &&
-	if test "$2" = -m
-	then
-		str="$3" && # test against a string
-		printf "%s\n" "$str" >tmp-expected-message &&
-		file="tmp-expected-message"
-	fi
-	test_cmp "$file" actual-message
+	git show --no-patch --pretty=format:%B "$1" >actual &&
+	case "$2" in
+	-m)
+		echo "$3" >expect &&
+		test_cmp expect actual ;;
+	*)
+		test_cmp "$2" actual ;;
+	esac
 }
 
 get_author () {
-- 
2.29.0.rc1

[PATCH v2 09/11] t/t3437: cleanup the 'setup' test and use named commits in the tests

[Charvi Mendiratta]

Remove unnecessary curly braces and use the named commits in the
tests so that they will still refer to the same commit if the setup
gets changed in the future whereas 'branch~2' will change which commit
it points to.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t3437-rebase-fixup-options.sh | 40 +++++++++++++++++----------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index cc0ae9411a..d651fb8901 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -43,9 +43,9 @@ get_author () {
 test_expect_success 'setup' '
 	cat >message <<-EOF &&
 	amend! B
-	${EMPTY}
+	$EMPTY
 	new subject
-	${EMPTY}
+	$EMPTY
 	new
 	body
 	EOF
@@ -70,40 +70,43 @@ test_expect_success 'setup' '
 	echo B1 >B &&
 	test_tick &&
 	git commit --fixup=HEAD -a &&
+	git tag B1 &&
 	test_tick &&
 	git commit --allow-empty -F - <<-EOF &&
 	amend! B
-	${EMPTY}
+	$EMPTY
 	B
-	${EMPTY}
+	$EMPTY
 	edited 1
 	EOF
 	test_tick &&
 	git commit --allow-empty -F - <<-EOF &&
 	amend! amend! B
-	${EMPTY}
+	$EMPTY
 	B
-	${EMPTY}
+	$EMPTY
 	edited 1
-	${EMPTY}
+	$EMPTY
 	edited 2
 	EOF
 	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}
+	$EMPTY
 	B
-	${EMPTY}
+	$EMPTY
 	edited 1
-	${EMPTY}
+	$EMPTY
 	edited 2
-	${EMPTY}
+	$EMPTY
 	edited 3
 	EOF
+	git tag B3 &&
 
 	GIT_AUTHOR_NAME="Rebase Author" &&
 	GIT_AUTHOR_EMAIL="rebase.author@example.com" &&
@@ -171,12 +174,12 @@ test_expect_success 'skipping fixup -C after fixup gives correct message' '
 '
 
 test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' '
-	git checkout --detach branch &&
+	git checkout --detach B3 &&
 	FAKE_LINES="1 fixup 2 fixup-C 3 fixup-C 4 squash 5 fixup-C 6" \
 		FAKE_COMMIT_AMEND=squashed \
 		FAKE_MESSAGE_COPY=actual-squash-message \
 		git -c commit.status=false rebase -ik --signoff A &&
-	git diff-tree --exit-code --patch HEAD branch -- &&
+	git diff-tree --exit-code --patch HEAD B3 -- &&
 	test_cmp_rev HEAD^ A &&
 	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
 		actual-squash-message
@@ -184,7 +187,7 @@ test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' '
 
 test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fixup -C' '
 	test_when_finished "test_might_fail git rebase --abort" &&
-	git checkout branch && git checkout --detach branch~2 &&
+	git checkout --detach B2~ &&
 	git log -1 --pretty=format:%b >expected-message &&
 	FAKE_LINES="1 fixup 2 fixup-C 3 fixup-C 4" git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
@@ -194,12 +197,11 @@ test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fix
 test_expect_success 'multiple fixup -c opens editor once' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A3 &&
-	base=$(git rev-parse HEAD~4) &&
 	FAKE_COMMIT_MESSAGE="Modified-A3" \
 		FAKE_LINES="1 fixup-C 2 fixup-c 3 fixup-c 4" \
 		EXPECT_HEADER_COUNT=4 \
-		git rebase -i $base &&
-	test_cmp_rev $base HEAD^ &&
+		git rebase -i A &&
+	test_cmp_rev HEAD^ A &&
 	get_author HEAD >actual-author &&
 	test_cmp expected-author actual-author &&
 	test 1 = $(git show | grep Modified-A3 | wc -l)
@@ -217,12 +219,12 @@ test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '
 '
 
 test_expect_success 'fixup -C works upon --autosquash with amend!' '
-	git checkout --detach branch &&
+	git checkout --detach B3 &&
 	FAKE_COMMIT_AMEND=squashed \
 		FAKE_MESSAGE_COPY=actual-squash-message \
 		git -c commit.status=false rebase -ik --autosquash \
 						--signoff A &&
-	git diff-tree --exit-code --patch HEAD branch -- &&
+	git diff-tree --exit-code --patch HEAD B3 -- &&
 	test_cmp_rev HEAD^ A &&
 	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
 		actual-squash-message
-- 
2.29.0.rc1

Re: [PATCH v2 09/11] t/t3437: cleanup the 'setup' test and use named commits in the tests

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

> Remove unnecessary curly braces and use the named commits in the
> tests so that they will still refer to the same commit if the setup
> gets changed in the future whereas 'branch~2' will change which commit
> it points to.

Doing two things in the same commit?  I think ${EMPTY} thing is a
general style clean-up, while tagging is a bit more meaningful
change to make it easier to understand tests and is a change at a
more conceptual level.  The ${EMPTY} change would be better done at
the same time when the here document was cleaned up in [v2 05/11],
I would think.

Thanks.

Re: [PATCH v2 09/11] t/t3437: cleanup the 'setup' test and use named commits in the tests

[Charvi Mendiratta]

On Tue, 9 Feb 2021 at 03:11, Junio C Hamano <gitster@pobox.com> wrote:
>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> > Remove unnecessary curly braces and use the named commits in the
> > tests so that they will still refer to the same commit if the setup
> > gets changed in the future whereas 'branch~2' will change which commit
> > it points to.
>
> Doing two things in the same commit?  I think ${EMPTY} thing is a
> general style clean-up, while tagging is a bit more meaningful
> change to make it easier to understand tests and is a change at a
> more conceptual level.  The ${EMPTY} change would be better done at
> the same time when the here document was cleaned up in [v2 05/11],
> I would think.
>

Okay, will move it to the other patch.

Thanks and Regards,
Charvi

[PATCH v2 10/11] t/t3437: fixup the test 'multiple fixup -c opens editor once'

[Charvi Mendiratta]

In the test, FAKE_COMMIT_MESSAGE replaces the commit message each
time it is invoked so there will be only one instance of "Modified-A3"
no matter how many times we invoke the editor. Let's fix this and use
FAKE_COMMIT_AMEND instead so that it adds "Modified-A3" once for each
time the editor is invoked.

This patch also removes the check for counting the number of
"Modified-A3" lines and instead compares the whole message to check
that the commenting code works correctly for 'fixup -c' as well as
'fixup -C'.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t3437-rebase-fixup-options.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index d651fb8901..6899d25393 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -197,14 +197,16 @@ test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fix
 test_expect_success 'multiple fixup -c opens editor once' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A3 &&
-	FAKE_COMMIT_MESSAGE="Modified-A3" \
+	git log -1 --pretty=format:%B >expected-message &&
+	test_write_lines "" "Modified-A3" >>expected-message &&
+	FAKE_COMMIT_AMEND="Modified-A3" \
 		FAKE_LINES="1 fixup-C 2 fixup-c 3 fixup-c 4" \
 		EXPECT_HEADER_COUNT=4 \
 		git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
 	get_author HEAD >actual-author &&
 	test_cmp expected-author actual-author &&
-	test 1 = $(git show | grep Modified-A3 | wc -l)
+	test_commit_message HEAD expected-message
 '
 
 test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '
-- 
2.29.0.rc1

[PATCH v2 11/11] doc/rebase -i: fix typo in the documentation of 'fixup' command

[Charvi Mendiratta]

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 Documentation/git-rebase.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index a6903419c4..8bfa5a9272 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -894,7 +894,7 @@ is used.  In that case the suggested commit message is only the message
 of the "fixup -c" commit, and an editor is opened allowing you to edit
 the message.  The contents (patch) of the "fixup -c" commit are still
 incorporated into the folded commit. If there is more than one "fixup -c"
-commit, the message from the last last one is used.  You can also use
+commit, the message from the final one is used.  You can also use
 "fixup -C" to get the same behavior as "fixup -c" except without opening
 an editor.
 
-- 
2.29.0.rc1

[PATCH v3 00/11][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase

[Charvi Mendiratta]

This patch series is build on the top of "cm/rebase-i" in the 'next' branch and
improves it. It fixup the source code of 'fixup [-C | -c]' command in the
sequencer, do some fixes in rebase -i, improves the 'fixup_-C' like commands
in lib-rebase.sh, update the test-script 't3437' and fixes a typo in the
documentation.

Changes from v2 :
* Update the rebase-todo help
* Remove the changes and resets to fixup_-* command
* Update the documentation of FAKE_LINES
* Move the changes of "unnecessary curly braces in test" to the other patch
  (from v2-9/11 to v2-5/11)

Thanks all for the suggestions.

Charvi Mendiratta (11):
  sequencer: fixup the datatype of the 'flag' argument
  sequencer: rename a few functions
  rebase -i: clarify and fix 'fixup -c' rebase-todo help
  t/lib-rebase: update the documentation of FAKE_LINES
  t/t3437: fixup here-docs in the 'setup' test
  t/t3437: remove the dependency of 'expected-message' file from tests
  t/t3437: check the author date of fixed up commit
  t/t3437: simplify and document the test helpers
  t/t3437: use named commits in the tests
  t/t3437: fixup the test 'multiple fixup -c opens editor once'
  doc/rebase -i: fix typo in the documentation of 'fixup' command

 Documentation/git-rebase.txt    |   2 +-
 rebase-interactive.c            |   9 +--
 sequencer.c                     |  23 +++---
 t/lib-rebase.sh                 |   7 +-
 t/t3437-rebase-fixup-options.sh | 122 +++++++++++++++++---------------
 5 files changed, 87 insertions(+), 76 deletions(-)

--
2.29.0.rc1

Re: [PATCH v3 00/11][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

> This patch series is build on the top of "cm/rebase-i" in the 'next' branch and
> improves it. It fixup the source code of 'fixup [-C | -c]' command in the
> sequencer, do some fixes in rebase -i, improves the 'fixup_-C' like commands
> in lib-rebase.sh, update the test-script 't3437' and fixes a typo in the
> documentation.
>
> Changes from v2 :
> * Update the rebase-todo help
> * Remove the changes and resets to fixup_-* command
> * Update the documentation of FAKE_LINES
> * Move the changes of "unnecessary curly braces in test" to the other patch
>   (from v2-9/11 to v2-5/11)
>
> Thanks all for the suggestions.

Thanks.  Looking good.  Unless there is any other nits, let's
declare victory and merge the two topics down to 'next' and then to
'master' for the next release?

Re: [PATCH v3 00/11][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase

[Charvi Mendiratta]

Hi Junio,

On Thu, 11 Feb 2021 at 22:49, Junio C Hamano <gitster@pobox.com> wrote:
>[...]
> Thanks.  Looking good.  Unless there is any other nits, let's
> declare victory and merge the two topics down to 'next' and then to
> 'master' for the next release?

Thanks for confirming. Here in these two topics  `fixup -C` works with
"amend!" commit in interactive rebase and we are still working on some
improvements on the new patch series ( to be sent)  that implements
"amend! " commit . So I think to rebase that  work also on this topic,
to make project history clear and avoid the confusion ( it also
improves the same test script in this topic).

So maybe please wait for that, before merging to master.

Thanks and Regards,
Charvi

Re: [PATCH v3 00/11][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase

[Junio C Hamano]

Charvi Mendiratta <charvi077@gmail.com> writes:

> On Thu, 11 Feb 2021 at 22:49, Junio C Hamano <gitster@pobox.com> wrote:
>>[...]
>> Thanks.  Looking good.  Unless there is any other nits, let's
>> declare victory and merge the two topics down to 'next' and then to
>> 'master' for the next release?
>
> Thanks for confirming. Here in these two topics  `fixup -C` works with
> "amend!" commit in interactive rebase and we are still working on some
> improvements on the new patch series ( to be sent)  that implements
> "amend! " commit . So I think to rebase that  work also on this topic,
> to make project history clear and avoid the confusion ( it also
> improves the same test script in this topic).
>
> So maybe please wait for that, before merging to master.

Sorry, but I do not quite understand.

Aren't you talking about adding even more features to what is
already there in the cm/rebase-i plus cm/rebase-i-updates topics?
Or are you saying that what is in these two topics is still buggy
and we need fixes to it before we can give them to the general
public?

I had an impression that it was the former, and if that is the case,
then moving them to 'next' and then to 'master', regardless of the
follow-up changes, would be a useful thing to do.  Of course, if it
is the latter, i.e. these two topics make "git rebase -i" worse by
introducing an unfinished feature that is not yet usable and/or
buggy without further work, yes, it would be prudent to wait merging
the cm/rebase-i-updates topic to 'next' and replace it with a fixed
version.

But then you'd be stopping me from merging the "updates" one to
'next', not to 'master'.

Re: [PATCH v3 00/11][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase

[Charvi Mendiratta]

On Fri, 12 Feb 2021 at 04:14, Junio C Hamano <gitster@pobox.com> wrote:

> Sorry, but I do not quite understand.
>

I thought to rebase the "[WIP] Implementation of amend! commit"
that adds option to `git commit --fixup` , as discussed
earlier[1] onto cm/rebase-i.
Because cm/rebase-i branch includes the working of the
"amend!" commit upon --autosquash and also in sequencer with
command `fixup -C`. But..

> Aren't you talking about adding even more features to what is
> already there in the cm/rebase-i plus cm/rebase-i-updates topics?

..Yes, it's correct ...

> Or are you saying that what is in these two topics is still buggy
> and we need fixes to it before we can give them to the general
> public?
>
> I had an impression that it was the former, and if that is the case,
> then moving them to 'next' and then to 'master', regardless of the
> follow-up changes, would be a useful thing to do.

...Okay, I agree and will do it in this way.

Thanks and Regards,
Charvi

[1] https://lore.kernel.org/git/CAPSFM5f+cm87N5TO3V+rJvWyrcazybNb_Zu_bJZ+sBH4N4iyow@mail.gmail.com/

[PATCH v3 01/11] sequencer: fixup the datatype of the 'flag' argument

[Charvi Mendiratta]

As 'flag' is a combination of bits, so change its datatype from
'enum todo_item_flags' to 'unsigned'.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index d09ce446b6..f3928cf45c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1744,8 +1744,7 @@ static const char skip_first_commit_msg_str[] = N_("The 1st commit message will
 static const char skip_nth_commit_msg_fmt[] = N_("The commit message #%d will be skipped:");
 static const char combined_commit_msg_fmt[] = N_("This is a combination of %d commits.");
 
-static int check_fixup_flag(enum todo_command command,
-			    enum todo_item_flags flag)
+static int check_fixup_flag(enum todo_command command, unsigned flag)
 {
 	return command == TODO_FIXUP && ((flag & TODO_REPLACE_FIXUP_MSG) ||
 					 (flag & TODO_EDIT_FIXUP_MSG));
@@ -1850,7 +1849,7 @@ static void update_squash_message_for_fixup(struct strbuf *msg)
 
 static int append_squash_message(struct strbuf *buf, const char *body,
 			 enum todo_command command, struct replay_opts *opts,
-			 enum todo_item_flags flag)
+			 unsigned flag)
 {
 	const char *fixup_msg;
 	size_t commented_len = 0, fixup_off;
@@ -1906,7 +1905,7 @@ static int update_squash_messages(struct repository *r,
 				  enum todo_command command,
 				  struct commit *commit,
 				  struct replay_opts *opts,
-				  enum todo_item_flags flag)
+				  unsigned flag)
 {
 	struct strbuf buf = STRBUF_INIT;
 	int res = 0;
-- 
2.29.0.rc1

[PATCH v3 02/11] sequencer: rename a few functions

[Charvi Mendiratta]

Rename functions to make them more descriptive and while at it, remove
unnecessary 'inline' of the skip_fixupish() function.

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

diff --git a/sequencer.c b/sequencer.c
index f3928cf45c..abc6d5cdfd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1744,7 +1744,7 @@ static const char skip_first_commit_msg_str[] = N_("The 1st commit message will
 static const char skip_nth_commit_msg_fmt[] = N_("The commit message #%d will be skipped:");
 static const char combined_commit_msg_fmt[] = N_("This is a combination of %d commits.");
 
-static int check_fixup_flag(enum todo_command command, unsigned flag)
+static int is_fixup_flag(enum todo_command command, unsigned flag)
 {
 	return command == TODO_FIXUP && ((flag & TODO_REPLACE_FIXUP_MSG) ||
 					 (flag & TODO_EDIT_FIXUP_MSG));
@@ -1873,7 +1873,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,
 	strbuf_addstr(buf, body + commented_len);
 
 	/* fixup -C after squash behaves like squash */
-	if (check_fixup_flag(command, flag) && !seen_squash(opts)) {
+	if (is_fixup_flag(command, flag) && !seen_squash(opts)) {
 		/*
 		 * We're replacing the commit message so we need to
 		 * append the Signed-off-by: trailer if the user
@@ -1928,7 +1928,7 @@ static int update_squash_messages(struct repository *r,
 			    opts->current_fixup_count + 2);
 		strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
 		strbuf_release(&header);
-		if (check_fixup_flag(command, flag) && !seen_squash(opts))
+		if (is_fixup_flag(command, flag) && !seen_squash(opts))
 			update_squash_message_for_fixup(&buf);
 	} else {
 		struct object_id head;
@@ -1951,11 +1951,11 @@ static int update_squash_messages(struct repository *r,
 		strbuf_addf(&buf, "%c ", comment_line_char);
 		strbuf_addf(&buf, _(combined_commit_msg_fmt), 2);
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
-		strbuf_addstr(&buf, check_fixup_flag(command, flag) ?
+		strbuf_addstr(&buf, is_fixup_flag(command, flag) ?
 			      _(skip_first_commit_msg_str) :
 			      _(first_commit_msg_str));
 		strbuf_addstr(&buf, "\n\n");
-		if (check_fixup_flag(command, flag))
+		if (is_fixup_flag(command, flag))
 			strbuf_add_commented_lines(&buf, body, strlen(body));
 		else
 			strbuf_addstr(&buf, body);
@@ -1968,7 +1968,7 @@ static int update_squash_messages(struct repository *r,
 			     oid_to_hex(&commit->object.oid));
 	find_commit_subject(message, &body);
 
-	if (command == TODO_SQUASH || check_fixup_flag(command, flag)) {
+	if (command == TODO_SQUASH || is_fixup_flag(command, flag)) {
 		res = append_squash_message(&buf, body, command, opts, flag);
 	} else if (command == TODO_FIXUP) {
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
@@ -5661,7 +5661,7 @@ static int subject2item_cmp(const void *fndata,
 
 define_commit_slab(commit_todo_item, struct todo_item *);
 
-static inline int skip_fixup_amend_squash(const char *subject, const char **p) {
+static int skip_fixupish(const char *subject, const char **p) {
 	return skip_prefix(subject, "fixup! ", p) ||
 	       skip_prefix(subject, "amend! ", p) ||
 	       skip_prefix(subject, "squash! ", p);
@@ -5725,13 +5725,13 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 		format_subject(&buf, subject, " ");
 		subject = subjects[i] = strbuf_detach(&buf, &subject_len);
 		unuse_commit_buffer(item->commit, commit_buffer);
-		if (skip_fixup_amend_squash(subject, &p)) {
+		if (skip_fixupish(subject, &p)) {
 			struct commit *commit2;
 
 			for (;;) {
 				while (isspace(*p))
 					p++;
-				if (!skip_fixup_amend_squash(p, &p))
+				if (!skip_fixupish(p, &p))
 					break;
 			}
 
-- 
2.29.0.rc1

[PATCH v3 03/11] rebase -i: clarify and fix 'fixup -c' rebase-todo help

[Charvi Mendiratta]

When `-c` says "edit the commit message" it's not clear what will be
edited. The original's commit message or the replacement's message or a
combination of the two. Word it such that it states more precisely what
exactly will be edited. While at it, also drop the jarring period and
capitalization, neither of which is otherwise present in the message.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 rebase-interactive.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index c3bd02adee..b6cbd16a17 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -44,9 +44,10 @@ void append_todo_help(int command_count,
 "r, reword <commit> = use commit, but edit the commit message\n"
 "e, edit <commit> = use commit, but stop for amending\n"
 "s, squash <commit> = use commit, but meld into previous commit\n"
-"f, fixup [-C | -c] <commit> = like \"squash\", but discard this\n"
-"                   commit's log message. Use -C to replace with this\n"
-"                   commit message or -c to edit the commit message\n"
+"f, fixup [-C | -c] <commit> = like \"squash\" but keep only the previous\n"
+"                   commit's log message, unless -C is used, in which case\n"
+"                   keep only this commit's message; -c is same as -C but\n"
+"                   opens the editor\n"
 "x, exec <command> = run command (the rest of the line) using shell\n"
 "b, break = stop here (continue rebase later with 'git rebase --continue')\n"
 "d, drop <commit> = remove commit\n"
@@ -55,7 +56,7 @@ void append_todo_help(int command_count,
 "m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]\n"
 ".       create a merge commit using the original merge commit's\n"
 ".       message (or the oneline, if no original merge commit was\n"
-".       specified). Use -c <commit> to reword the commit message.\n"
+".       specified); use -c <commit> to reword the commit message\n"
 "\n"
 "These lines can be re-ordered; they are executed from top to bottom.\n");
 	unsigned edit_todo = !(shortrevisions && shortonto);
-- 
2.29.0.rc1

[PATCH v3 04/11] t/lib-rebase: update the documentation of FAKE_LINES

[Charvi Mendiratta]

FAKE_LINES helper function use underscore to embed a space in a single
command. Let's document it and also update the list of commands.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/lib-rebase.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index e10e38060b..57cee517b2 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -15,10 +15,11 @@
 #       specified line.
 #
 #   "<cmd> <lineno>" -- add a line with the specified command
-#       ("pick", "squash", "fixup", "edit", "reword" or "drop") and the
-#       SHA1 taken from the specified line.
+#       ("pick", "squash", "fixup"|"fixup_-C"|"fixup_-c", "edit", "reword" or "drop")
+#       and the SHA1 taken from the specified line.
 #
-#   "exec_cmd_with_args" -- add an "exec cmd with args" line.
+#   "_" -- add a space, like "fixup_-C" implies "fixup -C" and
+#       "exec_cmd_with_args" add an "exec cmd with args" line.
 #
 #   "#" -- Add a comment line.
 #
-- 
2.29.0.rc1

[PATCH v3 05/11] t/t3437: fixup here-docs in the 'setup' test

[Charvi Mendiratta]

The most common way to format here-docs in Git test scripts is for the
body and EOF to be indented the same amount as the command which opened
the here-doc. Fix a few here-docs in this script to conform to that
standard and also remove the unnecessary curly braces.

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

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 945df2555b..f599da3e08 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -38,13 +38,13 @@ get_author () {
 
 test_expect_success 'setup' '
 	cat >message <<-EOF &&
-		amend! B
-		${EMPTY}
-		new subject
-		${EMPTY}
-		new
-		body
-		EOF
+	amend! B
+	$EMPTY
+	new subject
+	$EMPTY
+	new
+	body
+	EOF
 
 	sed "1,2d" message >expected-message &&
 
@@ -70,38 +70,38 @@ test_expect_success 'setup' '
 	git commit --fixup=HEAD -a &&
 	test_tick &&
 	git commit --allow-empty -F - <<-EOF &&
-		amend! B
-		${EMPTY}
-		B
-		${EMPTY}
-		edited 1
-		EOF
+	amend! B
+	$EMPTY
+	B
+	$EMPTY
+	edited 1
+	EOF
 	test_tick &&
 	git commit --allow-empty -F - <<-EOF &&
-		amend! amend! B
-		${EMPTY}
-		B
-		${EMPTY}
-		edited 1
-		${EMPTY}
-		edited 2
-		EOF
+	amend! amend! B
+	$EMPTY
+	B
+	$EMPTY
+	edited 1
+	$EMPTY
+	edited 2
+	EOF
 	echo B2 >B &&
 	test_tick &&
 	FAKE_COMMIT_AMEND="edited squash" git commit --squash=HEAD -a &&
 	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
+	amend! amend! amend! B
+	$EMPTY
+	B
+	$EMPTY
+	edited 1
+	$EMPTY
+	edited 2
+	$EMPTY
+	edited 3
+	EOF
 
 	GIT_AUTHOR_NAME="Rebase Author" &&
 	GIT_AUTHOR_EMAIL="rebase.author@example.com" &&
-- 
2.29.0.rc1

[PATCH v3 06/11] t/t3437: remove the dependency of 'expected-message' file from tests

[Charvi Mendiratta]

As it is currently implemented, it's too difficult to follow along and
remember the value of "expected-message" from test to test. It also
makes it difficult to extend tests or add new tests in between existing
tests without negatively impacting other tests.

Let's set up "expected-message" to the precise content needed by the
test, so that both the problems go away and also makes easier to run
tests selectively with '--run' or 'GIT_SKIP_TESTS'

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

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index f599da3e08..d368ab4d4b 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -46,8 +46,6 @@ test_expect_success 'setup' '
 	body
 	EOF
 
-	sed "1,2d" message >expected-message &&
-
 	test_commit A A &&
 	test_commit B B &&
 	get_author HEAD >expected-author &&
@@ -134,6 +132,7 @@ test_expect_success 'simple fixup -c works' '
 test_expect_success 'fixup -C removes amend! from message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A1 &&
+	git log -1 --pretty=format:%b >expected-message &&
 	FAKE_LINES="1 fixup_-C 2" git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
 	test_cmp_rev HEAD^{tree} A1^{tree} &&
@@ -145,13 +144,14 @@ test_expect_success 'fixup -C removes amend! from message' '
 test_expect_success 'fixup -C with conflicts gives correct message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A1 &&
+	git log -1 --pretty=format:%b >expected-message &&
+	test_write_lines "" "edited" >>expected-message &&
 	test_must_fail env FAKE_LINES="1 fixup_-C 2" git rebase -i conflicts &&
 	git checkout --theirs -- A &&
 	git add A &&
 	FAKE_COMMIT_AMEND=edited git rebase --continue &&
 	test_cmp_rev HEAD^ conflicts &&
 	test_cmp_rev HEAD^{tree} A1^{tree} &&
-	test_write_lines "" edited >>expected-message &&
 	test_commit_message HEAD expected-message &&
 	get_author HEAD >actual-author &&
 	test_cmp expected-author actual-author
-- 
2.29.0.rc1

[PATCH v3 07/11] t/t3437: check the author date of fixed up commit

[Charvi Mendiratta]

Add '%at' format in the get_author() function and update the test to
check that the author date of the fixed up commit is unchanged.

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

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index d368ab4d4b..505211a589 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -33,7 +33,7 @@ test_commit_message () {
 
 get_author () {
 	rev="$1" &&
-	git log -1 --pretty=format:"%an %ae" "$rev"
+	git log -1 --pretty=format:"%an %ae %at" "$rev"
 }
 
 test_expect_success 'setup' '
@@ -196,6 +196,8 @@ test_expect_success 'multiple fixup -c opens editor once' '
 		EXPECT_HEADER_COUNT=4 \
 		git rebase -i $base &&
 	test_cmp_rev $base HEAD^ &&
+	get_author HEAD >actual-author &&
+	test_cmp expected-author actual-author &&
 	test 1 = $(git show | grep Modified-A3 | wc -l)
 '
 
-- 
2.29.0.rc1

[PATCH v3 08/11] t/t3437: simplify and document the test helpers

[Charvi Mendiratta]

Let's simplify the test_commit_message() helper function and add
comments to the function.

This patch also document the working of 'fixup -C' with "amend!" in the
test-description.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t3437-rebase-fixup-options.sh | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 505211a589..6b464989e9 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -9,7 +9,9 @@ This test checks the "fixup [-C|-c]" command of rebase interactive.
 In addition to amending the contents of the commit, "fixup -C"
 replaces the original commit message with the message of the fixup
 commit. "fixup -c" also replaces the original message, but opens the
-editor to allow the user to edit the message before committing.
+editor to allow the user to edit the message before committing. Similar
+to the "fixup" command that works with "fixup!", "fixup -C" works with
+"amend!" upon --autosquash.
 '
 
 . ./test-lib.sh
@@ -18,17 +20,19 @@ editor to allow the user to edit the message before committing.
 
 EMPTY=""
 
+# test_commit_message <rev> -m <msg>
+# test_commit_message <rev> <path>
+# Verify that the commit message of <rev> matches
+# <msg> or the content of <path>.
 test_commit_message () {
-	rev="$1" && # commit or tag we want to test
-	file="$2" && # test against the content of a file
-	git show --no-patch --pretty=format:%B "$rev" >actual-message &&
-	if test "$2" = -m
-	then
-		str="$3" && # test against a string
-		printf "%s\n" "$str" >tmp-expected-message &&
-		file="tmp-expected-message"
-	fi
-	test_cmp "$file" actual-message
+	git show --no-patch --pretty=format:%B "$1" >actual &&
+	case "$2" in
+	-m)
+		echo "$3" >expect &&
+		test_cmp expect actual ;;
+	*)
+		test_cmp "$2" actual ;;
+	esac
 }
 
 get_author () {
-- 
2.29.0.rc1

[PATCH v3 09/11] t/t3437: use named commits in the tests

[Charvi Mendiratta]

Use the named commits in the tests so that they will still refer to the
same commit if the setup gets changed in the future whereas 'branch~2'
will change which commit it points to.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t3437-rebase-fixup-options.sh | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 6b464989e9..6e981fa487 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -70,6 +70,7 @@ test_expect_success 'setup' '
 	echo B1 >B &&
 	test_tick &&
 	git commit --fixup=HEAD -a &&
+	git tag B1 &&
 	test_tick &&
 	git commit --allow-empty -F - <<-EOF &&
 	amend! B
@@ -91,6 +92,7 @@ test_expect_success 'setup' '
 	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 &&
@@ -104,6 +106,7 @@ test_expect_success 'setup' '
 	$EMPTY
 	edited 3
 	EOF
+	git tag B3 &&
 
 	GIT_AUTHOR_NAME="Rebase Author" &&
 	GIT_AUTHOR_EMAIL="rebase.author@example.com" &&
@@ -171,12 +174,12 @@ test_expect_success 'skipping fixup -C after fixup gives correct message' '
 '
 
 test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' '
-	git checkout --detach branch &&
+	git checkout --detach B3 &&
 	FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4 squash 5 fixup_-C 6" \
 		FAKE_COMMIT_AMEND=squashed \
 		FAKE_MESSAGE_COPY=actual-squash-message \
 		git -c commit.status=false rebase -ik --signoff A &&
-	git diff-tree --exit-code --patch HEAD branch -- &&
+	git diff-tree --exit-code --patch HEAD B3 -- &&
 	test_cmp_rev HEAD^ A &&
 	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
 		actual-squash-message
@@ -184,7 +187,7 @@ test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' '
 
 test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fixup -C' '
 	test_when_finished "test_might_fail git rebase --abort" &&
-	git checkout branch && git checkout --detach branch~2 &&
+	git checkout --detach B2~ &&
 	git log -1 --pretty=format:%b >expected-message &&
 	FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4" git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
@@ -194,12 +197,11 @@ test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fix
 test_expect_success 'multiple fixup -c opens editor once' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A3 &&
-	base=$(git rev-parse HEAD~4) &&
 	FAKE_COMMIT_MESSAGE="Modified-A3" \
 		FAKE_LINES="1 fixup_-C 2 fixup_-c 3 fixup_-c 4" \
 		EXPECT_HEADER_COUNT=4 \
-		git rebase -i $base &&
-	test_cmp_rev $base HEAD^ &&
+		git rebase -i A &&
+	test_cmp_rev HEAD^ A &&
 	get_author HEAD >actual-author &&
 	test_cmp expected-author actual-author &&
 	test 1 = $(git show | grep Modified-A3 | wc -l)
@@ -217,12 +219,12 @@ test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '
 '
 
 test_expect_success 'fixup -C works upon --autosquash with amend!' '
-	git checkout --detach branch &&
+	git checkout --detach B3 &&
 	FAKE_COMMIT_AMEND=squashed \
 		FAKE_MESSAGE_COPY=actual-squash-message \
 		git -c commit.status=false rebase -ik --autosquash \
 						--signoff A &&
-	git diff-tree --exit-code --patch HEAD branch -- &&
+	git diff-tree --exit-code --patch HEAD B3 -- &&
 	test_cmp_rev HEAD^ A &&
 	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
 		actual-squash-message
-- 
2.29.0.rc1

[PATCH v3 10/11] t/t3437: fixup the test 'multiple fixup -c opens editor once'

[Charvi Mendiratta]

In the test, FAKE_COMMIT_MESSAGE replaces the commit message each
time it is invoked so there will be only one instance of "Modified-A3"
no matter how many times we invoke the editor. Let's fix this and use
FAKE_COMMIT_AMEND instead so that it adds "Modified-A3" once for each
time the editor is invoked.

This patch also removes the check for counting the number of
"Modified-A3" lines and instead compares the whole message to check
that the commenting code works correctly for 'fixup -c' as well as
'fixup -C'.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/t3437-rebase-fixup-options.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 6e981fa487..a5a20354e3 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -197,14 +197,16 @@ test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fix
 test_expect_success 'multiple fixup -c opens editor once' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A3 &&
-	FAKE_COMMIT_MESSAGE="Modified-A3" \
+	git log -1 --pretty=format:%B >expected-message &&
+	test_write_lines "" "Modified-A3" >>expected-message &&
+	FAKE_COMMIT_AMEND="Modified-A3" \
 		FAKE_LINES="1 fixup_-C 2 fixup_-c 3 fixup_-c 4" \
 		EXPECT_HEADER_COUNT=4 \
 		git rebase -i A &&
 	test_cmp_rev HEAD^ A &&
 	get_author HEAD >actual-author &&
 	test_cmp expected-author actual-author &&
-	test 1 = $(git show | grep Modified-A3 | wc -l)
+	test_commit_message HEAD expected-message
 '
 
 test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '
-- 
2.29.0.rc1

[PATCH v3 11/11] doc/rebase -i: fix typo in the documentation of 'fixup' command

[Charvi Mendiratta]

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 Documentation/git-rebase.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index a6903419c4..8bfa5a9272 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -894,7 +894,7 @@ is used.  In that case the suggested commit message is only the message
 of the "fixup -c" commit, and an editor is opened allowing you to edit
 the message.  The contents (patch) of the "fixup -c" commit are still
 incorporated into the folded commit. If there is more than one "fixup -c"
-commit, the message from the last last one is used.  You can also use
+commit, the message from the final one is used.  You can also use
 "fixup -C" to get the same behavior as "fixup -c" except without opening
 an editor.
 
-- 
2.29.0.rc1