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

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

[Jörg Sommer]

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(-)

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

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

[Jörg Sommer]

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

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

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

[Jörg Sommer]

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(-)

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

[PATCH] New tests to check rebase with preserve merges

[Jörg Sommer]

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

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1236 bytes --]

Hi,

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.

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

> @@ -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

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

[Jörg Sommer]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

Hallo,

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”.

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)

[-- Attachment #2: Digital signature http://en.wikipedia.org/wiki/OpenPGP --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 919 bytes --]

Hi,

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

> 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

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

[Björn Steinbrink]

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?

Björn

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

[Jörg Sommer]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

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

Bye, Jörg.
-- 
Viele Leute glauben, dass sie denken, wenn sie lediglich
ihre Vorurteile neu ordnen.

[-- Attachment #2: Digital signature http://en.wikipedia.org/wiki/OpenPGP --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Björn Steinbrink]

On 2008.03.22 10:40:51 +0100, Jörg Sommer wrote:
> 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

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

[Jörg Sommer]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

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

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.”

[-- Attachment #2: Digital signature http://en.wikipedia.org/wiki/OpenPGP --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Björn Steinbrink]

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, 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

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

[Jörg Sommer]

[-- Attachment #1: Type: text/plain, Size: 2311 bytes --]

Hallo Björn,

Björn Steinbrink schrieb am Sat 22. Mar, 16:12 (+0100):
> 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)

[-- Attachment #2: Digital signature http://en.wikipedia.org/wiki/OpenPGP --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Jörg Sommer]

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(-)

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

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

[Jörg Sommer]

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.

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

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

[Jörg Sommer]

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

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

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

[Jörg Sommer]

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(-)

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

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

[Jörg Sommer]

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

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 641 bytes --]

Hi,

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.

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