Merge lp:~bialix/bzr/diff-header-encoding into lp:bzr

[Alexander Belchenko]

Show unicode filenames in diff headers using terminal encoding.

This branch ensure filenames in diff header on windows always will be in mbcs encoding. My next step is to add new command-line option ``--path-encoding`` to some commands to control that encoding.

I would like to get some review on this work to ensure it's heading the right way. Also I would like to know: should I add --path-encoding option in this patch or it could be done as follow-on patch.

[John A Meinel (jameinel)]

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

Alexander Belchenko wrote:
> Alexander Belchenko has proposed merging lp:~bialix/bzr/diff-header-encoding into lp:bzr.
>
> Requested reviews:
> John A Meinel (jameinel)
> bzr-core (bzr-core)
> Related bugs:
> #382699 diff headers should contain non-ascii filenames in user_encoding, not in utf-8
> https://bugs.launchpad.net/bugs/382699
>
>
> This branch ensure filenames in diff header on windows always will be in mbcs encoding. My next step is to add new command-line option ``--path-encoding`` to some commands to control that encoding.
>
> I would like to get some review on this work to ensure it's heading the right way. Also I would like to know: should I add --path-encoding option in this patch or it could be done as follow-on patch.
>

Looks good to me.

 merge: approve

John
=:->

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

iEYEARECAAYFAkv9gWwACgkQJdeBCYSNAANcCgCgreM1BHLlYxRjw4wl/LikySLA
FVwAoL8H4Dq25YI1tjn9w4KoJCHOFPUW
=P51i
-----END PGP SIGNATURE-----

review: Approve

[Martin Pool (mbp)]

You can't assume paths will always be in utf-8 on unix. You should probably use the output encoding.

So the plan is that in the diff output stream the paths will always be mbcs and the body of the diff may be in some other encoding?

Is 'path_encoding' really the ideal name for this? To me it that name seems confusable with the filesystem path encoding.

review: Needs Fixing

[John A Meinel (jameinel)]

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

Martin Pool wrote:
> Review: Needs Fixing
> You can't assume paths will always be in utf-8 on unix. You should probably use the output encoding.
>

Prior to this, we always used utf-8, so I don't feel that this is a blocker.

> So the plan is that in the diff output stream the paths will always be mbcs and the body of the diff may be in some other encoding?
>
> Is 'path_encoding' really the ideal name for this? To me it that name seems confusable with the filesystem path encoding.

Well, for diff, it is the encoding of the path portion, but since we
don't transcode the file content, I don't know that we would want to use
a generic "output" encoding.

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

iEYEARECAAYFAkv9i7oACgkQJdeBCYSNAANN7wCggQbU+iq69/41viy2LpvoLigO
P5EAoMEZrPxtzLJdOYpm9voqzBPjzMdz
=m8IN
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

On 27 May 2010 07:00, John A Meinel <email address hidden> wrote:
>> Is 'path_encoding' really the ideal name for this?  To me it that name seems confusable with the filesystem path encoding.
>
> Well, for diff, it is the encoding of the path portion, but since we
> don't transcode the file content, I don't know that we would want to use
> a generic "output" encoding.

Within a diff we will have
- actual file content - not transcoded (but potentially could be in
future if we knew the file encoding and the output encoding)
- diff syntax (like +,-,line numbers) - mostly ascii so it's fairly
easy, but would need to be encoded for eg ucs-2
- filenames - presumably should be in the same encoding as the diff
syntax, but more likely to be non-ascii so more important to encode

Also a diff may occur within the context of a merge directive or log
output, and there too I think you would want all the non-file metadata
in the same encoding, wouldn't you?

--
Martin <http://launchpad.net/~mbp/>

[Robert Collins (lifeless)]

So AIUI the problem has been that we were using utf8 for path metadata, and mcbs on windows for other stuff.

Isn't the appropriate thing to do to pick 1 encoding - e.g. the most capable of the filesystem encoding and the terminal encoding (or perhaps least capable), and use that for:
 - paths
 - diff syntax
 - bzr chatter

And raw content for the file content itself (but perhaps escaped to not mangle terminals?)

review: Needs Information

[Martin Pool (mbp)]

