{"thread":{"id":"61008","subject":"[PATCH] rebase -i: improve error message when picking merge","startedAt":"2024-02-26T10:58:11Z","lastAt":"2024-06-03T15:42:27Z","messageCount":26,"participants":["Phillip Wood via GitGitGadget","phillip.wood123@gmail.com","Patrick Steinhardt","Rubén Justo","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"489369","messageId":"pull.1672.git.1708945087691.gitgitgadget@gmail.com","threadId":"61008","inReplyTo":null,"subject":"[PATCH] rebase -i: improve error message when picking merge","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-26T10:58:07Z","receivedAt":"2024-02-26T10:58:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe only todo commands that accept a merge commit are \"merge\" and\n\"reset\". All the other commands like \"pick\" or \"reword\" fail when they\ntry to pick a a merge commit and print the message\n\n error: commit abc123 is a merge but no -m option was given.\n\nfollowed by a hint about the command being rescheduled. This message is\ndesigned to help the user when they cherry-pick a merge and forget to\npass \"-m\". For users who are rebasing the message is confusing as there\nis no way for rebase to cherry-pick the merge.\n\nImprove the user experience by detecting the error when the todo list is\nparsed rather than waiting for the \"pick\" command to fail and print a\nmessage recommending the \"merge\" command instead. We recommend \"merge\"\nrather than \"exec git cherry-pick -m ...\" on the assumption that\ncherry-picking merges is relatively rare and it is more likely that the\nuser chose \"pick\" by a mistake.\n\nIt would be possible to support cherry-picking merges by allowing the\nuser to pass \"-m\" to \"pick\" commands but that adds complexity to do\nsomething that can already be achieved with\n\n exec git cherry-pick -m1 abc123\n\nThe change is relatively straight forward but is complicated slightly as\nwe now need to tell the parser if we're rebasing or not.\n\nReported-by: Stefan Haller \nSigned-off-by: Phillip Wood \n---\n rebase -i: improve error message when picking merge\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1672\n\n builtin/rebase.c | 2 +-\n rebase-interactive.c | 7 ++---\n sequencer.c | 49 ++++++++++++++++++++++++++++++-----\n sequencer.h | 2 +-\n t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++\n 5 files changed, 81 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 5b086f651a6..a33e41c44da 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -297,7 +297,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \telse {\n \t\tdiscard_index(&the_index);\n \t\tif (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n-\t\t\t\t\t\t&todo_list))\n+\t\t\t\t\t\t&todo_list, 1))\n \t\t\tBUG(\"unusable todo list\");\n \n \t\tret = complete_action(the_repository, &replay, flags,\ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex d9718409b3d..78d5ed1a41d 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -114,7 +114,8 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\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-\t\tincorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |\n+\t\tincorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf,\n+\t\t\t\t\t\t\ttodo_list, 1) |\n \t\t\tfile_exists(rebase_path_dropped());\n \n \tif (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,\n@@ -134,7 +135,7 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n \tif (initial && 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+\tif (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo, 1)) {\n \t\tfprintf(stderr, _(edit_todo_list_advice));\n \t\treturn -4;\n \t}\n@@ -234,7 +235,7 @@ int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_\n \tint res = 0;\n \n \tif (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {\n-\t\ttodo_list_parse_insn_buffer(r, backup.buf.buf, &backup);\n+\t\ttodo_list_parse_insn_buffer(r, backup.buf.buf, &backup, 1);\n \t\tres = todo_list_check(&backup, todo_list);\n \t}\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 91de546b323..cf808c24d20 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2550,8 +2550,37 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n \treturn 0;\n }\n \n+static int error_merge_commit(enum todo_command command)\n+{\n+\tswitch(command) {\n+\tcase TODO_PICK:\n+\t\treturn error(_(\"'%s' does not accept merge commits, \"\n+\t\t\t \"please use '%s'\"),\n+\t\t\t todo_command_info[command].str, \"merge -C\");\n+\n+\tcase TODO_REWORD:\n+\t\treturn error(_(\"'%s' does not accept merge commits, \"\n+\t\t\t \"please use '%s'\"),\n+\t\t\t todo_command_info[command].str, \"merge -c\");\n+\n+\tcase TODO_EDIT:\n+\t\treturn error(_(\"'%s' does not accept merge commits, \"\n+\t\t\t \"please use '%s' followed by '%s'\"),\n+\t\t\t todo_command_info[command].str,\n+\t\t\t \"merge -C\", \"break\");\n+\n+\tcase TODO_FIXUP:\n+\tcase TODO_SQUASH:\n+\t\treturn error(_(\"cannot squash merge commit into another commit\"));\n+\n+\tdefault:\n+\t\tBUG(\"unexpected todo_command\");\n+\t}\n+}\n+\n static int parse_insn_line(struct repository *r, struct todo_item *item,\n-\t\t\t const char *buf, const char *bol, char *eol)\n+\t\t\t const char *buf, const char *bol, char *eol,\n+\t\t\t int rebasing)\n {\n \tstruct object_id commit_oid;\n \tchar *end_of_object_name;\n@@ -2655,7 +2684,12 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \t\treturn status;\n \n \titem->commit = lookup_commit_reference(r, &commit_oid);\n-\treturn item->commit ? 0 : -1;\n+\tif (!item->commit)\n+\t\treturn -1;\n+\tif (rebasing && item->command != TODO_MERGE &&\n+\t item->commit->parents && item->commit->parents->next)\n+\t\treturn error_merge_commit(item->command);\n+\treturn 0;\n }\n \n int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)\n@@ -2686,7 +2720,7 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *\n }\n \n int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n-\t\t\t\tstruct todo_list *todo_list)\n+\t\t\t\tstruct todo_list *todo_list, int rebasing)\n {\n \tstruct todo_item *item;\n \tchar *p = buf, *next_p;\n@@ -2704,7 +2738,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n \n \t\titem = append_new_todo(todo_list);\n \t\titem->offset_in_buf = p - todo_list->buf.buf;\n-\t\tif (parse_insn_line(r, item, buf, p, eol)) {\n+\t\tif (parse_insn_line(r, item, buf, p, eol, rebasing)) {\n \t\t\tres = error(_(\"invalid line %d: %.*s\"),\n \t\t\t\ti, (int)(eol - p), p);\n \t\t\titem->command = TODO_COMMENT + 1;\n@@ -2852,7 +2886,8 @@ static int read_populate_todo(struct repository *r,\n \tif (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)\n \t\treturn -1;\n \n-\tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);\n+\tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list,\n+\t\t\t\t\t is_rebase_i(opts));\n \tif (res) {\n \t\tif (is_rebase_i(opts))\n \t\t\treturn error(_(\"please fix this using \"\n@@ -2882,7 +2917,7 @@ static int read_populate_todo(struct repository *r,\n \t\tstruct todo_list done = TODO_LIST_INIT;\n \n \t\tif (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&\n-\t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done))\n+\t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done, 1))\n \t\t\ttodo_list->done_nr = count_commands(&done);\n \t\telse\n \t\t\ttodo_list->done_nr = 0;\n@@ -6286,7 +6321,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \tstrbuf_release(&buf2);\n \t/* Nothing is done yet, and we're reparsing, so let's reset the count */\n \tnew_todo.total_nr = 0;\n-\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n+\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo, 1) < 0)\n \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n \t\t new_todo.buf.buf);\n \ndiff --git a/sequencer.h b/sequencer.h\nindex dcef7bb99c0..ed2c4b38514 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -136,7 +136,7 @@ struct todo_list {\n }\n \n int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n-\t\t\t\tstruct todo_list *todo_list);\n+\t\t\t\tstruct todo_list *todo_list, int rebasing);\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);\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 64b641002e4..20b8589ad07 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -2203,6 +2203,39 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' '\n \ttest_path_is_missing execed\n '\n \n+test_expect_success 'non-merge commands reject merge commits' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tgit checkout E &&\n+\tgit merge I &&\n+\toid=$(git rev-parse HEAD) &&\n+\tcat >todo <<-EOF &&\n+\tpick $oid\n+\treword $oid\n+\tedit $oid\n+\tfixup $oid\n+\tsquash $oid\n+\tEOF\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\ttest_must_fail git rebase -i HEAD 2>actual\n+\t) &&\n+\tcat >expect <<-EOF &&\n+\terror: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ}\n+\terror: invalid line 1: pick $oid\n+\terror: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ}\n+\terror: invalid line 2: reword $oid\n+\terror: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}\n+\terror: invalid line 3: edit $oid\n+\terror: cannot squash merge commit into another commit\n+\terror: invalid line 4: fixup $oid\n+\terror: cannot squash merge commit into another commit\n+\terror: invalid line 5: squash $oid\n+\tYou can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.\n+\tOr you can abort the rebase with ${SQ}git rebase --abort${SQ}.\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n # This must be the last test in this file\n test_expect_success '$EDITOR and friends are unchanged' '\n \ttest_editor_unchanged\n\nbase-commit: c875e0b8e036c12cfbf6531962108a063c7a821c\n-- \ngitgitgadget\n"},{"id":"492160","messageId":"db27d746-5b83-45fa-a8e7-d88022ce1ca5@gmail.com","threadId":"61008","inReplyTo":"pull.1672.git.1708945087691.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase -i: improve error message when picking merge","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-03T13:42:19Z","receivedAt":"2024-04-03T13:42:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"If anyone has time to review this I'd be very grateful. I've added a \ncouple of people to the cc list who have recently contributed to the \nsequencer but if anyone else fancies taking a look please do.\n\nThanks\n\nPhillip\n\nOn 26/02/2024 10:58, Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood \n> \n> The only todo commands that accept a merge commit are \"merge\" and\n> \"reset\". All the other commands like \"pick\" or \"reword\" fail when they\n> try to pick a a merge commit and print the message\n> \n> error: commit abc123 is a merge but no -m option was given.\n> \n> followed by a hint about the command being rescheduled. This message is\n> designed to help the user when they cherry-pick a merge and forget to\n> pass \"-m\". For users who are rebasing the message is confusing as there\n> is no way for rebase to cherry-pick the merge.\n> \n> Improve the user experience by detecting the error when the todo list is\n> parsed rather than waiting for the \"pick\" command to fail and print a\n> message recommending the \"merge\" command instead. We recommend \"merge\"\n> rather than \"exec git cherry-pick -m ...\" on the assumption that\n> cherry-picking merges is relatively rare and it is more likely that the\n> user chose \"pick\" by a mistake.\n> \n> It would be possible to support cherry-picking merges by allowing the\n> user to pass \"-m\" to \"pick\" commands but that adds complexity to do\n> something that can already be achieved with\n> \n> exec git cherry-pick -m1 abc123\n> \n> The change is relatively straight forward but is complicated slightly as\n> we now need to tell the parser if we're rebasing or not.\n> \n> Reported-by: Stefan Haller \n> Signed-off-by: Phillip Wood \n> ---\n> rebase -i: improve error message when picking merge\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1672\n> \n> builtin/rebase.c | 2 +-\n> rebase-interactive.c | 7 ++---\n> sequencer.c | 49 ++++++++++++++++++++++++++++++-----\n> sequencer.h | 2 +-\n> t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++\n> 5 files changed, 81 insertions(+), 12 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 5b086f651a6..a33e41c44da 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -297,7 +297,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n> \telse {\n> \t\tdiscard_index(&the_index);\n> \t\tif (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n> -\t\t\t\t\t\t&todo_list))\n> +\t\t\t\t\t\t&todo_list, 1))\n> \t\t\tBUG(\"unusable todo list\");\n> \n> \t\tret = complete_action(the_repository, &replay, flags,\n> diff --git a/rebase-interactive.c b/rebase-interactive.c\n> index d9718409b3d..78d5ed1a41d 100644\n> --- a/rebase-interactive.c\n> +++ b/rebase-interactive.c\n> @@ -114,7 +114,8 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\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> -\t\tincorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |\n> +\t\tincorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf,\n> +\t\t\t\t\t\t\ttodo_list, 1) |\n> \t\t\tfile_exists(rebase_path_dropped());\n> \n> \tif (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,\n> @@ -134,7 +135,7 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n> \tif (initial && 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> +\tif (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo, 1)) {\n> \t\tfprintf(stderr, _(edit_todo_list_advice));\n> \t\treturn -4;\n> \t}\n> @@ -234,7 +235,7 @@ int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_\n> \tint res = 0;\n> \n> \tif (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {\n> -\t\ttodo_list_parse_insn_buffer(r, backup.buf.buf, &backup);\n> +\t\ttodo_list_parse_insn_buffer(r, backup.buf.buf, &backup, 1);\n> \t\tres = todo_list_check(&backup, todo_list);\n> \t}\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 91de546b323..cf808c24d20 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2550,8 +2550,37 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n> \treturn 0;\n> }\n> \n> +static int error_merge_commit(enum todo_command command)\n> +{\n> +\tswitch(command) {\n> +\tcase TODO_PICK:\n> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n> +\t\t\t \"please use '%s'\"),\n> +\t\t\t todo_command_info[command].str, \"merge -C\");\n> +\n> +\tcase TODO_REWORD:\n> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n> +\t\t\t \"please use '%s'\"),\n> +\t\t\t todo_command_info[command].str, \"merge -c\");\n> +\n> +\tcase TODO_EDIT:\n> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n> +\t\t\t \"please use '%s' followed by '%s'\"),\n> +\t\t\t todo_command_info[command].str,\n> +\t\t\t \"merge -C\", \"break\");\n> +\n> +\tcase TODO_FIXUP:\n> +\tcase TODO_SQUASH:\n> +\t\treturn error(_(\"cannot squash merge commit into another commit\"));\n> +\n> +\tdefault:\n> +\t\tBUG(\"unexpected todo_command\");\n> +\t}\n> +}\n> +\n> static int parse_insn_line(struct repository *r, struct todo_item *item,\n> -\t\t\t const char *buf, const char *bol, char *eol)\n> +\t\t\t const char *buf, const char *bol, char *eol,\n> +\t\t\t int rebasing)\n> {\n> \tstruct object_id commit_oid;\n> \tchar *end_of_object_name;\n> @@ -2655,7 +2684,12 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n> \t\treturn status;\n> \n> \titem->commit = lookup_commit_reference(r, &commit_oid);\n> -\treturn item->commit ? 0 : -1;\n> +\tif (!item->commit)\n> +\t\treturn -1;\n> +\tif (rebasing && item->command != TODO_MERGE &&\n> +\t item->commit->parents && item->commit->parents->next)\n> +\t\treturn error_merge_commit(item->command);\n> +\treturn 0;\n> }\n> \n> int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)\n> @@ -2686,7 +2720,7 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *\n> }\n> \n> int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n> -\t\t\t\tstruct todo_list *todo_list)\n> +\t\t\t\tstruct todo_list *todo_list, int rebasing)\n> {\n> \tstruct todo_item *item;\n> \tchar *p = buf, *next_p;\n> @@ -2704,7 +2738,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n> \n> \t\titem = append_new_todo(todo_list);\n> \t\titem->offset_in_buf = p - todo_list->buf.buf;\n> -\t\tif (parse_insn_line(r, item, buf, p, eol)) {\n> +\t\tif (parse_insn_line(r, item, buf, p, eol, rebasing)) {\n> \t\t\tres = error(_(\"invalid line %d: %.*s\"),\n> \t\t\t\ti, (int)(eol - p), p);\n> \t\t\titem->command = TODO_COMMENT + 1;\n> @@ -2852,7 +2886,8 @@ static int read_populate_todo(struct repository *r,\n> \tif (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)\n> \t\treturn -1;\n> \n> -\tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);\n> +\tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list,\n> +\t\t\t\t\t is_rebase_i(opts));\n> \tif (res) {\n> \t\tif (is_rebase_i(opts))\n> \t\t\treturn error(_(\"please fix this using \"\n> @@ -2882,7 +2917,7 @@ static int read_populate_todo(struct repository *r,\n> \t\tstruct todo_list done = TODO_LIST_INIT;\n> \n> \t\tif (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&\n> -\t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done))\n> +\t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done, 1))\n> \t\t\ttodo_list->done_nr = count_commands(&done);\n> \t\telse\n> \t\t\ttodo_list->done_nr = 0;\n> @@ -6286,7 +6321,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \tstrbuf_release(&buf2);\n> \t/* Nothing is done yet, and we're reparsing, so let's reset the count */\n> \tnew_todo.total_nr = 0;\n> -\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n> +\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo, 1) < 0)\n> \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n> \t\t new_todo.buf.buf);\n> \n> diff --git a/sequencer.h b/sequencer.h\n> index dcef7bb99c0..ed2c4b38514 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -136,7 +136,7 @@ struct todo_list {\n> }\n> \n> int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n> -\t\t\t\tstruct todo_list *todo_list);\n> +\t\t\t\tstruct todo_list *todo_list, int rebasing);\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> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 64b641002e4..20b8589ad07 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -2203,6 +2203,39 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' '\n> \ttest_path_is_missing execed\n> '\n> \n> +test_expect_success 'non-merge commands reject merge commits' '\n> +\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n> +\tgit checkout E &&\n> +\tgit merge I &&\n> +\toid=$(git rev-parse HEAD) &&\n> +\tcat >todo <<-EOF &&\n> +\tpick $oid\n> +\treword $oid\n> +\tedit $oid\n> +\tfixup $oid\n> +\tsquash $oid\n> +\tEOF\n> +\t(\n> +\t\tset_replace_editor todo &&\n> +\t\ttest_must_fail git rebase -i HEAD 2>actual\n> +\t) &&\n> +\tcat >expect <<-EOF &&\n> +\terror: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ}\n> +\terror: invalid line 1: pick $oid\n> +\terror: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ}\n> +\terror: invalid line 2: reword $oid\n> +\terror: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}\n> +\terror: invalid line 3: edit $oid\n> +\terror: cannot squash merge commit into another commit\n> +\terror: invalid line 4: fixup $oid\n> +\terror: cannot squash merge commit into another commit\n> +\terror: invalid line 5: squash $oid\n> +\tYou can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.\n> +\tOr you can abort the rebase with ${SQ}git rebase --abort${SQ}.\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n> # This must be the last test in this file\n> test_expect_success '$EDITOR and friends are unchanged' '\n> \ttest_editor_unchanged\n> \n> base-commit: c875e0b8e036c12cfbf6531962108a063c7a821c\n"},{"id":"492214","messageId":"Zg5D1qnKd9bGiXBw@tanuki","threadId":"61008","inReplyTo":"db27d746-5b83-45fa-a8e7-d88022ce1ca5@gmail.com","subject":"Re: [PATCH] rebase -i: improve error message when picking merge","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T06:08:22Z","receivedAt":"2024-04-04T06:08:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 03, 2024 at 02:42:19PM +0100, phillip.wood123@gmail.com wrote:\n> If anyone has time to review this I'd be very grateful. I've added a couple\n> of people to the cc list who have recently contributed to the sequencer but\n> if anyone else fancies taking a look please do.\n> \n> Thanks\n> \n> Phillip\n\nI ain't got much of a stake in this, but I've put in my 2c anyway. :)\n\nPatrick\n"},{"id":"492215","messageId":"Zg5D3dXYFM2SONE-@tanuki","threadId":"61008","inReplyTo":"pull.1672.git.1708945087691.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase -i: improve error message when picking merge","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T06:08:29Z","receivedAt":"2024-04-04T06:08:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood \n> \n> The only todo commands that accept a merge commit are \"merge\" and\n> \"reset\". All the other commands like \"pick\" or \"reword\" fail when they\n> try to pick a a merge commit and print the message\n> \n> error: commit abc123 is a merge but no -m option was given.\n> \n> followed by a hint about the command being rescheduled. This message is\n> designed to help the user when they cherry-pick a merge and forget to\n> pass \"-m\". For users who are rebasing the message is confusing as there\n> is no way for rebase to cherry-pick the merge.\n> \n> Improve the user experience by detecting the error when the todo list is\n> parsed rather than waiting for the \"pick\" command to fail and print a\n> message recommending the \"merge\" command instead. We recommend \"merge\"\n> rather than \"exec git cherry-pick -m ...\" on the assumption that\n> cherry-picking merges is relatively rare and it is more likely that the\n> user chose \"pick\" by a mistake.\n> \n> It would be possible to support cherry-picking merges by allowing the\n> user to pass \"-m\" to \"pick\" commands but that adds complexity to do\n> something that can already be achieved with\n> \n> exec git cherry-pick -m1 abc123\n\nOkay. I think it's reasonable to abort early and give some advice here.\nIt's certainly a lot more user friendly than to abort during the rebase.\nAnd if we ever gain the ability to e.g. `pick -m1` in the instruction\nsheet directly we can adapt accordingly.\n\n> The change is relatively straight forward but is complicated slightly as\n> we now need to tell the parser if we're rebasing or not.\n> \n> Reported-by: Stefan Haller \n> Signed-off-by: Phillip Wood \n> ---\n> rebase -i: improve error message when picking merge\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1672\n> \n> builtin/rebase.c | 2 +-\n> rebase-interactive.c | 7 ++---\n> sequencer.c | 49 ++++++++++++++++++++++++++++++-----\n> sequencer.h | 2 +-\n> t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++\n> 5 files changed, 81 insertions(+), 12 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 5b086f651a6..a33e41c44da 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -297,7 +297,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n> \telse {\n> \t\tdiscard_index(&the_index);\n> \t\tif (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n> -\t\t\t\t\t\t&todo_list))\n> +\t\t\t\t\t\t&todo_list, 1))\n\nI think these booleans are somewhat hard to read. I may be\noverengineering this, so please feel free to push back, but would it\nmake more sense to introduce a `unsigned flags` field and then have a\n`PARSE_INSN_IS_REBASING` flag?\n\n> \t\t\tBUG(\"unusable todo list\");\n> \n> \t\tret = complete_action(the_repository, &replay, flags,\n> diff --git a/rebase-interactive.c b/rebase-interactive.c\n> index d9718409b3d..78d5ed1a41d 100644\n> --- a/rebase-interactive.c\n> +++ b/rebase-interactive.c\n> @@ -114,7 +114,8 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\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> -\t\tincorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |\n> +\t\tincorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf,\n> +\t\t\t\t\t\t\ttodo_list, 1) |\n> \t\t\tfile_exists(rebase_path_dropped());\n> \n> \tif (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,\n> @@ -134,7 +135,7 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n> \tif (initial && 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> +\tif (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo, 1)) {\n> \t\tfprintf(stderr, _(edit_todo_list_advice));\n> \t\treturn -4;\n> \t}\n> @@ -234,7 +235,7 @@ int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_\n> \tint res = 0;\n> \n> \tif (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {\n> -\t\ttodo_list_parse_insn_buffer(r, backup.buf.buf, &backup);\n> +\t\ttodo_list_parse_insn_buffer(r, backup.buf.buf, &backup, 1);\n> \t\tres = todo_list_check(&backup, todo_list);\n> \t}\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 91de546b323..cf808c24d20 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2550,8 +2550,37 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n> \treturn 0;\n> }\n> \n> +static int error_merge_commit(enum todo_command command)\n> +{\n> +\tswitch(command) {\n> +\tcase TODO_PICK:\n> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n> +\t\t\t \"please use '%s'\"),\n> +\t\t\t todo_command_info[command].str, \"merge -C\");\n\nI wonder how actionable these commands are. Can we give the full command\nthat the user can use instead, including the commit ID?\n\nThat raises another question though: how exactly is the user supposed to\nperform the merge? Should they merge the merge commit, resulting in two\nmerge commits? Should they pick one of the sides, and if so, which one?\nI guess the answer is \"it depends\", which makes it harder for us to come\nup with actionable advice here.\n\n> +\tcase TODO_REWORD:\n> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n> +\t\t\t \"please use '%s'\"),\n> +\t\t\t todo_command_info[command].str, \"merge -c\");\n\nI was about to suggest that the above two cases should be merged. But\nthen I realized that it's \"merge -c\" here, but \"merge -C\" in the first\ncase.\n\nPatrick\n\n> +\tcase TODO_EDIT:\n> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n> +\t\t\t \"please use '%s' followed by '%s'\"),\n> +\t\t\t todo_command_info[command].str,\n> +\t\t\t \"merge -C\", \"break\");\n> +\n> +\tcase TODO_FIXUP:\n> +\tcase TODO_SQUASH:\n> +\t\treturn error(_(\"cannot squash merge commit into another commit\"));\n> +\n> +\tdefault:\n> +\t\tBUG(\"unexpected todo_command\");\n> +\t}\n> +}\n> +\n> static int parse_insn_line(struct repository *r, struct todo_item *item,\n> -\t\t\t const char *buf, const char *bol, char *eol)\n> +\t\t\t const char *buf, const char *bol, char *eol,\n> +\t\t\t int rebasing)\n> {\n> \tstruct object_id commit_oid;\n> \tchar *end_of_object_name;\n> @@ -2655,7 +2684,12 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n> \t\treturn status;\n> \n> \titem->commit = lookup_commit_reference(r, &commit_oid);\n> -\treturn item->commit ? 0 : -1;\n> +\tif (!item->commit)\n> +\t\treturn -1;\n> +\tif (rebasing && item->command != TODO_MERGE &&\n> +\t item->commit->parents && item->commit->parents->next)\n> +\t\treturn error_merge_commit(item->command);\n> +\treturn 0;\n> }\n> \n> int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)\n> @@ -2686,7 +2720,7 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *\n> }\n> \n> int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n> -\t\t\t\tstruct todo_list *todo_list)\n> +\t\t\t\tstruct todo_list *todo_list, int rebasing)\n> {\n> \tstruct todo_item *item;\n> \tchar *p = buf, *next_p;\n> @@ -2704,7 +2738,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n> \n> \t\titem = append_new_todo(todo_list);\n> \t\titem->offset_in_buf = p - todo_list->buf.buf;\n> -\t\tif (parse_insn_line(r, item, buf, p, eol)) {\n> +\t\tif (parse_insn_line(r, item, buf, p, eol, rebasing)) {\n> \t\t\tres = error(_(\"invalid line %d: %.*s\"),\n> \t\t\t\ti, (int)(eol - p), p);\n> \t\t\titem->command = TODO_COMMENT + 1;\n> @@ -2852,7 +2886,8 @@ static int read_populate_todo(struct repository *r,\n> \tif (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)\n> \t\treturn -1;\n> \n> -\tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);\n> +\tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list,\n> +\t\t\t\t\t is_rebase_i(opts));\n> \tif (res) {\n> \t\tif (is_rebase_i(opts))\n> \t\t\treturn error(_(\"please fix this using \"\n> @@ -2882,7 +2917,7 @@ static int read_populate_todo(struct repository *r,\n> \t\tstruct todo_list done = TODO_LIST_INIT;\n> \n> \t\tif (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&\n> -\t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done))\n> +\t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done, 1))\n> \t\t\ttodo_list->done_nr = count_commands(&done);\n> \t\telse\n> \t\t\ttodo_list->done_nr = 0;\n> @@ -6286,7 +6321,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \tstrbuf_release(&buf2);\n> \t/* Nothing is done yet, and we're reparsing, so let's reset the count */\n> \tnew_todo.total_nr = 0;\n> -\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n> +\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo, 1) < 0)\n> \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n> \t\t new_todo.buf.buf);\n> \n> diff --git a/sequencer.h b/sequencer.h\n> index dcef7bb99c0..ed2c4b38514 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -136,7 +136,7 @@ struct todo_list {\n> }\n> \n> int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n> -\t\t\t\tstruct todo_list *todo_list);\n> +\t\t\t\tstruct todo_list *todo_list, int rebasing);\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> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 64b641002e4..20b8589ad07 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -2203,6 +2203,39 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' '\n> \ttest_path_is_missing execed\n> '\n> \n> +test_expect_success 'non-merge commands reject merge commits' '\n> +\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n> +\tgit checkout E &&\n> +\tgit merge I &&\n> +\toid=$(git rev-parse HEAD) &&\n> +\tcat >todo <<-EOF &&\n> +\tpick $oid\n> +\treword $oid\n> +\tedit $oid\n> +\tfixup $oid\n> +\tsquash $oid\n> +\tEOF\n> +\t(\n> +\t\tset_replace_editor todo &&\n> +\t\ttest_must_fail git rebase -i HEAD 2>actual\n> +\t) &&\n> +\tcat >expect <<-EOF &&\n> +\terror: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ}\n> +\terror: invalid line 1: pick $oid\n> +\terror: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ}\n> +\terror: invalid line 2: reword $oid\n> +\terror: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}\n> +\terror: invalid line 3: edit $oid\n> +\terror: cannot squash merge commit into another commit\n> +\terror: invalid line 4: fixup $oid\n> +\terror: cannot squash merge commit into another commit\n> +\terror: invalid line 5: squash $oid\n> +\tYou can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.\n> +\tOr you can abort the rebase with ${SQ}git rebase --abort${SQ}.\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n> # This must be the last test in this file\n> test_expect_success '$EDITOR and friends are unchanged' '\n> \ttest_editor_unchanged\n> \n> base-commit: c875e0b8e036c12cfbf6531962108a063c7a821c\n> -- \n> gitgitgadget\n> \n"},{"id":"492248","messageId":"875639c6-1efc-4fb2-b80c-2205b67c263b@gmail.com","threadId":"61008","inReplyTo":"Zg5D3dXYFM2SONE-@tanuki","subject":"Re: [PATCH] rebase -i: improve error message when picking merge","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-04T15:29:37Z","receivedAt":"2024-04-04T15:29:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Patrick\n\nOn 04/04/2024 07:08, Patrick Steinhardt wrote:\n> On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote:\n> [...]\n>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>> index 5b086f651a6..a33e41c44da 100644\n>> --- a/builtin/rebase.c\n>> +++ b/builtin/rebase.c\n>> @@ -297,7 +297,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n>> \telse {\n>> \t\tdiscard_index(&the_index);\n>> \t\tif (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n>> -\t\t\t\t\t\t&todo_list))\n>> +\t\t\t\t\t\t&todo_list, 1))\n> \n> I think these booleans are somewhat hard to read. I may be\n> overengineering this, so please feel free to push back, but would it\n> make more sense to introduce a `unsigned flags` field and then have a\n> `PARSE_INSN_IS_REBASING` flag?\n\nI agree the boolean is a bit opaque, I don't think they are that unusual \nin the code base but that doesn't mean they're readable. I had hoped to \npass an instance of 'struct replay_opts' to parse_insn_line() and then \ncall is_rebase_i() which there which would have been nicer. \nUnfortunately \"git rebase --edit-todo\" does not instantiate an instance \nof that struct as it is not needed for editing the todo list so I went \nwith a boolean instead. There are one or two places where we need to use \nis_rebase_i() for this when calling parse_insn_line() which makes using \nan unsigned flags argument a bit messy. I'll have a look and see how \nhard it would be to ensure we always have a valid 'struct replay_opts' \nwhen calling parse_insn_line().\n\n>> +static int error_merge_commit(enum todo_command command)\n>> +{\n>> +\tswitch(command) {\n>> +\tcase TODO_PICK:\n>> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n>> +\t\t\t \"please use '%s'\"),\n>> +\t\t\t todo_command_info[command].str, \"merge -C\");\n> \n> I wonder how actionable these commands are. Can we give the full command\n> that the user can use instead, including the commit ID?\n\nThe calling function also prints the offending line so the user can see \nthe commit details there.\n\n> That raises another question though: how exactly is the user supposed to\n> perform the merge? Should they merge the merge commit, resulting in two\n> merge commits? Should they pick one of the sides, and if so, which one?\n> I guess the answer is \"it depends\", which makes it harder for us to come\n> up with actionable advice here.\n\nI agree it would be nice to be more precise but I'm not sure we can tell \nwhat the user is actually trying to do so I think the best we can do is \npoint them in the direction of the 'merge' command.\n\n>> +\tcase TODO_REWORD:\n>> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n>> +\t\t\t \"please use '%s'\"),\n>> +\t\t\t todo_command_info[command].str, \"merge -c\");\n> \n> I was about to suggest that the above two cases should be merged. But\n> then I realized that it's \"merge -c\" here, but \"merge -C\" in the first\n> case.\n\nYes, it is a subtle difference.\n\nThanks very much for taking a look at this\n\nPhillip\n\n> Patrick\n> \n>> +\tcase TODO_EDIT:\n>> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n>> +\t\t\t \"please use '%s' followed by '%s'\"),\n>> +\t\t\t todo_command_info[command].str,\n>> +\t\t\t \"merge -C\", \"break\");\n>> +\n>> +\tcase TODO_FIXUP:\n>> +\tcase TODO_SQUASH:\n>> +\t\treturn error(_(\"cannot squash merge commit into another commit\"));\n>> +\n>> +\tdefault:\n>> +\t\tBUG(\"unexpected todo_command\");\n>> +\t}\n>> +}\n>> +\n>> static int parse_insn_line(struct repository *r, struct todo_item *item,\n>> -\t\t\t const char *buf, const char *bol, char *eol)\n>> +\t\t\t const char *buf, const char *bol, char *eol,\n>> +\t\t\t int rebasing)\n>> {\n>> \tstruct object_id commit_oid;\n>> \tchar *end_of_object_name;\n>> @@ -2655,7 +2684,12 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n>> \t\treturn status;\n>> \n>> \titem->commit = lookup_commit_reference(r, &commit_oid);\n>> -\treturn item->commit ? 0 : -1;\n>> +\tif (!item->commit)\n>> +\t\treturn -1;\n>> +\tif (rebasing && item->command != TODO_MERGE &&\n>> +\t item->commit->parents && item->commit->parents->next)\n>> +\t\treturn error_merge_commit(item->command);\n>> +\treturn 0;\n>> }\n>> \n>> int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)\n>> @@ -2686,7 +2720,7 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *\n>> }\n>> \n>> int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n>> -\t\t\t\tstruct todo_list *todo_list)\n>> +\t\t\t\tstruct todo_list *todo_list, int rebasing)\n>> {\n>> \tstruct todo_item *item;\n>> \tchar *p = buf, *next_p;\n>> @@ -2704,7 +2738,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n>> \n>> \t\titem = append_new_todo(todo_list);\n>> \t\titem->offset_in_buf = p - todo_list->buf.buf;\n>> -\t\tif (parse_insn_line(r, item, buf, p, eol)) {\n>> +\t\tif (parse_insn_line(r, item, buf, p, eol, rebasing)) {\n>> \t\t\tres = error(_(\"invalid line %d: %.*s\"),\n>> \t\t\t\ti, (int)(eol - p), p);\n>> \t\t\titem->command = TODO_COMMENT + 1;\n>> @@ -2852,7 +2886,8 @@ static int read_populate_todo(struct repository *r,\n>> \tif (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)\n>> \t\treturn -1;\n>> \n>> -\tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);\n>> +\tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list,\n>> +\t\t\t\t\t is_rebase_i(opts));\n>> \tif (res) {\n>> \t\tif (is_rebase_i(opts))\n>> \t\t\treturn error(_(\"please fix this using \"\n>> @@ -2882,7 +2917,7 @@ static int read_populate_todo(struct repository *r,\n>> \t\tstruct todo_list done = TODO_LIST_INIT;\n>> \n>> \t\tif (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&\n>> -\t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done))\n>> +\t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done, 1))\n>> \t\t\ttodo_list->done_nr = count_commands(&done);\n>> \t\telse\n>> \t\t\ttodo_list->done_nr = 0;\n>> @@ -6286,7 +6321,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n>> \tstrbuf_release(&buf2);\n>> \t/* Nothing is done yet, and we're reparsing, so let's reset the count */\n>> \tnew_todo.total_nr = 0;\n>> -\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n>> +\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo, 1) < 0)\n>> \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n>> \t\t new_todo.buf.buf);\n>> \n>> diff --git a/sequencer.h b/sequencer.h\n>> index dcef7bb99c0..ed2c4b38514 100644\n>> --- a/sequencer.h\n>> +++ b/sequencer.h\n>> @@ -136,7 +136,7 @@ struct todo_list {\n>> }\n>> \n>> int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n>> -\t\t\t\tstruct todo_list *todo_list);\n>> +\t\t\t\tstruct todo_list *todo_list, int rebasing);\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>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> index 64b641002e4..20b8589ad07 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -2203,6 +2203,39 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' '\n>> \ttest_path_is_missing execed\n>> '\n>> \n>> +test_expect_success 'non-merge commands reject merge commits' '\n>> +\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n>> +\tgit checkout E &&\n>> +\tgit merge I &&\n>> +\toid=$(git rev-parse HEAD) &&\n>> +\tcat >todo <<-EOF &&\n>> +\tpick $oid\n>> +\treword $oid\n>> +\tedit $oid\n>> +\tfixup $oid\n>> +\tsquash $oid\n>> +\tEOF\n>> +\t(\n>> +\t\tset_replace_editor todo &&\n>> +\t\ttest_must_fail git rebase -i HEAD 2>actual\n>> +\t) &&\n>> +\tcat >expect <<-EOF &&\n>> +\terror: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ}\n>> +\terror: invalid line 1: pick $oid\n>> +\terror: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ}\n>> +\terror: invalid line 2: reword $oid\n>> +\terror: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}\n>> +\terror: invalid line 3: edit $oid\n>> +\terror: cannot squash merge commit into another commit\n>> +\terror: invalid line 4: fixup $oid\n>> +\terror: cannot squash merge commit into another commit\n>> +\terror: invalid line 5: squash $oid\n>> +\tYou can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.\n>> +\tOr you can abort the rebase with ${SQ}git rebase --abort${SQ}.\n>> +\tEOF\n>> +\ttest_cmp expect actual\n>> +'\n>> +\n>> # This must be the last test in this file\n>> test_expect_success '$EDITOR and friends are unchanged' '\n>> \ttest_editor_unchanged\n>>\n>> base-commit: c875e0b8e036c12cfbf6531962108a063c7a821c\n>> -- \n>> gitgitgadget\n>>\n"},{"id":"492265","messageId":"57462d2e-e255-4d35-b778-176f03675df0@gmail.com","threadId":"61008","inReplyTo":"pull.1672.git.1708945087691.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase -i: improve error message when picking merge","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-04T19:44:53Z","receivedAt":"2024-04-04T19:45:00Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood \n> \n> The only todo commands that accept a merge commit are \"merge\" and\n> \"reset\". All the other commands like \"pick\" or \"reword\" fail when they\n> try to pick a a merge commit and print the message\n> \n> error: commit abc123 is a merge but no -m option was given.\n> \n> followed by a hint about the command being rescheduled. This message is\n> designed to help the user when they cherry-pick a merge and forget to\n> pass \"-m\". For users who are rebasing the message is confusing as there\n> is no way for rebase to cherry-pick the merge.\n> \n> Improve the user experience by detecting the error when the todo list is\n> parsed rather than waiting for the \"pick\" command to fail and print a\n> message recommending the \"merge\" command instead. We recommend \"merge\"\n> rather than \"exec git cherry-pick -m ...\" on the assumption that\n> cherry-picking merges is relatively rare and it is more likely that the\n> user chose \"pick\" by a mistake.\n> \n> It would be possible to support cherry-picking merges by allowing the\n> user to pass \"-m\" to \"pick\" commands but that adds complexity to do\n> something that can already be achieved with\n> \n> exec git cherry-pick -m1 abc123\n> \n> The change is relatively straight forward but is complicated slightly as\n> we now need to tell the parser if we're rebasing or not.\n> \n> Reported-by: Stefan Haller \n> Signed-off-by: Phillip Wood \n> ---\n\nHi Phillip.\n\nThe change makes sense, but this is confusing to me:\n\nWith this ...\n\n\t$ GIT_EDITOR='echo pick 17381ab62a >' ./git rebase -i HEAD\n\terror: 'pick' does not accept merge commits, please use 'merge -C'\n\terror: invalid line 1: pick 17381ab62a\n\tYou can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.\n\tOr you can abort the rebase with 'git rebase --abort'.\n\n... I find these repeated messages confusing:\n\n\t$ GIT_EDITOR=: ./git rebase --edit-todo\n\terror: 'pick' does not accept merge commits, please use 'merge -C'\n\terror: invalid line 1: pick 17381ab62a\n\terror: 'pick' does not accept merge commits, please use 'merge -C'\n\terror: invalid line 1: pick 17381ab62a\n\tYou can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.\n\tOr you can abort the rebase with 'git rebase --abort'.\n"},{"id":"492285","messageId":"dbb4615c-443f-474f-a33c-cfb4486d436b@gmail.com","threadId":"61008","inReplyTo":"57462d2e-e255-4d35-b778-176f03675df0@gmail.com","subject":"Re: [PATCH] rebase -i: improve error message when picking merge","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-05T09:30:37Z","receivedAt":"2024-04-05T09:30:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nThanks for trying this out.\n\nOn 04/04/2024 20:44, Rubén Justo wrote:\n> On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote:\n> The change makes sense, but this is confusing to me:\n> \n> With this ...\n> \n> \t$ GIT_EDITOR='echo pick 17381ab62a >' ./git rebase -i HEAD\n> \terror: 'pick' does not accept merge commits, please use 'merge -C'\n> \terror: invalid line 1: pick 17381ab62a\n> \tYou can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.\n> \tOr you can abort the rebase with 'git rebase --abort'.\n> \n> ... I find these repeated messages confusing:\n> \n> \t$ GIT_EDITOR=: ./git rebase --edit-todo\n> \terror: 'pick' does not accept merge commits, please use 'merge -C'\n> \terror: invalid line 1: pick 17381ab62a\n\nThe two lines above are printed when \"rebase --edit-todo\" loads the todo \nlist for the user to edit. With a real editor rather than \":\" we would \nthen print\n\n hint: Waiting for your editor to close the file ...\n\nThen hopefully the user would fix the errors. If not then when the \neditor had finished we'd print the remaining errors as below.\n\n> \terror: 'pick' does not accept merge commits, please use 'merge -C'\n> \terror: invalid line 1: pick 17381ab62a\n> \tYou can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.\n> \tOr you can abort the rebase with 'git rebase --abort'.\n\nI think that printing the error messages when the todo list is parsed \nbefore it is given to the user to edit is helpful as it reminds the user \nwhat the problem is. Your test looks confusing because it doesn't really \nsimulate the user editing the todo list.\n\nBest Wishes\n\nPhillip\n"},{"id":"492386","messageId":"7e4eae67-8db9-4be3-8b96-e2e89fa82abf@gmail.com","threadId":"61008","inReplyTo":"dbb4615c-443f-474f-a33c-cfb4486d436b@gmail.com","subject":"Re: [PATCH] rebase -i: improve error message when picking merge","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-06T14:24:23Z","receivedAt":"2024-04-06T14:24:28Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Apr 05, 2024 at 10:30:37AM +0100, phillip.wood123@gmail.com wrote:\n> Hi Rubén\n> \n> Thanks for trying this out.\n> \n> On 04/04/2024 20:44, Rubén Justo wrote:\n> > On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote:\n> > The change makes sense, but this is confusing to me:\n> > \n> > With this ...\n> > \n> > \t$ GIT_EDITOR='echo pick 17381ab62a >' ./git rebase -i HEAD\n> > \terror: 'pick' does not accept merge commits, please use 'merge -C'\n> > \terror: invalid line 1: pick 17381ab62a\n> > \tYou can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.\n> > \tOr you can abort the rebase with 'git rebase --abort'.\n> > \n> > ... I find these repeated messages confusing:\n> > \n> > \t$ GIT_EDITOR=: ./git rebase --edit-todo\n> > \terror: 'pick' does not accept merge commits, please use 'merge -C'\n> > \terror: invalid line 1: pick 17381ab62a\n> \n> The two lines above are printed when \"rebase --edit-todo\" loads the todo\n> list for the user to edit. With a real editor rather than \":\" we would then\n> print\n> \n> hint: Waiting for your editor to close the file ...\n> \n> Then hopefully the user would fix the errors. If not then when the editor\n> had finished we'd print the remaining errors as below.\n> \n> > \terror: 'pick' does not accept merge commits, please use 'merge -C'\n> > \terror: invalid line 1: pick 17381ab62a\n> > \tYou can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.\n> > \tOr you can abort the rebase with 'git rebase --abort'.\n> \n> I think that printing the error messages when the todo list is parsed before\n> it is given to the user to edit is helpful as it reminds the user what the\n> problem is. Your test looks confusing because it doesn't really simulate the\n> user editing the todo list.\n\nCertainly, the test was not clear to express my confusion.\n\nThe ones that are printed _before_ the editor is run are the ones that\nconfuse me, because when the user exits the editor we leave those\nlines there:\n\n\t$ GIT_EDITOR='sed -i s/pick/merge/' ./git rebase --edit\n\terror: 'pick' does not accept merge commits, please use 'merge -C'\n\terror: invalid line 1: pick 17381ab62a\n\nBut maybe it is my interpretation. Your reasoning of giving it as a\nhelp to the user makes sense.\n\nThank you for your explanations.\n\n> Best Wishes\n> \n> Phillip\n"},{"id":"492461","messageId":"994b3c35-b5f9-4e0b-b268-4ae19dfc6e15@gmail.com","threadId":"61008","inReplyTo":"7e4eae67-8db9-4be3-8b96-e2e89fa82abf@gmail.com","subject":"Re: [PATCH] rebase -i: improve error message when picking merge","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-07T13:55:42Z","receivedAt":"2024-04-07T13:55:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 06/04/2024 15:24, Rubén Justo wrote:\n> On Fri, Apr 05, 2024 at 10:30:37AM +0100, phillip.wood123@gmail.com wrote:\n>> On 04/04/2024 20:44, Rubén Justo wrote:\n>>> On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote:\n>>> ... I find these repeated messages confusing:\n>>>\n>> I think that printing the error messages when the todo list is parsed before\n>> it is given to the user to edit is helpful as it reminds the user what the\n>> problem is. Your test looks confusing because it doesn't really simulate the\n>> user editing the todo list.\n> \n> Certainly, the test was not clear to express my confusion.\n> \n> The ones that are printed _before_ the editor is run are the ones that\n> confuse me, because when the user exits the editor we leave those\n> lines there:\n> \n> \t$ GIT_EDITOR='sed -i s/pick/merge/' ./git rebase --edit\n> \terror: 'pick' does not accept merge commits, please use 'merge -C'\n> \terror: invalid line 1: pick 17381ab62a\n> \n> But maybe it is my interpretation. Your reasoning of giving it as a\n> help to the user makes sense.\n\nIdeally we'd clear the error messages when the user edited the todo list \nbut there isn't an easy way to do that. In principle we could stop \nprinting the error messages when parsing the list before it is edited \nbut I'm not sure how easy that would be. The behavior you are seeing is \nalready present is not changed by this patch - existing error messages \nbehave in the same way.\n\nBest Wishes\n\nPhillip\n\n> Thank you for your explanations.\n> \n>> Best Wishes\n>>\n>> Phillip\n"},{"id":"492536","messageId":"pull.1672.v2.git.1712585787.gitgitgadget@gmail.com","threadId":"61008","inReplyTo":"pull.1672.git.1708945087691.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] rebase -i: improve error message when picking merge","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-04-08T14:16:25Z","receivedAt":"2024-04-08T14:16:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"If the user tries to pick a merge commit error out when parsing the todo\nlist rather than complaining when trying to pick the commit.\n\nThanks to Patrick and Rubén for their comments on V1. Changes since V1:\n\n * Rebased to avoid a conflict with jk/core-comment-string\n * Patch 1 is a new preparatory change that plumbs 'struct replay_opts'\n through to parse_insn_line()\n * Patch 2 is updated to use is_rebase_i() rather than requiring the caller\n to pass a boolean indicating whether we're rebasing.\n\nPhillip Wood (2):\n rebase -i: pass struct replay_opts to parse_insn_line()\n rebase -i: improve error message when picking merge\n\n builtin/rebase.c | 17 +++++++----\n rebase-interactive.c | 21 ++++++++-----\n rebase-interactive.h | 9 ++++--\n sequencer.c | 57 ++++++++++++++++++++++++++++-------\n sequencer.h | 4 +--\n t/t3404-rebase-interactive.sh | 33 ++++++++++++++++++++\n 6 files changed, 111 insertions(+), 30 deletions(-)\n\n\nbase-commit: 19981daefd7c147444462739375462b49412ce33\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1672\n\nRange-diff vs v1:\n\n -: ----------- > 1: 1bcf92c6105 rebase -i: pass struct replay_opts to parse_insn_line()\n 1: 7726c496385 ! 2: fbc6746e018 rebase -i: improve error message when picking merge\n @@ Commit message\n \n exec git cherry-pick -m1 abc123\n \n - The change is relatively straight forward but is complicated slightly as\n - we now need to tell the parser if we're rebasing or not.\n -\n Reported-by: Stefan Haller \n Signed-off-by: Phillip Wood \n \n - ## builtin/rebase.c ##\n -@@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n - \telse {\n - \t\tdiscard_index(&the_index);\n - \t\tif (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n --\t\t\t\t\t\t&todo_list))\n -+\t\t\t\t\t\t&todo_list, 1))\n - \t\t\tBUG(\"unusable todo list\");\n - \n - \t\tret = complete_action(the_repository, &replay, flags,\n -\n - ## rebase-interactive.c ##\n -@@ rebase-interactive.c: int edit_todo_list(struct repository *r, struct todo_list *todo_list,\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 --\t\tincorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |\n -+\t\tincorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf,\n -+\t\t\t\t\t\t\ttodo_list, 1) |\n - \t\t\tfile_exists(rebase_path_dropped());\n - \n - \tif (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,\n -@@ rebase-interactive.c: int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n - \tif (initial && 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 -+\tif (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo, 1)) {\n - \t\tfprintf(stderr, _(edit_todo_list_advice));\n - \t\treturn -4;\n - \t}\n -@@ rebase-interactive.c: int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_\n - \tint res = 0;\n - \n - \tif (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {\n --\t\ttodo_list_parse_insn_buffer(r, backup.buf.buf, &backup);\n -+\t\ttodo_list_parse_insn_buffer(r, backup.buf.buf, &backup, 1);\n - \t\tres = todo_list_check(&backup, todo_list);\n - \t}\n - \n -\n ## sequencer.c ##\n @@ sequencer.c: static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n \treturn 0;\n }\n \n +-static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,\n +static int error_merge_commit(enum todo_command command)\n +{\n +\tswitch(command) {\n @@ sequencer.c: static int check_label_or_ref_arg(enum todo_command command, const\n +\t}\n +}\n +\n - static int parse_insn_line(struct repository *r, struct todo_item *item,\n --\t\t\t const char *buf, const char *bol, char *eol)\n -+\t\t\t const char *buf, const char *bol, char *eol,\n -+\t\t\t int rebasing)\n ++static int parse_insn_line(struct repository *r, struct replay_opts *opts,\n + \t\t\t struct todo_item *item, const char *buf,\n + \t\t\t const char *bol, char *eol)\n {\n - \tstruct object_id commit_oid;\n - \tchar *end_of_object_name;\n -@@ sequencer.c: static int parse_insn_line(struct repository *r, struct todo_item *item,\n +@@ sequencer.c: static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED\n \t\treturn status;\n \n \titem->commit = lookup_commit_reference(r, &commit_oid);\n -\treturn item->commit ? 0 : -1;\n +\tif (!item->commit)\n +\t\treturn -1;\n -+\tif (rebasing && item->command != TODO_MERGE &&\n ++\tif (is_rebase_i(opts) && item->command != TODO_MERGE &&\n +\t item->commit->parents && item->commit->parents->next)\n +\t\treturn error_merge_commit(item->command);\n +\treturn 0;\n }\n \n int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)\n -@@ sequencer.c: int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *\n - }\n - \n - int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n --\t\t\t\tstruct todo_list *todo_list)\n -+\t\t\t\tstruct todo_list *todo_list, int rebasing)\n - {\n - \tstruct todo_item *item;\n - \tchar *p = buf, *next_p;\n -@@ sequencer.c: int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n - \n - \t\titem = append_new_todo(todo_list);\n - \t\titem->offset_in_buf = p - todo_list->buf.buf;\n --\t\tif (parse_insn_line(r, item, buf, p, eol)) {\n -+\t\tif (parse_insn_line(r, item, buf, p, eol, rebasing)) {\n - \t\t\tres = error(_(\"invalid line %d: %.*s\"),\n - \t\t\t\ti, (int)(eol - p), p);\n - \t\t\titem->command = TODO_COMMENT + 1;\n -@@ sequencer.c: static int read_populate_todo(struct repository *r,\n - \tif (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)\n - \t\treturn -1;\n - \n --\tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);\n -+\tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list,\n -+\t\t\t\t\t is_rebase_i(opts));\n - \tif (res) {\n - \t\tif (is_rebase_i(opts))\n - \t\t\treturn error(_(\"please fix this using \"\n -@@ sequencer.c: static int read_populate_todo(struct repository *r,\n - \t\tstruct todo_list done = TODO_LIST_INIT;\n - \n - \t\tif (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&\n --\t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done))\n -+\t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done, 1))\n - \t\t\ttodo_list->done_nr = count_commands(&done);\n - \t\telse\n - \t\t\ttodo_list->done_nr = 0;\n -@@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n - \tstrbuf_release(&buf2);\n - \t/* Nothing is done yet, and we're reparsing, so let's reset the count */\n - \tnew_todo.total_nr = 0;\n --\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n -+\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo, 1) < 0)\n - \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n - \t\t new_todo.buf.buf);\n - \n -\n - ## sequencer.h ##\n -@@ sequencer.h: struct todo_list {\n - }\n - \n - int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n --\t\t\t\tstruct todo_list *todo_list);\n -+\t\t\t\tstruct todo_list *todo_list, int rebasing);\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 \n ## t/t3404-rebase-interactive.sh ##\n @@ t/t3404-rebase-interactive.sh: test_expect_success 'bad labels and refs rejected when parsing todo list' '\n\n-- \ngitgitgadget\n"},{"id":"492537","messageId":"1bcf92c6105e6e7ac2b2981575e112072fec383e.1712585787.git.gitgitgadget@gmail.com","threadId":"61008","inReplyTo":"pull.1672.v2.git.1712585787.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] rebase -i: pass struct replay_opts to parse_insn_line()","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-04-08T14:16:26Z","receivedAt":"2024-04-08T14:16:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThis new parameter will be used in the next commit. As adding the\nparameter requires quite a few changes to plumb it through the call\nchain these are separated into their own commit to avoid cluttering up\nthe next commit with incidental changes.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 17 +++++++++++------\n rebase-interactive.c | 21 +++++++++++++--------\n rebase-interactive.h | 9 ++++++---\n sequencer.c | 22 ++++++++++++----------\n sequencer.h | 4 ++--\n 5 files changed, 44 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 891f28468e8..a33a2ed413a 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -194,7 +194,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \treturn replay;\n }\n \n-static int edit_todo_file(unsigned flags)\n+static int edit_todo_file(unsigned flags, struct replay_opts *opts)\n {\n \tconst char *todo_file = rebase_path_todo();\n \tstruct todo_list todo_list = TODO_LIST_INIT,\n@@ -205,7 +205,8 @@ 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, comment_line_str);\n-\tres = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags);\n+\tres = edit_todo_list(the_repository, opts, &todo_list, &new_todo,\n+\t\t\t NULL, NULL, flags);\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\tres = error_errno(_(\"could not write '%s'\"), todo_file);\n@@ -296,8 +297,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \t\terror(_(\"could not generate todo list\"));\n \telse {\n \t\tdiscard_index(&the_index);\n-\t\tif (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n-\t\t\t\t\t\t&todo_list))\n+\t\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n+\t\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n \t\t\tBUG(\"unusable todo list\");\n \n \t\tret = complete_action(the_repository, &replay, flags,\n@@ -352,9 +353,13 @@ static int run_sequencer_rebase(struct rebase_options *opts)\n \t\treplay_opts_release(&replay_opts);\n \t\tbreak;\n \t}\n-\tcase ACTION_EDIT_TODO:\n-\t\tret = edit_todo_file(flags);\n+\tcase ACTION_EDIT_TODO: {\n+\t\tstruct replay_opts replay_opts = get_replay_opts(opts);\n+\n+\t\tret = edit_todo_file(flags, &replay_opts);\n+\t\treplay_opts_release(&replay_opts);\n \t\tbreak;\n+\t}\n \tcase ACTION_SHOW_CURRENT_PATCH: {\n \t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex c343e16fcdd..56fd7206a95 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -101,9 +101,10 @@ void append_todo_help(int command_count,\n \tstrbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_str);\n }\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+int edit_todo_list(struct repository *r, struct replay_opts *opts,\n+\t\t struct todo_list *todo_list, struct todo_list *new_todo,\n+\t\t const char *shortrevisions, const char *shortonto,\n+\t\t unsigned flags)\n {\n \tconst char *todo_file = rebase_path_todo(),\n \t\t*todo_backup = rebase_path_todo_backup();\n@@ -114,7 +115,9 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\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-\t\tincorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |\n+\t\tincorrect = todo_list_parse_insn_buffer(r, opts,\n+\t\t\t\t\t\t\ttodo_list->buf.buf,\n+\t\t\t\t\t\t\ttodo_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@@ -134,13 +137,13 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n \tif (initial && 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+\tif (todo_list_parse_insn_buffer(r, opts, new_todo->buf.buf, new_todo)) {\n \t\tfprintf(stderr, _(edit_todo_list_advice));\n \t\treturn -4;\n \t}\n \n \tif (incorrect) {\n-\t\tif (todo_list_check_against_backup(r, new_todo)) {\n+\t\tif (todo_list_check_against_backup(r, opts, new_todo)) {\n \t\t\twrite_file(rebase_path_dropped(), \"%s\", \"\");\n \t\t\treturn -4;\n \t\t}\n@@ -228,13 +231,15 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)\n \treturn res;\n }\n \n-int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_list)\n+int todo_list_check_against_backup(struct repository *r,\n+\t\t\t\t struct replay_opts *opts,\n+\t\t\t\t struct todo_list *todo_list)\n {\n \tstruct todo_list backup = TODO_LIST_INIT;\n \tint res = 0;\n \n \tif (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {\n-\t\ttodo_list_parse_insn_buffer(r, backup.buf.buf, &backup);\n+\t\ttodo_list_parse_insn_buffer(r, opts, backup.buf.buf, &backup);\n \t\tres = todo_list_check(&backup, todo_list);\n \t}\n \ndiff --git a/rebase-interactive.h b/rebase-interactive.h\nindex 7239c60f791..8e5b181b334 100644\n--- a/rebase-interactive.h\n+++ b/rebase-interactive.h\n@@ -3,17 +3,20 @@\n \n struct strbuf;\n struct repository;\n+struct replay_opts;\n struct todo_list;\n \n void append_todo_help(int command_count,\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+int edit_todo_list(struct repository *r, struct replay_opts *opts,\n+\t\t struct todo_list *todo_list, struct todo_list *new_todo,\n+\t\t const char *shortrevisions, const char *shortonto,\n+\t\t unsigned flags);\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+\t\t\t\t struct replay_opts *opts,\n \t\t\t\t struct todo_list *todo_list);\n \n #endif\ndiff --git a/sequencer.c b/sequencer.c\nindex 2c19846385b..a3154ba3347 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2573,8 +2573,9 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n \treturn 0;\n }\n \n-static int parse_insn_line(struct repository *r, struct todo_item *item,\n-\t\t\t const char *buf, const char *bol, char *eol)\n+static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,\n+\t\t\t struct todo_item *item, const char *buf,\n+\t\t\t const char *bol, char *eol)\n {\n \tstruct object_id commit_oid;\n \tchar *end_of_object_name;\n@@ -2708,8 +2709,8 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *\n \treturn ret;\n }\n \n-int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n-\t\t\t\tstruct todo_list *todo_list)\n+int todo_list_parse_insn_buffer(struct repository *r, struct replay_opts *opts,\n+\t\t\t\tchar *buf, struct todo_list *todo_list)\n {\n \tstruct todo_item *item;\n \tchar *p = buf, *next_p;\n@@ -2727,7 +2728,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n \n \t\titem = append_new_todo(todo_list);\n \t\titem->offset_in_buf = p - todo_list->buf.buf;\n-\t\tif (parse_insn_line(r, item, buf, p, eol)) {\n+\t\tif (parse_insn_line(r, opts, item, buf, p, eol)) {\n \t\t\tres = error(_(\"invalid line %d: %.*s\"),\n \t\t\t\ti, (int)(eol - p), p);\n \t\t\titem->command = TODO_COMMENT + 1;\n@@ -2875,7 +2876,7 @@ static int read_populate_todo(struct repository *r,\n \tif (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)\n \t\treturn -1;\n \n-\tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);\n+\tres = todo_list_parse_insn_buffer(r, opts, todo_list->buf.buf, todo_list);\n \tif (res) {\n \t\tif (is_rebase_i(opts))\n \t\t\treturn error(_(\"please fix this using \"\n@@ -2905,7 +2906,7 @@ static int read_populate_todo(struct repository *r,\n \t\tstruct todo_list done = TODO_LIST_INIT;\n \n \t\tif (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&\n-\t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done))\n+\t\t !todo_list_parse_insn_buffer(r, opts, done.buf.buf, &done))\n \t\t\ttodo_list->done_nr = count_commands(&done);\n \t\telse\n \t\t\ttodo_list->done_nr = 0;\n@@ -5251,7 +5252,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \t\t\tgoto release_todo_list;\n \n \t\tif (file_exists(rebase_path_dropped())) {\n-\t\t\tif ((res = todo_list_check_against_backup(r, &todo_list)))\n+\t\t\tif ((res = todo_list_check_against_backup(r, opts,\n+\t\t\t\t\t\t\t\t &todo_list)))\n \t\t\t\tgoto release_todo_list;\n \n \t\t\tunlink(rebase_path_dropped());\n@@ -6294,7 +6296,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn error(_(\"nothing to do\"));\n \t}\n \n-\tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n+\tres = edit_todo_list(r, opts, todo_list, &new_todo, shortrevisions,\n \t\t\t shortonto, flags);\n \tif (res == -1)\n \t\treturn -1;\n@@ -6322,7 +6324,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \tstrbuf_release(&buf2);\n \t/* Nothing is done yet, and we're reparsing, so let's reset the count */\n \tnew_todo.total_nr = 0;\n-\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n+\tif (todo_list_parse_insn_buffer(r, opts, 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 \ndiff --git a/sequencer.h b/sequencer.h\nindex 437eabd38af..33943e5ed28 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -137,8 +137,8 @@ struct todo_list {\n \t.buf = STRBUF_INIT, \\\n }\n \n-int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n-\t\t\t\tstruct todo_list *todo_list);\n+int todo_list_parse_insn_buffer(struct repository *r, struct replay_opts *opts,\n+\t\t\t\tchar *buf, struct 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-- \ngitgitgadget\n\n"},{"id":"492538","messageId":"fbc6746e0188ed7b69c238935ec85b69112ddd79.1712585788.git.gitgitgadget@gmail.com","threadId":"61008","inReplyTo":"pull.1672.v2.git.1712585787.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] rebase -i: improve error message when picking merge","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-04-08T14:16:27Z","receivedAt":"2024-04-08T14:16:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe only todo commands that accept a merge commit are \"merge\" and\n\"reset\". All the other commands like \"pick\" or \"reword\" fail when they\ntry to pick a a merge commit and print the message\n\n error: commit abc123 is a merge but no -m option was given.\n\nfollowed by a hint about the command being rescheduled. This message is\ndesigned to help the user when they cherry-pick a merge and forget to\npass \"-m\". For users who are rebasing the message is confusing as there\nis no way for rebase to cherry-pick the merge.\n\nImprove the user experience by detecting the error when the todo list is\nparsed rather than waiting for the \"pick\" command to fail and print a\nmessage recommending the \"merge\" command instead. We recommend \"merge\"\nrather than \"exec git cherry-pick -m ...\" on the assumption that\ncherry-picking merges is relatively rare and it is more likely that the\nuser chose \"pick\" by a mistake.\n\nIt would be possible to support cherry-picking merges by allowing the\nuser to pass \"-m\" to \"pick\" commands but that adds complexity to do\nsomething that can already be achieved with\n\n exec git cherry-pick -m1 abc123\n\nReported-by: Stefan Haller \nSigned-off-by: Phillip Wood \n---\n sequencer.c | 37 +++++++++++++++++++++++++++++++++--\n t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++++++++++\n 2 files changed, 68 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex a3154ba3347..4012c6f88d9 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2573,7 +2573,35 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n \treturn 0;\n }\n \n-static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,\n+static int error_merge_commit(enum todo_command command)\n+{\n+\tswitch(command) {\n+\tcase TODO_PICK:\n+\t\treturn error(_(\"'%s' does not accept merge commits, \"\n+\t\t\t \"please use '%s'\"),\n+\t\t\t todo_command_info[command].str, \"merge -C\");\n+\n+\tcase TODO_REWORD:\n+\t\treturn error(_(\"'%s' does not accept merge commits, \"\n+\t\t\t \"please use '%s'\"),\n+\t\t\t todo_command_info[command].str, \"merge -c\");\n+\n+\tcase TODO_EDIT:\n+\t\treturn error(_(\"'%s' does not accept merge commits, \"\n+\t\t\t \"please use '%s' followed by '%s'\"),\n+\t\t\t todo_command_info[command].str,\n+\t\t\t \"merge -C\", \"break\");\n+\n+\tcase TODO_FIXUP:\n+\tcase TODO_SQUASH:\n+\t\treturn error(_(\"cannot squash merge commit into another commit\"));\n+\n+\tdefault:\n+\t\tBUG(\"unexpected todo_command\");\n+\t}\n+}\n+\n+static int parse_insn_line(struct repository *r, struct replay_opts *opts,\n \t\t\t struct todo_item *item, const char *buf,\n \t\t\t const char *bol, char *eol)\n {\n@@ -2679,7 +2707,12 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED\n \t\treturn status;\n \n \titem->commit = lookup_commit_reference(r, &commit_oid);\n-\treturn item->commit ? 0 : -1;\n+\tif (!item->commit)\n+\t\treturn -1;\n+\tif (is_rebase_i(opts) && item->command != TODO_MERGE &&\n+\t item->commit->parents && item->commit->parents->next)\n+\t\treturn error_merge_commit(item->command);\n+\treturn 0;\n }\n \n int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d1bead61fad..8565fd4b5ae 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -2215,6 +2215,39 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' '\n \ttest_path_is_missing execed\n '\n \n+test_expect_success 'non-merge commands reject merge commits' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tgit checkout E &&\n+\tgit merge I &&\n+\toid=$(git rev-parse HEAD) &&\n+\tcat >todo <<-EOF &&\n+\tpick $oid\n+\treword $oid\n+\tedit $oid\n+\tfixup $oid\n+\tsquash $oid\n+\tEOF\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\ttest_must_fail git rebase -i HEAD 2>actual\n+\t) &&\n+\tcat >expect <<-EOF &&\n+\terror: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ}\n+\terror: invalid line 1: pick $oid\n+\terror: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ}\n+\terror: invalid line 2: reword $oid\n+\terror: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}\n+\terror: invalid line 3: edit $oid\n+\terror: cannot squash merge commit into another commit\n+\terror: invalid line 4: fixup $oid\n+\terror: cannot squash merge commit into another commit\n+\terror: invalid line 5: squash $oid\n+\tYou can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.\n+\tOr you can abort the rebase with ${SQ}git rebase --abort${SQ}.\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n # This must be the last test in this file\n test_expect_success '$EDITOR and friends are unchanged' '\n \ttest_editor_unchanged\n-- \ngitgitgadget\n"},{"id":"492590","messageId":"xmqqle5n7ajt.fsf@gitster.g","threadId":"61008","inReplyTo":"fbc6746e0188ed7b69c238935ec85b69112ddd79.1712585788.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] rebase -i: improve error message when picking merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T22:29:42Z","receivedAt":"2024-04-08T22:29:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> From: Phillip Wood \n>\n> The only todo commands that accept a merge commit are \"merge\" and\n> \"reset\". All the other commands like \"pick\" or \"reword\" fail when they\n> try to pick a a merge commit and print the message\n>\n> error: commit abc123 is a merge but no -m option was given.\n>\n> followed by a hint about the command being rescheduled. This message is\n> designed to help the user when they cherry-pick a merge and forget to\n> pass \"-m\". For users who are rebasing the message is confusing as there\n> is no way for rebase to cherry-pick the merge.\n>\n> Improve the user experience by detecting the error when the todo list is\n> parsed rather than waiting for the \"pick\" command to fail and print a\n> message recommending the \"merge\" command instead. We recommend \"merge\"\n> rather than \"exec git cherry-pick -m ...\" on the assumption that\n> cherry-picking merges is relatively rare and it is more likely that the\n> user chose \"pick\" by a mistake.\n\nNow, the mention of \"all the other commands\" makes me curious what\nshould happen when your \"squash\" and \"fixup\" named a merge commit.\nI think it should just error out without any recourse, but it is\nmore than likely that I am missing some use cases where it is useful\nto \"squash\" or \"fixup\" a merge commit on top of an existing commit?\n\n> It would be possible to support cherry-picking merges by allowing the\n> user to pass \"-m\" to \"pick\" commands but that adds complexity to do\n> something that can already be achieved with\n>\n> exec git cherry-pick -m1 abc123\n\nI have no strong opinions between this and \"merge\" for \"pick\",\n\"edit\", and \"reword\".\n\n> Reported-by: Stefan Haller \n> Signed-off-by: Phillip Wood \n> ---\n> sequencer.c | 37 +++++++++++++++++++++++++++++++++--\n> t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++++++++++\n> 2 files changed, 68 insertions(+), 2 deletions(-)\n\nSo, having thought about my version of a solution from the problem\ndescription above without looking at your answers, let's see how you\nsolved it.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index a3154ba3347..4012c6f88d9 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2573,7 +2573,35 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n> \treturn 0;\n> }\n> \n> -static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,\n> +static int error_merge_commit(enum todo_command command)\n> +{\n> +\tswitch(command) {\n> +\tcase TODO_PICK:\n> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n> +\t\t\t \"please use '%s'\"),\n> +\t\t\t todo_command_info[command].str, \"merge -C\");\n> +\n> +\tcase TODO_REWORD:\n> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n> +\t\t\t \"please use '%s'\"),\n> +\t\t\t todo_command_info[command].str, \"merge -c\");\n> +\n> +\tcase TODO_EDIT:\n> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n> +\t\t\t \"please use '%s' followed by '%s'\"),\n> +\t\t\t todo_command_info[command].str,\n> +\t\t\t \"merge -C\", \"break\");\n\nOK. And when hitting the \"break\", they know that they are supposed\nto say \"git commit --amend\" and then \"git rebase --continue\"?\n\n> +\tcase TODO_FIXUP:\n> +\tcase TODO_SQUASH:\n> +\t\treturn error(_(\"cannot squash merge commit into another commit\"));\n\nOK, this is as I expected.\n\n> +\tdefault:\n> +\t\tBUG(\"unexpected todo_command\");\n> +\t}\n> +}\n> +\n> +static int parse_insn_line(struct repository *r, struct replay_opts *opts,\n> \t\t\t struct todo_item *item, const char *buf,\n> \t\t\t const char *bol, char *eol)\n> {\n> @@ -2679,7 +2707,12 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED\n> \t\treturn status;\n> \n> \titem->commit = lookup_commit_reference(r, &commit_oid);\n> -\treturn item->commit ? 0 : -1;\n> +\tif (!item->commit)\n> +\t\treturn -1;\n> +\tif (is_rebase_i(opts) && item->command != TODO_MERGE &&\n> +\t item->commit->parents && item->commit->parents->next)\n> +\t\treturn error_merge_commit(item->command);\n\nThis is good for now, but we may see command other than TODO_MERGE\nlearn how to handle a merge commit, and when that happens, I wonder\nwhat we want to do here. One thought is to do this:\n\n\tif (is_rebase_i(opts) && is_merge_commit(item->commit))\n \treturn error_merge_commit(item);\n\nand teach error_merge_commit() to silently return 0 on TODO_MERGE.\nOther commands, when they learn how to deal with a merge commit,\nwill then update their entries in error_merge_commit().\n\nWould we want to customize the message from error_merge_commit() to\nmake it closer to cut-and-paste ready? For that, something like\n\n\tint error_merge_commit(struct todo_item *item)\n\t{\n\t\tswitch (item->command) {\n\t\tcase TODO_PICK:\n\t\t\treturn error(_(\"'%s'\" is bad, plase use \"\n\t\t\t\t '%s %s'\"),\n\t\t\t\ttodo_command_info[item->command].str,\n\t\t\t\t\"merge -C\",\n\t\t\t\toid_to_hex(item->commit->oid));\n\t\t...\n\t\t}\n\t}\n\nmight go in a more friendly way.\n\n"},{"id":"492617","messageId":"ZhS-IllueSZa1NXs@tanuki","threadId":"61008","inReplyTo":"xmqqle5n7ajt.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] rebase -i: improve error message when picking merge","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-09T04:03:46Z","receivedAt":"2024-04-09T04:03:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 08, 2024 at 03:29:42PM -0700, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n> \n> > From: Phillip Wood \n> >\n> > The only todo commands that accept a merge commit are \"merge\" and\n> > \"reset\". All the other commands like \"pick\" or \"reword\" fail when they\n> > try to pick a a merge commit and print the message\n> >\n> > error: commit abc123 is a merge but no -m option was given.\n> >\n> > followed by a hint about the command being rescheduled. This message is\n> > designed to help the user when they cherry-pick a merge and forget to\n> > pass \"-m\". For users who are rebasing the message is confusing as there\n> > is no way for rebase to cherry-pick the merge.\n> >\n> > Improve the user experience by detecting the error when the todo list is\n> > parsed rather than waiting for the \"pick\" command to fail and print a\n> > message recommending the \"merge\" command instead. We recommend \"merge\"\n> > rather than \"exec git cherry-pick -m ...\" on the assumption that\n> > cherry-picking merges is relatively rare and it is more likely that the\n> > user chose \"pick\" by a mistake.\n> \n> Now, the mention of \"all the other commands\" makes me curious what\n> should happen when your \"squash\" and \"fixup\" named a merge commit.\n> I think it should just error out without any recourse, but it is\n> more than likely that I am missing some use cases where it is useful\n> to \"squash\" or \"fixup\" a merge commit on top of an existing commit?\n> \n> > It would be possible to support cherry-picking merges by allowing the\n> > user to pass \"-m\" to \"pick\" commands but that adds complexity to do\n> > something that can already be achieved with\n> >\n> > exec git cherry-pick -m1 abc123\n> \n> I have no strong opinions between this and \"merge\" for \"pick\",\n> \"edit\", and \"reword\".\n> \n> > Reported-by: Stefan Haller \n> > Signed-off-by: Phillip Wood \n> > ---\n> > sequencer.c | 37 +++++++++++++++++++++++++++++++++--\n> > t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++++++++++\n> > 2 files changed, 68 insertions(+), 2 deletions(-)\n> \n> So, having thought about my version of a solution from the problem\n> description above without looking at your answers, let's see how you\n> solved it.\n> \n> > diff --git a/sequencer.c b/sequencer.c\n> > index a3154ba3347..4012c6f88d9 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -2573,7 +2573,35 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n> > \treturn 0;\n> > }\n> > \n> > -static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,\n> > +static int error_merge_commit(enum todo_command command)\n> > +{\n> > +\tswitch(command) {\n> > +\tcase TODO_PICK:\n> > +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n> > +\t\t\t \"please use '%s'\"),\n> > +\t\t\t todo_command_info[command].str, \"merge -C\");\n> > +\n> > +\tcase TODO_REWORD:\n> > +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n> > +\t\t\t \"please use '%s'\"),\n> > +\t\t\t todo_command_info[command].str, \"merge -c\");\n> > +\n> > +\tcase TODO_EDIT:\n> > +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n> > +\t\t\t \"please use '%s' followed by '%s'\"),\n> > +\t\t\t todo_command_info[command].str,\n> > +\t\t\t \"merge -C\", \"break\");\n> \n> OK. And when hitting the \"break\", they know that they are supposed\n> to say \"git commit --amend\" and then \"git rebase --continue\"?\n> \n> > +\tcase TODO_FIXUP:\n> > +\tcase TODO_SQUASH:\n> > +\t\treturn error(_(\"cannot squash merge commit into another commit\"));\n> \n> OK, this is as I expected.\n> \n> > +\tdefault:\n> > +\t\tBUG(\"unexpected todo_command\");\n> > +\t}\n> > +}\n> > +\n> > +static int parse_insn_line(struct repository *r, struct replay_opts *opts,\n> > \t\t\t struct todo_item *item, const char *buf,\n> > \t\t\t const char *bol, char *eol)\n> > {\n> > @@ -2679,7 +2707,12 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED\n> > \t\treturn status;\n> > \n> > \titem->commit = lookup_commit_reference(r, &commit_oid);\n> > -\treturn item->commit ? 0 : -1;\n> > +\tif (!item->commit)\n> > +\t\treturn -1;\n> > +\tif (is_rebase_i(opts) && item->command != TODO_MERGE &&\n> > +\t item->commit->parents && item->commit->parents->next)\n> > +\t\treturn error_merge_commit(item->command);\n> \n> This is good for now, but we may see command other than TODO_MERGE\n> learn how to handle a merge commit, and when that happens, I wonder\n> what we want to do here. One thought is to do this:\n> \n> \tif (is_rebase_i(opts) && is_merge_commit(item->commit))\n> \treturn error_merge_commit(item);\n> \n> and teach error_merge_commit() to silently return 0 on TODO_MERGE.\n> Other commands, when they learn how to deal with a merge commit,\n> will then update their entries in error_merge_commit().\n> \n> Would we want to customize the message from error_merge_commit() to\n> make it closer to cut-and-paste ready? For that, something like\n> \n> \tint error_merge_commit(struct todo_item *item)\n> \t{\n> \t\tswitch (item->command) {\n> \t\tcase TODO_PICK:\n> \t\t\treturn error(_(\"'%s'\" is bad, plase use \"\n> \t\t\t\t '%s %s'\"),\n> \t\t\t\ttodo_command_info[item->command].str,\n> \t\t\t\t\"merge -C\",\n> \t\t\t\toid_to_hex(item->commit->oid));\n> \t\t...\n> \t\t}\n> \t}\n> \n> might go in a more friendly way.\n\nI was asking basically the same thing in [1]. Quoting that part:\n\n> I wonder how actionable these commands are. Can we give the full command\n> that the user can use instead, including the commit ID?\n> \n> That raises another question though: how exactly is the user supposed to\n> perform the merge? Should they merge the merge commit, resulting in two\n> merge commits? Should they pick one of the sides, and if so, which one?\n> I guess the answer is \"it depends\", which makes it harder for us to come\n> up with actionable advice here.\n\nSo I think it's okay to not mention the exact commit here because we\ncannot reliably second-guess the ultimate extent. My basic assumption is\nthat in many cases the user may not even be aware of them trying to pick\na merge commit, and that it may not have been their intent.\n\nI might just as well be wrong about that assumption though.\n\nPatrick\n\n[1]: https://lore.kernel.org/git/Zg5D3dXYFM2SONE-@tanuki/\n"},{"id":"492618","messageId":"ZhS-KktVxRH-Xl3V@tanuki","threadId":"61008","inReplyTo":"1bcf92c6105e6e7ac2b2981575e112072fec383e.1712585787.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] rebase -i: pass struct replay_opts to parse_insn_line()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-09T04:03:54Z","receivedAt":"2024-04-09T04:03:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 08, 2024 at 02:16:26PM +0000, Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood \n> \n> This new parameter will be used in the next commit.\n\nNit: we don't typically write commit messages as a continuation of their\nsubject. Other than that this commit looks good to me.\n\nPatrick\n\n> As adding the\n> parameter requires quite a few changes to plumb it through the call\n> chain these are separated into their own commit to avoid cluttering up\n> the next commit with incidental changes.\n> \n> Signed-off-by: Phillip Wood \n> ---\n> builtin/rebase.c | 17 +++++++++++------\n> rebase-interactive.c | 21 +++++++++++++--------\n> rebase-interactive.h | 9 ++++++---\n> sequencer.c | 22 ++++++++++++----------\n> sequencer.h | 4 ++--\n> 5 files changed, 44 insertions(+), 29 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 891f28468e8..a33a2ed413a 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -194,7 +194,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n> \treturn replay;\n> }\n> \n> -static int edit_todo_file(unsigned flags)\n> +static int edit_todo_file(unsigned flags, struct replay_opts *opts)\n> {\n> \tconst char *todo_file = rebase_path_todo();\n> \tstruct todo_list todo_list = TODO_LIST_INIT,\n> @@ -205,7 +205,8 @@ 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, comment_line_str);\n> -\tres = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags);\n> +\tres = edit_todo_list(the_repository, opts, &todo_list, &new_todo,\n> +\t\t\t NULL, NULL, flags);\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\tres = error_errno(_(\"could not write '%s'\"), todo_file);\n> @@ -296,8 +297,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n> \t\terror(_(\"could not generate todo list\"));\n> \telse {\n> \t\tdiscard_index(&the_index);\n> -\t\tif (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n> -\t\t\t\t\t\t&todo_list))\n> +\t\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n> +\t\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n> \t\t\tBUG(\"unusable todo list\");\n> \n> \t\tret = complete_action(the_repository, &replay, flags,\n> @@ -352,9 +353,13 @@ static int run_sequencer_rebase(struct rebase_options *opts)\n> \t\treplay_opts_release(&replay_opts);\n> \t\tbreak;\n> \t}\n> -\tcase ACTION_EDIT_TODO:\n> -\t\tret = edit_todo_file(flags);\n> +\tcase ACTION_EDIT_TODO: {\n> +\t\tstruct replay_opts replay_opts = get_replay_opts(opts);\n> +\n> +\t\tret = edit_todo_file(flags, &replay_opts);\n> +\t\treplay_opts_release(&replay_opts);\n> \t\tbreak;\n> +\t}\n> \tcase ACTION_SHOW_CURRENT_PATCH: {\n> \t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> \n> diff --git a/rebase-interactive.c b/rebase-interactive.c\n> index c343e16fcdd..56fd7206a95 100644\n> --- a/rebase-interactive.c\n> +++ b/rebase-interactive.c\n> @@ -101,9 +101,10 @@ void append_todo_help(int command_count,\n> \tstrbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_str);\n> }\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> +int edit_todo_list(struct repository *r, struct replay_opts *opts,\n> +\t\t struct todo_list *todo_list, struct todo_list *new_todo,\n> +\t\t const char *shortrevisions, const char *shortonto,\n> +\t\t unsigned flags)\n> {\n> \tconst char *todo_file = rebase_path_todo(),\n> \t\t*todo_backup = rebase_path_todo_backup();\n> @@ -114,7 +115,9 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\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> -\t\tincorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |\n> +\t\tincorrect = todo_list_parse_insn_buffer(r, opts,\n> +\t\t\t\t\t\t\ttodo_list->buf.buf,\n> +\t\t\t\t\t\t\ttodo_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> @@ -134,13 +137,13 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n> \tif (initial && 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> +\tif (todo_list_parse_insn_buffer(r, opts, new_todo->buf.buf, new_todo)) {\n> \t\tfprintf(stderr, _(edit_todo_list_advice));\n> \t\treturn -4;\n> \t}\n> \n> \tif (incorrect) {\n> -\t\tif (todo_list_check_against_backup(r, new_todo)) {\n> +\t\tif (todo_list_check_against_backup(r, opts, new_todo)) {\n> \t\t\twrite_file(rebase_path_dropped(), \"%s\", \"\");\n> \t\t\treturn -4;\n> \t\t}\n> @@ -228,13 +231,15 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)\n> \treturn res;\n> }\n> \n> -int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_list)\n> +int todo_list_check_against_backup(struct repository *r,\n> +\t\t\t\t struct replay_opts *opts,\n> +\t\t\t\t struct todo_list *todo_list)\n> {\n> \tstruct todo_list backup = TODO_LIST_INIT;\n> \tint res = 0;\n> \n> \tif (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {\n> -\t\ttodo_list_parse_insn_buffer(r, backup.buf.buf, &backup);\n> +\t\ttodo_list_parse_insn_buffer(r, opts, backup.buf.buf, &backup);\n> \t\tres = todo_list_check(&backup, todo_list);\n> \t}\n> \n> diff --git a/rebase-interactive.h b/rebase-interactive.h\n> index 7239c60f791..8e5b181b334 100644\n> --- a/rebase-interactive.h\n> +++ b/rebase-interactive.h\n> @@ -3,17 +3,20 @@\n> \n> struct strbuf;\n> struct repository;\n> +struct replay_opts;\n> struct todo_list;\n> \n> void append_todo_help(int command_count,\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> +int edit_todo_list(struct repository *r, struct replay_opts *opts,\n> +\t\t struct todo_list *todo_list, struct todo_list *new_todo,\n> +\t\t const char *shortrevisions, const char *shortonto,\n> +\t\t unsigned flags);\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> +\t\t\t\t struct replay_opts *opts,\n> \t\t\t\t struct todo_list *todo_list);\n> \n> #endif\n> diff --git a/sequencer.c b/sequencer.c\n> index 2c19846385b..a3154ba3347 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2573,8 +2573,9 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n> \treturn 0;\n> }\n> \n> -static int parse_insn_line(struct repository *r, struct todo_item *item,\n> -\t\t\t const char *buf, const char *bol, char *eol)\n> +static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,\n> +\t\t\t struct todo_item *item, const char *buf,\n> +\t\t\t const char *bol, char *eol)\n> {\n> \tstruct object_id commit_oid;\n> \tchar *end_of_object_name;\n> @@ -2708,8 +2709,8 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *\n> \treturn ret;\n> }\n> \n> -int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n> -\t\t\t\tstruct todo_list *todo_list)\n> +int todo_list_parse_insn_buffer(struct repository *r, struct replay_opts *opts,\n> +\t\t\t\tchar *buf, struct todo_list *todo_list)\n> {\n> \tstruct todo_item *item;\n> \tchar *p = buf, *next_p;\n> @@ -2727,7 +2728,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n> \n> \t\titem = append_new_todo(todo_list);\n> \t\titem->offset_in_buf = p - todo_list->buf.buf;\n> -\t\tif (parse_insn_line(r, item, buf, p, eol)) {\n> +\t\tif (parse_insn_line(r, opts, item, buf, p, eol)) {\n> \t\t\tres = error(_(\"invalid line %d: %.*s\"),\n> \t\t\t\ti, (int)(eol - p), p);\n> \t\t\titem->command = TODO_COMMENT + 1;\n> @@ -2875,7 +2876,7 @@ static int read_populate_todo(struct repository *r,\n> \tif (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)\n> \t\treturn -1;\n> \n> -\tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);\n> +\tres = todo_list_parse_insn_buffer(r, opts, todo_list->buf.buf, todo_list);\n> \tif (res) {\n> \t\tif (is_rebase_i(opts))\n> \t\t\treturn error(_(\"please fix this using \"\n> @@ -2905,7 +2906,7 @@ static int read_populate_todo(struct repository *r,\n> \t\tstruct todo_list done = TODO_LIST_INIT;\n> \n> \t\tif (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&\n> -\t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done))\n> +\t\t !todo_list_parse_insn_buffer(r, opts, done.buf.buf, &done))\n> \t\t\ttodo_list->done_nr = count_commands(&done);\n> \t\telse\n> \t\t\ttodo_list->done_nr = 0;\n> @@ -5251,7 +5252,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n> \t\t\tgoto release_todo_list;\n> \n> \t\tif (file_exists(rebase_path_dropped())) {\n> -\t\t\tif ((res = todo_list_check_against_backup(r, &todo_list)))\n> +\t\t\tif ((res = todo_list_check_against_backup(r, opts,\n> +\t\t\t\t\t\t\t\t &todo_list)))\n> \t\t\t\tgoto release_todo_list;\n> \n> \t\t\tunlink(rebase_path_dropped());\n> @@ -6294,7 +6296,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \t\treturn error(_(\"nothing to do\"));\n> \t}\n> \n> -\tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n> +\tres = edit_todo_list(r, opts, todo_list, &new_todo, shortrevisions,\n> \t\t\t shortonto, flags);\n> \tif (res == -1)\n> \t\treturn -1;\n> @@ -6322,7 +6324,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \tstrbuf_release(&buf2);\n> \t/* Nothing is done yet, and we're reparsing, so let's reset the count */\n> \tnew_todo.total_nr = 0;\n> -\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n> +\tif (todo_list_parse_insn_buffer(r, opts, 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> diff --git a/sequencer.h b/sequencer.h\n> index 437eabd38af..33943e5ed28 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -137,8 +137,8 @@ struct todo_list {\n> \t.buf = STRBUF_INIT, \\\n> }\n> \n> -int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n> -\t\t\t\tstruct todo_list *todo_list);\n> +int todo_list_parse_insn_buffer(struct repository *r, struct replay_opts *opts,\n> +\t\t\t\tchar *buf, struct 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> -- \n> gitgitgadget\n> \n"},{"id":"492619","messageId":"xmqqh6gb2kdz.fsf@gitster.g","threadId":"61008","inReplyTo":"ZhS-IllueSZa1NXs@tanuki","subject":"Re: [PATCH v2 2/2] rebase -i: improve error message when picking merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-09T05:08:24Z","receivedAt":"2024-04-09T05:08:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> So I think it's okay to not mention the exact commit here because we\n> cannot reliably second-guess the ultimate extent. My basic assumption is\n> that in many cases the user may not even be aware of them trying to pick\n> a merge commit, and that it may not have been their intent.\n\nHmph, if that assumption is really true, would suggesting \"merge -C\"\nbe the right thing in the first place?\n\nI do not deal with non-linear stuff in \"rebase -i\" or sequencer, so\nI may not be a good person to judge how good your basic assumption\nis, though.\n\nThanks.\n"},{"id":"492622","messageId":"ZhTaVfEEza0Z00em@tanuki","threadId":"61008","inReplyTo":"xmqqh6gb2kdz.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] rebase -i: improve error message when picking merge","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-09T06:04:05Z","receivedAt":"2024-04-09T06:04:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 08, 2024 at 10:08:24PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > So I think it's okay to not mention the exact commit here because we\n> > cannot reliably second-guess the ultimate extent. My basic assumption is\n> > that in many cases the user may not even be aware of them trying to pick\n> > a merge commit, and that it may not have been their intent.\n> \n> Hmph, if that assumption is really true, would suggesting \"merge -C\"\n> be the right thing in the first place?\n> \n> I do not deal with non-linear stuff in \"rebase -i\" or sequencer, so\n> I may not be a good person to judge how good your basic assumption\n> is, though.\n\nYeah, neither do I. The assumption really only comes from my own gut\nfeeling, which can certainly be quite wrong.\n\nPatrick\n"},{"id":"492634","messageId":"1b74f6f9-f4b9-4909-82b3-26f19b7a1347@gmail.com","threadId":"61008","inReplyTo":"xmqqle5n7ajt.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] rebase -i: improve error message when picking merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-09T15:04:33Z","receivedAt":"2024-04-09T15:04:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 08/04/2024 23:29, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n>> +\tcase TODO_EDIT:\n>> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n>> +\t\t\t \"please use '%s' followed by '%s'\"),\n>> +\t\t\t todo_command_info[command].str,\n>> +\t\t\t \"merge -C\", \"break\");\n> \n> OK. And when hitting the \"break\", they know that they are supposed\n> to say \"git commit --amend\" and then \"git rebase --continue\"?\n\nYes. I guess we could add a hint to that effect if you think its worth it.\n\n>> \titem->commit = lookup_commit_reference(r, &commit_oid);\n>> -\treturn item->commit ? 0 : -1;\n>> +\tif (!item->commit)\n>> +\t\treturn -1;\n>> +\tif (is_rebase_i(opts) && item->command != TODO_MERGE &&\n>> +\t item->commit->parents && item->commit->parents->next)\n>> +\t\treturn error_merge_commit(item->command);\n> \n> This is good for now, but we may see command other than TODO_MERGE\n> learn how to handle a merge commit, and when that happens, I wonder\n> what we want to do here. One thought is to do this:\n> \n> \tif (is_rebase_i(opts) && is_merge_commit(item->commit))\n> \treturn error_merge_commit(item);\n> \n> and teach error_merge_commit() to silently return 0 on TODO_MERGE.\n> Other commands, when they learn how to deal with a merge commit,\n> will then update their entries in error_merge_commit().\nf\nIt feels funny to call error_merge_commit() for a command that we know \nsupports merges but I can see that would make it easier to extend in the \nfuture.\n\n> Would we want to customize the message from error_merge_commit() to\n> make it closer to cut-and-paste ready? For that, something like\n> \n> \tint error_merge_commit(struct todo_item *item)\n> \t{\n> \t\tswitch (item->command) {\n> \t\tcase TODO_PICK:\n> \t\t\treturn error(_(\"'%s'\" is bad, plase use \"\n> \t\t\t\t '%s %s'\"),\n> \t\t\t\ttodo_command_info[item->command].str,\n> \t\t\t\t\"merge -C\",\n> \t\t\t\toid_to_hex(item->commit->oid));\n> \t\t...\n> \t\t}\n> \t}\n> \n> might go in a more friendly way.\n\nIf we want something the user can cut and paste then we would need to \nsuggest a second parent for the merge. We cannot just use the existing \nparent as it may be rebased which means tracking all the commits we see \nwhile parsing the list. If the second parent is rebased we need to make \nsure the user labels it so we can use that label here. For that reason \n(and the fact that I don't think can we really tell what the user was \nhoping to achieve) I think the best we can do is point the user in the \ndirection of the \"merge\" command even though that leaves them to figure \nout what to do with that command.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"492645","messageId":"xmqqpluy1f9e.fsf@gitster.g","threadId":"61008","inReplyTo":"1b74f6f9-f4b9-4909-82b3-26f19b7a1347@gmail.com","subject":"Re: [PATCH v2 2/2] rebase -i: improve error message when picking merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-09T19:56:45Z","receivedAt":"2024-04-09T19:56:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>>> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n>>> +\t\t\t \"please use '%s' followed by '%s'\"),\n>>> +\t\t\t todo_command_info[command].str,\n>>> +\t\t\t \"merge -C\", \"break\");\n>> OK. And when hitting the \"break\", they know that they are supposed\n>> to say \"git commit --amend\" and then \"git rebase --continue\"?\n>\n> Yes. I guess we could add a hint to that effect if you think its worth it.\n\nAs I said elsewhere, I do not deal with non-linear stuff using\nsequencer, so I _may_ be missing something obvious to the target\naudience of this message. But if I were dipping my toes to try\nmucking with sequencer and edit the todo myself by inserting a random\nmerge commit there and got this message, I would have probably\nappreciated if the message were a bit more explicit _why_ I would\nwant to use the 'break' there. Otherwise, I probably would be lost\nsitting in front of the shell command prompt. If it were\n\n\t'pick' does not take a merge commit. If you wanted to\n\treplay the merge, use 'merge -C' on the commit, and then\n\t'break' to give the control back to you so that you can\n\tdo 'git commit --amend && git rebase --continue'.\n\nthat would have given me 70% of what I needed (the other 30% is why\nI would want to \"--amend\", instead of just taking the result of\n'merge -C' as-is, though).\n\nWe can formulate the message in such a way that a succinct first\npart is sufficient for people who know the command, while the latter\nmore verbose and prescriptive part can be hidden behind the advice\nmechanism.\n\n> It feels funny to call error_merge_commit() for a command that we know\n> supports merges but I can see that would make it easier to extend in\n> the future.\n\nYes, I think that it is just a sign that the function is misnamed.\n\"Gee we have a merge commit, please tell me what you want to do with\nit\" is what the caller is asking, and the error messages are side\neffects the caller does not have to care too much about.\n"},{"id":"492869","messageId":"88bc0787-e7ae-49e5-99e8-97f6c55ea8c6@gmail.com","threadId":"61008","inReplyTo":"xmqqpluy1f9e.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] rebase -i: improve error message when picking merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-12T13:24:39Z","receivedAt":"2024-04-12T13:24:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 09/04/2024 20:56, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>>>> +\t\treturn error(_(\"'%s' does not accept merge commits, \"\n>>>> +\t\t\t \"please use '%s' followed by '%s'\"),\n>>>> +\t\t\t todo_command_info[command].str,\n>>>> +\t\t\t \"merge -C\", \"break\");\n>>> OK. And when hitting the \"break\", they know that they are supposed\n>>> to say \"git commit --amend\" and then \"git rebase --continue\"?\n>>\n>> Yes. I guess we could add a hint to that effect if you think its worth it.\n> \n> As I said elsewhere, I do not deal with non-linear stuff using\n> sequencer, so I _may_ be missing something obvious to the target\n> audience of this message. But if I were dipping my toes to try\n> mucking with sequencer and edit the todo myself by inserting a random\n> merge commit there and got this message, I would have probably\n> appreciated if the message were a bit more explicit _why_ I would\n> want to use the 'break' there. Otherwise, I probably would be lost\n> sitting in front of the shell command prompt. If it were\n> \n> \t'pick' does not take a merge commit. If you wanted to\n> \treplay the merge, use 'merge -C' on the commit, and then\n> \t'break' to give the control back to you so that you can\n> \tdo 'git commit --amend && git rebase --continue'.\n> \n> that would have given me 70% of what I needed (the other 30% is why\n> I would want to \"--amend\", instead of just taking the result of\n> 'merge -C' as-is, though).\n> \n> We can formulate the message in such a way that a succinct first\n> part is sufficient for people who know the command, while the latter\n> more verbose and prescriptive part can be hidden behind the advice\n> mechanism.\n\nI'll re-roll with a short error message plus some advice. I'll have a \nthink about adding advice for the other commands as well explaining how \nthe \"merge\" command should be used as well.\n\n>> It feels funny to call error_merge_commit() for a command that we know\n>> supports merges but I can see that would make it easier to extend in\n>> the future.\n> \n> Yes, I think that it is just a sign that the function is misnamed.\n> \"Gee we have a merge commit, please tell me what you want to do with\n> it\" is what the caller is asking, and the error messages are side\n> effects the caller does not have to care too much about.\n\nSure\n\nThanks\n\nPhillip\n"},{"id":"495955","messageId":"pull.1672.v3.git.1717076630.gitgitgadget@gmail.com","threadId":"61008","inReplyTo":"pull.1672.v2.git.1712585787.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] rebase -i: improve error message when picking merge","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-30T13:43:48Z","receivedAt":"2024-05-30T13:43:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"If the user tries to pick a merge commit error out when parsing the todo\nlist rather than complaining when trying to pick the commit.\n\nSorry for the delay in re-rolling, thanks to Junio and Patrick for their\ncomments on V2. I've rebased on to master to avoid a conflict with\n'ps/the-index-is-no-more' and updated patch 2 to\n\n * Add advice on how rebase a merge commit as suggested by Junio. To avoid\n duplication between the error messages and the advice I've shortened the\n error messages.\n\n * Rework the control flow to make it easier to extend checks on merge\n commits if new commands are added in the future as suggested by Junio\n\nPhillip Wood (2):\n rebase -i: pass struct replay_opts to parse_insn_line()\n rebase -i: improve error message when picking merge\n\n Documentation/config/advice.txt | 2 +\n advice.c | 1 +\n advice.h | 1 +\n builtin/rebase.c | 17 ++++---\n rebase-interactive.c | 21 +++++----\n rebase-interactive.h | 9 ++--\n sequencer.c | 83 ++++++++++++++++++++++++++++-----\n sequencer.h | 4 +-\n t/t3404-rebase-interactive.sh | 45 ++++++++++++++++++\n 9 files changed, 153 insertions(+), 30 deletions(-)\n\n\nbase-commit: 3a57aa566a21e7a510c64881bc6bdff7eb397988\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1672\n\nRange-diff vs v2:\n\n 1: 1bcf92c6105 ! 1: 91c6f2f1b45 rebase -i: pass struct replay_opts to parse_insn_line()\n @@ builtin/rebase.c: static int edit_todo_file(unsigned flags)\n @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \t\terror(_(\"could not generate todo list\"));\n \telse {\n - \t\tdiscard_index(&the_index);\n + \t\tdiscard_index(the_repository->index);\n -\t\tif (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n -\t\t\t\t\t\t&todo_list))\n +\t\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n 2: fbc6746e018 ! 2: 86052416b22 rebase -i: improve error message when picking merge\n @@ Commit message\n pass \"-m\". For users who are rebasing the message is confusing as there\n is no way for rebase to cherry-pick the merge.\n \n - Improve the user experience by detecting the error when the todo list is\n - parsed rather than waiting for the \"pick\" command to fail and print a\n - message recommending the \"merge\" command instead. We recommend \"merge\"\n - rather than \"exec git cherry-pick -m ...\" on the assumption that\n - cherry-picking merges is relatively rare and it is more likely that the\n - user chose \"pick\" by a mistake.\n + Improve the user experience by detecting the error and printing some\n + advice on how to fix it when the todo list is parsed rather than waiting\n + for the \"pick\" command to fail. The advice recommends \"merge\" rather\n + than \"exec git cherry-pick -m ...\" on the assumption that cherry-picking\n + merges is relatively rare and it is more likely that the user chose\n + \"pick\" by a mistake.\n \n It would be possible to support cherry-picking merges by allowing the\n user to pass \"-m\" to \"pick\" commands but that adds complexity to do\n @@ Commit message\n Reported-by: Stefan Haller \n Signed-off-by: Phillip Wood \n \n + ## Documentation/config/advice.txt ##\n +@@ Documentation/config/advice.txt: advice.*::\n + \t\t`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,\n + \t\t`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`\n + \t\tsimultaneously.\n ++\trebaseTodoError::\n ++\t\tShown when there is an error after editing the rebase todo list.\n + \trefSyntax::\n + \t\tShown when the user provides an illegal ref name, to\n + \t\ttell the user about the ref syntax documentation.\n +\n + ## advice.c ##\n +@@ advice.c: static struct {\n + \t[ADVICE_PUSH_UNQUALIFIED_REF_NAME]\t\t= { \"pushUnqualifiedRefName\" },\n + \t[ADVICE_PUSH_UPDATE_REJECTED]\t\t\t= { \"pushUpdateRejected\" },\n + \t[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]\t\t= { \"pushNonFastForward\" }, /* backwards compatibility */\n ++\t[ADVICE_REBASE_TODO_ERROR]\t\t\t= { \"rebaseTodoError\" },\n + \t[ADVICE_REF_SYNTAX]\t\t\t\t= { \"refSyntax\" },\n + \t[ADVICE_RESET_NO_REFRESH_WARNING]\t\t= { \"resetNoRefresh\" },\n + \t[ADVICE_RESOLVE_CONFLICT]\t\t\t= { \"resolveConflict\" },\n +\n + ## advice.h ##\n +@@ advice.h: enum advice_type {\n + \tADVICE_PUSH_UNQUALIFIED_REF_NAME,\n + \tADVICE_PUSH_UPDATE_REJECTED,\n + \tADVICE_PUSH_UPDATE_REJECTED_ALIAS,\n ++\tADVICE_REBASE_TODO_ERROR,\n + \tADVICE_REF_SYNTAX,\n + \tADVICE_RESET_NO_REFRESH_WARNING,\n + \tADVICE_RESOLVE_CONFLICT,\n +\n ## sequencer.c ##\n @@ sequencer.c: static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n \treturn 0;\n }\n \n -static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,\n -+static int error_merge_commit(enum todo_command command)\n ++static int check_merge_commit_insn(enum todo_command command)\n +{\n +\tswitch(command) {\n +\tcase TODO_PICK:\n -+\t\treturn error(_(\"'%s' does not accept merge commits, \"\n -+\t\t\t \"please use '%s'\"),\n -+\t\t\t todo_command_info[command].str, \"merge -C\");\n ++\t\terror(_(\"'%s' does not accept merge commits\"),\n ++\t\t todo_command_info[command].str);\n ++\t\tadvise_if_enabled(ADVICE_REBASE_TODO_ERROR, _(\n ++\t\t\t/*\n ++\t\t\t * TRANSLATORS: 'pick' and 'merge -C' should not be\n ++\t\t\t * translated.\n ++\t\t\t */\n ++\t\t\t\"'pick' does not take a merge commit. If you wanted to\\n\"\n ++\t\t\t\"replay the merge, use 'merge -C' on the commit.\"));\n ++\t\treturn -1;\n +\n +\tcase TODO_REWORD:\n -+\t\treturn error(_(\"'%s' does not accept merge commits, \"\n -+\t\t\t \"please use '%s'\"),\n -+\t\t\t todo_command_info[command].str, \"merge -c\");\n ++\t\terror(_(\"'%s' does not accept merge commits\"),\n ++\t\t todo_command_info[command].str);\n ++\t\tadvise_if_enabled(ADVICE_REBASE_TODO_ERROR, _(\n ++\t\t\t/*\n ++\t\t\t * TRANSLATORS: 'reword' and 'merge -c' should not be\n ++\t\t\t * translated.\n ++\t\t\t */\n ++\t\t\t\"'reword' does not take a merge commit. If you wanted to\\n\"\n ++\t\t\t\"replay the merge and reword the commit message, use\\n\"\n ++\t\t\t\"'merge -c' on the commit\"));\n ++\t\treturn -1;\n +\n +\tcase TODO_EDIT:\n -+\t\treturn error(_(\"'%s' does not accept merge commits, \"\n -+\t\t\t \"please use '%s' followed by '%s'\"),\n -+\t\t\t todo_command_info[command].str,\n -+\t\t\t \"merge -C\", \"break\");\n ++\t\terror(_(\"'%s' does not accept merge commits\"),\n ++\t\t todo_command_info[command].str);\n ++\t\tadvise_if_enabled(ADVICE_REBASE_TODO_ERROR, _(\n ++\t\t\t/*\n ++\t\t\t * TRANSLATORS: 'edit', 'merge -C' and 'break' should\n ++\t\t\t * not be translated.\n ++\t\t\t */\n ++\t\t\t\"'edit' does not take a merge commit. If you wanted to\\n\"\n ++\t\t\t\"replay the merge, use 'merge -C' on the commit, and then\\n\"\n ++\t\t\t\"'break' to give the control back to you so that you can\\n\"\n ++\t\t\t\"do 'git commit --amend && git rebase --continue'.\"));\n ++\t\treturn -1;\n +\n +\tcase TODO_FIXUP:\n +\tcase TODO_SQUASH:\n +\t\treturn error(_(\"cannot squash merge commit into another commit\"));\n +\n ++\tcase TODO_MERGE:\n ++\t\treturn 0;\n ++\n +\tdefault:\n +\t\tBUG(\"unexpected todo_command\");\n +\t}\n @@ sequencer.c: static int parse_insn_line(struct repository *r, struct replay_opts\n -\treturn item->commit ? 0 : -1;\n +\tif (!item->commit)\n +\t\treturn -1;\n -+\tif (is_rebase_i(opts) && item->command != TODO_MERGE &&\n ++\tif (is_rebase_i(opts) &&\n +\t item->commit->parents && item->commit->parents->next)\n -+\t\treturn error_merge_commit(item->command);\n ++\t\treturn check_merge_commit_insn(item->command);\n +\treturn 0;\n }\n \n @@ t/t3404-rebase-interactive.sh: test_expect_success 'bad labels and refs rejected\n +\t\ttest_must_fail git rebase -i HEAD 2>actual\n +\t) &&\n +\tcat >expect <<-EOF &&\n -+\terror: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ}\n ++\terror: ${SQ}pick${SQ} does not accept merge commits\n ++\thint: ${SQ}pick${SQ} does not take a merge commit. If you wanted to\n ++\thint: replay the merge, use ${SQ}merge -C${SQ} on the commit.\n ++\thint: Disable this message with \"git config advice.rebaseTodoError false\"\n +\terror: invalid line 1: pick $oid\n -+\terror: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ}\n ++\terror: ${SQ}reword${SQ} does not accept merge commits\n ++\thint: ${SQ}reword${SQ} does not take a merge commit. If you wanted to\n ++\thint: replay the merge and reword the commit message, use\n ++\thint: ${SQ}merge -c${SQ} on the commit\n ++\thint: Disable this message with \"git config advice.rebaseTodoError false\"\n +\terror: invalid line 2: reword $oid\n -+\terror: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}\n ++\terror: ${SQ}edit${SQ} does not accept merge commits\n ++\thint: ${SQ}edit${SQ} does not take a merge commit. If you wanted to\n ++\thint: replay the merge, use ${SQ}merge -C${SQ} on the commit, and then\n ++\thint: ${SQ}break${SQ} to give the control back to you so that you can\n ++\thint: do ${SQ}git commit --amend && git rebase --continue${SQ}.\n ++\thint: Disable this message with \"git config advice.rebaseTodoError false\"\n +\terror: invalid line 3: edit $oid\n +\terror: cannot squash merge commit into another commit\n +\terror: invalid line 4: fixup $oid\n\n-- \ngitgitgadget\n"},{"id":"495956","messageId":"91c6f2f1b458fca8db19483cd43229b737214d3d.1717076630.git.gitgitgadget@gmail.com","threadId":"61008","inReplyTo":"pull.1672.v3.git.1717076630.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] rebase -i: pass struct replay_opts to parse_insn_line()","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-30T13:43:49Z","receivedAt":"2024-05-30T13:43:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThis new parameter will be used in the next commit. As adding the\nparameter requires quite a few changes to plumb it through the call\nchain these are separated into their own commit to avoid cluttering up\nthe next commit with incidental changes.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 17 +++++++++++------\n rebase-interactive.c | 21 +++++++++++++--------\n rebase-interactive.h | 9 ++++++---\n sequencer.c | 22 ++++++++++++----------\n sequencer.h | 4 ++--\n 5 files changed, 44 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0466d9414af..9fb0a6888d6 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -193,7 +193,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \treturn replay;\n }\n \n-static int edit_todo_file(unsigned flags)\n+static int edit_todo_file(unsigned flags, struct replay_opts *opts)\n {\n \tconst char *todo_file = rebase_path_todo();\n \tstruct todo_list todo_list = TODO_LIST_INIT,\n@@ -204,7 +204,8 @@ 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, comment_line_str);\n-\tres = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags);\n+\tres = edit_todo_list(the_repository, opts, &todo_list, &new_todo,\n+\t\t\t NULL, NULL, flags);\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\tres = error_errno(_(\"could not write '%s'\"), todo_file);\n@@ -295,8 +296,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \t\terror(_(\"could not generate todo list\"));\n \telse {\n \t\tdiscard_index(the_repository->index);\n-\t\tif (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n-\t\t\t\t\t\t&todo_list))\n+\t\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n+\t\t\t\t\t\ttodo_list.buf.buf, &todo_list))\n \t\t\tBUG(\"unusable todo list\");\n \n \t\tret = complete_action(the_repository, &replay, flags,\n@@ -351,9 +352,13 @@ static int run_sequencer_rebase(struct rebase_options *opts)\n \t\treplay_opts_release(&replay_opts);\n \t\tbreak;\n \t}\n-\tcase ACTION_EDIT_TODO:\n-\t\tret = edit_todo_file(flags);\n+\tcase ACTION_EDIT_TODO: {\n+\t\tstruct replay_opts replay_opts = get_replay_opts(opts);\n+\n+\t\tret = edit_todo_file(flags, &replay_opts);\n+\t\treplay_opts_release(&replay_opts);\n \t\tbreak;\n+\t}\n \tcase ACTION_SHOW_CURRENT_PATCH: {\n \t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex c343e16fcdd..56fd7206a95 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -101,9 +101,10 @@ void append_todo_help(int command_count,\n \tstrbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_str);\n }\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+int edit_todo_list(struct repository *r, struct replay_opts *opts,\n+\t\t struct todo_list *todo_list, struct todo_list *new_todo,\n+\t\t const char *shortrevisions, const char *shortonto,\n+\t\t unsigned flags)\n {\n \tconst char *todo_file = rebase_path_todo(),\n \t\t*todo_backup = rebase_path_todo_backup();\n@@ -114,7 +115,9 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\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-\t\tincorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |\n+\t\tincorrect = todo_list_parse_insn_buffer(r, opts,\n+\t\t\t\t\t\t\ttodo_list->buf.buf,\n+\t\t\t\t\t\t\ttodo_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@@ -134,13 +137,13 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n \tif (initial && 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+\tif (todo_list_parse_insn_buffer(r, opts, new_todo->buf.buf, new_todo)) {\n \t\tfprintf(stderr, _(edit_todo_list_advice));\n \t\treturn -4;\n \t}\n \n \tif (incorrect) {\n-\t\tif (todo_list_check_against_backup(r, new_todo)) {\n+\t\tif (todo_list_check_against_backup(r, opts, new_todo)) {\n \t\t\twrite_file(rebase_path_dropped(), \"%s\", \"\");\n \t\t\treturn -4;\n \t\t}\n@@ -228,13 +231,15 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)\n \treturn res;\n }\n \n-int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_list)\n+int todo_list_check_against_backup(struct repository *r,\n+\t\t\t\t struct replay_opts *opts,\n+\t\t\t\t struct todo_list *todo_list)\n {\n \tstruct todo_list backup = TODO_LIST_INIT;\n \tint res = 0;\n \n \tif (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {\n-\t\ttodo_list_parse_insn_buffer(r, backup.buf.buf, &backup);\n+\t\ttodo_list_parse_insn_buffer(r, opts, backup.buf.buf, &backup);\n \t\tres = todo_list_check(&backup, todo_list);\n \t}\n \ndiff --git a/rebase-interactive.h b/rebase-interactive.h\nindex 7239c60f791..8e5b181b334 100644\n--- a/rebase-interactive.h\n+++ b/rebase-interactive.h\n@@ -3,17 +3,20 @@\n \n struct strbuf;\n struct repository;\n+struct replay_opts;\n struct todo_list;\n \n void append_todo_help(int command_count,\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+int edit_todo_list(struct repository *r, struct replay_opts *opts,\n+\t\t struct todo_list *todo_list, struct todo_list *new_todo,\n+\t\t const char *shortrevisions, const char *shortonto,\n+\t\t unsigned flags);\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+\t\t\t\t struct replay_opts *opts,\n \t\t\t\t struct todo_list *todo_list);\n \n #endif\ndiff --git a/sequencer.c b/sequencer.c\nindex aa2a2398357..7ab465da14a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2626,8 +2626,9 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n \treturn 0;\n }\n \n-static int parse_insn_line(struct repository *r, struct todo_item *item,\n-\t\t\t const char *buf, const char *bol, char *eol)\n+static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,\n+\t\t\t struct todo_item *item, const char *buf,\n+\t\t\t const char *bol, char *eol)\n {\n \tstruct object_id commit_oid;\n \tchar *end_of_object_name;\n@@ -2761,8 +2762,8 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *\n \treturn ret;\n }\n \n-int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n-\t\t\t\tstruct todo_list *todo_list)\n+int todo_list_parse_insn_buffer(struct repository *r, struct replay_opts *opts,\n+\t\t\t\tchar *buf, struct todo_list *todo_list)\n {\n \tstruct todo_item *item;\n \tchar *p = buf, *next_p;\n@@ -2780,7 +2781,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n \n \t\titem = append_new_todo(todo_list);\n \t\titem->offset_in_buf = p - todo_list->buf.buf;\n-\t\tif (parse_insn_line(r, item, buf, p, eol)) {\n+\t\tif (parse_insn_line(r, opts, item, buf, p, eol)) {\n \t\t\tres = error(_(\"invalid line %d: %.*s\"),\n \t\t\t\ti, (int)(eol - p), p);\n \t\t\titem->command = TODO_COMMENT + 1;\n@@ -2930,7 +2931,7 @@ static int read_populate_todo(struct repository *r,\n \tif (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)\n \t\treturn -1;\n \n-\tres = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);\n+\tres = todo_list_parse_insn_buffer(r, opts, todo_list->buf.buf, todo_list);\n \tif (res) {\n \t\tif (is_rebase_i(opts))\n \t\t\treturn error(_(\"please fix this using \"\n@@ -2960,7 +2961,7 @@ static int read_populate_todo(struct repository *r,\n \t\tstruct todo_list done = TODO_LIST_INIT;\n \n \t\tif (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&\n-\t\t !todo_list_parse_insn_buffer(r, done.buf.buf, &done))\n+\t\t !todo_list_parse_insn_buffer(r, opts, done.buf.buf, &done))\n \t\t\ttodo_list->done_nr = count_commands(&done);\n \t\telse\n \t\t\ttodo_list->done_nr = 0;\n@@ -5319,7 +5320,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \t\t\tgoto release_todo_list;\n \n \t\tif (file_exists(rebase_path_dropped())) {\n-\t\t\tif ((res = todo_list_check_against_backup(r, &todo_list)))\n+\t\t\tif ((res = todo_list_check_against_backup(r, opts,\n+\t\t\t\t\t\t\t\t &todo_list)))\n \t\t\t\tgoto release_todo_list;\n \n \t\t\tunlink(rebase_path_dropped());\n@@ -6363,7 +6365,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn error(_(\"nothing to do\"));\n \t}\n \n-\tres = edit_todo_list(r, todo_list, &new_todo, shortrevisions,\n+\tres = edit_todo_list(r, opts, todo_list, &new_todo, shortrevisions,\n \t\t\t shortonto, flags);\n \tif (res == -1)\n \t\treturn -1;\n@@ -6391,7 +6393,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \tstrbuf_release(&buf2);\n \t/* Nothing is done yet, and we're reparsing, so let's reset the count */\n \tnew_todo.total_nr = 0;\n-\tif (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)\n+\tif (todo_list_parse_insn_buffer(r, opts, 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 \ndiff --git a/sequencer.h b/sequencer.h\nindex a309ddd712b..304ba4b4d35 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -136,8 +136,8 @@ struct todo_list {\n \t.buf = STRBUF_INIT, \\\n }\n \n-int todo_list_parse_insn_buffer(struct repository *r, char *buf,\n-\t\t\t\tstruct todo_list *todo_list);\n+int todo_list_parse_insn_buffer(struct repository *r, struct replay_opts *opts,\n+\t\t\t\tchar *buf, struct 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-- \ngitgitgadget\n\n"},{"id":"495957","messageId":"86052416b22143a5c71ad011ca8d6e4cf80bcc50.1717076630.git.gitgitgadget@gmail.com","threadId":"61008","inReplyTo":"pull.1672.v3.git.1717076630.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] rebase -i: improve error message when picking merge","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-30T13:43:50Z","receivedAt":"2024-05-30T13:43:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe only todo commands that accept a merge commit are \"merge\" and\n\"reset\". All the other commands like \"pick\" or \"reword\" fail when they\ntry to pick a a merge commit and print the message\n\n error: commit abc123 is a merge but no -m option was given.\n\nfollowed by a hint about the command being rescheduled. This message is\ndesigned to help the user when they cherry-pick a merge and forget to\npass \"-m\". For users who are rebasing the message is confusing as there\nis no way for rebase to cherry-pick the merge.\n\nImprove the user experience by detecting the error and printing some\nadvice on how to fix it when the todo list is parsed rather than waiting\nfor the \"pick\" command to fail. The advice recommends \"merge\" rather\nthan \"exec git cherry-pick -m ...\" on the assumption that cherry-picking\nmerges is relatively rare and it is more likely that the user chose\n\"pick\" by a mistake.\n\nIt would be possible to support cherry-picking merges by allowing the\nuser to pass \"-m\" to \"pick\" commands but that adds complexity to do\nsomething that can already be achieved with\n\n exec git cherry-pick -m1 abc123\n\nReported-by: Stefan Haller \nSigned-off-by: Phillip Wood \n---\n Documentation/config/advice.txt | 2 ++\n advice.c | 1 +\n advice.h | 1 +\n sequencer.c | 63 +++++++++++++++++++++++++++++++--\n t/t3404-rebase-interactive.sh | 45 +++++++++++++++++++++++\n 5 files changed, 110 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex 0e35ae5240f..fa612417568 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -96,6 +96,8 @@ advice.*::\n \t\t`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,\n \t\t`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`\n \t\tsimultaneously.\n+\trebaseTodoError::\n+\t\tShown when there is an error after editing the rebase todo list.\n \trefSyntax::\n \t\tShown when the user provides an illegal ref name, to\n \t\ttell the user about the ref syntax documentation.\ndiff --git a/advice.c b/advice.c\nindex 0a122c2020b..558a46fc0b3 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -70,6 +70,7 @@ static struct {\n \t[ADVICE_PUSH_UNQUALIFIED_REF_NAME]\t\t= { \"pushUnqualifiedRefName\" },\n \t[ADVICE_PUSH_UPDATE_REJECTED]\t\t\t= { \"pushUpdateRejected\" },\n \t[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]\t\t= { \"pushNonFastForward\" }, /* backwards compatibility */\n+\t[ADVICE_REBASE_TODO_ERROR]\t\t\t= { \"rebaseTodoError\" },\n \t[ADVICE_REF_SYNTAX]\t\t\t\t= { \"refSyntax\" },\n \t[ADVICE_RESET_NO_REFRESH_WARNING]\t\t= { \"resetNoRefresh\" },\n \t[ADVICE_RESOLVE_CONFLICT]\t\t\t= { \"resolveConflict\" },\ndiff --git a/advice.h b/advice.h\nindex c8d29f97f39..5105d90129d 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -37,6 +37,7 @@ enum advice_type {\n \tADVICE_PUSH_UNQUALIFIED_REF_NAME,\n \tADVICE_PUSH_UPDATE_REJECTED,\n \tADVICE_PUSH_UPDATE_REJECTED_ALIAS,\n+\tADVICE_REBASE_TODO_ERROR,\n \tADVICE_REF_SYNTAX,\n \tADVICE_RESET_NO_REFRESH_WARNING,\n \tADVICE_RESOLVE_CONFLICT,\ndiff --git a/sequencer.c b/sequencer.c\nindex 7ab465da14a..ee209c46417 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2626,7 +2626,61 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n \treturn 0;\n }\n \n-static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,\n+static int check_merge_commit_insn(enum todo_command command)\n+{\n+\tswitch(command) {\n+\tcase TODO_PICK:\n+\t\terror(_(\"'%s' does not accept merge commits\"),\n+\t\t todo_command_info[command].str);\n+\t\tadvise_if_enabled(ADVICE_REBASE_TODO_ERROR, _(\n+\t\t\t/*\n+\t\t\t * TRANSLATORS: 'pick' and 'merge -C' should not be\n+\t\t\t * translated.\n+\t\t\t */\n+\t\t\t\"'pick' does not take a merge commit. If you wanted to\\n\"\n+\t\t\t\"replay the merge, use 'merge -C' on the commit.\"));\n+\t\treturn -1;\n+\n+\tcase TODO_REWORD:\n+\t\terror(_(\"'%s' does not accept merge commits\"),\n+\t\t todo_command_info[command].str);\n+\t\tadvise_if_enabled(ADVICE_REBASE_TODO_ERROR, _(\n+\t\t\t/*\n+\t\t\t * TRANSLATORS: 'reword' and 'merge -c' should not be\n+\t\t\t * translated.\n+\t\t\t */\n+\t\t\t\"'reword' does not take a merge commit. If you wanted to\\n\"\n+\t\t\t\"replay the merge and reword the commit message, use\\n\"\n+\t\t\t\"'merge -c' on the commit\"));\n+\t\treturn -1;\n+\n+\tcase TODO_EDIT:\n+\t\terror(_(\"'%s' does not accept merge commits\"),\n+\t\t todo_command_info[command].str);\n+\t\tadvise_if_enabled(ADVICE_REBASE_TODO_ERROR, _(\n+\t\t\t/*\n+\t\t\t * TRANSLATORS: 'edit', 'merge -C' and 'break' should\n+\t\t\t * not be translated.\n+\t\t\t */\n+\t\t\t\"'edit' does not take a merge commit. If you wanted to\\n\"\n+\t\t\t\"replay the merge, use 'merge -C' on the commit, and then\\n\"\n+\t\t\t\"'break' to give the control back to you so that you can\\n\"\n+\t\t\t\"do 'git commit --amend && git rebase --continue'.\"));\n+\t\treturn -1;\n+\n+\tcase TODO_FIXUP:\n+\tcase TODO_SQUASH:\n+\t\treturn error(_(\"cannot squash merge commit into another commit\"));\n+\n+\tcase TODO_MERGE:\n+\t\treturn 0;\n+\n+\tdefault:\n+\t\tBUG(\"unexpected todo_command\");\n+\t}\n+}\n+\n+static int parse_insn_line(struct repository *r, struct replay_opts *opts,\n \t\t\t struct todo_item *item, const char *buf,\n \t\t\t const char *bol, char *eol)\n {\n@@ -2732,7 +2786,12 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED\n \t\treturn status;\n \n \titem->commit = lookup_commit_reference(r, &commit_oid);\n-\treturn item->commit ? 0 : -1;\n+\tif (!item->commit)\n+\t\treturn -1;\n+\tif (is_rebase_i(opts) &&\n+\t item->commit->parents && item->commit->parents->next)\n+\t\treturn check_merge_commit_insn(item->command);\n+\treturn 0;\n }\n \n int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d1bead61fad..f92baad1381 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -2215,6 +2215,51 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' '\n \ttest_path_is_missing execed\n '\n \n+test_expect_success 'non-merge commands reject merge commits' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tgit checkout E &&\n+\tgit merge I &&\n+\toid=$(git rev-parse HEAD) &&\n+\tcat >todo <<-EOF &&\n+\tpick $oid\n+\treword $oid\n+\tedit $oid\n+\tfixup $oid\n+\tsquash $oid\n+\tEOF\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\ttest_must_fail git rebase -i HEAD 2>actual\n+\t) &&\n+\tcat >expect <<-EOF &&\n+\terror: ${SQ}pick${SQ} does not accept merge commits\n+\thint: ${SQ}pick${SQ} does not take a merge commit. If you wanted to\n+\thint: replay the merge, use ${SQ}merge -C${SQ} on the commit.\n+\thint: Disable this message with \"git config advice.rebaseTodoError false\"\n+\terror: invalid line 1: pick $oid\n+\terror: ${SQ}reword${SQ} does not accept merge commits\n+\thint: ${SQ}reword${SQ} does not take a merge commit. If you wanted to\n+\thint: replay the merge and reword the commit message, use\n+\thint: ${SQ}merge -c${SQ} on the commit\n+\thint: Disable this message with \"git config advice.rebaseTodoError false\"\n+\terror: invalid line 2: reword $oid\n+\terror: ${SQ}edit${SQ} does not accept merge commits\n+\thint: ${SQ}edit${SQ} does not take a merge commit. If you wanted to\n+\thint: replay the merge, use ${SQ}merge -C${SQ} on the commit, and then\n+\thint: ${SQ}break${SQ} to give the control back to you so that you can\n+\thint: do ${SQ}git commit --amend && git rebase --continue${SQ}.\n+\thint: Disable this message with \"git config advice.rebaseTodoError false\"\n+\terror: invalid line 3: edit $oid\n+\terror: cannot squash merge commit into another commit\n+\terror: invalid line 4: fixup $oid\n+\terror: cannot squash merge commit into another commit\n+\terror: invalid line 5: squash $oid\n+\tYou can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.\n+\tOr you can abort the rebase with ${SQ}git rebase --abort${SQ}.\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n # This must be the last test in this file\n test_expect_success '$EDITOR and friends are unchanged' '\n \ttest_editor_unchanged\n-- \ngitgitgadget\n"},{"id":"495966","messageId":"xmqq7cfbp6pb.fsf@gitster.g","threadId":"61008","inReplyTo":"pull.1672.v3.git.1717076630.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/2] rebase -i: improve error message when picking merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-30T17:09:36Z","receivedAt":"2024-05-30T17:09:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> If the user tries to pick a merge commit error out when parsing the todo\n> list rather than complaining when trying to pick the commit.\n>\n> Sorry for the delay in re-rolling, thanks to Junio and Patrick for their\n> comments on V2. I've rebased on to master to avoid a conflict with\n> 'ps/the-index-is-no-more' and updated patch 2 to\n>\n> * Add advice on how rebase a merge commit as suggested by Junio. To avoid\n> duplication between the error messages and the advice I've shortened the\n> error messages.\n>\n> * Rework the control flow to make it easier to extend checks on merge\n> commits if new commands are added in the future as suggested by Junio\n>\n> Phillip Wood (2):\n> rebase -i: pass struct replay_opts to parse_insn_line()\n> rebase -i: improve error message when picking merge\n>\n> Documentation/config/advice.txt | 2 +\n> advice.c | 1 +\n> advice.h | 1 +\n> builtin/rebase.c | 17 ++++---\n> rebase-interactive.c | 21 +++++----\n> rebase-interactive.h | 9 ++--\n> sequencer.c | 83 ++++++++++++++++++++++++++++-----\n> sequencer.h | 4 +-\n> t/t3404-rebase-interactive.sh | 45 ++++++++++++++++++\n> 9 files changed, 153 insertions(+), 30 deletions(-)\n>\n>\n> base-commit: 3a57aa566a21e7a510c64881bc6bdff7eb397988\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v3\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v3\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1672\n>\n> Range-diff vs v2:\n>\n> 1: 1bcf92c6105 ! 1: 91c6f2f1b45 rebase -i: pass struct replay_opts to parse_insn_line()\n> @@ builtin/rebase.c: static int edit_todo_file(unsigned flags)\n> @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n> \t\terror(_(\"could not generate todo list\"));\n> \telse {\n> - \t\tdiscard_index(&the_index);\n> + \t\tdiscard_index(the_repository->index);\n> -\t\tif (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n> -\t\t\t\t\t\t&todo_list))\n> +\t\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n\nOK. It would probably have been unnecessary to rebase only for this\nupdate.\n\n> + ## Documentation/config/advice.txt ##\n> +@@ Documentation/config/advice.txt: advice.*::\n> + \t\t`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,\n> + \t\t`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`\n> + \t\tsimultaneously.\n> ++\trebaseTodoError::\n> ++\t\tShown when there is an error after editing the rebase todo list.\n\nThis thing is new. It is unclear to me if this description is clear\nenough to readers that we are checking the edited todo list for\nerrors. It is clear enough from the actual code change, and the\nreaders come to this list after advise_if_enabled() triggers and\nreports that the rebaseTodoError knob allows them to squelch it, so\nit probably is OK.\n\nThanks, will replace. Let's see if we see comments from others and\nthen mark it for 'next' soonish.\n\n"},{"id":"496068","messageId":"28796edf-65eb-4f28-8651-af97e00cabc9@gmail.com","threadId":"61008","inReplyTo":"xmqq7cfbp6pb.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] rebase -i: improve error message when picking merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-03T09:22:32Z","receivedAt":"2024-06-03T09:22:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 30/05/2024 18:09, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n>> 1: 1bcf92c6105 ! 1: 91c6f2f1b45 rebase -i: pass struct replay_opts to parse_insn_line()\n>> @@ builtin/rebase.c: static int edit_todo_file(unsigned flags)\n>> @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n>> \t\terror(_(\"could not generate todo list\"));\n>> \telse {\n>> - \t\tdiscard_index(&the_index);\n>> + \t\tdiscard_index(the_repository->index);\n>> -\t\tif (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,\n>> -\t\t\t\t\t\t&todo_list))\n>> +\t\tif (todo_list_parse_insn_buffer(the_repository, &replay,\n> \n> OK. It would probably have been unnecessary to rebase only for this\n> update.\n\nI agree the conflict is easy to solve but GitGitGadget gets upset and \nrefuses to run the ci if it cannot merge the branch into master\n\n>> + ## Documentation/config/advice.txt ##\n>> +@@ Documentation/config/advice.txt: advice.*::\n>> + \t\t`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,\n>> + \t\t`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`\n>> + \t\tsimultaneously.\n>> ++\trebaseTodoError::\n>> ++\t\tShown when there is an error after editing the rebase todo list.\n> \n> This thing is new. It is unclear to me if this description is clear\n> enough to readers that we are checking the edited todo list for\n> errors.\n\nI'm happy to improve it if you have any suggestions - I had hoped \"after \nediting\" would be clear enough.\n\n> It is clear enough from the actual code change, and the\n> readers come to this list after advise_if_enabled() triggers and\n> reports that the rebaseTodoError knob allows them to squelch it, so\n> it probably is OK.\n> \n> Thanks, will replace. Let's see if we see comments from others and\n> then mark it for 'next' soonish.\n\nThanks\n\nPhillip\n"},{"id":"496156","messageId":"xmqqh6ea6nj5.fsf@gitster.g","threadId":"61008","inReplyTo":"28796edf-65eb-4f28-8651-af97e00cabc9@gmail.com","subject":"Re: [PATCH v3 0/2] rebase -i: improve error message when picking merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-03T15:42:22Z","receivedAt":"2024-06-03T15:42:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> OK. It would probably have been unnecessary to rebase only for this\n>> update.\n>\n> I agree the conflict is easy to solve but GitGitGadget gets upset and\n> refuses to run the ci if it cannot merge the branch into master\n\nHeh. TIL. That's a tad unfortunate that we have to let the tail\nwag the dog, but OK.\n"}]}