[MERGE] Better infrastructure for dealing with 'bad request' responses from a smart server
John Arbash Meinel
john at arbash-meinel.com
Wed Apr 9 08:50:10 BST 2008
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Andrew Bennetts wrote:
| This patch provides a more convenient and robust way to handle responses from a
| smart server that say that the server doesn't recognise the request (because the
| server version is too old). This is important, because detecting this error is
| how the client knows it needs to retry the request using older verbs.
|
| Previously this was done ad hoc in bzrlib/remote.py and
| bzrlib/transport/remote.py. This patch instead makes the client protocol
| responsible for detecting the error: read_response_tuple will now raise
| UnknownSmartMethod (a new exception) if instead of an ordinary response it gets
| that error from the server.
|
| Aside from being a nicer API, a big advantage of this changes is that knowledge
| of the protocol-level details of how this error is communicated over the wire is
| now confined to the protocol logic. This will make it easier to improve this
| aspect of the protocol in the next protocol version.
|
| -Andrew.
|
|
To start with, I think this is a great improvement. I'm glad you can bring it in
before the v3 stuff. Though it is probably a bit easier there.
I certainly prefer having a specific error to each location special-casing itself.
I also think that try/except is better for this sort of handling than 'response
== ('error', ...)"
Especially given that you have to check the exact string of the response.
I'm not 100% sure about some of the test case changes, namely this one:
@@ -536,23 +563,7 @@
~ advisory anyway (a transport could be read-write, but then the
~ underlying filesystem could be readonly anyway).
~ """
- - client = FakeClient([(
- - ('error', "Generic bzr smart protocol error: "
- - "bad request 'Transport.is_readonly'"), '')])
- - transport = RemoteTransport('bzr://example.com/', medium=False,
- - _client=client)
- - self.assertEqual(False, transport.is_readonly())
- - self.assertEqual(
- - [('call', 'Transport.is_readonly', ())],
- - client._calls)
- -
- - def test_error_from_old_0_11_server(self):
- - """Same as test_error_from_old_server, but with the slightly different
- - error message from bzr 0.11 servers.
- - """
- - client = FakeClient([(
- - ('error', "Generic bzr smart protocol error: "
- - "bad request u'Transport.is_readonly'"), '')])
+ client = FakeClient([(('unknown verb', 'Transport.is_readonly'), '')])
~ transport = RemoteTransport('bzr://example.com/', medium=False,
~ _client=client)
~ self.assertEqual(False, transport.is_readonly())
But as I'm not 100% sure what you are doing, I would trust your knowledge first.
The rest looks good so:
BB:approve
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFH/HUyJdeBCYSNAAMRAi3UAJ44k7za3CGDZE9VNBQcbGeLpyCxkQCfZHLe
ElBb4HGofr3glAUwND/TSGs=
=5ZRl
-----END PGP SIGNATURE-----
More information about the bazaar
mailing list