[RFC] intertree-implementations

[Robert Collins]

On Wed, 2006-07-26 at 17:00 -0500, John Arbash Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Robert Collins wrote:
> > On Mon, 2006-07-24 at 21:57 +1000, Robert Collins wrote:
> >> This is the first cut at InterTree - I've hooked it up to Tree.compare.
> >> From here I plan to expand the tests until all the parameters are
> >> present and minimally tested. At that point I intend to deprecate
> >> compare_trees as the public interface.
> >>
> >> I'm sending this in now for comments and feedback..
> > 
> > Heres an update - this changes the api name to be 'Tree.changes_from'
> > which is IMO clearer - it tells you what the order of the parameters is.
> > 
> > Also attached is my branch to make all the functionality of
> > compare_trees available as Tree.changes_from(). This builds on the basic
> > implementation in the first patch. It splits Johns new delta tests into
> > InterTree tests and WorkingTree tests, depending on whether they depend
> > on working tree features like 'unknowns' or not, and adds tests for all
> > the features of compare_trees.
> > 
> > I'm seeking a +1 on both of these now.
> > 
> > Cheers,
> > Rob
> > 
> 
> ...
> 
> > +class TestCaseWithTwoTrees(TestCaseWithTree):
> > +
> > +    def make_to_branch_and_tree(self, relpath):
> > +        """Make a to_workingtree_format branch and tree."""
> > +        made_control = self.make_bzrdir(relpath, 
> > +            format=self.workingtree_format_to._matchingbzrdir)
> > +        made_control.create_repository()
> > +        made_control.create_branch()
> > +        return self.workingtree_format_to.initialize(made_control)
> > +
> > +    def get_to_tree_no_parents_no_content(self, empty_tree):
> > +        return super(TestCaseWithTwoTrees, self).get_tree_no_parents_no_content(empty_tree, converter=self.workingtree_to_test_tree_to)
> 
> 1) This should be line wrapped at 79 characters.

hmm, not sure how I missed that. will wrap.

> 2) Considering how ugly it is to re-wrap every function in the base
> class, it seems like it might make more sense to have a default
> converter, which can be set per-class, and which gets used by the
> base-class implementation.

The are *two* parallel converters needed, one for the source and one for
the two. Having a default only helps for one side.


> > === modified file 'bzrlib/tree.py'
> > --- bzrlib/tree.py	2006-07-25 06:21:42 +0000
> > +++ bzrlib/tree.py	2006-07-26 04:04:19 +0000
> > @@ -25,6 +25,7 @@
> >  from bzrlib.errors import BzrError, BzrCheckError
> >  from bzrlib import errors
> >  from bzrlib.inventory import Inventory
> > +from bzrlib.inter import InterObject
> >  from bzrlib.osutils import fingerprint_file
> >  import bzrlib.revision
> >  from bzrlib.trace import mutter, note
> > @@ -50,6 +51,18 @@
> >      trees or versioned trees.
> >      """
> >      
> > +    def changes_from(self, other):
> > +        """Return a TreeDelta of the changes from other to this tree.
> > +
> > +        :param other: A tree to compare with.
> > +        The comparison will be performed by an InterTree object looked up on 
> > +        self and other.
> > +        """
> > +        # Martin observes that Tree.changes_from returns a TreeDelta and this
> > +        # may confuse people, because the class name of the returned object is
> > +        # a synonym of the object referenced in the method name.
> > +        return InterTree.get(other, self).compare()
> > +    
> 
> I don't really understand the comment here. "the class name of the
> returned object is a synonym of the object referenced in the method name".
>
> Is it that you are passing in a 'Tree' and getting back a 'TreeDelta'?
> That seems very reasonable to me.

We call 'changes_from' and get a TreeDelta not a TreeChanges.

Now, I think TreeDelta is a better name than TreeChanges, and that
things are fine as they are, which is why its written in the third
person :).


> > === renamed file 'bzrlib/tests/test_delta.py' => 'bzrlib/tests/workingtree_implementations/test_changes_from.py'
> 
> It seems like you moved the test_delta in to
> workingtree_implementations, and then removed most of the tests.
> It would seem more accurate to move it where the bulk of the content goes.
> 
> ...
> 
> I suppose you already had a file over here. Anyway, no big deal, just
> looked odd.
..

Yes, I already had a file for the tests, so I moved your tests into it,
and the ones that needed more work than is reasonable at this point, I
left in place, then moved that test file to workingtree_implementations.


> The rest of the tests look good. +1 from me.

Thanks.

This depends on my tree_implementations patch.. I'll merge it when that
gets fully ok'd.

-Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060727/02fb4831/attachment.pgp