From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
To: git@vger.kernel.org
Subject: [PATCH 8/8] rebase: improve resumption from incorrect initial todo list
Date: Thu, 23 Mar 2023 17:22:35 +0100 [thread overview]
Message-ID: <20230323162235.995574-9-oswald.buddenhagen@gmx.de> (raw)
In-Reply-To: <20230323162235.995574-1-oswald.buddenhagen@gmx.de>
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. This makes aborting cheaper and will simplify
things in a later change.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
builtin/rebase.c | 4 +-
builtin/revert.c | 3 +-
sequencer.c | 89 ++++++++++++++++++++---------------
sequencer.h | 4 +-
t/t3404-rebase-interactive.sh | 31 ++++++++++++
5 files changed, 91 insertions(+), 40 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index e703b29835..61e5363ac7 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -333,7 +333,9 @@ static int run_sequencer_rebase(struct rebase_options *opts)
case ACTION_CONTINUE: {
struct replay_opts replay_opts = get_replay_opts(opts);
- ret = sequencer_continue(the_repository, &replay_opts);
+ ret = sequencer_continue(the_repository, &replay_opts, flags,
+ opts->onto_name, &opts->onto->object.oid,
+ &opts->orig_head->object.oid);
replay_opts_release(&replay_opts);
break;
}
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);
if (cmd == 'a')
return sequencer_rollback(the_repository, opts);
if (cmd == 's')
diff --git a/sequencer.c b/sequencer.c
index aef42122f1..0b4d16b8e8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3369,7 +3369,7 @@ int sequencer_skip(struct repository *r, struct replay_opts *opts)
if (!is_directory(git_path_seq_dir()))
return 0;
- return sequencer_continue(r, opts);
+ return sequencer_continue(r, opts, 0, NULL, NULL, NULL);
give_advice:
error(_("there is nothing to skip"));
@@ -5096,7 +5096,13 @@ static int commit_staged_changes(struct repository *r,
return 0;
}
-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);
+
+int sequencer_continue(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 = TODO_LIST_INIT;
int res;
@@ -5117,6 +5123,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
unlink(rebase_path_dropped());
}
+ if (!todo_list.done_nr) {
+ res = start_rebase(r, opts, flags,
+ onto_name, onto,
+ orig_head, &todo_list);
+ goto release_todo_list;
+ }
+
opts->reflog_message = reflog_message(opts, "continue", NULL);
if (commit_staged_changes(r, opts, &todo_list)) {
res = -1;
@@ -6096,9 +6109,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
enum rebase_action action)
{
char shortonto[GIT_MAX_HEXSZ + 1];
- const char *todo_file = rebase_path_todo();
struct todo_list new_todo = TODO_LIST_INIT;
- struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
+ struct strbuf *buf = &todo_list->buf;
int res;
find_unique_abbrev_r(shortonto, onto, DEFAULT_ABBREV);
@@ -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);
-
- 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;
}
+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)
+{
+ const char *todo_file = rebase_path_todo();
+ struct strbuf buf2 = STRBUF_INIT;
+
+ /* Expand the commit IDs */
+ todo_list_to_strbuf(r, todo_list, &buf2, -1, 0);
+ strbuf_swap(&todo_list->buf, &buf2);
+ strbuf_release(&buf2);
+ todo_list->total_nr -= todo_list->nr;
+ if (todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) < 0)
+ BUG("invalid todo list after expanding IDs:\n%s",
+ todo_list->buf.buf);
+
+ if (opts->allow_ff && skip_unnecessary_picks(r, todo_list, &onto))
+ return error(_("could not skip unnecessary pick commands"));
+
+ if (todo_list_write_to_file(r, todo_list, todo_file, NULL, NULL, -1,
+ flags & ~(TODO_LIST_SHORTEN_IDS |
+ TODO_LIST_APPEND_TODO_HELP),
+ ACTION_CONTINUE))
+ return error_errno(_("could not write '%s'"), todo_file);
+
+ if (checkout_onto(r, opts, onto_name, onto, orig_head))
+ return -1;
+
+ if (require_clean_work_tree(r, "rebase", NULL, 1, 1))
+ return -1;
+
+ todo_list_write_total_nr(todo_list);
+ return pick_commits(r, todo_list, opts);
+}
+
struct subject2item_entry {
struct hashmap_entry entry;
int i;
diff --git a/sequencer.h b/sequencer.h
index 24bf71d5db..33bcff89e0 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -158,7 +158,9 @@ void todo_list_filter_update_refs(struct repository *r,
void sequencer_init_config(struct replay_opts *opts);
int sequencer_pick_revisions(struct repository *repo,
struct replay_opts *opts);
-int sequencer_continue(struct repository *repo, struct replay_opts *opts);
+int sequencer_continue(struct repository *repo, struct replay_opts *opts, unsigned flags,
+ const char *onto_name, const struct object_id *onto,
+ const struct object_id *orig_head);
int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
int sequencer_skip(struct repository *repo, struct replay_opts *opts);
void replay_opts_release(struct replay_opts *opts);
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c625aad10a..dd47f0bbce 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1597,6 +1597,37 @@ test_expect_success 'static check of bad command' '
test C = $(git cat-file commit HEAD^ | sed -ne \$p)
'
+
+test_expect_success 'continue after bad first command' '
+ test_when_finished "git rebase --abort ||:" &&
+ git checkout primary^0 &&
+ git reflog expire --expire=all HEAD &&
+ (
+ 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 &&
+ test_cmp_rev HEAD~1 primary~1 &&
+ test "$(git log -1 --format=%B)" = "E_reworded"
+'
+
+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 &&
--
2.40.0.152.g15d061e6df
next prev parent reply other threads:[~2023-03-23 16:47 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 1/8] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-03-23 19:40 ` Phillip Wood
2023-03-23 20:00 ` Junio C Hamano
2023-03-23 21:08 ` Felipe Contreras
2023-08-09 17:15 ` [PATCH v2 0/3] rebase refactoring Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-08-15 14:01 ` Phillip Wood
2023-10-20 9:36 ` [PATCH v3 0/3] rebase refactoring Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
2023-10-20 21:51 ` Junio C Hamano
2023-10-20 9:36 ` [PATCH v3 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-10-20 22:07 ` [PATCH v3 0/3] rebase refactoring Junio C Hamano
2023-10-23 15:43 ` Phillip Wood
2023-10-23 19:02 ` Junio C Hamano
2023-03-23 16:22 ` [PATCH 2/8] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-03-23 19:39 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 3/8] sequencer: pass around rebase action explicitly Oswald Buddenhagen
2023-03-23 19:27 ` Phillip Wood
2023-03-23 21:27 ` Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 4/8] sequencer: create enum for edit_todo_list() return value Oswald Buddenhagen
2023-03-23 19:27 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 5/8] rebase: preserve interactive todo file on checkout failure Oswald Buddenhagen
2023-03-23 19:31 ` Phillip Wood
2023-03-23 22:38 ` Oswald Buddenhagen
2023-03-24 14:15 ` Phillip Wood
2023-03-24 14:42 ` Oswald Buddenhagen
2023-03-23 20:16 ` Junio C Hamano
2023-03-23 23:23 ` Oswald Buddenhagen
2023-03-24 4:31 ` Junio C Hamano
2023-03-23 16:22 ` [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash() Oswald Buddenhagen
2023-03-23 19:46 ` Phillip Wood
2023-03-23 22:13 ` Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id Oswald Buddenhagen
2023-03-23 19:34 ` Phillip Wood
2023-03-23 21:36 ` Oswald Buddenhagen
2023-03-24 14:18 ` Phillip Wood
2023-03-23 16:22 ` Oswald Buddenhagen [this message]
2023-03-26 14:28 ` [PATCH 8/8] rebase: improve resumption from incorrect initial todo list Phillip Wood
2023-04-26 15:34 ` Oswald Buddenhagen
2023-05-17 12:13 ` Phillip Wood
2023-08-24 16:46 ` Oswald Buddenhagen
2023-03-23 19:38 ` [PATCH 0/8] sequencer refactoring Phillip Wood
2023-03-25 11:08 ` Phillip Wood
2023-04-06 12:09 ` Phillip Wood
2023-05-17 13:10 ` Phillip Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230323162235.995574-9-oswald.buddenhagen@gmx.de \
--to=oswald.buddenhagen@gmx.de \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).