[MERGE/RFC] Branch.dotted_revno_to_revision_id()

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ian Clatworthy wrote:
> Ian Clatworthy wrote:
>> This patch proposes a new API for Branch called
>> dotted_revno_to_revision_id(). The dotted-revno
>> => revision-id lookup in revisionspec.py has been
>> refactored to use this new method.
> 
> Here's an updated version that provides the reverse API
> as well, choosing the optimal algorithm for getting to
> the result.
> 
> Ian C.
> 

     @needs_read_lock
+    def dotted_revno_to_revision_id(self, revno, _reverse_cache=False):
+        """Return the revision_id for a dotted revno.
+
+        :param revno: a tuple like (1,) or (1,1,2)
+        :param _cache_reverse: a private parameter enabling storage
+           of the reverse mapping in a top level cache. (This should
+           only be done in selective circumstances as we want to
+           avoid having the mapping cached multiple times.)
+        :return: the revision_id
+        :raises errors.NoSuchRevision: if the revno doesn't exist
+        """
+        rev_id = self._dotted_revno_to_revision_id(revno)
+        if _reverse_cache:
+            self._revision_id_to_revno_top_cache[rev_id] = revno
+        return rev_id
+

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

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" ?


...

+        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:

^- I'm pretty sure that if _revision_id_to_revno_cache exists but does
not contain a value for revision_id, then it isn't in the ancestry *at
all* and your can stop further processing.

So that can become:
+        if result is None and self._revision_id_to_revno_cache:
+            return self._revision_id_to_revno_cache.get(revision_id)
+        if result is None:

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[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.)


         if dotted:
- -            branch.lock_read()
             try:
- -                revision_id_to_revno =
branch.get_revision_id_to_revno_map()
- -                revisions = [revision_id for revision_id, revno
- -                             in revision_id_to_revno.iteritems()
- -                             if revno == match_revno]
- -            finally:
- -                branch.unlock()
- -            if len(revisions) != 1:
+                revision = branch.dotted_revno_to_revision_id(match_revno,
+                    _reverse_cache=True)
+            except errors.NoSuchRevision:
                 raise errors.InvalidRevisionSpec(self.user_spec, branch)
             else:
                 # there is no traditional 'revno' for dotted-decimal
revnos.
                 # so for  API compatability we return None.
- -                return branch, None, revisions[0]
+                return branch, None, revision

^- 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.

+    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.


BB:tweak

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkl4bXwACgkQJdeBCYSNAAP2twCeIxqjNXnEA9tjFd0DqMjpNhE2
alcAoJlcepkTAeUbilPQzGaEZnaz0p7n
=aCG3
-----END PGP SIGNATURE-----