[PATCH] Fix interactive rebase on dropped commits.

[PATCH] Fix interactive rebase on dropped commits.

[Stephen Haberman]

Interactive rebase got its rev-list of commits to keep by --left-right and
--cherry-pick. Adding --cherry-pick would throw out commits that were just
duplicating changes already in the rebase target.

Which is desirable, except the dropped commit has forgotten about when it came
to rewriting the parents of its descendents, so the descendents would get
cherry-picked as-in and essentially make the rebase a no-op.

This change adds a $DOTEST/dropped directory to remember dropped commits and
rewrite its children's parents as the children's (possibly rewritten)
grandparents.

Signed-off-by: Stephen Haberman <stephen@exigencecorp.com>
---
 git-rebase--interactive.sh                  |   52 ++++++++++++---
 t/t3409-rebase-interactive-cherry-picked.sh |   94 +++++++++++++++++++++++++++
 2 files changed, 136 insertions(+), 10 deletions(-)
 create mode 100644 t/t3409-rebase-interactive-cherry-picked.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 929d681..2d62590 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -37,6 +37,7 @@ DONE="$DOTEST"/done
 MSG="$DOTEST"/message
 SQUASH_MSG="$DOTEST"/message-squash
 REWRITTEN="$DOTEST"/rewritten
+DROPPED="$DOTEST"/dropped
 PRESERVE_MERGES=
 STRATEGY=
 ONTO=
@@ -146,15 +147,7 @@ pick_one () {
 
 pick_one_preserving_merges () {
 	fast_forward=t
-	case "$1" in
-	-n)
-		fast_forward=f
-		sha1=$2
-		;;
-	*)
-		sha1=$1
-		;;
-	esac
+	case "$1" in -n) fast_forward=f sha1=$2 ;; *) sha1=$1 ;; esac
 	sha1=$(git rev-parse $sha1)
 
 	if test -f "$DOTEST"/current-commit
@@ -183,7 +176,28 @@ pick_one_preserving_merges () {
 				;;
 			esac
 		else
-			new_parents="$new_parents $p"
+			if test -f "$DROPPED"/$p
+			then
+				fast_forward=f
+				cat "$DROPPED"/$p | while read grandparent
+				do
+					if test -f "$REWRITTEN"/$grandparent
+					then
+						new_p=$(cat "$REWRITTEN"/$grandparent)
+					else
+						new_p=$grandparent
+					fi
+					case "$new_parents" in
+					*$new_p*)
+						;; # do nothing; that parent is already there
+					*)
+						new_parents="$new_parents $new_p"
+						;;
+					esac
+				done
+			else
+				new_parents="$new_parents $p"
+			fi
 		fi
 	done
 	case $fast_forward in
@@ -574,6 +588,24 @@ do
 #
 EOF
 
+		# Watch for commits that been dropped by --cherry-pick
+		if test t = "$PRESERVE_MERGES"
+		then
+			mkdir "$DROPPED"
+			# drop the --cherry-pick parameter this time
+			git rev-list $MERGES_OPTION --abbrev-commit \
+				--abbrev=7 $UPSTREAM...$HEAD --left-right | \
+				sed -n "s/^>//p" | while read rev
+			do
+				grep --quiet "$rev" "$TODO"
+				if [ $? -ne 0 ]
+				then
+					full=$(git rev-parse $rev)
+					git rev-list --parents -1 $rev | cut -d' ' -f2- | sed 's/ /\n/g' > "$DROPPED"/$full
+				fi
+			done
+		fi
+
 		has_action "$TODO" ||
 			die_abort "Nothing to do"
 
