Code review comment for lp:~jr/bzr/bzr-gpgme

On Wed, Jun 22, 2011 at 11:43:37AM -0000, John A Meinel wrote:
> [1 needs fixing] You are adding a call to "get_ancestry()" which Jelmer
> just recently marked as deprecated. I imagine this is pretty easy to fix
> following his examples.

This was a bad merge, now fixed.

> [2 comment] You import 'gpgme' inside of builtins.py. I would have
> expected that sort of thing to be hidden inside 'gpg.py'. Something like
> "gpg.support_verifying_signatures()" or "check_can_verify_signatures", etc.

Done.

> [3 tweak] You have too much code in "cmd_verify_signatures". Most of
> that should probably be moved into gpg.py and the cmd_* code should be a
> reasonably small shim. That usually makes it easier to re-use and test.
> Especially if a GUI ever wants to borrow some of the verify code.

Done.

> [4 needsinfo] GpgStrategy.verify(self, content, testament)
>
> - From what I can tell, you create a new gpgme Context for every revision.
> Which I *think* means spawning a new gpg process. It seems like a
> scalable design would want to be able to hold the context for an
> extended period of time. Could it be a lazy attribute of GPGStrategy?

I've made the context global to the GpgStrategy class (although I
don't think it starts a new gpg process for each one).

> [5 needsinfo] Would we want a way to distinguish from "signature is
> properly signed by an un-acceptable user" from "signature not properly
> signed"?

There's no such concept as an unacceptable user, only acceptable ones.
Anything else is an unknown key and listed as such.

> [6 needsinfo] You mention that it slows down "bzr log" quite a bit. Have
> you tried --lsprof to see what that cause is?

The main cause is calls to verify() and get_key() in gpgme according to --lsprof.

> [7 needsfixing] bzrlib/tests/blackbox/test_sign_my_commits.py
> You're adding one blank line per statement, which isn't preferred form.

Done

> [8 needsfixing] I don't think we need the short-form of "verify". I
> agree with Jelmer that it sounds more like repository checking, and I
> don't think it is something people will run enough to need a short form for.
>
> The docs also need to be updated for this fact.

Done

> [9 needsfixing] We'll also want an entry in
> doc/en/release-notes/bzr-2.4.txt and probably
> doc/whatsnew/whats-new-in-2.4.txt.

Done

Jonathan