rebase -i: improve error message when picking merge

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 and printing some
advice on how to fix it when the todo list is parsed rather than waiting
for the "pick" command to fail. The advice recommends "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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This commit is contained in:
Phillip Wood 2024-05-30 13:43:50 +00:00 committed by Junio C Hamano
parent 0c26738aa4
commit 4c063c82e9
5 changed files with 110 additions and 2 deletions

View file

@ -96,6 +96,8 @@ advice.*::
`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,
`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
simultaneously.
rebaseTodoError::
Shown when there is an error after editing the rebase todo list.
refSyntax::
Shown when the user provides an illegal ref name, to
tell the user about the ref syntax documentation.

View file

@ -69,6 +69,7 @@ static struct {
[ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName" },
[ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected" },
[ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward" }, /* backwards compatibility */
[ADVICE_REBASE_TODO_ERROR] = { "rebaseTodoError" },
[ADVICE_REF_SYNTAX] = { "refSyntax" },
[ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" },
[ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" },

View file

@ -37,6 +37,7 @@ enum advice_type {
ADVICE_PUSH_UNQUALIFIED_REF_NAME,
ADVICE_PUSH_UPDATE_REJECTED,
ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
ADVICE_REBASE_TODO_ERROR,
ADVICE_REF_SYNTAX,
ADVICE_RESET_NO_REFRESH_WARNING,
ADVICE_RESOLVE_CONFLICT,

View file

@ -2573,7 +2573,61 @@ 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 check_merge_commit_insn(enum todo_command command)
{
switch(command) {
case TODO_PICK:
error(_("'%s' does not accept merge commits"),
todo_command_info[command].str);
advise_if_enabled(ADVICE_REBASE_TODO_ERROR, _(
/*
* TRANSLATORS: 'pick' and 'merge -C' should not be
* translated.
*/
"'pick' does not take a merge commit. If you wanted to\n"
"replay the merge, use 'merge -C' on the commit."));
return -1;
case TODO_REWORD:
error(_("'%s' does not accept merge commits"),
todo_command_info[command].str);
advise_if_enabled(ADVICE_REBASE_TODO_ERROR, _(
/*
* TRANSLATORS: 'reword' and 'merge -c' should not be
* translated.
*/
"'reword' does not take a merge commit. If you wanted to\n"
"replay the merge and reword the commit message, use\n"
"'merge -c' on the commit"));
return -1;
case TODO_EDIT:
error(_("'%s' does not accept merge commits"),
todo_command_info[command].str);
advise_if_enabled(ADVICE_REBASE_TODO_ERROR, _(
/*
* TRANSLATORS: 'edit', 'merge -C' and 'break' should
* not be translated.
*/
"'edit' does not take a merge commit. If you wanted to\n"
"replay the merge, use 'merge -C' on the commit, and then\n"
"'break' to give the control back to you so that you can\n"
"do 'git commit --amend && git rebase --continue'."));
return -1;
case TODO_FIXUP:
case TODO_SQUASH:
return error(_("cannot squash merge commit into another commit"));
case TODO_MERGE:
return 0;
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 +2733,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->commit->parents && item->commit->parents->next)
return check_merge_commit_insn(item->command);
return 0;
}
int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)

View file

@ -2215,6 +2215,51 @@ 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
hint: ${SQ}pick${SQ} does not take a merge commit. If you wanted to
hint: replay the merge, use ${SQ}merge -C${SQ} on the commit.
hint: Disable this message with "git config advice.rebaseTodoError false"
error: invalid line 1: pick $oid
error: ${SQ}reword${SQ} does not accept merge commits
hint: ${SQ}reword${SQ} does not take a merge commit. If you wanted to
hint: replay the merge and reword the commit message, use
hint: ${SQ}merge -c${SQ} on the commit
hint: Disable this message with "git config advice.rebaseTodoError false"
error: invalid line 2: reword $oid
error: ${SQ}edit${SQ} does not accept merge commits
hint: ${SQ}edit${SQ} does not take a merge commit. If you wanted to
hint: replay the merge, use ${SQ}merge -C${SQ} on the commit, and then
hint: ${SQ}break${SQ} to give the control back to you so that you can
hint: do ${SQ}git commit --amend && git rebase --continue${SQ}.
hint: Disable this message with "git config advice.rebaseTodoError false"
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