Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Phillip,

On Mon, 12 Nov 2018, Phillip Wood wrote:

> It's good to see these patches progressing, I've just got a couple of
> comments related to Johannes' points below
> 
> On 12/11/2018 16:21, Johannes Schindelin wrote:
> > Hi Elijah,
> > 
> > On Wed, 7 Nov 2018, Elijah Newren wrote:
> > 
> >> Interactive rebases are implemented in terms of cherry-pick rather than
> >> the merge-recursive builtin, but cherry-pick also calls into the recursive
> >> merge machinery by default and can accept special merge strategies and/or
> >> special strategy options.  As such, there really is not any need for
> >> having both git-rebase--merge and git-rebase--interactive anymore.
> >>
> >> Delete git-rebase--merge.sh and have the --merge option be implemented
> >> by the now built-in interactive machinery.
> > 
> > Okay.
> > 
> >> Note that this change fixes a few known test failures (see t3421).
> > 
> > Nice!
> > 
> >> testcase modification notes:
> >>   t3406: --interactive and --merge had slightly different progress output
> >>          while running; adjust a test to match
> >>   t3420: these test precise output while running, but rebase--am,
> >>          rebase--merge, and rebase--interactive all were built on very
> >>          different commands (am, merge-recursive, cherry-pick), so the
> >>          tests expected different output for each type.  Now we expect
> >>          --merge and --interactive to have the same output.
> >>   t3421: --interactive fixes some bugs in --merge!  Wahoo!
> >>   t3425: topology linearization was inconsistent across flavors of rebase,
> >>          as already noted in a TODO comment in the testcase.  This was not
> >>          considered a bug before, so getting a different linearization due
> >>          to switching out backends should not be considered a bug now.
> > 
> > Ideally, the test would be fixed, then. If it fails for other reasons than
> > a real regression, it is not a very good regression test, is it.
> > 
> >>   t5407: different rebase types varied slightly in how many times checkout
> >>          or commit or equivalents were called based on a quick comparison
> >>          of this tests and previous ones which covered different rebase
> >>          flavors.  I think this is just attributable to this difference.
> > 
> > This concerns me.
> > 
> > In bigger repositories (no, I am not talking about Linux kernel sized
> > ones, I consider those small-ish) there are a ton of files, and checkout
> > and commit (and even more so reset) sadly do not have a runtime complexity
> > growing with the number of modified files, but with the number of tracked
> > files (and some commands even with the number of worktree files).
> > 
> > So a larger number of commit/checkout operations makes me expect
> > performance regressions.
> > 
> > In this light, could you elaborate a bit on the differences you see
> > between rebase -i and rebase -m?
> > 
> >>   t9903: --merge uses the interactive backend so the prompt expected is
> >>          now REBASE-i.
> > 
> > We should be able to make that test pass, still, by writing out a special
> > file (e.g. $state_dir/opt_m) and testing for that. Users are oddly upset
> > when their expectations are broken... (and I actually agree with them.)
> > 
> >> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> >> index 3407d835bd..35084f5681 100644
> >> --- a/Documentation/git-rebase.txt
> >> +++ b/Documentation/git-rebase.txt
> >> @@ -504,15 +504,7 @@ See also INCOMPATIBLE OPTIONS below.
> >>  INCOMPATIBLE OPTIONS
> >>  --------------------
> >>  
> >> -git-rebase has many flags that are incompatible with each other,
> >> -predominantly due to the fact that it has three different underlying
> >> -implementations:
> >> -
> >> - * one based on linkgit:git-am[1] (the default)
> >> - * one based on git-merge-recursive (merge backend)
> >> - * one based on linkgit:git-cherry-pick[1] (interactive backend)
> >> -
> > 
> > Could we retain this part, with `s/three/two/` and `/git-merge/d`? *Maybe*
> > `s/interactive backend/interactive\/merge backend/`
> 
> I was hoping we could get rid of that, I'm not sure how useful it is to
> users.

That's a good point. If the commit message makes the case for that (it is
an implementation detail that confuses users), I am fine with removing it,
too.

Thanks,
Dscho