On 27 May 2010 11:41, Robert Collins <email address hidden> wrote:
> Review: Needs Information
> So AIUI the problem has been that we were using utf8 for path metadata, and mcbs on windows for other stuff.
>
> Isn't the appropriate thing to do to pick 1 encoding - e.g. the most capable of the filesystem encoding and the terminal encoding (or perhaps least capable), and use that for:
>  - paths
>  - diff syntax
>  - bzr chatter
>
> And raw content for the file content itself (but perhaps escaped to not mangle terminals?)

I was pointing to having one encoding for all of these too, except
that I don't think that's the right heuristic to pick it. afaict if
it's going to a terminal we should use the terminal encoding,
otherwise we should use the output encoding.

--
Martin <http://launchpad.net/~mbp/>

[Robert Collins (lifeless)]

Alexander, will that work for you?

[Alexander Belchenko (bialix)]

Robert Collins пишет:
> Review: Needs Information
> So AIUI the problem has been that we were using utf8 for path metadata, and mcbs on windows for other stuff.

What is "other stuff" here? bzr diff produce "binary output", i.e. it
does not try to encode the diff itself, but only (generated by bzr
itself) header information like

=== modified file "foo.txt"
--- foo.txt
+++ foo.txt

If this part is not called header, then please forgive me.

My patch only touches that header.

I don't think we can transcoding file content until we have some sort of
encoding properties for files.

> Isn't the appropriate thing to do to pick 1 encoding - e.g. the most capable of the filesystem encoding and the terminal encoding (or perhaps least capable), and use that for:
> - paths
> - diff syntax
> - bzr chatter
>
> And raw content for the file content itself (but perhaps escaped to not mangle terminals?)

The problem is there is no "only one right choice" on Windows.

If user want to see diff in the terminal then it will be better to use
terminal encoding.

If user want to save diff into file and use it as patch (with GNU patch)
then the best we can do is to use mbcs (or user_encoding).

The problem is bzr can't detect user intention.

[Alexander Belchenko (bialix)]

Robert Collins пишет:
> Alexander, will that work for you?

I don't know. I'm happy with qdiff which does the right thing. I'm
working on this patch because Russian people often asking me about this
problem.

So I've sent RFC mail to ru_bzr ML to collect opinions from other
people. Because in main bzr ML there is still no feedback from non-ascii
people :-/

--
All the dude wanted was his rug back

[Martin Pool (mbp)]

On 27 May 2010 21:31, Alexander Belchenko <email address hidden> wrote:
> Robert Collins пишет:
>> Review: Needs Information
>> So AIUI the problem has been that we were using utf8 for path metadata, and mcbs on windows for other stuff.
>
> What is "other stuff" here? bzr diff produce "binary output", i.e. it
> does not try to encode the diff itself, but only (generated by bzr
> itself) header information like
>
> === modified file "foo.txt"
> --- foo.txt
> +++ foo.txt
>
> If this part is not called header, then please forgive me.
>
> My patch only touches that header.
>
> I don't think we can transcoding file content until we have some sort of
> encoding properties for files.

'other stuff' includes the '@@' '===' '---' and the line numbers, as I
discussed above. They're not very interesting to encode but they do
have an encoding. I think they should be the same as the paths.

>
>> Isn't the appropriate thing to do to pick 1 encoding - e.g. the most capable of the filesystem encoding and the terminal encoding (or perhaps least capable), and use that for:
>>  - paths
>>  - diff syntax
>>  - bzr chatter
>>
>> And raw content for the file content itself (but perhaps escaped to not mangle terminals?)
>
> The problem is there is no "only one right choice" on Windows.
>
> If user want to see diff in the terminal then it will be better to use
> terminal encoding.
>
> If user want to save diff into file and use it as patch (with GNU patch)
> then the best we can do is to use mbcs (or user_encoding).
>
> The problem is bzr can't detect user intention.

We can't tell if stdout is redirected to a file? On Unix we can tell
with isatty().

I think these headings should be in output_encoding, which already has
special logic to decide whether to use the terminal encoding or
something else.

--
Martin <http://launchpad.net/~mbp/>

[Alexander Belchenko (bialix)]

