git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 8/8] rebase: improve resumption from incorrect initial todo list

From
Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Date
Mar 23, 2023, 16:22 UTC
Message-ID
<20230323162235.995574-9-oswald.buddenhagen@gmx.de>
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
Previous: Oswald BuddenhagenNext: Phillip Wood
Message 2 of 49 in “sequencer refactoring”
  1. 0/8 sequencer refactoringOswald Buddenhagen, Mar 23, 2023
  2. 8/8 rebase: improve resumption from incorrect initial todo listOswald Buddenhagen, Mar 23, 2023
  3. Phillip WoodMar 26, 2023
  4. Oswald BuddenhagenApr 26, 2023
  5. Phillip WoodMay 17, 2023
  6. Oswald BuddenhagenAug 24, 2023
  7. 1/8 rebase: simplify code related to imply_merge()Oswald Buddenhagen, Mar 23, 2023
  8. Phillip WoodMar 23, 2023
  9. Junio C HamanoMar 23, 2023
  10. Felipe ContrerasMar 23, 2023
  11. 0/3 rebase refactoringOswald Buddenhagen, Aug 9, 2023
  12. 1/3 rebase: simplify code related to imply_merge()Oswald Buddenhagen, Aug 9, 2023
  13. 2/3 rebase: handle --strategy via imply_merge() as wellOswald Buddenhagen, Aug 9, 2023
  14. 3/3 rebase: move parse_opt_keep_empty() downOswald Buddenhagen, Aug 9, 2023
  15. Phillip WoodAug 15, 2023
  16. 0/3 rebase refactoringOswald Buddenhagen, Oct 20, 2023
  17. 2/3 rebase: handle --strategy via imply_merge() as wellOswald Buddenhagen, Oct 20, 2023
  18. Junio C HamanoOct 20, 2023
  19. 3/3 rebase: move parse_opt_keep_empty() downOswald Buddenhagen, Oct 20, 2023
  20. 1/3 rebase: simplify code related to imply_merge()Oswald Buddenhagen, Oct 20, 2023
  21. Junio C HamanoOct 20, 2023
  22. Phillip WoodOct 23, 2023
  23. Junio C HamanoOct 23, 2023
  24. 2/8 rebase: move parse_opt_keep_empty() downOswald Buddenhagen, Mar 23, 2023
  25. Phillip WoodMar 23, 2023
  26. 4/8 sequencer: create enum for edit_todo_list() return valueOswald Buddenhagen, Mar 23, 2023
  27. Phillip WoodMar 23, 2023
  28. 5/8 rebase: preserve interactive todo file on checkout failureOswald Buddenhagen, Mar 23, 2023
  29. Phillip WoodMar 23, 2023
  30. Oswald BuddenhagenMar 23, 2023
  31. Phillip WoodMar 24, 2023
  32. Oswald BuddenhagenMar 24, 2023
  33. Junio C HamanoMar 23, 2023
  34. Oswald BuddenhagenMar 23, 2023
  35. Junio C HamanoMar 24, 2023
  36. 3/8 sequencer: pass around rebase action explicitlyOswald Buddenhagen, Mar 23, 2023
  37. Phillip WoodMar 23, 2023
  38. Oswald BuddenhagenMar 23, 2023
  39. 6/8 sequencer: simplify allocation of result array in todo_list_rearrange_squash()Oswald Buddenhagen, Mar 23, 2023
  40. Phillip WoodMar 23, 2023
  41. Oswald BuddenhagenMar 23, 2023
  42. 7/8 sequencer: pass `onto` to complete_action() as object-idOswald Buddenhagen, Mar 23, 2023
  43. Phillip WoodMar 23, 2023
  44. Oswald BuddenhagenMar 23, 2023
  45. Phillip WoodMar 24, 2023
  46. Phillip WoodMar 23, 2023
  47. Phillip WoodMar 25, 2023
  48. Phillip WoodApr 6, 2023
  49. Phillip WoodMay 17, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.