[PATCH 0/4] First step of 'show' rewrite

[PATCH 0/4] First step of 'show' rewrite

[Austin Clements]

This is the first step in my rewrite of notmuch-show.  Currently, MIME
traversal is controlled by show_message_body, which invokes formatter
callbacks.  As a result, formatters have no control over traversal,
which complicates formatters and makes others (like raw) nigh
impossible to implement correctly.  The callback-driven approach
forces formatters to be written in many small pieces and masks the
true control flow.  It also results in an wide interface with
show_message_body that has been getting wider over time.

The goal of this rewrite is to reverse this relationship, so that
formatters control MIME traversal.  This lets formatters traverse the
MIME in the most appropriate way, eliminates the callbacks, makes the
control flow through a formatter clear, and dramatically narrows
interfaces.

This first series doesn't change the way formatters work, but it does
introduce the MIME-traversal API they'll use.  This API consists of a
total of two functions: one to get the root MIME part of a message and
one to get a child of a MIME part.  This allows basic MIME traversal
to be implemented in a simple, two-line for loop.  The series also
updates show_message_body to use this new API, though ultimately
show_message_body will go away entirely.

[PATCH 1/4] show: Pass notmuch_message_t instead of path to show_message_body.

[Austin Clements]

In addition to simplifying the code, we'll need the notmuch_message_t*
in show_message_body shortly.
---
 notmuch-client.h |    2 +-
 notmuch-reply.c  |    3 +--
 notmuch-show.c   |    3 +--
 show-message.c   |    3 ++-
 4 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index b50cb38..d7fb6ee 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -162,7 +162,7 @@ char *
 query_string_from_args (void *ctx, int argc, char *argv[]);
 
 notmuch_status_t
-show_message_body (const char *filename,
+show_message_body (notmuch_message_t *message,
 		   const notmuch_show_format_t *format,
 		   notmuch_show_params_t *params);
 
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 7ac879f..f8d5f64 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -546,8 +546,7 @@ notmuch_reply_format_default(void *ctx,
 		notmuch_message_get_header (message, "date"),
 		notmuch_message_get_header (message, "from"));
 
-	show_message_body (notmuch_message_get_filename (message),
-			   format, params);
+	show_message_body (message, format, params);
 
 	notmuch_message_destroy (message);
     }
diff --git a/notmuch-show.c b/notmuch-show.c
index 603992a..1dee3aa 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -753,8 +753,7 @@ show_message (void *ctx,
     }
 
     if (format->part_content)
-	show_message_body (notmuch_message_get_filename (message),
-			   format, params);
+	show_message_body (message, format, params);
 
     if (params->part <= 0) {
 	fputs (format->body_end, stdout);
diff --git a/show-message.c b/show-message.c
index d83f04e..09fa607 100644
--- a/show-message.c
+++ b/show-message.c
@@ -175,7 +175,7 @@ show_message_part (GMimeObject *part,
 }
 
 notmuch_status_t
-show_message_body (const char *filename,
+show_message_body (notmuch_message_t *message,
 		   const notmuch_show_format_t *format,
 		   notmuch_show_params_t *params)
 {
@@ -183,6 +183,7 @@ show_message_body (const char *filename,
     GMimeParser *parser = NULL;
     GMimeMessage *mime_message = NULL;
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
+    const char *filename = notmuch_message_get_filename (message);
     FILE *file = NULL;
     show_message_state_t state;
 
-- 
1.7.5.4

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

This wraps all of the complex MIME part handling in a single, simple
function that gets part N from *any* MIME object, so traversing a MIME
part tree becomes a two-line for loop.  Furthermore, the MIME node
structure provides easy access to envelopes for message parts as well
as cryptographic information.

This code is directly derived from the current show_message_body code
(much of it is identical), but the control relation is inverted:
instead of show_message_body controlling the traversal of the MIME
structure and invoking callbacks, the caller controls the traversal of
the MIME structure.
---
 Makefile.local   |    1 +
 mime-node.c      |  234 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 notmuch-client.h |   80 ++++++++++++++++++
 3 files changed, 315 insertions(+), 0 deletions(-)
 create mode 100644 mime-node.c

diff --git a/Makefile.local b/Makefile.local
index c94402b..c46ed26 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -312,6 +312,7 @@ notmuch_client_srcs =		\
 	notmuch-time.c		\
 	query-string.c		\
 	show-message.c		\
+	mime-node.c		\
 	json.c
 
 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
diff --git a/mime-node.c b/mime-node.c
new file mode 100644
index 0000000..942738b
--- /dev/null
+++ b/mime-node.c
@@ -0,0 +1,234 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2009 Carl Worth
+ * Copyright © 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth <cworth@cworth.org>
+ *          Keith Packard <keithp@keithp.com>
+ *          Austin Clements <aclements@csail.mit.edu>
+ */
+
+#include "notmuch-client.h"
+
+/* Context that gets inherited from the root node. */
+typedef struct mime_node_context {
+    /* Per-message resources.  These are allocated internally and must
+     * be destroyed. */
+    FILE *file;
+    GMimeStream *stream;
+    GMimeParser *parser;
+    GMimeMessage *mime_message;
+    
+    /* Context provided by the caller. */
+    GMimeCipherContext *cryptoctx;
+    notmuch_bool_t decrypt;
+} mime_node_context_t;
+
+static int
+_mime_node_context_free (mime_node_context_t *res)
+{
+    if (res->mime_message)
+	g_object_unref (res->mime_message);
+
+    if (res->parser)
+	g_object_unref (res->parser);
+
+    if (res->stream)
+	g_object_unref (res->stream);
+
+    if (res->file)
+	fclose (res->file);
+
+    return 0;
+}
+
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+		GMimeCipherContext* cryptoctx, notmuch_bool_t decrypt,
+		mime_node_t **root_out)
+{
+    const char *filename = notmuch_message_get_filename (message);
+    mime_node_context_t *mctx;
+    mime_node_t *root = NULL;
+    notmuch_status_t status;
+
+    root = talloc_zero (ctx, mime_node_t);
+    if (root == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    /* Create the tree-wide context */
+    mctx = talloc_zero (root, mime_node_context_t);
+    if (mctx == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+    talloc_set_destructor (mctx, _mime_node_context_free);
+
+    mctx->file = fopen (filename, "r");
+    if (! mctx->file) {
+	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
+	status = NOTMUCH_STATUS_FILE_ERROR;
+	goto DONE;
+    }
+
+    mctx->stream = g_mime_stream_file_new (mctx->file);
+    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx->stream), FALSE);
+
+    mctx->parser = g_mime_parser_new_with_stream (mctx->stream);
+
+    mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
+
+    mctx->cryptoctx = cryptoctx;
+    mctx->decrypt = decrypt;
+
+    /* Create the root node */
+    root->part = GMIME_OBJECT (mctx->mime_message);
+    root->envelope_file = message;
+    root->children = 1;
+    root->ctx = mctx;
+
+    *root_out = root;
+    return NOTMUCH_STATUS_SUCCESS;
+
+DONE:
+    talloc_free (root);
+    return status;
+}
+
+static int
+_signature_validity_free (GMimeSignatureValidity **proxy)
+{
+    g_mime_signature_validity_free (*proxy);
+    return 0;
+}
+
+static mime_node_t *
+_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+{
+    mime_node_t *out = talloc_zero (parent, mime_node_t);
+    GError *err = NULL;
+
+    /* Set basic node properties */
+    out->part = part;
+    out->ctx = parent->ctx;
+    if (!talloc_reference (out, out->ctx)) {
+	fprintf (stderr, "Out of memory.\n");
+	talloc_free (out);
+	return NULL;
+    }
+
+    /* Deal with the different types of parts */
+    if (GMIME_IS_PART (part)) {
+	out->children = 0;
+    } else if (GMIME_IS_MULTIPART (part)) {
+	out->children = g_mime_multipart_get_count (GMIME_MULTIPART (part));
+    } else if (GMIME_IS_MESSAGE_PART (part)) {
+	/* Promote part to an envelope and open it */
+	GMimeMessagePart *message_part = GMIME_MESSAGE_PART (part);
+	GMimeMessage *message = g_mime_message_part_get_message (message_part);
+	out->envelope_part = message_part;
+	out->part = GMIME_OBJECT (message);
+	out->children = 1;
+    } else {
+	fprintf (stderr, "Warning: Unknown mime part type: %s.\n",
+		 g_type_name (G_OBJECT_TYPE (part)));
+	talloc_free (out);
+	return NULL;
+    }
+
+    /* Handle PGP/MIME parts */
+    if (GMIME_IS_MULTIPART_ENCRYPTED (part) && out->ctx->decrypt) {
+	if (out->children != 2) {
+	    /* this violates RFC 3156 section 4, so we won't bother with it. */
+	    fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
+		     "message (should be exactly 2)\n",
+		     out->children);
+	} else {
+	    out->is_encrypted = TRUE;
+	    GMimeMultipartEncrypted *encrypteddata =
+		GMIME_MULTIPART_ENCRYPTED (part);
+	    out->decrypted_child = g_mime_multipart_encrypted_decrypt
+		(encrypteddata, out->ctx->cryptoctx, &err);
+	    if (out->decrypted_child) {
+		out->decrypt_success = TRUE;
+		out->is_signed = TRUE;
+		out->sig_validity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
+	    } else {
+		fprintf (stderr, "Failed to decrypt part: %s\n",
+			 (err ? err->message : "no error explanation given"));
+	    }
+	}
+    } else if (GMIME_IS_MULTIPART_SIGNED (part) && out->ctx->cryptoctx) {
+	if (out->children != 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 "
+		     "(should be exactly 2)\n",
+		     out->children);
+	} else {
+	    out->is_signed = TRUE;
+	    /* For some reason the GMimeSignatureValidity returned
+	     * here is not a const (inconsistent with that
+	     * returned by
+	     * g_mime_multipart_encrypted_get_signature_validity,
+	     * and therefore needs to be properly disposed of.
+	     * Hopefully the API will become more consistent. */
+	    GMimeSignatureValidity *sig_validity = g_mime_multipart_signed_verify
+		(GMIME_MULTIPART_SIGNED (part), out->ctx->cryptoctx, &err);
+	    out->sig_validity = sig_validity;
+	    if (sig_validity) {
+		GMimeSignatureValidity **proxy =
+		    talloc (out, GMimeSignatureValidity *);
+		*proxy = sig_validity;
+		talloc_set_destructor (proxy, _signature_validity_free);
+	    }
+	}
+    }
+
+    if (out->is_signed && !out->sig_validity)
+	fprintf (stderr, "Failed to verify signed part: %s\n",
+		 (err ? err->message : "no error explanation given"));
+
+    if (err)
+	g_error_free (err);
+
+    return out;
+}
+
+mime_node_t *
+mime_node_child (const mime_node_t *parent, int child)
+{
+    if (!parent || child < 0 || child >= parent->children)
+	return NULL;
+
+    if (GMIME_IS_MULTIPART (parent->part)) {
+	GMimeMultipart *multipart = GMIME_MULTIPART (parent->part);
+	if (child == 1 && parent->decrypted_child)
+	    return _mime_node_create (parent, parent->decrypted_child);
+	return _mime_node_create (parent, g_mime_multipart_get_part (multipart, child));
+    } else if (GMIME_IS_MESSAGE (parent->part)) {
+	GMimeMessage *message = GMIME_MESSAGE (parent->part);
+	GMimeObject *child = g_mime_message_get_mime_part (message);
+	return _mime_node_create (parent, child);
+    } else {
+	/* This should have been caught by message_part_create */
+	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
+			g_type_name (G_OBJECT_TYPE (parent->part)));
+    }
+}
diff --git a/notmuch-client.h b/notmuch-client.h
index d7fb6ee..58bd21c 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -238,4 +238,84 @@ notmuch_config_set_maildir_synchronize_flags (notmuch_config_t *config,
 notmuch_bool_t
 debugger_is_active (void);
 
+/* mime-node.c */
+
+/* mime_node_t represents a single node in a MIME tree.  A MIME tree
+ * abstracts the different ways of traversing different types of MIME
+ * parts, allowing a MIME message to be viewed as a generic tree of
+ * parts.  Message-type parts have one child, multipart-type parts
+ * have multiple children, and leaf parts have zero children.
+ */
+typedef struct mime_node {
+    /* The MIME object of this part.  This will be a GMimeMessage,
+     * GMimePart, GMimeMultipart, or a subclass of one of these.
+     *
+     * This will never be a GMimeMessagePart because GMimeMessagePart
+     * is structurally redundant with GMimeMessage.  If this part is a
+     * message (that is, 'part' is a GMimeMessage), then either
+     * envelope_file will be set to a notmuch_message_t (for top-level
+     * messages) or envelope_part will be set to a GMimeMessagePart
+     * (for embedded message parts).
+     */
+    GMimeObject *part;
+
+    /* If part is a GMimeMessage, these record the envelope of the
+     * message: either a notmuch_message_t representing a top-level
+     * message, or a GMimeMessagePart representing a MIME part
+     * containing a message.
+     */
+    notmuch_message_t *envelope_file;
+    GMimeMessagePart *envelope_part;
+
+    /* The number of children of this part. */
+    int children;
+
+    /* True if this is the container for an encrypted or signed part.
+     * This does *not* mean that decryption or signature verification
+     * succeeded. */
+    notmuch_bool_t is_encrypted, is_signed;
+    /* True if decryption of this part's child succeeded.  In this
+     * case, the decrypted part is substituted for the second child of
+     * this part (which would usually be the encrypted data). */
+    notmuch_bool_t decrypt_success;
+    /* For signed or encrypted containers, the validity of the
+     * signature.  May be NULL if signature verification failed. */
+    const GMimeSignatureValidity *sig_validity;
+
+    /* Internal: Context inherited from the root iterator. */
+    struct mime_node_context *ctx;
+
+    /* Internal: For successfully decrypted multipart parts, the
+     * decrypted part to substitute for the second child. */
+    GMimeObject *decrypted_child;
+} mime_node_t;
+
+/* 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.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Root node is returned in *node_out.
+ *
+ * NOTMUCH_STATUS_FILE_ERROR: Failed to open message file.
+ *
+ * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
+ */
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+		GMimeCipherContext* cryptoctx, notmuch_bool_t decrypt,
+		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
+ * node.
+ *
+ * In case of any failure, this function returns NULL, (after printing
+ * an error message on stderr).
+ */
+mime_node_t *
+mime_node_child (const mime_node_t *parent, int child);
+
 #endif
-- 
1.7.5.4

[PATCH 3/4] Utility function to seek in MIME trees in depth-first order.

[Austin Clements]

This function matches how we number parts for the --part argument to
show.  It will allow us to jump directly to the desired part, rather
than traversing the entire tree and carefully tracking whether or not
we're "in the zone".
---
 mime-node.c      |   25 +++++++++++++++++++++++++
 notmuch-client.h |    5 +++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 942738b..40fff75 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -232,3 +232,28 @@ mime_node_child (const mime_node_t *parent, int child)
 			g_type_name (G_OBJECT_TYPE (parent->part)));
     }
 }
+
+static mime_node_t *
+_mime_node_seek_dfs_walk (mime_node_t *node, int *n)
+{
+    mime_node_t *ret = NULL;
+    int i;
+
+    if (*n <= 0)
+	return node;
+
+    *n = *n - 1;
+    for (i = 0; i < node->children && !ret; i++) {
+	mime_node_t *child = mime_node_child (node, i);
+	ret = _mime_node_seek_dfs_walk (child, n);
+	if (!ret)
+	    talloc_free (child);
+    }
+    return ret;
+}
+
+mime_node_t *
+mime_node_seek_dfs (mime_node_t *node, int n)
+{
+    return _mime_node_seek_dfs_walk (node, &n);
+}
diff --git a/notmuch-client.h b/notmuch-client.h
index 58bd21c..8b1454f 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -318,4 +318,9 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
 mime_node_t *
 mime_node_child (const mime_node_t *parent, int child);
 
+/* Return the nth child of node in a depth-first traversal.  If n is
+ * 0, returns node itself.  Returns NULL if there is no such part. */
+mime_node_t *
+mime_node_seek_dfs (mime_node_t *node, int n);
+
 #endif
-- 
1.7.5.4

[PATCH 4/4] show: Rewrite show_message_body to use the MIME tree interface.

[Austin Clements]

This removes all of the MIME traversal logic from show_message_body
and leaves only its interaction with the format callbacks.

Besides isolating concerns, since traversal happens behind a trivial
interface, there is now much less code duplication in
show_message_part.  Also, this uses mime_node_seek_dfs to start at the
requested part, eliminating all of the logic about parts being
selected or being in_zone (and reducing the "show state" to only a
part counter).  notmuch_show_params_t no longer needs to be passed
through the recursion because the only two fields that mattered
(related to crypto) are now handled by the MIME tree.

The few remaining complexities in show_message_part highlight
irregularities in the format callbacks with respect to top-level
messages and embedded message parts.

Since this is a rewrite, the diff is not very enlightening.  It's
easier to look at the old code and the new code side-by-side.
---
 show-message.c |  229 +++++++++++++-------------------------------------------
 1 files changed, 52 insertions(+), 177 deletions(-)

diff --git a/show-message.c b/show-message.c
index 09fa607..4560aea 100644
--- a/show-message.c
+++ b/show-message.c
@@ -24,154 +24,54 @@
 
 typedef struct show_message_state {
     int part_count;
-    int in_zone;
 } show_message_state_t;
 
 static void
-show_message_part (GMimeObject *part,
+show_message_part (mime_node_t *node,
 		   show_message_state_t *state,
 		   const notmuch_show_format_t *format,
-		   notmuch_show_params_t *params,
 		   int first)
 {
-    GMimeObject *decryptedpart = NULL;
-    int selected;
+    /* Formatters expect the envelope for embedded message parts */
+    GMimeObject *part = node->envelope_part ?
+	GMIME_OBJECT (node->envelope_part) : node->part;
+    int i;
+
+    if (!first)
+	fputs (format->part_sep, stdout);
+
+    /* Format this part */
+    if (format->part_start)
+	format->part_start (part, &(state->part_count));
+
+    if (node->is_encrypted && format->part_encstatus)
+	format->part_encstatus (node->decrypt_success);
+
+    if (node->is_signed && format->part_sigstatus)
+	format->part_sigstatus (node->sig_validity);
+
+    format->part_content (part);
+
+    if (node->envelope_part) {
+	fputs (format->header_start, stdout);
+	if (format->header_message_part)
+	    format->header_message_part (GMIME_MESSAGE (node->part));
+	fputs (format->header_end, stdout);
+
+	fputs (format->body_start, stdout);
+    }
+
+    /* Recurse over the children */
     state->part_count += 1;
+    for (i = 0; i < node->children; i++)
+	show_message_part (mime_node_child (node, i), state, format, i == 0);
 
-    if (! (GMIME_IS_PART (part) || GMIME_IS_MULTIPART (part) || GMIME_IS_MESSAGE_PART (part))) {
-	fprintf (stderr, "Warning: Not displaying unknown mime part: %s.\n",
-		 g_type_name (G_OBJECT_TYPE (part)));
-	return;
-    }
+    /* Finish this part */
+    if (node->envelope_part)
+	fputs (format->body_end, stdout);
 
-    selected = (params->part <= 0 || state->part_count == params->part);
-
-    if (selected || state->in_zone) {
-	if (!first && (params->part <= 0 || state->in_zone))
-	    fputs (format->part_sep, stdout);
-
-	if (format->part_start)
-	    format->part_start (part, &(state->part_count));
-    }
-
-    /* handle PGP/MIME parts */
-    if (GMIME_IS_MULTIPART (part) && params->cryptoctx) {
-	GMimeMultipart *multipart = GMIME_MULTIPART (part);
-	GError* err = NULL;
-
-	if (GMIME_IS_MULTIPART_ENCRYPTED (part) && params->decrypt)
-	{
-	    if ( g_mime_multipart_get_count (multipart) != 2 ) {
-		/* this violates RFC 3156 section 4, so we won't bother with it. */
-		fprintf (stderr,
-			 "Error: %d part(s) for a multipart/encrypted message (should be exactly 2)\n",
-			 g_mime_multipart_get_count (multipart));
-	    } else {
-		GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
-		decryptedpart = g_mime_multipart_encrypted_decrypt (encrypteddata, params->cryptoctx, &err);
-		if (decryptedpart) {
-		    if ((selected || state->in_zone) && format->part_encstatus)
-			format->part_encstatus (1);
-		    const GMimeSignatureValidity *sigvalidity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
-		    if (!sigvalidity)
-			fprintf (stderr, "Failed to verify signed part: %s\n", (err ? err->message : "no error explanation given"));
-		    if ((selected || state->in_zone) && format->part_sigstatus)
-			format->part_sigstatus (sigvalidity);
-		} else {
-		    fprintf (stderr, "Failed to decrypt part: %s\n", (err ? err->message : "no error explanation given"));
-		    if ((selected || state->in_zone) && format->part_encstatus)
-			format->part_encstatus (0);
-		}
-	    }
-	}
-	else if (GMIME_IS_MULTIPART_SIGNED (part))
-	{
-	    if ( g_mime_multipart_get_count (multipart) != 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 (should be exactly 2)\n",
-			 g_mime_multipart_get_count (multipart));
-	    } else {
-		/* For some reason the GMimeSignatureValidity returned
-		 * here is not a const (inconsistent with that
-		 * returned by
-		 * g_mime_multipart_encrypted_get_signature_validity,
-		 * and therefore needs to be properly disposed of.
-		 * Hopefully the API will become more consistent. */
-		GMimeSignatureValidity *sigvalidity = g_mime_multipart_signed_verify (GMIME_MULTIPART_SIGNED (part), params->cryptoctx, &err);
-		if (!sigvalidity) {
-		    fprintf (stderr, "Failed to verify signed part: %s\n", (err ? err->message : "no error explanation given"));
-		}
-		if ((selected || state->in_zone) && format->part_sigstatus)
-		    format->part_sigstatus (sigvalidity);
-		if (sigvalidity)
-		    g_mime_signature_validity_free (sigvalidity);
-	    }
-	}
-
-	if (err)
-	    g_error_free (err);
-    }
-    /* end handle PGP/MIME parts */
-
-    if (selected || state->in_zone)
-	format->part_content (part);
-
-    if (GMIME_IS_MULTIPART (part)) {
-	GMimeMultipart *multipart = GMIME_MULTIPART (part);
-	int i;
-
-	if (selected)
-	    state->in_zone = 1;
-
-	if (decryptedpart) {
-	    /* We emit the useless application/pgp-encrypted version
-	     * part here only to keep the emitted output as consistent
-	     * as possible between decrypted output and the
-	     * unprocessed multipart/mime. For some strange reason,
-	     * the actual encrypted data is the second part of the
-	     * multipart. */
-	    show_message_part (g_mime_multipart_get_part (multipart, 0), state, format, params, TRUE);
-	    show_message_part (decryptedpart, state, format, params, FALSE);
-	} else {
-	    for (i = 0; i < g_mime_multipart_get_count (multipart); i++) {
-		show_message_part (g_mime_multipart_get_part (multipart, i),
-				   state, format, params, i == 0);
-	    }
-	}
-
-	if (selected)
-	    state->in_zone = 0;
-
-    } else if (GMIME_IS_MESSAGE_PART (part)) {
-	GMimeMessage *mime_message = g_mime_message_part_get_message (GMIME_MESSAGE_PART (part));
-
-	if (selected)
-	    state->in_zone = 1;
-
-	if (selected || (!selected && state->in_zone)) {
-	    fputs (format->header_start, stdout);
-	    if (format->header_message_part)
-		format->header_message_part (mime_message);
-	    fputs (format->header_end, stdout);
-
-	    fputs (format->body_start, stdout);
-	}
-
-	show_message_part (g_mime_message_get_mime_part (mime_message),
-			   state, format, params, TRUE);
-
-	if (selected || (!selected && state->in_zone))
-	    fputs (format->body_end, stdout);
-
-	if (selected)
-	    state->in_zone = 0;
-    }
-
-    if (selected || state->in_zone) {
-	if (format->part_end)
-	    format->part_end (part);
-    }
+    if (format->part_end)
+	format->part_end (part);
 }
 
 notmuch_status_t
