Merge lp:~vila/bzr/1249732-acceptable-keys-from-config into lp:bzr

Proposed by Vincent Ladeuil on 2013-11-10
Status: Merged
Approved by: Richard Wilbur on 2014-04-16
Approved revision: 6592
Merged at revision: 6595
Proposed branch: lp:~vila/bzr/1249732-acceptable-keys-from-config
Merge into: lp:bzr
Diff against target: 89 lines (+19/-16)
4 files modified
bzrlib/commit_signature_commands.py (+3/-3)
bzrlib/gpg.py (+4/-13)
bzrlib/tests/test_gpg.py (+9/-0)
doc/en/release-notes/bzr-2.7.txt (+3/-0)
To merge this branch: bzr merge lp:~vila/bzr/1249732-acceptable-keys-from-config
Reviewer Review Type Date Requested Status
Richard Wilbur 2013-11-10 Approve on 2014-04-16
Bazaar Codereview Subscribers 2013-12-06 Pending
Review via email: mp+194640@code.launchpad.net

Commit message

Fix command line override handling for acceptable_keys

Description of the change

The root cause is that some code was providing support to a command-line
override for a config option (and mixed with converting a string value to a
list object).

This was missed when the 'acceptable_keys' was migrated to the new config
stacks which provide both features.

So basically a missing test for gpg didn't track the regression.

Test added, code simplified, bug gone ;)

To post a comment you must log in.
Richard Wilbur (richard-wilbur) wrote :

Good fix! Looks like gpg now is happy to deal with Unicode?
+1

review: Approve
6591. By Vincent Ladeuil on 2013-11-10

Fix typo.

Vincent Ladeuil (vila) wrote :

> Looks like gpg now is happy to deal with Unicode?

Nope, that error message in the dead code was misleading, no option name was involved so I don't really know what it was referring too (yet another missing test ;).

I don't know enough about that code area to say whether or not unicode support is good or not. But this merge proposal is not related to unicode support either.

Richard Wilbur (richard-wilbur) wrote :

Do the internet RFC's allow Unicode in E-mail addresses and/or domain names?

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

Upon further consideration, it looks like it would be useful to add a test of a list of acceptable_keys. If Unicode occurs in E-mail addresses and/or domain names it would also be useful to add a test of with an acceptable_keys list including one or more containing Unicode characters.

review: Needs Fixing
John A Meinel (jameinel) wrote :

