[merge] Enable writable http transports

[John Arbash Meinel]

Vincent Ladeuil wrote:
> The webdav plugin:
> 
>   https://launchpad.net/people/v-ladeuil/+branch/bzr.webdav/trunk
> 
> could make good use of the attached patch.
> 
> A branch is available here:
> 
>   https://launchpad.net/people/v-ladeuil/+branch/bzr/for-webdav
> 
> The attached bundle have been build after a merge from bzr.dev
> 0.11.0dev0 so I put again the overview of the modifications
> below:
> 
> # message:
> #   Allows writable tranport for copy_to and multiple '+' in transport
> #   protocols.
> #   
> #   * bzrlib/transport/http/__init__.py:
> #   (HttpTransportBase.__init__): Allows multiple '+qualifier' in
> #   transport protocol.
> #   (HttpTransportBase.copy_to): Allows writable transports.
> #   
> #   * bzrlib/transport/__init__.py:
> #   (Transport.put_file_non_atomic): Protect file position.
> 
> 
> 
> ------------------------------------------------------------------------
> 
> # Bazaar revision bundle v0.8
> #
> # message:
> #   Merge bzr.dev
> # committer: v.ladeuil+lp at free.fr
> # date: Mon 2006-09-11 11:50:07.848999977 +0200
> 
> === modified file bzrlib/transport/__init__.py
> --- bzrlib/transport/__init__.py
> +++ bzrlib/transport/__init__.py
> @@ -586,6 +586,8 @@
>                          create it, and then try again.
>          :param dir_mode: Possible access permissions for new directories.
>          """
> +        # Keep file position in case something goes wrong at first put_file try
> +        f_pos = f.tell()
>          # Default implementation just does an atomic put.
>          try:
>              return self.put_file(relpath, f, mode=mode)
> @@ -595,6 +597,7 @@
>              parent_dir = osutils.dirname(relpath)
>              if parent_dir:
>                  self.mkdir(parent_dir, mode=dir_mode)
> +                f.seek(f_pos)
>                  return self.put_file(relpath, f, mode=mode)

^- Vila and I discussed this a little bit on IRC.
The problem is that urllib directly returns a socket, which doesn't have
tell() or seek(). So far, we haven't had any problems without it,
because SFTP and Local both fail at open() time, not on the first write.
However HTTP actually fails after trying to send the contents of the files.

The webdav plugin itself works around this by using a different
implementation of put_file_non_atomic() (It implements _bytes, and just
reads the file 1 time).

I would be interested to hear from the group, though. Whether we would
prefer to require transports to fail on put() without changing the file
pointer, or for the file-like objects to have tell()/seek() members.
Or just require that any transport that would modify the file pointer
implement custom handlers for stuff like this.

Note: this also has an issue for our old implementation of append/put,
where we get an exception, then call mkdir, and then try again. Because
the file position may or may not have changed, but may or may not have a
seek/tell to force it back to the right location.

If I think about it, though. I think we wrap a StringIO() around the
object returned by urllib, because Gzip expects tell() and seek(). I
would like to avoid that, but we may not have a choice.


>      def __init__(self, base):
>          """Set the base path where files will be stored."""
> -        proto_match = re.match(r'^(https?)(\+\w+)?://', base)
> +        proto_match = re.match(r'^(https?)(\+\w+)*://', base)

^- This is so you can have multiple levels, right? "http+foo+bar+baz://"

>          if not proto_match:
>              raise AssertionError("not a http url: %r" % base)
>          self._proto = proto_match.group(1)
> @@ -321,7 +321,7 @@
>          # At this point HttpTransport might be able to check and see if
>          # the remote location is the same, and rather than download, and
>          # then upload, it could just issue a remote copy_this command.
> -        if isinstance(other, HttpTransportBase):
> +        if isinstance(other, HttpTransportBase) and other.is_readonly():
>              raise TransportNotPossible('http cannot be the target of copy_to()')
>          else:
>              return super(HttpTransportBase, self).\


^- We also talked about this on IRC, and we decided it was better for
Webdav to just re-implement 'copy_to()'. However, I realize now that it
isn't the case.
Because you could have a plain HTTP transport trying to 'copy_to()' a
WebDav transport.
I think a better solution would be to just remove 'copy_to' from
HttpTransportBase completely, and then you'll just get the standard
error from 'put()' about not being able to write to the remote
transport. No reason to short-cut early when the results may be wrong :)

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/20060912/453fcd78/attachment.pgp