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

[methane]

[methane (songofacandy)]

Fix UnicodeEncodeError when invoking external diff with non ASCII filenames.

[Robert Collins (lifeless)]

I recall subprocess being very slow to import; it worries me that we'll
be importing it all the time.

A quick check on my machine took about 10ms to import, so its definitely
noticable.

-Rob

[John A Meinel (jameinel)]

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

Robert Collins wrote:
> I recall subprocess being very slow to import; it worries me that we'll
> be importing it all the time.
>
> A quick check on my machine took about 10ms to import, so its definitely
> noticable.
>
> -Rob
>

It can pretty easily be moved into the lazy_import section.

Also, this doesn't handle stuff like Unicode filenames that are valid on
disk, but not valid in user encoding. (Which can be pretty easy to
trigger on Windows.)

However, it is probably better than what we have today.

Actually, we have a different problem. In that 'get_user_encoding()' !=
'sys.get_filesystemencoding'. I'm not 100% sure what needs to be passed
to programs.

The real fix is to use CreateProcessW and pass it a unicode string,
rather than trying to get everything round-tripped across an encoding
barrier.

This may be a stepping point in that direction, though.

John
=:->

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

iEYEARECAAYFAkt+9AQACgkQJdeBCYSNAANcDACeMCCHcSVntHnO3XkvWLzCj82R
H2IAoMGFEiBOXh+t4Z0W+C64WG59gPWo
=6JUh
-----END PGP SIGNATURE-----

[methane (songofacandy)]

>> I recall subprocess being very slow to import; it worries me that we'll
>> be importing it all the time.
>>
>> A quick check on my machine took about 10ms to import, so its definitely
>> noticable.
>>
>> -Rob
>>
>
>It can pretty easily be moved into the lazy_import section.

osutils.Popen is a class. So subprocess imported every times
when import osutils.
I'll make osutils.Popen as a factory function instead of class.

> Actually, we have a different problem. In that 'get_user_encoding()' !=
> 'sys.get_filesystemencoding'. I'm not 100% sure what needs to be passed
> to programs.

Yes. Arguments contains file path and others texts.
I think there are no single right way to encode process arguments.
But I change to use filesystemencoding because file paths appears in
arguments rather than non-ASCII text arguments.
In most case, filesystemencoding == user_encoding(). So I think using
filesystemencoding solves 90% case.

> The real fix is to use CreateProcessW and pass it a unicode string,
> rather than trying to get everything round-tripped across an encoding
> barrier.

On Windows, CreateProcessW is good. But this bug affects not only Windows.
Popen encoding arguments with defaultencoding so popen can't encode utf-8
file path on Linux, too.

To use CreateProcessW, we should decode bytes strings into unicode.
This brings another problem: "What encoding should we use when decoding".
And to use CreateProcessW, we should change subprocess internals or reimplement
subprocess module.

So, just for now, I'd like to fix 90% case.

[lp:~songofacandy/bzr/fix-523746]

4737. By methane on 2010-02-20

Make Popen as a factory function.

4738. By methane on 2010-02-20

Use _fs_enc instead of user_encoding()

4739. By methane on 2010-02-20

Add test and fix easy miss.

[methane (songofacandy)]

I resubmitted merge proposal.

[lp:~songofacandy/bzr/fix-523746]

4740. By methane on 2010-02-20

Fix import ordering.

[John A Meinel (jameinel)]

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

INADA Naoki wrote:
>>> I recall subprocess being very slow to import; it worries me that we'll
>>> be importing it all the time.
>>>
>>> A quick check on my machine took about 10ms to import, so its definitely
>>> noticable.
>>>
>>> -Rob
>>>
>> It can pretty easily be moved into the lazy_import section.
>
> osutils.Popen is a class. So subprocess imported every times
> when import osutils.
> I'll make osutils.Popen as a factory function instead of class.
>
>
>> Actually, we have a different problem. In that 'get_user_encoding()' !=
>> 'sys.get_filesystemencoding'. I'm not 100% sure what needs to be passed
>> to programs.
>
> Yes. Arguments contains file path and others texts.
> I think there are no single right way to encode process arguments.
> But I change to use filesystemencoding because file paths appears in
> arguments rather than non-ASCII text arguments.
> In most case, filesystemencoding == user_encoding(). So I think using
> filesystemencoding solves 90% case.

Only outsidef of Windows. On Windows filesystemencoding == mbcs, but
user_encoding is something like 'cp1252'. (and terminal encoding is
cp437...)

Now, I don't know whether arbitrary programs are going to use filesystem
encoding or user_encoding for decoding their arguments. I *think* that
argument decoding would be an OS sort of thing, and we use
filesystemencoding for ENV variables and thus it would make sense for
arguments. bzr itself used user_encoding before we use the Unicode api.

>
>> The real fix is to use CreateProcessW and pass it a unicode string,
>> rather than trying to get everything round-tripped across an encoding
>> barrier.
>
> On Windows, CreateProcessW is good. But this bug affects not only Windows.
> Popen encoding arguments with defaultencoding so popen can't encode utf-8
> file path on Linux, too.

