Merge lp:~vila/bzr/537956-parent-loop-incomplete into lp:bzr

[Vincent Ladeuil]

As explained in bug #537956, ParentLoop objects doesn't carry enough information to
implement bzr resolve --take-other.

This patch fixes ParentLoop.format and add tests expected to fail.

This was built on top of https://code.edge.launchpad.net/~vila/bzr/531967-unify-name-conflicts/+merge/21039 pending review.

[Vincent Ladeuil (vila)]

Despite my usage of the pre-requisite branch, the patch is a bit more verbose than I wished.
Apologies to reviewers.

The relevant stuff is in conflicts.py and class TestResolveParentLoop in test_conflicts.py.

The other parts aren't wrong in themselves either, and worth merging anyway.

[lp:~vila/bzr/537956-parent-loop-incomplete]

4639. By Vincent Ladeuil on 2010-03-12

The ParentLoop.format has been updated, fix fallouts.

[Martin Pool (mbp)]

review: Approve

[Vincent Ladeuil (vila)]

Waiting for the pre-requisite branch to land.

[Andrew Bennetts (spiv)]

(following on from the discussion in the pre-requisite branch, as this branch
has an updated version of that test code)

Vincent Ladeuil wrote:
[...]
> + def do_nothing(self):
> + return (None, None, [])
>
> def do_create_file(self):
> return ('file', 'file-id',
> [('add', ('file', 'file-id', 'file', 'trunk content\n'))])
>
> + def do_create_file_a(self):
> + return ('file', 'file-a-id',
> + [('add', ('file', 'file-a-id', 'file', 'file a content\n'))])
> +
> + def check_file_content_a(self):
> + self.assertFileEqual('file a content\n', 'branch/file')
> +
> + def do_create_file_b(self):
> + return ('file', 'file-b-id',
> + [('add', ('file', 'file-b-id', 'file', 'file b content\n'))])
> +
> + def check_file_content_b(self):
> + self.assertFileEqual('file b content\n', 'branch/file')

The repetition here is pretty ugly. I think what would be better is a more
data-driven description of the scenarios.

So, instead of specifying e.g.:

    _actions_base='create_file'

Why not specify the build_snapshot recipe directly in the scenario, and have a
module-global set of canned recipe items to keep it short:

    base_snapshot=[canned['trunk']]

(Note how more complex snapshots with multiple elements are still easy to
assemble, because you specify the whole snapshot list in the scenario.)

I think this would be easier and clearer than your zoo of do_* methods.

I think your check_* methods could be similarly transformed, e.g. to:

   check=[('exists', 'branch/dir1'), ...]

Let's look at the bigger picture. I think that all conflict scenarios have this
shape:

-----
Define scenario template "S" that describes:
  * how to create 'base' tree (and revision)
  * how to create 'left' tree (and revision, parent rev 'base')
  * how to create 'right' tree (and revision, parent rev 'base')
  * the conflict(s) expected
  * how to check that changes in 'base'->'left' have been taken
  * how to check that changes in 'base'->'right' have been taken

From that template S, we generate two concrete scenarios from S, "Sl" and "Sr":
  Sl: define this=left, other=right
  Sr: define this=right, other=left

Then the test case verifies each scenario by:
  * creating the 'base', 'this' and 'other' trees
  * performing the merge of 'other' into 'this'
  * verifying the expected conflicts were generated
  * resolving with --take-this or --take-other, and running the corresponding
    checks (for either 'base'->'this', or 'base'->'other')
-----

Have I missed anything?

If I got that right, please add that high-level description to this test module,
it will make the implementation details less of a concern for me because at
least the intent will be clear, which helps understand the implementation. But
I do think that structure could be expressed more clearly and succintly than
your current code.

(Also, if I'm right, that suggests to me that the tests for --take-this vs.
--take-other could just as easily be another scenario multiplication rather two
test methods, so leaving a TestCase with one test method but many scenarios!)

