[PATCH 0/6] cli: improve handling of crypto parameters contexts

[PATCH 0/6] cli: improve handling of crypto parameters contexts

[Jameson Graef Rollins]

I wanted to see if I could hack in S/MIME support, but ultimately
failed (the interface is too stupid for me to deal with at the
moment).  However, on the way there I did manage to make some tangible
improvements to how crypto is handled in show and reply.

Most importantly I've moved the initialization of the gpg context to
mime_node.c where it is created lazily, only when needed.  This should
provide some speed up in notmuch show and reply when crypto flags are
provided but the messages have no crypto parts.

I was also able to get rid of a bunch of those pesky
"#ifdef GMIME_ATLEAST_26" conditionals.

This should provide a better framework for someone to try to hack in
S/MIME support at a later date if they're so inclined.

jamie.

[PATCH 1/6] cli: new crypto structure to store crypto contexts and parameters

[Jameson Graef Rollins]

The main point here is to keep track of the crypto stuff together in
one place.  In notmuch-show the crypto struct is a sub structure of
the parameters struct.  In notmuch-reply, which had been using a
notmuch_show_params_t to store the crypto parameters, we can now just
use the general crypto struct.

I slip in a name change of the crypto context itself to better reflect
what the context is specifically for: it's actually a GPG context,
which is a sub type of Crypto context.  There are other types of
Crypto contexts (Pkcs7 in particular) so we want to be clear.

The following patches will use this to simplify some function
interfaces.
---
 notmuch-client.h |   16 ++++++++++------
 notmuch-reply.c  |   34 +++++++++++++++++-----------------
 notmuch-show.c   |   22 +++++++++++-----------
 3 files changed, 38 insertions(+), 34 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 19b7f01..2ad24cf 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -74,17 +74,21 @@ typedef struct notmuch_show_format {
     const char *message_set_end;
 } notmuch_show_format_t;
 
+typedef struct notmuch_crypto {
+#ifdef GMIME_ATLEAST_26
+    GMimeCryptoContext* gpgctx;
+#else
+    GMimeCipherContext* gpgctx;
+#endif
+    notmuch_bool_t decrypt;
+} notmuch_crypto_t;
+
 typedef struct notmuch_show_params {
     notmuch_bool_t entire_thread;
     notmuch_bool_t omit_excluded;
     notmuch_bool_t raw;
     int part;
-#ifdef GMIME_ATLEAST_26
-    GMimeCryptoContext* cryptoctx;
-#else
-    GMimeCipherContext* cryptoctx;
-#endif
-    notmuch_bool_t decrypt;
+    notmuch_crypto_t crypto;
 } notmuch_show_params_t;
 
 /* There's no point in continuing when we've detected that we've done
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 7184a5d..ed87899 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -515,7 +515,7 @@ static int
 notmuch_reply_format_default(void *ctx,
 			     notmuch_config_t *config,
 			     notmuch_query_t *query,
-			     notmuch_show_params_t *params,
+			     notmuch_crypto_t *crypto,
 			     notmuch_bool_t reply_all)
 {
     GMimeMessage *reply;
@@ -544,7 +544,7 @@ notmuch_reply_format_default(void *ctx,
 	g_object_unref (G_OBJECT (reply));
 	reply = NULL;
 
-	if (mime_node_open (ctx, message, params->cryptoctx, params->decrypt,
+	if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
 			    &root) == NOTMUCH_STATUS_SUCCESS) {
 	    format_part_reply (root);
 	    talloc_free (root);
@@ -559,7 +559,7 @@ static int
 notmuch_reply_format_json(void *ctx,
 			  notmuch_config_t *config,
 			  notmuch_query_t *query,
-			  notmuch_show_params_t *params,
+			  notmuch_crypto_t *crypto,
 			  notmuch_bool_t reply_all)
 {
     GMimeMessage *reply;
@@ -574,7 +574,7 @@ notmuch_reply_format_json(void *ctx,
 
     messages = notmuch_query_search_messages (query);
     message = notmuch_messages_get (messages);
-    if (mime_node_open (ctx, message, params->cryptoctx, params->decrypt,
+    if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
 			&node) != NOTMUCH_STATUS_SUCCESS)
 	return 1;
 
@@ -605,7 +605,7 @@ static int
 notmuch_reply_format_headers_only(void *ctx,
 				  notmuch_config_t *config,
 				  notmuch_query_t *query,
-				  unused (notmuch_show_params_t *params),
+				  unused (notmuch_crypto_t *crypto),
 				  notmuch_bool_t reply_all)
 {
     GMimeMessage *reply;
@@ -674,8 +674,8 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
     notmuch_query_t *query;
     char *query_string;
     int opt_index, ret = 0;
-    int (*reply_format_func)(void *ctx, notmuch_config_t *config, notmuch_query_t *query, notmuch_show_params_t *params, notmuch_bool_t reply_all);
-    notmuch_show_params_t params = { .part = -1 };
+    int (*reply_format_func)(void *ctx, notmuch_config_t *config, notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
+    notmuch_crypto_t crypto = { .decrypt = FALSE };
     int format = FORMAT_DEFAULT;
     int reply_all = TRUE;
 
@@ -689,7 +689,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
 	  (notmuch_keyword_t []){ { "all", TRUE },
 				  { "sender", FALSE },
 				  { 0, 0 } } },
-	{ NOTMUCH_OPT_BOOLEAN, &params.decrypt, "decrypt", 'd', 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &crypto.decrypt, "decrypt", 'd', 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -706,18 +706,18 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
     else
 	reply_format_func = notmuch_reply_format_default;
 
-    if (params.decrypt) {
+    if (crypto.decrypt) {
 #ifdef GMIME_ATLEAST_26
 	/* TODO: GMimePasswordRequestFunc */
-	params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg");
+	crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg");
 #else
 	GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);
-	params.cryptoctx = g_mime_gpg_context_new (session, "gpg");
+	crypto.gpgctx = g_mime_gpg_context_new (session, "gpg");
 #endif
-	if (params.cryptoctx) {
-	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE);
+	if (crypto.gpgctx) {
+	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) crypto.gpgctx, FALSE);
 	} else {
-	    params.decrypt = FALSE;
+	    crypto.decrypt = FALSE;
 	    fprintf (stderr, "Failed to construct gpg context.\n");
 	}
 #ifndef GMIME_ATLEAST_26
@@ -750,14 +750,14 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
 	return 1;
     }
 
-    if (reply_format_func (ctx, config, query, &params, reply_all) != 0)
+    if (reply_format_func (ctx, config, query, &crypto, reply_all) != 0)
 	return 1;
 
     notmuch_query_destroy (query);
     notmuch_database_destroy (notmuch);
 
-    if (params.cryptoctx)
-	g_object_unref(params.cryptoctx);
+    if (crypto.gpgctx)
+	g_object_unref(crypto.gpgctx);
 
     return ret;
 }
diff --git a/notmuch-show.c b/notmuch-show.c
index 95427d4..d254179 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -810,8 +810,8 @@ show_message (void *ctx,
     mime_node_t *root, *part;
     notmuch_status_t status;
 
-    status = mime_node_open (local, message, params->cryptoctx,
-			     params->decrypt, &root);
+    status = mime_node_open (local, message, params->crypto.gpgctx,
+			     params->crypto.decrypt, &root);
     if (status)
 	goto DONE;
     part = mime_node_seek_dfs (root, (params->part < 0 ? 0 : params->part));
@@ -1002,7 +1002,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
                                   { 0, 0 } } },
 	{ NOTMUCH_OPT_INT, &params.part, "part", 'p', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &params.entire_thread, "entire-thread", 't', 0 },
-	{ NOTMUCH_OPT_BOOLEAN, &params.decrypt, "decrypt", 'd', 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &params.crypto.decrypt, "decrypt", 'd', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 },
 	{ 0, 0, 0, 0, 0 }
     };
@@ -1047,18 +1047,18 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 	break;
     }
 