diff --git a/t/t3409-rebase-interactive-cherry-picked.sh b/t/t3409-rebase-interactive-cherry-picked.sh
new file mode 100644
index 0000000..3d20180
--- /dev/null
+++ b/t/t3409-rebase-interactive-cherry-picked.sh
@@ -0,0 +1,94 @@
+#!/bin/sh
+
+test_description='rebase interactive does not rebase'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo "setup" >a &&
+	git add a &&
+	git commit -m "setup" &&
+	git clone ./. server &&
+	rm -fr server/.git/hooks &&
+	git remote add origin ./server &&
+	git config --add branch.master.remote origin &&
+	git config --add branch.master.merge refs/heads/master &&
+	git fetch &&
+
+	git checkout -b stable master &&
+	echo "setup.stable" >a &&
+	git commit -a -m "stable" &&
+	git push origin stable
+'
+#
+# A --C------            <-- origin/stable
+#  \  |      \
+#   B -- D -- E -- F     <-- origin/topic2
+#    \|             \
+#     g -- h ------- i   <-- topic2
+#
+# Trying to push F..i
+#
+# merge-base(F, h) has two options: B and C
+#
+test_expect_success 'merging in stable with tricky double baserev does not fool the script' '
+	# B: start our topic2 branch, and share it
+	git checkout -b topic2 origin/stable &&
+	git config --add branch.topic2.merge refs/heads/topic2 &&
+	echo "commit B" >a.topic2 &&
+	git add a.topic2 &&
+	git commit -m "commit B created topic2" &&
+	git push origin topic2 &&
+
+	# C: now, separately, move ahead stable, and share it
+	git checkout stable
+	echo "commit C" >a &&
+	git commit -a -m "commit C moved stable" &&
+	git push origin stable &&
+
+	# D: have another client commit (in this case, it is the server, but close enough) moves topic2
+	cd server &&
+	git checkout topic2 &&
+	echo "commit D continuing topic2" >a.client2 &&
+	git add a.client2 &&
+	git commit -m "commit D by client2" &&
+
+	# E: the same other client merges the moved stable
+	git merge stable &&
+
+	# F: the same other client moves topic2 again
+	echo "commit F" >a.client2 &&
+	git commit -a -m "commit F by client2" &&
+	F_hash=$(git rev-parse HEAD) &&
+	cd .. &&
+
+	# g: now locally merge in the moved stable (even though our topic2 is out of date)
+	git checkout topic2 &&
+	git merge stable &&
+	g_hash=$(git rev-parse HEAD) &&
+
+	# h: advance local topic2
+	echo "commit H" >a.topic2 &&
+	git commit -a -m "commit H continues local fork" &&
+	h_hash=$(git rev-parse HEAD) &&
+
+	# i: make a new merge commit
+	git pull --no-rebase &&
+	i_hash=$(git rev-parse HEAD) &&
+
+	# Watch merge rejected as something that should get rebased
+	# ! git push origin topic2
+	test "$i_hash $h_hash $F_hash" = "$(git rev-list --parents --no-walk HEAD)"
+
+	# Now fix it the merge by rebasing it
+	git reset --hard ORIG_HEAD &&
+	GIT_EDITOR=: git rebase -i -p origin/topic2 &&
+	h2_hash=$(git rev-parse HEAD) &&
+
+	# Previously $F_hash was dropped and it was the same $h_hash $g_hash
+	test "$h2_hash $F_hash" = "$(git rev-list --parents --no-walk HEAD)"
+'
+
+test_done
+
+
-- 
1.6.0.2

[PATCH v2 RFC] Fix interactive rebase on dropped commits.

[Stephen Haberman]

Interactive rebase got it's rev-list of commits to keep by --left-right and
--cherry-pick. Adding --cherry-pick would throw out commits that were just
duplicating changes already in the rebase target.

Which is cool, except the dropped commit was forgotten about when it came to
rewriting the parents of its descendents, so the descendents would get
cherry-picked as-in and essentially make the rebase a noop.

This change adds a $DOTEST/dropped directory to remember dropped commits and
rewrite its children's parents as the children's grandparents (possibly
rewritten).

Signed-off-by: Stephen Haberman <stephen@exigencecorp.com>
---

This includes 3410, which borrows the 3404 DAG in a separate
test to do more a complex test of the dropped commits than my
previous test.

