[MERGE][#230550] Fix spurious "No repository present" errors when accessing branches in shared repos via bzr+http.
Andrew Bennetts
andrew at canonical.com
Tue May 20 01:46:57 BST 2008
John Arbash Meinel wrote:
[...]
>
> A lot of these seem like incompatible API changes. Are they all on private
> classes? I'm looking at stuff like:
>
> ~ class SmartTCPClientMedium(SmartClientStreamMedium):
> ~ """A client medium using TCP."""
>
> - - def __init__(self, host, port):
> + def __init__(self, host, port, base):
>
> ^- Which now has an extra required parameter. I realize this isn't really a
> candidate for subclassing like Repository is. But AFAICT it *is* an API break,
> and should thus be documented in NEWS.
Good point. Documented.
> SmartClient.remote_path_from_transport
>
> sort of makes me want to cry. Why does it have to know so much about the URLs it
> supports. Why does it treat "bzr://" so different from "bzr+http://"? It just
> feels dirty, with the knowledge in the wrong location. I won't say it isn't
> necessary, and certainly the function already exists and is being used.
Yeah, I don't like it much either. But to calculate a remote path given a full
URL (i.e. a transport) you need to use the base path of the smart medium. Now
that 'base' is on SmartClientMedium directly, perhaps this can be pushed down
to the individual SmartClientMedium subclasses. Hmm...
Ok, I've done that. It wasn't too painful given that I'd already moved 'base',
and now the special logic for bzr+http:// is confined to HttpTransportBase.
> Is there any way we could have a couple more smoke tests for round-tripping
> 'bzr+http' ? I feel like it keeps breaking, and the test suite should be
> catching this for us.
>
> Maybe we need implementation tests for "SmartMediums" or something like that.
> (Since that would have caught the _remote_is_at_least_1_2, that broke HTTPUrllib
> and HTTPPycurl). I certainly feel like our test infrastructure is heavily
> lacking if every release keeps breaking bzr+http.
I would like to improve the testing too. On the plus side, manual testing is
finally happening more often now that bzr+http is actually usable. If
http://bazaar.launchpad.net/ had the smart server re-enabled, we'd probably get
lots of testing by users for "free”...
I would like to improve the automated tests sometime, but I have other
priorities this week :/
> BB:approve
>
> For this patch, but I would really like us to find an answer for these issues.
Thanks! I'll merge it with the changes mentioned above. Incremental diff
attached for the curious.
-Andrew.
-------------- next part --------------
=== modified file 'NEWS'
--- NEWS 2008-05-19 23:54:37 +0000
+++ NEWS 2008-05-20 00:03:23 +0000
@@ -60,7 +60,8 @@
(Robert Collins)
* The constructors of ``SmartClientMedium`` and its subclasses now require a
- ``base`` parameter. (Andrew Bennetts)
+ ``base`` parameter. ``SmartClientMedium`` implementations now also need
+ to provide a ``remote_path_from_transport`` method. (Andrew Bennetts)
* ``VersionedFile.join`` is deprecated. This method required local
instances of both versioned file objects and was thus hostile to being
=== modified file 'bzrlib/smart/client.py'
--- bzrlib/smart/client.py 2008-05-19 08:42:32 +0000
+++ bzrlib/smart/client.py 2008-05-20 00:07:00 +0000
@@ -14,12 +14,10 @@
# along with this program; if not, write to the Free Software
# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
-import urllib
-
import bzrlib
from bzrlib.smart import message, protocol
from bzrlib.trace import warning
-from bzrlib import urlutils, errors
+from bzrlib import errors
class _SmartClient(object):
@@ -155,21 +153,5 @@
anything but path, so it is only safe to use it in requests sent over
the medium from the matching transport.
"""
- base = self._medium.base
- if base.startswith('bzr+'):
- base = base[4:]
- if (base.startswith('http://') or base.startswith('https://')):
- # XXX: There seems to be a bug here: http+urllib:// and
- # http+pycurl:// ought to be treated the same as http://, I think.
- # - Andrew Bennetts, 2008-05-19.
- medium_base = base
- else:
- medium_base = urlutils.join(base, '/')
-
- transport_base = transport.base
- if transport_base.startswith('bzr+'):
- transport_base = transport_base[4:]
-
- rel_url = urlutils.relative_url(medium_base, transport_base)
- return urllib.unquote(rel_url)
+ return self._medium.remote_path_from_transport(transport)
=== modified file 'bzrlib/smart/medium.py'
--- bzrlib/smart/medium.py 2008-05-19 10:55:48 +0000
+++ bzrlib/smart/medium.py 2008-05-20 00:40:59 +0000
@@ -27,11 +27,13 @@
import os
import socket
import sys
+import urllib
from bzrlib import (
errors,
osutils,
symbol_versioning,
+ urlutils,
)
from bzrlib.smart.protocol import (
MESSAGE_VERSION_THREE,
@@ -479,6 +481,17 @@
The default implementation does nothing.
"""
+ def remote_path_from_transport(self, transport):
+ """Convert transport into a path suitable for using in a request.
+
+ Note that the resulting remote path doesn't encode the host name or
+ anything but path, so it is only safe to use it in requests sent over
+ the medium from the matching transport.
+ """
+ medium_base = urlutils.join(self.base, '/')
+ rel_url = urlutils.relative_url(medium_base, transport.base)
+ return urllib.unquote(rel_url)
+
class SmartClientStreamMedium(SmartClientMedium):
"""Stream based medium common class.
=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py 2008-05-19 08:42:32 +0000
+++ bzrlib/tests/test_remote.py 2008-05-20 00:34:41 +0000
@@ -46,7 +46,7 @@
from bzrlib.smart import server, medium
from bzrlib.smart.client import _SmartClient
from bzrlib.symbol_versioning import one_four
-from bzrlib.transport import get_transport
+from bzrlib.transport import get_transport, http
from bzrlib.transport.memory import MemoryTransport
from bzrlib.transport.remote import RemoteTransport, RemoteTCPTransport
@@ -184,7 +184,7 @@
return result[1], FakeProtocol(result[2], self)
-class FakeMedium(object):
+class FakeMedium(medium.SmartClientMedium):
def __init__(self, client_calls, base):
self._remote_is_at_least_1_2 = True
@@ -209,38 +209,49 @@
self.assertTrue(result)
-class Test_SmartClient_remote_path_from_transport(tests.TestCase):
- """Tests for the behaviour of _SmartClient.remote_path_from_transport."""
+class Test_ClientMedium_remote_path_from_transport(tests.TestCase):
+ """Tests for the behaviour of client_medium.remote_path_from_transport."""
def assertRemotePath(self, expected, client_base, transport_base):
- """Assert that the result of _SmartClient.remote_path_from_transport
- is the expected value for a given client_base and transport_base.
+ """Assert that the result of
+ SmartClientMedium.remote_path_from_transport is the expected value for
+ a given client_base and transport_base.
"""
- class DummyMedium(object):
- base = client_base
- client = _SmartClient(DummyMedium())
+ client_medium = medium.SmartClientMedium(client_base)
transport = get_transport(transport_base)
- result = client.remote_path_from_transport(transport)
+ result = client_medium.remote_path_from_transport(transport)
self.assertEqual(expected, result)
-
+
def test_remote_path_from_transport(self):
- """_SmartClient.remote_path_from_transport calculates a URL for the
- given transport relative to the root of the client base URL.
+ """SmartClientMedium.remote_path_from_transport calculates a URL for
+ the given transport relative to the root of the client base URL.
"""
self.assertRemotePath('xyz/', 'bzr://host/path', 'bzr://host/xyz')
self.assertRemotePath(
'path/xyz/', 'bzr://host/path', 'bzr://host/path/xyz')
+ def assertRemotePathHTTP(self, expected, transport_base, relpath):
+ """Assert that the result of
+ HttpTransportBase.remote_path_from_transport is the expected value for
+ a given transport_base and relpath of that transport. (Note that
+ HttpTransportBase is a subclass of SmartClientMedium)
+ """
+ base_transport = get_transport(transport_base)
+ client_medium = base_transport.get_smart_medium()
+ cloned_transport = base_transport.clone(relpath)
+ result = client_medium.remote_path_from_transport(cloned_transport)
+ self.assertEqual(expected, result)
+
def test_remote_path_from_transport_http(self):
"""Remote paths for HTTP transports are calculated differently to other
transports. They are just relative to the client base, not the root
directory of the host.
"""
for scheme in ['http:', 'https:', 'bzr+http:', 'bzr+https:']:
- self.assertRemotePath(
- '../xyz/', scheme + '//host/path', scheme + '//host/xyz')
- self.assertRemotePath(
- 'xyz/', scheme + '//host/path', scheme + '//host/path/xyz')
+ self.assertRemotePathHTTP(
+ '../xyz/', scheme + '//host/path', '../xyz/')
+ self.assertRemotePathHTTP(
+ 'xyz/', scheme + '//host/path', 'xyz/')
class TestBzrDirOpenBranch(tests.TestCase):
=== modified file 'bzrlib/transport/http/__init__.py'
--- bzrlib/transport/http/__init__.py 2008-05-19 10:55:48 +0000
+++ bzrlib/transport/http/__init__.py 2008-05-20 00:12:11 +0000
@@ -527,6 +527,15 @@
def should_probe(self):
return True
+ def remote_path_from_transport(self, transport):
+ # Strip the optional 'bzr+' prefix from transport so it will have the
+ # same scheme as self.
+ transport_base = transport.base
+ if transport_base.startswith('bzr+'):
+ transport_base = transport_base[4:]
+ rel_url = urlutils.relative_url(self.base, transport_base)
+ return urllib.unquote(rel_url)
+
class SmartClientHTTPMediumRequest(medium.SmartClientMediumRequest):
"""A SmartClientMediumRequest that works with an HTTP medium."""
More information about the bazaar
mailing list