[rfc] [patch] pycurl transport

[Robert Collins]

On Wed, 2006-01-11 at 15:13 +1100, Martin Pool wrote:
> === added file 'bzrlib/transport/pycurlhttp.py'
> --- /dev/null   
> +++ bzrlib/transport/pycurlhttp.py      
> @@ -0,0 +1,64 @@
> +# Copyright (C) 2006 Canonical Ltd
> +
> +# This program is free software; you can redistribute it and/or
> modify
> +# it under the terms of the GNU General Public License as published
> by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> 02111-1307  USA
> +
> +"""http/https transport using pycurl"""
> +
> +# TODO: test reporting of http errors
> +

PEP8 - separate std lib, third party and internal imports, and put in
alhpabetical order, and two lines of gap at the top level:

===
from StringIO import StringIO

import pycurl

from bzrlib.errors import TransportError, NoSuchFile
from bzrlib.trace import mutter
from bzrlib.transport import Transport
from bzrlib.transport.http import HttpTransportBase


class PyCurlTransport...
===

> +from StringIO import StringIO
> +import pycurl
> +
> +from bzrlib.trace import mutter
> +from bzrlib.errors import TransportError, NoSuchFile
> +from bzrlib.transport import Transport
> +from bzrlib.transport.http import HttpTransportBase
> +

One line of VWS after the class before the first class scope item other
than a docstring:

class PyCurlTransport(HttpTransportBase):

    def __init__(self, base):



> +class PyCurlTransport(HttpTransportBase):
> +    def __init__(self, base):
> +        super(PyCurlTransport, self).__init__(base)
> +        self.curl = pycurl.Curl()
> +        mutter('imported pycurl %s' % pycurl.version)
> +
> +    def get(self, relpath, decode=False):
> +        if decode:
> +            raise NotImplementedError
> +        return self._get_url(self.abspath(relpath))

Whats decode here? Its not in the main transport api .... and having it
raise makes it look rather - uhm - unneeded ?

> +    def _get_url(self, abspath):
> +        sio = StringIO()
> +        # pycurl needs plain ascii

All the transport functions should only be getting ascii - though I
haven't audited to make this the case yet. (They only work with URLs,
all URLs are 7-bit safe.)

> +        if isinstance(abspath, unicode):
> +            # XXX: HttpTransportBase.abspath should probably
> url-escape
> +            # unicode characters if any in the path - domain name
> must be
> +            # IDNA-escaped
> +            abspath = abspath.encode('ascii')
> +        self.curl.setopt(pycurl.URL, abspath)
> +        ## self.curl.setopt(pycurl.VERBOSE, 1)
> +        self.curl.setopt(pycurl.WRITEFUNCTION, sio.write)
> +        headers = ['Cache-control: must-revalidate',
> +                   'Pragma:']
> +        self.curl.setopt(pycurl.HTTPHEADER, headers)
> +        self.curl.perform()
> +        code = self.curl.getinfo(pycurl.HTTP_CODE)
> +        if code == 404:
> +            raise NoSuchFile(abspath)
> +        elif not 200 <= code <= 399:
> +            raise TransportError('http error %d acccessing %s' % 
> +                    (code, self.curl.getinfo(pycurl.EFFECTIVE_URL)))
> +        sio.seek(0)
> +        return sio
> +
> +
> 
> === modified file 'bzrlib/transport/__init__.py'
> --- bzrlib/transport/__init__.py        
> +++ bzrlib/transport/__init__.py        
> @@ -13,6 +13,7 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program; if not, write to the Free Software
>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> 02111-1307  USA
> +
>  """Transport is an abstraction layer to handle file access.
>  
>  The abstraction is to allow access from the local filesystem, as well
> @@ -82,7 +83,7 @@
>          using a subdirectory or parent directory. This allows
> connections 
>          to be pooled, rather than a new one needed for each subdir.
>          """
> -        raise NotImplementedError
> +        raise NotImplementedError(self.clone)

.. These should go into mainline regardless - +1 on them ;)



> === modified file 'bzrlib/transport/http.py'
> --- bzrlib/transport/http.py    
> +++ bzrlib/transport/http.py    
> @


...

this looks good +1 with the above tweaks ...

some small extra thoughts:
perhaps it would be cleaner to do:
mkdir bzrlib/transport/http
bzr add bzrlib/transport/http
bzr mv bzrlib/transport/http.py bzrlib/transport/http/__init__.py

Then, have _pycurl.py and _urllib.py containing the respective pycurl
and urllib implementations, with the common code in __init__.py

As for which implementation is default, if we change the protocol
handler to allow a *list* of handlers for a prefix, then we can just
probe until one allows itself to be constructed, and if none do, then
complain/provide the last error.

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: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060112/c985c399/attachment.pgp