Martin Pool пишет:
> On 27 May 2010 21:31, Alexander Belchenko <email address hidden> wrote:
>> Robert Collins пишет:
>>> Review: Needs Information
>>> So AIUI the problem has been that we were using utf8 for path metadata, and mcbs on windows for other stuff.
>> What is "other stuff" here? bzr diff produce "binary output", i.e. it
>> does not try to encode the diff itself, but only (generated by bzr
>> itself) header information like
>>
>> === modified file "foo.txt"
>> --- foo.txt
>> +++ foo.txt
>>
>> If this part is not called header, then please forgive me.
>>
>> My patch only touches that header.
>>
>> I don't think we can transcoding file content until we have some sort of
>> encoding properties for files.
>
> 'other stuff' includes the '@@' '===' '---' and the line numbers, as I
> discussed above. They're not very interesting to encode but they do
> have an encoding. I think they should be the same as the paths.

As I can see '@@' is always ascii and they're generated by difflib
itself, right? I'd better do not touch them without very important reason.

>>> Isn't the appropriate thing to do to pick 1 encoding - e.g. the most capable of the filesystem encoding and the terminal encoding (or perhaps least capable), and use that for:
>>> - paths
>>> - diff syntax
>>> - bzr chatter
>>>
>>> And raw content for the file content itself (but perhaps escaped to not mangle terminals?)
>> The problem is there is no "only one right choice" on Windows.
>>
>> If user want to see diff in the terminal then it will be better to use
>> terminal encoding.
>>
>> If user want to save diff into file and use it as patch (with GNU patch)
>> then the best we can do is to use mbcs (or user_encoding).
>>
>> The problem is bzr can't detect user intention.
>
> We can't tell if stdout is redirected to a file? On Unix we can tell
> with isatty().

On Windows it's not reliable as I can see:

C:\Temp\8>python -c "import sys; print >>sys.stderr, sys.stdout.isatty()"
True

C:\Temp\8>python -c "import sys; print >>sys.stderr,
sys.stdout.isatty()"|less
False

C:\Temp\8>python -c "import sys; print >>sys.stderr,
sys.stdout.isatty()" > NUL
True

C:\Temp\8>python -c "import sys; print >>sys.stderr,
sys.stdout.isatty()" > temp.txt
False

> I think these headings should be in output_encoding, which already has
> special logic to decide whether to use the terminal encoding or
> something else.

Then, I think I should switch to terminal_encoding which is the right
thing for main use case: see the diff on the screen. And provide
--path-encoding for other cases.

[John A Meinel (jameinel)]

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

Alexander Belchenko wrote:
> Martin Pool пишет:
>> On 27 May 2010 21:31, Alexander Belchenko <email address hidden> wrote:
>>> Robert Collins пишет:
>>>> Review: Needs Information
>>>> So AIUI the problem has been that we were using utf8 for path metadata, and mcbs on windows for other stuff.
>>> What is "other stuff" here? bzr diff produce "binary output", i.e. it
>>> does not try to encode the diff itself, but only (generated by bzr
>>> itself) header information like
>>>
>>> === modified file "foo.txt"
>>> --- foo.txt
>>> +++ foo.txt
>>>
>>> If this part is not called header, then please forgive me.
>>>
>>> My patch only touches that header.
>>>
>>> I don't think we can transcoding file content until we have some sort of
>>> encoding properties for files.
>> 'other stuff' includes the '@@' '===' '---' and the line numbers, as I
>> discussed above. They're not very interesting to encode but they do
>> have an encoding. I think they should be the same as the paths.
>
> As I can see '@@' is always ascii and they're generated by difflib
> itself, right? I'd better do not touch them without very important reason.
>

Martin's point is that if you selected "--encoding=utf-16" then you
should have "\0@\0@\0 \0m\0o\0d\0i...\0f\0o\0o\0.\0t\0x\0t" in the
output, rather than a mix of 8-bit ascii followed by 16-bit unicode chars.

Yes, it turns out that most (all?) 8-bit encodings use ascii as a
subset, but that certainly isn't true for 16-bit encodings.

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

iEYEARECAAYFAkv/34AACgkQJdeBCYSNAAOR8QCgzXHT1MsCrZLopOaQpHGKmJNR
fI8AnRMBiSrbj1wKt8GzRiwau/McmriV
=wzCu
-----END PGP SIGNATURE-----

[Alexander Belchenko (bialix)]