@@ -179,49 +79,24 @@ show_message_body (notmuch_message_t *message,
 		   const notmuch_show_format_t *format,
 		   notmuch_show_params_t *params)
 {
-    GMimeStream *stream = NULL;
-    GMimeParser *parser = NULL;
-    GMimeMessage *mime_message = NULL;
-    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
-    const char *filename = notmuch_message_get_filename (message);
-    FILE *file = NULL;
+    notmuch_status_t ret;
     show_message_state_t state;
+    mime_node_t *root, *part;
 
-    state.part_count = 0;
-    state.in_zone = 0;
+    ret = mime_node_open (NULL, message, params->cryptoctx, params->decrypt,
+			  &root);
+    if (ret)
+	return ret;
 
-    file = fopen (filename, "r");
-    if (! file) {
-	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
-	ret = NOTMUCH_STATUS_FILE_ERROR;
-	goto DONE;
-    }
+    /* The caller of show_message_body has already handled the
+     * outermost envelope, so skip it. */
+    state.part_count = MAX (params->part, 1);
 
-    stream = g_mime_stream_file_new (file);
-    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream), FALSE);
+    part = mime_node_seek_dfs (root, state.part_count);
+    if (part)
+	show_message_part (part, &state, format, TRUE);
 
-    parser = g_mime_parser_new_with_stream (stream);
+    talloc_free (root);
 
-    mime_message = g_mime_parser_construct_message (parser);
-
-    show_message_part (g_mime_message_get_mime_part (mime_message),
-		       &state,
-		       format,
-		       params,
-		       TRUE);
-
-  DONE:
-    if (mime_message)
-	g_object_unref (mime_message);
-
-    if (parser)
-	g_object_unref (parser);
-
-    if (stream)
-	g_object_unref (stream);
-
-    if (file)
-	fclose (file);
-
-    return ret;
+    return NOTMUCH_STATUS_SUCCESS;
 }
-- 
1.7.5.4

Re: [PATCH 4/4] show: Rewrite show_message_body to use the MIME tree interface.

[Jani Nikula]

On Sun, 27 Nov 2011 21:21:11 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Since this is a rewrite, the diff is not very enlightening.  It's
> easier to look at the old code and the new code side-by-side.

Hi Austin, try the git format-patch --break-rewrites option. It works
nicely on patches like this. See below.

BR,
Jani.


From 4df3de0d35b7fb5b142b3413d56cda2811406fd9 Mon Sep 17 00:00:00 2001
From: Austin Clements <amdragon@MIT.EDU>
Date: Sun, 27 Nov 2011 21:21:11 -0500
Subject: [PATCH] show: Rewrite show_message_body to use the MIME tree
 interface.

This removes all of the MIME traversal logic from show_message_body
and leaves only its interaction with the format callbacks.

Besides isolating concerns, since traversal happens behind a trivial
interface, there is now much less code duplication in
show_message_part.  Also, this uses mime_node_seek_dfs to start at the
requested part, eliminating all of the logic about parts being
selected or being in_zone (and reducing the "show state" to only a
part counter).  notmuch_show_params_t no longer needs to be passed
through the recursion because the only two fields that mattered
(related to crypto) are now handled by the MIME tree.

The few remaining complexities in show_message_part highlight
irregularities in the format callbacks with respect to top-level
messages and embedded message parts.

Since this is a rewrite, the diff is not very enlightening.  It's
easier to look at the old code and the new code side-by-side.
---
 show-message.c |  329 +++++++++++++++++--------------------------------------
 1 files changed, 102 insertions(+), 227 deletions(-)
 rewrite show-message.c (81%)

diff --git a/show-message.c b/show-message.c
dissimilarity index 81%
index 09fa607..4560aea 100644
--- a/show-message.c
+++ b/show-message.c
@@ -1,227 +1,102 @@
-/* notmuch - Not much of an email program, (just index and search)
- *
- * Copyright © 2009 Carl Worth
- * Copyright © 2009 Keith Packard
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation, either version 3 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see http://www.gnu.org/licenses/ .
- *
- * Authors: Carl Worth <cworth@cworth.org>
- *	    Keith Packard <keithp@keithp.com>
- */
-
-#include "notmuch-client.h"
-
-typedef struct show_message_state {
-    int part_count;
-    int in_zone;
-} show_message_state_t;
-
-static void
-show_message_part (GMimeObject *part,
-		   show_message_state_t *state,
-		   const notmuch_show_format_t *format,
-		   notmuch_show_params_t *params,
-		   int first)
-{
-    GMimeObject *decryptedpart = NULL;
-    int selected;
-    state->part_count += 1;
-
-    if (! (GMIME_IS_PART (part) || GMIME_IS_MULTIPART (part) || GMIME_IS_MESSAGE_PART (part))) {
-	fprintf (stderr, "Warning: Not displaying unknown mime part: %s.\n",
-		 g_type_name (G_OBJECT_TYPE (part)));
-	return;
-    }
-
-    selected = (params->part <= 0 || state->part_count == params->part);
-
-    if (selected || state->in_zone) {
-	if (!first && (params->part <= 0 || state->in_zone))
-	    fputs (format->part_sep, stdout);
-
-	if (format->part_start)
-	    format->part_start (part, &(state->part_count));
-    }
-
-    /* handle PGP/MIME parts */
-    if (GMIME_IS_MULTIPART (part) && params->cryptoctx) {
-	GMimeMultipart *multipart = GMIME_MULTIPART (part);
-	GError* err = NULL;
-
-	if (GMIME_IS_MULTIPART_ENCRYPTED (part) && params->decrypt)
-	{
-	    if ( g_mime_multipart_get_count (multipart) != 2 ) {
-		/* this violates RFC 3156 section 4, so we won't bother with it. */
-		fprintf (stderr,
-			 "Error: %d part(s) for a multipart/encrypted message (should be exactly 2)\n",
-			 g_mime_multipart_get_count (multipart));
-	    } else {
-		GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
-		decryptedpart = g_mime_multipart_encrypted_decrypt (encrypteddata, params->cryptoctx, &err);
-		if (decryptedpart) {
-		    if ((selected || state->in_zone) && format->part_encstatus)
-			format->part_encstatus (1);
-		    const GMimeSignatureValidity *sigvalidity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
-		    if (!sigvalidity)
-			fprintf (stderr, "Failed to verify signed part: %s\n", (err ? err->message : "no error explanation given"));
-		    if ((selected || state->in_zone) && format->part_sigstatus)
-			format->part_sigstatus (sigvalidity);
-		} else {
-		    fprintf (stderr, "Failed to decrypt part: %s\n", (err ? err->message : "no error explanation given"));
-		    if ((selected || state->in_zone) && format->part_encstatus)
-			format->part_encstatus (0);
-		}
-	    }
-	}
-	else if (GMIME_IS_MULTIPART_SIGNED (part))
-	{
-	    if ( g_mime_multipart_get_count (multipart) != 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 (should be exactly 2)\n",
-			 g_mime_multipart_get_count (multipart));
-	    } else {
-		/* For some reason the GMimeSignatureValidity returned
-		 * here is not a const (inconsistent with that
-		 * returned by
-		 * g_mime_multipart_encrypted_get_signature_validity,
-		 * and therefore needs to be properly disposed of.
-		 * Hopefully the API will become more consistent. */
-		GMimeSignatureValidity *sigvalidity = g_mime_multipart_signed_verify (GMIME_MULTIPART_SIGNED (part), params->cryptoctx, &err);
-		if (!sigvalidity) {
-		    fprintf (stderr, "Failed to verify signed part: %s\n", (err ? err->message : "no error explanation given"));
-		}
-		if ((selected || state->in_zone) && format->part_sigstatus)
-		    format->part_sigstatus (sigvalidity);
-		if (sigvalidity)
-		    g_mime_signature_validity_free (sigvalidity);
-	    }
-	}
-
-	if (err)
-	    g_error_free (err);
-    }
-    /* end handle PGP/MIME parts */
-
-    if (selected || state->in_zone)
-	format->part_content (part);
-
-    if (GMIME_IS_MULTIPART (part)) {
-	GMimeMultipart *multipart = GMIME_MULTIPART (part);
-	int i;
-
-	if (selected)
-	    state->in_zone = 1;
-
-	if (decryptedpart) {
-	    /* We emit the useless application/pgp-encrypted version
-	     * part here only to keep the emitted output as consistent
-	     * as possible between decrypted output and the
-	     * unprocessed multipart/mime. For some strange reason,
-	     * the actual encrypted data is the second part of the
-	     * multipart. */
-	    show_message_part (g_mime_multipart_get_part (multipart, 0), state, format, params, TRUE);
-	    show_message_part (decryptedpart, state, format, params, FALSE);
-	} else {
-	    for (i = 0; i < g_mime_multipart_get_count (multipart); i++) {
-		show_message_part (g_mime_multipart_get_part (multipart, i),
-				   state, format, params, i == 0);
-	    }
-	}
-
-	if (selected)
-	    state->in_zone = 0;
-
-    } else if (GMIME_IS_MESSAGE_PART (part)) {
-	GMimeMessage *mime_message = g_mime_message_part_get_message (GMIME_MESSAGE_PART (part));
-
-	if (selected)
-	    state->in_zone = 1;
-
-	if (selected || (!selected && state->in_zone)) {
-	    fputs (format->header_start, stdout);
-	    if (format->header_message_part)
-		format->header_message_part (mime_message);
-	    fputs (format->header_end, stdout);
-
-	    fputs (format->body_start, stdout);
-	}
-
-	show_message_part (g_mime_message_get_mime_part (mime_message),
-			   state, format, params, TRUE);
-
-	if (selected || (!selected && state->in_zone))
-	    fputs (format->body_end, stdout);
-
-	if (selected)
-	    state->in_zone = 0;
-    }
-
-    if (selected || state->in_zone) {
-	if (format->part_end)
-	    format->part_end (part);
-    }
-}
-
-notmuch_status_t
-show_message_body (notmuch_message_t *message,
-		   const notmuch_show_format_t *format,
-		   notmuch_show_params_t *params)
-{
-    GMimeStream *stream = NULL;
-    GMimeParser *parser = NULL;
-    GMimeMessage *mime_message = NULL;
-    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
-    const char *filename = notmuch_message_get_filename (message);
-    FILE *file = NULL;
-    show_message_state_t state;
-
-    state.part_count = 0;
-    state.in_zone = 0;
-
-    file = fopen (filename, "r");
-    if (! file) {
-	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
-	ret = NOTMUCH_STATUS_FILE_ERROR;
-	goto DONE;
-    }
-
-    stream = g_mime_stream_file_new (file);
-    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream), FALSE);
-
-    parser = g_mime_parser_new_with_stream (stream);
-
-    mime_message = g_mime_parser_construct_message (parser);
-
-    show_message_part (g_mime_message_get_mime_part (mime_message),
-		       &state,
-		       format,
-		       params,
-		       TRUE);
-
-  DONE:
-    if (mime_message)
-	g_object_unref (mime_message);
-
-    if (parser)
-	g_object_unref (parser);
-
-    if (stream)
-	g_object_unref (stream);
-
-    if (file)
-	fclose (file);
-
-    return ret;
-}
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2009 Carl Worth
+ * Copyright © 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth <cworth@cworth.org>
+ *	    Keith Packard <keithp@keithp.com>
+ */
+
+#include "notmuch-client.h"
+
+typedef struct show_message_state {
+    int part_count;
+} show_message_state_t;
+
+static void
+show_message_part (mime_node_t *node,
+		   show_message_state_t *state,
+		   const notmuch_show_format_t *format,
+		   int first)
+{
+    /* Formatters expect the envelope for embedded message parts */
+    GMimeObject *part = node->envelope_part ?
+	GMIME_OBJECT (node->envelope_part) : node->part;
+    int i;
+
+    if (!first)
+	fputs (format->part_sep, stdout);
+
+    /* Format this part */
+    if (format->part_start)
+	format->part_start (part, &(state->part_count));
+
+    if (node->is_encrypted && format->part_encstatus)
+	format->part_encstatus (node->decrypt_success);
+
+    if (node->is_signed && format->part_sigstatus)
+	format->part_sigstatus (node->sig_validity);
+
+    format->part_content (part);
+
+    if (node->envelope_part) {
+	fputs (format->header_start, stdout);
+	if (format->header_message_part)
+	    format->header_message_part (GMIME_MESSAGE (node->part));
+	fputs (format->header_end, stdout);
+
+	fputs (format->body_start, stdout);
+    }
+
+    /* Recurse over the children */
+    state->part_count += 1;
+    for (i = 0; i < node->children; i++)
+	show_message_part (mime_node_child (node, i), state, format, i == 0);
+
+    /* Finish this part */
+    if (node->envelope_part)
+	fputs (format->body_end, stdout);
+
+    if (format->part_end)
+	format->part_end (part);
+}
+
+notmuch_status_t
+show_message_body (notmuch_message_t *message,
+		   const notmuch_show_format_t *format,
+		   notmuch_show_params_t *params)
+{
+    notmuch_status_t ret;
+    show_message_state_t state;
+    mime_node_t *root, *part;
+
+    ret = mime_node_open (NULL, message, params->cryptoctx, params->decrypt,
+			  &root);
+    if (ret)
+	return ret;
+
+    /* The caller of show_message_body has already handled the
+     * outermost envelope, so skip it. */
+    state.part_count = MAX (params->part, 1);
+
+    part = mime_node_seek_dfs (root, state.part_count);
+    if (part)
+	show_message_part (part, &state, format, TRUE);
+
+    talloc_free (root);
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
-- 
1.7.5.4

Re: [PATCH 0/4] First step of 'show' rewrite

[Jameson Graef Rollins]

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

On Sun, 27 Nov 2011 21:21:07 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This is the first step in my rewrite of notmuch-show.

Hey, Austin.  Thank you so much for working on this!  This is some
really awesome work (as usual).  I unfortunately don't have time to play
with this at the moment, but I'm very excited to as soon as I get the
opportunity.  On first glance it looks like a very nice optimization,
and should allow us to handle notmuch show operations in a much more
flexible way.

More complete review soon.

jamie.

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

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Jani Nikula]

Hi, generally looks good to me, but please find a few comments below.

BR,
Jani.

On Sun, 27 Nov 2011 21:21:09 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This wraps all of the complex MIME part handling in a single, simple
> function that gets part N from *any* MIME object, so traversing a MIME
> part tree becomes a two-line for loop.  Furthermore, the MIME node
> structure provides easy access to envelopes for message parts as well
> as cryptographic information.
> 
> This code is directly derived from the current show_message_body code
> (much of it is identical), but the control relation is inverted:
> instead of show_message_body controlling the traversal of the MIME
> structure and invoking callbacks, the caller controls the traversal of
> the MIME structure.
> ---
>  Makefile.local   |    1 +
>  mime-node.c      |  234 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  notmuch-client.h |   80 ++++++++++++++++++
>  3 files changed, 315 insertions(+), 0 deletions(-)
>  create mode 100644 mime-node.c
> 
> diff --git a/Makefile.local b/Makefile.local
> index c94402b..c46ed26 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -312,6 +312,7 @@ notmuch_client_srcs =		\
>  	notmuch-time.c		\
>  	query-string.c		\
>  	show-message.c		\
> +	mime-node.c		\
>  	json.c
>  
>  notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
> diff --git a/mime-node.c b/mime-node.c
> new file mode 100644
> index 0000000..942738b
> --- /dev/null
> +++ b/mime-node.c
> @@ -0,0 +1,234 @@
> +/* notmuch - Not much of an email program, (just index and search)
> + *
> + * Copyright © 2009 Carl Worth
> + * Copyright © 2009 Keith Packard
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Authors: Carl Worth <cworth@cworth.org>
> + *          Keith Packard <keithp@keithp.com>
> + *          Austin Clements <aclements@csail.mit.edu>
> + */
> +
> +#include "notmuch-client.h"
> +
> +/* Context that gets inherited from the root node. */
> +typedef struct mime_node_context {
> +    /* Per-message resources.  These are allocated internally and must
> +     * be destroyed. */
> +    FILE *file;
> +    GMimeStream *stream;
> +    GMimeParser *parser;
> +    GMimeMessage *mime_message;
> +    

Leftover indentation spaces above.

> +    /* Context provided by the caller. */
> +    GMimeCipherContext *cryptoctx;
> +    notmuch_bool_t decrypt;
> +} mime_node_context_t;
> +
> +static int
> +_mime_node_context_free (mime_node_context_t *res)
> +{
> +    if (res->mime_message)
> +	g_object_unref (res->mime_message);
> +
> +    if (res->parser)
> +	g_object_unref (res->parser);
> +
> +    if (res->stream)
> +	g_object_unref (res->stream);
> +
> +    if (res->file)
> +	fclose (res->file);
> +
> +    return 0;
> +}
> +
> +notmuch_status_t
> +mime_node_open (const void *ctx, notmuch_message_t *message,
> +		GMimeCipherContext* cryptoctx, notmuch_bool_t decrypt,

The style here seems to be * next to the variable name, not type.

> +		mime_node_t **root_out)
> +{
> +    const char *filename = notmuch_message_get_filename (message);
> +    mime_node_context_t *mctx;
> +    mime_node_t *root = NULL;

No need to initialize as it's initialized right away below.

> +    notmuch_status_t status;
> +
> +    root = talloc_zero (ctx, mime_node_t);
> +    if (root == NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +
> +    /* Create the tree-wide context */
> +    mctx = talloc_zero (root, mime_node_context_t);
> +    if (mctx == NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +    talloc_set_destructor (mctx, _mime_node_context_free);
> +
> +    mctx->file = fopen (filename, "r");
> +    if (! mctx->file) {
> +	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
> +	status = NOTMUCH_STATUS_FILE_ERROR;
> +	goto DONE;
> +    }
> +
> +    mctx->stream = g_mime_stream_file_new (mctx->file);

AFAICT the GMimeStreamFile object owns the FILE * pointer now, and
closes it later. Calling fclose() on it in _mime_node_context_free()
would be undefined behaviour. But please don't just take my word for it.

> +    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx->stream), FALSE);
> +
> +    mctx->parser = g_mime_parser_new_with_stream (mctx->stream);
> +
> +    mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
> +
> +    mctx->cryptoctx = cryptoctx;
> +    mctx->decrypt = decrypt;
> +
> +    /* Create the root node */
> +    root->part = GMIME_OBJECT (mctx->mime_message);
> +    root->envelope_file = message;
> +    root->children = 1;
> +    root->ctx = mctx;
> +
> +    *root_out = root;
> +    return NOTMUCH_STATUS_SUCCESS;
> +
> +DONE:
> +    talloc_free (root);
> +    return status;
> +}
> +
> +static int
> +_signature_validity_free (GMimeSignatureValidity **proxy)
> +{
> +    g_mime_signature_validity_free (*proxy);
> +    return 0;
> +}
> +
> +static mime_node_t *
> +_mime_node_create (const mime_node_t *parent, GMimeObject *part)
> +{
> +    mime_node_t *out = talloc_zero (parent, mime_node_t);
> +    GError *err = NULL;
> +
> +    /* Set basic node properties */
> +    out->part = part;
> +    out->ctx = parent->ctx;
> +    if (!talloc_reference (out, out->ctx)) {
> +	fprintf (stderr, "Out of memory.\n");
> +	talloc_free (out);
> +	return NULL;
> +    }
> +
> +    /* Deal with the different types of parts */
> +    if (GMIME_IS_PART (part)) {
> +	out->children = 0;
> +    } else if (GMIME_IS_MULTIPART (part)) {
> +	out->children = g_mime_multipart_get_count (GMIME_MULTIPART (part));
> +    } else if (GMIME_IS_MESSAGE_PART (part)) {
> +	/* Promote part to an envelope and open it */
> +	GMimeMessagePart *message_part = GMIME_MESSAGE_PART (part);
> +	GMimeMessage *message = g_mime_message_part_get_message (message_part);
> +	out->envelope_part = message_part;
> +	out->part = GMIME_OBJECT (message);
> +	out->children = 1;
> +    } else {
> +	fprintf (stderr, "Warning: Unknown mime part type: %s.\n",
> +		 g_type_name (G_OBJECT_TYPE (part)));
> +	talloc_free (out);
> +	return NULL;
> +    }
> +
> +    /* Handle PGP/MIME parts */
> +    if (GMIME_IS_MULTIPART_ENCRYPTED (part) && out->ctx->decrypt) {
> +	if (out->children != 2) {
> +	    /* this violates RFC 3156 section 4, so we won't bother with it. */
> +	    fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
> +		     "message (should be exactly 2)\n",
> +		     out->children);
> +	} else {
> +	    out->is_encrypted = TRUE;
> +	    GMimeMultipartEncrypted *encrypteddata =
> +		GMIME_MULTIPART_ENCRYPTED (part);
> +	    out->decrypted_child = g_mime_multipart_encrypted_decrypt
> +		(encrypteddata, out->ctx->cryptoctx, &err);
> +	    if (out->decrypted_child) {
> +		out->decrypt_success = TRUE;
> +		out->is_signed = TRUE;
> +		out->sig_validity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
> +	    } else {
> +		fprintf (stderr, "Failed to decrypt part: %s\n",
> +			 (err ? err->message : "no error explanation given"));
> +	    }
> +	}
> +    } else if (GMIME_IS_MULTIPART_SIGNED (part) && out->ctx->cryptoctx) {
> +	if (out->children != 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 "
> +		     "(should be exactly 2)\n",
> +		     out->children);
> +	} else {
> +	    out->is_signed = TRUE;
> +	    /* For some reason the GMimeSignatureValidity returned
> +	     * here is not a const (inconsistent with that
> +	     * returned by
> +	     * g_mime_multipart_encrypted_get_signature_validity,
> +	     * and therefore needs to be properly disposed of.
> +	     * Hopefully the API will become more consistent. */
> +	    GMimeSignatureValidity *sig_validity = g_mime_multipart_signed_verify
> +		(GMIME_MULTIPART_SIGNED (part), out->ctx->cryptoctx, &err);
> +	    out->sig_validity = sig_validity;
> +	    if (sig_validity) {
> +		GMimeSignatureValidity **proxy =
> +		    talloc (out, GMimeSignatureValidity *);
> +		*proxy = sig_validity;
> +		talloc_set_destructor (proxy, _signature_validity_free);
> +	    }
> +	}
> +    }
> +
> +    if (out->is_signed && !out->sig_validity)
> +	fprintf (stderr, "Failed to verify signed part: %s\n",
> +		 (err ? err->message : "no error explanation given"));
> +
> +    if (err)
> +	g_error_free (err);
> +
> +    return out;
> +}
> +
> +mime_node_t *
> +mime_node_child (const mime_node_t *parent, int child)
> +{
> +    if (!parent || child < 0 || child >= parent->children)
> +	return NULL;
> +
> +    if (GMIME_IS_MULTIPART (parent->part)) {
> +	GMimeMultipart *multipart = GMIME_MULTIPART (parent->part);
> +	if (child == 1 && parent->decrypted_child)
> +	    return _mime_node_create (parent, parent->decrypted_child);
> +	return _mime_node_create (parent, g_mime_multipart_get_part (multipart, child));
> +    } else if (GMIME_IS_MESSAGE (parent->part)) {
> +	GMimeMessage *message = GMIME_MESSAGE (parent->part);
> +	GMimeObject *child = g_mime_message_get_mime_part (message);
> +	return _mime_node_create (parent, child);
> +    } else {
> +	/* This should have been caught by message_part_create */
> +	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
> +			g_type_name (G_OBJECT_TYPE (parent->part)));
> +    }
> +}
> diff --git a/notmuch-client.h b/notmuch-client.h
> index d7fb6ee..58bd21c 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -238,4 +238,84 @@ notmuch_config_set_maildir_synchronize_flags (notmuch_config_t *config,
>  notmuch_bool_t
>  debugger_is_active (void);
>  
> +/* mime-node.c */
> +
> +/* mime_node_t represents a single node in a MIME tree.  A MIME tree
> + * abstracts the different ways of traversing different types of MIME
> + * parts, allowing a MIME message to be viewed as a generic tree of
> + * parts.  Message-type parts have one child, multipart-type parts
> + * have multiple children, and leaf parts have zero children.
> + */
> +typedef struct mime_node {
> +    /* The MIME object of this part.  This will be a GMimeMessage,
> +     * GMimePart, GMimeMultipart, or a subclass of one of these.
> +     *
> +     * This will never be a GMimeMessagePart because GMimeMessagePart
> +     * is structurally redundant with GMimeMessage.  If this part is a
> +     * message (that is, 'part' is a GMimeMessage), then either
> +     * envelope_file will be set to a notmuch_message_t (for top-level
> +     * messages) or envelope_part will be set to a GMimeMessagePart
> +     * (for embedded message parts).
> +     */
> +    GMimeObject *part;
> +
> +    /* If part is a GMimeMessage, these record the envelope of the
> +     * message: either a notmuch_message_t representing a top-level
> +     * message, or a GMimeMessagePart representing a MIME part
> +     * containing a message.
> +     */
> +    notmuch_message_t *envelope_file;
> +    GMimeMessagePart *envelope_part;
> +
> +    /* The number of children of this part. */
> +    int children;
> +
> +    /* True if this is the container for an encrypted or signed part.
> +     * This does *not* mean that decryption or signature verification
> +     * succeeded. */
> +    notmuch_bool_t is_encrypted, is_signed;
> +    /* True if decryption of this part's child succeeded.  In this
> +     * case, the decrypted part is substituted for the second child of
> +     * this part (which would usually be the encrypted data). */
> +    notmuch_bool_t decrypt_success;
> +    /* For signed or encrypted containers, the validity of the
> +     * signature.  May be NULL if signature verification failed. */
> +    const GMimeSignatureValidity *sig_validity;
> +
> +    /* Internal: Context inherited from the root iterator. */
> +    struct mime_node_context *ctx;
> +
> +    /* Internal: For successfully decrypted multipart parts, the
> +     * decrypted part to substitute for the second child. */
> +    GMimeObject *decrypted_child;
> +} mime_node_t;
> +
> +/* 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.
> + *
> + * Return value:
> + *
> + * NOTMUCH_STATUS_SUCCESS: Root node is returned in *node_out.
> + *
> + * NOTMUCH_STATUS_FILE_ERROR: Failed to open message file.
> + *
> + * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
> + */
> +notmuch_status_t
> +mime_node_open (const void *ctx, notmuch_message_t *message,
> +		GMimeCipherContext* cryptoctx, notmuch_bool_t decrypt,
> +		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
> + * node.
> + *
> + * In case of any failure, this function returns NULL, (after printing
> + * an error message on stderr).
> + */
> +mime_node_t *
> +mime_node_child (const mime_node_t *parent, int child);
> +
>  #endif
> -- 
> 1.7.5.4
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

Thanks for the review.  A new version is on its way...

Quoth Jani Nikula on Nov 29 at  9:11 pm:
> > +    mctx->stream = g_mime_stream_file_new (mctx->file);
> 
> AFAICT the GMimeStreamFile object owns the FILE * pointer now, and
> closes it later. Calling fclose() on it in _mime_node_context_free()
> would be undefined behaviour. But please don't just take my word for it.

The next line tells GMime not to close the stream on its own.

Though, while I believe the code is correct, I'm not sure why we can't
just let GMime close the stream and remove our fclose.  The original
code for this was introduced in 357aba3e (along with most of the code
in show-message.c) with the cryptic comment that otherwise GMime would
close stdout.  Since I'd rather not regress some bug I don't
understand, I'll leave the code the way it is.

> > +    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx->stream), FALSE);

