Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
Hi Elijah, On Wed, 14 Nov 2018, Elijah Newren wrote: > On Mon, Nov 12, 2018 at 8:21 AM Johannes Schindelin > wrote: > > > 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. > > I agree, it's not the best regression test. It'd be better if it > defined and tested for "the correct behavior", but I suspect it has > some value in that it's is guaranteeing that the rebase flavors at > least give a "somewhat reasonable" result. Sadly, It just allowed all > three rebase types to have slightly different "somewhat reasonable" > answers. True. I hope, though, that we can address this relatively easily (by toggling the `topo_order` flag). > > > 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? > > I wrote this comment months ago and don't remember the full details. > From the wording and as best I remember, I suspect I was at least > partially annoyed by the fact that these and other regression tests > for rebase seemed to not be testing for "correct" behavior, but > "existing" behavior. That wouldn't be so bad, except that existing > behavior differed on the exact same test setup for different rebase > backends and the differences in behavior were checked and enforced in > the regression tests. So, it became a matter of taste as to how much > things should be made identical and to which backend, or whether it > was more important to at least just consolidate the code first and > keep the results at least reasonable. At the time, I figured getting > fewer backends, all with reasonable behavior was a bit more important, > but since this difference is worrisome to you, I will try to dig > further into it. I agree that there is a ton of inconsistency going on, both in the behavior of the different backends as well as in the tests and their quality. The best explanation I have for this is: it grew historically, and we always have to strike a balance between strict rule enforcement and fun. > > > 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.) > > I agree users are upset when expectations are broken, but why would > they expect REBASE-m? In fact, I thought the prompt was a reflection > of what backend was in use, so that if someone reported an issue to > the git mailing list or a power user wanted to know where the control > files were located, they could look for them. As such, I thought that > switching the prompt to REBASE-i was the right thing to do so that it > would match user expectations. Yes, the backend changed; that's part > of the release notes. When you put it that way, I have to agree with you. > Is there documentation somewhere that specifies the meaning of this > prompt differently than the expectations I had somehow built up? I was just looking from my perspective, with my experience: if I was a heavy user of `git rebase -m` (I am not), I would stumble over this prompt. That's all. With your explanation, I agree that this is the best course, though. > > > 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 > > >
Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
Hi Phillip, On Mon, Nov 12, 2018 at 10:21 AM Phillip Wood wrote: > >> -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 :-) > >> -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'? They'd still get an error message about incompatible options; see my email to Dscho. However, since it tripped you both up, I'll make the clean up here a separate commit with some comments. > >> +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 Indeed, will fix.
Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
Hi Dscho, On Mon, Nov 12, 2018 at 8:21 AM Johannes Schindelin wrote: > > 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. I agree, it's not the best regression test. It'd be better if it defined and tested for "the correct behavior", but I suspect it has some value in that it's is guaranteeing that the rebase flavors at least give a "somewhat reasonable" result. Sadly, It just allowed all three rebase types to have slightly different "somewhat reasonable" answers. > > > 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? I wrote this comment months ago and don't remember the full details. >From the wording and as best I remember, I suspect I was at least partially annoyed by the fact that these and other regression tests for rebase seemed to not be testing for "correct" behavior, but "existing" behavior. That wouldn't be so bad, except that existing behavior differed on the exact same test setup for different rebase backends and the differences in behavior were checked and enforced in the regression tests. So, it became a matter of taste as to how much things should be made identical and to which backend, or whether it was more important to at least just consolidate the code first and keep the results at least reasonable. At the time, I figured getting fewer backends, all with reasonable behavior was a bit more important, but since this difference is worrisome to you, I will try to dig further into it. > > 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.) I agree users are upset when expectations are broken, but why would they expect REBASE-m? In fact, I thought the prompt was a reflection of what backend was in use, so that if someone reported an issue to the git mailing list or a power user wanted to know where the control files were located, they could look for them. As such, I thought that switching the prompt to REBASE-i was the right thing to do so that it would match user expectations. Yes, the backend changed; that's part of the release notes. Is there documentation somewhere that specifies the meaning of this prompt differently than the expectations I had somehow built up? > > 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/` Removing this was actually a suggestion by Phillip back in June at the end of https://public-inbox.org/git/13ccb4d9-582b-d26b-f595-59cb0b703...@talktalk.net/, for the purpose of removing an implementation details that users don't need to know about. As noted elsewhere in the thread, between you and Phillip, I'll add some comments to the commit message about this. > > -if test -n "$git_am_opt"; then > > - incompatible_opts=$(echo " $git_am_opt " | \ > > - sed -e 's/ -q / /g' -e 's/^ \(.*\)
Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
Hi Dscho, Thanks for the detailed review! I'll try to get back to all your comments, but for now I feel bad that I didn't see and respond to at least one sooner... On Mon, Nov 12, 2018 at 8:21 AM Johannes Schindelin wrote: > > 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...) https://github.com/newren/git/tree/rebase-new-default, but ignore the last two commits; they are separate and haven't been brought up to date despite having been minimally rebased.
Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
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 > >>
Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
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
Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
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 */ > +
[PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
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. Note that this change fixes a few known test failures (see t3421). 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. 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. t9903: --merge uses the interactive backend so the prompt expected is now REBASE-i. Signed-off-by: Elijah Newren --- .gitignore| 1 - Documentation/git-rebase.txt | 17 +--- Makefile | 1 - builtin/rebase.c | 10 +- git-legacy-rebase.sh | 55 +- git-rebase--merge.sh | 164 -- t/t3406-rebase-message.sh | 7 +- t/t3420-rebase-autostash.sh | 78 ++ t/t3421-rebase-topology-linear.sh | 10 +- t/t3425-rebase-topology-merges.sh | 6 +- t/t5407-post-rewrite-hook.sh | 1 + t/t9903-bash-prompt.sh| 2 +- 12 files changed, 50 insertions(+), 302 deletions(-) delete mode 100644 git-rebase--merge.sh diff --git a/.gitignore b/.gitignore index 0d77ea5894..910b1d2d2f 100644 --- a/.gitignore +++ b/.gitignore @@ -124,7 +124,6 @@ /git-rebase--am /git-rebase--common /git-rebase--interactive -/git-rebase--merge /git-rebase--preserve-merges /git-receive-pack /git-reflog 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) - -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 diff --git a/Makefile b/Makefile index bbfbb4292d..18e79fdea8 100644 --- a/Makefile +++ b/Makefile @@ -628,7 +628,6 @@ SCRIPT_LIB += git-parse-remote SCRIPT_LIB += git-rebase--am SCRIPT_LIB += git-rebase--common SCRIPT_LIB += git-rebase--preserve-merges -SCRIPT_LIB += git-rebase--merge SCRIPT_LIB += git-sh-setup SCRIPT_LIB += git-sh-i18n 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 */
