Merge lp:~songofacandy/bzr/fix-523746-2 into lp:bzr/2.0

[methane]

[methane (songofacandy)]

Make sure that temporary file name is ASCII.

[Martin Packman (gz)]

I think the basic idea is sound here, just a couple of questions.

Are the mangled filenames going to be in the output of the external tool? If so there needs to be some unmangling code.

Can relpath be a string like u"docs/マニュアル/index.html"? If so we risk losing path components, should use 'replace' for encoding errors and then do .replace("?", "_") to make it windows-happy.

Could use `sys.getfilesystemencoding() or 'ascii'` rather than just 'ascii' as the encoding. Likewise could use `(path) or 'FILE'` rather than `'_'+(path)` to avoid the empty string.

review: Needs Information

[methane (songofacandy)]

> Are the mangled filenames going to be in the output of the external tool? If
> so there needs to be some unmangling code.

These filenames are just an input of the external diff tool.

> Can relpath be a string like u"docs/マニュアル/index.html"? If so we risk losing
> path components, should use 'replace' for encoding errors and then do
> .replace("?", "_") to make it windows-happy.

You're right! Adding "_" prefix is not enough.

> Could use `sys.getfilesystemencoding() or 'ascii'` rather than just 'ascii' as
> the encoding. Likewise could use `(path) or 'FILE'` rather than `'_'+(path)`
> to avoid the empty string.

I've made it.
Please review again.

[Martin Packman (gz)]

Ack, was that big a follow up needed? I only intended to suggest something like:

- full_path = osutils.pathjoin(self._root, prefix, relpath)
+ full_path = osutils.pathjoin(self._root, prefix, relpath.encode(
+ sys.getfilesystemencoding() or 'ascii', 'replace').replace(
+ "?", "_"))

The new encoding-then-decoding seems redundant, as do the changes in `_get_command`, what prompted you to add that when the last version was so simple?

[methane (songofacandy)]

> Ack, was that big a follow up needed? I only intended to suggest something
> like:
>
> - full_path = osutils.pathjoin(self._root, prefix, relpath)
> + full_path = osutils.pathjoin(self._root, prefix, relpath.encode(
> + sys.getfilesystemencoding() or 'ascii', 'replace').replace(
> + "?", "_"))
>
> The new encoding-then-decoding seems redundant, as do the changes in
> `_get_command`, what prompted you to add that when the last version was so
> simple?

Multibyte encoding may contains '?' as a trailing char.
So ``full_path.encode('mbcs', 'replace').replace('?', '_')`` may break
multibyte char.

And I hate self._root or prefix is changed through encode().decode() and bzr
attempts to write outside of _root.
So I do:
1) Write temporary file with safe unicode path.
2) Launch external diff with encoded path.

[Martin Packman (gz)]

> Multibyte encoding may contains '?' as a trailing char.
> So ``full_path.encode('mbcs', 'replace').replace('?', '_')`` may break
> multibyte char.

Really? Even SJIS avoids that one, and it's pretty good at breaking naïve programs. I get the reasoning now though.

> And I hate self._root or prefix is changed through encode().decode() and bzr
> attempts to write outside of _root.
> So I do:
> 1) Write temporary file with safe unicode path.
> 2) Launch external diff with encoded path.

Hm, I see. Probably overly cautious though, the windows filesystem layer will just be decoding by the system locale internally anyway.

[methane (songofacandy)]

> > Multibyte encoding may contains '?' as a trailing char.
> > So ``full_path.encode('mbcs', 'replace').replace('?', '_')`` may break
> > multibyte char.
>
> Really? Even SJIS avoids that one, and it's pretty good at breaking naïve
> programs. I get the reasoning now though.

I've checked SJIS and BIG5 a little while ago, and find '?' (0x3f) is not in trailing char.
So replace('?', '_') may be safe.

But I doesn't like depending character mapping.
I think encoded string is magic bytes and modifying it is dangerous.

[Martin Packman (gz)]

> But I doesn't like depending character mapping.
> I think encoded string is magic bytes and modifying it is dangerous.

Yeah, fair enough. Perhaps dealing with this just for tempfiles is too much and sticking to ascii-only for the mangling is the best choice.

[methane (songofacandy)]

> Perhaps dealing with this just for tempfiles is too much
> and sticking to ascii-only for the mangling is the best choice.

So I use ascii before.
But I find filename of tempfile is shown on GUI Diff title.
"old/????/????.txt - new/????/????.txt" isn't cool.

[John A Meinel (jameinel)]

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

Martin [gz] wrote:
> Ack, was that big a follow up needed? I only intended to suggest something like:
>
> - full_path = osutils.pathjoin(self._root, prefix, relpath)
> + full_path = osutils.pathjoin(self._root, prefix, relpath.encode(
> + sys.getfilesystemencoding() or 'ascii', 'replace').replace(
> + "?", "_"))

^- This should probably be 'osutils._fs_enc' where we already handle fs
encoding of None.

>
> The new encoding-then-decoding seems redundant, as do the changes in `_get_command`, what prompted you to add that when the last version was so simple?

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

iEYEARECAAYFAkuH3lsACgkQJdeBCYSNAANLJACgvw/A8y0EKW4chN31ewdmof2r
oqoAn3NAsSjyZ7vcvcdL6sOLKEjgg2Tq
=eWJM
-----END PGP SIGNATURE-----

