[MERGE] Nested Trees: revert

[Ian Clatworthy]

Aaron Bentley wrote:

> I've revised revert so that
> 1. specifying a path within a subtree doesn't cause a PathNotChild error
> 2. when paths are specified, only those paths within each subtree are
> reverted.

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.

bb:resubmit

> +    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?

> +    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'

> +    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?

> +    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. :-)

In summary, the primary issue is the clarity/quality of the tests,
particularly test_revert_split().

Ian C.