John A Meinel пишет:
> Alexander Belchenko wrote:
>> Martin Pool пишет:
>>> On 27 May 2010 21:31, Alexander Belchenko <email address hidden> wrote:
>>>> Robert Collins пишет:
>>>>> Review: Needs Information
>>>>> So AIUI the problem has been that we were using utf8 for path metadata, and mcbs on windows for other stuff.
>>>> What is "other stuff" here? bzr diff produce "binary output", i.e. it
>>>> does not try to encode the diff itself, but only (generated by bzr
>>>> itself) header information like
>>>>
>>>> === modified file "foo.txt"
>>>> --- foo.txt
>>>> +++ foo.txt
>>>>
>>>> If this part is not called header, then please forgive me.
>>>>
>>>> My patch only touches that header.
>>>>
>>>> I don't think we can transcoding file content until we have some sort of
>>>> encoding properties for files.
>>> 'other stuff' includes the '@@' '===' '---' and the line numbers, as I
>>> discussed above. They're not very interesting to encode but they do
>>> have an encoding. I think they should be the same as the paths.
>> As I can see '@@' is always ascii and they're generated by difflib
>> itself, right? I'd better do not touch them without very important reason.
>>
>
> Martin's point is that if you selected "--encoding=utf-16" then you
> should have "\0@\0@\0 \0m\0o\0d\0i...\0f\0o\0o\0.\0t\0x\0t" in the
> output, rather than a mix of 8-bit ascii followed by 16-bit unicode chars.
>
> Yes, it turns out that most (all?) 8-bit encodings use ascii as a
> subset, but that certainly isn't true for 16-bit encodings.

Hmmm. Good point. But I think it won't fly anyway. What we should do
with file content? It still will be just 8-bit binary stream?

Or this makes sense only if the file content itself is utf-16? Right?

[Alexander Belchenko (bialix)]

Alexander Belchenko пишет:
> John A Meinel пишет:
>> Alexander Belchenko wrote:
>>> Martin Pool пишет:
>>>> On 27 May 2010 21:31, Alexander Belchenko <email address hidden> wrote:
>>>>> Robert Collins пишет:
>>>>>> Review: Needs Information
>>>>>> So AIUI the problem has been that we were using utf8 for path metadata, and mcbs on windows for other stuff.
>>>>> What is "other stuff" here? bzr diff produce "binary output", i.e. it
>>>>> does not try to encode the diff itself, but only (generated by bzr
>>>>> itself) header information like
>>>>>
>>>>> === modified file "foo.txt"
>>>>> --- foo.txt
>>>>> +++ foo.txt
>>>>>
>>>>> If this part is not called header, then please forgive me.
>>>>>
>>>>> My patch only touches that header.
>>>>>
>>>>> I don't think we can transcoding file content until we have some sort of
>>>>> encoding properties for files.
>>>> 'other stuff' includes the '@@' '===' '---' and the line numbers, as I
>>>> discussed above. They're not very interesting to encode but they do
>>>> have an encoding. I think they should be the same as the paths.
>>> As I can see '@@' is always ascii and they're generated by difflib
>>> itself, right? I'd better do not touch them without very important reason.
>>>
>> Martin's point is that if you selected "--encoding=utf-16" then you
>> should have "\0@\0@\0 \0m\0o\0d\0i...\0f\0o\0o\0.\0t\0x\0t" in the
>> output, rather than a mix of 8-bit ascii followed by 16-bit unicode chars.
>>
>> Yes, it turns out that most (all?) 8-bit encodings use ascii as a
>> subset, but that certainly isn't true for 16-bit encodings.
>
> Hmmm. Good point. But I think it won't fly anyway. What we should do
> with file content? It still will be just 8-bit binary stream?
>
> Or this makes sense only if the file content itself is utf-16? Right?

It won't work with external diff. I think supporting utf-16 is separate
issue.

[John A Meinel (jameinel)]

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

...
>>> Martin's point is that if you selected "--encoding=utf-16" then you
>>> should have "\0@\0@\0 \0m\0o\0d\0i...\0f\0o\0o\0.\0t\0x\0t" in the
>>> output, rather than a mix of 8-bit ascii followed by 16-bit unicode chars.
>>>
>>> Yes, it turns out that most (all?) 8-bit encodings use ascii as a
>>> subset, but that certainly isn't true for 16-bit encodings.
>> Hmmm. Good point. But I think it won't fly anyway. What we should do
>> with file content? It still will be just 8-bit binary stream?
>>
>> Or this makes sense only if the file content itself is utf-16? Right?
>
> It won't work with external diff. I think supporting utf-16 is separate
> issue.

