ian.clatworthy at internode.on.net
Fri Jan 23 19:45:37 GMT 2009
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.
> ^- I don't quite understand the name here. "revno_top_cache" ? Perhaps
> "self._partial_revision_id_to_revno_cache" ?
> 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:
> + 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)
> + 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.
> 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.
> 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.
Thanks for the fast review. It's much appreciated when other patches
(like faster-log) are dependent on it.
More information about the bazaar