[PATCH] rebase-i-p: squashing and limiting todo

STALE3736d

12 messages, 4 authors, 2016-06-15 · open the first message on its own page

[PATCH] rebase-i-p: squashing and limiting todo

From: Stephen Haberman <hidden>
Date: 2016-06-15 22:45:29

This is v3, rebased on top of sp/maint with sp/master and sh/maint-rebase3.

So, it should apply cleanly to that. Junio's feedback aside, the only change
from v2 is that maint-rebase3's dropped commit check needs to be done before
Johannes's recent addition of:

    test -s "$TODO" || echo noop >> "$TODO"

Because now the todo may temporarily have picks in it while probing for parents
that could later be removed by the dropped/cherry-picked commit check.

Previously todo never had temporary entries, so it didn't matter when the
dropped commit check was done.

(Apologies for not seeing Junio's what's cooking first and using his
maint/master instead of Shawn's. Let me know if I need to redo this and I will
get even more practice at rebasing.)

(Gah, resending with the list cc'd this time. Dammit, sorry about that.)

Thanks,
Stephen

Stephen Haberman (7):
  rebase-i-p: test to exclude commits from todo based on its parents
  rebase-i-p: use HEAD for updating the ref instead of mapping OLDHEAD
  rebase-i-p: delay saving current-commit to REWRITTEN if squashing
  rebase-i-p: fix 'no squashing merges' tripping up non-merges
  rebase-i-p: only list commits that require rewriting in todo
  rebase-i-p: do not include non-first-parent commits touching UPSTREAM
  rebase-i-p: if todo was reordered use HEAD as the rewritten parent

 git-rebase--interactive.sh               |  131 ++++++++++++++++++-----------
 t/t3411-rebase-preserve-around-merges.sh |  136 ++++++++++++++++++++++++++++++
 2 files changed, 218 insertions(+), 49 deletions(-)
 create mode 100644 t/t3411-rebase-preserve-around-merges.sh

[PATCH] rebase-i-p: delay saving current-commit to REWRITTEN if squashing

From: Stephen Haberman <hidden>
Date: 2016-06-15 22:45:29

If the current-commit was dumped to REWRITTEN, but then we squash the next
commit in to it, we have invalidated the HEAD was just written to REWRITTEN.
Instead, append the squash hash to current-commit and save both of them the next
time around.

Signed-off-by: Stephen Haberman <redacted>
---
 git-rebase--interactive.sh |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c968117..23cf7a5 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -170,13 +170,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=
-- 
1.6.0.2

[PATCH] rebase-i-p: test to exclude commits from todo based on its parents

From: Stephen Haberman <hidden>
Date: 2016-06-15 22:45:29

The first case was based off a script from Avi Kivity [off-list ref].

The second case includes a merge-of-a-merge to ensure both are included in todo.

Signed-off-by: Stephen Haberman <redacted>
---
 t/t3411-rebase-preserve-around-merges.sh |  136 ++++++++++++++++++++++++++++++
 1 files changed, 136 insertions(+), 0 deletions(-)
 create mode 100644 t/t3411-rebase-preserve-around-merges.sh
diff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh
new file mode 100644
index 0000000..b3973c9
--- /dev/null
+++ b/t/t3411-rebase-preserve-around-merges.sh
@@ -0,0 +1,136 @@
+#!/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:
+#
+# A1 - B1 - D1 - E1 - F1
+#       \        /
+#        -- C1 --
+
+test_expect_success 'setup' '
+	touch a &&
+	touch b &&
+	git add a &&
+	git commit -m A1 &&
+	git tag A1
+	git add b &&
+	git commit -m B1 &&
+	git tag B1 &&
+	git checkout -b branch &&
+	touch c &&
+	git add c &&
+	git commit -m C1 &&
+	git checkout master &&
+	touch d &&
+	git add d &&
+	git commit -m D1 &&
+	git merge branch &&
+	touch f &&
+	git add f &&
+	git commit -m F1 &&
+	git tag F1
+'
+
+# Should result in:
+#
+# A1 - B1 - D2 - E2
+#       \        /
+#        -- C1 --
+#
+test_expect_failure 'squash F1 into D1' '
+	FAKE_LINES="1 squash 3 2" git rebase -i -p B1 &&
+	test "$(git rev-parse HEAD^2)" = "$(git rev-parse branch)" &&
+	test "$(git rev-parse HEAD~2)" = "$(git rev-parse B1)" &&
+	git tag E2
+'
+
+# Start with:
+#
+# A1 - B1 - D2 - E2
+#  \
+#   G1 ---- L1 ---- M1
+#    \             /
+#     H1 -- J1 -- K1
+#      \         /
+#        -- I1 --
+#
+# And rebase G1..M1 onto E2
+
+test_expect_failure 'rebase two levels of merge' '
+	git checkout -b branch2 A1 &&
+	touch g &&
+	git add g &&
+	git commit -m G1 &&
+	git checkout -b branch3 &&
+	touch h
+	git add h &&
+	git commit -m H1 &&
+	git checkout -b branch4 &&
+	touch i &&
+	git add i &&
+	git commit -m I1 &&
+	git tag I1 &&
+	git checkout branch3 &&
+	touch j &&
+	git add j &&
+	git commit -m J1 &&
+	git merge I1 --no-commit &&
+	git commit -m K1 &&
+	git tag K1 &&
+	git checkout branch2 &&
+	touch l &&
+	git add l &&
+	git commit -m L1 &&
+	git merge K1 --no-commit &&
+	git commit -m M1 &&
+	GIT_EDITOR=: git rebase -i -p E2 &&
+	test "$(git rev-parse HEAD~3)" = "$(git rev-parse E2)" &&
+	test "$(git rev-parse HEAD~2)" = "$(git rev-parse HEAD^2^2~2)" &&
+	test "$(git rev-parse HEAD^2^1^1)" = "$(git rev-parse HEAD^2^2^1)"
+'
+
+test_done
+
-- 
1.6.0.2

[PATCH] rebase-i-p: use HEAD for updating the ref instead of mapping OLDHEAD

From: Stephen Haberman <hidden>
Date: 2016-06-15 22:45:29

If OLDHEAD was reordered in the todo, and its mapped NEWHEAD was used to set the
ref, commits reordered after OLDHEAD in the todo would should up as un-committed
changes.

Signed-off-by: Stephen Haberman <redacted>
---
 git-rebase--interactive.sh |   15 +--------------
 1 files changed, 1 insertions(+), 14 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 30e4523..c968117 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -376,20 +376,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)" &&
