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