{"thread":{"id":"59300","subject":"[PATCH 0/2] sequencer parsing fixes","startedAt":"2023-02-23T20:55:08Z","lastAt":"2023-02-26T10:29:36Z","messageCount":8,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","Johannes Schindelin","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"472574","messageId":"pull.1486.git.1677185701.gitgitgadget@gmail.com","threadId":"59300","inReplyTo":null,"subject":"[PATCH 0/2] sequencer parsing fixes","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-23T20:54:59Z","receivedAt":"2023-02-23T20:55:08Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Fix a couple of small bugs in the parsing of todo lists\n\nPhillip Wood (2):\n  rebase -i: match whole word in is_command()\n  rebase -i: fix parsing of \"fixup -C<commit>\"\n\n sequencer.c                     | 18 ++++++++----------\n t/lib-rebase.sh                 | 10 +++++++---\n t/t3404-rebase-interactive.sh   | 12 +++++++-----\n t/t3437-rebase-fixup-options.sh | 26 ++++++++++++++++++++++++++\n t/test-lib-functions.sh         |  8 ++++++++\n 5 files changed, 56 insertions(+), 18 deletions(-)\n\n\nbase-commit: 06dd2baa8da4a73421b959ec026a43711b9d77f9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1486%2Fphillipwood%2Fsequencer-parsing-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1486/phillipwood/sequencer-parsing-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1486\n-- \ngitgitgadget\n"},{"id":"472575","messageId":"b050e7c1a4daf6c65aa9b2b6f44248d7b403918e.1677185702.git.gitgitgadget@gmail.com","threadId":"59300","inReplyTo":"pull.1486.git.1677185701.gitgitgadget@gmail.com","subject":"[PATCH 1/2] rebase -i: match whole word in is_command()","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-23T20:55:00Z","receivedAt":"2023-02-23T20:55:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen matching an unabbreviated command is_command() only does a prefix\nmatch which means it parses \"pickled\" as TODO_PICK. parse_insn_line()\ndoes error out because is_command() only advances as far as the end of\n\"pick\" so it looks like the command name is not followed by a space but\nthe error message is \"missing arguments for pick\" rather than telling\nthe user that the \"pickled\" is not a valid command.\n\nFix this by ensuring the match is follow by whitespace or the end of the\nstring as we already do for abbreviated commands. The (*bol = p) at the\nend of the condition is a bit cute for my taste but I decided to leave\nit be for now. Rather than add new tests the existing tests for bad\ncommands are adapted to use a bad command name that triggers the prefix\nmatching bug.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c                   | 12 ++++++------\n t/lib-rebase.sh               |  2 +-\n t/t3404-rebase-interactive.sh | 12 +++++++-----\n 3 files changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 65a34f9676c..d19ee189b57 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2479,12 +2479,11 @@ static int is_command(enum todo_command command, const char **bol)\n {\n \tconst char *str = todo_command_info[command].str;\n \tconst char nick = todo_command_info[command].c;\n-\tconst char *p = *bol + 1;\n+\tconst char *p = *bol;\n \n-\treturn skip_prefix(*bol, str, bol) ||\n-\t\t((nick && **bol == nick) &&\n-\t\t (*p == ' ' || *p == '\\t' || *p == '\\n' || *p == '\\r' || !*p) &&\n-\t\t (*bol = p));\n+\treturn (skip_prefix(p, str, &p) || (nick && *p++ == nick)) &&\n+\t\t(*p == ' ' || *p == '\\t' || *p == '\\n' || *p == '\\r' || !*p) &&\n+\t\t(*bol = p);\n }\n \n static int parse_insn_line(struct repository *r, struct todo_item *item,\n@@ -2513,7 +2512,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \t\t\tbreak;\n \t\t}\n \tif (i >= TODO_COMMENT)\n-\t\treturn -1;\n+\t\treturn error(_(\"invalid command '%.*s'\"),\n+\t\t\t     (int)strcspn(bol, \" \\t\\r\\n\"), bol);\n \n \t/* Eat up extra spaces/ tabs before object name */\n \tpadding = strspn(bol, \" \\t\");\ndiff --git a/t/lib-rebase.sh b/t/lib-rebase.sh\nindex b57541356bd..1d2f0429aea 100644\n--- a/t/lib-rebase.sh\n+++ b/t/lib-rebase.sh\n@@ -60,7 +60,7 @@ set_fake_editor () {\n \t\t\">\")\n \t\t\techo >> \"$1\";;\n \t\tbad)\n-\t\t\taction=\"badcmd\";;\n+\t\t\taction=\"pickled\";;\n \t\tfakesha)\n \t\t\ttest \\& != \"$action\" || action=pick\n \t\t\techo \"$action XXXXXXX False commit\" >> \"$1\"\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 462cefd25df..3862a4301a6 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1449,14 +1449,15 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ig\n \n test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' '\n \tcat >expect <<-EOF &&\n-\terror: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 primary~4)\n+\terror: invalid command '\\''pickled'\\''\n+\terror: invalid line 1: pickled $(git rev-list --pretty=oneline --abbrev-commit -1 primary~4)\n \tWarning: some commits may have been dropped accidentally.\n \tDropped commits (newer to older):\n \t - $(git rev-list --pretty=oneline --abbrev-commit -1 primary)\n \t - $(git rev-list --pretty=oneline --abbrev-commit -1 primary~4)\n \tTo avoid this message, use \"drop\" to explicitly remove a commit.\n \tEOF\n-\thead -n4 expect >expect.2 &&\n+\thead -n5 expect >expect.2 &&\n \ttail -n1 expect >>expect.2 &&\n \ttail -n4 expect.2 >expect.3 &&\n \ttest_config rebase.missingCommitsCheck warn &&\n@@ -1467,7 +1468,7 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = wa\n \t\t\tgit rebase -i --root &&\n \t\tcp .git/rebase-merge/git-rebase-todo.backup orig &&\n \t\tFAKE_LINES=\"2 3 4\" git rebase --edit-todo 2>actual.2 &&\n-\t\thead -n6 actual.2 >actual &&\n+\t\thead -n7 actual.2 >actual &&\n \t\ttest_cmp expect actual &&\n \t\tcp orig .git/rebase-merge/git-rebase-todo &&\n \t\tFAKE_LINES=\"1 2 3 4\" git rebase --edit-todo 2>actual.2 &&\n@@ -1483,7 +1484,8 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = wa\n \n test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = error' '\n \tcat >expect <<-EOF &&\n-\terror: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 primary~4)\n+\terror: invalid command '\\''pickled'\\''\n+\terror: invalid line 1: pickled $(git rev-list --pretty=oneline --abbrev-commit -1 primary~4)\n \tWarning: some commits may have been dropped accidentally.\n \tDropped commits (newer to older):\n \t - $(git rev-list --pretty=oneline --abbrev-commit -1 primary)\n@@ -1583,7 +1585,7 @@ test_expect_success 'static check of bad command' '\n \t\tset_fake_editor &&\n \t\ttest_must_fail env FAKE_LINES=\"1 2 3 bad 4 5\" \\\n \t\tgit rebase -i --root 2>actual &&\n-\t\ttest_i18ngrep \"badcmd $(git rev-list --oneline -1 primary~1)\" \\\n+\t\ttest_i18ngrep \"pickled $(git rev-list --oneline -1 primary~1)\" \\\n \t\t\t\tactual &&\n \t\ttest_i18ngrep \"You can fix this with .git rebase --edit-todo..\" \\\n \t\t\t\tactual &&\n-- \ngitgitgadget\n\n"},{"id":"472576","messageId":"d670e284c175db67337ca7ce774ecd9afb089736.1677185702.git.gitgitgadget@gmail.com","threadId":"59300","inReplyTo":"pull.1486.git.1677185701.gitgitgadget@gmail.com","subject":"[PATCH 2/2] rebase -i: fix parsing of \"fixup -C<commit>\"","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-23T20:55:01Z","receivedAt":"2023-02-23T20:55:12Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf the user omits the space between \"-C\" and the commit in a fixup\ncommand then it is parsed as an ordinary fixup and the commit message is\nnot updated as it should be. Fix this by making the space between \"-C\"\nand \"<commit>\" optional as it is for the \"merge\" command.\n\nNote that set_replace_editor() is changed to set $GIT_SEQUENCE_EDITOR\ninstead of $EDITOR in order to be able to replace the todo list and\nreword commits with $FAKE_COMMIT_MESSAGE. This is safe as all the\nexisting users are using set_replace_editor() to replace the todo list.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c                     |  6 ++----\n t/lib-rebase.sh                 |  8 ++++++--\n t/t3437-rebase-fixup-options.sh | 26 ++++++++++++++++++++++++++\n t/test-lib-functions.sh         |  8 ++++++++\n 4 files changed, 42 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d19ee189b57..9ddae1ce944 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2542,12 +2542,10 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \t}\n \n \tif (item->command == TODO_FIXUP) {\n-\t\tif (skip_prefix(bol, \"-C\", &bol) &&\n-\t\t   (*bol == ' ' || *bol == '\\t')) {\n+\t\tif (skip_prefix(bol, \"-C\", &bol)) {\n \t\t\tbol += strspn(bol, \" \\t\");\n \t\t\titem->flags |= TODO_REPLACE_FIXUP_MSG;\n-\t\t} else if (skip_prefix(bol, \"-c\", &bol) &&\n-\t\t\t\t  (*bol == ' ' || *bol == '\\t')) {\n+\t\t} else if (skip_prefix(bol, \"-c\", &bol)) {\n \t\t\tbol += strspn(bol, \" \\t\");\n \t\t\titem->flags |= TODO_EDIT_FIXUP_MSG;\n \t\t}\ndiff --git a/t/lib-rebase.sh b/t/lib-rebase.sh\nindex 1d2f0429aea..7ca5b918f04 100644\n--- a/t/lib-rebase.sh\n+++ b/t/lib-rebase.sh\n@@ -211,6 +211,9 @@ check_reworded_commits () {\n # usage: set_replace_editor <file>\n #\n # Replace the todo file with the exact contents of the given file.\n+# N.B. sets GIT_SEQUENCE_EDITOR rather than EDITOR so it can be\n+# combined with set_fake_editor to reword commits and replace the\n+# todo list\n set_replace_editor () {\n \tcat >script <<-\\EOF &&\n \tcat FILENAME >\"$1\"\n@@ -219,6 +222,7 @@ set_replace_editor () {\n \tcat \"$1\"\n \tEOF\n \n-\tsed -e \"s/FILENAME/$1/g\" <script | write_script fake-editor.sh &&\n-\ttest_set_editor \"$(pwd)/fake-editor.sh\"\n+\tsed -e \"s/FILENAME/$1/g\" script |\n+\t\twrite_script fake-sequence-editor.sh &&\n+\ttest_set_sequence_editor \"$(pwd)/fake-sequence-editor.sh\"\n }\ndiff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh\nindex 274699dadb8..dd3b301fa7a 100755\n--- a/t/t3437-rebase-fixup-options.sh\n+++ b/t/t3437-rebase-fixup-options.sh\n@@ -51,6 +51,7 @@ test_expect_success 'setup' '\n \tbody\n \tEOF\n \n+\ttest_commit initial &&\n \ttest_commit A A &&\n \ttest_commit B B &&\n \tget_author HEAD >expected-author &&\n@@ -209,4 +210,29 @@ test_expect_success 'fixup -C works upon --autosquash with amend!' '\n \t\tactual-squash-message\n '\n \n+test_expect_success 'fixup -[Cc]<commit> works' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tcat >todo <<-\\EOF &&\n+\tpick A\n+\tfixup -CA1\n+\tpick B\n+\tfixup -cA2\n+\tEOF\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\tFAKE_COMMIT_MESSAGE=\"edited and fixed up\" \\\n+\t\t\tgit rebase -i initial initial\n+\t) &&\n+\tgit log --pretty=format:%B initial.. >actual &&\n+\tcat >expect <<-EOF &&\n+\tedited and fixed up\n+\t$EMPTY\n+\tnew subject\n+\t$EMPTY\n+\tnew\n+\tbody\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 58cfd2f1fda..999d46fafe7 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -32,6 +32,14 @@ test_set_editor () {\n \texport EDITOR\n }\n \n+# Like test_set_editor but sets GIT_SEQUENCE_EDITOR instead of EDITOR\n+test_set_sequence_editor () {\n+\tFAKE_SEQUENCE_EDITOR=\"$1\"\n+\texport FAKE_SEQUENCE_EDITOR\n+\tGIT_SEQUENCE_EDITOR='\"$FAKE_SEQUENCE_EDITOR\"'\n+\texport GIT_SEQUENCE_EDITOR\n+}\n+\n test_decode_color () {\n \tawk '\n \t\tfunction name(n) {\n-- \ngitgitgadget\n"},{"id":"472579","messageId":"xmqq5ybs6mv0.fsf@gitster.g","threadId":"59300","inReplyTo":"b050e7c1a4daf6c65aa9b2b6f44248d7b403918e.1677185702.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] rebase -i: match whole word in is_command()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-23T22:25:07Z","receivedAt":"2023-02-23T22:25:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>  \tconst char *str = todo_command_info[command].str;\n>  \tconst char nick = todo_command_info[command].c;\n> -\tconst char *p = *bol + 1;\n> +\tconst char *p = *bol;\n>  \n> -\treturn skip_prefix(*bol, str, bol) ||\n> -\t\t((nick && **bol == nick) &&\n> -\t\t (*p == ' ' || *p == '\\t' || *p == '\\n' || *p == '\\r' || !*p) &&\n> -\t\t (*bol = p));\n> +\treturn (skip_prefix(p, str, &p) || (nick && *p++ == nick)) &&\n> +\t\t(*p == ' ' || *p == '\\t' || *p == '\\n' || *p == '\\r' || !*p) &&\n> +\t\t(*bol = p);\n\nOK.  So we skip the command name string in the line given by the\nend-user (or see if the first letter matches the single letter\ncommand) and make sure it is followed by a whitespace or EOL in\neither case.  The old code was not doing the \"end of word\" check\nfor the longhand at all, which was clearly wrong.\n\nI too find \"&& (*bol = p)\" that pretends to be a Boolean condition\nbut is there only for its side effect distasteful, but I agree with\nyou that fixing it is outside the scope of this patch.\n\n> @@ -2513,7 +2512,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n>  \t\t\tbreak;\n>  \t\t}\n>  \tif (i >= TODO_COMMENT)\n> -\t\treturn -1;\n> +\t\treturn error(_(\"invalid command '%.*s'\"),\n> +\t\t\t     (int)strcspn(bol, \" \\t\\r\\n\"), bol);\n\nNice.\n\n> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh\n> index b57541356bd..1d2f0429aea 100644\n> --- a/t/lib-rebase.sh\n> +++ b/t/lib-rebase.sh\n> @@ -60,7 +60,7 @@ set_fake_editor () {\n>  \t\t\">\")\n>  \t\t\techo >> \"$1\";;\n>  \t\tbad)\n> -\t\t\taction=\"badcmd\";;\n> +\t\t\taction=\"pickled\";;\n\n;-)\n"},{"id":"472580","messageId":"xmqq1qmg6md6.fsf@gitster.g","threadId":"59300","inReplyTo":"d670e284c175db67337ca7ce774ecd9afb089736.1677185702.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] rebase -i: fix parsing of \"fixup -C<commit>\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-23T22:35:49Z","receivedAt":"2023-02-23T22:35:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>  \tif (item->command == TODO_FIXUP) {\n> -\t\tif (skip_prefix(bol, \"-C\", &bol) &&\n> -\t\t   (*bol == ' ' || *bol == '\\t')) {\n> +\t\tif (skip_prefix(bol, \"-C\", &bol)) {\n>  \t\t\tbol += strspn(bol, \" \\t\");\n>  \t\t\titem->flags |= TODO_REPLACE_FIXUP_MSG;\n\nOK.  An explicit check followed by strspn() is an odd way to write\nthis even if it meant to require at least one whitespace, but I\nagree that this one probably did not even mean to require a\nwhitespace there, and the updated code looks much easier to read.\n\n> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh\n> index 1d2f0429aea..7ca5b918f04 100644\n> --- a/t/lib-rebase.sh\n> +++ b/t/lib-rebase.sh\n> ...\n> +test_expect_success 'fixup -[Cc]<commit> works' '\n> +\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n> +\tcat >todo <<-\\EOF &&\n> +\tpick A\n> +\tfixup -CA1\n> +\tpick B\n> +\tfixup -cA2\n> +\tEOF\n\nBy the way, this is much easier to follow, than the todo file written\nin the ugly FAKE_LINES language, to see what is being tested.\n\nWill queue.  Thanks.\n"},{"id":"472650","messageId":"7fd72747-e076-3741-adb4-954bd4c7e9e5@gmx.de","threadId":"59300","inReplyTo":"pull.1486.git.1677185701.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] sequencer parsing fixes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-02-24T14:02:11Z","receivedAt":"2023-02-24T14:03:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Thu, 23 Feb 2023, Phillip Wood via GitGitGadget wrote:\n\n> Fix a couple of small bugs in the parsing of todo lists\n\nThank you, those fixes look quite good to me.\n\nCiao,\nJohannes\n"},{"id":"472654","messageId":"xmqqa6132dyf.fsf@gitster.g","threadId":"59300","inReplyTo":"7fd72747-e076-3741-adb4-954bd4c7e9e5@gmx.de","subject":"Re: [PATCH 0/2] sequencer parsing fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-24T17:03:20Z","receivedAt":"2023-02-24T17:03:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi Phillip,\n>\n> On Thu, 23 Feb 2023, Phillip Wood via GitGitGadget wrote:\n>\n>> Fix a couple of small bugs in the parsing of todo lists\n>\n> Thank you, those fixes look quite good to me.\n\nThanks, both.  Let's merge them down quickly.\n"},{"id":"472744","messageId":"Y/s0iV5cS9fJ2ONl@coredump.intra.peff.net","threadId":"59300","inReplyTo":"b050e7c1a4daf6c65aa9b2b6f44248d7b403918e.1677185702.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] rebase -i: match whole word in is_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-26T10:29:29Z","receivedAt":"2023-02-26T10:29:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2023 at 08:55:00PM +0000, Phillip Wood via GitGitGadget wrote:\n\n> Fix this by ensuring the match is follow by whitespace or the end of the\n> string as we already do for abbreviated commands. The (*bol = p) at the\n> end of the condition is a bit cute for my taste but I decided to leave\n> it be for now. Rather than add new tests the existing tests for bad\n> commands are adapted to use a bad command name that triggers the prefix\n> matching bug.\n\nFWIW, coverity complained about the (*bol = p) assignment in the\nconditional, since \"p\" must be non-NULL at this point.\n\nSo this is email is a combination of:\n\n  - a data point that it is not just you that finds it a bit cute (in\n    case you do want to change it later); and\n\n  - a hearty thank you for mentioning it in the commit message, since\n    just looking at the code left me scratching my head at whether this\n    was a bug.\n\n-Peff\n"}]}