-    if (params.decrypt || verify) {
+    if (params.crypto.decrypt || verify) {
 #ifdef GMIME_ATLEAST_26
 	/* TODO: GMimePasswordRequestFunc */
-	params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg");
+	params.crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg");
 #else
 	GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);
-	params.cryptoctx = g_mime_gpg_context_new (session, "gpg");
+	params.crypto.gpgctx = g_mime_gpg_context_new (session, "gpg");
 #endif
-	if (params.cryptoctx) {
-	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE);
+	if (params.crypto.gpgctx) {
+	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.crypto.gpgctx, FALSE);
 	} else {
-	    params.decrypt = FALSE;
+	    params.crypto.decrypt = FALSE;
 	    fprintf (stderr, "Failed to construct gpg context.\n");
 	}
 #ifndef GMIME_ATLEAST_26
@@ -1118,8 +1118,8 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     notmuch_query_destroy (query);
     notmuch_database_destroy (notmuch);
 
-    if (params.cryptoctx)
-	g_object_unref(params.cryptoctx);
+    if (params.crypto.gpgctx)
+	g_object_unref(params.crypto.gpgctx);
 
     return ret;
 }
-- 
1.7.10

[PATCH 2/6] cli: modify mime_node_context to use the new notmuch_crypto_t

[Jameson Graef Rollins]

This simplifies some more interfaces and gets rid of another #ifdef.
---
 mime-node.c |   23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index a95bdab..79a3654 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -33,12 +33,7 @@ typedef struct mime_node_context {
     GMimeMessage *mime_message;
 
     /* Context provided by the caller. */
-#ifdef GMIME_ATLEAST_26
-    GMimeCryptoContext *cryptoctx;
-#else
-    GMimeCipherContext *cryptoctx;
-#endif
-    notmuch_bool_t decrypt;
+    notmuch_crypto_t crypto;
 } mime_node_context_t;
 
 static int
@@ -118,8 +113,8 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
 	goto DONE;
     }
 
-    mctx->cryptoctx = cryptoctx;
-    mctx->decrypt = decrypt;
+    mctx->crypto.gpgctx = cryptoctx;
+    mctx->crypto.decrypt = decrypt;
 
     /* Create the root node */
     root->part = GMIME_OBJECT (mctx->mime_message);
@@ -195,7 +190,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 
     /* Handle PGP/MIME parts */
     if (GMIME_IS_MULTIPART_ENCRYPTED (part)
-	&& node->ctx->cryptoctx && node->ctx->decrypt) {
+	&& node->ctx->crypto.gpgctx && node->ctx->crypto.decrypt) {
 	if (node->nchildren != 2) {
 	    /* this violates RFC 3156 section 4, so we won't bother with it. */
 	    fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
@@ -208,10 +203,10 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 #ifdef GMIME_ATLEAST_26
 	    GMimeDecryptResult *decrypt_result = NULL;
 	    node->decrypted_child = g_mime_multipart_encrypted_decrypt
-		(encrypteddata, node->ctx->cryptoctx, &decrypt_result, &err);
+		(encrypteddata, node->ctx->crypto.gpgctx, &decrypt_result, &err);
 #else
 	    node->decrypted_child = g_mime_multipart_encrypted_decrypt
-		(encrypteddata, node->ctx->cryptoctx, &err);
+		(encrypteddata, node->ctx->crypto.gpgctx, &err);
 #endif
 	    if (node->decrypted_child) {
 		node->decrypt_success = node->verify_attempted = TRUE;
@@ -229,7 +224,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 			 (err ? err->message : "no error explanation given"));
 	    }
 	}