[PATCH v2 0/4] First step of 'show' rewrite

[Austin Clements]

This version addresses the comments in Jani's review
(id:8739d6u4ju.fsf@nikula.org).

[PATCH 1/4] show: Pass notmuch_message_t instead of path to show_message_body.

[Austin Clements]

In addition to simplifying the code, we'll need the notmuch_message_t*
in show_message_body shortly.
---
 notmuch-client.h |    2 +-
 notmuch-reply.c  |    3 +--
 notmuch-show.c   |    3 +--
 show-message.c   |    3 ++-
 4 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index b50cb38..d7fb6ee 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -162,7 +162,7 @@ char *
 query_string_from_args (void *ctx, int argc, char *argv[]);
 
 notmuch_status_t
-show_message_body (const char *filename,
+show_message_body (notmuch_message_t *message,
 		   const notmuch_show_format_t *format,
 		   notmuch_show_params_t *params);
 
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 7ac879f..f8d5f64 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -546,8 +546,7 @@ notmuch_reply_format_default(void *ctx,
 		notmuch_message_get_header (message, "date"),
 		notmuch_message_get_header (message, "from"));
 
-	show_message_body (notmuch_message_get_filename (message),
-			   format, params);
+	show_message_body (message, format, params);
 
 	notmuch_message_destroy (message);
     }
diff --git a/notmuch-show.c b/notmuch-show.c
index 603992a..1dee3aa 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -753,8 +753,7 @@ show_message (void *ctx,
     }
 
     if (format->part_content)
-	show_message_body (notmuch_message_get_filename (message),
-			   format, params);
+	show_message_body (message, format, params);
 
     if (params->part <= 0) {
 	fputs (format->body_end, stdout);
diff --git a/show-message.c b/show-message.c
index d83f04e..09fa607 100644
--- a/show-message.c
+++ b/show-message.c
@@ -175,7 +175,7 @@ show_message_part (GMimeObject *part,
 }
 
 notmuch_status_t
-show_message_body (const char *filename,
+show_message_body (notmuch_message_t *message,
 		   const notmuch_show_format_t *format,
 		   notmuch_show_params_t *params)
 {
@@ -183,6 +183,7 @@ show_message_body (const char *filename,
     GMimeParser *parser = NULL;
     GMimeMessage *mime_message = NULL;
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
+    const char *filename = notmuch_message_get_filename (message);
     FILE *file = NULL;
     show_message_state_t state;
 
-- 
1.7.5.4

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

This wraps all of the complex MIME part handling in a single, simple
function that gets part N from *any* MIME object, so traversing a MIME
part tree becomes a two-line for loop.  Furthermore, the MIME node
structure provides easy access to envelopes for message parts as well
as cryptographic information.

This code is directly derived from the current show_message_body code
(much of it is identical), but the control relation is inverted:
instead of show_message_body controlling the traversal of the MIME
structure and invoking callbacks, the caller controls the traversal of
the MIME structure.
---
 Makefile.local   |    1 +
 mime-node.c      |  234 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 notmuch-client.h |   80 ++++++++++++++++++
 3 files changed, 315 insertions(+), 0 deletions(-)
 create mode 100644 mime-node.c

diff --git a/Makefile.local b/Makefile.local
index c94402b..c46ed26 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -312,6 +312,7 @@ notmuch_client_srcs =		\
 	notmuch-time.c		\
 	query-string.c		\
 	show-message.c		\
+	mime-node.c		\
 	json.c
 
 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
diff --git a/mime-node.c b/mime-node.c
new file mode 100644
index 0000000..a8e4a59
--- /dev/null
+++ b/mime-node.c
@@ -0,0 +1,234 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2009 Carl Worth
+ * Copyright © 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth <cworth@cworth.org>
+ *          Keith Packard <keithp@keithp.com>
+ *          Austin Clements <aclements@csail.mit.edu>
+ */
+
+#include "notmuch-client.h"
+
+/* Context that gets inherited from the root node. */
+typedef struct mime_node_context {
+    /* Per-message resources.  These are allocated internally and must
+     * be destroyed. */
+    FILE *file;
+    GMimeStream *stream;
+    GMimeParser *parser;
+    GMimeMessage *mime_message;
+
+    /* Context provided by the caller. */
+    GMimeCipherContext *cryptoctx;
+    notmuch_bool_t decrypt;
+} mime_node_context_t;
+
+static int
+_mime_node_context_free (mime_node_context_t *res)
+{
+    if (res->mime_message)
+	g_object_unref (res->mime_message);
+
+    if (res->parser)
+	g_object_unref (res->parser);
+
+    if (res->stream)
+	g_object_unref (res->stream);
+
+    if (res->file)
+	fclose (res->file);
+
+    return 0;
+}
+
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+		GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
+		mime_node_t **root_out)
+{
+    const char *filename = notmuch_message_get_filename (message);
+    mime_node_context_t *mctx;
+    mime_node_t *root;
+    notmuch_status_t status;
+
+    root = talloc_zero (ctx, mime_node_t);
+    if (root == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    /* Create the tree-wide context */
+    mctx = talloc_zero (root, mime_node_context_t);
+    if (mctx == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+    talloc_set_destructor (mctx, _mime_node_context_free);
+
+    mctx->file = fopen (filename, "r");
+    if (! mctx->file) {
+	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
+	status = NOTMUCH_STATUS_FILE_ERROR;
+	goto DONE;
+    }
+
+    mctx->stream = g_mime_stream_file_new (mctx->file);
+    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx->stream), FALSE);
+
+    mctx->parser = g_mime_parser_new_with_stream (mctx->stream);
+
+    mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
+
+    mctx->cryptoctx = cryptoctx;
+    mctx->decrypt = decrypt;
+
+    /* Create the root node */
+    root->part = GMIME_OBJECT (mctx->mime_message);
+    root->envelope_file = message;
+    root->children = 1;
+    root->ctx = mctx;
+
+    *root_out = root;
+    return NOTMUCH_STATUS_SUCCESS;
+
+DONE:
+    talloc_free (root);
+    return status;
+}
+
+static int
+_signature_validity_free (GMimeSignatureValidity **proxy)
+{
+    g_mime_signature_validity_free (*proxy);
+    return 0;
+}
+
+static mime_node_t *
+_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+{
+    mime_node_t *out = talloc_zero (parent, mime_node_t);
+    GError *err = NULL;
+
+    /* Set basic node properties */
+    out->part = part;
+    out->ctx = parent->ctx;
+    if (!talloc_reference (out, out->ctx)) {
+	fprintf (stderr, "Out of memory.\n");
+	talloc_free (out);
+	return NULL;
+    }
+
+    /* Deal with the different types of parts */
+    if (GMIME_IS_PART (part)) {
+	out->children = 0;
+    } else if (GMIME_IS_MULTIPART (part)) {
+	out->children = g_mime_multipart_get_count (GMIME_MULTIPART (part));
+    } else if (GMIME_IS_MESSAGE_PART (part)) {
+	/* Promote part to an envelope and open it */
+	GMimeMessagePart *message_part = GMIME_MESSAGE_PART (part);
+	GMimeMessage *message = g_mime_message_part_get_message (message_part);
+	out->envelope_part = message_part;
+	out->part = GMIME_OBJECT (message);
+	out->children = 1;
+    } else {
+	fprintf (stderr, "Warning: Unknown mime part type: %s.\n",
+		 g_type_name (G_OBJECT_TYPE (part)));
+	talloc_free (out);
+	return NULL;
+    }
+
+    /* Handle PGP/MIME parts */
+    if (GMIME_IS_MULTIPART_ENCRYPTED (part) && out->ctx->decrypt) {
+	if (out->children != 2) {
+	    /* this violates RFC 3156 section 4, so we won't bother with it. */
+	    fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
+		     "message (should be exactly 2)\n",
+		     out->children);
+	} else {
+	    out->is_encrypted = TRUE;
+	    GMimeMultipartEncrypted *encrypteddata =
+		GMIME_MULTIPART_ENCRYPTED (part);
+	    out->decrypted_child = g_mime_multipart_encrypted_decrypt
+		(encrypteddata, out->ctx->cryptoctx, &err);
+	    if (out->decrypted_child) {
+		out->decrypt_success = TRUE;
+		out->is_signed = TRUE;
+		out->sig_validity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
+	    } else {
+		fprintf (stderr, "Failed to decrypt part: %s\n",
+			 (err ? err->message : "no error explanation given"));
+	    }
+	}
+    } else if (GMIME_IS_MULTIPART_SIGNED (part) && out->ctx->cryptoctx) {
+	if (out->children != 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 "
+		     "(should be exactly 2)\n",
+		     out->children);
+	} else {
+	    out->is_signed = TRUE;
+	    /* For some reason the GMimeSignatureValidity returned
+	     * here is not a const (inconsistent with that
+	     * returned by
+	     * g_mime_multipart_encrypted_get_signature_validity,
+	     * and therefore needs to be properly disposed of.
+	     * Hopefully the API will become more consistent. */
+	    GMimeSignatureValidity *sig_validity = g_mime_multipart_signed_verify
+		(GMIME_MULTIPART_SIGNED (part), out->ctx->cryptoctx, &err);
+	    out->sig_validity = sig_validity;
+	    if (sig_validity) {
+		GMimeSignatureValidity **proxy =
+		    talloc (out, GMimeSignatureValidity *);
+		*proxy = sig_validity;
+		talloc_set_destructor (proxy, _signature_validity_free);
+	    }
+	}
+    }
+
+    if (out->is_signed && !out->sig_validity)
+	fprintf (stderr, "Failed to verify signed part: %s\n",
+		 (err ? err->message : "no error explanation given"));
+
+    if (err)
+	g_error_free (err);
+
+    return out;
+}
+
+mime_node_t *
+mime_node_child (const mime_node_t *parent, int child)
+{
+    if (!parent || child < 0 || child >= parent->children)
+	return NULL;
+
+    if (GMIME_IS_MULTIPART (parent->part)) {
+	GMimeMultipart *multipart = GMIME_MULTIPART (parent->part);
+	if (child == 1 && parent->decrypted_child)
+	    return _mime_node_create (parent, parent->decrypted_child);
+	return _mime_node_create (parent, g_mime_multipart_get_part (multipart, child));
+    } else if (GMIME_IS_MESSAGE (parent->part)) {
+	GMimeMessage *message = GMIME_MESSAGE (parent->part);
+	GMimeObject *child = g_mime_message_get_mime_part (message);
+	return _mime_node_create (parent, child);
+    } else {
+	/* This should have been caught by message_part_create */
+	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
+			g_type_name (G_OBJECT_TYPE (parent->part)));
+    }
+}
diff --git a/notmuch-client.h b/notmuch-client.h
index d7fb6ee..752c234 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -238,4 +238,84 @@ notmuch_config_set_maildir_synchronize_flags (notmuch_config_t *config,
 notmuch_bool_t
 debugger_is_active (void);
 
+/* mime-node.c */
+
+/* mime_node_t represents a single node in a MIME tree.  A MIME tree
+ * abstracts the different ways of traversing different types of MIME
+ * parts, allowing a MIME message to be viewed as a generic tree of
+ * parts.  Message-type parts have one child, multipart-type parts
+ * have multiple children, and leaf parts have zero children.
+ */
+typedef struct mime_node {
+    /* The MIME object of this part.  This will be a GMimeMessage,
+     * GMimePart, GMimeMultipart, or a subclass of one of these.
+     *
+     * This will never be a GMimeMessagePart because GMimeMessagePart
+     * is structurally redundant with GMimeMessage.  If this part is a
+     * message (that is, 'part' is a GMimeMessage), then either
+     * envelope_file will be set to a notmuch_message_t (for top-level
+     * messages) or envelope_part will be set to a GMimeMessagePart
+     * (for embedded message parts).
+     */
+    GMimeObject *part;
+
+    /* If part is a GMimeMessage, these record the envelope of the
+     * message: either a notmuch_message_t representing a top-level
+     * message, or a GMimeMessagePart representing a MIME part
+     * containing a message.
+     */
+    notmuch_message_t *envelope_file;
+    GMimeMessagePart *envelope_part;
+
+    /* The number of children of this part. */
+    int children;
+
+    /* True if this is the container for an encrypted or signed part.
+     * This does *not* mean that decryption or signature verification
+     * succeeded. */
+    notmuch_bool_t is_encrypted, is_signed;
+    /* True if decryption of this part's child succeeded.  In this
+     * case, the decrypted part is substituted for the second child of
+     * this part (which would usually be the encrypted data). */
+    notmuch_bool_t decrypt_success;
+    /* For signed or encrypted containers, the validity of the
+     * signature.  May be NULL if signature verification failed. */
+    const GMimeSignatureValidity *sig_validity;
+
+    /* Internal: Context inherited from the root iterator. */
+    struct mime_node_context *ctx;
+
+    /* Internal: For successfully decrypted multipart parts, the
+     * decrypted part to substitute for the second child. */
+    GMimeObject *decrypted_child;
+} mime_node_t;
+
+/* 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.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Root node is returned in *node_out.
+ *
+ * NOTMUCH_STATUS_FILE_ERROR: Failed to open message file.
+ *
+ * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
+ */
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+		GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
+		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
+ * node.
+ *
+ * In case of any failure, this function returns NULL, (after printing
+ * an error message on stderr).
+ */
+mime_node_t *
+mime_node_child (const mime_node_t *parent, int child);
+
 #endif
-- 
1.7.5.4

[PATCH 3/4] Utility function to seek in MIME trees in depth-first order.

[Austin Clements]

This function matches how we number parts for the --part argument to
show.  It will allow us to jump directly to the desired part, rather
than traversing the entire tree and carefully tracking whether or not
we're "in the zone".
---
 mime-node.c      |   25 +++++++++++++++++++++++++
 notmuch-client.h |    5 +++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index a8e4a59..207818e 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -232,3 +232,28 @@ mime_node_child (const mime_node_t *parent, int child)
 			g_type_name (G_OBJECT_TYPE (parent->part)));
     }
 }
+
+static mime_node_t *
+_mime_node_seek_dfs_walk (mime_node_t *node, int *n)
+{
+    mime_node_t *ret = NULL;
+    int i;
+
+    if (*n <= 0)
+	return node;
+
+    *n = *n - 1;
+    for (i = 0; i < node->children && !ret; i++) {
+	mime_node_t *child = mime_node_child (node, i);
+	ret = _mime_node_seek_dfs_walk (child, n);
+	if (!ret)
+	    talloc_free (child);
+    }
+    return ret;
+}
+
+mime_node_t *
+mime_node_seek_dfs (mime_node_t *node, int n)
+{
+    return _mime_node_seek_dfs_walk (node, &n);
+}
diff --git a/notmuch-client.h b/notmuch-client.h
index 752c234..5482969 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -318,4 +318,9 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
 mime_node_t *
 mime_node_child (const mime_node_t *parent, int child);
 
+/* Return the nth child of node in a depth-first traversal.  If n is
+ * 0, returns node itself.  Returns NULL if there is no such part. */
+mime_node_t *
+mime_node_seek_dfs (mime_node_t *node, int n);
+
 #endif
-- 
1.7.5.4

[PATCH 4/4] show: Rewrite show_message_body to use the MIME tree interface.

[Austin Clements]

This removes all of the MIME traversal logic from show_message_body
and leaves only its interaction with the format callbacks.

Besides isolating concerns, since traversal happens behind a trivial
interface, there is now much less code duplication in
show_message_part.  Also, this uses mime_node_seek_dfs to start at the
requested part, eliminating all of the logic about parts being
selected or being in_zone (and reducing the "show state" to only a
part counter).  notmuch_show_params_t no longer needs to be passed
through the recursion because the only two fields that mattered
(related to crypto) are now handled by the MIME tree.

The few remaining complexities in show_message_part highlight
irregularities in the format callbacks with respect to top-level
messages and embedded message parts.

Since this is a rewrite, the diff is not very enlightening.  It's
easier to look at the old code and the new code side-by-side.
---
 show-message.c |  329 +++++++++++++++++--------------------------------------
 1 files changed, 102 insertions(+), 227 deletions(-)
 rewrite show-message.c (81%)