> 
> > 
> >> -Flags only understood by the am backend:
> >> +The following options:
> >>  
> >>   * --committer-date-is-author-date
> >>   * --ignore-date
> >> @@ -520,15 +512,12 @@ Flags only understood by the am backend:
> >>   * --ignore-whitespace
> >>   * -C
> >>  
> >> -Flags understood by both merge and interactive backends:
> >> +are incompatible with the following options:
> >>  
> >>   * --merge
> >>   * --strategy
> >>   * --strategy-option
> >>   * --allow-empty-message
> >> -
> >> -Flags only understood by the interactive backend:
> >> -
> 
> It's nice to see this being simplified
> 
> >>   * --[no-]autosquash
> >>   * --rebase-merges
> >>   * --preserve-merges
> >> @@ -539,7 +528,7 @@ Flags only understood by the interactive backend:
> >>   * --edit-todo
> >>   * --root when used in combination with --onto
> >>  
> >> -Other incompatible flag pairs:
> >> +In addition, the following pairs of options are incompatible:
> >>  
> >>   * --preserve-merges and --interactive
> >>   * --preserve-merges and --signoff
> > 
> > The rest of the diff is funny to read, but the post image makes a lot of
> > sense.
> > 
> >> diff --git a/builtin/rebase.c b/builtin/rebase.c
> >> index be004406a6..d55bbb9eb9 100644
> >> --- a/builtin/rebase.c
> >> +++ b/builtin/rebase.c
> >> @@ -118,7 +118,7 @@ static void imply_interactive(struct rebase_options *opts, const char *option)
> >>  	case REBASE_PRESERVE_MERGES:
> >>  		break;
> >>  	case REBASE_MERGE:
> >> -		/* we silently *upgrade* --merge to --interactive if needed */
> >> +		/* we now implement --merge via --interactive */
> >>  	default:
> >>  		opts->type = REBASE_INTERACTIVE; /* implied */
> >>  		break;
> >> @@ -475,10 +475,6 @@ static int run_specific_rebase(struct rebase_options *opts)
> >>  		backend = "git-rebase--am";
> >>  		backend_func = "git_rebase__am";
> >>  		break;
> >> -	case REBASE_MERGE:
> >> -		backend = "git-rebase--merge";
> >> -		backend_func = "git_rebase__merge";
> >> -		break;
> >>  	case REBASE_PRESERVE_MERGES:
> >>  		backend = "git-rebase--preserve-merges";
> >>  		backend_func = "git_rebase__preserve_merges";
> >> @@ -1156,6 +1152,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >>  		}
> >>  	}
> >>  
> >> +	if (options.type == REBASE_MERGE) {
> >> +		imply_interactive(&options, "--merge");
> >> +	}
> >> +
> >>  	if (options.root && !options.onto_name)
> >>  		imply_interactive(&options, "--root without --onto");
> >>  
> > 
> > Okay, this makes sense.
> > 
> >> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> >> index da27bfca5a..4abcceed06 100755
> >> --- a/git-legacy-rebase.sh
> >> +++ b/git-legacy-rebase.sh
> >> @@ -218,6 +218,7 @@ then
> >>  	state_dir="$apply_dir"
> >>  elif test -d "$merge_dir"
> >>  then
> >> +	type=interactive
> >>  	if test -d "$merge_dir"/rewritten
> >>  	then
> >>  		type=preserve-merges
> >> @@ -225,10 +226,7 @@ then
> >>  		preserve_merges=t
> >>  	elif test -f "$merge_dir"/interactive
> >>  	then
> >> -		type=interactive
> >>  		interactive_rebase=explicit
> >> -	else
> >> -		type=merge
> >>  	fi
> >>  	state_dir="$merge_dir"
> >>  fi
> >> @@ -469,6 +467,7 @@ then
> >>  	test -z "$interactive_rebase" && interactive_rebase=implied
> >>  fi
> >>  
> >> +actually_interactive=
> >>  if test -n "$interactive_rebase"
> >>  then
> >>  	if test -z "$preserve_merges"
> >> @@ -477,11 +476,12 @@ then
> >>  	else
> >>  		type=preserve-merges
> >>  	fi
> >> -
> >> +	actually_interactive=t
> >>  	state_dir="$merge_dir"
> >>  elif test -n "$do_merge"
> >>  then
> >> -	type=merge
> >> +	interactive_rebase=implied
> >> +	type=interactive
> >>  	state_dir="$merge_dir"
> >>  else
> >>  	type=am
> >> @@ -493,21 +493,13 @@ then
> >>  	git_format_patch_opt="$git_format_patch_opt --progress"
> >>  fi
> >>  
> >> -if test -n "$git_am_opt"; then
> >> -	incompatible_opts=$(echo " $git_am_opt " | \
> >> -			    sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
> >> -	if test -n "$interactive_rebase"
> >> +incompatible_opts=$(echo " $git_am_opt " | \
> >> +		    sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
> > 
> > Why are we no longer guarding this behind the condition that the user
> > specified *any* option intended for the `am` backend?
> 
> I was confused by this as well, what if the user asks for 'rebase
> --exec=<cmd> --ignore-whitespace'?
> 
> > 
> >> +if test -n "$incompatible_opts"
> >> +then
> >> +	if test -n "$actually_interactive" || test "$do_merge"
> >>  	then
> >> -		if test -n "$incompatible_opts"
> >> -		then
> >> -			die "$(gettext "error: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
> >> -		fi
> >> -	fi
> >> -	if test -n "$do_merge"; then
> >> -		if test -n "$incompatible_opts"
> >> -		then
> >> -			die "$(gettext "error: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
> >> -		fi
> >> +		die "$(gettext "error: cannot combine am options ($incompatible_opts) with either interactive or merge options")"
> >>  	fi
> >>  fi
> >>  
> 
> If you want to change the error message here, I think you need to change
> the corresponding message in builtin/rebase.c
> 
> Best Wishes
> 
> Phillip
> 
> > 
> > The rest of this hunk makes sense.
> > 
> >> @@ -672,7 +664,7 @@ require_clean_work_tree "rebase" "$(gettext "Please commit or stash them.")"
> >>  # but this should be done only when upstream and onto are the same
> >>  # and if this is not an interactive rebase.
> >>  mb=$(git merge-base "$onto" "$orig_head")
> >> -if test -z "$interactive_rebase" && test "$upstream" = "$onto" &&
> >> +if test -z "$actually_interactive" && test "$upstream" = "$onto" &&
> >>  	test "$mb" = "$onto" && test -z "$restrict_revision" &&
> >>  	# linear history?
> >>  	! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > /dev/null
> >> @@ -716,6 +708,19 @@ then
> >>  	GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
> >>  fi
> >>  
> >> +if test -z "$actually_interactive" && test "$mb" = "$orig_head"
> >> +then
> >> +	# If the $onto is a proper descendant of the tip of the branch, then
> >> +	# we just fast-forwarded.
> > 
> > This comment is misleading if it comes before, rather than after, the
> > actual `checkout -q` command.
> > 
> >> +	say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
> >> +	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
> >> +		git checkout -q "$onto^0" || die "could not detach HEAD"
> >> +	git update-ref ORIG_HEAD $orig_head
> >> +	move_to_original_branch
> >> +	finish_rebase
> >> +	exit 0
> >> +fi
> >> +
> >>  test -n "$interactive_rebase" && run_specific_rebase
> >>  
> >>  # Detach HEAD and reset the tree
> >> @@ -725,16 +730,6 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
> >>  	git checkout -q "$onto^0" || die "could not detach HEAD"
> >>  git update-ref ORIG_HEAD $orig_head
> > 
> > It is a pity that this hunk header hides the lines that you copied into
> > the following conditional before moving it before the "if interactive, run
> > it" line:
> > 
> >>  
> >> -# If the $onto is a proper descendant of the tip of the branch, then
> >> -# we just fast-forwarded.
> >> -if test "$mb" = "$orig_head"
> >> -then
> >> -	say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
> >> -	move_to_original_branch
> >> -	finish_rebase
> >> -	exit 0
> >> -fi
> >> -
> >>  if test -n "$rebase_root"
> >>  then
> >>  	revisions="$onto..$orig_head"
> > 
> > What you did is correct, if duplicating code, and definitely the easiest
> > way to do it. Just move the comment, and we're good here.
> > 
> >> diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
> >> deleted file mode 100644
> >> [... snip ...]
> > 
> > Nice. Really nice!
> > 
> >> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> >> index 0392e36d23..04d6c71899 100755
> >> --- a/t/t3406-rebase-message.sh
> >> +++ b/t/t3406-rebase-message.sh
> >> @@ -17,14 +17,9 @@ test_expect_success 'setup' '
> >>  	git tag start
> >>  '
> >>  
> >> -cat >expect <<\EOF
> >> -Already applied: 0001 A
> >> -Already applied: 0002 B
> >> -Committed: 0003 Z
> >> -EOF
> >> -
> > 
> > As I had mentioned in the previous round: I think we can retain these
> > messages for the `--merge` mode. And I think we should: users of --merge
> > have most likely become to expect them.
> > 
> > The most elegant way would probably be by adding code to the sequencer
> > that outputs these lines if in "merge" mode (and add a flag to say that we
> > *are* in "merge" mode).
> > 
> > To that end, the `make_script()` function would not generate `# pick` but
> > `drop` lines, I think, so that the sequencer can see those and print the
> > `Already applied: <message>` lines. And a successful `TODO_PICK` would
> > write out `Committed: <message>`.
> > 
> >>  test_expect_success 'rebase -m' '
> >>  	git rebase -m master >report &&
> >> +	>expect &&
> >>  	sed -n -e "/^Already applied: /p" \
> >>  		-e "/^Committed: /p" report >actual &&
> >>  	test_cmp expect actual
> >> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> >> index f355c6825a..49077200c5 100755
> >> --- a/t/t3420-rebase-autostash.sh
> >> +++ b/t/t3420-rebase-autostash.sh
> >> @@ -53,41 +53,6 @@ create_expected_success_interactive () {
> >>  	EOF
> >>  }
> >>  
> >> -create_expected_success_merge () {
> >> -	cat >expected <<-EOF
> >> -	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> >> -	HEAD is now at $(git rev-parse --short feature-branch) third commit
> >> -	First, rewinding head to replay your work on top of it...
> >> -	Merging unrelated-onto-branch with HEAD~1
> >> -	Merging:
> >> -	$(git rev-parse --short unrelated-onto-branch) unrelated commit
> >> -	$(git rev-parse --short feature-branch^) second commit
> >> -	found 1 common ancestor:
> >> -	$(git rev-parse --short feature-branch~2) initial commit
> >> -	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
> >> -	 Author: A U Thor <author@example.com>
> >> -	 Date: Thu Apr 7 15:14:13 2005 -0700
> >> -	 2 files changed, 2 insertions(+)
> >> -	 create mode 100644 file1
> >> -	 create mode 100644 file2
> >> -	Committed: 0001 second commit
> >> -	Merging unrelated-onto-branch with HEAD~0
> >> -	Merging:
> >> -	$(git rev-parse --short rebased-feature-branch~1) second commit
> >> -	$(git rev-parse --short feature-branch) third commit
> >> -	found 1 common ancestor:
> >> -	$(git rev-parse --short feature-branch~1) second commit
> >> -	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
> >> -	 Author: A U Thor <author@example.com>
> >> -	 Date: Thu Apr 7 15:15:13 2005 -0700
> >> -	 1 file changed, 1 insertion(+)
> >> -	 create mode 100644 file3
> >> -	Committed: 0002 third commit
> >> -	All done.
> >> -	Applied autostash.
> >> -	EOF
> >> -}
> >> -
> >>  create_expected_failure_am () {
> >>  	cat >expected <<-EOF
> >>  	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> >> @@ -112,43 +77,6 @@ create_expected_failure_interactive () {
> >>  	EOF
> >>  }
> >>  
> >> -create_expected_failure_merge () {
> >> -	cat >expected <<-EOF
> >> -	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> >> -	HEAD is now at $(git rev-parse --short feature-branch) third commit
> >> -	First, rewinding head to replay your work on top of it...
> >> -	Merging unrelated-onto-branch with HEAD~1
> >> -	Merging:
> >> -	$(git rev-parse --short unrelated-onto-branch) unrelated commit
> >> -	$(git rev-parse --short feature-branch^) second commit
> >> -	found 1 common ancestor:
> >> -	$(git rev-parse --short feature-branch~2) initial commit
> >> -	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
> >> -	 Author: A U Thor <author@example.com>
> >> -	 Date: Thu Apr 7 15:14:13 2005 -0700
> >> -	 2 files changed, 2 insertions(+)
> >> -	 create mode 100644 file1
> >> -	 create mode 100644 file2
> >> -	Committed: 0001 second commit
> >> -	Merging unrelated-onto-branch with HEAD~0
> >> -	Merging:
> >> -	$(git rev-parse --short rebased-feature-branch~1) second commit
> >> -	$(git rev-parse --short feature-branch) third commit
> >> -	found 1 common ancestor:
> >> -	$(git rev-parse --short feature-branch~1) second commit
> >> -	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
> >> -	 Author: A U Thor <author@example.com>
> >> -	 Date: Thu Apr 7 15:15:13 2005 -0700
> >> -	 1 file changed, 1 insertion(+)
> >> -	 create mode 100644 file3
> >> -	Committed: 0002 third commit
> >> -	All done.
> >> -	Applying autostash resulted in conflicts.
> >> -	Your changes are safe in the stash.
> >> -	You can run "git stash pop" or "git stash drop" at any time.
> >> -	EOF
> >> -}
> >> -
> >>  testrebase () {
> >>  	type=$1
> >>  	dotest=$2
> >> @@ -177,6 +105,9 @@ testrebase () {
> >>  	test_expect_success "rebase$type --autostash: check output" '
> >>  		test_when_finished git branch -D rebased-feature-branch &&
> >>  		suffix=${type#\ --} && suffix=${suffix:-am} &&
> >> +		if test ${suffix} = "merge"; then
> >> +			suffix=interactive
> >> +		fi &&
> >>  		create_expected_success_$suffix &&
> >>  		test_i18ncmp expected actual
> >>  	'
> >> @@ -274,6 +205,9 @@ testrebase () {
> >>  	test_expect_success "rebase$type: check output with conflicting stash" '
> >>  		test_when_finished git branch -D rebased-feature-branch &&
> >>  		suffix=${type#\ --} && suffix=${suffix:-am} &&
> >> +		if test ${suffix} = "merge"; then
> >> +			suffix=interactive
> >> +		fi &&
> >>  		create_expected_failure_$suffix &&
> >>  		test_i18ncmp expected actual
> >>  	'
> > 
> > As I stated earlier, I am uncomfortable with this solution. We change
> > behavior rather noticeably, and the regression test tells us the same. We
> > need to either try to deprecate `git rebase --merge`, or we need to at
> > least attempt to recreate the old behavior with the sequencer.
> > 
> >> diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
> >> index 99b2aac921..911ef49f70 100755
> >> --- a/t/t3421-rebase-topology-linear.sh
> >> +++ b/t/t3421-rebase-topology-linear.sh
> >> @@ -111,7 +111,7 @@ test_run_rebase () {
> >>  	"
> >>  }
> >>  test_run_rebase success ''
> >> -test_run_rebase failure -m
> >> +test_run_rebase success -m
> >>  test_run_rebase success -i
> >>  test_run_rebase success -p
> >>  
> >> @@ -126,7 +126,7 @@ test_run_rebase () {
> >>  	"
> >>  }
> >>  test_run_rebase success ''
> >> -test_run_rebase failure -m
> >> +test_run_rebase success -m
> >>  test_run_rebase success -i
> >>  test_run_rebase success -p
> >>  
> >> @@ -141,7 +141,7 @@ test_run_rebase () {
> >>  	"
> >>  }
> >>  test_run_rebase success ''
> >> -test_run_rebase failure -m
> >> +test_run_rebase success -m
> >>  test_run_rebase success -i
> >>  test_run_rebase success -p
> >>  
> >> @@ -284,7 +284,7 @@ test_run_rebase () {
> >>  	"
> >>  }
> >>  test_run_rebase success ''
> >> -test_run_rebase failure -m
> >> +test_run_rebase success -m
> >>  test_run_rebase success -i
> >>  test_run_rebase success -p
> >>  
> >> @@ -315,7 +315,7 @@ test_run_rebase () {
> >>  	"
> >>  }
> >>  test_run_rebase success ''
> >> -test_run_rebase failure -m
> >> +test_run_rebase success -m
> >>  test_run_rebase success -i
> >>  test_run_rebase failure -p
> >>  
> > 
> > Nice.
> > 
> >> diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
> >> index 846f85c27e..cd505c0711 100755
> >> --- a/t/t3425-rebase-topology-merges.sh
> >> +++ b/t/t3425-rebase-topology-merges.sh
> >> @@ -72,7 +72,7 @@ test_run_rebase () {
> >>  }
> >>  #TODO: make order consistent across all flavors of rebase
> >>  test_run_rebase success 'e n o' ''
> >> -test_run_rebase success 'e n o' -m
> >> +test_run_rebase success 'n o e' -m
> >>  test_run_rebase success 'n o e' -i
> >>  
> >>  test_run_rebase () {
> >> @@ -89,7 +89,7 @@ test_run_rebase () {
> >>  }
> >>  #TODO: make order consistent across all flavors of rebase
> >>  test_run_rebase success 'd e n o' ''
> >> -test_run_rebase success 'd e n o' -m
> >> +test_run_rebase success 'd n o e' -m
> >>  test_run_rebase success 'd n o e' -i
> >>  
> >>  test_run_rebase () {
> >> @@ -106,7 +106,7 @@ test_run_rebase () {
> >>  }
> >>  #TODO: make order consistent across all flavors of rebase
> >>  test_run_rebase success 'd e n o' ''
> >> -test_run_rebase success 'd e n o' -m
> >> +test_run_rebase success 'd n o e' -m
> >>  test_run_rebase success 'd n o e' -i
> >>  
> >>  test_expect_success "rebase -p is no-op in non-linear history" "
> > 
> > This is a bit unfortunate. I wonder if we could do something like this, to
> > retain the current behavior:
> > 
> > -- snip --
> > diff --git a/sequencer.c b/sequencer.c
> > index 9e1ab3a2a7..5018957e49 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -4394,7 +4394,8 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
> >  	revs.reverse = 1;
> >  	revs.right_only = 1;
> >  	revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> > -	revs.topo_order = 1;
> > +	if (!(flags & TODO_LIST_NO_TOPO_ORDER))
> > +		revs.topo_order = 1;
> >  
> >  	revs.pretty_given = 1;
> >  	git_config_get_string("rebase.instructionFormat", &format);
> > -- snap -
> > 
> > (and then pass TODO_LIST_NO_TOPO_ORDER if in "merge" mode)?
> > 
> >> diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
> >> index 9b2a274c71..145c61251d 100755
> >> --- a/t/t5407-post-rewrite-hook.sh
> >> +++ b/t/t5407-post-rewrite-hook.sh
> >> @@ -120,6 +120,7 @@ test_expect_success 'git rebase -m --skip' '
> >>  	git rebase --continue &&
> >>  	echo rebase >expected.args &&
> >>  	cat >expected.data <<-EOF &&
> >> +	$(git rev-parse C) $(git rev-parse HEAD^)
> >>  	$(git rev-parse D) $(git rev-parse HEAD)
> >>  	EOF
> >>  	verify_hook_input
> > 
> > This suggests to me that we call `commit` too many times. I think we
> > really need to address this without changing the test. This is a change in
> > behavior.
> > 
> >> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> >> index 81a5179e28..5cadedb2a9 100755
> >> --- a/t/t9903-bash-prompt.sh
> >> +++ b/t/t9903-bash-prompt.sh
> >> @@ -180,7 +180,7 @@ test_expect_success 'prompt - interactive rebase' '
> >>  '
> >>  
> >>  test_expect_success 'prompt - rebase merge' '
> >> -	printf " (b2|REBASE-m 1/3)" >expected &&
> >> +	printf " (b2|REBASE-i 1/3)" >expected &&
> > 
> > As I said, this should be easily addressed by having a tell-tale in
> > $state_dir/. Come to think of it, we must have had one, right? Let's see
> > how the bash-prompt does it.
> > 
> > *clicketyclick*
> > 
> > Ah, it is the *absence* of the `interactive` file... Which makes me wonder
> > whether we should not simply retain that behavior for `git rebase
> > --merge`?
> > 
> >>  	git checkout b2 &&
> >>  	test_when_finished "git checkout master" &&
> >>  	test_must_fail git rebase --merge b1 b2 &&
> >> -- 
> >> 2.19.1.858.g526e8fe740.dirty
> >>
> >>
> > 
> > Thank you for this pleasant read. I think there is still quite a bit of
> > work to do, but you already did most of it.
> > 
> > Out of curiosity, do you have a public repository with these patches in a
> > branch? (I might have an hour to play with it tonight...)
> > 
> > Thanks!
> > Dscho
> > 
> 
>