-    } else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->cryptoctx) {
+    } else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto.gpgctx) {
 	if (node->nchildren != 2) {
 	    /* this violates RFC 3156 section 5, so we won't bother with it. */
 	    fprintf (stderr, "Error: %d part(s) for a multipart/signed message "
@@ -238,7 +233,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 	} else {
 #ifdef GMIME_ATLEAST_26
 	    node->sig_list = g_mime_multipart_signed_verify
-		(GMIME_MULTIPART_SIGNED (part), node->ctx->cryptoctx, &err);
+		(GMIME_MULTIPART_SIGNED (part), node->ctx->crypto.gpgctx, &err);
 	    node->verify_attempted = TRUE;
 
 	    if (!node->sig_list)
@@ -254,7 +249,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 	     * In GMime 2.6, they're both non-const, so we'll be able
 	     * to clean up this asymmetry. */
 	    GMimeSignatureValidity *sig_validity = g_mime_multipart_signed_verify
-		(GMIME_MULTIPART_SIGNED (part), node->ctx->cryptoctx, &err);
+		(GMIME_MULTIPART_SIGNED (part), node->ctx->crypto.gpgctx, &err);
 	    node->verify_attempted = TRUE;
 	    node->sig_validity = sig_validity;
 	    if (sig_validity) {
-- 
1.7.10

[PATCH 3/6] cli: modify mime_node_open to take crypto struct as argument

[Jameson Graef Rollins]

Again, for interface simplification and getting rid of more #ifdefs.
---
 mime-node.c      |   10 ++--------
 notmuch-client.h |   14 +++++---------
 notmuch-reply.c  |    6 ++----
 notmuch-show.c   |    3 +--
 4 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 79a3654..4faeffc 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -56,12 +56,7 @@ _mime_node_context_free (mime_node_context_t *res)
 
 notmuch_status_t
 mime_node_open (const void *ctx, notmuch_message_t *message,
-#ifdef GMIME_ATLEAST_26
-		GMimeCryptoContext *cryptoctx,
-#else
-		GMimeCipherContext *cryptoctx,
-#endif
-		notmuch_bool_t decrypt, mime_node_t **root_out)
+		notmuch_crypto_t *crypto, mime_node_t **root_out)
 {
     const char *filename = notmuch_message_get_filename (message);
     mime_node_context_t *mctx;
@@ -113,8 +108,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
 	goto DONE;
     }
 
-    mctx->crypto.gpgctx = cryptoctx;
-    mctx->crypto.decrypt = decrypt;
+    mctx->crypto = *crypto;
 
     /* Create the root node */
     root->part = GMIME_OBJECT (mctx->mime_message);
diff --git a/notmuch-client.h b/notmuch-client.h
index 2ad24cf..d86fab3 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -345,9 +345,10 @@ struct mime_node {
 };
 
 /* Construct a new MIME node pointing to the root message part of
- * message.  If cryptoctx is non-NULL, it will be used to verify
- * signatures on any child parts.  If decrypt is true, then cryptoctx
- * will additionally be used to decrypt any encrypted child parts.
+ * message.  If crypto.gpgctx is non-NULL, it will be used to verify
+ * signatures on any child parts.  If crypto.decrypt is true, then
+ * crypto.gpgctx will additionally be used to decrypt any encrypted
+ * child parts.
  *
  * Return value:
  *
@@ -359,12 +360,7 @@ struct mime_node {
  */
 notmuch_status_t
 mime_node_open (const void *ctx, notmuch_message_t *message,
-#ifdef GMIME_ATLEAST_26
-		GMimeCryptoContext *cryptoctx,
-#else
-		GMimeCipherContext *cryptoctx,
-#endif
-		notmuch_bool_t decrypt, mime_node_t **node_out);
+		notmuch_crypto_t *crypto, mime_node_t **node_out);
 
 /* Return a new MIME node for the requested child part of parent.
  * parent will be used as the talloc context for the returned child
diff --git a/notmuch-reply.c b/notmuch-reply.c
index ed87899..6662adb 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -544,8 +544,7 @@ notmuch_reply_format_default(void *ctx,
 	g_object_unref (G_OBJECT (reply));
 	reply = NULL;
 
-	if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
-			    &root) == NOTMUCH_STATUS_SUCCESS) {
+	if (mime_node_open (ctx, message, crypto, &root) == NOTMUCH_STATUS_SUCCESS) {
 	    format_part_reply (root);
 	    talloc_free (root);
 	}
@@ -574,8 +573,7 @@ notmuch_reply_format_json(void *ctx,
 
     messages = notmuch_query_search_messages (query);
     message = notmuch_messages_get (messages);
-    if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
-			&node) != NOTMUCH_STATUS_SUCCESS)
+    if (mime_node_open (ctx, message, crypto, &node) != NOTMUCH_STATUS_SUCCESS)
 	return 1;
 
     reply = create_reply_message (ctx, config, message, reply_all);
diff --git a/notmuch-show.c b/notmuch-show.c
index d254179..8b4d308 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -810,8 +810,7 @@ show_message (void *ctx,
     mime_node_t *root, *part;
     notmuch_status_t status;
 
-    status = mime_node_open (local, message, params->crypto.gpgctx,
-			     params->crypto.decrypt, &root);
+    status = mime_node_open (local, message, &(params->crypto), &root);
     if (status)
 	goto DONE;
     part = mime_node_seek_dfs (root, (params->part < 0 ? 0 : params->part));
-- 
1.7.10

[PATCH 4/6] cli: intialize crypto structure in show and reply

[Jameson Graef Rollins]

This makes sure it has proper initialization values when it's created.
---
 notmuch-reply.c |    5 ++++-
 notmuch-show.c  |   10 +++++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index 6662adb..3c967a0 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -673,7 +673,10 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
     char *query_string;
     int opt_index, ret = 0;
     int (*reply_format_func)(void *ctx, notmuch_config_t *config, notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
-    notmuch_crypto_t crypto = { .decrypt = FALSE };
+    notmuch_crypto_t crypto = {
+	.decrypt = FALSE,
+	.gpgctx = NULL,
+    };
     int format = FORMAT_DEFAULT;
     int reply_all = TRUE;
 
diff --git a/notmuch-show.c b/notmuch-show.c
index 8b4d308..c606333 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -983,7 +983,15 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     char *query_string;
     int opt_index, ret;
     const notmuch_show_format_t *format = &format_text;
-    notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE };
+    notmuch_crypto_t crypto = {
+	.decrypt = FALSE,
+	.gpgctx = NULL,
+    };
+    notmuch_show_params_t params = {
+	.part = -1,
+	.omit_excluded = TRUE,
+	.crypto = crypto,
+    };
     int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
     notmuch_bool_t verify = FALSE;
     int exclude = EXCLUDE_TRUE;
-- 
1.7.10

[PATCH 5/6] cli: new crypto verify flag to handle verification

[Jameson Graef Rollins]

Use this flag rather than depend on the existence of an initialized
gpgctx, to determine whether we should verify a multipart/signed.  We
will be moving to create the ctx lazily, so we don't want to depend on
it being previously initialized if it's not needed.
---
 mime-node.c      |    5 ++---
 notmuch-client.h |    1 +
 notmuch-reply.c  |    1 +
 notmuch-show.c   |   14 +++++++++++---
 4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 4faeffc..8cdabc8 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -183,8 +183,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
     }
 
     /* Handle PGP/MIME parts */
-    if (GMIME_IS_MULTIPART_ENCRYPTED (part)
-	&& node->ctx->crypto.gpgctx && node->ctx->crypto.decrypt) {
+    if (GMIME_IS_MULTIPART_ENCRYPTED (part) && node->ctx->crypto.decrypt) {
 	if (node->nchildren != 2) {
 	    /* this violates RFC 3156 section 4, so we won't bother with it. */
 	    fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
@@ -218,7 +217,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 			 (err ? err->message : "no error explanation given"));
 	    }
 	}
-    } else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto.gpgctx) {
+    } else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto.verify) {
 	if (node->nchildren != 2) {
 	    /* this violates RFC 3156 section 5, so we won't bother with it. */
 	    fprintf (stderr, "Error: %d part(s) for a multipart/signed message "
diff --git a/notmuch-client.h b/notmuch-client.h
index d86fab3..1ca111f 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -80,6 +80,7 @@ typedef struct notmuch_crypto {
 #else
     GMimeCipherContext* gpgctx;
 #endif
+    notmuch_bool_t verify;
     notmuch_bool_t decrypt;
 } notmuch_crypto_t;
 
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 3c967a0..997fdd1 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -675,6 +675,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
     int (*reply_format_func)(void *ctx, notmuch_config_t *config, notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
     notmuch_crypto_t crypto = {
 	.decrypt = FALSE,
+	.verify = FALSE,
 	.gpgctx = NULL,
     };
     int format = FORMAT_DEFAULT;
diff --git a/notmuch-show.c b/notmuch-show.c
index c606333..99a10bd 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -985,6 +985,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     const notmuch_show_format_t *format = &format_text;
     notmuch_crypto_t crypto = {
 	.decrypt = FALSE,
+	.verify = FALSE,
 	.gpgctx = NULL,
     };
     notmuch_show_params_t params = {
@@ -993,7 +994,6 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 	.crypto = crypto,
     };
     int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
-    notmuch_bool_t verify = FALSE;
     int exclude = EXCLUDE_TRUE;
 
     notmuch_opt_desc_t options[] = {
@@ -1010,7 +1010,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 	{ NOTMUCH_OPT_INT, &params.part, "part", 'p', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &params.entire_thread, "entire-thread", 't', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &params.crypto.decrypt, "decrypt", 'd', 0 },
-	{ NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &params.crypto.verify, "verify", 'v', 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -1020,6 +1020,10 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 	return 1;
     }
 
+    /* decryption implies verification */
+    if (params.crypto.decrypt)
+	params.crypto.verify = TRUE;
+
     if (format_sel == NOTMUCH_FORMAT_NOT_SPECIFIED) {
 	/* if part was requested and format was not specified, use format=raw */
 	if (params.part >= 0)
@@ -1054,7 +1058,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 	break;
     }
 
-    if (params.crypto.decrypt || verify) {
+    if (params.crypto.decrypt || params.crypto.verify) {
 #ifdef GMIME_ATLEAST_26
 	/* TODO: GMimePasswordRequestFunc */
 	params.crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg");
@@ -1065,6 +1069,10 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 	if (params.crypto.gpgctx) {
 	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.crypto.gpgctx, FALSE);
 	} else {
+	    /* If we fail to create the gpgctx set the verify and
+	     * decrypt flags to FALSE so we don't try to do any
+	     * further verification or decryption */
+	    params.crypto.verify = FALSE;
 	    params.crypto.decrypt = FALSE;
 	    fprintf (stderr, "Failed to construct gpg context.\n");
 	}
-- 
1.7.10

[PATCH 6/6] cli: lazily create the crypto gpg context only when needed

[Jameson Graef Rollins]

Move the creation of the crypto ctx into mime-node.c and create it
only when needed.  This removes code duplication from notmuch-show and
notmuch-reply, and should speed up these functions considerably if the
crypto flags are provided but the messages don't have any
cryptographic parts.
---
 mime-node.c     |   25 +++++++++++++++++++++++++
 notmuch-reply.c |   19 -------------------
 notmuch-show.c  |   23 -----------------------
 3 files changed, 25 insertions(+), 42 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 8cdabc8..7278c74 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -182,6 +182,31 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 	return NULL;
     }
 
+    /* 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) {
+#ifdef GMIME_ATLEAST_26
+	    /* TODO: GMimePasswordRequestFunc */
+	    node->ctx->crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg");
+#else
+	    GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);
+	    node->ctx->crypto.gpgctx = g_mime_gpg_context_new (session, "gpg");
+	    g_object_unref (session);
+#endif
+	    if (node->ctx->crypto.gpgctx) {
+		g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) node->ctx->crypto.gpgctx, FALSE);
+	    } else {
+		/* If we fail to create the gpgctx set the verify and
+		 * decrypt flags to FALSE so we don't try to do any
+		 * further verification or decryption */
+		node->ctx->crypto.verify = FALSE;
+		node->ctx->crypto.decrypt = FALSE;
+		fprintf (stderr, "Failed to construct gpg context.\n");
+	    }
+	}
+    }
+
     /* Handle PGP/MIME parts */
     if (GMIME_IS_MULTIPART_ENCRYPTED (part) && node->ctx->crypto.decrypt) {
 	if (node->nchildren != 2) {
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 997fdd1..a56cf9f 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -708,25 +708,6 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
     else
 	reply_format_func = notmuch_reply_format_default;
 
-    if (crypto.decrypt) {
-#ifdef GMIME_ATLEAST_26
-	/* TODO: GMimePasswordRequestFunc */
-	crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg");
-#else
-	GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);
-	crypto.gpgctx = g_mime_gpg_context_new (session, "gpg");
-#endif
-	if (crypto.gpgctx) {
-	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) crypto.gpgctx, FALSE);
-	} else {
-	    crypto.decrypt = FALSE;
-	    fprintf (stderr, "Failed to construct gpg context.\n");
-	}
-#ifndef GMIME_ATLEAST_26
-	g_object_unref (session);
-#endif
-    }
-
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
 	return 1;
