HPSS: Client-side objects

[Andrew Bennetts]

Aaron Bentley wrote:
> Andrew Bennetts wrote:
> > +    def open_repository(self):
> > +        path = self._path_for_remote_call(self._client)
> > +        response = self._client.call('BzrDir.find_repository', path)
> > +        assert response[0] in ('ok', 'norepository'), \
> > +            'unexpected response code %s' % (response,)
> > +        if response[0] == 'norepository':
> > +            raise errors.NoRepositoryPresent(self)
> > +        assert len(response) == 4, 'incorrect response length %s' % (response,)
> > +        if response[1] == '':
> > +            format = RemoteRepositoryFormat()
> > +            format.rich_root_data = response[2] == 'True'
> 
> ^^^ It seems the HPSS is returning 'ok', 'yes', 'no', 'True' (and
> presumably 'False') for boolean values.  I think perhaps you should
> standardize.

Changed to use 'yes'/'no' rather than 'True'/'False'.

> Also, the x = y == z is not especially clear.  I think we recommend
> surrounding the boolean expression with parentheses, e.g. x = (y == z)

Done.

> > +    def can_convert_format(self):
> > +        """Upgrading of remote bzrdirs is not supported yet."""
> > +        return False
> 
> Should we consider supporting the VFS upgrade approach?

It's not really safe to — the client could then upgrade the branch to a format
the server cannot read.  In general some thought needs to be given to how
upgrades should work.  A smart server could legitimately choose to store data in
a different format to the standard formats, I think (e.g. in a database),
although obviously this won't work while most things still fall back to VFS
methods.  So "upgrade" then becomes something that means "please upgrade this
branch to support these features: rich roots, subtrees", rather than "change the
files on disk like so".

So I think it's ok to refuse remote upgrades for now.  It's not a very common
desire anyway, because it's so slow at the moment.

> > +class RemoteBranch(branch.Branch):
> 
> ^^^^ Hmm.  Didn't we already have a class named this?  Oh yeah, back
> around revno 1390 :-)

:)

> > +    @needs_write_lock
> > +    def set_revision_history(self, rev_history):
> > +        # Send just the tip revision of the history; the server will generate
> > +        # the full history from that.  If the revision doesn't exist in this
> > +        # branch, NoSuchRevision will be raised.
> 
> This is not good enough, because we don't have a guarantee that the
> caller will provide the lefthand history.  See the assertions in
> Branch6.set_revision_history.

(I assume you mean BzrBranch6._write_revision_history, because it doesn't
override set_revision_history directly...)

I've spoken with Robert about this.  If BzrBranch6 can support
set_last_revision_info, then this should work.  The only difference in behaviour
with a BzrBranch6-via-RemoteBranch vs. a direct BzrBranch6 will be that passing
a non-lefthand history will silently become a lefthand history to the tip,
rather than an error.  I think this is ok.

> > +        path = self.bzrdir._path_for_remote_call(self._client)
> > +        if rev_history == []:
> > +            rev_id = ''
> 
> ^^^ I think NULL_REVISION/'null:' is much better than '' for this purpose.

NULL_REVISION is never stored to disk.  It's not part of the external ABI for
dealing bzr data.  So similarly, I don't think it's appropriate to send it
across the wire.  I considered using "null:" explicitly, but the apparent
consistency seems likely to lead to confusion.  "" is a perfectly reasonable way
to serialise the null revision in this context, I think.

What don't you like about ''?

> > === modified file 'bzrlib/branch.py'
> > --- bzrlib/branch.py	2007-04-13 07:18:57 +0000
> > +++ bzrlib/branch.py	2007-04-16 05:57:08 +0000
> > @@ -579,7 +579,8 @@
> >  
> >      def get_push_location(self):
> >          """Return the None or the location to push this branch to."""
> > -        raise NotImplementedError(self.get_push_location)bzrlib/branch.py
> > +        push_loc = self.get_config().get_user_option('push_location')
> > +        return push_loc
> 
> ^^^ I thought we were leaving details like this to particular
> implementations.

Nothing overrides it so far, so I call WHUI (We Haven't Used It).  I just moved
it up from BzrBranch to Branch, so that RemoteBranch can re-use it.

Particular implementations can always override get_config if they really want.

> >      def _get_checkout_format(self):
> >          """Return the most suitable metadir for a checkout of this branch.
> > -        Weaves are used if this branch's repostory uses weaves.
> > +        Weaves are used if this branch's repository uses weaves.
> >          """
> > +        from bzrlib.remote import RemoteBzrDir
> >          if isinstance(self.bzrdir, bzrdir.BzrDirPreSplitOut):
> >              from bzrlib.repofmt import weaverepo
> >              format = bzrdir.BzrDirMetaFormat1()
> >              format.repository_format = weaverepo.RepositoryFormat7()
> > +        elif isinstance(self.bzrdir, RemoteBzrDir):
> > +            format = bzrdir.BzrDirMetaFormat1()
> 
> ^^^ For a RemoteBzrDir, we should be using the same formats as the
> remote branch uses.  Can't we at least fall back to the VFS stuff?

I've changed this back, and overridden _get_checkout_format in RemoteBranch
instead.  It's a bit iffy to override an underscored method (it's not clear if
it's meant to be private to Branch, or to Branch+subclasses), but it at means we
can delegate to the VFS branch.

> > +# We can't use register_control_format because it adds it at a lower priority
> > +# than the existing branches, whereas this should take priority.
> 
> Can't we just register it first, then?

This just changed, actually, after discussion on IRC last night.  There's now a
register_control_server_format that is used for this registration:

    @classmethod
    def register_control_server_format(klass, format):
        """Register a control format for client-server environments.

        These formats will be tried before ones registered with
        register_control_format.  This gives implementations that decide to the
        chance to grab it before anything looks at the contents of the format
        file.
        """
        klass._control_formats.insert(0, format)

-Andrew.