PING: reviews needed

[John Arbash Meinel]

Andrew Bennetts wrote:
> Hi all,
> 
> I'm still after reviews for the following branches:
> 
>   * http://people.ubuntu.com/~andrew/bzr/chroot-transport/
>     Adds a ChrootTransportDecorator.  This is so I can setup a HTTP smart server
>     serving e.g. /srv/project.com/source without exposing the rest of the file
>     system.

I gave you feedback on this one. It seems like it works, but it seems
like it is a band-aid rather than fixing the real problem. If it is
required to get proper server-side support without exposing the whole
filesystem, then we can merge it.

We would want some sort of 'chroot+' anyway, so it would all be possible
after the fact, without having to deprecate anything.

> 
>   * http://people.ubuntu.com/~andrew/bzr/http-smart-server/
>     (depends on chroot-transport).  This is just a near-trivial bug fix to make
>     sure that a request indicates it needs no more data once its response has
>     been sent.  

Well, I thought we had a less-than trivial case here, because the diff
against chroot-transport is 3.9K lines. But it turns out that is because
you merged memory-something into it. Which has already been merged into
bzr.dev.

I don't remember you posting this as a request before. But +1 to the
changes.

This needs to be cleaned up to merge cleanly, because of the convoluted
ancestry, it can't seem to find a clean base. You have chroot-transport
in there, and then you merged the 'memory' fixes, but the second ones
have been merged into bzr.dev, but not the first, etc, etc.

I did find that 'bzr remerge --format=weave' helped, though on some
files it did worse than 'bzr remerge --reprocess'. (bzrlib/errors.py was
one of those). Anyway, I think I was able to sort through all of it.


> 
>   * http://people.ubuntu.com/~andrew/bzr/smart-server-unicode/
>     (depends on http-smart-server).  Don't always decode/encode request/response
>     tuples as UTF-8, because sometimes we actually want the bytestrings.

This is pretty hard to read, because you merged lots of things into this
branch. So there isn't a simple ancestor that we can use to see what the
real changes are. I went ahead and tried to clean up a merge from
http-smart-server, so I would be able to review it. But I didn't spend a
lot of time making sure my merge was correct.

After sorting through all of the diff and merge issues, it seems pretty
straightforward. Basically you changed the smart protocol to not decode
automatically, and move decoding up higher in the stack. The only
request I remember asking for was that you use something that needs to
be url escaped (so it has a '+' or a '%' or something like that). So
that you are actually thoroughly testing the request interface. That is
actually more important than Unicode at the moment, since we don't make
any unicode requests (yet).

Otherwise +1 from me.

> 
>   * http://people.ubuntu.com/~andrew/bzr/wsgi-smart-server/
>     (depends on smart-server-unicode).  Actually, this has mostly been reviewed
>     once by John already.  The only changes since then have been to make use of
>     ChrootTransportDecorator so that it's reasonably secure by default.
> 
> -Andrew.

This last one looks fine too. So if you can make these clean to be
merged, I'm okay with merging it. Though I would like to make a request
for cleaning up Transport actions, rather than the current
implementation of ChrootTransport.

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061108/7a873308/attachment.pgp