diff --git a/notmuch-show.c b/notmuch-show.c
index 99a10bd..0616f68 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1058,29 +1058,6 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 	break;
     }
 
-    if (params.crypto.decrypt || params.crypto.verify) {
-#ifdef GMIME_ATLEAST_26
-	/* TODO: GMimePasswordRequestFunc */
-	params.crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg");
-#else
-	GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);
-	params.crypto.gpgctx = g_mime_gpg_context_new (session, "gpg");
-#endif
-	if (params.crypto.gpgctx) {
-	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.crypto.gpgctx, FALSE);
-	} else {
-	    /* If we fail to create the gpgctx set the verify and
-	     * decrypt flags to FALSE so we don't try to do any
-	     * further verification or decryption */
-	    params.crypto.verify = FALSE;
-	    params.crypto.decrypt = FALSE;
-	    fprintf (stderr, "Failed to construct gpg context.\n");
-	}
-#ifndef GMIME_ATLEAST_26
-	g_object_unref (session);
-#endif
-    }
-
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
 	return 1;
-- 
1.7.10

Re: [PATCH 1/6] cli: new crypto structure to store crypto contexts and parameters

[Jani Nikula]

On Thu, 17 May 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> The main point here is to keep track of the crypto stuff together in
> one place.  In notmuch-show the crypto struct is a sub structure of
> the parameters struct.  In notmuch-reply, which had been using a
> notmuch_show_params_t to store the crypto parameters, we can now just
> use the general crypto struct.

Looks good. My only (potentially unwarranted) worry is dropping the use
of notmuch_show_params_t in reply. This diverges the show/reply code,
making any future unification of them slightly harder. And if reply ever
needs params, we'll need to bring it back. But perhaps I worry too
much. :)

BR,
Jani.


>
> I slip in a name change of the crypto context itself to better reflect
> what the context is specifically for: it's actually a GPG context,
> which is a sub type of Crypto context.  There are other types of
> Crypto contexts (Pkcs7 in particular) so we want to be clear.
>
> The following patches will use this to simplify some function
> interfaces.
> ---
>  notmuch-client.h |   16 ++++++++++------
>  notmuch-reply.c  |   34 +++++++++++++++++-----------------
>  notmuch-show.c   |   22 +++++++++++-----------
>  3 files changed, 38 insertions(+), 34 deletions(-)
>
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 19b7f01..2ad24cf 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -74,17 +74,21 @@ typedef struct notmuch_show_format {
>      const char *message_set_end;
>  } notmuch_show_format_t;
>  
> +typedef struct notmuch_crypto {
> +#ifdef GMIME_ATLEAST_26
> +    GMimeCryptoContext* gpgctx;
> +#else
> +    GMimeCipherContext* gpgctx;
> +#endif
> +    notmuch_bool_t decrypt;
> +} notmuch_crypto_t;
> +
>  typedef struct notmuch_show_params {
>      notmuch_bool_t entire_thread;
>      notmuch_bool_t omit_excluded;
>      notmuch_bool_t raw;
>      int part;
> -#ifdef GMIME_ATLEAST_26
> -    GMimeCryptoContext* cryptoctx;
> -#else
> -    GMimeCipherContext* cryptoctx;
> -#endif
> -    notmuch_bool_t decrypt;
> +    notmuch_crypto_t crypto;
>  } notmuch_show_params_t;
>  
>  /* There's no point in continuing when we've detected that we've done
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 7184a5d..ed87899 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -515,7 +515,7 @@ static int
>  notmuch_reply_format_default(void *ctx,
>  			     notmuch_config_t *config,
>  			     notmuch_query_t *query,
> -			     notmuch_show_params_t *params,
> +			     notmuch_crypto_t *crypto,
>  			     notmuch_bool_t reply_all)
>  {
>      GMimeMessage *reply;
> @@ -544,7 +544,7 @@ notmuch_reply_format_default(void *ctx,
>  	g_object_unref (G_OBJECT (reply));
>  	reply = NULL;
>  
> -	if (mime_node_open (ctx, message, params->cryptoctx, params->decrypt,
> +	if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
>  			    &root) == NOTMUCH_STATUS_SUCCESS) {
>  	    format_part_reply (root);
>  	    talloc_free (root);
> @@ -559,7 +559,7 @@ static int
>  notmuch_reply_format_json(void *ctx,
>  			  notmuch_config_t *config,
>  			  notmuch_query_t *query,
> -			  notmuch_show_params_t *params,
> +			  notmuch_crypto_t *crypto,
>  			  notmuch_bool_t reply_all)
>  {
>      GMimeMessage *reply;
> @@ -574,7 +574,7 @@ notmuch_reply_format_json(void *ctx,
>  
>      messages = notmuch_query_search_messages (query);
>      message = notmuch_messages_get (messages);
> -    if (mime_node_open (ctx, message, params->cryptoctx, params->decrypt,
> +    if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
>  			&node) != NOTMUCH_STATUS_SUCCESS)
>  	return 1;
>  
> @@ -605,7 +605,7 @@ static int
>  notmuch_reply_format_headers_only(void *ctx,
>  				  notmuch_config_t *config,
>  				  notmuch_query_t *query,
> -				  unused (notmuch_show_params_t *params),
> +				  unused (notmuch_crypto_t *crypto),
>  				  notmuch_bool_t reply_all)
>  {
>      GMimeMessage *reply;
> @@ -674,8 +674,8 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>      notmuch_query_t *query;
>      char *query_string;
>      int opt_index, ret = 0;
> -    int (*reply_format_func)(void *ctx, notmuch_config_t *config, notmuch_query_t *query, notmuch_show_params_t *params, notmuch_bool_t reply_all);
> -    notmuch_show_params_t params = { .part = -1 };
> +    int (*reply_format_func)(void *ctx, notmuch_config_t *config, notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
> +    notmuch_crypto_t crypto = { .decrypt = FALSE };
>      int format = FORMAT_DEFAULT;
>      int reply_all = TRUE;
>  
> @@ -689,7 +689,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>  	  (notmuch_keyword_t []){ { "all", TRUE },
>  				  { "sender", FALSE },
>  				  { 0, 0 } } },
> -	{ NOTMUCH_OPT_BOOLEAN, &params.decrypt, "decrypt", 'd', 0 },
> +	{ NOTMUCH_OPT_BOOLEAN, &crypto.decrypt, "decrypt", 'd', 0 },
>  	{ 0, 0, 0, 0, 0 }
>      };
>  
> @@ -706,18 +706,18 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>      else
>  	reply_format_func = notmuch_reply_format_default;
>  
> -    if (params.decrypt) {
> +    if (crypto.decrypt) {
>  #ifdef GMIME_ATLEAST_26
>  	/* TODO: GMimePasswordRequestFunc */
> -	params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg");
> +	crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg");
>  #else
>  	GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);
> -	params.cryptoctx = g_mime_gpg_context_new (session, "gpg");
> +	crypto.gpgctx = g_mime_gpg_context_new (session, "gpg");
>  #endif
> -	if (params.cryptoctx) {
> -	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE);
> +	if (crypto.gpgctx) {
> +	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) crypto.gpgctx, FALSE);
>  	} else {
> -	    params.decrypt = FALSE;
> +	    crypto.decrypt = FALSE;
>  	    fprintf (stderr, "Failed to construct gpg context.\n");
>  	}
>  #ifndef GMIME_ATLEAST_26
> @@ -750,14 +750,14 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>  	return 1;
>      }
>  
> -    if (reply_format_func (ctx, config, query, &params, reply_all) != 0)
> +    if (reply_format_func (ctx, config, query, &crypto, reply_all) != 0)
>  	return 1;
>  
>      notmuch_query_destroy (query);
>      notmuch_database_destroy (notmuch);
>  
> -    if (params.cryptoctx)
> -	g_object_unref(params.cryptoctx);
> +    if (crypto.gpgctx)
> +	g_object_unref(crypto.gpgctx);
>  
>      return ret;
>  }
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 95427d4..d254179 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -810,8 +810,8 @@ show_message (void *ctx,
>      mime_node_t *root, *part;
>      notmuch_status_t status;
>  
> -    status = mime_node_open (local, message, params->cryptoctx,
> -			     params->decrypt, &root);
> +    status = mime_node_open (local, message, params->crypto.gpgctx,
> +			     params->crypto.decrypt, &root);
>      if (status)
>  	goto DONE;
>      part = mime_node_seek_dfs (root, (params->part < 0 ? 0 : params->part));
> @@ -1002,7 +1002,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>                                    { 0, 0 } } },
>  	{ NOTMUCH_OPT_INT, &params.part, "part", 'p', 0 },
>  	{ NOTMUCH_OPT_BOOLEAN, &params.entire_thread, "entire-thread", 't', 0 },
> -	{ NOTMUCH_OPT_BOOLEAN, &params.decrypt, "decrypt", 'd', 0 },
> +	{ NOTMUCH_OPT_BOOLEAN, &params.crypto.decrypt, "decrypt", 'd', 0 },
>  	{ NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 },
>  	{ 0, 0, 0, 0, 0 }
>      };
> @@ -1047,18 +1047,18 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>  	break;
>      }
>  
> -    if (params.decrypt || verify) {
> +    if (params.crypto.decrypt || verify) {
>  #ifdef GMIME_ATLEAST_26
>  	/* TODO: GMimePasswordRequestFunc */
> -	params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg");
> +	params.crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg");
>  #else
>  	GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);
> -	params.cryptoctx = g_mime_gpg_context_new (session, "gpg");
> +	params.crypto.gpgctx = g_mime_gpg_context_new (session, "gpg");
>  #endif
> -	if (params.cryptoctx) {
> -	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE);
> +	if (params.crypto.gpgctx) {
> +	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.crypto.gpgctx, FALSE);
>  	} else {
> -	    params.decrypt = FALSE;
> +	    params.crypto.decrypt = FALSE;
>  	    fprintf (stderr, "Failed to construct gpg context.\n");
>  	}
>  #ifndef GMIME_ATLEAST_26
> @@ -1118,8 +1118,8 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>      notmuch_query_destroy (query);
>      notmuch_database_destroy (notmuch);
>  
> -    if (params.cryptoctx)
> -	g_object_unref(params.cryptoctx);
> +    if (params.crypto.gpgctx)
> +	g_object_unref(params.crypto.gpgctx);
>  
>      return ret;
>  }
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/6] cli: modify mime_node_context to use the new notmuch_crypto_t