-- 
1.6.0.2

[PATCH] rebase-i-p: only list commits that require rewriting in todo

From: Stephen Haberman <hidden>
Date: 2016-06-15 22:45:29

This is heavily based on Stephan Beyer's git sequencer rewrite of rebase-i-p.

Each commit is still found by rev-list UPSTREAM..HEAD, but a commit is only
included in todo if at least one its parents has been marked for rewriting.

Signed-off-by: Stephen Haberman <redacted>
---
 git-rebase--interactive.sh |   77 +++++++++++++++++++++++++++++--------------
 1 files changed, 52 insertions(+), 25 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 274251f..331cb18 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -579,18 +579,67 @@ first and then run 'git rebase --continue' again."
 				echo $ONTO > "$REWRITTEN"/$c ||
 					die "Could not init rewritten commits"
 			done
+			# No cherry-pick because our first pass is to determine
+			# parents to rewrite and skipping dropped commits would
+			# prematurely end our probe
 			MERGES_OPTION=
 		else
-			MERGES_OPTION=--no-merges
+			MERGES_OPTION="--no-merges --cherry-pick"
 		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 --topo-order \
 			$UPSTREAM...$HEAD | \
-			sed -n "s/^>/pick /p" > "$TODO"
+			sed -n "s/^>//p" | while read shortsha1 rest
+		do
+			if test t != "$PRESERVE_MERGES"
+			then
+				echo "pick $shortsha1 $rest" >> "$TODO"
+			else
+				sha1=$(git rev-parse $shortsha1)
+				preserve=t
+				for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -f2-)
+				do
+					if test -f "$REWRITTEN"/$p
+					then
+						preserve=f
+					fi
+				done
+				if test f = "$preserve"
+				then
+					touch "$REWRITTEN"/$sha1
+					echo "pick $shortsha1 $rest" >> "$TODO"
+				fi
+			fi
+		done
+
+		# Watch for commits that been dropped by --cherry-pick
+		if test t = "$PRESERVE_MERGES"
+		then
+			mkdir "$DROPPED"
+			# Save all non-cherry-picked changes
+			git rev-list $UPSTREAM...$HEAD --left-right --cherry-pick | \
+				sed -n "s/^>//p" > "$DOTEST"/not-cherry-picks
+			# Now all commits and note which ones are missing in
+			# not-cherry-picks and hence being dropped
+			git rev-list $UPSTREAM...$HEAD --left-right | \
+ 				sed -n "s/^>//p" | while read rev
+			do
+				if test -f "$REWRITTEN"/$rev -a "$(grep "$rev" "$DOTEST"/not-cherry-picks)" = ""
+				then
+					# Use -f2 because if rev-list is telling us 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 it
+					git rev-list --parents -1 $rev | cut -d' ' -f2 > "$DROPPED"/$rev
+					cat "$TODO" | grep -v "${rev:0:7}" > "${TODO}2" ; mv "${TODO}2" "$TODO"
+					rm "$REWRITTEN"/$rev
+				fi
+			done
+		fi
 		test -s "$TODO" || echo noop >> "$TODO"
 		cat >> "$TODO" << EOF
 
