threads / patch / 12767

patchNew test for preserve merges and squash

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

## tl;dr

8 messages between Mar 20, 2008 and Mar 21, 2008. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Jörg Sommer· Mar 20, 2008, 00:03 UTC · lore
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?

Show changes to t/t3404-rebase-interactive.sh +20 −0
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
Junio C Hamano· Mar 20, 2008, 00:28 UTC · re: Jörg Sommer · lore

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

Jörg Sommer <joerg@alea.gnuu.de> writes:
Show 7 quoted lines
> 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 ;-).
Show 18 quoted lines
> +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.

Jörg Sommer· Mar 20, 2008, 10:18 UTC · re: Junio C Hamano · lore

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

Hi,
Junio C Hamano schrieb am Wed 19. Mar, 17:28 (-0700):
Show 11 quoted lines
> 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.
Johannes Schindelin· Mar 20, 2008, 11:09 UTC · re: Jörg Sommer · lore

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

Hi,
On Thu, 20 Mar 2008, Jörg Sommer wrote:
Show 15 quoted lines
> 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'.

Show 5 quoted lines
> > 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

Jörg Sommer· Mar 20, 2008, 13:01 UTC · re: Johannes Schindelin · lore

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

Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
---
 t/t3404-rebase-interactive.sh |   52 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)
Show 8 quoted lines
> > > 
> > > 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:
Show changes to t/t3404-rebase-interactive.sh +52 −0
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
Johannes Schindelin· Mar 20, 2008, 23:46 UTC · re: Jörg Sommer · lore

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

Hi,
On Thu, 20 Mar 2008, Jörg Sommer wrote:
Show 35 quoted lines
> 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

Jörg Sommer· Mar 21, 2008, 12:56 UTC · re: Johannes Schindelin · lore

Why rebase with preserve merges asks for merged commits

Hallo Johannes,
Johannes Schindelin schrieb am Fri 21. Mar, 00:46 (+0100):
Show 41 quoted lines
> 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.
Johannes Schindelin· Mar 21, 2008, 13:14 UTC · re: Jörg Sommer · lore

Re: Why rebase with preserve merges asks for merged commits

Hi,
On Fri, 21 Mar 2008, Jörg Sommer wrote:
Show 16 quoted lines
> 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.

← back to recent threads