Conceptually if someone had utf-16 content, they would then want utf-16
diff + utf-16 headers.

Martin's point is basically about this. I don't know what (if any) 8-bit
encodings don't use the ascii subset, but that might also be an issue.

Short term, this is probably better than it was, As long as it gives us
a way to transition to even better.

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

iEYEARECAAYFAkv/6vwACgkQJdeBCYSNAAOorQCcCllgJ6rZZb8M71F+NAS2xjTU
Ge8AoI2NvGUb0kXpAqliAMm+NzAjDE8N
=s/WC
-----END PGP SIGNATURE-----

[Martin Packman (gz)]

Okay, so, there are no perfect answers here, but this is a clear improvement and should be landed.

The problem with diff is that it's half user-readable text format, and half machine-readable patch input. Using the locale byte-encoding on Windows means throwing away information, but the without storing the encodings of the actual files we're already stuffed on correctness anyway (and as Alexander mentions, that means there's no way of generating a sensible UTF-16 diff file, even if such things were taken as valid input anywhere).

The terminal encoding issue is a worthwhile point, and a future enhancement could be to detect where we have a terminal handle rather than a pipe (which might still end up on the terminal, but again, we're stuffed on that), and use the unicode console apis.

+ else:
+ return 'utf8'

As mentioned by Martin Pool, this should probably instead be something like:

+ return locale.getpreferredencoding()

That would actually be fine on win32 too, but we're happy with the lossy downcoding the Python mbcs codec does, right?

review: Approve

[Alexander Belchenko (bialix)]

OK, I've changed default encoding from mbcs to terminal encoding, thus optimizing my patch to use case: "show me a diff on the screen" rather than provide compatibility with GNU diff/patch tools. If user need to save diff to file then user need to change terminal encoding with `chcp` command.

Also I've added a NEWS.

vila and mgz persuaded me to land this patch in its current form and add --path-encoding option later.

[Alexander Belchenko (bialix)]

PING.

[Andrew Bennetts (spiv)]

Thanks for the ping. I've poked Martin (Pool) in person; he's promised to take another look at this.

[Alexander Belchenko (bialix)]

Andrew Bennetts пишет:
> Thanks for the ping. I've poked Martin (Pool) in person; he's promised to take another look at this.

Thank you, mr. lotzman!

[Vincent Ladeuil (vila)]

@poolie: ping

[Martin Pool (mbp)]

Sorry for the lag.

I agree this is probably a step forward except that you really should not just default to utf8 on non-Windows, as Martin[gz] and I said previously in this review. You could just default to output_encoding. But in fact, why not always default to output_encoding, and then on win32 we should arrange for that to be either the terminal encoding or something else, as appropriate.

review: Needs Fixing

[Alexander Belchenko (bialix)]

What do you mean by "output_encoding"?

[Martin Pool (mbp)]

> What do you mean by "output_encoding"?

I was confused by the fact that the value returned by get_terminal_encoding is often referred to as output_encoding. Possibly we should eventually have that separated in the ui output stream.

In short what I'd recommend for now is that you change

+def get_diff_header_encoding():
123 + if sys.platform == 'win32':
124 + return get_terminal_encoding()
125 + else:
126 + return 'utf8'
127 +
128 +

to simply return get_terminal_encoding: that seems at least as likely to be correct on unix? On my machine it returns 'UTF-8'.

[Vincent Ladeuil (vila)]

@Sacha: I think Martin's request is just a tweak, if you agree with that, please land once it's fixed.
I'll mark this proposal approved, make it wip if needed.

=== modified file 'NEWS'
--- NEWS	2010-06-17 02:06:20 +0000
+++ NEWS	2010-06-17 08:55:41 +0000
@@ -66,6 +66,12 @@
   which previously caused "SyntaxError: No command for line".
   (Martin Pool)
 
+* Show unicode filenames in diff headers using terminal encoding. 
+  (Alexander Belchenko, Bug #382699)
+  NOTE for Windows users: If user need to save diff to file then user need to
+  change encoding of the terminal to ANSI encoding with command ``chcp XXX``
+  (e.g. ``chcp 1251`` for Russian Windows).
+
 Improvements
 ************
 

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-06-07 02:38:04 +0000
+++ bzrlib/builtins.py	2010-06-17 08:55:41 +0000
@@ -1970,11 +1970,15 @@
          old_branch, new_branch,
          specific_files, extra_trees) = get_trees_and_branches_to_diff_locked(
             file_list, revision, old, new, self.add_cleanup, apply_view=True)
+        # GNU diff on Windows uses ANSI encoding for filenames
+        path_encoding = osutils.get_diff_header_encoding()
         return show_diff_trees(old_tree, new_tree, sys.stdout,
                                specific_files=specific_files,
                                external_diff_options=diff_options,
                                old_label=old_label, new_label=new_label,
-                               extra_trees=extra_trees, using=using,
+                               extra_trees=extra_trees,
+                               path_encoding=path_encoding,
+                               using=using,
                                format_cls=format)
 
 
@@ -3892,8 +3896,10 @@
     def _do_preview(self, merger):
         from bzrlib.diff import show_diff_trees
         result_tree = self._get_preview(merger)
+        path_encoding = osutils.get_diff_header_encoding()
         show_diff_trees(merger.this_tree, result_tree, self.outf,
-                        old_label='', new_label='')
+                        old_label='', new_label='',
+                        path_encoding=path_encoding)
 
     def _do_merge(self, merger, change_reporter, allow_pending, verified):
         merger.change_reporter = change_reporter

=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py	2010-05-25 17:27:52 +0000
+++ bzrlib/diff.py	2010-06-17 08:55:41 +0000
@@ -99,8 +99,8 @@
     if sequence_matcher is None:
         sequence_matcher = patiencediff.PatienceSequenceMatcher
     ud = patiencediff.unified_diff(oldlines, newlines,
-                      fromfile=old_filename.encode(path_encoding),
-                      tofile=new_filename.encode(path_encoding),
+                      fromfile=old_filename.encode(path_encoding, 'replace'),
+                      tofile=new_filename.encode(path_encoding, 'replace'),
                       sequencematcher=sequence_matcher)
 
     ud = list(ud)
@@ -713,11 +713,11 @@
             from_text = _get_text(self.old_tree, from_file_id, from_path)
             to_text = _get_text(self.new_tree, to_file_id, to_path)
             self.text_differ(from_label, from_text, to_label, to_text,
-                             self.to_file)
+                             self.to_file, path_encoding=self.path_encoding)
         except errors.BinaryFile:
             self.to_file.write(
                   ("Binary files %s and %s differ\n" %
-                  (from_label, to_label)).encode(self.path_encoding))
+                  (from_label, to_label)).encode(self.path_encoding,'replace'))
         return self.CHANGED
 
 
