[PATCH] New test for preserve merges and squash

[PATCH] New test for preserve merges and squash

[Jörg Sommer]

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

The current version of git fails this test. I think it's a bug, because
the patch is what I expect to happen. Or am I wrong?

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 62e65d7..05f3828 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -213,6 +213,26 @@ test_expect_success 'preserve merges with -p' '
 	test $(git show HEAD~2:file1) = B
 '
 
+# This test uses to-be-preserved from the test 'preserve merges with -p'
+test_expect_success 'squash and preserve merges' '
+	test_tick &&
+	git checkout -b squash-and-preserve-merges master &&
+	echo A > file1 &&
+	git commit -m SaPM-1 file1 &&
+	echo B > file1 &&
+	git commit -m SaPM-2 file1 &&
+	git merge to-be-preserved &&
+	echo C > file1 &&
+	git commit -m SaPM-3 file1 &&
+
+        EXPECT_COUNT=4 FAKE_LINES="1 2 squash 4 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 &&
+	test $(git show HEAD~2:file1) = A
+'
+
 test_expect_success '--continue tries to commit' '
 	test_tick &&
 	! git rebase -i --onto new-branch1 HEAD^ &&
-- 
1.5.4.4

Re: [PATCH] New test for preserve merges and squash

[Junio C Hamano]

Jörg Sommer <joerg@alea.gnuu.de> writes:

> Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
> ---
>  t/t3404-rebase-interactive.sh |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
>
> The current version of git fails this test. I think it's a bug, because
> the patch is what I expect to happen. Or am I wrong?

Please mark such tests with test_expect_fail.

And also Cc: the guilty parties.

	$ git-blame master -- git-rebase--interactive.sh
        $ git shortlog -n -s master -- git-rebase--interactive.sh

would tell you who they are ;-).

> +test_expect_success 'squash and preserve merges' '
> +	test_tick &&
> +	git checkout -b squash-and-preserve-merges master &&
> +	echo A > file1 &&
> +	git commit -m SaPM-1 file1 &&
> +	echo B > file1 &&
> +	git commit -m SaPM-2 file1 &&
> +	git merge to-be-preserved &&
> +	echo C > file1 &&
> +	git commit -m SaPM-3 file1 &&
> +
> +        EXPECT_COUNT=4 FAKE_LINES="1 2 squash 4 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 &&
> +	test $(git show HEAD~2:file1) = A
> +'

Please split such a test into two parts, a part that sets things up (which
would succeed), and the part that runs the command it tries to test
("rebase -i" followed by the validation of the result, which you are
expecting to fail).  That way, somebody who is trying to fix the breakage
can stop the test script by inserting "exit" between the two, run the
rebase command by hand to see what is going on, etc. to diagnose the
issue.

Re: [PATCH] New test for preserve merges and squash

[Jörg Sommer]

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

Hi,

Junio C Hamano schrieb am Wed 19. Mar, 17:28 (-0700):
> Jörg Sommer <joerg@alea.gnuu.de> writes:
> 
> > Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
> > ---
> >  t/t3404-rebase-interactive.sh |   20 ++++++++++++++++++++
> >  1 files changed, 20 insertions(+), 0 deletions(-)
> >
> > The current version of git fails this test. I think it's a bug, because
> > the patch is what I expect to happen. Or am I wrong?
> 
> Please mark such tests with test_expect_fail.

No, I expect the test succeeds. Currently, it fails. This might be,
because my expectations are wrong or there's a bug.

> And also Cc: the guilty parties.

Okay, I've added Johannes the author of f09c9b8c5ff to the Cc: list.

Johannes, can you explain why this test fails?

Bye, Jörg.
-- 
> Definiere ‚Demokratie‘ …
… eine Mehrheit beweist einer Minderheit, dass Widerstand zwecklos ist.

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

Re: [PATCH] New test for preserve merges and squash

[Johannes Schindelin]

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

Hi,

On Thu, 20 Mar 2008, Jörg Sommer wrote:

> Junio C Hamano schrieb am Wed 19. Mar, 17:28 (-0700):
> > Jörg Sommer <joerg@alea.gnuu.de> writes:
> > 
> > > Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
> > > ---
> > >  t/t3404-rebase-interactive.sh |   20 ++++++++++++++++++++
> > >  1 files changed, 20 insertions(+), 0 deletions(-)
> > >
> > > The current version of git fails this test. I think it's a bug, 
> > > because the patch is what I expect to happen. Or am I wrong?
> > 
> > Please mark such tests with test_expect_fail.
> 
> No, I expect the test succeeds. Currently, it fails. This might be, 
> because my expectations are wrong or there's a bug.

The definition of "test_expect_fail" is: this should succeed, but needs 
fixin'.

