threads / patch / 12805

patchNew tests to check rebase with preserve merges

Subject: [PATCH] New tests to check rebase with preserve merges

## tl;dr

19 messages between Mar 22, 2008 and Mar 22, 2008. Diffs are folded; open one to read it.

replies: 18people: 3as markdown or json

Jörg Sommer· Mar 22, 2008, 01:19 UTC · lore

[PATCH] rebase with preserve merges should not show merged commits

The current version of git-rebase--interactive shows the user the commits coming from a merge.

M---A---B
 \       \
  o---o---+---o branch

Rebasing branch on M with preserve merges gives the commits A and B. But if you mark them for editing or remove them the rebase fails. You must keep them as they are. It's useless to bother the user with these commits and might lead to mistakes.

Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
---
 git-rebase--interactive.sh |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

Show changes to git-rebase--interactive.sh +3 −1

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 8aa7371..3879841 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -162,6 +162,8 @@ pick_one_preserving_merges () {
 				new_parents="$new_parents $new_p"
 				;;
 			esac
+		else
+			new_parents="$new_parents $p"
 		fi
 	done
 	case $fast_forward in
@@ -523,7 +525,7 @@ do
 		SHORTONTO=$(git rev-parse --short $ONTO)
 		git rev-list $MERGES_OPTION --pretty=oneline --abbrev-commit \
 			--abbrev=7 --reverse --left-right --cherry-pick \
-			$UPSTREAM...$HEAD | \
+			--first-parent $UPSTREAM...$HEAD | \
 			sed -n "s/^>/pick /p" > "$TODO"
 		cat >> "$TODO" << EOF

-- 
1.5.4.4

Jörg Sommer· Mar 22, 2008, 01:19 UTC · re: Jörg Sommer · lore

[PATCH] Check for non‐foreign commits in rebase-interactive test

Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
---
 t/t3404-rebase-interactive.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Show changes to t/t3404-rebase-interactive.sh +2 −2

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9cf873f..7d1e469 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -185,7 +185,7 @@ test_expect_success 'retain authorship when squashing' '
 
 test_expect_success '-p handles "no changes" gracefully' '
 	HEAD=$(git rev-parse HEAD) &&
-	git rebase -i -p HEAD^ &&
+	EXPECT_COUNT=1 git rebase -i -p HEAD^ &&
 	test $HEAD = $(git rev-parse HEAD)
 '
 
@@ -205,7 +205,7 @@ test_expect_success 'preserve merges with -p' '
 	test_tick &&
 	git commit -m K file1 &&
 	test_tick &&
-	git rebase -i -p --onto branch1 master &&
+	EXPECT_COUNT=3 git rebase -i -p --onto branch1 master &&
 	test $(git rev-parse HEAD^^2) = $(git rev-parse to-be-preserved) &&
 	test $(git rev-parse HEAD~3) = $(git rev-parse branch1) &&
 	test $(git show HEAD:file1) = C &&

-- 
1.5.4.4

Jörg Sommer· Mar 22, 2008, 01:19 UTC · re: Jörg Sommer · lore

[PATCH] Handle fast forward correctly in rebase with preserve merges

Rebase-interactive with preserve merges does fast forward commits while the parent of the old commit is not the parent of the new commit. If the parent of the changed commit is not touched, e.g. has no entry in the REWRITTEN database, a fast forward happens. With these commits “A---B---C” and rebase “A---C---B” would do a fast forward for C which leads to an incorrect result.

The fast forward is also not realised, i.e. the HEAD is not updated.

After all is done, it was assumed that the new head is the rewritten old head. But if the old head was applied before current head—as in the example above—the commits after the rewritten old head are lost.

Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
---
 git-rebase--interactive.sh |   35 ++++++++++++++++++-----------------
 1 files changed, 18 insertions(+), 17 deletions(-)

Show changes to git-rebase--interactive.sh +18 −17

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 3879841..04fe3bf 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -144,6 +144,7 @@ pick_one_preserving_merges () {
 		die "Cannot write current commit's replacement sha1"
 	fi
 
+	current_sha1=$(git rev-parse --verify HEAD)
 	# rewrite parents; if none were rewritten, we can fast-forward.
 	fast_forward=t
 	preserve=t
@@ -166,18 +167,31 @@ pick_one_preserving_merges () {
 			new_parents="$new_parents $p"
 		fi
 	done
+
+	# Don't do a fast forward, if current commit is not the parent of
+	# the new commit
+	case "$new_parents" in
+	""|" $current_sha1"*)
+		;;
+	*)
+		fast_forward=f
+		;;
+	esac
+
 	case $fast_forward in
 	t)
 		output warn "Fast forward to $sha1"
 		test $preserve = f || echo $sha1 > "$REWRITTEN"/$sha1