I think the patch got read backwards. When we parse commandline args, the
parser turns everything into Unicode objects. So this was just trying to
turn them back into str objects.
I like the idea of testing a list rather than just a single entry. There is
also a typo in NEWS (bazzar.conf)
Otherwise it gets an approve from me. (I can't vote on LP from my phone)

John
=:->
On Nov 11, 2013 1:33 AM, "Richard Wilbur" <email address hidden> wrote:

> Review: Needs Fixing
>
> Upon further consideration, it looks like it would be useful to add a test
> of a list of acceptable_keys. If Unicode occurs in E-mail addresses and/or
> domain names it would also be useful to add a test of with an
> acceptable_keys list including one or more containing Unicode characters.
> --
>
> https://code.launchpad.net/~vila/bzr/1249732-acceptable-keys-from-config/+merge/194640
> Your team Bazaar Codereview Subscribers is subscribed to branch lp:bzr.
>

6592. By Vincent Ladeuil on 2013-11-11

Fix typo.

Vincent Ladeuil (vila) wrote :

/me blinks

Thanks Haw Loeung for your heads-up (not sure what you did (probably add a review request) but I got a lp mail about that proposal, read the cover letter thinking it was yours and went: "yeah, good catch, let's land this).

I completely forgot about that proposal and thought it has landed eons ago ;)

@Richard: Are you happy with John's precisions ?

Vincent Ladeuil (vila) wrote :

@Richard: ping

Richard Wilbur (richard-wilbur) wrote :

@Vincent: Sorry for the long delay. I'll look at it tonight or
tomorrow afternoon. Thanks for the ping!

On Wed, Jan 8, 2014 at 3:51 AM, Vincent Ladeuil <email address hidden> wrote:
> @Richard: ping
> --
> https://code.launchpad.net/~vila/bzr/1249732-acceptable-keys-from-config/+merge/194640
> You are reviewing the proposed merge of lp:~vila/bzr/1249732-acceptable-keys-from-config into lp:bzr.

Richard Wilbur (richard-wilbur) wrote :

John Meinel wrote:
> I think the patch got read backwards. When we parse commandline args, the
> parser turns everything into Unicode objects. So this was just trying to
> turn them back into str objects.

It looks like the changes in this branch remove the code that was trying to turn the commandline args from Unicode objects back into str objects. Is that operation still needed? Other comments in the code imply that gpg is allergic to Unicode (or at least would highly prefer str).

The existing code doesn't look like it expects to handle a list. It looks like it was set up to handle a single object where it checks for Unicode and converts to str.

> I like the idea of testing a list rather than just a single entry.

@Vincent: How hard would it be to create a test of a list of commandline args?

> There is also a typo in NEWS (bazzar.conf)

@Vincent: Thanks for fixing the typo.

review: Needs Information
Vincent Ladeuil (vila) wrote :

Sorry for the dealy (damn, will that MP ever land ;)

> John Meinel wrote:
> > I think the patch got read backwards. When we parse commandline args, the
> > parser turns everything into Unicode objects. So this was just trying to
> > turn them back into str objects.
>
> It looks like the changes in this branch remove the code that was trying to
> turn the commandline args from Unicode objects back into str objects.

I think the comment (gpg Context.keylist(pattern) does not like unicode) and
the error message ('Only ASCII permitted in option names') were both
misleading and wrong (we receive option values not option names) and I
suspect the unicode/str issue is more about internals than about proper
unicode support for keys (but I know little about gpg, really, that MP is
about a misuse of bzrlib.config rather than about gpg itsel ;).

> Is that operation still needed?

With the proposed implementation, definitely no, bzrlib.config *always*
return values in unicode.

> Other comments in the code imply that gpg is allergic
> to Unicode (or at least would highly prefer str).

That would require another MP from someone more knowledgeable than me about
gpg.

>
> The existing code doesn't look like it expects to handle a list.

It was and the proposed code still does.

> It looks
> like it was set up to handle a single object where it checks for Unicode and
> converts to str.

but then, it splits that string:

- patterns = key_patterns.split(",")
+ patterns = command_line_input.split(',')

so handling a single string at the start allowed simpler code.

>
> > I like the idea of testing a list rather than just a single entry.

Well, the tests use a list with a single element so we know the list is at
least properly handled for this specific case (there are even tests that
feed just a key which is converted into a list at the line mentioned above,
split returns a list of a single element when the separator is not found).

>
> @Vincent: How hard would it be to create a test of a list of commandline
> args?

There is not enough gpg test infra (a set of keys produced outside the test
suite are hard-coded across several tests) in place to allow me to just add
tests with more than one key. I wouldn't care that much about list handling
there though.

Richard Wilbur (richard-wilbur) wrote :

@Vincent: Thanks for all the work on this one. I think it is ready to land.
+1

review: Approve
Richard Wilbur (richard-wilbur) wrote :

I tried running hydrazine after approving this merge proposal and it
takes an IOError exception because the filename is too long (shown
below). I wonder if that would be remedied by upgrading from Python
2.6 to 2.7 or beyond?

$ ./feed-pqm bzr
Looking for ['Approved'] mps in https://api.launchpad.net/1.0/bzr
https://code.launchpad.net/~vila/bzr/1249732-acceptable-keys-from-config/+merge/194640
       message: Fix command line override handling for acceptable_keys
        source: lp:~vila/bzr/1249732-acceptable-keys-from-config
        target: lp:bzr
        status: Approved
       created: 2013-11-10 15:34:32.127695+00:00
      reviewed: 2014-04-16 04:34:09.513847+00:00
    registrant: vila
Traceback (most recent call last):
  File "./feed-pqm", line 261, in <module>
    sys.exit(main(sys.argv))
  File "./feed-pqm", line 219, in main
    show_mp(mp)
  File "./feed-pqm", line 104, in show_mp
    (vote.comment and vote.comment.vote or 'Requested'),
  File "/usr/lib/pymodules/python2.6/lazr/restfulclient/resource.py",
line 638, in __getattr__
    return super(Entry, self).__getattr__(name)
  File "/usr/lib/pymodules/python2.6/lazr/restfulclient/resource.py",
line 301, in __getattr__
    return self.lp_get_parameter(attr)
  File "/usr/lib/pymodules/python2.6/lazr/restfulclient/resource.py",
line 196, in lp_get_parameter
    self._ensure_representation()
  File "/usr/lib/pymodules/python2.6/lazr/restfulclient/resource.py",
line 331, in _ensure_representation
    representation = self._root._browser.get(self._wadl_resource)
  File "/usr/lib/pymodules/python2.6/lazr/restfulclient/_browser.py",
line 316, in get
    response, content = self._request(url, extra_headers=headers)
  File "/usr/lib/pymodules/python2.6/lazr/restfulclient/_browser.py",
line 260, in _request
    str(url), method=method, body=data, headers=headers)
  File "/usr/lib/pymodules/python2.6/httplib2/__init__.py", line 1444,