The implementation is also simpler--instead of following all
of a dropped commit's parents, we follow just the first. We can
trust rev-list is telling us to drop it for a reason, so only
the first-parent is needed so we can correctly rewrite commits
based on top of ours.

t3404 is still passing. (Finally.)

 git-rebase--interactive.sh                |   37 +++++++-
 t/t3410-rebase-preserve-dropped-merges.sh |  140 +++++++++++++++++++++++++++++
 2 files changed, 175 insertions(+), 2 deletions(-)
 create mode 100644 t/t3410-rebase-preserve-dropped-merges.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index edb6ec6..e8cb8a2 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -37,6 +37,7 @@ DONE="$DOTEST"/done
 MSG="$DOTEST"/message
 SQUASH_MSG="$DOTEST"/message-squash
 REWRITTEN="$DOTEST"/rewritten
+DROPPED="$DOTEST"/dropped
 PRESERVE_MERGES=
 STRATEGY=
 ONTO=
@@ -169,8 +170,12 @@ pick_one_preserving_merges () {
 
 	# rewrite parents; if none were rewritten, we can fast-forward.
 	new_parents=
-	for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -f2-)
+	pend=" $(git rev-list --parents -1 $sha1 | cut -d' ' -f2-)"
+	while [ "$pend" != "" ]
 	do
+		p=$(expr "$pend" : ' \([^ ]*\)')
+		pend="${pend# $p}"
+
 		if test -f "$REWRITTEN"/$p
 		then
 			new_p=$(cat "$REWRITTEN"/$p)
