[RFC] Splitting bzr-specific functions out of Repository

[Jelmer Vernooij]

Am Montag, den 28.07.2008, 09:48 +1000 schrieb Robert Collins:
> On Sun, 2008-07-27 at 21:05 +0200, Jelmer Vernooij wrote:
> > Would anybody be opposed to splitting the bzr-specific functions out of
> > Repository into a BzrRepository class that is a subclass of Repository,
> > similar to BzrBranch and Branch?
> I find the bzrbranch/branch split to be quite unsatisfactory. It has
> lead to some [transient] bugs in fact.
We don't have any other implementations of Branch in bzrlib that are
exercised by the tests other than the implementations that derive from
BzrBranch. That probably hasn't helped; it may be useful to add some
tests specifically for Branch.

> Presumably you're suggesting this to make your life easier :) - what in
> particular is causing you friction/headaches? (Just having the methods
> there is at most unclear). Is it needing a more clear definition of what
> you need to implement, or ???
Having the methods there is unclear, also means there's no easy way for
me to determine what I do and what I don't have to implement: other
parts of Bazaar seem to rely on _ functions even though they are
underscore functions. 

This is probably boils down to the discussion we had in London about
various degrees of accessibility different functions have in Bazaar.

> > The functions I'm thinking of that could be in BzrRepository only are:
> > 
> > Repository.get_inventory_xml -> BzrRepository.get_inventory_text
> > Repository.get_revision_xml -> BzrRepository.get_revision_text
> > Repository.get_revision_sha1 -> BzrRepository.get_revision_sha1
> > Repository.get_inventory_sha1 -> BzrRepository.get_inventory_sha1
> > Repository.serialise_inventory -> BzrRepository.serialise_inventory
> These are already largely generic, for a given serialiser [but not as
> generic as they need to be - see split inventories for instance, and
> obviously bzr-svn and bzr-git are radically different].
> 
> If we don't want these on the public interface, I'd rather we do the
> deprecation dance and give them _ names.
Giving them _ names won't guarantee that other parts of bzrlib won't
 try to run them.

bzr-svn was implemented by trying to run various bzr commands, seeing
what Repository methods were being executed and then implementing those
methods. It has happened in the past that _ methods that weren't used by
any of the builtin commands started being used in some code path
executed by the builtin commands, breaking bzr-svn. It's also possible
that I didn't properly exercise all code paths that may use _ functions
while testing bzr-svn.

It would be much nicer (and work on bzr-git is what brought this up
again for me) if it was possible to see easily what methods I would have
to implement.

Another way of working around this (instead of adding another level of
subclasses) would be of course be to have the _ and __ functions, _
meaning "not part of the public API but part of the interface" and __
meaning completely private. 

Cheers,

Jelmer

-- 
Jelmer Vernooij <jelmer at samba.org> - http://samba.org/~jelmer/
Jabber: jelmer at jabber.fsfe.org