I don't want to sound too critic...

[Andrew Bennetts (spiv)]

One more thing:

I wrote:
>
> Let's look at the bigger picture. I think that all conflict scenarios have this
> shape:

I'll add that it took me *many* rereadings of the code to get this summary as
correct as it is. This is part of my fundamental concern with the existing
tests, it's been very hard for me to figure out exactly what the tests are
doing. I'd love for the code to be so simple that this is instantly obvious
(which may be an unattainable goal?) but I'll happily settle for explanatory
comments.

-Andrew.

[Martin Pool (mbp)]

On 25 March 2010 17:09, Andrew Bennetts <email address hidden> wrote:
> One more thing:
>
> I wrote:
>>
>> Let's look at the bigger picture.  I think that all conflict scenarios have this
>> shape:
>
> I'll add that it took me *many* rereadings of the code to get this summary as
> correct as it is.  This is part of my fundamental concern with the existing
> tests, it's been very hard for me to figure out exactly what the tests are
> doing.  I'd love for the code to be so simple that this is instantly obvious
> (which may be an unattainable goal?) but I'll happily settle for explanatory
> comments.

I agree, I partially rubberstamped it because it was not obviously
wrong more than being obviously correct. I don't want to block
changes to this on getting all the existing tests to be very clear,
but we should move in that direction.

Thanks for reviewing it so thoroughly, Andrew.

--
Martin <http://launchpad.net/~mbp/>

[Andrew Bennetts (spiv)]

Martin Pool wrote:
> I agree, I partially rubberstamped it because it was not obviously
> wrong more than being obviously correct. I don't want to block
> changes to this on getting all the existing tests to be very clear,
> but we should move in that direction.

Right, and with a large comment like the one I suggested I'd be very happy to
see this land, so I should have said:

 review approve

But I'd really like to see this testing improvement polished soon after it lands
:)

review: Approve

[Vincent Ladeuil (vila)]

>>>>> Andrew Bennetts <email address hidden> writes:

    > Review: Approve
    > Martin Pool wrote:
    >> I agree, I partially rubberstamped it because it was not obviously
    >> wrong more than being obviously correct. I don't want to block
    >> changes to this on getting all the existing tests to be very clear,
    >> but we should move in that direction.

    > Right, and with a large comment like the one I suggested I'd be very happy to
    > see this land, so I should have said:

    > review approve

Thanks.

    > But I'd really like to see this testing improvement
    > polished soon after it lands :)

Given that these two branches are sons of 'please put your
blackbox tests into the proper file' review comment from John and
that from there I decided to rewrite them as whitebox ones and
entered a mine field from where these two bugs (and one in
dirstate) showed up, rest assured that your remarks will be
promptly taken into account :)

         Vincent

[Vincent Ladeuil (vila)]

>>>>> Andrew Bennetts <email address hidden> writes:

    > (following on from the discussion in the pre-requisite branch, as this branch
    > has an updated version of that test code)

    > Vincent Ladeuil wrote:
    > [...]
    >> + def do_nothing(self):
    >> + return (None, None, [])
    >>
    >> def do_create_file(self):
    >> return ('file', 'file-id',
    >> [('add', ('file', 'file-id', 'file', 'trunk content\n'))])
    >>
    >> + def do_create_file_a(self):
    >> + return ('file', 'file-a-id',
    >> + [('add', ('file', 'file-a-id', 'file', 'file a content\n'))])
    >> +
    >> + def check_file_content_a(self):
    >> + self.assertFileEqual('file a content\n', 'branch/file')
    >> +
    >> + def do_create_file_b(self):
    >> + return ('file', 'file-b-id',
    >> + [('add', ('file', 'file-b-id', 'file', 'file b content\n'))])
    >> +
    >> + def check_file_content_b(self):
    >> + self.assertFileEqual('file b content\n', 'branch/file')

    > The repetition here is pretty ugly.

