[PATCH] Enabling writable http transports (allows plugins to extend bzr)

[John Arbash Meinel]

Vincent LADEUIL wrote:

...

>     jam> And send it here.
> 
> Attached.
> 
> I tried to open the tiniest possible hole in which I can enter.
> 
>         Vincent
> 
> 
> 
> ------------------------------------------------------------------------
> 
> # Bazaar revision bundle v0.8
> #
> # message:
> #   Enable writable http transports.
> #   
> #   * bzrlib/transport/http/__init__.py:
> #   (HttpTransportBase.abspath):  Forbids  directories  only when  the
> #   transport is read-only (I think it was the original intent), as it
> #   blocks all attempts to write a plugin that allows writing on http.
> #   (HttpServer.__init__):  Allows  subclasses  to specify  their  own
> #   request handler.
> #   
> # committer: Vincent LADEUIL <v.ladeuil at alplog.fr>
> # date: Mon 2006-08-21 16:23:04.253000021 +0200
> 
> === modified file bzrlib/transport/http/__init__.py
> --- bzrlib/transport/http/__init__.py
> +++ bzrlib/transport/http/__init__.py
> @@ -170,10 +170,13 @@
>              # TODO: Don't call this with an array - no magic interfaces
>              relpath_parts = relpath[:]
>          if len(relpath_parts) > 1:
> +	    # TODO: Check  that the  "within branch" part  of the
> +	    # error messages below is relevant in all contexts
>              if relpath_parts[0] == '':
>                  raise ValueError("path %r within branch %r seems to be absolute"
>                                   % (relpath, self._path))
> -            if relpath_parts[-1] == '':
> +	    # read only transports never manipulate directories
> +            if self.is_readonly() and relpath_parts[-1] == '':
>                  raise ValueError("path %r within branch %r seems to be a directory"
>                                   % (relpath, self._path))

^- these changes seem okay, except you have tabs in the source code. We
require everything to be spaces.

I'm also a little concerned that is_readonly() is not going to be a
cheap function in the future, so it may not be proper to call it for
every abspath() call.

In discussion with Robert on IRC, the thought is that 'is_readonly()' is
actually meant to check if Transport.base is writable. So it actually
should go to the remote host, and ask if it can write to that directory.
It isn't meant as just a test for the Transport as a whole.

Now, it is possible that is_readonly() should cache the status, making
the assumption that the readonly status won't change during the lifetime
of a Transport object.

I'd like to get Robert's thoughts on this.


>          basepath = self._path.split('/')
> @@ -494,10 +497,14 @@
>      # used to form the url that connects to this server
>      _url_protocol = 'http'
>  
> +    # Subclasses can provide a specific request handler
> +    def __init__(self, request_handler=TestingHTTPRequestHandler):
> +	self.request_handler = request_handler
> +
>      def _http_start(self):
>          httpd = None
>          httpd = TestingHTTPServer(('localhost', 0),
> -                                  TestingHTTPRequestHandler,
> +                                  self.request_handler,
>                                    self)
>          host, port = httpd.socket.getsockname()
>          self._http_base_url = '%s://localhost:%s/' % (self._url_protocol, port)

This change also seems reasonable.

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/20060821/0530aa07/attachment.pgp