@@ -606,28 +655,6 @@ 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"
 
-- 
1.6.0.2

[PATCH] rebase-i-p: fix 'no squashing merges' tripping up non-merges

From: Stephen Haberman <hidden>
Date: 2016-06-15 22:45:29

Also only check out the first parent if this commit if not a squash--if it is a
squash, we want to explicitly ignore the parent and leave the wc as is, as
cherry-pick will apply the squash on top of it.

Signed-off-by: Stephen Haberman <redacted>
---
 git-rebase--interactive.sh |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 23cf7a5..274251f 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -219,15 +219,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"
-- 
1.6.0.2

[PATCH] rebase-i-p: do not include non-first-parent commits touching UPSTREAM

From: Stephen Haberman <hidden>
Date: 2016-06-15 22:45:29

This covers an odd boundary case found by Avi Kivity's script where a branch
coming off of UPSTREAM is merged into HEAD. Initially it show up in
UPSTREAM..HEAD, but technically UPSTREAM is not moving, the rest of head is, so
we should not need to rewrite the merge.

This adds a check saying we can keep `preserve=t` if `p=UPSTREAM`...unless this
is the first first-parent commit in our UPSTREAM..HEAD rev-list, which could
very well point to UPSTREAM, but we still need to consider it as rewritten so we
start pulling in the rest of the UPSTREAM..HEAD commits that point to it.

Signed-off-by: Stephen Haberman <redacted>
---
 git-rebase--interactive.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 331cb18..3821692 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -583,6 +583,7 @@ first and then run 'git rebase --continue' again."
 			# parents to rewrite and skipping dropped commits would
 			# prematurely end our probe
 			MERGES_OPTION=
+			first_after_upstream="$(git rev-list --reverse --first-parent $UPSTREAM..$HEAD | head -n 1)"
 		else
 			MERGES_OPTION="--no-merges --cherry-pick"
 		fi
@@ -603,7 +604,7 @@ first and then run 'git rebase --continue' again."
 				preserve=t
 				for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -f2-)
 				do
-					if test -f "$REWRITTEN"/$p
+					if test -f "$REWRITTEN"/$p -a \( $p != $UPSTREAM -o $sha1 = $first_after_upstream \)
 					then
 						preserve=f
 					fi
-- 
1.6.0.2

[PATCH] rebase-i-p: if todo was reordered use HEAD as the rewritten parent

From: Stephen Haberman <hidden>
Date: 2016-06-15 22:45:29

This seems like the best guess we can make until git sequencer marks are
available. That being said, within the context of re-ordering a commit before
its parent in todo, I think applying it on top of the current commit seems like
a reasonable assumption of what the user intended.