Yes.

    > I think what would be better is a more data-driven
    > description of the scenarios.

    > So, instead of specifying e.g.:

    > _actions_base='create_file'

    > Why not specify the build_snapshot recipe directly in the scenario, and have a
    > module-global set of canned recipe items to keep it short:

    > base_snapshot=[canned['trunk']]

I went away from centralizing all recipes in the base class in a
later iteration as it broke the locality and there was almost no
reuse cases (but again I'm referring to later work, so now that I
can push my loom again, take a look at
lp:~vila/bzr/conflict-manager). Revno 4639 in particular goes
into the direction you're asking for (but still miss some
points).

One point worth noting there too is that conflating the actions
modifying the working tree with the info needed to characterized
the conflict object needs to be class specific. But in light of
your remarks, I now think that I should just have separate dicts
for each side and let the parametrization assign these dicts to
the right attribute (instead of trying to flatten them which make
mirror_scenarios more complex than it could be).

    > (Note how more complex snapshots with multiple elements are
    > still easy to assemble, because you specify the whole
    > snapshot list in the scenario.)

That's not the problem and as explained below I think it's even
YAGNI.

The problem is more around defining which information is needed
to check the conflict object (path, file-id) in which branch.

That's also why I refrained from trying to address multiple
conflicts tests at that point.

    > I think this would be easier and clearer than your zoo of
    > do_* methods.

Hehe, hello bialix's zoo :)

While working with these tests having the actions defined in the
right class made it easier to get them right (it's amazing how
hard it was to predict the correct conflict type when defining a
scenario, even with blackbox ones already available...). Well,
the actions themselves wasn't that hard, but what info was used
to create the conflict object ...

[lp:~vila/bzr/537956-parent-loop-incomplete]

4640. By Vincent Ladeuil on 2010-03-25

Merge fixes from review from the pre-requisite branch

=== modified file 'NEWS'
--- NEWS	2010-03-25 09:11:32 +0000
+++ NEWS	2010-03-25 09:11:32 +0000
@@ -246,6 +246,10 @@
 * Documentation about testing with ``subunit`` has been tweaked.
   (Robert Collins)
 
+* Known failures has been added for resolve --take-other on ParentLoop
+  conflicts. This reflects bug #537956 without fixing it.
+  (Vincent Ladeuil)
+
 * New ``bzrlib.tests.test_import_tariff`` can make assertions about what
   Python modules are loaded, to guard against startup time or library
   dependency regressions.

=== modified file 'bzrlib/atomicfile.py'
--- bzrlib/atomicfile.py	2010-02-22 08:28:41 +0000
+++ bzrlib/atomicfile.py	2010-03-25 09:11:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2004, 2005 Canonical Ltd
+# Copyright (C) 2005, 2006, 2008, 2009, 2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by

=== modified file 'bzrlib/conflicts.py'
--- bzrlib/conflicts.py	2010-03-25 09:11:32 +0000
+++ bzrlib/conflicts.py	2010-03-25 09:11:32 +0000
@@ -555,7 +555,7 @@
 
 
 class ContentsConflict(PathConflict):
-    """The files are of different types, or not present"""
+    """The files are of different types (or both binary), or not present"""
 
     has_files = True
 
@@ -707,7 +707,7 @@
 
     typestring = 'parent loop'
 
-    format = 'Conflict moving %(conflict_path)s into %(path)s.  %(action)s.'
+    format = 'Conflict moving %(path)s into %(conflict_path)s. %(action)s.'
 
     def action_take_this(self, tree):
         # just acccept bzr proposal

=== modified file 'bzrlib/tests/test_conflicts.py'
--- bzrlib/tests/test_conflicts.py	2010-03-25 09:11:32 +0000
+++ bzrlib/tests/test_conflicts.py	2010-03-25 09:11:32 +0000
@@ -247,6 +247,8 @@
     _item_id = None
 
     # Set by _this_actions and other_actions
