[PATCH v2 0/1] Fix rebase -p --onto

[PATCH v2 0/1] Fix rebase -p --onto

[Greg Price]

I sent a version of the following patch last week to fix the behavior
of rebase -p to respect --onto.  Some questions were raised and I
believe I've addressed all of them.

The code already distinguishes the cases Junio described in a natural
way, as demonstrated by a test case in the revised patch.  The use
case Hannes described has never worked, as Dscho explained, and in any
case concerns a different part of the code from this patch -- when the
great day comes that rebase -i -p can reorder commits, this patch will
work smoothly with that.

Are there further comments?  If not, I believe this is ready for merge.

Greg

[PATCH v2 1/1] Fix rebase -p --onto

[Greg Price]

In a rebase with --onto, the correct test for whether we can skip
rewriting a commit is if it is already on top of $ONTO, not $UPSTREAM.
Without --onto, this distinction does not exist and the behavior does
not change.

In a situation with two merged branches on a common base X:

 X---o---o---o---M
  \             /
   x---x---x---x

 Y

if we try to move the branches from their base on X to be based on Y,
so as to get

 X

 Y---o'--o'--o'--M'
  \             /
   x'--x'--x'--x'

then we fail.  The command `git rebase -p --onto Y X M` moves only the
first-parent chain, like so:

 X
  \
   x---x---x---x
                \
 Y---o'--o'--o'--M'

because it mistakenly drops the other branch(es) x---x---x---x from
the TODO file.  This tests and fixes this behavior.

Signed-off-by: Greg Price <price@ksplice.com>
---
 git-rebase--interactive.sh      |    2 +-
 t/t3414-rebase-preserve-onto.sh |   80 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+), 1 deletions(-)
 create mode 100755 t/t3414-rebase-preserve-onto.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f96d887..23ded48 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -703,7 +703,7 @@ first and then run 'git rebase --continue' again."
 					preserve=t
 					for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)
 					do
-						if test -f "$REWRITTEN"/$p -a \( $p != $UPSTREAM -o $sha1 = $first_after_upstream \)
+						if test -f "$REWRITTEN"/$p -a \( $p != $ONTO -o $sha1 = $first_after_upstream \)
 						then
 							preserve=f
 						fi
diff --git a/t/t3414-rebase-preserve-onto.sh b/t/t3414-rebase-preserve-onto.sh
new file mode 100755
index 0000000..80019ee
--- /dev/null
+++ b/t/t3414-rebase-preserve-onto.sh
@@ -0,0 +1,80 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Greg Price
+#
+
+test_description='git rebase -p should respect --onto
+
+In a rebase with --onto, we should rewrite all the commits that
+aren'"'"'t on top of $ONTO, even if they are on top of $UPSTREAM.
+'
+. ./test-lib.sh
+
+. ../lib-rebase.sh
+
+# Set up branches like this:
+# A1---B1---E1---F1---G1
+#  \    \             /
+#   \    \--C1---D1--/
+#    H1
+
+test_expect_success 'setup' '
+	test_commit A1 &&
+	test_commit B1 &&
+	test_commit C1 &&
+	test_commit D1 &&
+	git reset --hard B1 &&
+	test_commit E1 &&
+	test_commit F1 &&
+	test_merge G1 D1 &&
+	git reset --hard A1 &&
+	test_commit H1
+'
+
+# Now rebase merge G1 from both branches' base B1, both should move:
+# A1---B1---E1---F1---G1
+#  \    \             /
+#   \    \--C1---D1--/
+#    \
+#     H1---E2---F2---G2
+#      \             /
+#       \--C2---D2--/
+
+test_expect_success 'rebase from B1 onto H1' '
+	git checkout G1 &&
+	git rebase -p --onto H1 B1 &&
+	test "$(git rev-parse HEAD^1^1^1)" = "$(git rev-parse H1)" &&
+	test "$(git rev-parse HEAD^2^1^1)" = "$(git rev-parse H1)"
+'
+
+# On the other hand if rebase from E1 which is within one branch,
+# then the other branch stays:
+# A1---B1---E1---F1---G1
+#  \    \             /
+#   \    \--C1---D1--/
+#    \             \
+#     H1-----F3-----G3
+
+test_expect_success 'rebase from E1 onto H1' '
+	git checkout G1 &&
+	git rebase -p --onto H1 E1 &&
+	test "$(git rev-parse HEAD^1^1)" = "$(git rev-parse H1)" &&
+	test "$(git rev-parse HEAD^2)" = "$(git rev-parse D1)"
+'
+
+# And the same if we rebase from a commit in the second-parent branch.
+# A1---B1---E1---F1----G1
+#  \    \          \   /
+#   \    \--C1---D1-\-/
+#    \               \
+#     H1------D3------G4
+
+test_expect_success 'rebase from C1 onto H1' '
+	git checkout G1 &&
+	git rev-list --first-parent --pretty=oneline C1..G1 &&
+	git rebase -p --onto H1 C1 &&
+	test "$(git rev-parse HEAD^2^1)" = "$(git rev-parse H1)" &&
+	test "$(git rev-parse HEAD^1)" = "$(git rev-parse F1)"
+'
+
+test_done
-- 
1.6.3.1.499.ge7b8da

