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

[Phillip Wood <phillip.wood@talktalk.net>]

Hi Elijah

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.

> 
>> -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
>