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

Proposed by Andrew Bennetts on 2009-06-02
Status: Merged
Approved by: John A Meinel on 2009-06-02
Approved revision: 4386
Merged at revision: not available
Proposed branch: lp:~spiv/bzr/hpss-perm-denied-during-push-create
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 119 lines
To merge this branch: bzr merge lp:~spiv/bzr/hpss-perm-denied-during-push-create
Reviewer Review Type Date Requested Status
John A Meinel 2009-06-02 Approve on 2009-06-02
Review via email: mp+6959@code.launchpad.net
To post a comment you must log in.
Andrew Bennetts (spiv) wrote :

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) wrote :

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
4387. By Andrew Bennetts on 2009-06-03

Add test suggested by John.

4388. By Andrew Bennetts on 2009-06-03

Merge, resolving NEWS conflict.

4389. By Andrew Bennetts on 2009-06-03

Merge, resolving NEWS conflict.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-06-03 05:44:18 +0000
3+++ NEWS 2009-06-03 07:35:51 +0000
4@@ -62,6 +62,10 @@
5 succeeded from the user's perspective, no output is written to stderr
6 or stdout. (Maritza Mendez, #363837)
7
8+* Translate errors received from a smart server in response to a
9+ ``BzrDirFormat.initialize`` or ``BzrDirFormat.initialize_ex`` request.
10+ This was causing tracebacks even for mundane errors like
11+ ``PermissionDenied``. (Andrew Bennetts, #381329)
12
13 Documentation
14 *************
15
16=== modified file 'bzrlib/bzrdir.py'
17--- bzrlib/bzrdir.py 2009-05-28 11:46:14 +0000
18+++ bzrlib/bzrdir.py 2009-06-03 07:35:51 +0000
19@@ -3049,7 +3049,10 @@
20 return local_dir_format.initialize_on_transport(transport)
21 client = _SmartClient(client_medium)
22 path = client.remote_path_from_transport(transport)
23- response = client.call('BzrDirFormat.initialize', path)
24+ try:
25+ response = client.call('BzrDirFormat.initialize', path)
26+ except errors.ErrorFromSmartServer, err:
27+ remote._translate_error(err, path=path)
28 if response[0] != 'ok':
29 raise errors.SmartProtocolError('unexpected response code %s' % (response,))
30 format = RemoteBzrDirFormat()
31@@ -3115,6 +3118,13 @@
32 stack_on_pwd=stack_on_pwd, repo_format_name=repo_format_name,
33 make_working_trees=make_working_trees, shared_repo=shared_repo,
34 vfs_only=True)
35+ return self._initialize_on_transport_ex_rpc(client, path, transport,
36+ use_existing_dir, create_prefix, force_new_repo, stacked_on,
37+ stack_on_pwd, repo_format_name, make_working_trees, shared_repo)
38+
39+ def _initialize_on_transport_ex_rpc(self, client, path, transport,
40+ use_existing_dir, create_prefix, force_new_repo, stacked_on,
41+ stack_on_pwd, repo_format_name, make_working_trees, shared_repo):
42 args = []
43 args.append(self._serialize_NoneTrueFalse(use_existing_dir))
44 args.append(self._serialize_NoneTrueFalse(create_prefix))
45@@ -3147,6 +3157,8 @@
46 stack_on_pwd=stack_on_pwd, repo_format_name=repo_format_name,
47 make_working_trees=make_working_trees, shared_repo=shared_repo,
48 vfs_only=True)
49+ except errors.ErrorFromSmartServer, err:
50+ remote._translate_error(err, path=path)
51 repo_path = response[0]
52 bzrdir_name = response[6]
53 require_stacking = response[7]
54
55=== modified file 'bzrlib/tests/test_remote.py'
56--- bzrlib/tests/test_remote.py 2009-05-07 05:08:46 +0000
57+++ bzrlib/tests/test_remote.py 2009-06-03 07:35:51 +0000
58@@ -741,6 +741,61 @@
59 self.assertEqual(network_name, repo._format.network_name())
60
61
62+class TestBzrDirFormatInitializeEx(TestRemote):
63+
64+ def test_success(self):
65+ """Simple test for typical successful call."""
66+ fmt = bzrdir.RemoteBzrDirFormat()
67+ default_format_name = BzrDirFormat.get_default_format().network_name()
68+ transport = self.get_transport()
69+ client = FakeClient(transport.base)
70+ client.add_expected_call(
71+ 'BzrDirFormat.initialize_ex',
72+ (default_format_name, 'path', 'False', 'False', 'False', '',
73+ '', '', '', 'False'),
74+ 'success',
75+ ('.', 'no', 'no', 'yes', 'repo fmt', 'repo bzrdir fmt',
76+ 'bzrdir fmt', 'False', '', '', 'repo lock token'))
77+ # XXX: It would be better to call fmt.initialize_on_transport_ex, but
78+ # it's currently hard to test that without supplying a real remote
79+ # transport connected to a real server.
80+ result = fmt._initialize_on_transport_ex_rpc(client, 'path',
81+ transport, False, False, False, None, None, None, None, False)
82+ client.finished_test()
83+
84+ def test_error(self):
85+ """Error responses are translated, e.g. 'PermissionDenied' raises the
86+ corresponding error from the client.
87+ """
88+ fmt = bzrdir.RemoteBzrDirFormat()
89+ default_format_name = BzrDirFormat.get_default_format().network_name()
90+ transport = self.get_transport()
91+ client = FakeClient(transport.base)
92+ client.add_expected_call(
93+ 'BzrDirFormat.initialize_ex',
94+ (default_format_name, 'path', 'False', 'False', 'False', '',
95+ '', '', '', 'False'),
96+ 'error',
97+ ('PermissionDenied', 'path', 'extra info'))
98+ # XXX: It would be better to call fmt.initialize_on_transport_ex, but
99+ # it's currently hard to test that without supplying a real remote
100+ # transport connected to a real server.
101+ err = self.assertRaises(errors.PermissionDenied,
102+ fmt._initialize_on_transport_ex_rpc, client, 'path', transport,
103+ False, False, False, None, None, None, None, False)
104+ self.assertEqual('path', err.path)
105+ self.assertEqual(': extra info', err.extra)
106+ client.finished_test()
107+
108+ def test_error_from_real_server(self):
109+ """Integration test for error translation."""
110+ transport = self.make_smart_server('foo')
111+ transport = transport.clone('no-such-path')
112+ fmt = bzrdir.RemoteBzrDirFormat()
113+ err = self.assertRaises(errors.NoSuchFile,
114+ fmt.initialize_on_transport_ex, transport, create_prefix=False)
115+
116+
117 class OldSmartClient(object):
118 """A fake smart client for test_old_version that just returns a version one
119 response to the 'hello' (query version) command.