> > And also Cc: the guilty parties.
> 
> Okay, I've added Johannes the author of f09c9b8c5ff to the Cc: list.
> 
> Johannes, can you explain why this test fails?

At the moment, I can't, because I indeed missed your original mail.  Maybe 
I'll find time this afternoon to dive into gmane to find your original 
mail.

Ciao,
Dscho

[PATCH] New test for rebase with preserve merges and squash

[Jörg Sommer]

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

> > > 
> > > Please mark such tests with test_expect_fail.
> > 
> > No, I expect the test succeeds. Currently, it fails. This might be, 
> > because my expectations are wrong or there's a bug.
> 
> The definition of "test_expect_fail" is: this should succeed, but needs 
> fixin'.

Ahh, I didn't know this. Here's a new one:

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 62e65d7..c849415 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -362,4 +362,56 @@ test_expect_success 'rebase with a file named HEAD in worktree' '
 
 '
 
+test_expect_success 'squash and preserve merges' '
+	test_tick &&
+	git checkout -b squash-and-preserve-merges master &&
+	echo A > file1 &&
+	git commit -m SaPM-1 file1 &&
+	echo B > file1 &&
+	git commit -m SaPM-2 file1 &&
+	git merge to-be-preserved &&
+	echo C > file1 &&
+	git commit -m SaPM-3 file1
+'
+
+# This test should fail, because the prompt includes the commit from the
+# merge not only the merge:
+# -> pick 9604163 unrelated
+#    pick 5ef0364 SaPM-1
+#    pick 22aadcf SaPM-2
+#    pick 828f7d8 Merge branch 'to-be-preserved' into squash-and-preserve-merges
+#    pick 2a15a54 SaPM-3
+test_expect_failure 'expect preserve merges shown not commits from merge' '
+        EXPECT_COUNT=4 FAKE_LINES="1 2 squash 4 3" \
+          git rebase -i -p --onto branch1 master ||
+        { git rebase --abort;
+          EXPECT_COUNT=5 FAKE_LINES="1 2 3 squash 5 4" \
+          git rebase -i -p --onto branch1 master;
+          false; }
+'
+
+# The rebase changes nothing: SaPM-3 is still the last commit while it
+# should have been merged with SaMP-2 and the branch is not rebased on
+# branch1
+#
+# % git forest squash-and-preserve-merges
+# ╓─[squash-and-preserve-merges]──SaPM-3
+# ╟ Merge branch 'to-be-preserved' into squash-and-preserve-merges
+# ╠═╗ 
+# ╟ ║ SaPM-2
+# ╟ ║ SaPM-1
+# ║ ╟─[to-be-preserved]──unrelated
+# ╟─║─[master]──E
+# ╠═╝ 
+# ╟ D
+# ╟ C
+# ╟ B
+# ╙─[A]──A
+test_expect_failure 'squash and preserve merges' '
+	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 &&
+	test $(git show HEAD~2:file1) = A
+'
+
 test_done
-- 
1.5.4.4

Re: [PATCH] New test for rebase with preserve merges and squash

[Johannes Schindelin]

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

Hi,

On Thu, 20 Mar 2008, Jörg Sommer wrote:

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 62e65d7..c849415 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -362,4 +362,56 @@ test_expect_success 'rebase with a file named HEAD in worktree' '
>  
>  '
>  
> +test_expect_success 'squash and preserve merges' '
> +	test_tick &&
> +	git checkout -b squash-and-preserve-merges master &&
> +	echo A > file1 &&
> +	git commit -m SaPM-1 file1 &&
> +	echo B > file1 &&
> +	git commit -m SaPM-2 file1 &&
> +	git merge to-be-preserved &&
> +	echo C > file1 &&
> +	git commit -m SaPM-3 file1
> +'
> +
> +# This test should fail, because the prompt includes the commit from the
> +# merge not only the merge:
> +# -> pick 9604163 unrelated
> +#    pick 5ef0364 SaPM-1
> +#    pick 22aadcf SaPM-2
> +#    pick 828f7d8 Merge branch 'to-be-preserved' into squash-and-preserve-merges
> +#    pick 2a15a54 SaPM-3
> +test_expect_failure 'expect preserve merges shown not commits from merge' '
> +        EXPECT_COUNT=4 FAKE_LINES="1 2 squash 4 3" \
> +          git rebase -i -p --onto branch1 master ||
> +        { git rebase --abort;
> +          EXPECT_COUNT=5 FAKE_LINES="1 2 3 squash 5 4" \
> +          git rebase -i -p --onto branch1 master;
> +          false; }
> +'

I'm sorry, but I have to tell you: I do not like that style at all (for 
one, your expect_failure can succeed for all kind of reasons, the exit 
value of git rebase --abort is not even checked).

