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

[Robert Collins]

On Fri, 2009-03-20 at 13:16 +1100, Andrew Bennetts wrote:
> To be paranoid we perhaps should make sure the jail state
> propagates to new threads, although very little bzrlib code uses
> threads at
> the moment.

Possibly, OTOH we generally avoid threads today, so perhaps this is just
something to document clearly in the writing-plugins, and server docs.

> This may cause some trouble for server-side plugins that actually do
> want to
> access branches outside the jail, e.g. to automatically mirror a
> branch to
> another location.  We should spend a bit of time making sure we have a
> reasonable API for those plugins to use to do that (a API to extend
> the
> jail, or circumvent it?).

I'm not against making a clean API for this, but do suggest its optional
- wait for people doing it then pick the cleanest solution they come up
with :).

> In support of that, this patch also adds an ignore_fallbacks option to
> BzrDir.open_branch (and BranchFormat.open, and so on).  This allows
> the
> smart server methods that do need to open a branch to do so without
> opening fallback branches that are forbidden (and unnecessary to the
> implementation of the server request).  I think this is potentially
> useful
> in other cases as well.
> 
> There are some outstanding issues:
> 
>  * Tests fail. 

Andrew and I have been talking about these tests already, broadly we
feel that clone|sprout on a reference containing bzr dir isn't something
we do in the UI, and its safe to just make it error; Andrew is working
on this at the moment, I think.


>  * Robert would rather that open_branch didn't have a new flag.
> Instead he
>    proposes (Robert please jump in if I'm misrepresenting this in any
> way)
>    that Branch itself should check if it's being opened while in a
> jail, and
>    if so automatically skip opening the fallbacks.

You're misrepresenting it :). I'm happy with a new flag - and suspect we
will need it from time to time in other legitimate code.

I'm not happy having to explicitly use it in the smart server. So one
way would be to say I want a three-valued flag:
auto - open fallbacks outside of the server, don't inside the server
open - always open, fail if unable to
never - never open.

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.

>   So the server could
>    continue to use plain bzrdir.open_branch().  I don't like the idea
> of a
>    method giving a different type of result (a fully working branch
> vs. a
>    partially working branch) based on global state; Robert doesn't
> like
>    adding yet another flag to open_branch, and forcing people like
> plugin
>    authors to decide if they need to open a fallback-enabled branch or
> not.
>    If branches were more lazy about when they opened fallback
> locations,
>    perhaps we'd both be happy, but implementing that would probably be
>    non-trivial.

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*.

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

> I'd also like feedback on the patch in general; despite the open
> issues it's
> actually quite close to being ready to land.

Parameters added to def open() deserve docstrings :)

+                #format =
self._bzrdir.find_branch_format().network_name()

Either do it, or delete it :)

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
-------------- 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/20090320/b1714656/attachment.pgp