Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sequencer parsing fixes #1486

Closed
wants to merge 2 commits into from
Closed

sequencer parsing fixes #1486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b050e7c
rebase -i: match whole word in is_command()
phillipwood Feb 22, 2023
d670e28
rebase -i: fix parsing of "fixup -C<commit>"
phillipwood Feb 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 8 additions & 10 deletions sequencer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2479,12 +2479,11 @@ static int is_command(enum todo_command command, const char **bol)
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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

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

;-)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jeff King wrote (reply to this):

On Thu, Feb 23, 2023 at 08:55:00PM +0000, Phillip Wood via GitGitGadget wrote:

> 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

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,
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -2542,12 +2542,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> 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.


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;
}
Expand Down
10 changes: 7 additions & 3 deletions t/lib-rebase.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand All @@ -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"
}
12 changes: 7 additions & 5 deletions t/t3404-rebase-interactive.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand All @@ -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 &&
Expand All @@ -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)
Expand Down Expand Up @@ -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 &&
Expand Down
26 changes: 26 additions & 0 deletions t/t3437-rebase-fixup-options.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down Expand Up @@ -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
8 changes: 8 additions & 0 deletions t/test-lib-functions.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down