[methane (songofacandy)]

> Martin [gz] wrote:
> > Ack, was that big a follow up needed? I only intended to suggest something
> like:
> >
> > - full_path = osutils.pathjoin(self._root, prefix, relpath)
> > + full_path = osutils.pathjoin(self._root, prefix, relpath.encode(
> > + sys.getfilesystemencoding() or 'ascii', 'replace').replace(
> > + "?", "_"))
>
> ^- This should probably be 'osutils._fs_enc' where we already handle fs
> encoding of None.

subprocess.Popen uses os.execv(), and os.execv() use Py_FileSystemDefaultEncoding (=sys.getfilesystemencoding()) to encode arguments. When filesystemencoding is NULL, fallbacks to defaultencoding.

osutils._fs_enc fallbacks to 'utf-8'. So using osutils._fs_enc may cause UnicodeEncodeError on Popen.
# I think osutils._fs_enc fallbacks to utf-8 is good because I hope bzr uses
# utf-8 when I set LANG=C.

[John A Meinel (jameinel)]

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

INADA Naoki wrote:
>> Martin [gz] wrote:
>>> Ack, was that big a follow up needed? I only intended to suggest something
>> like:
>>> - full_path = osutils.pathjoin(self._root, prefix, relpath)
>>> + full_path = osutils.pathjoin(self._root, prefix, relpath.encode(
>>> + sys.getfilesystemencoding() or 'ascii', 'replace').replace(
>>> + "?", "_"))
>> ^- This should probably be 'osutils._fs_enc' where we already handle fs
>> encoding of None.
>
> subprocess.Popen uses os.execv(), and os.execv() use Py_FileSystemDefaultEncoding (=sys.getfilesystemencoding()) to encode arguments. When filesystemencoding is NULL, fallbacks to defaultencoding.
>
> osutils._fs_enc fallbacks to 'utf-8'. So using osutils._fs_enc may cause UnicodeEncodeError on Popen.
> # I think osutils._fs_enc fallbacks to utf-8 is good because I hope bzr uses
> # utf-8 when I set LANG=C.

If you set LANG=C then we see fs_enc as ascii, not None.

The change was primarily for BSD, where fs_enc was None, and we wanted
UTF-8 as the most reasonable option.

However, if execv is going to fail if we pass something other than
ascii, then fair enough.

John
=:->

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

iEYEARECAAYFAkuH/RkACgkQJdeBCYSNAAPZdACgo0Dy748l7zTEzTElE5Gj8tL5
ZnMAoJO+qwFcz1H92xlmlDNt2VoskBII
=W/Bm
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

This sounds far too controversial for landing in 2.0 (especially without tests), can we have a summary from Naoki and reviewers about landing this patch into bzr.dev instead ?

review: Needs Information

[methane (songofacandy)]

This problem depends on OS, locale settings and other environment.
So I agree with you that this patch should land to bzr.dev and be tested on
vary environment while bzr 2.2 is released.

[methane (songofacandy)]

I've made new merge proposal to bzr.dev.
https://code.launchpad.net/~songofacandy/bzr/fix-523746-dev/+merge/20537

=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py	2009-07-29 21:35:05 +0000
+++ bzrlib/diff.py	2010-02-26 11:17:14 +0000
@@ -682,7 +682,17 @@
 
     def _get_command(self, old_path, new_path):
         my_map = {'old_path': old_path, 'new_path': new_path}
-        return [t % my_map for t in self.command_template]
+        command = [t % my_map for t in self.command_template]
+        if sys.platform == 'win32': # Popen doesn't accept unicode on win32
+            command_encoded = []
+            for c in command:
+                if isinstance(c, unicode):
+                    command_encoded.append(c.encode('mbcs'))
+                else:
+                    command_encoded.append(c)
+            return command_encoded
+        else:
+            return command
 
     def _execute(self, old_path, new_path):
         command = self._get_command(old_path, new_path)
@@ -709,7 +719,15 @@
         return True
 
     def _write_file(self, file_id, tree, prefix, relpath):
-        full_path = osutils.pathjoin(self._root, prefix, relpath)
+        if sys.platform == 'win32':
+            fenc = 'mbcs'
+        else:
+            fenc = sys.getfilesystemencoding()
+        # encoded_str.replace('?', '_') may break multibyte char.
+        # So we should encode, decode, then replace(u'?', u'_')
+        relpath_tmp = relpath.encode(fenc, 'replace').decode(fenc, 'replace')
+        relpath_tmp = relpath_tmp.replace(u'?', u'_')
+        full_path = osutils.pathjoin(self._root, prefix, relpath_tmp)
         if self._try_symlink_root(tree, prefix):
             return full_path
         parent_dir = osutils.dirname(full_path)
@@ -750,9 +768,9 @@
     def diff(self, file_id, old_path, new_path, old_kind, new_kind):
         if (old_kind, new_kind) != ('file', 'file'):
             return DiffPath.CANNOT_DIFF
-        self._prepare_files(file_id, old_path, new_path)
-        self._execute(osutils.pathjoin('old', old_path),
-                      osutils.pathjoin('new', new_path))
+        old_disk_path, new_disk_path = \
+                self._prepare_files(file_id, old_path, new_path)
+        self._execute(old_disk_path, new_disk_path)
 
 
 class DiffTree(object):