invoking external diff failed for non-ascii files

[Alexander Belchenko]

C:\Temp\4\a>bzr diff -c-1
=== modified file 'Тест.txt'
--- Тест.txt 2009-06-02 10:38:34 +0000
+++ Тест.txt 2009-06-02 10:41:44 +0000
@@ -1,1 +1,1 @@
-foo
+bar

I've set up WinMerge as external diff application. When I'm trying to run qdiff from qlog I've got error dialog with following traceback:

=== modified file 'Тест.txt'
bzr: ERROR: exceptions.UnicodeEncodeError: 'ascii' codec can't encode characters in position 44-47: ordinal not in range(128)

Traceback (most recent call last):
  File "bzrlib\commands.pyo", line 729, in exception_to_return_code
  File "bzrlib\commands.pyo", line 924, in run_bzr
  File "bzrlib\commands.pyo", line 560, in run_argv_aliases
  File "C:\work\Bazaar\plugins\qbzr\lib\commands.py", line 753, in run
  File "bzrlib\commands.pyo", line 924, in run_bzr
  File "bzrlib\commands.pyo", line 560, in run_argv_aliases
  File "bzrlib\commands.pyo", line 939, in ignore_pipe
  File "bzrlib\builtins.pyo", line 1811, in run
  File "bzrlib\diff.pyo", line 427, in show_diff_trees
  File "bzrlib\diff.pyo", line 825, in show_diff
  File "bzrlib\diff.pyo", line 885, in _show_diff
  File "bzrlib\diff.pyo", line 908, in diff
  File "bzrlib\diff.pyo", line 486, in _diff_many
  File "bzrlib\diff.pyo", line 741, in diff
  File "bzrlib\diff.pyo", line 682, in _execute
  File "subprocess.pyo", line 594, in __init__
  File "subprocess.pyo", line 822, in _execute_child
UnicodeEncodeError: 'ascii' codec can't encode characters in position 44-47: ordinal not in range(128)

bzr 1.15 on python 2.5.4 (win32)
arguments: ['C:\\Program Files\\Bazaar\\bzr.EXE', 'qsubprocess', '"diff" "--using=C:/program files/winmerge/winmergeu.exe" -r revid:<email address hidden>:<email address hidden>']
encoding: 'cp1251', fsenc: 'mbcs', lang: None
plugins:
  acad C:\work\Bazaar\plugins\acad [0.8]
  bzrtools C:\Program Files\Bazaar\plugins\bzrtools [1.15]
  extmerge C:\work\Bazaar\plugins\extmerge [unknown]
  fastimport C:\work\Bazaar\plugins\fastimport [0.8dev]
  launchpad C:\Program Files\Bazaar\plugins\launchpad [1.15]
  qbzr C:\work\Bazaar\plugins\qbzr [0.10dev]
  rebase C:\work\Bazaar\plugins\rebase [0.5.1dev]
  scmproj C:\work\Bazaar\plugins\scmproj [0.4.5.dev.2]
  x_bit C:\work\Bazaar\plugins\x_bit [1.0]
*** Bazaar has encountered an internal error.
    Please report a bug at https://bugs.launchpad.net/bzr/+filebug
    including this traceback, and a description of what you
    were doing when the error occurred.

[Gary van der Merwe (garyvdm)]

Please test if the following command works? If not, then this is bug with bzr, and not qbzr

bzr dif --using="C:/program files/winmerge/winmergeu.exe" -r revid:<email address hidden>:<email address hidden>

[Alexander Belchenko (bialix)]

Yes, plain diff failed. Steps to reproduce:

bzr init
echo foo > Тест.txt
bzr add
bzr ci -m1
echo bar > Тест.txt
bzr ci -m2

bzr --no-plugins diff --using "C:/Program Files/winmerge/winmergeu.exe" -c-1
=== modified file '╨в╨╡╤Б╤В.txt'
bzr: ERROR: exceptions.UnicodeEncodeError: 'ascii' codec can't encode characters in position 44-47: ordinal not in r
ange(128)