Sure. And on Linux, it has pretty much standardized that everything is
in utf-8. Which is *soooo* much nicer. (file content, filenames, env
vars, terminal encoding, etc.)

>
> To use CreateProcessW, we should decode bytes strings into unicode.
> This brings another problem: "What encoding should we use when decoding".
> And to use CreateProcessW, we should change subprocess internals or reimplement
> subprocess module.
>
> So, just for now, I'd like to fix 90% case.
>

As long as we don't break other cases, I'm fine with that.

John
=:->

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

iEYEARECAAYFAkuC3GUACgkQJdeBCYSNAAMUuwCfS28ggzvo1JNRialesoqv8Slw
U2MAn0J3tGr5HGGOPIqEz+w7CCm10hKm
=V9+v
-----END PGP SIGNATURE-----

[lp:~songofacandy/bzr/fix-523746]

4741. By methane on 2010-02-23

Don't use Conditional Expressions for Python 2.4 compatibility.

Unmerged revisions

4741. By methane on 2010-02-23

Don't use Conditional Expressions for Python 2.4 compatibility.

4740. By methane on 2010-02-20

Fix import ordering.

4739. By methane on 2010-02-20

Add test and fix easy miss.

4738. By methane on 2010-02-20

Use _fs_enc instead of user_encoding()

4737. By methane on 2010-02-20

Make Popen as a factory function.

4736. By methane on 2010-02-19

Wrap Popen() to support unicode arguments.

=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py	2009-07-29 21:35:05 +0000
+++ bzrlib/diff.py	2010-02-20 05:02:13 +0000
@@ -131,11 +131,11 @@
         stderr = None
 
     try:
-        pipe = subprocess.Popen(diffcmd,
-                                stdin=subprocess.PIPE,
-                                stdout=subprocess.PIPE,
-                                stderr=stderr,
-                                env=env)
+        pipe = osutils.Popen(diffcmd,
+                             stdin=subprocess.PIPE,
+                             stdout=subprocess.PIPE,
+                             stderr=stderr,
+                             env=env)
     except OSError, e:
         if e.errno == errno.ENOENT:
             raise errors.NoDiff(str(e))
@@ -171,11 +171,6 @@
 
         if not diff_opts:
             diff_opts = []
-        if sys.platform == 'win32':
-            # Popen doesn't do the proper encoding for external commands
-            # Since we are dealing with an ANSI api, use mbcs encoding
-            old_filename = old_filename.encode('mbcs')
-            new_filename = new_filename.encode('mbcs')
         diffcmd = ['diff',
                    '--label', old_filename,
                    old_abspath,
@@ -687,8 +682,8 @@
     def _execute(self, old_path, new_path):
         command = self._get_command(old_path, new_path)
         try:
-            proc = subprocess.Popen(command, stdout=subprocess.PIPE,
-                                    cwd=self._root)
+            proc = osutils.Popen(command, stdout=subprocess.PIPE,
+                                 cwd=self._root)
         except OSError, e:
             if e.errno == errno.ENOENT:
                 raise errors.ExecutableMissing(command[0])

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2009-10-08 03:55:30 +0000
+++ bzrlib/osutils.py	2010-02-20 05:02:13 +0000
@@ -34,11 +34,11 @@
                     splitdrive as _nt_splitdrive,
                     )
 import posixpath
+import subprocess
 import shutil
 from shutil import (
     rmtree,
     )
-import subprocess
 import tempfile
 from tempfile import (
     mkdtemp,
@@ -1907,3 +1907,12 @@
     if use_cache:
         _cached_concurrency = concurrency
     return concurrency
+ 
+def Popen(subprocess_args, *args, **kwargs):
+    """A wrapper for Popen for unicode arguments."""
+    if not isinstance(subprocess_args, basestring):
+        subprocess_args = [s.encode(_fs_enc) if isinstance(s, unicode) else s
+                           for s in subprocess_args]
+    elif isinstance(subprocess_args, unicode):
+        subprocess_args = subprocess_args.encode(_fs_enc)
+    return subprocess.Popen(subprocess_args, *args, **kwargs)

=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2009-10-08 17:13:24 +0000
+++ bzrlib/tests/test_osutils.py	2010-02-20 05:02:13 +0000
@@ -24,6 +24,7 @@
 import stat
 import sys
 import time
+import subprocess
 
 from bzrlib import (
     errors,
@@ -1841,3 +1842,9 @@
     def test_local_concurrency(self):
         concurrency = osutils.local_concurrency()
         self.assertIsInstance(concurrency, int)
+
+class TestPopen(tests.TestCase):
+
+    def test_unicode_args(self):
+        proc = osutils.Popen(["echo", u"a\N{Euro Sign}b"], stdout=subprocess.PIPE)
+        self.assertEquals(proc.wait(), 0)