Merge lp:~vila/bzr/472161-ftp-utf8 into lp:bzr

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

This fix bug #472161 by... simply deleting _remote_path().

It also makes osutils.fancy_rename() more robust against uses with true unicode paths.

I'm not super proud of the trick to disable the unicode tests against medusa,
but I didn't want to make more involved changes for an ftp server that we
don't support with python >= 2.6.

I finally found RFC3640 which clearly states that Unicode paths are to be supported
as utf8-encoded by default.

The RFC mentions the FEAT command to ensure that the server supports UTF8 but I didn't
implement it to avoid the additional roundtrip (we'll find soon enough if UTF8 is not supported).

Feedback and real-world tests welcome !

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/472161-ftp-utf8 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This fix bug #472161 by... simply deleting _remote_path().
>
> It also makes osutils.fancy_rename() more robust against uses with true unicode paths.
>
> I'm not super proud of the trick to disable the unicode tests against medusa,
> but I didn't want to make more involved changes for an ftp server that we
> don't support with python >= 2.6.
>
> I finally found RFC3640 which clearly states that Unicode paths are to be supported
> as utf8-encoded by default.
>
> The RFC mentions the FEAT command to ensure that the server supports UTF8 but I didn't
> implement it to avoid the additional roundtrip (we'll find soon enough if UTF8 is not supported).
>
> Feedback and real-world tests welcome !
>

 review: approve
 merge: approve

I did not test this manually, but the code looks good and your
descriptions seem fine.

I think this is something that really just needs real-world testing. FTP
servers are pretty notorious for handling edge cases differently.

+ no_unicode_support = getattr(self._server,
'_no_unicode_support', False)
+ if no_unicode_support:
+ raise TestSkipped("test server cannot handle unicode paths")
+

^- Arguably this would be better seeing something actually fail and
raising knownFailure. At least, I prefer XFAIL because then when we fix
stuff, we know which tests should be updated.

I probably wouldn't make it an '_' attribute. Instead just do
"no_unicode_support".

But what you have is fine if you just want to land it.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktGAL0ACgkQJdeBCYSNAAOBXQCgmykxzpdWWyWBGROuIIiVm3p6
EEQAnA+UwN8IcUmrAiPG4v5SC2Lk8KL7
=yNOA
-----END PGP SIGNATURE-----

review: Approve

=== modified file 'NEWS'
--- NEWS	2010-01-07 06:34:14 +0000
+++ NEWS	2010-01-07 14:15:28 +0000
@@ -61,6 +61,9 @@
   returns ``EINTR`` by calling ``PyErr_CheckSignals``.  This affected the
   optional ``_readdir_pyx`` extension.  (Andrew Bennetts, #495023)
 
+* FTP transports support Unicode paths by encoding/decoding them as utf8.
+  (Vincent Ladeuil, #472161)
+
 * Give a clearer message if the lockdir disappears after being apparently
   successfully taken.  (Martin Pool, #498378)
 
 
=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2009-12-23 00:15:34 +0000
+++ bzrlib/osutils.py	2010-01-07 14:15:27 +0000
@@ -202,13 +202,15 @@
     :param old: The old path, to rename from
     :param new: The new path, to rename to
     :param rename_func: The potentially non-atomic rename function
-    :param unlink_func: A way to delete the target file if the full rename succeeds
+    :param unlink_func: A way to delete the target file if the full rename
+        succeeds
     """
-
+    new = safe_unicode(new)
     # sftp rename doesn't allow overwriting, so play tricks:
     base = os.path.basename(new)
     dirname = os.path.dirname(new)
-    tmp_name = u'tmp.%s.%.9f.%d.%s' % (base, time.time(), os.getpid(), rand_chars(10))
+    tmp_name = u'tmp.%s.%.9f.%d.%s' % (base, time.time(),
+                                       os.getpid(), rand_chars(10))
     tmp_name = pathjoin(dirname, tmp_name)
 
     # Rename the file out of the way, but keep track if it didn't exist
 
=== modified file 'bzrlib/tests/ftp_server/medusa_based.py'
--- bzrlib/tests/ftp_server/medusa_based.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/ftp_server/medusa_based.py	2010-01-07 14:15:27 +0000
@@ -213,6 +213,8 @@
 class FTPTestServer(transport.Server):
     """Common code for FTP server facilities."""
 
+    _no_unicode_support = True
+
     def __init__(self):
         self._root = None
         self._ftp_server = None
 
=== modified file 'bzrlib/tests/ftp_server/pyftpdlib_based.py'
--- bzrlib/tests/ftp_server/pyftpdlib_based.py	2009-10-06 08:24:14 +0000
+++ bzrlib/tests/ftp_server/pyftpdlib_based.py	2010-01-07 14:15:27 +0000
@@ -50,14 +50,15 @@
 
     def listdir(self, path):
         """List the content of a directory."""
-        # FIXME: need tests with unicode paths
         return [osutils.safe_utf8(s) for s in os.listdir(path)]
 
     def fs2ftp(self, fspath):
         p = ftpserver.AbstractedFS.fs2ftp(self, fspath)
-        # FIXME: need tests with unicode paths
         return osutils.safe_utf8(p)
 
+    def ftp2fs(self, ftppath):
+        p = osutils.safe_unicode(ftppath)
+        return ftpserver.AbstractedFS.ftp2fs(self, p)
 
 class BzrConformingFTPHandler(ftpserver.FTPHandler):
 
 
=== modified file 'bzrlib/tests/per_transport.py'
--- bzrlib/tests/per_transport.py	2009-10-05 12:50:34 +0000
+++ bzrlib/tests/per_transport.py	2010-01-07 14:15:27 +0000
@@ -1497,6 +1497,10 @@
                  u'\u65e5', # Kanji person
                 ]
 
+        no_unicode_support = getattr(self._server, '_no_unicode_support', False)
+        if no_unicode_support:
+            raise TestSkipped("test server cannot handle unicode paths")
+
         try:
             self.build_tree(files, transport=t, line_endings='binary')
         except UnicodeError:
 
=== modified file 'bzrlib/transport/ftp/__init__.py'
--- bzrlib/transport/ftp/__init__.py	2009-10-06 08:24:14 +0000
+++ bzrlib/transport/ftp/__init__.py	2010-01-07 14:15:27 +0000
@@ -205,17 +205,6 @@
         #raise TransportError(msg='Error for path: %s' % (path,), orig_error=e)
         raise
 
-    def _remote_path(self, relpath):
-        # XXX: It seems that ftplib does not handle Unicode paths
-        # at the same time, medusa won't handle utf8 paths So if
-        # we .encode(utf8) here (see ConnectedTransport
-        # implementation), then we get a Server failure.  while
-        # if we use str(), we get a UnicodeError, and the test
-        # suite just skips testing UnicodePaths.
-        relative = str(urlutils.unescape(relpath))
-        remote_path = self._combine_paths(self._path, relative)
-        return remote_path
-
     def has(self, relpath):
         """Does the target location exist?"""
         # FIXME jam 20060516 We *do* ask about directories in the test suite