[MERGE] set parent trees api.

[Robert Collins]

On Fri, 2006-08-18 at 17:38 +1000, Martin Pool wrote:
> On 17 Aug 2006, Robert Collins <robertc at robertcollins.net> wrote:
> > On Thu, 2006-08-17 at 02:10 -0400, Aaron Bentley wrote:
> > > -----BEGIN PGP SIGNED MESSAGE-----
> > > Hash: SHA1
> > > 
> > > Robert Collins wrote:
> > > > This is an implementation of the parent tree setting api I discussed
> > > > here and specced on
> > > > https://launchpad.net/products/bzr/+spec/workingtree-parent-api
> 
> +1 with some changes.
> 
> >      def __init__(self):
> > -        BzrNewError.__init__(self)    
> > +        BzrNewError.__init__(self)
> > +
> > +
> > +class GhostRevision(BzrNewError):
> > +    """Revision {%(revision_id)s} is a ghost."""
> > +
> > +    def __init__(self, revision_id):
> > +        BzrNewError.__init__(self)
> > +        self.revision_id = revision_id
> 
> Hm, this is OK but not really a statement of what the error condition
> is.  In defining exceptions we should consider who might want to catch
> them, and how they will look to the user.  Considering the one place 
> this is thrown from, how about "Ghost revision %s cannot be a working
> tree parent", or some such.

Can do.


>   
> >      @needs_write_lock
> > +    def add_parent_tree_id(self, revision_id, allow_leftmost_as_ghost=False):
> > +        """Add revision_id as a parent.
> > +
> > +        This is equivalent to retrieving the current list of parent ids
> > +        and setting the list to its value plus revision_id.
> > +
> > +        :param revision_id: The revision id to add to the parent list. It may
> > +        be a ghost revision.
> > +        """
> 
> That sentence seems slightly contradictory of the
> allow_leftmost_as_ghost parameters.

Will tweak.

> > +        self.set_parent_ids(self.get_parent_ids() + [revision_id],
> > +            allow_leftmost_as_ghost=allow_leftmost_as_ghost)
> > +
> > +    @needs_write_lock
> > +    def add_parent_tree(self, parent_tuple, allow_leftmost_as_ghost=False):
> > +        """Add revision_id, tree tuple as a parent.
> > +
> > +        This is equivalent to retrieving the current list of parent trees
> > +        and setting the list to its value plus parent_tuple. See also
> > +        add_parent_tree_id - if you only have a parent id available it will be
> > +        simpler to use that api. If you have the parent already available, using
> > +        this api is preferred.
> > +
> > +        :param parent_tuple: The (revision id, tree) to add to the parent list.             If the revision_id is a ghost, pass None for the tree.
> > +        """
> 
> Looks like a newline got deleted.
> 
> Why not just take the tree and call get_revision_id() on it?  I see
> that's not declared in the Tree base class, but it is on RevisionTree.
> Interfaces that take an n-tuple of heterogenous objects seem a bit error
> prone.  Having seen the way this looks in the tests I'd fairly strongly prefer
> it just took a tree. 

Because the tree may be a ghost. We either need a marker for a tree that
cannot be retrieved, or we need a fake tree which claims to be revision
X but has the content of an EmptyTree. I think that the way I've done it
is less likely to introduce bugs than having a fake tree.

The reason I use a tuple is for compatability with the set_parent_trees
api rather than having one take rev_id, tree_or_none and the other take
a list of tuples.

I'd rather do it the way I have, if you feel strongly I will change it.

> > +            except errors.RevisionNotPresent:
> > +                trees.append((rev_id, None))
> > +                pass
> 
> Isn't the 'pass' redundant?

ya thunko during adding functionality.
....
> > +        self.set_pending_merges([revid for revid, tree in merges])
> > +
> 
> Similarly, why not just take trees?

Because any of the trees may not be able to be represented as an object.
{except the left most one, unless allow_leftmost.. is true}.

> Maybe the use of 'parent' here is leftover from before; it seems correct
> but a little unclear.  How about instead

Will change that.

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/20060818/73429873/attachment.pgp