Signed-off-by: Stephen Haberman <redacted>
---
 git-rebase--interactive.sh               |    9 +++++++++
 t/t3411-rebase-preserve-around-merges.sh |    4 ++--
 2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 3821692..1fc4f44 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -194,6 +194,15 @@ pick_one_preserving_merges () {
 		if test -f "$REWRITTEN"/$p
 		then
 			new_p=$(cat "$REWRITTEN"/$p)
+
+			# If the todo reordered commits, and our parent is marked for
+			# rewriting, but hasn't been gotten to yet, assume the user meant to
+			# drop it on top of the current HEAD
+			if test -z "$new_p"
+			then
+				new_p=$(git rev-parse HEAD)
+			fi
+
 			test $p != $new_p && fast_forward=f
 			case "$new_parents" in
 			*$new_p*)
diff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh
index b3973c9..dfad5dd 100644
--- a/t/t3411-rebase-preserve-around-merges.sh
+++ b/t/t3411-rebase-preserve-around-merges.sh
@@ -80,7 +80,7 @@ test_expect_success 'setup' '
 #       \        /
 #        -- C1 --
 #
-test_expect_failure 'squash F1 into D1' '
+test_expect_success 'squash F1 into D1' '
 	FAKE_LINES="1 squash 3 2" git rebase -i -p B1 &&
 	test "$(git rev-parse HEAD^2)" = "$(git rev-parse branch)" &&
 	test "$(git rev-parse HEAD~2)" = "$(git rev-parse B1)" &&
@@ -99,7 +99,7 @@ test_expect_failure 'squash F1 into D1' '
 #
 # And rebase G1..M1 onto E2
 
-test_expect_failure 'rebase two levels of merge' '
+test_expect_success 'rebase two levels of merge' '
 	git checkout -b branch2 A1 &&
 	touch g &&
 	git add g &&
-- 
1.6.0.2

Re: [PATCH] rebase-i-p: only list commits that require rewriting in todo

From: Jeff King <hidden>
Date: 2016-06-15 22:45:30

On Wed, Oct 15, 2008 at 02:44:38AM -0500, Stephen Haberman wrote:
+					cat "$TODO" | grep -v "${rev:0:7}" > "${TODO}2" ; mv "${TODO}2" "$TODO"
Substring expansion (like ${rev:0:7}) is not portable. At least it
doesn't work on FreeBSD /bin/sh, and "it's not even in POSIX", I
believe.

-Peff

Re: [PATCH] rebase-i-p: delay saving current-commit to REWRITTEN if squashing

From: Jeff King <hidden>
Date: 2016-06-15 22:45:31

On Wed, Oct 15, 2008 at 02:44:36AM -0500, Stephen Haberman wrote:
+		if [ "$fast_forward" == "t" ]
This one even fails on my Linux box. :) "==" is a bash-ism.

-Peff

Re: [PATCH] rebase-i-p: delay saving current-commit to REWRITTEN if squashing

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:45:31

Hi,

On Wed, 22 Oct 2008, Jeff King wrote:
On Wed, Oct 15, 2008 at 02:44:36AM -0500, Stephen Haberman wrote:
quoted
+		if [ "$fast_forward" == "t" ]
This one even fails on my Linux box. :) "==" is a bash-ism.
Did we not also prefer "test" to "["?

Ciao,
Dscho

Re: [PATCH] rebase-i-p: delay saving current-commit to REWRITTEN if squashing

From: Fredrik Skolmli <hidden>
Date: 2016-06-15 22:45:31

On Wed, Oct 22, 2008 at 05:21:53PM +0200, Johannes Schindelin wrote:
Hi,

On Wed, 22 Oct 2008, Jeff King wrote:
quoted
On Wed, Oct 15, 2008 at 02:44:36AM -0500, Stephen Haberman wrote:
quoted
+		if [ "$fast_forward" == "t" ]
This one even fails on my Linux box. :) "==" is a bash-ism.
Did we not also prefer "test" to "["?
We did.

Documentation/CodingGuidelines, line 51:
    - We prefer "test" over "[ ... ]".

-- 
Kind regards,
Fredrik Skolmli
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help