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 :)