[PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.

[Stephen Haberman <stephen@exigencecorp.com>]

This commit fixes Avi Kivity's use case of squashing two commits on either side
of a merge together.

Changes include:

- Delaying storing the rewrite information if the current commit we are
  applying is being squashed. This means storing multiple lines in the
  current-commit file and recording each of them as rewritten to the
  same HEAD on the next commit.

- Move the "no squashing merges" check into the case statement for
  merges as previously it was catching catching even single-parent
  squashes.

- Conditionally pass "--first-parent" to `git rev-list` based on whether
  this is a rebase is preserving merges or not.

- At the end, just take the current head for the new branch ref instead
  of trying to look up what the OLDHEAD was rewritten to. This fails
  because the OLDHEAD was squashed/moved up earlier in the timeline, so
  even if we can find its rewritten HEAD, that is no longer what we
  ended up at.

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

I agree with Avi on what the rebase -i -p behavior should be for his
scenario. This patch makes it so. However, the bane of my existence,
t3404 is failing ~12 tests in, which is a real PITA to debug, so please
let me know if this is a worthwhile tangent to continue on.

(That last change of dropping the OLDHEAD->NEWHEAD guessing is probably
what is causing t3404 to fail, but I can't reason why it'd need to do
that rather than just use HEAD.)

I've read in the archives about the git-sequencer stuff, which sounds
cool, my thought is that, if anything, this will clarify git rebase -i -p
behavior and add tests that can later be ensured to still pass when
git-sequencer is dropped in.

 git-rebase--interactive.sh               |   53 ++++++++++---------
 t/t3411-rebase-preserve-around-merges.sh |   83 ++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 25 deletions(-)
 create mode 100644 t/t3411-rebase-preserve-around-merges.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index edb6ec6..9914111 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -159,13 +159,18 @@ pick_one_preserving_merges () {
 
 	if test -f "$DOTEST"/current-commit
 	then
-		current_commit=$(cat "$DOTEST"/current-commit) &&
-		git rev-parse HEAD > "$REWRITTEN"/$current_commit &&
-		rm "$DOTEST"/current-commit ||
-		die "Cannot write current commit's replacement sha1"
+		if [ "$fast_forward" == "t" ]
+		then
+			cat "$DOTEST"/current-commit | while read current_commit
+			do
+				git rev-parse HEAD > "$REWRITTEN"/$current_commit
+			done
+			rm "$DOTEST"/current-commit ||
+			die "Cannot write current commit's replacement sha1"
+		fi
 	fi
 
-	echo $sha1 > "$DOTEST"/current-commit
+	echo $sha1 >> "$DOTEST"/current-commit
 
 	# rewrite parents; if none were rewritten, we can fast-forward.
 	new_parents=
@@ -193,15 +198,19 @@ pick_one_preserving_merges () {
 			die "Cannot fast forward to $sha1"
 		;;
 	f)
-		test "a$1" = a-n && die "Refusing to squash a merge: $sha1"
-
 		first_parent=$(expr "$new_parents" : ' \([^ ]*\)')
-		# detach HEAD to current parent
-		output git checkout $first_parent 2> /dev/null ||
-			die "Cannot move HEAD to $first_parent"
+
+		if [ "$1" != "-n" ]
+		then
+			# detach HEAD to current parent
+			output git checkout $first_parent 2> /dev/null ||
+				die "Cannot move HEAD to $first_parent"
+		fi
 
 		case "$new_parents" in
 		' '*' '*)
+			test "a$1" = a-n && die "Refusing to squash a merge: $sha1"
+
 			# redo merge
 			author_script=$(get_author_ident_from_commit $sha1)
 			eval "$author_script"
@@ -350,20 +359,7 @@ do_next () {
 	HEADNAME=$(cat "$DOTEST"/head-name) &&
 	OLDHEAD=$(cat "$DOTEST"/head) &&
 	SHORTONTO=$(git rev-parse --short $(cat "$DOTEST"/onto)) &&
-	if test -d "$REWRITTEN"
-	then
-		test -f "$DOTEST"/current-commit &&
-			current_commit=$(cat "$DOTEST"/current-commit) &&
-			git rev-parse HEAD > "$REWRITTEN"/$current_commit
-		if test -f "$REWRITTEN"/$OLDHEAD
-		then
-			NEWHEAD=$(cat "$REWRITTEN"/$OLDHEAD)
-		else
-			NEWHEAD=$OLDHEAD
-		fi
-	else
-		NEWHEAD=$(git rev-parse HEAD)
-	fi &&
+	NEWHEAD=$(git rev-parse HEAD) &&
 	case $HEADNAME in
 	refs/*)
 		message="$GIT_REFLOG_ACTION: $HEADNAME onto $SHORTONTO)" &&
@@ -561,11 +557,18 @@ first and then run 'git rebase --continue' again."
 			MERGES_OPTION=--no-merges
 		fi
 
+		if test t = "$PRESERVE_MERGES"
+		then
+			first_parent="--first-parent"
+		else
+			first_parent=""
+		fi
+
 		SHORTUPSTREAM=$(git rev-parse --short $UPSTREAM)
 		SHORTHEAD=$(git rev-parse --short $HEAD)
 		SHORTONTO=$(git rev-parse --short $ONTO)
 		git rev-list $MERGES_OPTION --pretty=oneline --abbrev-commit \
-			--abbrev=7 --reverse --left-right --cherry-pick \
+			--abbrev=7 --reverse --left-right --cherry-pick $first_parent \
 			$UPSTREAM...$HEAD | \
 			sed -n "s/^>/pick /p" > "$TODO"
 		cat >> "$TODO" << EOF
diff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh
new file mode 100644
index 0000000..b130f5f
--- /dev/null
+++ b/t/t3411-rebase-preserve-around-merges.sh
@@ -0,0 +1,83 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Stephen Haberman
+#
+
+test_description='git rebase preserve merges
+
+This test runs git rebase with and tries to squash a commit from after a merge
+to before the merge.
+'
+. ./test-lib.sh
+
+# Copy/paste from t3404-rebase-interactive.sh
+echo "#!$SHELL_PATH" >fake-editor.sh
+cat >> fake-editor.sh <<\EOF
+case "$1" in
+*/COMMIT_EDITMSG)
+	test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
+	test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
+	exit
+	;;
+esac
+test -z "$EXPECT_COUNT" ||
+	test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) ||
+	exit
+test -z "$FAKE_LINES" && exit
+grep -v '^#' < "$1" > "$1".tmp
+rm -f "$1"
+cat "$1".tmp
+action=pick
+for line in $FAKE_LINES; do
+	case $line in
+	squash|edit)
+		action="$line";;
+	*)
+		echo sed -n "${line}s/^pick/$action/p"
+		sed -n "${line}p" < "$1".tmp
+		sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
+		action=pick;;
+	esac
+done
+EOF
+
+test_set_editor "$(pwd)/fake-editor.sh"
+chmod a+x fake-editor.sh
+
+# set up two branches like this:
+#
+# A - B - D - E - F
+#      \     /
+#       - C -
+
+test_expect_success 'setup' '
+	touch a &&
+	touch b &&
+	git add a &&
+	git commit -m A &&
+	git add b &&
+	git commit -m B &&
+	git tag B &&
+	git checkout -b branch &&
+	touch c &&
+	git add c &&
+	git commit -m C &&
+	git checkout master &&
+	touch d &&
+	git add d &&
+	git commit -m D &&
+	git merge branch &&
+	touch f &&
+	git add f &&
+	git commit -m F &&
+	git tag F
+'
+
+test_expect_success 'squash F into D' '
+	FAKE_LINES="1 squash 3 2" git rebase -i -p B &&
+	test "$(git rev-parse HEAD^2)" = "$(git rev-parse branch)" &&
+	test "$(git rev-parse HEAD~2)" = "$(git rev-parse B)"
+'
+
+test_done
+
-- 
1.6.0.2