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

[Andrew Bennetts]

John Arbash Meinel wrote:
> 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.

I've added these lines to ErrorFromSmartServer's __init__:

        try:
            self.error_verb = error_tuple[0]
        except IndexError:
            self.error_verb = None
        self.error_args = error_tuple[1:]

And I've changed remote.py to use these new attributes when appropriate.  I
still do “err.error_tuple == ...” sometimes, as equality is always safe, and is
more precise.  (If unexpected data is received, I'd rather the client raised an
error rather than ignoring it.)

> +        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':

You're right.  It'd be nice to have a more reusable way to convert error
responses into exceptions, so these mistakes are harder to make.

I've fixed this instance, and added a test
(test_remote.TestRepositoryGetRevisionGraph.test_unexpected_error).

> +        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?

Hysterical raisins.  Early errors codes tended to be alllower, newer ones have
been CamelCase.

> +            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.

I agree (and not log if the numbers are the same as the previous time we
logged them), but this patch is plenty big enough without adding random
drive-bys.

I'll happily review a quick patch from you to make that change though.

> 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?

Good catch.  I've added a test for this case.

> +        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?

Well, many uses of add_success_response do, obviously.
add_success_response_with_body doesn't at the moment, but there's no particular
reason why it couldn't.  I'd rather leave this explicit (and consistent with
add_success_response).

Again, thanks for the review.

-Andrew.