On Thu, Jul 20, 2023 at 3:42 AM Phillip Wood [off-list ref] wrote:

On 19/07/2023 15:43, Alex Henrie wrote:

quoted

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.

quoted

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

quoted

+             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