[MERGE] Better infrastructure for dealing with 'bad request' responses from a smart server

[John Arbash Meinel]

-----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-----