HPSS: Client-side objects

[Robert Collins]

On Tue, 2007-04-17 at 13:08 +1000, Andrew Bennetts wrote:
> This subset of the full hpss diff adds RemoteBranch, RemoteRepository,
> RemoteBzrDir, and some tests for those, and some other small changes.

Perhaps 'remote' should be a package with 
remote/bzrdir.py
remote/branch.py
remote/repository.py


+# TODO: At some point, handle upgrades by just passing the
+# whole request across to run on the server.

Perhaps this should be a bug?


+class RemoteBzrDir(BzrDir):
+    """Control directory on a remote server, accessed by HPSS."""

perhaps
    """Control directory on a remote server accessed over bzr:."""

+        # XXX: We should go into find_format, but not allow it to find
+        # RemoteBzrDirFormat and make sure it finds the real underlying
format.
+        self._real_bzrdir = None

I dont get this XXX - could you explain it a little more, or file a bug
for it ?

+        #self._real_bzrdir._format.probe_transport(transport)

This comment can be removed

+        response = self._client.call('probe_dont_use', path)
+        if response == ('no',):
+            raise errors.NotBranchError(path=transport.base)

The probe_dont_use verb name should be changed. It seems like its a
useful API to keep. Perhaps 'BzrDir.open' is the right name.


+        if response == ('no',):
+            raise errors.NotBranchError(path=transport.base)

This is clearly faulty in the case of e.g. 'barfoo' coming back as the
response.

+    def _ensure_real(self):
+        """Ensure that there is a _real_bzrdir set.
+
+        used before calls to self._real_bzrdir.
+        """

Perhaps the second line could start with 'U' :) (And elsewhere in
remote.py)

+        if not self._real_bzrdir:
+            default_format = BzrDirFormat.get_default_format()
+            self._real_bzrdir =
default_format.open(self.root_transport,
+                _found=True)

Why does this get a format object rather than calling
'BzrDir.open_on_transport' ?


+    def create_repository(self, shared=False):
+        self._real_bzrdir.create_repository(shared=shared)
+        return self.open_repository()
+
+    def create_branch(self):
+        real_branch = self._real_bzrdir.create_branch()
+        return RemoteBranch(self, self.find_repository(), real_branch)
+
+    def create_workingtree(self, revision_id=None):
+        real_workingtree =
self._real_bzrdir.create_workingtree(revision_id=revision_id)
+        return RemoteWorkingTree(self, real_workingtree)
+
...

+    def get_branch_transport(self, branch_format):
+        return self._real_bzrdir.get_branch_transport(branch_format)
+
+    def get_repository_transport(self, repository_format):
+        return
self._real_bzrdir.get_repository_transport(repository_format)
+
+    def get_workingtree_transport(self, workingtree_format):
+        return
self._real_bzrdir.get_workingtree_transport(workingtree_format)
+

These should have an '_ensure_real' guard. Perhaps _ensure_real() should
return the thing, then you could do:
return self._ensure_real().get_workingtree_transport(workingtree_format)


+    For the moment everything is delegated to IO-like operations over
+    the transport.
+    """

'perhaps most things' rather than 'everything'. Or better 'For the momst
most operations are performed using local transport-backed Repository
objects.'


+    def get_revision_graph(self, revision_id=None):
The body of this method strongly reminds me of the protocol cleanup we
need. I think we should probably do that asap before merging the hpss
client side objects - it will make the cleanup I would be asking for
easier. (Which is - dont use assertions, catch errors correctly,
re-raise appropriate errors locally).

+            # FIXME
fix WHAT?

+    def lock_read(self):
+        # wrong eventually - want a local lock cache context

^ This should be clarified. Specifically it should either create its own
cache context as Repository does, or perhaps reuse the one of its
_ensure_real'd instance.

+            if mode != 'w':
+                return
This could use a comment - that only write lock remote repositories need
a method call to perform the unlock.




+        # XXX: This assumes that the branch control directory
is .bzr/branch,
+        # which isn't necessarily true.
+        LockableFiles.__init__(
+            self, bzrdir.root_transport.clone('.bzr/branch'),
+            'lock', lockdir.LockDir)

This should be fixed. Its easy: use the get_branch_transport method.


+        #branch.Branch.__init__(self)

Delete it or explain it :)


the unlock() method on both RemoteBranch and RemoteRepository should
probably discard the _real object when the final unlock occurs; perhaps
this is premature though. Just a randomish thought.


+    def test_open_remote_branch(self):
+        # open a standalone branch in the working directory
+        b = remote.RemoteBzrDir(self.transport)
+        branch = b.open_branch()

Shouldn't branch be checked for something? e.g. assertIsInstance?

+        # XXX: This seems more like a UI function than something that
really
+        # belongs in this class.

This should be a bug, not a XXX.

+# We can't use register_control_format because it adds it at a lower
priority
+# than the existing branches, whereas this should take priority.
+BzrDirFormat._control_formats.insert(0, RemoteBzrDirFormat)

Lets extend register_control_format.


+        if isinstance(to_repository, RemoteRepository):
+            to_repository._ensure_real()
+            to_repository = to_repository._real_repository
+        if isinstance(from_repository, RemoteRepository):
+            from_repository._ensure_real()
+            from_repository = from_repository._real_repository

^ this is bogus I think. Really it should be done by an
InterRemoteRepository which chooses to delegate to the real
repositories.

With everything I mention addressed, I'm +1 on this - but all the remote
methods need their error handling cleaned up - which is why I think that
should be done first.

-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/20070417/d80f7580/attachment.pgp