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

[Denys Duchier]

Thanks for the review,

Aaron Bentley <aaron.bentley at utoronto.ca> writes:

>> +        def revno2id(revno):
>> +            if revno:
>> +                return rh[revno-1]
>> +            else:
>> +                return None
>
> It looks like you could use Branch.get_rev_id here.  Is there a reason
> not to?

none that I can see either. changed.  but I also had to fix get_rev_id: I
believe the elif should just be an if.

>> +        rev2no
>> +            None or a function from revid to revno.  This function should
>> +            also accept 0 and None and return None in those cases.
>
> I don't understand the purpose of this.  Why not just Branch.get_rev_id?

it's been a long while, so I forgot some things.  I believe the thinking behind
allowing a rev2no keyword argument was that if you have a custom way to iterate
over revisions, you might also need a custom way to number them (for example,
you might be iterating over revisions not in the current history, maybe because
you'd not be looking at the left-most parent ancestry path, in which case you
might want to always return None as a revno since it doesn't really make sense
to number these revisions).  However, I don't understand anymore why I wanted
the function to also accept 0.  If this keyword argument really bothers you, I
can take it out, but it does no harm.

> revision_tree already returns EmptyTree if the input is None

thanks. changed.

>> === modified file 'bzrlib/revisionspec.py'
>> --- bzrlib/revisionspec.py	
>> +++ bzrlib/revisionspec.py	
>> @@ -206,6 +206,12 @@
>>          except ValueError:
>>              return RevisionInfo(branch, None, self.spec)
>>  
>> +    def in_store(self, branch):
>> +        if branch.has_revision(self.spec):
>> +            return RevisionInfo(branch, None, rev_id=self.spec)
>> +        else:
>> +            raise NoSuchRevision(branch, self.prefix + self.spec)
>
> I don't understand the motivation for adding this.  Any function that
> uses a revision-id already needs to handle cases where the revision
> isn't in the repository.

This one was introduced specifically to handle the case requested by jam of a
lone revid not in the branch's history, and it is used in (builtin.py)cmd_log.

Cheers,

--Denys