Bazaar

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

  1. 140563-optparse-barfs-on-unicode
  2. Merge into bzr.dev
Proposed by Rory Yorke
Status: Merged
Approved by: Martin Packman
Approved revision: no longer in the source branch.
Merged at revision: 5529
Proposed branch: lp:~ryorke/bzr/140563-optparse-barfs-on-unicode
Merge into: lp:bzr
Diff against target: 52 lines (+20/-1)
3 files modified
bzrlib/commands.py (+7/-1)
bzrlib/tests/blackbox/test_exceptions.py (+11/-0)
doc/en/release-notes/bzr-2.3.txt (+2/-0)
To merge this branch: bzr merge lp:~ryorke/bzr/140563-optparse-barfs-on-unicode
Reviewer Review Type Date Requested Status
Martin Packman (community) Approve
Review via email: mp+39222@code.launchpad.net

Commit message

Trap UnicodeError from optparse parsing.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Martin Packman (gz) wrote :

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
Revision history for this message
Rory Yorke (ryorke) wrote :

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"?

Revision history for this message
Martin Packman (gz) wrote :

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.

Revision history for this message
Martin Packman (gz) wrote :

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
Revision history for this message
John A Meinel (jameinel) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/commands.py'
2--- bzrlib/commands.py 2010-10-08 06:22:30 +0000
3+++ bzrlib/commands.py 2010-10-29 17:14:43 +0000
4@@ -815,7 +815,13 @@
5 else:
6 args = argv
7
8- options, args = parser.parse_args(args)
9+ # for python 2.5 and later, optparse raises this exception if a non-ascii
10+ # option name is given. See http://bugs.python.org/issue2931
11+ try:
12+ options, args = parser.parse_args(args)
13+ except UnicodeEncodeError,e:
14+ raise errors.BzrCommandError('Only ASCII permitted in option names')
15+
16 opts = dict([(k, v) for k, v in options.__dict__.iteritems() if
17 v is not option.OptionParser.DEFAULT_VALUE])
18 return args, opts
19
20=== modified file 'bzrlib/tests/blackbox/test_exceptions.py'
21--- bzrlib/tests/blackbox/test_exceptions.py 2010-02-17 17:11:16 +0000
22+++ bzrlib/tests/blackbox/test_exceptions.py 2010-10-29 17:14:43 +0000
23@@ -47,6 +47,17 @@
24 r'exceptions\.AssertionError: always fails\n')
25 self.assertContainsRe(err, r'Bazaar has encountered an internal error')
26
27+class TestOptParseBugHandling(TestCase):
28+ "Test that we handle http://bugs.python.org/issue2931"
29+
30+ def test_nonascii_optparse(self):
31+ """Reasonable error raised when non-ascii in option name"""
32+ if sys.version_info < (2,5):
33+ error_re='no such option'
34+ else:
35+ error_re='Only ASCII permitted in option names'
36+ out = self.run_bzr_error((error_re,),
37+ ['st',u'-\xe4'])
38
39 class TestDeprecationWarning(tests.TestCaseWithTransport):
40 """The deprecation warning is controlled via a global variable:
41
42=== modified file 'doc/en/release-notes/bzr-2.3.txt'
43--- doc/en/release-notes/bzr-2.3.txt 2010-10-26 13:53:31 +0000
44+++ doc/en/release-notes/bzr-2.3.txt 2010-10-29 17:14:43 +0000
45@@ -61,6 +61,8 @@
46
47 * Make ``bzr tag --quiet`` really quiet. (Neil Martinsen-Burrell, #239523)
48
49+* Report error if non-ASCII command option given. (Rory Yorke, #140563)
50+
51 Documentation
52 *************
53