Hi Oswald
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
When the user butchers the todo file during rebase -i setup, the
--continue which would follow --edit-todo would have skipped the last
steps of the setup. Notably, this would bypass the fast-forward over
untouched picks (though the actual picking loop would still fast-forward
the commits, one by one).
Fix this by splitting off the tail of complete_action() to a new
start_rebase() function and call that from sequencer_continue() when no
commands have been executed yet.
More or less as a side effect, we no longer checkout `onto` before exiting
when the todo file is bad.
I think the implications of this change deserve to be discussed in the
commit message. Three things spring to mind but there may be others I
haven't thought of
- Previously when rebase stopped and handed control back to the user
HEAD would have already been detached. This patch changes that
meaning we can have an active rebase of a branch while that branch is
checked out. What does "git status" show in this case? What does the
shell prompt show? Will it confuse users?
- Previously if the user created a commit before running "rebase
--continue" we'd rebase on to that commit. Now that commit will be
silently dropped.
- Previously if the user checkout out another commit before running
"rebase --continue" we'd rebase on to that commit. Now we we rebase
on to the original "onto" commit.
> This makes aborting cheaper and will simplify
> things in a later change.
Given that we're stopping so the user can fix the problem and continue
the rebase I don't think optimizing for aborting is a convincing reason
for this change on its own.
diff --git a/builtin/revert.c b/builtin/revert.c
index 62986a7b1b..00d3e19c62 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
return ret;
}
if (cmd == 'c')
- return sequencer_continue(the_repository, opts);
+ return sequencer_continue(the_repository, opts,
+ 0, NULL, NULL, NULL);
It's a bit unfortunate that we have to start passing all these extra
parameters, could the sequencer read them itself in read_populate_opts()?
-int sequencer_continue(struct repository *r, struct replay_opts *opts)
+static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags,
+ const char *onto_name, const struct object_id *onto,
+ const struct object_id *orig_head, struct todo_list *todo_list);
It would be nice to avoid this forward declaration. I think you could do
that by adding a preparatory patch that moves either checkout_onto() or
sequencer_continue()
@@ -6142,49 +6154,52 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
return error(_("nothing to do"));
} else if (res == EDIT_TODO_INCORRECT) {
- checkout_onto(r, opts, onto_name, onto, orig_head);
todo_list_release(&new_todo);
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)
- BUG("invalid todo list after expanding IDs:\n%s",
- new_todo.buf.buf);
I don't think we need to move this code. If start_rebase() is called
from sequencer_continue() the initial edit of the todo list failed and
has been fixed by running "git rebase --edit-todo". In that case the
oids have already been expanded on disc.
- if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) {
- todo_list_release(&new_todo);
- return error(_("could not skip unnecessary pick commands"));
- }
-
- if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,
- flags & ~(TODO_LIST_SHORTEN_IDS), action)) {
- todo_list_release(&new_todo);
- return error_errno(_("could not write '%s'"), todo_file);
- }
-
- res = -1;
-
- if (checkout_onto(r, opts, onto_name, onto, orig_head))
- goto cleanup;
-
- if (require_clean_work_tree(r, "rebase", NULL, 1, 1))
- goto cleanup;
-
- todo_list_write_total_nr(&new_todo);
- res = pick_commits(r, &new_todo, opts);
-
-cleanup:
+ res = start_rebase(r, opts, flags, onto_name, onto, orig_head, &new_todo);
todo_list_release(&new_todo);
return res;
}
+test_expect_success 'continue after bad first command' '
+ test_when_finished "git rebase --abort ||:" &&
+ git checkout primary^0 &&
If you want a specific commit it's better to use a tag name as those are
fixed whereas the branches get rebased all over the place in this test file.
+ git reflog expire --expire=all HEAD &&
Is this really necessary, can you pass -n to "git reflog" below?
+ (
+ set_fake_editor &&
+ test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \
+ git rebase -i HEAD~3 &&
+ test_cmp_rev HEAD primary &&
+ FAKE_LINES="pick 2 pick 3 reword 4" git rebase --edit-todo &&
+ FAKE_COMMIT_MESSAGE="E_reworded" git rebase --continue
+ ) &&
+ git reflog > reflog &&
+ test $(grep -c fast-forward reflog) = 1 &&
Using test_line_count would make test failures easier to debug.
+ test_cmp_rev HEAD~1 primary~1 &&
+ test "$(git log -1 --format=%B)" = "E_reworded"
It is slightly more work, but please use test_cmp for things like this
as it makes it so much easier to debug test failures.
Best Wishes
Phillip
+'
+
+test_expect_success 'abort after bad first command' '
+ test_when_finished "git rebase --abort ||:" &&
+ git checkout primary^0 &&
+ (
+ set_fake_editor &&
+ test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \
+ git rebase -i HEAD~3
+ ) &&
+ git rebase --abort &&
+ test_cmp_rev HEAD primary
+'
+
test_expect_success 'tabs and spaces are accepted in the todolist' '
rebase_setup_and_clean indented-comment &&
write_script add-indent.sh <<-\EOF &&
