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

[John Arbash Meinel]

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

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'm fine with doing it, but it seems like we may have an api deficiency
if you need to do it.

The ugly hack that is StubSSHServer...

Using 1 thread per file is how subprocess.py does I/O in subprocess.py
(if running under windows). So I don't know that there is much else we
can do. I know paramiko uses threads as well, so it is easily possible
that this is the best we can do.

I'm a little concerned that you combine stdout with stderr and send them
both over the pipe. I would assume real ssh uses a separate channel for
stderr. This is probably okay for now, but I have the feeling it
indicates a limitation of our model if we expect it to work that way.

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.

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


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

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

So I have some comments, but nothing that would really block it from
0.11. (+0.9, which you can upgrade to 1.0 if you like)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFDx6EJdeBCYSNAAMRAnf5AKCWRyI19yApsC5l0BSIZQs0wpXX1wCdErsm
8PqAVnvdLu8vgadqxzs0Uls=
=bsO9
-----END PGP SIGNATURE-----