[MERGE] Chroot transport overhaul

[Martin Pool]

On 3/28/07, Robert Collins <robertc at robertcollins.net> wrote:
> Rewritten chroot transport that prevents accidental chroot escapes when
> using urlutils against the transport's url.(Robert Collins, Andrew Bennetts)

I think reviews can go more efficiently if you say more about what you
are changing and how.  A few sentences would do.

Then we can just check that the design is reasonable, and that you
implemented what you said, rather than inferring the design from a
multi-hundred-line patch.

+    * Rewritten chroot transport that prevents accidental chroot escapes when
+      using urlutils against the transport's url.
+      (Robert Collins, Andrew Bennetts)

You changed the urls, as well as just changing the implementation, I think?

-        if isinstance(current_transport, chroot.ChrootTransportDecorator):
-            raise TestSkipped("ChrootTransportDecorator disallows clone('..')")

Removing the duplication is nice, but what was it doing before and why
can it be removed?

+    """Server for chroot transports."""

"Why would you need a server for chroots?"  Can you add a sentence
saying what it's actually for?

+class ChrootTransport(Transport):

Wants a docstring.

+        # protocol dictionary, we dont *just in case* there are parts of

"don't"

+
+    def setUp(self):
+        self.scheme = 'chroot-%d:///' % id(self)
+        register_transport(self.scheme, self._factory)
+
+    def tearDown(self):
+        unregister_transport(self.scheme, self._factory)

Treating them all as new schemes seems a bit ugly, couldn't we just
register the pattern once and treat the id as a parameter, or maybe
put it in the hostname field?

+0 just because of that last issue.

-- 
Martin