{"thread":{"id":"59260","subject":"[PATCH] rebase -i: check labels and refs when parsing todo list","startedAt":"2023-02-17T14:38:04Z","lastAt":"2023-02-21T17:27:12Z","messageCount":10,"participants":["Phillip Wood via GitGitGadget","Derrick Stolee","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"472254","messageId":"pull.1482.git.1676644675638.gitgitgadget@gmail.com","threadId":"59260","inReplyTo":null,"subject":"[PATCH] rebase -i: check labels and refs when parsing todo list","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-17T14:37:55Z","receivedAt":"2023-02-17T14:38:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nCheck that the argument to the \"label\" and \"update-ref\" commands is a\nvalid refname when the todo list is parsed rather than waiting until the\ncommand is executed. This means that the user can deal with any errors\nat the beginning of the rebase rather than having it stop halfway\nthrough due to a typo in a label name. The \"update-ref\" command is\nchanged to reject single level refs as it is all to easy to type\n\"update-ref branch\" which is incorrect rather than \"update-ref\nrefs/heads/branch\"\n\nNote that it is not straight forward to check the arguments to \"reset\"\nand \"merge\" commands as they may be any revision, not just a refname and\nwe do not have an equivalent of check_refname_format() for revisions.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    rebase -i: check labels and refs when parsing todo list\n    \n    Hopefully detecting these errors when the user edits the todo list will\n    give a better experience.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1482%2Fphillipwood%2Frebase-check-labels-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1482/phillipwood/rebase-check-labels-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1482\n\n sequencer.c                   | 37 ++++++++++++++++++++++++++++++++++-\n t/t3404-rebase-interactive.sh | 25 ++++++++++++++++++++++-\n 2 files changed, 60 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3e4a1972897..b4b5c3f331d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2477,6 +2477,26 @@ static int is_command(enum todo_command command, const char **bol)\n \t\t (*bol = p));\n }\n \n+static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n+{\n+\tint allow_onelevel =\n+\t\tcommand == TODO_LABEL ? REFNAME_ALLOW_ONELEVEL : 0;\n+\n+\tif ((command == TODO_LABEL && !strcmp(arg, \"#\")) ||\n+\t    check_refname_format(arg, allow_onelevel)) {\n+\t\tif (command == TODO_LABEL)\n+\t\t\terror(_(\"'%s' is not a valid label\"), arg);\n+\t\telse if (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n+\t\t\terror(_(\"'%s' is not a valid refname\"), arg);\n+\t\telse\n+\t\t\terror(_(\"update-ref requires a fully qualified refname e.g. refs/heads/%s\"),\n+\t\t\t      arg);\n+\t\treturn -1;\n+\t}\n+\n+\treturn 0;\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@@ -2523,8 +2543,23 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \t\treturn error(_(\"missing arguments for %s\"),\n \t\t\t     command_to_string(item->command));\n \n-\tif (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n+\tif (item->command == TODO_LABEL ||\n \t    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {\n+\t\tint ret = 0;\n+\n+\t\titem->commit = NULL;\n+\t\titem->arg_offset = bol - buf;\n+\t\titem->arg_len = (int)(eol - bol);\n+\t\tif (item->command != TODO_RESET) {\n+\t\t\tsaved = *eol;\n+\t\t\t*eol = '\\0';\n+\t\t\tret = check_label_or_ref_arg(item->command, bol);\n+\t\t\t*eol = saved;\n+\t\t}\n+\t\treturn ret;\n+\t}\n+\n+\tif (item->command == TODO_EXEC) {\n \t\titem->commit = NULL;\n \t\titem->arg_offset = bol - buf;\n \t\titem->arg_len = (int)(eol - bol);\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 462cefd25df..2cf2d2b8a24 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -2120,7 +2120,30 @@ test_expect_success '--update-refs: check failed ref update' '\n \ttail -n 6 err >err.last &&\n \tsed -e \"s/Rebasing.*Successfully/Successfully/g\" -e \"s/^\\t//g\" \\\n \t\t<err.last >err.trimmed &&\n-\ttest_cmp expect err.trimmed\n+\ttest_cmp expect err.trimmed &&\n+\tgit rebase --abort\n+'\n+\n+test_expect_success 'bad labels and refs rejected when parsing todo list' '\n+\tcat >todo <<-\\EOF &&\n+\texec >execed\n+\tlabel #\n+\tlabel :invalid\n+\tupdate-ref :bad\n+\tupdate-ref topic\n+\tEOF\n+\trm -f execed &&\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\ttest_must_fail git rebase -i HEAD 2>err\n+\t) &&\n+\tgrep \"'\\''#'\\'' is not a valid label\" err &&\n+\tgrep \"'\\'':invalid'\\'' is not a valid label\" err &&\n+\tgrep \"'\\'':bad'\\'' is not a valid refname\" err &&\n+\tgrep \"update-ref requires a fully qualified refname e.g. refs/heads/topic\" \\\n+\t\terr &&\n+\ttest_path_is_missing execed &&\n+\tgit rebase --abort\n '\n \n # This must be the last test in this file\n\nbase-commit: b1485644f936ee83a995ec24d23f713f4230a1ae\n-- \ngitgitgadget\n"},{"id":"472255","messageId":"0578e154-4c28-ffd9-03fc-5f9f5939cd17@github.com","threadId":"59260","inReplyTo":"pull.1482.git.1676644675638.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase -i: check labels and refs when parsing todo list","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-02-17T15:32:47Z","receivedAt":"2023-02-17T15:32:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/17/2023 9:37 AM, Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n> Check that the argument to the \"label\" and \"update-ref\" commands is a\n> valid refname when the todo list is parsed rather than waiting until the\n> command is executed. This means that the user can deal with any errors\n> at the beginning of the rebase rather than having it stop halfway\n> through due to a typo in a label name.\n\nThanks for thinking about this user experience. This is a good\nimprovement to the user interaction.\n\n> The \"update-ref\" command is\n> changed to reject single level refs as it is all to easy to type\n> \"update-ref branch\" which is incorrect rather than \"update-ref\n> refs/heads/branch\"\n\nI think it's good to start by adding the restriction in this\ncheck, but we could revisit this requirement in the future to\nsee if it is worth allowing the user to drop \"refs/heads/\" and\nlet it be implied. It just adds some complexity to the parsing,\nso this patch adds helpful scaffolding (in checks and tests)\nsuch that we could do that later in a safer way.\n\n> +static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n> +{\n> +\tint allow_onelevel =\n> +\t\tcommand == TODO_LABEL ? REFNAME_ALLOW_ONELEVEL : 0;\n> +\n> +\tif ((command == TODO_LABEL && !strcmp(arg, \"#\")) ||\n\nInteresting that \"#\" means something special for the label, and\nit's not limited to just the start of the label name, but must\nbe the entire string. Is this not something that\ncheck_refname_format() would catch? Is the motivation that users\nmight add what they think is a comment, such as:\n\n  label # make a label here\n\nbut oddly, this doesn't include something strange like\n\n  label #make a label here\n\n> +\t    check_refname_format(arg, allow_onelevel)) {\n> +\t\tif (command == TODO_LABEL)\n> +\t\t\terror(_(\"'%s' is not a valid label\"), arg);\n\nIf we have any kind of error and we are in TODO_LABEL, then\nwe can use a label message. Good.\n\n> +\t\telse if (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n> +\t\t\terror(_(\"'%s' is not a valid refname\"), arg);\n> +\t\telse\n> +\t\t\terror(_(\"update-ref requires a fully qualified refname e.g. refs/heads/%s\"),\n> +\t\t\t      arg);\n\nThis took me a little while to grok, but I think I have it\nnow: when in the update-ref mode, it could fail because of a\none-level ref (the else case) or it could fail because the\nref name uses forbidden characters (the else if case).\n\nThis nesting of conditions seems a bit fragile if we were to\nadd a new todo_command to check here. Perhaps reorganize it\nto switch on the command?\n\n\tswitch (command) {\n\tcase TODO_LABEL:\n\t\tif (!strcmp(arg, \"#\") ||\n\t\t    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n\t\t\treturn error(_(\"'%s' is not a valid label\", arg);\n\t\tbreak;\n\n\tcase TODO_UPDATE_REF:\n\t\tif (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n\t\t\treturn error(_(\"'%s' is not a valid refname\"), arg);\n\t\telse if (check_refname_format(arg, 0))\n\t\t\treturn error(_(\"update-ref reqruies a fully qualified refname (e.g. refs/heads/%s)\",\n\t\t\t\t     arg);\n\t\tbreak;\n\n\tdefault:\n\t\tBUG(\"unexpected todo_command\");\n\t}\n\n\treturn 0;\n\n> @@ -2523,8 +2543,23 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n>  \t\treturn error(_(\"missing arguments for %s\"),\n>  \t\t\t     command_to_string(item->command));\n>  \n> -\tif (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n> +\tif (item->command == TODO_LABEL ||\n>  \t    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {\n> +\t\tint ret = 0;\n> +\n> +\t\titem->commit = NULL;\n> +\t\titem->arg_offset = bol - buf;\n> +\t\titem->arg_len = (int)(eol - bol);\n> +\t\tif (item->command != TODO_RESET) {\n> +\t\t\tsaved = *eol;\n> +\t\t\t*eol = '\\0';\n> +\t\t\tret = check_label_or_ref_arg(item->command, bol);\n> +\t\t\t*eol = saved;\n> +\t\t}\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tif (item->command == TODO_EXEC) {\n>  \t\titem->commit = NULL;\n>  \t\titem->arg_offset = bol - buf;\n>  \t\titem->arg_len = (int)(eol - bol);\n\n(What's missing from this context is \"return 0;\")\n\nIs there an important reason why you separated TODO_EXEC and\nits identical item->arg_(offset|len) parsing into its own\nblock? It seems like we could modify your change to look like\nthis:\n\n\n\tif (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n  \t    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {\n\t\tint ret = 0;\n\n\t\titem->commit = NULL;\n\t\titem->arg_offset = bol - buf;\n\t\titem->arg_len = (int)(eol - bol);\n\t\tif (item->command == TODO_RESET ||\n\t\t    item->command == TODO_UPDATE_REF) {\n\t\t\tsaved = *eol;\n\t\t\t*eol = '\\0';\n\t\t\tret = check_label_or_ref_arg(item->command, bol);\n\t\t\t*eol = saved;\n\t\t}\n\t\treturn ret;\n\t}\n\nand the diff will have fewer new lines as well as fewer\nduplicate lines in the post-image. Am I missing something\nabout TODO_EXEC being special?\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 462cefd25df..2cf2d2b8a24 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -2120,7 +2120,30 @@ test_expect_success '--update-refs: check failed ref update' '\n>  \ttail -n 6 err >err.last &&\n>  \tsed -e \"s/Rebasing.*Successfully/Successfully/g\" -e \"s/^\\t//g\" \\\n>  \t\t<err.last >err.trimmed &&\n> -\ttest_cmp expect err.trimmed\n> +\ttest_cmp expect err.trimmed &&\n> +\tgit rebase --abort\n> +'\n\nPerhaps this `git rebase --abort` should be part of a\n`test_when_finished test_may_fail git rebase --abort` at\nthe start of the test so that your new test can succeed\neven if an earlier test step caused the test to fail.\n\n> +test_expect_success 'bad labels and refs rejected when parsing todo list' '\n> +\tcat >todo <<-\\EOF &&\n> +\texec >execed\n> +\tlabel #\n> +\tlabel :invalid\n> +\tupdate-ref :bad\n> +\tupdate-ref topic\n> +\tEOF\n> +\trm -f execed &&\n> +\t(\n> +\t\tset_replace_editor todo &&\n> +\t\ttest_must_fail git rebase -i HEAD 2>err\n> +\t) &&\n> +\tgrep \"'\\''#'\\'' is not a valid label\" err &&\n> +\tgrep \"'\\'':invalid'\\'' is not a valid label\" err &&\n> +\tgrep \"'\\'':bad'\\'' is not a valid refname\" err &&\n> +\tgrep \"update-ref requires a fully qualified refname e.g. refs/heads/topic\" \\\n> +\t\terr &&\n> +\ttest_path_is_missing execed &&\n> +\tgit rebase --abort\n>  '\n\nAgain, the `git rebase --abort` seems like protection for\nfuture tests, so a test_when_finished would help.\n\nThanks,\n-Stolee\n"},{"id":"472325","messageId":"5a2a62cc-134c-adb0-7cab-c08c16c2dfdf@dunelm.org.uk","threadId":"59260","inReplyTo":"0578e154-4c28-ffd9-03fc-5f9f5939cd17@github.com","subject":"Re: [PATCH] rebase -i: check labels and refs when parsing todo list","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-02-20T09:24:57Z","receivedAt":"2023-02-20T09:25:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stolee\n\nOn 17/02/2023 15:32, Derrick Stolee wrote:\n> On 2/17/2023 9:37 AM, Phillip Wood via GitGitGadget wrote:\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> Check that the argument to the \"label\" and \"update-ref\" commands is a\n>> valid refname when the todo list is parsed rather than waiting until the\n>> command is executed. This means that the user can deal with any errors\n>> at the beginning of the rebase rather than having it stop halfway\n>> through due to a typo in a label name.\n> \n> Thanks for thinking about this user experience. This is a good\n> improvement to the user interaction.\n> \n>> The \"update-ref\" command is\n>> changed to reject single level refs as it is all to easy to type\n>> \"update-ref branch\" which is incorrect rather than \"update-ref\n>> refs/heads/branch\"\n> \n> I think it's good to start by adding the restriction in this\n> check, but we could revisit this requirement in the future to\n> see if it is worth allowing the user to drop \"refs/heads/\" and\n> let it be implied. It just adds some complexity to the parsing,\n> so this patch adds helpful scaffolding (in checks and tests)\n> such that we could do that later in a safer way.\n> \n>> +static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n>> +{\n>> +\tint allow_onelevel =\n>> +\t\tcommand == TODO_LABEL ? REFNAME_ALLOW_ONELEVEL : 0;\n>> +\n>> +\tif ((command == TODO_LABEL && !strcmp(arg, \"#\")) ||\n> \n> Interesting that \"#\" means something special for the label, and\n> it's not limited to just the start of the label name, but must\n> be the entire string. Is this not something that\n> check_refname_format() would catch?\n\n\"#\" is a valid refname, but the merge command uses it to separate the \nmerge parents from the commit summary so it cannot be used as a label. \nIt is rejected at the start of do_label() (arguably that check could be \nremoved with this series but I'm tempted to leave it alone)\n\n> Is the motivation that users\n> might add what they think is a comment, such as:\n> \n>    label # make a label here\n> \n> but oddly, this doesn't include something strange like\n> \n>    label #make a label here\n\nI've actually got some patches based on this that add support for \ncomments like this, but they are not the reason that \"#\" is forbidden as \na label here.\n\n>> +\t    check_refname_format(arg, allow_onelevel)) {\n>> +\t\tif (command == TODO_LABEL)\n>> +\t\t\terror(_(\"'%s' is not a valid label\"), arg);\n> \n> If we have any kind of error and we are in TODO_LABEL, then\n> we can use a label message. Good.\n> \n>> +\t\telse if (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n>> +\t\t\terror(_(\"'%s' is not a valid refname\"), arg);\n>> +\t\telse\n>> +\t\t\terror(_(\"update-ref requires a fully qualified refname e.g. refs/heads/%s\"),\n>> +\t\t\t      arg);\n> \n> This took me a little while to grok, but I think I have it\n> now: when in the update-ref mode, it could fail because of a\n> one-level ref (the else case) or it could fail because the\n> ref name uses forbidden characters (the else if case).\n> \n> This nesting of conditions seems a bit fragile if we were to\n> add a new todo_command to check here. Perhaps reorganize it\n> to switch on the command?\n> \n> \tswitch (command) {\n> \tcase TODO_LABEL:\n> \t\tif (!strcmp(arg, \"#\") ||\n> \t\t    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n> \t\t\treturn error(_(\"'%s' is not a valid label\", arg);\n> \t\tbreak;\n> \n> \tcase TODO_UPDATE_REF:\n> \t\tif (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n> \t\t\treturn error(_(\"'%s' is not a valid refname\"), arg);\n> \t\telse if (check_refname_format(arg, 0))\n> \t\t\treturn error(_(\"update-ref reqruies a fully qualified refname (e.g. refs/heads/%s)\",\n> \t\t\t\t     arg);\n> \t\tbreak;\n> \n> \tdefault:\n> \t\tBUG(\"unexpected todo_command\");\n> \t}\n> \n> \treturn 0;\n\nThat's definitely clearer, thanks for the suggestion\n\n>> @@ -2523,8 +2543,23 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n>>   \t\treturn error(_(\"missing arguments for %s\"),\n>>   \t\t\t     command_to_string(item->command));\n>>   \n>> -\tif (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n>> +\tif (item->command == TODO_LABEL ||\n>>   \t    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {\n>> +\t\tint ret = 0;\n>> +\n>> +\t\titem->commit = NULL;\n>> +\t\titem->arg_offset = bol - buf;\n>> +\t\titem->arg_len = (int)(eol - bol);\n>> +\t\tif (item->command != TODO_RESET) {\n>> +\t\t\tsaved = *eol;\n>> +\t\t\t*eol = '\\0';\n>> +\t\t\tret = check_label_or_ref_arg(item->command, bol);\n>> +\t\t\t*eol = saved;\n>> +\t\t}\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tif (item->command == TODO_EXEC) {\n>>   \t\titem->commit = NULL;\n>>   \t\titem->arg_offset = bol - buf;\n>>   \t\titem->arg_len = (int)(eol - bol);\n> \n> (What's missing from this context is \"return 0;\")\n> \n> Is there an important reason why you separated TODO_EXEC and\n> its identical item->arg_(offset|len) parsing into its own\n> block?\n\nIn a word no! This patch was pulled out from the series I mentioned \nabove that adds support for comments. There it makes sense to treat exec \ncommands separately as they cannot have comments but you're right that \nthere is no need to do that in this patch.\n\n> It seems like we could modify your change to look like\n> this:\n> \n> \n> \tif (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n>    \t    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {\n> \t\tint ret = 0;\n> \n> \t\titem->commit = NULL;\n> \t\titem->arg_offset = bol - buf;\n> \t\titem->arg_len = (int)(eol - bol);\n> \t\tif (item->command == TODO_RESET ||\n> \t\t    item->command == TODO_UPDATE_REF) {\n> \t\t\tsaved = *eol;\n> \t\t\t*eol = '\\0';\n> \t\t\tret = check_label_or_ref_arg(item->command, bol);\n> \t\t\t*eol = saved;\n> \t\t}\n> \t\treturn ret;\n> \t}\n> \n> and the diff will have fewer new lines as well as fewer\n> duplicate lines in the post-image. Am I missing something\n> about TODO_EXEC being special?\n\nI'll update as you suggest\n\n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> index 462cefd25df..2cf2d2b8a24 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -2120,7 +2120,30 @@ test_expect_success '--update-refs: check failed ref update' '\n>>   \ttail -n 6 err >err.last &&\n>>   \tsed -e \"s/Rebasing.*Successfully/Successfully/g\" -e \"s/^\\t//g\" \\\n>>   \t\t<err.last >err.trimmed &&\n>> -\ttest_cmp expect err.trimmed\n>> +\ttest_cmp expect err.trimmed &&\n>> +\tgit rebase --abort\n>> +'\n> \n> Perhaps this `git rebase --abort` should be part of a\n> `test_when_finished test_may_fail git rebase --abort` at\n> the start of the test so that your new test can succeed\n> even if an earlier test step caused the test to fail.\n\nThat's a good idea (and for the next test as well)\n\nThanks for you're thoughtful comments and suggestions\n\nBest Wishes\n\nPhillip\n\n>> +test_expect_success 'bad labels and refs rejected when parsing todo list' '\n>> +\tcat >todo <<-\\EOF &&\n>> +\texec >execed\n>> +\tlabel #\n>> +\tlabel :invalid\n>> +\tupdate-ref :bad\n>> +\tupdate-ref topic\n>> +\tEOF\n>> +\trm -f execed &&\n>> +\t(\n>> +\t\tset_replace_editor todo &&\n>> +\t\ttest_must_fail git rebase -i HEAD 2>err\n>> +\t) &&\n>> +\tgrep \"'\\''#'\\'' is not a valid label\" err &&\n>> +\tgrep \"'\\'':invalid'\\'' is not a valid label\" err &&\n>> +\tgrep \"'\\'':bad'\\'' is not a valid refname\" err &&\n>> +\tgrep \"update-ref requires a fully qualified refname e.g. refs/heads/topic\" \\\n>> +\t\terr &&\n>> +\ttest_path_is_missing execed &&\n>> +\tgit rebase --abort\n>>   '\n> \n> Again, the `git rebase --abort` seems like protection for\n> future tests, so a test_when_finished would help.\n> \n> Thanks,\n> -Stolee\n"},{"id":"472330","messageId":"7a39050a-c857-a536-9637-04189c029eec@dunelm.org.uk","threadId":"59260","inReplyTo":"5a2a62cc-134c-adb0-7cab-c08c16c2dfdf@dunelm.org.uk","subject":"Re: [PATCH] rebase -i: check labels and refs when parsing todo list","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-02-20T10:03:11Z","receivedAt":"2023-02-20T10:04:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 20/02/2023 09:24, Phillip Wood wrote:\n> Thanks for you're thoughtful comments and suggestions\n\nHmm, perhaps I'm not quite as awake as I thought I was. I meant \"your \nthoughtful comments\" of course.\n\nBest Wishes\n\nPhillip\n"},{"id":"472335","messageId":"pull.1482.v2.git.1676902774366.gitgitgadget@gmail.com","threadId":"59260","inReplyTo":"pull.1482.git.1676644675638.gitgitgadget@gmail.com","subject":"[PATCH v2] rebase -i: check labels and refs when parsing todo list","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-20T14:19:34Z","receivedAt":"2023-02-20T14:19:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nCheck that the argument to the \"label\" and \"update-ref\" commands is a\nvalid refname when the todo list is parsed rather than waiting until the\ncommand is executed. This means that the user can deal with any errors\nat the beginning of the rebase rather than having it stop halfway\nthrough due to a typo in a label name. The \"update-ref\" command is\nchanged to reject single level refs as it is all to easy to type\n\"update-ref branch\" which is incorrect rather than \"update-ref\nrefs/heads/branch\"\n\nNote that it is not straight forward to check the arguments to \"reset\"\nand \"merge\" commands as they may be any revision, not just a refname and\nwe do not have an equivalent of check_refname_format() for revisions.\n\nHelped-by: Derrick Stolee <derrickstolee@github.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    rebase -i: check labels and refs when parsing todo list\n    \n    Hopefully detecting these errors when the user edits the todo list will\n    give a better experience.\n    \n    Thanks to Stolee for his comments on V1, I've updated the patch based on\n    those, there are no functional changes but the code is hopefully\n    clearer.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1482%2Fphillipwood%2Frebase-check-labels-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1482/phillipwood/rebase-check-labels-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1482\n\nRange-diff vs v1:\n\n 1:  5a31d880064 ! 1:  2cc62fe5986 rebase -i: check labels and refs when parsing todo list\n     @@ Commit message\n          and \"merge\" commands as they may be any revision, not just a refname and\n          we do not have an equivalent of check_refname_format() for revisions.\n      \n     +    Helped-by: Derrick Stolee <derrickstolee@github.com>\n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n      \n       ## sequencer.c ##\n     @@ sequencer.c: static int is_command(enum todo_command command, const char **bol)\n       \n      +static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n      +{\n     -+\tint allow_onelevel =\n     -+\t\tcommand == TODO_LABEL ? REFNAME_ALLOW_ONELEVEL : 0;\n     ++\tswitch (command) {\n     ++\tcase TODO_LABEL:\n     ++\t  /*\n     ++\t   * '#' is not a valid label as the merge command uses it to\n     ++\t   * separate merge parents from the commit subject.\n     ++\t   */\n     ++\t  if (!strcmp(arg, \"#\") ||\n     ++\t\t    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n     ++\t\t\treturn error(_(\"'%s' is not a valid label\"), arg);\n     ++\t\tbreak;\n      +\n     -+\tif ((command == TODO_LABEL && !strcmp(arg, \"#\")) ||\n     -+\t    check_refname_format(arg, allow_onelevel)) {\n     -+\t\tif (command == TODO_LABEL)\n     -+\t\t\terror(_(\"'%s' is not a valid label\"), arg);\n     -+\t\telse if (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n     -+\t\t\terror(_(\"'%s' is not a valid refname\"), arg);\n     -+\t\telse\n     -+\t\t\terror(_(\"update-ref requires a fully qualified refname e.g. refs/heads/%s\"),\n     -+\t\t\t      arg);\n     -+\t\treturn -1;\n     ++\tcase TODO_UPDATE_REF:\n     ++\t\tif (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n     ++\t\t\treturn error(_(\"'%s' is not a valid refname\"), arg);\n     ++\t\tif (check_refname_format(arg, 0))\n     ++\t\t\treturn error(_(\"update-ref requires a fully qualified \"\n     ++\t\t\t\t       \"refname e.g. refs/heads/%s\"), arg);\n     ++\t\tbreak;\n     ++\n     ++\tdefault:\n     ++\t\tBUG(\"unexpected todo_command\");\n      +\t}\n      +\n      +\treturn 0;\n     @@ sequencer.c: static int is_command(enum todo_command command, const char **bol)\n       \t\t\t   const char *buf, const char *bol, char *eol)\n       {\n      @@ sequencer.c: static int parse_insn_line(struct repository *r, struct todo_item *item,\n     - \t\treturn error(_(\"missing arguments for %s\"),\n     - \t\t\t     command_to_string(item->command));\n       \n     --\tif (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n     -+\tif (item->command == TODO_LABEL ||\n     + \tif (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n       \t    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {\n      +\t\tint ret = 0;\n      +\n     -+\t\titem->commit = NULL;\n     -+\t\titem->arg_offset = bol - buf;\n     -+\t\titem->arg_len = (int)(eol - bol);\n     -+\t\tif (item->command != TODO_RESET) {\n     + \t\titem->commit = NULL;\n     + \t\titem->arg_offset = bol - buf;\n     + \t\titem->arg_len = (int)(eol - bol);\n     +-\t\treturn 0;\n     ++\t\tif (item->command == TODO_LABEL ||\n     ++\t\t    item->command == TODO_UPDATE_REF) {\n      +\t\t\tsaved = *eol;\n      +\t\t\t*eol = '\\0';\n      +\t\t\tret = check_label_or_ref_arg(item->command, bol);\n      +\t\t\t*eol = saved;\n      +\t\t}\n      +\t\treturn ret;\n     -+\t}\n     -+\n     -+\tif (item->command == TODO_EXEC) {\n     - \t\titem->commit = NULL;\n     - \t\titem->arg_offset = bol - buf;\n     - \t\titem->arg_len = (int)(eol - bol);\n     + \t}\n     + \n     + \tif (item->command == TODO_FIXUP) {\n      \n       ## t/t3404-rebase-interactive.sh ##\n     +@@ t/t3404-rebase-interactive.sh: test_expect_success '--update-refs: --edit-todo with no update-ref lines' '\n     + '\n     + \n     + test_expect_success '--update-refs: check failed ref update' '\n     ++\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n     + \tgit checkout -B update-refs-error no-conflict-branch &&\n     + \tgit branch -f base HEAD~4 &&\n     + \tgit branch -f first HEAD~3 &&\n      @@ t/t3404-rebase-interactive.sh: test_expect_success '--update-refs: check failed ref update' '\n     - \ttail -n 6 err >err.last &&\n     - \tsed -e \"s/Rebasing.*Successfully/Successfully/g\" -e \"s/^\\t//g\" \\\n     - \t\t<err.last >err.trimmed &&\n     --\ttest_cmp expect err.trimmed\n     -+\ttest_cmp expect err.trimmed &&\n     -+\tgit rebase --abort\n     -+'\n     -+\n     + \ttest_cmp expect err.trimmed\n     + '\n     + \n      +test_expect_success 'bad labels and refs rejected when parsing todo list' '\n     ++\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n      +\tcat >todo <<-\\EOF &&\n      +\texec >execed\n      +\tlabel #\n     @@ t/t3404-rebase-interactive.sh: test_expect_success '--update-refs: check failed\n      +\tgrep \"'\\'':bad'\\'' is not a valid refname\" err &&\n      +\tgrep \"update-ref requires a fully qualified refname e.g. refs/heads/topic\" \\\n      +\t\terr &&\n     -+\ttest_path_is_missing execed &&\n     -+\tgit rebase --abort\n     - '\n     - \n     ++\ttest_path_is_missing execed\n     ++'\n     ++\n       # This must be the last test in this file\n     + test_expect_success '$EDITOR and friends are unchanged' '\n     + \ttest_editor_unchanged\n\n\n sequencer.c                   | 39 ++++++++++++++++++++++++++++++++++-\n t/t3404-rebase-interactive.sh | 23 +++++++++++++++++++++\n 2 files changed, 61 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3e4a1972897..33bdc8bca43 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2477,6 +2477,34 @@ static int is_command(enum todo_command command, const char **bol)\n \t\t (*bol = p));\n }\n \n+static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n+{\n+\tswitch (command) {\n+\tcase TODO_LABEL:\n+\t  /*\n+\t   * '#' is not a valid label as the merge command uses it to\n+\t   * separate merge parents from the commit subject.\n+\t   */\n+\t  if (!strcmp(arg, \"#\") ||\n+\t\t    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n+\t\t\treturn error(_(\"'%s' is not a valid label\"), arg);\n+\t\tbreak;\n+\n+\tcase TODO_UPDATE_REF:\n+\t\tif (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n+\t\t\treturn error(_(\"'%s' is not a valid refname\"), arg);\n+\t\tif (check_refname_format(arg, 0))\n+\t\t\treturn error(_(\"update-ref requires a fully qualified \"\n+\t\t\t\t       \"refname e.g. refs/heads/%s\"), arg);\n+\t\tbreak;\n+\n+\tdefault:\n+\t\tBUG(\"unexpected todo_command\");\n+\t}\n+\n+\treturn 0;\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@@ -2525,10 +2553,19 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \n \tif (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n \t    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {\n+\t\tint ret = 0;\n+\n \t\titem->commit = NULL;\n \t\titem->arg_offset = bol - buf;\n \t\titem->arg_len = (int)(eol - bol);\n-\t\treturn 0;\n+\t\tif (item->command == TODO_LABEL ||\n+\t\t    item->command == TODO_UPDATE_REF) {\n+\t\t\tsaved = *eol;\n+\t\t\t*eol = '\\0';\n+\t\t\tret = check_label_or_ref_arg(item->command, bol);\n+\t\t\t*eol = saved;\n+\t\t}\n+\t\treturn ret;\n \t}\n \n \tif (item->command == TODO_FIXUP) {\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 462cefd25df..efeb74ad50a 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -2072,6 +2072,7 @@ test_expect_success '--update-refs: --edit-todo with no update-ref lines' '\n '\n \n test_expect_success '--update-refs: check failed ref update' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tgit checkout -B update-refs-error no-conflict-branch &&\n \tgit branch -f base HEAD~4 &&\n \tgit branch -f first HEAD~3 &&\n@@ -2123,6 +2124,28 @@ test_expect_success '--update-refs: check failed ref update' '\n \ttest_cmp expect err.trimmed\n '\n \n+test_expect_success 'bad labels and refs rejected when parsing todo list' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tcat >todo <<-\\EOF &&\n+\texec >execed\n+\tlabel #\n+\tlabel :invalid\n+\tupdate-ref :bad\n+\tupdate-ref topic\n+\tEOF\n+\trm -f execed &&\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\ttest_must_fail git rebase -i HEAD 2>err\n+\t) &&\n+\tgrep \"'\\''#'\\'' is not a valid label\" err &&\n+\tgrep \"'\\'':invalid'\\'' is not a valid label\" err &&\n+\tgrep \"'\\'':bad'\\'' is not a valid refname\" err &&\n+\tgrep \"update-ref requires a fully qualified refname e.g. refs/heads/topic\" \\\n+\t\terr &&\n+\ttest_path_is_missing execed\n+'\n+\n # This must be the last test in this file\n test_expect_success '$EDITOR and friends are unchanged' '\n \ttest_editor_unchanged\n\nbase-commit: b1485644f936ee83a995ec24d23f713f4230a1ae\n-- \ngitgitgadget\n"},{"id":"472375","messageId":"f99d7043-7757-2e19-d1e1-4ee9b887af1f@github.com","threadId":"59260","inReplyTo":"pull.1482.v2.git.1676902774366.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] rebase -i: check labels and refs when parsing todo list","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-02-21T14:24:53Z","receivedAt":"2023-02-21T14:25:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/20/2023 9:19 AM, Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\n> +static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n> +{\n> +\tswitch (command) {\n> +\tcase TODO_LABEL:\n> +\t  /*\n> +\t   * '#' is not a valid label as the merge command uses it to\n> +\t   * separate merge parents from the commit subject.\n> +\t   */\n> +\t  if (!strcmp(arg, \"#\") ||\n> +\t\t    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n\nTabbing is strange here. Within the case there seems to be \"\\t  \" to\nthe left of each line. Then the conditional has strange spacing. I\nthink it should be:\n\n\t\tif (!strcmp(arg, \"#\") ||\n\t\t    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n\n(The \"check_refname_format()\" line is correct in your patch, but the\nlines above it are not, for some reason.)\n\nThe rest of the switch statement is correctly tabbed.\n\n> @@ -2525,10 +2553,19 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n>  \n>  \tif (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n>  \t    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {\n> +\t\tint ret = 0;\n> +\n>  \t\titem->commit = NULL;\n>  \t\titem->arg_offset = bol - buf;\n>  \t\titem->arg_len = (int)(eol - bol);\n> -\t\treturn 0;\n> +\t\tif (item->command == TODO_LABEL ||\n> +\t\t    item->command == TODO_UPDATE_REF) {\n> +\t\t\tsaved = *eol;\n> +\t\t\t*eol = '\\0';\n> +\t\t\tret = check_label_or_ref_arg(item->command, bol);\n> +\t\t\t*eol = saved;\n> +\t\t}\n> +\t\treturn ret;\n>  \t}\n\nThis diff is much simpler to understand for the purpose of this\npatch. I saw your comment about splitting out TODO_EXEC for a\nfuture change, and that would be fine when it happens, too.\n\nThanks for the updates. Outside of that strange whitespace issue,\nthis patch LGTM.\n\nThanks,\n-Stolee\n"},{"id":"472379","messageId":"09ba35c3-1434-ac23-7ac0-1295dd8aca60@dunelm.org.uk","threadId":"59260","inReplyTo":"f99d7043-7757-2e19-d1e1-4ee9b887af1f@github.com","subject":"Re: [PATCH v2] rebase -i: check labels and refs when parsing todo list","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-02-21T16:29:59Z","receivedAt":"2023-02-21T16:30:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 21/02/2023 14:24, Derrick Stolee wrote:\n> On 2/20/2023 9:19 AM, Phillip Wood via GitGitGadget wrote:\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n>> +static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n>> +{\n>> +\tswitch (command) {\n>> +\tcase TODO_LABEL:\n>> +\t  /*\n>> +\t   * '#' is not a valid label as the merge command uses it to\n>> +\t   * separate merge parents from the commit subject.\n>> +\t   */\n>> +\t  if (!strcmp(arg, \"#\") ||\n>> +\t\t    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n> \n> Tabbing is strange here. Within the case there seems to be \"\\t  \" to\n> the left of each line. Then the conditional has strange spacing. I\n> think it should be:\n> \n> \t\tif (!strcmp(arg, \"#\") ||\n> \t\t    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n> \n> (The \"check_refname_format()\" line is correct in your patch, but the\n> lines above it are not, for some reason.)\n\nYes you're right, I'm not sure what happened there. I'll re-roll\n\nThanks\n\nPhillip\n\n> The rest of the switch statement is correctly tabbed.\n> \n>> @@ -2525,10 +2553,19 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n>>   \n>>   \tif (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n>>   \t    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {\n>> +\t\tint ret = 0;\n>> +\n>>   \t\titem->commit = NULL;\n>>   \t\titem->arg_offset = bol - buf;\n>>   \t\titem->arg_len = (int)(eol - bol);\n>> -\t\treturn 0;\n>> +\t\tif (item->command == TODO_LABEL ||\n>> +\t\t    item->command == TODO_UPDATE_REF) {\n>> +\t\t\tsaved = *eol;\n>> +\t\t\t*eol = '\\0';\n>> +\t\t\tret = check_label_or_ref_arg(item->command, bol);\n>> +\t\t\t*eol = saved;\n>> +\t\t}\n>> +\t\treturn ret;\n>>   \t}\n> \n> This diff is much simpler to understand for the purpose of this\n> patch. I saw your comment about splitting out TODO_EXEC for a\n> future change, and that would be fine when it happens, too.\n> \n> Thanks for the updates. Outside of that strange whitespace issue,\n> this patch LGTM.\n> \n> Thanks,\n> -Stolee\n"},{"id":"472380","messageId":"pull.1482.v3.git.1676999137195.gitgitgadget@gmail.com","threadId":"59260","inReplyTo":"pull.1482.v2.git.1676902774366.gitgitgadget@gmail.com","subject":"[PATCH v3] rebase -i: check labels and refs when parsing todo list","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-02-21T17:05:37Z","receivedAt":"2023-02-21T17:05:51Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nCheck that the argument to the \"label\" and \"update-ref\" commands is a\nvalid refname when the todo list is parsed rather than waiting until the\ncommand is executed. This means that the user can deal with any errors\nat the beginning of the rebase rather than having it stop halfway\nthrough due to a typo in a label name. The \"update-ref\" command is\nchanged to reject single level refs as it is all to easy to type\n\"update-ref branch\" which is incorrect rather than \"update-ref\nrefs/heads/branch\"\n\nNote that it is not straight forward to check the arguments to \"reset\"\nand \"merge\" commands as they may be any revision, not just a refname and\nwe do not have an equivalent of check_refname_format() for revisions.\n\nHelped-by: Derrick Stolee <derrickstolee@github.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    rebase -i: check labels and refs when parsing todo list\n    \n    Hopefully detecting these errors when the user edits the todo list will\n    give a better experience.\n    \n    Changes from V2:\n    \n     * Fix some whitespace issues pointed out by Stolee.\n    \n    Thanks to Stolee for his comments on V1, I've updated the patch based on\n    those, there are no functional changes but the code is hopefully\n    clearer.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1482%2Fphillipwood%2Frebase-check-labels-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1482/phillipwood/rebase-check-labels-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1482\n\nRange-diff vs v2:\n\n 1:  2cc62fe5986 ! 1:  52a23db3cfe rebase -i: check labels and refs when parsing todo list\n     @@ sequencer.c: static int is_command(enum todo_command command, const char **bol)\n      +{\n      +\tswitch (command) {\n      +\tcase TODO_LABEL:\n     -+\t  /*\n     -+\t   * '#' is not a valid label as the merge command uses it to\n     -+\t   * separate merge parents from the commit subject.\n     -+\t   */\n     -+\t  if (!strcmp(arg, \"#\") ||\n     ++\t\t/*\n     ++\t\t * '#' is not a valid label as the merge command uses it to\n     ++\t\t * separate merge parents from the commit subject.\n     ++\t\t */\n     ++\t\tif (!strcmp(arg, \"#\") ||\n      +\t\t    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n      +\t\t\treturn error(_(\"'%s' is not a valid label\"), arg);\n      +\t\tbreak;\n\n\n sequencer.c                   | 39 ++++++++++++++++++++++++++++++++++-\n t/t3404-rebase-interactive.sh | 23 +++++++++++++++++++++\n 2 files changed, 61 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3e4a1972897..ff83a55bb36 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2477,6 +2477,34 @@ static int is_command(enum todo_command command, const char **bol)\n \t\t (*bol = p));\n }\n \n+static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n+{\n+\tswitch (command) {\n+\tcase TODO_LABEL:\n+\t\t/*\n+\t\t * '#' is not a valid label as the merge command uses it to\n+\t\t * separate merge parents from the commit subject.\n+\t\t */\n+\t\tif (!strcmp(arg, \"#\") ||\n+\t\t    check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n+\t\t\treturn error(_(\"'%s' is not a valid label\"), arg);\n+\t\tbreak;\n+\n+\tcase TODO_UPDATE_REF:\n+\t\tif (check_refname_format(arg, REFNAME_ALLOW_ONELEVEL))\n+\t\t\treturn error(_(\"'%s' is not a valid refname\"), arg);\n+\t\tif (check_refname_format(arg, 0))\n+\t\t\treturn error(_(\"update-ref requires a fully qualified \"\n+\t\t\t\t       \"refname e.g. refs/heads/%s\"), arg);\n+\t\tbreak;\n+\n+\tdefault:\n+\t\tBUG(\"unexpected todo_command\");\n+\t}\n+\n+\treturn 0;\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@@ -2525,10 +2553,19 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \n \tif (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n \t    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {\n+\t\tint ret = 0;\n+\n \t\titem->commit = NULL;\n \t\titem->arg_offset = bol - buf;\n \t\titem->arg_len = (int)(eol - bol);\n-\t\treturn 0;\n+\t\tif (item->command == TODO_LABEL ||\n+\t\t    item->command == TODO_UPDATE_REF) {\n+\t\t\tsaved = *eol;\n+\t\t\t*eol = '\\0';\n+\t\t\tret = check_label_or_ref_arg(item->command, bol);\n+\t\t\t*eol = saved;\n+\t\t}\n+\t\treturn ret;\n \t}\n \n \tif (item->command == TODO_FIXUP) {\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 462cefd25df..efeb74ad50a 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -2072,6 +2072,7 @@ test_expect_success '--update-refs: --edit-todo with no update-ref lines' '\n '\n \n test_expect_success '--update-refs: check failed ref update' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tgit checkout -B update-refs-error no-conflict-branch &&\n \tgit branch -f base HEAD~4 &&\n \tgit branch -f first HEAD~3 &&\n@@ -2123,6 +2124,28 @@ test_expect_success '--update-refs: check failed ref update' '\n \ttest_cmp expect err.trimmed\n '\n \n+test_expect_success 'bad labels and refs rejected when parsing todo list' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tcat >todo <<-\\EOF &&\n+\texec >execed\n+\tlabel #\n+\tlabel :invalid\n+\tupdate-ref :bad\n+\tupdate-ref topic\n+\tEOF\n+\trm -f execed &&\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\ttest_must_fail git rebase -i HEAD 2>err\n+\t) &&\n+\tgrep \"'\\''#'\\'' is not a valid label\" err &&\n+\tgrep \"'\\'':invalid'\\'' is not a valid label\" err &&\n+\tgrep \"'\\'':bad'\\'' is not a valid refname\" err &&\n+\tgrep \"update-ref requires a fully qualified refname e.g. refs/heads/topic\" \\\n+\t\terr &&\n+\ttest_path_is_missing execed\n+'\n+\n # This must be the last test in this file\n test_expect_success '$EDITOR and friends are unchanged' '\n \ttest_editor_unchanged\n\nbase-commit: b1485644f936ee83a995ec24d23f713f4230a1ae\n-- \ngitgitgadget\n"},{"id":"472381","messageId":"44bb24fb-ab5b-f4a2-f9ae-90439e9c33a8@github.com","threadId":"59260","inReplyTo":"pull.1482.v3.git.1676999137195.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] rebase -i: check labels and refs when parsing todo list","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-02-21T17:07:49Z","receivedAt":"2023-02-21T17:07:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/21/2023 12:05 PM, Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>     Changes from V2:\n>     \n>      * Fix some whitespace issues pointed out by Stolee.\n\nThanks for the quick update. LGTM.\n\n-Stolee\n"},{"id":"472384","messageId":"xmqqa617gcba.fsf@gitster.g","threadId":"59260","inReplyTo":"09ba35c3-1434-ac23-7ac0-1295dd8aca60@dunelm.org.uk","subject":"Re: [PATCH v2] rebase -i: check labels and refs when parsing todo list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-21T17:26:01Z","receivedAt":"2023-02-21T17:27:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n>> (The \"check_refname_format()\" line is correct in your patch, but the\n>> lines above it are not, for some reason.)\n>\n> Yes you're right, I'm not sure what happened there. I'll re-roll\n\nNah, thanks, both.  I've applied with reformatting plus Derrick's\nAcked-by.\n\n"}]}