+		output git reset --hard $sha1
+		if test "a$1" = a-n
+		then
+			output git reset --soft $current_sha1
+		fi
 		;;
 	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"
 
 		echo $sha1 > "$DOTEST"/current-commit
 		case "$new_parents" in
@@ -330,20 +344,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.5.4.4

Jörg Sommer· Mar 22, 2008, 01:19 UTC · re: Jörg Sommer · lore

Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
---
 t/t3404-rebase-interactive.sh |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

Show changes to t/t3404-rebase-interactive.sh +27 −0

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 7d1e469..50974f0 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -212,6 +212,33 @@ test_expect_success 'preserve merges with -p' '
 	test $(git show HEAD~2:file1) = B
 '
 
+test_expect_success 'preserve merges with -p (case 2)' '
+	test_tick &&
+	EXPECT_COUNT=3 FAKE_LINES="1 3 2" git rebase -v -i -p branch1 &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse to-be-preserved) &&
+	test $(git rev-parse HEAD~3) = $(git rev-parse branch1) &&
+	test $(git show HEAD~2:file1) = B &&
+	test $(git show HEAD~1:file1) = C
+'
+
+test_expect_success 'preserve merges with -p (case 3)' '
+	test_tick &&
+	EXPECT_COUNT=3 FAKE_LINES="3 1 2" git rebase -i -p branch1 &&
+	test $(git rev-parse HEAD~2^2) = $(git rev-parse to-be-preserved) &&
+	test $(git rev-parse HEAD~3) = $(git rev-parse branch1) &&
+	test $(git show HEAD~1:file1) = B &&
+	test $(git show HEAD:file1) = C
+'
+
+test_expect_success 'preserve merges really uses fast forward' '
+	head=$(git rev-parse HEAD) &&
+	test_tick &&
+	EXPECT_COUNT=3 git rebase -v -i -p branch1 2>pm-ff-err &&
+	cat pm-ff-err &&
+	test $(grep "^Fast forward" pm-ff-err | wc -l) -eq 3 &&
+	test $(git rev-parse HEAD) = $head
+'
+
 test_expect_success '--continue tries to commit' '
 	test_tick &&
 	! git rebase -i --onto new-branch1 HEAD^ &&

-- 
1.5.4.4

Johannes Schindelin· Mar 22, 2008, 01:33 UTC · re: Jörg Sommer · lore

Re: [PATCH] rebase with preserve merges should not show merged commits

Hi,

On Sat, 22 Mar 2008, Jörg Sommer wrote:

Show 11 quoted lines

> The current version of git-rebase--interactive shows the user the 
> commits coming from a merge.
> 
> M---A---B
>  \       \
>   o---o---+---o branch
> 
> Rebasing branch on M with preserve merges gives the commits A and B. But 
> if you mark them for editing or remove them the rebase fails. You must 
> keep them as they are. It's useless to bother the user with these 
> commits and might lead to mistakes.

I don't understand. Rebasing with "rebase --onto <something else> M" _should_ show A and B.

Besides, I think that this would break exactly that case:

Show 6 quoted lines

> @@ -523,7 +525,7 @@ do
>  		SHORTONTO=$(git rev-parse --short $ONTO)
>  		git rev-list $MERGES_OPTION --pretty=oneline --abbrev-commit \
>  			--abbrev=7 --reverse --left-right --cherry-pick \
> -			$UPSTREAM...$HEAD | \
> +			--first-parent $UPSTREAM...$HEAD | \

