Merge lp:~ryorke/bzr/140563-optparse-barfs-on-unicode into lp:bzr

[Rory Yorke]

Trap UnicodeError from optparse parsing.

This change is for bug 140563.

optparse can't handle non-ASCII option names. This commit doesn't try to fix
that, but, as suggested in the bug discussion, reports an error that might
give the user a better idea of what the problem is.

A test has been added to blackbox.test_non_ascii for this; it seems like the
right place, though running this test with different encodings doesn't really
make sense.

There's a patch that puportedly fixes optparse; see

  http://bugs.python.org/issue2931

I don't know how Python development works, but I can't imagine it'll be
backported to older versions (<2.7), so we may be stuck with this for a while.

[Martin Packman (gz)]

Nitpick, I'd like a comment by that try/except explaining the optparse problem we're working around and linking the upstream bug.

I get different behaviour on windows. Rather than throwing, it mangles the bytes. This breaks your test, and then the whole run as the fix for bug 633216 hasn't landed.

Suggests you could perhaps give optparse bytes on the nix path, then decode what you get back instead?

review: Needs Fixing

[Rory Yorke (ryorke)]

OK, so after some tests, I have:

| Platform | test_nonascii_optparse | "bzr st -ä" gives |
|-------------+------------------------+---------------------------------------------|
| Lin py24 | fail | ERROR: no such option: -ä |
| Lin py25-26 | pass | ERROR: Only ASCII permitted in option names |
| Win py24 | error | ERROR: no such option: -ä |
| Win py27 | pass* | ERROR: Only ASCII permitted in option names |

On Windows/Python 2.7, four of the five tests are skipped; each one's for a
different user encoding. This seems to be unrelated to command options, but
is something to do with a path:

  Unable to represent path u'\u0422\u0435\u0441\u0442' in terminal encoding
  "cp437 " (even though it is valid in filesystem encoding "mbcs"))

On Windows/Python 2.4, the test errors out spectacularly with a
UnicodeEncodeError (assume this is what you referred to).

I'm not sure what to do. One possibility is allowing either error form ("only
ascii" and "no such option"); in this case the test passes everywhere.

I don't understand your suggestion. What's the "nix path"?

[Martin Packman (gz)]

Sorry Rory, looked at this in rather a rush and drew the wrong conclusion. Well done for investigating and finding out the problem was me being weird and running Python 2.4 and not me being weird and running windows.

> On Windows/Python 2.7, four of the five tests are skipped; each one's for a
> different user encoding. This seems to be unrelated to command options, but
> is something to do with a path:

This is something else I should have mentioned, you don't really want to put your test there, as you don't need the weird locale fiddling. How about bzrlib.tests.blackbox.test_exceptions instead?

> On Windows/Python 2.4, the test errors out spectacularly with a
> UnicodeEncodeError (assume this is what you referred to).

Yep.

> I'm not sure what to do. One possibility is allowing either error form ("only
> ascii" and "no such option"); in this case the test passes everywhere.

Well, you could either skip or just check the different output on `sys.version_info < (2, 5)` as that's where the optparse bug seems to have been introduced.

> I don't understand your suggestion. What's the "nix path"?

Rather moot now, but I was wondering if the decode step could be moved after the optparse step on posix systems as the command line is 8 bit natively there whereas it's 16 bit natively on modern windows systems.

[lp:~ryorke/bzr/140563-optparse-barfs-on-unicode]

5516. By Rory Yorke on 2010-10-29

Added comment on try-except around pargse_args(), w/ pointer to Python bug.

5517. By Rory Yorke on 2010-10-29

Moved test to bb.test_exceptions, and test differently for Python 2.4.

In Python 2.4, optparse doesn't raise a bug if fed non-ASCII option
names; check for that case separately. This change is needed to prevent
bzr selftest breaking on Windows with Python 2.4.

[Martin Packman (gz)]

Thanks Rory, this looks solid now. As you've got the important stuff down, also start thinking about trivial things like PEP 8, spaces around equals signs and commas and such like.

review: Approve

[John A Meinel (jameinel)]

sent to pqm by email

=== modified file 'bzrlib/commands.py'
--- bzrlib/commands.py	2010-10-08 06:22:30 +0000
+++ bzrlib/commands.py	2010-10-29 17:14:43 +0000
@@ -815,7 +815,13 @@
     else:
         args = argv
 
-    options, args = parser.parse_args(args)
+    # for python 2.5 and later, optparse raises this exception if a non-ascii
+    # option name is given.  See http://bugs.python.org/issue2931
+    try:
+        options, args = parser.parse_args(args)
+    except UnicodeEncodeError,e:
+        raise errors.BzrCommandError('Only ASCII permitted in option names')
+
     opts = dict([(k, v) for k, v in options.__dict__.iteritems() if
                  v is not option.OptionParser.DEFAULT_VALUE])
     return args, opts

=== modified file 'bzrlib/tests/blackbox/test_exceptions.py'
--- bzrlib/tests/blackbox/test_exceptions.py	2010-02-17 17:11:16 +0000
+++ bzrlib/tests/blackbox/test_exceptions.py	2010-10-29 17:14:43 +0000
@@ -47,6 +47,17 @@
                 r'exceptions\.AssertionError: always fails\n')
         self.assertContainsRe(err, r'Bazaar has encountered an internal error')
 
+class TestOptParseBugHandling(TestCase):
+    "Test that we handle http://bugs.python.org/issue2931"
+
+    def test_nonascii_optparse(self):
+        """Reasonable error raised when non-ascii in option name"""
+        if sys.version_info < (2,5):
+            error_re='no such option'
+        else:
+            error_re='Only ASCII permitted in option names'
+        out = self.run_bzr_error((error_re,),
+                                 ['st',u'-\xe4'])
 
 class TestDeprecationWarning(tests.TestCaseWithTransport):
     """The deprecation warning is controlled via a global variable:

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-10-26 13:53:31 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-10-29 17:14:43 +0000
@@ -61,6 +61,8 @@
 
 * Make ``bzr tag --quiet`` really quiet. (Neil Martinsen-Burrell, #239523)
 
+* Report error if non-ASCII command option given. (Rory Yorke, #140563)
+
 Documentation
 *************