{"thread":{"id":"59451","subject":"[PATCH 8/8] rebase: improve resumption from incorrect initial todo list","startedAt":"2023-03-23T16:47:29Z","lastAt":"2023-10-23T19:02:16Z","messageCount":49,"participants":["Oswald Buddenhagen","Phillip Wood","Junio C Hamano","Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"474003","messageId":"20230323162235.995574-9-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20230323162235.995574-1-oswald.buddenhagen@gmx.de","subject":"[PATCH 8/8] rebase: improve resumption from incorrect initial todo list","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:35Z","receivedAt":"2023-03-23T16:47:29Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"When the user butchers the todo file during rebase -i setup, the\n--continue which would follow --edit-todo would have skipped the last\nsteps of the setup. Notably, this would bypass the fast-forward over\nuntouched picks (though the actual picking loop would still fast-forward\nthe commits, one by one).\n\nFix this by splitting off the tail of complete_action() to a new\nstart_rebase() function and call that from sequencer_continue() when no\ncommands have been executed yet.\n\nMore or less as a side effect, we no longer checkout `onto` before exiting\nwhen the todo file is bad. This makes aborting cheaper and will simplify\nthings in a later change.\n\nSigned-off-by: Oswald Buddenhagen \n---\n builtin/rebase.c | 4 +-\n builtin/revert.c | 3 +-\n sequencer.c | 89 ++++++++++++++++++++---------------\n sequencer.h | 4 +-\n t/t3404-rebase-interactive.sh | 31 ++++++++++++\n 5 files changed, 91 insertions(+), 40 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e703b29835..61e5363ac7 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -333,7 +333,9 @@ static int run_sequencer_rebase(struct rebase_options *opts)\n \tcase ACTION_CONTINUE: {\n \t\tstruct replay_opts replay_opts = get_replay_opts(opts);\n \n-\t\tret = sequencer_continue(the_repository, &replay_opts);\n+\t\tret = sequencer_continue(the_repository, &replay_opts, flags,\n+\t\t\t\t\t opts->onto_name, &opts->onto->object.oid,\n+\t\t\t\t\t &opts->orig_head->object.oid);\n \t\treplay_opts_release(&replay_opts);\n \t\tbreak;\n \t}\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 62986a7b1b..00d3e19c62 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\treturn ret;\n \t}\n \tif (cmd == 'c')\n-\t\treturn sequencer_continue(the_repository, opts);\n+\t\treturn sequencer_continue(the_repository, opts,\n+\t\t\t\t\t 0, NULL, NULL, NULL);\n \tif (cmd == 'a')\n \t\treturn sequencer_rollback(the_repository, opts);\n \tif (cmd == 's')\ndiff --git a/sequencer.c b/sequencer.c\nindex aef42122f1..0b4d16b8e8 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3369,7 +3369,7 @@ int sequencer_skip(struct repository *r, struct replay_opts *opts)\n \tif (!is_directory(git_path_seq_dir()))\n \t\treturn 0;\n \n-\treturn sequencer_continue(r, opts);\n+\treturn sequencer_continue(r, opts, 0, NULL, NULL, NULL);\n \n give_advice:\n \terror(_(\"there is nothing to skip\"));\n@@ -5096,7 +5096,13 @@ static int commit_staged_changes(struct repository *r,\n \treturn 0;\n }\n \n-int sequencer_continue(struct repository *r, struct replay_opts *opts)\n+static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags,\n+\t\t\tconst char *onto_name, const struct object_id *onto,\n+\t\t\tconst struct object_id *orig_head, struct todo_list *todo_list);\n+\n+int sequencer_continue(struct repository *r, struct replay_opts *opts, unsigned flags,\n+\t\t const char *onto_name, const struct object_id *onto,\n+\t\t const struct object_id *orig_head)\n {\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n \tint res;\n@@ -5117,6 +5123,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \t\t\tunlink(rebase_path_dropped());\n \t\t}\n \n+\t\tif (!todo_list.done_nr) {\n+\t\t\tres = start_rebase(r, opts, flags,\n+\t\t\t\t\t onto_name, onto,\n+\t\t\t\t\t orig_head, &todo_list);\n+\t\t\tgoto release_todo_list;\n+\t\t}\n+\n \t\topts->reflog_message = reflog_message(opts, \"continue\", NULL);\n \t\tif (commit_staged_changes(r, opts, &todo_list)) {\n \t\t\tres = -1;\n@@ -6096,9 +6109,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\t enum rebase_action action)\n {\n \tchar shortonto[GIT_MAX_HEXSZ + 1];\n-\tconst char *todo_file = rebase_path_todo();\n \tstruct todo_list new_todo = TODO_LIST_INIT;\n-\tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n+\tstruct strbuf *buf = &todo_list->buf;\n \tint res;\n \n \tfind_unique_abbrev_r(shortonto, onto, DEFAULT_ABBREV);\n@@ -6142,49 +6154,52 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \n \t\treturn error(_(\"nothing to do\"));\n \t} else if (res == EDIT_TODO_INCORRECT) {\n-\t\tcheckout_onto(r, opts, onto_name, onto, orig_head);\n \t\ttodo_list_release(&new_todo);\n \n \t\treturn -1;\n \t}\n \n-\t/* Expand the commit IDs */\n-\ttodo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);\n-\tstrbuf_swap(&new_todo.buf, &buf2);\n-\tstrbuf_release(&buf2);\n-\tnew_todo.total_nr -= new_todo.nr;\n-\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n-\t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n-\t\t new_todo.buf.buf);\n-\n-\tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) {\n-\t\ttodo_list_release(&new_todo);\n-\t\treturn error(_(\"could not skip unnecessary pick commands\"));\n-\t}\n-\n-\tif (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,\n-\t\t\t\t flags & ~(TODO_LIST_SHORTEN_IDS), action)) {\n-\t\ttodo_list_release(&new_todo);\n-\t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n-\t}\n-\n-\tres = -1;\n-\n-\tif (checkout_onto(r, opts, onto_name, onto, orig_head))\n-\t\tgoto cleanup;\n-\n-\tif (require_clean_work_tree(r, \"rebase\", NULL, 1, 1))\n-\t\tgoto cleanup;\n-\n-\ttodo_list_write_total_nr(&new_todo);\n-\tres = pick_commits(r, &new_todo, opts);\n-\n-cleanup:\n+\tres = start_rebase(r, opts, flags, onto_name, onto, orig_head, &new_todo);\n \ttodo_list_release(&new_todo);\n \n \treturn res;\n }\n \n+static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags,\n+\t\t\tconst char *onto_name, const struct object_id *onto,\n+\t\t\tconst struct object_id *orig_head, struct todo_list *todo_list)\n+{\n+\tconst char *todo_file = rebase_path_todo();\n+\tstruct strbuf buf2 = STRBUF_INIT;\n+\n+\t/* Expand the commit IDs */\n+\ttodo_list_to_strbuf(r, todo_list, &buf2, -1, 0);\n+\tstrbuf_swap(&todo_list->buf, &buf2);\n+\tstrbuf_release(&buf2);\n+\ttodo_list->total_nr -= todo_list->nr;\n+\tif (todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) < 0)\n+\t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n+\t\t todo_list->buf.buf);\n+\n+\tif (opts->allow_ff && skip_unnecessary_picks(r, todo_list, &onto))\n+\t\treturn error(_(\"could not skip unnecessary pick commands\"));\n+\n+\tif (todo_list_write_to_file(r, todo_list, todo_file, NULL, NULL, -1,\n+\t\t\t\t flags & ~(TODO_LIST_SHORTEN_IDS |\n+\t\t\t\t\t TODO_LIST_APPEND_TODO_HELP),\n+\t\t\t\t ACTION_CONTINUE))\n+\t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n+\n+\tif (checkout_onto(r, opts, onto_name, onto, orig_head))\n+\t\treturn -1;\n+\n+\tif (require_clean_work_tree(r, \"rebase\", NULL, 1, 1))\n+\t\treturn -1;\n+\n+\ttodo_list_write_total_nr(todo_list);\n+\treturn pick_commits(r, todo_list, opts);\n+}\n+\n struct subject2item_entry {\n \tstruct hashmap_entry entry;\n \tint i;\ndiff --git a/sequencer.h b/sequencer.h\nindex 24bf71d5db..33bcff89e0 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -158,7 +158,9 @@ void todo_list_filter_update_refs(struct repository *r,\n void sequencer_init_config(struct replay_opts *opts);\n int sequencer_pick_revisions(struct repository *repo,\n \t\t\t struct replay_opts *opts);\n-int sequencer_continue(struct repository *repo, struct replay_opts *opts);\n+int sequencer_continue(struct repository *repo, struct replay_opts *opts, unsigned flags,\n+\t\t const char *onto_name, const struct object_id *onto,\n+\t\t const struct object_id *orig_head);\n int sequencer_rollback(struct repository *repo, struct replay_opts *opts);\n int sequencer_skip(struct repository *repo, struct replay_opts *opts);\n void replay_opts_release(struct replay_opts *opts);\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex c625aad10a..dd47f0bbce 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1597,6 +1597,37 @@ test_expect_success 'static check of bad command' '\n \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+\n+test_expect_success 'continue after bad first command' '\n+\ttest_when_finished \"git rebase --abort ||:\" &&\n+\tgit checkout primary^0 &&\n+\tgit reflog expire --expire=all HEAD &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"bad 1 pick 1 pick 2 reword 3\" \\\n+\t\t\tgit rebase -i HEAD~3 &&\n+\t\ttest_cmp_rev HEAD primary &&\n+\t\tFAKE_LINES=\"pick 2 pick 3 reword 4\" git rebase --edit-todo &&\n+\t\tFAKE_COMMIT_MESSAGE=\"E_reworded\" git rebase --continue\n+\t) &&\n+\tgit reflog > reflog &&\n+\ttest $(grep -c fast-forward reflog) = 1 &&\n+\ttest_cmp_rev HEAD~1 primary~1 &&\n+\ttest \"$(git log -1 --format=%B)\" = \"E_reworded\"\n+'\n+\n+test_expect_success 'abort after bad first command' '\n+\ttest_when_finished \"git rebase --abort ||:\" &&\n+\tgit checkout primary^0 &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_LINES=\"bad 1 pick 1 pick 2 reword 3\" \\\n+\t\t\tgit rebase -i HEAD~3\n+\t) &&\n+\tgit rebase --abort &&\n+\ttest_cmp_rev HEAD primary\n+'\n+\n test_expect_success 'tabs and spaces are accepted in the todolist' '\n \trebase_setup_and_clean indented-comment &&\n \twrite_script add-indent.sh <<-\\EOF &&\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"474004","messageId":"20230323162235.995574-2-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20230323162235.995574-1-oswald.buddenhagen@gmx.de","subject":"[PATCH 1/8] rebase: simplify code related to imply_merge()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:28Z","receivedAt":"2023-03-23T16:47:32Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"The code's evolution left in some bits surrounding enum rebase_type that\ndon't really make sense any more. In particular, it makes no sense to\ninvoke imply_merge() if the type is already known not to be\nREBASE_APPLY, and it makes no sense to assign the type after calling\nimply_merge().\n\nSigned-off-by: Oswald Buddenhagen \n---\n builtin/rebase.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 5b7b908b66..8ffea0f0d8 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -372,7 +372,6 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n \n \timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n \topts->keep_empty = !unset;\n-\topts->type = REBASE_MERGE;\n \treturn 0;\n }\n \n@@ -1494,9 +1493,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (options.type == REBASE_MERGE)\n-\t\timply_merge(&options, \"--merge\");\n-\n \tif (options.root && !options.onto_name)\n \t\timply_merge(&options, \"--root without --onto\");\n \n@@ -1534,7 +1530,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \tif (options.type == REBASE_UNSPECIFIED) {\n \t\tif (!strcmp(options.default_backend, \"merge\"))\n-\t\t\timply_merge(&options, \"--merge\");\n+\t\t\toptions.type = REBASE_MERGE;\n \t\telse if (!strcmp(options.default_backend, \"apply\"))\n \t\t\toptions.type = REBASE_APPLY;\n \t\telse\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"474005","messageId":"20230323162235.995574-3-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20230323162235.995574-1-oswald.buddenhagen@gmx.de","subject":"[PATCH 2/8] rebase: move parse_opt_keep_empty() down","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:29Z","receivedAt":"2023-03-23T16:47:33Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"This moves it right next to parse_opt_empty(), which is a much more\nlogical place. As a side effect, this removes the need for a forward\ndeclaration of imply_merge().\n\nSigned-off-by: Oswald Buddenhagen \n---\n builtin/rebase.c | 25 ++++++++++++-------------\n 1 file changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 8ffea0f0d8..491759db19 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -362,19 +362,6 @@ static int run_sequencer_rebase(struct rebase_options *opts)\n \treturn ret;\n }\n \n-static void imply_merge(struct rebase_options *opts, const char *option);\n-static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n-\t\t\t\tint unset)\n-{\n-\tstruct rebase_options *opts = opt->value;\n-\n-\tBUG_ON_OPT_ARG(arg);\n-\n-\timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n-\topts->keep_empty = !unset;\n-\treturn 0;\n-}\n-\n static int is_merge(struct rebase_options *opts)\n {\n \treturn opts->type == REBASE_MERGE;\n@@ -969,6 +956,18 @@ static enum empty_type parse_empty_value(const char *value)\n \tdie(_(\"unrecognized empty type '%s'; valid values are \\\"drop\\\", \\\"keep\\\", and \\\"ask\\\".\"), value);\n }\n \n+static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n+\t\t\t\tint unset)\n+{\n+\tstruct rebase_options *opts = opt->value;\n+\n+\tBUG_ON_OPT_ARG(arg);\n+\n+\timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n+\topts->keep_empty = !unset;\n+\treturn 0;\n+}\n+\n static int parse_opt_empty(const struct option *opt, const char *arg, int unset)\n {\n \tstruct rebase_options *options = opt->value;\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"474007","messageId":"20230323162235.995574-1-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":null,"subject":"[PATCH 0/8] sequencer refactoring","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:27Z","receivedAt":"2023-03-23T16:47:37Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"This is a preparatory series for the separately posted 'rebase --rewind' patch,\nbut I think it has value in itself.\n\n\nOswald Buddenhagen (8):\n rebase: simplify code related to imply_merge()\n rebase: move parse_opt_keep_empty() down\n sequencer: pass around rebase action explicitly\n sequencer: create enum for edit_todo_list() return value\n rebase: preserve interactive todo file on checkout failure\n sequencer: simplify allocation of result array in\n todo_list_rearrange_squash()\n sequencer: pass `onto` to complete_action() as object-id\n rebase: improve resumption from incorrect initial todo list\n\n builtin/rebase.c | 63 +++++++--------\n builtin/revert.c | 3 +-\n rebase-interactive.c | 36 ++++-----\n rebase-interactive.h | 27 ++++++-\n sequencer.c | 139 +++++++++++++++++++---------------\n sequencer.h | 15 ++--\n t/t3404-rebase-interactive.sh | 34 ++++++++-\n 7 files changed, 196 insertions(+), 121 deletions(-)\n\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"474013","messageId":"20230323162235.995574-5-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20230323162235.995574-1-oswald.buddenhagen@gmx.de","subject":"[PATCH 4/8] sequencer: create enum for edit_todo_list() return value","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:31Z","receivedAt":"2023-03-23T16:47:53Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"This is a lot cleaner than open-coding magic numbers.\n\nSigned-off-by: Oswald Buddenhagen \n---\n rebase-interactive.c | 15 ++++++++-------\n rebase-interactive.h | 11 ++++++++++-\n sequencer.c | 8 ++++----\n 3 files changed, 22 insertions(+), 12 deletions(-)\n\ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex 111a2071ae..a3d8925b06 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -94,7 +94,8 @@ void append_todo_help(int command_count, enum rebase_action action,\n \tstrbuf_add_commented_lines(buf, msg, strlen(msg));\n }\n \n-int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n+enum edit_todo_result edit_todo_list(\n+\t\t struct repository *r, struct todo_list *todo_list,\n \t\t struct todo_list *new_todo, const char *shortrevisions,\n \t\t const char *shortonto, unsigned flags,\n \t\t enum rebase_action action)\n@@ -123,37 +124,37 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n \t\treturn error(_(\"could not write '%s'.\"), rebase_path_todo_backup());\n \n \tif (launch_sequence_editor(todo_file, &new_todo->buf, NULL))\n-\t\treturn -2;\n+\t\treturn EDIT_TODO_FAILED;\n \n \tstrbuf_stripspace(&new_todo->buf, 1);\n \tif (action != ACTION_EDIT_TODO && new_todo->buf.len == 0)\n-\t\treturn -3;\n+\t\treturn EDIT_TODO_ABORT;\n \n \tif (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {\n \t\tfprintf(stderr, _(edit_todo_list_advice));\n-\t\treturn -4;\n+\t\treturn EDIT_TODO_INCORRECT;\n \t}\n \n \tif (incorrect) {\n \t\tif (todo_list_check_against_backup(r, new_todo)) {\n \t\t\twrite_file(rebase_path_dropped(), \"%s\", \"\");\n-\t\t\treturn -4;\n+\t\t\treturn EDIT_TODO_INCORRECT;\n \t\t}\n \n \t\tif (incorrect > 0)\n \t\t\tunlink(rebase_path_dropped());\n \t} else if (todo_list_check(todo_list, new_todo)) {\n \t\twrite_file(rebase_path_dropped(), \"%s\", \"\");\n-\t\treturn -4;\n+\t\treturn EDIT_TODO_INCORRECT;\n \t}\n \n \t/*\n \t * See if branches need to be added or removed from the update-refs\n \t * file based on the new todo list.\n \t */\n \ttodo_list_filter_update_refs(r, new_todo);\n \n-\treturn 0;\n+\treturn EDIT_TODO_OK;\n }\n \n define_commit_slab(commit_seen, unsigned char);\ndiff --git a/rebase-interactive.h b/rebase-interactive.h\nindex d9873d3497..5aa4111b4f 100644\n--- a/rebase-interactive.h\n+++ b/rebase-interactive.h\n@@ -16,10 +16,19 @@ enum rebase_action {\n \tACTION_LAST\n };\n \n+enum edit_todo_result {\n+\tEDIT_TODO_OK = 0, // must be 0\n+\tEDIT_TODO_IOERROR = -1, // generic i/o error; must be -1\n+\tEDIT_TODO_FAILED = -2, // editing failed\n+\tEDIT_TODO_ABORT = -3, // user requested abort\n+\tEDIT_TODO_INCORRECT = -4 // file violates syntax or constraints\n+};\n+\n void append_todo_help(int command_count, enum rebase_action action,\n \t\t const char *shortrevisions, const char *shortonto,\n \t\t struct strbuf *buf);\n-int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n+enum edit_todo_result edit_todo_list(\n+\t\t struct repository *r, struct todo_list *todo_list,\n \t\t struct todo_list *new_todo, const char *shortrevisions,\n \t\t const char *shortonto, unsigned flags,\n \t\t enum rebase_action action);\ndiff --git a/sequencer.c b/sequencer.c\nindex f05174d151..b1c29c8802 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6125,20 +6125,20 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \n \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n \t\t\t shortonto, flags, action);\n-\tif (res == -1)\n+\tif (res == EDIT_TODO_IOERROR)\n \t\treturn -1;\n-\telse if (res == -2) {\n+\telse if (res == EDIT_TODO_FAILED) {\n \t\tapply_autostash(rebase_path_autostash());\n \t\tsequencer_remove_state(opts);\n \n \t\treturn -1;\n-\t} else if (res == -3) {\n+\t} else if (res == EDIT_TODO_ABORT) {\n \t\tapply_autostash(rebase_path_autostash());\n \t\tsequencer_remove_state(opts);\n \t\ttodo_list_release(&new_todo);\n \n \t\treturn error(_(\"nothing to do\"));\n-\t} else if (res == -4) {\n+\t} else if (res == EDIT_TODO_INCORRECT) {\n \t\tcheckout_onto(r, opts, onto_name, &onto->object.oid, orig_head);\n \t\ttodo_list_release(&new_todo);\n \n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"474014","messageId":"20230323162235.995574-6-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20230323162235.995574-1-oswald.buddenhagen@gmx.de","subject":"[PATCH 5/8] rebase: preserve interactive todo file on checkout failure","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:32Z","receivedAt":"2023-03-23T16:47:54Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"Creating a suitable todo file is a potentially labor-intensive process,\nso be less cavalier about discarding it when something goes wrong (e.g.,\nthe user messed with the repo while editing the todo).\n\nSigned-off-by: Oswald Buddenhagen \n---\n builtin/rebase.c | 1 +\n sequencer.c | 4 ++++\n sequencer.h | 1 +\n t/t3404-rebase-interactive.sh | 3 ++-\n 4 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex a309addd50..728c869db4 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -153,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \treplay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);\n \treplay.quiet = !(opts->flags & REBASE_NO_QUIET);\n \treplay.verbose = opts->flags & REBASE_VERBOSE;\n+\treplay.precious_todo = opts->flags & REBASE_INTERACTIVE_EXPLICIT;\n \treplay.reschedule_failed_exec = opts->reschedule_failed_exec;\n \treplay.committer_date_is_author_date =\n \t\t\t\t\topts->committer_date_is_author_date;\ndiff --git a/sequencer.c b/sequencer.c\nindex b1c29c8802..f8a7f4e721 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4570,6 +4570,10 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,\n \t\t.default_reflog_action = sequencer_reflog_action(opts)\n \t};\n \tif (reset_head(r, &ropts)) {\n+\t\t// Editing the todo may have been costly; don't just discard it.\n+\t\tif (opts->precious_todo)\n+\t\t\texit(1); // Error was already printed\n+\n \t\tapply_autostash(rebase_path_autostash());\n \t\tsequencer_remove_state(opts);\n \t\treturn error(_(\"could not detach HEAD\"));\ndiff --git a/sequencer.h b/sequencer.h\nindex 1a3e616af2..a1b8ca6eb1 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -47,6 +47,7 @@ struct replay_opts {\n \tint keep_redundant_commits;\n \tint verbose;\n \tint quiet;\n+\tint precious_todo;\n \tint reschedule_failed_exec;\n \tint committer_date_is_author_date;\n \tint ignore_date;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ff0afad63e..c625aad10a 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -288,13 +288,14 @@ test_expect_success 'abort' '\n '\n \n test_expect_success 'abort with error when new base cannot be checked out' '\n+\ttest_when_finished \"git rebase --abort ||:\" &&\n \tgit rm --cached file1 &&\n \tgit commit -m \"remove file in base\" &&\n \ttest_must_fail git rebase -i primary > output 2>&1 &&\n \ttest_i18ngrep \"The following untracked working tree files would be overwritten by checkout:\" \\\n \t\toutput &&\n \ttest_i18ngrep \"file1\" output &&\n-\ttest_path_is_missing .git/rebase-merge &&\n+\ttest_path_is_dir .git/rebase-merge &&\n \trm file1 &&\n \tgit reset --hard HEAD^\n '\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"474015","messageId":"20230323162235.995574-4-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20230323162235.995574-1-oswald.buddenhagen@gmx.de","subject":"[PATCH 3/8] sequencer: pass around rebase action explicitly","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:30Z","receivedAt":"2023-03-23T16:48:02Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"... instead of deriving it from other arguments. This is a lot cleaner\nand more extensible.\n\nSigned-off-by: Oswald Buddenhagen \n---\n builtin/rebase.c | 25 ++++++++++---------------\n rebase-interactive.c | 21 +++++++++++----------\n rebase-interactive.h | 16 ++++++++++++++--\n sequencer.c | 16 +++++++++-------\n sequencer.h | 8 +++++---\n 5 files changed, 49 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 491759db19..a309addd50 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -58,16 +58,6 @@ enum empty_type {\n \tEMPTY_ASK\n };\n \n-enum action {\n-\tACTION_NONE = 0,\n-\tACTION_CONTINUE,\n-\tACTION_SKIP,\n-\tACTION_ABORT,\n-\tACTION_QUIT,\n-\tACTION_EDIT_TODO,\n-\tACTION_SHOW_CURRENT_PATCH\n-};\n-\n static const char *action_names[] = {\n \t\"undefined\",\n \t\"continue\",\n@@ -104,7 +94,7 @@ struct rebase_options {\n \t\tREBASE_INTERACTIVE_EXPLICIT = 1<<4,\n \t} flags;\n \tstruct strvec git_am_opts;\n-\tenum action action;\n+\tenum rebase_action action;\n \tchar *reflog_action;\n \tint signoff;\n \tint allow_rerere_autoupdate;\n@@ -198,9 +188,11 @@ static int edit_todo_file(unsigned flags)\n \t\treturn error_errno(_(\"could not read '%s'.\"), todo_file);\n \n \tstrbuf_stripspace(&todo_list.buf, 1);\n-\tres = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags);\n+\tres = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags,\n+\t\t\t ACTION_EDIT_TODO);\n \tif (!res && todo_list_write_to_file(the_repository, &new_todo, todo_file,\n-\t\t\t\t\t NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS)))\n+\t\t\t\t\t NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS),\n+\t\t\t\t\t ACTION_EDIT_TODO))\n \t\tres = error_errno(_(\"could not write '%s'\"), todo_file);\n \n \ttodo_list_release(&todo_list);\n@@ -294,7 +286,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \t\tret = complete_action(the_repository, &replay, flags,\n \t\t\tshortrevisions, opts->onto_name, opts->onto,\n \t\t\t&opts->orig_head->object.oid, &opts->exec,\n-\t\t\topts->autosquash, opts->update_refs, &todo_list);\n+\t\t\topts->autosquash, opts->update_refs, &todo_list,\n+\t\t\topts->action);\n \t}\n \n cleanup:\n@@ -1246,7 +1239,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\telse if (options.exec.nr)\n \t\t\ttrace2_cmd_mode(\"interactive-exec\");\n \t\telse\n-\t\t\ttrace2_cmd_mode(action_names[options.action]);\n+\t\t\ttrace2_cmd_mode(\n+\t\t\t\t(BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(action_names) == ACTION_LAST),\n+\t\t\t\t action_names[options.action]));\n \t}\n \n \toptions.reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex 7407c59319..111a2071ae 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -35,7 +35,7 @@ static enum missing_commit_check_level get_missing_commit_check_level(void)\n \treturn MISSING_COMMIT_CHECK_IGNORE;\n }\n \n-void append_todo_help(int command_count,\n+void append_todo_help(int command_count, enum rebase_action action,\n \t\t const char *shortrevisions, const char *shortonto,\n \t\t struct strbuf *buf)\n {\n@@ -62,9 +62,8 @@ void append_todo_help(int command_count,\n \" updated at the end of the rebase\\n\"\n \"\\n\"\n \"These lines can be re-ordered; they are executed from top to bottom.\\n\");\n-\tunsigned edit_todo = !(shortrevisions && shortonto);\n \n-\tif (!edit_todo) {\n+\tif (action == ACTION_NONE) {\n \t\tstrbuf_addch(buf, '\\n');\n \t\tstrbuf_commented_addf(buf, Q_(\"Rebase %s onto %s (%d command)\",\n \t\t\t\t\t \"Rebase %s onto %s (%d commands)\",\n@@ -83,7 +82,7 @@ void append_todo_help(int command_count,\n \n \tstrbuf_add_commented_lines(buf, msg, strlen(msg));\n \n-\tif (edit_todo)\n+\tif (action == ACTION_EDIT_TODO)\n \t\tmsg = _(\"\\nYou are editing the todo file \"\n \t\t\t\"of an ongoing interactive rebase.\\n\"\n \t\t\t\"To continue rebase after editing, run:\\n\"\n@@ -97,35 +96,37 @@ void append_todo_help(int command_count,\n \n int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n \t\t struct todo_list *new_todo, const char *shortrevisions,\n-\t\t const char *shortonto, unsigned flags)\n+\t\t const char *shortonto, unsigned flags,\n+\t\t enum rebase_action action)\n {\n \tconst char *todo_file = rebase_path_todo(),\n \t\t*todo_backup = rebase_path_todo_backup();\n-\tunsigned initial = shortrevisions && shortonto;\n \tint incorrect = 0;\n \n \t/* If the user is editing the todo list, we first try to parse\n \t * it. If there is an error, we do not return, because the user\n \t * might want to fix it in the first place. */\n-\tif (!initial)\n+\tif (action != ACTION_NONE)\n \t\tincorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |\n \t\t\tfile_exists(rebase_path_dropped());\n \n \tif (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,\n-\t\t\t\t -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))\n+\t\t\t\t -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP,\n+\t\t\t\t action))\n \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n \n \tif (!incorrect &&\n \t todo_list_write_to_file(r, todo_list, todo_backup,\n \t\t\t\t shortrevisions, shortonto, -1,\n-\t\t\t\t (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)\n+\t\t\t\t (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS,\n+\t\t\t\t action) < 0)\n \t\treturn error(_(\"could not write '%s'.\"), rebase_path_todo_backup());\n \n \tif (launch_sequence_editor(todo_file, &new_todo->buf, NULL))\n \t\treturn -2;\n \n \tstrbuf_stripspace(&new_todo->buf, 1);\n-\tif (initial && new_todo->buf.len == 0)\n+\tif (action != ACTION_EDIT_TODO && new_todo->buf.len == 0)\n \t\treturn -3;\n \n \tif (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {\ndiff --git a/rebase-interactive.h b/rebase-interactive.h\nindex 7239c60f79..d9873d3497 100644\n--- a/rebase-interactive.h\n+++ b/rebase-interactive.h\n@@ -5,12 +5,24 @@ struct strbuf;\n struct repository;\n struct todo_list;\n \n-void append_todo_help(int command_count,\n+enum rebase_action {\n+\tACTION_NONE = 0,\n+\tACTION_CONTINUE,\n+\tACTION_SKIP,\n+\tACTION_ABORT,\n+\tACTION_QUIT,\n+\tACTION_EDIT_TODO,\n+\tACTION_SHOW_CURRENT_PATCH,\n+\tACTION_LAST\n+};\n+\n+void append_todo_help(int command_count, enum rebase_action action,\n \t\t const char *shortrevisions, const char *shortonto,\n \t\t struct strbuf *buf);\n int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n \t\t struct todo_list *new_todo, const char *shortrevisions,\n-\t\t const char *shortonto, unsigned flags);\n+\t\t const char *shortonto, unsigned flags,\n+\t\t enum rebase_action action);\n \n int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);\n int todo_list_check_against_backup(struct repository *r,\ndiff --git a/sequencer.c b/sequencer.c\nindex 7c275c9a65..f05174d151 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5894,14 +5894,15 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis\n \n int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,\n \t\t\t const char *file, const char *shortrevisions,\n-\t\t\t const char *shortonto, int num, unsigned flags)\n+\t\t\t const char *shortonto, int num, unsigned flags,\n+\t\t\t enum rebase_action action)\n {\n \tint res;\n \tstruct strbuf buf = STRBUF_INIT;\n \n \ttodo_list_to_strbuf(r, todo_list, &buf, num, flags);\n \tif (flags & TODO_LIST_APPEND_TODO_HELP)\n-\t\tappend_todo_help(count_commands(todo_list),\n+\t\tappend_todo_help(count_commands(todo_list), action,\n \t\t\t\t shortrevisions, shortonto, &buf);\n \n \tres = write_message(buf.buf, buf.len, file, 0);\n@@ -5941,7 +5942,8 @@ static int skip_unnecessary_picks(struct repository *r,\n \tif (i > 0) {\n \t\tconst char *done_path = rebase_path_done();\n \n-\t\tif (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0)) {\n+\t\tif (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0,\n+\t\t\t\t\t ACTION_NONE)) {\n \t\t\terror_errno(_(\"could not write to '%s'\"), done_path);\n \t\t\treturn -1;\n \t\t}\n@@ -6086,8 +6088,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\t const char *shortrevisions, const char *onto_name,\n \t\t struct commit *onto, const struct object_id *orig_head,\n \t\t struct string_list *commands, unsigned autosquash,\n-\t\t unsigned update_refs,\n-\t\t struct todo_list *todo_list)\n+\t\t unsigned update_refs, struct todo_list *todo_list,\n+\t\t enum rebase_action action)\n {\n \tchar shortonto[GIT_MAX_HEXSZ + 1];\n \tconst char *todo_file = rebase_path_todo();\n@@ -6122,7 +6124,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t}\n \n \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n-\t\t\t shortonto, flags);\n+\t\t\t shortonto, flags, action);\n \tif (res == -1)\n \t\treturn -1;\n \telse if (res == -2) {\n@@ -6158,7 +6160,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t}\n \n \tif (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,\n-\t\t\t\t flags & ~(TODO_LIST_SHORTEN_IDS))) {\n+\t\t\t\t flags & ~(TODO_LIST_SHORTEN_IDS), action)) {\n \t\ttodo_list_release(&new_todo);\n \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n \t}\ndiff --git a/sequencer.h b/sequencer.h\nindex 33dbaf5b66..1a3e616af2 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -4,6 +4,7 @@\n #include \"strbuf.h\"\n #include \"wt-status.h\"\n \n+enum rebase_action;\n struct commit;\n struct index_state;\n struct repository;\n@@ -134,7 +135,8 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n \t\t\t\tstruct todo_list *todo_list);\n int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,\n \t\t\t const char *file, const char *shortrevisions,\n-\t\t\t const char *shortonto, int num, unsigned flags);\n+\t\t\t const char *shortonto, int num, unsigned flags,\n+\t\t\t enum rebase_action action);\n void todo_list_release(struct todo_list *todo_list);\n const char *todo_item_get_arg(struct todo_list *todo_list,\n \t\t\t struct todo_item *item);\n@@ -187,8 +189,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\t const char *shortrevisions, const char *onto_name,\n \t\t struct commit *onto, const struct object_id *orig_head,\n \t\t struct string_list *commands, unsigned autosquash,\n-\t\t unsigned update_refs,\n-\t\t struct todo_list *todo_list);\n+\t\t unsigned update_refs, struct todo_list *todo_list,\n+\t\t enum rebase_action action);\n int todo_list_rearrange_squash(struct todo_list *todo_list);\n \n /*\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"474016","messageId":"20230323162235.995574-7-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20230323162235.995574-1-oswald.buddenhagen@gmx.de","subject":"[PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:33Z","receivedAt":"2023-03-23T16:48:03Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"The operation doesn't change the number of elements in the array, so we do\nnot need to allocate the result piecewise.\n\nSigned-off-by: Oswald Buddenhagen \n---\n sequencer.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f8a7f4e721..fb224445fa 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6225,7 +6225,7 @@ static int skip_fixupish(const char *subject, const char **p) {\n int todo_list_rearrange_squash(struct todo_list *todo_list)\n {\n \tstruct hashmap subject2item;\n-\tint rearranged = 0, *next, *tail, i, nr = 0, alloc = 0;\n+\tint rearranged = 0, *next, *tail, i, nr = 0;\n \tchar **subjects;\n \tstruct commit_todo_item commit_todo;\n \tstruct todo_item *items = NULL;\n@@ -6334,6 +6334,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t}\n \n \tif (rearranged) {\n+\t\titems = ALLOC_ARRAY(items, todo_list->nr);\n+\n \t\tfor (i = 0; i < todo_list->nr; i++) {\n \t\t\tenum todo_command command = todo_list->items[i].command;\n \t\t\tint cur = i;\n@@ -6346,16 +6348,15 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t\t\t\tcontinue;\n \n \t\t\twhile (cur >= 0) {\n-\t\t\t\tALLOC_GROW(items, nr + 1, alloc);\n \t\t\t\titems[nr++] = todo_list->items[cur];\n \t\t\t\tcur = next[cur];\n \t\t\t}\n \t\t}\n \n+\t\tassert(nr == todo_list->nr);\n+\t\ttodo_list->alloc = nr;\n \t\tFREE_AND_NULL(todo_list->items);\n \t\ttodo_list->items = items;\n-\t\ttodo_list->nr = nr;\n-\t\ttodo_list->alloc = alloc;\n \t}\n \n \tfree(next);\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"474017","messageId":"20230323162235.995574-8-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20230323162235.995574-1-oswald.buddenhagen@gmx.de","subject":"[PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:34Z","receivedAt":"2023-03-23T16:48:05Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"... instead of as a commit, which makes the purpose clearer and will\nsimplify things later.\n\nAs a side effect, this change revealed that skip_unnecessary_picks() was\nbutchering the commit object due to missing const-correctness. Slightly\nadjust its API to rectify this.\n\nSigned-off-by: Oswald Buddenhagen \n---\n builtin/rebase.c | 2 +-\n sequencer.c | 21 ++++++++++-----------\n sequencer.h | 2 +-\n 3 files changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 728c869db4..e703b29835 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -285,7 +285,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \t\t\tBUG(\"unusable todo list\");\n \n \t\tret = complete_action(the_repository, &replay, flags,\n-\t\t\tshortrevisions, opts->onto_name, opts->onto,\n+\t\t\tshortrevisions, opts->onto_name, &opts->onto->object.oid,\n \t\t\t&opts->orig_head->object.oid, &opts->exec,\n \t\t\topts->autosquash, opts->update_refs, &todo_list,\n \t\t\topts->action);\ndiff --git a/sequencer.c b/sequencer.c\nindex fb224445fa..aef42122f1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2083,7 +2083,7 @@ static void flush_rewritten_pending(void)\n \tstrbuf_release(&buf);\n }\n \n-static void record_in_rewritten(struct object_id *oid,\n+static void record_in_rewritten(const struct object_id *oid,\n \t\tenum todo_command next_command)\n {\n \tFILE *out = fopen_or_warn(rebase_path_rewritten_pending(), \"a\");\n@@ -5918,7 +5918,7 @@ int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,\n /* skip picking commits whose parents are unchanged */\n static int skip_unnecessary_picks(struct repository *r,\n \t\t\t\t struct todo_list *todo_list,\n-\t\t\t\t struct object_id *base_oid)\n+\t\t\t\t const struct object_id **base_oid)\n {\n \tstruct object_id *parent_oid;\n \tint i;\n@@ -5939,9 +5939,9 @@ static int skip_unnecessary_picks(struct repository *r,\n \t\tif (item->commit->parents->next)\n \t\t\tbreak; /* merge commit */\n \t\tparent_oid = &item->commit->parents->item->object.oid;\n-\t\tif (!oideq(parent_oid, base_oid))\n+\t\tif (!oideq(parent_oid, *base_oid))\n \t\t\tbreak;\n-\t\toidcpy(base_oid, &item->commit->object.oid);\n+\t\t*base_oid = &item->commit->object.oid;\n \t}\n \tif (i > 0) {\n \t\tconst char *done_path = rebase_path_done();\n@@ -5958,7 +5958,7 @@ static int skip_unnecessary_picks(struct repository *r,\n \t\ttodo_list->done_nr += i;\n \n \t\tif (is_fixup(peek_command(todo_list, 0)))\n-\t\t\trecord_in_rewritten(base_oid, peek_command(todo_list, 0));\n+\t\t\trecord_in_rewritten(*base_oid, peek_command(todo_list, 0));\n \t}\n \n \treturn 0;\n@@ -6090,19 +6090,18 @@ static int todo_list_add_update_ref_commands(struct todo_list *todo_list)\n \n int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,\n \t\t const char *shortrevisions, const char *onto_name,\n-\t\t struct commit *onto, const struct object_id *orig_head,\n+\t\t const struct object_id *onto, const struct object_id *orig_head,\n \t\t struct string_list *commands, unsigned autosquash,\n \t\t unsigned update_refs, struct todo_list *todo_list,\n \t\t enum rebase_action action)\n {\n \tchar shortonto[GIT_MAX_HEXSZ + 1];\n \tconst char *todo_file = rebase_path_todo();\n \tstruct todo_list new_todo = TODO_LIST_INIT;\n \tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n-\tstruct object_id oid = onto->object.oid;\n \tint res;\n \n-\tfind_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);\n+\tfind_unique_abbrev_r(shortonto, onto, DEFAULT_ABBREV);\n \n \tif (buf->len == 0) {\n \t\tstruct todo_item *item = append_new_todo(todo_list);\n@@ -6143,7 +6142,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \n \t\treturn error(_(\"nothing to do\"));\n \t} else if (res == EDIT_TODO_INCORRECT) {\n-\t\tcheckout_onto(r, opts, onto_name, &onto->object.oid, orig_head);\n+\t\tcheckout_onto(r, opts, onto_name, onto, orig_head);\n \t\ttodo_list_release(&new_todo);\n \n \t\treturn -1;\n@@ -6158,7 +6157,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n \t\t new_todo.buf.buf);\n \n-\tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {\n+\tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) {\n \t\ttodo_list_release(&new_todo);\n \t\treturn error(_(\"could not skip unnecessary pick commands\"));\n \t}\n@@ -6171,7 +6170,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \n \tres = -1;\n \n-\tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n+\tif (checkout_onto(r, opts, onto_name, onto, orig_head))\n \t\tgoto cleanup;\n \n \tif (require_clean_work_tree(r, \"rebase\", NULL, 1, 1))\ndiff --git a/sequencer.h b/sequencer.h\nindex a1b8ca6eb1..24bf71d5db 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -188,7 +188,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \n int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,\n \t\t const char *shortrevisions, const char *onto_name,\n-\t\t struct commit *onto, const struct object_id *orig_head,\n+\t\t const struct object_id *onto, const struct object_id *orig_head,\n \t\t struct string_list *commands, unsigned autosquash,\n \t\t unsigned update_refs, struct todo_list *todo_list,\n \t\t enum rebase_action action);\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"474027","messageId":"fa584725-52a5-ab7f-3f7b-2fc70fa8fbe1@dunelm.org.uk","threadId":"59451","inReplyTo":"20230323162235.995574-4-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 3/8] sequencer: pass around rebase action explicitly","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-23T19:27:25Z","receivedAt":"2023-03-23T19:27:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nI appreciate the sentiment behind this patch, but it looks like an \nawful lot of churn just to clean up a couple of lines in \nappend_todo_help() and edit_todo_list(). I suspect you may want this \nchange for your --rewind patch, if so I think it would be better to post \nit in that series where we can better judge the benefit.\n\nBest Wishes\n\nPhillip\n\nOn 23/03/2023 16:22, Oswald Buddenhagen wrote:\n> ... instead of deriving it from other arguments. This is a lot cleaner\n> and more extensible.\n> \n> Signed-off-by: Oswald Buddenhagen \n> ---\n> builtin/rebase.c | 25 ++++++++++---------------\n> rebase-interactive.c | 21 +++++++++++----------\n> rebase-interactive.h | 16 ++++++++++++++--\n> sequencer.c | 16 +++++++++-------\n> sequencer.h | 8 +++++---\n> 5 files changed, 49 insertions(+), 37 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 491759db19..a309addd50 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -58,16 +58,6 @@ enum empty_type {\n> \tEMPTY_ASK\n> };\n> \n> -enum action {\n> -\tACTION_NONE = 0,\n> -\tACTION_CONTINUE,\n> -\tACTION_SKIP,\n> -\tACTION_ABORT,\n> -\tACTION_QUIT,\n> -\tACTION_EDIT_TODO,\n> -\tACTION_SHOW_CURRENT_PATCH\n> -};\n> -\n> static const char *action_names[] = {\n> \t\"undefined\",\n> \t\"continue\",\n> @@ -104,7 +94,7 @@ struct rebase_options {\n> \t\tREBASE_INTERACTIVE_EXPLICIT = 1<<4,\n> \t} flags;\n> \tstruct strvec git_am_opts;\n> -\tenum action action;\n> +\tenum rebase_action action;\n> \tchar *reflog_action;\n> \tint signoff;\n> \tint allow_rerere_autoupdate;\n> @@ -198,9 +188,11 @@ static int edit_todo_file(unsigned flags)\n> \t\treturn error_errno(_(\"could not read '%s'.\"), todo_file);\n> \n> \tstrbuf_stripspace(&todo_list.buf, 1);\n> -\tres = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags);\n> +\tres = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags,\n> +\t\t\t ACTION_EDIT_TODO);\n> \tif (!res && todo_list_write_to_file(the_repository, &new_todo, todo_file,\n> -\t\t\t\t\t NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS)))\n> +\t\t\t\t\t NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS),\n> +\t\t\t\t\t ACTION_EDIT_TODO))\n> \t\tres = error_errno(_(\"could not write '%s'\"), todo_file);\n> \n> \ttodo_list_release(&todo_list);\n> @@ -294,7 +286,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n> \t\tret = complete_action(the_repository, &replay, flags,\n> \t\t\tshortrevisions, opts->onto_name, opts->onto,\n> \t\t\t&opts->orig_head->object.oid, &opts->exec,\n> -\t\t\topts->autosquash, opts->update_refs, &todo_list);\n> +\t\t\topts->autosquash, opts->update_refs, &todo_list,\n> +\t\t\topts->action);\n> \t}\n> \n> cleanup:\n> @@ -1246,7 +1239,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\telse if (options.exec.nr)\n> \t\t\ttrace2_cmd_mode(\"interactive-exec\");\n> \t\telse\n> -\t\t\ttrace2_cmd_mode(action_names[options.action]);\n> +\t\t\ttrace2_cmd_mode(\n> +\t\t\t\t(BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(action_names) == ACTION_LAST),\n> +\t\t\t\t action_names[options.action]));\n> \t}\n> \n> \toptions.reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n> diff --git a/rebase-interactive.c b/rebase-interactive.c\n> index 7407c59319..111a2071ae 100644\n> --- a/rebase-interactive.c\n> +++ b/rebase-interactive.c\n> @@ -35,7 +35,7 @@ static enum missing_commit_check_level get_missing_commit_check_level(void)\n> \treturn MISSING_COMMIT_CHECK_IGNORE;\n> }\n> \n> -void append_todo_help(int command_count,\n> +void append_todo_help(int command_count, enum rebase_action action,\n> \t\t const char *shortrevisions, const char *shortonto,\n> \t\t struct strbuf *buf)\n> {\n> @@ -62,9 +62,8 @@ void append_todo_help(int command_count,\n> \" updated at the end of the rebase\\n\"\n> \"\\n\"\n> \"These lines can be re-ordered; they are executed from top to bottom.\\n\");\n> -\tunsigned edit_todo = !(shortrevisions && shortonto);\n> \n> -\tif (!edit_todo) {\n> +\tif (action == ACTION_NONE) {\n> \t\tstrbuf_addch(buf, '\\n');\n> \t\tstrbuf_commented_addf(buf, Q_(\"Rebase %s onto %s (%d command)\",\n> \t\t\t\t\t \"Rebase %s onto %s (%d commands)\",\n> @@ -83,7 +82,7 @@ void append_todo_help(int command_count,\n> \n> \tstrbuf_add_commented_lines(buf, msg, strlen(msg));\n> \n> -\tif (edit_todo)\n> +\tif (action == ACTION_EDIT_TODO)\n> \t\tmsg = _(\"\\nYou are editing the todo file \"\n> \t\t\t\"of an ongoing interactive rebase.\\n\"\n> \t\t\t\"To continue rebase after editing, run:\\n\"\n> @@ -97,35 +96,37 @@ void append_todo_help(int command_count,\n> \n> int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n> \t\t struct todo_list *new_todo, const char *shortrevisions,\n> -\t\t const char *shortonto, unsigned flags)\n> +\t\t const char *shortonto, unsigned flags,\n> +\t\t enum rebase_action action)\n> {\n> \tconst char *todo_file = rebase_path_todo(),\n> \t\t*todo_backup = rebase_path_todo_backup();\n> -\tunsigned initial = shortrevisions && shortonto;\n> \tint incorrect = 0;\n> \n> \t/* If the user is editing the todo list, we first try to parse\n> \t * it. If there is an error, we do not return, because the user\n> \t * might want to fix it in the first place. */\n> -\tif (!initial)\n> +\tif (action != ACTION_NONE)\n> \t\tincorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |\n> \t\t\tfile_exists(rebase_path_dropped());\n> \n> \tif (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,\n> -\t\t\t\t -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))\n> +\t\t\t\t -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP,\n> +\t\t\t\t action))\n> \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n> \n> \tif (!incorrect &&\n> \t todo_list_write_to_file(r, todo_list, todo_backup,\n> \t\t\t\t shortrevisions, shortonto, -1,\n> -\t\t\t\t (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)\n> +\t\t\t\t (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS,\n> +\t\t\t\t action) < 0)\n> \t\treturn error(_(\"could not write '%s'.\"), rebase_path_todo_backup());\n> \n> \tif (launch_sequence_editor(todo_file, &new_todo->buf, NULL))\n> \t\treturn -2;\n> \n> \tstrbuf_stripspace(&new_todo->buf, 1);\n> -\tif (initial && new_todo->buf.len == 0)\n> +\tif (action != ACTION_EDIT_TODO && new_todo->buf.len == 0)\n> \t\treturn -3;\n> \n> \tif (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {\n> diff --git a/rebase-interactive.h b/rebase-interactive.h\n> index 7239c60f79..d9873d3497 100644\n> --- a/rebase-interactive.h\n> +++ b/rebase-interactive.h\n> @@ -5,12 +5,24 @@ struct strbuf;\n> struct repository;\n> struct todo_list;\n> \n> -void append_todo_help(int command_count,\n> +enum rebase_action {\n> +\tACTION_NONE = 0,\n> +\tACTION_CONTINUE,\n> +\tACTION_SKIP,\n> +\tACTION_ABORT,\n> +\tACTION_QUIT,\n> +\tACTION_EDIT_TODO,\n> +\tACTION_SHOW_CURRENT_PATCH,\n> +\tACTION_LAST\n> +};\n> +\n> +void append_todo_help(int command_count, enum rebase_action action,\n> \t\t const char *shortrevisions, const char *shortonto,\n> \t\t struct strbuf *buf);\n> int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n> \t\t struct todo_list *new_todo, const char *shortrevisions,\n> -\t\t const char *shortonto, unsigned flags);\n> +\t\t const char *shortonto, unsigned flags,\n> +\t\t enum rebase_action action);\n> \n> int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);\n> int todo_list_check_against_backup(struct repository *r,\n> diff --git a/sequencer.c b/sequencer.c\n> index 7c275c9a65..f05174d151 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5894,14 +5894,15 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis\n> \n> int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,\n> \t\t\t const char *file, const char *shortrevisions,\n> -\t\t\t const char *shortonto, int num, unsigned flags)\n> +\t\t\t const char *shortonto, int num, unsigned flags,\n> +\t\t\t enum rebase_action action)\n> {\n> \tint res;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \n> \ttodo_list_to_strbuf(r, todo_list, &buf, num, flags);\n> \tif (flags & TODO_LIST_APPEND_TODO_HELP)\n> -\t\tappend_todo_help(count_commands(todo_list),\n> +\t\tappend_todo_help(count_commands(todo_list), action,\n> \t\t\t\t shortrevisions, shortonto, &buf);\n> \n> \tres = write_message(buf.buf, buf.len, file, 0);\n> @@ -5941,7 +5942,8 @@ static int skip_unnecessary_picks(struct repository *r,\n> \tif (i > 0) {\n> \t\tconst char *done_path = rebase_path_done();\n> \n> -\t\tif (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0)) {\n> +\t\tif (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0,\n> +\t\t\t\t\t ACTION_NONE)) {\n> \t\t\terror_errno(_(\"could not write to '%s'\"), done_path);\n> \t\t\treturn -1;\n> \t\t}\n> @@ -6086,8 +6088,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \t\t const char *shortrevisions, const char *onto_name,\n> \t\t struct commit *onto, const struct object_id *orig_head,\n> \t\t struct string_list *commands, unsigned autosquash,\n> -\t\t unsigned update_refs,\n> -\t\t struct todo_list *todo_list)\n> +\t\t unsigned update_refs, struct todo_list *todo_list,\n> +\t\t enum rebase_action action)\n> {\n> \tchar shortonto[GIT_MAX_HEXSZ + 1];\n> \tconst char *todo_file = rebase_path_todo();\n> @@ -6122,7 +6124,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \t}\n> \n> \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n> -\t\t\t shortonto, flags);\n> +\t\t\t shortonto, flags, action);\n> \tif (res == -1)\n> \t\treturn -1;\n> \telse if (res == -2) {\n> @@ -6158,7 +6160,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \t}\n> \n> \tif (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,\n> -\t\t\t\t flags & ~(TODO_LIST_SHORTEN_IDS))) {\n> +\t\t\t\t flags & ~(TODO_LIST_SHORTEN_IDS), action)) {\n> \t\ttodo_list_release(&new_todo);\n> \t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n> \t}\n> diff --git a/sequencer.h b/sequencer.h\n> index 33dbaf5b66..1a3e616af2 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -4,6 +4,7 @@\n> #include \"strbuf.h\"\n> #include \"wt-status.h\"\n> \n> +enum rebase_action;\n> struct commit;\n> struct index_state;\n> struct repository;\n> @@ -134,7 +135,8 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n> \t\t\t\tstruct todo_list *todo_list);\n> int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,\n> \t\t\t const char *file, const char *shortrevisions,\n> -\t\t\t const char *shortonto, int num, unsigned flags);\n> +\t\t\t const char *shortonto, int num, unsigned flags,\n> +\t\t\t enum rebase_action action);\n> void todo_list_release(struct todo_list *todo_list);\n> const char *todo_item_get_arg(struct todo_list *todo_list,\n> \t\t\t struct todo_item *item);\n> @@ -187,8 +189,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \t\t const char *shortrevisions, const char *onto_name,\n> \t\t struct commit *onto, const struct object_id *orig_head,\n> \t\t struct string_list *commands, unsigned autosquash,\n> -\t\t unsigned update_refs,\n> -\t\t struct todo_list *todo_list);\n> +\t\t unsigned update_refs, struct todo_list *todo_list,\n> +\t\t enum rebase_action action);\n> int todo_list_rearrange_squash(struct todo_list *todo_list);\n> \n> /*\n"},{"id":"474028","messageId":"81ad705d-beef-03d5-e56b-e25a0eccea1e@dunelm.org.uk","threadId":"59451","inReplyTo":"20230323162235.995574-5-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 4/8] sequencer: create enum for edit_todo_list() return value","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-23T19:27:57Z","receivedAt":"2023-03-23T19:28:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nThis is a really useful cleanup\n\nThanks\n\nPhillip\n\nOn 23/03/2023 16:22, Oswald Buddenhagen wrote:\n> This is a lot cleaner than open-coding magic numbers.\n> \n> Signed-off-by: Oswald Buddenhagen \n> ---\n> rebase-interactive.c | 15 ++++++++-------\n> rebase-interactive.h | 11 ++++++++++-\n> sequencer.c | 8 ++++----\n> 3 files changed, 22 insertions(+), 12 deletions(-)\n> \n> diff --git a/rebase-interactive.c b/rebase-interactive.c\n> index 111a2071ae..a3d8925b06 100644\n> --- a/rebase-interactive.c\n> +++ b/rebase-interactive.c\n> @@ -94,7 +94,8 @@ void append_todo_help(int command_count, enum rebase_action action,\n> \tstrbuf_add_commented_lines(buf, msg, strlen(msg));\n> }\n> \n> -int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n> +enum edit_todo_result edit_todo_list(\n> +\t\t struct repository *r, struct todo_list *todo_list,\n> \t\t struct todo_list *new_todo, const char *shortrevisions,\n> \t\t const char *shortonto, unsigned flags,\n> \t\t enum rebase_action action)\n> @@ -123,37 +124,37 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n> \t\treturn error(_(\"could not write '%s'.\"), rebase_path_todo_backup());\n> \n> \tif (launch_sequence_editor(todo_file, &new_todo->buf, NULL))\n> -\t\treturn -2;\n> +\t\treturn EDIT_TODO_FAILED;\n> \n> \tstrbuf_stripspace(&new_todo->buf, 1);\n> \tif (action != ACTION_EDIT_TODO && new_todo->buf.len == 0)\n> -\t\treturn -3;\n> +\t\treturn EDIT_TODO_ABORT;\n> \n> \tif (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {\n> \t\tfprintf(stderr, _(edit_todo_list_advice));\n> -\t\treturn -4;\n> +\t\treturn EDIT_TODO_INCORRECT;\n> \t}\n> \n> \tif (incorrect) {\n> \t\tif (todo_list_check_against_backup(r, new_todo)) {\n> \t\t\twrite_file(rebase_path_dropped(), \"%s\", \"\");\n> -\t\t\treturn -4;\n> +\t\t\treturn EDIT_TODO_INCORRECT;\n> \t\t}\n> \n> \t\tif (incorrect > 0)\n> \t\t\tunlink(rebase_path_dropped());\n> \t} else if (todo_list_check(todo_list, new_todo)) {\n> \t\twrite_file(rebase_path_dropped(), \"%s\", \"\");\n> -\t\treturn -4;\n> +\t\treturn EDIT_TODO_INCORRECT;\n> \t}\n> \n> \t/*\n> \t * See if branches need to be added or removed from the update-refs\n> \t * file based on the new todo list.\n> \t */\n> \ttodo_list_filter_update_refs(r, new_todo);\n> \n> -\treturn 0;\n> +\treturn EDIT_TODO_OK;\n> }\n> \n> define_commit_slab(commit_seen, unsigned char);\n> diff --git a/rebase-interactive.h b/rebase-interactive.h\n> index d9873d3497..5aa4111b4f 100644\n> --- a/rebase-interactive.h\n> +++ b/rebase-interactive.h\n> @@ -16,10 +16,19 @@ enum rebase_action {\n> \tACTION_LAST\n> };\n> \n> +enum edit_todo_result {\n> +\tEDIT_TODO_OK = 0, // must be 0\n> +\tEDIT_TODO_IOERROR = -1, // generic i/o error; must be -1\n> +\tEDIT_TODO_FAILED = -2, // editing failed\n> +\tEDIT_TODO_ABORT = -3, // user requested abort\n> +\tEDIT_TODO_INCORRECT = -4 // file violates syntax or constraints\n> +};\n> +\n> void append_todo_help(int command_count, enum rebase_action action,\n> \t\t const char *shortrevisions, const char *shortonto,\n> \t\t struct strbuf *buf);\n> -int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n> +enum edit_todo_result edit_todo_list(\n> +\t\t struct repository *r, struct todo_list *todo_list,\n> \t\t struct todo_list *new_todo, const char *shortrevisions,\n> \t\t const char *shortonto, unsigned flags,\n> \t\t enum rebase_action action);\n> diff --git a/sequencer.c b/sequencer.c\n> index f05174d151..b1c29c8802 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -6125,20 +6125,20 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \n> \tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n> \t\t\t shortonto, flags, action);\n> -\tif (res == -1)\n> +\tif (res == EDIT_TODO_IOERROR)\n> \t\treturn -1;\n> -\telse if (res == -2) {\n> +\telse if (res == EDIT_TODO_FAILED) {\n> \t\tapply_autostash(rebase_path_autostash());\n> \t\tsequencer_remove_state(opts);\n> \n> \t\treturn -1;\n> -\t} else if (res == -3) {\n> +\t} else if (res == EDIT_TODO_ABORT) {\n> \t\tapply_autostash(rebase_path_autostash());\n> \t\tsequencer_remove_state(opts);\n> \t\ttodo_list_release(&new_todo);\n> \n> \t\treturn error(_(\"nothing to do\"));\n> -\t} else if (res == -4) {\n> +\t} else if (res == EDIT_TODO_INCORRECT) {\n> \t\tcheckout_onto(r, opts, onto_name, &onto->object.oid, orig_head);\n> \t\ttodo_list_release(&new_todo);\n> \n"},{"id":"474029","messageId":"47558c14-ba2c-18ec-0532-b21fdfd223f8@dunelm.org.uk","threadId":"59451","inReplyTo":"20230323162235.995574-6-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-23T19:31:04Z","receivedAt":"2023-03-23T19:31:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nOn 23/03/2023 16:22, Oswald Buddenhagen wrote:\n> Creating a suitable todo file is a potentially labor-intensive process,\n> so be less cavalier about discarding it when something goes wrong (e.g.,\n> the user messed with the repo while editing the todo).\n\nI was thinking about this problem the other day in the context of \nrescheduling commands when they cannot be executed because they would \noverwrite an untracked file. My thought was that we should prepend a \n\"reset\" command to the todo list so that the checkout happened when the \nuser continued the rebase. How does this patch ensure the checkout \nhappens when the user continues the rebase?\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Oswald Buddenhagen \n> ---\n> builtin/rebase.c | 1 +\n> sequencer.c | 4 ++++\n> sequencer.h | 1 +\n> t/t3404-rebase-interactive.sh | 3 ++-\n> 4 files changed, 8 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index a309addd50..728c869db4 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -153,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n> \treplay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);\n> \treplay.quiet = !(opts->flags & REBASE_NO_QUIET);\n> \treplay.verbose = opts->flags & REBASE_VERBOSE;\n> +\treplay.precious_todo = opts->flags & REBASE_INTERACTIVE_EXPLICIT;\n> \treplay.reschedule_failed_exec = opts->reschedule_failed_exec;\n> \treplay.committer_date_is_author_date =\n> \t\t\t\t\topts->committer_date_is_author_date;\n> diff --git a/sequencer.c b/sequencer.c\n> index b1c29c8802..f8a7f4e721 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4570,6 +4570,10 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,\n> \t\t.default_reflog_action = sequencer_reflog_action(opts)\n> \t};\n> \tif (reset_head(r, &ropts)) {\n> +\t\t// Editing the todo may have been costly; don't just discard it.\n> +\t\tif (opts->precious_todo)\n> +\t\t\texit(1); // Error was already printed\n> +\n> \t\tapply_autostash(rebase_path_autostash());\n> \t\tsequencer_remove_state(opts);\n> \t\treturn error(_(\"could not detach HEAD\"));\n> diff --git a/sequencer.h b/sequencer.h\n> index 1a3e616af2..a1b8ca6eb1 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -47,6 +47,7 @@ struct replay_opts {\n> \tint keep_redundant_commits;\n> \tint verbose;\n> \tint quiet;\n> +\tint precious_todo;\n> \tint reschedule_failed_exec;\n> \tint committer_date_is_author_date;\n> \tint ignore_date;\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index ff0afad63e..c625aad10a 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -288,13 +288,14 @@ test_expect_success 'abort' '\n> '\n> \n> test_expect_success 'abort with error when new base cannot be checked out' '\n> +\ttest_when_finished \"git rebase --abort ||:\" &&\n> \tgit rm --cached file1 &&\n> \tgit commit -m \"remove file in base\" &&\n> \ttest_must_fail git rebase -i primary > output 2>&1 &&\n> \ttest_i18ngrep \"The following untracked working tree files would be overwritten by checkout:\" \\\n> \t\toutput &&\n> \ttest_i18ngrep \"file1\" output &&\n> -\ttest_path_is_missing .git/rebase-merge &&\n> +\ttest_path_is_dir .git/rebase-merge &&\n> \trm file1 &&\n> \tgit reset --hard HEAD^\n> '\n"},{"id":"474030","messageId":"a3833d93-5db0-454e-526e-04681e5e5276@dunelm.org.uk","threadId":"59451","inReplyTo":"20230323162235.995574-8-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-23T19:34:57Z","receivedAt":"2023-03-23T19:35:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nOn 23/03/2023 16:22, Oswald Buddenhagen wrote:\n> ... instead of as a commit, which makes the purpose clearer and will\n> simplify things later.\n\ngiven that we want onto to be a commit I'm not sure how this makes \nanything clearer.\n\n> As a side effect, this change revealed that skip_unnecessary_picks() was\n> butchering the commit object due to missing const-correctness. Slightly\n> adjust its API to rectify this.\n\nI don't think this is correct. If you look at the original code it makes \na copy of the oid and uses the copy when calling skip_unnecessary_picks()\n\n> int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,\n> \t\t const char *shortrevisions, const char *onto_name,\n> -\t\t struct commit *onto, const struct object_id *orig_head,\n> +\t\t const struct object_id *onto, const struct object_id *orig_head,\n> \t\t struct string_list *commands, unsigned autosquash,\n> \t\t unsigned update_refs, struct todo_list *todo_list,\n> \t\t enum rebase_action action)\n> {\n> \tchar shortonto[GIT_MAX_HEXSZ + 1];\n> \tconst char *todo_file = rebase_path_todo();\n> \tstruct todo_list new_todo = TODO_LIST_INIT;\n> \tstruct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;\n> -\tstruct object_id oid = onto->object.oid;\n\nHere we copy the onto's oid\n\n> @@ -6158,7 +6157,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n> \t\t new_todo.buf.buf);\n> \n> -\tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {\n\nHere we pass the copy to skip_unnecessary_picks()\n\nBest Wishes\n\nPhillip\n\n> +\tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) {\n> \t\ttodo_list_release(&new_todo);\n> \t\treturn error(_(\"could not skip unnecessary pick commands\"));\n> \t}\n> @@ -6171,7 +6170,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \n> \tres = -1;\n> \n> -\tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n> +\tif (checkout_onto(r, opts, onto_name, onto, orig_head))\n> \t\tgoto cleanup;\n> \n> \tif (require_clean_work_tree(r, \"rebase\", NULL, 1, 1))\n> diff --git a/sequencer.h b/sequencer.h\n> index a1b8ca6eb1..24bf71d5db 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -188,7 +188,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n> \n> int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,\n> \t\t const char *shortrevisions, const char *onto_name,\n> -\t\t struct commit *onto, const struct object_id *orig_head,\n> +\t\t const struct object_id *onto, const struct object_id *orig_head,\n> \t\t struct string_list *commands, unsigned autosquash,\n> \t\t unsigned update_refs, struct todo_list *todo_list,\n> \t\t enum rebase_action action);\n"},{"id":"474031","messageId":"b54f00c1-e8e3-67d7-0288-805ae18d7335@dunelm.org.uk","threadId":"59451","inReplyTo":"20230323162235.995574-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 0/8] sequencer refactoring","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-23T19:38:09Z","receivedAt":"2023-03-23T19:38:28Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nThanks for working on this. I've only had time for a quick read of the \nfirst 7 patches but there are some worthwhile clean ups here and the \nseries is well structured. I'll try and have a thorough look at the last \npatch but I'm going to be off line next week so it may take a while.\n\nBest Wishes\n\nPhillip\n\nOn 23/03/2023 16:22, Oswald Buddenhagen wrote:\n> This is a preparatory series for the separately posted 'rebase --rewind' patch,\n> but I think it has value in itself.\n> \n> \n> Oswald Buddenhagen (8):\n> rebase: simplify code related to imply_merge()\n> rebase: move parse_opt_keep_empty() down\n> sequencer: pass around rebase action explicitly\n> sequencer: create enum for edit_todo_list() return value\n> rebase: preserve interactive todo file on checkout failure\n> sequencer: simplify allocation of result array in\n> todo_list_rearrange_squash()\n> sequencer: pass `onto` to complete_action() as object-id\n> rebase: improve resumption from incorrect initial todo list\n> \n> builtin/rebase.c | 63 +++++++--------\n> builtin/revert.c | 3 +-\n> rebase-interactive.c | 36 ++++-----\n> rebase-interactive.h | 27 ++++++-\n> sequencer.c | 139 +++++++++++++++++++---------------\n> sequencer.h | 15 ++--\n> t/t3404-rebase-interactive.sh | 34 ++++++++-\n> 7 files changed, 196 insertions(+), 121 deletions(-)\n> \n"},{"id":"474032","messageId":"28e9e0fe-3a00-50f7-2204-57f69a20c693@dunelm.org.uk","threadId":"59451","inReplyTo":"20230323162235.995574-3-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 2/8] rebase: move parse_opt_keep_empty() down","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-23T19:39:05Z","receivedAt":"2023-03-23T19:39:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\nOn 23/03/2023 16:22, Oswald Buddenhagen wrote:\n> This moves it right next to parse_opt_empty(), which is a much more\n> logical place. As a side effect, this removes the need for a forward\n> declaration of imply_merge().\n\nThis looks good, it is nice to get rid of that forward declaration\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Oswald Buddenhagen \n> ---\n> builtin/rebase.c | 25 ++++++++++++-------------\n> 1 file changed, 12 insertions(+), 13 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 8ffea0f0d8..491759db19 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -362,19 +362,6 @@ static int run_sequencer_rebase(struct rebase_options *opts)\n> \treturn ret;\n> }\n> \n> -static void imply_merge(struct rebase_options *opts, const char *option);\n> -static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n> -\t\t\t\tint unset)\n> -{\n> -\tstruct rebase_options *opts = opt->value;\n> -\n> -\tBUG_ON_OPT_ARG(arg);\n> -\n> -\timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n> -\topts->keep_empty = !unset;\n> -\treturn 0;\n> -}\n> -\n> static int is_merge(struct rebase_options *opts)\n> {\n> \treturn opts->type == REBASE_MERGE;\n> @@ -969,6 +956,18 @@ static enum empty_type parse_empty_value(const char *value)\n> \tdie(_(\"unrecognized empty type '%s'; valid values are \\\"drop\\\", \\\"keep\\\", and \\\"ask\\\".\"), value);\n> }\n> \n> +static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n> +\t\t\t\tint unset)\n> +{\n> +\tstruct rebase_options *opts = opt->value;\n> +\n> +\tBUG_ON_OPT_ARG(arg);\n> +\n> +\timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n> +\topts->keep_empty = !unset;\n> +\treturn 0;\n> +}\n> +\n> static int parse_opt_empty(const struct option *opt, const char *arg, int unset)\n> {\n> \tstruct rebase_options *options = opt->value;\n"},{"id":"474033","messageId":"2b296b75-3f8d-28a9-a3d8-8134450852da@dunelm.org.uk","threadId":"59451","inReplyTo":"20230323162235.995574-2-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 1/8] rebase: simplify code related to imply_merge()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-23T19:40:17Z","receivedAt":"2023-03-23T19:40:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nOn 23/03/2023 16:22, Oswald Buddenhagen wrote:\n> The code's evolution left in some bits surrounding enum rebase_type that\n> don't really make sense any more. In particular, it makes no sense to\n> invoke imply_merge() if the type is already known not to be\n> REBASE_APPLY, and it makes no sense to assign the type after calling\n> imply_merge().\n\nThese look sensible, did imply_merges() use to do something more which \nmade these calls useful?\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Oswald Buddenhagen \n> ---\n> builtin/rebase.c | 6 +-----\n> 1 file changed, 1 insertion(+), 5 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 5b7b908b66..8ffea0f0d8 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -372,7 +372,6 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n> \n> \timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n> \topts->keep_empty = !unset;\n> -\topts->type = REBASE_MERGE;\n> \treturn 0;\n> }\n> \n> @@ -1494,9 +1493,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t}\n> \t}\n> \n> -\tif (options.type == REBASE_MERGE)\n> -\t\timply_merge(&options, \"--merge\");\n> -\n> \tif (options.root && !options.onto_name)\n> \t\timply_merge(&options, \"--root without --onto\");\n> \n> @@ -1534,7 +1530,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \n> \tif (options.type == REBASE_UNSPECIFIED) {\n> \t\tif (!strcmp(options.default_backend, \"merge\"))\n> -\t\t\timply_merge(&options, \"--merge\");\n> +\t\t\toptions.type = REBASE_MERGE;\n> \t\telse if (!strcmp(options.default_backend, \"apply\"))\n> \t\t\toptions.type = REBASE_APPLY;\n> \t\telse\n"},{"id":"474035","messageId":"d1fb77a0-9ed8-4f3d-5bad-bc443b5522d2@dunelm.org.uk","threadId":"59451","inReplyTo":"20230323162235.995574-7-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-23T19:46:28Z","receivedAt":"2023-03-23T19:46:51Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nOn 23/03/2023 16:22, Oswald Buddenhagen wrote:\n> The operation doesn't change the number of elements in the array, so we do\n> not need to allocate the result piecewise.\n\nI think the reasoning behind this patch is sound.\n\n> Signed-off-by: Oswald Buddenhagen \n> ---\n> sequencer.c | 9 +++++----\n> 1 file changed, 5 insertions(+), 4 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index f8a7f4e721..fb224445fa 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -6225,7 +6225,7 @@ static int skip_fixupish(const char *subject, const char **p) {\n> int todo_list_rearrange_squash(struct todo_list *todo_list)\n> {\n> \tstruct hashmap subject2item;\n> -\tint rearranged = 0, *next, *tail, i, nr = 0, alloc = 0;\n> +\tint rearranged = 0, *next, *tail, i, nr = 0;\n> \tchar **subjects;\n> \tstruct commit_todo_item commit_todo;\n> \tstruct todo_item *items = NULL;\n> @@ -6334,6 +6334,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n> \t}\n> \n> \tif (rearranged) {\n> +\t\titems = ALLOC_ARRAY(items, todo_list->nr);\n> +\n> \t\tfor (i = 0; i < todo_list->nr; i++) {\n> \t\t\tenum todo_command command = todo_list->items[i].command;\n> \t\t\tint cur = i;\n> @@ -6346,16 +6348,15 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n> \t\t\t\tcontinue;\n> \n> \t\t\twhile (cur >= 0) {\n> -\t\t\t\tALLOC_GROW(items, nr + 1, alloc);\n> \t\t\t\titems[nr++] = todo_list->items[cur];\n> \t\t\t\tcur = next[cur];\n> \t\t\t}\n> \t\t}\n> \n> +\t\tassert(nr == todo_list->nr);\n\nIf this assert fails we may have already had some out of bounds memory \naccesses.\n\n> +\t\ttodo_list->alloc = nr;\n> \t\tFREE_AND_NULL(todo_list->items);\n\nI think it would be cleaner to keep the original ordering and free the \nold list before assigning todo_list->alloc\n\n> \t\ttodo_list->items = items;\n> -\t\ttodo_list->nr = nr;\n> -\t\ttodo_list->alloc = alloc;\n> \t}\n\nBest Wishes\n\nPhillip\n\n> \tfree(next);\n"},{"id":"474036","messageId":"xmqqiler8cga.fsf@gitster.g","threadId":"59451","inReplyTo":"2b296b75-3f8d-28a9-a3d8-8134450852da@dunelm.org.uk","subject":"Re: [PATCH 1/8] rebase: simplify code related to imply_merge()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-23T20:00:53Z","receivedAt":"2023-03-23T20:00:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 23/03/2023 16:22, Oswald Buddenhagen wrote:\n>> The code's evolution left in some bits surrounding enum rebase_type that\n>> don't really make sense any more. In particular, it makes no sense to\n>> invoke imply_merge() if the type is already known not to be\n>> REBASE_APPLY, and it makes no sense to assign the type after calling\n>> imply_merge().\n>\n> These look sensible, did imply_merges() use to do something more which\n> made these calls useful?\n\nGood question.\n\n>> @@ -1494,9 +1493,6 @@ int cmd_rebase(int argc, const char **argv,\n>> const char *prefix)\n>> \t\t}\n>> \t}\n>> -\tif (options.type == REBASE_MERGE)\n>> -\t\timply_merge(&options, \"--merge\");\n\nThis piece is reasonable, of course. We already know we are in\nmerge mode so there is nothing implied.\n\nBefore this hunk, there is a bit of code to react to\noptions.strategy given. The code complains if we are using the\napply backend, and sets the options.type to REBASE_MERGE, which is\nsuspiciously similar to what imply_merge() is doing. I wonder if\nthe code should be simplified to make a call to imply_merge() while\nwe are doing similar simplification like this patch does?\n"},{"id":"474037","messageId":"xmqq8rfn8bps.fsf@gitster.g","threadId":"59451","inReplyTo":"20230323162235.995574-6-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-23T20:16:47Z","receivedAt":"2023-03-23T20:16:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> Creating a suitable todo file is a potentially labor-intensive process,\n> so be less cavalier about discarding it when something goes wrong (e.g.,\n> the user messed with the repo while editing the todo).\n\nIs there a reason why we do not always keep it? Why is the file\nsometimes precious but not precious at all in other times?\n\nTying the previous bit to \"-i was explicitly given\" feels a bit\nunintuitive---when the sequencer machinery was implicitly chosen,\nand gives the control back to the user, should a user be forbidden\nto muck with the todo list?\n\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index a309addd50..728c869db4 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -153,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n> \treplay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);\n> \treplay.quiet = !(opts->flags & REBASE_NO_QUIET);\n> \treplay.verbose = opts->flags & REBASE_VERBOSE;\n> +\treplay.precious_todo = opts->flags & REBASE_INTERACTIVE_EXPLICIT;\n> \treplay.reschedule_failed_exec = opts->reschedule_failed_exec;\n> \treplay.committer_date_is_author_date =\n> \t\t\t\t\topts->committer_date_is_author_date;\n> diff --git a/sequencer.c b/sequencer.c\n> index b1c29c8802..f8a7f4e721 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4570,6 +4570,10 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,\n> \t\t.default_reflog_action = sequencer_reflog_action(opts)\n> \t};\n> \tif (reset_head(r, &ropts)) {\n> +\t\t// Editing the todo may have been costly; don't just discard it.\n> +\t\tif (opts->precious_todo)\n> +\t\t\texit(1); // Error was already printed\n\nNo // comments, please.\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index ff0afad63e..c625aad10a 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -288,13 +288,14 @@ test_expect_success 'abort' '\n> '\n> \n> test_expect_success 'abort with error when new base cannot be checked out' '\n> +\ttest_when_finished \"git rebase --abort ||:\" &&\n> \tgit rm --cached file1 &&\n> \tgit commit -m \"remove file in base\" &&\n> \ttest_must_fail git rebase -i primary > output 2>&1 &&\n> \ttest_i18ngrep \"The following untracked working tree files would be overwritten by checkout:\" \\\n> \t\toutput &&\n> \ttest_i18ngrep \"file1\" output &&\n> -\ttest_path_is_missing .git/rebase-merge &&\n> +\ttest_path_is_dir .git/rebase-merge &&\n> \trm file1 &&\n> \tgit reset --hard HEAD^\n> '\n\nAre we happy to just see that the directory still exists? I thought\nthe original motivation explained in the proposed log message was to\nkeep the todo list file, so shouldn't you be checking if the file is\nthere (and if you can reliably ensure that the file has contents\nthat are expected, that would be even better)?\n\nAlso, as the keeping of the todo list is now conditional, we should\nhave another test that checks that the file is gone when that\ncondition (\"INTERACTIVE_EXPLICIT\"?) does not trigger, I think.\n\nOther than that, nicely written. Thanks.\n"},{"id":"474047","messageId":"CAMP44s3g5FZ5VgvF27h4AzqHrxpOvYtG9RRVr=TJTkTAGxBKqg@mail.gmail.com","threadId":"59451","inReplyTo":"xmqqiler8cga.fsf@gitster.g","subject":"Re: [PATCH 1/8] rebase: simplify code related to imply_merge()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-03-23T21:08:52Z","receivedAt":"2023-03-23T21:09:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Mar 23, 2023 at 2:07 PM Junio C Hamano wrote:\n>\n> Phillip Wood writes:\n>\n> > On 23/03/2023 16:22, Oswald Buddenhagen wrote:\n> >> The code's evolution left in some bits surrounding enum rebase_type that\n> >> don't really make sense any more. In particular, it makes no sense to\n> >> invoke imply_merge() if the type is already known not to be\n> >> REBASE_APPLY, and it makes no sense to assign the type after calling\n> >> imply_merge().\n> >\n> > These look sensible, did imply_merges() use to do something more which\n> > made these calls useful?\n>\n> Good question.\n\nIt used to be called imply_interactive(), so --merge did require an\ninteractive rebase.\n\n-- \nFelipe Contreras\n"},{"id":"474053","messageId":"ZBzEMwQ6p+ca6wdD@ugly","threadId":"59451","inReplyTo":"fa584725-52a5-ab7f-3f7b-2fc70fa8fbe1@dunelm.org.uk","subject":"Re: [PATCH 3/8] sequencer: pass around rebase action explicitly","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T21:27:15Z","receivedAt":"2023-03-23T21:27:32Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Mar 23, 2023 at 07:27:25PM +0000, Phillip Wood wrote:\n>I appreciate the sentiment behind this patch, but it looks like an \n>awful lot of churn just to clean up a couple of lines in \n>append_todo_help() and edit_todo_list().\n>\nas much as i dislike churn, i dislike fragile \"magic\" code even more.\n"},{"id":"474054","messageId":"ZBzGSbm7GZVK17ja@ugly","threadId":"59451","inReplyTo":"a3833d93-5db0-454e-526e-04681e5e5276@dunelm.org.uk","subject":"Re: [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T21:36:09Z","receivedAt":"2023-03-23T21:36:13Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Mar 23, 2023 at 07:34:57PM +0000, Phillip Wood wrote:\n>On 23/03/2023 16:22, Oswald Buddenhagen wrote:\n>> ... instead of as a commit, which makes the purpose clearer and will\n>> simplify things later.\n>\n>given that we want onto to be a commit I'm not sure how this makes \n>anything clearer.\n>\nit makes it clearer that we need only the oid, not any other part of the \ncommit object. and pulling ahead the \"extraction\" reduces the visual \nnoise further down.\n\n>> As a side effect, this change revealed that skip_unnecessary_picks() was\n>> butchering the commit object due to missing const-correctness. Slightly\n>> adjust its API to rectify this.\n>\n>I don't think this is correct. If you look at the original code it makes \n>a copy of the oid and uses the copy when calling skip_unnecessary_picks()\n>\noops, you're quite right. (facepalm)\nimo the change still makes sense, though, as it replaces the relatively \nexpensive deep copies with simple pointer updates. so just fix the \ncommit message?\n\n"},{"id":"474058","messageId":"ZBzPIPQ+GlnPo7Mj@ugly","threadId":"59451","inReplyTo":"d1fb77a0-9ed8-4f3d-5bad-bc443b5522d2@dunelm.org.uk","subject":"Re: [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T22:13:52Z","receivedAt":"2023-03-23T22:16:46Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Mar 23, 2023 at 07:46:28PM +0000, Phillip Wood wrote:\n>> +\t\tassert(nr == todo_list->nr);\n>\n>If this assert fails we may have already had some out of bounds memory \n>accesses.\n>\nthe loop could have run short, too.\nbut anyway, this isn't a runtime check, it's an assertion of a loop \ninvariant.\n\n>> +\t\ttodo_list->alloc = nr;\n>> \t\tFREE_AND_NULL(todo_list->items);\n>\n>I think it would be cleaner to keep the original ordering and free the \n>old list before assigning todo_list->alloc\n>\nmy reasoning is that it's closer to the assert which also refers to it, \nand it really makes sense to have _that_ first. also, the value is more \nlikely to be still in a register at that point.\n\n>> \t\ttodo_list->items = items;\n>> -\t\ttodo_list->nr = nr;\n>> -\t\ttodo_list->alloc = alloc;\n>> \t}\n>\n"},{"id":"474059","messageId":"ZBzU5lzZBtI8/Q7+@ugly","threadId":"59451","inReplyTo":"47558c14-ba2c-18ec-0532-b21fdfd223f8@dunelm.org.uk","subject":"Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T22:38:30Z","receivedAt":"2023-03-23T22:42:24Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Mar 23, 2023 at 07:31:04PM +0000, Phillip Wood wrote:\n>On 23/03/2023 16:22, Oswald Buddenhagen wrote:\n>> Creating a suitable todo file is a potentially labor-intensive process,\n>> so be less cavalier about discarding it when something goes wrong (e.g.,\n>> the user messed with the repo while editing the todo).\n>\n>I was thinking about this problem the other day in the context of \n>rescheduling commands when they cannot be executed because they would \n>overwrite an untracked file. My thought was that we should prepend a \n>\"reset\" command to the todo list so that the checkout happened when the \n>user continued the rebase.\n>\nso you basically want to convert the magic `onto` into an explicit todo \ncommand? i'm not sure what the advantage would be, and i certainly can \nthink of disadvantages re. usability and backwards compat.\n\n>How does this patch ensure the checkout happens when the user continues \n>the rebase?\n>\nthe idea was never that the user --continue's. we're talking about a \nfatal error, and the patch's purpose is only to allow the user to \nsalvage their work manually.\nit's an interesting question, though, esp. in light of patch 8/8 of this \nseries.\n"},{"id":"474061","messageId":"ZBzfZZWthdEM+gKK@ugly","threadId":"59451","inReplyTo":"xmqq8rfn8bps.fsf@gitster.g","subject":"Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T23:23:17Z","receivedAt":"2023-03-23T23:23:41Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Mar 23, 2023 at 01:16:47PM -0700, Junio C Hamano wrote:\n>Oswald Buddenhagen writes:\n>\n>> Creating a suitable todo file is a potentially labor-intensive process,\n>> so be less cavalier about discarding it when something goes wrong (e.g.,\n>> the user messed with the repo while editing the todo).\n>\n>Is there a reason why we do not always keep it? Why is the file\n>sometimes precious but not precious at all in other times?\n>\nthe unedited initial todo just isn't precious. that implies that in a \nnon-interactive rebase, it is always worthless at the time of the \ninitial reset.\n\n>Tying the previous bit to \"-i was explicitly given\" feels a bit\n>unintuitive---when the sequencer machinery was implicitly chosen,\n>and gives the control back to the user, should a user be forbidden\n>to muck with the todo list?\n>\nthat would be an --edit-todo and --continue during a mid-rebase stop. \nrather different case.\n\n>No // comments, please.\n>\n(apparently with a special exception for examples in the apidocs, \npresumably because escaping nested comments would be just too ugly.)\n\n>> -\ttest_path_is_missing .git/rebase-merge &&\n>> +\ttest_path_is_dir .git/rebase-merge &&\n>\n>Are we happy to just see that the directory still exists? I thought\n>the original motivation explained in the proposed log message was to\n>keep the todo list file, so shouldn't you be checking if the file is\n>there\n>\nfair point.\n\n>(and if you can reliably ensure that the file has contents\n>that are expected, that would be even better)?\n>\ni could grep for a shortened sha1 i would obtain from the branch. but \ngiven that the error scenario of a present but somehow corrupted todo \nseems implausible given the circumstances, that seems like overkill.\n\n>Also, as the keeping of the todo list is now conditional, we should\n>have another test that checks that the file is gone when that\n>condition (\"INTERACTIVE_EXPLICIT\"?) does not trigger, I think.\n>\nthat would be for t3400-rebase.sh.\ni suppose we could extend 'Show verbose error when HEAD could not be \ndetached'.\n"},{"id":"474072","messageId":"xmqq355u7ota.fsf@gitster.g","threadId":"59451","inReplyTo":"ZBzfZZWthdEM+gKK@ugly","subject":"Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-24T04:31:29Z","receivedAt":"2023-03-24T04:31:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> On Thu, Mar 23, 2023 at 01:16:47PM -0700, Junio C Hamano wrote:\n>>Oswald Buddenhagen writes:\n>>\n>>> Creating a suitable todo file is a potentially labor-intensive process,\n>>> so be less cavalier about discarding it when something goes wrong (e.g.,\n>>> the user messed with the repo while editing the todo).\n>>\n>>Is there a reason why we do not always keep it? Why is the file\n>>sometimes precious but not precious at all in other times?\n>>\n> the unedited initial todo just isn't precious. that implies that in a\n> non-interactive rebase, it is always worthless at the time of the\n> initial reset.\n\nI see. Thanks for clarifying.\n\nJust FYI, the primary purpose reviewers ask questions on the\nproposed change is to help submitters polish their patch (both the\nproposed log message text and the code) to clarify points they found\nhard to understand and/or they suspect would be hard to understand\nfor other readers. So please do not be happy by just receiving \"I\nsee, thanks\" and stop there. Instead, please update the patch so\nthat future readers would not have to ask similar question again.\n\n>>(and if you can reliably ensure that the file has contents\n>>that are expected, that would be even better)?\n>>\n> i could grep for a shortened sha1 i would obtain from the branch. but\n> given that the error scenario of a present but somehow corrupted todo\n> seems implausible given the circumstances, that seems like overkill.\n\nIt is OK. If it were easy to prepare the \"todo should look like\nthis\" golden copy, then doing test_cmp the actual file with it would\nhave been a simple way to ensure both existence of and sane contents\nin the file at the same time, but if it isn't cheap to prepare such\nan expected output, I agree with you that it is not worth the extra\neffort.\n\nThanks.\n"},{"id":"474084","messageId":"55ee99fc-69e6-7635-10fb-56de9d3b17b6@dunelm.org.uk","threadId":"59451","inReplyTo":"ZBzU5lzZBtI8/Q7+@ugly","subject":"Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-24T14:15:47Z","receivedAt":"2023-03-24T14:15:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nOn 23/03/2023 22:38, Oswald Buddenhagen wrote:\n> On Thu, Mar 23, 2023 at 07:31:04PM +0000, Phillip Wood wrote:\n>> On 23/03/2023 16:22, Oswald Buddenhagen wrote:\n>>> Creating a suitable todo file is a potentially labor-intensive process,\n>>> so be less cavalier about discarding it when something goes wrong (e.g.,\n>>> the user messed with the repo while editing the todo).\n>>\n>> I was thinking about this problem the other day in the context of \n>> rescheduling commands when they cannot be executed because they would \n>> overwrite an untracked file. My thought was that we should prepend a \n>> \"reset\" command to the todo list so that the checkout happened when \n>> the user continued the rebase.\n>>\n> so you basically want to convert the magic `onto` into an explicit todo \n> command? i'm not sure what the advantage would be, and i certainly can \n> think of disadvantages re. usability and backwards compat.\n\nIf the initial checkout of \"onto\" fails I want the rebase to stop so the \nuser can try and fix the problem (usually remove an untracked file) and \nthen run \"git rebase --continue\" to continue the rebase including the \ninitial checkout. Adding a \"reset\" command to the beginning of the todo \nlist when the initial checkout fails is one way of achieving that.\n\n>> How does this patch ensure the checkout happens when the user \n>> continues the rebase?\n>>\n> the idea was never that the user --continue's. we're talking about a \n> fatal error,\n\nIf it is a fatal error what is stopping the user from running \"rebase \n--continue\" and wreaking havoc? You seem to be expecting the user to \nknow that they need to\n\n (1) run \"git rebase --edit-todo\" to save the todo list somewhere safe\n (2) run \"git rebase --abort\" to abort the rebase and restore any\n autostash. (Have you checked that --abort is safe to run when\n HEAD is not detached?)\n (3) fix whatever prevented the checkout from working\n (4) re-run \"git rebase\" and restore the saved todo list when prompted\n to edit it\n\nIt would be much more user friendly to simply allow them to fix the \nproblem with the checkout and run \"git rebase --continue\"\n\nBest Wishes\n\nPhillip\n\n> and the patch's purpose is only to allow the user to \n> salvage their work manually.\n> it's an interesting question, though, esp. in light of patch 8/8 of this \n> series.\n"},{"id":"474085","messageId":"88947332-fca5-978e-4405-5c616bd91d29@dunelm.org.uk","threadId":"59451","inReplyTo":"ZBzGSbm7GZVK17ja@ugly","subject":"Re: [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-24T14:18:35Z","receivedAt":"2023-03-24T14:18:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nOn 23/03/2023 21:36, Oswald Buddenhagen wrote:\n> On Thu, Mar 23, 2023 at 07:34:57PM +0000, Phillip Wood wrote:\n>> On 23/03/2023 16:22, Oswald Buddenhagen wrote:\n>>> ... instead of as a commit, which makes the purpose clearer and will\n>>> simplify things later.\n>>\n>> given that we want onto to be a commit I'm not sure how this makes \n>> anything clearer.\n>>\n> it makes it clearer that we need only the oid, not any other part of the \n> commit object. and pulling ahead the \"extraction\" reduces the visual \n> noise further down.\n> \n>>> As a side effect, this change revealed that skip_unnecessary_picks() was\n>>> butchering the commit object due to missing const-correctness. Slightly\n>>> adjust its API to rectify this.\n>>\n>> I don't think this is correct. If you look at the original code it \n>> makes a copy of the oid and uses the copy when calling \n>> skip_unnecessary_picks()\n>>\n> oops, you're quite right. (facepalm)\n> imo the change still makes sense, though, as it replaces the relatively \n> expensive deep copies with simple pointer updates. so just fix the \n> commit message?\n\nI think you should just drop this patch. Copying struct object_id is \ncheap and idiomatic in the code base. If you grep for\n\n\"struct object_id \\*\\*[a-zA-Z0-9_]*[,)]\"\n\nyou'll see that there are very few matches and all but one of those are \npassing an array.\n\nBest Wishes\n\nPhillip\n\n> \n"},{"id":"474087","messageId":"ZB226RPsBRghnruI@ugly","threadId":"59451","inReplyTo":"55ee99fc-69e6-7635-10fb-56de9d3b17b6@dunelm.org.uk","subject":"Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-24T14:42:49Z","receivedAt":"2023-03-24T14:43:13Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Fri, Mar 24, 2023 at 02:15:47PM +0000, Phillip Wood wrote:\n>On 23/03/2023 22:38, Oswald Buddenhagen wrote:\n>> so you basically want to convert the magic `onto` into an explicit \n>> todo command? i'm not sure what the advantage would be, and i \n>> certainly can think of disadvantages re. usability and backwards \n>> compat.\n>\n>If the initial checkout of \"onto\" fails I want the rebase to stop so the \n>user can try and fix the problem (usually remove an untracked file) and \n>then run \"git rebase --continue\" to continue the rebase including the \n>initial checkout. Adding a \"reset\" command to the beginning of the todo \n>list when the initial checkout fails is one way of achieving that.\n>\ni suppose, but patch 8/8 does pretty much the same, only with fewer side \neffects.\n\n>>> How does this patch ensure the checkout happens when the user \n>>> continues the rebase?\n>>>\n>> the idea was never that the user --continue's. we're talking about a \n>> fatal error,\n>\n>If it is a fatal error what is stopping the user from running \"rebase \n>--continue\" and wreaking havoc? [...]\n>\n>It would be much more user friendly to simply allow them to fix the \n>problem with the checkout and run \"git rebase --continue\"\n>\ni'll reorder the patch to the end of the series, as after the currently \nlast patch we'll be in a much better position to deal with the fallout \nin a sane way.\n\nthanks!\n"},{"id":"474167","messageId":"f72b3820-124c-3e2c-30e2-ca3f46b74dc0@dunelm.org.uk","threadId":"59451","inReplyTo":"20230323162235.995574-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 0/8] sequencer refactoring","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-25T11:08:06Z","receivedAt":"2023-03-25T11:08:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nOn 23/03/2023 16:22, Oswald Buddenhagen wrote:\n> This is a preparatory series for the separately posted 'rebase --rewind' patch,\n> but I think it has value in itself.\n\nI had a hard time applying these patches. In the end I had success with \nchecking out next, applying \nhttps://lore.kernel.org/git/20230323162234.995514-1-oswald.buddenhagen@gmx.de \nand then applying this series. It is very helpful to detail the base \ncommit in the cover letter. A series like this should normally be based \non master see Documentation/SubmittingPatches.\n\nHaving applied the patches I'm unable to compile them with DEVELOPER=1 \n(see Documentation/CodingGuidelines)\n\nIn file included from log-tree.c:20:\nsequencer.h:7:6: error: ISO C forbids forward references to ‘enum’ types \n[-Werror=pedantic]\n 7 | enum rebase_action;\n | ^~~~~~~~~~~~~\nsequencer.h:140:34: error: ISO C forbids forward references to ‘enum’ \ntypes [-Werror=pedantic]\n 140 | enum rebase_action action);\n | ^~~~~~~~~~~~~\nsequencer.h:196:26: error: ISO C forbids forward references to ‘enum’ \ntypes [-Werror=pedantic]\n 196 | enum rebase_action action);\n | ^~~~~~~~~~~~~\n\nIn file included from ./cache.h:12,\n from ./builtin.h:6,\n from builtin/rebase.c:8:\nbuiltin/rebase.c: In function ‘cmd_rebase’:\nbuiltin/rebase.c:1246:95: error: left-hand operand of comma expression \nhas no effect [-Werror=unused-value]\n 1246 | \n(BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(action_names) == ACTION_LAST),\n | \n ^\n./trace2.h:158:69: note: in definition of macro ‘trace2_cmd_mode’\n 158 | #define trace2_cmd_mode(sv) trace2_cmd_mode_fl(__FILE__, \n__LINE__, (sv))\n | \n ^~\n\nsequencer.c: In function ‘todo_list_rearrange_squash’:\nsequencer.c:6346:23: error: operation on ‘items’ may be undefined \n[-Werror=sequence-point]\n 6346 | items = ALLOC_ARRAY(items, todo_list->nr);\n\n\nBest Wishes\n\nPhillip\n\n> \n> Oswald Buddenhagen (8):\n> rebase: simplify code related to imply_merge()\n> rebase: move parse_opt_keep_empty() down\n> sequencer: pass around rebase action explicitly\n> sequencer: create enum for edit_todo_list() return value\n> rebase: preserve interactive todo file on checkout failure\n> sequencer: simplify allocation of result array in\n> todo_list_rearrange_squash()\n> sequencer: pass `onto` to complete_action() as object-id\n> rebase: improve resumption from incorrect initial todo list\n> \n> builtin/rebase.c | 63 +++++++--------\n> builtin/revert.c | 3 +-\n> rebase-interactive.c | 36 ++++-----\n> rebase-interactive.h | 27 ++++++-\n> sequencer.c | 139 +++++++++++++++++++---------------\n> sequencer.h | 15 ++--\n> t/t3404-rebase-interactive.sh | 34 ++++++++-\n> 7 files changed, 196 insertions(+), 121 deletions(-)\n> \n"},{"id":"474190","messageId":"8a188876-c456-7269-28de-9ff406204030@dunelm.org.uk","threadId":"59451","inReplyTo":"20230323162235.995574-9-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 8/8] rebase: improve resumption from incorrect initial todo list","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-26T14:28:01Z","receivedAt":"2023-03-26T14:28:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nOn 23/03/2023 16:22, Oswald Buddenhagen wrote:\n> When the user butchers the todo file during rebase -i setup, the\n> --continue which would follow --edit-todo would have skipped the last\n> steps of the setup. Notably, this would bypass the fast-forward over\n> untouched picks (though the actual picking loop would still fast-forward\n> the commits, one by one).\n> \n> Fix this by splitting off the tail of complete_action() to a new\n> start_rebase() function and call that from sequencer_continue() when no\n> commands have been executed yet.\n> \n> More or less as a side effect, we no longer checkout `onto` before exiting\n> when the todo file is bad. \n\nI think the implications of this change deserve to be discussed in the \ncommit message. Three things spring to mind but there may be others I \nhaven't thought of\n\n - Previously when rebase stopped and handed control back to the user\n HEAD would have already been detached. This patch changes that\n meaning we can have an active rebase of a branch while that branch is\n checked out. What does \"git status\" show in this case? What does the\n shell prompt show? Will it confuse users?\n\n - Previously if the user created a commit before running \"rebase\n --continue\" we'd rebase on to that commit. Now that commit will be\n silently dropped.\n\n - Previously if the user checkout out another commit before running\n \"rebase --continue\" we'd rebase on to that commit. Now we we rebase\n on to the original \"onto\" commit.\n\n > This makes aborting cheaper and will simplify\n > things in a later change.\n\nGiven that we're stopping so the user can fix the problem and continue \nthe rebase I don't think optimizing for aborting is a convincing reason \nfor this change on its own.\n\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 62986a7b1b..00d3e19c62 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \t\treturn ret;\n> \t}\n> \tif (cmd == 'c')\n> -\t\treturn sequencer_continue(the_repository, opts);\n> +\t\treturn sequencer_continue(the_repository, opts,\n> +\t\t\t\t\t 0, NULL, NULL, NULL);\n\nIt's a bit unfortunate that we have to start passing all these extra \nparameters, could the sequencer read them itself in read_populate_opts()?\n\n> -int sequencer_continue(struct repository *r, struct replay_opts *opts)\n> +static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags,\n> +\t\t\tconst char *onto_name, const struct object_id *onto,\n> +\t\t\tconst struct object_id *orig_head, struct todo_list *todo_list);\n\nIt would be nice to avoid this forward declaration. I think you could do \nthat by adding a preparatory patch that moves either checkout_onto() or \nsequencer_continue()\n\n> @@ -6142,49 +6154,52 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \n> \t\treturn error(_(\"nothing to do\"));\n> \t} else if (res == EDIT_TODO_INCORRECT) {\n> -\t\tcheckout_onto(r, opts, onto_name, onto, orig_head);\n> \t\ttodo_list_release(&new_todo);\n> \n> \t\treturn -1;\n> \t}\n> \n> -\t/* Expand the commit IDs */\n> -\ttodo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);\n> -\tstrbuf_swap(&new_todo.buf, &buf2);\n> -\tstrbuf_release(&buf2);\n> -\tnew_todo.total_nr -= new_todo.nr;\n> -\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n> -\t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n> -\t\t new_todo.buf.buf);\n\nI don't think we need to move this code. If start_rebase() is called \nfrom sequencer_continue() the initial edit of the todo list failed and \nhas been fixed by running \"git rebase --edit-todo\". In that case the \noids have already been expanded on disc.\n\n> -\tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) {\n> -\t\ttodo_list_release(&new_todo);\n> -\t\treturn error(_(\"could not skip unnecessary pick commands\"));\n> -\t}\n> -\n> -\tif (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,\n> -\t\t\t\t flags & ~(TODO_LIST_SHORTEN_IDS), action)) {\n> -\t\ttodo_list_release(&new_todo);\n> -\t\treturn error_errno(_(\"could not write '%s'\"), todo_file);\n> -\t}\n> -\n> -\tres = -1;\n> -\n> -\tif (checkout_onto(r, opts, onto_name, onto, orig_head))\n> -\t\tgoto cleanup;\n> -\n> -\tif (require_clean_work_tree(r, \"rebase\", NULL, 1, 1))\n> -\t\tgoto cleanup;\n> -\n> -\ttodo_list_write_total_nr(&new_todo);\n> -\tres = pick_commits(r, &new_todo, opts);\n> -\n> -cleanup:\n> +\tres = start_rebase(r, opts, flags, onto_name, onto, orig_head, &new_todo);\n> \ttodo_list_release(&new_todo);\n> \n> \treturn res;\n> }\n> \n\n> +test_expect_success 'continue after bad first command' '\n> +\ttest_when_finished \"git rebase --abort ||:\" &&\n> +\tgit checkout primary^0 &&\n\nIf you want a specific commit it's better to use a tag name as those are \nfixed whereas the branches get rebased all over the place in this test file.\n\n> +\tgit reflog expire --expire=all HEAD &&\n\nIs this really necessary, can you pass -n to \"git reflog\" below?\n\n> +\t(\n> +\t\tset_fake_editor &&\n> +\t\ttest_must_fail env FAKE_LINES=\"bad 1 pick 1 pick 2 reword 3\" \\\n> +\t\t\tgit rebase -i HEAD~3 &&\n> +\t\ttest_cmp_rev HEAD primary &&\n> +\t\tFAKE_LINES=\"pick 2 pick 3 reword 4\" git rebase --edit-todo &&\n> +\t\tFAKE_COMMIT_MESSAGE=\"E_reworded\" git rebase --continue\n> +\t) &&\n> +\tgit reflog > reflog &&\n> +\ttest $(grep -c fast-forward reflog) = 1 &&\n\nUsing test_line_count would make test failures easier to debug.\n\n> +\ttest_cmp_rev HEAD~1 primary~1 &&\n> +\ttest \"$(git log -1 --format=%B)\" = \"E_reworded\"\n\nIt is slightly more work, but please use test_cmp for things like this \nas it makes it so much easier to debug test failures.\n\nBest Wishes\n\nPhillip\n\n> +'\n> +\n> +test_expect_success 'abort after bad first command' '\n> +\ttest_when_finished \"git rebase --abort ||:\" &&\n> +\tgit checkout primary^0 &&\n> +\t(\n> +\t\tset_fake_editor &&\n> +\t\ttest_must_fail env FAKE_LINES=\"bad 1 pick 1 pick 2 reword 3\" \\\n> +\t\t\tgit rebase -i HEAD~3\n> +\t) &&\n> +\tgit rebase --abort &&\n> +\ttest_cmp_rev HEAD primary\n> +'\n> +\n> test_expect_success 'tabs and spaces are accepted in the todolist' '\n> \trebase_setup_and_clean indented-comment &&\n> \twrite_script add-indent.sh <<-\\EOF &&\n"},{"id":"474931","messageId":"7663c59d-2bdd-56b2-dab9-3011f9398c09@gmail.com","threadId":"59451","inReplyTo":"f72b3820-124c-3e2c-30e2-ca3f46b74dc0@dunelm.org.uk","subject":"Re: [PATCH 0/8] sequencer refactoring","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-06T12:09:12Z","receivedAt":"2023-04-06T12:09:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 25/03/2023 11:08, Phillip Wood wrote:\n> Hi Oswald\n> \n> On 23/03/2023 16:22, Oswald Buddenhagen wrote:\n>> This is a preparatory series for the separately posted 'rebase \n>> --rewind' patch,\n>> but I think it has value in itself.\n> \n> I had a hard time applying these patches. In the end I had success with \n> checking out next, applying \n> https://lore.kernel.org/git/20230323162234.995514-1-oswald.buddenhagen@gmx.de and then applying this series. It is very helpful to detail the base commit in the cover letter. A series like this should normally be based on master see Documentation/SubmittingPatches.\n> \n> Having applied the patches I'm unable to compile them with DEVELOPER=1 \n> (see Documentation/CodingGuidelines)\n> \n> In file included from log-tree.c:20:\n> sequencer.h:7:6: error: ISO C forbids forward references to ‘enum’ types \n> [-Werror=pedantic]\n>     7 | enum rebase_action;\n>       |      ^~~~~~~~~~~~~\n> sequencer.h:140:34: error: ISO C forbids forward references to ‘enum’ \n> types [-Werror=pedantic]\n>   140 |                             enum rebase_action action);\n>       |                                  ^~~~~~~~~~~~~\n> sequencer.h:196:26: error: ISO C forbids forward references to ‘enum’ \n> types [-Werror=pedantic]\n>   196 |                     enum rebase_action action);\n>       |                          ^~~~~~~~~~~~~\n> \n> In file included from ./cache.h:12,\n>                  from ./builtin.h:6,\n>                  from builtin/rebase.c:8:\n> builtin/rebase.c: In function ‘cmd_rebase’:\n> builtin/rebase.c:1246:95: error: left-hand operand of comma expression \n> has no effect [-Werror=unused-value]\n>  1246 | (BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(action_names) == ACTION_LAST),\n>       |                               ^\n> ./trace2.h:158:69: note: in definition of macro ‘trace2_cmd_mode’\n>   158 | #define trace2_cmd_mode(sv) trace2_cmd_mode_fl(__FILE__, \n> __LINE__, (sv))\n>       |     ^~\n\nI think the errors above are best addressed by dropping patch 3 as I \ndon't think the benefit is worth the churn. You say that the existing \ncode is fragile but it is not that hard to follow and is battle tested \nand known to work. If you need to change things to support --rewind then \nit would be better to do so in a series that adds that option.\n\n> sequencer.c: In function ‘todo_list_rearrange_squash’:\n> sequencer.c:6346:23: error: operation on ‘items’ may be undefined \n> [-Werror=sequence-point]\n>  6346 |                 items = ALLOC_ARRAY(items, todo_list->nr);\n\nThis is easily fixed by deleting \"items =\" as ALLOC_ARRAY() does the \nassignment for us.\n\nAfter dropping patches 3 and 7 and fixing the ARROC_ARRAY() above all \nthe rebase tests pass for each commit and the CI passes - \nhttps://github.com/phillipwood/git/actions/runs/4627831184\n\nBest Wishes\n\nPhillip\n\n> \n> Best Wishes\n> \n> Phillip\n> \n>>\n>> Oswald Buddenhagen (8):\n>>    rebase: simplify code related to imply_merge()\n>>    rebase: move parse_opt_keep_empty() down\n>>    sequencer: pass around rebase action explicitly\n>>    sequencer: create enum for edit_todo_list() return value\n>>    rebase: preserve interactive todo file on checkout failure\n>>    sequencer: simplify allocation of result array in\n>>      todo_list_rearrange_squash()\n>>    sequencer: pass `onto` to complete_action() as object-id\n>>    rebase: improve resumption from incorrect initial todo list\n>>\n>>   builtin/rebase.c              |  63 +++++++--------\n>>   builtin/revert.c              |   3 +-\n>>   rebase-interactive.c          |  36 ++++-----\n>>   rebase-interactive.h          |  27 ++++++-\n>>   sequencer.c                   | 139 +++++++++++++++++++---------------\n>>   sequencer.h                   |  15 ++--\n>>   t/t3404-rebase-interactive.sh |  34 ++++++++-\n>>   7 files changed, 196 insertions(+), 121 deletions(-)\n>>\n\n"},{"id":"476114","messageId":"ZElEis+PLDYR+Jvr@ugly","threadId":"59451","inReplyTo":"8a188876-c456-7269-28de-9ff406204030@dunelm.org.uk","subject":"Re: [PATCH 8/8] rebase: improve resumption from incorrect initial todo list","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-04-26T15:34:34Z","receivedAt":"2023-04-26T15:34:41Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Sun, Mar 26, 2023 at 03:28:01PM +0100, Phillip Wood wrote:\n>On 23/03/2023 16:22, Oswald Buddenhagen wrote:\n>> When the user butchers the todo file during rebase -i setup, the\n>> --continue which would follow --edit-todo would have skipped the last\n>> steps of the setup. Notably, this would bypass the fast-forward over\n>> untouched picks (though the actual picking loop would still fast-forward\n>> the commits, one by one).\n>> \n>> Fix this by splitting off the tail of complete_action() to a new\n>> start_rebase() function and call that from sequencer_continue() when no\n>> commands have been executed yet.\n>> \n>> More or less as a side effect, we no longer checkout `onto` before exiting\n>> when the todo file is bad. \n>\n>I think the implications of this change deserve to be discussed in the \n>commit message. Three things spring to mind but there may be others I \n>haven't thought of\n>\n> - Previously when rebase stopped and handed control back to the user\n> HEAD would have already been detached. This patch changes that\n> meaning we can have an active rebase of a branch while that branch is\n> checked out. What does \"git status\" show in this case? What does the\n> shell prompt show? Will it confuse users?\n>\nthe failed state is identical to the \"still editing the initial todo\" \nstate as far as \"git status\" and the shell prompt are concerned. this \nseems reasonable. i'll add it to the commit message.\n\n> - Previously if the user created a commit before running \"rebase\n> --continue\" we'd rebase on to that commit. Now that commit will be\n> silently dropped.\n>\nthis is arguably a problem, but not much different from the pre-existing \nbehavior of changes to HEAD done during the initial todo edit being \nlost.\nto avoid that, we'd need to lock HEAD while editing the todo. is that \nrealistic at all?\non top of that, i should verify HEAD against orig-head in \nstart_rebase(). though the only way for the user to get out of that \nsituation is saving the todo contents and --abort'ing (and we must take \ncare not the touch HEAD).\n\nthis is somewhat similar to the abysmal situation of the final \nupdate-ref failing if the target ref has been modified while being \nrebased. we'd need to lock that ref for the entire duration of the \nrebase to avoid that.\n\n> - Previously if the user checkout out another commit before running\n> \"rebase --continue\" we'd rebase on to that commit. Now we we rebase\n> on to the original \"onto\" commit.\n>\nthis can be subsumed into the above case.\n\n> > This makes aborting cheaper and will simplify\n> > things in a later change.\n>\n>Given that we're stopping so the user can fix the problem and continue \n>the rebase I don't think optimizing for aborting is a convincing reason \n>for this change on its own.\n>\nthis is all part of the \"More or less as a side effect\" paragraph, so \nthis isn't a relevant objection.\n\n>> diff --git a/builtin/revert.c b/builtin/revert.c\n>> index 62986a7b1b..00d3e19c62 100644\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n>> @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n>> \t\treturn ret;\n>> \t}\n>> \tif (cmd == 'c')\n>> -\t\treturn sequencer_continue(the_repository, opts);\n>> +\t\treturn sequencer_continue(the_repository, opts,\n>> +\t\t\t\t\t 0, NULL, NULL, NULL);\n>\n>It's a bit unfortunate that we have to start passing all these extra \n>parameters, could the sequencer read them itself in read_populate_opts()?\n>\nthat wouldn't help in this case, as these are dummy values which aren't \ngoing to be used.\n\nbut more broadly, the whole state management is a total mess. i have \nthis notes-to-self patch on top of my local branch:\n\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -476,6 +476,7 @@ static const char *state_dir_path(const char *filename, struct rebase_options *o\n }\n\n /* Initialize the rebase options from the state directory. */\n+// FIXME: this is partly redundant with the sequencer's read_populate_opts().\n static int read_basic_state(struct rebase_options *opts)\n {\n struct strbuf head_name = STRBUF_INIT;\n@@ -552,6 +553,7 @@ static int read_basic_state(struct rebase_options *opts)\n return 0;\n }\n\n+// This is written only by the apply backend\n static int rebase_write_basic_state(struct rebase_options *opts)\n {\n write_file(state_dir_path(\"head-name\", opts), \"%s\",\n\n\n>> -int sequencer_continue(struct repository *r, struct replay_opts *opts)\n>> +static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags,\n>> +\t\t\tconst char *onto_name, const struct object_id *onto,\n>> +\t\t\tconst struct object_id *orig_head, struct todo_list *todo_list);\n>\n>It would be nice to avoid this forward declaration. I think you could do \n>that by adding a preparatory patch that moves either checkout_onto() or \n>sequencer_continue()\n>\ni went for the \"minimal churn\" approach.\n\nbut more broadly, the code distribution between rebase.c and sequencer.c \nneeds a *major* re-think. moving these functions into place could be \npart of that effort.\n\n>> +\tgit reflog expire --expire=all HEAD &&\n>\n>Is this really necessary, can you pass -n to \"git reflog\" below?\n>\nstarting from a clean slate makes it more straight-forward to make it\nreliable. i don't see any real downsides to the approach.\n\n>> +\tgit reflog > reflog &&\n>> +\ttest $(grep -c fast-forward reflog) = 1 &&\n>\n>Using test_line_count would make test failures easier to debug.\n>\nthat's calling for a new test_filtered_line_count function which would \nhave quite some users.\nfor the time being, both grep + test_line_count and grep -c are rather \nprevalent, in this file the latter in particular.\n\n>> +\ttest_cmp_rev HEAD~1 primary~1 &&\n>> +\ttest \"$(git log -1 --format=%B)\" = \"E_reworded\"\n>\n>It is slightly more work, but please use test_cmp for things like this \n>as it makes it so much easier to debug test failures.\n>\nfair enough, but the precedents again speak a different language.\n\nregards\n"},{"id":"477446","messageId":"08c4c313-35c8-63e9-7d66-a35b24a449dd@gmail.com","threadId":"59451","inReplyTo":"ZElEis+PLDYR+Jvr@ugly","subject":"Re: [PATCH 8/8] rebase: improve resumption from incorrect initial todo list","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-05-17T12:13:28Z","receivedAt":"2023-05-17T12:13:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nOn 26/04/2023 16:34, Oswald Buddenhagen wrote:\n> On Sun, Mar 26, 2023 at 03:28:01PM +0100, Phillip Wood wrote:\n>> On 23/03/2023 16:22, Oswald Buddenhagen wrote:\n>>> When the user butchers the todo file during rebase -i setup, the\n>>> --continue which would follow --edit-todo would have skipped the last\n>>> steps of the setup. Notably, this would bypass the fast-forward over\n>>> untouched picks (though the actual picking loop would still fast-forward\n>>> the commits, one by one).\n>>>\n>>> Fix this by splitting off the tail of complete_action() to a new\n>>> start_rebase() function and call that from sequencer_continue() when no\n>>> commands have been executed yet.\n>>>\n>>> More or less as a side effect, we no longer checkout `onto` before \n>>> exiting\n>>> when the todo file is bad. \n>>\n>> I think the implications of this change deserve to be discussed in the \n>> commit message. Three things spring to mind but there may be others I \n>> haven't thought of\n>>\n>>  - Previously when rebase stopped and handed control back to the user\n>>    HEAD would have already been detached. This patch changes that\n>>    meaning we can have an active rebase of a branch while that branch is\n>>    checked out. What does \"git status\" show in this case? What does the\n>>    shell prompt show? Will it confuse users?\n>>\n> the failed state is identical to the \"still editing the initial todo\" \n> state as far as \"git status\" and the shell prompt are concerned. this \n> seems reasonable. i'll add it to the commit message.\n\nWhen you do that please mention what \"git status\" and the shell prompt \nactually print in this case. Ideally \"git status\" should mention that \nthe todo list needs to be edited if there are still errors in it, though \nit would not surprise me if it is not that helpful at the moment.\n\n>>  - Previously if the user created a commit before running \"rebase\n>>    --continue\" we'd rebase on to that commit. Now that commit will be\n>>    silently dropped.\n>>\n> this is arguably a problem, but not much different from the pre-existing \n> behavior of changes to HEAD done during the initial todo edit being lost.\n\nI think there is a significant difference in that we're moving from a \nsituation where we lose commits that are created while rebase is running \nto one where we're losing commits created while rebase is stopped. If a \nuser tries to create a commit while rebase is running then they're \nasking for trouble. I don't think creating commits when rebase is \nstopped is unreasonable in the same way.\n\n> to avoid that, we'd need to lock HEAD while editing the todo. is that \n> realistic at all?\n\nI don't think it is practical to lock HEAD while git is not running. We \ncould just check HEAD has not changed when the rebase continues after \nthe user has fixed the todo list as you suggest below.\n\n> on top of that, i should verify HEAD against orig-head in \n> start_rebase(). though the only way for the user to get out of that \n> situation is saving the todo contents and --abort'ing (and we must take \n> care not the touch HEAD).\n\nI think in that case it wouldn't be terrible to lose the edited todo \nlist as it is a bit of a corner case. The simplest thing to do would be \nto print an error and remove .git/rebase-merge.\n\n> this is somewhat similar to the abysmal situation of the final \n> update-ref failing if the target ref has been modified while being \n> rebased. we'd need to lock that ref for the entire duration of the \n> rebase to avoid that.\n\n\"abysmal\" is rather harsh - it would also be bad to overwrite the ref in \nthat case. I think it in relatively hard to get into that situation \nthough as \"git checkout\" wont checkout a branch that is being updated by \na rebase.\n\n>>  - Previously if the user checkout out another commit before running\n>>    \"rebase --continue\" we'd rebase on to that commit. Now we we rebase\n>>    on to the original \"onto\" commit.\n>>\n> this can be subsumed into the above case.\n\nMeaning check and error out if HEAD has changed?\n\n>> > This makes aborting cheaper and will simplify\n>> > things in a later change.\n>>\n>> Given that we're stopping so the user can fix the problem and continue \n>> the rebase I don't think optimizing for aborting is a convincing \n>> reason for this change on its own.\n>>\n> this is all part of the \"More or less as a side effect\" paragraph, so \n> this isn't a relevant objection.\n\nI'm simply saying that we should not be optimizing for \"rebase --abort\" \nin this case. Do you think we should?\n\n>>> diff --git a/builtin/revert.c b/builtin/revert.c\n>>> index 62986a7b1b..00d3e19c62 100644\n>>> --- a/builtin/revert.c\n>>> +++ b/builtin/revert.c\n>>> @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char \n>>> **argv, struct replay_opts *opts)\n>>>           return ret;\n>>>       }\n>>>       if (cmd == 'c')\n>>> -        return sequencer_continue(the_repository, opts);\n>>> +        return sequencer_continue(the_repository, opts,\n>>> +                      0, NULL, NULL, NULL);\n>>\n>> It's a bit unfortunate that we have to start passing all these extra \n>> parameters, could the sequencer read them itself in read_populate_opts()?\n>>\n> that wouldn't help in this case, as these are dummy values which aren't \n> going to be used.\n\nIf we only need to pass these when rebasing maybe we should have \nseparate wrappers for continuing a rebase and a cherry-pick/revert. If \nwe don't always need these parameters when continuing a rebase we could \nhave a separate function when these parameters are required and leave \nthe signature of sequencer_continue() unchanged.\n\n> but more broadly, the whole state management is a total mess.\n\nFor historic reasons there are separate functions to write the state for \nthe \"merge\" backed and the \"apply\" backend. That is not ideal but it is \nhardly a \"total mess\". The code for reading the state files is more \ncontrived than the code that writes them. I do have some patches to try \nand reduce the duplication when reading the state files.\n\n\n>>> -int sequencer_continue(struct repository *r, struct replay_opts *opts)\n>>> +static int start_rebase(struct repository *r, struct replay_opts \n>>> *opts, unsigned flags,\n>>> +            const char *onto_name, const struct object_id *onto,\n>>> +            const struct object_id *orig_head, struct todo_list \n>>> *todo_list);\n>>\n>> It would be nice to avoid this forward declaration. I think you could \n>> do that by adding a preparatory patch that moves either \n>> checkout_onto() or sequencer_continue()\n>>\n> i went for the \"minimal churn\" approach.\n\nThere is a balance to be had, but we don't want to build up a lot of \nforward declarations over time just because it is easier than moving the \nfunction in a preparatory patch. A simple patch to move a function is \neasy to review with --color-moved.\n\n>>> +    git reflog > reflog &&\n>>> +    test $(grep -c fast-forward reflog) = 1 &&\n>>\n>> Using test_line_count would make test failures easier to debug.\n>>\n> that's calling for a new test_filtered_line_count function which would \n> have quite some users.\n> for the time being, both grep + test_line_count and grep -c are rather \n> prevalent, in this file the latter in particular.\n\nThe style of our tests has evolved over time. When adding new tests it \nis better to focus on making them easy to debug. I don't think you need \nto add a new function here, just\n\n\tgrep fast-forward reflog >filtered-reflog\n\ttest_line_count = 1 filtered-reflog\n\n>>> +    test_cmp_rev HEAD~1 primary~1 &&\n>>> +    test \"$(git log -1 --format=%B)\" = \"E_reworded\"\n>>\n>> It is slightly more work, but please use test_cmp for things like this \n>> as it makes it so much easier to debug test failures.\n>>\n> fair enough, but the precedents again speak a different language.\n\nYes older tests tend to be written in a style that is harder to debug.\n\nBest Wishes\n\nPhillip\n"},{"id":"477447","messageId":"a6e31eb9-81e7-4d7f-28cc-73b5e46525a4@gmail.com","threadId":"59451","inReplyTo":"20230323162235.995574-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 0/8] sequencer refactoring","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-05-17T13:10:20Z","receivedAt":"2023-05-17T13:10:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nOn 23/03/2023 16:22, Oswald Buddenhagen wrote:\n> This is a preparatory series for the separately posted 'rebase --rewind' patch,\n> but I think it has value in itself.\n\nWhen you re-roll this I think it would be worth splitting it into two \nseparate series.\n\nPatches 1, 2, 4 & 6 are simple clean ups which don't need much work \nbeyond making sure that (a) the commit messages have a good explanation \nof the reason for the change (try \"git log --author \"Jeff King\" for \nexamples of good commit messages) and (b) the code follows our coding \nguidelines (mostly no '//' comments if I remember correctly).\n\nPatches 5 & 8 address real problems but are more involved and it will \ntake more time to consider the UI changes and get them right.\n\nI'd rather we dropped patches 3 & 7.\n\nBest Wishes\n\nPhillip\n\n> \n> Oswald Buddenhagen (8):\n> rebase: simplify code related to imply_merge()\n> rebase: move parse_opt_keep_empty() down\n> sequencer: pass around rebase action explicitly\n> sequencer: create enum for edit_todo_list() return value\n> rebase: preserve interactive todo file on checkout failure\n> sequencer: simplify allocation of result array in\n> todo_list_rearrange_squash()\n> sequencer: pass `onto` to complete_action() as object-id\n> rebase: improve resumption from incorrect initial todo list\n> \n> builtin/rebase.c | 63 +++++++--------\n> builtin/revert.c | 3 +-\n> rebase-interactive.c | 36 ++++-----\n> rebase-interactive.h | 27 ++++++-\n> sequencer.c | 139 +++++++++++++++++++---------------\n> sequencer.h | 15 ++--\n> t/t3404-rebase-interactive.sh | 34 ++++++++-\n> 7 files changed, 196 insertions(+), 121 deletions(-)\n> \n\n"},{"id":"480374","messageId":"20230809171531.2564844-2-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20230809171531.2564844-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v2 1/3] rebase: simplify code related to imply_merge()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-09T17:15:29Z","receivedAt":"2023-08-09T17:15:46Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"The code's evolution left in some bits surrounding enum rebase_type that\ndon't really make sense any more. In particular, it makes no sense to\ninvoke imply_merge() if the type is already known not to be\nREBASE_APPLY, and it makes no sense to assign the type after calling\nimply_merge().\n\nenum rebase_type had more values until commit a74b35081c (\"rebase: drop\nsupport for `--preserve-merges`\") and commit 10cdb9f38a (\"rebase: rename\nthe two primary rebase backends\"). The latter commit also renamed\nimply_interactive() to imply_merge().\n\nSigned-off-by: Oswald Buddenhagen \n\n---\nv2:\n- more verbose commit message\n\nCc: Junio C Hamano \nCc: Phillip Wood \n---\n builtin/rebase.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 50cb85751f..44cc1eed12 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -386,7 +386,6 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n \n \timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n \topts->keep_empty = !unset;\n-\topts->type = REBASE_MERGE;\n \treturn 0;\n }\n \n@@ -1505,9 +1504,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (options.type == REBASE_MERGE)\n-\t\timply_merge(&options, \"--merge\");\n-\n \tif (options.root && !options.onto_name)\n \t\timply_merge(&options, \"--root without --onto\");\n \n@@ -1552,7 +1548,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \tif (options.type == REBASE_UNSPECIFIED) {\n \t\tif (!strcmp(options.default_backend, \"merge\"))\n-\t\t\timply_merge(&options, \"--merge\");\n+\t\t\toptions.type = REBASE_MERGE;\n \t\telse if (!strcmp(options.default_backend, \"apply\"))\n \t\t\toptions.type = REBASE_APPLY;\n \t\telse\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"480378","messageId":"20230809171531.2564844-3-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20230809171531.2564844-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v2 2/3] rebase: handle --strategy via imply_merge() as well","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-09T17:15:30Z","receivedAt":"2023-08-09T17:15:49Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"At least after the successive trimming of enum rebase_type mentioned in\nthe previous commit, this code did exactly what imply_merge() does, so\njust call it instead.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Oswald Buddenhagen \n\n---\nCc: Phillip Wood \n---\n builtin/rebase.c | 13 +------------\n 1 file changed, 1 insertion(+), 12 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 44cc1eed12..4a093bb125 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1490,18 +1490,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \tif (options.strategy) {\n \t\toptions.strategy = xstrdup(options.strategy);\n-\t\tswitch (options.type) {\n-\t\tcase REBASE_APPLY:\n-\t\t\tdie(_(\"--strategy requires --merge or --interactive\"));\n-\t\tcase REBASE_MERGE:\n-\t\t\t/* compatible */\n-\t\t\tbreak;\n-\t\tcase REBASE_UNSPECIFIED:\n-\t\t\toptions.type = REBASE_MERGE;\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tBUG(\"unhandled rebase type (%d)\", options.type);\n-\t\t}\n+\t\timply_merge(&options, \"--strategy\");\n \t}\n \n \tif (options.root && !options.onto_name)\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"480379","messageId":"20230809171531.2564844-1-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"xmqqiler8cga.fsf@gitster.g","subject":"[PATCH v2 0/3] rebase refactoring","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-09T17:15:28Z","receivedAt":"2023-08-09T17:15:50Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"broken out of the bigger series, as the aggregation just unnecessarily holds it\nup.\n\nOswald Buddenhagen (3):\n rebase: simplify code related to imply_merge()\n rebase: handle --strategy via imply_merge() as well\n rebase: move parse_opt_keep_empty() down\n\n builtin/rebase.c | 44 ++++++++++++++------------------------------\n 1 file changed, 14 insertions(+), 30 deletions(-)\n\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"480381","messageId":"20230809171531.2564844-4-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20230809171531.2564844-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v2 3/3] rebase: move parse_opt_keep_empty() down","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-09T17:15:31Z","receivedAt":"2023-08-09T17:15:52Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"This moves it right next to parse_opt_empty(), which is a much more\nlogical place. As a side effect, this removes the need for a forward\ndeclaration of imply_merge().\n\nAcked-by: Phillip Wood \nSigned-off-by: Oswald Buddenhagen \n\n---\ni'm not sure how to \"translate\" phillip's informal approval; the\nacked-by doesn't seem quite right. please adjust as necessary.\n\nCc: Junio C Hamano \n---\n builtin/rebase.c | 25 ++++++++++++-------------\n 1 file changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 4a093bb125..13ca5a644b 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -376,19 +376,6 @@ static int run_sequencer_rebase(struct rebase_options *opts)\n \treturn ret;\n }\n \n-static void imply_merge(struct rebase_options *opts, const char *option);\n-static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n-\t\t\t\tint unset)\n-{\n-\tstruct rebase_options *opts = opt->value;\n-\n-\tBUG_ON_OPT_ARG(arg);\n-\n-\timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n-\topts->keep_empty = !unset;\n-\treturn 0;\n-}\n-\n static int is_merge(struct rebase_options *opts)\n {\n \treturn opts->type == REBASE_MERGE;\n@@ -982,6 +969,18 @@ static enum empty_type parse_empty_value(const char *value)\n \tdie(_(\"unrecognized empty type '%s'; valid values are \\\"drop\\\", \\\"keep\\\", and \\\"ask\\\".\"), value);\n }\n \n+static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n+\t\t\t\tint unset)\n+{\n+\tstruct rebase_options *opts = opt->value;\n+\n+\tBUG_ON_OPT_ARG(arg);\n+\n+\timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n+\topts->keep_empty = !unset;\n+\treturn 0;\n+}\n+\n static int parse_opt_empty(const struct option *opt, const char *arg, int unset)\n {\n \tstruct rebase_options *options = opt->value;\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"480675","messageId":"8b629666-ca43-4a38-ba70-1c017d547984@gmail.com","threadId":"59451","inReplyTo":"20230809171531.2564844-4-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH v2 3/3] rebase: move parse_opt_keep_empty() down","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-15T14:01:36Z","receivedAt":"2023-08-15T14:02:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nOn 09/08/2023 18:15, Oswald Buddenhagen wrote:\n> This moves it right next to parse_opt_empty(), which is a much more\n> logical place. As a side effect, this removes the need for a forward\n> declaration of imply_merge().\n> \n> Acked-by: Phillip Wood \n> Signed-off-by: Oswald Buddenhagen \n> \n> ---\n> i'm not sure how to \"translate\" phillip's informal approval; the\n> acked-by doesn't seem quite right. please adjust as necessary.\n\nI think we should just delete that trailer, I don't approve of this \nchange any more strongly than I do the rest of the series - they all \nlook like useful improvements to me, thanks for working on them.\n\nBest Wishes\n\nPhillip\n\n> Cc: Junio C Hamano \n> ---\n> builtin/rebase.c | 25 ++++++++++++-------------\n> 1 file changed, 12 insertions(+), 13 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 4a093bb125..13ca5a644b 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -376,19 +376,6 @@ static int run_sequencer_rebase(struct rebase_options *opts)\n> \treturn ret;\n> }\n> \n> -static void imply_merge(struct rebase_options *opts, const char *option);\n> -static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n> -\t\t\t\tint unset)\n> -{\n> -\tstruct rebase_options *opts = opt->value;\n> -\n> -\tBUG_ON_OPT_ARG(arg);\n> -\n> -\timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n> -\topts->keep_empty = !unset;\n> -\treturn 0;\n> -}\n> -\n> static int is_merge(struct rebase_options *opts)\n> {\n> \treturn opts->type == REBASE_MERGE;\n> @@ -982,6 +969,18 @@ static enum empty_type parse_empty_value(const char *value)\n> \tdie(_(\"unrecognized empty type '%s'; valid values are \\\"drop\\\", \\\"keep\\\", and \\\"ask\\\".\"), value);\n> }\n> \n> +static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n> +\t\t\t\tint unset)\n> +{\n> +\tstruct rebase_options *opts = opt->value;\n> +\n> +\tBUG_ON_OPT_ARG(arg);\n> +\n> +\timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n> +\topts->keep_empty = !unset;\n> +\treturn 0;\n> +}\n> +\n> static int parse_opt_empty(const struct option *opt, const char *arg, int unset)\n> {\n> \tstruct rebase_options *options = opt->value;\n"},{"id":"481001","messageId":"ZOeJWODUB4QeLaNP@ugly","threadId":"59451","inReplyTo":"08c4c313-35c8-63e9-7d66-a35b24a449dd@gmail.com","subject":"Re: [PATCH 8/8] rebase: improve resumption from incorrect initial todo list","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-24T16:46:16Z","receivedAt":"2023-08-24T16:47:08Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Wed, May 17, 2023 at 01:13:28PM +0100, Phillip Wood wrote:\n>On 26/04/2023 16:34, Oswald Buddenhagen wrote:\n>> the failed state is identical to the \"still editing the initial todo\" \n>> state as far as \"git status\" and the shell prompt are concerned. this \n>> seems reasonable. i'll add it to the commit message.\n>\n>When you do that please mention what \"git status\" and the shell prompt \n>actually print in this case.\n>\ni'll go with \"This seems reasonable, irrespective of the actual \npresentation (which could be improved)\".\n\n>Ideally \"git status\" should mention that the todo list needs to be \n>edited if there are still errors in it, though it would not surprise me \n>if it is not that helpful at the moment.\n>\nthat would require actually validating the todo instead of just printing \nit. or maybe the presence of the backup file could be used to make \nreliable inferences. have fun! ;)\n\n>>>  - Previously if the user created a commit before running \"rebase\n>>>    --continue\" we'd rebase on to that commit. Now that commit will be\n>>>    silently dropped.\n>>>\n>> this is arguably a problem, but not much different from the pre-existing \n>> behavior of changes to HEAD done during the initial todo edit being lost.\n>\n>I think there is a significant difference in that we're moving from a \n>situation where we lose commits that are created while rebase is running \n>to one where we're losing commits created while rebase is stopped. If a \n>user tries to create a commit while rebase is running then they're \n>asking for trouble. I don't think creating commits when rebase is \n>stopped is unreasonable in the same way.\n>\ni think that this is a completely meaningless distinction. a rebase is \n\"running\" while the state directory exists. having multiple terminals \nopen is the norm, and when havoc ensues it doesn't matter to the user \nwhether one of the terminals had an editor launched by git open at the \ntime.\n\n>> to avoid that, we'd need to lock HEAD while editing the todo. is that \n>> realistic at all?\n>\n>I don't think it is practical to lock HEAD while git is not running.\n>\nwhat measure of \"practical\" are you applying?\ni'm assuming that no persistent locking infra exists currently. but i \ndon't see a reason why it _couldn't_ - having some functions to populate \nand query .git/locked-refs/** in the right places doesn't seem like a \nfundamentally hard problem.\n\n>We could just check HEAD has not changed when the rebase continues \n>after the user has fixed the todo list as you suggest below.\n>\nthat's a good safeguard which i intend to implement, but when it \ntriggers, the user will have to deal with the conflict. it would be much \nnicer to avoid it in the first place.\n\n>> on top of that, i should verify HEAD against orig-head in \n>> start_rebase(). though the only way for the user to get out of that \n>> situation is saving the todo contents and --abort'ing (and we must \n>> take care not the touch HEAD).\n>\n>I think in that case it wouldn't be terrible to lose the edited todo \n>list as it is a bit of a corner case.\n>\nactually, yes, it would be. that's why i posted a patch that avoids it.\n\n>> this is somewhat similar to the abysmal situation of the final \n>> update-ref failing if the target ref has been modified while being \n>> rebased. we'd need to lock that ref for the entire duration of the \n>> rebase to avoid that.\n>\n>\"abysmal\" is rather harsh - it would also be bad to overwrite the ref in \n>that case. I think it in relatively hard to get into that situation \n>though as \"git checkout\" wont checkout a branch that is being updated by \n>a rebase.\n>\ni have no clue how it happened (certainly something to do with many open \nterminals), but i actually got into that situation shortly before \nwriting that mail, and i assure you that \"abysmal\" is absolutely not an \noverstatement. i mean, what do you expect a user to think when presented \nwith two diverging heads when trying to finish a rebase?\n\n>>>  - Previously if the user checkout out another commit before running\n>>>    \"rebase --continue\" we'd rebase on to that commit. Now we we rebase\n>>>    on to the original \"onto\" commit.\n>>>\n>> this can be subsumed into the above case.\n>\n>Meaning check and error out if HEAD has changed?\n>\nyes\n\n>>> > This makes aborting cheaper and will simplify\n>>> > things in a later change.\n>>>\n>>> Given that we're stopping so the user can fix the problem and continue \n>>> the rebase I don't think optimizing for aborting is a convincing \n>>> reason for this change on its own.\n>>>\n>> this is all part of the \"More or less as a side effect\" paragraph, so \n>> this isn't a relevant objection.\n>\n>I'm simply saying that we should not be optimizing for \"rebase --abort\" \n>in this case. Do you think we should?\n>\nyou're missing the point. the optimization isn't something anyone aimed \nfor.\n\nregards\n"},{"id":"483548","messageId":"20231020093654.922890-1-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20230809171531.2564844-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v3 0/3] rebase refactoring","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-10-20T09:36:51Z","receivedAt":"2023-10-20T09:36:57Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"broken out of the bigger series, as the aggregation just unnecessarily holds it\nup.\n\nv3: removed \"stray\" footer. so more of a RESEND than an actual new version.\n\nOswald Buddenhagen (3):\n rebase: simplify code related to imply_merge()\n rebase: handle --strategy via imply_merge() as well\n rebase: move parse_opt_keep_empty() down\n\n builtin/rebase.c | 44 ++++++++++++++------------------------------\n 1 file changed, 14 insertions(+), 30 deletions(-)\n\n-- \n2.42.0.419.g70bf8a5751\n\n"},{"id":"483549","messageId":"20231020093654.922890-3-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20231020093654.922890-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-10-20T09:36:53Z","receivedAt":"2023-10-20T09:36:57Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"At least after the successive trimming of enum rebase_type mentioned in\nthe previous commit, this code did exactly what imply_merge() does, so\njust call it instead.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Oswald Buddenhagen \n\n---\nCc: Phillip Wood \n---\n builtin/rebase.c | 13 +------------\n 1 file changed, 1 insertion(+), 12 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 44cc1eed12..4a093bb125 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1490,18 +1490,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \tif (options.strategy) {\n \t\toptions.strategy = xstrdup(options.strategy);\n-\t\tswitch (options.type) {\n-\t\tcase REBASE_APPLY:\n-\t\t\tdie(_(\"--strategy requires --merge or --interactive\"));\n-\t\tcase REBASE_MERGE:\n-\t\t\t/* compatible */\n-\t\t\tbreak;\n-\t\tcase REBASE_UNSPECIFIED:\n-\t\t\toptions.type = REBASE_MERGE;\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tBUG(\"unhandled rebase type (%d)\", options.type);\n-\t\t}\n+\t\timply_merge(&options, \"--strategy\");\n \t}\n \n \tif (options.root && !options.onto_name)\n-- \n2.42.0.419.g70bf8a5751\n\n"},{"id":"483550","messageId":"20231020093654.922890-4-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20231020093654.922890-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v3 3/3] rebase: move parse_opt_keep_empty() down","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-10-20T09:36:54Z","receivedAt":"2023-10-20T09:36:57Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"This moves it right next to parse_opt_empty(), which is a much more\nlogical place. As a side effect, this removes the need for a forward\ndeclaration of imply_merge().\n\nSigned-off-by: Oswald Buddenhagen \n\n---\nCc: Junio C Hamano \nCc: Phillip Wood \n---\n builtin/rebase.c | 25 ++++++++++++-------------\n 1 file changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 4a093bb125..13ca5a644b 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -376,19 +376,6 @@ static int run_sequencer_rebase(struct rebase_options *opts)\n \treturn ret;\n }\n \n-static void imply_merge(struct rebase_options *opts, const char *option);\n-static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n-\t\t\t\tint unset)\n-{\n-\tstruct rebase_options *opts = opt->value;\n-\n-\tBUG_ON_OPT_ARG(arg);\n-\n-\timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n-\topts->keep_empty = !unset;\n-\treturn 0;\n-}\n-\n static int is_merge(struct rebase_options *opts)\n {\n \treturn opts->type == REBASE_MERGE;\n@@ -982,6 +969,18 @@ static enum empty_type parse_empty_value(const char *value)\n \tdie(_(\"unrecognized empty type '%s'; valid values are \\\"drop\\\", \\\"keep\\\", and \\\"ask\\\".\"), value);\n }\n \n+static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n+\t\t\t\tint unset)\n+{\n+\tstruct rebase_options *opts = opt->value;\n+\n+\tBUG_ON_OPT_ARG(arg);\n+\n+\timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n+\topts->keep_empty = !unset;\n+\treturn 0;\n+}\n+\n static int parse_opt_empty(const struct option *opt, const char *arg, int unset)\n {\n \tstruct rebase_options *options = opt->value;\n-- \n2.42.0.419.g70bf8a5751\n\n"},{"id":"483551","messageId":"20231020093654.922890-2-oswald.buddenhagen@gmx.de","threadId":"59451","inReplyTo":"20231020093654.922890-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v3 1/3] rebase: simplify code related to imply_merge()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-10-20T09:36:52Z","receivedAt":"2023-10-20T09:36:57Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"The code's evolution left in some bits surrounding enum rebase_type that\ndon't really make sense any more. In particular, it makes no sense to\ninvoke imply_merge() if the type is already known not to be\nREBASE_APPLY, and it makes no sense to assign the type after calling\nimply_merge().\n\nenum rebase_type had more values until commit a74b35081c (\"rebase: drop\nsupport for `--preserve-merges`\") and commit 10cdb9f38a (\"rebase: rename\nthe two primary rebase backends\"). The latter commit also renamed\nimply_interactive() to imply_merge().\n\nSigned-off-by: Oswald Buddenhagen \n\n---\nv2:\n- more verbose commit message\n\nCc: Junio C Hamano \nCc: Phillip Wood \n---\n builtin/rebase.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 50cb85751f..44cc1eed12 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -386,7 +386,6 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,\n \n \timply_merge(opts, unset ? \"--no-keep-empty\" : \"--keep-empty\");\n \topts->keep_empty = !unset;\n-\topts->type = REBASE_MERGE;\n \treturn 0;\n }\n \n@@ -1505,9 +1504,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (options.type == REBASE_MERGE)\n-\t\timply_merge(&options, \"--merge\");\n-\n \tif (options.root && !options.onto_name)\n \t\timply_merge(&options, \"--root without --onto\");\n \n@@ -1552,7 +1548,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \tif (options.type == REBASE_UNSPECIFIED) {\n \t\tif (!strcmp(options.default_backend, \"merge\"))\n-\t\t\timply_merge(&options, \"--merge\");\n+\t\t\toptions.type = REBASE_MERGE;\n \t\telse if (!strcmp(options.default_backend, \"apply\"))\n \t\t\toptions.type = REBASE_APPLY;\n \t\telse\n-- \n2.42.0.419.g70bf8a5751\n\n"},{"id":"483606","messageId":"xmqqa5sdotcy.fsf@gitster.g","threadId":"59451","inReplyTo":"20231020093654.922890-3-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-20T21:51:25Z","receivedAt":"2023-10-20T21:51:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> At least after the successive trimming of enum rebase_type mentioned in\n> the previous commit, this code did exactly what imply_merge() does, so\n> just call it instead.\n>\n> Suggested-by: Junio C Hamano \n> Signed-off-by: Oswald Buddenhagen \n\nHmph, I do not recall suggesting it, but the resulting code does\nmake sense. ;-)\n\n>\n> ---\n> Cc: Phillip Wood \n> ---\n> builtin/rebase.c | 13 +------------\n> 1 file changed, 1 insertion(+), 12 deletions(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 44cc1eed12..4a093bb125 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1490,18 +1490,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \n> \tif (options.strategy) {\n> \t\toptions.strategy = xstrdup(options.strategy);\n> -\t\tswitch (options.type) {\n> -\t\tcase REBASE_APPLY:\n> -\t\t\tdie(_(\"--strategy requires --merge or --interactive\"));\n> -\t\tcase REBASE_MERGE:\n> -\t\t\t/* compatible */\n> -\t\t\tbreak;\n> -\t\tcase REBASE_UNSPECIFIED:\n> -\t\t\toptions.type = REBASE_MERGE;\n> -\t\t\tbreak;\n> -\t\tdefault:\n> -\t\t\tBUG(\"unhandled rebase type (%d)\", options.type);\n> -\t\t}\n> +\t\timply_merge(&options, \"--strategy\");\n> \t}\n> \n> \tif (options.root && !options.onto_name)\n"},{"id":"483607","messageId":"xmqq5y31osmg.fsf@gitster.g","threadId":"59451","inReplyTo":"20231020093654.922890-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH v3 0/3] rebase refactoring","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-20T22:07:19Z","receivedAt":"2023-10-20T22:07:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> broken out of the bigger series, as the aggregation just unnecessarily holds it\n> up.\n>\n> v3: removed \"stray\" footer. so more of a RESEND than an actual new version.\n>\n> Oswald Buddenhagen (3):\n> rebase: simplify code related to imply_merge()\n> rebase: handle --strategy via imply_merge() as well\n> rebase: move parse_opt_keep_empty() down\n>\n> builtin/rebase.c | 44 ++++++++++++++------------------------------\n> 1 file changed, 14 insertions(+), 30 deletions(-)\n\nLooking quite straight-forward and I didn't see anythihng\npotentially controversial.\n\nWill queue. Thanks.\n"},{"id":"483681","messageId":"4f8616d1-35f2-418d-9d28-b230ca45090d@gmail.com","threadId":"59451","inReplyTo":"xmqq5y31osmg.fsf@gitster.g","subject":"Re: [PATCH v3 0/3] rebase refactoring","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-10-23T15:43:32Z","receivedAt":"2023-10-23T15:43:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 20/10/2023 23:07, Junio C Hamano wrote:\n> Oswald Buddenhagen writes:\n> \n>> broken out of the bigger series, as the aggregation just unnecessarily holds it\n>> up.\n>>\n>> v3: removed \"stray\" footer. so more of a RESEND than an actual new version.\n>>\n>> Oswald Buddenhagen (3):\n>> rebase: simplify code related to imply_merge()\n>> rebase: handle --strategy via imply_merge() as well\n>> rebase: move parse_opt_keep_empty() down\n>>\n>> builtin/rebase.c | 44 ++++++++++++++------------------------------\n>> 1 file changed, 14 insertions(+), 30 deletions(-)\n> \n> Looking quite straight-forward and I didn't see anythihng\n> potentially controversial.\n\nYes they look good, thanks Oswald\n\nBest Wishes\n\nPhillip\n\n"},{"id":"483707","messageId":"xmqqbkcpnowd.fsf@gitster.g","threadId":"59451","inReplyTo":"4f8616d1-35f2-418d-9d28-b230ca45090d@gmail.com","subject":"Re: [PATCH v3 0/3] rebase refactoring","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-23T19:02:10Z","receivedAt":"2023-10-23T19:02:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 20/10/2023 23:07, Junio C Hamano wrote:\n>> Oswald Buddenhagen writes:\n>> \n>>> broken out of the bigger series, as the aggregation just unnecessarily holds it\n>>> up.\n>>>\n>>> v3: removed \"stray\" footer. so more of a RESEND than an actual new version.\n>>>\n>>> Oswald Buddenhagen (3):\n>>> rebase: simplify code related to imply_merge()\n>>> rebase: handle --strategy via imply_merge() as well\n>>> rebase: move parse_opt_keep_empty() down\n>>>\n>>> builtin/rebase.c | 44 ++++++++++++++------------------------------\n>>> 1 file changed, 14 insertions(+), 30 deletions(-)\n>> Looking quite straight-forward and I didn't see anythihng\n>> potentially controversial.\n>\n> Yes they look good, thanks Oswald\n\nThanks, both. The topic has already been merged to 'next'.\n"}]}