[2/2] rebase -i: fix parsing of "fixup -C<commit>"

Commit Message

Phillip Wood Feb. 23, 2023, 8:55 p.m. UTC

From: Phillip Wood <[e-mail]>

If the user omits the space between "-C" and the commit in a fixup
command then it is parsed as an ordinary fixup and the commit message is
not updated as it should be. Fix this by making the space between "-C"
and "<commit>" optional as it is for the "merge" command.

Note that set_replace_editor() is changed to set $GIT_SEQUENCE_EDITOR
instead of $EDITOR in order to be able to replace the todo list and
reword commits with $FAKE_COMMIT_MESSAGE. This is safe as all the
existing users are using set_replace_editor() to replace the todo list.

Signed-off-by: Phillip Wood <[e-mail]>
---
 sequencer.c                     |  6 ++----
 t/lib-rebase.sh                 |  8 ++++++--
 t/t3437-rebase-fixup-options.sh | 26 ++++++++++++++++++++++++++
 t/test-lib-functions.sh         |  8 ++++++++
 4 files changed, 42 insertions(+), 6 deletions(-)

Comments

Junio C Hamano Feb. 23, 2023, 10:35 p.m. UTC | #1

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

>  	if (item->command == TODO_FIXUP) {
> -		if (skip_prefix(bol, "-C", &bol) &&
> -		   (*bol == ' ' || *bol == '\t')) {
> +		if (skip_prefix(bol, "-C", &bol)) {
>  			bol += strspn(bol, " \t");
>  			item->flags |= TODO_REPLACE_FIXUP_MSG;

OK.  An explicit check followed by strspn() is an odd way to write
this even if it meant to require at least one whitespace, but I
agree that this one probably did not even mean to require a
whitespace there, and the updated code looks much easier to read.

> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> index 1d2f0429aea..7ca5b918f04 100644
> --- a/t/lib-rebase.sh
> +++ b/t/lib-rebase.sh
> ...
> +test_expect_success 'fixup -[Cc]<commit> works' '
> +	test_when_finished "test_might_fail git rebase --abort" &&
> +	cat >todo <<-\EOF &&
> +	pick A
> +	fixup -CA1
> +	pick B
> +	fixup -cA2
> +	EOF

By the way, this is much easier to follow, than the todo file written
in the ugly FAKE_LINES language, to see what is being tested.

Will queue.  Thanks.

Patch

diff --git a/sequencer.c b/sequencer.c
index d19ee189b57..9ddae1ce944 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2542,12 +2542,10 @@  static int parse_insn_line(struct repository *r, struct todo_item *item,
 	}
 
 	if (item->command == TODO_FIXUP) {
-		if (skip_prefix(bol, "-C", &bol) &&
-		   (*bol == ' ' || *bol == '\t')) {
+		if (skip_prefix(bol, "-C", &bol)) {
 			bol += strspn(bol, " \t");
 			item->flags |= TODO_REPLACE_FIXUP_MSG;
-		} else if (skip_prefix(bol, "-c", &bol) &&
-				  (*bol == ' ' || *bol == '\t')) {
+		} else if (skip_prefix(bol, "-c", &bol)) {
 			bol += strspn(bol, " \t");
 			item->flags |= TODO_EDIT_FIXUP_MSG;
 		}
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 1d2f0429aea..7ca5b918f04 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -211,6 +211,9 @@  check_reworded_commits () {
 # usage: set_replace_editor <file>
 #
 # Replace the todo file with the exact contents of the given file.
+# N.B. sets GIT_SEQUENCE_EDITOR rather than EDITOR so it can be
+# combined with set_fake_editor to reword commits and replace the
+# todo list
 set_replace_editor () {
 	cat >script <<-\EOF &&
 	cat FILENAME >"$1"
@@ -219,6 +222,7 @@  set_replace_editor () {
 	cat "$1"
 	EOF
 
-	sed -e "s/FILENAME/$1/g" <script | write_script fake-editor.sh &&
-	test_set_editor "$(pwd)/fake-editor.sh"
+	sed -e "s/FILENAME/$1/g" script |
+		write_script fake-sequence-editor.sh &&
+	test_set_sequence_editor "$(pwd)/fake-sequence-editor.sh"
 }
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 274699dadb8..dd3b301fa7a 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -51,6 +51,7 @@  test_expect_success 'setup' '
 	body
 	EOF
 
+	test_commit initial &&
 	test_commit A A &&
 	test_commit B B &&
 	get_author HEAD >expected-author &&
@@ -209,4 +210,29 @@  test_expect_success 'fixup -C works upon --autosquash with amend!' '
 		actual-squash-message
 '
 
+test_expect_success 'fixup -[Cc]<commit> works' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	cat >todo <<-\EOF &&
+	pick A
+	fixup -CA1
+	pick B
+	fixup -cA2
+	EOF
+	(
+		set_replace_editor todo &&
+		FAKE_COMMIT_MESSAGE="edited and fixed up" \
+			git rebase -i initial initial
+	) &&
+	git log --pretty=format:%B initial.. >actual &&
+	cat >expect <<-EOF &&
+	edited and fixed up
+	$EMPTY
+	new subject
+	$EMPTY
+	new
+	body
+	EOF
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 58cfd2f1fda..999d46fafe7 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -32,6 +32,14 @@  test_set_editor () {
 	export EDITOR
 }
 
+# Like test_set_editor but sets GIT_SEQUENCE_EDITOR instead of EDITOR
+test_set_sequence_editor () {
+	FAKE_SEQUENCE_EDITOR="$1"
+	export FAKE_SEQUENCE_EDITOR
+	GIT_SEQUENCE_EDITOR='"$FAKE_SEQUENCE_EDITOR"'
+	export GIT_SEQUENCE_EDITOR
+}
+
 test_decode_color () {
 	awk '
 		function name(n) {