[MERGE] Move various weave specific code out of the base Repository class to weaverepo.py.

[Robert Collins]

On Mon, 2007-09-24 at 11:00 +1000, Ian Clatworthy wrote:
> Robert Collins wrote:
> > This patch gets rid of some cruft that we put in for weaves in the
> > repository interface; moving it instead to the weaverepo format specific
> > location.
> 
> I've reviewed this now and it looks pretty close to me. I've voted
> comment rather than tweak because I'm not 100% confident as to why the
> duplication is required. If you can briefly explain why on IRC or email,
> I'll upgrade my vote, assuming the issue is my lack of knowledge in this
> area.
> 
> bb:comment
> 
> > Due to the current subclassing structure in weaverepo.py there is some
> > duplicate code; but frankly I could not care less - its in our legacy,
> > deprecated, warn-on-use formats anyway.
> 
> I'm ok with the duplicated code given it's legacy code. BUT *I* don't
> understand why the code needs to be in WeaveMetaDirRepository at all.
> (Taking it out of Repository into AllInOneRepository
> on the other hand makes sense to me.)

It needs to apply to both AllInOneRepository and WeaveMetaDirRepository
because it is due to using the inventory weave for ancestor queries, and
weaves degrade badly in the presence of ghosts and ghost filling.

> Other comments below ...
> 
> >      @needs_read_lock
> >      def get_revisions(self, revision_ids):
> > +        """Get many revisions at once."""
> > +        return self._get_revisions(revision_ids)
> > +
> > +    @needs_read_lock
> > +    def _get_revisions(self, revision_ids):
> > +        """Core work logic to get many revisions without sanity checks."""
> >          revision_ids = [osutils.safe_revision_id(r) for r in revision_ids]
> > +        for rev_id in revision_ids:
> > +            if not rev_id or not isinstance(rev_id, basestring):
> > +                raise errors.InvalidRevisionId(revision_id=rev_id, branch=self)
> >          revs = self._revision_store.get_revisions(revision_ids,
> >                                                    self.get_transaction())
> 
> Something needs to change here IMO. Either the checks needs to move from
> _get_revisions into get_revisions or the docstring for _get_revisions
> needs to be more explicit about what checks are really required in it.

Knits don't need checks; weave based repositories do, which is why:
get_revision calls get_revisions
get_revision_reconcile calls _get_revisions

In weaves get_revisions does the checks, in knits get_revisions is a
trival thunk to _get_revisions.

The sanity checks that are needed are in the comment in weaverepo.py's
get_revisions method, the docstring for the base class should *not*
document them, because the base api does not require any.

> >      @needs_read_lock
> > -    def get_revision_graph(self, revision_id=None):
> > -        """Return a dictionary containing the revision graph.
> 
> get_revision_graph() is a public API on Repository and has just been
> removed from the base class. It might be implemented in all the
> subclasses but if it's part of the Repository API, then it ought to be
> declared there (together with a NotImplementedError given that's our
> policy for abstract methods.)

I can put it back, but its a method we are trying to remove at the
moment anyway, its existence is for now guaranteed via the repository
interface tests...

-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: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070924/1a832103/attachment.pgp