{"thread":{"id":"60012","subject":"[PATCH] sequencer: finish parsing the todo list despite an invalid first line","startedAt":"2023-07-19T14:44:28Z","lastAt":"2023-07-24T20:08:59Z","messageCount":23,"participants":["Alex Henrie","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"479634","messageId":"20230719144339.447852-1-alexhenrie24@gmail.com","threadId":"60012","inReplyTo":null,"subject":"[PATCH] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-07-19T14:43:15Z","receivedAt":"2023-07-19T14:44:28Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"ddb81e5072 (rebase-interactive: use todo_list_write_to_file() in\nedit_todo_list(), 2019-03-05) made edit_todo_list more efficient by\nreplacing transform_todo_file with todo_list_parse_insn_buffer.\nUnfortunately, that innocuous change caused a regression because\ntodo_list_parse_insn_buffer would stop parsing after encountering an\ninvalid 'fixup' line. If the user accidentally made the first line a\n'fixup' and tried to recover from their mistake with `git rebase\n--edit-todo`, all of the commands after the first would be lost.\n\nTo avoid throwing away important parts of the todo list, change\ntodo_list_parse_insn_buffer to keep going and not return early on error.\n\nSigned-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\n sequencer.c                   |  2 +-\n t/t3404-rebase-interactive.sh | 19 +++++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cc9821ece2..adc9cfb4df 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2702,7 +2702,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n \t\tif (fixup_okay)\n \t\t\t; /* do nothing */\n \t\telse if (is_fixup(item->command))\n-\t\t\treturn error(_(\"cannot '%s' without a previous commit\"),\n+\t\t\tres = error(_(\"cannot '%s' without a previous commit\"),\n \t\t\t\tcommand_to_string(item->command));\n \t\telse if (!is_noop(item->command))\n \t\t\tfixup_okay = 1;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ff0afad63e..d2801ffee4 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1596,6 +1596,25 @@ test_expect_success 'static check of bad command' '\n \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_success 'the first command cannot be a fixup' '\n+\t# When using `git rebase --edit-todo` to recover from this error, ensure\n+\t# that none of the original todo list is lost\n+\trebase_setup_and_clean fixup-first &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"fixup 1 2 3 4 5\" \\\n+\t\t\t       git rebase -i --root 2>actual &&\n+\t\ttest_i18ngrep \"cannot .fixup. without a previous commit\" \\\n+\t\t\t\tactual &&\n+\t\ttest_i18ngrep \"You can fix this with .git rebase --edit-todo..\" \\\n+\t\t\t\tactual &&\n+\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >orig &&\n+\t\ttest_must_fail git rebase --edit-todo &&\n+\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n+\t\ttest_cmp orig actual\n+\t)\n+'\n+\n test_expect_success 'tabs and spaces are accepted in the todolist' '\n \trebase_setup_and_clean indented-comment &&\n \twrite_script add-indent.sh <<-\\EOF &&\n-- \n2.41.0\n\n"},{"id":"479659","messageId":"xmqq351ja8p8.fsf@gitster.g","threadId":"60012","inReplyTo":"20230719144339.447852-1-alexhenrie24@gmail.com","subject":"Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-19T21:32:35Z","receivedAt":"2023-07-19T21:32:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie <alexhenrie24@gmail.com> writes:\n\n> ddb81e5072 (rebase-interactive: use todo_list_write_to_file() in\n> edit_todo_list(), 2019-03-05) made edit_todo_list more efficient by\n> replacing transform_todo_file with todo_list_parse_insn_buffer.\n> Unfortunately, that innocuous change caused a regression because\n> todo_list_parse_insn_buffer would stop parsing after encountering an\n> invalid 'fixup' line. If the user accidentally made the first line a\n> 'fixup' and tried to recover from their mistake with `git rebase\n> --edit-todo`, all of the commands after the first would be lost.\n>\n> To avoid throwing away important parts of the todo list, change\n> todo_list_parse_insn_buffer to keep going and not return early on error.\n>\n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n>  sequencer.c                   |  2 +-\n>  t/t3404-rebase-interactive.sh | 19 +++++++++++++++++++\n>  2 files changed, 20 insertions(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index cc9821ece2..adc9cfb4df 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2702,7 +2702,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n>  \t\tif (fixup_okay)\n>  \t\t\t; /* do nothing */\n>  \t\telse if (is_fixup(item->command))\n> -\t\t\treturn error(_(\"cannot '%s' without a previous commit\"),\n> +\t\t\tres = error(_(\"cannot '%s' without a previous commit\"),\n>  \t\t\t\tcommand_to_string(item->command));\n>  \t\telse if (!is_noop(item->command))\n>  \t\t\tfixup_okay = 1;\n\nWell spotted.\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index ff0afad63e..d2801ffee4 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1596,6 +1596,25 @@ test_expect_success 'static check of bad command' '\n>  \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n>  '\n>  \n> +test_expect_success 'the first command cannot be a fixup' '\n\nA very good test to the point.\n\n> +\t# When using `git rebase --edit-todo` to recover from this error, ensure\n> +\t# that none of the original todo list is lost\n> +\trebase_setup_and_clean fixup-first &&\n> +\t(\n> +\t\tset_fake_editor &&\n> +\t\ttest_must_fail env FAKE_LINES=\"fixup 1 2 3 4 5\" \\\n> +\t\t\t       git rebase -i --root 2>actual &&\n> +\t\ttest_i18ngrep \"cannot .fixup. without a previous commit\" \\\n> +\t\t\t\tactual &&\n> +\t\ttest_i18ngrep \"You can fix this with .git rebase --edit-todo..\" \\\n> +\t\t\t\tactual &&\n\nThese days, we do not add new uses of test_i18n_grep; just replacing\nit with \"grep\" would be good enough, so I'll touch them up locally.\n\n> +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >orig &&\n> +\t\ttest_must_fail git rebase --edit-todo &&\n> +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n\nMakes me wonder if \"grep -v\" is too loose (i.e. are there good\nreasons to expect/allow that comments would be different and will\nnot compare well if they are left in?) and is too tight (i.e. can\nthe rebase machinery when rewriting the todo file reformat the\ncontents on the non-comment lines in such a way that they do not\ncompare byte-for-byte identical?).  But we'll find out if its the\nlatter (and we do not care too much if future changes to the command\nwill start clobbering the comment lines).\n\nWill queue with minimum fixups.\n\nThanks.\n\n> +\t\ttest_cmp orig actual\n> +\t)\n> +'\n> +\n>  test_expect_success 'tabs and spaces are accepted in the todolist' '\n>  \trebase_setup_and_clean indented-comment &&\n>  \twrite_script add-indent.sh <<-\\EOF &&\n"},{"id":"479669","messageId":"395274b4-37a9-8c95-203f-94178c99772a@gmail.com","threadId":"60012","inReplyTo":"20230719144339.447852-1-alexhenrie24@gmail.com","subject":"Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-20T09:42:31Z","receivedAt":"2023-07-20T09:46:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nThanks for working on this.\n\nOn 19/07/2023 15:43, Alex Henrie wrote:\n> ddb81e5072 (rebase-interactive: use todo_list_write_to_file() in\n> edit_todo_list(), 2019-03-05) made edit_todo_list more efficient by\n> replacing transform_todo_file with todo_list_parse_insn_buffer.\n> Unfortunately, that innocuous change caused a regression because\n> todo_list_parse_insn_buffer would stop parsing after encountering an\n> invalid 'fixup' line. If the user accidentally made the first line a\n> 'fixup' and tried to recover from their mistake with `git rebase\n> --edit-todo`, all of the commands after the first would be lost.\n\nI found this description a little confusing as transform_todo_file() \nalso called todo_list_parse_insn_buffer(). transform_todo_file() does \nnot exist anymore but it looked like\n\nstatic int transform_todo_file(unsigned flags)\n{\n         const char *todo_file = rebase_path_todo();\n         struct todo_list todo_list = TODO_LIST_INIT;\n         int res;\n\n         if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)\n                 return error_errno(_(\"could not read '%s'.\"), todo_file);\n\n         if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n                                         &todo_list)) {\n                 todo_list_release(&todo_list);\n                 return error(_(\"unusable todo list: '%s'\"), todo_file);\n         }\n\n         res = todo_list_write_to_file(the_repository, &todo_list, \ntodo_file,\n                                       NULL, NULL, -1, flags);\n         todo_list_release(&todo_list);\n\n         if (res)\n                 return error_errno(_(\"could not write '%s'.\"), todo_file);\n         return 0;\n}\n\nIf it could not parse the todo list it did not try and write it to disc. \nAfter ddb81e5072 this changed as edit_todo_list() tries to shorten the \nOIDs in the todo list before it is edited even if it cannot be parsed. \nThe fix below works around that by making sure we always try and parse \nthe whole todo list even if the the first line is a fixup command. That \nis a worthwhile improvement because it means we notify the user of all \nthe errors we find rather than just the first one and is in keeping with \nthe way we handle other invalid lines. It does not however fix the root \ncause of this regression which is the change in behavior in \nedit_todo_list().\n\nAfter the user edits the todo file we do not try to transform the OIDs \nif it cannot be parsed or has missing commits. Therefore it still \ncontains the shortened OIDs and editing hints and there is no need for \nedit_todo_list() to call write_todo_list() when \"incorrect\" is true.\n\n\n> To avoid throwing away important parts of the todo list, change\n> todo_list_parse_insn_buffer to keep going and not return early on error.\n> \n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n>   sequencer.c                   |  2 +-\n>   t/t3404-rebase-interactive.sh | 19 +++++++++++++++++++\n>   2 files changed, 20 insertions(+), 1 deletion(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index cc9821ece2..adc9cfb4df 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2702,7 +2702,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n>   \t\tif (fixup_okay)\n>   \t\t\t; /* do nothing */\n>   \t\telse if (is_fixup(item->command))\n> -\t\t\treturn error(_(\"cannot '%s' without a previous commit\"),\n> +\t\t\tres = error(_(\"cannot '%s' without a previous commit\"),\n>   \t\t\t\tcommand_to_string(item->command));\n>   \t\telse if (!is_noop(item->command))\n>   \t\t\tfixup_okay = 1;\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index ff0afad63e..d2801ffee4 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1596,6 +1596,25 @@ test_expect_success 'static check of bad command' '\n>   \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n>   '\n>   \n> +test_expect_success 'the first command cannot be a fixup' '\n> +\t# When using `git rebase --edit-todo` to recover from this error, ensure\n> +\t# that none of the original todo list is lost\n> +\trebase_setup_and_clean fixup-first &&\n> +\t(\n> +\t\tset_fake_editor &&\n> +\t\ttest_must_fail env FAKE_LINES=\"fixup 1 2 3 4 5\" \\\n> +\t\t\t       git rebase -i --root 2>actual &&\n\nThanks for taking the time to add a test. It is not worth a re-roll on \nits own, but there is no need to use \"--root\" here. It is confusing as \nit is not clear if we're refusing \"fixup\" as the first command because \nwe're rewriting the root commit or if we always refuse to have \"fixup\" \nas the first command.\n\nAs an aside this restriction is pretty easy to defeat. In fact I think \nwe probably allow a todo list that starts with\n\n[new root]\nfixup <commit>\n\nwhich is a bug. We certainly allow todo lists starting with\n\nexec true / label <label> / reset <commit>\nfixup <commit>\n\nbut that is not the concern of this patch.\n\n> +\t\ttest_i18ngrep \"cannot .fixup. without a previous commit\" \\\n> +\t\t\t\tactual &&\n> +\t\ttest_i18ngrep \"You can fix this with .git rebase --edit-todo..\" \\\n> +\t\t\t\tactual &&\n> +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >orig &&\n> +\t\ttest_must_fail git rebase --edit-todo &&\n> +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n> +\t\ttest_cmp orig actual\n\nWe check that the uncommitted lines after running \"git rebase \n--edit-todo\" match the uncommitted lines after the initial edit. That's \nfine to detect if the second edit truncates the file but it will still \npass if the initial edit starts truncating the todo list as well. As we \nexpect that git should not change an incorrect todo list we do not need \nto filter out the lines beginning with \"#\".\n\nTo ensure we detect a regression where the first edit truncates the todo \nlist we could do something like\n\n\ttest_when_finished \"git rebase --abort\" &&\n\tcat >todo <<-EOF &&\n\tfixup $(git log -1 --format=\"%h %s\" B)\n\tpick $(git log -1 --format=\"%h %s\" C)\n\tEOF\n\n\t(\n\t\tset_replace_editor todo &&\n\t\ttest_must_fail git rebase -i A 2>actual\n\t) &&\n\ttest_i18ngrep \"cannot .fixup. without a previous commit\" actual &&\n\ttest_i18ngrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n\t# check initial edit has not truncated todo list\n\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n\ttest_cmp todo actual &&\n\tcat .git/rebase-merge/git-rebase-todo >expect &&\n\ttest_must_fail git rebase --edit-todo &&\n\t# check the list is unchanged by --edit-todo\n\ttest_cmp expect .git/rebase-merge/git-rebase-todo\n\nWe could perhaps check the error message from \"git rebase --edit-todo\" \nas well.\n\nThanks for finding this and working on a fix\n\nPhillip\n\n> +\t)\n> +'\n> +\n>   test_expect_success 'tabs and spaces are accepted in the todolist' '\n>   \trebase_setup_and_clean indented-comment &&\n>   \twrite_script add-indent.sh <<-\\EOF &&\n"},{"id":"479698","messageId":"CAMMLpeSN_M1HW1D3HyuY+S+GwUrQ_4dP9qoSQ72hbQv3pwK5kg@mail.gmail.com","threadId":"60012","inReplyTo":"395274b4-37a9-8c95-203f-94178c99772a@gmail.com","subject":"Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-07-20T22:37:00Z","receivedAt":"2023-07-20T22:37:51Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Thu, Jul 20, 2023 at 3:42 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n\n> On 19/07/2023 15:43, Alex Henrie wrote:\n> > ddb81e5072 (rebase-interactive: use todo_list_write_to_file() in\n> > edit_todo_list(), 2019-03-05) made edit_todo_list more efficient by\n> > replacing transform_todo_file with todo_list_parse_insn_buffer.\n> > Unfortunately, that innocuous change caused a regression because\n> > todo_list_parse_insn_buffer would stop parsing after encountering an\n> > invalid 'fixup' line. If the user accidentally made the first line a\n> > 'fixup' and tried to recover from their mistake with `git rebase\n> > --edit-todo`, all of the commands after the first would be lost.\n>\n> I found this description a little confusing as transform_todo_file()\n> also called todo_list_parse_insn_buffer(). transform_todo_file() does\n> not exist anymore but it looked like\n>\n> static int transform_todo_file(unsigned flags)\n> {\n>          const char *todo_file = rebase_path_todo();\n>          struct todo_list todo_list = TODO_LIST_INIT;\n>          int res;\n>\n>          if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)\n>                  return error_errno(_(\"could not read '%s'.\"), todo_file);\n>\n>          if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n>                                          &todo_list)) {\n>                  todo_list_release(&todo_list);\n>                  return error(_(\"unusable todo list: '%s'\"), todo_file);\n>          }\n>\n>          res = todo_list_write_to_file(the_repository, &todo_list,\n> todo_file,\n>                                        NULL, NULL, -1, flags);\n>          todo_list_release(&todo_list);\n>\n>          if (res)\n>                  return error_errno(_(\"could not write '%s'.\"), todo_file);\n>          return 0;\n> }\n>\n> If it could not parse the todo list it did not try and write it to disc.\n> After ddb81e5072 this changed as edit_todo_list() tries to shorten the\n> OIDs in the todo list before it is edited even if it cannot be parsed.\n> The fix below works around that by making sure we always try and parse\n> the whole todo list even if the the first line is a fixup command. That\n> is a worthwhile improvement because it means we notify the user of all\n> the errors we find rather than just the first one and is in keeping with\n> the way we handle other invalid lines. It does not however fix the root\n> cause of this regression which is the change in behavior in\n> edit_todo_list().\n>\n> After the user edits the todo file we do not try to transform the OIDs\n> if it cannot be parsed or has missing commits. Therefore it still\n> contains the shortened OIDs and editing hints and there is no need for\n> edit_todo_list() to call write_todo_list() when \"incorrect\" is true.\n\nWhen the user first runs `git rebase`, the commit template contains\nthe following message:\n\n# However, if you remove everything, the rebase will be aborted.\n\nWhen running `git rebase --edit-todo`, that message is replaced with:\n\n# You are editing the todo file of an ongoing interactive rebase.\n# To continue rebase after editing, run:\n#     git rebase --continue\n\nThe second message is indeed more accurate after the rebase has\nstarted: Deleting all of the lines in `git rebase --edit-todo` drops\nall of the commits; it does not abort the rebase.\n\nIt would be nice to preserve as much of the user's original input as\npossible, but that's not a project that I'm going to tackle. As far as\na minimal fix for the regression, we can either leave the todo file\nuntouched and display inaccurate advice during `git rebase\n--edit-todo`, or we can lose any long commit IDs that the user entered\nand display equivalent short hex IDs instead. I would prefer the\nlatter.\n\n> > +test_expect_success 'the first command cannot be a fixup' '\n> > +     # When using `git rebase --edit-todo` to recover from this error, ensure\n> > +     # that none of the original todo list is lost\n> > +     rebase_setup_and_clean fixup-first &&\n> > +     (\n> > +             set_fake_editor &&\n> > +             test_must_fail env FAKE_LINES=\"fixup 1 2 3 4 5\" \\\n> > +                            git rebase -i --root 2>actual &&\n>\n> Thanks for taking the time to add a test. It is not worth a re-roll on\n> its own, but there is no need to use \"--root\" here. It is confusing as\n> it is not clear if we're refusing \"fixup\" as the first command because\n> we're rewriting the root commit or if we always refuse to have \"fixup\"\n> as the first command.\n\nGood point. I used --root because I copied and pasted from the\npreceding test, but HEAD~4 would make the intent of the test more\nclear. That change and the grep change that Junio suggested are\nprobably worth a v2.\n\n> > +             test_i18ngrep \"cannot .fixup. without a previous commit\" \\\n> > +                             actual &&\n> > +             test_i18ngrep \"You can fix this with .git rebase --edit-todo..\" \\\n> > +                             actual &&\n> > +             grep -v \"^#\" .git/rebase-merge/git-rebase-todo >orig &&\n> > +             test_must_fail git rebase --edit-todo &&\n> > +             grep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n> > +             test_cmp orig actual\n>\n> We check that the uncommitted lines after running \"git rebase\n> --edit-todo\" match the uncommitted lines after the initial edit. That's\n> fine to detect if the second edit truncates the file but it will still\n> pass if the initial edit starts truncating the todo list as well. As we\n> expect that git should not change an incorrect todo list we do not need\n> to filter out the lines beginning with \"#\".\n>\n> To ensure we detect a regression where the first edit truncates the todo\n> list we could do something like\n>\n>         test_when_finished \"git rebase --abort\" &&\n>         cat >todo <<-EOF &&\n>         fixup $(git log -1 --format=\"%h %s\" B)\n>         pick $(git log -1 --format=\"%h %s\" C)\n>         EOF\n>\n>         (\n>                 set_replace_editor todo &&\n>                 test_must_fail git rebase -i A 2>actual\n>         ) &&\n>         test_i18ngrep \"cannot .fixup. without a previous commit\" actual &&\n>         test_i18ngrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n>         # check initial edit has not truncated todo list\n>         grep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n>         test_cmp todo actual &&\n>         cat .git/rebase-merge/git-rebase-todo >expect &&\n>         test_must_fail git rebase --edit-todo &&\n>         # check the list is unchanged by --edit-todo\n>         test_cmp expect .git/rebase-merge/git-rebase-todo\n\nTo me it seems pretty far-fetched that a future bug would cause the\n_initial_ commit template to be missing anything. But if you're\nconcerned about it, would you like to send a follow-up patch to revise\nthe test as you see fit?\n\n> We could perhaps check the error message from \"git rebase --edit-todo\"\n> as well.\n\nThat sounds like another good change for v2.\n\nThanks for the feedback,\n\n-Alex\n"},{"id":"479712","messageId":"20230721053906.14315-1-alexhenrie24@gmail.com","threadId":"60012","inReplyTo":"20230719144339.447852-1-alexhenrie24@gmail.com","subject":"[PATCH v2 0/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-07-21T05:38:56Z","receivedAt":"2023-07-21T05:53:06Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Changes from v1:\n- Use `grep` instead of `test_i18ngrep`\n- Check for an error message from --edit-todo\n- Move and rephrase the comment in the test\n\nThanks to Junio and Phillip for your feedback.\n\nAlex Henrie (1):\n  sequencer: finish parsing the todo list despite an invalid first line\n\n sequencer.c                   |  2 +-\n t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\nRange-diff against v1:\n1:  ceb53efb79 ! 1:  8005d81440 sequencer: finish parsing the todo list despite an invalid first line\n    @@ t/t3404-rebase-interactive.sh: test_expect_success 'static check of bad command'\n      '\n      \n     +test_expect_success 'the first command cannot be a fixup' '\n    -+\t# When using `git rebase --edit-todo` to recover from this error, ensure\n    -+\t# that none of the original todo list is lost\n     +\trebase_setup_and_clean fixup-first &&\n     +\t(\n     +\t\tset_fake_editor &&\n     +\t\ttest_must_fail env FAKE_LINES=\"fixup 1 2 3 4 5\" \\\n     +\t\t\t       git rebase -i --root 2>actual &&\n    -+\t\ttest_i18ngrep \"cannot .fixup. without a previous commit\" \\\n    -+\t\t\t\tactual &&\n    -+\t\ttest_i18ngrep \"You can fix this with .git rebase --edit-todo..\" \\\n    -+\t\t\t\tactual &&\n    ++\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n    ++\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n     +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >orig &&\n     +\t\ttest_must_fail git rebase --edit-todo &&\n    ++\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n    ++\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n     +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n    ++\t\t# check that --edit-todo did not lose any of the todo list\n     +\t\ttest_cmp orig actual\n     +\t)\n     +'\n-- \n2.41.0\n\n"},{"id":"479713","messageId":"20230721053906.14315-2-alexhenrie24@gmail.com","threadId":"60012","inReplyTo":"20230721053906.14315-1-alexhenrie24@gmail.com","subject":"[PATCH v2 1/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-07-21T05:38:57Z","receivedAt":"2023-07-21T05:53:08Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"ddb81e5072 (rebase-interactive: use todo_list_write_to_file() in\nedit_todo_list(), 2019-03-05) made edit_todo_list more efficient by\nreplacing transform_todo_file with todo_list_parse_insn_buffer.\nUnfortunately, that innocuous change caused a regression because\ntodo_list_parse_insn_buffer would stop parsing after encountering an\ninvalid 'fixup' line. If the user accidentally made the first line a\n'fixup' and tried to recover from their mistake with `git rebase\n--edit-todo`, all of the commands after the first would be lost.\n\nTo avoid throwing away important parts of the todo list, change\ntodo_list_parse_insn_buffer to keep going and not return early on error.\n\nSigned-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\n sequencer.c                   |  2 +-\n t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cc9821ece2..adc9cfb4df 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2702,7 +2702,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n \t\tif (fixup_okay)\n \t\t\t; /* do nothing */\n \t\telse if (is_fixup(item->command))\n-\t\t\treturn error(_(\"cannot '%s' without a previous commit\"),\n+\t\t\tres = error(_(\"cannot '%s' without a previous commit\"),\n \t\t\t\tcommand_to_string(item->command));\n \t\telse if (!is_noop(item->command))\n \t\t\tfixup_okay = 1;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ff0afad63e..d133cbae32 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1596,6 +1596,24 @@ test_expect_success 'static check of bad command' '\n \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_success 'the first command cannot be a fixup' '\n+\trebase_setup_and_clean fixup-first &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"fixup 1 2 3 4 5\" \\\n+\t\t\t       git rebase -i --root 2>actual &&\n+\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n+\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n+\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >orig &&\n+\t\ttest_must_fail git rebase --edit-todo &&\n+\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n+\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n+\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n+\t\t# check that --edit-todo did not lose any of the todo list\n+\t\ttest_cmp orig actual\n+\t)\n+'\n+\n test_expect_success 'tabs and spaces are accepted in the todolist' '\n \trebase_setup_and_clean indented-comment &&\n \twrite_script add-indent.sh <<-\\EOF &&\n-- \n2.41.0\n\n"},{"id":"479714","messageId":"20230721055841.28146-1-alexhenrie24@gmail.com","threadId":"60012","inReplyTo":"20230721053906.14315-1-alexhenrie24@gmail.com","subject":"[PATCH v3 0/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-07-21T05:58:16Z","receivedAt":"2023-07-21T05:59:35Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Changes from v2:\n- Include accidentally omitted file redirect so that the output of\n  --edit-todo is actually tested\n\nAlex Henrie (1):\n  sequencer: finish parsing the todo list despite an invalid first line\n\n sequencer.c                   |  2 +-\n t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\nRange-diff against v2:\n1:  8005d81440 ! 1:  b1af2df3f5 sequencer: finish parsing the todo list despite an invalid first line\n    @@ t/t3404-rebase-interactive.sh: test_expect_success 'static check of bad command'\n     +\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n     +\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n     +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >orig &&\n    -+\t\ttest_must_fail git rebase --edit-todo &&\n    ++\t\ttest_must_fail git rebase --edit-todo 2>actual &&\n     +\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n     +\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n     +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n-- \n2.41.0\n\n"},{"id":"479715","messageId":"20230721055841.28146-2-alexhenrie24@gmail.com","threadId":"60012","inReplyTo":"20230721055841.28146-1-alexhenrie24@gmail.com","subject":"[PATCH v3 1/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-07-21T05:58:17Z","receivedAt":"2023-07-21T05:59:37Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"ddb81e5072 (rebase-interactive: use todo_list_write_to_file() in\nedit_todo_list(), 2019-03-05) made edit_todo_list more efficient by\nreplacing transform_todo_file with todo_list_parse_insn_buffer.\nUnfortunately, that innocuous change caused a regression because\ntodo_list_parse_insn_buffer would stop parsing after encountering an\ninvalid 'fixup' line. If the user accidentally made the first line a\n'fixup' and tried to recover from their mistake with `git rebase\n--edit-todo`, all of the commands after the first would be lost.\n\nTo avoid throwing away important parts of the todo list, change\ntodo_list_parse_insn_buffer to keep going and not return early on error.\n\nSigned-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\n sequencer.c                   |  2 +-\n t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cc9821ece2..adc9cfb4df 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2702,7 +2702,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n \t\tif (fixup_okay)\n \t\t\t; /* do nothing */\n \t\telse if (is_fixup(item->command))\n-\t\t\treturn error(_(\"cannot '%s' without a previous commit\"),\n+\t\t\tres = error(_(\"cannot '%s' without a previous commit\"),\n \t\t\t\tcommand_to_string(item->command));\n \t\telse if (!is_noop(item->command))\n \t\t\tfixup_okay = 1;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ff0afad63e..fba89146cf 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1596,6 +1596,24 @@ test_expect_success 'static check of bad command' '\n \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_success 'the first command cannot be a fixup' '\n+\trebase_setup_and_clean fixup-first &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"fixup 1 2 3 4 5\" \\\n+\t\t\t       git rebase -i --root 2>actual &&\n+\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n+\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n+\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >orig &&\n+\t\ttest_must_fail git rebase --edit-todo 2>actual &&\n+\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n+\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n+\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n+\t\t# check that --edit-todo did not lose any of the todo list\n+\t\ttest_cmp orig actual\n+\t)\n+'\n+\n test_expect_success 'tabs and spaces are accepted in the todolist' '\n \trebase_setup_and_clean indented-comment &&\n \twrite_script add-indent.sh <<-\\EOF &&\n-- \n2.41.0\n\n"},{"id":"479716","messageId":"20230721060848.35641-2-alexhenrie24@gmail.com","threadId":"60012","inReplyTo":"20230721060848.35641-1-alexhenrie24@gmail.com","subject":"[PATCH v4 1/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-07-21T06:07:51Z","receivedAt":"2023-07-21T06:10:34Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"ddb81e5072 (rebase-interactive: use todo_list_write_to_file() in\nedit_todo_list(), 2019-03-05) made edit_todo_list more efficient by\nreplacing transform_todo_file with todo_list_parse_insn_buffer.\nUnfortunately, that innocuous change caused a regression because\ntodo_list_parse_insn_buffer would stop parsing after encountering an\ninvalid 'fixup' line. If the user accidentally made the first line a\n'fixup' and tried to recover from their mistake with `git rebase\n--edit-todo`, all of the commands after the first would be lost.\n\nTo avoid throwing away important parts of the todo list, change\ntodo_list_parse_insn_buffer to keep going and not return early on error.\n\nSigned-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\n sequencer.c                   |  2 +-\n t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cc9821ece2..adc9cfb4df 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2702,7 +2702,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n \t\tif (fixup_okay)\n \t\t\t; /* do nothing */\n \t\telse if (is_fixup(item->command))\n-\t\t\treturn error(_(\"cannot '%s' without a previous commit\"),\n+\t\t\tres = error(_(\"cannot '%s' without a previous commit\"),\n \t\t\t\tcommand_to_string(item->command));\n \t\telse if (!is_noop(item->command))\n \t\t\tfixup_okay = 1;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ff0afad63e..8ffd2a7318 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1596,6 +1596,24 @@ test_expect_success 'static check of bad command' '\n \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_success 'the first command cannot be a fixup' '\n+\trebase_setup_and_clean fixup-first &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"fixup 1 2 3 4\" \\\n+\t\t\t       git rebase -i HEAD~4 2>actual &&\n+\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n+\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n+\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >orig &&\n+\t\ttest_must_fail git rebase --edit-todo 2>actual &&\n+\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n+\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n+\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n+\t\t# check that --edit-todo did not lose any of the todo list\n+\t\ttest_cmp orig actual\n+\t)\n+'\n+\n test_expect_success 'tabs and spaces are accepted in the todolist' '\n \trebase_setup_and_clean indented-comment &&\n \twrite_script add-indent.sh <<-\\EOF &&\n-- \n2.41.0\n\n"},{"id":"479717","messageId":"20230721060848.35641-1-alexhenrie24@gmail.com","threadId":"60012","inReplyTo":"20230721055841.28146-1-alexhenrie24@gmail.com","subject":"[PATCH v4 0/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-07-21T06:07:50Z","receivedAt":"2023-07-21T06:10:39Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Changes from v3:\n- Rebase onto HEAD~4 instead of --root (which was the original motivation\n  for sending a new patch and I forgot to include that change; I probably\n  shouldn't be doing Git development late at night...)\n\nAlex Henrie (1):\n  sequencer: finish parsing the todo list despite an invalid first line\n\n sequencer.c                   |  2 +-\n t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\nRange-diff against v3:\n1:  b1af2df3f5 ! 1:  f6fcdcd9a9 sequencer: finish parsing the todo list despite an invalid first line\n    @@ t/t3404-rebase-interactive.sh: test_expect_success 'static check of bad command'\n     +\trebase_setup_and_clean fixup-first &&\n     +\t(\n     +\t\tset_fake_editor &&\n    -+\t\ttest_must_fail env FAKE_LINES=\"fixup 1 2 3 4 5\" \\\n    -+\t\t\t       git rebase -i --root 2>actual &&\n    ++\t\ttest_must_fail env FAKE_LINES=\"fixup 1 2 3 4\" \\\n    ++\t\t\t       git rebase -i HEAD~4 2>actual &&\n     +\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n     +\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n     +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >orig &&\n-- \n2.41.0\n\n"},{"id":"479719","messageId":"c7b7f078-6561-5081-9c23-0cec65b71c97@gmail.com","threadId":"60012","inReplyTo":"CAMMLpeSN_M1HW1D3HyuY+S+GwUrQ_4dP9qoSQ72hbQv3pwK5kg@mail.gmail.com","subject":"Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-21T09:31:56Z","receivedAt":"2023-07-21T09:32:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nOn 20/07/2023 23:37, Alex Henrie wrote:\n> On Thu, Jul 20, 2023 at 3:42 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n> \n>> On 19/07/2023 15:43, Alex Henrie wrote:\n>>> ddb81e5072 (rebase-interactive: use todo_list_write_to_file() in\n>>> edit_todo_list(), 2019-03-05) made edit_todo_list more efficient by\n>>> replacing transform_todo_file with todo_list_parse_insn_buffer.\n>>> Unfortunately, that innocuous change caused a regression because\n>>> todo_list_parse_insn_buffer would stop parsing after encountering an\n>>> invalid 'fixup' line. If the user accidentally made the first line a\n>>> 'fixup' and tried to recover from their mistake with `git rebase\n>>> --edit-todo`, all of the commands after the first would be lost.\n>>\n>> I found this description a little confusing as transform_todo_file()\n>> also called todo_list_parse_insn_buffer(). transform_todo_file() does\n>> not exist anymore but it looked like\n>>\n>> static int transform_todo_file(unsigned flags)\n>> {\n>>           const char *todo_file = rebase_path_todo();\n>>           struct todo_list todo_list = TODO_LIST_INIT;\n>>           int res;\n>>\n>>           if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)\n>>                   return error_errno(_(\"could not read '%s'.\"), todo_file);\n>>\n>>           if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n>>                                           &todo_list)) {\n>>                   todo_list_release(&todo_list);\n>>                   return error(_(\"unusable todo list: '%s'\"), todo_file);\n>>           }\n>>\n>>           res = todo_list_write_to_file(the_repository, &todo_list,\n>> todo_file,\n>>                                         NULL, NULL, -1, flags);\n>>           todo_list_release(&todo_list);\n>>\n>>           if (res)\n>>                   return error_errno(_(\"could not write '%s'.\"), todo_file);\n>>           return 0;\n>> }\n>>\n>> If it could not parse the todo list it did not try and write it to disc.\n>> After ddb81e5072 this changed as edit_todo_list() tries to shorten the\n>> OIDs in the todo list before it is edited even if it cannot be parsed.\n>> The fix below works around that by making sure we always try and parse\n>> the whole todo list even if the the first line is a fixup command. That\n>> is a worthwhile improvement because it means we notify the user of all\n>> the errors we find rather than just the first one and is in keeping with\n>> the way we handle other invalid lines. It does not however fix the root\n>> cause of this regression which is the change in behavior in\n>> edit_todo_list().\n>>\n>> After the user edits the todo file we do not try to transform the OIDs\n>> if it cannot be parsed or has missing commits. Therefore it still\n>> contains the shortened OIDs and editing hints and there is no need for\n>> edit_todo_list() to call write_todo_list() when \"incorrect\" is true.\n> \n> When the user first runs `git rebase`, the commit template contains\n> the following message:\n> \n> # However, if you remove everything, the rebase will be aborted.\n> \n> When running `git rebase --edit-todo`, that message is replaced with:\n> \n> # You are editing the todo file of an ongoing interactive rebase.\n> # To continue rebase after editing, run:\n> #     git rebase --continue\n> \n> The second message is indeed more accurate after the rebase has\n> started: Deleting all of the lines in `git rebase --edit-todo` drops\n> all of the commits; it does not abort the rebase.\n\nOh, good point\n\n> It would be nice to preserve as much of the user's original input as\n> possible, but that's not a project that I'm going to tackle.\n\nI think with your patch we do that anyway as other invalid lines are \nalready passed through verbatim.\n\n> As far as\n> a minimal fix for the regression, we can either leave the todo file\n> untouched and display inaccurate advice during `git rebase\n> --edit-todo`, or we can lose any long commit IDs that the user entered\n> and display equivalent short hex IDs instead. I would prefer the\n> latter.\n\nThat's fine but the commit message should explain that decision and \nclarify why ddb81e5072 caused the regression as \ntodo_list_parse_insn_buffer() is unchanged by that commit. The \nregression is caused by ddb81e5072 trying to shorten the OIDs and add \nthe correct advice even when we cannot parse the todo list. That is a \nworthwhile change that we want to keep but means we need to tweak \ntodo_list_parse_insn_buffer() in order to avoid truncating the todo list.\n\n>>> +             test_i18ngrep \"cannot .fixup. without a previous commit\" \\\n>>> +                             actual &&\n>>> +             test_i18ngrep \"You can fix this with .git rebase --edit-todo..\" \\\n>>> +                             actual &&\n>>> +             grep -v \"^#\" .git/rebase-merge/git-rebase-todo >orig &&\n>>> +             test_must_fail git rebase --edit-todo &&\n>>> +             grep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n>>> +             test_cmp orig actual\n>>\n>> We check that the uncommitted lines after running \"git rebase\n>> --edit-todo\" match the uncommitted lines after the initial edit. That's\n>> fine to detect if the second edit truncates the file but it will still\n>> pass if the initial edit starts truncating the todo list as well. As we\n>> expect that git should not change an incorrect todo list we do not need\n>> to filter out the lines beginning with \"#\".\n>>\n>> To ensure we detect a regression where the first edit truncates the todo\n>> list we could do something like\n>>\n>>          test_when_finished \"git rebase --abort\" &&\n>>          cat >todo <<-EOF &&\n>>          fixup $(git log -1 --format=\"%h %s\" B)\n>>          pick $(git log -1 --format=\"%h %s\" C)\n>>          EOF\n>>\n>>          (\n>>                  set_replace_editor todo &&\n>>                  test_must_fail git rebase -i A 2>actual\n>>          ) &&\n>>          test_i18ngrep \"cannot .fixup. without a previous commit\" actual &&\n>>          test_i18ngrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n>>          # check initial edit has not truncated todo list\n>>          grep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n>>          test_cmp todo actual &&\n>>          cat .git/rebase-merge/git-rebase-todo >expect &&\n>>          test_must_fail git rebase --edit-todo &&\n>>          # check the list is unchanged by --edit-todo\n>>          test_cmp expect .git/rebase-merge/git-rebase-todo\n> \n> To me it seems pretty far-fetched that a future bug would cause the\n> _initial_ commit template to be missing anything.\n\nIndeed but I'm talking about the initial todo list _after_ it has been \nedited by the user, not the initial template. If we started trying to \nlengthen the OIDs and remove the advice after the initial edit even when \nthe list cannot be parsed then we'd have exactly this problem.\n\nBest Wishes\n\nPhillip\n\n> But if you're\n> concerned about it, would you like to send a follow-up patch to revise\n> the test as you see fit?\n> \n>> We could perhaps check the error message from \"git rebase --edit-todo\"\n>> as well.\n> \n> That sounds like another good change for v2.\n> \n> Thanks for the feedback,\n> \n> -Alex\n"},{"id":"479730","messageId":"d6e54535-79a3-2d2c-3152-4cabc5bbd9b8@gmail.com","threadId":"60012","inReplyTo":"c7b7f078-6561-5081-9c23-0cec65b71c97@gmail.com","subject":"Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-21T13:08:42Z","receivedAt":"2023-07-21T13:08:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 21/07/2023 10:31, Phillip Wood wrote:\n> That's fine but the commit message should explain that decision and \n> clarify why ddb81e5072 caused the regression\n\nMaybe something like\n\nBefore the todo list is edited it is rewritten to shorten the OIDs of\nthe commits being picked and to append advice about editing the\nlist. The exact advice depends on whether the todo list is being\nedited for the first time or not. After the todo list has been edited\nit is rewritten to lengthen the OIDs of the commits being picked and to\nremove the advice. If the edited list cannot be parsed then this last\nstep is skipped.\n\nPrior to db81e50724 (rebase-interactive: use todo_list_write_to_file()\nin edit_todo_list(), 2019-03-05) if the existing todo list could not\nbe parsed then the initial rewrite was skipped as well. This had the\nunfortunate consequence that if the list could not be parsed after the\ninitial edit the advice given to the user was wrong when they\nre-edited the list. This change relied on\ntodo_list_parse_insn_buffer() returning the whole todo list even when\nit cannot be parsed. Unfortunately if the list starts with a \"fixup\"\ncommand then it will be truncated and the remaining lines are\nlost. Fix this by continuing to parse after an initial \"fixup\" commit\nas we do when we see any other invalid line.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"479731","messageId":"8e38a723-9fda-2d76-c767-7b76cb428a73@gmail.com","threadId":"60012","inReplyTo":"20230721060848.35641-1-alexhenrie24@gmail.com","subject":"Re: [PATCH v4 0/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-21T13:13:09Z","receivedAt":"2023-07-21T13:13:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 21/07/2023 07:07, Alex Henrie wrote:\n> Changes from v3:\n> - Rebase onto HEAD~4 instead of --root (which was the original motivation\n>    for sending a new patch and I forgot to include that change; I probably\n>    shouldn't be doing Git development late at night...)\n\nPlease don't feel that you have to re-roll straight away when someone \nreviews your patch - it's fine to wait a while if you're busy. Also if \nyou disagree with some of the reviewer's comments it can be helpful to \nwait for them to respond before sending a new version in order to try \nand reach a consensus about the best way forward.\n\nBest Wishes\n\nPhillip\n\n> Alex Henrie (1):\n>    sequencer: finish parsing the todo list despite an invalid first line\n> \n>   sequencer.c                   |  2 +-\n>   t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++\n>   2 files changed, 19 insertions(+), 1 deletion(-)\n> \n> Range-diff against v3:\n> 1:  b1af2df3f5 ! 1:  f6fcdcd9a9 sequencer: finish parsing the todo list despite an invalid first line\n>      @@ t/t3404-rebase-interactive.sh: test_expect_success 'static check of bad command'\n>       +\trebase_setup_and_clean fixup-first &&\n>       +\t(\n>       +\t\tset_fake_editor &&\n>      -+\t\ttest_must_fail env FAKE_LINES=\"fixup 1 2 3 4 5\" \\\n>      -+\t\t\t       git rebase -i --root 2>actual &&\n>      ++\t\ttest_must_fail env FAKE_LINES=\"fixup 1 2 3 4\" \\\n>      ++\t\t\t       git rebase -i HEAD~4 2>actual &&\n>       +\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n>       +\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n>       +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >orig &&\n\n"},{"id":"479739","messageId":"xmqqpm4lwasj.fsf@gitster.g","threadId":"60012","inReplyTo":"d6e54535-79a3-2d2c-3152-4cabc5bbd9b8@gmail.com","subject":"Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-21T15:21:00Z","receivedAt":"2023-07-21T15:25:39Z","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> On 21/07/2023 10:31, Phillip Wood wrote:\n>> That's fine but the commit message should explain that decision and\n>> clarify why ddb81e5072 caused the regression\n>\n> Maybe something like\n>\n> Before the todo list is edited it is rewritten to shorten the OIDs of\n> the commits being picked and to append advice about editing the\n> list. The exact advice depends on whether the todo list is being\n> edited for the first time or not. After the todo list has been edited\n> it is rewritten to lengthen the OIDs of the commits being picked and to\n> remove the advice. If the edited list cannot be parsed then this last\n> step is skipped.\n>\n> Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()\n> in edit_todo_list(), 2019-03-05) if the existing todo list could not\n> be parsed then the initial rewrite was skipped as well. This had the\n> unfortunate consequence that if the list could not be parsed after the\n> initial edit the advice given to the user was wrong when they\n> re-edited the list. This change relied on\n> todo_list_parse_insn_buffer() returning the whole todo list even when\n> it cannot be parsed. Unfortunately if the list starts with a \"fixup\"\n> command then it will be truncated and the remaining lines are\n> lost. Fix this by continuing to parse after an initial \"fixup\" commit\n> as we do when we see any other invalid line.\n>\n> Best Wishes\n>\n> Phillip\n\nThat does sounds a lot easier to understand as an explanation for\nthe reason why this change is necessary and sufficient.\n\nThanks for reviewing.\n\n"},{"id":"479777","messageId":"20230722212830.132135-2-alexhenrie24@gmail.com","threadId":"60012","inReplyTo":"20230722212830.132135-1-alexhenrie24@gmail.com","subject":"[PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-07-22T21:28:25Z","receivedAt":"2023-07-22T21:29:15Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Before the todo list is edited it is rewritten to shorten the OIDs of\nthe commits being picked and to append advice about editing the list.\nThe exact advice depends on whether the todo list is being edited for\nthe first time or not. After the todo list has been edited it is\nrewritten to lengthen the OIDs of the commits being picked and to remove\nthe advice. If the edited list cannot be parsed then this last step is\nskipped.\n\nPrior to db81e50724 (rebase-interactive: use todo_list_write_to_file()\nin edit_todo_list(), 2019-03-05) if the existing todo list could not be\nparsed then the initial rewrite was skipped as well. This had the\nunfortunate consequence that if the list could not be parsed after the\ninitial edit the advice given to the user was wrong when they re-edited\nthe list. This change relied on todo_list_parse_insn_buffer() returning\nthe whole todo list even when it cannot be parsed. Unfortunately if the\nlist starts with a \"fixup\" command then it will be truncated and the\nremaining lines are lost. Fix this by continuing to parse after an\ninitial \"fixup\" commit as we do when we see any other invalid line.\n\nSigned-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\n sequencer.c                   |  2 +-\n t/t3404-rebase-interactive.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cc9821ece2..adc9cfb4df 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2702,7 +2702,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n \t\tif (fixup_okay)\n \t\t\t; /* do nothing */\n \t\telse if (is_fixup(item->command))\n-\t\t\treturn error(_(\"cannot '%s' without a previous commit\"),\n+\t\t\tres = error(_(\"cannot '%s' without a previous commit\"),\n \t\t\t\tcommand_to_string(item->command));\n \t\telse if (!is_noop(item->command))\n \t\t\tfixup_okay = 1;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ff0afad63e..c734983da0 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1596,6 +1596,33 @@ test_expect_success 'static check of bad command' '\n \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_success 'the first command cannot be a fixup' '\n+\trebase_setup_and_clean fixup-first &&\n+\t(\n+\t\tcat >orig <<-EOF &&\n+\t\tfixup $(git log -1 --format=\"%h %s\" B)\n+\t\tpick $(git log -1 --format=\"%h %s\" C)\n+\t\tEOF\n+\n+\t\t(\n+\t\t\tset_replace_editor orig &&\n+\t\t\ttest_must_fail git rebase -i A 2>actual\n+\t\t) &&\n+\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n+\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n+\t\t# verify that the todo list has not been truncated\n+\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n+\t\ttest_cmp orig actual &&\n+\n+\t\ttest_must_fail git rebase --edit-todo 2>actual &&\n+\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n+\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n+\t\t# verify that the todo list has not been truncated\n+\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n+\t\ttest_cmp orig actual\n+\t)\n+'\n+\n test_expect_success 'tabs and spaces are accepted in the todolist' '\n \trebase_setup_and_clean indented-comment &&\n \twrite_script add-indent.sh <<-\\EOF &&\n-- \n2.41.0\n\n"},{"id":"479778","messageId":"20230722212830.132135-1-alexhenrie24@gmail.com","threadId":"60012","inReplyTo":"20230721060848.35641-1-alexhenrie24@gmail.com","subject":"[PATCH v5 0/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-07-22T21:28:24Z","receivedAt":"2023-07-22T21:29:19Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Changes from v4:\n- Put Phillip's suggested explanation in the commit message\n- Check for truncation both after the first `git rebase` and after\n  `git rebase --edit-todo`\n\nThanks to Phillip and Junio for your feedback.\n\nAlex Henrie (1):\n  sequencer: finish parsing the todo list despite an invalid first line\n\n sequencer.c                   |  2 +-\n t/t3404-rebase-interactive.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 28 insertions(+), 1 deletion(-)\n\nRange-diff against v4:\n1:  f6fcdcd9a9 ! 1:  6fbe4fd3e6 sequencer: finish parsing the todo list despite an invalid first line\n    @@ Metadata\n      ## Commit message ##\n         sequencer: finish parsing the todo list despite an invalid first line\n     \n    -    ddb81e5072 (rebase-interactive: use todo_list_write_to_file() in\n    -    edit_todo_list(), 2019-03-05) made edit_todo_list more efficient by\n    -    replacing transform_todo_file with todo_list_parse_insn_buffer.\n    -    Unfortunately, that innocuous change caused a regression because\n    -    todo_list_parse_insn_buffer would stop parsing after encountering an\n    -    invalid 'fixup' line. If the user accidentally made the first line a\n    -    'fixup' and tried to recover from their mistake with `git rebase\n    -    --edit-todo`, all of the commands after the first would be lost.\n    +    Before the todo list is edited it is rewritten to shorten the OIDs of\n    +    the commits being picked and to append advice about editing the list.\n    +    The exact advice depends on whether the todo list is being edited for\n    +    the first time or not. After the todo list has been edited it is\n    +    rewritten to lengthen the OIDs of the commits being picked and to remove\n    +    the advice. If the edited list cannot be parsed then this last step is\n    +    skipped.\n     \n    -    To avoid throwing away important parts of the todo list, change\n    -    todo_list_parse_insn_buffer to keep going and not return early on error.\n    +    Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()\n    +    in edit_todo_list(), 2019-03-05) if the existing todo list could not be\n    +    parsed then the initial rewrite was skipped as well. This had the\n    +    unfortunate consequence that if the list could not be parsed after the\n    +    initial edit the advice given to the user was wrong when they re-edited\n    +    the list. This change relied on todo_list_parse_insn_buffer() returning\n    +    the whole todo list even when it cannot be parsed. Unfortunately if the\n    +    list starts with a \"fixup\" command then it will be truncated and the\n    +    remaining lines are lost. Fix this by continuing to parse after an\n    +    initial \"fixup\" commit as we do when we see any other invalid line.\n     \n         Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n     \n    @@ t/t3404-rebase-interactive.sh: test_expect_success 'static check of bad command'\n     +test_expect_success 'the first command cannot be a fixup' '\n     +\trebase_setup_and_clean fixup-first &&\n     +\t(\n    -+\t\tset_fake_editor &&\n    -+\t\ttest_must_fail env FAKE_LINES=\"fixup 1 2 3 4\" \\\n    -+\t\t\t       git rebase -i HEAD~4 2>actual &&\n    ++\t\tcat >orig <<-EOF &&\n    ++\t\tfixup $(git log -1 --format=\"%h %s\" B)\n    ++\t\tpick $(git log -1 --format=\"%h %s\" C)\n    ++\t\tEOF\n    ++\n    ++\t\t(\n    ++\t\t\tset_replace_editor orig &&\n    ++\t\t\ttest_must_fail git rebase -i A 2>actual\n    ++\t\t) &&\n     +\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n     +\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n    -+\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >orig &&\n    ++\t\t# verify that the todo list has not been truncated\n    ++\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n    ++\t\ttest_cmp orig actual &&\n    ++\n     +\t\ttest_must_fail git rebase --edit-todo 2>actual &&\n     +\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n     +\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n    ++\t\t# verify that the todo list has not been truncated\n     +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n    -+\t\t# check that --edit-todo did not lose any of the todo list\n     +\t\ttest_cmp orig actual\n     +\t)\n     +'\n-- \n2.41.0\n\n"},{"id":"479802","messageId":"0d1c5bfd-3ae5-83f0-a333-bbb8510a973a@gmail.com","threadId":"60012","inReplyTo":"20230722212830.132135-2-alexhenrie24@gmail.com","subject":"Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-24T10:02:37Z","receivedAt":"2023-07-24T10:10:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nOn 22/07/2023 22:28, Alex Henrie wrote:\n> Before the todo list is edited it is rewritten to shorten the OIDs of\n> the commits being picked and to append advice about editing the list.\n> The exact advice depends on whether the todo list is being edited for\n> the first time or not. After the todo list has been edited it is\n> rewritten to lengthen the OIDs of the commits being picked and to remove\n> the advice. If the edited list cannot be parsed then this last step is\n> skipped.\n> \n> Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()\n> in edit_todo_list(), 2019-03-05) if the existing todo list could not be\n> parsed then the initial rewrite was skipped as well. This had the\n> unfortunate consequence that if the list could not be parsed after the\n> initial edit the advice given to the user was wrong when they re-edited\n> the list. This change relied on todo_list_parse_insn_buffer() returning\n> the whole todo list even when it cannot be parsed. Unfortunately if the\n> list starts with a \"fixup\" command then it will be truncated and the\n> remaining lines are lost. Fix this by continuing to parse after an\n> initial \"fixup\" commit as we do when we see any other invalid line.\n\nThis version looks great apart from the test being run in an unnecessary \nsubshell which looks like it got left in from the last version. Junio \nmight be able to correct that when he applies the patch.\n\nThanks for updating the test and commit message\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n>   sequencer.c                   |  2 +-\n>   t/t3404-rebase-interactive.sh | 27 +++++++++++++++++++++++++++\n>   2 files changed, 28 insertions(+), 1 deletion(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index cc9821ece2..adc9cfb4df 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2702,7 +2702,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n>   \t\tif (fixup_okay)\n>   \t\t\t; /* do nothing */\n>   \t\telse if (is_fixup(item->command))\n> -\t\t\treturn error(_(\"cannot '%s' without a previous commit\"),\n> +\t\t\tres = error(_(\"cannot '%s' without a previous commit\"),\n>   \t\t\t\tcommand_to_string(item->command));\n>   \t\telse if (!is_noop(item->command))\n>   \t\t\tfixup_okay = 1;\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index ff0afad63e..c734983da0 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1596,6 +1596,33 @@ test_expect_success 'static check of bad command' '\n>   \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n>   '\n>   \n> +test_expect_success 'the first command cannot be a fixup' '\n> +\trebase_setup_and_clean fixup-first &&\n> +\t(\n> +\t\tcat >orig <<-EOF &&\n> +\t\tfixup $(git log -1 --format=\"%h %s\" B)\n> +\t\tpick $(git log -1 --format=\"%h %s\" C)\n> +\t\tEOF\n> +\n> +\t\t(\n> +\t\t\tset_replace_editor orig &&\n> +\t\t\ttest_must_fail git rebase -i A 2>actual\n> +\t\t) &&\n> +\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n> +\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n> +\t\t# verify that the todo list has not been truncated\n> +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n> +\t\ttest_cmp orig actual &&\n> +\n> +\t\ttest_must_fail git rebase --edit-todo 2>actual &&\n> +\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n> +\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n> +\t\t# verify that the todo list has not been truncated\n> +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n> +\t\ttest_cmp orig actual\n> +\t)\n> +'\n> +\n>   test_expect_success 'tabs and spaces are accepted in the todolist' '\n>   \trebase_setup_and_clean indented-comment &&\n>   \twrite_script add-indent.sh <<-\\EOF &&\n\n"},{"id":"479810","messageId":"CAMMLpeTBBS7FExevcvCWut8wFbcDSDBhUUq+tCaXfOPiY+3GXA@mail.gmail.com","threadId":"60012","inReplyTo":"0d1c5bfd-3ae5-83f0-a333-bbb8510a973a@gmail.com","subject":"Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-07-24T15:26:21Z","receivedAt":"2023-07-24T15:27:07Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Mon, Jul 24, 2023 at 4:02 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n\n> On 22/07/2023 22:28, Alex Henrie wrote:\n> > Before the todo list is edited it is rewritten to shorten the OIDs of\n> > the commits being picked and to append advice about editing the list.\n> > The exact advice depends on whether the todo list is being edited for\n> > the first time or not. After the todo list has been edited it is\n> > rewritten to lengthen the OIDs of the commits being picked and to remove\n> > the advice. If the edited list cannot be parsed then this last step is\n> > skipped.\n> >\n> > Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()\n> > in edit_todo_list(), 2019-03-05) if the existing todo list could not be\n> > parsed then the initial rewrite was skipped as well. This had the\n> > unfortunate consequence that if the list could not be parsed after the\n> > initial edit the advice given to the user was wrong when they re-edited\n> > the list. This change relied on todo_list_parse_insn_buffer() returning\n> > the whole todo list even when it cannot be parsed. Unfortunately if the\n> > list starts with a \"fixup\" command then it will be truncated and the\n> > remaining lines are lost. Fix this by continuing to parse after an\n> > initial \"fixup\" commit as we do when we see any other invalid line.\n>\n> This version looks great apart from the test being run in an unnecessary\n> subshell which looks like it got left in from the last version. Junio\n> might be able to correct that when he applies the patch.\n\nI think I see what you mean now: Because this test never performs a\nsuccessful rebase, rebase_setup_and_clean is overkill. I can send a v6\ntonight that uses 'test_when_finished \"git rebase --abort\"' instead.\n\nThanks,\n\n-Alex\n"},{"id":"479811","messageId":"ecabaf7d-d5a3-d2d9-2610-87c6a7b2570e@gmail.com","threadId":"60012","inReplyTo":"CAMMLpeTBBS7FExevcvCWut8wFbcDSDBhUUq+tCaXfOPiY+3GXA@mail.gmail.com","subject":"Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-24T16:00:34Z","receivedAt":"2023-07-24T16:00:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nOn 24/07/2023 16:26, Alex Henrie wrote:\n> On Mon, Jul 24, 2023 at 4:02 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n> \n>> On 22/07/2023 22:28, Alex Henrie wrote:\n>>> Before the todo list is edited it is rewritten to shorten the OIDs of\n>>> the commits being picked and to append advice about editing the list.\n>>> The exact advice depends on whether the todo list is being edited for\n>>> the first time or not. After the todo list has been edited it is\n>>> rewritten to lengthen the OIDs of the commits being picked and to remove\n>>> the advice. If the edited list cannot be parsed then this last step is\n>>> skipped.\n>>>\n>>> Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()\n>>> in edit_todo_list(), 2019-03-05) if the existing todo list could not be\n>>> parsed then the initial rewrite was skipped as well. This had the\n>>> unfortunate consequence that if the list could not be parsed after the\n>>> initial edit the advice given to the user was wrong when they re-edited\n>>> the list. This change relied on todo_list_parse_insn_buffer() returning\n>>> the whole todo list even when it cannot be parsed. Unfortunately if the\n>>> list starts with a \"fixup\" command then it will be truncated and the\n>>> remaining lines are lost. Fix this by continuing to parse after an\n>>> initial \"fixup\" commit as we do when we see any other invalid line.\n>>\n>> This version looks great apart from the test being run in an unnecessary\n>> subshell which looks like it got left in from the last version. Junio\n>> might be able to correct that when he applies the patch.\n> \n> I think I see what you mean now: Because this test never performs a\n> successful rebase, rebase_setup_and_clean is overkill. I can send a v6\n> tonight that uses 'test_when_finished \"git rebase --abort\"' instead.\n\nI don't mind either way on that particular issue though I agree we could \njust use 'test_when_finished \"git rebase --abort\"' instead. What I was \nreferring to was the subshell that comes after 'rebase_setup_and_clean'. \nThe change I was looking for was removing the '(' after \n\"rebase_setup_and_clean\" at the beginning of the test, removing ')' at \nthe end of the test and adjusting the indentation.\n\n+test_expect_success 'the first command cannot be a fixup' '\n+\trebase_setup_and_clean fixup-first &&\n+\t(\n\nThis subshell is unnecessary as we're not changing directory or \nexporting any environment variables\n\n+\t\tcat >orig <<-EOF &&\n+\t\tfixup $(git log -1 --format=\"%h %s\" B)\n+\t\tpick $(git log -1 --format=\"%h %s\" C)\n+\t\tEOF\n+\n+\t\t(\n\nThis subshell is required as we're setting GIT_SEQUENCE_EDITOR in the \nenvironment. We only want that set for the initial rebase. In particular \nwe do not want it set when we run \"git rebase --edit-todo\" below which \nis why we exit the subshell as soon as the initial rebase exits.\n\n\nBest Wishes\n\nPhillip\n\n+\t\t\tset_replace_editor orig &&\n+\t\t\ttest_must_fail git rebase -i A 2>actual\n+\t\t) &&\n+\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n+\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n+\t\t# verify that the todo list has not been truncated\n+\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n+\t\ttest_cmp orig actual &&\n+\n+\t\ttest_must_fail git rebase --edit-todo 2>actual &&\n+\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n+\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n+\t\t# verify that the todo list has not been truncated\n+\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n+\t\ttest_cmp orig actual\n+\t)\n+'\n\n\n> Thanks,\n> \n> -Alex\n"},{"id":"479818","messageId":"xmqqjzupqn3q.fsf@gitster.g","threadId":"60012","inReplyTo":"20230722212830.132135-2-alexhenrie24@gmail.com","subject":"Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-24T16:40:41Z","receivedAt":"2023-07-24T16:40:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie <alexhenrie24@gmail.com> writes:\n\n> Before the todo list is edited it is rewritten to shorten the OIDs of\n> the commits being picked and to append advice about editing the list.\n> The exact advice depends on whether the todo list is being edited for\n> the first time or not. After the todo list has been edited it is\n> rewritten to lengthen the OIDs of the commits being picked and to remove\n> the advice. If the edited list cannot be parsed then this last step is\n> skipped.\n>\n> Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()\n> in edit_todo_list(), 2019-03-05) if the existing todo list could not be\n> parsed then the initial rewrite was skipped as well. This had the\n> unfortunate consequence that if the list could not be parsed after the\n> initial edit the advice given to the user was wrong when they re-edited\n> the list. This change relied on todo_list_parse_insn_buffer() returning\n> the whole todo list even when it cannot be parsed. Unfortunately if the\n> list starts with a \"fixup\" command then it will be truncated and the\n> remaining lines are lost. Fix this by continuing to parse after an\n> initial \"fixup\" commit as we do when we see any other invalid line.\n>\n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n>  sequencer.c                   |  2 +-\n>  t/t3404-rebase-interactive.sh | 27 +++++++++++++++++++++++++++\n>  2 files changed, 28 insertions(+), 1 deletion(-)\n\nVery cleanly explained.  Thanks for an update.\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index ff0afad63e..c734983da0 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1596,6 +1596,33 @@ test_expect_success 'static check of bad command' '\n>  \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n>  '\n>  \n> +test_expect_success 'the first command cannot be a fixup' '\n> +\trebase_setup_and_clean fixup-first &&\n> +\t(\n> +\t\tcat >orig <<-EOF &&\n> +\t\tfixup $(git log -1 --format=\"%h %s\" B)\n> +\t\tpick $(git log -1 --format=\"%h %s\" C)\n> +\t\tEOF\n> +\n> +\t\t(\n> +\t\t\tset_replace_editor orig &&\n> +\t\t\ttest_must_fail git rebase -i A 2>actual\n> +\t\t) &&\n> +\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n> +\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n> +\t\t# verify that the todo list has not been truncated\n> +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n> +\t\ttest_cmp orig actual &&\n> +\n> +\t\ttest_must_fail git rebase --edit-todo 2>actual &&\n> +\t\tgrep \"cannot .fixup. without a previous commit\" actual &&\n> +\t\tgrep \"You can fix this with .git rebase --edit-todo..\" actual &&\n> +\t\t# verify that the todo list has not been truncated\n> +\t\tgrep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n> +\t\ttest_cmp orig actual\n> +\t)\n> +'\n\nThe structure of this new test piece, including the use of \"log -1\n--format\", seems to follow existing tests, and very readable.  Why\ndo we have one extra level of subshell, though?  There is no \"cd\"\nthat may affect the later test pieces, and set_something_editor that\ntouches environment that may affect the later test pieces is called\nin its own subshell already.\n\nOther than that, looking good (there may be a valid reason why the\ntest piece needs the subshell around it, but it was just not apparent\nto me).\n\n>  test_expect_success 'tabs and spaces are accepted in the todolist' '\n>  \trebase_setup_and_clean indented-comment &&\n>  \twrite_script add-indent.sh <<-\\EOF &&\n"},{"id":"479821","messageId":"CAMMLpeSQ4xPMSOCyN7hnq6efQp0w9uzCXBcG5my8U3Yamzrnpg@mail.gmail.com","threadId":"60012","inReplyTo":"xmqqjzupqn3q.fsf@gitster.g","subject":"Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-07-24T17:39:03Z","receivedAt":"2023-07-24T17:39:43Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Mon, Jul 24, 2023 at 10:40 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Alex Henrie <alexhenrie24@gmail.com> writes:\n\n> > +test_expect_success 'the first command cannot be a fixup' '\n> > +     rebase_setup_and_clean fixup-first &&\n> > +     (\n> > +             cat >orig <<-EOF &&\n> > +             fixup $(git log -1 --format=\"%h %s\" B)\n> > +             pick $(git log -1 --format=\"%h %s\" C)\n> > +             EOF\n> > +\n> > +             (\n> > +                     set_replace_editor orig &&\n> > +                     test_must_fail git rebase -i A 2>actual\n> > +             ) &&\n> > +             grep \"cannot .fixup. without a previous commit\" actual &&\n> > +             grep \"You can fix this with .git rebase --edit-todo..\" actual &&\n> > +             # verify that the todo list has not been truncated\n> > +             grep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n> > +             test_cmp orig actual &&\n> > +\n> > +             test_must_fail git rebase --edit-todo 2>actual &&\n> > +             grep \"cannot .fixup. without a previous commit\" actual &&\n> > +             grep \"You can fix this with .git rebase --edit-todo..\" actual &&\n> > +             # verify that the todo list has not been truncated\n> > +             grep -v \"^#\" .git/rebase-merge/git-rebase-todo >actual &&\n> > +             test_cmp orig actual\n> > +     )\n> > +'\n>\n> The structure of this new test piece, including the use of \"log -1\n> --format\", seems to follow existing tests, and very readable.  Why\n> do we have one extra level of subshell, though?  There is no \"cd\"\n> that may affect the later test pieces, and set_something_editor that\n> touches environment that may affect the later test pieces is called\n> in its own subshell already.\n>\n> Other than that, looking good (there may be a valid reason why the\n> test piece needs the subshell around it, but it was just not apparent\n> to me).\n\nThe only reason for the outer subshell is that I thought it was\nrequired when using rebase_setup_and_clean, but I see now that\nrebase_setup_and_clean is used in several tests without a subshell.\nI'll drop it altogether in v6 and use `test_when_finished \"git rebase\n--abort\"` instead.\n\nThanks,\n\n-Alex\n"},{"id":"479825","messageId":"xmqqedkxp3fn.fsf@gitster.g","threadId":"60012","inReplyTo":"xmqqjzupqn3q.fsf@gitster.g","subject":"Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-24T18:30:52Z","receivedAt":"2023-07-24T18:31:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> The structure of this new test piece, including the use of \"log -1\n> --format\", seems to follow existing tests, and very readable.  Why\n> do we have one extra level of subshell, though?  There is no \"cd\"\n> that may affect the later test pieces, and set_something_editor that\n> touches environment that may affect the later test pieces is called\n> in its own subshell already.\n>\n> Other than that, looking good (there may be a valid reason why the\n> test piece needs the subshell around it, but it was just not apparent\n> to me).\n\nAh, now I notice that Phillip also noticed the same thing.\n\nI just removed the outer subshell while queuing.  Thanks for working\non this, and thanks Phillip for excellent reviews.\n\n"},{"id":"479828","messageId":"CAMMLpeQwG5TbzwUKY=QOnHA-mey2NZgM+4ssF+qDrWJ=Vxs9Uw@mail.gmail.com","threadId":"60012","inReplyTo":"xmqqedkxp3fn.fsf@gitster.g","subject":"Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-07-24T20:08:19Z","receivedAt":"2023-07-24T20:08:59Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Mon, Jul 24, 2023 at 12:30 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> > The structure of this new test piece, including the use of \"log -1\n> > --format\", seems to follow existing tests, and very readable.  Why\n> > do we have one extra level of subshell, though?  There is no \"cd\"\n> > that may affect the later test pieces, and set_something_editor that\n> > touches environment that may affect the later test pieces is called\n> > in its own subshell already.\n> >\n> > Other than that, looking good (there may be a valid reason why the\n> > test piece needs the subshell around it, but it was just not apparent\n> > to me).\n>\n> Ah, now I notice that Phillip also noticed the same thing.\n>\n> I just removed the outer subshell while queuing.  Thanks for working\n> on this, and thanks Phillip for excellent reviews.\n\nThat works. Thanks to both of you!\n\n-Alex\n"}]}