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

Martin Pool (mbp) 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.

« Back to merge proposal