Merge lp:~spiv/bzr/hpss-perm-denied-during-push-create into lp:~bzr/bzr/trunk-old

[Andrew Bennetts]

[Andrew Bennetts (spiv)]

Catches and translates ErrorFromSmartServer in RemoteBzrDirFormat. This makes it consistent with other callers of _SmartClient.call (in bzrlib.remote and bzrlib.transport.remote), and fixes ugly tracebacks when creating new BzrDirs on a smart server.

This also adds some very basic unit tests for the BzrDirFormat.initialize_ex verb, and refactors the RemoteBzrDirFormat.initialize_on_transport_ex a little to make that testing easier. The test coverage here is far from great, but it is a least a small improvement.

[John A Meinel (jameinel)]

I thought we also were having some problems with "NoSuchDirectory" which seems a lot easier to test. Just use "trans = self.make_smart_server('path')" and then call initialize_on_transport_ex with a subdir.

Anyway, the change itself seems ok.

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-06-03 05:44:18 +0000
+++ NEWS	2009-06-03 07:35:51 +0000
@@ -62,6 +62,10 @@
   succeeded from the user's perspective, no output is written to stderr 
   or stdout.  (Maritza Mendez, #363837)
 
+* Translate errors received from a smart server in response to a
+  ``BzrDirFormat.initialize`` or ``BzrDirFormat.initialize_ex`` request.
+  This was causing tracebacks even for mundane errors like
+  ``PermissionDenied``.  (Andrew Bennetts, #381329)
 
 Documentation
 *************
 
=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2009-05-28 11:46:14 +0000
+++ bzrlib/bzrdir.py	2009-06-03 07:35:51 +0000
@@ -3049,7 +3049,10 @@
             return local_dir_format.initialize_on_transport(transport)
         client = _SmartClient(client_medium)
         path = client.remote_path_from_transport(transport)
-        response = client.call('BzrDirFormat.initialize', path)
+        try:
+            response = client.call('BzrDirFormat.initialize', path)
+        except errors.ErrorFromSmartServer, err:
+            remote._translate_error(err, path=path)
         if response[0] != 'ok':
             raise errors.SmartProtocolError('unexpected response code %s' % (response,))
         format = RemoteBzrDirFormat()
@@ -3115,6 +3118,13 @@
                 stack_on_pwd=stack_on_pwd, repo_format_name=repo_format_name,
                 make_working_trees=make_working_trees, shared_repo=shared_repo,
                 vfs_only=True)
+        return self._initialize_on_transport_ex_rpc(client, path, transport,
+            use_existing_dir, create_prefix, force_new_repo, stacked_on,
+            stack_on_pwd, repo_format_name, make_working_trees, shared_repo)
+
+    def _initialize_on_transport_ex_rpc(self, client, path, transport,
+        use_existing_dir, create_prefix, force_new_repo, stacked_on,
+        stack_on_pwd, repo_format_name, make_working_trees, shared_repo):
         args = []
         args.append(self._serialize_NoneTrueFalse(use_existing_dir))
         args.append(self._serialize_NoneTrueFalse(create_prefix))
@@ -3147,6 +3157,8 @@
                 stack_on_pwd=stack_on_pwd, repo_format_name=repo_format_name,
                 make_working_trees=make_working_trees, shared_repo=shared_repo,
                 vfs_only=True)
+        except errors.ErrorFromSmartServer, err:
+            remote._translate_error(err, path=path)
         repo_path = response[0]
         bzrdir_name = response[6]
         require_stacking = response[7]
 
=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py	2009-05-07 05:08:46 +0000
+++ bzrlib/tests/test_remote.py	2009-06-03 07:35:51 +0000
@@ -741,6 +741,61 @@
         self.assertEqual(network_name, repo._format.network_name())
 
 
+class TestBzrDirFormatInitializeEx(TestRemote):
+
+    def test_success(self):
+        """Simple test for typical successful call."""
+        fmt = bzrdir.RemoteBzrDirFormat()
+        default_format_name = BzrDirFormat.get_default_format().network_name()
+        transport = self.get_transport()
+        client = FakeClient(transport.base)
+        client.add_expected_call(
+            'BzrDirFormat.initialize_ex',
+                (default_format_name, 'path', 'False', 'False', 'False', '',
+                 '', '', '', 'False'),
+            'success',
+                ('.', 'no', 'no', 'yes', 'repo fmt', 'repo bzrdir fmt',
+                 'bzrdir fmt', 'False', '', '', 'repo lock token'))
+        # XXX: It would be better to call fmt.initialize_on_transport_ex, but
+        # it's currently hard to test that without supplying a real remote
+        # transport connected to a real server.
+        result = fmt._initialize_on_transport_ex_rpc(client, 'path',
+            transport, False, False, False, None, None, None, None, False)
+        client.finished_test()
+
+    def test_error(self):
+        """Error responses are translated, e.g. 'PermissionDenied' raises the
+        corresponding error from the client.
+        """
+        fmt = bzrdir.RemoteBzrDirFormat()
+        default_format_name = BzrDirFormat.get_default_format().network_name()
+        transport = self.get_transport()
+        client = FakeClient(transport.base)
+        client.add_expected_call(
+            'BzrDirFormat.initialize_ex',
+                (default_format_name, 'path', 'False', 'False', 'False', '',
+                 '', '', '', 'False'),
+            'error',
+                ('PermissionDenied', 'path', 'extra info'))
+        # XXX: It would be better to call fmt.initialize_on_transport_ex, but
+        # it's currently hard to test that without supplying a real remote
+        # transport connected to a real server.
+        err = self.assertRaises(errors.PermissionDenied,
+            fmt._initialize_on_transport_ex_rpc, client, 'path', transport,
+            False, False, False, None, None, None, None, False)
+        self.assertEqual('path', err.path)
+        self.assertEqual(': extra info', err.extra)
+        client.finished_test()
+
+    def test_error_from_real_server(self):
+        """Integration test for error translation."""
+        transport = self.make_smart_server('foo')
+        transport = transport.clone('no-such-path')
+        fmt = bzrdir.RemoteBzrDirFormat()
+        err = self.assertRaises(errors.NoSuchFile,
+            fmt.initialize_on_transport_ex, transport, create_prefix=False)
+
+
 class OldSmartClient(object):
     """A fake smart client for test_old_version that just returns a version one
     response to the 'hello' (query version) command.