[PATCH v2 2/2] rebase -i: improve error message when picking merge
- From
- Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
- Date
- Apr 8, 2024, 14:16 UTC
- Message-ID
- <fbc6746e0188ed7b69c238935ec85b69112ddd79.1712585788.git.gitgitgadget@gmail.com>
- In-Reply-To
- <pull.1672.v2.git.1712585787.gitgitgadget@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
The only todo commands that accept a merge commit are "merge" and "reset". All the other commands like "pick" or "reword" fail when they try to pick a a merge commit and print the message
error: commit abc123 is a merge but no -m option was given.
followed by a hint about the command being rescheduled. This message is designed to help the user when they cherry-pick a merge and forget to pass "-m". For users who are rebasing the message is confusing as there is no way for rebase to cherry-pick the merge.
Improve the user experience by detecting the error when the todo list is parsed rather than waiting for the "pick" command to fail and print a message recommending the "merge" command instead. We recommend "merge" rather than "exec git cherry-pick -m ..." on the assumption that cherry-picking merges is relatively rare and it is more likely that the user chose "pick" by a mistake.
It would be possible to support cherry-picking merges by allowing the user to pass "-m" to "pick" commands but that adds complexity to do something that can already be achieved with
exec git cherry-pick -m1 abc123
Reported-by: Stefan Haller <lists@haller-berlin.de> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- sequencer.c | 37 +++++++++++++++++++++++++++++++++-- t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c index a3154ba3347..4012c6f88d9 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2573,7 +2573,35 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg) return 0; } -static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED, +static int error_merge_commit(enum todo_command command) +{ + switch(command) { + case TODO_PICK: + return error(_("'%s' does not accept merge commits, " + "please use '%s'"), + todo_command_info[command].str, "merge -C"); + + case TODO_REWORD: + return error(_("'%s' does not accept merge commits, " + "please use '%s'"), + todo_command_info[command].str, "merge -c"); + + case TODO_EDIT: + return error(_("'%s' does not accept merge commits, " + "please use '%s' followed by '%s'"), + todo_command_info[command].str, + "merge -C", "break"); + + case TODO_FIXUP: + case TODO_SQUASH: + return error(_("cannot squash merge commit into another commit")); + + default: + BUG("unexpected todo_command"); + } +} + +static int parse_insn_line(struct repository *r, struct replay_opts *opts, struct todo_item *item, const char *buf, const char *bol, char *eol) { @@ -2679,7 +2707,12 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED return status; item->commit = lookup_commit_reference(r, &commit_oid); - return item->commit ? 0 : -1; + if (!item->commit) + return -1; + if (is_rebase_i(opts) && item->command != TODO_MERGE && + item->commit->parents && item->commit->parents->next) + return error_merge_commit(item->command); + return 0; } int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index d1bead61fad..8565fd4b5ae 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -2215,6 +2215,39 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' ' test_path_is_missing execed ' +test_expect_success 'non-merge commands reject merge commits' ' + test_when_finished "test_might_fail git rebase --abort" && + git checkout E && + git merge I && + oid=$(git rev-parse HEAD) && + cat >todo <<-EOF && + pick $oid + reword $oid + edit $oid + fixup $oid + squash $oid + EOF + ( + set_replace_editor todo && + test_must_fail git rebase -i HEAD 2>actual + ) && + cat >expect <<-EOF && + error: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} + error: invalid line 1: pick $oid + error: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ} + error: invalid line 2: reword $oid + error: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ} + error: invalid line 3: edit $oid + error: cannot squash merge commit into another commit + error: invalid line 4: fixup $oid + error: cannot squash merge commit into another commit + error: invalid line 5: squash $oid + You can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}. + Or you can abort the rebase with ${SQ}git rebase --abort${SQ}. + EOF + test_cmp expect actual +' + # This must be the last test in this file test_expect_success '$EDITOR and friends are unchanged' ' test_editor_unchanged
-- gitgitgadget