qbzr crashed with UnicodeDecodeError in run_subprocess_command()

Can hit the problem by making the blackbox tests use a non-ascii path like so:

=== modified file 'bzrlib/tests/blackbox/test_conflicts.py'
--- bzrlib/tests/blackbox/test_conflicts.py 2010-11-07 16:32:51 +0000
+++ bzrlib/tests/blackbox/test_conflicts.py 2011-04-13 14:05:01 +0000
@@ -43,7 +43,7 @@
         ('%s/myfile' % (this_path,), 'contentsa2\n'),
         ('%s/my_other_file' % (this_path,), 'contentsa2\n'),
         ])
- this_tree.rename_one('mydir', 'mydir3')
+ this_tree.rename_one('mydir', u'mydir3\xa0')
     this_tree.commit(message='change')
     this_tree.merge_from_branch(other_tree.branch)
     return this_tree, other_tree

A user on IRC has just run into this as well with QBzr 0.21.0.dev, also during a checkout.

Extract from his log is at:
<http://pastebin.com/H8vaabTX>

Ignore comment #2 which is intended for bug 686161 not this one.

This is a problem from the changes in r1234 (really), which blindly calls unicode() on all non-private attributes of an exception instance. If an exception has a non-ascii bytestring in its arguments, this exception will occur. Other objects that can't be stringified are also a potential issue.

The code is currently:

    try:
        return commands.run_bzr(argv)
    except Exception, e:
        d = {}
        for key, val in e.__dict__.iteritems():
            if not key.startswith('_'):
                if not isinstance(val, unicode):
                    val = unicode(val)
                d[key] = val
        print "%s%s" % (SUB_ERROR,
                        bencode.bencode((e.__class__.__name__,
                                         encode_unicode_escape(d))))
        raise

Adding more try/catch is an option, or being more careful about accessing the exception instance.

So, qbzr failing to properly report the error to the main gui process from subprocess. I think the wrong part here is blind `val = unicode(val)` which in fact should explicitly specify the encoding, i.e. val = unicode(val, ENCODING) and maybe even errors handling as 'replace'.

But I'm not sure what is the correct encoding should be here.

Ghaa, annotation suggest that I wanted encode unicode with `unicode-escape` encoding. But I didn't do that. Can't say why.

I don't think there is any problem with bzr.

@Marting[gz]: here is what I think should be the fix for this problem:

=== modified file 'lib/subprocess.py'
--- lib/subprocess.py 2010-10-21 11:32:48 +0000
+++ lib/subprocess.py 2011-04-14 12:55:40 +0000
@@ -890,9 +890,12 @@
         d = {}
         for key, val in e.__dict__.iteritems():
             if not key.startswith('_'):
- if not isinstance(val, unicode):
- val = unicode(val)
- d[key] = val
+ if isinstance(val, unicode):
+ d[key] = val
+ elif isinstance(val, str):

What do you think about it?

@Vasily: если можешь повторить проблему на своей машине, то пожалуйста протестируй мой патч.
+ d[key] = unicode(val, osutils.get_user_encoding(), 'replace')
+ else:
+ d[key] = repr(val)
         print "%s%s" % (SUB_ERROR,
                         bencode.bencode((e.__class__.__name__,
                                          encode_unicode_escape(d))))

I've attahced the patch for qbzr, please test it if you can reproduce this problem.

That's the right idea Alexander, but the ability to pass multiple parameters to the unicode() function is there to catch you out - it only works on buffer-type objects, and exception instance attributes can be anything.

    >>> unicode(object())
    u'<object object at 0x00AB14C8>'
    >>> unicode(object(), "ascii")
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: coercing to Unicode: need string or buffer, object found

I'm still not quite sure what exactly this exception stringifying needs, but will put up a branch for review.

Sorry Alexander, your patch actually avoids that problem, I misread. There's another corner case with repr I think may be worth handling though.