If I am not mistaken, you now mark A and B to be _not_ in the list of commits all of a sudden, even if A and B _are_ reachable from "branch", but not from "M".

So I think this is exactly one of the cases which made me unsure if your expectation was always right.

IOW I think this is _very_ easy to get wrong, and needs careful thought.

Ciao, Dscho

Jörg Sommer· Mar 22, 2008, 09:43 UTC · re: Johannes Schindelin · lore

Re: [PATCH] rebase with preserve merges should not show merged commits

Hallo,

Johannes Schindelin schrieb am Sat 22. Mar, 02:33 (+0100):

Show 16 quoted lines

> On Sat, 22 Mar 2008, Jörg Sommer wrote:
> 
> > The current version of git-rebase--interactive shows the user the 
> > commits coming from a merge.
> > 
> > M---A---B
> >  \       \
> >   o---o---+---o branch
> > 
> > Rebasing branch on M with preserve merges gives the commits A and B. But 
> > if you mark them for editing or remove them the rebase fails. You must 
> > keep them as they are. It's useless to bother the user with these 
> > commits and might lead to mistakes.
> 
> I don't understand.  Rebasing with "rebase --onto <something else> M" 
> _should_ show A and B.

But IMO not “rebase --onto <something else> --preserve-merges -i M”.

Schöne Grüße, Jörg.

-- 
Geld allein macht nicht glücklich, aber es ist besser in einem Taxi zu
weinen, als in der Straßenbahn.
	       	   	       			(Marcel Reich‐Ranicki)

Johannes Schindelin· Mar 22, 2008, 11:22 UTC · re: Jörg Sommer · lore

Re: [PATCH] rebase with preserve merges should not show merged commits

Hi,

On Sat, 22 Mar 2008, Jörg Sommer wrote:

Show 19 quoted lines

> Johannes Schindelin schrieb am Sat 22. Mar, 02:33 (+0100):
> > On Sat, 22 Mar 2008, Jörg Sommer wrote:
> > 
> > > The current version of git-rebase--interactive shows the user the 
> > > commits coming from a merge.
> > > 
> > > M---A---B
> > >  \       \
> > >   o---o---+---o branch
> > > 
> > > Rebasing branch on M with preserve merges gives the commits A and B. But 
> > > if you mark them for editing or remove them the rebase fails. You must 
> > > keep them as they are. It's useless to bother the user with these 
> > > commits and might lead to mistakes.
> > 
> > I don't understand.  Rebasing with "rebase --onto <something else> M" 
> > _should_ show A and B.
> 
> But IMO not “rebase --onto <something else> --preserve-merges -i M”.

Umm, yes it should.

You are asking to transplant everything up to and including M onto something else.

Ciao, Dscho

Björn Steinbrink· Mar 22, 2008, 01:52 UTC · re: Jörg Sommer · lore

Re: [PATCH] rebase with preserve merges should not show merged commits

On 2008.03.22 02:19:42 +0100, Jörg Sommer wrote:

Show 11 quoted lines

> The current version of git-rebase--interactive shows the user the commits
> coming from a merge.
> 
> M---A---B
>  \       \
>   o---o---+---o branch
> 
> Rebasing branch on M with preserve merges gives the commits A and B. But
> if you mark them for editing or remove them the rebase fails. You must
> keep them as they are. It's useless to bother the user with these commits
> and might lead to mistakes.

Uhm, why do you completely remove the possibility to edit A instead of fixing the code so that the editing actually works?

Björn

Jörg Sommer· Mar 22, 2008, 09:40 UTC · re: Björn Steinbrink · lore

Re: [PATCH] rebase with preserve merges should not show merged commits

Hallo Björn,

Björn Steinbrink schrieb am Sat 22. Mar, 02:52 (+0100):

Show 15 quoted lines

