Code review comment for lp:~vila/bzr/1249732-acceptable-keys-from-config

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.