[MERGE] set parent trees api.

[Martin Pool]

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.

> === modified file 'bzrlib/tests/workingtree_implementations/__init__.py'
> --- bzrlib/tests/workingtree_implementations/__init__.py	2006-07-29 04:09:55 +0000
> +++ bzrlib/tests/workingtree_implementations/__init__.py	2006-08-17 03:50:11 +0000
> @@ -60,6 +60,7 @@
>          'bzrlib.tests.workingtree_implementations.test_is_control_filename',
>          'bzrlib.tests.workingtree_implementations.test_is_ignored',
>          'bzrlib.tests.workingtree_implementations.test_locking',
> +        'bzrlib.tests.workingtree_implementations.test_parents',
>          'bzrlib.tests.workingtree_implementations.test_pull',
>          'bzrlib.tests.workingtree_implementations.test_workingtree',
>          ]
> 
> === modified file 'bzrlib/workingtree.py'
> --- bzrlib/workingtree.py	2006-08-17 03:35:26 +0000
> +++ bzrlib/workingtree.py	2006-08-17 04:27:59 +0000
> @@ -653,6 +653,34 @@
>          self._write_inventory(inv)
>  
>      @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.

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

> +        self.set_parent_ids(self.get_parent_ids() + [parent_tuple[0]],
> +            allow_leftmost_as_ghost=allow_leftmost_as_ghost)
> +
> +    @needs_write_lock
>      def add_pending_merge(self, *revision_ids):
>          # TODO: Perhaps should check at this point that the
>          # history of the revision is actually present?
> @@ -686,6 +714,49 @@
>          return p
>  
>      @needs_write_lock
> +    def set_parent_ids(self, revision_ids, allow_leftmost_as_ghost=False):
> +        """Set the parent ids to revision_ids.
> +        
> +        See also set_parent_trees. This api will try to retrieve the tree data
> +        for each element of revision_ids from the trees repository. If you have
> +        tree data already available, it is more efficient to use
> +        set_parent_trees rather than set_parent_ids. set_parent_ids is however
> +        an easier API to use.
> +
> +        :param revision_ids: The revision_ids to set as the parent ids of this
> +            working tree. Any of these may be ghosts.
> +        """
> +        trees = []
> +        for rev_id in revision_ids:
> +            try:
> +                trees.append(
> +                    (rev_id, self.branch.repository.revision_tree(rev_id)))
> +            except errors.RevisionNotPresent:
> +                trees.append((rev_id, None))
> +                pass

Isn't the 'pass' redundant?

> +        self.set_parent_trees(trees,
> +            allow_leftmost_as_ghost=allow_leftmost_as_ghost)
> +
> +    @needs_write_lock
> +    def set_parent_trees(self, parents_list, allow_leftmost_as_ghost=False):
> +        """Set the parents of the working tree.
> +
> +        :param parents_list: A list of (revision_id, tree) tuples. 
> +            If tree is None, then that element is treated as an unreachable
> +            parent tree - i.e. a ghost.
> +        """
> +        parent = parents_list[:1]
> +        if len(parent):
> +            if (not allow_leftmost_as_ghost and not
> +                self.branch.repository.has_revision(parent[0][0])):
> +                raise errors.GhostRevision(parent[0][0])
> +            self.set_last_revision(parent[0][0])
> +        else:
> +            self.set_last_revision(None)
> +        merges = parents_list[1:]
> +        self.set_pending_merges([revid for revid, tree in merges])
> +

Similarly, why not just take trees?

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

   if len(parents_list) > 0:
     leftmost = parents_list[0]
     ...
   else:


-- 
Martin