make test Unexpected passes

make test Unexpected passes

[Ramsay Jones]

Hi Ben, Junio,

Tonight, the testsuite passed with a couple of 'unexpected passes', viz:

$ tail -17 ptest-out
[13:24:29]
All tests successful.

Test Summary Report
-------------------
t3421-rebase-topology-linear.sh                  (Wstat: 0 Tests: 76 Failed: 0)
  TODO passed:   50, 54
t6036-recursive-corner-cases.sh                  (Wstat: 0 Tests: 22 Failed: 0)
  TODO passed:   11
Files=746, Tests=13515, 445 wallclock secs ( 3.83 usr  0.61 sys + 52.78 cusr 27.89 csys = 85.11 CPU)
Result: PASS
make clean-except-prove-cache
make[2]: Entering directory `/home/ramsay/git/t'
rm -f -r 'trash directory'.* 'test-results'
rm -f -r valgrind/bin
make[2]: Leaving directory `/home/ramsay/git/t'
make[1]: Leaving directory `/home/ramsay/git/t'
$ 

In the first case, t3421-*.sh, git bisect fingered commit f32ec670
("git-rebase--merge: don't include absent parent as a base", 20-04-2016).

In the second case, t6036-*.sh, git bisect fingered commit b61f9d6e
("ll-merge: use a longer conflict marker for internal merge", 14-04-2016).

I won't have any time tonight to look into this any further (are these
false positives?), so I thought I would just make sure you were aware
of these 'unexpected passes'.

ATB,
Ramsay Jones

Re: make test Unexpected passes

[Elijah Newren]

On Fri, Apr 22, 2016 at 1:05 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
> Hi Ben, Junio,
>
> In the second case, t6036-*.sh, git bisect fingered commit b61f9d6e
> ("ll-merge: use a longer conflict marker for internal merge", 14-04-2016).

Yeah, the t6036 testcase 'git detects conflict w/
criss-cross+contrived resolution' could be made to pass by tweaking
the conflict markers.  In fact, any tweak would make it appear to
pass, but the test could be updated to still fail by updating the
contrived resolution of a previous merge to use the newer conflict
marker style as well.

The test is kind of fragile in this way, and is really there just to
document this slightly weird and never seen in practice corner case.
I don't think we'll ever fix the underlying "problem"; not even sure
if it's possible without fundamentally re-thinking our merge strategy
altogether.  I just thought that when I was writing all those tests
that documenting this particular behavior as a testcase had some
value, but if the conflict markers are going to be updated
periodically, then the cost of the test may outweigh its value and we
should just toss this one test from that file.

Elijah

Re: make test Unexpected passes

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> Yeah, the t6036 testcase 'git detects conflict w/
> criss-cross+contrived resolution' could be made to pass by tweaking
> the conflict markers.  In fact, any tweak would make it appear to
> pass, but the test could be updated to still fail by updating the
> contrived resolution of a previous merge to use the newer conflict
> marker style as well.

Isn't what the test expects bogus in the first place?  I'd suggest
removing the test as "pointless waste of resource".

Comments?

-- >8 --
Subject: [PATCH] t6036: remove pointless test that expects failure

One test in t6036 prepares a file whose contents contain these
lines:

	<<<<<<< Temporary merge branch 1
	C
	=======
	B
	>>>>>>> Temporary merge branch 2

and uses recursive merge strategy to run criss-cross merge with it.

Manual merge resolution by users fundamentally depends on being able
to tell what is the tracked contents and what is the separator lines
added by "git merge" to tell users which block of lines came from
where.  You can deliberately craft a file with lines that resemble
conflict marker lines to make it impossible for the user (here, the
outer merge of merge-recursive also counts as a user of the internal
merge) to tell which part is which, and write a test to demonstrate
that with such a file that "git merge" fundamentally cannot work
with and has to fail on.  It however is pointless and waste of time
and resource to run such a test that asserts the obvious.

In real life, people who do need to track files with such lines that
have <<<< ==== >>>> as their prefixes set the conflict-marker-size
attribute to make sure they will be able to tell between the tracked
lines that happen to begin with these (confusing) prefixes and the
marker lines that are added by "git merge".

Remove the test as pointless waste of resource.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t6036-recursive-corner-cases.sh | 83 ---------------------------------------
 1 file changed, 83 deletions(-)

diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh
index cc1ee6a..c7b0ae6 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -304,89 +304,6 @@ test_expect_success 'git detects conflict merging criss-cross+modify/delete, rev
 	test $(git rev-parse :3:file) = $(git rev-parse B:file)
 '
 
-#
-# criss-cross + modify/modify with very contrived file contents:
-#
-#      B   D
-#      o---o
-#     / \ / \
-#  A o   X   ? F
-#     \ / \ /
-#      o---o
-#      C   E
-#
-#   Commit A: file with contents 'A\n'
-#   Commit B: file with contents 'B\n'
-#   Commit C: file with contents 'C\n'
-#   Commit D: file with contents 'D\n'
-#   Commit E: file with contents:
-#      <<<<<<< Temporary merge branch 1
-#      C
-#      =======
-#      B
-#      >>>>>>> Temporary merge branch 2
-#
-# Now, when we merge commits D & E, does git detect the conflict?
-
-test_expect_success 'setup differently handled merges of content conflict' '
-	git clean -fdqx &&
-	rm -rf .git &&
-	git init &&
-
-	echo A >file &&
-	git add file &&
-	test_tick &&
-	git commit -m A &&
-
-	git branch B &&
-	git checkout -b C &&
-	echo C >file &&
-	git add file &&
-	test_tick &&
-	git commit -m C &&
-
-	git checkout B &&
-	echo B >file &&
-	git add file &&
-	test_tick &&
-	git commit -m B &&
-
-	git checkout B^0 &&
-	test_must_fail git merge C &&
-	echo D >file &&
-	git add file &&
-	test_tick &&
-	git commit -m D &&
-	git tag D &&
-
-	git checkout C^0 &&
-	test_must_fail git merge B &&
-	cat <<EOF >file &&
-<<<<<<< Temporary merge branch 1
-C
-=======
-B
->>>>>>> Temporary merge branch 2
-EOF
-	git add file &&
-	test_tick &&
-	git commit -m E &&
-	git tag E
-'
-
-test_expect_failure 'git detects conflict w/ criss-cross+contrived resolution' '
-	git checkout D^0 &&
-
-	test_must_fail git merge -s recursive E^0 &&
-
-	test 3 -eq $(git ls-files -s | wc -l) &&
-	test 3 -eq $(git ls-files -u | wc -l) &&
-	test 0 -eq $(git ls-files -o | wc -l) &&
-
-	test $(git rev-parse :2:file) = $(git rev-parse D:file) &&
-	test $(git rev-parse :3:file) = $(git rev-parse E:file)
-'
-
 #
 # criss-cross + d/f conflict via add/add:
 #   Commit A: Neither file 'a' nor directory 'a/' exists.

Re: make test Unexpected passes

[Elijah Newren]

On Wed, Apr 27, 2016 at 3:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Isn't what the test expects bogus in the first place?  I'd suggest
> removing the test as "pointless waste of resource".
>
> Comments?
>
> -- >8 --

Yes, toss it; I find your arguments below compelling.

> Manual merge resolution by users fundamentally depends on being able
> to tell what is the tracked contents and what is the separator lines
> added by "git merge" to tell users which block of lines came from
> where.  You can deliberately craft a file with lines that resemble
> conflict marker lines to make it impossible for the user (here, the
> outer merge of merge-recursive also counts as a user of the internal
> merge) to tell which part is which, and write a test to demonstrate
> that with such a file that "git merge" fundamentally cannot work
> with and has to fail on.  It however is pointless and waste of time
> and resource to run such a test that asserts the obvious.
>
> In real life, people who do need to track files with such lines that
> have <<<< ==== >>>> as their prefixes set the conflict-marker-size
> attribute to make sure they will be able to tell between the tracked
> lines that happen to begin with these (confusing) prefixes and the
> marker lines that are added by "git merge".

Re: make test Unexpected passes

[Ben Woosley]

Ramsay Jones <ramsay <at> ramsayjones.plus.com> writes:

> 
> Hi Ben, Junio,
> 
> Tonight, the testsuite passed with a couple of 'unexpected passes', viz:
>
> In the first case, t3421-*.sh, git bisect fingered commit f32ec670
> ("git-rebase--merge: don't include absent parent as a base", 20-04-2016).
>
> ATB,
> Ramsay Jones
> 
 
Yep,

These know breakages:

ok 50 - rebase -m --onto --root
ok 54 - rebase -m without --onto --root with disjoint history

Have to do with rebasing a root/orphan branch with the -m flag,
which defaults to -- merge=recursive, which is the case the patch fixed.

Here are the necessary changes:

--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -253,7 +253,7 @@ test_run_rebase () {
 	"
 }
 test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
 test_run_rebase success -i
 test_run_rebase success -p
 
@@ -268,7 +268,7 @@ test_run_rebase () {
 	"
 }
 test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
 test_run_rebase success -i
 test_run_rebase failure -p
 
-- 
2.8.1.211.g8e54d77

Re: make test Unexpected passes

[Junio C Hamano]

Ben Woosley <ben.woosley@gmail.com> writes:

> These know breakages:
>
> ok 50 - rebase -m --onto --root
> ok 54 - rebase -m without --onto --root with disjoint history
>
> Have to do with rebasing a root/orphan branch with the -m flag,
> which defaults to -- merge=recursive, which is the case the patch fixed.
>
> Here are the necessary changes:
> ...

Thanks, will squash them in and reword the log message accordingly.

    git-rebase--merge: don't include absent parent as a base

    Absent this fix, attempts to rebase an orphan branch using "rebase -m"
    fails with:

        $ git rebase -m ORPHAN_TARGET_BASE
        First, rewinding head to replay your work on top of it...
        ...
    Note the default rebase behavior does not fail:

        $ git rebase ORPHAN_TARGET_BASE
        First, rewinding head to replay your work on top of it...
        Applying: ORPHAN_ROOT_COMMIT_MSG
        Using index info to reconstruct a base tree...

    A few tests were expecting the old behaviour to forbid rebasing such
    a history with "rebase -m", which now need to expect them to succeed.

    Signed-off-by: Ben Woosley <ben.woosley@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>