sequencer parsing fixes #1486
Closed
sequencer parsing fixes #1486
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
phillipwood
force-pushed
the
sequencer-parsing-fixes
branch
from
5dcfeb8
to
d670e28
Compare
|
/submit |
|
Submitted as pull.1486.git.1677185701.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -2479,12 +2479,11 @@ static int is_command(enum todo_command command, const char **bol) | |||
| { | |||
There was a problem hiding this comment.
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";;
;-)
| @@ -2542,12 +2542,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, | |||
| } | |||
There was a problem hiding this comment.
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.
|
This branch is now known as |
|
This patch series was integrated into seen via git@1b4aefc. |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Phillip,
On Thu, 23 Feb 2023, Phillip Wood via GitGitGadget wrote:
> Fix a couple of small bugs in the parsing of todo lists
Thank you, those fixes look quite good to me.
Ciao,
Johannes |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Phillip,
>
> On Thu, 23 Feb 2023, Phillip Wood via GitGitGadget wrote:
>
>> 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. |
|
This patch series was integrated into seen via git@f5900b0. |
|
This patch series was integrated into seen via git@ffd9be0. |
|
There was a status update in the "New Topics" section about the branch source: <pull.1486.git.1677185701.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@e6d112f. |
|
This patch series was integrated into seen via git@a9e0338. |
|
This patch series was integrated into next via git@5a2a256. |
| @@ -2479,12 +2479,11 @@ static int is_command(enum todo_command command, const char **bol) | |||
| { | |||
There was a problem hiding this comment.
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
|
User |
|
This patch series was integrated into seen via git@9405f58. |
|
This patch series was integrated into seen via git@2369584. |
|
This patch series was integrated into seen via git@61c2316. |
|
This patch series was integrated into seen via git@a2d2b52. |
|
This patch series was integrated into master via git@a2d2b52. |
|
This patch series was integrated into next via git@a2d2b52. |
|
Closed via a2d2b52. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix a couple of small bugs in the parsing of todo lists
Cc: Johannes Schindelin johannes.schindelin@gmx.de
cc: Jeff King peff@peff.net