Merge lp:~parthm/bzr/563646-commit-unicode-message into lp:bzr

[Parth Malwankar]

better unicode handling when commit message (-m) is same as unicode filename. (parthm, #563646)

=== Fixes Bug #563646 ===

When a file has the same name as the commit message (-m), bzr displays a warning. When the file name is unicode, this was causing ui.text.show_warning to fail with UnicodeEncodeError.
ui.text.show_warning is updated to msg.encode in case the message is unicode. This now works as expected.
Test case is added for the above scenario.

[abc]% ~/src/bzr.dev/563646-commit-unicode-message/bzr --no-plugins ci -m €
bzr: warning: The commit message is a file name: "€".
(use --file "€" to take commit message from that file)
Committing to: /home/parthm/tmp/abc/
added d
Committed revision 7.
[abc]%

[Robert Collins (lifeless)]

Great

review: Approve

[Robert Collins (lifeless)]

Oh, the vertical white space on line 40 of your diff is a little ugly.

[Parth Malwankar (parthm)]

> Oh, the vertical white space on line 40 of your diff is a little ugly.

Removed blank line.

[Martin Packman (gz)]

What should happen if the filename is not in the console output codepage?

Running the test currently I get this error:

Traceback (most recent call last):
  File "C:\Python24\lib\site-packages\testtools\runtest.py", line 128, in _run_user
    return fn(*args)
  File "C:\Python24\lib\site-packages\testtools\testcase.py", line 368, in _run_test_method
    testMethod()
  File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\tests\blackbox\test_commit.py", line 118, in test_unicode_commit_message
    out, err = self.run_bzr(['commit', '-m', file_name])
  File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\tests\__init__.py", line 1844, in run_bzr
    working_dir=working_dir,
  File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\tests\__init__.py", line 1743, in _run_bzr_autosplit
    encoding=encoding, stdin=stdin, working_dir=working_dir,
  File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\tests\__init__.py", line 1777, in _run_bzr_core
    args)
  File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\tests\__init__.py", line 2082, in apply_redirected
    return a_callable(*args, **kwargs)
  File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\commands.py", line 1219, in run_bzr_catch_user_errors
    return run_bzr(argv)
  File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\commands.py", line 1111, in run_bzr
    ret = run(*run_argv)
  File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\commands.py", line 685, in run_argv_aliases
    return self.run(**all_cmd_args)
  File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\commands.py", line 700, in run
    return self._operation.run_simple(*args, **kwargs)
  File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\cleanup.py", line 121, in run_simple
    return _do_with_cleanups(
  File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\cleanup.py", line 156, in _do_with_cleanups
    result = func(*args, **kwargs)
  File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\builtins.py", line 3143, in run
    ui.ui_factory.show_warning(warning_msg)
  File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\ui\text.py", line 238, in show_warning
    msg = msg.encode(te)
UnicodeEncodeError: 'cp932' codec can't encode character u'\u20ac' in position 36: illegal multibyte sequence

review: Needs Fixing

[Robert Collins (lifeless)]

On Wed, Apr 21, 2010 at 2:45 AM, Martin [gz] <email address hidden> wrote:

> Review: Needs Fixing
> What should happen if the filename is not in the console output codepage?
>

So, the filename is unicode in this change, so it will get encoded in the
console codepage.

however, the error you see is more interesting:

> ... File "C:\bzr\bzr\563646-commit-unicode-message\bzrlib\ui\text.py", line
> 238, in show_warning
> msg = msg.encode(te)
> UnicodeEncodeError: 'cp932' codec can't encode character u'\u20ac' in
> position 36: illegal multibyte sequence

My copy of text.py does not do msg.encode(te) - do you have an unmerged
patch you are using ?

[Parth Malwankar (parthm)]

On Tue, Apr 20, 2010 at 10:15 PM, Martin [gz] <email address hidden> wrote:
> Review: Needs Fixing
> What should happen if the filename is not in the console output codepage?
>
> Running the test currently I get this error:
>

I am able to run the tests on WinXP without any problems.
Is there any specific setting on your windows system that I need to
do to reproduce this?

I do have a msg.encode(te) in my branch.

    def show_warning(self, msg):
        self.clear_term()
        if isinstance(msg, unicode):
            te = osutils.get_terminal_encoding()
            msg = msg.encode(te)
        self.stderr.write("bzr: warning: %s\n" % msg)

[Martin Packman (gz)]

I'm guessing the euro sign is in your codepage. Try a character that's not in any of them, Alexander likes u"\u1234" for instance.

[Parth Malwankar (parthm)]

> I'm guessing the euro sign is in your codepage. Try a character that's not in
> any of them, Alexander likes u"\u1234" for instance.

I am able to reproduce this error now. We have a path that can't
be shown in terminal encoding.

My initial though is to use either "msg = msg.encode(te. 'replace')"
in show_warning instead of msg.encode(te).

Or to handle UnicodeEncodeError one level up
and show a general warning w/o the filename e.g.

Instead of:

bzr: warning: The commit message is a file name: "€".
(use --file "€" to take commit message from that file)

we show:

bzr: warning: The commit message is a file name.
(use --file to take commit message from that file)

The latter may be more acceptable rather than using
'replace'. In the case of 'replace' user will end
up with a message "... commit message is file name: "?".
(use --file "?" ...)" which may cause more confusion.

Thoughts? Any other ideas?

[Parth Malwankar (parthm)]

> Or to handle UnicodeEncodeError one level up
> and show a general warning w/o the filename e.g.
>

Just to be clear, what I mean is that we show
the complete message with the filename in the
usual case but in case of a UnicodeEncodeError
doing this we show the message omitting the
file name. The thing I don't like about this
option is that we would be doing special handling
one level up.

[Alexander Belchenko (bialix)]

Parth Malwankar пишет:
>> I'm guessing the euro sign is in your codepage. Try a character that's not in
>> any of them, Alexander likes u"\u1234" for instance.
>
> I am able to reproduce this error now. We have a path that can't
> be shown in terminal encoding.
>
> My initial though is to use either "msg = msg.encode(te. 'replace')"
> in show_warning instead of msg.encode(te).
>
> Or to handle UnicodeEncodeError one level up
> and show a general warning w/o the filename e.g.
>
> Instead of:
>
> bzr: warning: The commit message is a file name: "€".
> (use --file "€" to take commit message from that file)
>
> we show:
>
> bzr: warning: The commit message is a file name.
> (use --file to take commit message from that file)

+1

--
All the dude wanted was his rug back

[John A Meinel (jameinel)]

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

Parth Malwankar wrote:
>> I'm guessing the euro sign is in your codepage. Try a character that's not in
>> any of them, Alexander likes u"\u1234" for instance.
>
> I am able to reproduce this error now. We have a path that can't
> be shown in terminal encoding.
>
> My initial though is to use either "msg = msg.encode(te. 'replace')"
> in show_warning instead of msg.encode(te).
>
> Or to handle UnicodeEncodeError one level up
> and show a general warning w/o the filename e.g.
>
> Instead of:
>
> bzr: warning: The commit message is a file name: "€".
> (use --file "€" to take commit message from that file)
>
> we show:
>
> bzr: warning: The commit message is a file name.
> (use --file to take commit message from that file)
>
> The latter may be more acceptable rather than using
> 'replace'. In the case of 'replace' user will end
> up with a message "... commit message is file name: "?".
> (use --file "?" ...)" which may cause more confusion.
>
>
> Thoughts? Any other ideas?
>

I prefer using 'replace'. Mostly because it is very rare to have only
unprintable characters. So you are more likely to get:

bzr: warning: The commit message is a file name: "foo?.txt".

Rather than just
bzr: warning: The commit message is a file name: "?".

And having *some* context is better than having no context.

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

iEYEARECAAYFAkvQWZcACgkQJdeBCYSNAAPAXwCgyw9N9LJADhwoIgTz04JHoLc+
CqIAoNMq2NO3yLf5G2S/ez76JSiSsmkZ
=GD/U
-----END PGP SIGNATURE-----

[Parth Malwankar (parthm)]

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Parth Malwankar wrote:
> >> I'm guessing the euro sign is in your codepage. Try a character that's not
> in
> >> any of them, Alexander likes u"\u1234" for instance.
> >
> > I am able to reproduce this error now. We have a path that can't
> > be shown in terminal encoding.
> >
> > My initial though is to use either "msg = msg.encode(te. 'replace')"
> > in show_warning instead of msg.encode(te).
> >
> > Or to handle UnicodeEncodeError one level up
> > and show a general warning w/o the filename e.g.
> >
> > Instead of:
> >
> > bzr: warning: The commit message is a file name: "€".
> > (use --file "€" to take commit message from that file)
> >
> > we show:
> >
> > bzr: warning: The commit message is a file name.
> > (use --file to take commit message from that file)
> >
> > The latter may be more acceptable rather than using
> > 'replace'. In the case of 'replace' user will end
> > up with a message "... commit message is file name: "?".
> > (use --file "?" ...)" which may cause more confusion.
> >
> >
> > Thoughts? Any other ideas?
> >
>
> I prefer using 'replace'. Mostly because it is very rare to have only
> unprintable characters. So you are more likely to get:
>
>
> bzr: warning: The commit message is a file name: "foo?.txt".
>
> Rather than just
> bzr: warning: The commit message is a file name: "?".
>
> And having *some* context is better than having no context.
>

Sounds fine to me. This also keeps the code
cleaner.

Only thing is that this is for show_warning
so this is how all warnings would behave.
That should be ok as if the characters are
replaced they can't be displayed in the terminal
encoding anyway. Thats better than show_warning
crashing.

> John
> =:->
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (Cygwin)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkvQWZcACgkQJdeBCYSNAAPAXwCgyw9N9LJADhwoIgTz04JHoLc+
> CqIAoNMq2NO3yLf5G2S/ez76JSiSsmkZ
> =GD/U
> -----END PGP SIGNATURE-----

[Martin Packman (gz)]

Test no longer fails here. I don't like sprouting apis that don't know if they should be taking bytes or unicode and try to muddle through inside the function, but I guess this function already has the problem so the change isn't making anything worse.

review: Abstain

[Parth Malwankar (parthm)]

@Martin Pool: There is a review request by you pending on this mp (2010-04-22). This mp is already in the "ready to land" queue. If this looks ok to you or you want to abstain, I can go ahead and land it.

[Martin Pool (mbp)]

That looks ok to me. It would be nice to directly test the ui handling of unicode, but you can land without it.

Thanks for all the patches!

review: Approve

=== modified file 'NEWS'
--- NEWS	2010-05-05 14:44:53 +0000
+++ NEWS	2010-05-05 16:03:27 +0000
@@ -91,6 +91,11 @@
 * Unicode characters in aliases are now handled correctly and do not cause
   UnicodeEncodeError exception. (Parth Malwankar, #529930)
 
+* Unicode commit messages that are the same as a file name no longer cause
+  UnicodeEncodeError. ``ui.text.show_warning`` now handles unicode
+  messages.
+  (Parth Malwankar, #563646)
+
 Improvements
 ************
 

=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2010-04-07 22:22:27 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2010-05-05 16:03:27 +0000
@@ -18,6 +18,7 @@
 """Tests for the commit CLI of bzr."""
 
 import os
+import re
 import sys
 
 from bzrlib import (
@@ -107,6 +108,40 @@
                               'modified hello\.txt\n'
                               'Committed revision 2\.\n$')
 
+    def test_unicode_commit_message_is_filename(self):
+        """Unicode commit message same as a filename (Bug #563646).
+        """
+        file_name = u'\N{euro sign}'
+        self.run_bzr(['init'])
+        open(file_name, 'w').write('hello world')
+        self.run_bzr(['add'])
+        out, err = self.run_bzr(['commit', '-m', file_name])
+        reflags = re.MULTILINE|re.DOTALL|re.UNICODE
+        te = osutils.get_terminal_encoding()
+        self.assertContainsRe(err.decode(te),
+            u'The commit message is a file name:',
+            flags=reflags)
+
+        # Run same test with a filename that causes encode
+        # error for the terminal encoding. We do this
+        # by forcing terminal encoding of ascii for
+        # osutils.get_terminal_encoding which is used
+        # by ui.text.show_warning
+        default_get_terminal_enc = osutils.get_terminal_encoding
+        try:
+            osutils.get_terminal_encoding = lambda: 'ascii'
+            file_name = u'foo\u1234'
+            open(file_name, 'w').write('hello world')
+            self.run_bzr(['add'])
+            out, err = self.run_bzr(['commit', '-m', file_name])
+            reflags = re.MULTILINE|re.DOTALL|re.UNICODE
+            te = osutils.get_terminal_encoding()
+            self.assertContainsRe(err.decode(te, 'replace'),
+                u'The commit message is a file name:',
+                flags=reflags)
+        finally:
+            osutils.get_terminal_encoding = default_get_terminal_enc
+
     def test_warn_about_forgotten_commit_message(self):
         """Test that the lack of -m parameter is caught"""
         wt = self.make_branch_and_tree('.')

=== modified file 'bzrlib/ui/text.py'
--- bzrlib/ui/text.py	2010-03-25 07:34:15 +0000
+++ bzrlib/ui/text.py	2010-05-05 16:03:27 +0000
@@ -233,6 +233,9 @@
 
     def show_warning(self, msg):
         self.clear_term()
+        if isinstance(msg, unicode):
+            te = osutils.get_terminal_encoding()
+            msg = msg.encode(te, 'replace')
         self.stderr.write("bzr: warning: %s\n" % msg)
 
     def _progress_updated(self, task):