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=-4.5 required=3.0 tests=AWL,BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,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 88DE81F87F for ; Mon, 12 Nov 2018 18:21:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728378AbeKMEPk (ORCPT ); Mon, 12 Nov 2018 23:15:40 -0500 Received: from smtp-out-1.talktalk.net ([62.24.135.65]:51599 "EHLO smtp-out-1.talktalk.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727247AbeKMEPk (ORCPT ); Mon, 12 Nov 2018 23:15:40 -0500 Received: from [192.168.2.201] ([92.22.32.73]) by smtp.talktalk.net with SMTP id MGpkgho5dwhzSMGpkg157p; Mon, 12 Nov 2018 18:21:13 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=talktalk.net; s=cmr1711; t=1542046873; bh=+8kGor27kX/PA7VJ5PmWB6T7exM3dQwsJin7YKEW5zE=; h=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To; b=Uw1Hc9yBgaDMe2x60AqUAqv2oYId4b8KDWTVWLb0ur29+sk5x3osExKMVEv2BZwY0 3Gh5ZQEEMd4nVvsqZ79DDLsNL03f+7mOHvQYjR/k+MQCfkXAHrrk0oO1QElgsVygoY oLjDhZDnbl8HExgb3C46UiNXDYIxja+V8bgqBi0U= X-Originating-IP: [92.22.32.73] X-Spam: 0 X-OAuthority: v=2.3 cv=e8Iot5h/ c=1 sm=1 tr=0 a=w3K0eKD2tyZHkEydg3BQCA==:117 a=w3K0eKD2tyZHkEydg3BQCA==:17 a=IkcTkHD0fZMA:10 a=A1X0JdhQAAAA:8 a=namNX2ffzn1O4OSF0a8A:9 a=XQyNOiE4GU4OpH0Z:21 a=DakGSJnJn3hb0C-X:21 a=QEXdDO2ut3YA:10 a=Df3jFdWbhGDLdZNm0fyq:22 Reply-To: phillip.wood@dunelm.org.uk Subject: Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive To: Johannes Schindelin , Elijah Newren Cc: git@vger.kernel.org, gitster@pobox.com, predatoramigo@gmail.com References: <20181108060158.27145-1-newren@gmail.com> <20181108060158.27145-3-newren@gmail.com> From: Phillip Wood Message-ID: Date: Mon, 12 Nov 2018 18:21:12 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit X-CMAE-Envelope: MS4wfLroUGd8pIQeZm+q1HmbtmpTaVDBIgsebVpwOKAP4ZN886sB0rAWm/7bx/A9+Rxh0RIkomIT4QHgrzmVKKwgdncQpWjjOKpOzRnKD8xUA/MXLQ+V6q64 kwmvIJPMmf2KeXFHZ5sMMbHe5ER6Ll1KEdp6xdWut5I6Nq155z1/Gt71y4bCQKrtc7DWnAn3FBXxIYcCFmu1mNGJlYkicxvuc7iXfvLq+dzYkJfM5+rdYZyD dZAorgpAwbfFZ1oinuG+SSP+qvdSKsna9x46EFgGNfcG1rUKkyzjFb8ttnqF2dCW+X9A/4IwgOMIaq/tyEcG6A== Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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= --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: ` 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 >