From: Jameson Graef Rollins <jrollins@finestructure.net>
To: Austin Clements <amdragon@MIT.EDU>
Subject: Re: [PATCH v2 5/5] cli: lazily create the crypto gpg context only
	when needed
In-Reply-To: <20120518192157.GV11804@mit.edu>
References: <1337362357-31281-1-git-send-email-jrollins@finestructure.net>
	<1337362357-31281-2-git-send-email-jrollins@finestructure.net>
	<1337362357-31281-3-git-send-email-jrollins@finestructure.net>
	<1337362357-31281-4-git-send-email-jrollins@finestructure.net>
	<1337362357-31281-5-git-send-email-jrollins@finestructure.net>
	<1337362357-31281-6-git-send-email-jrollins@finestructure.net>
	<20120518192157.GV11804@mit.edu>
User-Agent: Notmuch/0.12+183~g9d5ff3c (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Fri, 18 May 2012 12:45:08 -0700
Message-ID: <87txzd9su3.fsf@servo.finestructure.net>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha256; protocol="application/pgp-signature"
Cc: Notmuch Mail <notmuch@notmuchmail.org>
Precedence: list

--=-=-=

On Fri, May 18 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>> +    /* Lazily create the gpgctx if it's needed and hasn't been initialized yet */
>> +    if ((GMIME_IS_MULTIPART_ENCRYPTED (part) || GMIME_IS_MULTIPART_SIGNED (part))
>> +	&& (node->ctx->crypto->verify || node->ctx->crypto->decrypt)) {
>> +	if (!node->ctx->crypto->gpgctx) {
>
> These if conditions could be combined, like
>
>     if ((GMIME_IS_MULTIPART_ENCRYPTED (part) || GMIME_IS_MULTIPART_SIGNED (part))
> 	&& (node->ctx->crypto->verify || node->ctx->crypto->decrypt)
> 	&& !node->ctx->crypto->gpgctx) {
>
> When I see two nested 'if's like this, I expect there to be an else
> part to the inner if or something after the inner if (why else would
> it be separate?) and then I wind up matching braces when I don't see
> anything.  Also, one 'if' would save a level of indentation.

To explain what I was explaining on IRC, and what I should have noted in
the original patch, I did this on purpose because I'm looking forward to
the S/MIME support I was trying to get working.

gmime 2.6 provides a separate context for pkcs7 (S/MIME).  In this
context initialization section we will therefore have to test for
initialization of the relevant context.  Since I knew that going into
this I decided to anticipate it by constructing things this way now
future diffs wouldn't have to include the indentation of this section
and could therefore be cleaner and smaller.  If folks have issue with
that explanation let me know.

> Perhaps "If crypto->gpgctx is NULL, it will be lazily initialized."?
> The variable does have to be "initialized", in the sense that it can't
> be uninitialized data.

That sounds like a better wording.  I'll fix.

> It's slightly awkward that it's the caller's responsibility to free
> this lazily constructed object.  That should probably be documented.
> We could more carefully reference count it, but I think that would
> actually be worse because the reference count would probably bounce
> through zero frequently.

I agree that this is awkward.  Is there a suggestion on how to do it
better?  We only want to initialize it if it's needed, and only
_mime_node_create knows that.  And we don't want to free it with
_mime_node_context_free, or something, only to have to reinitialize it
again with the next node or message.  Thoughts?

jamie.

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iQIcBAEBCAAGBQJPtqbEAAoJEO00zqvie6q8GOYP+wZO99ubAxHOriE9RK5KqZQM
ATLxdRhZ9r1AElwTo7sZpyh1uAiprArjN9U7UM5BwZKICfXaaw7kJ9uNdQibdb2B
m7DnPUtf4dFkUTHY0G4866evzUX2FehJwVHAmJ61mjlUtSNTqfkvgQ9aCBC0JuUS
yNFxLrSFtn4tEI38PCMrG+rkjcJwcGA/6KA5yvipjjLrYSG+YWvvdFA0q26MUC9g
oqycy0T4KminjofXckKoiGL/gs32i1u5AV8eORKBXDivbt6twU/aRnUucCIhqrMx
RXUZad7OLTG2G/OLTrXs/pNjIl39DJYivzzh8+xLEYeL7EX0Tw85wwxxojLI/hvA
xXRayU7R1pA3C8666OyA65uy22/Os/TSWYB2VmHqeIlgAt5I5bUGLaV6sKhN81Ho
4QbBAh0Fs976EUdLwKhZsUQ5VqhJk8whB1mmoa9PtQw6/thiNV28BgCkM8DIxWOK
DXDVkVmlbmUj93eYNYxqYaTncpQqX1cBEIlO/Mz+xON959kjwYRCS/D8yKXm6rZ9
95HpztmcxBawjYhaErPtGwCGFGXBY4fs87Dx+8zLemgXRrNUPoWG/3lPfvBze+t2
Ws4P7/9hO8aiqV9MMb7gQLqnE1xTNV9m5cT17aHe8klKVR8a1igyF2e8+3V3lpfH
PZuNaT4xDaV9x0/SGvio
=IZeH
-----END PGP SIGNATURE-----
--=-=-=--