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

[Alex Henrie <alexhenrie24@gmail.com>]

On Thu, Jul 20, 2023 at 3:42 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

> 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.

When the user first runs `git rebase`, the commit template contains
the following message:

# However, if you remove everything, the rebase will be aborted.

When running `git rebase --edit-todo`, that message is replaced with:

# You are editing the todo file of an ongoing interactive rebase.
# To continue rebase after editing, run:
#     git rebase --continue

The second message is indeed more accurate after the rebase has
started: Deleting all of the lines in `git rebase --edit-todo` drops
all of the commits; it does not abort the rebase.

It would be nice to preserve as much of the user's original input as
possible, but that's not a project that I'm going to tackle. As far as
a minimal fix for the regression, we can either leave the todo file
untouched and display inaccurate advice during `git rebase
--edit-todo`, or we can lose any long commit IDs that the user entered
and display equivalent short hex IDs instead. I would prefer the
latter.

> > +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.

Good point. I used --root because I copied and pasted from the
preceding test, but HEAD~4 would make the intent of the test more
clear. That change and the grep change that Junio suggested are
probably worth a v2.

> > +             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

To me it seems pretty far-fetched that a future bug would cause the
_initial_ commit template to be missing anything. But if you're
concerned about it, would you like to send a follow-up patch to revise
the test as you see fit?

> We could perhaps check the error message from "git rebase --edit-todo"
> as well.

That sounds like another good change for v2.

Thanks for the feedback,

-Alex