[merge][#261315][1.7] RemoteBranch sets up stacking (more) correctly

[Martin Pool]

On Wed, Sep 17, 2008 at 12:08 PM, John Arbash Meinel
<john at arbash-meinel.com> wrote:

>> I thought I sent this last night; I'm not sure why.

... why it didn't actually get through.


> - -        self._ensure_real()
> - -        return self._real_branch.get_stacked_on_url()
> +        try:
> +            response = self._client.call('Branch.get_stacked_on_url',
> +                self._remote_path())
> +            if response[0] != 'ok':
> +                raise errors.UnexpectedSmartServerResponse(response)
> +            return response[1]
> +        except errors.ErrorFromSmartServer, err:
> +            # there may not be a repository yet, so we can't call through
> +            # its _translate_error
> +            _translate_error(err, branch=self)
> +        except errors.UnknownSmartMethod, err:
> +            self._ensure_real()
> +            return self._real_branch.get_stacked_on_url()
>
> ^- The "if response[0] != 'ok'" feels like it should be outside the
> except clauses.

I kind of see what you mean, that we want conceptually:

  switch (client.call)
  case ok:
     ...
  case error:
  case unknownmethod:

However, in actual Python I think it's better to have it inside, as
the response variable is not actually bound unless it succeeds, so I
like to have it right there.  Obviously either way could work.

> +    def test_get_stacked_on_real_branch(self):
> +        base_branch = self.make_branch('base', format='1.6')
> +        stacked_branch = self.make_branch('stacked', format='1.6')
> +        stacked_branch.set_stacked_on_url('../base')
> +        client = FakeClient(self.get_url())
> +        client.add_expected_call(
> +            'BzrDir.open_branch', ('stacked/',),
> +            'success', ('ok', ''))
> +        client.add_expected_call(
> +            'BzrDir.find_repositoryV2', ('stacked/',),
> +            'success', ('ok', '', 'no', 'no', 'no'))
> +        # called twice, once from constructor and then again by us
> +        client.add_expected_call(
> +            'Branch.get_stacked_on_url', ('stacked/',),
> +            'success', ('ok', '../base'))
> +        client.add_expected_call(
> +            'Branch.get_stacked_on_url', ('stacked/',),
> +            'success', ('ok', '../base'))
>
> ^- This is a bit of a surprise. I suppose this won't really happen in
> code that uses Branch? (As it should generally be unaware whether
> something is stacked or not.)

Surprise that there are two rpcs?

It's always called when we open the branch.  It would be nice to do
that as part of a more general open-the-branch call, which should
perhaps also find the repository and get the last revision.

And if it's specifically called on the branch then we have to ask for it again.

Arguably it should be cached inside the lock context.  In practice
this may not be a priority because it's should not often be called
after opening the branch.  So to really get a benefit here we'd want
to open the object locked, which would be useful in other cases, but
requires changing the opening api.


-- 
Martin <http://launchpad.net/~mbp/>