[PATCH v3 0/2] rebase -i: improve error message when picking merge
- From
- Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
- Date
- May 30, 2024, 13:43 UTC
- Message-ID
- <pull.1672.v3.git.1717076630.gitgitgadget@gmail.com>
- In-Reply-To
- <pull.1672.v2.git.1712585787.gitgitgadget@gmail.com>
If the user tries to pick a merge commit error out when parsing the todo list rather than complaining when trying to pick the commit.
Sorry for the delay in re-rolling, thanks to Junio and Patrick for their comments on V2. I've rebased on to master to avoid a conflict with 'ps/the-index-is-no-more' and updated patch 2 to
* Add advice on how rebase a merge commit as suggested by Junio. To avoid duplication between the error messages and the advice I've shortened the error messages.
* Rework the control flow to make it easier to extend checks on merge commits if new commands are added in the future as suggested by Junio
Phillip Wood (2): rebase -i: pass struct replay_opts to parse_insn_line() rebase -i: improve error message when picking merge
Documentation/config/advice.txt | 2 + advice.c | 1 + advice.h | 1 + builtin/rebase.c | 17 ++++--- rebase-interactive.c | 21 +++++---- rebase-interactive.h | 9 ++-- sequencer.c | 83 ++++++++++++++++++++++++++++----- sequencer.h | 4 +- t/t3404-rebase-interactive.sh | 45 ++++++++++++++++++ 9 files changed, 153 insertions(+), 30 deletions(-)
base-commit: 3a57aa566a21e7a510c64881bc6bdff7eb397988 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/1672
Range-diff vs v2:
1: 1bcf92c6105 ! 1: 91c6f2f1b45 rebase -i: pass struct replay_opts to parse_insn_line()
@@ builtin/rebase.c: static int edit_todo_file(unsigned flags)
@@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
error(_("could not generate todo list"));
else {
- discard_index(&the_index);
+ discard_index(the_repository->index);
- if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
- &todo_list))
+ if (todo_list_parse_insn_buffer(the_repository, &replay,
2: fbc6746e018 ! 2: 86052416b22 rebase -i: improve error message when picking merge
@@ Commit message
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.
+ 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
@@ Commit message
Reported-by: Stefan Haller <lists@haller-berlin.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
+ ## Documentation/config/advice.txt ##
+@@ Documentation/config/advice.txt: 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.
+
+ ## advice.c ##
+@@ advice.c: 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" },
+
+ ## advice.h ##
+@@ advice.h: 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,
+
## sequencer.c ##
@@ sequencer.c: 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)
++static int check_merge_commit_insn(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");
++ 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:
-+ return error(_("'%s' does not accept merge commits, "
-+ "please use '%s'"),
-+ todo_command_info[command].str, "merge -c");
++ 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:
-+ return error(_("'%s' does not accept merge commits, "
-+ "please use '%s' followed by '%s'"),
-+ todo_command_info[command].str,
-+ "merge -C", "break");
++ 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");
+ }
@@ sequencer.c: static int parse_insn_line(struct repository *r, struct replay_opts
- return item->commit ? 0 : -1;
+ if (!item->commit)
+ return -1;
-+ if (is_rebase_i(opts) && item->command != TODO_MERGE &&
++ if (is_rebase_i(opts) &&
+ item->commit->parents && item->commit->parents->next)
-+ return error_merge_commit(item->command);
++ return check_merge_commit_insn(item->command);
+ return 0;
}
@@ t/t3404-rebase-interactive.sh: test_expect_success 'bad labels and refs rejected
+ 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: ${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, please use ${SQ}merge -c${SQ}
++ 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, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}
++ 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-- gitgitgadget