Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line

> On 19/07/2023 15:43, Alex Henrie wrote:
> > ddb81e5072 (rebase-interactive: use todo_list_write_to_file() in
> > edit_todo_list(), 2019-03-05) made edit_todo_list more efficient by
> > replacing transform_todo_file with todo_list_parse_insn_buffer.
> > Unfortunately, that innocuous change caused a regression because
> > todo_list_parse_insn_buffer would stop parsing after encountering an
> > invalid 'fixup' line. If the user accidentally made the first line a
> > 'fixup' and tried to recover from their mistake with `git rebase
> > --edit-todo`, all of the commands after the first would be lost.
>
> I found this description a little confusing as transform_todo_file()
> also called todo_list_parse_insn_buffer(). transform_todo_file() does
> not exist anymore but it looked like
>
> static int transform_todo_file(unsigned flags)
> {
>          const char *todo_file = rebase_path_todo();
>          struct todo_list todo_list = TODO_LIST_INIT;
>          int res;
>
>          if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
>                  return error_errno(_("could not read '%s'."), todo_file);
>
>          if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
>                                          &todo_list)) {
>                  todo_list_release(&todo_list);
>                  return error(_("unusable todo list: '%s'"), todo_file);
>          }
>
>          res = todo_list_write_to_file(the_repository, &todo_list,
> todo_file,
>                                        NULL, NULL, -1, flags);
>          todo_list_release(&todo_list);
>
>          if (res)
>                  return error_errno(_("could not write '%s'."), todo_file);
>          return 0;
> }
>
> If it could not parse the todo list it did not try and write it to disc.
> After ddb81e5072 this changed as edit_todo_list() tries to shorten the
> OIDs in the todo list before it is edited even if it cannot be parsed.
> The fix below works around that by making sure we always try and parse
> the whole todo list even if the the first line is a fixup command. That
> is a worthwhile improvement because it means we notify the user of all
> the errors we find rather than just the first one and is in keeping with
> the way we handle other invalid lines. It does not however fix the root
> cause of this regression which is the change in behavior in
> edit_todo_list().
>
> After the user edits the todo file we do not try to transform the OIDs
> if it cannot be parsed or has missing commits. Therefore it still
> contains the shortened OIDs and editing hints and there is no need for
> edit_todo_list() to call write_todo_list() when "incorrect" is true.

> > +test_expect_success 'the first command cannot be a fixup' '
> > +     # When using `git rebase --edit-todo` to recover from this error, ensure
> > +     # that none of the original todo list is lost
> > +     rebase_setup_and_clean fixup-first &&
> > +     (
> > +             set_fake_editor &&
> > +             test_must_fail env FAKE_LINES="fixup 1 2 3 4 5" \
> > +                            git rebase -i --root 2>actual &&
>
> Thanks for taking the time to add a test. It is not worth a re-roll on
> its own, but there is no need to use "--root" here. It is confusing as
> it is not clear if we're refusing "fixup" as the first command because
> we're rewriting the root commit or if we always refuse to have "fixup"
> as the first command.

> > +             test_i18ngrep "cannot .fixup. without a previous commit" \
> > +                             actual &&
> > +             test_i18ngrep "You can fix this with .git rebase --edit-todo.." \
> > +                             actual &&
> > +             grep -v "^#" .git/rebase-merge/git-rebase-todo >orig &&
> > +             test_must_fail git rebase --edit-todo &&
> > +             grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
> > +             test_cmp orig actual
>
> We check that the uncommitted lines after running "git rebase
> --edit-todo" match the uncommitted lines after the initial edit. That's
> fine to detect if the second edit truncates the file but it will still
> pass if the initial edit starts truncating the todo list as well. As we
> expect that git should not change an incorrect todo list we do not need
> to filter out the lines beginning with "#".
>
> To ensure we detect a regression where the first edit truncates the todo
> list we could do something like
>
>         test_when_finished "git rebase --abort" &&
>         cat >todo <<-EOF &&
>         fixup $(git log -1 --format="%h %s" B)
>         pick $(git log -1 --format="%h %s" C)
>         EOF
>
>         (
>                 set_replace_editor todo &&
>                 test_must_fail git rebase -i A 2>actual
>         ) &&
>         test_i18ngrep "cannot .fixup. without a previous commit" actual &&
>         test_i18ngrep "You can fix this with .git rebase --edit-todo.." actual &&
>         # check initial edit has not truncated todo list
>         grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
>         test_cmp todo actual &&
>         cat .git/rebase-merge/git-rebase-todo >expect &&
>         test_must_fail git rebase --edit-todo &&
>         # check the list is unchanged by --edit-todo
>         test_cmp expect .git/rebase-merge/git-rebase-todo