+    # FIXME: rename them this_args and other_args so the tests can use them
+    # more freely
     _this_path = None
     _this_id = None
     _other_path = None
@@ -318,23 +320,27 @@
     def _get_check(self, name):
         return getattr(self, 'check_%s' % name)
 
-    def assertConflict(self, wt):
-        confs = wt.conflicts()
-        self.assertLength(1, confs)
-        c = confs[0]
-        self.assertIsInstance(c, self._conflict_type)
-        self._assert_conflict(wt, c)
-
-    def check_resolved(self, wt, path, action):
-        conflicts.resolve(wt, [path], action=action)
-        # Check that we don't have any conflicts nor unknown left
-        self.assertLength(0, wt.conflicts())
-        self.assertLength(0, list(wt.unknowns()))
+    def do_nothing(self):
+        return (None, None, [])
 
     def do_create_file(self):
         return ('file', 'file-id',
                 [('add', ('file', 'file-id', 'file', 'trunk content\n'))])
 
+    def do_create_file_a(self):
+        return ('file', 'file-a-id',
+                [('add', ('file', 'file-a-id', 'file', 'file a content\n'))])
+
+    def check_file_content_a(self):
+        self.assertFileEqual('file a content\n', 'branch/file')
+
+    def do_create_file_b(self):
+        return ('file', 'file-b-id',
+                [('add', ('file', 'file-b-id', 'file', 'file b content\n'))])
+
+    def check_file_content_b(self):
+        self.assertFileEqual('file b content\n', 'branch/file')
+
     def do_create_dir(self):
         return ('dir', 'dir-id', [('add', ('dir', 'dir-id', 'directory', ''))])
 
@@ -358,6 +364,13 @@
         self.failIfExists('branch/file')
         self.failUnlessExists('branch/new-file')
 
+    def do_rename_file2(self):
+        return ('new-file2', 'file-id', [('rename', ('file', 'new-file2'))])
+
+    def check_file_renamed2(self):
+        self.failIfExists('branch/file')
+        self.failUnlessExists('branch/new-file2')
+
     def do_rename_dir(self):
         return ('new-dir', 'dir-id', [('rename', ('dir', 'new-dir'))])
 
@@ -384,17 +397,34 @@
         wt.merge_from_branch(b, 'other')
         return wt
 
+    def assertConflict(self, wt):
+        confs = wt.conflicts()
+        self.assertLength(1, confs)
+        c = confs[0]
+        self.assertIsInstance(c, self._conflict_type)
+        self._assert_conflict(wt, c)
+
+    def _get_resolve_path_arg(self, wt, action):
+        return self._item_path
+
+    def check_resolved(self, wt, action):
+        path = self._get_resolve_path_arg(wt, action)
+        conflicts.resolve(wt, [path], action=action)
+        # Check that we don't have any conflicts nor unknown left
+        self.assertLength(0, wt.conflicts())
+        self.assertLength(0, list(wt.unknowns()))
+
     def test_resolve_taking_this(self):
         wt = self._merge_other_into_this()
         self.assertConflict(wt)
-        self.check_resolved(wt, self._item_path, 'take_this')
+        self.check_resolved(wt, 'take_this')
         check_this = self._get_check(self._check_this)
         check_this()
 
     def test_resolve_taking_other(self):
         wt = self._merge_other_into_this()
         self.assertConflict(wt)
-        self.check_resolved(wt, self._item_path, 'take_other')
+        self.check_resolved(wt, 'take_other')
         check_other = self._get_check(self._check_other)
         check_other()
 
@@ -404,16 +434,12 @@
     _conflict_type = conflicts.ContentsConflict,
     @classmethod
     def scenarios(klass):
