[MERGE] Update to bzr.dev.

[Andrew Bennetts]

More review comments dribbling in for you :)

Robert Collins wrote:
> === modified file 'bzrlib/repofmt/knitrepo.py'
[...]
>  class _KnitParentsProvider(object):

As far as I can see this class is now unused, so I think we can now delete it.

> +class _KnitsParentsProvider(object):
> +
> +    def __init__(self, knit, prefix=()):
> +        """Create a parent provider for string keys mapped to tuple keys."""
> +        self._knit = knit
> +        self._prefix = prefix
> +
> +    def __repr__(self):
> +        return 'KnitsParentsProvider(%r)' % self._knit
> +
> +    def get_parent_map(self, keys):
> +        """See graph._StackedParentsProvider.get_parent_map"""
> +        parent_map = self._knit.get_parent_map(
> +            [self._prefix + (key,) for key in keys])
> +        result = {}
> +        for key, parents in parent_map.items():
> +            revid = key[-1]
> +            if len(parents) == 0:
> +                parents = (_mod_revision.NULL_REVISION,)
> +            else:
> +                parents = tuple(parent[-1] for parent in parents)
> +            result[revid] = parents
> +        for revision_id in keys:
> +            if revision_id == _mod_revision.NULL_REVISION:
> +                result[revision_id] = ()
> +        return result

Shouldn't the “keys” argument of this get_parent_map be named “revision_ids”?
This code is also inconsistent about what it calls elements of that argument
when it iterates over it, at first it calls them “key”, later it calls them
“revision_id”.

Using revision_ids/revision_id would also avoid the minor confusion of using
“key” to mean to different types of object in the same function.  Seeing
“(key,)” and “key[-1]” in close proximity breaks my brain a little.

(If you can't keeping the type of “key” objects straight, how are mortals meant
to? <wink>)

> +    def _temp_inventories(self):
> +        result = self._format._get_inventories(self._transport, self,
> +            'inventory.new')
> +        # Reconciling when the output has no revisions would result in no
> +        # writes - but we want to ensure there is an inventory for
> +        # compatibility with older clients that don't lazy-load.
> +        result.get_parent_map([('A',)])
> +        return result

It's not clear to me if doing a lookup here makes sure the inventory.new is
created, or makes sure it already exists, or something else.  I think this
comment needs to explicitly say that “we lookup an arbitrary key from the
inventory here so that...”.   At the moment it is pretty indirect about saying
what it is doing and why.

>      def initialize(self, a_bzrdir, shared=False):
[...]
> -        # trigger a write of the inventory store.

I think we still need a comment that says this or similar.  You have this:

>          # the revision id here is irrelevant: it will not be stored, and cannot
> -        # already exist.
[...]
> +        # already exist, we do this to create files on disk for older clients.
> +        result.inventories.get_parent_map([('A',)])
> +        result.revisions.get_parent_map([('A',)])
> +        result.signatures.get_parent_map([('A',)])

But that's not quite enough.  See my comment about _temp_inventories.

-Andrew.