Code review comment for lp:~slyguy/bzr/bug-440952-bzrdir

On Thu, 2009-10-15 at 17:05 +0000, Brian de Alwis wrote:
> Brian de Alwis has proposed merging lp:~slyguy/bzr/bug-440952-bzrdir into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This branch contains an attempt to tackle bug 440952 (https://bugs.launchpad.net/bugs/440952).

I see you already have some excellent reviews. I wonder perhaps if
changing open_branch/get_branch_reference is overkill, given the
complexity it adds? It makes a clearly defined and *predictable*
overhead function sometimes twice as expensive. And the function can be
a good fraction of a second.

I'd rather not see 'has_repository' as a public method. I don't see
*bzr* repo's suffering from many extra probes - bzrdirs without branches
that are not repositories are pretty rare. However, the svn mapping
creates a svndir object at every path under an svn repo and at every dir
in an svn tree, which we then loop up on - so the performance impact
there could be substantial.

So in balance, I'd also rather see this done from the command runner or
Branch.open. For the latter, just putting the bzrdir that was found *if*
one was found in NotBranchError would let
Branch.open/Branch.open_containing do the translation without altering
the core logic to be unpredictable. Doing it from the command runner is
probably too high up.

 review: needsfixing