-        common = dict(_actions_base='create_file',
-                      _item_path='file', item_id='file-id',
-                      )
         base_scenarios = [
             (('file_modified', dict(actions='modify_file',
                                    check='file_has_more_content')),
              ('file_deleted', dict(actions='delete_file',
                                    check='file_doesnt_exist')),
-             dict(_actions_base='create_file',
-                  _item_path='file', item_id='file-id',)),
+             dict(_actions_base='create_file', _item_path='file')),
             ]
         return klass.mirror_scenarios(base_scenarios)
 
@@ -430,25 +456,31 @@
 
     @classmethod
     def scenarios(klass):
-        for_dirs = dict(_actions_base='create_dir',
+        for_file = dict(_actions_base='create_file',
+                  _item_path='new-file', _item_id='file-id',)
+        for_dir = dict(_actions_base='create_dir',
                         _item_path='new-dir', _item_id='dir-id',)
         base_scenarios = [
             (('file_renamed',
               dict(actions='rename_file', check='file_renamed')),
              ('file_deleted',
               dict(actions='delete_file', check='file_doesnt_exist')),
-             dict(_actions_base='create_file',
-                  _item_path='new-file', _item_id='file-id',)),
+             for_file),
+            (('file_renamed',
+              dict(actions='rename_file', check='file_renamed')),
+             ('file_renamed2',
+              dict(actions='rename_file2', check='file_renamed2')),
+             for_file),
             (('dir_renamed',
               dict(actions='rename_dir', check='dir_renamed')),
              ('dir_deleted',
               dict(actions='delete_dir', check='dir_doesnt_exist')),
-             for_dirs),
+             for_dir),
             (('dir_renamed',
               dict(actions='rename_dir', check='dir_renamed')),
              ('dir_renamed2',
               dict(actions='rename_dir2', check='dir_renamed2')),
-             for_dirs),
+             for_dir),
         ]
         return klass.mirror_scenarios(base_scenarios)
 
@@ -478,60 +510,25 @@
         wt.set_conflicts(conflicts.ConflictList([old_c]))
 
 
-class TestResolveDuplicateEntry(TestResolveConflicts):
-
-    preamble = """
-$ bzr init trunk
-$ cd trunk
-$ echo 'trunk content' >file
-$ bzr add file
-$ bzr commit -m 'Create trunk'
-
-$ echo 'trunk content too' >file2
-$ bzr add file2
-$ bzr commit -m 'Add file2 in trunk'
-
-$ bzr branch . -r 1 ../branch
-$ cd ../branch
-$ echo 'branch content' >file2
-$ bzr add file2
-$ bzr commit -m 'Add file2 in branch'
-
-$ bzr merge ../trunk
-2>+N  file2
-2>R   file2 => file2.moved
-2>Conflict adding file file2.  Moved existing file to file2.moved.
-2>1 conflicts encountered.
-"""
-
-    def test_keep_this(self):
-        self.run_script("""
-$ bzr rm file2  --force
-$ bzr mv file2.moved file2
-$ bzr resolve file2
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
-""")
-
-    def test_keep_other(self):
-        self.failIfExists('branch/file2.moved')
-        self.run_script("""
-$ bzr rm file2.moved --force
-$ bzr resolve file2
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
-""")
-        self.failIfExists('branch/file2.moved')
-
-    def test_resolve_taking_this(self):
-        self.run_script("""
-$ bzr resolve --take-this file2
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
-""")
-
-    def test_resolve_taking_other(self):
-        self.run_script("""
-$ bzr resolve --take-other file2
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
-""")
+class TestResolveDuplicateEntry(TestParametrizedResolveConflicts):
+
+    _conflict_type = conflicts.DuplicateEntry,
+    @classmethod
+    def scenarios(klass):
+        base_scenarios = [
+            (('filea_created', dict(actions='create_file_a',
+                                    check='file_content_a')),
+             ('fileb_created', dict(actions='create_file_b',
+                                   check='file_content_b')),
+             dict(_actions_base='nothing', _item_path='file')),
+            ]
+        return klass.mirror_scenarios(base_scenarios)
+
+    def assertDuplicateEntry(self, wt, c):
+        self.assertEqual(self._this_id, c.file_id)
+        self.assertEqual(self._item_path + '.moved', c.path)
+        self.assertEqual(self._item_path, c.conflict_path)
+    _assert_conflict = assertDuplicateEntry
 
 
 class TestResolveUnversionedParent(TestResolveConflicts):
