Merge lp:~gz/bzr/undecodable_argv_745712 into lp:bzr

[Martin Packman]

better message when argv can't be decoded in the application locale

When sys.argv can't be decoded according to the application locale encoding, Bazaar needs to report a nice clear error. The code nearly does that already, apart from the fact the BzrError raised from osutils.get_unicode_argv goes uncaught as commands._specified_or_unicode_argv which calls it is outside the normal exception catching and reporting code.

To fix this the argv decoding is now done inside commands.run_bzr rather than commands.main as it was previously. This is something of a subtle behaviour change, but the other alternative is changing the signatures of all the run_* methods, which strikes me as worse.

Should the error message also give more advice about how to fix locale issues, like the locale.setlocale except clause in the main bzr script?

Hm... just seen bb.test_commit.TestCommit.test_unsupported_encoding_commit_message fail which is testing for the old buggy behaviour, will sort that out that tomorrow.

[Martin Pool (mbp)]

strictly that should be testnotapplicable.

[Martin Pool (mbp)]

- argv = _specified_or_unicode_argv(argv)
+ if argv is not None:
+ argv = argv[1:]

It looks like you have one too few spaces there.

The whole topic of locales is probably a bit too complex to explain in an error message. We could mention a URL about it.

+++ bzrlib/osutils.py 2011-04-15 00:49:24 +0000
@@ -96,8 +96,8 @@
         user_encoding = get_user_encoding()
         return [a.decode(user_encoding) for a in sys.argv[1:]]
     except UnicodeDecodeError:
- raise errors.BzrError(("Parameter '%r' is unsupported by the current "
- "encoding." % a))
+ raise errors.BzrError("Parameter %r encoding is unsupported by the "
+ "application locale." % (a,))

