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

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

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

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

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.

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

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

@Richard: ping

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

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.

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.

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

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'

sent to pqm by email

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