"Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
writes:
> However, the bug resurfaced as a side effect of 393adf7a6f6 (sequencer:
> directly call pick_commits() from complete_action(), 2019-11-24): as of
> this commit, the sequencer no longer re-reads the todo list after
> writing it out with expanded commit IDs.
Ouch. The analysis above is quite understandable.
> @@ -5128,6 +5128,18 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> return -1;
> }
>
> + /* Expand the commit IDs */
> + todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);
> + strbuf_swap(&new_todo.buf, &buf2);
> + strbuf_release(&buf2);
> + new_todo.total_nr -= new_todo.nr;
> + if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0) {
> + fprintf(stderr, _(edit_todo_list_advice));
> + checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
> + todo_list_release(&new_todo);
> + return error(_("invalid todo list after expanding IDs"));
> + }
The above happens after edit_todo_list() returns and then the
resulting data (i.e. new_todo) that came from the on-disk file has
been parsed with an existing call to todo_lsit_parse_insn_buffer()?
I am wondering when this if() statement would trigger, iow, under
what condition the result of "Expand the commit IDs" operation fails
to be parsed correctly, and what the user can do to remedy it.
Especially given that incoming new_todo has passed the existing
parse and check without hitting "return -1" we see above in the
context, I am not sure if there is anything, other than any
potential glitch in the added code above, that can cause this second
parse_insn_buffer() to fail. Shouldn't the body of if() be simply a
BUG()?
Or am I somehow missing a hunk that removes an existing call to
parse&check?
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ae6e55ce79..1cc9f36bc7 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1264,13 +1264,24 @@ test_expect_success SHA1 'short SHA-1 setup' '
> test_expect_success SHA1 'short SHA-1 collide' '
> test_when_finished "reset_rebase && git checkout master" &&
> git checkout collide &&
> + colliding_sha1=6bcda37 &&
> + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
> (
> unset test_tick &&
> test_tick &&
> set_fake_editor &&
> FAKE_COMMIT_MESSAGE="collide2 ac4f2ee" \
> - FAKE_LINES="reword 1 2" git rebase -i HEAD~2
> - )
> + FAKE_LINES="reword 1 break 2" git rebase -i HEAD~2 &&
> + test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
> + grep "^pick $colliding_sha1 " \
> + .git/rebase-merge/git-rebase-todo.tmp &&
> + grep "^pick [0-9a-f]\{40\}" \
> + .git/rebase-merge/git-rebase-todo &&
> + git rebase --continue
> + ) &&
> + collide2="$(git rev-parse HEAD~1 | cut -c 1-4)" &&
> + collide3="$(git rev-parse collide3 | cut -c 1-4)" &&
> + test "$collide2" = "$collide3"
> '
>
> test_expect_success 'respect core.abbrev' '
