[MERGE][0.16] Smart server protocol versioning

[Andrew Bennetts]

Martin Pool wrote:
> Martin Pool has voted +0.
> Status is now: Semi-approved
> Comment:
> I think having an explicit version marker would be very good.
> 
> If we're going to have one I would rather it was more explicit and less 
> likely to accidentally match than just '2\n', like say 'bzr remote 2\n'. 
> This in turn should be a symbolic constant, not hardcoded in a couple of 
> places, and certainly get rid of the magic 2 meaning two bytes.

Ok.  In fact, there are now two strings (with corresponding constants):

REQUEST_VERSION_TWO: 'bzr request 2\n'
RESPONSE_VERSION_TWO: 'bzr response 2\n'

Making this prefix larger it occured to me that a version 2 server couldn't just
depend on reading len(REQUEST_VERSION_TWO) bytes to identify the protocol,
because a complete version 1 request could be smaller than that (e.g.
'hello\n'), so I also updated the logic to read until \n, and treat that as the
version string.

(Doing the same on the client side also ties in nicely with your suggestion
about error handling below.)

> This deserves a mention in NEWS.

Done.

> There should also be a mention in a docstring or hacking of the way 
> protocol versioning works - at least that there are two, and also the 
> design rule we agreed today but that wasn't mentioned here as far as i 
> can see, that the server must always answer in the same version as the 
> request.

Done.  I've updated the big overview docstring in bzrlib.smart with this
information, and correctly some other out of date info while I was there.  I
think it's much clearer now.

> That leaves open the question of what the server is meant to do if it 
> doesn't understand the request - both for <=0.15 servers, and if we add 
> new protocols in the future.  It would be nice if this caused a clean 
> error on the client like "bzr server version 1.2.3 doesn't understand 
> protocol 4" - then the client can either fall back or just present that 
> to the user.
> 
> At the moment you do
> 
> +        version = self._request.read_bytes(2)
> +        if version != SmartClientRequestProtocolTwo._version_string:
> +            raise errors.SmartProtocolError('bad protocol marker %r' % 
> version)
> 
> How about this: if the server doesn't understand the request protocol at 
> all (as opposed to an error later on) it returns a single line error, 
> rather than a version marker.  We can present that to the user.  0.15 
> will come tolerably close to that behaviour if we just clean out the 
> encoding of its error before presenting it.

I think that's a good idea.  I'll do that.

-Andrew.