[MERGE] Repository.get_parent_map

[Robert Collins]

On Mon, 2008-01-14 at 17:30 +1100, Andrew Bennetts wrote:

> bb:tweak
> 
> > === modified file 'bzrlib/remote.py'
> [...]
> > +    def _get_parent_map(self, keys):
> > +        """Helper for get_parent_map that performs the RPC."""
> > +        keys = set(keys)
> > +        if NULL_REVISION in keys:
> > +            keys.discard(NULL_REVISION)
> > +            found_parents = {NULL_REVISION:()}
> > +            if not keys:
> > +                return found_parents
> > +        else:
> > +            found_parents = {}
> > +        path = self.bzrdir._path_for_remote_call(self._client)
> > +        for key in keys:
> > +            assert type(key) is str
> > +        response = self._client.call_expecting_body(
> > +            'Repository.get_parent_map', path, *keys)
> > +        if response[0][0] not in ['ok']:
> > +            raise errors.UnexpectedSmartServerResponse(response[0])
> > +        if response[0][0] == 'ok':
> > +            coded = response[1].read_body_bytes()
> > +            if coded == '':
> > +                # no revisions found
> > +                return {}
> > +            lines = coded.split('\n')
> > +            revision_graph = {}
> > +            for line in lines:
> > +                d = tuple(line.split())
> > +                if len(d) > 1:
> > +                    revision_graph[d[0]] = d[1:]
> > +                else:
> > +                    # No parents - so give the Graph result (NULL_REVISION,).
> > +                    revision_graph[d[0]] = (NULL_REVISION,)
> > +            return revision_graph
> 
> There's no good infrastructure for this at the moment, but you need to handle
> talking to older servers that won't recognise this verb.

Do we? I thought our policy was 'newest server everywhere'

> For now, the way to do it is:
> 
>         verb = 'Repository.get_parent_map'
>         if (response == ('error', "Generic bzr smart protocol error: "
>                 "bad request '%s'" % verb) or
>               response == ('error', "Generic bzr smart protocol error: "
>                 "bad request u'%s'" % verb)):
>            response.cancel_read_body()
>            # Fallback code goes here
> 
> For that matter, I think you need a cancel_read_body() in the not-ok
> case too.

ok, I'll add something for this.

> > +                size_so_far += 2 + len(revision_id) + sum(map(len, parents))
> 
> The arithmetic here deserves a comment.  Is the 2 there to compensate for the 
> “ ” and “\n” delimiters?  Is that right when there's more than one parent for a
> revision?

its close enough ;). Comment added.

> > +                # get all the directly asked for parents, and then flesh out to
> > +                # 64K or so.
> > +                if first_loop_done and size_so_far > 65000:
> > +                    next_revs = set()
> > +                    break
> > +            # don't query things we've already queried
> > +            next_revs.difference_update(queried_revs)
> > +            first_loop_done = True
> 
> I think there's a minor bug here: if after the first time through the while loop
> size_so_far > 65000, then it will still add a little bit more to result, even
> though both first_loop_done and size_so_far are true.  One fix for this would be
> to put:
> 
>     if size_so_far > 65000:
>         break
> 
> at the very end of the loop.

I'm not worried about any single revision being added. However I don't
want to add (say) 2MB of data in pathological cases of wide graphs.

> > === modified file 'bzrlib/tests/test_remote.py'
> [...]
> > +class TestRepositoryGetGraph(TestRemoteRepository):
> > +
> > +    def test_get_graph(self):
> > +        # get_graph returns a graph with get_parent_map as the repository.
> > +        responses = []
> > +        transport_path = 'quack'
> > +        repo, client = self.setup_fake_client_and_repository(
> > +            responses, transport_path)
> > +        graph = repo.get_graph()
> > +        self.assertEqual(graph._parents_provider, repo)
> 
> The code in the test method doesn't seem to verify the statement in the comment
> describing the test.

Tweaked.

-Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080115/b96cb3fe/attachment-0001.pgp