Merge lp:~krause/bzr/1037108 into lp:bzr

Proposed by Thomas Krause on 2012-08-16
Status: Rejected
Rejected by: Richard Wilbur on 2013-10-10
Proposed branch: lp:~krause/bzr/1037108
Merge into: lp:bzr
Diff against target: 15 lines (+2/-2)
1 file modified
bzrlib/log.py (+2/-2)
To merge this branch: bzr merge lp:~krause/bzr/1037108
Reviewer Review Type Date Requested Status
Richard Wilbur branch superceded Disapprove on 2013-10-10
John A Meinel 2012-08-16 Needs Fixing on 2012-08-21
Review via email: mp+119912@code.launchpad.net

Description of the change

When accessing a repository which has signed commits and where the name of the author contains non ASCII characters you will get an UnicodeEncodeError when trying to get the log if signature display is enabled.

E.g. you can try

bzr log --signatures lp:~pixeldrama/annis/restr_vis -r 973..975

The problem is in bzrlib/log.py in the function "format_signature_validity" which mixes a constant non-unicode string with a unicode string which is given as argument for the format function.

This branch fixes this for this specific function by using unicode string constants (u"abc").

Please note that I was not able to add a testcase since neither all other testcases were successfull on my computer nor did I find a good place to put the testcase.

To post a comment you must log in.
John A Meinel (jameinel) wrote :

Since this is an issue for 'log', I think either bzrlib/tests/test_log.py or bzrlib/tests/blackbox/test_log.py would be good places. (I prefer the former as a white-box test, vs the later as a whole-command test.)

All of those tests should pass on all platforms, and you can just run those tests with:
  bzr selftest -s bt.test_log -s bb.test_log

The change itself seems ok, but maybe incomplete. It seems that the real problem is that we are not decoding result[1]. So it comes in as a UTF-8 string (8-bit str with high bits set.) When what we need is a python Unicode string. It is also good to have the format strings be unicode.

So a fix like:

unicode_result = result[1].decode('utf-8') # Can we assume utf8?
if result[0] == gpg.SIGNATURE_VALID:
  return u'valid signature from {0}'.format(unicode_result)
...

etc.

review: Needs Fixing
Richard Wilbur (richard-wilbur) wrote :

Thomas,
Thanks for the hard work to fix the original bug. I am marking this branch as superceded. The problem has changed--pygpgme has taken care of the UTF8 -> Unicode conversion John mentioned above. Reagan Sanders has created a new branch lp:~vexo/bzr/fix-1123460 which addresses this problem in the present contex and also contains tests of the verify_signature code.

So, thanks again and I look forward to the opportunity to approve your next contribution.

review: Disapprove (branch superceded)

Unmerged revisions

6554. By Thomas Krause on 2012-08-15

use unicode constants

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/log.py'
2--- bzrlib/log.py 2012-03-14 11:30:42 +0000
3+++ bzrlib/log.py 2012-08-16 12:41:27 +0000
4@@ -334,9 +334,9 @@
5 gpg_strategy = gpg.GPGStrategy(None)
6 result = repo.verify_revision_signature(rev_id, gpg_strategy)
7 if result[0] == gpg.SIGNATURE_VALID:
8- return "valid signature from {0}".format(result[1])
9+ return u"valid signature from {0}".format(result[1])
10 if result[0] == gpg.SIGNATURE_KEY_MISSING:
11- return "unknown key {0}".format(result[1])
12+ return u"unknown key {0}".format(result[1])
13 if result[0] == gpg.SIGNATURE_NOT_VALID:
14 return "invalid signature!"
15 if result[0] == gpg.SIGNATURE_NOT_SIGNED: