[MERGE] [hpss part 2] Use the Command pattern for smart server request handling

[Andrew Bennetts]

John Arbash Meinel wrote:
> I really like the move towards the Command pattern.
> 
> As we move forward, I'm just curious if we have a direct test for the
> wire-level details of these commands. Just to make sure that as we
> 'upgrade' we don't break compatibility between versions. I'm not asking
> you to do so, just to confirm that we have (or don't have) it.

We do (but not in this bundle, obviously).

> > +++ bzrlib/smart/vfs.py
> > @@ -0,0 +1,198 @@
> 
> v- Copyright should include 2007.
> 
> > +# Copyright (C) 2006 Canonical Ltd

Fixed.

> > +def vfs_enabled():
> > +    """Is the VFS enabled ?
> > +
> > +    the VFS is disabled when the NO_SMART_VFS environment variable is set.
> > +
> > +    :return: True if it is enabled.
> > +    """
> > +    return not 'NO_SMART_VFS' in os.environ
> 
> ^- Any bzr related env var should probably be "BZR_*" so BZR_NO_SMART_VFS.

Ok.  Changed.

> v- Why did you downgrade to 2006?
> 
> > === modified file bzrlib/smart/request.py
> > --- bzrlib/smart/request.py
> > +++ bzrlib/smart/request.py
> > @@ -1,4 +1,4 @@
> > -# Copyright (C) 2006, 2007 Canonical Ltd
> > +# Copyright (C) 2006 Canonical Ltd

Mistake in merging the changes from hpss into bzr.dev.  Fixed.

> v- This also seems to be a "regression". In as much as you are more
> likely to get conflicts from updates to a:
> 
> from foo import a, b, c, d, e
> 
> rather than
> 
> from foo import (
>   a,
>   b,
>   c,
>   d,
>   )
> 
> Also, you can trivially keep the latter sorted in Vim by selecting the
> lines, and running :!sort
> 
> > -from bzrlib import (
> > -    bzrdir,
> > -    errors,
> > -    revision
> > -    )
> > +from bzrlib import bzrdir, errors, registry, revision

Fixed.

> v- Single line doc strings close the quotes on the same line.
> 
> > +class SmartServerRequest(object):
> > +    """Base class for request handlers.
> > +    """

Fixed.

> v- We have a history of not documenting __init__, but all of the
> documentation tools seem to complain about that. I'm usually not sure
> what to put there, but I suppose at least a description of ':param:' is
> useful. (minor)
> 
> > +    def __init__(self, backing_transport):
> > +        self._backing_transport = backing_transport

It is definitely useful to have parameter descriptions for __init__.  Putting a
summary of "Constructor." is short and accurate, so I usually do that if I want
to make documentation tools happy.

I added this docstring:

        """Constructor.

        :param backing_transport: the base transport to be used when performing
            this request.
        """

> v- Docstring?
> 
> > +class GetBundleRequest(SmartServerRequest):
> > +

Added:

    """Get a bundle of from the null revision to the specified revision."""

> v- Maybe we should have a naming structure for how to handle
> "development" verbs. Like starting them with an '_' :)
> 
> > +# This exists solely to help RemoteObjectHacking.  It should be removed
> > +# eventually.  It should not be considered part of the real smart server
> > +# protocol!
> > +class ProbeDontUseRequest(SmartServerRequest):

Do you mean for the class name, or the registration, or both?

Either way, it doesn't matter right now, because this request class doesn't
belong in this bundle because it isn't used by it.  I've removed it from this
branch.

> v- If you update the env var, don't forget about here.

Don't worry, I used grep :)

> v- If you are doing it in 'setUp()' do you need to do it in each test?
> 
> >      def test_get_error_unexpected(self):
> >          """Error reported by server with no specific representation"""
> > +        self._captureVar('NO_SMART_VFS', None)

I don't do it in this class' setUp.

> >          class FlakyTransport(object):
> >              base = 'a_url'
> >              def get_bytes(self, path):
> > @@ -865,12 +871,14 @@
> >  
> >      def test_smart_transport_has(self):
> >          """Checking for file existence over smart."""
> > +        self._captureVar('NO_SMART_VFS', None)

Nor in this one's.

Thanks for the review.  I'm sending the fixed branch to pqm.

-Andrew.