> On 2008.03.22 02:19:42 +0100, Jörg Sommer wrote:
> > The current version of git-rebase--interactive shows the user the commits
> > coming from a merge.
> > 
> > M---A---B
> >  \       \
> >   o---o---+---o branch
> > 
> > Rebasing branch on M with preserve merges gives the commits A and B. But
> > if you mark them for editing or remove them the rebase fails. You must
> > keep them as they are. It's useless to bother the user with these commits
> > and might lead to mistakes.
> 
> Uhm, why do you completely remove the possibility to edit A instead of
> fixing the code so that the editing actually works?

Because I didn't see why it's useful to edit A and create A' and merge in A again, later.

M---A---B
 \       \
  C---D---+---o branch

M---A--------------B
 \                  \
  C---B'---D'---A'---+---o branch

Bye, Jörg.

-- 
Viele Leute glauben, dass sie denken, wenn sie lediglich
ihre Vorurteile neu ordnen.

Björn Steinbrink· Mar 22, 2008, 12:37 UTC · re: Jörg Sommer · lore

Re: [PATCH] rebase with preserve merges should not show merged commits

On 2008.03.22 10:40:51 +0100, Jörg Sommer wrote:

Show 29 quoted lines

> Hallo Björn,
> 
> Björn Steinbrink schrieb am Sat 22. Mar, 02:52 (+0100):
> > On 2008.03.22 02:19:42 +0100, Jörg Sommer wrote:
> > > The current version of git-rebase--interactive shows the user the commits
> > > coming from a merge.
> > > 
> > > M---A---B
> > >  \       \
> > >   o---o---+---o branch
> > > 
> > > Rebasing branch on M with preserve merges gives the commits A and B. But
> > > if you mark them for editing or remove them the rebase fails. You must
> > > keep them as they are. It's useless to bother the user with these commits
> > > and might lead to mistakes.
> > 
> > Uhm, why do you completely remove the possibility to edit A instead of
> > fixing the code so that the editing actually works?
> 
> Because I didn't see why it's useful to edit A and create A' and merge in
> A again, later.
> 
> M---A---B
>  \       \
>   C---D---+---o branch
> 
> M---A--------------B
>  \                  \
>   C---B'---D'---A'---+---o branch

Hm? Why do you have A' and B' on the other side of the merge? Using -p means that you deliberately _disable_ the linearization. The structure of the history is not supposed to change at all. You're just editing A and the merge should pull A(edited) and B in.

Björn

Jörg Sommer· Mar 22, 2008, 14:06 UTC · re: Björn Steinbrink · lore

Re: [PATCH] rebase with preserve merges should not show merged commits

Hallo Björn,

Björn Steinbrink schrieb am Sat 22. Mar, 02:52 (+0100):

Show 14 quoted lines

> On 2008.03.22 02:19:42 +0100, Jörg Sommer wrote:
> > The current version of git-rebase--interactive shows the user the commits
> > coming from a merge.
> > 
> > M---A---B
> >  \       \
> >   o---o---+---o branch
> > 
> > Rebasing branch on M with preserve merges gives the commits A and B. But
> > if you mark them for editing or remove them the rebase fails. You must
> > keep them as they are. It's useless to bother the user with these commits
> > and might lead to mistakes.
> 
> Uhm, why do you completely remove the possibility to edit A

Ahh, now I see what you've tried to say. I did add the option --first-parent for rebase interactive *without* preserve merges, too. I'll update my patch.

Bye, Jörg.

-- 
“Science is the game we play with God to find out what his rules are.”

Björn Steinbrink· Mar 22, 2008, 15:12 UTC · re: Jörg Sommer · lore

Re: [PATCH] rebase with preserve merges should not show merged commits

On 2008.03.22 15:06:48 +0100, Jörg Sommer wrote:

Show 19 quoted lines

