Code review comment for lp:~vila/bzr/472161-ftp-utf8

-----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-----