diff --git a/show-message.c b/show-message.c
dissimilarity index 81%
index 09fa607..4560aea 100644
--- a/show-message.c
+++ b/show-message.c
@@ -1,227 +1,102 @@
-/* notmuch - Not much of an email program, (just index and search)
- *
- * Copyright © 2009 Carl Worth
- * Copyright © 2009 Keith Packard
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation, either version 3 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see http://www.gnu.org/licenses/ .
- *
- * Authors: Carl Worth <cworth@cworth.org>
- *	    Keith Packard <keithp@keithp.com>
- */
-
-#include "notmuch-client.h"
-
-typedef struct show_message_state {
-    int part_count;
-    int in_zone;
-} show_message_state_t;
-
-static void
-show_message_part (GMimeObject *part,
-		   show_message_state_t *state,
-		   const notmuch_show_format_t *format,
-		   notmuch_show_params_t *params,
-		   int first)
-{
-    GMimeObject *decryptedpart = NULL;
-    int selected;
-    state->part_count += 1;
-
-    if (! (GMIME_IS_PART (part) || GMIME_IS_MULTIPART (part) || GMIME_IS_MESSAGE_PART (part))) {
-	fprintf (stderr, "Warning: Not displaying unknown mime part: %s.\n",
-		 g_type_name (G_OBJECT_TYPE (part)));
-	return;
-    }
-
-    selected = (params->part <= 0 || state->part_count == params->part);
-
-    if (selected || state->in_zone) {
-	if (!first && (params->part <= 0 || state->in_zone))
-	    fputs (format->part_sep, stdout);
-
-	if (format->part_start)
-	    format->part_start (part, &(state->part_count));
-    }
-
-    /* handle PGP/MIME parts */
-    if (GMIME_IS_MULTIPART (part) && params->cryptoctx) {
-	GMimeMultipart *multipart = GMIME_MULTIPART (part);
-	GError* err = NULL;
-
-	if (GMIME_IS_MULTIPART_ENCRYPTED (part) && params->decrypt)
-	{
-	    if ( g_mime_multipart_get_count (multipart) != 2 ) {
-		/* this violates RFC 3156 section 4, so we won't bother with it. */
-		fprintf (stderr,
-			 "Error: %d part(s) for a multipart/encrypted message (should be exactly 2)\n",
-			 g_mime_multipart_get_count (multipart));
-	    } else {
-		GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
-		decryptedpart = g_mime_multipart_encrypted_decrypt (encrypteddata, params->cryptoctx, &err);
-		if (decryptedpart) {
-		    if ((selected || state->in_zone) && format->part_encstatus)
-			format->part_encstatus (1);
-		    const GMimeSignatureValidity *sigvalidity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
-		    if (!sigvalidity)
-			fprintf (stderr, "Failed to verify signed part: %s\n", (err ? err->message : "no error explanation given"));
-		    if ((selected || state->in_zone) && format->part_sigstatus)
-			format->part_sigstatus (sigvalidity);
-		} else {
-		    fprintf (stderr, "Failed to decrypt part: %s\n", (err ? err->message : "no error explanation given"));
-		    if ((selected || state->in_zone) && format->part_encstatus)
-			format->part_encstatus (0);
-		}
-	    }
-	}
-	else if (GMIME_IS_MULTIPART_SIGNED (part))
-	{
-	    if ( g_mime_multipart_get_count (multipart) != 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 (should be exactly 2)\n",
-			 g_mime_multipart_get_count (multipart));
-	    } else {
-		/* For some reason the GMimeSignatureValidity returned
-		 * here is not a const (inconsistent with that
-		 * returned by
-		 * g_mime_multipart_encrypted_get_signature_validity,
-		 * and therefore needs to be properly disposed of.
-		 * Hopefully the API will become more consistent. */
-		GMimeSignatureValidity *sigvalidity = g_mime_multipart_signed_verify (GMIME_MULTIPART_SIGNED (part), params->cryptoctx, &err);
-		if (!sigvalidity) {
-		    fprintf (stderr, "Failed to verify signed part: %s\n", (err ? err->message : "no error explanation given"));
-		}
-		if ((selected || state->in_zone) && format->part_sigstatus)
-		    format->part_sigstatus (sigvalidity);
-		if (sigvalidity)
-		    g_mime_signature_validity_free (sigvalidity);
-	    }
-	}
-
-	if (err)
-	    g_error_free (err);
-    }
-    /* end handle PGP/MIME parts */
-
-    if (selected || state->in_zone)
-	format->part_content (part);
-
-    if (GMIME_IS_MULTIPART (part)) {
-	GMimeMultipart *multipart = GMIME_MULTIPART (part);
-	int i;
-
-	if (selected)
-	    state->in_zone = 1;
-
-	if (decryptedpart) {
-	    /* We emit the useless application/pgp-encrypted version
-	     * part here only to keep the emitted output as consistent
-	     * as possible between decrypted output and the
-	     * unprocessed multipart/mime. For some strange reason,
-	     * the actual encrypted data is the second part of the
-	     * multipart. */
-	    show_message_part (g_mime_multipart_get_part (multipart, 0), state, format, params, TRUE);
-	    show_message_part (decryptedpart, state, format, params, FALSE);
-	} else {
-	    for (i = 0; i < g_mime_multipart_get_count (multipart); i++) {
-		show_message_part (g_mime_multipart_get_part (multipart, i),
-				   state, format, params, i == 0);
-	    }
-	}
-
-	if (selected)
-	    state->in_zone = 0;
-
-    } else if (GMIME_IS_MESSAGE_PART (part)) {
-	GMimeMessage *mime_message = g_mime_message_part_get_message (GMIME_MESSAGE_PART (part));
-
-	if (selected)
-	    state->in_zone = 1;
-
-	if (selected || (!selected && state->in_zone)) {
-	    fputs (format->header_start, stdout);
-	    if (format->header_message_part)
-		format->header_message_part (mime_message);
-	    fputs (format->header_end, stdout);
-
-	    fputs (format->body_start, stdout);
-	}
-
-	show_message_part (g_mime_message_get_mime_part (mime_message),
-			   state, format, params, TRUE);
-
-	if (selected || (!selected && state->in_zone))
-	    fputs (format->body_end, stdout);
-
-	if (selected)
-	    state->in_zone = 0;
-    }
-
-    if (selected || state->in_zone) {
-	if (format->part_end)
-	    format->part_end (part);
-    }
-}
-
-notmuch_status_t
-show_message_body (notmuch_message_t *message,
-		   const notmuch_show_format_t *format,
-		   notmuch_show_params_t *params)
-{
-    GMimeStream *stream = NULL;
-    GMimeParser *parser = NULL;
-    GMimeMessage *mime_message = NULL;
-    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
-    const char *filename = notmuch_message_get_filename (message);
-    FILE *file = NULL;
-    show_message_state_t state;
-
-    state.part_count = 0;
-    state.in_zone = 0;
-
-    file = fopen (filename, "r");
-    if (! file) {
-	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
-	ret = NOTMUCH_STATUS_FILE_ERROR;
-	goto DONE;
-    }
-
-    stream = g_mime_stream_file_new (file);
-    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream), FALSE);
-
-    parser = g_mime_parser_new_with_stream (stream);
-
-    mime_message = g_mime_parser_construct_message (parser);
-
-    show_message_part (g_mime_message_get_mime_part (mime_message),
-		       &state,
-		       format,
-		       params,
-		       TRUE);
-
-  DONE:
-    if (mime_message)
-	g_object_unref (mime_message);
-
-    if (parser)
-	g_object_unref (parser);
-
-    if (stream)
-	g_object_unref (stream);
-
-    if (file)
-	fclose (file);
-
-    return ret;
-}
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2009 Carl Worth
+ * Copyright © 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth <cworth@cworth.org>
+ *	    Keith Packard <keithp@keithp.com>
+ */
+
+#include "notmuch-client.h"
+
+typedef struct show_message_state {
+    int part_count;
+} show_message_state_t;
+
+static void
+show_message_part (mime_node_t *node,
+		   show_message_state_t *state,
+		   const notmuch_show_format_t *format,
+		   int first)
+{
+    /* Formatters expect the envelope for embedded message parts */
+    GMimeObject *part = node->envelope_part ?
+	GMIME_OBJECT (node->envelope_part) : node->part;
+    int i;
+
+    if (!first)
+	fputs (format->part_sep, stdout);
+
+    /* Format this part */
+    if (format->part_start)
+	format->part_start (part, &(state->part_count));
+
+    if (node->is_encrypted && format->part_encstatus)
+	format->part_encstatus (node->decrypt_success);
+
+    if (node->is_signed && format->part_sigstatus)
+	format->part_sigstatus (node->sig_validity);
+
+    format->part_content (part);
+
+    if (node->envelope_part) {
+	fputs (format->header_start, stdout);
+	if (format->header_message_part)
+	    format->header_message_part (GMIME_MESSAGE (node->part));
+	fputs (format->header_end, stdout);
+
+	fputs (format->body_start, stdout);
+    }
+
+    /* Recurse over the children */
+    state->part_count += 1;
+    for (i = 0; i < node->children; i++)
+	show_message_part (mime_node_child (node, i), state, format, i == 0);
+
+    /* Finish this part */
+    if (node->envelope_part)
+	fputs (format->body_end, stdout);
+
+    if (format->part_end)
+	format->part_end (part);
+}
+
+notmuch_status_t
+show_message_body (notmuch_message_t *message,
+		   const notmuch_show_format_t *format,
+		   notmuch_show_params_t *params)
+{
+    notmuch_status_t ret;
+    show_message_state_t state;
+    mime_node_t *root, *part;
+
+    ret = mime_node_open (NULL, message, params->cryptoctx, params->decrypt,
+			  &root);
+    if (ret)
+	return ret;
+
+    /* The caller of show_message_body has already handled the
+     * outermost envelope, so skip it. */
+    state.part_count = MAX (params->part, 1);
+
+    part = mime_node_seek_dfs (root, state.part_count);
+    if (part)
+	show_message_part (part, &state, format, TRUE);
+
+    talloc_free (root);
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
-- 
1.7.5.4

Re: [PATCH v2 0/4] First step of 'show' rewrite

[Austin Clements]

Just a reminder that this series awaits review and is the first step
in a better, simpler, and cleaner show that will make possible things
like improvements to the JSON format, better encoding handling, proper
raw support, and hierarchical part numbering.

(I/git screwed up the CC line on the original patch series email.  It
was supposed to be CC'd to everyone who had expressed an interest in
this; sorry for any confusion.)

Quoth myself on Dec 04 at  2:31 pm:
> This version addresses the comments in Jani's review
> (id:8739d6u4ju.fsf@nikula.org).

Re: [PATCH v2 0/4] First step of 'show' rewrite

[Dmitry Kurochkin]

Hi Austin.

On Fri, 9 Dec 2011 12:39:13 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Just a reminder that this series awaits review and is the first step
> in a better, simpler, and cleaner show that will make possible things
> like improvements to the JSON format, better encoding handling, proper
> raw support, and hierarchical part numbering.
> 

Thank you very much for this work.  And shame on me for not reviewing
it.  But I have been too busy with other stuff lately.  I hope I can get
to it this weekend.

Regards,
  Dmitry

> (I/git screwed up the CC line on the original patch series email.  It
> was supposed to be CC'd to everyone who had expressed an interest in
> this; sorry for any confusion.)
> 
> Quoth myself on Dec 04 at  2:31 pm:
> > This version addresses the comments in Jani's review
> > (id:8739d6u4ju.fsf@nikula.org).

Re: [PATCH v2 0/4] First step of 'show' rewrite

[Jameson Graef Rollins]

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

On Fri, 09 Dec 2011 21:40:05 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Thank you very much for this work.  And shame on me for not reviewing
> it.  But I have been too busy with other stuff lately.  I hope I can get
> to it this weekend.

ditto.

jamie.

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

Re: [PATCH 1/4] show: Pass notmuch_message_t instead of path to show_message_body.

[Dmitry Kurochkin]

On Sun,  4 Dec 2011 14:31:37 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> In addition to simplifying the code, we'll need the notmuch_message_t*
> in show_message_body shortly.
> ---
>  notmuch-client.h |    2 +-
>  notmuch-reply.c  |    3 +--
>  notmuch-show.c   |    3 +--
>  show-message.c   |    3 ++-
>  4 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/notmuch-client.h b/notmuch-client.h
> index b50cb38..d7fb6ee 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -162,7 +162,7 @@ char *
>  query_string_from_args (void *ctx, int argc, char *argv[]);
>  
>  notmuch_status_t
> -show_message_body (const char *filename,
> +show_message_body (notmuch_message_t *message,
>  		   const notmuch_show_format_t *format,
>  		   notmuch_show_params_t *params);
>  
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 7ac879f..f8d5f64 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -546,8 +546,7 @@ notmuch_reply_format_default(void *ctx,
>  		notmuch_message_get_header (message, "date"),
>  		notmuch_message_get_header (message, "from"));
>  
> -	show_message_body (notmuch_message_get_filename (message),
> -			   format, params);
> +	show_message_body (message, format, params);
>  
>  	notmuch_message_destroy (message);
>      }
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 603992a..1dee3aa 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -753,8 +753,7 @@ show_message (void *ctx,
>      }
>  
>      if (format->part_content)
> -	show_message_body (notmuch_message_get_filename (message),
> -			   format, params);
> +	show_message_body (message, format, params);
>  
>      if (params->part <= 0) {
>  	fputs (format->body_end, stdout);
> diff --git a/show-message.c b/show-message.c
> index d83f04e..09fa607 100644
> --- a/show-message.c
> +++ b/show-message.c
> @@ -175,7 +175,7 @@ show_message_part (GMimeObject *part,
>  }
>  
>  notmuch_status_t
> -show_message_body (const char *filename,
> +show_message_body (notmuch_message_t *message,
>  		   const notmuch_show_format_t *format,
>  		   notmuch_show_params_t *params)

Is show_message_body() (or functions that it calls/would call) supposed
to modify the message structure?  If not, we should make it const.

I would also make all pointers constant (i.e. const notmuch_message_t
*const message), but I can not insist since it is not common in notmuch.

Regards,
  Dmitry

>  {
> @@ -183,6 +183,7 @@ show_message_body (const char *filename,
>      GMimeParser *parser = NULL;
>      GMimeMessage *mime_message = NULL;
>      notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
> +    const char *filename = notmuch_message_get_filename (message);
>      FILE *file = NULL;
>      show_message_state_t state;
>  
> -- 
> 1.7.5.4
> 

[PATCH v3 0/4] First step of 'show' rewrite

[Austin Clements]

This is a rebase to current master that resolves some trivial ordering
merge conflicts in notmuch-client.h.

[PATCH 1/4] show: Pass notmuch_message_t instead of path to show_message_body.

[Austin Clements]

In addition to simplifying the code, we'll need the notmuch_message_t*
in show_message_body shortly.
---
 notmuch-client.h |    2 +-
 notmuch-reply.c  |    3 +--
 notmuch-show.c   |    3 +--
 show-message.c   |    3 ++-
 4 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 703f856..be21781 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -162,7 +162,7 @@ char *
 query_string_from_args (void *ctx, int argc, char *argv[]);
 
 notmuch_status_t
-show_message_body (const char *filename,
+show_message_body (notmuch_message_t *message,
 		   const notmuch_show_format_t *format,
 		   notmuch_show_params_t *params);
 
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 7ac879f..f8d5f64 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -546,8 +546,7 @@ notmuch_reply_format_default(void *ctx,
 		notmuch_message_get_header (message, "date"),
 		notmuch_message_get_header (message, "from"));
 
-	show_message_body (notmuch_message_get_filename (message),
-			   format, params);
+	show_message_body (message, format, params);
 
 	notmuch_message_destroy (message);
     }
diff --git a/notmuch-show.c b/notmuch-show.c
index 603992a..1dee3aa 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -753,8 +753,7 @@ show_message (void *ctx,
     }
 
     if (format->part_content)
-	show_message_body (notmuch_message_get_filename (message),
-			   format, params);
+	show_message_body (message, format, params);
 
     if (params->part <= 0) {
 	fputs (format->body_end, stdout);
diff --git a/show-message.c b/show-message.c
index d83f04e..09fa607 100644
--- a/show-message.c
+++ b/show-message.c
@@ -175,7 +175,7 @@ show_message_part (GMimeObject *part,
 }
 
 notmuch_status_t
-show_message_body (const char *filename,
+show_message_body (notmuch_message_t *message,
 		   const notmuch_show_format_t *format,
 		   notmuch_show_params_t *params)
 {
@@ -183,6 +183,7 @@ show_message_body (const char *filename,
     GMimeParser *parser = NULL;
     GMimeMessage *mime_message = NULL;
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
+    const char *filename = notmuch_message_get_filename (message);
     FILE *file = NULL;
     show_message_state_t state;
 
-- 
1.7.7.3

Re: [PATCH 1/4] show: Pass notmuch_message_t instead of path to show_message_body.

[Austin Clements]

Quoth Dmitry Kurochkin on Dec 09 at 11:05 pm:
> On Sun,  4 Dec 2011 14:31:37 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> >  }
> >  
> >  notmuch_status_t
> > -show_message_body (const char *filename,
> > +show_message_body (notmuch_message_t *message,
> >  		   const notmuch_show_format_t *format,
> >  		   notmuch_show_params_t *params)
> 
> Is show_message_body() (or functions that it calls/would call) supposed
> to modify the message structure?  If not, we should make it const.

That would be nice, but lack of const in libnotmuch makes it difficult
to do this (for example, notmuch_message_get_filename, which
show_message_body calls, takes a non-const notmuch_message_t *).

OTOH, since functions like notmuch_message_get_filename lazily compute
fields of notmuch_message_t and C has no equivalent of C++'s mutable,
it's not clear making the message const is even the right thing to do.

> I would also make all pointers constant (i.e. const notmuch_message_t
> *const message), but I can not insist since it is not common in notmuch.
> 
> Regards,
>   Dmitry

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

This wraps all of the complex MIME part handling in a single, simple
function that gets part N from *any* MIME object, so traversing a MIME
part tree becomes a two-line for loop.  Furthermore, the MIME node
structure provides easy access to envelopes for message parts as well
as cryptographic information.

This code is directly derived from the current show_message_body code
(much of it is identical), but the control relation is inverted:
instead of show_message_body controlling the traversal of the MIME
structure and invoking callbacks, the caller controls the traversal of
the MIME structure.
---
 Makefile.local   |    1 +
 mime-node.c      |  234 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 notmuch-client.h |   80 ++++++++++++++++++
 3 files changed, 315 insertions(+), 0 deletions(-)
 create mode 100644 mime-node.c

diff --git a/Makefile.local b/Makefile.local
index 28e371a..aeb068d 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -313,6 +313,7 @@ notmuch_client_srcs =		\
 	notmuch-time.c		\
 	query-string.c		\
 	show-message.c		\
+	mime-node.c		\
 	json.c
 
 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
diff --git a/mime-node.c b/mime-node.c
new file mode 100644
index 0000000..a8e4a59
--- /dev/null
+++ b/mime-node.c
@@ -0,0 +1,234 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2009 Carl Worth
+ * Copyright © 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth <cworth@cworth.org>
+ *          Keith Packard <keithp@keithp.com>
+ *          Austin Clements <aclements@csail.mit.edu>
+ */
+
+#include "notmuch-client.h"
+
+/* Context that gets inherited from the root node. */
+typedef struct mime_node_context {
+    /* Per-message resources.  These are allocated internally and must
+     * be destroyed. */
+    FILE *file;
+    GMimeStream *stream;
+    GMimeParser *parser;
+    GMimeMessage *mime_message;
+
+    /* Context provided by the caller. */
+    GMimeCipherContext *cryptoctx;
+    notmuch_bool_t decrypt;
+} mime_node_context_t;
+
+static int
+_mime_node_context_free (mime_node_context_t *res)
+{
+    if (res->mime_message)
+	g_object_unref (res->mime_message);
+
+    if (res->parser)
+	g_object_unref (res->parser);
+
+    if (res->stream)
+	g_object_unref (res->stream);
+
+    if (res->file)
+	fclose (res->file);
+
+    return 0;
+}
+
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+		GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
+		mime_node_t **root_out)
+{
+    const char *filename = notmuch_message_get_filename (message);
+    mime_node_context_t *mctx;
+    mime_node_t *root;
+    notmuch_status_t status;
+
+    root = talloc_zero (ctx, mime_node_t);
+    if (root == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    /* Create the tree-wide context */
+    mctx = talloc_zero (root, mime_node_context_t);
+    if (mctx == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+    talloc_set_destructor (mctx, _mime_node_context_free);
+
+    mctx->file = fopen (filename, "r");
+    if (! mctx->file) {
+	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
+	status = NOTMUCH_STATUS_FILE_ERROR;
+	goto DONE;
+    }
+
+    mctx->stream = g_mime_stream_file_new (mctx->file);
+    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx->stream), FALSE);
+
+    mctx->parser = g_mime_parser_new_with_stream (mctx->stream);
+
+    mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
+
+    mctx->cryptoctx = cryptoctx;
+    mctx->decrypt = decrypt;
+
+    /* Create the root node */
+    root->part = GMIME_OBJECT (mctx->mime_message);
+    root->envelope_file = message;
+    root->children = 1;
+    root->ctx = mctx;
+
+    *root_out = root;
+    return NOTMUCH_STATUS_SUCCESS;
+
+DONE:
+    talloc_free (root);
+    return status;
+}
+
+static int
+_signature_validity_free (GMimeSignatureValidity **proxy)
+{
+    g_mime_signature_validity_free (*proxy);
+    return 0;
+}
+
+static mime_node_t *
+_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+{
+    mime_node_t *out = talloc_zero (parent, mime_node_t);
+    GError *err = NULL;
+
+    /* Set basic node properties */
+    out->part = part;
+    out->ctx = parent->ctx;
+    if (!talloc_reference (out, out->ctx)) {
+	fprintf (stderr, "Out of memory.\n");
+	talloc_free (out);
+	return NULL;
+    }
+
+    /* Deal with the different types of parts */
+    if (GMIME_IS_PART (part)) {
+	out->children = 0;
+    } else if (GMIME_IS_MULTIPART (part)) {
+	out->children = g_mime_multipart_get_count (GMIME_MULTIPART (part));
+    } else if (GMIME_IS_MESSAGE_PART (part)) {
+	/* Promote part to an envelope and open it */
+	GMimeMessagePart *message_part = GMIME_MESSAGE_PART (part);
+	GMimeMessage *message = g_mime_message_part_get_message (message_part);
+	out->envelope_part = message_part;
+	out->part = GMIME_OBJECT (message);
+	out->children = 1;
+    } else {
+	fprintf (stderr, "Warning: Unknown mime part type: %s.\n",
+		 g_type_name (G_OBJECT_TYPE (part)));
+	talloc_free (out);
+	return NULL;
+    }
+
+    /* Handle PGP/MIME parts */
+    if (GMIME_IS_MULTIPART_ENCRYPTED (part) && out->ctx->decrypt) {
+	if (out->children != 2) {
+	    /* this violates RFC 3156 section 4, so we won't bother with it. */
+	    fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
+		     "message (should be exactly 2)\n",
+		     out->children);
+	} else {
+	    out->is_encrypted = TRUE;
+	    GMimeMultipartEncrypted *encrypteddata =
+		GMIME_MULTIPART_ENCRYPTED (part);
+	    out->decrypted_child = g_mime_multipart_encrypted_decrypt
+		(encrypteddata, out->ctx->cryptoctx, &err);
+	    if (out->decrypted_child) {
+		out->decrypt_success = TRUE;
+		out->is_signed = TRUE;
+		out->sig_validity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
+	    } else {
+		fprintf (stderr, "Failed to decrypt part: %s\n",
+			 (err ? err->message : "no error explanation given"));
+	    }
+	}
+    } else if (GMIME_IS_MULTIPART_SIGNED (part) && out->ctx->cryptoctx) {
+	if (out->children != 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 "
+		     "(should be exactly 2)\n",
+		     out->children);
+	} else {
+	    out->is_signed = TRUE;
+	    /* For some reason the GMimeSignatureValidity returned
+	     * here is not a const (inconsistent with that
+	     * returned by
+	     * g_mime_multipart_encrypted_get_signature_validity,
+	     * and therefore needs to be properly disposed of.
+	     * Hopefully the API will become more consistent. */
+	    GMimeSignatureValidity *sig_validity = g_mime_multipart_signed_verify
+		(GMIME_MULTIPART_SIGNED (part), out->ctx->cryptoctx, &err);
+	    out->sig_validity = sig_validity;
+	    if (sig_validity) {
+		GMimeSignatureValidity **proxy =
+		    talloc (out, GMimeSignatureValidity *);
+		*proxy = sig_validity;
+		talloc_set_destructor (proxy, _signature_validity_free);
+	    }
+	}
+    }
+
+    if (out->is_signed && !out->sig_validity)
+	fprintf (stderr, "Failed to verify signed part: %s\n",
+		 (err ? err->message : "no error explanation given"));
+
+    if (err)
+	g_error_free (err);
+
+    return out;
+}
+
+mime_node_t *
+mime_node_child (const mime_node_t *parent, int child)
+{
+    if (!parent || child < 0 || child >= parent->children)
+	return NULL;
+
+    if (GMIME_IS_MULTIPART (parent->part)) {
+	GMimeMultipart *multipart = GMIME_MULTIPART (parent->part);
+	if (child == 1 && parent->decrypted_child)
+	    return _mime_node_create (parent, parent->decrypted_child);
+	return _mime_node_create (parent, g_mime_multipart_get_part (multipart, child));
+    } else if (GMIME_IS_MESSAGE (parent->part)) {
+	GMimeMessage *message = GMIME_MESSAGE (parent->part);
+	GMimeObject *child = g_mime_message_get_mime_part (message);
+	return _mime_node_create (parent, child);
+    } else {
+	/* This should have been caught by message_part_create */
+	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
+			g_type_name (G_OBJECT_TYPE (parent->part)));
+    }
+}
diff --git a/notmuch-client.h b/notmuch-client.h
index be21781..fce1187 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -238,5 +238,85 @@ notmuch_config_set_maildir_synchronize_flags (notmuch_config_t *config,
 notmuch_bool_t
 debugger_is_active (void);
 
+/* mime-node.c */
+
+/* mime_node_t represents a single node in a MIME tree.  A MIME tree
+ * abstracts the different ways of traversing different types of MIME
+ * parts, allowing a MIME message to be viewed as a generic tree of
+ * parts.  Message-type parts have one child, multipart-type parts
+ * have multiple children, and leaf parts have zero children.
+ */
+typedef struct mime_node {
+    /* The MIME object of this part.  This will be a GMimeMessage,
+     * GMimePart, GMimeMultipart, or a subclass of one of these.
+     *
+     * This will never be a GMimeMessagePart because GMimeMessagePart
+     * is structurally redundant with GMimeMessage.  If this part is a
+     * message (that is, 'part' is a GMimeMessage), then either
+     * envelope_file will be set to a notmuch_message_t (for top-level
+     * messages) or envelope_part will be set to a GMimeMessagePart
+     * (for embedded message parts).
+     */
+    GMimeObject *part;
+
+    /* If part is a GMimeMessage, these record the envelope of the
+     * message: either a notmuch_message_t representing a top-level
+     * message, or a GMimeMessagePart representing a MIME part
+     * containing a message.
+     */
+    notmuch_message_t *envelope_file;
+    GMimeMessagePart *envelope_part;
+
+    /* The number of children of this part. */
+    int children;
+
+    /* True if this is the container for an encrypted or signed part.
+     * This does *not* mean that decryption or signature verification
+     * succeeded. */
+    notmuch_bool_t is_encrypted, is_signed;
+    /* True if decryption of this part's child succeeded.  In this
+     * case, the decrypted part is substituted for the second child of
+     * this part (which would usually be the encrypted data). */
+    notmuch_bool_t decrypt_success;
+    /* For signed or encrypted containers, the validity of the
+     * signature.  May be NULL if signature verification failed. */
+    const GMimeSignatureValidity *sig_validity;
+
+    /* Internal: Context inherited from the root iterator. */
+    struct mime_node_context *ctx;
+
+    /* Internal: For successfully decrypted multipart parts, the
+     * decrypted part to substitute for the second child. */
+    GMimeObject *decrypted_child;
+} mime_node_t;
+
+/* 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.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Root node is returned in *node_out.
+ *
+ * NOTMUCH_STATUS_FILE_ERROR: Failed to open message file.
+ *
+ * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
+ */
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+		GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
+		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
+ * node.
+ *
+ * In case of any failure, this function returns NULL, (after printing
+ * an error message on stderr).
+ */
+mime_node_t *
+mime_node_child (const mime_node_t *parent, int child);
+
 #include "command-line-arguments.h"
 #endif
-- 
1.7.7.3

[PATCH 3/4] Utility function to seek in MIME trees in depth-first order.

[Austin Clements]

This function matches how we number parts for the --part argument to
show.  It will allow us to jump directly to the desired part, rather
than traversing the entire tree and carefully tracking whether or not
we're "in the zone".
---
 mime-node.c      |   25 +++++++++++++++++++++++++
 notmuch-client.h |    5 +++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index a8e4a59..207818e 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -232,3 +232,28 @@ mime_node_child (const mime_node_t *parent, int child)
 			g_type_name (G_OBJECT_TYPE (parent->part)));
     }
 }
+
+static mime_node_t *
+_mime_node_seek_dfs_walk (mime_node_t *node, int *n)
+{
+    mime_node_t *ret = NULL;
+    int i;
+
+    if (*n <= 0)
+	return node;
+
+    *n = *n - 1;
+    for (i = 0; i < node->children && !ret; i++) {
+	mime_node_t *child = mime_node_child (node, i);
+	ret = _mime_node_seek_dfs_walk (child, n);
+	if (!ret)
+	    talloc_free (child);
+    }
+    return ret;
+}
+
+mime_node_t *
+mime_node_seek_dfs (mime_node_t *node, int n)
+{
+    return _mime_node_seek_dfs_walk (node, &n);
+}
diff --git a/notmuch-client.h b/notmuch-client.h
index fce1187..f215d4b 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -318,5 +318,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
 mime_node_t *
 mime_node_child (const mime_node_t *parent, int child);
 
+/* Return the nth child of node in a depth-first traversal.  If n is
+ * 0, returns node itself.  Returns NULL if there is no such part. */
+mime_node_t *
+mime_node_seek_dfs (mime_node_t *node, int n);
+
 #include "command-line-arguments.h"
 #endif
-- 
1.7.7.3

[PATCH 4/4] show: Rewrite show_message_body to use the MIME tree interface.

[Austin Clements]

This removes all of the MIME traversal logic from show_message_body
and leaves only its interaction with the format callbacks.

Besides isolating concerns, since traversal happens behind a trivial
interface, there is now much less code duplication in
show_message_part.  Also, this uses mime_node_seek_dfs to start at the
requested part, eliminating all of the logic about parts being
selected or being in_zone (and reducing the "show state" to only a
part counter).  notmuch_show_params_t no longer needs to be passed
through the recursion because the only two fields that mattered
(related to crypto) are now handled by the MIME tree.

The few remaining complexities in show_message_part highlight
irregularities in the format callbacks with respect to top-level
messages and embedded message parts.

Since this is a rewrite, the diff is not very enlightening.  It's
easier to look at the old code and the new code side-by-side.
---
 show-message.c |  329 +++++++++++++++++--------------------------------------
 1 files changed, 102 insertions(+), 227 deletions(-)
 rewrite show-message.c (81%)

diff --git a/show-message.c b/show-message.c
dissimilarity index 81%
index 09fa607..4560aea 100644
--- a/show-message.c
+++ b/show-message.c
@@ -1,227 +1,102 @@
-/* notmuch - Not much of an email program, (just index and search)
- *
- * Copyright © 2009 Carl Worth
- * Copyright © 2009 Keith Packard
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation, either version 3 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see http://www.gnu.org/licenses/ .
- *
- * Authors: Carl Worth <cworth@cworth.org>
- *	    Keith Packard <keithp@keithp.com>
- */
-
-#include "notmuch-client.h"
-
-typedef struct show_message_state {
-    int part_count;
-    int in_zone;
-} show_message_state_t;
-
-static void
-show_message_part (GMimeObject *part,
-		   show_message_state_t *state,
-		   const notmuch_show_format_t *format,
-		   notmuch_show_params_t *params,
-		   int first)
-{
-    GMimeObject *decryptedpart = NULL;
-    int selected;
-    state->part_count += 1;
-
-    if (! (GMIME_IS_PART (part) || GMIME_IS_MULTIPART (part) || GMIME_IS_MESSAGE_PART (part))) {
-	fprintf (stderr, "Warning: Not displaying unknown mime part: %s.\n",
-		 g_type_name (G_OBJECT_TYPE (part)));
-	return;
-    }
-
-    selected = (params->part <= 0 || state->part_count == params->part);
-
-    if (selected || state->in_zone) {
-	if (!first && (params->part <= 0 || state->in_zone))
-	    fputs (format->part_sep, stdout);
-
-	if (format->part_start)
-	    format->part_start (part, &(state->part_count));
-    }
-
-    /* handle PGP/MIME parts */
-    if (GMIME_IS_MULTIPART (part) && params->cryptoctx) {
-	GMimeMultipart *multipart = GMIME_MULTIPART (part);
-	GError* err = NULL;
-
-	if (GMIME_IS_MULTIPART_ENCRYPTED (part) && params->decrypt)
-	{
-	    if ( g_mime_multipart_get_count (multipart) != 2 ) {
-		/* this violates RFC 3156 section 4, so we won't bother with it. */
-		fprintf (stderr,
-			 "Error: %d part(s) for a multipart/encrypted message (should be exactly 2)\n",
-			 g_mime_multipart_get_count (multipart));
-	    } else {
-		GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
-		decryptedpart = g_mime_multipart_encrypted_decrypt (encrypteddata, params->cryptoctx, &err);
-		if (decryptedpart) {
-		    if ((selected || state->in_zone) && format->part_encstatus)
-			format->part_encstatus (1);
-		    const GMimeSignatureValidity *sigvalidity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
-		    if (!sigvalidity)
-			fprintf (stderr, "Failed to verify signed part: %s\n", (err ? err->message : "no error explanation given"));
-		    if ((selected || state->in_zone) && format->part_sigstatus)
-			format->part_sigstatus (sigvalidity);
-		} else {
-		    fprintf (stderr, "Failed to decrypt part: %s\n", (err ? err->message : "no error explanation given"));
-		    if ((selected || state->in_zone) && format->part_encstatus)
-			format->part_encstatus (0);
-		}
-	    }
-	}
-	else if (GMIME_IS_MULTIPART_SIGNED (part))
-	{
-	    if ( g_mime_multipart_get_count (multipart) != 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 (should be exactly 2)\n",
-			 g_mime_multipart_get_count (multipart));
-	    } else {
-		/* For some reason the GMimeSignatureValidity returned
-		 * here is not a const (inconsistent with that
-		 * returned by
-		 * g_mime_multipart_encrypted_get_signature_validity,
-		 * and therefore needs to be properly disposed of.
-		 * Hopefully the API will become more consistent. */
-		GMimeSignatureValidity *sigvalidity = g_mime_multipart_signed_verify (GMIME_MULTIPART_SIGNED (part), params->cryptoctx, &err);
-		if (!sigvalidity) {
-		    fprintf (stderr, "Failed to verify signed part: %s\n", (err ? err->message : "no error explanation given"));
-		}
-		if ((selected || state->in_zone) && format->part_sigstatus)
-		    format->part_sigstatus (sigvalidity);
-		if (sigvalidity)
-		    g_mime_signature_validity_free (sigvalidity);
-	    }
-	}
-
-	if (err)
-	    g_error_free (err);
-    }
-    /* end handle PGP/MIME parts */
-
-    if (selected || state->in_zone)
-	format->part_content (part);
-
-    if (GMIME_IS_MULTIPART (part)) {
-	GMimeMultipart *multipart = GMIME_MULTIPART (part);
-	int i;
-
-	if (selected)
-	    state->in_zone = 1;
-
-	if (decryptedpart) {
-	    /* We emit the useless application/pgp-encrypted version
-	     * part here only to keep the emitted output as consistent
-	     * as possible between decrypted output and the
-	     * unprocessed multipart/mime. For some strange reason,
-	     * the actual encrypted data is the second part of the
-	     * multipart. */
-	    show_message_part (g_mime_multipart_get_part (multipart, 0), state, format, params, TRUE);
-	    show_message_part (decryptedpart, state, format, params, FALSE);
-	} else {
-	    for (i = 0; i < g_mime_multipart_get_count (multipart); i++) {
-		show_message_part (g_mime_multipart_get_part (multipart, i),
-				   state, format, params, i == 0);
-	    }
-	}
-
-	if (selected)
-	    state->in_zone = 0;
-
-    } else if (GMIME_IS_MESSAGE_PART (part)) {
-	GMimeMessage *mime_message = g_mime_message_part_get_message (GMIME_MESSAGE_PART (part));
-
-	if (selected)
-	    state->in_zone = 1;
-
-	if (selected || (!selected && state->in_zone)) {
-	    fputs (format->header_start, stdout);
-	    if (format->header_message_part)
-		format->header_message_part (mime_message);
-	    fputs (format->header_end, stdout);
-
-	    fputs (format->body_start, stdout);
-	}
-
-	show_message_part (g_mime_message_get_mime_part (mime_message),
-			   state, format, params, TRUE);
-
-	if (selected || (!selected && state->in_zone))
-	    fputs (format->body_end, stdout);
-
-	if (selected)
-	    state->in_zone = 0;
-    }
-
-    if (selected || state->in_zone) {
-	if (format->part_end)
-	    format->part_end (part);
-    }
-}
-
-notmuch_status_t
-show_message_body (notmuch_message_t *message,
-		   const notmuch_show_format_t *format,
-		   notmuch_show_params_t *params)
-{
-    GMimeStream *stream = NULL;
-    GMimeParser *parser = NULL;
-    GMimeMessage *mime_message = NULL;
-    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
-    const char *filename = notmuch_message_get_filename (message);
-    FILE *file = NULL;
-    show_message_state_t state;
-
-    state.part_count = 0;
-    state.in_zone = 0;
-
-    file = fopen (filename, "r");
-    if (! file) {
-	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
-	ret = NOTMUCH_STATUS_FILE_ERROR;
-	goto DONE;
-    }
-
-    stream = g_mime_stream_file_new (file);
-    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream), FALSE);
-
-    parser = g_mime_parser_new_with_stream (stream);
-
-    mime_message = g_mime_parser_construct_message (parser);
-
-    show_message_part (g_mime_message_get_mime_part (mime_message),
-		       &state,
-		       format,
-		       params,
-		       TRUE);
-
-  DONE:
-    if (mime_message)
-	g_object_unref (mime_message);
-
-    if (parser)
-	g_object_unref (parser);
-
-    if (stream)
-	g_object_unref (stream);
-
-    if (file)
-	fclose (file);
-
-    return ret;
-}
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2009 Carl Worth
+ * Copyright © 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth <cworth@cworth.org>
+ *	    Keith Packard <keithp@keithp.com>
+ */
+
+#include "notmuch-client.h"
+
+typedef struct show_message_state {
+    int part_count;
+} show_message_state_t;
+
+static void
+show_message_part (mime_node_t *node,
+		   show_message_state_t *state,
+		   const notmuch_show_format_t *format,
+		   int first)
+{
+    /* Formatters expect the envelope for embedded message parts */
+    GMimeObject *part = node->envelope_part ?
+	GMIME_OBJECT (node->envelope_part) : node->part;
+    int i;
+
+    if (!first)
+	fputs (format->part_sep, stdout);
+
+    /* Format this part */
+    if (format->part_start)
+	format->part_start (part, &(state->part_count));
+
+    if (node->is_encrypted && format->part_encstatus)
+	format->part_encstatus (node->decrypt_success);
+
+    if (node->is_signed && format->part_sigstatus)
+	format->part_sigstatus (node->sig_validity);
+
+    format->part_content (part);
+
+    if (node->envelope_part) {
+	fputs (format->header_start, stdout);
+	if (format->header_message_part)
+	    format->header_message_part (GMIME_MESSAGE (node->part));
+	fputs (format->header_end, stdout);
+
+	fputs (format->body_start, stdout);
+    }
+
+    /* Recurse over the children */
+    state->part_count += 1;
+    for (i = 0; i < node->children; i++)
+	show_message_part (mime_node_child (node, i), state, format, i == 0);
+
+    /* Finish this part */
+    if (node->envelope_part)
+	fputs (format->body_end, stdout);
+
+    if (format->part_end)
+	format->part_end (part);
+}
+
+notmuch_status_t
+show_message_body (notmuch_message_t *message,
+		   const notmuch_show_format_t *format,
+		   notmuch_show_params_t *params)
+{
+    notmuch_status_t ret;
+    show_message_state_t state;
+    mime_node_t *root, *part;
+
+    ret = mime_node_open (NULL, message, params->cryptoctx, params->decrypt,
+			  &root);
+    if (ret)
+	return ret;
+
+    /* The caller of show_message_body has already handled the
+     * outermost envelope, so skip it. */
+    state.part_count = MAX (params->part, 1);
+
+    part = mime_node_seek_dfs (root, state.part_count);
+    if (part)
+	show_message_part (part, &state, format, TRUE);
+
+    talloc_free (root);
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
-- 
1.7.7.3

Re: [PATCH 1/4] show: Pass notmuch_message_t instead of path to show_message_body.

[Dmitry Kurochkin]

On Fri, 9 Dec 2011 14:54:26 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Dmitry Kurochkin on Dec 09 at 11:05 pm:
> > On Sun,  4 Dec 2011 14:31:37 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > >  }
> > >  
> > >  notmuch_status_t
> > > -show_message_body (const char *filename,
> > > +show_message_body (notmuch_message_t *message,
> > >  		   const notmuch_show_format_t *format,
> > >  		   notmuch_show_params_t *params)
> > 
> > Is show_message_body() (or functions that it calls/would call) supposed
> > to modify the message structure?  If not, we should make it const.
> 
> That would be nice, but lack of const in libnotmuch makes it difficult
> to do this (for example, notmuch_message_get_filename, which
> show_message_body calls, takes a non-const notmuch_message_t *).
> 
> OTOH, since functions like notmuch_message_get_filename lazily compute
> fields of notmuch_message_t and C has no equivalent of C++'s mutable,
> it's not clear making the message const is even the right thing to do.
> 

If there are fields that are computed lazily (I just did not know it),
we can not make it const.


The patch looks good.  I think it can be pushed before the rest of the
patches are reviewed/ready.

Regards,
  Dmitry

> > I would also make all pointers constant (i.e. const notmuch_message_t
> > *const message), but I can not insist since it is not common in notmuch.
> > 
> > Regards,
> >   Dmitry

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Dmitry Kurochkin]