@@ -183,7 +188,13 @@ pick_one_preserving_merges () {
 				;;
 			esac
 		else
-			new_parents="$new_parents $p"
+			if test -f "$DROPPED"/$p
+			then
+				fast_forward=f
+				pend="$pend $(cat "$DROPPED"/$p)"
+			else
+				new_parents="$new_parents $new_p"
+			fi
 		fi
 	done
 	case $fast_forward in
@@ -582,6 +593,28 @@ first and then run 'git rebase --continue' again."
 #
 EOF
 
+		# Watch for commits that been dropped by --cherry-pick
+		if test t = "$PRESERVE_MERGES"
+		then
+			mkdir "$DROPPED"
+			# drop the --cherry-pick parameter this time
+			git rev-list $MERGES_OPTION --abbrev-commit \
+				--abbrev=7 $UPSTREAM...$HEAD --left-right | \
+				sed -n "s/^>//p" | while read rev
+			do
+				grep --quiet "$rev" "$TODO"
+				if [ $? -ne 0 ]
+				then
+					# Use -f2 because if rev-list is telling this commit is not
+					# worthwhile, we don't want to track its multiple heads,
+					# just the history of its first-parent for others that will
+					# be rebasing on top of us
+					full=$(git rev-parse $rev)
+					git rev-list --parents -1 $rev | cut -d' ' -f2 > "$DROPPED"/$full
+				fi
+			done
+		fi
+
 		has_action "$TODO" ||
 			die_abort "Nothing to do"
 
diff --git a/t/t3410-rebase-preserve-dropped-merges.sh b/t/t3410-rebase-preserve-dropped-merges.sh
new file mode 100644
index 0000000..7c8862b
--- /dev/null
+++ b/t/t3410-rebase-preserve-dropped-merges.sh
@@ -0,0 +1,140 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Stephen Haberman
+#
+
+test_description='git rebase preserve merges
+
+This test runs git rebase with preserve merges and ensures commits
+dropped by the --cherry-pick flag have their childrens parents
+rewritten.
+'
+. ./test-lib.sh
+
+# set up two branches like this:
+#
+# A - B - C - D - E
+#   \
+#     F - G - H
+#       \
+#         I
+#
+# where B, D and G touch the same file.
+
+test_expect_success 'setup' '
+	: > file1 &&
+	git add file1 &&
+	test_tick &&
+	git commit -m A &&
+	git tag A &&
+	echo 1 > file1 &&
+	test_tick &&
+	git commit -m B file1 &&
+	: > file2 &&
+	git add file2 &&
+	test_tick &&
+	git commit -m C &&
+	echo 2 > file1 &&
+	test_tick &&
+	git commit -m D file1 &&
+	: > file3 &&
+	git add file3 &&
+	test_tick &&
+	git commit -m E &&
+	git tag E &&
+	git checkout -b branch1 A &&
+	: > file4 &&
+	git add file4 &&
+	test_tick &&
+	git commit -m F &&
+	git tag F &&
+	echo 3 > file1 &&
+	test_tick &&
+	git commit -m G file1 &&
+	git tag G &&
+	: > file5 &&
+	git add file5 &&
+	test_tick &&
+	git commit -m H &&
+	git tag H &&
+	git checkout -b branch2 F &&
+	: > file6 &&
+	git add file6 &&
+	test_tick &&
+	git commit -m I &&
+	git tag I
+'
+
+# A - B - C - D - E
+#   \             \ \
+#     F - G - H -- L \        -->   L
+#       \            |               \
+#         I -- G2 -- J -- K           I -- K
+# G2 = same changes as G
+test_expect_success 'skip same-resolution merges with -p' '
+	git checkout branch1 &&
+	! git merge E &&
+	echo 23 > file1 &&
+	git add file1 &&
+	git commit -m L &&
+	git checkout branch2 &&
+	echo 3 > file1 &&
+	git commit -a -m G2 &&
+	! git merge E &&
+	echo 23 > file1 &&
+	git add file1 &&
+	git commit -m J &&
+	echo file7 > file7 &&
+	git add file7 &&
+	git commit -m K &&
+	GIT_EDITOR=: git rebase -i -p branch1 &&
+	test $(git rev-parse branch2^^) = $(git rev-parse branch1) &&
+	test "23" = "$(cat file1)" &&
+	test "" = "$(cat file6)" &&
+	test "file7" = "$(cat file7)" &&
+
+	git checkout branch1 &&
+	git reset --hard H &&
+	git checkout branch2 &&
+	git reset --hard I
+'
+
+# A - B - C - D - E
+#   \             \ \
+#     F - G - H -- L \        -->   L
+#       \            |               \
+#         I -- G2 -- J -- K           I -- G2 -- K
+# G2 = different changes as G
+test_expect_success 'keep different-resolution merges with -p' '
+	git checkout branch1 &&
+	! git merge E &&
+	echo 23 > file1 &&
+	git add file1 &&
+	git commit -m L &&
+	git checkout branch2 &&
+	echo 4 > file1 &&
+	git commit -a -m G2 &&
+	! git merge E &&
+	echo 24 > file1 &&
+	git add file1 &&
+	git commit -m J &&
+	echo file7 > file7 &&
+	git add file7 &&
+	git commit -m K &&
+	! GIT_EDITOR=: git rebase -i -p branch1 &&
+	echo 234 > file1 &&
+	git add file1 &&
+	GIT_EDITOR=: git rebase --continue &&
+	test $(git rev-parse branch2^^^) = $(git rev-parse branch1) &&
+	test "234" = "$(cat file1)" &&
+	test "" = "$(cat file6)" &&
+	test "file7" = "$(cat file7)" &&
+
+	git checkout branch1 &&
+	git reset --hard H &&
+	git checkout branch2 &&
+	git reset --hard I
+'
+
+test_done
+
-- 
1.6.0.2

[PATCH v3] rebase--interactive: fix parent rewriting for dropped commits

[Stephen Haberman]

`rebase -i -p` got its rev-list of commits to keep by --left-right and
--cherry-pick. Adding --cherry-pick would drop commits that duplicated changes
already in the rebase target.

The dropped commits were then forgotten about when it came to rewriting the
parents of their descendents, so the descendents would get cherry-picked with
their old, unwritten parents and essentially make the rebase a no-op.

This commit adds a $DOTEST/dropped directory to remember dropped commits and
rewrite their children's parent as the dropped commit's possibly-rewritten
first-parent.

Signed-off-by: Stephen Haberman <stephen@exigencecorp.com>
---

These two lines changed from the v2 patch:

    pend=" $(cat "$DROPPED"/$p)$pend"

It now puts $p at the start of $pend instead of the end. I have no tests
that assert this behavior (vs. the old of putting it at the end), but it
seems rational to put the dropped-replacement parent first so that the
current commit's parents come out in the same order (otherwise a commit
with parents "p1 p2dropped p3" would end up "p1 p3 p2firstparent").

