Skip to contentSkip to conversation

sequencer parsing fixes

PATCH In review Phillip Wood via GitGitGadget 4 participants 8 messages Feb 23, 2023 – Feb 26, 2023

lore
Share & tools
lore
#0 lore raw

cover sequencer parsing fixes

Fix a couple of small bugs in the parsing of todo lists

Phillip Wood (2):
rebase -i: match whole word in is_command()
rebase -i: fix parsing of "fixup -C<commit>"

 sequencer.c                     | 18 ++++++++----------
 t/lib-rebase.sh                 | 10 +++++++---
 t/t3404-rebase-interactive.sh   | 12 +++++++-----
 t/t3437-rebase-fixup-options.sh | 26 ++++++++++++++++++++++++++
 t/test-lib-functions.sh         |  8 ++++++++
 5 files changed, 56 insertions(+), 18 deletions(-)

base-commit: 06dd2baa8da4a73421b959ec026a43711b9d77f9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1486%2Fphillipwood%2Fsequencer-parsing-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1486/phillipwood/sequencer-parsing-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1486

Signature · 1 line
gitgitgadget
#1 lore raw

PATCH 1/2 rebase -i: match whole word in is_command()

From: Phillip Wood <[email protected]>

When matching an unabbreviated command is_command() only does a prefix match which means it parses "pickled" as TODO_PICK. parse_insn_line() does error out because is_command() only advances as far as the end of "pick" so it looks like the command name is not followed by a space but the error message is "missing arguments for pick" rather than telling the user that the "pickled" is not a valid command.

Fix this by ensuring the match is follow by whitespace or the end of the string as we already do for abbreviated commands. The (*bol = p) at the end of the condition is a bit cute for my taste but I decided to leave it be for now. Rather than add new tests the existing tests for bad commands are adapted to use a bad command name that triggers the prefix matching bug.

---
 sequencer.c                   | 12 ++++++------
 t/lib-rebase.sh               |  2 +-
 t/t3404-rebase-interactive.sh | 12 +++++++-----
 3 files changed, 14 insertions(+), 12 deletions(-)
sequencer.c +6 -6
index 65a34f9676c..d19ee189b57 100644
@@ -2479,12 +2479,11 @@ static int is_command(enum todo_command command, const char **bol)
 {
 	const char *str = todo_command_info[command].str;
 	const char nick = todo_command_info[command].c;
-	const char *p = *bol + 1;
+	const char *p = *bol;
 
-	return skip_prefix(*bol, str, bol) ||
-		((nick && **bol == nick) &&
-		 (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p) &&
-		 (*bol = p));
+	return (skip_prefix(p, str, &p) || (nick && *p++ == nick)) &&
+		(*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p) &&
+		(*bol = p);
 }
 
 static int parse_insn_line(struct repository *r, struct todo_item *item,
@@ -2513,7 +2512,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 			break;
 		}
 	if (i >= TODO_COMMENT)
-		return -1;
+		return error(_("invalid command '%.*s'"),
+			     (int)strcspn(bol, " \t\r\n"), bol);
 
 	/* Eat up extra spaces/ tabs before object name */
 	padding = strspn(bol, " \t");
t/lib-rebase.sh +1 -1
index b57541356bd..1d2f0429aea 100644
@@ -60,7 +60,7 @@ set_fake_editor () {
 		">")
 			echo >> "$1";;
 		bad)
-			action="badcmd";;
+			action="pickled";;
 		fakesha)
 			test \& != "$action" || action=pick
 			echo "$action XXXXXXX False commit" >> "$1"
t/t3404-rebase-interactive.sh +7 -5
index 462cefd25df..3862a4301a6 100755
@@ -1449,14 +1449,15 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ig
 
 test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' '
 	cat >expect <<-EOF &&
-	error: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 primary~4)
+	error: invalid command '\''pickled'\''
+	error: invalid line 1: pickled $(git rev-list --pretty=oneline --abbrev-commit -1 primary~4)
 	Warning: some commits may have been dropped accidentally.
 	Dropped commits (newer to older):
 	 - $(git rev-list --pretty=oneline --abbrev-commit -1 primary)
 	 - $(git rev-list --pretty=oneline --abbrev-commit -1 primary~4)
 	To avoid this message, use "drop" to explicitly remove a commit.
 	EOF
-	head -n4 expect >expect.2 &&
+	head -n5 expect >expect.2 &&
 	tail -n1 expect >>expect.2 &&
 	tail -n4 expect.2 >expect.3 &&
 	test_config rebase.missingCommitsCheck warn &&
@@ -1467,7 +1468,7 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = wa
 			git rebase -i --root &&
 		cp .git/rebase-merge/git-rebase-todo.backup orig &&
 		FAKE_LINES="2 3 4" git rebase --edit-todo 2>actual.2 &&
-		head -n6 actual.2 >actual &&
+		head -n7 actual.2 >actual &&
 		test_cmp expect actual &&
 		cp orig .git/rebase-merge/git-rebase-todo &&
 		FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
