On 2/8/2011 4:46 PM, Martin Pool wrote:
> On 8 February 2011 20:26, Andrew Bennetts <email address hidden> wrote:
>> Some quick comments (rather than a comprehensive review):
>>
>> 1. "for br in [branch_c]:" is a weird expression (why have a loop when there's exactly 1 item?)
>
> Ah, for the sake of easier testing I had cut out the others. It
> should test all of them.
>
>> 2. this fix appears reasonable, but you only fix one method when there are many other uses of self._fallback_vfs in e.g. GroupCompressVersionedFiles. Either there are more bugs waiting to be fixed here (hopefully easy ones to reveal with tests similar to the one you add in this patch), or the reason why only get_known_graph_ancestry was broken is subtle/unclear. Do you know which? I'm a bit worried that this may correct the immediate traceback without actually making the Launchpad scanner functional because of further issues.
>
> I was hoping someone would tell me :-)
>
> More seriously, I do think all the others need to be checked too, and
> also by searching the bug tracker we might find reports where similar
> things are failing.
>
I honestly haven't seen much failure of this. Mostly because stacking is
rare outside of launchpad, and stacking on stacked is a whole next-level
of unlikely. Since not even launchpad creates branches like this easily.
I wouldn't be surprised to have internal bugs that are latent. Doing a
quick grep doesn't show anything obvious.
_fallback_vfs is only mentioned in bzrlib/groupcompress.py and knit.py
(since those are the only VersionedFiles that actually support it, it
should make sense.)
Specific references in groupcompress:
get_known_graph_ancestry(), the code in question
_get_parent_map_with_sources(), uses the public api
_find_from_fallback(), public_api
keys(), public_api
in knit.py:
_logical_check(), public_api
get_known_graph_ancestry(), current code
_get_parent_map_with_sources(), public_api (copy & paste because of inheritance issues)
_get_remaining_record_stream, public_api
get_sha1s(), public_api
insert_record_stream(), just used to tell if we have a stacking
boundary, and need to do extra sanity checks on the stream
iter_lines_added_or_present_in_keys(), public_api, probably should be
deprecated anyway
keys(), public_api
_ContentMapGenerator._work, public_api
_KnitAnnotator._get_needed_texts, only used to signal fallback to
non-optimized code
In the end, what we could do is promote "self._index.find_ancestry()" to
be a public attribute of VersionedFiles rather than hidden on the _index
object, and then it could be done via public api, rather than chaining
the transitive closure of fallback_vfs. That would allow us to
consistently go via public api. Albeit by using a slightly different api
that can chain.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 2/8/2011 4:46 PM, Martin Pool wrote: rsionedFiles. Either there are more bugs waiting to be fixed here (hopefully easy ones to reveal with tests similar to the one you add in this patch), or the reason why only get_known_ graph_ancestry was broken is subtle/unclear. Do you know which? I'm a bit worried that this may correct the immediate traceback without actually making the Launchpad scanner functional because of further issues.
> On 8 February 2011 20:26, Andrew Bennetts <email address hidden> wrote:
>> Some quick comments (rather than a comprehensive review):
>>
>> 1. "for br in [branch_c]:" is a weird expression (why have a loop when there's exactly 1 item?)
>
> Ah, for the sake of easier testing I had cut out the others. It
> should test all of them.
>
>> 2. this fix appears reasonable, but you only fix one method when there are many other uses of self._fallback_vfs in e.g. GroupCompressVe
>
> I was hoping someone would tell me :-)
>
> More seriously, I do think all the others need to be checked too, and
> also by searching the bug tracker we might find reports where similar
> things are failing.
>
I honestly haven't seen much failure of this. Mostly because stacking is
rare outside of launchpad, and stacking on stacked is a whole next-level
of unlikely. Since not even launchpad creates branches like this easily.
I wouldn't be surprised to have internal bugs that are latent. Doing a
quick grep doesn't show anything obvious.
_fallback_vfs is only mentioned in bzrlib/ groupcompress. py and knit.py
(since those are the only VersionedFiles that actually support it, it
should make sense.)
Specific references in groupcompress:
get_known_ graph_ancestry( ), the code in question map_with_ sources( ), uses the public api fallback( ), public_api
_get_parent_
_find_from_
keys(), public_api
in knit.py:
_logical_check(), public_api graph_ancestry( ), current code map_with_ sources( ), public_api (copy & paste because of
inheritance issues) record_ stream, public_api record_ stream( ), just used to tell if we have a stacking added_or_ present_ in_keys( ), public_api, probably should be
get_known_
_get_parent_
_get_remaining_
get_sha1s(), public_api
insert_
boundary, and need to do extra sanity checks on the stream
iter_lines_
deprecated anyway
keys(), public_api
_ContentMapGene rator._ work, public_api _get_needed_ texts, only used to signal fallback to
_KnitAnnotator.
non-optimized code
In the end, what we could do is promote "self._ index.find_ ancestry( )" to
be a public attribute of VersionedFiles rather than hidden on the _index
object, and then it could be done via public api, rather than chaining
the transitive closure of fallback_vfs. That would allow us to
consistently go via public api. Albeit by using a slightly different api
that can chain.
John enigmail. mozdev. org/
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAk1 R0SQACgkQJdeBCY SNAAMZIwCeNQtpj B/0NPaQjNb98wW0 Vxg2 pJhAoqWGbDgXq8o v5R3oyxkW
T9IAnAl/
=i3cD
-----END PGP SIGNATURE-----