Code review comment for lp:~zyga/bzr/find_ancestors

Revision history for this message
John A Meinel (jameinel) wrote :

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

On 10/10/2012 9:23 PM, Zygmunt Krynicki wrote:
> Review: Needs Information
>
>> Thanks for having a go at fixing this.
>
> Thanks for the feedback!
>
>> To land, this branch really needs: * Tests, to demonstrate the
>> problem and evaluate any attempted fix against * Logic, rather
>> than than just raising NotImplementedError and skipping that
>> case
>
> Sure, at the moment it's just an early thing I managed to do
> (having no understanding of bzr internals) that allowed me to use
> bzr.
>
>> Did you have a look at Wouter's branch, which implements the
>> method? Does that also work for you?
>
> I did see that branch but I recall reading that the solution is
> incorrect. I am unable to judge that myself. Could you please
> suggest if skipping BTreeBuilder instances makes sense? If not,
> should the (can they) implement the same _find_ancestors() method?
>
> Also, if you could point me out to any docs that describe how bzr
> internals work would help me a lot.
>
> Thanks ZK
>

They can implement _find_ancestors() and they should be queried if
available.

_find_ancestors was meant as a way to dig more quickly into ancestors
when the data storage isn't great at random lookups (rather than
proceeding step-by-step into ancestors, grab whatever ancestors you
can get cheaply, and return).

Anyway, the implementation for BTreeBuilder can be trivial (return 1
generation of ancestors). It is a performance optimization to have a
tighter inner loop. If you don't, it will just call the function again.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Cygwin)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iEYEARECAAYFAlB1uYUACgkQJdeBCYSNAANZGQCfSUM+Upaw1/lHs9BJH0X1KNf7
pxcAoMDpJjoE71nselXAWa7gYaPRmQA0
=jhNq
-----END PGP SIGNATURE-----

« Back to merge proposal