Traceback (most recent call last):
  File "bzrlib\commands.pyo", line 729, in exception_to_return_code
  File "bzrlib\commands.pyo", line 924, in run_bzr
  File "bzrlib\commands.pyo", line 560, in run_argv_aliases
  File "bzrlib\commands.pyo", line 939, in ignore_pipe
  File "bzrlib\builtins.pyo", line 1811, in run
  File "bzrlib\diff.pyo", line 427, in show_diff_trees
  File "bzrlib\diff.pyo", line 825, in show_diff
  File "bzrlib\diff.pyo", line 885, in _show_diff
  File "bzrlib\diff.pyo", line 908, in diff
  File "bzrlib\diff.pyo", line 486, in _diff_many
  File "bzrlib\diff.pyo", line 741, in diff
  File "bzrlib\diff.pyo", line 682, in _execute
  File "subprocess.pyo", line 594, in __init__
  File "subprocess.pyo", line 822, in _execute_child
UnicodeEncodeError: 'ascii' codec can't encode characters in position 44-47: ordinal not in range(128)

bzr 1.15 on python 2.5.4 (win32)
arguments: ['C:\\Program Files\\Bazaar\\bzr.EXE', '--no-plugins', 'diff', '--using', 'C:/Program Files/winmerge/winm
ergeu.exe', '-c-1']
encoding: 'cp1251', fsenc: 'mbcs', lang: 'ru'
plugins:
*** Bazaar has encountered an internal error.
    Please report a bug at https://bugs.launchpad.net/bzr/+filebug
    including this traceback, and a description of what you
    were doing when the error occurred.

This is bug in bzr itself.

[Alexander Belchenko (bialix)]

May be it's win32-specific bug.

[John A Meinel (jameinel)]

I would guess it is a problem with trying to create a command line for a non-ascii (and probably non-OEM) filename.

My guess is that python isn't using or isn't exporting up to us a way to CreateProcessW with unicode command line arguments.

And, of course, even if we get that right, the external 'diff' program needs to support that as well...

[Alexander Belchenko (bialix)]

John A Meinel пишет:
> I would guess it is a problem with trying to create a command line for a
> non-ascii (and probably non-OEM) filename.
>
> My guess is that python isn't using or isn't exporting up to us a way to
> CreateProcessW with unicode command line arguments.

AFAIK Python don't try to use unicode API there, maybe because of
Windows-98 compatibility. Or maybe python developers don't dig so deep
there.

> And, of course, even if we get that right, the external 'diff' program
> needs to support that as well...

Well, WinMerge works OK. I've just tested with this simple script:

-----------------------------
# -*- coding: cp1251 -*-

import subprocess

f1 = 'Тест.txt'
f2 = 'Тест2.txt'

subprocess.Popen('"C:/Program Files/WinMerge/WinMergeU.exe" %s %s' %
(f1, f2))
-----------------------------

[John A Meinel (jameinel)]

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

Alexander Belchenko wrote:
> John A Meinel пишет:
>> I would guess it is a problem with trying to create a command line for a
>> non-ascii (and probably non-OEM) filename.
>>
>> My guess is that python isn't using or isn't exporting up to us a way to
>> CreateProcessW with unicode command line arguments.
>
> AFAIK Python don't try to use unicode API there, maybe because of
> Windows-98 compatibility. Or maybe python developers don't dig so deep
> there.
>
>> And, of course, even if we get that right, the external 'diff' program
>> needs to support that as well...
>
> Well, WinMerge works OK. I've just tested with this simple script:
>
> -----------------------------
> # -*- coding: cp1251 -*-
>
> import subprocess
>
> f1 = 'Тест.txt'
> f2 = 'Тест2.txt'
>
> subprocess.Popen('"C:/Program Files/WinMerge/WinMergeU.exe" %s %s' %
> (f1, f2))
> -----------------------------
>

This indicates that the filenames are in OEM encoding (cp1251)
especially given the fact that you are doing:

  f1 = 'Тест.txt'

Which means that it isn't even a Unicode string.

I would certainly expect that to work. The question is whether it
supports paths which are valid Unicode but do not have an OEM
representation on your filesytem.

As mentioned previously, you could consider the filenames:
  u'\u0422\u0435\u0441\u0442.txt'
and
  u'\u062c\u0648\u062c\u0648.txt'

(Getting support for OEM valid filenames is still an improvement, but to
do it "correctly" we should support all the way up to the full Unicode
api. Just to be clear that if we have a patch to support OEM strings, we
should merge it, but we should try to get a patch which supports all
filenames.)

John
=:->

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

iEYEARECAAYFAkoua+AACgkQJdeBCYSNAANz9gCfShn6+V6dO5jPu7I9Ds4ghDzi
gIAAoNLkbT1N0rNSD2olHmRR58q0qHfc
=hs91
-----END PGP SIGNATURE-----

[Alexander Belchenko (bialix)]

John A Meinel пишет:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Alexander Belchenko wrote:
>> John A Meinel пишет:
>>> I would guess it is a problem with trying to create a command line for a
>>> non-ascii (and probably non-OEM) filename.
>>>
>>> My guess is that python isn't using or isn't exporting up to us a way to
>>> CreateProcessW with unicode command line arguments.
>> AFAIK Python don't try to use unicode API there, maybe because of
>> Windows-98 compatibility. Or maybe python developers don't dig so deep
>> there.
>>
>>> And, of course, even if we get that right, the external 'diff' program
>>> needs to support that as well...
>> Well, WinMerge works OK. I've just tested with this simple script:
>>
>> -----------------------------
>> # -*- coding: cp1251 -*-
>>
>> import subprocess
>>
>> f1 = 'Тест.txt'
>> f2 = 'Тест2.txt'
>>
>> subprocess.Popen('"C:/Program Files/WinMerge/WinMergeU.exe" %s %s' %
>> (f1, f2))
>> -----------------------------
>>
>
> This indicates that the filenames are in OEM encoding (cp1251)

No, OEM encoding is cp866. cp1251 is ANSI encoding. ANSI encoding used
for all non-unicode Win32 API functions, e.g. CreateProcessA (vs
CreateProcessW that uses unicode).

> especially given the fact that you are doing:
>
> f1 = 'Тест.txt'
>
> Which means that it isn't even a Unicode string.

Yes, this is the point of my test.

> I would certainly expect that to work. The question is whether it
> supports paths which are valid Unicode but do not have an OEM
> representation on your filesytem.

ANSI, not OEM. I.e. value from bzrlib.osutils.get_user_encoding()

> As mentioned previously, you could consider the filenames:
> u'\u0422\u0435\u0441\u0442.txt'
> and
> u'\u062c\u0648\u062c\u0648.txt'

I'm sure ANSI Win32 API unable to handle filenames outside of user
codepage. And because Python does not provide easy way to use unicode
API, I'm not sure you can do any better than print warning/error message.

> (Getting support for OEM valid filenames is still an improvement, but to
> do it "correctly" we should support all the way up to the full Unicode
> api. Just to be clear that if we have a patch to support OEM strings, we
> should merge it, but we should try to get a patch which supports all
> filenames.)

This may require to write own C-extension. It seems even pywin32 does
not provide unicode API for Create Process.

>
> John
> =:->
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (Cygwin)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iEYEARECAAYFAkoua+AACgkQJdeBCYSNAANz9gCfShn6+V6dO5jPu7I9Ds4ghDzi
> gIAAoNLkbT1N0rNSD2olHmRR58q0qHfc
> =hs91
> -----END PGP SIGNATURE-----
>

[John A Meinel (jameinel)]

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

...

> This indicates that the filenames are in OEM encoding (cp1251)
>
>> No, OEM encoding is cp866. cp1251 is ANSI encoding. ANSI encoding used
>> for all non-unicode Win32 API functions, e.g. CreateProcessA (vs
>> CreateProcessW that uses unicode).
>

cp1251 is window's version of latin-1.

> especially given the fact that you are doing:
>
> f1 = '"5AB.txt'
>
> Which means that it isn't even a Unicode string.
>
>> Yes, this is the point of my test.
>
> I would certainly expect that to work. The question is whether it
> supports paths which are valid Unicode but do not have an OEM
> representation on your filesytem.

I'll note that:
>>> x = u'\u0422\u0435\u0441\u0442.txt' # 'Тест.txt'
>>> x.encode('cp1251')
'\xd2\xe5\xf1\xf2.txt'
>>> x.encode('cp866')
'\x92\xa5\xe1\xe2.txt'

I don't know what you would get for
>>> x.encode('mbcs')

As it depends on the given filesystem. It fails here with:
>>> x.encode('mbcs')
'????.txt'

As 'Тест.txt' is not valid in my filesystem encoding. (I'm pretty sure
that 'mbcs' == OEM encoding, which is unknown until you actually
evaluate it.)

I'm guessing, but I would guesst that on your system, you will get:
>>> x.encode('mbcs')
'\xd2\xe5\xf1\xf2.txt'

or put another way
>>> x.encode('cp1251') == x.encode('mbcs')
True

The issue is that there is sys.getfilesystemencoding() (which always
returns 'mbcs' which only Windows knows what the OEM codepage is), and
there is "locale.getpreferredencoding()".

The former is the encoding for 8-bit paths on disk. The latter is the
encoding for the *content* of files.

In your test case, you explicitly define the encoding of the file as
cp1251, which means that it likely exactly matches the filesystem
encoding, which is why things "just work".

WinMerge doesn't need to be Unicode aware, and your python Popen doesn't
need to be Unicode aware, because they are just passing OEM strings to
eachother. Or perhaps I should say "they are passing MBCS strings" to
eachother. Where MBCS has a custom definition based on a Windows setting
(which I don't, ATM, know how to modify).

That is why I like testing with:
  u'\u062c\u0648\u062c\u0648.txt' # جوجو.txt

Because I know that it is never going to be a valid MBCS / OEM /
sys.getfilesystemencoding() / locale.getpreferredencoding() /
bzrlib.osutils.get_user_encoding() etc

But it *is* a valid Win32 filesystem (UTF-16) path.

...

>
>> This may require to write own C-extension. It seems even pywin32 does
>> not provide unicode API for Create Process.
>
> John
> =:->

I was actually pretty surprised at this. Looking here:
http://msdn.microsoft.com/en-us/library/ms682425(VS.85).aspx

It looks like CreateProcess should be part of the Kernel32.lib api, but
it certainly isn't exported via ctypes.windll.kernel32.

I'm pretty sure you can define your own foreign function with ctypes,
without needing it to already be exposed via ctypes.windll (it is just
easier when it has already been done for you). So we wouldn't *have* to
wrap it.

I also see that subprocess itself does:

from win32process import CreateProcess, STARTUPINFO

And you also have
from _subprocess import *

So it seems that ...

[John A Meinel (jameinel)]

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

John Arbash Meinel wrote:
>
> ...
>
>> This indicates that the filenames are in OEM encoding (cp1251)
>
>>> No, OEM encoding is cp866. cp1251 is ANSI encoding. ANSI encoding used
>>> for all non-unicode Win32 API functions, e.g. CreateProcessA (vs
>>> CreateProcessW that uses unicode).
>
> cp1251 is window's version of latin-1.
>

Before anyone else corrects me. This is false.

I wrote it originally, and forgot to delete it when I updated the rest
of the text.

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

iEYEARECAAYFAkoufKoACgkQJdeBCYSNAAOnngCfV9qxuDUeUZ3Wf40moT9oxMJd
CjYAnAyVe11Ur2SQrn2d5P6cm/IdDl7t
=YAYW
-----END PGP SIGNATURE-----

[Alexander Belchenko (bialix)]

John A Meinel пишет:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> John Arbash Meinel wrote:
>> ...
>>
>>> This indicates that the filenames are in OEM encoding (cp1251)
>>>> No, OEM encoding is cp866. cp1251 is ANSI encoding. ANSI encoding used
>>>> for all non-unicode Win32 API functions, e.g. CreateProcessA (vs
>>>> CreateProcessW that uses unicode).
>> cp1251 is window's version of latin-1.
>>
>
> Before anyone else corrects me. This is false.
>
> I wrote it originally, and forgot to delete it when I updated the rest
> of the text.

You mean cp1252. This one is codepage for latin-1.

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

[Alexander Belchenko (bialix)]

John A Meinel пишет:
> ...
>
>> This indicates that the filenames are in OEM encoding (cp1251)
>>
>>> No, OEM encoding is cp866. cp1251 is ANSI encoding. ANSI encoding used
>>> for all non-unicode Win32 API functions, e.g. CreateProcessA (vs
>>> CreateProcessW that uses unicode).
>
> cp1251 is window's version of latin-1.
>
>> especially given the fact that you are doing:
>>
>> f1 = '"5AB.txt'
>>
>> Which means that it isn't even a Unicode string.
>>
>>> Yes, this is the point of my test.
>> I would certainly expect that to work. The question is whether it
>> supports paths which are valid Unicode but do not have an OEM
>> representation on your filesytem.
>
> I'll note that:
>>>> x = u'\u0422\u0435\u0441\u0442.txt' # 'Тест.txt'
>>>> x.encode('cp1251')
> '\xd2\xe5\xf1\xf2.txt'
>>>> x.encode('cp866')
> '\x92\xa5\xe1\xe2.txt'
>
> I don't know what you would get for
>>>> x.encode('mbcs')
>
> As it depends on the given filesystem. It fails here with:
>>>> x.encode('mbcs')
> '????.txt'
>
> As 'Тест.txt' is not valid in my filesystem encoding. (I'm pretty sure
> that 'mbcs' == OEM encoding, which is unknown until you actually
> evaluate it.)

I'm still think you're using OEM term incorrectly.

OEM refers to old DOS days. There is even special function in Win32 API
to decode from ANSI to OEM and vice versa.

All we talking about is related to ANSI encoding. Not OEM.
OEM encoding used only for terminal encoding (by default), that's why we
have bzrlib.osutils.get_terminal_encoding() function.

Please, use 'ANSI encoding' term.

> I'm guessing, but I would guesst that on your system, you will get:
>>>> x.encode('mbcs')
> '\xd2\xe5\xf1\xf2.txt'

Here is my quick test in IPython:

In [1]: x = u'\u0422\u0435\u0441\u0442.txt'

In [2]: x.encode('mbcs')
Out[2]: '\xd2\xe5\xf1\xf2.txt'

>
> or put another way
>>>> x.encode('cp1251') == x.encode('mbcs')
> True

I think this make a big sense. MBCS == ANSI == default user codepage.
Although on Japanese/Chinese Windows the result could be completely
different.

> The issue is that there is sys.getfilesystemencoding() (which always
> returns 'mbcs' which only Windows knows what the OEM codepage is), and
> there is "locale.getpreferredencoding()".
>
> The former is the encoding for 8-bit paths on disk. The latter is the
> encoding for the *content* of files.

This bug is about filenames. So, if you think 'mbcs' is right thing here
-- I'm agree.

> In your test case, you explicitly define the encoding of the file as
> cp1251, which means that it likely exactly matches the filesystem
> encoding, which is why things "just work".

I don't see how the content of file related to filename.

> WinMerge doesn't need to be Unicode aware, and your python Popen doesn't
> need to be Unicode aware, because they are just passing OEM strings to
> eachother. Or perhaps I should say "they are passing MBCS strings" to
> eachother. Where MBCS has a custom definition based on a Windows setting
> (which I don't, ATM, know how to modify).

I think MBCS alias could be changed via windows system settings dialogs.
But actually I have no desire to test it. As I said things could be more
tricky with CJK codepages.

> That i...

[John A Meinel (jameinel)]

So... I think the 'trivial' patch is:
=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py 2009-06-03 05:44:18 +0000
+++ bzrlib/diff.py 2009-06-10 03:14:17 +0000
@@ -171,6 +171,11 @@

         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 = old_filename.encode('mbcs')
         diffcmd = ['diff',
                    '--label', old_filename,
                    old_abspath,

[John A Meinel (jameinel)]

I associated a branch with the patch I wrote. Does someone want to actually try it out?

[Martin Packman (gz)]

Patch is wrong as it stands, due to a copy and paste error. With that fixed it basically works, but see the log attached for a bunch of other more subtle problems.

Some replies to John's ponderings earlier in the bug:

> It looks like CreateProcess should be part of the Kernel32.lib api, but
> it certainly isn't exported via ctypes.windll.kernel32.

You're forgetting to append the A/W here?

> Which I'm a little bit surprised at, given that I thought CreateProcess
> would always be a #define to CreateProcessW on unicode platforms, which
> means the char* would be interpreted as a wchar_t *...

Python (2 at least) builds with /D_MBCS not /D_UNICODE so that's the A version.

Anyway, certainly it's possible to hack around subprocess sucking, and I have in the past, but really this needs fixing upstream.

[Martin Packman (gz)]

Bah, I accidentally elided the first commit in that log which makes it look even stranger. Can't edit attachments, but it happened prior to the second `bzr mv`.

[John A Meinel (jameinel)]

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

Martin wrote:
> Patch is wrong as it stands, due to a copy and paste error. With that
> fixed it basically works, but see the log attached for a bunch of other
> more subtle problems.
>
> Some replies to John's ponderings earlier in the bug:
>
>> It looks like CreateProcess should be part of the Kernel32.lib api, but
>> it certainly isn't exported via ctypes.windll.kernel32.
>
> You're forgetting to append the A/W here?
>
>> Which I'm a little bit surprised at, given that I thought CreateProcess
>> would always be a #define to CreateProcessW on unicode platforms, which
>> means the char* would be interpreted as a wchar_t *...
>
> Python (2 at least) builds with /D_MBCS not /D_UNICODE so that's the A
> version.
>
> Anyway, certainly it's possible to hack around subprocess sucking, and I
> have in the past, but really this needs fixing upstream.
>
> ** Attachment added: "Console session with 1.16-external_diff_encoded-382709 branch"
> http://launchpadlibrarian.net/28620049/external_diff_encoded.log
>

Well, I was confused because for other functions I get automatic
completion. So I can do "ctypes.windll.kernel32.C<tab>" and it tells me
"CreateConsoleScreenBuffer"

However, if I manually type CreateProcessW I get:
  <_FuncPtr object at 0x0233DA08>

So it seems to be available, just not loaded by default.

(copy & paste bug might just be launchpad's display of the code, you
might try the branch I linked)

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

iEYEARECAAYFAkpMXPUACgkQJdeBCYSNAAPgnQCePi6e7R6alW/WJXwmRYl7Kksh
MxMAoIlf71JFd8hx2GsnUEcq4c/evyzU
=FT1S
-----END PGP SIGNATURE-----

[Martin Packman (gz)]

> Well, I was confused because for other functions I get automatic
> completion. So I can do "ctypes.windll.kernel32.C<tab>" and it tells me
> "CreateConsoleScreenBuffer"

Sounds like a side effect of whatever IDE you use, it probably just has a list of names it thinks might be in kernel32 somewhere.

> (copy & paste bug might just be launchpad's display of the code, you
> might try the branch I linked)

Sorry if I wasn't clear enough, the problem's in your branch. Pull the attached bundle to fix.