[MERGE] Add ErrorFromSmartServer exception, and raise it from read_response_tuple for all protocol versions (protocol v3 patch 5/7)

[John Arbash Meinel]

John Arbash Meinel has voted tweak.
Status is now: Conditionally approved
Comment:
Is peaking at "error_tuple[]" the right way to do it?
You do stuff like:

if e.error_tuple = (some, value):

and

if e.error_tuple[0] == 'xxxx':

I would worry if error_tuple is empty, then the latter would raise an 
IndexError instead of the original error. I would be happier if the 
errors were more standardized, so you would have:
e.error_type or some other member that represents the first entry in 
error_tuple.

Otherwise, I think I would use slices instead of direct indexes. So 
something like:

if e.error_tuple[:1] == ('xxxxx',):

That way if the tuple is empty, you get () != ('xxxx',) rather than an 
index error. (Of course, error_tuple can't be None, etc.


+        try:
+            response = self._client.call_expecting_body(
+                'Repository.get_revision_graph', path, revision_id)
+        except errors.ErrorFromSmartServer, err:
+            if err.error_tuple[0] == 'nosuchrevision':
+                raise NoSuchRevision(self, revision_id)
+        if response[0][0] != 'ok':

^-- I think there is a latent bug here. If an error is raised which is 
not
"NoSuchRevision" then it will fall through. But 'response' isn't set 
either,
because an exception was raised. I think you need:

+            if err.error_tuple[0] == 'nosuchrevision':
+                raise NoSuchRevision(self, revision_id)
+            raise
+        if response[0][0] != 'ok':


+        except errors.ErrorFromSmartServer, err:
+            if err.error_tuple[0] == 'LockContention':
+                raise errors.LockContention('(remote lock)')
+            elif err.error_tuple[0] == 'UnlockableTransport':
+                raise 
errors.UnlockableTransport(self.bzrdir.root_transport)
+            elif err.error_tuple[0] == 'LockFailed':
+                raise errors.LockFailed(err.error_tuple[1], 
err.error_tuple[2])

^- More of a side question. why is the other error "nobranch" but these 
errors
are all 'LockContention' (CamelCase). Seems inconsistent. Unless the 
first is
a plain response and the others are exceptions?

+            if self._requested_parents is not None and len(ancestry) != 
0:
+                self._requested_parents.update(present_keys)
+                mutter('Current RemoteRepository graph hit rate: %d%%',
+                    100.0 * len(self._requested_parents) / 
len(ancestry))

^- Since we are here, *I* would really like it if this printed the 
actual
counts for _requested_parents and ancestry.


Again here:
@@ -1503,10 +1518,11 @@
              self._ensure_real()
              self._clear_cached_state()
              return self._real_branch.set_last_revision_info(revno, 
revision_id)
+        except errors.ErrorFromSmartServer, err:
+            if err.error_tuple[0] == 'NoSuchRevision':
+                raise NoSuchRevision(self, err.error_tuple[1])
          if response == ('ok',):
              self._clear_cached_state()
-        elif response[0] == 'NoSuchRevision':
-            raise NoSuchRevision(self, response[1])
          else:
              raise errors.UnexpectedSmartServerResponse(response)

I think you need the plain 'raise' section. Should we have explicit 
tests for
these?



+        client.add_success_response_with_body('', 'ok')

^- Why does 'add_success_...' need to supply 'ok'. Do some of them 
return
something other than 'ok' for success?

I guess later on you have:
+        repo, client = self.setup_fake_client_and_repository('path')
+        client.add_success_response('something unexpected!')


For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C20080423231923.GI9546%40steerpike.home.puzzling.org%3E