Code review comment for lp:~mbp/bzr/715000-stacking

John A Meinel (jameinel) wrote :

Hash: SHA1

On 2/8/2011 3:26 AM, Andrew Bennetts 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?)
> 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 agree, I think he meant:

for br in [branch_a, branch_b, branch_c]:

Though I'm fine with a test of just branch_c, if we are just wanting to
test the transitivity of operations.

I wouldn't be surprised if there are some bugs in other places, but I
think things like "get_parent_map" already recurse via the public api,
rather than recursing to find the transitive fallbacks, and then
iterating. (so branch_c.repo.get_parent_map() calls
branch_b.repo.get_parent_map() which calls
branch_a.repo.get_parent_map() rather than
branch_c.repo.get_parent_map() going through all fallbacks directly.)

I sort of like the public api recursion approach more than the
_transitive_fallbacks approach. If only because it means that
RepoA.do_something() isn't poking at RepoB._fallback_repositories. It
only looks at its own private attributes, and a fallbacks public attributes.

transitive_fallbacks does ok with this, if you consider that the
function is actually a public Repository api. (Which I think it should
be, and should be tested as such, with a per_repository implementation

Anyway, I'm fine with this as a fix for what we are currently
encountering. I think we can do an audit of the codebase for everywhere
that _fallback_vfs or _fallback_repositories is being encountered, but
that can be separate to Martin's patch.

Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla -


« Back to merge proposal