Also, this line changed:

    new_parents="$new_parents $p"

Previously it said "$new_parents $new_p" which was a copy/paste bug--$new_p
was from up in the code a bit and here we just want to use $p.

I also cleaned up the commit message.

 git-rebase--interactive.sh                |   37 +++++++-
 t/t3410-rebase-preserve-dropped-merges.sh |  140 +++++++++++++++++++++++++++++
 2 files changed, 175 insertions(+), 2 deletions(-)
 create mode 100644 t/t3410-rebase-preserve-dropped-merges.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index edb6ec6..4d53347 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -37,6 +37,7 @@ DONE="$DOTEST"/done
 MSG="$DOTEST"/message
 SQUASH_MSG="$DOTEST"/message-squash
 REWRITTEN="$DOTEST"/rewritten
+DROPPED="$DOTEST"/dropped
 PRESERVE_MERGES=
 STRATEGY=
 ONTO=
@@ -169,8 +170,12 @@ pick_one_preserving_merges () {
 
 	# rewrite parents; if none were rewritten, we can fast-forward.
 	new_parents=
-	for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -f2-)
+	pend=" $(git rev-list --parents -1 $sha1 | cut -d' ' -f2-)"
+	while [ "$pend" != "" ]
 	do
+		p=$(expr "$pend" : ' \([^ ]*\)')
+		pend="${pend# $p}"
+
 		if test -f "$REWRITTEN"/$p
 		then
 			new_p=$(cat "$REWRITTEN"/$p)
