[PATCH v3 0/2] rebase -i: improve error message when picking merge

["Phillip Wood via GitGitGadget" <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