[MERGE/RFC] Branch.dotted_revno_to_revision_id()

[Ian Clatworthy]

John Arbash Meinel wrote:
> Ian Clatworthy wrote:

> Your parameter and your docstring don't match "_reverse_cache=False"
> versus ":param _cache_reverse"

Oops. Fixed. I went with _cache_reverse in the end as "verb adverb"
seemed the slightly better ordering.

> self._revision_id_to_revno_top_cache
> 
> ^- I don't quite understand the name here. "revno_top_cache" ? Perhaps
> "self._partial_revision_id_to_revno_cache" ?

Done.

> Or, I guess:
> 
> +        if result is None and self._revision_id_to_revno_cache:
> +            result = self._revision_id_to_revno_cache.get(revision_id)
> +	     if result is None:
> +                raise NoSuchRevision(...)
> +        if result is None:

Done.

> +    def _dotted_revno_to_revision_id(self, revno):
> +        """Worker function for dotted_revno_to_revision_id.
> +
> +        Subclasses should override this if they wish to
> +        provide a more efficient implementation.
> +        """
> +        if len(revno) == 1:
> +            return self.get_rev_id(revno[0])
> +        revision_id_to_revno = self.get_revision_id_to_revno_map()
> +        for revision_id, this_revno in revision_id_to_revno.iteritems():
> +            if revno == this_revno:
> +                return revision_id
> +        else:
> +            revno_str = '.'.join(map(str, revno))
> +            raise errors.NoSuchRevision(self, revno_str)
> 
> ^- This for loop replaces a list comprehension, which is good that it
> can stop early, but bad that they are generally slower. I'm curious what
> the difference would be. (There are also slightly ugly hacks that allow
> you to stop a list comprehension early.)

So I've put this back as a list comprehension for now, minimising any
risk of a performance regression. I'll reconsider this after smarter
caching is in place.

> +                revision = branch.dotted_revno_to_revision_id(match_revno,
> +                    _reverse_cache=True)
> +            except errors.NoSuchRevision:

> ^- If we are changing this, I would use "revision_id" here to be clear
> about what the object is.

Done.

> As for overall design...
> 
> I think if we are going to put these somewhere, Branch is the place to
> have them, since the values depend on the Branch tip revision.

Precisely.

> Conceptually, it would be nice if branches that shared some common
> ancestry could share the common revnos, but I think that is complexity
> we could add if/when we found it to actually be useful.

I think that's very doable. For example, I'd like to extend
bzr-revnocache to cache just the graph differences from the
parent if and when the parent is a local branch. That will
greatly drop the storage overhead for a typical setup of
a mirror+feature branches.

> +    def _revision_id_to_dotted_revno(self, revision_id):
> 
> ^- In other places I've used the "def _do_*" notation to indicate that
> this is the function that child classes should override. I would
> recommend it here, but it isn't something set in stone.

Done.

> BB:tweak

Thanks for the fast review. It's much appreciated when other patches
(like faster-log) are dependent on it.

Ian C.