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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 6/21/2011 2:06 PM, Jonathan Riddell wrote:
>> 113 + if from_revno is None or to_revno is None:
>> 114 + raise errors.BzrCommandError('Cannot verify a range of \
>> 115 + non-revision-history revisions')
>> As far as I know this isn't possible - perhaps this should be an AssertionError rather than a BzrCommandError?
>
> This happens if you specify versions from a merged revision, bzr verify -r 3.1.1..3.1.2 which isn't supported by this code
> The code comes from builtins.cmd_re_sign()
>

Some thoughts:

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

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

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

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

[5 needsinfo] Would we want a way to distinguish from "signature is
properly signed by an un-acceptable user" from "signature not properly
signed"? The latter hints more of corruption. And certainly from a UI
level, it might prompt me to say "oh, I need to add this person" rather
than "why isn't there signature valid?".

Something like SIGNATURE_UNTRUSTED, or
SIGNATURE_VALID_BUT_NOT_AUTHORIZED, etc.

[6 needsinfo] You mention that it slows down "bzr log" quite a bit. Have
you tried --lsprof to see what that cause is? (Are we re-computing the
testament from scratch for each rev?) I think it would be reasonable to
consider being able to separate out "check the signatures are gpg-valid"
from "the revision matches its testament". Just because at present, the
latter is *very* expensive, but the former should be pretty cheap.

I believe that we want to make the latter cheaper so that it can scale
to more active use. However, I'm curious what the specific performance
overhead is.

[7 needsfixing] bzrlib/tests/blackbox/test_sign_my_commits.py
You're adding one blank line per statement, which isn't preferred form.
You also have a continuation line which is a bit unnatural. Something
more like:

def test_verify_commits(self):
    wt = self.setup_tree()
    self.monkey_patch_gpg()
    self.run_bzr('sign-my-commits')
    out = self.run_bzr('verify', retcode=1)
    self.assertEquals(('',
 '4 commits with valid signatures\n'
 '0 commits with unknown keys\n'
 '0 commits not valid'
 '1 commit not signed\n'), out)

I'm also wondering why we are writing the 'valid signature' information
to stderr, when the "point" of verify is to do this and report back. (It
feels like a stdout target.)

same thing for "test_verify_commits_acceptable_key"

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

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

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk4B1GgACgkQJdeBCYSNAAMbeQCfQ2XQoknSM7n5vMOvorGRN1Vj
6OwAoMNbgiqJM6sRCjvB6lrdoNZfnVvl
=3sx8
-----END PGP SIGNATURE-----