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

[Vincent Ladeuil]

Fix command line override handling for acceptable_keys

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 ;)

[Richard Wilbur (richard-wilbur)]

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

review: Approve

[Vincent Ladeuil (vila)]

> 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)]

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

review: Needs Information

[Richard Wilbur (richard-wilbur)]

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)]

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.
>

[Vincent Ladeuil (vila)]

/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)]

@Richard: ping

[Richard Wilbur (richard-wilbur)]

@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)]

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)]

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)]

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

review: Approve

[Richard Wilbur (richard-wilbur)]

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)]

sent to pqm by email

[Vincent Ladeuil (vila)]

> 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).

=== 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 @@
                         ' acceptable for verification.',
                    short_name='k',
                    type=str,),
-            'revision', 
+            'revision',
             'verbose',
           ]
     takes_args = ['location?']
@@ -160,8 +160,8 @@
                     # Ignore ghosts
                     continue
                 revisions.append(rev_id)
-        count, result, all_verifiable =\
-                                gpg.bulk_verify_signatures(repo, revisions, gpg_strategy)
+        count, result, all_verifiable = gpg.bulk_verify_signatures(
+            repo, revisions, gpg_strategy)
         if all_verifiable:
                write(gettext("All commits signed with verifiable keys"))
                if verbose:
 
=== 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 @@
                                 command line
         :return: nothing
         """
-        key_patterns = None
+        patterns = None
         acceptable_keys_config = self._config_stack.get('acceptable_keys')
-        try:
-            if isinstance(acceptable_keys_config, unicode):
-                acceptable_keys_config = str(acceptable_keys_config)
-        except UnicodeEncodeError:
-            # gpg Context.keylist(pattern) does not like unicode
-            raise errors.BzrCommandError(
-                gettext('Only ASCII permitted in option names'))
-
         if acceptable_keys_config is not None:
-            key_patterns = acceptable_keys_config
+            patterns = acceptable_keys_config
         if command_line_input is not None: # command line overrides config
-            key_patterns = command_line_input
-        if key_patterns is not None:
-            patterns = key_patterns.split(",")
+            patterns = command_line_input.split(',')
 
+        if patterns:
             self.acceptable_keys = []
             for pattern in patterns:
                 result = self.context.keylist(pattern)
 
=== 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 @@
         self.assertEqual(my_gpg.acceptable_keys,
                          [u'B5DEED5FCB15DAE6ECEF919587681B1EE3080E45'])
 
+    def test_set_acceptable_keys_from_config(self):
+        self.requireFeature(features.gpgme)
+        self.import_keys()
+        my_gpg = gpg.GPGStrategy(FakeConfig(
+                'acceptable_keys=[e-mail]'))
+        my_gpg.set_acceptable_keys(None)
+        self.assertEqual(my_gpg.acceptable_keys,
+                         [u'B5DEED5FCB15DAE6ECEF919587681B1EE3080E45'])
+
     def test_set_acceptable_keys_unknown(self):
         self.requireFeature(features.gpgme)
         my_gpg = gpg.GPGStrategy(FakeConfig())
 
=== 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 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* 'acceptable_keys' from 'bazaar.conf' is now properly handled.
+  (Vincent Ladeuil, #1249732)
+
 * Option names are now checked to be valid [dotted] python identifiers. Also
   ignore invalid references (i.e. using invalid option names) while
   expanding option values. (Vincent Ladeuil, #1235099)