in request
    (response, content) = self._request(conn, authority, uri,
request_uri, method, body, headers, redirections, cachekey)
  File "/usr/lib/pymodules/python2.6/lazr/restfulclient/_browser.py",
line 154, in _request
    redirections, cachekey)
  File "/usr/lib/pymodules/python2.6/httplib2/__init__.py", line 1252,
in _request
    _updateCache(headers, response, content, self.cache, cachekey)
  File "/usr/lib/pymodules/python2.6/httplib2/__init__.py", line 431,
in _updateCache
    cache.set(cachekey, text)
  File "/usr/lib/pymodules/python2.6/httplib2/__init__.py", line 701, in set
    f = file(cacheFullPath, "wb")
IOError: [Errno 36] File name too long:
'/home/rwilbur/.launchpadlib/api.launchpad.net/cache/api.launchpad.net,1.0,~vila,bzr,1249732-acceptable-keys-from-config,+merge,194640,comments,512879-application,json,0454099ae69ddb0c0d041245b581070d'

review: Approve
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Vincent Ladeuil (vila) wrote :

> I tried running hydrazine after approving this merge proposal and it
> takes an IOError exception because the filename is too long (shown
> below). I wonder if that would be remedied by upgrading from Python
> 2.6 to 2.7 or beyond?