Hi Austin.

+    /* The number of children of this part. */
+    int children;

Consider renaming to children_count or similar to make it clear that it
is a counter and not the actual children.

+    notmuch_bool_t decrypt_success;

Perhaps s/decrypt_success/is_decrypted/ for consistency with
is_encrypted?

+mime_node_child (const mime_node_t *parent, int child);
+
 #include "command-line-arguments.h"
 #endif

#include should go above declarations.

+    mime_node_t *out = talloc_zero (parent, mime_node_t);

Perhaps s/out/node/?

+           /* this violates RFC 3156 section 4, so we won't bother with it. */
+           fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
+                    "message (should be exactly 2)\n",
+                    out->children);

Perhaps s/should be exactly/must be exactly/?  That is what the
RFC says.  Same for signature.

+           out->is_encrypted = TRUE;
+           out->is_signed = TRUE;

These are set only if we do decryption/verification.  But their
names and comments imply that they should reflect properties of
the part and be set independently from whether we are actually
doing decryption or verification.

+           /* For some reason the GMimeSignatureValidity returned
+            * here is not a const (inconsistent with that
+            * returned by
+            * g_mime_multipart_encrypted_get_signature_validity,
+            * and therefore needs to be properly disposed of.
+            * Hopefully the API will become more consistent. */

Ouch.  In gmime 2.6 this API has changed, but looks like the
issue is still there.  Is there a bug for it?  If yes, we should
add a reference to the comment.  Otherwise, we should file the
bug and then add a reference to the comment :)

Both decryption and verification use cryptoctx.  But decryption
is done iff decrypt is true (without checking if cryptoctx is
set) and verification is done iff cryptoctx is set (without any
special flag).  Why is it asymmetric?  Do we need to check if
cryptoctx is set for decryption?

In mime_node_child():

+       GMimeMultipart *multipart = GMIME_MULTIPART (parent->part);
+       if (child == 1 && parent->decrypted_child)
+           return _mime_node_create (parent, parent->decrypted_child);

Multipart is not needed here, please move it's declaration below
the condition.

+       GMimeObject *child = g_mime_message_get_mime_part (message);

The child variable eclipses the (int child) parameter.  Consider
using a different name for the variable (or parameter).

+           return _mime_node_create (parent, parent->decrypted_child);
+       return _mime_node_create (parent, g_mime_multipart_get_part (multipart, child));
...
+       return _mime_node_create (parent, child);

All returns are similar.  Consider declaring a local variable for
storing the part and using a single return, i.e.:

  GMimeObject *part;
  if (...)
    part = ...;
  else ...
    part = ...;

  return _mime_node_create (parent, part);

Regards,
  Dmitry

Re: [PATCH 3/4] Utility function to seek in MIME trees in depth-first order.

[Dmitry Kurochkin]

On Fri,  9 Dec 2011 14:54:27 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This function matches how we number parts for the --part argument to
> show.  It will allow us to jump directly to the desired part, rather
> than traversing the entire tree and carefully tracking whether or not
> we're "in the zone".
> ---
>  mime-node.c      |   25 +++++++++++++++++++++++++
>  notmuch-client.h |    5 +++++
>  2 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/mime-node.c b/mime-node.c
> index a8e4a59..207818e 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -232,3 +232,28 @@ mime_node_child (const mime_node_t *parent, int child)
>  			g_type_name (G_OBJECT_TYPE (parent->part)));
>      }
>  }
> +
> +static mime_node_t *
> +_mime_node_seek_dfs_walk (mime_node_t *node, int *n)
> +{
> +    mime_node_t *ret = NULL;
> +    int i;
> +

Can we move declarations below the if (which does not need them)?  I
always have troubles remembering if (recent enough) C standard allows
that or it is a GCC extension.  FWIW in the previous patch there are
declarations in the middle of a block, e.g.:

	} else {
	    out->is_signed = TRUE;
            ...
	    GMimeSignatureValidity *sig_validity = g_mime_multipart_signed_verify
		(GMIME_MULTIPART_SIGNED (part), out->ctx->cryptoctx, &err);

So either we can move these declarations to where they are needed, or we
should fix it in _mime_node_create().

> +    if (*n <= 0)

Comment for mime_node_seek_dfs() says that the function returns the node
itself for n = 0, but does not say anything about n < 0.  I would expect
the function to return NULL for n < 0.  In any case, the comment below
should probably mention what happens for n < 0;

> +	return node;
> +
> +    *n = *n - 1;

Perhaps *n -= 1?  Or even --(*n)?

> +    for (i = 0; i < node->children && !ret; i++) {

Consider s/i++/++i/.

Regards,
  Dmitry

> +	mime_node_t *child = mime_node_child (node, i);
> +	ret = _mime_node_seek_dfs_walk (child, n);
> +	if (!ret)
> +	    talloc_free (child);
> +    }
> +    return ret;
> +}
> +
> +mime_node_t *
> +mime_node_seek_dfs (mime_node_t *node, int n)
> +{
> +    return _mime_node_seek_dfs_walk (node, &n);
> +}
> diff --git a/notmuch-client.h b/notmuch-client.h
> index fce1187..f215d4b 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -318,5 +318,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
>  mime_node_t *
>  mime_node_child (const mime_node_t *parent, int child);
>  
> +/* Return the nth child of node in a depth-first traversal.  If n is
> + * 0, returns node itself.  Returns NULL if there is no such part. */
> +mime_node_t *
> +mime_node_seek_dfs (mime_node_t *node, int n);
> +
>  #include "command-line-arguments.h"
>  #endif
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 0/4] First step of 'show' rewrite

[Jameson Graef Rollins]

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

Hey, Austin.  This is a really great rework of mime part handling.  Your
new solution is much more elegant, and makes things much cleaner.

The patch series looks very good to me.  I only have a couple of
comments on the crypto processing stuff, to follow.

I definitely support pushing this series (or maybe a v3 of it) into
master.

jamie.

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

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Jameson Graef Rollins]

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

On Sat, 10 Dec 2011 03:25:48 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> +    notmuch_bool_t decrypt_success;
> 
> Perhaps s/decrypt_success/is_decrypted/ for consistency with
> is_encrypted?

This difference doesn't seem so bad to me, since the is_ variables point
to states of the original message, where as decrypt_success reflects the
processing of the message only.

> +           out->is_encrypted = TRUE;
> +           out->is_signed = TRUE;
> 
> These are set only if we do decryption/verification.  But their
> names and comments imply that they should reflect properties of
> the part and be set independently from whether we are actually
> doing decryption or verification.

I don't think this directly affects anything at the moment, but this is
a good point.  I can imagine that it might be good to maintain that
distinction down the line so it's probably worth being consistent here.

> +           /* For some reason the GMimeSignatureValidity returned
> +            * here is not a const (inconsistent with that
> +            * returned by
> +            * g_mime_multipart_encrypted_get_signature_validity,
> +            * and therefore needs to be properly disposed of.
> +            * Hopefully the API will become more consistent. */
> 
> Ouch.  In gmime 2.6 this API has changed, but looks like the
> issue is still there.  Is there a bug for it?  If yes, we should
> add a reference to the comment.  Otherwise, we should file the
> bug and then add a reference to the comment :)

Don't blame any of this stuff on Austin.  This is left over from the
original crypto processing patches.  I know dkg was in touch with the
gmime maintainers on this issue, but I'm not sure if a bug was filed.

> Both decryption and verification use cryptoctx.  But decryption
> is done iff decrypt is true (without checking if cryptoctx is
> set) and verification is done iff cryptoctx is set (without any
> special flag).  Why is it asymmetric?  Do we need to check if
> cryptoctx is set for decryption?

We probably should.  We can probably fix this in followup patches, since
 Austin is just working off of what dkg and I put in there originally.

jamie.

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

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Jameson Graef Rollins]

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