One thing I would do here is make the error message include the detected encoding to give people an idea where to start. (It may already be in the crash report but perhaps that's not so visible.)

Aside from that and the point about skip it looks good. Thanks for tackling it.

review: Needs Fixing

[Martin Packman (gz)]

> It looks like you have one too few spaces there.

Worse, a tab. I've rewritten history to hide that embarrassment, shouldn't try and finish branches late.

> The whole topic of locales is probably a bit too complex to explain in an
> error message. We could mention a URL about it.

Along the lines of "See `bzr help locale` for more information."?

> One thing I would do here is make the error message include the detected
> encoding to give people an idea where to start. (It may already be in the
> crash report but perhaps that's not so visible.)

Will do, I nearly did this but the spelling of ascii as 'ANSI_X3.4-1968' looked needlessly scary to me.

> Aside from that and the point about skip it looks good. Thanks for tackling
> it.

Skip change also in the new-old revisions.

Left to do is the bb.test_commit failure, and also one in bb.test_command_encoding which does something dodgy (with a comment saying as much).

[Martin Packman (gz)]

I removed the two problematic testing bits I found, there may be more fallout if plugins are doing dodgy things with TestCase.run_bzr (which they are in my experience), I'll follow up if that's the case.

I deleted test_unsupported_encoding_commit_message outright which loses the blame but not the coverage as it does the same thing as the new test in a less generic manner. Should really have found it before making the branch, poolie had linked it from the bug. The TestCommandEncoding.test_exact assertion might be useful to find a way to keep, though I'm not convinced writing arbitrary bytes to the text output is a generally good idea.

[Martin Pool (mbp)]

great, thankyou.

review: Approve

[Martin Pool (mbp)]

sent to pqm by email

=== modified file 'bzrlib/commands.py'
--- bzrlib/commands.py	2011-04-05 01:12:15 +0000
+++ bzrlib/commands.py	2011-04-15 21:25:54 +0000
@@ -1031,7 +1031,7 @@
         Specify the number of processes that can be run concurrently (selftest).
     """
     trace.mutter("bazaar version: " + bzrlib.__version__)
-    argv = list(argv)
+    argv = _specified_or_unicode_argv(argv)
     trace.mutter("bzr arguments: %r", argv)
 
     opt_lsprof = opt_profile = opt_no_plugins = opt_builtin =  \
@@ -1177,7 +1177,7 @@
         new_argv = []
         try:
             # ensure all arguments are unicode strings
-            for a in argv[1:]:
+            for a in argv:
                 if isinstance(a, unicode):
                     new_argv.append(a)
                 else:
@@ -1199,7 +1199,8 @@
 
     :return: exit code of bzr command.
     """
-    argv = _specified_or_unicode_argv(argv)
+    if argv is not None:
+        argv = argv[1:]
     _register_builtin_commands()
     ret = run_bzr_catch_errors(argv)
     trace.mutter("return code %d", ret)
 
=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2011-04-04 13:37:25 +0000
+++ bzrlib/osutils.py	2011-04-15 21:25:54 +0000
@@ -96,8 +96,8 @@
         user_encoding = get_user_encoding()
         return [a.decode(user_encoding) for a in sys.argv[1:]]
     except UnicodeDecodeError:
-        raise errors.BzrError(("Parameter '%r' is unsupported by the current "
-                                                            "encoding." % a))
+        raise errors.BzrError("Parameter %r encoding is unsupported by %s "
+            "application locale." % (a, user_encoding))
 
 
 def make_readonly(filename):
 
=== modified file 'bzrlib/tests/blackbox/test_command_encoding.py'
--- bzrlib/tests/blackbox/test_command_encoding.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/blackbox/test_command_encoding.py	2011-04-15 21:25:54 +0000
@@ -54,13 +54,12 @@
         register_command(cmd_echo_exact)
         try:
             self.assertEqual('foo', bzr('echo-exact foo'))
-            # This is cheating a little bit, because 'foo\xb5' shouldn't
-            # get past main()
-            self.assertEqual('foo\xb5', bzr('echo-exact foo\xb5'))
             # Exact should fail to decode the string
             self.assertRaises(UnicodeEncodeError,
                 bzr,
                 ['echo-exact', u'foo\xb5'])
+            # Previously a non-ascii bytestring was also tested, as 'exact'
+            # outputs bytes untouched, but needed buggy argv parsing to work
         finally:
             plugin_cmds.remove('echo-exact')
 
 
=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2011-04-07 13:08:39 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2011-04-15 21:25:54 +0000
@@ -321,26 +321,6 @@
         tree.add('foo.c')
         self.run_bzr('commit -m ""', retcode=3)
 
-    def test_unsupported_encoding_commit_message(self):
-        if sys.platform == 'win32':
-            raise tests.TestNotApplicable('Win32 parses arguments directly'
-                ' as Unicode, so we can\'t pass invalid non-ascii')
-        tree = self.make_branch_and_tree('.')
-        self.build_tree_contents([('foo.c', 'int main() {}')])
-        tree.add('foo.c')
-        # LANG env variable has no effect on Windows
-        # but some characters anyway cannot be represented
-        # in default user encoding
-        char = probe_bad_non_ascii(osutils.get_user_encoding())
-        if char is None:
-            raise TestSkipped('Cannot find suitable non-ascii character'
-                'for user_encoding (%s)' % osutils.get_user_encoding())
-        out,err = self.run_bzr_subprocess('commit -m "%s"' % char,
-                                          retcode=1,
-                                          env_changes={'LANG': 'C'})
-        self.assertContainsRe(err, r'bzrlib.errors.BzrError: Parameter.*is '
-                                    'unsupported by the current encoding.')
-
     def test_other_branch_commit(self):
         # this branch is to ensure consistent behaviour, whether we're run
         # inside a branch, or not.
 
=== modified file 'bzrlib/tests/blackbox/test_exceptions.py'
--- bzrlib/tests/blackbox/test_exceptions.py	2011-03-05 01:37:14 +0000
+++ bzrlib/tests/blackbox/test_exceptions.py	2011-04-15 21:25:54 +0000
@@ -16,6 +16,7 @@
 
 """Tests for display of exceptions."""
 
+import os
 import sys
 
 from bzrlib import (
@@ -45,6 +46,21 @@
                 r'exceptions\.AssertionError: always fails\n')
         self.assertContainsRe(err, r'Bazaar has encountered an internal error')
 
+    def test_undecodable_argv(self):
+        """A user error must be reported if argv is not in the locale encoding
+
+        A subprocess with an environment ascii-only setting is used so the test
+        can run without worrying about the locale the test suite is using.
+        """
+        if os.name != "posix":
+            raise tests.TestNotApplicable("Needs system beholden to C locales")
+        out, err = self.run_bzr_subprocess(["\xa0"],
+            env_changes={"LANG": "C"},
+            universal_newlines=True,
+            retcode=errors.EXIT_ERROR)
+        self.assertContainsRe(err, r"^bzr: ERROR: .*'\\xa0'.* unsupported")
+        self.assertEquals(out, "")
+
 
 class TestOptParseBugHandling(TestCase):
     "Test that we handle http://bugs.python.org/issue2931"
 
=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-04-15 10:31:18 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-04-15 21:25:54 +0000
@@ -53,6 +53,9 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* Arguments that can't be decoded to unicode in the current posix locale give
+  a clearer error message without a traceback. (Martin [gz], #745712)
+
 * ``bzrlib.log._DEFAULT_REQUEST_PARAMS`` is no longer accidentally
   mutated by ``bzrlib.log._apply_log_request_defaults``.  In practice
   these default values aren't relied on very often so this probably