On Thu, Jul 20, 2023 at 3:42 AM Phillip Wood <phillip.wood123@xxxxxxxxx> 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
