[rfc] [patch] speeding up bzr log with a tree delta iterator

[Aaron Bentley]

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

Hi Denys,

At your request, I've reviewed this.

Denys Duchier wrote:
| I have a new version:
|
| 	Branch.revision_iter(rev1,rev2=None,reverse=False,delta=False)
|
| which allows rev1 to be a revision id not in this branch's history, as
long as
| rev2 is None or equal to rev1.  In that case, the predecessor is taken
from the
| left-most parent.  I also added a test case for this.

I'm not certain that this is what John had in mind.  We have a
rule-of-thumb that interfaces should take either revision ids or revnos,
but not both.  Taking both suggests a confused interface.

It seems to me that the single-revision case is a special case, and
should probably not be handled by the revision iterator.  In any case,
don't feel required to implement it, just because you're working in a
related area.


| +    def revision_iter(self, rev1, rev2=None, reverse=False, delta=False):
| +        raise NotImplementedError('revision_iter is abstract')

Note that None is a valid revision ID in many contexts.  I think you'll
get away with it here, but in general, you can't.

| +        # iterate
| +        while again:

This looks like it would be clearer as a for loop.

e.g.
if reverse:
~  revno_iter = ((r, r, r + 1) for r in xrange(rev2, rev1 - 1, -1))
else:
~  revno_iter = ((r, r - 1, r) for r in xrange(rev1, rev2 + 1))

for revno, lo_revno, hi_revno in revno_iter:
...


| === modified file 'bzrlib/delta.py'
| --- bzrlib/delta.py	
| +++ bzrlib/delta.py	
| @@ -16,6 +16,8 @@
|
|  from bzrlib.inventory import InventoryEntry
|  from bzrlib.trace import mutter
| +from bzrlib.errors import InvalidRevisionNumber, InvalidRevisionId
| +from bzrlib.tree import EmptyTree

These imports look like they're not being used.


| @@ -188,32 +177,20 @@
|      else:
|          searchRE = None
|
| -    which_revs = _enumerate_history(branch)
| +    revision_history = branch.revision_history()
| +
| +    if len(revision_history)>0:
| +        if start_revision is None:
| +            start_revision = 1
| +        if end_revision is None:
| +            end_revision = len(revision_history)

Are you deliberately avoiding handling the 0-length revision history
case here?

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFD09g40F+nu1YWqI0RAgjUAJ9odryXionjFDNEROsBK4YO27Mg2QCcDlyv
JJhLm1KTdKi0sA13bsB9rRk=
=xM9E
-----END PGP SIGNATURE-----