On Fri,  9 Dec 2011 14:54:26 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> +    /* Handle PGP/MIME parts */
> +    if (GMIME_IS_MULTIPART_ENCRYPTED (part) && out->ctx->decrypt) {
> +	if (out->children != 2) {
> +	    /* this violates RFC 3156 section 4, so we won't bother with it. */
> +	    fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
> +		     "message (should be exactly 2)\n",
> +		     out->children);
> +	} else {
> +	    out->is_encrypted = TRUE;

As per Dmitry's previous point, maybe it's better to do something like:

if (GMIME_IS_MULTIPART_ENCRYPTED (part)) {
    out->is_encrypted = TRUE;
    if (out->ctx->decrypt) {
        if (out->children != 2) {
...

And similarly for is_signed.

> +	    GMimeMultipartEncrypted *encrypteddata =
> +		GMIME_MULTIPART_ENCRYPTED (part);
> +	    out->decrypted_child = g_mime_multipart_encrypted_decrypt
> +		(encrypteddata, out->ctx->cryptoctx, &err);
> +	    if (out->decrypted_child) {
> +		out->decrypt_success = TRUE;
> +		out->is_signed = TRUE;
> +		out->sig_validity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);

Encrypted messages are not necessarily signed, so we need to be careful
about setting is_signed = TRUE based just on decryption status.  The
problem is that gmime's handling of this stuff (at least last I looked
in 2.4) is not so good.
g_mime_multipart_encrypted_get_signature_validity () should return
GMIME_SIGNATURE_STATUS_UNKNOWN if there's no signature, so I think
is_signed should be set TRUE only if sig_validity is not UNKNOWN.

I've really been meaning to overhaul this stuff for gmime 2.6.
Hopefully I'll start looking at that after these patches go through.

jamie.

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

Re: [PATCH 4/4] show: Rewrite show_message_body to use the MIME tree interface.

[Dmitry Kurochkin]

Hi Austin.

I enjoyed reviewing this patch.  It is a pleasure to see how complex and
confusing code becomes much smaller and cleaner.

I still have some questions with the new code.  It seems confusing to me
that part_content is called first and then go envelope headers.  But I
this is just the first step of the rewrite, right? :)

The only comment I have:

+    format->part_content (part);

For all other format members that are function pointers, we have a check
for NULL.  Perhaps we should add it here as well?

Regards,
  Dmitry

Re: [PATCH v3 0/4] First step of 'show' rewrite

[Dmitry Kurochkin]

Austin, good job! :) Thanks for this work.  (and continue, please :))

Most comments in my review are minor and/or concern the old code
(i.e. the new code does not make it worse).  Please feel free to ignore
them.

I vote for pushing this series as soon as Austin finds it appropriate.
If Austin makes a new version of this patch series with some of my
comments fixed, it gets my auto-approve without another round of review :)

Regards,
  Dmitry

Re: [PATCH v3 0/4] First step of 'show' rewrite

[David Bremner]

On Sun, 11 Dec 2011 14:41:50 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:

> I vote for pushing this series as soon as Austin finds it appropriate.
> If Austin makes a new version of this patch series with some of my
> comments fixed, it gets my auto-approve without another round of review :)

As discussed on IRC, we will basically follow this plan. Austin will
need to rebase anyway because of the hook patches.

d

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

Thanks for the thorough review!

Quoth Dmitry Kurochkin on Dec 10 at  3:25 am:
> Hi Austin.
> 
> +    /* The number of children of this part. */
> +    int children;
> 
> Consider renaming to children_count or similar to make it clear that it
> is a counter and not the actual children.

Good point.  Renamed to nchildren, which is shorter but still conveys
that it's a count.

> +    notmuch_bool_t decrypt_success;
> 
> Perhaps s/decrypt_success/is_decrypted/ for consistency with
> is_encrypted?

I actually got rid of is_encrypted/is_signed in the new version (see
below).

> +mime_node_child (const mime_node_t *parent, int child);
> +
>  #include "command-line-arguments.h"
>  #endif
> 
> #include should go above declarations.

That one's bremner's fault.  I'll follow up with a patch to move this.

> +    mime_node_t *out = talloc_zero (parent, mime_node_t);
> 
> Perhaps s/out/node/?

Done.

> +           /* this violates RFC 3156 section 4, so we won't bother with it. */
> +           fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
> +                    "message (should be exactly 2)\n",
> +                    out->children);
> 
> Perhaps s/should be exactly/must be exactly/?  That is what the
> RFC says.  Same for signature.

Done.

> +           out->is_encrypted = TRUE;
> +           out->is_signed = TRUE;
> 
> These are set only if we do decryption/verification.  But their
> names and comments imply that they should reflect properties of
> the part and be set independently from whether we are actually
> doing decryption or verification.

Good point.  I replaced these fields with new fields,
decrypt_attempted and sig_attempted, which are used the way the old
fields were, but actually reflect the desired semantics and the
information that the callers need.  I first tried keeping the is_*
fields and making them reflect the purely structural properties of the
message, but then I realized that that was both redundant with the
type of the MIME part and wasn't what callers actually needed to know.
As an added benefit, sig_attempted sidesteps the question of whether a
multipart/{signed,encrypted} without any signatures is "signed" and
makes it possible for callers to distinguish between unsigned parts,
signature verification failures, and encrypted parts with no signers.

> +           /* For some reason the GMimeSignatureValidity returned
> +            * here is not a const (inconsistent with that
> +            * returned by
> +            * g_mime_multipart_encrypted_get_signature_validity,
> +            * and therefore needs to be properly disposed of.
> +            * Hopefully the API will become more consistent. */
> 
> Ouch.  In gmime 2.6 this API has changed, but looks like the
> issue is still there.  Is there a bug for it?  If yes, we should
> add a reference to the comment.  Otherwise, we should file the
> bug and then add a reference to the comment :)

It looks like they're both non-const in 2.6 (which makes more sense to
me).  I updated the comment to mention this.

> Both decryption and verification use cryptoctx.  But decryption
> is done iff decrypt is true (without checking if cryptoctx is
> set) and verification is done iff cryptoctx is set (without any
> special flag).  Why is it asymmetric?  Do we need to check if
> cryptoctx is set for decryption?

Oops, it wasn't supposed to be asymmetric.  Decryption now requires
cryptoctx to be set (it probably would have crashed before without
it).

> In mime_node_child():
> 
> +       GMimeMultipart *multipart = GMIME_MULTIPART (parent->part);
> +       if (child == 1 && parent->decrypted_child)
> +           return _mime_node_create (parent, parent->decrypted_child);
> 
> Multipart is not needed here, please move it's declaration below
> the condition.
>
> +       GMimeObject *child = g_mime_message_get_mime_part (message);
> 
> The child variable eclipses the (int child) parameter.  Consider
> using a different name for the variable (or parameter).

The above two were superseded by the next change.

> +           return _mime_node_create (parent, parent->decrypted_child);
> +       return _mime_node_create (parent, g_mime_multipart_get_part (multipart, child));
> ...
> +       return _mime_node_create (parent, child);
> 
> All returns are similar.  Consider declaring a local variable for
> storing the part and using a single return, i.e.:
> 
>   GMimeObject *part;
>   if (...)
>     part = ...;
>   else ...
>     part = ...;
> 
>   return _mime_node_create (parent, part);

Good idea.  This made things compact enough to simplify the rest of
this function.

> Regards,
>   Dmitry
> 

-- 
Austin Clements                                      MIT/'06/PhD/CSAIL
amdragon@mit.edu                           http://web.mit.edu/amdragon
       Somewhere in the dream we call reality you will find me,
              searching for the reality we call dreams.

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

Quoth Jameson Graef Rollins on Dec 10 at  1:17 pm:
> On Sat, 10 Dec 2011 03:25:48 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > +           out->is_encrypted = TRUE;
> > +           out->is_signed = TRUE;
> > 
> > These are set only if we do decryption/verification.  But their
> > names and comments imply that they should reflect properties of
> > the part and be set independently from whether we are actually
> > doing decryption or verification.
> 
> I don't think this directly affects anything at the moment, but this is
> a good point.  I can imagine that it might be good to maintain that
> distinction down the line so it's probably worth being consistent here.

See my response to Dmitry's original email.  The structural properties
can be derived directly from the type of the part field, so I got rid
of these fields.

> > Both decryption and verification use cryptoctx.  But decryption
> > is done iff decrypt is true (without checking if cryptoctx is
> > set) and verification is done iff cryptoctx is set (without any
> > special flag).  Why is it asymmetric?  Do we need to check if
> > cryptoctx is set for decryption?
> 
> We probably should.  We can probably fix this in followup patches, since
>  Austin is just working off of what dkg and I put in there originally.

Actually, this one was my fault from when I tweaked the control flow
to use the MIME node context.

[PATCH v4 0/4] First step of 'show' rewrite

[Austin Clements]

This addresses Dmitry's and Jameson's review comments from
  id:"87k46572f7.fsf@gmail.com"
  id:"87liqk5dly.fsf@servo.finestructure.net"
  id:"87hb187iu6.fsf@gmail.com"
and rebases to current master.

I'd like a quick eye over this to make sure I didn't screw anything up
in the latest version, but otherwise I think it's ready to go in.

[PATCH v4 1/4] show: Pass notmuch_message_t instead of path to show_message_body.

[Austin Clements]

In addition to simplifying the code, we'll need the notmuch_message_t*
in show_message_body shortly.
---
 notmuch-client.h |    2 +-
 notmuch-reply.c  |    3 +--
 notmuch-show.c   |    3 +--
 show-message.c   |    3 ++-
 4 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index c602e2e..c521efa 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -162,7 +162,7 @@ char *
 query_string_from_args (void *ctx, int argc, char *argv[]);
 
 notmuch_status_t
-show_message_body (const char *filename,
+show_message_body (notmuch_message_t *message,
 		   const notmuch_show_format_t *format,
 		   notmuch_show_params_t *params);
 
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 7ac879f..f8d5f64 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -546,8 +546,7 @@ notmuch_reply_format_default(void *ctx,
 		notmuch_message_get_header (message, "date"),
 		notmuch_message_get_header (message, "from"));
 
-	show_message_body (notmuch_message_get_filename (message),
-			   format, params);
+	show_message_body (message, format, params);
 
 	notmuch_message_destroy (message);
     }
diff --git a/notmuch-show.c b/notmuch-show.c
index 19fb49f..0200b9c 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -758,8 +758,7 @@ show_message (void *ctx,
     }
 
     if (format->part_content)
-	show_message_body (notmuch_message_get_filename (message),
-			   format, params);
+	show_message_body (message, format, params);
 
     if (params->part <= 0) {
 	fputs (format->body_end, stdout);
diff --git a/show-message.c b/show-message.c
index d83f04e..09fa607 100644
--- a/show-message.c
+++ b/show-message.c
@@ -175,7 +175,7 @@ show_message_part (GMimeObject *part,
 }
 
 notmuch_status_t
-show_message_body (const char *filename,
+show_message_body (notmuch_message_t *message,
 		   const notmuch_show_format_t *format,
 		   notmuch_show_params_t *params)
 {
@@ -183,6 +183,7 @@ show_message_body (const char *filename,
     GMimeParser *parser = NULL;
     GMimeMessage *mime_message = NULL;
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
+    const char *filename = notmuch_message_get_filename (message);
     FILE *file = NULL;
     show_message_state_t state;
 
-- 
1.7.7.3

[PATCH v4 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

This wraps all of the complex MIME part handling in a single, simple
function that gets part N from *any* MIME object, so traversing a MIME
part tree becomes a two-line for loop.  Furthermore, the MIME node
structure provides easy access to envelopes for message parts as well
as cryptographic information.

This code is directly derived from the current show_message_body code
(much of it is identical), but the control relation is inverted:
instead of show_message_body controlling the traversal of the MIME
structure and invoking callbacks, the caller controls the traversal of
the MIME structure.
---
 Makefile.local   |    1 +
 mime-node.c      |  238 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 notmuch-client.h |   83 +++++++++++++++++++
 3 files changed, 322 insertions(+), 0 deletions(-)
 create mode 100644 mime-node.c

diff --git a/Makefile.local b/Makefile.local
index 97f397f..516f26e 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -315,6 +315,7 @@ notmuch_client_srcs =		\
 	notmuch-time.c		\
 	query-string.c		\
 	show-message.c		\
+	mime-node.c		\
 	json.c
 
 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
diff --git a/mime-node.c b/mime-node.c
new file mode 100644
index 0000000..fd8e754
--- /dev/null
+++ b/mime-node.c
@@ -0,0 +1,238 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2009 Carl Worth
+ * Copyright © 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth <cworth@cworth.org>
+ *          Keith Packard <keithp@keithp.com>
+ *          Austin Clements <aclements@csail.mit.edu>
+ */
+
+#include "notmuch-client.h"
+
+/* Context that gets inherited from the root node. */
+typedef struct mime_node_context {
+    /* Per-message resources.  These are allocated internally and must
+     * be destroyed. */
+    FILE *file;
+    GMimeStream *stream;
+    GMimeParser *parser;
+    GMimeMessage *mime_message;
+
+    /* Context provided by the caller. */
+    GMimeCipherContext *cryptoctx;
+    notmuch_bool_t decrypt;
+} mime_node_context_t;
+
+static int
+_mime_node_context_free (mime_node_context_t *res)
+{
+    if (res->mime_message)
+	g_object_unref (res->mime_message);
+
+    if (res->parser)
+	g_object_unref (res->parser);
+
+    if (res->stream)
+	g_object_unref (res->stream);
+
+    if (res->file)
+	fclose (res->file);
+
+    return 0;
+}
+
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+		GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
+		mime_node_t **root_out)
+{
+    const char *filename = notmuch_message_get_filename (message);
+    mime_node_context_t *mctx;
+    mime_node_t *root;
+    notmuch_status_t status;
+
+    root = talloc_zero (ctx, mime_node_t);
+    if (root == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    /* Create the tree-wide context */
+    mctx = talloc_zero (root, mime_node_context_t);
+    if (mctx == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+    talloc_set_destructor (mctx, _mime_node_context_free);
+
+    mctx->file = fopen (filename, "r");
+    if (! mctx->file) {
+	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
+	status = NOTMUCH_STATUS_FILE_ERROR;
+	goto DONE;
+    }
+
+    mctx->stream = g_mime_stream_file_new (mctx->file);
+    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx->stream), FALSE);
+
+    mctx->parser = g_mime_parser_new_with_stream (mctx->stream);
+
+    mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
+
+    mctx->cryptoctx = cryptoctx;
+    mctx->decrypt = decrypt;
+
+    /* Create the root node */
+    root->part = GMIME_OBJECT (mctx->mime_message);
+    root->envelope_file = message;
+    root->nchildren = 1;
+    root->ctx = mctx;
+
+    *root_out = root;
+    return NOTMUCH_STATUS_SUCCESS;
+
+DONE:
+    talloc_free (root);
+    return status;
+}
+
+static int
+_signature_validity_free (GMimeSignatureValidity **proxy)
+{
+    g_mime_signature_validity_free (*proxy);
+    return 0;
+}
+
+static mime_node_t *
+_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+{
+    mime_node_t *node = talloc_zero (parent, mime_node_t);
+    GError *err = NULL;
+
+    /* Set basic node properties */
+    node->part = part;
+    node->ctx = parent->ctx;
+    if (!talloc_reference (node, node->ctx)) {
+	fprintf (stderr, "Out of memory.\n");
+	talloc_free (node);
+	return NULL;
+    }
+
+    /* Deal with the different types of parts */
+    if (GMIME_IS_PART (part)) {
+	node->nchildren = 0;
+    } else if (GMIME_IS_MULTIPART (part)) {
+	node->nchildren = g_mime_multipart_get_count (GMIME_MULTIPART (part));
+    } else if (GMIME_IS_MESSAGE_PART (part)) {
+	/* Promote part to an envelope and open it */
+	GMimeMessagePart *message_part = GMIME_MESSAGE_PART (part);
+	GMimeMessage *message = g_mime_message_part_get_message (message_part);
+	node->envelope_part = message_part;
+	node->part = GMIME_OBJECT (message);
+	node->nchildren = 1;
+    } else {
+	fprintf (stderr, "Warning: Unknown mime part type: %s.\n",
+		 g_type_name (G_OBJECT_TYPE (part)));
+	talloc_free (node);
+	return NULL;
+    }
+
+    /* Handle PGP/MIME parts */
+    if (GMIME_IS_MULTIPART_ENCRYPTED (part)
+	&& node->ctx->cryptoctx && node->ctx->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 "
+		     "message (must be exactly 2)\n",
+		     node->nchildren);
+	} else {
+	    GMimeMultipartEncrypted *encrypteddata =
+		GMIME_MULTIPART_ENCRYPTED (part);
+	    node->decrypt_attempted = TRUE;
+	    node->decrypted_child = g_mime_multipart_encrypted_decrypt
+		(encrypteddata, node->ctx->cryptoctx, &err);
+	    if (node->decrypted_child) {
+		node->decrypt_success = node->sig_attempted = TRUE;
+		node->sig_validity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
+	    } else {
+		fprintf (stderr, "Failed to decrypt part: %s\n",
+			 (err ? err->message : "no error explanation given"));
+	    }
+	}
+    } else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->cryptoctx) {
+	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 "
+		     "(must be exactly 2)\n",
+		     node->nchildren);
+	} else {
+	    /* For some reason the GMimeSignatureValidity returned
+	     * here is not a const (inconsistent with that
+	     * returned by
+	     * g_mime_multipart_encrypted_get_signature_validity,
+	     * and therefore needs to be properly disposed of.
+	     *
+	     * 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);
+	    node->sig_attempted = TRUE;
+	    node->sig_validity = sig_validity;
+	    if (sig_validity) {
+		GMimeSignatureValidity **proxy =
+		    talloc (node, GMimeSignatureValidity *);
+		*proxy = sig_validity;
+		talloc_set_destructor (proxy, _signature_validity_free);
+	    }
+	}
+    }
+
+    if (node->sig_attempted && !node->sig_validity)
+	fprintf (stderr, "Failed to verify signed part: %s\n",
+		 (err ? err->message : "no error explanation given"));
+
+    if (err)
+	g_error_free (err);
+
+    return node;
+}
+
+mime_node_t *
+mime_node_child (const mime_node_t *parent, int child)
+{
+    GMimeObject *sub;
+
+    if (!parent || child < 0 || child >= parent->nchildren)
+	return NULL;
+
+    if (GMIME_IS_MULTIPART (parent->part)) {
+	if (child == 1 && parent->decrypted_child)
+	    sub = parent->decrypted_child;
+	else
+	    sub = g_mime_multipart_get_part
+		(GMIME_MULTIPART (parent->part), child);
+    } else if (GMIME_IS_MESSAGE (parent->part)) {
+	sub = g_mime_message_get_mime_part (GMIME_MESSAGE (parent->part));
+    } else {
+	/* This should have been caught by message_part_create */
+	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
+			g_type_name (G_OBJECT_TYPE (parent->part)));
+    }
+    return _mime_node_create (parent, sub);
+}
diff --git a/notmuch-client.h b/notmuch-client.h
index c521efa..e23b51c 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -241,5 +241,88 @@ notmuch_run_hook (const char *db_path, const char *hook);
 notmuch_bool_t
 debugger_is_active (void);
 
+/* mime-node.c */
+
+/* mime_node_t represents a single node in a MIME tree.  A MIME tree
+ * abstracts the different ways of traversing different types of MIME
+ * parts, allowing a MIME message to be viewed as a generic tree of
+ * parts.  Message-type parts have one child, multipart-type parts
+ * have multiple children, and leaf parts have zero children.
+ */
+typedef struct mime_node {
+    /* The MIME object of this part.  This will be a GMimeMessage,
+     * GMimePart, GMimeMultipart, or a subclass of one of these.
+     *
+     * This will never be a GMimeMessagePart because GMimeMessagePart
+     * is structurally redundant with GMimeMessage.  If this part is a
+     * message (that is, 'part' is a GMimeMessage), then either
+     * envelope_file will be set to a notmuch_message_t (for top-level
+     * messages) or envelope_part will be set to a GMimeMessagePart
+     * (for embedded message parts).
+     */
+    GMimeObject *part;
+
+    /* If part is a GMimeMessage, these record the envelope of the
+     * message: either a notmuch_message_t representing a top-level
+     * message, or a GMimeMessagePart representing a MIME part
+     * containing a message.
+     */
+    notmuch_message_t *envelope_file;
+    GMimeMessagePart *envelope_part;
+
+    /* The number of children of this part. */
+    int nchildren;
+
+    /* True if decryption of this part was attempted. */
+    notmuch_bool_t decrypt_attempted;
+    /* True if decryption of this part's child succeeded.  In this
+     * case, the decrypted part is substituted for the second child of
+     * this part (which would usually be the encrypted data). */
+    notmuch_bool_t decrypt_success;
+
+    /* True if signature verification on this part was attempted. */
+    notmuch_bool_t sig_attempted;
+    /* For signed or encrypted containers, the validity of the
+     * signature.  May be NULL if signature verification failed.  If
+     * there are simply no signatures, this will be non-NULL with an
+     * empty signers list. */
+    const GMimeSignatureValidity *sig_validity;
+
+    /* Internal: Context inherited from the root iterator. */
+    struct mime_node_context *ctx;
+
+    /* Internal: For successfully decrypted multipart parts, the
+     * decrypted part to substitute for the second child. */
+    GMimeObject *decrypted_child;
+} mime_node_t;
+
+/* 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.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Root node is returned in *node_out.
+ *
+ * NOTMUCH_STATUS_FILE_ERROR: Failed to open message file.
+ *
+ * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
+ */
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+		GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
+		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
+ * node.
+ *
+ * In case of any failure, this function returns NULL, (after printing
+ * an error message on stderr).
+ */
+mime_node_t *
+mime_node_child (const mime_node_t *parent, int child);
+
 #include "command-line-arguments.h"
 #endif
-- 
1.7.7.3

[PATCH v4 3/4] Utility function to seek in MIME trees in depth-first order.

[Austin Clements]

This function matches how we number parts for the --part argument to
show.  It will allow us to jump directly to the desired part, rather
than traversing the entire tree and carefully tracking whether or not
we're "in the zone".
---
 mime-node.c      |   27 +++++++++++++++++++++++++++
 notmuch-client.h |    5 +++++
 2 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index fd8e754..1d32823 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -236,3 +236,30 @@ mime_node_child (const mime_node_t *parent, int child)
     }
     return _mime_node_create (parent, sub);
 }
+
+static mime_node_t *
+_mime_node_seek_dfs_walk (mime_node_t *node, int *n)
+{
+    mime_node_t *ret = NULL;
+    int i;
+
+    if (*n == 0)
+	return node;
+
+    *n -= 1;
+    for (i = 0; i < node->nchildren && !ret; i++) {
+	mime_node_t *child = mime_node_child (node, i);
+	ret = _mime_node_seek_dfs_walk (child, n);
+	if (!ret)
+	    talloc_free (child);
+    }
+    return ret;
+}
+
+mime_node_t *
+mime_node_seek_dfs (mime_node_t *node, int n)
+{
+    if (n < 0)
+	return NULL;
+    return _mime_node_seek_dfs_walk (node, &n);
+}
diff --git a/notmuch-client.h b/notmuch-client.h
index e23b51c..2f36519 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -324,5 +324,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
 mime_node_t *
 mime_node_child (const mime_node_t *parent, int child);
 
+/* Return the nth child of node in a depth-first traversal.  If n is
+ * 0, returns node itself.  Returns NULL if there is no such part. */
+mime_node_t *
+mime_node_seek_dfs (mime_node_t *node, int n);
+
 #include "command-line-arguments.h"
 #endif
-- 
1.7.7.3

[PATCH v4 4/4] show: Rewrite show_message_body to use the MIME tree interface.

[Austin Clements]

This removes all of the MIME traversal logic from show_message_body
and leaves only its interaction with the format callbacks.

Besides isolating concerns, since traversal happens behind a trivial
interface, there is now much less code duplication in
show_message_part.  Also, this uses mime_node_seek_dfs to start at the
requested part, eliminating all of the logic about parts being
selected or being in_zone (and reducing the "show state" to only a
part counter).  notmuch_show_params_t no longer needs to be passed
through the recursion because the only two fields that mattered
(related to crypto) are now handled by the MIME tree.

The few remaining complexities in show_message_part highlight
irregularities in the format callbacks with respect to top-level
messages and embedded message parts.

Since this is a rewrite, the diff is not very enlightening.  It's
easier to look at the old code and the new code side-by-side.
---
 show-message.c |  229 +++++++++++++-------------------------------------------
 1 files changed, 52 insertions(+), 177 deletions(-)