@@ -1483,7 +1484,8 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = wa
 
 test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = error' '
 	cat >expect <<-EOF &&
-	error: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 primary~4)
+	error: invalid command '\''pickled'\''
+	error: invalid line 1: pickled $(git rev-list --pretty=oneline --abbrev-commit -1 primary~4)
 	Warning: some commits may have been dropped accidentally.
 	Dropped commits (newer to older):
 	 - $(git rev-list --pretty=oneline --abbrev-commit -1 primary)
@@ -1583,7 +1585,7 @@ test_expect_success 'static check of bad command' '
 		set_fake_editor &&
 		test_must_fail env FAKE_LINES="1 2 3 bad 4 5" \
 		git rebase -i --root 2>actual &&
-		test_i18ngrep "badcmd $(git rev-list --oneline -1 primary~1)" \
+		test_i18ngrep "pickled $(git rev-list --oneline -1 primary~1)" \
 				actual &&
 		test_i18ngrep "You can fix this with .git rebase --edit-todo.." \
 				actual &&
Signature · 1 line
gitgitgadget
#2 lore raw

Re: [PATCH 1/2] rebase -i: match whole word in is_command()

sequencer.c static int is_command(enum todo_command command, const char **bol) · line 2486

Quoted 12 lines · Phillip Wood via GitGitGadget
sequencer.c +4 -5
 	const char *str = todo_command_info[command].str;
 	const char nick = todo_command_info[command].c;
-	const char *p = *bol + 1;
+	const char *p = *bol;
 
-	return skip_prefix(*bol, str, bol) ||
-		((nick && **bol == nick) &&
-		 (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p) &&
-		 (*bol = p));
+	return (skip_prefix(p, str, &p) || (nick && *p++ == nick)) &&
+		(*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p) &&
+		(*bol = p);

OK. So we skip the command name string in the line given by the end-user (or see if the first letter matches the single letter command) and make sure it is followed by a whitespace or EOL in either case. The old code was not doing the "end of word" check for the longhand at all, which was clearly wrong.

I too find "&& (*bol = p)" that pretends to be a Boolean condition but is there only for its side effect distasteful, but I agree with you that fixing it is outside the scope of this patch.

sequencer.c static int parse_insn_line(struct repository *r, struct todo_item *item, · line 2516

Quoted 7 lines
sequencer.c +2 -1
@@ -2513,7 +2512,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 			break;
 		}
 	if (i >= TODO_COMMENT)
-		return -1;
+		return error(_("invalid command '%.*s'"),
+			     (int)strcspn(bol, " \t\r\n"), bol);

Nice.

t/lib-rebase.sh set_fake_editor () { · line 63

Quoted 10 lines
t/lib-rebase.sh +1 -1
index b57541356bd..1d2f0429aea 100644
@@ -60,7 +60,7 @@ set_fake_editor () {
 		">")
 			echo >> "$1";;
 		bad)
-			action="badcmd";;
+			action="pickled";;

;-)

#3 lore raw

Re: [PATCH 1/2] rebase -i: match whole word in is_command()

In reply to Phillip Wood via GitGitGadget
Quoted 6 lines · Phillip Wood via GitGitGadget

Fix this by ensuring the match is follow by whitespace or the end of the string as we already do for abbreviated commands. The (*bol = p) at the end of the condition is a bit cute for my taste but I decided to leave it be for now. Rather than add new tests the existing tests for bad commands are adapted to use a bad command name that triggers the prefix matching bug.

FWIW, coverity complained about the (*bol = p) assignment in the conditional, since "p" must be non-NULL at this point.

So this is email is a combination of:

- a data point that it is not just you that finds it a bit cute (in case you do want to change it later); and

- a hearty thank you for mentioning it in the commit message, since just looking at the code left me scratching my head at whether this was a bug.

-Peff

#4 lore raw

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

In reply to Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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.

---
 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(-)
sequencer.c +2 -4
index d19ee189b57..9ddae1ce944 100644
@@ -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;
 		}
t/lib-rebase.sh +6 -2
index 1d2f0429aea..7ca5b918f04 100644
@@ -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"
 }
t/t3437-rebase-fixup-options.sh +26 -0
index 274699dadb8..dd3b301fa7a 100755
@@ -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
t/test-lib-functions.sh +8 -0
index 58cfd2f1fda..999d46fafe7 100644
@@ -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) {
Signature · 1 line
gitgitgadget
#5 lore raw

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

sequencer.c static int parse_insn_line(struct repository *r, struct todo_item *item, · line 2547

Quoted 6 lines · Phillip Wood via GitGitGadget
sequencer.c +1 -2
 	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.

t/lib-rebase.sh

Quoted 13 lines
t/lib-rebase.sh +8 -0
index 1d2f0429aea..7ca5b918f04 100644
...
+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.

#7 lore raw
Quoted 7 lines · Johannes Schindelin

Hi Phillip,

Quoted 1 line · Phillip Wood via GitGitGadget

Fix a couple of small bugs in the parsing of todo lists

Thank you, those fixes look quite good to me.

Thanks, both. Let's merge them down quickly.