[Merge] lp:~mbp/bzr/715000-stacking into lp:bzr

[Martin Pool]

On 9 February 2011 10:26, John Arbash Meinel <john at arbash-meinel.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 2/8/2011 4:46 PM, Martin Pool wrote:
>> On 8 February 2011 20:26, Andrew Bennetts <andrew.bennetts at canonical.com> 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.

Back from the mp to the list:

I think renaming it is a good idea anyhow.
https://code.edge.launchpad.net/~mbp/bzr/715000-more-fallbacks/+merge/49025

One bit that looks problematic is _get_remaining_record_stream which
seems to assume it can find the source identified by
_get_parent_map_with_sources within it's own immediate fallbacks, and
it doesn't seem like that's guaranteed to be true.  However, I can't
reproduce an error with a bit of experimentation, and I don't see any
bug reports about the IndexError I would expect this to cause:

                    vf = self._fallback_vfs[parent_maps.index(source) - 1]

Aside from that I agree, they're all ok.

Martin