[MERGE][0.16] Smart server protocol versioning

[Andrew Bennetts]

Andrew Bennetts wrote:
> Martin Pool wrote:
[...]
> > 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.

Actually...

So 0.15 and older servers return a single line already if they don't understand
the request:

    $ echo garbage | bzr --no-plugins serve --inet | cat -v
    error^AGeneric bzr smart protocol error: bad request 'garbage'
    $

(both the request and response there are terminated with a newline)

The 0.16 server interprets any request that isn't explicitly versioned as
version one:

    $ echo garbage | ./bzr --no-plugins serve --inet | cat -v
    error^AGeneric bzr smart protocol error: bad request 'garbage'
   
i.e. servers old and new return a single line.  And all messages are
"understood", in the sense there's always a protocol version that interprets
them.  Currently the rule is that any request that isn't protocol 2 is
interpreted as protocol 1.

The code snippet you quoted was actually in the *client*, reading a response.

So all that's lacking is the "bzr server version 1.2.3 doesn't understand
protocol 4" message on the client, but we don't have this situation yet.  With
the "Allow new smart client to work with old (<= 0.15) smart servers" branch
this error never reaches the user because it automatically falls back to version
one.  So I don't think this makes sense yet.  I'm sure it will occur in future,
but we don't need to worry about it today.

If the fallback fails (so if we run bzr.dev, or even 0.15, against a
hypothetical version 7 server that doesn't support version 1 or 2), then the
first line of the response will be captured into a SmartProtocolError and
displayed to the user, so I think we're already ok.

I will update the protocol versioning docs to explicitly say that "unknown
request version" errors should be a single \n-terminated line for forward- and
backward-compatibility.

-Andrew.