Re: [PATCH v2 0/1] Fix rebase -p --onto

[Johannes Schindelin]

Hi,

On Wed, 22 Jul 2009, Greg Price wrote:

> [...] when the great day comes that rebase -i -p can reorder commits 
> [...]

If you want to help, I would be so thankful.  In that case, it might make 
more sense to hold off this patch and integrate it into the rebase-i-p 
patch series, rather than requiring a rebase of rebase-i-p on top of your 
patches, which will postpone said day only further.

Ciao,
Dscho

Re: [PATCH v2 0/1] Fix rebase -p --onto

[Greg Price]

On Wed, Jul 22, 2009 at 07:07:17PM +0200, Johannes Schindelin wrote:
> On Wed, 22 Jul 2009, Greg Price wrote:
> > [...] when the great day comes that rebase -i -p can reorder commits 
> 
> If you want to help, I would be so thankful.  In that case, it might make 
> more sense to hold off this patch and integrate it into the rebase-i-p 
> patch series, rather than requiring a rebase of rebase-i-p on top of your 
> patches, which will postpone said day only further.

Where is your patch series and what are the big things that need doing
before it can be merged?  I can't promise a great deal of time up
front, but I would be glad to look at it and may be able to pitch in.

Because this patch is about --onto together with -p and doesn't really
involve -i, I think it still makes sense to merge.  It's only one line
plus tests, so I don't think it will create that much of a rebase burden.

Cheers,
Greg

Re: [PATCH v2 0/1] Fix rebase -p --onto

[Johannes Schindelin]

Hi,

On Wed, 22 Jul 2009, Greg Price wrote:

> On Wed, Jul 22, 2009 at 07:07:17PM +0200, Johannes Schindelin wrote:
> > On Wed, 22 Jul 2009, Greg Price wrote:
> > > [...] when the great day comes that rebase -i -p can reorder commits 
> > 
> > If you want to help, I would be so thankful.  In that case, it might make 
> > more sense to hold off this patch and integrate it into the rebase-i-p 
> > patch series, rather than requiring a rebase of rebase-i-p on top of your 
> > patches, which will postpone said day only further.
> 
> Where is your patch series and what are the big things that need doing
> before it can be merged?  I can't promise a great deal of time up
> front, but I would be glad to look at it and may be able to pitch in.
> 
> Because this patch is about --onto together with -p and doesn't really
> involve -i, I think it still makes sense to merge.  It's only one line
> plus tests, so I don't think it will create that much of a rebase burden.

Fair enough.  I was fooled by the size of the mail; the main part is the 
test (which is good), though, so strike my objections.

Ciao,
Dscho