diff --git a/show-message.c b/show-message.c
index 09fa607..d02e744 100644
--- a/show-message.c
+++ b/show-message.c
@@ -24,154 +24,54 @@
 
 typedef struct show_message_state {
     int part_count;
-    int in_zone;
 } show_message_state_t;
 
 static void
-show_message_part (GMimeObject *part,
+show_message_part (mime_node_t *node,
 		   show_message_state_t *state,
 		   const notmuch_show_format_t *format,
-		   notmuch_show_params_t *params,
 		   int first)
 {
-    GMimeObject *decryptedpart = NULL;
-    int selected;
+    /* Formatters expect the envelope for embedded message parts */
+    GMimeObject *part = node->envelope_part ?
+	GMIME_OBJECT (node->envelope_part) : node->part;
+    int i;
+
+    if (!first)
+	fputs (format->part_sep, stdout);
+
+    /* Format this part */
+    if (format->part_start)
+	format->part_start (part, &(state->part_count));
+
+    if (node->decrypt_attempted && format->part_encstatus)
+	format->part_encstatus (node->decrypt_success);
+
+    if (node->sig_attempted && format->part_sigstatus)
+	format->part_sigstatus (node->sig_validity);
+
+    format->part_content (part);
+
+    if (node->envelope_part) {
+	fputs (format->header_start, stdout);
+	if (format->header_message_part)
+	    format->header_message_part (GMIME_MESSAGE (node->part));
+	fputs (format->header_end, stdout);
+
+	fputs (format->body_start, stdout);
+    }
+
+    /* Recurse over the children */
     state->part_count += 1;
+    for (i = 0; i < node->nchildren; i++)
+	show_message_part (mime_node_child (node, i), state, format, i == 0);
 
-    if (! (GMIME_IS_PART (part) || GMIME_IS_MULTIPART (part) || GMIME_IS_MESSAGE_PART (part))) {
-	fprintf (stderr, "Warning: Not displaying unknown mime part: %s.\n",
-		 g_type_name (G_OBJECT_TYPE (part)));
-	return;
-    }
+    /* Finish this part */
+    if (node->envelope_part)
+	fputs (format->body_end, stdout);
 
-    selected = (params->part <= 0 || state->part_count == params->part);
-
-    if (selected || state->in_zone) {
-	if (!first && (params->part <= 0 || state->in_zone))
-	    fputs (format->part_sep, stdout);
-
-	if (format->part_start)
-	    format->part_start (part, &(state->part_count));
-    }
-
-    /* handle PGP/MIME parts */
-    if (GMIME_IS_MULTIPART (part) && params->cryptoctx) {
-	GMimeMultipart *multipart = GMIME_MULTIPART (part);
-	GError* err = NULL;
-
-	if (GMIME_IS_MULTIPART_ENCRYPTED (part) && params->decrypt)
-	{
-	    if ( g_mime_multipart_get_count (multipart) != 2 ) {
-		/* this violates RFC 3156 section 4, so we won't bother with it. */
-		fprintf (stderr,
-			 "Error: %d part(s) for a multipart/encrypted message (should be exactly 2)\n",
-			 g_mime_multipart_get_count (multipart));
-	    } else {
-		GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
-		decryptedpart = g_mime_multipart_encrypted_decrypt (encrypteddata, params->cryptoctx, &err);
-		if (decryptedpart) {
-		    if ((selected || state->in_zone) && format->part_encstatus)
-			format->part_encstatus (1);
-		    const GMimeSignatureValidity *sigvalidity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
-		    if (!sigvalidity)
-			fprintf (stderr, "Failed to verify signed part: %s\n", (err ? err->message : "no error explanation given"));
-		    if ((selected || state->in_zone) && format->part_sigstatus)
-			format->part_sigstatus (sigvalidity);
-		} else {
-		    fprintf (stderr, "Failed to decrypt part: %s\n", (err ? err->message : "no error explanation given"));
-		    if ((selected || state->in_zone) && format->part_encstatus)
-			format->part_encstatus (0);
-		}
-	    }
-	}
-	else if (GMIME_IS_MULTIPART_SIGNED (part))
-	{
-	    if ( g_mime_multipart_get_count (multipart) != 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 (should be exactly 2)\n",
-			 g_mime_multipart_get_count (multipart));
-	    } else {
-		/* For some reason the GMimeSignatureValidity returned
-		 * here is not a const (inconsistent with that
-		 * returned by
-		 * g_mime_multipart_encrypted_get_signature_validity,
-		 * and therefore needs to be properly disposed of.
-		 * Hopefully the API will become more consistent. */
-		GMimeSignatureValidity *sigvalidity = g_mime_multipart_signed_verify (GMIME_MULTIPART_SIGNED (part), params->cryptoctx, &err);
-		if (!sigvalidity) {
-		    fprintf (stderr, "Failed to verify signed part: %s\n", (err ? err->message : "no error explanation given"));
-		}
-		if ((selected || state->in_zone) && format->part_sigstatus)
-		    format->part_sigstatus (sigvalidity);
-		if (sigvalidity)
-		    g_mime_signature_validity_free (sigvalidity);
-	    }
-	}
-
-	if (err)
-	    g_error_free (err);
-    }
-    /* end handle PGP/MIME parts */
-
-    if (selected || state->in_zone)
-	format->part_content (part);
-
-    if (GMIME_IS_MULTIPART (part)) {
-	GMimeMultipart *multipart = GMIME_MULTIPART (part);
-	int i;
-
-	if (selected)
-	    state->in_zone = 1;
-
-	if (decryptedpart) {
-	    /* We emit the useless application/pgp-encrypted version
-	     * part here only to keep the emitted output as consistent
-	     * as possible between decrypted output and the
-	     * unprocessed multipart/mime. For some strange reason,
-	     * the actual encrypted data is the second part of the
-	     * multipart. */
-	    show_message_part (g_mime_multipart_get_part (multipart, 0), state, format, params, TRUE);
-	    show_message_part (decryptedpart, state, format, params, FALSE);
-	} else {
-	    for (i = 0; i < g_mime_multipart_get_count (multipart); i++) {
-		show_message_part (g_mime_multipart_get_part (multipart, i),
-				   state, format, params, i == 0);
-	    }
-	}
-
-	if (selected)
-	    state->in_zone = 0;
-
-    } else if (GMIME_IS_MESSAGE_PART (part)) {
-	GMimeMessage *mime_message = g_mime_message_part_get_message (GMIME_MESSAGE_PART (part));
-
-	if (selected)
-	    state->in_zone = 1;
-
-	if (selected || (!selected && state->in_zone)) {
-	    fputs (format->header_start, stdout);
-	    if (format->header_message_part)
-		format->header_message_part (mime_message);
-	    fputs (format->header_end, stdout);
-
-	    fputs (format->body_start, stdout);
-	}
-
-	show_message_part (g_mime_message_get_mime_part (mime_message),
-			   state, format, params, TRUE);
-
-	if (selected || (!selected && state->in_zone))
-	    fputs (format->body_end, stdout);
-
-	if (selected)
-	    state->in_zone = 0;
-    }
-
-    if (selected || state->in_zone) {
-	if (format->part_end)
-	    format->part_end (part);
-    }
+    if (format->part_end)
+	format->part_end (part);
 }
 
 notmuch_status_t
@@ -179,49 +79,24 @@ show_message_body (notmuch_message_t *message,
 		   const notmuch_show_format_t *format,
 		   notmuch_show_params_t *params)
 {
-    GMimeStream *stream = NULL;
-    GMimeParser *parser = NULL;
-    GMimeMessage *mime_message = NULL;
-    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
-    const char *filename = notmuch_message_get_filename (message);
-    FILE *file = NULL;
+    notmuch_status_t ret;
     show_message_state_t state;
+    mime_node_t *root, *part;
 
-    state.part_count = 0;
-    state.in_zone = 0;
+    ret = mime_node_open (NULL, message, params->cryptoctx, params->decrypt,
+			  &root);
+    if (ret)
+	return ret;
 
-    file = fopen (filename, "r");
-    if (! file) {
-	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
-	ret = NOTMUCH_STATUS_FILE_ERROR;
-	goto DONE;
-    }
+    /* The caller of show_message_body has already handled the
+     * outermost envelope, so skip it. */
+    state.part_count = MAX (params->part, 1);
 
-    stream = g_mime_stream_file_new (file);
-    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream), FALSE);
+    part = mime_node_seek_dfs (root, state.part_count);
+    if (part)
+	show_message_part (part, &state, format, TRUE);
 
-    parser = g_mime_parser_new_with_stream (stream);
+    talloc_free (root);
 
-    mime_message = g_mime_parser_construct_message (parser);
-
-    show_message_part (g_mime_message_get_mime_part (mime_message),
-		       &state,
-		       format,
-		       params,
-		       TRUE);
-
-  DONE:
-    if (mime_message)
-	g_object_unref (mime_message);
-
-    if (parser)
-	g_object_unref (parser);
-
-    if (stream)
-	g_object_unref (stream);
-
-    if (file)
-	fclose (file);
-
-    return ret;
+    return NOTMUCH_STATUS_SUCCESS;
 }
-- 
1.7.7.3

Re: [PATCH 3/4] Utility function to seek in MIME trees in depth-first order.

[Austin Clements]

Quoth Dmitry Kurochkin on Dec 10 at  3:43 pm:
> On Fri,  9 Dec 2011 14:54:27 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > This function matches how we number parts for the --part argument to
> > show.  It will allow us to jump directly to the desired part, rather
> > than traversing the entire tree and carefully tracking whether or not
> > we're "in the zone".
> > ---
> >  mime-node.c      |   25 +++++++++++++++++++++++++
> >  notmuch-client.h |    5 +++++
> >  2 files changed, 30 insertions(+), 0 deletions(-)
> > 
> > diff --git a/mime-node.c b/mime-node.c
> > index a8e4a59..207818e 100644
> > --- a/mime-node.c
> > +++ b/mime-node.c
> > @@ -232,3 +232,28 @@ mime_node_child (const mime_node_t *parent, int child)
> >  			g_type_name (G_OBJECT_TYPE (parent->part)));
> >      }
> >  }
> > +
> > +static mime_node_t *
> > +_mime_node_seek_dfs_walk (mime_node_t *node, int *n)
> > +{
> > +    mime_node_t *ret = NULL;
> > +    int i;
> > +
> 
> Can we move declarations below the if (which does not need them)?  I
> always have troubles remembering if (recent enough) C standard allows
> that or it is a GCC extension.  FWIW in the previous patch there are
> declarations in the middle of a block, e.g.:
> 
> 	} else {
> 	    out->is_signed = TRUE;
>             ...
> 	    GMimeSignatureValidity *sig_validity = g_mime_multipart_signed_verify
> 		(GMIME_MULTIPART_SIGNED (part), out->ctx->cryptoctx, &err);
> 
> So either we can move these declarations to where they are needed, or we
> should fix it in _mime_node_create().

Since prevailing notmuch style seems to be top-declarations, I fixed
up _mime_node_create instead (personally I prefer C99-style
declarations, but *shrug*).

> > +    if (*n <= 0)
> 
> Comment for mime_node_seek_dfs() says that the function returns the node
> itself for n = 0, but does not say anything about n < 0.  I would expect
> the function to return NULL for n < 0.  In any case, the comment below
> should probably mention what happens for n < 0;

Good point.  I made it return NULL for n < 0.  I think this logically
falls under "Returns NULL if there is no such part."

> > +	return node;
> > +
> > +    *n = *n - 1;
> 
> Perhaps *n -= 1?  Or even --(*n)?

Changed to *n -= 1.

> > +    for (i = 0; i < node->children && !ret; i++) {
> 
> Consider s/i++/++i/.

notmuch uses i++ remarkably consistently, so I left this.

> Regards,
>   Dmitry

Re: [PATCH 4/4] show: Rewrite show_message_body to use the MIME tree interface.

[Austin Clements]

Quoth Dmitry Kurochkin on Dec 11 at  2:34 pm:
> Hi Austin.
> 
> I enjoyed reviewing this patch.  It is a pleasure to see how complex and
> confusing code becomes much smaller and cleaner.
> 
> I still have some questions with the new code.  It seems confusing to me
> that part_content is called first and then go envelope headers.  But I
> this is just the first step of the rewrite, right? :)

Yeah, this is weird, but I'm just being compatible with the existing
code at this point.  This code is about to go away.

> The only comment I have:
> 
> +    format->part_content (part);
> 
> For all other format members that are function pointers, we have a check
> for NULL.  Perhaps we should add it here as well?

I would if I weren't about to delete this.  ]:--8)

Re: [PATCH v4 0/4] First step of 'show' rewrite

[Dmitry Kurochkin]

On Fri, 23 Dec 2011 22:45:44 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This addresses Dmitry's and Jameson's review comments from
>   id:"87k46572f7.fsf@gmail.com"
>   id:"87liqk5dly.fsf@servo.finestructure.net"
>   id:"87hb187iu6.fsf@gmail.com"
> and rebases to current master.
> 
> I'd like a quick eye over this to make sure I didn't screw anything up
> in the latest version, but otherwise I think it's ready to go in.
> 

Thanks, Austin!  I will try to look at it this weekend.

Regards,
  Dmitry

> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v4 0/4] First step of 'show' rewrite

[Jameson Graef Rollins]

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

On Fri, 23 Dec 2011 22:45:44 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This addresses Dmitry's and Jameson's review comments from
>   id:"87k46572f7.fsf@gmail.com"
>   id:"87liqk5dly.fsf@servo.finestructure.net"
>   id:"87hb187iu6.fsf@gmail.com"
> and rebases to current master.
> 
> I'd like a quick eye over this to make sure I didn't screw anything up
> in the latest version, but otherwise I think it's ready to go in.

Hey, Austin.  Thanks so much for the rework.  I have one very minor
comment, but other than that it all looks really good to me (as usual).
All tests pass as expected when applied to master.  I'm looking forward
to what else you've got in store on top of this!

jamie.

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

Re: [PATCH v4 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Jameson Graef Rollins]

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

On Fri, 23 Dec 2011 22:45:46 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> +    /* True if decryption of this part was attempted. */
> +    notmuch_bool_t decrypt_attempted;
> +    /* True if decryption of this part's child succeeded.  In this
> +     * case, the decrypted part is substituted for the second child of
> +     * this part (which would usually be the encrypted data). */
> +    notmuch_bool_t decrypt_success;
> +
> +    /* True if signature verification on this part was attempted. */
> +    notmuch_bool_t sig_attempted;

I think these new variables make sense, and reflect the correct
semantics, as you already mentioned.

I do, however, think the later variable should be called
"verify_attempted" (or "verification_", or "ver_"?), instead of
"sig_attempted", since verification is the complementary action on a
signed part, just as decryption is for an encrypted one.
"sig_attempted" somehow implies to me that one is trying to make a
signature, not verify an existing one.

jamie.

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

Re: [PATCH v4 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Dmitry Kurochkin]

On Fri, 23 Dec 2011 23:55:34 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Fri, 23 Dec 2011 22:45:46 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > +    /* True if decryption of this part was attempted. */
> > +    notmuch_bool_t decrypt_attempted;
> > +    /* True if decryption of this part's child succeeded.  In this
> > +     * case, the decrypted part is substituted for the second child of
> > +     * this part (which would usually be the encrypted data). */
> > +    notmuch_bool_t decrypt_success;
> > +
> > +    /* True if signature verification on this part was attempted. */
> > +    notmuch_bool_t sig_attempted;
> 
> I think these new variables make sense, and reflect the correct
> semantics, as you already mentioned.
> 
> I do, however, think the later variable should be called
> "verify_attempted" (or "verification_", or "ver_"?), instead of
> "sig_attempted", since verification is the complementary action on a
> signed part, just as decryption is for an encrypted one.
> "sig_attempted" somehow implies to me that one is trying to make a
> signature, not verify an existing one.
> 

I agree, verify_attempted seems to be consistent with decrypt_attempted.
Please, do not make it "ver_", it is easy associated with version.

I guess I prefer long clear names in general, do not mind pushing few
more buttons...

Regards,
  Dmitry

> jamie.
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH v5 0/4] First step of 'show' rewrite

[Austin Clements]

Rename sig_attempted to verify_attempted.

[PATCH v5 1/4] show: Pass notmuch_message_t instead of path to show_message_body.

[Austin Clements]

In addition to simplifying the code, we'll need the notmuch_message_t*
in show_message_body shortly.
---
 notmuch-client.h |    2 +-
 notmuch-reply.c  |    3 +--
 notmuch-show.c   |    3 +--
 show-message.c   |    3 ++-
 4 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index c602e2e..c521efa 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -162,7 +162,7 @@ char *
 query_string_from_args (void *ctx, int argc, char *argv[]);
 
 notmuch_status_t
-show_message_body (const char *filename,
+show_message_body (notmuch_message_t *message,
 		   const notmuch_show_format_t *format,
 		   notmuch_show_params_t *params);
 
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 7ac879f..f8d5f64 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -546,8 +546,7 @@ notmuch_reply_format_default(void *ctx,
 		notmuch_message_get_header (message, "date"),
 		notmuch_message_get_header (message, "from"));
 
-	show_message_body (notmuch_message_get_filename (message),
-			   format, params);
+	show_message_body (message, format, params);
 
 	notmuch_message_destroy (message);
     }
diff --git a/notmuch-show.c b/notmuch-show.c
index 19fb49f..0200b9c 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -758,8 +758,7 @@ show_message (void *ctx,
     }
 
     if (format->part_content)
-	show_message_body (notmuch_message_get_filename (message),
-			   format, params);
+	show_message_body (message, format, params);
 
     if (params->part <= 0) {
 	fputs (format->body_end, stdout);
diff --git a/show-message.c b/show-message.c
index d83f04e..09fa607 100644
--- a/show-message.c
+++ b/show-message.c
@@ -175,7 +175,7 @@ show_message_part (GMimeObject *part,
 }
 
 notmuch_status_t
-show_message_body (const char *filename,
+show_message_body (notmuch_message_t *message,
 		   const notmuch_show_format_t *format,
 		   notmuch_show_params_t *params)
 {
@@ -183,6 +183,7 @@ show_message_body (const char *filename,
     GMimeParser *parser = NULL;
     GMimeMessage *mime_message = NULL;
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
+    const char *filename = notmuch_message_get_filename (message);
     FILE *file = NULL;
     show_message_state_t state;
 
-- 
1.7.7.3

[PATCH v5 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

This wraps all of the complex MIME part handling in a single, simple
function that gets part N from *any* MIME object, so traversing a MIME
part tree becomes a two-line for loop.  Furthermore, the MIME node
structure provides easy access to envelopes for message parts as well
as cryptographic information.

This code is directly derived from the current show_message_body code
(much of it is identical), but the control relation is inverted:
instead of show_message_body controlling the traversal of the MIME
structure and invoking callbacks, the caller controls the traversal of
the MIME structure.
---
 Makefile.local   |    1 +
 mime-node.c      |  238 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 notmuch-client.h |   83 +++++++++++++++++++
 3 files changed, 322 insertions(+), 0 deletions(-)
 create mode 100644 mime-node.c

diff --git a/Makefile.local b/Makefile.local
index 97f397f..516f26e 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -315,6 +315,7 @@ notmuch_client_srcs =		\
 	notmuch-time.c		\
 	query-string.c		\
 	show-message.c		\
+	mime-node.c		\
 	json.c
 
 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
diff --git a/mime-node.c b/mime-node.c
new file mode 100644
index 0000000..760c55f
--- /dev/null
+++ b/mime-node.c
@@ -0,0 +1,238 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2009 Carl Worth
+ * Copyright © 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth <cworth@cworth.org>
+ *          Keith Packard <keithp@keithp.com>
+ *          Austin Clements <aclements@csail.mit.edu>
+ */
+
+#include "notmuch-client.h"
+
+/* Context that gets inherited from the root node. */
+typedef struct mime_node_context {
+    /* Per-message resources.  These are allocated internally and must
+     * be destroyed. */
+    FILE *file;
+    GMimeStream *stream;
+    GMimeParser *parser;
+    GMimeMessage *mime_message;
+
+    /* Context provided by the caller. */
+    GMimeCipherContext *cryptoctx;
+    notmuch_bool_t decrypt;
+} mime_node_context_t;
+
+static int
+_mime_node_context_free (mime_node_context_t *res)
+{
+    if (res->mime_message)
+	g_object_unref (res->mime_message);
+
+    if (res->parser)
+	g_object_unref (res->parser);
+
+    if (res->stream)
+	g_object_unref (res->stream);
+
+    if (res->file)
+	fclose (res->file);
+
+    return 0;
+}
+
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+		GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
+		mime_node_t **root_out)
+{
+    const char *filename = notmuch_message_get_filename (message);
+    mime_node_context_t *mctx;
+    mime_node_t *root;
+    notmuch_status_t status;
+
+    root = talloc_zero (ctx, mime_node_t);
+    if (root == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    /* Create the tree-wide context */
+    mctx = talloc_zero (root, mime_node_context_t);
+    if (mctx == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+    talloc_set_destructor (mctx, _mime_node_context_free);
+
+    mctx->file = fopen (filename, "r");
+    if (! mctx->file) {
+	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
+	status = NOTMUCH_STATUS_FILE_ERROR;
+	goto DONE;
+    }
+
+    mctx->stream = g_mime_stream_file_new (mctx->file);
+    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx->stream), FALSE);
+
+    mctx->parser = g_mime_parser_new_with_stream (mctx->stream);
+
+    mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
+
+    mctx->cryptoctx = cryptoctx;
+    mctx->decrypt = decrypt;
+
+    /* Create the root node */
+    root->part = GMIME_OBJECT (mctx->mime_message);
+    root->envelope_file = message;
+    root->nchildren = 1;
+    root->ctx = mctx;
+
+    *root_out = root;
+    return NOTMUCH_STATUS_SUCCESS;
+
+DONE:
+    talloc_free (root);
+    return status;
+}
+
+static int
+_signature_validity_free (GMimeSignatureValidity **proxy)
+{
+    g_mime_signature_validity_free (*proxy);
+    return 0;
+}
+
+static mime_node_t *
+_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+{
+    mime_node_t *node = talloc_zero (parent, mime_node_t);
+    GError *err = NULL;
+
+    /* Set basic node properties */
+    node->part = part;
+    node->ctx = parent->ctx;
+    if (!talloc_reference (node, node->ctx)) {
+	fprintf (stderr, "Out of memory.\n");
+	talloc_free (node);
+	return NULL;
+    }
+
+    /* Deal with the different types of parts */
+    if (GMIME_IS_PART (part)) {
+	node->nchildren = 0;
+    } else if (GMIME_IS_MULTIPART (part)) {
+	node->nchildren = g_mime_multipart_get_count (GMIME_MULTIPART (part));
+    } else if (GMIME_IS_MESSAGE_PART (part)) {
+	/* Promote part to an envelope and open it */
+	GMimeMessagePart *message_part = GMIME_MESSAGE_PART (part);
+	GMimeMessage *message = g_mime_message_part_get_message (message_part);
+	node->envelope_part = message_part;
+	node->part = GMIME_OBJECT (message);
+	node->nchildren = 1;
+    } else {
+	fprintf (stderr, "Warning: Unknown mime part type: %s.\n",
+		 g_type_name (G_OBJECT_TYPE (part)));
+	talloc_free (node);
+	return NULL;
+    }
+
+    /* Handle PGP/MIME parts */
+    if (GMIME_IS_MULTIPART_ENCRYPTED (part)
+	&& node->ctx->cryptoctx && node->ctx->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 "
+		     "message (must be exactly 2)\n",
+		     node->nchildren);
+	} else {
+	    GMimeMultipartEncrypted *encrypteddata =
+		GMIME_MULTIPART_ENCRYPTED (part);
+	    node->decrypt_attempted = TRUE;
+	    node->decrypted_child = g_mime_multipart_encrypted_decrypt
+		(encrypteddata, node->ctx->cryptoctx, &err);
+	    if (node->decrypted_child) {
+		node->decrypt_success = node->verify_attempted = TRUE;
+		node->sig_validity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
+	    } else {
+		fprintf (stderr, "Failed to decrypt part: %s\n",
+			 (err ? err->message : "no error explanation given"));
+	    }
+	}
+    } else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->cryptoctx) {
+	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 "
+		     "(must be exactly 2)\n",
+		     node->nchildren);
+	} else {
+	    /* For some reason the GMimeSignatureValidity returned
+	     * here is not a const (inconsistent with that
+	     * returned by
+	     * g_mime_multipart_encrypted_get_signature_validity,
+	     * and therefore needs to be properly disposed of.
+	     *
+	     * 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);
+	    node->verify_attempted = TRUE;
+	    node->sig_validity = sig_validity;
+	    if (sig_validity) {
+		GMimeSignatureValidity **proxy =
+		    talloc (node, GMimeSignatureValidity *);
+		*proxy = sig_validity;
+		talloc_set_destructor (proxy, _signature_validity_free);
+	    }
+	}
+    }
+
+    if (node->verify_attempted && !node->sig_validity)
+	fprintf (stderr, "Failed to verify signed part: %s\n",
+		 (err ? err->message : "no error explanation given"));
+
+    if (err)
+	g_error_free (err);
+
+    return node;
+}
+
+mime_node_t *
+mime_node_child (const mime_node_t *parent, int child)
+{
+    GMimeObject *sub;
+
+    if (!parent || child < 0 || child >= parent->nchildren)
+	return NULL;
+
+    if (GMIME_IS_MULTIPART (parent->part)) {
+	if (child == 1 && parent->decrypted_child)
+	    sub = parent->decrypted_child;
+	else
+	    sub = g_mime_multipart_get_part
+		(GMIME_MULTIPART (parent->part), child);
+    } else if (GMIME_IS_MESSAGE (parent->part)) {
+	sub = g_mime_message_get_mime_part (GMIME_MESSAGE (parent->part));
+    } else {
+	/* This should have been caught by message_part_create */
+	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
+			g_type_name (G_OBJECT_TYPE (parent->part)));
+    }
+    return _mime_node_create (parent, sub);
+}
diff --git a/notmuch-client.h b/notmuch-client.h
index c521efa..64b255c 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -241,5 +241,88 @@ notmuch_run_hook (const char *db_path, const char *hook);
 notmuch_bool_t
 debugger_is_active (void);
 