> Björn Steinbrink schrieb am Sat 22. Mar, 02:52 (+0100):
> > On 2008.03.22 02:19:42 +0100, Jörg Sommer wrote:
> > > The current version of git-rebase--interactive shows the user the commits
> > > coming from a merge.
> > > 
> > > M---A---B
> > >  \       \
> > >   o---o---+---o branch
> > > 
> > > Rebasing branch on M with preserve merges gives the commits A and B. But
> > > if you mark them for editing or remove them the rebase fails. You must
> > > keep them as they are. It's useless to bother the user with these commits
> > > and might lead to mistakes.
> > 
> > Uhm, why do you completely remove the possibility to edit A
> 
> Ahh, now I see what you've tried to say. I did add the option
> --first-parent for rebase interactive *without* preserve merges, too.
> I'll update my patch.

I didn't even look at it closely enough to notice that. --preserve-merges preserves the structure of the history. You seem to interpret it as to preserve the merges against the original parents, except for the first one, and that's simply not what it's meant to do. I can see how that might be useful, but you'd have to add that as an additional mode of operation, and not break the normal one.

Björn

Jörg Sommer· Mar 22, 2008, 15:37 UTC · re: Björn Steinbrink · lore

Re: [PATCH] rebase with preserve merges should not show merged commits

Hallo Björn,

Björn Steinbrink schrieb am Sat 22. Mar, 16:12 (+0100):

Show 25 quoted lines

> On 2008.03.22 15:06:48 +0100, Jörg Sommer wrote:
> > Björn Steinbrink schrieb am Sat 22. Mar, 02:52 (+0100):
> > > On 2008.03.22 02:19:42 +0100, Jörg Sommer wrote:
> > > > The current version of git-rebase--interactive shows the user the commits
> > > > coming from a merge.
> > > > 
> > > > M---A---B
> > > >  \       \
> > > >   o---o---+---o branch
> > > > 
> > > > Rebasing branch on M with preserve merges gives the commits A and B. But
> > > > if you mark them for editing or remove them the rebase fails. You must
> > > > keep them as they are. It's useless to bother the user with these commits
> > > > and might lead to mistakes.
> > > 
> > > Uhm, why do you completely remove the possibility to edit A
> > 
> > Ahh, now I see what you've tried to say. I did add the option
> > --first-parent for rebase interactive *without* preserve merges, too.
> > I'll update my patch.
> 
> I didn't even look at it closely enough to notice that.
> --preserve-merges preserves the structure of the history. You seem to
> interpret it as to preserve the merges against the original parents,
> except for the first one,

Yes, exactly this is my intent.

> and that's simply not what it's meant to do.

That's a pity. So it's meant to be for such cases:

M---A---B
 \       \
  o---C---+---o branch

