From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.5 required=3.0 tests=AWL,BAYES_00, FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id F23C01F87F for ; Mon, 12 Nov 2018 16:21:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729271AbeKMCPN (ORCPT ); Mon, 12 Nov 2018 21:15:13 -0500 Received: from mout.gmx.net ([212.227.17.20]:50811 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727709AbeKMCPN (ORCPT ); Mon, 12 Nov 2018 21:15:13 -0500 Received: from [192.168.0.129] ([37.201.193.149]) by mail.gmx.com (mrgmx101 [212.227.17.168]) with ESMTPSA (Nemesis) id 0LpwZn-1fshlN0wVj-00fmEK; Mon, 12 Nov 2018 17:21:06 +0100 Date: Mon, 12 Nov 2018 17:21:05 +0100 (STD) From: Johannes Schindelin X-X-Sender: virtualbox@gitforwindows.org To: Elijah Newren cc: git@vger.kernel.org, gitster@pobox.com, predatoramigo@gmail.com Subject: Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive In-Reply-To: <20181108060158.27145-3-newren@gmail.com> Message-ID: References: <20181108060158.27145-1-newren@gmail.com> <20181108060158.27145-3-newren@gmail.com> User-Agent: Alpine 2.21.1 (DEB 209 2017-03-23) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Provags-ID: V03:K1:2rbVchXD9T+64adYSDqwi/i3e+DC2titRSkmWTFe5L9gGmFR4p0 c+Z/ib7DouMrYG0OMCssTBTLjTtIbyhlyPYFEmLSVJzYnCzKxL0RINBOfjAGiDzt1KulAQ/ K3p3sebnQ+sp8vp7FfWMfiXP/Gjylm8FL1IhXMSX7TYrNsEWN3wC2thl43jjGD09NSR0V2o f0iHWjPqm3UjaASfRfddA== X-UI-Out-Filterresults: notjunk:1;V01:K0:W9UyjZT5DmM=:KF/evn6K+/NmEucU0Y9UBD 89xmPq/oJMmEMkQdlDYZ4ATldYnMkqBB4tO4CkZ17fV4Qrve/ojmHgLqyMFYGYNhhVZqhGHZG OaZuOaHALUMTNNN2d9wzuVPFb/cFbA+TTN+ajF4nP2+bymPOkEIqg2OJ55ASvyoxMDv+KboOU ghWu2B0hmsAJy2awOfUVLTiBkK1S3/jgweVYQRbYPche4q9yfmoZKB9g9PrQx/dG0kdvnf9u4 PyvV2kau/lrY0fyVNyxcdgoQyOZGziG3BY7zkFR0cnqrFxf68+h0WmlYXeQCmdns945zBQNvq 6C8xUqYsEZtZF+lnmdHdmITeHhs4p+JXVP386RKAiR0V95+wEj+V0Lx5NhpZ//4mR2/XUsSj3 DZI/kjRw87//XVCTxkkfLSwW/4jZhlSQz3hIpRrwzAfCxJNGb9AAw7vUquGxtuqn13GACimEy eqDeisIhuq6H9alOlC71Jmeeb7yBd4KE0Hfs90nGWdAVuq1DGyoSoBVsB/3rsDhbcvNUnypFZ ci7xTv2FuCbj6eA1jFcCP9erJ6h+7YmtM6r0qItexN4tHRsARgoGoDK4/qEHObmzf2lB6PYGl 9Qhxg3JAcsu4I0Cpxk3DsHQEc+/BtoOqRjRKwwbxUC0UZUVhkwEvQQBGFgrJSGpW3J/HW++5L zBIHOtqguwYmWW6xaPNJzK7T4nTFXmGQElO6aNPyK98BYLvkCDtqW4rEC16kszjfOWYDYlaDA zSYW9lYS9cSxi4rq2pQpXzxga9QhNKZJgQk7HOM6SiAxwwrFxMvjGDkIC1bjW2xyrMBs0BHrF 3aiC2iEjtR8y50gsDehUMC+u4xHg4Rm+E9KpPY+JZenyFwRg1FHHN/ghWBqBmGVe25snqy8PK VunKlOb7oUViUNicoW7FkrMJB2DTNgjl077XVVnt76QNwK8kKONCN1IDcOzSvJ2mHExveYLb9 HFRANA1OIHQ== Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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/` > -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: > - > * --[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? > +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 > 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: ` lines. And a successful `TODO_PICK` would write out `Committed: `. > 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 > - 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 > - 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 > - 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 > - 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