It seems so as I couldn't reproduce it by using the same command from trusty (so pyt27 indeed).

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bzrlib/commit_signature_commands.py'
--- bzrlib/commit_signature_commands.py 2012-03-11 16:51:49 +0000
+++ bzrlib/commit_signature_commands.py 2013-11-11 11:33:19 +0000
@@ -111,7 +111,7 @@
111 ' acceptable for verification.',111 ' acceptable for verification.',
112 short_name='k',112 short_name='k',
113 type=str,),113 type=str,),
114 'revision', 114 'revision',
115 'verbose',115 'verbose',
116 ]116 ]
117 takes_args = ['location?']117 takes_args = ['location?']
@@ -160,8 +160,8 @@
160 # Ignore ghosts160 # Ignore ghosts
161 continue161 continue
162 revisions.append(rev_id)162 revisions.append(rev_id)
163 count, result, all_verifiable =\163 count, result, all_verifiable = gpg.bulk_verify_signatures(
164 gpg.bulk_verify_signatures(repo, revisions, gpg_strategy)164 repo, revisions, gpg_strategy)
165 if all_verifiable:165 if all_verifiable:
166 write(gettext("All commits signed with verifiable keys"))166 write(gettext("All commits signed with verifiable keys"))
167 if verbose:167 if verbose:
168168
=== modified file 'bzrlib/gpg.py'
--- bzrlib/gpg.py 2012-06-29 08:12:32 +0000
+++ bzrlib/gpg.py 2013-11-11 11:33:19 +0000
@@ -336,23 +336,14 @@
336 command line336 command line
337 :return: nothing337 :return: nothing
338 """338 """
339 key_patterns = None339 patterns = None
340 acceptable_keys_config = self._config_stack.get('acceptable_keys')340 acceptable_keys_config = self._config_stack.get('acceptable_keys')
341 try:
342 if isinstance(acceptable_keys_config, unicode):
343 acceptable_keys_config = str(acceptable_keys_config)
344 except UnicodeEncodeError:
345 # gpg Context.keylist(pattern) does not like unicode
346 raise errors.BzrCommandError(
347 gettext('Only ASCII permitted in option names'))
348
349 if acceptable_keys_config is not None:341 if acceptable_keys_config is not None:
350 key_patterns = acceptable_keys_config342 patterns = acceptable_keys_config
351 if command_line_input is not None: # command line overrides config343 if command_line_input is not None: # command line overrides config
352 key_patterns = command_line_input344 patterns = command_line_input.split(',')
353 if key_patterns is not None:
354 patterns = key_patterns.split(",")
355345
346 if patterns:
356 self.acceptable_keys = []347 self.acceptable_keys = []
357 for pattern in patterns:348 for pattern in patterns:
358 result = self.context.keylist(pattern)349 result = self.context.keylist(pattern)
359350
=== modified file 'bzrlib/tests/test_gpg.py'
--- bzrlib/tests/test_gpg.py 2012-06-26 23:04:30 +0000
+++ bzrlib/tests/test_gpg.py 2013-11-11 11:33:19 +0000
@@ -499,6 +499,15 @@
499 self.assertEqual(my_gpg.acceptable_keys,499 self.assertEqual(my_gpg.acceptable_keys,
500 [u'B5DEED5FCB15DAE6ECEF919587681B1EE3080E45'])500 [u'B5DEED5FCB15DAE6ECEF919587681B1EE3080E45'])
501501
502 def test_set_acceptable_keys_from_config(self):
503 self.requireFeature(features.gpgme)
504 self.import_keys()
505 my_gpg = gpg.GPGStrategy(FakeConfig(
506 'acceptable_keys=bazaar@example.com'))
507 my_gpg.set_acceptable_keys(None)
508 self.assertEqual(my_gpg.acceptable_keys,
509 [u'B5DEED5FCB15DAE6ECEF919587681B1EE3080E45'])
510
502 def test_set_acceptable_keys_unknown(self):511 def test_set_acceptable_keys_unknown(self):
503 self.requireFeature(features.gpgme)512 self.requireFeature(features.gpgme)
504 my_gpg = gpg.GPGStrategy(FakeConfig())513 my_gpg = gpg.GPGStrategy(FakeConfig())
505514
=== modified file 'doc/en/release-notes/bzr-2.7.txt'
--- doc/en/release-notes/bzr-2.7.txt 2013-10-07 16:35:59 +0000
+++ doc/en/release-notes/bzr-2.7.txt 2013-11-11 11:33:19 +0000
@@ -32,6 +32,9 @@
32.. Fixes for situations where bzr would previously crash or give incorrect32.. Fixes for situations where bzr would previously crash or give incorrect
33 or undesirable results.33 or undesirable results.
3434
35* 'acceptable_keys' from 'bazaar.conf' is now properly handled.
36 (Vincent Ladeuil, #1249732)
37
35* Option names are now checked to be valid [dotted] python identifiers. Also38* Option names are now checked to be valid [dotted] python identifiers. Also
36 ignore invalid references (i.e. using invalid option names) while39 ignore invalid references (i.e. using invalid option names) while
37 expanding option values. (Vincent Ladeuil, #1235099)40 expanding option values. (Vincent Ladeuil, #1235099)