@@ -701,56 +698,101 @@
 """)
 
 
-class OldTestResolvePathConflict(TestResolveConflicts):
-
-    preamble = """
-$ bzr init trunk
-$ cd trunk
-$ echo 'Boo!' >file
-$ bzr add
-$ bzr commit -m 'Create trunk'
-
-$ bzr mv file file-in-trunk
-$ bzr commit -m 'Renamed to file-in-trunk'
-
-$ bzr branch . -r 1 ../branch
-$ cd ../branch
-$ bzr mv file file-in-branch
-$ bzr commit -m 'Renamed to file-in-branch'
-
-$ bzr merge ../trunk
-2>R   file-in-branch => file-in-trunk
-2>Path conflict: file-in-branch / file-in-trunk
-2>1 conflicts encountered.
-"""
-
-    def test_keep_source(self):
-        self.run_script("""
-$ bzr resolve file-in-trunk
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
-""")
-
-    def test_keep_target(self):
-        self.run_script("""
-$ bzr mv file-in-trunk file-in-branch
-$ bzr resolve file-in-branch
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
-""")
-
-    def test_resolve_taking_this(self):
-        self.run_script("""
-$ bzr resolve --take-this file-in-branch
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
-""")
-
-    def test_resolve_taking_other(self):
-        self.run_script("""
-$ bzr resolve --take-other file-in-branch
-$ bzr commit --strict -m 'No more conflicts nor unknown files'
-""")
-
-
-class TestResolveParentLoop(TestResolveConflicts):
+class TestResolveParentLoop(TestParametrizedResolveConflicts):
+
+    _conflict_type = conflicts.ParentLoop,
+    @classmethod
+    def scenarios(klass):
+        base_scenarios = [
+            (('dir1_into_dir2', dict(actions='move_dir1_into_dir2',
+                                      check='dir1_moved')),
+             ('dir2_into_dir1', dict(actions='move_dir2_into_dir1',
+                                      check='dir2_moved')),
+             dict(_actions_base='create_dir1_dir2')),
+            (('dir1_into_dir4', dict(actions='move_dir1_into_dir4',
+                                      check='dir1_2_moved')),
+             ('dir3_into_dir2', dict(actions='move_dir3_into_dir2',
+                                      check='dir3_4_moved')),
+             dict(_actions_base='create_dir1_4')),
+            ]
+        return klass.mirror_scenarios(base_scenarios)
+
+    def do_create_dir1_dir2(self):
+        return (None, None,
+                [('add', ('dir1', 'dir1-id', 'directory', '')),
+                 ('add', ('dir2', 'dir2-id', 'directory', '')),
+                 ])
+
+    def do_move_dir1_into_dir2(self):
+        # The arguments are the file-id to move and the targeted file-id dir.
+        return ('dir1-id', 'dir2-id', [('rename', ('dir1', 'dir2/dir1'))])
+
+    def check_dir1_moved(self):
+        self.failIfExists('branch/dir1')
+        self.failUnlessExists('branch/dir2/dir1')
+
+    def do_move_dir2_into_dir1(self):
+        # The arguments are the file-id to move and the targeted file-id dir.
+        return ('dir2-id', 'dir1-id', [('rename', ('dir2', 'dir1/dir2'))])
+
+    def check_dir2_moved(self):
+        self.failIfExists('branch/dir2')
+        self.failUnlessExists('branch/dir1/dir2')
+
+    def do_create_dir1_4(self):
+        return (None, None,
+                [('add', ('dir1', 'dir1-id', 'directory', '')),
+                 ('add', ('dir1/dir2', 'dir2-id', 'directory', '')),
+                 ('add', ('dir3', 'dir3-id', 'directory', '')),
+                 ('add', ('dir3/dir4', 'dir4-id', 'directory', '')),
+                 ])
+
+    def do_move_dir1_into_dir4(self):
+        # The arguments are the file-id to move and the targeted file-id dir.
+        return ('dir1-id', 'dir4-id',
+                [('rename', ('dir1', 'dir3/dir4/dir1'))])
+
+    def check_dir1_2_moved(self):
+        self.failIfExists('branch/dir1')
+        self.failUnlessExists('branch/dir3/dir4/dir1')
+        self.failUnlessExists('branch/dir3/dir4/dir1/dir2')
+
+    def do_move_dir3_into_dir2(self):
+        # The arguments are the file-id to move and the targeted file-id dir.
+        return ('dir3-id', 'dir2-id',
+                [('rename', ('dir3', 'dir1/dir2/dir3'))])
+
+    def check_dir3_4_moved(self):
+        self.failIfExists('branch/dir3')
+        self.failUnlessExists('branch/dir1/dir2/dir3')
+        self.failUnlessExists('branch/dir1/dir2/dir3/dir4')
+
+    def _get_resolve_path_arg(self, wt, action):
+        # ParentLoop is unsual as it says: 
+        # moving <conflict_path> into <path>.  Cancelled move.
+        # But since <path> doesn't exist in the working tree, we need to use
+        # <conflict_path> instead
+        path = wt.id2path(self._other_id)
+        return path
+
+    def assertParentLoop(self, wt, c):
+        if 'taking_other(' in self.id() and 'dir4' in self.id():
+            raise tests.KnownFailure(
+                "ParentLoop doesn't carry enough info to resolve")
+        # The relevant file-ids are other_args swapped (which is the main
+        # reason why they should be renamed other_args instead of Other_path
+        # and other_id). In the conflict object, they represent:
+        # c.file_id: the directory being moved
+        # c.conflict_id_id: The target directory
+        self.assertEqual(self._other_path, c.file_id)
+        self.assertEqual(self._other_id, c.conflict_file_id)
+        # The conflict paths are irrelevant (they are deterministic but not
+        # worth checking since they don't provide the needed information
+        # anyway)
+    _assert_conflict = assertParentLoop
+
+
+class OldTestResolveParentLoop(TestResolveConflicts):
 
     preamble = """
 $ bzr init trunk
@@ -768,7 +810,7 @@
 $ bzr commit -m 'Moved dir1 into dir2'
 
 $ bzr merge ../trunk
-2>Conflict moving dir2/dir1 into dir2.  Cancelled move.
+2>Conflict moving dir2 into dir2/dir1. Cancelled move.
 2>1 conflicts encountered.
 """
 

=== modified file 'bzrlib/tests/test_transform.py'
--- bzrlib/tests/test_transform.py	2010-02-09 19:04:02 +0000
+++ bzrlib/tests/test_transform.py	2010-03-25 09:11:32 +0000
@@ -735,7 +735,7 @@
                                          ' versioned, but has versioned'
                                          ' children.  Versioned directory.')
         self.assertEqual(conflicts_s[6], 'Conflict moving oz/emeraldcity into'
-                                         ' oz/emeraldcity.  Cancelled move.')
+                                         ' oz/emeraldcity. Cancelled move.')
 
     def prepare_wrong_parent_kind(self):
         tt, root = self.get_transform()

=== modified file 'bzrlib/trace.py'
--- bzrlib/trace.py	2010-03-06 05:28:17 +0000
+++ bzrlib/trace.py	2010-03-25 09:11:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
+# Copyright (C) 2005-2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by