git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Stephen Haberman <stephen@exigencecorp.com>
Date
Oct 6, 2008, 15:21 UTC
Message-ID
<20081006102118.3e817a0f.stephen@exigencecorp.com>
In-Reply-To
<48E8DD7E.9040706@redhat.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
Previous: Avi KivityNext: Stephen Haberman
Message 2 of 8 in “git rebase -i -p broken?”
  1. Avi KivityOct 5, 2008
  2. rebase--interactive: if preserving merges, use first-parent to limit what is shown.Stephen Haberman, Oct 6, 2008
  3. Stephen HabermanOct 7, 2008
  4. Stephen HabermanOct 7, 2008
  5. Shawn O. PearceOct 7, 2008
  6. Avi KivityOct 7, 2008
  7. Stephan BeyerOct 7, 2008
  8. Avi KivityOct 7, 2008

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.