{"thread":{"id":"51378","subject":"[PATCH 0/3] Wip/quieter sequencer status parsing","startedAt":"2019-06-25T10:11:27Z","lastAt":"2019-07-01T14:41:05Z","messageCount":17,"participants":["Phillip Wood via GitGitGadget","Johannes Schindelin","Phillip Wood","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"377958","messageId":"pull.275.git.gitgitgadget@gmail.com","threadId":"51378","inReplyTo":null,"subject":"[PATCH 0/3] Wip/quieter sequencer status parsing","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-25T10:11:24Z","receivedAt":"2019-06-25T10:11:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"If we cannot parse the oid in the sequencer todo file do not show an error\nwhen running git status but instead report that a cherry-pick or revert is\nin progress. This addresses a confusing error message reported in \nhttps://public-inbox.org/git/3bc58c33-4268-4e7c-bf1a-fe349b3cb037@www.fastmail.com/\n\nThese patches are based on maint\n\nPhillip Wood (3):\n  sequencer: always allow tab after command name\n  sequencer: factor out todo command name parsing\n  status: do not report errors in sequencer/todo\n\n sequencer.c            | 43 +++++++++++++++++++++---------------------\n t/t7512-status-help.sh | 16 ++++++++++++++++\n 2 files changed, 37 insertions(+), 22 deletions(-)\n\n\nbase-commit: b697d92f56511e804b8ba20ccbe7bdc85dc66810\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-275%2Fphillipwood%2Fwip%2Fquieter%C2%A0sequencer%C2%A0status%C2%A0parsing-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-275/phillipwood/wip/quieter sequencer status parsing-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/275\n-- \ngitgitgadget\n"},{"id":"377959","messageId":"a5bede1cf9cba89c354779e559e19d7a2fffa1eb.1561457483.git.gitgitgadget@gmail.com","threadId":"51378","inReplyTo":"pull.275.git.gitgitgadget@gmail.com","subject":"[PATCH 1/3] sequencer: always allow tab after command name","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-25T10:11:25Z","receivedAt":"2019-06-25T10:11:28Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThe code that parses the todo list allows an unabbreviated command name\nto be followed by a space or a tab, but if the command name is\nabbreviated it only allows a space after it. Fix this inconsistency.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f88a97fb10..919e3153f5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2100,7 +2100,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \t\tif (skip_prefix(bol, todo_command_info[i].str, &bol)) {\n \t\t\titem->command = i;\n \t\t\tbreak;\n-\t\t} else if ((bol + 1 == eol || bol[1] == ' ') &&\n+\t\t} else if ((bol + 1 == eol || bol[1] == ' ' || bol[1] == '\\t') &&\n \t\t\t   *bol == todo_command_info[i].c) {\n \t\t\tbol++;\n \t\t\titem->command = i;\n-- \ngitgitgadget\n\n"},{"id":"377960","messageId":"ebe8a2b3a1638e8457b1b703b8c4890a9b8904a0.1561457483.git.gitgitgadget@gmail.com","threadId":"51378","inReplyTo":"pull.275.git.gitgitgadget@gmail.com","subject":"[PATCH 2/3] sequencer: factor out todo command name parsing","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-25T10:11:25Z","receivedAt":"2019-06-25T10:11:29Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nFactor out the code that parses the name of the command at the start of\neach line in the todo file into it's own function so that it can be used\nin the next commit.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c | 19 +++++++++++++------\n 1 file changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 919e3153f5..793f86bf9a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list,\n \treturn todo_list->buf.buf + item->arg_offset;\n }\n \n+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+\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+}\n+\n static int parse_insn_line(struct repository *r, struct todo_item *item,\n \t\t\t   const char *buf, const char *bol, char *eol)\n {\n@@ -2097,12 +2109,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \t}\n \n \tfor (i = 0; i < TODO_COMMENT; i++)\n-\t\tif (skip_prefix(bol, todo_command_info[i].str, &bol)) {\n-\t\t\titem->command = i;\n-\t\t\tbreak;\n-\t\t} else if ((bol + 1 == eol || bol[1] == ' ' || bol[1] == '\\t') &&\n-\t\t\t   *bol == todo_command_info[i].c) {\n-\t\t\tbol++;\n+\t\tif (is_command(i, &bol)) {\n \t\t\titem->command = i;\n \t\t\tbreak;\n \t\t}\n-- \ngitgitgadget\n\n"},{"id":"377961","messageId":"af4b823caac84899b5ac71da61af5ec00f88bb2f.1561457483.git.gitgitgadget@gmail.com","threadId":"51378","inReplyTo":"pull.275.git.gitgitgadget@gmail.com","subject":"[PATCH 3/3] status: do not report errors in sequencer/todo","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-25T10:11:26Z","receivedAt":"2019-06-25T10:11:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\ncommit 4a72486de9 (\"fix cherry-pick/revert status after commit\",\n2019-04-16) used parse_insn_line() to parse the first line of the todo\nlist to check if it was a pick or revert. However if the todo list is\nleft over from an old cherry-pick or revert and references a commit that\nno longer exists then parse_insn_line() prints an error message which is\nconfusing for users [1]. Instead parse just the command name so that the\nuser is alerted to the presence of stale sequencer state by status\nreporting that a cherry-pick or revert is in progress.\n\nNote that we should not be leaving stale sequencer state lying around\n(or at least not as often) after commit b07d9bfd17 (\"commit/reset: try\nto clean up sequencer state\", 2019-04-16). However the user may still\nhave stale state that predates that commit.\n\nAlso avoid printing an error message if for some reason the user has a\nfile called `sequencer` in $GIT_DIR.\n\n[1] https://public-inbox.org/git/3bc58c33-4268-4e7c-bf1a-fe349b3cb037@www.fastmail.com/\n\nReported-by: Espen Antonsen <espen@inspired.no>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c            | 24 ++++++++----------------\n t/t7512-status-help.sh | 16 ++++++++++++++++\n 2 files changed, 24 insertions(+), 16 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 793f86bf9a..f8eab1697e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2177,34 +2177,26 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \n int sequencer_get_last_command(struct repository *r, enum replay_action *action)\n {\n-\tstruct todo_item item;\n-\tchar *eol;\n-\tconst char *todo_file;\n+\tconst char *todo_file, *bol;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint ret = -1;\n+\tint ret = 0;\n \n \ttodo_file = git_path_todo_file();\n \tif (strbuf_read_file(&buf, todo_file, 0) < 0) {\n-\t\tif (errno == ENOENT)\n+\t\tif (errno == ENOENT || errno == ENOTDIR)\n \t\t\treturn -1;\n \t\telse\n \t\t\treturn error_errno(\"unable to open '%s'\", todo_file);\n \t}\n-\teol = strchrnul(buf.buf, '\\n');\n-\tif (buf.buf != eol && eol[-1] == '\\r')\n-\t\teol--; /* strip Carriage Return */\n-\tif (parse_insn_line(r, &item, buf.buf, buf.buf, eol))\n-\t\tgoto fail;\n-\tif (item.command == TODO_PICK)\n+\tbol = buf.buf + strspn(buf.buf, \" \\t\\r\\n\");\n+\tif (is_command(TODO_PICK, &bol) && (*bol == ' ' || *bol == '\\t'))\n \t\t*action = REPLAY_PICK;\n-\telse if (item.command == TODO_REVERT)\n+\telse if (is_command(TODO_REVERT, &bol) &&\n+\t\t (*bol == ' ' || *bol == '\\t'))\n \t\t*action = REPLAY_REVERT;\n \telse\n-\t\tgoto fail;\n-\n-\tret = 0;\n+\t\tret = -1;\n \n- fail:\n \tstrbuf_release(&buf);\n \n \treturn ret;\ndiff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh\nindex c1eb72555d..3c9308709a 100755\n--- a/t/t7512-status-help.sh\n+++ b/t/t7512-status-help.sh\n@@ -798,6 +798,22 @@ EOF\n \ttest_i18ncmp expected actual\n '\n \n+test_expect_success 'status shows cherry-pick with invalid oid' '\n+\tmkdir .git/sequencer &&\n+\ttest_write_lines \"pick invalid-oid\" >.git/sequencer/todo &&\n+\tgit status --untracked-files=no >actual 2>err &&\n+\tgit cherry-pick --quit &&\n+\ttest_must_be_empty err &&\n+\ttest_i18ncmp expected actual\n+'\n+\n+test_expect_success 'status does not show error if .git/sequencer is a file' '\n+\ttest_when_finished \"rm .git/sequencer\" &&\n+\ttest_write_lines hello >.git/sequencer &&\n+\tgit status --untracked-files=no 2>err &&\n+\ttest_must_be_empty err\n+'\n+\n test_expect_success 'status showing detached at and from a tag' '\n \ttest_commit atag tagging &&\n \tgit checkout atag &&\n-- \ngitgitgadget\n"},{"id":"377975","messageId":"nycvar.QRO.7.76.6.1906251356110.44@tvgsbejvaqbjf.bet","threadId":"51378","inReplyTo":"af4b823caac84899b5ac71da61af5ec00f88bb2f.1561457483.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] status: do not report errors in sequencer/todo","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-25T11:56:46Z","receivedAt":"2019-06-25T11:56:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Tue, 25 Jun 2019, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> commit 4a72486de9 (\"fix cherry-pick/revert status after commit\",\n> 2019-04-16) used parse_insn_line() to parse the first line of the todo\n> list to check if it was a pick or revert. However if the todo list is\n> left over from an old cherry-pick or revert and references a commit that\n> no longer exists then parse_insn_line() prints an error message which is\n> confusing for users [1]. Instead parse just the command name so that the\n> user is alerted to the presence of stale sequencer state by status\n> reporting that a cherry-pick or revert is in progress.\n>\n> Note that we should not be leaving stale sequencer state lying around\n> (or at least not as often) after commit b07d9bfd17 (\"commit/reset: try\n> to clean up sequencer state\", 2019-04-16). However the user may still\n> have stale state that predates that commit.\n>\n> Also avoid printing an error message if for some reason the user has a\n> file called `sequencer` in $GIT_DIR.\n>\n> [1] https://public-inbox.org/git/3bc58c33-4268-4e7c-bf1a-fe349b3cb037@www.fastmail.com/\n\nVery nice, I particularly like the simplicity of the code in `sequencer.c`\nafter this patch.\n\nThe entire patch series looks good to me.\n\nThanks,\nDscho\n\n>\n> Reported-by: Espen Antonsen <espen@inspired.no>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  sequencer.c            | 24 ++++++++----------------\n>  t/t7512-status-help.sh | 16 ++++++++++++++++\n>  2 files changed, 24 insertions(+), 16 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 793f86bf9a..f8eab1697e 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2177,34 +2177,26 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n>\n>  int sequencer_get_last_command(struct repository *r, enum replay_action *action)\n>  {\n> -\tstruct todo_item item;\n> -\tchar *eol;\n> -\tconst char *todo_file;\n> +\tconst char *todo_file, *bol;\n>  \tstruct strbuf buf = STRBUF_INIT;\n> -\tint ret = -1;\n> +\tint ret = 0;\n>\n>  \ttodo_file = git_path_todo_file();\n>  \tif (strbuf_read_file(&buf, todo_file, 0) < 0) {\n> -\t\tif (errno == ENOENT)\n> +\t\tif (errno == ENOENT || errno == ENOTDIR)\n>  \t\t\treturn -1;\n>  \t\telse\n>  \t\t\treturn error_errno(\"unable to open '%s'\", todo_file);\n>  \t}\n> -\teol = strchrnul(buf.buf, '\\n');\n> -\tif (buf.buf != eol && eol[-1] == '\\r')\n> -\t\teol--; /* strip Carriage Return */\n> -\tif (parse_insn_line(r, &item, buf.buf, buf.buf, eol))\n> -\t\tgoto fail;\n> -\tif (item.command == TODO_PICK)\n> +\tbol = buf.buf + strspn(buf.buf, \" \\t\\r\\n\");\n> +\tif (is_command(TODO_PICK, &bol) && (*bol == ' ' || *bol == '\\t'))\n>  \t\t*action = REPLAY_PICK;\n> -\telse if (item.command == TODO_REVERT)\n> +\telse if (is_command(TODO_REVERT, &bol) &&\n> +\t\t (*bol == ' ' || *bol == '\\t'))\n>  \t\t*action = REPLAY_REVERT;\n>  \telse\n> -\t\tgoto fail;\n> -\n> -\tret = 0;\n> +\t\tret = -1;\n>\n> - fail:\n>  \tstrbuf_release(&buf);\n>\n>  \treturn ret;\n> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh\n> index c1eb72555d..3c9308709a 100755\n> --- a/t/t7512-status-help.sh\n> +++ b/t/t7512-status-help.sh\n> @@ -798,6 +798,22 @@ EOF\n>  \ttest_i18ncmp expected actual\n>  '\n>\n> +test_expect_success 'status shows cherry-pick with invalid oid' '\n> +\tmkdir .git/sequencer &&\n> +\ttest_write_lines \"pick invalid-oid\" >.git/sequencer/todo &&\n> +\tgit status --untracked-files=no >actual 2>err &&\n> +\tgit cherry-pick --quit &&\n> +\ttest_must_be_empty err &&\n> +\ttest_i18ncmp expected actual\n> +'\n> +\n> +test_expect_success 'status does not show error if .git/sequencer is a file' '\n> +\ttest_when_finished \"rm .git/sequencer\" &&\n> +\ttest_write_lines hello >.git/sequencer &&\n> +\tgit status --untracked-files=no 2>err &&\n> +\ttest_must_be_empty err\n> +'\n> +\n>  test_expect_success 'status showing detached at and from a tag' '\n>  \ttest_commit atag tagging &&\n>  \tgit checkout atag &&\n> --\n> gitgitgadget\n>\n"},{"id":"377979","messageId":"21a27493-220d-3c4c-63c0-7ffe60e19c49@gmail.com","threadId":"51378","inReplyTo":"pull.275.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Wip/quieter sequencer status parsing","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-06-25T13:26:53Z","receivedAt":"2019-06-25T13:26:57Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Please ignore the 'Wip' in the title, I forget to edit it on gitgitgadget\n\nBest Wishes\n\nPhillip\n\nOn 25/06/2019 11:11, Phillip Wood via GitGitGadget wrote:\n> If we cannot parse the oid in the sequencer todo file do not show an error\n> when running git status but instead report that a cherry-pick or revert is\n> in progress. This addresses a confusing error message reported in\n> https://public-inbox.org/git/3bc58c33-4268-4e7c-bf1a-fe349b3cb037@www.fastmail.com/\n> \n> These patches are based on maint\n> \n> Phillip Wood (3):\n>    sequencer: always allow tab after command name\n>    sequencer: factor out todo command name parsing\n>    status: do not report errors in sequencer/todo\n> \n>   sequencer.c            | 43 +++++++++++++++++++++---------------------\n>   t/t7512-status-help.sh | 16 ++++++++++++++++\n>   2 files changed, 37 insertions(+), 22 deletions(-)\n> \n> \n> base-commit: b697d92f56511e804b8ba20ccbe7bdc85dc66810\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-275%2Fphillipwood%2Fwip%2Fquieter%C2%A0sequencer%C2%A0status%C2%A0parsing-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-275/phillipwood/wip/quieter sequencer status parsing-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/275\n> \n"},{"id":"378026","messageId":"ebaa5956-37aa-29b6-60c7-f287e06db505@web.de","threadId":"51378","inReplyTo":"ebe8a2b3a1638e8457b1b703b8c4890a9b8904a0.1561457483.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] sequencer: factor out todo command name parsing","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-06-25T17:03:04Z","receivedAt":"2019-06-25T17:03:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.06.19 um 12:11 schrieb Phillip Wood via GitGitGadget:\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Factor out the code that parses the name of the command at the start of\n> each line in the todo file into it's own function so that it can be used\n> in the next commit.\n\n\"Factor out\" sounds like functionality is intended to not be changed...\n\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  sequencer.c | 19 +++++++++++++------\n>  1 file changed, 13 insertions(+), 6 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 919e3153f5..793f86bf9a 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list,\n>  \treturn todo_list->buf.buf + item->arg_offset;\n>  }\n>\n> +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> +\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                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n... but this adds support for LF, CR and NUL as separators after short\ncommands...\n\n> +\t\t (*bol = p));\n> +}\n> +\n>  static int parse_insn_line(struct repository *r, struct todo_item *item,\n>  \t\t\t   const char *buf, const char *bol, char *eol)\n>  {\n> @@ -2097,12 +2109,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n>  \t}\n>\n>  \tfor (i = 0; i < TODO_COMMENT; i++)\n> -\t\tif (skip_prefix(bol, todo_command_info[i].str, &bol)) {\n> -\t\t\titem->command = i;\n> -\t\t\tbreak;\n> -\t\t} else if ((bol + 1 == eol || bol[1] == ' ' || bol[1] == '\\t') &&\n                            ^^^^^^^^^^^^^^\n... while this removes the check against the string's length.\n\nIs this safe? It probably (hopefully?) is, as skip_prefix() requires bol\nto point to a NUL-terminated string already.\n\nBut is_command() could do the old (and shorter) eol check just as well,\nso the next question is: Why this change?\n\n> -\t\t\t   *bol == todo_command_info[i].c) {\n> -\t\t\tbol++;\n> +\t\tif (is_command(i, &bol)) {\n>  \t\t\titem->command = i;\n>  \t\t\tbreak;\n>  \t\t}\n>\n\n"},{"id":"378031","messageId":"08709a9f-0c4b-996c-260f-e4702f61c4d5@gmail.com","threadId":"51378","inReplyTo":"ebaa5956-37aa-29b6-60c7-f287e06db505@web.de","subject":"Re: [PATCH 2/3] sequencer: factor out todo command name parsing","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-06-25T17:54:37Z","receivedAt":"2019-06-25T17:54:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi René\n\nOn 25/06/2019 18:03, René Scharfe wrote:\n> Am 25.06.19 um 12:11 schrieb Phillip Wood via GitGitGadget:\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> Factor out the code that parses the name of the command at the start of\n>> each line in the todo file into it's own function so that it can be used\n>> in the next commit.\n> \n> \"Factor out\" sounds like functionality is intended to not be changed...\n\nIndeed, I thought I should have explained a bit more in the commit\nmessage after I sent it. I think it is unchanged but it's not\nimmediately obvious.\n\n>>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> ---\n>>  sequencer.c | 19 +++++++++++++------\n>>  1 file changed, 13 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 919e3153f5..793f86bf9a 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list,\n>>  \treturn todo_list->buf.buf + item->arg_offset;\n>>  }\n>>\n>> +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>> +\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>                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n> ... but this adds support for LF, CR and NUL as separators after short\n> commands...\n\nThey're there to detect the end of the line, as I don't want to have to\npass in eol from other callers. The check that the nick name is not '\\0'\nmeans that if we pass in an empty string we do not read past the end as\nthe test will fail before it gets to the \"*p == ...\" part.\n\n> \n>> +\t\t (*bol = p));\n>> +}\n>> +\n>>  static int parse_insn_line(struct repository *r, struct todo_item *item,\n>>  \t\t\t   const char *buf, const char *bol, char *eol)\n>>  {\n>> @@ -2097,12 +2109,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n>>  \t}\n>>\n>>  \tfor (i = 0; i < TODO_COMMENT; i++)\n>> -\t\tif (skip_prefix(bol, todo_command_info[i].str, &bol)) {\n>> -\t\t\titem->command = i;\n>> -\t\t\tbreak;\n>> -\t\t} else if ((bol + 1 == eol || bol[1] == ' ' || bol[1] == '\\t') &&\n>                             ^^^^^^^^^^^^^^\n> ... while this removes the check against the string's length.\n> \n> Is this safe? It probably (hopefully?) is, as skip_prefix() requires bol\n> to point to a NUL-terminated string already.\n\nYes the string is NUL-terminated\n\n> But is_command() could do the old (and shorter) eol check just as well,\n> so the next question is: Why this change?\n\nFor the next patch so other callers don't have to find the end of the\nline if they don't need it. The new checks for '\\n' etc. replace the\n'bol + 1 == eol' part.\n\nI'll reroll with a better commit message\n\nBest Wishes\n\nPhillip\n\n> \n>> -\t\t\t   *bol == todo_command_info[i].c) {\n>> -\t\t\tbol++;\n>> +\t\tif (is_command(i, &bol)) {\n>>  \t\t\titem->command = i;\n>>  \t\t\tbreak;\n>>  \t\t}\n>>\n> \n\n"},{"id":"378041","messageId":"xmqqo92le6ns.fsf@gitster-ct.c.googlers.com","threadId":"51378","inReplyTo":"af4b823caac84899b5ac71da61af5ec00f88bb2f.1561457483.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] status: do not report errors in sequencer/todo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-25T20:44:55Z","receivedAt":"2019-06-25T20:45:07Z","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> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> commit 4a72486de9 (\"fix cherry-pick/revert status after commit\",\n> 2019-04-16) used parse_insn_line() to parse the first line of the todo\n> list to check if it was a pick or revert. However if the todo list is\n> left over from an old cherry-pick or revert and references a commit that\n> no longer exists then parse_insn_line() prints an error message which is\n> confusing for users [1]. Instead parse just the command name so that the\n> user is alerted to the presence of stale sequencer state by status\n> reporting that a cherry-pick or revert is in progress.\n\nI have to wonder what would happen when \"git cherry-pick --continue\"\nis given from such a stale state.  It would have to fail as it would\nnot know the shape of the change to be replayed, no?  \n\nIs it easy to detect and say \"there is an abandoned state file from\nstale cherry-pick\" and optionally offer to remove it (as we have no\nchance of continuing anyway)?\n\nOr is it likely that such an effort would end up being wasted, as...\n\n> Note that we should not be leaving stale sequencer state lying around\n> (or at least not as often) after commit b07d9bfd17 (\"commit/reset: try\n> to clean up sequencer state\", 2019-04-16).\n\n...this already happened?\n\n> Also avoid printing an error message if for some reason the user has a\n> file called `sequencer` in $GIT_DIR.\n\nI agree that it is not a good place for giving diagnostics, but\ngetting ENOTDIR would be an indication that next time when the user\nwants to do a rebase, range pick or range revert, it would not work\nunless somebody removes that `sequencer` file, right?  Are our users\nsufficiently covered on that front (e.g. giving \"we cannot do this\noperation as $GIT_DIR/sequencer is in the way. please remove that\nfile\" would be OK, but silently removing and possibly losing info we\ndidn't even look at may be bad)?\n\nIn any case, the \"unable to open 'sequencer/todo'\" you are removing\nwith this patch was *not* about helping the issue above which is an\nunrelated tangent, so let me not be distracted by that ;-)\n\n> +test_expect_success 'status shows cherry-pick with invalid oid' '\n> +\tmkdir .git/sequencer &&\n> +\ttest_write_lines \"pick invalid-oid\" >.git/sequencer/todo &&\n> +\tgit status --untracked-files=no >actual 2>err &&\n> +\tgit cherry-pick --quit &&\n> +\ttest_must_be_empty err &&\n> +\ttest_i18ncmp expected actual\n> +'\n> +\n> +test_expect_success 'status does not show error if .git/sequencer is a file' '\n> +\ttest_when_finished \"rm .git/sequencer\" &&\n\nIn short, I was wondering what happens if we lose this line, and\nthen we later had a command that needs to use .git/sequencer/todo or\nsomething.\n\n> +\ttest_write_lines hello >.git/sequencer &&\n> +\tgit status --untracked-files=no 2>err &&\n> +\ttest_must_be_empty err\n> +'\n> +\n>  test_expect_success 'status showing detached at and from a tag' '\n>  \ttest_commit atag tagging &&\n>  \tgit checkout atag &&\n"},{"id":"378057","messageId":"51d31257-7093-a0fa-9681-929b9fea059c@gmail.com","threadId":"51378","inReplyTo":"xmqqo92le6ns.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] status: do not report errors in sequencer/todo","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-06-26T08:57:58Z","receivedAt":"2019-06-26T08:58:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nThanks for the comments\n\nOn 25/06/2019 21:44, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> commit 4a72486de9 (\"fix cherry-pick/revert status after commit\",\n>> 2019-04-16) used parse_insn_line() to parse the first line of the todo\n>> list to check if it was a pick or revert. However if the todo list is\n>> left over from an old cherry-pick or revert and references a commit that\n>> no longer exists then parse_insn_line() prints an error message which is\n>> confusing for users [1]. Instead parse just the command name so that the\n>> user is alerted to the presence of stale sequencer state by status\n>> reporting that a cherry-pick or revert is in progress.\n> \n> I have to wonder what would happen when \"git cherry-pick --continue\"\n> is given from such a stale state.  It would have to fail as it would\n> not know the shape of the change to be replayed, no?\n\nYes it would fail with\n   error: invalid line <line-no>: <line>\n   error: unusable instruction sheet: '.git/sequencer/todo'\n   fatal: cherry-pick failed\n\n> Is it easy to detect and say \"there is an abandoned state file from\n> stale cherry-pick\" and optionally offer to remove it (as we have no\n> chance of continuing anyway)?\n\nI guess we could have a specific error return if get_oid() failed and \nsuggest that then\n\n> Or is it likely that such an effort would end up being wasted, as...\n> \n>> Note that we should not be leaving stale sequencer state lying around\n>> (or at least not as often) after commit b07d9bfd17 (\"commit/reset: try\n>> to clean up sequencer state\", 2019-04-16).\n> \n> ...this already happened?\n\nProbably. It is still possible for the user to run checkout in the \nmiddle of a cherry-pick and forget to finish it but if they're using a \nprompt with git support it should tell them that a cherry-pick is in \nprogress as `git status` detects that it is.\n\n>> Also avoid printing an error message if for some reason the user has a\n>> file called `sequencer` in $GIT_DIR.\n> \n> I agree that it is not a good place for giving diagnostics, but\n> getting ENOTDIR would be an indication that next time when the user\n> wants to do a rebase, range pick or range revert, it would not work\n> unless somebody removes that `sequencer` file, right?  Are our users\n> sufficiently covered on that front (e.g. giving \"we cannot do this\n> operation as $GIT_DIR/sequencer is in the way. please remove that\n> file\" would be OK, but silently removing and possibly losing info we\n> didn't even look at may be bad)?\n\nmkdir() will fail with ENOTDIR which should be reported with \nerror_errno() I think.\n\nBest Wishes\n\nPhillip\n\n> In any case, the \"unable to open 'sequencer/todo'\" you are removing\n> with this patch was *not* about helping the issue above which is an\n> unrelated tangent, so let me not be distracted by that ;-)\n> \n>> +test_expect_success 'status shows cherry-pick with invalid oid' '\n>> +\tmkdir .git/sequencer &&\n>> +\ttest_write_lines \"pick invalid-oid\" >.git/sequencer/todo &&\n>> +\tgit status --untracked-files=no >actual 2>err &&\n>> +\tgit cherry-pick --quit &&\n>> +\ttest_must_be_empty err &&\n>> +\ttest_i18ncmp expected actual\n>> +'\n>> +\n>> +test_expect_success 'status does not show error if .git/sequencer is a file' '\n>> +\ttest_when_finished \"rm .git/sequencer\" &&\n> \n> In short, I was wondering what happens if we lose this line, and\n> then we later had a command that needs to use .git/sequencer/todo or\n> something.\n> \n>> +\ttest_write_lines hello >.git/sequencer &&\n>> +\tgit status --untracked-files=no 2>err &&\n>> +\ttest_must_be_empty err\n>> +'\n>> +\n>>   test_expect_success 'status showing detached at and from a tag' '\n>>   \ttest_commit atag tagging &&\n>>   \tgit checkout atag &&\n"},{"id":"378163","messageId":"a5bede1cf9cba89c354779e559e19d7a2fffa1eb.1561644762.git.gitgitgadget@gmail.com","threadId":"51378","inReplyTo":"pull.275.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] sequencer: always allow tab after command name","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-27T14:12:44Z","receivedAt":"2019-06-27T14:12:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThe code that parses the todo list allows an unabbreviated command name\nto be followed by a space or a tab, but if the command name is\nabbreviated it only allows a space after it. Fix this inconsistency.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f88a97fb10..919e3153f5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2100,7 +2100,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \t\tif (skip_prefix(bol, todo_command_info[i].str, &bol)) {\n \t\t\titem->command = i;\n \t\t\tbreak;\n-\t\t} else if ((bol + 1 == eol || bol[1] == ' ') &&\n+\t\t} else if ((bol + 1 == eol || bol[1] == ' ' || bol[1] == '\\t') &&\n \t\t\t   *bol == todo_command_info[i].c) {\n \t\t\tbol++;\n \t\t\titem->command = i;\n-- \ngitgitgadget\n\n"},{"id":"378164","messageId":"c3885c0b8f55957695650c2d1e25f9036d7e80dd.1561644762.git.gitgitgadget@gmail.com","threadId":"51378","inReplyTo":"pull.275.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] sequencer: factor out todo command name parsing","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-27T14:12:45Z","receivedAt":"2019-06-27T14:12:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nFactor out the code that parses the name of the command at the start of\neach line in the todo file into its own function so that it can be used\nin the next commit.\n\nAs I don't want to burden other callers with having to pass in a pointer\nto the end of the line the test for an abbreviated command is\nchanged. This change should not affect the behavior. Instead of testing\n`eol == bol + 1` the new code checks for the end of the line by testing\nfor '\\n', '\\r' or '\\0' following the abbreviated name. To avoid reading\npast the end of an empty string it also checks that there is actually a\nsingle character abbreviation before testing if it matches. This\nprevents it from matching '\\0' as the abbreviated name and then trying\nto read another character.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c | 19 +++++++++++++------\n 1 file changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 919e3153f5..793f86bf9a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list,\n \treturn todo_list->buf.buf + item->arg_offset;\n }\n \n+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+\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+}\n+\n static int parse_insn_line(struct repository *r, struct todo_item *item,\n \t\t\t   const char *buf, const char *bol, char *eol)\n {\n@@ -2097,12 +2109,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \t}\n \n \tfor (i = 0; i < TODO_COMMENT; i++)\n-\t\tif (skip_prefix(bol, todo_command_info[i].str, &bol)) {\n-\t\t\titem->command = i;\n-\t\t\tbreak;\n-\t\t} else if ((bol + 1 == eol || bol[1] == ' ' || bol[1] == '\\t') &&\n-\t\t\t   *bol == todo_command_info[i].c) {\n-\t\t\tbol++;\n+\t\tif (is_command(i, &bol)) {\n \t\t\titem->command = i;\n \t\t\tbreak;\n \t\t}\n-- \ngitgitgadget\n\n"},{"id":"378165","messageId":"pull.275.v2.git.gitgitgadget@gmail.com","threadId":"51378","inReplyTo":"pull.275.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Quieter sequencer status parsing","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-27T14:12:43Z","receivedAt":"2019-06-27T14:12:51Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Thanks for the comments on v1. I've updated the commit message of the second\npatch to try and explain why the new function is not a straight-forward copy\nof the old code\n\nIf we cannot parse the oid in the sequencer todo file do not show an error\nwhen running git status but instead report that a cherry-pick or revert is\nin progress. This addresses a confusing error message reported in \nhttps://public-inbox.org/git/3bc58c33-4268-4e7c-bf1a-fe349b3cb037@www.fastmail.com/\n\nThese patches are based on maint\n\nCc: Johannes Schindelin Johannes.Schindelin@gmx.de\n[Johannes.Schindelin@gmx.de]\n\nPhillip Wood (3):\n  sequencer: always allow tab after command name\n  sequencer: factor out todo command name parsing\n  status: do not report errors in sequencer/todo\n\n sequencer.c            | 43 +++++++++++++++++++++---------------------\n t/t7512-status-help.sh | 16 ++++++++++++++++\n 2 files changed, 37 insertions(+), 22 deletions(-)\n\n\nbase-commit: b697d92f56511e804b8ba20ccbe7bdc85dc66810\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-275%2Fphillipwood%2Fwip%2Fquieter%C2%A0sequencer%C2%A0status%C2%A0parsing-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-275/phillipwood/wip/quieter sequencer status parsing-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/275\n\nRange-diff vs v1:\n\n 1:  a5bede1cf9 = 1:  a5bede1cf9 sequencer: always allow tab after command name\n 2:  ebe8a2b3a1 ! 2:  c3885c0b8f sequencer: factor out todo command name parsing\n     @@ -3,9 +3,19 @@\n          sequencer: factor out todo command name parsing\n      \n          Factor out the code that parses the name of the command at the start of\n     -    each line in the todo file into it's own function so that it can be used\n     +    each line in the todo file into its own function so that it can be used\n          in the next commit.\n      \n     +    As I don't want to burden other callers with having to pass in a pointer\n     +    to the end of the line the test for an abbreviated command is\n     +    changed. This change should not affect the behavior. Instead of testing\n     +    `eol == bol + 1` the new code checks for the end of the line by testing\n     +    for '\\n', '\\r' or '\\0' following the abbreviated name. To avoid reading\n     +    past the end of an empty string it also checks that there is actually a\n     +    single character abbreviation before testing if it matches. This\n     +    prevents it from matching '\\0' as the abbreviated name and then trying\n     +    to read another character.\n     +\n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n      \n       diff --git a/sequencer.c b/sequencer.c\n 3:  af4b823caa = 3:  3e8fb61fb8 status: do not report errors in sequencer/todo\n\n-- \ngitgitgadget\n"},{"id":"378166","messageId":"3e8fb61fb835a831dbd0303ff751aff0298a1e4c.1561644762.git.gitgitgadget@gmail.com","threadId":"51378","inReplyTo":"pull.275.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] status: do not report errors in sequencer/todo","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-27T14:12:46Z","receivedAt":"2019-06-27T14:12:52Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\ncommit 4a72486de9 (\"fix cherry-pick/revert status after commit\",\n2019-04-16) used parse_insn_line() to parse the first line of the todo\nlist to check if it was a pick or revert. However if the todo list is\nleft over from an old cherry-pick or revert and references a commit that\nno longer exists then parse_insn_line() prints an error message which is\nconfusing for users [1]. Instead parse just the command name so that the\nuser is alerted to the presence of stale sequencer state by status\nreporting that a cherry-pick or revert is in progress.\n\nNote that we should not be leaving stale sequencer state lying around\n(or at least not as often) after commit b07d9bfd17 (\"commit/reset: try\nto clean up sequencer state\", 2019-04-16). However the user may still\nhave stale state that predates that commit.\n\nAlso avoid printing an error message if for some reason the user has a\nfile called `sequencer` in $GIT_DIR.\n\n[1] https://public-inbox.org/git/3bc58c33-4268-4e7c-bf1a-fe349b3cb037@www.fastmail.com/\n\nReported-by: Espen Antonsen <espen@inspired.no>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c            | 24 ++++++++----------------\n t/t7512-status-help.sh | 16 ++++++++++++++++\n 2 files changed, 24 insertions(+), 16 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 793f86bf9a..f8eab1697e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2177,34 +2177,26 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \n int sequencer_get_last_command(struct repository *r, enum replay_action *action)\n {\n-\tstruct todo_item item;\n-\tchar *eol;\n-\tconst char *todo_file;\n+\tconst char *todo_file, *bol;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint ret = -1;\n+\tint ret = 0;\n \n \ttodo_file = git_path_todo_file();\n \tif (strbuf_read_file(&buf, todo_file, 0) < 0) {\n-\t\tif (errno == ENOENT)\n+\t\tif (errno == ENOENT || errno == ENOTDIR)\n \t\t\treturn -1;\n \t\telse\n \t\t\treturn error_errno(\"unable to open '%s'\", todo_file);\n \t}\n-\teol = strchrnul(buf.buf, '\\n');\n-\tif (buf.buf != eol && eol[-1] == '\\r')\n-\t\teol--; /* strip Carriage Return */\n-\tif (parse_insn_line(r, &item, buf.buf, buf.buf, eol))\n-\t\tgoto fail;\n-\tif (item.command == TODO_PICK)\n+\tbol = buf.buf + strspn(buf.buf, \" \\t\\r\\n\");\n+\tif (is_command(TODO_PICK, &bol) && (*bol == ' ' || *bol == '\\t'))\n \t\t*action = REPLAY_PICK;\n-\telse if (item.command == TODO_REVERT)\n+\telse if (is_command(TODO_REVERT, &bol) &&\n+\t\t (*bol == ' ' || *bol == '\\t'))\n \t\t*action = REPLAY_REVERT;\n \telse\n-\t\tgoto fail;\n-\n-\tret = 0;\n+\t\tret = -1;\n \n- fail:\n \tstrbuf_release(&buf);\n \n \treturn ret;\ndiff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh\nindex c1eb72555d..3c9308709a 100755\n--- a/t/t7512-status-help.sh\n+++ b/t/t7512-status-help.sh\n@@ -798,6 +798,22 @@ EOF\n \ttest_i18ncmp expected actual\n '\n \n+test_expect_success 'status shows cherry-pick with invalid oid' '\n+\tmkdir .git/sequencer &&\n+\ttest_write_lines \"pick invalid-oid\" >.git/sequencer/todo &&\n+\tgit status --untracked-files=no >actual 2>err &&\n+\tgit cherry-pick --quit &&\n+\ttest_must_be_empty err &&\n+\ttest_i18ncmp expected actual\n+'\n+\n+test_expect_success 'status does not show error if .git/sequencer is a file' '\n+\ttest_when_finished \"rm .git/sequencer\" &&\n+\ttest_write_lines hello >.git/sequencer &&\n+\tgit status --untracked-files=no 2>err &&\n+\ttest_must_be_empty err\n+'\n+\n test_expect_success 'status showing detached at and from a tag' '\n \ttest_commit atag tagging &&\n \tgit checkout atag &&\n-- \ngitgitgadget\n"},{"id":"378174","messageId":"xmqqlfxnaquo.fsf@gitster-ct.c.googlers.com","threadId":"51378","inReplyTo":"a5bede1cf9cba89c354779e559e19d7a2fffa1eb.1561644762.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] sequencer: always allow tab after command name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-27T17:19:11Z","receivedAt":"2019-06-27T17:19:19Z","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> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> The code that parses the todo list allows an unabbreviated command name\n> to be followed by a space or a tab, but if the command name is\n> abbreviated it only allows a space after it. Fix this inconsistency.\n\nMakes sense.\n\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  sequencer.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index f88a97fb10..919e3153f5 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2100,7 +2100,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n>  \t\tif (skip_prefix(bol, todo_command_info[i].str, &bol)) {\n>  \t\t\titem->command = i;\n>  \t\t\tbreak;\n> -\t\t} else if ((bol + 1 == eol || bol[1] == ' ') &&\n> +\t\t} else if ((bol + 1 == eol || bol[1] == ' ' || bol[1] == '\\t') &&\n>  \t\t\t   *bol == todo_command_info[i].c) {\n>  \t\t\tbol++;\n>  \t\t\titem->command = i;\n"},{"id":"378177","messageId":"xmqqh88baqcu.fsf@gitster-ct.c.googlers.com","threadId":"51378","inReplyTo":"c3885c0b8f55957695650c2d1e25f9036d7e80dd.1561644762.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] sequencer: factor out todo command name parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-27T17:29:53Z","receivedAt":"2019-06-27T17:30:02Z","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> As I don't want to burden other callers with having to pass in a pointer\n> to the end of the line the test for an abbreviated command is\n> changed.\n\nA comma missing somewhere between \"As <<REASON>>, <<CONSEQUENCE>>\",\nperhaps after \"end of the line\"?\n\n> This change should not affect the behavior. Instead of testing\n> `eol == bol + 1` the new code checks for the end of the line by testing\n> for '\\n', '\\r' or '\\0' following the abbreviated name. To avoid reading\n> past the end of an empty string it also checks that there is actually a\n> single character abbreviation before testing if it matches. This\n> prevents it from matching '\\0' as the abbreviated name and then trying\n> to read another character.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  sequencer.c | 19 +++++++++++++------\n>  1 file changed, 13 insertions(+), 6 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 919e3153f5..793f86bf9a 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2076,6 +2076,18 @@ const char *todo_item_get_arg(struct todo_list *todo_list,\n>  \treturn todo_list->buf.buf + item->arg_offset;\n>  }\n>  \n> +static int is_command(enum todo_command command, const char **bol)\n> +{\n\nThis is a tangent, but the reason why the caller of this function is\nnamed parse_insn_line() (and not parse_command_line()) is because a\n\"command\" often refers \"rebase\", \"cherry-pick\" etc., and we need a\nword to differenciate these from what is to be done as an individual\nstep.  Once the codebase stabilizes (read: I am excluding this kind\nof change outside the scope of a series like this one), we'd need to\nclean up the names in this file, I think.\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> +\n> +\treturn skip_prefix(*bol, str, bol) ||\n> +\t\t((nick && **bol == nick) &&\n\nOK, making sure that nick is not NUL is the key to avoid stepping\npast the NUL after the line that begins at *bol, as explained in the\nadditional paragraph in the proposed log message.\n\nLooking good.\n\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>  \t\t\t   const char *buf, const char *bol, char *eol)\n>  {\n> @@ -2097,12 +2109,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n>  \t}\n>  \n>  \tfor (i = 0; i < TODO_COMMENT; i++)\n> -\t\tif (skip_prefix(bol, todo_command_info[i].str, &bol)) {\n> -\t\t\titem->command = i;\n> -\t\t\tbreak;\n> -\t\t} else if ((bol + 1 == eol || bol[1] == ' ' || bol[1] == '\\t') &&\n> -\t\t\t   *bol == todo_command_info[i].c) {\n> -\t\t\tbol++;\n> +\t\tif (is_command(i, &bol)) {\n>  \t\t\titem->command = i;\n>  \t\t\tbreak;\n>  \t\t}\n"},{"id":"378395","messageId":"1015625d-9975-3991-ebd2-25ba212286e3@gmail.com","threadId":"51378","inReplyTo":"51d31257-7093-a0fa-9681-929b9fea059c@gmail.com","subject":"Re: [PATCH 3/3] status: do not report errors in sequencer/todo","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-07-01T14:40:57Z","receivedAt":"2019-07-01T14:41:05Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 26/06/2019 09:57, Phillip Wood wrote:\n> On 25/06/2019 21:44, Junio C Hamano wrote:\n>> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>\n>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>>\n>>> commit 4a72486de9 (\"fix cherry-pick/revert status after commit\",\n>>> 2019-04-16) used parse_insn_line() to parse the first line of the todo\n>>> list to check if it was a pick or revert. However if the todo list is\n>>> left over from an old cherry-pick or revert and references a commit that\n>>> no longer exists then parse_insn_line() prints an error message which is\n>>> confusing for users [1]. Instead parse just the command name so that the\n>>> user is alerted to the presence of stale sequencer state by status\n>>> reporting that a cherry-pick or revert is in progress.\n>>\n>> Or is it likely that such an effort would end up being wasted, as...\n>>\n>>> Note that we should not be leaving stale sequencer state lying around\n>>> (or at least not as often) after commit b07d9bfd17 (\"commit/reset: try\n>>> to clean up sequencer state\", 2019-04-16).\n>>\n>> ...this already happened?\n> \n> Probably. It is still possible for the user to run checkout in the \n> middle of a cherry-pick and forget to finish it but if they're using a \n> prompt with git support it should tell them that a cherry-pick is in \n> progress as `git status` detects that it is.\n\nUnfortunately it is not true that the prompt will see the in-progress \ncherry-pick as it does not use git status - see \nhttps://public-inbox.org/git/0f04fa2930d5cc7dfd2a5c5185573f7ecefa6055.1561990865.git.gitgitgadget@gmail.com/T/#u \nfor a fix\n\nBest Wishes\n\nPhillip\n"}]}