[MERGE] [0.11] Working bzr+ssh://, make "bzr serve --inet" read-only by default.

[Robert Collins]

On Mon, 2006-09-18 at 17:32 -0500, John Arbash Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Andrew Bennetts wrote:
> > Hi all,
> > 
> > http://people.ubuntu.com/~andrew/bzr/bzr-ssh-testing/ has some changes I'd like
> > to see merged, ideally for 0.11 despite the freeze:
> > 
> >   * make bzr+ssh:// URLs, added in previous smart server merges, work.  Most of
> >     the infrastructure was already there, but wasn't fully hooked up or tested.
> >     This branch rectifies that.  I'd really love for this to get in 0.11, as it
> >     will greatly increase the number of people that will experiment with the new
> >     smart server work.
> > 
> >   * adds a --allow-writes option "bzr serve" command (and default to only
> >     allowing read-only access).  This will make it harder for people to shoot
> >     themselves in the foot.  It would be particularly useful for serving
> >     anonymous bzr branches out of inetd -- so again, significantly boosting the
> >     utility of the smart server work so far.
> > 
> > So, I'm both seeking reviews of this code, and opinions on if it's ok to make an
> > exception for merging this new functionality into 0.11.
> > 
> > -Andrew.
> 
> First, a small comment that if you want to get something reviewed
> quickly, it really is easier to review if you include at least a diff.
> (A bundle can be nice too, since we can directly merge from that, but a
> diff at a minimum, so that we can make comments directly, rather than
> looking at it separately.)
> 
> You might be interested in something like:
> 
> bzr diff -rancestor:$HOME/path/bzr.dev/
> 
> I have that aliased as 'BDA' in my .zshrc, though you could also put
> something like it into ~/.bazaar/bazaar.conf.
> 
> It generates the same diff that a bundle would, but without the extra
> bundle information.
> 
> So... on to the review:
> 
> TestBzrServe... almost wants to be a base class like
> 'TestCaseWithTransport' to be 'TestCaseWithBzrServer'. This isn't
> necessary for now, but something like it would probably be a good
> refactoring.

There are several different ways to start the server, and different ways
to test the shutdown happened. I think we're in fairly good shape here.
Loopback tests are an appropriate way to get tests using a smart server:
and we have some extractable helpers in test_smart_transport now.

I really dont think we want any more end to end black box tests for
this: The important things we are testing here are: the remote command
is constructed correctly, and that the local pipes in the client object
are assigned appropriately. Unit tests for this are highly desirable,
and I hope we can in fact delete this 'smoke test' once we have
sufficient intra and inter unit tests for this area of code : it was not
snuck up on via testing so much as crash landed.

> Can we ensure that doing 'del transport' actually cleans up the
> connection? (I assume you are doing this because you want to avoid
> getting an exception from the Transport object, which thinks it is still
> connected).

I insisted on this because of the lack of clarity on how transports and
their shared connection clean up - I wanted to be darn sure nothing
could sneak in badly.

> I'm fine with doing it, but it seems like we may have an api deficiency
> if you need to do it.

I'm sure we do have such a deficiency.

> It seems like 'self.command_executed' should actually be a list, so we
> can tell if there are multiple connection attempts.
> 
> Also, it would probably be better to have the StubSSHServer not defined
> inside the test function, so that it could be accessed by other tests in
> the future.

See above for my thoughts on this: not a good idea.

> Why are we translating ReadOnlyError => TransportNotPossible. It seems
> like we would want to tell the user that the remote directory is readonly.

The current API delivers read only errors from a filesystem as
TransportNotPossible('read only transport'). So its compatible with the
rest of the code. We should in the 0.12 cycle add a ReadOnlyTransport
error which is a subclass of TransportNotPossible - or something
similar.

> How do you handle ssh connecting to the local SSH connection without a
> password?

I'm not sure what you mean here.

> Anyway, the changes look good. And while I don't think it is a trivial
> change, it does fit Robert's new definition of trivial (does not
> destabilize existing apis).

Yah, To some extent I'm exercising release-manager judgement here. I
dont think theres a hard rule that can work always :(.

Thanks!
Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060919/c0f0f01f/attachment.pgp