@@ -920,7 +920,10 @@
             extra_factories = []
         if external_diff_options:
             opts = external_diff_options.split()
-            def diff_file(olab, olines, nlab, nlines, to_file):
+            def diff_file(olab, olines, nlab, nlines, to_file, path_encoding=None):
+                """:param path_encoding: not used but required
+                        to match the signature of internal_diff.
+                """
                 external_diff(olab, olines, nlab, nlines, to_file, opts)
         else:
             diff_file = internal_diff

=== modified file 'bzrlib/log.py'
--- bzrlib/log.py	2010-06-08 09:50:27 +0000
+++ bzrlib/log.py	2010-06-17 08:55:41 +0000
@@ -70,6 +70,7 @@
     diff,
     errors,
     foreign,
+    osutils,
     repository as _mod_repository,
     revision as _mod_revision,
     revisionspec,
@@ -432,8 +433,9 @@
         else:
             specific_files = None
         s = StringIO()
+        path_encoding = osutils.get_diff_header_encoding()
         diff.show_diff_trees(tree_1, tree_2, s, specific_files, old_label='',
-            new_label='')
+            new_label='', path_encoding=path_encoding)
         return s.getvalue()
 
     def _create_log_revision_iterator(self):

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2010-06-07 01:13:05 +0000
+++ bzrlib/osutils.py	2010-06-17 08:55:41 +0000
@@ -1952,6 +1952,10 @@
     return user_encoding
 
 
