Merge lp://staging/~zyga/bzr/find_ancestors into lp://staging/bzr

Thanks for having a go at fixing this.

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

Did you have a look at Wouter's branch, which implements the method? Does that also work for you?

<lp:~larstiq/bzr/bug541626>

If so, picking up that code and writing a test case that exercises it would be a good way forwards.

> 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

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

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

The bug is not terribly clear, but as I read it:

* The first branch just copied the method over, which is not correct
* No one commented on Wouter's branch, which implements _find_ancestors differently

It's not clear to me if it really makes sense for the builder to have a _find_ancestors method, but without a test that demonstrates the situation in which one is present in the graph, it's hard to understand.

> Also, if you could point me out to any docs that describe how bzr internals
> work would help me a lot.

Refer to the docs/developers directory, also available online:

<http://doc.bazaar.canonical.com/bzr.dev/developers/>

See particularly:

<http://doc.bazaar.canonical.com/bzr.dev/developers/overview.html>
<http://doc.bazaar.canonical.com/bzr.dev/developers/indices.html>
<http://people.canonical.com/~mwh/bzrlibapi/bzrlib.btree_index.html>

Also the relevant parts of:

<http://doc.bazaar.canonical.com/bzr.dev/developers/HACKING.html>
<http://doc.bazaar.canonical.com/bzr.dev/developers/testing.html>

> > 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?
>
> The bug is not terribly clear, but as I read it:

This bug can be triggered in a very simple way using bzr-fastimport. If this helps I can prepare reproducible instructions. This prevents one from using bzr-fastimport to consume a patch.

> * The first branch just copied the method over, which is not correct
> * No one commented on Wouter's branch, which implements _find_ancestors
> differently
>
> It's not clear to me if it really makes sense for the builder to have a
> _find_ancestors method, but without a test that demonstrates the situation in
> which one is present in the graph, it's hard to understand.

I read that as a request for reproducible instructions. I'm on it!

> > Also, if you could point me out to any docs that describe how bzr internals
> > work would help me a lot.
>
> Refer to the docs/developers directory, also available online:

Thanks a lot, I'll get right to it!

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

On 10/10/2012 10:33 PM, Zygmunt Krynicki wrote:
>>> 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?
>>
>> The bug is not terribly clear, but as I read it:
>
> This bug can be triggered in a very simple way using
> bzr-fastimport. If this helps I can prepare reproducible
> instructions. This prevents one from using bzr-fastimport to
> consume a patch.

Right, bzr-fastimport is the only code that uses find_ancestry before
the BTreeBuilder content is actually committed to the repository, and
it becomes a normal BTreeIndex. Which is why it only fails for
bzr-fastimport, and we don't have any tests inside bzr that fail (bzr
itself never needs to search the ancestry that it is now adding).

I believe the reason for searching the ancestry in bzr-fastimport is
genuine, and the find_ancestry call is reasonable to implement. I
thought I looked at Wouter's branch and made a comment on it, but it
has been quite a while since I looked at that.

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

iEYEARECAAYFAlB2R3EACgkQJdeBCYSNAAOwkgCcDh63D0ttLH7esE0/e/fXF/0b
i3gAniNjj07MLTiJgK3L62ro31eBdJlU
=YITc
-----END PGP SIGNATURE-----