@@ -183,7 +188,13 @@ pick_one_preserving_merges () {
 				;;
 			esac
 		else
-			new_parents="$new_parents $p"
+			if test -f "$DROPPED"/$p
+			then
+				fast_forward=f
+				pend=" $(cat "$DROPPED"/$p)$pend"
+			else
+				new_parents="$new_parents $p"
+			fi
 		fi
 	done
 	case $fast_forward in
@@ -582,6 +593,28 @@ first and then run 'git rebase --continue' again."
 #
 EOF
 
+		# Watch for commits that been dropped by --cherry-pick
+		if test t = "$PRESERVE_MERGES"
+		then
+			mkdir "$DROPPED"
+			# drop the --cherry-pick parameter this time
+			git rev-list $MERGES_OPTION --abbrev-commit \
+				--abbrev=7 $UPSTREAM...$HEAD --left-right | \
+				sed -n "s/^>//p" | while read rev
+			do
+				grep --quiet "$rev" "$TODO"
+				if [ $? -ne 0 ]
+				then
+					# Use -f2 because if rev-list is telling this commit is not
+					# worthwhile, we don't want to track its multiple heads,
+					# just the history of its first-parent for others that will
+					# be rebasing on top of us
+					full=$(git rev-parse $rev)
+					git rev-list --parents -1 $rev | cut -d' ' -f2 > "$DROPPED"/$full
+				fi
+			done
+		fi
+
 		has_action "$TODO" ||
 			die_abort "Nothing to do"
 
diff --git a/t/t3410-rebase-preserve-dropped-merges.sh b/t/t3410-rebase-preserve-dropped-merges.sh
new file mode 100644
index 0000000..7c8862b
--- /dev/null
+++ b/t/t3410-rebase-preserve-dropped-merges.sh
@@ -0,0 +1,140 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Stephen Haberman
+#
+
+test_description='git rebase preserve merges
+
+This test runs git rebase with preserve merges and ensures commits
+dropped by the --cherry-pick flag have their childrens parents
+rewritten.
+'
+. ./test-lib.sh
+
+# set up two branches like this:
+#
+# A - B - C - D - E
+#   \
+#     F - G - H
+#       \
+#         I
+#
+# where B, D and G touch the same file.
+
+test_expect_success 'setup' '
+	: > file1 &&
+	git add file1 &&
+	test_tick &&
+	git commit -m A &&
+	git tag A &&
+	echo 1 > file1 &&
+	test_tick &&
+	git commit -m B file1 &&
+	: > file2 &&
+	git add file2 &&
+	test_tick &&
+	git commit -m C &&
+	echo 2 > file1 &&
+	test_tick &&
+	git commit -m D file1 &&
+	: > file3 &&
+	git add file3 &&
+	test_tick &&
+	git commit -m E &&
+	git tag E &&
+	git checkout -b branch1 A &&
+	: > file4 &&
+	git add file4 &&
+	test_tick &&
+	git commit -m F &&
+	git tag F &&
+	echo 3 > file1 &&
+	test_tick &&
+	git commit -m G file1 &&
+	git tag G &&
+	: > file5 &&
+	git add file5 &&
+	test_tick &&
+	git commit -m H &&
+	git tag H &&
+	git checkout -b branch2 F &&
+	: > file6 &&
+	git add file6 &&
+	test_tick &&
+	git commit -m I &&
+	git tag I
+'
+
+# A - B - C - D - E
+#   \             \ \
+#     F - G - H -- L \        -->   L
+#       \            |               \
+#         I -- G2 -- J -- K           I -- K
+# G2 = same changes as G
+test_expect_success 'skip same-resolution merges with -p' '
+	git checkout branch1 &&
+	! git merge E &&
+	echo 23 > file1 &&
+	git add file1 &&
+	git commit -m L &&
+	git checkout branch2 &&
+	echo 3 > file1 &&
+	git commit -a -m G2 &&
+	! git merge E &&
+	echo 23 > file1 &&
+	git add file1 &&
+	git commit -m J &&
+	echo file7 > file7 &&
+	git add file7 &&
+	git commit -m K &&
+	GIT_EDITOR=: git rebase -i -p branch1 &&
+	test $(git rev-parse branch2^^) = $(git rev-parse branch1) &&
+	test "23" = "$(cat file1)" &&
+	test "" = "$(cat file6)" &&
+	test "file7" = "$(cat file7)" &&
+
+	git checkout branch1 &&
+	git reset --hard H &&
+	git checkout branch2 &&
+	git reset --hard I
+'
+
+# A - B - C - D - E
+#   \             \ \
+#     F - G - H -- L \        -->   L
+#       \            |               \
+#         I -- G2 -- J -- K           I -- G2 -- K
+# G2 = different changes as G
+test_expect_success 'keep different-resolution merges with -p' '
+	git checkout branch1 &&
+	! git merge E &&
+	echo 23 > file1 &&
+	git add file1 &&
+	git commit -m L &&
+	git checkout branch2 &&
+	echo 4 > file1 &&
+	git commit -a -m G2 &&
+	! git merge E &&
+	echo 24 > file1 &&
+	git add file1 &&
+	git commit -m J &&
+	echo file7 > file7 &&
+	git add file7 &&
+	git commit -m K &&
+	! GIT_EDITOR=: git rebase -i -p branch1 &&
+	echo 234 > file1 &&
+	git add file1 &&
+	GIT_EDITOR=: git rebase --continue &&
+	test $(git rev-parse branch2^^^) = $(git rev-parse branch1) &&
+	test "234" = "$(cat file1)" &&
+	test "" = "$(cat file6)" &&
+	test "file7" = "$(cat file7)" &&
+
+	git checkout branch1 &&
+	git reset --hard H &&
+	git checkout branch2 &&
+	git reset --hard I
+'
+
+test_done
+
-- 
1.6.0.2