[MERGE] RevisionSpec.as_revision_id()

[John Arbash Meinel]

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

Aaron Bentley wrote:
| John Arbash Meinel wrote:
|> John Arbash Meinel wrote:
|> ...
|
|> |
|> | I'm happy to add those tests.
|> |
|> | John
|> | =:->
|
|> The basic changes should be done in the attached patch. I didn't make
|> some of
|> the changes that I disagreed with, but I did add the tests and I tried
|> to at
|> least add comments to make it clearer what is going on.
|
| It's a pity you didn't see my message where I said I'd take over since
| you were on vacation.
|
| I've come up with a version I feel is much cleaner.
|
| The key thing is does is make the partial_revision_history_cache the
| primary cache for format-6 branches.  This means that whenever there's a
| full-history cache, there's also a partial-history cache which contains
| the full history.  This removes the need for two codepaths-- branch6
| branches can use the partial cache always.
|
| I've added _extend_partial_history, which extends the partial history,
| optionally stopping once it's reached a particular index.  This means
| that determining a previously-found revid is still done using [].index.
|  It also avoids doing more iteration, though frankly I don't think that
| would have a significant cost.
|
| This will also be easy to extend to revision_id_to_revno.  I have a
| branch that does it, but it causes lock tests to fail.  I haven't
| determined yet whether the failure is spurious.
|
| Time for "bzr tag -r -2 foo" on the Emacs import (best of 4):
| Your version
| ============
| real	0m0.428s
| user	0m0.316s
| sys	0m0.076s
|
| My version
| ==========
| real	0m0.324s
| user	0m0.260s
| sys	0m0.052s


+        :param index: The index which should be present.
+        """
+        repo = self.repository
+        if len(self._partial_revision_history_cache) == 0:
+            iterator = repo.iter_reverse_revision_history(self.last_revision())
+        else:
+            start_revision = self._partial_revision_history_cache[-1]
+            iterator = repo.iter_reverse_revision_history(start_revision)
+            #skip the last revision in the list
+            assert iterator.next() == start_revision

^- This should not be done in an assert, as it will be skipped when we run
python -O.

So I would recommend:

next_rev = iterator.next()
assert next_rev == start_revision

+        for revision_id in iterator:
+            self._partial_revision_history_cache.append(revision_id)
+            if (stop_index is not None and
+                len(self._partial_revision_history_cache) > stop_index):
+                break


...

+        last_revno, last_revision_id = self.last_revision_info()
+        if revno <= 0 or revno > last_revno:
+            raise errors.NoSuchRevision(self, revno)
+
+        if history is not None:
+            assert len(history) == last_revno, 'revno/history mismatch'
+
+        index = last_revno - revno
+        if len(self._partial_revision_history_cache) <= index:
+            self._extend_partial_history(stop_index=index)
+        if len(self._partial_revision_history_cache) > index:
+            return self._partial_revision_history_cache[index]
+        else:
+            raise errors.NoSuchRevision(self, revno)
+
^- Here, if history is not None, you check its length, but then ignore it. The
point of passing in history is that the calling function already had to compute it.


I also didn't see any code for "cache_revision_history" to actually set any data
in _partial_revision_history_cache. In theory it should set
self._revision_history_cache = self._partial_revision_history_cache = history


And I'm *guessing* that the actual performance difference you are seeing is that
Robert's patch for improved iter_history... was finally merged into bzr.dev, but
isn't in my patch. But I certainly could be wrong.

BB:tweak

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

iD8DBQFH80qQJdeBCYSNAAMRAjE0AKCtjZZ0qxPP1LZqeJfVCraFnh5zRACdGof5
n7Is0xn8Zntw+UelVowLeLE=
=9q4c
-----END PGP SIGNATURE-----