And I do not understand the message "expect preserve merges shown not 
commits from merge".

Which makes things kinda awkwardly hard for me.

So I have to guess that you think that in this case:

  B---D-E-F-G
 /   ,---'
A---C

a revision range B..G excludes C?  Not so.  "^B ^C G" would, but there is 
no way to specify this with a call to rebase.

So I find it quite logical that your test case fails.

Hth,
Dscho

Why rebase with preserve merges asks for merged commits

[Jörg Sommer]

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

Hallo Johannes,

Johannes Schindelin schrieb am Fri 21. Mar, 00:46 (+0100):
> On Thu, 20 Mar 2008, Jörg Sommer wrote:
> 
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > index 62e65d7..c849415 100755
> > --- a/t/t3404-rebase-interactive.sh
> > +++ b/t/t3404-rebase-interactive.sh
> > @@ -362,4 +362,56 @@ test_expect_success 'rebase with a file named HEAD in worktree' '
> >  
> >  '
> >  
> > +test_expect_success 'squash and preserve merges' '
> > +	test_tick &&
> > +	git checkout -b squash-and-preserve-merges master &&
> > +	echo A > file1 &&
> > +	git commit -m SaPM-1 file1 &&
> > +	echo B > file1 &&
> > +	git commit -m SaPM-2 file1 &&
> > +	git merge to-be-preserved &&
> > +	echo C > file1 &&
> > +	git commit -m SaPM-3 file1
> > +'
> > +
> > +# This test should fail, because the prompt includes the commit from the
> > +# merge not only the merge:
> > +# -> pick 9604163 unrelated
> > +#    pick 5ef0364 SaPM-1
> > +#    pick 22aadcf SaPM-2
> > +#    pick 828f7d8 Merge branch 'to-be-preserved' into squash-and-preserve-merges
> > +#    pick 2a15a54 SaPM-3
> > +test_expect_failure 'expect preserve merges shown not commits from merge' '
> > +        EXPECT_COUNT=4 FAKE_LINES="1 2 squash 4 3" \
> > +          git rebase -i -p --onto branch1 master ||
> > +        { git rebase --abort;
> > +          EXPECT_COUNT=5 FAKE_LINES="1 2 3 squash 5 4" \
> > +          git rebase -i -p --onto branch1 master;
> > +          false; }
> > +'
> 
> I'm sorry, but I have to tell you: I do not like that style at all (for 
> one, your expect_failure can succeed for all kind of reasons, the exit 
> value of git rebase --abort is not even checked).

Yes, it's ugly. So, let me step back. Think of the following situation:

M----------U          to-be-preserved
 \          \
  `--A---B---+---C    squash-and-preserve-merges

When I do a rebase M..C with preserve merges I can decide about all commits
including U which came in with the merge.

U  pick 9604163 unrelated
A  pick 5ef0364 SaPM-1
B  pick 22aadcf SaPM-2
+  pick 828f7d8 Merge branch 'to-be-preserved' into squash-and-preserve-merges
C  pick 2a15a54 SaPM-3

Why I can decide about the commit U from the branch to-be-preserved? I
expect to see not the commit from the merged branches, because they
aren't related to the branch to be rebased.

Let's take a different situation:

M--U1--U2--U3
 \           \
  `---A---B---+---C

A rebase M..C with preserve merges asks you about the commits M..U3, too.
Why? I preserve the merge that pulls in these commmits.

Bye, Jörg.
-- 
Die meisten Menschen wollen lieber durch Lob ruiniert
als durch Kritik gerettet werden.

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

Re: Why rebase with preserve merges asks for merged commits

[Johannes Schindelin]

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

Hi,

On Fri, 21 Mar 2008, Jörg Sommer wrote:

> Think of the following situation:
> 
> M----------U          to-be-preserved
>  \          \
>   `--A---B---+---C    squash-and-preserve-merges
> 
> When I do a rebase M..C with preserve merges I can decide about all commits
> including U which came in with the merge.
> 
> U  pick 9604163 unrelated
> A  pick 5ef0364 SaPM-1
> B  pick 22aadcf SaPM-2
> +  pick 828f7d8 Merge branch 'to-be-preserved' into squash-and-preserve-merges
> C  pick 2a15a54 SaPM-3
> 
> Why I can decide about the commit U from the branch to-be-preserved?

Well, -p obviously does not take the commits (it takes a superset in your 
case) you would expect it to take.  I am not sure if that is always the 
correct thing to assume, though.

In the short run, you can do it manually by

	$ git checkout B
	$ git rebase --onto branch1 M
	$ git merge U
	$ git cherry-pick C

Ciao,
Dscho

P.S.: Oh, BTW, just as in German, you have to put the verb before a 
subject in an English question.