M---A---B
|    \
|     `-B'
 \       \
  o---C'--+---o branch

> I can see how that might be useful, but you'd have to add that as an
> additional mode of operation, and not break the normal one.

What's the intention of the patch that adds --first-parent somewhere that you've mentioned in the IRC?

I would like to send some tests for bugs I've seen. How do I correctly cleanup after rebase failed? It's necessary to not break following tests.

test_expect_failure '…' '
	…
	git rebase -i …
'

Should I do something like this:

test_expect_failure '…' '
	…
	if !git rebase -i …;
	then
		git rebase --abort;
		false
	fi
'

Bye, Jörg.

-- 
Manchmal denke ich, das sicherste Indiz dafür, daß anderswo im Universum
intelligentes Leben existiert, ist, daß niemand versucht hat, mit uns
Kontakt aufzunehmen.                           (Calvin und Hobbes)

Jörg Sommer· Mar 22, 2008, 14:08 UTC · re: Jörg Sommer · lore

[PATCH v2 1/5] rebase with preserve merges should not show merged commits

The current version of git-rebase--interactive shows the user the commits coming from a merge.

M---A---B
 \       \
  o---o---+---o branch

Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
---
 git-rebase--interactive.sh |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

Show changes to git-rebase--interactive.sh +3 −1

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 8aa7371..e1ce44e 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -162,6 +162,8 @@ pick_one_preserving_merges () {
 				new_parents="$new_parents $new_p"
 				;;
 			esac
+		else
+			new_parents="$new_parents $p"
 		fi
 	done
 	case $fast_forward in
@@ -513,7 +515,7 @@ do
 				echo $ONTO > "$REWRITTEN"/$c ||
 					die "Could not init rewritten commits"
 			done
-			MERGES_OPTION=
+			MERGES_OPTION=--first-parent
 		else
 			MERGES_OPTION=--no-merges
 		fi

-- 
1.5.4.4

Jörg Sommer· Mar 22, 2008, 14:08 UTC · re: Jörg Sommer · lore

[PATCH v2 2/5] New test: no merges without preserve merges

This test checks that no merges are included, if --preserve-merges is not given.

To see a difference between with and without merges add a second commit to the branch to-be-preserved. Otherwise you exchange one merge with one commit, which isn't cognizable with EXPECT_COUNT.

The for loop in the test looks somewhat strange, but I didn't saw a different way (than || exit 1) to make the test fail if an inner test fails. Recall: The exit code of a for loop is the exit code of the last command in the last pass, i.e. “for a in 1 2; do test $a != 1; do” returns success.

Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
---
 t/t3404-rebase-interactive.sh |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

This patch must be applied after the first patch that fixes rebase, because it triggers a bug.

Show changes to t/t3404-rebase-interactive.sh +15 −0

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9cf873f..8de1f21 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -195,6 +195,9 @@ test_expect_success 'preserve merges with -p' '
 	git add unrelated-file &&
 	test_tick &&
 	git commit -m "unrelated" &&
+	echo 2 > unrelated-file &&
+	test_tick &&
+	git commit -m "second unrelated commit" unrelated-file &&
 	git checkout -b to-be-rebased master &&
 	echo B > file1 &&
 	test_tick &&
@@ -212,6 +215,18 @@ test_expect_success 'preserve merges with -p' '
 	test $(git show HEAD~2:file1) = B
 '
 
+test_expect_success 'no merges without preserve merges' '
+	head=$(git rev-parse HEAD) &&
+	test_tick &&
+	EXPECT_COUNT=4 git rebase -i branch1 &&
+	test $(git rev-parse HEAD) != $head &&
+        for i in 0 1 2 3
+	do
+		test $? -eq 0 &&
+		test "$(git rev-list --parents -1 HEAD~$i | tr -dc " ")" = " "
+	done
+'
+
 test_expect_success '--continue tries to commit' '
 	test_tick &&
 	! git rebase -i --onto new-branch1 HEAD^ &&

-- 
1.5.4.4

Jörg Sommer· Mar 22, 2008, 14:08 UTC · re: Jörg Sommer · lore

[PATCH v2 3/5] Check for non‐foreign commits in rebase-interactive test

Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
---
 t/t3404-rebase-interactive.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Show changes to t/t3404-rebase-interactive.sh +2 −2

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8de1f21..8a801a0 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -185,7 +185,7 @@ test_expect_success 'retain authorship when squashing' '
 
 test_expect_success '-p handles "no changes" gracefully' '
 	HEAD=$(git rev-parse HEAD) &&
-	git rebase -i -p HEAD^ &&
+	EXPECT_COUNT=1 git rebase -i -p HEAD^ &&
 	test $HEAD = $(git rev-parse HEAD)
 '
 
@@ -208,7 +208,7 @@ test_expect_success 'preserve merges with -p' '
 	test_tick &&
 	git commit -m K file1 &&
 	test_tick &&
-	git rebase -i -p --onto branch1 master &&
+	EXPECT_COUNT=3 git rebase -i -p --onto branch1 master &&
 	test $(git rev-parse HEAD^^2) = $(git rev-parse to-be-preserved) &&
 	test $(git rev-parse HEAD~3) = $(git rev-parse branch1) &&
 	test $(git show HEAD:file1) = C &&

-- 
1.5.4.4

Jörg Sommer· Mar 22, 2008, 14:08 UTC · re: Jörg Sommer · lore

[PATCH v2 4/5] Handle fast forward correctly in rebase with preserve merges

The fast forward is also not realised, i.e. the HEAD is not updated.

Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
---
 git-rebase--interactive.sh |   35 ++++++++++++++++++-----------------
 1 files changed, 18 insertions(+), 17 deletions(-)

Show changes to git-rebase--interactive.sh +18 −17

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e1ce44e..8626ef6 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -144,6 +144,7 @@ pick_one_preserving_merges () {
 		die "Cannot write current commit's replacement sha1"
 	fi
 
+	current_sha1=$(git rev-parse --verify HEAD)
 	# rewrite parents; if none were rewritten, we can fast-forward.
 	fast_forward=t
 	preserve=t
@@ -166,18 +167,31 @@ pick_one_preserving_merges () {
 			new_parents="$new_parents $p"
 		fi
 	done
+
+	# Don't do a fast forward, if current commit is not the parent of
+	# the new commit
+	case "$new_parents" in
+	""|" $current_sha1"*)
+		;;
+	*)
+		fast_forward=f
+		;;
+	esac
+
 	case $fast_forward in
 	t)
 		output warn "Fast forward to $sha1"
 		test $preserve = f || echo $sha1 > "$REWRITTEN"/$sha1
+		output git reset --hard $sha1
+		if test "a$1" = a-n
+		then
+			output git reset --soft $current_sha1
+		fi
 		;;
 	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"
 
 		echo $sha1 > "$DOTEST"/current-commit
 		case "$new_parents" in
@@ -330,20 +344,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.5.4.4

Jörg Sommer· Mar 22, 2008, 14:08 UTC · re: Jörg Sommer · lore

[PATCH v2 5/5] New tests to check rebase with preserve merges

Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
---
 t/t3404-rebase-interactive.sh |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

Show changes to t/t3404-rebase-interactive.sh +27 −0

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8a801a0..2172065 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -215,6 +215,33 @@ test_expect_success 'preserve merges with -p' '
 	test $(git show HEAD~2:file1) = B
 '
 
+test_expect_success 'preserve merges with -p (case 2)' '
+	test_tick &&
+	EXPECT_COUNT=3 FAKE_LINES="1 3 2" git rebase -v -i -p branch1 &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse to-be-preserved) &&
+	test $(git rev-parse HEAD~3) = $(git rev-parse branch1) &&
+	test $(git show HEAD~2:file1) = B &&
+	test $(git show HEAD~1:file1) = C
+'
+
+test_expect_success 'preserve merges with -p (case 3)' '
+	test_tick &&
+	EXPECT_COUNT=3 FAKE_LINES="3 1 2" git rebase -i -p branch1 &&
+	test $(git rev-parse HEAD~2^2) = $(git rev-parse to-be-preserved) &&
+	test $(git rev-parse HEAD~3) = $(git rev-parse branch1) &&
+	test $(git show HEAD~1:file1) = B &&
+	test $(git show HEAD:file1) = C
+'
+
+test_expect_success 'preserve merges really uses fast forward' '
+	head=$(git rev-parse HEAD) &&
+	test_tick &&
+	EXPECT_COUNT=3 git rebase -v -i -p branch1 2>pm-ff-err &&
+	cat pm-ff-err &&
+	test $(grep "^Fast forward" pm-ff-err | wc -l) -eq 3 &&
+	test $(git rev-parse HEAD) = $head
+'
+
 test_expect_success 'no merges without preserve merges' '
 	head=$(git rev-parse HEAD) &&
 	test_tick &&

-- 
1.5.4.4

Johannes Schindelin· Mar 22, 2008, 14:46 UTC · re: Jörg Sommer · lore

Re: [PATCH v2 1/5] rebase with preserve merges should not show merged commits

Hi,

On Sat, 22 Mar 2008, Jörg Sommer wrote:

Show 11 quoted lines

> The current version of git-rebase--interactive shows the user the 
> commits coming from a merge.
> 
> M---A---B
>  \       \
>   o---o---+---o branch
> 
> Rebasing branch on M with preserve merges gives the commits A and B. But 
> if you mark them for editing or remove them the rebase fails. You must 
> keep them as they are. It's useless to bother the user with these 
> commits and might lead to mistakes.

It is not useless. It's just that you seemed to have found buggy behaviour. I am _totally_ opposed to your patch without even reading more than the commit message.

Ciao, Dscho