Code review comment for lp:~spiv/bzr/sprout-does-not-reopen-repo

Vincent Ladeuil wrote:
> Review: Needs Information
> I would almost agree with John about the practical aspect (it works, let's land).
>
> On the other hand, you have already spent a good amount of time on this which will be necessary again if we want to provide a cleaner solution.
>
> But...
>
> 267 registry,
> 268 + repository,
> 269 revision as _mod_revision,
>
> seems weird. I'd like jelmer's feedback here, but I'm pretty sure we pull
> controldir out of bzrdir to make sure it was isolated from bzr specifics and
> having to reintroduce repository here sounds like a step backward.

That seems to be a leftover from an earlier version of this patch. It's
not actually used. I've removed that import (and gardened a couple of
others noticed by pyflakes). Thanks for spotting that.

> Also,
>
> 10 + # This fallback is already configured. This probably only
> 11 + # happens because BzrDir.sprout is a horrible mess. To avoid
> 12 + # confusing _unstack we don't add this a second time.
> 13 + # XXX: log a warning, maybe?
> 14 + return
>
> is a sign that you're close to nailing the issue and I think you should ;)

If I thought I was close I'd have nailed it already!

> On the overall, we have a 'repository' attribute on the branch object, so
> having to provide it should only occur when we are creating this object. Do we
> have that many different ways to create branches that you end up modifying so
> many signatures ? Doesn't it reveal a deeper issue ?

I agree, I think there's a deeper issue here, as my cover letter tried
to say. I'm not sure what a good way solution is.

Regarding the number of ways to create branches, here's one way to count
that, based on this diff:

 * 8 definitions of BranchFormat.initialize
 * 4 definitions of ControlDir.create_branch
 * 1 definition of Branch.sprout

That doesn't count definitions in test code.

Plus there's open methods, which create branch objects, which are also
affected:

 * 3 definitions of BranchFormat.open

So yes, we do have 16 different ways to create a branch object, hence
why I touch so many methods. I don't see an obvious solution to that.

A possible approach might be to have the low-level
BranchFormat.open/initialize methods be designed to return Branches
without the repository object being set, so returning only partially
initialized Branch objects. Then have higher-level APIs (along the
lines of the existing open_containg_tree_or_branch? or something new
here too?) that assemble these into fully initialized objects with
.repository attributes filled in. There's the related consideration
that parts of the code want to open and/or create some combination of
branch/repo/tree in one logical step (and ideally one network
roundtrip), but the current design opens each of the bzrdir, repository,
and branch separately. I think this is part of the cause of the double
add_fallback ugliness that's an XXX for that matter.

So my suspicion is a fairly fundamental redesign of how we open/create
these things would be help a lot... but getting it right may take some
effort. And it's likely to be way more disruptive to bzr-svn and
friends than a new keyword param! Until someone has a concrete design
to propose it's hard to evaluate the costs and benefits.

In short: I'm really not sure, hence why my cover letter asked the
question :)