[MERGE] external_references repository patch.

[Andrew Bennetts]

bb:tweak

Robert Collins wrote:
[...]
> === modified file 'bzrlib/remote.py'
> --- bzrlib/remote.py	2008-02-07 03:47:24 +0000
> +++ bzrlib/remote.py	2008-02-12 05:17:25 +0000
> @@ -148,16 +148,29 @@
>                  
>      def open_repository(self):
>          path = self._path_for_remote_call(self._client)
> -        response = self._client.call('BzrDir.find_repository', path)
> +        verb = 'BzrDir.find_repositoryV2'
> +        response = self._client.call(verb, path)
> +        if (response == ('error', "Generic bzr smart protocol error: "
> +                "bad request '%s'" % verb) or
> +              response == ('error', "Generic bzr smart protocol error: "
> +                "bad request u'%s'" % verb)):
> +            verb = 'BzrDir.find_repository'

You should use self._response_is_unknown_method for this.

> +            response = self._client.call(verb, path)
>          assert response[0] in ('ok', 'norepository'), \
>              'unexpected response code %s' % (response,)
>          if response[0] == 'norepository':
>              raise errors.NoRepositoryPresent(self)
> -        assert len(response) == 4, 'incorrect response length %s' % (response,)
> +        if verb == 'BzrDir.find_repository':
> +            # servers that don't support the V2 method don't support external
> +            # references either.
> +            response = response + ('no', )
> +        assert len(response) == 5, 'incorrect response length %s' % (response,)

This is a pre-existing flaw, but this really should be raising
SmartProtocolError or similar if the data off the network is malformed, rather
than relying on an assert statement.

(Hmm.  It may be worth having a different error to SmartProtocolError so we can
distinguish between serialisation errors and erroneous responses that are
correctly serialised; i.e. "the protocol encoding went wrong" vs. "the
request/response handler went wrong"...)

>          if response[1] == '':
>              format = RemoteRepositoryFormat()
>              format.rich_root_data = (response[2] == 'yes')
>              format.supports_tree_reference = (response[3] == 'yes')
> +            # No wire format to check this yet.
> +            format.supports_external_lookups = (response[4] == 'yes')

I'm not sure what that comment means.  What's “this”?

> === modified file 'bzrlib/tests/repository_implementations/test_repository.py'
> --- bzrlib/tests/repository_implementations/test_repository.py	2008-02-06 00:06:53 +0000
> +++ bzrlib/tests/repository_implementations/test_repository.py	2008-02-12 02:38:27 +0000
> @@ -267,6 +267,11 @@
>          text = repo._format.get_format_description()
>          self.failUnless(len(text))
>  
> +    def test_format_supports_external_lookups(self):
> +        repo = self.make_repository('.')
> +        self.assertSubset(
> +            [repo._format.supports_external_lookups], (True, False))

Hmm.  We probably should have "assertIn" or "assertContains", I think it would
be clearer for this.  (Not a requirement for this branch to merge, just me
making a wish...)

> === modified file 'bzrlib/tests/test_remote.py'
[...]

No complaints with the tests.  I like that you reuse the existing tests as much
as possible, getting good coverage for both versions of the method.

-Andrew.