[MERGE] Nested Trees: revert

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Ian,

Ian Clatworthy wrote:
> Thanks. This looks much better now. There are still some isolated code
> quality issues I'd like improved/explained though before I can approve it.

I've attempted to address your concerns, but I'm unclear whether what
I've done is enough.

>> +    def test_revert_split(self):
>> +        tree = self.make_branch_and_tree('tree',
>> +            format='dirstate-with-subtree')
>> +        self.build_tree(['tree/subdir/', 'tree/subdir/file'])
>> +        tree.add(['subdir', 'subdir/file'], ['subdir-id', 'file-id'])
>> +        tree.commit('added stuff')
>> +        subtree = tree.extract('subdir-id')
>> +        repo = subtree.branch.repository
>> +        old_tree = repo.revision_tree(subtree.last_revision())
>> +        subtree.commit('committed subtree')
>> +
>> +        self.failUnlessExists('tree/subdir/file')
>> +        # revert should unsplit this tree
>> +        subtree.revert(old_tree=old_tree)
>> +        self.failUnlessExists('tree/subdir/subdir/file')
>> +        self.failIfExists('tree/subdir/file')
>> +        subtree.revert()
>> +        self.failUnlessExists('tree/subdir/file')
>> +        self.failIfExists('tree/subdir/subdir/file')
> 
> This test needs more explanatory comments and perhaps better use of
> blank lines between logical sections. In particular, the subdir/subdir
> behaviour is weird and not something I'd expect to happen in a real-life
> scenario?

I would expect it to happen in a real-life scenario.  Recursion is only
downward, so if you're in a subtree, and you revert to a revision before
the tree was split, you would expect the subtree to be reverted to that
previous revision, not the whole tree.

>> +    def test_revert_join(self):
>> +        """Handle case where parent directory is a removed root"""
>> +        tree = self.make_branch_and_tree('tree',
>> +            format='dirstate-with-subtree')
>> +        subtree = self.make_branch_and_tree('tree/subtree',
>> +            format='dirstate-with-subtree')
>> +        self.build_tree(['tree/file1', 'tree/subtree/file2'])
>> +        tree.add('file1')
>> +        tree.commit('empty commit')
>> +        subtree.add('file2')
>> +        subtree.commit('added file', rev_id='subtree-1')
>> +        tree.subsume(subtree)
>> +        tree.commit('subsumed file1')
>> +        old_tree = tree.branch.repository.revision_tree('subtree-1')
>> +        self.build_tree_contents([('tree/file1', 'new contents')])
>> +
>> +        # We leave an unversioned backup copy of file2
>> +        tree.revert(old_tree=old_tree)
>> +        self.failUnlessExists('tree/file1')
>> +        self.assertIs(None, tree.path2id('file1'))
>> +        self.failUnlessExists('tree/file2')
>> +        self.assertIsNot(None, tree.path2id('file2'))
> 
> Ditto w.r.t. clarity here. Most of the commit messages look wrong to me
> as well:
> 
> * 'empty commit' -> 'commit file1'
> * 'added file' -> 'added file2
> * 'subsumed file1' -> 'subsumed subtree'

Fixed.


> 
>> +    def prepare_nested_revert(self):
>> +        tree = self.make_branch_and_tree('tree', format='pack-0.92-subtree')
>> +        subtree = self.make_branch_and_tree('tree/subtree',
>> +                                            format='pack-0.92-subtree')
>> +        subtree.set_root_id('subtree-id')
>> +        subtree2 = self.make_branch_and_tree('tree/subtree2',
>> +                                             format='pack-0.92-subtree')
>> +        subtree.set_root_id('subtree2-id')
>> +        tree.add_reference(subtree)
>> +        self.build_tree_contents([('tree/foo', 'foo'),
>> +                                  ('tree/subtree/bar', 'bar'),
>> +                                  ('tree/subtree2/baz', 'baz')])
>> +        tree.add('foo')
>> +        subtree.add('bar')
>> +        tree.commit('rev 1')
> 
> The test under this bit are great but the setup here looks wrong. I
> assume the second call to subtree.set_root_id() ought to have been
> subtree2.set_root_id() instead. We ought to be adding both subtrees via
> add_reference() & adding baz to subtree2 as well, yes?

Fixed.

>> +    def fixup_new_roots(self):
>> +        """Reinterpret requests to change the root directory
>> +
>> +        Instead of creating a root directory, or moving an existing directory,
>> +        all the attributes and children of the new root are applied to the
>> +        existing root directory.
>> +
>> +        This means that the old root trans-id becomes obsolete, so it is
>> +        recommended only to invoke this after the root trans-id has become
>> +        irrelevant.
>> +        """
> 
> I can't see any mistakes in this method but I find it hard to
> comprehend, particularly the 2nd half of it. Much of that is just the
> inherent complexity in TreeTransform, I guess. Again, I'd like to see
> more use of whitespace between sections so there's a clearer association
> between the comments and the code. I'm sure it makes sense to a
> TreeTransform guru like you but the code needs to be clear to us mere
> mortals. :-)

Is this any better?

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkoAnGoACgkQ0F+nu1YWqI0aDwCffJTj9qA0WsvH8cdv/w0LZirH
qHIAn3zVgUO1C+ssvJTn2U7RP4sMHfaZ
=+EDz
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: revert.patch
Type: text/x-patch
Size: 174154 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090505/123c7e9a/attachment-0001.bin