+/* mime-node.c */
+
+/* mime_node_t represents a single node in a MIME tree.  A MIME tree
+ * abstracts the different ways of traversing different types of MIME
+ * parts, allowing a MIME message to be viewed as a generic tree of
+ * parts.  Message-type parts have one child, multipart-type parts
+ * have multiple children, and leaf parts have zero children.
+ */
+typedef struct mime_node {
+    /* The MIME object of this part.  This will be a GMimeMessage,
+     * GMimePart, GMimeMultipart, or a subclass of one of these.
+     *
+     * This will never be a GMimeMessagePart because GMimeMessagePart
+     * is structurally redundant with GMimeMessage.  If this part is a
+     * message (that is, 'part' is a GMimeMessage), then either
+     * envelope_file will be set to a notmuch_message_t (for top-level
+     * messages) or envelope_part will be set to a GMimeMessagePart
+     * (for embedded message parts).
+     */
+    GMimeObject *part;
+
+    /* If part is a GMimeMessage, these record the envelope of the
+     * message: either a notmuch_message_t representing a top-level
+     * message, or a GMimeMessagePart representing a MIME part
+     * containing a message.
+     */
+    notmuch_message_t *envelope_file;
+    GMimeMessagePart *envelope_part;
+
+    /* The number of children of this part. */
+    int nchildren;
+
+    /* True if decryption of this part was attempted. */
+    notmuch_bool_t decrypt_attempted;
+    /* True if decryption of this part's child succeeded.  In this
+     * case, the decrypted part is substituted for the second child of
+     * this part (which would usually be the encrypted data). */
+    notmuch_bool_t decrypt_success;
+
+    /* True if signature verification on this part was attempted. */
+    notmuch_bool_t verify_attempted;
+    /* For signed or encrypted containers, the validity of the
+     * signature.  May be NULL if signature verification failed.  If
+     * there are simply no signatures, this will be non-NULL with an
+     * empty signers list. */
+    const GMimeSignatureValidity *sig_validity;
+
+    /* Internal: Context inherited from the root iterator. */
+    struct mime_node_context *ctx;
+
+    /* Internal: For successfully decrypted multipart parts, the
+     * decrypted part to substitute for the second child. */
+    GMimeObject *decrypted_child;
+} mime_node_t;
+
+/* 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.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Root node is returned in *node_out.
+ *
+ * NOTMUCH_STATUS_FILE_ERROR: Failed to open message file.
+ *
+ * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
+ */
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+		GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
+		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
+ * node.
+ *
+ * In case of any failure, this function returns NULL, (after printing
+ * an error message on stderr).
+ */
+mime_node_t *
+mime_node_child (const mime_node_t *parent, int child);
+
 #include "command-line-arguments.h"
 #endif
-- 
1.7.7.3

[PATCH v5 3/4] Utility function to seek in MIME trees in depth-first order.

[Austin Clements]

This function matches how we number parts for the --part argument to
show.  It will allow us to jump directly to the desired part, rather
than traversing the entire tree and carefully tracking whether or not
we're "in the zone".
---
 mime-node.c      |   27 +++++++++++++++++++++++++++
 notmuch-client.h |    5 +++++
 2 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 760c55f..d26bb44 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -236,3 +236,30 @@ mime_node_child (const mime_node_t *parent, int child)
     }
     return _mime_node_create (parent, sub);
 }
+
+static mime_node_t *
+_mime_node_seek_dfs_walk (mime_node_t *node, int *n)
+{
+    mime_node_t *ret = NULL;
+    int i;
+
+    if (*n == 0)
+	return node;
+
+    *n -= 1;
+    for (i = 0; i < node->nchildren && !ret; i++) {
+	mime_node_t *child = mime_node_child (node, i);
+	ret = _mime_node_seek_dfs_walk (child, n);
+	if (!ret)
+	    talloc_free (child);
+    }
+    return ret;
+}
+
+mime_node_t *
+mime_node_seek_dfs (mime_node_t *node, int n)
+{
+    if (n < 0)
+	return NULL;
+    return _mime_node_seek_dfs_walk (node, &n);
+}
diff --git a/notmuch-client.h b/notmuch-client.h
index 64b255c..517c010 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -324,5 +324,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
 mime_node_t *
 mime_node_child (const mime_node_t *parent, int child);
 
+/* Return the nth child of node in a depth-first traversal.  If n is
+ * 0, returns node itself.  Returns NULL if there is no such part. */
+mime_node_t *
+mime_node_seek_dfs (mime_node_t *node, int n);
+
 #include "command-line-arguments.h"
 #endif
-- 
1.7.7.3

[PATCH v5 4/4] show: Rewrite show_message_body to use the MIME tree interface.

[Austin Clements]

This removes all of the MIME traversal logic from show_message_body
and leaves only its interaction with the format callbacks.

Besides isolating concerns, since traversal happens behind a trivial
interface, there is now much less code duplication in
show_message_part.  Also, this uses mime_node_seek_dfs to start at the
requested part, eliminating all of the logic about parts being
selected or being in_zone (and reducing the "show state" to only a
part counter).  notmuch_show_params_t no longer needs to be passed
through the recursion because the only two fields that mattered
(related to crypto) are now handled by the MIME tree.

The few remaining complexities in show_message_part highlight
irregularities in the format callbacks with respect to top-level
messages and embedded message parts.

Since this is a rewrite, the diff is not very enlightening.  It's
easier to look at the old code and the new code side-by-side.
---
 show-message.c |  229 +++++++++++++-------------------------------------------
 1 files changed, 52 insertions(+), 177 deletions(-)

diff --git a/show-message.c b/show-message.c
index 09fa607..8768889 100644
--- a/show-message.c
+++ b/show-message.c
@@ -24,154 +24,54 @@
 
 typedef struct show_message_state {
     int part_count;
-    int in_zone;
 } show_message_state_t;
 
 static void
-show_message_part (GMimeObject *part,
+show_message_part (mime_node_t *node,
 		   show_message_state_t *state,
 		   const notmuch_show_format_t *format,
-		   notmuch_show_params_t *params,
 		   int first)
 {
-    GMimeObject *decryptedpart = NULL;
-    int selected;
+    /* Formatters expect the envelope for embedded message parts */
+    GMimeObject *part = node->envelope_part ?
+	GMIME_OBJECT (node->envelope_part) : node->part;
+    int i;
+
+    if (!first)
+	fputs (format->part_sep, stdout);
+
+    /* Format this part */
+    if (format->part_start)
+	format->part_start (part, &(state->part_count));
+
+    if (node->decrypt_attempted && format->part_encstatus)
+	format->part_encstatus (node->decrypt_success);
+
+    if (node->verify_attempted && format->part_sigstatus)
+	format->part_sigstatus (node->sig_validity);
+
+    format->part_content (part);
+
+    if (node->envelope_part) {
+	fputs (format->header_start, stdout);
+	if (format->header_message_part)
+	    format->header_message_part (GMIME_MESSAGE (node->part));
+	fputs (format->header_end, stdout);
+
+	fputs (format->body_start, stdout);
+    }
+
+    /* Recurse over the children */
     state->part_count += 1;
+    for (i = 0; i < node->nchildren; i++)
+	show_message_part (mime_node_child (node, i), state, format, i == 0);
 
-    if (! (GMIME_IS_PART (part) || GMIME_IS_MULTIPART (part) || GMIME_IS_MESSAGE_PART (part))) {
-	fprintf (stderr, "Warning: Not displaying unknown mime part: %s.\n",
-		 g_type_name (G_OBJECT_TYPE (part)));
-	return;
-    }
+    /* Finish this part */
+    if (node->envelope_part)
+	fputs (format->body_end, stdout);
 
-    selected = (params->part <= 0 || state->part_count == params->part);
-
-    if (selected || state->in_zone) {
-	if (!first && (params->part <= 0 || state->in_zone))
-	    fputs (format->part_sep, stdout);
-
-	if (format->part_start)
-	    format->part_start (part, &(state->part_count));
-    }
-
-    /* handle PGP/MIME parts */
-    if (GMIME_IS_MULTIPART (part) && params->cryptoctx) {
-	GMimeMultipart *multipart = GMIME_MULTIPART (part);
-	GError* err = NULL;
-
-	if (GMIME_IS_MULTIPART_ENCRYPTED (part) && params->decrypt)
-	{
-	    if ( g_mime_multipart_get_count (multipart) != 2 ) {
-		/* this violates RFC 3156 section 4, so we won't bother with it. */
-		fprintf (stderr,
-			 "Error: %d part(s) for a multipart/encrypted message (should be exactly 2)\n",
-			 g_mime_multipart_get_count (multipart));
-	    } else {
-		GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
-		decryptedpart = g_mime_multipart_encrypted_decrypt (encrypteddata, params->cryptoctx, &err);
-		if (decryptedpart) {
-		    if ((selected || state->in_zone) && format->part_encstatus)
-			format->part_encstatus (1);
-		    const GMimeSignatureValidity *sigvalidity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
-		    if (!sigvalidity)
-			fprintf (stderr, "Failed to verify signed part: %s\n", (err ? err->message : "no error explanation given"));
-		    if ((selected || state->in_zone) && format->part_sigstatus)
-			format->part_sigstatus (sigvalidity);
-		} else {
-		    fprintf (stderr, "Failed to decrypt part: %s\n", (err ? err->message : "no error explanation given"));
-		    if ((selected || state->in_zone) && format->part_encstatus)
-			format->part_encstatus (0);
-		}
-	    }
-	}
-	else if (GMIME_IS_MULTIPART_SIGNED (part))
-	{
-	    if ( g_mime_multipart_get_count (multipart) != 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 (should be exactly 2)\n",
-			 g_mime_multipart_get_count (multipart));
-	    } else {
-		/* For some reason the GMimeSignatureValidity returned
-		 * here is not a const (inconsistent with that
-		 * returned by
-		 * g_mime_multipart_encrypted_get_signature_validity,
-		 * and therefore needs to be properly disposed of.
-		 * Hopefully the API will become more consistent. */
-		GMimeSignatureValidity *sigvalidity = g_mime_multipart_signed_verify (GMIME_MULTIPART_SIGNED (part), params->cryptoctx, &err);
-		if (!sigvalidity) {
-		    fprintf (stderr, "Failed to verify signed part: %s\n", (err ? err->message : "no error explanation given"));
-		}
-		if ((selected || state->in_zone) && format->part_sigstatus)
-		    format->part_sigstatus (sigvalidity);
-		if (sigvalidity)
-		    g_mime_signature_validity_free (sigvalidity);
-	    }
-	}
-
-	if (err)
-	    g_error_free (err);
-    }
-    /* end handle PGP/MIME parts */
-
-    if (selected || state->in_zone)
-	format->part_content (part);
-
-    if (GMIME_IS_MULTIPART (part)) {
-	GMimeMultipart *multipart = GMIME_MULTIPART (part);
-	int i;
-
-	if (selected)
-	    state->in_zone = 1;
-
-	if (decryptedpart) {
-	    /* We emit the useless application/pgp-encrypted version
-	     * part here only to keep the emitted output as consistent
-	     * as possible between decrypted output and the
-	     * unprocessed multipart/mime. For some strange reason,
-	     * the actual encrypted data is the second part of the
-	     * multipart. */
-	    show_message_part (g_mime_multipart_get_part (multipart, 0), state, format, params, TRUE);
-	    show_message_part (decryptedpart, state, format, params, FALSE);
-	} else {
-	    for (i = 0; i < g_mime_multipart_get_count (multipart); i++) {
-		show_message_part (g_mime_multipart_get_part (multipart, i),
-				   state, format, params, i == 0);
-	    }
-	}
-
-	if (selected)
-	    state->in_zone = 0;
-
-    } else if (GMIME_IS_MESSAGE_PART (part)) {
-	GMimeMessage *mime_message = g_mime_message_part_get_message (GMIME_MESSAGE_PART (part));
-
-	if (selected)
-	    state->in_zone = 1;
-
-	if (selected || (!selected && state->in_zone)) {
-	    fputs (format->header_start, stdout);
-	    if (format->header_message_part)
-		format->header_message_part (mime_message);
-	    fputs (format->header_end, stdout);
-
-	    fputs (format->body_start, stdout);
-	}
-
-	show_message_part (g_mime_message_get_mime_part (mime_message),
-			   state, format, params, TRUE);
-
-	if (selected || (!selected && state->in_zone))
-	    fputs (format->body_end, stdout);
-
-	if (selected)
-	    state->in_zone = 0;
-    }
-
-    if (selected || state->in_zone) {
-	if (format->part_end)
-	    format->part_end (part);
-    }
+    if (format->part_end)
+	format->part_end (part);
 }
 
 notmuch_status_t
@@ -179,49 +79,24 @@ show_message_body (notmuch_message_t *message,
 		   const notmuch_show_format_t *format,
 		   notmuch_show_params_t *params)
 {
-    GMimeStream *stream = NULL;
-    GMimeParser *parser = NULL;
-    GMimeMessage *mime_message = NULL;
-    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
-    const char *filename = notmuch_message_get_filename (message);
-    FILE *file = NULL;
+    notmuch_status_t ret;
     show_message_state_t state;
+    mime_node_t *root, *part;
 
-    state.part_count = 0;
-    state.in_zone = 0;
+    ret = mime_node_open (NULL, message, params->cryptoctx, params->decrypt,
+			  &root);
+    if (ret)
+	return ret;
 
-    file = fopen (filename, "r");
-    if (! file) {
-	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
-	ret = NOTMUCH_STATUS_FILE_ERROR;
-	goto DONE;
-    }
+    /* The caller of show_message_body has already handled the
+     * outermost envelope, so skip it. */
+    state.part_count = MAX (params->part, 1);
 
-    stream = g_mime_stream_file_new (file);
-    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream), FALSE);
+    part = mime_node_seek_dfs (root, state.part_count);
+    if (part)
+	show_message_part (part, &state, format, TRUE);
 
-    parser = g_mime_parser_new_with_stream (stream);
+    talloc_free (root);
 
-    mime_message = g_mime_parser_construct_message (parser);
-
-    show_message_part (g_mime_message_get_mime_part (mime_message),
-		       &state,
-		       format,
-		       params,
-		       TRUE);
-
-  DONE:
-    if (mime_message)
-	g_object_unref (mime_message);
-
-    if (parser)
-	g_object_unref (parser);
-
-    if (stream)
-	g_object_unref (stream);
-
-    if (file)
-	fclose (file);
-
-    return ret;
+    return NOTMUCH_STATUS_SUCCESS;
 }
-- 
1.7.7.3

Re: [PATCH v4 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

Quoth Jameson Graef Rollins on Dec 23 at 11:55 pm:
> On Fri, 23 Dec 2011 22:45:46 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > +    /* True if decryption of this part was attempted. */
> > +    notmuch_bool_t decrypt_attempted;
> > +    /* True if decryption of this part's child succeeded.  In this
> > +     * case, the decrypted part is substituted for the second child of
> > +     * this part (which would usually be the encrypted data). */
> > +    notmuch_bool_t decrypt_success;
> > +
> > +    /* True if signature verification on this part was attempted. */
> > +    notmuch_bool_t sig_attempted;
> 
> I think these new variables make sense, and reflect the correct
> semantics, as you already mentioned.
> 
> I do, however, think the later variable should be called
> "verify_attempted" (or "verification_", or "ver_"?), instead of
> "sig_attempted", since verification is the complementary action on a
> signed part, just as decryption is for an encrypted one.
> "sig_attempted" somehow implies to me that one is trying to make a
> signature, not verify an existing one.

The intent was to parallel sig_validity, but I see your point.  v5
with verify_attempted will be coming along soon (when our power comes
back).

Re: [PATCH v5 0/4] First step of 'show' rewrite

[Dmitry Kurochkin]

All looks good to me.  Let's push it.

I have some minor comments, but they are all more of a style preference
(e.g. sig_validity vs signature_validity, using conditional operator (x
? y : z) where possible).

Regards,
  Dmitry

Re: [PATCH 3/4] Utility function to seek in MIME trees in depth-first order.

[Dmitry Kurochkin]

On Fri, 23 Dec 2011 22:46:19 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Dmitry Kurochkin on Dec 10 at  3:43 pm:
> > On Fri,  9 Dec 2011 14:54:27 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > > This function matches how we number parts for the --part argument to
> > > show.  It will allow us to jump directly to the desired part, rather
> > > than traversing the entire tree and carefully tracking whether or not
> > > we're "in the zone".
> > > ---
> > >  mime-node.c      |   25 +++++++++++++++++++++++++
> > >  notmuch-client.h |    5 +++++
> > >  2 files changed, 30 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/mime-node.c b/mime-node.c
> > > index a8e4a59..207818e 100644
> > > --- a/mime-node.c
> > > +++ b/mime-node.c
> > > @@ -232,3 +232,28 @@ mime_node_child (const mime_node_t *parent, int child)
> > >  			g_type_name (G_OBJECT_TYPE (parent->part)));
> > >      }
> > >  }
> > > +
> > > +static mime_node_t *
> > > +_mime_node_seek_dfs_walk (mime_node_t *node, int *n)
> > > +{
> > > +    mime_node_t *ret = NULL;
> > > +    int i;
> > > +
> > 
> > Can we move declarations below the if (which does not need them)?  I
> > always have troubles remembering if (recent enough) C standard allows
> > that or it is a GCC extension.  FWIW in the previous patch there are
> > declarations in the middle of a block, e.g.:
> > 
> > 	} else {
> > 	    out->is_signed = TRUE;
> >             ...
> > 	    GMimeSignatureValidity *sig_validity = g_mime_multipart_signed_verify
> > 		(GMIME_MULTIPART_SIGNED (part), out->ctx->cryptoctx, &err);
> > 
> > So either we can move these declarations to where they are needed, or we
> > should fix it in _mime_node_create().
> 
> Since prevailing notmuch style seems to be top-declarations, I fixed
> up _mime_node_create instead (personally I prefer C99-style
> declarations, but *shrug*).
> 

If there is any code which already uses C99-style declarations, then we
should use them in the new code IMO.  Perhaps whether to use C99-style
declarations or not should be a coding style requirement.

Regards,
  Dmitry

> > > +    if (*n <= 0)
> > 
> > Comment for mime_node_seek_dfs() says that the function returns the node
> > itself for n = 0, but does not say anything about n < 0.  I would expect
> > the function to return NULL for n < 0.  In any case, the comment below
> > should probably mention what happens for n < 0;
> 
> Good point.  I made it return NULL for n < 0.  I think this logically
> falls under "Returns NULL if there is no such part."
> 
> > > +	return node;
> > > +
> > > +    *n = *n - 1;
> > 
> > Perhaps *n -= 1?  Or even --(*n)?
> 
> Changed to *n -= 1.
> 
> > > +    for (i = 0; i < node->children && !ret; i++) {
> > 
> > Consider s/i++/++i/.
> 
> notmuch uses i++ remarkably consistently, so I left this.
> 
> > Regards,
> >   Dmitry

Re: [PATCH v5 0/4] First step of 'show' rewrite

[David Bremner]

On Sat, 24 Dec 2011 13:52:42 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Rename sig_attempted to verify_attempted.
> 

Pushed. 

d

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Daniel Kahn Gillmor]

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

On 12/23/2011 10:45 PM, Austin Clements wrote:
> Quoth Dmitry Kurochkin on Dec 10 at  3:25 am:
>> +           /* For some reason the GMimeSignatureValidity returned
>> +            * here is not a const (inconsistent with that
>> +            * returned by
>> +            * g_mime_multipart_encrypted_get_signature_validity,
>> +            * and therefore needs to be properly disposed of.
>> +            * Hopefully the API will become more consistent. */
>>
>> Ouch.  In gmime 2.6 this API has changed, but looks like the
>> issue is still there.  Is there a bug for it?  If yes, we should
>> add a reference to the comment.  Otherwise, we should file the
>> bug and then add a reference to the comment :)
> 
> It looks like they're both non-const in 2.6 (which makes more sense to
> me).  I updated the comment to mention this.

Here's the bug report where this was discussed with upstream, fwiw:

  https://bugzilla.gnome.org/show_bug.cgi?id=640911

	--dkg


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

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

Quoth Daniel Kahn Gillmor on Dec 27 at  9:27 am:
> On 12/23/2011 10:45 PM, Austin Clements wrote:
> > Quoth Dmitry Kurochkin on Dec 10 at  3:25 am:
> >> +           /* For some reason the GMimeSignatureValidity returned
> >> +            * here is not a const (inconsistent with that
> >> +            * returned by
> >> +            * g_mime_multipart_encrypted_get_signature_validity,
> >> +            * and therefore needs to be properly disposed of.
> >> +            * Hopefully the API will become more consistent. */
> >>
> >> Ouch.  In gmime 2.6 this API has changed, but looks like the
> >> issue is still there.  Is there a bug for it?  If yes, we should
> >> add a reference to the comment.  Otherwise, we should file the
> >> bug and then add a reference to the comment :)
> > 
> > It looks like they're both non-const in 2.6 (which makes more sense to
> > me).  I updated the comment to mention this.
> 
> Here's the bug report where this was discussed with upstream, fwiw:
> 
>   https://bugzilla.gnome.org/show_bug.cgi?id=640911

Oh, are we not supposed to be disposing of the GMimeSignatureValidity
returned by g_mime_multipart_encrypted_get_signature_validity
ourselves?  Comment 1 on that bug report suggests that we shouldn't.
The old show code did, so I ported that behavior over to the new code.