[Jani Nikula]

On Thu, 17 May 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> This simplifies some more interfaces and gets rid of another #ifdef.

LGTM.

> ---
>  mime-node.c |   23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/mime-node.c b/mime-node.c
> index a95bdab..79a3654 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -33,12 +33,7 @@ typedef struct mime_node_context {
>      GMimeMessage *mime_message;
>  
>      /* Context provided by the caller. */
> -#ifdef GMIME_ATLEAST_26
> -    GMimeCryptoContext *cryptoctx;
> -#else
> -    GMimeCipherContext *cryptoctx;
> -#endif
> -    notmuch_bool_t decrypt;
> +    notmuch_crypto_t crypto;
>  } mime_node_context_t;
>  
>  static int
> @@ -118,8 +113,8 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
>  	goto DONE;
>      }
>  
> -    mctx->cryptoctx = cryptoctx;
> -    mctx->decrypt = decrypt;
> +    mctx->crypto.gpgctx = cryptoctx;
> +    mctx->crypto.decrypt = decrypt;
>  
>      /* Create the root node */
>      root->part = GMIME_OBJECT (mctx->mime_message);
> @@ -195,7 +190,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
>  
>      /* Handle PGP/MIME parts */
>      if (GMIME_IS_MULTIPART_ENCRYPTED (part)
> -	&& node->ctx->cryptoctx && node->ctx->decrypt) {
> +	&& node->ctx->crypto.gpgctx && node->ctx->crypto.decrypt) {
>  	if (node->nchildren != 2) {
>  	    /* this violates RFC 3156 section 4, so we won't bother with it. */
>  	    fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
> @@ -208,10 +203,10 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
>  #ifdef GMIME_ATLEAST_26
>  	    GMimeDecryptResult *decrypt_result = NULL;
>  	    node->decrypted_child = g_mime_multipart_encrypted_decrypt
> -		(encrypteddata, node->ctx->cryptoctx, &decrypt_result, &err);
> +		(encrypteddata, node->ctx->crypto.gpgctx, &decrypt_result, &err);
>  #else
>  	    node->decrypted_child = g_mime_multipart_encrypted_decrypt
> -		(encrypteddata, node->ctx->cryptoctx, &err);
> +		(encrypteddata, node->ctx->crypto.gpgctx, &err);
>  #endif
>  	    if (node->decrypted_child) {
>  		node->decrypt_success = node->verify_attempted = TRUE;
> @@ -229,7 +224,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
>  			 (err ? err->message : "no error explanation given"));
>  	    }
>  	}
> -    } else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->cryptoctx) {
> +    } else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto.gpgctx) {
>  	if (node->nchildren != 2) {
>  	    /* this violates RFC 3156 section 5, so we won't bother with it. */
>  	    fprintf (stderr, "Error: %d part(s) for a multipart/signed message "
> @@ -238,7 +233,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
>  	} else {
>  #ifdef GMIME_ATLEAST_26
>  	    node->sig_list = g_mime_multipart_signed_verify
> -		(GMIME_MULTIPART_SIGNED (part), node->ctx->cryptoctx, &err);
> +		(GMIME_MULTIPART_SIGNED (part), node->ctx->crypto.gpgctx, &err);
>  	    node->verify_attempted = TRUE;
>  
>  	    if (!node->sig_list)
> @@ -254,7 +249,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
>  	     * In GMime 2.6, they're both non-const, so we'll be able
>  	     * to clean up this asymmetry. */
>  	    GMimeSignatureValidity *sig_validity = g_mime_multipart_signed_verify
> -		(GMIME_MULTIPART_SIGNED (part), node->ctx->cryptoctx, &err);
> +		(GMIME_MULTIPART_SIGNED (part), node->ctx->crypto.gpgctx, &err);
>  	    node->verify_attempted = TRUE;
>  	    node->sig_validity = sig_validity;
>  	    if (sig_validity) {
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 3/6] cli: modify mime_node_open to take crypto struct as argument

[Jani Nikula]

On Thu, 17 May 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> Again, for interface simplification and getting rid of more #ifdefs.
> ---
>  mime-node.c      |   10 ++--------
>  notmuch-client.h |   14 +++++---------
>  notmuch-reply.c  |    6 ++----
>  notmuch-show.c   |    3 +--
>  4 files changed, 10 insertions(+), 23 deletions(-)
>
> diff --git a/mime-node.c b/mime-node.c
> index 79a3654..4faeffc 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -56,12 +56,7 @@ _mime_node_context_free (mime_node_context_t *res)
>  
>  notmuch_status_t
>  mime_node_open (const void *ctx, notmuch_message_t *message,
> -#ifdef GMIME_ATLEAST_26
> -		GMimeCryptoContext *cryptoctx,
> -#else
> -		GMimeCipherContext *cryptoctx,
> -#endif
> -		notmuch_bool_t decrypt, mime_node_t **root_out)
> +		notmuch_crypto_t *crypto, mime_node_t **root_out)
>  {
>      const char *filename = notmuch_message_get_filename (message);
>      mime_node_context_t *mctx;
> @@ -113,8 +108,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
>  	goto DONE;
>      }
>  
> -    mctx->crypto.gpgctx = cryptoctx;
> -    mctx->crypto.decrypt = decrypt;
> +    mctx->crypto = *crypto;
>  
>      /* Create the root node */
>      root->part = GMIME_OBJECT (mctx->mime_message);
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 2ad24cf..d86fab3 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -345,9 +345,10 @@ struct mime_node {
>  };
>  
>  /* Construct a new MIME node pointing to the root message part of
> - * message.  If cryptoctx is non-NULL, it will be used to verify
> - * signatures on any child parts.  If decrypt is true, then cryptoctx
> - * will additionally be used to decrypt any encrypted child parts.
> + * message.  If crypto.gpgctx is non-NULL, it will be used to verify
> + * signatures on any child parts.  If crypto.decrypt is true, then
> + * crypto.gpgctx will additionally be used to decrypt any encrypted
> + * child parts.
>   *
>   * Return value:
>   *
> @@ -359,12 +360,7 @@ struct mime_node {
>   */
>  notmuch_status_t
>  mime_node_open (const void *ctx, notmuch_message_t *message,
> -#ifdef GMIME_ATLEAST_26
> -		GMimeCryptoContext *cryptoctx,
> -#else
> -		GMimeCipherContext *cryptoctx,
> -#endif
> -		notmuch_bool_t decrypt, mime_node_t **node_out);
> +		notmuch_crypto_t *crypto, mime_node_t **node_out);
>  
>  /* Return a new MIME node for the requested child part of parent.
>   * parent will be used as the talloc context for the returned child
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index ed87899..6662adb 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -544,8 +544,7 @@ notmuch_reply_format_default(void *ctx,
>  	g_object_unref (G_OBJECT (reply));
>  	reply = NULL;
>  
> -	if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
> -			    &root) == NOTMUCH_STATUS_SUCCESS) {
> +	if (mime_node_open (ctx, message, crypto, &root) == NOTMUCH_STATUS_SUCCESS) {
>  	    format_part_reply (root);
>  	    talloc_free (root);
>  	}
> @@ -574,8 +573,7 @@ notmuch_reply_format_json(void *ctx,
>  
>      messages = notmuch_query_search_messages (query);
>      message = notmuch_messages_get (messages);
> -    if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
> -			&node) != NOTMUCH_STATUS_SUCCESS)
> +    if (mime_node_open (ctx, message, crypto, &node) != NOTMUCH_STATUS_SUCCESS)
>  	return 1;
>  
>      reply = create_reply_message (ctx, config, message, reply_all);
> diff --git a/notmuch-show.c b/notmuch-show.c
> index d254179..8b4d308 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -810,8 +810,7 @@ show_message (void *ctx,
>      mime_node_t *root, *part;
>      notmuch_status_t status;
>  
> -    status = mime_node_open (local, message, params->crypto.gpgctx,
> -			     params->crypto.decrypt, &root);
> +    status = mime_node_open (local, message, &(params->crypto), &root);

The parenthesis around params->crypto are not needed, otherwise LGTM.

>      if (status)
>  	goto DONE;
>      part = mime_node_seek_dfs (root, (params->part < 0 ? 0 : params->part));
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jani Nikula]

On Thu, 17 May 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> This makes sure it has proper initialization values when it's created.

Please don't do this. It's unnecessary; if one field is initialized with
a designated initializer, the rest are initialized to zero (or NULL).

BR,
Jani.


> ---
>  notmuch-reply.c |    5 ++++-
>  notmuch-show.c  |   10 +++++++++-
>  2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 6662adb..3c967a0 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -673,7 +673,10 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>      char *query_string;
>      int opt_index, ret = 0;
>      int (*reply_format_func)(void *ctx, notmuch_config_t *config, notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
> -    notmuch_crypto_t crypto = { .decrypt = FALSE };
> +    notmuch_crypto_t crypto = {
> +	.decrypt = FALSE,
> +	.gpgctx = NULL,
> +    };
>      int format = FORMAT_DEFAULT;
>      int reply_all = TRUE;
>  
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 8b4d308..c606333 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -983,7 +983,15 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>      char *query_string;
>      int opt_index, ret;
>      const notmuch_show_format_t *format = &format_text;
> -    notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE };
> +    notmuch_crypto_t crypto = {
> +	.decrypt = FALSE,
> +	.gpgctx = NULL,
> +    };
> +    notmuch_show_params_t params = {
> +	.part = -1,
> +	.omit_excluded = TRUE,
> +	.crypto = crypto,
> +    };
>      int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
>      notmuch_bool_t verify = FALSE;
>      int exclude = EXCLUDE_TRUE;
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

On Thu, May 17 2012, Jani Nikula <jani@nikula.org> wrote:
> On Thu, 17 May 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
>> This makes sure it has proper initialization values when it's created.
>
> Please don't do this. It's unnecessary; if one field is initialized with
> a designated initializer, the rest are initialized to zero (or NULL).

It may be technically unnecessary, but why is that a reason to not do
it?  I intentionally did this to make it clear what the defaults are.
Otherwise the defaults are essentially undefined, which is not good.
Maybe the structure initializes to the correct defaults, but why count
on that when we can set them to the correct default, and have it clear
to readers of the code?

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jani Nikula]

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

On May 17, 2012 5:26 PM, "Jameson Graef Rollins" <jrollins@finestructure.net>
wrote:
>
> On Thu, May 17 2012, Jani Nikula <jani@nikula.org> wrote:
> > On Thu, 17 May 2012, Jameson Graef Rollins <jrollins@finestructure.net>
wrote:
> >> This makes sure it has proper initialization values when it's created.
> >
> > Please don't do this. It's unnecessary; if one field is initialized with
> > a designated initializer, the rest are initialized to zero (or NULL).
>
> It may be technically unnecessary, but why is that a reason to not do
> it?  I intentionally did this to make it clear what the defaults are.
> Otherwise the defaults are essentially undefined, which is not good.
> Maybe the structure initializes to the correct defaults, but why count
> on that when we can set them to the correct default, and have it clear
> to readers of the code?

The values are not undefined, they are properly initialized, and we can
count on it. For sure, not maybe. If you want to explicitly set them for
clarity, it's a matter of taste. Personally I find it too verbose, but then
again notmuch code is generally fairly verbose. If you insist on it, please
at least drop the extra temp crypto variable, and initialize the struct in
one initializer.

BR,
Jani.

>
> jamie.

[-- Attachment #2: Type: text/html, Size: 1634 bytes --]

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]

On Thu, May 17 2012, Jani Nikula <jani@nikula.org> wrote:
> The values are not undefined, they are properly initialized, and we can
> count on it. For sure, not maybe. If you want to explicitly set them for
> clarity, it's a matter of taste. Personally I find it too verbose, but then
> again notmuch code is generally fairly verbose.

I want them explicitly set for clarity, as well as safety.  Code is
meant to be read by humans, not computers.  Brevity is not always a
virtue if it sacrifices clarity.  It's much nicer to have the defaults
clearly stated in the initialization, than to force the reader to
understand how the initialization works and to interpret what that means
for the current case.  I also don't think it's safe to assume that the
variables will be always be "properly" initialized in your favor in
perpetuity.  It's much safer to explicitly set them to what you want
them to be rather than just assume they'll be set correctly.

> If you insist on it, please at least drop the extra temp crypto
> variable, and initialize the struct in one initializer.

I don't see why this matters either.  Again, I think this is just a
matter of taste.  I would rather the code be verbose where clarity
requires it, rather than always trying to make the code as terse as
possible.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jani Nikula]

On Thu, 17 May 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Thu, May 17 2012, Jani Nikula <jani@nikula.org> wrote:
>> The values are not undefined, they are properly initialized, and we can
>> count on it. For sure, not maybe. If you want to explicitly set them for
>> clarity, it's a matter of taste. Personally I find it too verbose, but then
>> again notmuch code is generally fairly verbose.
>
> I want them explicitly set for clarity, as well as safety.  Code is
> meant to be read by humans, not computers.  Brevity is not always a
> virtue if it sacrifices clarity.  It's much nicer to have the defaults
> clearly stated in the initialization, than to force the reader to
> understand how the initialization works and to interpret what that means
> for the current case.  I also don't think it's safe to assume that the
> variables will be always be "properly" initialized in your favor in
> perpetuity.  It's much safer to explicitly set them to what you want
> them to be rather than just assume they'll be set correctly.

In short, when you read enough code, having everything explicitly stated
becomes a burden. It's explicit, but you have to read it all, even when
there really is no need to.

>> If you insist on it, please at least drop the extra temp crypto
>> variable, and initialize the struct in one initializer.
>
> I don't see why this matters either.  Again, I think this is just a
> matter of taste.  I would rather the code be verbose where clarity
> requires it, rather than always trying to make the code as terse as
> possible.

You introduce an extra variable that every reader of your code has to
track down to realize that it's only ever used once to initialize
another variable. You make code harder for other people to read.

I have now offered my review and opinions on the matter; I will not
pursue this discussion further.


BR,
Jani.

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]

On Thu, May 17 2012, Jani Nikula <jani@nikula.org> wrote:
> In short, when you read enough code, having everything explicitly
> stated becomes a burden. It's explicit, but you have to read it all,
> even when there really is no need to.

Not everyone who reads the code is an expert.  I think it's important
for the code to be clear to everyone.  Experts can more easily gloss
over the trivial parts than novices can divine things that are hidden.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 2589 bytes --]

On 05/17/2012 12:45 PM, Jameson Graef Rollins wrote:
> I want them explicitly set for clarity, as well as safety.  Code is
> meant to be read by humans, not computers.  

I sympathize with this sentiment.

> It's much safer to explicitly set them to what you want
> them to be rather than just assume they'll be set correctly.

I don't think it's an assumption -- Jani is probably relying on the C
standard. Consider, for example, C99 [0]'s section 6.7.8.19, which says:

  all subobjects that are not initialized explicitly shall be
  initialized implicitly the same as objects that have static
  storage duration.

the latter clause references 6.7.8.10, which says:

   If an object that has static storage duration is not
   initialized explicitly, then:
     — if it has pointer type, it is initialized to a null pointer;
     — if it has arithmetic type, it is initialized to (positive or
       unsigned) zero;
     — if it is an aggregate, every member is initialized
       (recursively) according to these rules;

So it's not just "an assumption", it's a guarantee from the underlying
language standard.

That said, it's a guarantee i was unaware of until i researched this.
I'm certainly not a C guru, but i've internalized a fair amount of C's
rules and structure and i'd never heard of this
subobject-default-initialization-when-other-subobjects-are-initialized
rule before.   If i'd seen the uninitialized members of the struct, and
that they were being used without explicit initialization, i would have
had to do a bit of digging to understand what's happening.

The real tradeoff in this choice is whether we prefer:

 a) more compact code to facilitate quick reading by experts

   or

 b) more verbose code to facilitate comprehension by the non-expert.

I started this discussion leaning strongly toward the (b) perspective.
But now that i know the relevant bits of the standard, i can sympathize
with the (a) perspective as well :P

Overall, i think i'm still in the (b) camp.  But i think it's more
important that we don't allow dithering over this issue to prevent the
inclusion of this patch series, which is a step in the right direction
for handling S/MIME messages as well as PGP/MIME.

	--dkg

PS gcc's -pedantic argument provides the following warning:

 error: ISO C90 forbids specifying subobject to initialize

So we probably want to specify -std=c99 at least to ensure our choice of
subobject initialization is respected.

[0] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 1030 bytes --]

Re: [PATCH 3/6] cli: modify mime_node_open to take crypto struct as argument

[Austin Clements]

Quoth Jameson Graef Rollins on May 16 at  2:55 pm:
> Again, for interface simplification and getting rid of more #ifdefs.
> ---
>  mime-node.c      |   10 ++--------
>  notmuch-client.h |   14 +++++---------
>  notmuch-reply.c  |    6 ++----
>  notmuch-show.c   |    3 +--
>  4 files changed, 10 insertions(+), 23 deletions(-)
> 
> diff --git a/mime-node.c b/mime-node.c
> index 79a3654..4faeffc 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -56,12 +56,7 @@ _mime_node_context_free (mime_node_context_t *res)
>  
>  notmuch_status_t
>  mime_node_open (const void *ctx, notmuch_message_t *message,
> -#ifdef GMIME_ATLEAST_26
> -		GMimeCryptoContext *cryptoctx,
> -#else
> -		GMimeCipherContext *cryptoctx,
> -#endif
> -		notmuch_bool_t decrypt, mime_node_t **root_out)
> +		notmuch_crypto_t *crypto, mime_node_t **root_out)
>  {
>      const char *filename = notmuch_message_get_filename (message);
>      mime_node_context_t *mctx;
> @@ -113,8 +108,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
>  	goto DONE;
>      }
>  
> -    mctx->crypto.gpgctx = cryptoctx;
> -    mctx->crypto.decrypt = decrypt;
> +    mctx->crypto = *crypto;

I think you want to store the notmuch_crypto_t* pointer in the
mime_node_t, rather than copying the value.  This doesn't matter so
much at this point, but when you later start lazily constructing the
cyrpto context, storing it by value will force you to lazily
initialize it separately potentially for every mime_node_t instance.

>  
>      /* Create the root node */
>      root->part = GMIME_OBJECT (mctx->mime_message);
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 2ad24cf..d86fab3 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -345,9 +345,10 @@ struct mime_node {
>  };
>  
>  /* Construct a new MIME node pointing to the root message part of
> - * message.  If cryptoctx is non-NULL, it will be used to verify
> - * signatures on any child parts.  If decrypt is true, then cryptoctx
> - * will additionally be used to decrypt any encrypted child parts.
> + * message.  If crypto.gpgctx is non-NULL, it will be used to verify
> + * signatures on any child parts.  If crypto.decrypt is true, then
> + * crypto.gpgctx will additionally be used to decrypt any encrypted
> + * child parts.
>   *
>   * Return value:
>   *
> @@ -359,12 +360,7 @@ struct mime_node {
>   */
>  notmuch_status_t
>  mime_node_open (const void *ctx, notmuch_message_t *message,
> -#ifdef GMIME_ATLEAST_26
> -		GMimeCryptoContext *cryptoctx,
> -#else
> -		GMimeCipherContext *cryptoctx,
> -#endif
> -		notmuch_bool_t decrypt, mime_node_t **node_out);
> +		notmuch_crypto_t *crypto, mime_node_t **node_out);
>  
>  /* Return a new MIME node for the requested child part of parent.
>   * parent will be used as the talloc context for the returned child
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index ed87899..6662adb 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -544,8 +544,7 @@ notmuch_reply_format_default(void *ctx,
>  	g_object_unref (G_OBJECT (reply));
>  	reply = NULL;
>  
> -	if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
> -			    &root) == NOTMUCH_STATUS_SUCCESS) {
> +	if (mime_node_open (ctx, message, crypto, &root) == NOTMUCH_STATUS_SUCCESS) {
>  	    format_part_reply (root);
>  	    talloc_free (root);
>  	}
> @@ -574,8 +573,7 @@ notmuch_reply_format_json(void *ctx,
>  
>      messages = notmuch_query_search_messages (query);
>      message = notmuch_messages_get (messages);
> -    if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
> -			&node) != NOTMUCH_STATUS_SUCCESS)
> +    if (mime_node_open (ctx, message, crypto, &node) != NOTMUCH_STATUS_SUCCESS)
>  	return 1;
>  
>      reply = create_reply_message (ctx, config, message, reply_all);
> diff --git a/notmuch-show.c b/notmuch-show.c
> index d254179..8b4d308 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -810,8 +810,7 @@ show_message (void *ctx,
>      mime_node_t *root, *part;
>      notmuch_status_t status;
>  
> -    status = mime_node_open (local, message, params->crypto.gpgctx,
> -			     params->crypto.decrypt, &root);
> +    status = mime_node_open (local, message, &(params->crypto), &root);
>      if (status)
>  	goto DONE;
>      part = mime_node_seek_dfs (root, (params->part < 0 ? 0 : params->part));

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Austin Clements]

Quoth Jameson Graef Rollins on May 16 at  2:55 pm:
> This makes sure it has proper initialization values when it's created.
> ---
>  notmuch-reply.c |    5 ++++-
>  notmuch-show.c  |   10 +++++++++-
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 6662adb..3c967a0 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -673,7 +673,10 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>      char *query_string;
>      int opt_index, ret = 0;
>      int (*reply_format_func)(void *ctx, notmuch_config_t *config, notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
> -    notmuch_crypto_t crypto = { .decrypt = FALSE };
> +    notmuch_crypto_t crypto = {
> +	.decrypt = FALSE,
> +	.gpgctx = NULL,
> +    };
>      int format = FORMAT_DEFAULT;
>      int reply_all = TRUE;
>  
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 8b4d308..c606333 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -983,7 +983,15 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>      char *query_string;
>      int opt_index, ret;
>      const notmuch_show_format_t *format = &format_text;
> -    notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE };
> +    notmuch_crypto_t crypto = {
> +	.decrypt = FALSE,
> +	.gpgctx = NULL,
> +    };
> +    notmuch_show_params_t params = {
> +	.part = -1,
> +	.omit_excluded = TRUE,
> +	.crypto = crypto,
> +    };

You can omit the temporary variable and avoid the struct copy by doing
something like this:

notmuch_show_params_t params = {
    .part = -1,
    .omit_excluded = TRUE,
    .crypto = {
	.decrypt = FALSE,
	.gpgctx = NULL,
    },
};

>      int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
>      notmuch_bool_t verify = FALSE;
>      int exclude = EXCLUDE_TRUE;

Re: [PATCH 6/6] cli: lazily create the crypto gpg context only when needed

[Austin Clements]

Quoth Jameson Graef Rollins on May 16 at  2:55 pm:
> Move the creation of the crypto ctx into mime-node.c and create it
> only when needed.  This removes code duplication from notmuch-show and
> notmuch-reply, and should speed up these functions considerably if the
> crypto flags are provided but the messages don't have any
> cryptographic parts.
> ---
>  mime-node.c     |   25 +++++++++++++++++++++++++
>  notmuch-reply.c |   19 -------------------
>  notmuch-show.c  |   23 -----------------------
>  3 files changed, 25 insertions(+), 42 deletions(-)
> 
> diff --git a/mime-node.c b/mime-node.c
> index 8cdabc8..7278c74 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -182,6 +182,31 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)

This patch should also update the documentation comment for
mime_node_open.

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Tomi Ollila]

On Fri, May 18 2012, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:

>
> The real tradeoff in this choice is whether we prefer:
>
>  a) more compact code to facilitate quick reading by experts
>
>    or
>
>  b) more verbose code to facilitate comprehension by the non-expert.
>
> I started this discussion leaning strongly toward the (b) perspective.
> But now that i know the relevant bits of the standard, i can sympathize
> with the (a) perspective as well :P
>
> Overall, i think i'm still in the (b) camp.  But i think it's more
> important that we don't allow dithering over this issue to prevent the
> inclusion of this patch series, which is a step in the right direction
> for handling S/MIME messages as well as PGP/MIME.

I also think it is good to see explicit initializations when those aren't
needed but clarifies the code. After all it doesn't generate any extra
code to the target module. Also the &(params->crypto) is good from
clarification point of view.

Austin's .crypto { ... } initialization looks good & clear; In case there
will be new version of this patch series I'd like to see that used...

> 	--dkg
>
> PS gcc's -pedantic argument provides the following warning:
>
>  error: ISO C90 forbids specifying subobject to initialize
>
> So we probably want to specify -std=c99 at least to ensure our choice of
> subobject initialization is respected.

In order to do that id:"cover.1325977940.git.jani@nikula.org" needs to
be applied (and probably rebased).

> [0] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf

Tomi

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jani Nikula]

*sigh* I'm failing to detach myself from this conversation. :(

On Thu, 17 May 2012, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
> I don't think it's an assumption -- Jani is probably relying on the C
> standard. Consider, for example, C99 [0]'s section 6.7.8.19, which says:

Thanks for digging up the references.

> The real tradeoff in this choice is whether we prefer:
>
>  a) more compact code to facilitate quick reading by experts
>
>    or
>
>  b) more verbose code to facilitate comprehension by the non-expert.

We have -Wextra, which enables -Wmissing-field-initializers, which
requires us to use full initialization of struct fields when doing
regular, non-designated initialization. The point is that you might
introduce subtle bugs if you added new struct fields and forgot to check
the initializations. (This is why we have e.g. { 0, 0, 0, 0, 0 } instead
of just { 0 } in the initialization of notmuch_opt_desc_t arrays.)

IMHO the whole point of designated initializers is that the
initialization is not vulnerable to struct changes, and you can pick
which fields you choose to initialize explicitly. Also, it has the added
benefit of documenting the fields that are initialized, without having
to look at the struct definition.

Do we now want to initialize all struct fields explicitly, everywhere,
even when using designated initializers? Isn't that the question then?
Won't that maintain and promote the misconception that explicit
initialization is required, when it's really not, failing to educate the
non-experts and planting a seed of doubt in the experts...?

> I started this discussion leaning strongly toward the (b) perspective.
> But now that i know the relevant bits of the standard, i can sympathize
> with the (a) perspective as well :P

It's not always clear whether something is a matter of taste, style, or
language paradigm. If it feels like a paradigm, sticking with it
ultimately benefits *both* perspectives.

> Overall, i think i'm still in the (b) camp.  But i think it's more
> important that we don't allow dithering over this issue to prevent the
> inclusion of this patch series, which is a step in the right direction
> for handling S/MIME messages as well as PGP/MIME.

Agreed.

> PS gcc's -pedantic argument provides the following warning:
>
>  error: ISO C90 forbids specifying subobject to initialize
>
> So we probably want to specify -std=c99 at least to ensure our choice of
> subobject initialization is respected.

Unfortunately, the notmuch code base uses mixed standards, due to GCC
being so lax about it. Anything -pedantic produces warnings:
id:"cover.1325977940.git.jani@nikula.org". You may also want to try
clang to get better warnings.


BR,
Jani.

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 2087 bytes --]

On 05/18/2012 04:20 AM, Jani Nikula wrote:
> We have -Wextra, which enables -Wmissing-field-initializers, which
> requires us to use full initialization of struct fields when doing
> regular, non-designated initialization. The point is that you might
> introduce subtle bugs if you added new struct fields and forgot to check
> the initializations. (This is why we have e.g. { 0, 0, 0, 0, 0 } instead
> of just { 0 } in the initialization of notmuch_opt_desc_t arrays.)

i think we can agree that this is the right choice.  We might even want
to discourage non-designated initializations entirely.

> IMHO the whole point of designated initializers is that the
> initialization is not vulnerable to struct changes, and you can pick
> which fields you choose to initialize explicitly. Also, it has the added
> benefit of documenting the fields that are initialized, without having
> to look at the struct definition.

Agreed.

> Do we now want to initialize all struct fields explicitly, everywhere,
> even when using designated initializers? Isn't that the question then?

I'm not sure it has to be this dramatic and "all or nothing".  For
example, it could be reasonable to explicitly initialize some subobjects
and not others.  For example, the notmuch_crypto_t jamie is proposing
would effectively encode the default setting for the --verify and
--decrypt flags.  I could see wanting to explicitly initialize those
default policy choices, even if they happen to be identical to the
implicit "zero"ing.

> Won't that maintain and promote the misconception that explicit
> initialization is required, when it's really not, failing to educate the
> non-experts and planting a seed of doubt in the experts...?

i see your point here, which is why i'm not arguing that all subobjects
need to be explicitly initialized all the time.

> It's not always clear whether something is a matter of taste, style, or
> language paradigm. If it feels like a paradigm, sticking with it
> ultimately benefits *both* perspectives.

yep, understood.

	--dkg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 1030 bytes --]