+def get_diff_header_encoding():
+    return get_terminal_encoding()
+
+
 def get_host_name():
     """Return the current unicode host name.
 

=== modified file 'bzrlib/shelf_ui.py'
--- bzrlib/shelf_ui.py	2010-01-15 05:33:28 +0000
+++ bzrlib/shelf_ui.py	2010-06-17 08:55:41 +0000
@@ -241,7 +241,8 @@
             new_tree = self.work_tree
         old_path = old_tree.id2path(file_id)
         new_path = new_tree.id2path(file_id)
-        text_differ = diff.DiffText(old_tree, new_tree, diff_file)
+        text_differ = diff.DiffText(old_tree, new_tree, diff_file,
+            path_encoding=osutils.get_terminal_encoding())
         patch = text_differ.diff(file_id, old_path, new_path, 'file', 'file')
         diff_file.seek(0)
         return patches.parse_patch(diff_file)
@@ -493,7 +494,9 @@
         new_tree = tt.get_preview_tree()
         if self.write_diff_to is None:
             self.write_diff_to = ui.ui_factory.make_output_stream()
-        diff.show_diff_trees(merger.this_tree, new_tree, self.write_diff_to)
+        path_encoding = osutils.get_diff_header_encoding()
+        diff.show_diff_trees(merger.this_tree, new_tree, self.write_diff_to,
+            path_encoding=path_encoding)
         tt.finalize()
 
     def show_changes(self, merger):

=== modified file 'bzrlib/tests/test_diff.py'
--- bzrlib/tests/test_diff.py	2010-05-20 08:08:20 +0000
+++ bzrlib/tests/test_diff.py	2010-06-17 08:55:41 +0000
@@ -34,6 +34,7 @@
     )
 from bzrlib.symbol_versioning import deprecated_in
 from bzrlib.tests import features
+from bzrlib.tests.blackbox.test_diff import subst_dates
 
 
 class _AttribFeature(tests.Feature):
@@ -521,7 +522,6 @@
         self.assertNotContainsRe(d, r"file 'e'")
         self.assertNotContainsRe(d, r"file 'f'")
 
-
     def test_binary_unicode_filenames(self):
         """Test that contents of files are *not* encoded in UTF-8 when there
         is a binary file in the diff.
@@ -580,6 +580,49 @@
         self.assertContainsRe(d, "=== modified file 'mod_%s'"%autf8)
         self.assertContainsRe(d, "=== removed file 'del_%s'"%autf8)
 
+    def test_unicode_filename_path_encoding(self):
+        """Test for bug #382699: unicode filenames on Windows should be shown
+        in user encoding.
+        """
+        self.requireFeature(tests.UnicodeFilenameFeature)
+        # The word 'test' in Russian
+        _russian_test = u'\u0422\u0435\u0441\u0442'
+        directory = _russian_test + u'/'
+        test_txt = _russian_test + u'.txt'
+        u1234 = u'\u1234.txt'
+
+        tree = self.make_branch_and_tree('.')
+        self.build_tree_contents([
+            (test_txt, 'foo\n'),
+            (u1234, 'foo\n'),
+            (directory, None),
+            ])
+        tree.add([test_txt, u1234, directory])
+
+        sio = StringIO()
+        diff.show_diff_trees(tree.basis_tree(), tree, sio,
+            path_encoding='cp1251')
+
+        output = subst_dates(sio.getvalue())
+        shouldbe = ('''\
+=== added directory '%(directory)s'
+=== added file '%(test_txt)s'
+--- a/%(test_txt)s\tYYYY-MM-DD HH:MM:SS +ZZZZ
++++ b/%(test_txt)s\tYYYY-MM-DD HH:MM:SS +ZZZZ
+@@ -0,0 +1,1 @@
++foo
+
+=== added file '?.txt'
+--- a/?.txt\tYYYY-MM-DD HH:MM:SS +ZZZZ
++++ b/?.txt\tYYYY-MM-DD HH:MM:SS +ZZZZ
+@@ -0,0 +1,1 @@
++foo
+
+''' % {'directory': _russian_test.encode('cp1251'),
+       'test_txt': test_txt.encode('cp1251'),
+      })
+        self.assertEqualDiff(output, shouldbe)
+
 
 class DiffWasIs(diff.DiffPath):