sequencer parsing fixes
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
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
@@ -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
@@ -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
@@ -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
PATCH 2/2 rebase -i: fix parsing of "fixup -C<commit>"
In reply to Phillip Wood via GitGitGadgetFrom: 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
@@ -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
@@ -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
@@ -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
@@ -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
Re: [PATCH 1/2] rebase -i: match whole word in is_command()
In reply to Phillip Wood via GitGitGadgetsequencer.c static int is_command(enum todo_command command, const char **bol) · line 2486
Quoted 12 lines · Phillip Wood via GitGitGadget
sequencer.c +4
-5const 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@@ -60,7 +60,7 @@ set_fake_editor () { ">") echo >> "$1";; bad) - action="badcmd";; + action="pickled";;
;-)
Re: [PATCH 2/2] rebase -i: fix parsing of "fixup -C<commit>"
In reply to Phillip Wood via GitGitGadgetsequencer.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
-2if (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+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.
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.
Ciao,
Johannes
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.
Re: [PATCH 1/2] rebase -i: match whole word in is_command()
In reply to Phillip Wood via GitGitGadgetQuoted 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