Merge lp:~gz/bzr/smarter_smart_error_reporting_722416_B into lp:bzr
- smarter_smart_error_reporting_722416_B
- Merge into bzr.dev
Status: | Merged |
---|---|
Merged at revision: | 5694 |
Proposed branch: | lp:~gz/bzr/smarter_smart_error_reporting_722416_B |
Merge into: | lp:bzr |
Diff against target: |
245 lines (+95/-35) 5 files modified
bzrlib/remote.py (+21/-14) bzrlib/smart/message.py (+10/-20) bzrlib/smart/request.py (+3/-0) bzrlib/tests/test_remote.py (+58/-1) bzrlib/tests/test_smart_request.py (+3/-0) |
To merge this branch: | bzr merge lp:~gz/bzr/smarter_smart_error_reporting_722416_B |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
bzr-core | preliminary | Pending | |
Review via email: mp+51666@code.launchpad.net |
Commit message
Description of the change
This is step 3 (really) in fixing bug 722416. The aim of the branch is to turn an remote response in the form `('MemoryError',)` into a useful message for the user. I'm pretty sure the code is incorrect or incomplete however.
I've got two main problems here. Firstly, there seem to be two different _translate_error functions for the local-side error interpretation. Secondly, I can only reproduce one of the codepaths from the bug reports in the tests.
The stack I can test is going through autopack as-like bug 602614, which uses the bzrlib.
Note re-raising MemoryError locally isn't what we want to do, that's not the starving python. A new error class could be created though. Is there any additional 'advice' it would be useful to show to the user? Perhaps a suggestion to try the operation again locally or with 'nosmart+' instead?
Andrew Bennetts (spiv) wrote : | # |
Martin Packman (gz) wrote : | # |
> The intent was that message.
> error translations, i.e. error translations that are independent of the
> request
> that triggered them, such as UnknownMethod. Then the translations that want
> access to the branch object or whatever belong in remote.
> you're right that message.
This helps, thanks. As a MemoryError could be raised from any command, it makes sense for it to be re-raised from the context-free translation function. However, see the ☃ question below.
> I'm currently inclined to say:
>
> * leave the UnknownMethod and ErrorFromSmartS
> message.
> well
> tested.
> * move the context-free translations into a new function, probably in
> remote.py
> * invoke that new function as part of remote.
I'll do this for now, and resist the urge to pointlessly rewrite in a more functional style.
> > Note re-raising MemoryError locally isn't what we want to do, that's not the
> > starving python. A new error class could be created though. Is there any
> > additional 'advice' it would be useful to show to the user? Perhaps a
> > suggestion to try the operation again locally or with 'nosmart+' instead?
Your thoughts in response to this much the same as mine, so will just add a little more text and leave at that.
> It's probably a good idea for the error to say which remote server, though.
> Various scripts, plugins, stacking configurations, etc, may have multiple
> connections active at one time, and the user probably wants to know which one
> had the problem :)
That's a good point. Though... ☃ asks doesn't that mean we do want context then? It also goes for the other general errors that are being re-raised, so perhaps a different approach might be helpful in the future.
Martin Packman (gz) wrote : | # |
Sorry, bad diff in email again, launchpad hadn't finished updating the branch when it got sent out apparently, see the mp for the updates.
Andrew Bennetts (spiv) wrote : | # |
Martin [gz] wrote:
[...]
> > It's probably a good idea for the error to say which remote server, though.
> > Various scripts, plugins, stacking configurations, etc, may have multiple
> > connections active at one time, and the user probably wants to know which one
> > had the problem :)
>
> That's a good point. Though... ☃ asks doesn't that mean we do want
> context then? It also goes for the other general errors that are being
> re-raised, so perhaps a different approach might be helpful in the
> future.
I suppose you're right! Although there's sort of different levels of
context...
I think part of the reason I had the distinct message.
vs. remote.
bzr model stuff out of the basic protocol. i.e. there's no reason why
the message layer, which is about taking simple structures off the wire
and turning them into events like “here's a request body” should know
anything about what NoSuchRevision means.
I don't think I thought through that particular refactoring carefully at
the time, and so conflated “doesn't need context stuff from remote.py”
with “message-level logic.” But it was a step towards that, and now
with your patches I think it's actually pretty right: the message layer
knows that an error is either UnknownMethodError, or else it is a
ErrorFromSmartS
It's not really a goal, but if you like you can imagine a design
principle here as being “the smart protocol stuff could be used to write
useful servers and clients that have nothing to do with bzr.” So
bzrlib.
most of bzrlib.
nothing-
bzrlib.remote and bzrlib.
side.
That's a long winded way of saying: yes, I think you're right that the
medium or its URL or something probably belongs in the context, and that
the MemoryError translation (and possibly others) should use that.
And you're making me think that the no-context distinction isn't
important at all (now that I've figured out that it was the “belongs in
bzrlib.
though I had it a bit wrong).
-Andrew.
Preview Diff
1 | === modified file 'bzrlib/remote.py' | |||
2 | --- bzrlib/remote.py 2011-02-19 17:37:45 +0000 | |||
3 | +++ bzrlib/remote.py 2011-03-02 20:20:52 +0000 | |||
4 | @@ -2973,10 +2973,7 @@ | |||
5 | 2973 | 'Missing key %r in context %r', key_err.args[0], context) | 2973 | 'Missing key %r in context %r', key_err.args[0], context) |
6 | 2974 | raise err | 2974 | raise err |
7 | 2975 | 2975 | ||
12 | 2976 | if err.error_verb == 'IncompatibleRepositories': | 2976 | if err.error_verb == 'NoSuchRevision': |
9 | 2977 | raise errors.IncompatibleRepositories(err.error_args[0], | ||
10 | 2978 | err.error_args[1], err.error_args[2]) | ||
11 | 2979 | elif err.error_verb == 'NoSuchRevision': | ||
13 | 2980 | raise NoSuchRevision(find('branch'), err.error_args[0]) | 2977 | raise NoSuchRevision(find('branch'), err.error_args[0]) |
14 | 2981 | elif err.error_verb == 'nosuchrevision': | 2978 | elif err.error_verb == 'nosuchrevision': |
15 | 2982 | raise NoSuchRevision(find('repository'), err.error_args[0]) | 2979 | raise NoSuchRevision(find('repository'), err.error_args[0]) |
16 | @@ -2989,22 +2986,12 @@ | |||
17 | 2989 | detail=extra) | 2986 | detail=extra) |
18 | 2990 | elif err.error_verb == 'norepository': | 2987 | elif err.error_verb == 'norepository': |
19 | 2991 | raise errors.NoRepositoryPresent(find('bzrdir')) | 2988 | raise errors.NoRepositoryPresent(find('bzrdir')) |
20 | 2992 | elif err.error_verb == 'LockContention': | ||
21 | 2993 | raise errors.LockContention('(remote lock)') | ||
22 | 2994 | elif err.error_verb == 'UnlockableTransport': | 2989 | elif err.error_verb == 'UnlockableTransport': |
23 | 2995 | raise errors.UnlockableTransport(find('bzrdir').root_transport) | 2990 | raise errors.UnlockableTransport(find('bzrdir').root_transport) |
24 | 2996 | elif err.error_verb == 'LockFailed': | ||
25 | 2997 | raise errors.LockFailed(err.error_args[0], err.error_args[1]) | ||
26 | 2998 | elif err.error_verb == 'TokenMismatch': | 2991 | elif err.error_verb == 'TokenMismatch': |
27 | 2999 | raise errors.TokenMismatch(find('token'), '(remote token)') | 2992 | raise errors.TokenMismatch(find('token'), '(remote token)') |
28 | 3000 | elif err.error_verb == 'Diverged': | 2993 | elif err.error_verb == 'Diverged': |
29 | 3001 | raise errors.DivergedBranches(find('branch'), find('other_branch')) | 2994 | raise errors.DivergedBranches(find('branch'), find('other_branch')) |
30 | 3002 | elif err.error_verb == 'TipChangeRejected': | ||
31 | 3003 | raise errors.TipChangeRejected(err.error_args[0].decode('utf8')) | ||
32 | 3004 | elif err.error_verb == 'UnstackableBranchFormat': | ||
33 | 3005 | raise errors.UnstackableBranchFormat(*err.error_args) | ||
34 | 3006 | elif err.error_verb == 'UnstackableRepositoryFormat': | ||
35 | 3007 | raise errors.UnstackableRepositoryFormat(*err.error_args) | ||
36 | 3008 | elif err.error_verb == 'NotStacked': | 2995 | elif err.error_verb == 'NotStacked': |
37 | 3009 | raise errors.NotStacked(branch=find('branch')) | 2996 | raise errors.NotStacked(branch=find('branch')) |
38 | 3010 | elif err.error_verb == 'PermissionDenied': | 2997 | elif err.error_verb == 'PermissionDenied': |
39 | @@ -3020,6 +3007,24 @@ | |||
40 | 3020 | elif err.error_verb == 'NoSuchFile': | 3007 | elif err.error_verb == 'NoSuchFile': |
41 | 3021 | path = get_path() | 3008 | path = get_path() |
42 | 3022 | raise errors.NoSuchFile(path) | 3009 | raise errors.NoSuchFile(path) |
43 | 3010 | _translate_error_without_context(err) | ||
44 | 3011 | |||
45 | 3012 | |||
46 | 3013 | def _translate_error_without_context(err): | ||
47 | 3014 | """Translate any ErrorFromSmartServer values that don't require context""" | ||
48 | 3015 | if err.error_verb == 'IncompatibleRepositories': | ||
49 | 3016 | raise errors.IncompatibleRepositories(err.error_args[0], | ||
50 | 3017 | err.error_args[1], err.error_args[2]) | ||
51 | 3018 | elif err.error_verb == 'LockContention': | ||
52 | 3019 | raise errors.LockContention('(remote lock)') | ||
53 | 3020 | elif err.error_verb == 'LockFailed': | ||
54 | 3021 | raise errors.LockFailed(err.error_args[0], err.error_args[1]) | ||
55 | 3022 | elif err.error_verb == 'TipChangeRejected': | ||
56 | 3023 | raise errors.TipChangeRejected(err.error_args[0].decode('utf8')) | ||
57 | 3024 | elif err.error_verb == 'UnstackableBranchFormat': | ||
58 | 3025 | raise errors.UnstackableBranchFormat(*err.error_args) | ||
59 | 3026 | elif err.error_verb == 'UnstackableRepositoryFormat': | ||
60 | 3027 | raise errors.UnstackableRepositoryFormat(*err.error_args) | ||
61 | 3023 | elif err.error_verb == 'FileExists': | 3028 | elif err.error_verb == 'FileExists': |
62 | 3024 | raise errors.FileExists(err.error_args[0]) | 3029 | raise errors.FileExists(err.error_args[0]) |
63 | 3025 | elif err.error_verb == 'DirectoryNotEmpty': | 3030 | elif err.error_verb == 'DirectoryNotEmpty': |
64 | @@ -3044,4 +3049,6 @@ | |||
65 | 3044 | raise UnicodeEncodeError(encoding, val, start, end, reason) | 3049 | raise UnicodeEncodeError(encoding, val, start, end, reason) |
66 | 3045 | elif err.error_verb == 'ReadOnlyError': | 3050 | elif err.error_verb == 'ReadOnlyError': |
67 | 3046 | raise errors.TransportNotPossible('readonly transport') | 3051 | raise errors.TransportNotPossible('readonly transport') |
68 | 3052 | elif err.error_verb == 'MemoryError': | ||
69 | 3053 | raise errors.BzrError("remote server out of memory") | ||
70 | 3047 | raise errors.UnknownErrorFromSmartServer(err) | 3054 | raise errors.UnknownErrorFromSmartServer(err) |
71 | 3048 | 3055 | ||
72 | === modified file 'bzrlib/smart/message.py' | |||
73 | --- bzrlib/smart/message.py 2009-09-11 06:07:05 +0000 | |||
74 | +++ bzrlib/smart/message.py 2011-03-02 20:20:52 +0000 | |||
75 | @@ -303,7 +303,7 @@ | |||
76 | 303 | mutter(' result: %r', self.args) | 303 | mutter(' result: %r', self.args) |
77 | 304 | if self.status == 'E': | 304 | if self.status == 'E': |
78 | 305 | self._wait_for_response_end() | 305 | self._wait_for_response_end() |
80 | 306 | _translate_error(self.args) | 306 | _raise_smart_server_error(self.args) |
81 | 307 | return tuple(self.args) | 307 | return tuple(self.args) |
82 | 308 | 308 | ||
83 | 309 | def read_body_bytes(self, count=-1): | 309 | def read_body_bytes(self, count=-1): |
84 | @@ -335,27 +335,17 @@ | |||
85 | 335 | yield bytes_part | 335 | yield bytes_part |
86 | 336 | self._read_more() | 336 | self._read_more() |
87 | 337 | if self._body_stream_status == 'E': | 337 | if self._body_stream_status == 'E': |
89 | 338 | _translate_error(self._body_error_args) | 338 | _raise_smart_server_error(self._body_error_args) |
90 | 339 | 339 | ||
91 | 340 | def cancel_read_body(self): | 340 | def cancel_read_body(self): |
92 | 341 | self._wait_for_response_end() | 341 | self._wait_for_response_end() |
93 | 342 | 342 | ||
94 | 343 | 343 | ||
113 | 344 | def _translate_error(error_tuple): | 344 | def _raise_smart_server_error(error_tuple): |
114 | 345 | # Many exceptions need some state from the requestor to be properly | 345 | """Raise exception based on tuple received from smart server |
115 | 346 | # translated (e.g. they need a branch object). So this only translates a | 346 | |
116 | 347 | # few errors, and the rest are turned into a generic ErrorFromSmartServer. | 347 | Specific error translation is handled by bzrlib.remote._translate_error |
117 | 348 | error_name = error_tuple[0] | 348 | """ |
118 | 349 | error_args = error_tuple[1:] | 349 | if error_tuple[0] == 'UnknownMethod': |
119 | 350 | if error_name == 'UnknownMethod': | 350 | raise errors.UnknownSmartMethod(error_tuple[1]) |
120 | 351 | raise errors.UnknownSmartMethod(error_args[0]) | 351 | raise errors.ErrorFromSmartServer(error_tuple) |
103 | 352 | if error_name == 'LockContention': | ||
104 | 353 | raise errors.LockContention('(remote lock)') | ||
105 | 354 | elif error_name == 'LockFailed': | ||
106 | 355 | raise errors.LockFailed(*error_args[:2]) | ||
107 | 356 | elif error_name == 'FileExists': | ||
108 | 357 | raise errors.FileExists(error_args[0]) | ||
109 | 358 | elif error_name == 'NoSuchFile': | ||
110 | 359 | raise errors.NoSuchFile(error_args[0]) | ||
111 | 360 | else: | ||
112 | 361 | raise errors.ErrorFromSmartServer(error_tuple) | ||
121 | 362 | 352 | ||
122 | === modified file 'bzrlib/smart/request.py' | |||
123 | --- bzrlib/smart/request.py 2010-11-26 06:31:54 +0000 | |||
124 | +++ bzrlib/smart/request.py 2011-03-02 20:20:52 +0000 | |||
125 | @@ -446,6 +446,9 @@ | |||
126 | 446 | return ('TokenMismatch', err.given_token, err.lock_token) | 446 | return ('TokenMismatch', err.given_token, err.lock_token) |
127 | 447 | elif isinstance(err, errors.LockContention): | 447 | elif isinstance(err, errors.LockContention): |
128 | 448 | return ('LockContention',) | 448 | return ('LockContention',) |
129 | 449 | elif isinstance(err, MemoryError): | ||
130 | 450 | # GZ 2011-02-24: Copy bzrlib.trace -Dmem_dump functionality here? | ||
131 | 451 | return ('MemoryError',) | ||
132 | 449 | # Unserialisable error. Log it, and return a generic error | 452 | # Unserialisable error. Log it, and return a generic error |
133 | 450 | trace.log_exception_quietly() | 453 | trace.log_exception_quietly() |
134 | 451 | return ('error', str(err)) | 454 | return ('error', str(err)) |
135 | 452 | 455 | ||
136 | === modified file 'bzrlib/tests/test_remote.py' | |||
137 | --- bzrlib/tests/test_remote.py 2011-02-24 16:13:39 +0000 | |||
138 | +++ bzrlib/tests/test_remote.py 2011-03-02 20:20:52 +0000 | |||
139 | @@ -2775,6 +2775,16 @@ | |||
140 | 2775 | ('pack collection autopack',)], | 2775 | ('pack collection autopack',)], |
141 | 2776 | client._calls) | 2776 | client._calls) |
142 | 2777 | 2777 | ||
143 | 2778 | def test_oom_error_reporting(self): | ||
144 | 2779 | """An out-of-memory condition on the server is reported clearly""" | ||
145 | 2780 | transport_path = 'quack' | ||
146 | 2781 | repo, client = self.setup_fake_client_and_repository(transport_path) | ||
147 | 2782 | client.add_expected_call( | ||
148 | 2783 | 'PackRepository.autopack', ('quack/',), | ||
149 | 2784 | 'error', ('MemoryError',)) | ||
150 | 2785 | err = self.assertRaises(errors.BzrError, repo.autopack) | ||
151 | 2786 | self.assertContainsRe(str(err), "^remote server out of mem") | ||
152 | 2787 | |||
153 | 2778 | 2788 | ||
154 | 2779 | class TestErrorTranslationBase(tests.TestCaseWithMemoryTransport): | 2789 | class TestErrorTranslationBase(tests.TestCaseWithMemoryTransport): |
155 | 2780 | """Base class for unit tests for bzrlib.remote._translate_error.""" | 2790 | """Base class for unit tests for bzrlib.remote._translate_error.""" |
156 | @@ -2853,6 +2863,13 @@ | |||
157 | 2853 | detail='extra detail') | 2863 | detail='extra detail') |
158 | 2854 | self.assertEqual(expected_error, translated_error) | 2864 | self.assertEqual(expected_error, translated_error) |
159 | 2855 | 2865 | ||
160 | 2866 | def test_norepository(self): | ||
161 | 2867 | bzrdir = self.make_bzrdir('') | ||
162 | 2868 | translated_error = self.translateTuple(('norepository',), | ||
163 | 2869 | bzrdir=bzrdir) | ||
164 | 2870 | expected_error = errors.NoRepositoryPresent(bzrdir) | ||
165 | 2871 | self.assertEqual(expected_error, translated_error) | ||
166 | 2872 | |||
167 | 2856 | def test_LockContention(self): | 2873 | def test_LockContention(self): |
168 | 2857 | translated_error = self.translateTuple(('LockContention',)) | 2874 | translated_error = self.translateTuple(('LockContention',)) |
169 | 2858 | expected_error = errors.LockContention('(remote lock)') | 2875 | expected_error = errors.LockContention('(remote lock)') |
170 | @@ -2886,6 +2903,12 @@ | |||
171 | 2886 | expected_error = errors.DivergedBranches(branch, other_branch) | 2903 | expected_error = errors.DivergedBranches(branch, other_branch) |
172 | 2887 | self.assertEqual(expected_error, translated_error) | 2904 | self.assertEqual(expected_error, translated_error) |
173 | 2888 | 2905 | ||
174 | 2906 | def test_NotStacked(self): | ||
175 | 2907 | branch = self.make_branch('') | ||
176 | 2908 | translated_error = self.translateTuple(('NotStacked',), branch=branch) | ||
177 | 2909 | expected_error = errors.NotStacked(branch) | ||
178 | 2910 | self.assertEqual(expected_error, translated_error) | ||
179 | 2911 | |||
180 | 2889 | def test_ReadError_no_args(self): | 2912 | def test_ReadError_no_args(self): |
181 | 2890 | path = 'a path' | 2913 | path = 'a path' |
182 | 2891 | translated_error = self.translateTuple(('ReadError',), path=path) | 2914 | translated_error = self.translateTuple(('ReadError',), path=path) |
183 | @@ -2907,7 +2930,8 @@ | |||
184 | 2907 | 2930 | ||
185 | 2908 | def test_PermissionDenied_no_args(self): | 2931 | def test_PermissionDenied_no_args(self): |
186 | 2909 | path = 'a path' | 2932 | path = 'a path' |
188 | 2910 | translated_error = self.translateTuple(('PermissionDenied',), path=path) | 2933 | translated_error = self.translateTuple(('PermissionDenied',), |
189 | 2934 | path=path) | ||
190 | 2911 | expected_error = errors.PermissionDenied(path) | 2935 | expected_error = errors.PermissionDenied(path) |
191 | 2912 | self.assertEqual(expected_error, translated_error) | 2936 | self.assertEqual(expected_error, translated_error) |
192 | 2913 | 2937 | ||
193 | @@ -2936,6 +2960,39 @@ | |||
194 | 2936 | expected_error = errors.PermissionDenied(path, extra) | 2960 | expected_error = errors.PermissionDenied(path, extra) |
195 | 2937 | self.assertEqual(expected_error, translated_error) | 2961 | self.assertEqual(expected_error, translated_error) |
196 | 2938 | 2962 | ||
197 | 2963 | # GZ 2011-03-02: TODO test for PermissionDenied with non-ascii 'extra' | ||
198 | 2964 | |||
199 | 2965 | def test_NoSuchFile_context_path(self): | ||
200 | 2966 | local_path = "local path" | ||
201 | 2967 | translated_error = self.translateTuple(('ReadError', "remote path"), | ||
202 | 2968 | path=local_path) | ||
203 | 2969 | expected_error = errors.ReadError(local_path) | ||
204 | 2970 | self.assertEqual(expected_error, translated_error) | ||
205 | 2971 | |||
206 | 2972 | def test_NoSuchFile_without_context(self): | ||
207 | 2973 | remote_path = "remote path" | ||
208 | 2974 | translated_error = self.translateTuple(('ReadError', remote_path)) | ||
209 | 2975 | expected_error = errors.ReadError(remote_path) | ||
210 | 2976 | self.assertEqual(expected_error, translated_error) | ||
211 | 2977 | |||
212 | 2978 | def test_ReadOnlyError(self): | ||
213 | 2979 | translated_error = self.translateTuple(('ReadOnlyError',)) | ||
214 | 2980 | expected_error = errors.TransportNotPossible("readonly transport") | ||
215 | 2981 | self.assertEqual(expected_error, translated_error) | ||
216 | 2982 | |||
217 | 2983 | def test_MemoryError(self): | ||
218 | 2984 | translated_error = self.translateTuple(('MemoryError',)) | ||
219 | 2985 | expected_error = errors.BzrError("remote server out of memory") | ||
220 | 2986 | self.assertEqual(expected_error, translated_error) | ||
221 | 2987 | |||
222 | 2988 | def test_generic_IndexError(self): | ||
223 | 2989 | err = errors.ErrorFromSmartServer(('error', "list index out of range")) | ||
224 | 2990 | translated_error = self.translateErrorFromSmartServer(err) | ||
225 | 2991 | expected_error = errors.UnknownErrorFromSmartServer(err) | ||
226 | 2992 | self.assertEqual(expected_error, translated_error) | ||
227 | 2993 | |||
228 | 2994 | # GZ 2011-03-02: TODO test generic non-ascii error string | ||
229 | 2995 | |||
230 | 2939 | 2996 | ||
231 | 2940 | class TestErrorTranslationRobustness(TestErrorTranslationBase): | 2997 | class TestErrorTranslationRobustness(TestErrorTranslationBase): |
232 | 2941 | """Unit tests for bzrlib.remote._translate_error's robustness. | 2998 | """Unit tests for bzrlib.remote._translate_error's robustness. |
233 | 2942 | 2999 | ||
234 | === modified file 'bzrlib/tests/test_smart_request.py' | |||
235 | --- bzrlib/tests/test_smart_request.py 2010-07-13 19:02:12 +0000 | |||
236 | +++ bzrlib/tests/test_smart_request.py 2011-03-02 20:20:52 +0000 | |||
237 | @@ -172,6 +172,9 @@ | |||
238 | 172 | ('TokenMismatch', 'some-token', 'actual-token'), | 172 | ('TokenMismatch', 'some-token', 'actual-token'), |
239 | 173 | errors.TokenMismatch('some-token', 'actual-token')) | 173 | errors.TokenMismatch('some-token', 'actual-token')) |
240 | 174 | 174 | ||
241 | 175 | def test_MemoryError(self): | ||
242 | 176 | self.assertTranslationEqual(("MemoryError",), MemoryError()) | ||
243 | 177 | |||
244 | 175 | 178 | ||
245 | 176 | class TestRequestJail(TestCaseWithMemoryTransport): | 179 | class TestRequestJail(TestCaseWithMemoryTransport): |
246 | 177 | 180 |
Martin [gz] wrote: remote. _translate_ error function. However, I think the one I really smart.message. _translate_ error but I'm failing to
[...]
> This is step 3 (really) in fixing bug 722416. The aim of the branch is to turn
> an remote response in the form `('MemoryError',)` into a useful message for
> the user. I'm pretty sure the code is incorrect or incomplete however.
>
> I've got two main problems here. Firstly, there seem to be two different
> _translate_error functions for the local-side error interpretation. Secondly,
> I can only reproduce one of the codepaths from the bug reports in the tests.
>
> The stack I can test is going through autopack as-like bug 602614, which uses
> the bzrlib.
> need to patch is the bzrlib.
> write a test with a stack like the local-side one in bug 721598 as the
> important bits in test_remote seem to be Fake'd out. Help?
Hmm. I don't think we have good unit tests for smart.message. _translate_ error. Well, we do have ingProtocolThre e in test_smart_ transport, which currently tests erver paths there, but via the byte-level
bzrlib.
TestClientDecod
the UnknownMethod and ErrorFromSmartS
details of the v3 protocol which you don't really want to care about for this.
I suspect the other paths are only exercised incidentally via the broader test
suite.
The intent was that message. _translate_ error would be for all the context-free _translate_ error. So _translate_ error is probably the better place.
error translations, i.e. error translations that are independent of the request
that triggered them, such as UnknownMethod. Then the translations that want
access to the branch object or whatever belong in remote.
you're right that message.
I'm not sure the distinction is particularly important, and looking at it with _translate_ error erver are _translate_ error _translate_ error will see it
relatively fresh eyes it has the drawback that the message.
function is never used by v2 (or v1, but it has a fixed set of error names
anyway), and so the cases other than UnknownMethod and ErrorFromSmartS
thus duplicated anyway. I'm also not sure that drawback matters much though,
although worryingly NoSuchFile has richer behaviour in remote.
that will never be triggered in v3 because message.
first.
We have good unit tests I think that the protocol layer signals the presence of _translate_ error (at a glance I see ationSuccess is missing one or two newer errors like NotStacked). _translate_ error. I think the
an error to its caller, and from the other end test_remote does pretty good job
of unit testing remote.
TestErrorTransl
But as you've noticed there's a gap for message.
ideal solution is to write the unit tests for that gap, but I don't want to
force you to do that just for this fix. Hmm :/
I'm currently inclined to say:
* leave the UnknownMethod and ErrorFromSmartS erver paths in _translate_ error: they probably do belong there, and are already well _translate_ error
message.
tested.
* move the context-free translations into a new function, probably in remote.py
* invoke that new function as part of remote.
This I think satisfies all the concerns adequately:
* the context-free translations don't clutter up the more complex...