[MERGE/RFC] Add transport jail to bzr smart server; add ignore_fallbacks option to BzrDir.open_branch

[Robert Collins]

On Mon, 2009-03-23 at 16:42 +1100, Andrew Bennetts wrote:
> 
> To be clear, if we had that flag, I assume you'd like the default to
> be
> "auto"?

Yes.

> > The main reason for this is that in the server, for our code, we
> *never*
> > want to open a fallback. And any api that opens a branch for us
> (like
> > cloning metadir) has to honour this desire. We can either propogate
> the
> > flag everywhere (ugh), or have it Just Work when we are inside the
> > server. I'm entirely happy with people overriding it in either
> direction
> > inside the server. But I'm not happy with it potentially propogating
> all
> > over the place. Global state is generally bad... but not always.
> 
> I guess I don't see is as being such a big deal to ask callers to be
> explicit about whether they want a full branch or not.  The effort to
> update
> the affected server-side code in core was tiny.  So in our server-side
> code
> we never want to open a fallback, so all two places in the server that
> did
> so now pass ignore_fallbacks=True in my patch.

We have a tiny fraction of our functionality actually implemented server
side today. I don't see that staying the case, and I'm worried about
having ignore_branch_fallbacks appear sprinkled around. Maye this is
premature. OTOH when we started this there were two immediate cases,
which is > 1 :P

> I'm actually tempted to make open_branch(ignore_fallbacks=False)
> always fail
> server-side, even for unstacked branches, just to make potentially
> server-unsafe code fail as early as possible, but that's probably a
> bit
> extreme.

It would also mean that ignore_fallbacks=True is the only valid way to
call the API in the server, and that to me means we're waving dead
chickens. Better at that point to not have it controllable and just
DTRT.

> I'm not sure that we should be trying to hide the fact that the
> server-side
> is fundamentally different for a plugin (or core bzrlib code) than the
> client-side. 

We use the same API's everywhere in both server and client side code,
and most things use the same code paths. I'm not arguing for this as a
way to hide the differences, rather to not spend time telling the system
something it already knows.

> Plugins that want to work server-side on possibly stacked branches
> cannot
> avoid dealing with the issue.  Even post_change_branch_tip hooks that
> don't
> open their own branch will need to cope with the passed in branch
> possibly
> being a stacked branch with an inaccessible fallback.  Adding magic to
> branch opening won't make any difference to this case; the plugin
> needs to
> anticipate the issue or it will error on non-trivial operations like
> generating diffs.

Thats true. But even opening the branch won't work without opening the
jail etc. But not all plugins will need adjustments, For instance,
bzr-search can work just fine [it may not today, but if it doesn't thats
a bug :P].

> I guess in a nutshell my feeling is adding magic to branch open
> doesn't
> really offer much practical benefit to callers (adding a simple
> boolean flag
> to the open_branch call is the least of your worries in dealing with a
> branch with an inaccessible fallback), and the cost is quite high
> (because
> it's magic).
> 
> ...
> > The bit above here is fine though - I just wanted to ensure it was
> clear
> > that I'm ok with a parameter to open_branch, as long as its *not
> needed
> > in the common case*.
> 
> That's good to know.  Thanks for being clear :)
> 
> So, given that the flag is useful anyway, why block this patch on the
> question of whether it should be necessary server-side?  If we do
> decide
> that magic is appropriate we can add that later.  It's still
> relatively
> early in the release cycle, so there's plenty of time for feedback.

well... until plugin authors run into it we won't know, and I'm fairly
sure they won't think to ask for magic.

> > > Robert and I would very much like to hear other people's opinions
> on the
> > > explicit-flag vs. automatic-check-for-jail question, because we
> haven't
> > > yet been able to convince each other.
> > 
> > Indeed :).
> 
> This still holds!  People who aren't Robert: share your thoughts!


> > Other than those comments, the lack of tests on ignore_fallbacks,
> and
> > our continuing deadlock on defaulting-in-the-server, its bb:approve
> for
> > me.
> > 
> > bb:resubmit
> 
> :)
> 
> Here's an updated patch.  The two issues you give there aren't
> addressed so
> it's still in limbo, but the others should be, including test failures
> (although I need to do another full test run to be 100% sure).

Well, while I'm still not convinced either, I do think the lack of test
coverage is a can't merge issue. So its still:
bb:resubmit
on the test coverage.

-Rob

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090323/2ccf7ae1/attachment-0001.pgp