[PATCH 0/5] Rewrite mbox and raw show formats

[PATCH 0/5] Rewrite mbox and raw show formats

[Austin Clements]

The show (rewrite) must go on.  These are the last two formats in
notmuch show proper; the only remaining format is the reply format.

One of the ultimate goals of the show rewrite is to make raw actually
work (and probably to split it into two different formats).  This
series does not do that; it just ports the existing raw format.  I
plan to make the new raw formats once the rewrite is done.

[PATCH 1/5] show: Allow formatters to return errors

[Austin Clements]

Formatter errors are propagated to the exit status of notmuch show.

This isn't used by the JSON or text formatters, but it will be useful
for the raw format, which is pickier.
---
 notmuch-client.h |    6 ++--
 notmuch-show.c   |   69 +++++++++++++++++++++++++++++++++++------------------
 2 files changed, 48 insertions(+), 27 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index f4a62cc..a220fe4 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -67,9 +67,9 @@ struct notmuch_show_params;
 
 typedef struct notmuch_show_format {
     const char *message_set_start;
-    void (*part) (const void *ctx,
-		  struct mime_node *node, int indent,
-		  const struct notmuch_show_params *params);
+    notmuch_status_t (*part) (const void *ctx,
+			      struct mime_node *node, int indent,
+			      const struct notmuch_show_params *params);
     const char *message_start;
     void (*message) (const void *ctx,
 		     notmuch_message_t *message,
diff --git a/notmuch-show.c b/notmuch-show.c
index 6a171a4..648f468 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -23,7 +23,7 @@
 static void
 format_headers_message_part_text (GMimeMessage *message);
 
-static void
+static notmuch_status_t
 format_part_text (const void *ctx, mime_node_t *node,
 		  int indent, const notmuch_show_params_t *params);
 
@@ -34,7 +34,7 @@ static const notmuch_show_format_t format_text = {
     .message_set_end = ""
 };
 
-static void
+static notmuch_status_t
 format_part_json_entry (const void *ctx, mime_node_t *node,
 			int indent, const notmuch_show_params_t *params);
 
@@ -562,7 +562,7 @@ format_part_content_raw (GMimeObject *part)
 	g_object_unref(stream_stdout);
 }
 
-static void
+static notmuch_status_t
 format_part_text (const void *ctx, mime_node_t *node,
 		  int indent, const notmuch_show_params_t *params)
 {
@@ -650,6 +650,8 @@ format_part_text (const void *ctx, mime_node_t *node,
 	printf ("\fbody}\n");
 
     printf ("\f%s}\n", part_type);
+
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 static void
@@ -751,14 +753,16 @@ format_part_json (const void *ctx, mime_node_t *node, notmuch_bool_t first)
     printf ("%s}", terminator);
 }
 
-static void
+static notmuch_status_t
 format_part_json_entry (const void *ctx, mime_node_t *node, unused (int indent),
 			unused (const notmuch_show_params_t *params))
 {
     format_part_json (ctx, node, TRUE);
+
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
-static void
+static notmuch_status_t
 show_message (void *ctx,
 	      const notmuch_show_format_t *format,
 	      notmuch_message_t *message,
@@ -768,14 +772,18 @@ show_message (void *ctx,
     if (format->part) {
 	void *local = talloc_new (ctx);
 	mime_node_t *root, *part;
+	notmuch_status_t status;
 
-	if (mime_node_open (local, message, params->cryptoctx, params->decrypt,
-			    &root) == NOTMUCH_STATUS_SUCCESS &&
-	    (part = mime_node_seek_dfs (root, (params->part < 0 ?
-					       0 : params->part))))
-	    format->part (local, part, indent, params);
+	status = mime_node_open (local, message, params->cryptoctx,
+				 params->decrypt, &root);
+	if (status)
+	    goto DONE;
+	part = mime_node_seek_dfs (root, (params->part < 0 ? 0 : params->part));
+	if (part)
+	    status = format->part (local, part, indent, params);
+      DONE:
 	talloc_free (local);
-	return;
+	return status;
     }
 
     if (params->part <= 0) {
@@ -799,9 +807,11 @@ show_message (void *ctx,
 
 	fputs (format->message_end, stdout);
     }
+
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
-static void
+static notmuch_status_t
 show_messages (void *ctx,
 	       const notmuch_show_format_t *format,
 	       notmuch_messages_t *messages,
@@ -812,6 +822,7 @@ show_messages (void *ctx,
     notmuch_bool_t match;
     int first_set = 1;
     int next_indent;
+    notmuch_status_t status, res = NOTMUCH_STATUS_SUCCESS;
 
     fputs (format->message_set_start, stdout);
 
@@ -832,17 +843,22 @@ show_messages (void *ctx,
 	next_indent = indent;
 
 	if (match || params->entire_thread) {
-	    show_message (ctx, format, message, indent, params);
+	    status = show_message (ctx, format, message, indent, params);
+	    if (status && !res)
+		res = status;
 	    next_indent = indent + 1;
 
-	    fputs (format->message_set_sep, stdout);
+	    if (!status)
+		fputs (format->message_set_sep, stdout);
 	}
 
-	show_messages (ctx,
-		       format,
-		       notmuch_message_get_replies (message),
-		       next_indent,
-		       params);
+	status = show_messages (ctx,
+				format,
+				notmuch_message_get_replies (message),
+				next_indent,
+				params);
+	if (status && !res)
+	    res = status;
 
 	notmuch_message_destroy (message);
 
@@ -850,6 +866,8 @@ show_messages (void *ctx,
     }
 
     fputs (format->message_set_end, stdout);
+
+    return res;
 }
 
 /* Formatted output of single message */
@@ -914,13 +932,13 @@ do_show_single (void *ctx,
 
 	fclose (file);
 
+	return 0;
+
     } else {
 
-	show_message (ctx, format, message, 0, params);
+	return show_message (ctx, format, message, 0, params) != NOTMUCH_STATUS_SUCCESS;
 
     }
-
-    return 0;
 }
 
 /* Formatted output of threads */
@@ -934,6 +952,7 @@ do_show (void *ctx,
     notmuch_thread_t *thread;
     notmuch_messages_t *messages;
     int first_toplevel = 1;
+    notmuch_status_t status, res = NOTMUCH_STATUS_SUCCESS;
 
     fputs (format->message_set_start, stdout);
 
@@ -953,7 +972,9 @@ do_show (void *ctx,
 	    fputs (format->message_set_sep, stdout);
 	first_toplevel = 0;
 
-	show_messages (ctx, format, messages, 0, params);
+	status = show_messages (ctx, format, messages, 0, params);
+	if (status && !res)
+	    res = status;
 
 	notmuch_thread_destroy (thread);
 
@@ -961,7 +982,7 @@ do_show (void *ctx,
 
     fputs (format->message_set_end, stdout);
 
-    return 0;
+    return res != NOTMUCH_STATUS_SUCCESS;
 }
 
 enum {
-- 
1.7.7.3

[PATCH 2/5] show: Move format_message_mbox with the other new-style formats

[Austin Clements]

Just code motion.
---
 notmuch-show.c |  100 ++++++++++++++++++++++++++++----------------------------
 1 files changed, 50 insertions(+), 50 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 648f468..b3a4550 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -219,56 +219,6 @@ _is_from_line (const char *line)
 	return 0;
 }
 
-/* Print a message in "mboxrd" format as documented, for example,
- * here:
- *
- * http://qmail.org/qmail-manual-html/man5/mbox.html
- */
-static void
-format_message_mbox (const void *ctx,
-		     notmuch_message_t *message,
-		     unused (int indent))
-{
-    const char *filename;
-    FILE *file;
-    const char *from;
-
-    time_t date;
-    struct tm date_gmtime;
-    char date_asctime[26];
-
-    char *line = NULL;
-    size_t line_size;
-    ssize_t line_len;
-
-    filename = notmuch_message_get_filename (message);
-    file = fopen (filename, "r");
-    if (file == NULL) {
-	fprintf (stderr, "Failed to open %s: %s\n",
-		 filename, strerror (errno));
-	return;
-    }
-
-    from = notmuch_message_get_header (message, "from");
-    from = _extract_email_address (ctx, from);
-
-    date = notmuch_message_get_date (message);
-    gmtime_r (&date, &date_gmtime);
-    asctime_r (&date_gmtime, date_asctime);
-
-    printf ("From %s %s", from, date_asctime);
-
-    while ((line_len = getline (&line, &line_size, file)) != -1 ) {
-	if (_is_from_line (line))
-	    putchar ('>');
-	printf ("%s", line);
-    }
-
-    printf ("\n");
-
-    fclose (file);
-}
-
 static void
 format_headers_message_part_text (GMimeMessage *message)
 {
@@ -762,6 +712,56 @@ format_part_json_entry (const void *ctx, mime_node_t *node, unused (int indent),
     return NOTMUCH_STATUS_SUCCESS;
 }
 
+/* Print a message in "mboxrd" format as documented, for example,
+ * here:
+ *
+ * http://qmail.org/qmail-manual-html/man5/mbox.html
+ */
+static void
+format_message_mbox (const void *ctx,
+		     notmuch_message_t *message,
+		     unused (int indent))
+{
+    const char *filename;
+    FILE *file;
+    const char *from;
+
+    time_t date;
+    struct tm date_gmtime;
+    char date_asctime[26];
+
+    char *line = NULL;
+    size_t line_size;
+    ssize_t line_len;
+
+    filename = notmuch_message_get_filename (message);
+    file = fopen (filename, "r");
+    if (file == NULL) {
+	fprintf (stderr, "Failed to open %s: %s\n",
+		 filename, strerror (errno));
+	return;
+    }
+
+    from = notmuch_message_get_header (message, "from");
+    from = _extract_email_address (ctx, from);
+
+    date = notmuch_message_get_date (message);
+    gmtime_r (&date, &date_gmtime);
+    asctime_r (&date_gmtime, date_asctime);
+
+    printf ("From %s %s", from, date_asctime);
+
+    while ((line_len = getline (&line, &line_size, file)) != -1 ) {
+	if (_is_from_line (line))
+	    putchar ('>');
+	printf ("%s", line);
+    }
+
+    printf ("\n");
+
+    fclose (file);
+}
+
 static notmuch_status_t
 show_message (void *ctx,
 	      const notmuch_show_format_t *format,
-- 
1.7.7.3

[PATCH 3/5] show: Convert mbox format to new self-recursive style

[Austin Clements]

Given the lack of recursion, this is pretty easy.
---
 notmuch-show.c |   40 ++++++++++++++++++----------------------
 1 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index b3a4550..72c141e 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -45,25 +45,15 @@ static const notmuch_show_format_t format_json = {
     .message_set_end = "]"
 };
 
-static void
-format_message_mbox (const void *ctx,
-		     notmuch_message_t *message,
-		     unused (int indent));
+static notmuch_status_t
+format_part_mbox (const void *ctx, mime_node_t *node,
+		  int indent, const notmuch_show_params_t *params);
 
 static const notmuch_show_format_t format_mbox = {
-    "", NULL,
-        "", format_message_mbox,
-            "", NULL, NULL, "",
-            "",
-                NULL,
-                NULL,
-                NULL,
-                NULL,
-                NULL,
-                "",
-            "",
-        "", "",
-    ""
+    .message_set_start = "",
+    .part = format_part_mbox,
+    .message_set_sep = "",
+    .message_set_end = ""
 };
 
 static void
@@ -717,11 +707,12 @@ format_part_json_entry (const void *ctx, mime_node_t *node, unused (int indent),
  *
  * http://qmail.org/qmail-manual-html/man5/mbox.html
  */
-static void
-format_message_mbox (const void *ctx,
-		     notmuch_message_t *message,
-		     unused (int indent))
+static notmuch_status_t
+format_part_mbox (const void *ctx, mime_node_t *node, unused (int indent),
+		  unused (const notmuch_show_params_t *params))
 {
+    notmuch_message_t *message = node->envelope_file;
+
     const char *filename;
     FILE *file;
     const char *from;
@@ -734,12 +725,15 @@ format_message_mbox (const void *ctx,
     size_t line_size;
     ssize_t line_len;
 
+    if (!message)
+	INTERNAL_ERROR ("format_part_mbox requires a root part");
+
     filename = notmuch_message_get_filename (message);
     file = fopen (filename, "r");
     if (file == NULL) {
 	fprintf (stderr, "Failed to open %s: %s\n",
 		 filename, strerror (errno));
-	return;
+	return NOTMUCH_STATUS_FILE_ERROR;
     }
 
     from = notmuch_message_get_header (message, "from");
@@ -760,6 +754,8 @@ format_message_mbox (const void *ctx,
     printf ("\n");
 
     fclose (file);
+
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 static notmuch_status_t
-- 
1.7.7.3

[PATCH 4/5] show: Move format_part_content_raw with the other new-style formats

[Austin Clements]

Just code motion.
---
 notmuch-show.c |   54 +++++++++++++++++++++++++++---------------------------
 1 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 72c141e..6f6052c 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -475,33 +475,6 @@ format_part_sigstatus_json (mime_node_t *node)
 }
 #endif
 
-static void
-format_part_content_raw (GMimeObject *part)
-{
-    if (! GMIME_IS_PART (part))
-	return;
-
-    GMimeStream *stream_stdout;
-    GMimeStream *stream_filter = NULL;
-    GMimeDataWrapper *wrapper;
-
-    stream_stdout = g_mime_stream_file_new (stdout);
-    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
-
-    stream_filter = g_mime_stream_filter_new (stream_stdout);
-
-    wrapper = g_mime_part_get_content_object (GMIME_PART (part));
-
-    if (wrapper && stream_filter)
-	g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
-
-    if (stream_filter)
-	g_object_unref (stream_filter);
-
-    if (stream_stdout)
-	g_object_unref(stream_stdout);
-}
-
 static notmuch_status_t
 format_part_text (const void *ctx, mime_node_t *node,
 		  int indent, const notmuch_show_params_t *params)
@@ -758,6 +731,33 @@ format_part_mbox (const void *ctx, mime_node_t *node, unused (int indent),
     return NOTMUCH_STATUS_SUCCESS;
 }
 
+static void
+format_part_content_raw (GMimeObject *part)
+{
+    if (! GMIME_IS_PART (part))
+	return;
+
+    GMimeStream *stream_stdout;
+    GMimeStream *stream_filter = NULL;
+    GMimeDataWrapper *wrapper;
+
+    stream_stdout = g_mime_stream_file_new (stdout);
+    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
+
+    stream_filter = g_mime_stream_filter_new (stream_stdout);
+
+    wrapper = g_mime_part_get_content_object (GMIME_PART (part));
+
+    if (wrapper && stream_filter)
+	g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
+
+    if (stream_filter)
+	g_object_unref (stream_filter);
+
+    if (stream_stdout)
+	g_object_unref(stream_stdout);
+}
+
 static notmuch_status_t
 show_message (void *ctx,
 	      const notmuch_show_format_t *format,
-- 
1.7.7.3

[PATCH 5/5] show: Convert raw format to the new self-recursive style

[Austin Clements]

This is fully compatible for root and leaf parts, but drops support
for interior parts.  Showing interior parts in raw has always been
braindead broken, so I don't think anyone will miss this.  Tests have
been updated to reflect this.

The diff looks complicated, but the change is simple.  We delete
format_headers_message_part_text, since it's no longer needed; move
the special-case code for --part=0 that used to be in do_show_single
to format_part_raw; and re-indent the code for formatting leaf parts
in format_part_raw.
---
 notmuch-show.c |  165 ++++++++++++++++++++++----------------------------------
 test/multipart |   51 ++++--------------
 2 files changed, 76 insertions(+), 140 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 6f6052c..9c2f890 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -20,9 +20,6 @@
 
 #include "notmuch-client.h"
 
-static void
-format_headers_message_part_text (GMimeMessage *message);
-
 static notmuch_status_t
 format_part_text (const void *ctx, mime_node_t *node,
 		  int indent, const notmuch_show_params_t *params);
@@ -56,23 +53,16 @@ static const notmuch_show_format_t format_mbox = {
     .message_set_end = ""
 };
 
-static void
-format_part_content_raw (GMimeObject *part);
+static notmuch_status_t
+format_part_raw (unused (const void *ctx), mime_node_t *node,
+		 unused (int indent),
+		 unused (const notmuch_show_params_t *params));
 
 static const notmuch_show_format_t format_raw = {
-    "", NULL,
-	"", NULL,
-	    "", NULL, format_headers_message_part_text, "\n",
-            "",
-                NULL,
-                NULL,
-                NULL,
-                format_part_content_raw,
-                NULL,
-                "",
-            "",
-	"", "",
-    ""
+    .message_set_start = "",
+    .part = format_part_raw,
+    .message_set_sep = "",
+    .message_set_end = ""
 };
 
 static const char *
@@ -210,27 +200,6 @@ _is_from_line (const char *line)
 }
 
 static void
-format_headers_message_part_text (GMimeMessage *message)
-{
-    InternetAddressList *recipients;
-    const char *recipients_string;
-
-    printf ("Subject: %s\n", g_mime_message_get_subject (message));
-    printf ("From: %s\n", g_mime_message_get_sender (message));
-    recipients = g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_TO);
-    recipients_string = internet_address_list_to_string (recipients, 0);
-    if (recipients_string)
-	printf ("To: %s\n",
-		recipients_string);
-    recipients = g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_CC);
-    recipients_string = internet_address_list_to_string (recipients, 0);
-    if (recipients_string)
-	printf ("Cc: %s\n",
-		recipients_string);
-    printf ("Date: %s\n", g_mime_message_get_date_as_string (message));
-}
-
-static void
 format_headers_json (const void *ctx, GMimeMessage *message)
 {
     void *local = talloc_new (ctx);
@@ -731,31 +700,70 @@ format_part_mbox (const void *ctx, mime_node_t *node, unused (int indent),
     return NOTMUCH_STATUS_SUCCESS;
 }
 
-static void
-format_part_content_raw (GMimeObject *part)
+static notmuch_status_t
+format_part_raw (unused (const void *ctx), mime_node_t *node,
+		 unused (int indent),
+		 unused (const notmuch_show_params_t *params))
 {
-    if (! GMIME_IS_PART (part))
-	return;
+    if (node->envelope_file) {
+	const char *filename;
+	FILE *file;
+	size_t size;
+	char buf[4096];
 
-    GMimeStream *stream_stdout;
-    GMimeStream *stream_filter = NULL;
-    GMimeDataWrapper *wrapper;
+	filename = notmuch_message_get_filename (node->envelope_file);
+	if (filename == NULL) {
+	    fprintf (stderr, "Error: Cannot get message filename.\n");
+	    return NOTMUCH_STATUS_FILE_ERROR;
+	}
 
-    stream_stdout = g_mime_stream_file_new (stdout);
-    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
+	file = fopen (filename, "r");
+	if (file == NULL) {
+	    fprintf (stderr, "Error: Cannot open file %s: %s\n", filename, strerror (errno));
+	    return NOTMUCH_STATUS_FILE_ERROR;
+	}
 
-    stream_filter = g_mime_stream_filter_new (stream_stdout);
+	while (!feof (file)) {
+	    size = fread (buf, 1, sizeof (buf), file);
+	    if (ferror (file)) {
+		fprintf (stderr, "Error: Read failed from %s\n", filename);
+		fclose (file);
+		return NOTMUCH_STATUS_FILE_ERROR;
+	    }
 
-    wrapper = g_mime_part_get_content_object (GMIME_PART (part));
+	    if (fwrite (buf, size, 1, stdout) != 1) {
+		fprintf (stderr, "Error: Write failed\n");
+		fclose (file);
+		return NOTMUCH_STATUS_FILE_ERROR;
+	    }
+	}
 
-    if (wrapper && stream_filter)
-	g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
+	fclose (file);
+    } else if (GMIME_IS_PART (node->part)) {
+	GMimeStream *stream_stdout;
+	GMimeStream *stream_filter = NULL;
+	GMimeDataWrapper *wrapper;
 
-    if (stream_filter)
-	g_object_unref (stream_filter);
+	stream_stdout = g_mime_stream_file_new (stdout);
+	g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
 
-    if (stream_stdout)
-	g_object_unref(stream_stdout);
+	stream_filter = g_mime_stream_filter_new (stream_stdout);
+
+	wrapper = g_mime_part_get_content_object (GMIME_PART (node->part));
+
+	if (wrapper && stream_filter)
+	    g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
+
+	if (stream_filter)
+	    g_object_unref (stream_filter);
+
+	if (stream_stdout)
+	    g_object_unref(stream_stdout);
+    } else {
+	fprintf (stderr, "Error: Raw only supports root and leaf parts\n");
+	return NOTMUCH_STATUS_FILE_ERROR;
+    }
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 static notmuch_status_t
@@ -891,50 +899,7 @@ do_show_single (void *ctx,
 
     notmuch_message_set_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH, 1);
 
-    /* Special case for --format=raw of full single message, just cat out file */
-    if (params->raw && 0 == params->part) {
-
-	const char *filename;
-	FILE *file;
-	size_t size;
-	char buf[4096];
-
-	filename = notmuch_message_get_filename (message);
-	if (filename == NULL) {
-	    fprintf (stderr, "Error: Cannot message filename.\n");
-	    return 1;
-	}
-
-	file = fopen (filename, "r");
-	if (file == NULL) {
-	    fprintf (stderr, "Error: Cannot open file %s: %s\n", filename, strerror (errno));
-	    return 1;
-	}
-
-	while (!feof (file)) {
-	    size = fread (buf, 1, sizeof (buf), file);
-	    if (ferror (file)) {
-		fprintf (stderr, "Error: Read failed from %s\n", filename);
-		fclose (file);
-		return 1;
-	    }
-
-	    if (fwrite (buf, size, 1, stdout) != 1) {
-		fprintf (stderr, "Error: Write failed\n");
-		fclose (file);
-		return 1;
-	    }
-	}
-
-	fclose (file);
-
-	return 0;
-
-    } else {
-
-	return show_message (ctx, format, message, 0, params) != NOTMUCH_STATUS_SUCCESS;
-
-    }
+    return show_message (ctx, format, message, 0, params) != NOTMUCH_STATUS_SUCCESS;
 }
 
 /* Formatted output of threads */
diff --git a/test/multipart b/test/multipart
index a3036b4..cb16b43 100755
--- a/test/multipart
+++ b/test/multipart
@@ -448,59 +448,30 @@ notmuch show --format=raw --part=0 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUT
 test_expect_equal_file OUTPUT "${MAIL_DIR}"/multipart
 
 test_begin_subtest "--format=raw --part=1, message body"
-notmuch show --format=raw --part=1 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
-# output should *not* include newline
-echo >>OUTPUT
+notmuch show --format=raw --part=1 'id:87liy5ap00.fsf@yoom.home.cworth.org' >&OUTPUT
 cat <<EOF >EXPECTED
-Subject: html message
-From: Carl Worth <cworth@cworth.org>
-To: cworth@cworth.org
-Date: Fri, 05 Jan 2001 15:42:57 +0000
-
-<p>This is an embedded message, with a multipart/alternative part.</p>
-This is an embedded message, with a multipart/alternative part.
-This is a text attachment.
-And this message is signed.
-
--Carl
------BEGIN PGP SIGNATURE-----
-Version: GnuPG v1.4.11 (GNU/Linux)
-
-iEYEARECAAYFAk3SA/gACgkQ6JDdNq8qSWj0sACghqVJEQJUs3yV8zbTzhgnSIcD
-W6cAmQE4dcYrx/LPLtYLZm1jsGauE5hE
-=zkga
------END PGP SIGNATURE-----
+Error: Raw only supports root and leaf parts
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "--format=raw --part=2, multipart/mixed"
-notmuch show --format=raw --part=2 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
+notmuch show --format=raw --part=2 'id:87liy5ap00.fsf@yoom.home.cworth.org' >&OUTPUT
 cat <<EOF >EXPECTED
-Subject: html message
-From: Carl Worth <cworth@cworth.org>
-To: cworth@cworth.org
-Date: Fri, 05 Jan 2001 15:42:57 +0000
-
-<p>This is an embedded message, with a multipart/alternative part.</p>
-This is an embedded message, with a multipart/alternative part.
-This is a text attachment.
-And this message is signed.
-
--Carl
+Error: Raw only supports root and leaf parts
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "--format=raw --part=3, rfc822 part"
-test_subtest_known_broken
-
-notmuch show --format=raw --part=3 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
-test_expect_equal_file OUTPUT embedded_message
+notmuch show --format=raw --part=3 'id:87liy5ap00.fsf@yoom.home.cworth.org' >&OUTPUT
+cat <<EOF >EXPECTED
+Error: Raw only supports root and leaf parts
+EOF
+test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "--format=raw --part=4, rfc822's html part"
-notmuch show --format=raw --part=4 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
+notmuch show --format=raw --part=4 'id:87liy5ap00.fsf@yoom.home.cworth.org' >&OUTPUT
 cat <<EOF >EXPECTED
-<p>This is an embedded message, with a multipart/alternative part.</p>
-This is an embedded message, with a multipart/alternative part.
+Error: Raw only supports root and leaf parts
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
-- 
1.7.7.3

Re: [PATCH 0/5] Rewrite mbox and raw show formats

[Tomi Ollila]

On Sat,  3 Mar 2012 00:20:20 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> The show (rewrite) must go on.  These are the last two formats in
> notmuch show proper; the only remaining format is the reply format.
> 
> One of the ultimate goals of the show rewrite is to make raw actually
> work (and probably to split it into two different formats).  This
> series does not do that; it just ports the existing raw format.  I
> plan to make the new raw formats once the rewrite is done.

The whole patch series LGTM.

On somewhat related note: should there be NOTMUCH_STATUS_FAILURE (= 1)
in enum _notmuch_status (indicating unspecified failure). This would make
lines like
   return show_message (ctx, format, message, 0, params) != NOTMUCH_STATUS_SUCCESS;
look better (or were/are all of these failures due to memory exhaustion) ?

Tomi

Re: [PATCH 5/5] show: Convert raw format to the new self-recursive style

[Jameson Graef Rollins]

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

Hey, Austin.  As always, thank you so much for your hard work on this
rewrite.  It looks like things are definitely moving the right
direction.

I haven't done a full review of this patch set, and I've been pretty out
of the loop on this stuff recently, but I do notice that there are some
changes to the tests that don't look right to me.

On Sat,  3 Mar 2012 00:20:25 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This is fully compatible for root and leaf parts, but drops support
> for interior parts.  Showing interior parts in raw has always been
> braindead broken, so I don't think anyone will miss this.  Tests have
> been updated to reflect this.

I think I'm confused about this "drop support for interior parts".  What
constitutes an "interior part"?  Aren't all parts interior?  It looks
From the patch that maybe you're referring specifically to rfc822 parts?

I can understand not supporting output of multipart parts in raw; it's
unclear what a "raw" multipart even is.  But I think we do need to
support common-sense handling of rfc822 parts, even if it requires
special casing.  These following two test modifications illustrate the
issue:

>  test_begin_subtest "--format=raw --part=3, rfc822 part"
> -test_subtest_known_broken
> -
> -notmuch show --format=raw --part=3 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
> -test_expect_equal_file OUTPUT embedded_message
> +notmuch show --format=raw --part=3 'id:87liy5ap00.fsf@yoom.home.cworth.org' >&OUTPUT
> +cat <<EOF >EXPECTED
> +Error: Raw only supports root and leaf parts
> +EOF
> +test_expect_equal_file OUTPUT EXPECTED

I pretty strongly think that this test needs to remain how it was.  If
someone forwards me a message as a rfc822 part I should be able to
retrieve the full forwarded message directly, by e.g. redirecting it to
a file and recreating the original message exactly intact.  That's why I
constructed this test the way I did originally, and left it
known_broken.  If we can't support this now that's fine, but I still
think this test describes an important needed functionality that we
should strive to support at some point.  Maybe it needs an entirely new
output formatter, or some special casing, but I still think it's
reasonable to expect that we should support this.

>  test_begin_subtest "--format=raw --part=4, rfc822's html part"
> -notmuch show --format=raw --part=4 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
> +notmuch show --format=raw --part=4 'id:87liy5ap00.fsf@yoom.home.cworth.org' >&OUTPUT
>  cat <<EOF >EXPECTED
> -<p>This is an embedded message, with a multipart/alternative part.</p>
> -This is an embedded message, with a multipart/alternative part.
> +Error: Raw only supports root and leaf parts
>  EOF
>  test_expect_equal_file OUTPUT EXPECTED

Maybe this is ultimately a limitation of what we can expect the raw
formatter to do, but isn't this a leaf part?  As with whole rfc822
parts, I also expect that we should be able to retrieve interior leaf
parts of rfc822 message parts.  So I think I would prefer that this test
just move to test_subtest_known_broken until a better way to handle this
is figured out.

Without looking into the details of your whole show rewrite, I'm
guessing that we just can't reasonably expect the raw formatter to
handle rfc822 parts the way we need.  Is that correct?  Is there some
other formatter that might be better suited for this?  Like I say, I
think it's ok if we have to have some special casing for this particular
type of part.  But either way I think we should try to support handling
rfc822 parts in a useful way.

jamie.

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

Re: [PATCH 5/5] show: Convert raw format to the new self-recursive style

[Austin Clements]

TL;DR: I had a MIMEpiphany and I'm sending a new version of this
series that I think will address your concerns.

Quoth Jameson Graef Rollins on Mar 03 at  2:05 pm:
> Hey, Austin.  As always, thank you so much for your hard work on this
> rewrite.  It looks like things are definitely moving the right
> direction.
> 
> I haven't done a full review of this patch set, and I've been pretty out
> of the loop on this stuff recently, but I do notice that there are some
> changes to the tests that don't look right to me.
> 
> On Sat,  3 Mar 2012 00:20:25 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > This is fully compatible for root and leaf parts, but drops support
> > for interior parts.  Showing interior parts in raw has always been
> > braindead broken, so I don't think anyone will miss this.  Tests have
> > been updated to reflect this.
> 
> I think I'm confused about this "drop support for interior parts".  What
> constitutes an "interior part"?  Aren't all parts interior?  It looks
> From the patch that maybe you're referring specifically to rfc822 parts?

Interior parts are any part that isn't a root part (the whole message)
or a leaf part.

I had originally planned to simply deprecate raw in favor of two new
formats: "body", which outputted just bodies with transfer decoding
(which, for rfc822 parts, would include the attached message headers,
but not the envelope headers) and "source", which outputted full parts
with headers.  I had been thinking that "source" would be necessary
for fetching the entire message and "body" for fetching attachments
and other leaf parts, but then it dawned on me that even trivial
messages have two parts already: part 0 is an rfc822 part representing
the whole message and part 1 is the body.  Hence, modulo specialty
uses like crypto, "body" was already sufficient for both uses *and*
compatible with "raw" for root and leaf parts.

I've updated my series to reflect this.  Raw now works for any part.

> >  test_begin_subtest "--format=raw --part=4, rfc822's html part"
> > -notmuch show --format=raw --part=4 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
> > +notmuch show --format=raw --part=4 'id:87liy5ap00.fsf@yoom.home.cworth.org' >&OUTPUT
> >  cat <<EOF >EXPECTED
> > -<p>This is an embedded message, with a multipart/alternative part.</p>
> > -This is an embedded message, with a multipart/alternative part.
> > +Error: Raw only supports root and leaf parts
> >  EOF
> >  test_expect_equal_file OUTPUT EXPECTED
> 
> Maybe this is ultimately a limitation of what we can expect the raw
> formatter to do, but isn't this a leaf part?

Actually that was a typo in the test name.  Part 4 is a multipart, not
a leaf part.  The new series includes a patch for this.

[PATCH v2 0/8] Rewrite mbox and raw show formats

[Austin Clements]

v2 makes the new raw format support interior, as well as root and leaf
parts.

Patches 1, 2, and 8 are new but simple.  Patches 3 through 6 have not
changed.  Patch 7 has obviously changed quite a bit.

[PATCH v2 1/8] test: Fix typo in test description

[Austin Clements]

Part 4 is a multipart, not an html part.
---
 test/multipart |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/multipart b/test/multipart
index a3036b4..d36a65d 100755
--- a/test/multipart
+++ b/test/multipart
@@ -496,7 +496,7 @@ test_subtest_known_broken
 notmuch show --format=raw --part=3 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
 test_expect_equal_file OUTPUT embedded_message
 
-test_begin_subtest "--format=raw --part=4, rfc822's html part"
+test_begin_subtest "--format=raw --part=4, rfc822's multipart"
 notmuch show --format=raw --part=4 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
 cat <<EOF >EXPECTED
 <p>This is an embedded message, with a multipart/alternative part.</p>
-- 
1.7.7.3

[PATCH v2 2/8] test: Fix malformed multipart message

[Austin Clements]

Previously, there was only one CRLF between the terminating boundary
of the embedded multipart/alternative and the boundary of the
containing multipart.  However, according the RFC 1341, 7.2.1:

  The boundary must be followed immediately either by another CRLF and
  the header fields for the next part, or by two CRLFs, in which case
  there are no header fields for the next part

and

  The CRLF preceding the encapsulation line is considered part of the
  boundary so that it is possible to have a part that does not end
  with a CRLF (line break).

Thus, there must be *two* CRLFs between these boundaries: one that
ends the terminating boundary and one that begins the enclosing
boundary.

While GMime accepted the message we had before, it could not produce
such a message.
---
 test/multipart |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/test/multipart b/test/multipart
index d36a65d..475c6a2 100755
--- a/test/multipart
+++ b/test/multipart
@@ -46,6 +46,7 @@ Content-Disposition: inline
 EOF
 cat embedded_message >> ${MAIL_DIR}/multipart
 cat <<EOF >> ${MAIL_DIR}/multipart
+
 --=-=-=
 Content-Disposition: attachment; filename=attachment
 
-- 
1.7.7.3

[PATCH v2 3/8] show: Allow formatters to return errors

[Austin Clements]

Formatter errors are propagated to the exit status of notmuch show.

This isn't used by the JSON or text formatters, but it will be useful
for the raw format, which is pickier.
---
 notmuch-client.h |    6 ++--
 notmuch-show.c   |   71 +++++++++++++++++++++++++++++++++++-------------------
 2 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index f4a62cc..a220fe4 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -67,9 +67,9 @@ struct notmuch_show_params;
 
 typedef struct notmuch_show_format {
     const char *message_set_start;
-    void (*part) (const void *ctx,
-		  struct mime_node *node, int indent,
-		  const struct notmuch_show_params *params);
+    notmuch_status_t (*part) (const void *ctx,
+			      struct mime_node *node, int indent,
+			      const struct notmuch_show_params *params);
     const char *message_start;
     void (*message) (const void *ctx,
 		     notmuch_message_t *message,
diff --git a/notmuch-show.c b/notmuch-show.c
index 6a171a4..648f468 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -23,7 +23,7 @@
 static void
 format_headers_message_part_text (GMimeMessage *message);
 
-static void
+static notmuch_status_t
 format_part_text (const void *ctx, mime_node_t *node,
 		  int indent, const notmuch_show_params_t *params);
 
@@ -34,7 +34,7 @@ static const notmuch_show_format_t format_text = {
     .message_set_end = ""
 };
 
-static void
+static notmuch_status_t
 format_part_json_entry (const void *ctx, mime_node_t *node,
 			int indent, const notmuch_show_params_t *params);
 
@@ -562,7 +562,7 @@ format_part_content_raw (GMimeObject *part)
 	g_object_unref(stream_stdout);
 }
 
-static void
+static notmuch_status_t
 format_part_text (const void *ctx, mime_node_t *node,
 		  int indent, const notmuch_show_params_t *params)
 {
@@ -650,6 +650,8 @@ format_part_text (const void *ctx, mime_node_t *node,
 	printf ("\fbody}\n");
 
     printf ("\f%s}\n", part_type);
+
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 static void
@@ -751,14 +753,16 @@ format_part_json (const void *ctx, mime_node_t *node, notmuch_bool_t first)
     printf ("%s}", terminator);
 }
 
-static void
+static notmuch_status_t
 format_part_json_entry (const void *ctx, mime_node_t *node, unused (int indent),
 			unused (const notmuch_show_params_t *params))
 {
     format_part_json (ctx, node, TRUE);
+
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
-static void
+static notmuch_status_t
 show_message (void *ctx,
 	      const notmuch_show_format_t *format,
 	      notmuch_message_t *message,
@@ -768,14 +772,18 @@ show_message (void *ctx,
     if (format->part) {
 	void *local = talloc_new (ctx);
 	mime_node_t *root, *part;
-
-	if (mime_node_open (local, message, params->cryptoctx, params->decrypt,
-			    &root) == NOTMUCH_STATUS_SUCCESS &&
-	    (part = mime_node_seek_dfs (root, (params->part < 0 ?
-					       0 : params->part))))
-	    format->part (local, part, indent, params);
+	notmuch_status_t status;
+
+	status = mime_node_open (local, message, params->cryptoctx,
+				 params->decrypt, &root);
+	if (status)
+	    goto DONE;
+	part = mime_node_seek_dfs (root, (params->part < 0 ? 0 : params->part));
+	if (part)
+	    status = format->part (local, part, indent, params);
+      DONE:
 	talloc_free (local);
-	return;
+	return status;
     }
 
     if (params->part <= 0) {
@@ -799,9 +807,11 @@ show_message (void *ctx,
 
 	fputs (format->message_end, stdout);
     }
+
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
-static void
+static notmuch_status_t
 show_messages (void *ctx,
 	       const notmuch_show_format_t *format,
 	       notmuch_messages_t *messages,
@@ -812,6 +822,7 @@ show_messages (void *ctx,
     notmuch_bool_t match;
     int first_set = 1;
     int next_indent;
+    notmuch_status_t status, res = NOTMUCH_STATUS_SUCCESS;
 
     fputs (format->message_set_start, stdout);
 
@@ -832,17 +843,22 @@ show_messages (void *ctx,
 	next_indent = indent;
 
 	if (match || params->entire_thread) {
-	    show_message (ctx, format, message, indent, params);
+	    status = show_message (ctx, format, message, indent, params);
+	    if (status && !res)
+		res = status;
 	    next_indent = indent + 1;
 
-	    fputs (format->message_set_sep, stdout);
+	    if (!status)
+		fputs (format->message_set_sep, stdout);
 	}
 
-	show_messages (ctx,
-		       format,
-		       notmuch_message_get_replies (message),
-		       next_indent,
-		       params);
+	status = show_messages (ctx,
+				format,
+				notmuch_message_get_replies (message),
+				next_indent,
+				params);
+	if (status && !res)
+	    res = status;
 
 	notmuch_message_destroy (message);
 
@@ -850,6 +866,8 @@ show_messages (void *ctx,
     }
 
     fputs (format->message_set_end, stdout);
+
+    return res;
 }
 
 /* Formatted output of single message */
@@ -914,13 +932,13 @@ do_show_single (void *ctx,
 
 	fclose (file);
 
+	return 0;
+
     } else {
 
-	show_message (ctx, format, message, 0, params);
+	return show_message (ctx, format, message, 0, params) != NOTMUCH_STATUS_SUCCESS;
 
     }
-
-    return 0;
 }
 
 /* Formatted output of threads */
@@ -934,6 +952,7 @@ do_show (void *ctx,
     notmuch_thread_t *thread;
     notmuch_messages_t *messages;
     int first_toplevel = 1;
+    notmuch_status_t status, res = NOTMUCH_STATUS_SUCCESS;
 
     fputs (format->message_set_start, stdout);
 
@@ -953,7 +972,9 @@ do_show (void *ctx,
 	    fputs (format->message_set_sep, stdout);
 	first_toplevel = 0;
 
-	show_messages (ctx, format, messages, 0, params);
+	status = show_messages (ctx, format, messages, 0, params);
+	if (status && !res)
+	    res = status;
 
 	notmuch_thread_destroy (thread);
 
@@ -961,7 +982,7 @@ do_show (void *ctx,
 
     fputs (format->message_set_end, stdout);
 
-    return 0;
+    return res != NOTMUCH_STATUS_SUCCESS;
 }
 
 enum {
-- 
1.7.7.3

[PATCH v2 4/8] show: Move format_message_mbox with the other new-style formats

[Austin Clements]

Just code motion.
---
 notmuch-show.c |  100 ++++++++++++++++++++++++++++----------------------------
 1 files changed, 50 insertions(+), 50 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 648f468..b3a4550 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -219,56 +219,6 @@ _is_from_line (const char *line)
 	return 0;
 }
 
-/* Print a message in "mboxrd" format as documented, for example,
- * here:
- *
- * http://qmail.org/qmail-manual-html/man5/mbox.html
- */
-static void
-format_message_mbox (const void *ctx,
-		     notmuch_message_t *message,
-		     unused (int indent))
-{
-    const char *filename;
-    FILE *file;
-    const char *from;
-
-    time_t date;
-    struct tm date_gmtime;
-    char date_asctime[26];
-
-    char *line = NULL;
-    size_t line_size;
-    ssize_t line_len;
-
-    filename = notmuch_message_get_filename (message);
-    file = fopen (filename, "r");
-    if (file == NULL) {
-	fprintf (stderr, "Failed to open %s: %s\n",
-		 filename, strerror (errno));
-	return;
-    }
-
-    from = notmuch_message_get_header (message, "from");
-    from = _extract_email_address (ctx, from);
-
-    date = notmuch_message_get_date (message);
-    gmtime_r (&date, &date_gmtime);
-    asctime_r (&date_gmtime, date_asctime);
-
-    printf ("From %s %s", from, date_asctime);
-
-    while ((line_len = getline (&line, &line_size, file)) != -1 ) {
-	if (_is_from_line (line))
-	    putchar ('>');
-	printf ("%s", line);
-    }
-
-    printf ("\n");
-
-    fclose (file);
-}
-
 static void
 format_headers_message_part_text (GMimeMessage *message)
 {
@@ -762,6 +712,56 @@ format_part_json_entry (const void *ctx, mime_node_t *node, unused (int indent),
     return NOTMUCH_STATUS_SUCCESS;
 }
 
+/* Print a message in "mboxrd" format as documented, for example,
+ * here:
+ *
+ * http://qmail.org/qmail-manual-html/man5/mbox.html
+ */
+static void
+format_message_mbox (const void *ctx,
+		     notmuch_message_t *message,
+		     unused (int indent))
+{
+    const char *filename;
+    FILE *file;
+    const char *from;
+
+    time_t date;
+    struct tm date_gmtime;
+    char date_asctime[26];
+
+    char *line = NULL;
+    size_t line_size;
+    ssize_t line_len;
+
+    filename = notmuch_message_get_filename (message);
+    file = fopen (filename, "r");
+    if (file == NULL) {
+	fprintf (stderr, "Failed to open %s: %s\n",
+		 filename, strerror (errno));
+	return;
+    }
+
+    from = notmuch_message_get_header (message, "from");
+    from = _extract_email_address (ctx, from);
+
+    date = notmuch_message_get_date (message);
+    gmtime_r (&date, &date_gmtime);
+    asctime_r (&date_gmtime, date_asctime);
+
+    printf ("From %s %s", from, date_asctime);
+
+    while ((line_len = getline (&line, &line_size, file)) != -1 ) {
+	if (_is_from_line (line))
+	    putchar ('>');
+	printf ("%s", line);
+    }
+
+    printf ("\n");
+
+    fclose (file);
+}
+
 static notmuch_status_t
 show_message (void *ctx,
 	      const notmuch_show_format_t *format,
-- 
1.7.7.3

[PATCH v2 5/8] show: Convert mbox format to new self-recursive style

[Austin Clements]

Given the lack of recursion, this is pretty easy.
---
 notmuch-show.c |   40 ++++++++++++++++++----------------------
 1 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index b3a4550..72c141e 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -45,25 +45,15 @@ static const notmuch_show_format_t format_json = {
     .message_set_end = "]"
 };
 
-static void
-format_message_mbox (const void *ctx,
-		     notmuch_message_t *message,
-		     unused (int indent));
+static notmuch_status_t
+format_part_mbox (const void *ctx, mime_node_t *node,
+		  int indent, const notmuch_show_params_t *params);
 
 static const notmuch_show_format_t format_mbox = {
-    "", NULL,
-        "", format_message_mbox,
-            "", NULL, NULL, "",
-            "",
-                NULL,
-                NULL,
-                NULL,
-                NULL,
-                NULL,
-                "",
-            "",
-        "", "",
-    ""
+    .message_set_start = "",
+    .part = format_part_mbox,
+    .message_set_sep = "",
+    .message_set_end = ""
 };
 
 static void
@@ -717,11 +707,12 @@ format_part_json_entry (const void *ctx, mime_node_t *node, unused (int indent),
  *
  * http://qmail.org/qmail-manual-html/man5/mbox.html
  */
-static void
-format_message_mbox (const void *ctx,
-		     notmuch_message_t *message,
-		     unused (int indent))
+static notmuch_status_t
+format_part_mbox (const void *ctx, mime_node_t *node, unused (int indent),
+		  unused (const notmuch_show_params_t *params))
 {
+    notmuch_message_t *message = node->envelope_file;
+
     const char *filename;
     FILE *file;
     const char *from;
@@ -734,12 +725,15 @@ format_message_mbox (const void *ctx,
     size_t line_size;
     ssize_t line_len;
 
+    if (!message)
+	INTERNAL_ERROR ("format_part_mbox requires a root part");
+
     filename = notmuch_message_get_filename (message);
     file = fopen (filename, "r");
     if (file == NULL) {
 	fprintf (stderr, "Failed to open %s: %s\n",
 		 filename, strerror (errno));
-	return;
+	return NOTMUCH_STATUS_FILE_ERROR;
     }
 
     from = notmuch_message_get_header (message, "from");
@@ -760,6 +754,8 @@ format_message_mbox (const void *ctx,
     printf ("\n");
 
     fclose (file);
+
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 static notmuch_status_t
-- 
1.7.7.3

[PATCH v2 6/8] show: Move format_part_content_raw with the other new-style formats

[Austin Clements]

Just code motion.
---
 notmuch-show.c |   54 +++++++++++++++++++++++++++---------------------------
 1 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 72c141e..6f6052c 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -475,33 +475,6 @@ format_part_sigstatus_json (mime_node_t *node)
 }
 #endif
 
-static void
-format_part_content_raw (GMimeObject *part)
-{
-    if (! GMIME_IS_PART (part))
-	return;
-
-    GMimeStream *stream_stdout;
-    GMimeStream *stream_filter = NULL;
-    GMimeDataWrapper *wrapper;
-
-    stream_stdout = g_mime_stream_file_new (stdout);
-    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
-
-    stream_filter = g_mime_stream_filter_new (stream_stdout);
-
-    wrapper = g_mime_part_get_content_object (GMIME_PART (part));
-
-    if (wrapper && stream_filter)
-	g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
-
-    if (stream_filter)
-	g_object_unref (stream_filter);
-
-    if (stream_stdout)
-	g_object_unref(stream_stdout);
-}
-
 static notmuch_status_t
 format_part_text (const void *ctx, mime_node_t *node,
 		  int indent, const notmuch_show_params_t *params)
@@ -758,6 +731,33 @@ format_part_mbox (const void *ctx, mime_node_t *node, unused (int indent),
     return NOTMUCH_STATUS_SUCCESS;
 }
 
+static void
+format_part_content_raw (GMimeObject *part)
+{
+    if (! GMIME_IS_PART (part))
+	return;
+
+    GMimeStream *stream_stdout;
+    GMimeStream *stream_filter = NULL;
+    GMimeDataWrapper *wrapper;
+
+    stream_stdout = g_mime_stream_file_new (stdout);
+    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
+
+    stream_filter = g_mime_stream_filter_new (stream_stdout);
+
+    wrapper = g_mime_part_get_content_object (GMIME_PART (part));
+
+    if (wrapper && stream_filter)
+	g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
+
+    if (stream_filter)
+	g_object_unref (stream_filter);
+
+    if (stream_stdout)
+	g_object_unref(stream_stdout);
+}
+
 static notmuch_status_t
 show_message (void *ctx,
 	      const notmuch_show_format_t *format,
-- 
1.7.7.3

[PATCH v2 7/8] show: Convert raw format to the new self-recursive style, properly support interior parts

[Austin Clements]

This is fully compatible for root and leaf parts, but now has proper
support for interior parts.  This requires some design decisions that
were guided by what I would want if I were to save a part.
Specifically:

- Leaf parts are printed without headers and with transfer decoding.
  This is what makes sense for saving attachments.  (Furthermore, the
  transfer decoding is necessary since, without the headers, the
  caller would not be able to interpret non-transfer-decoded output.)

- Message parts are printed with their message headers, but without
  enclosing part headers.  This is what makes sense for saving a
  message as a whole (which is a message part) and for saving attached
  messages.  This is symmetric for whole messages and for attached
  messages, though we special-case the whole message for performance
  reasons (and corner-case correctness reasons: given malformed input,
  GMime may not be able to reproduce it from the parsed
  representation).

- Multipart parts are printed with their headers and all child parts.
  It's not clear what the best thing to do for multipart is, but this
  was the most natural to implement and can be justified because such
  parts can't be interpreted without their headers.

As an added benefit, we can move the special-case code for part 0 into
the raw formatter.
---
 notmuch-show.c |  159 ++++++++++++++++++++++++--------------------------------
 test/multipart |   74 +++++++++++++++++---------
 2 files changed, 116 insertions(+), 117 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 6f6052c..8ed6896 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -20,9 +20,6 @@
 
 #include "notmuch-client.h"
 
-static void
-format_headers_message_part_text (GMimeMessage *message);
-
 static notmuch_status_t
 format_part_text (const void *ctx, mime_node_t *node,
 		  int indent, const notmuch_show_params_t *params);
@@ -56,23 +53,16 @@ static const notmuch_show_format_t format_mbox = {
     .message_set_end = ""
 };
 
-static void
-format_part_content_raw (GMimeObject *part);
+static notmuch_status_t
+format_part_raw (unused (const void *ctx), mime_node_t *node,
+		 unused (int indent),
+		 unused (const notmuch_show_params_t *params));
 
 static const notmuch_show_format_t format_raw = {
-    "", NULL,
-	"", NULL,
-	    "", NULL, format_headers_message_part_text, "\n",
-            "",
-                NULL,
-                NULL,
-                NULL,
-                format_part_content_raw,
-                NULL,
-                "",
-            "",
-	"", "",
-    ""
+    .message_set_start = "",
+    .part = format_part_raw,
+    .message_set_sep = "",
+    .message_set_end = ""
 };
 
 static const char *
@@ -210,27 +200,6 @@ _is_from_line (const char *line)
 }
 
 static void
-format_headers_message_part_text (GMimeMessage *message)
-{
-    InternetAddressList *recipients;
-    const char *recipients_string;
-
-    printf ("Subject: %s\n", g_mime_message_get_subject (message));
-    printf ("From: %s\n", g_mime_message_get_sender (message));
-    recipients = g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_TO);
-    recipients_string = internet_address_list_to_string (recipients, 0);
-    if (recipients_string)
-	printf ("To: %s\n",
-		recipients_string);
-    recipients = g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_CC);
-    recipients_string = internet_address_list_to_string (recipients, 0);
-    if (recipients_string)
-	printf ("Cc: %s\n",
-		recipients_string);
-    printf ("Date: %s\n", g_mime_message_get_date_as_string (message));
-}
-
-static void
 format_headers_json (const void *ctx, GMimeMessage *message)
 {
     void *local = talloc_new (ctx);
@@ -731,31 +700,82 @@ format_part_mbox (const void *ctx, mime_node_t *node, unused (int indent),
     return NOTMUCH_STATUS_SUCCESS;
 }
 
-static void
-format_part_content_raw (GMimeObject *part)
+static notmuch_status_t
+format_part_raw (unused (const void *ctx), mime_node_t *node,
+		 unused (int indent),
+		 unused (const notmuch_show_params_t *params))
 {
-    if (! GMIME_IS_PART (part))
-	return;
+    if (node->envelope_file) {
+	/* Special case the entire message to avoid MIME parsing. */
+	const char *filename;
+	FILE *file;
+	size_t size;
+	char buf[4096];
+
+	filename = notmuch_message_get_filename (node->envelope_file);
+	if (filename == NULL) {
+	    fprintf (stderr, "Error: Cannot get message filename.\n");
+	    return NOTMUCH_STATUS_FILE_ERROR;
+	}
+
+	file = fopen (filename, "r");
+	if (file == NULL) {
+	    fprintf (stderr, "Error: Cannot open file %s: %s\n", filename, strerror (errno));
+	    return NOTMUCH_STATUS_FILE_ERROR;
+	}
+
+	while (!feof (file)) {
+	    size = fread (buf, 1, sizeof (buf), file);
+	    if (ferror (file)) {
+		fprintf (stderr, "Error: Read failed from %s\n", filename);
+		fclose (file);
+		return NOTMUCH_STATUS_FILE_ERROR;
+	    }
+
+	    if (fwrite (buf, size, 1, stdout) != 1) {
+		fprintf (stderr, "Error: Write failed\n");
+		fclose (file);
+		return NOTMUCH_STATUS_FILE_ERROR;
+	    }
+	}
+
+	fclose (file);
+	return NOTMUCH_STATUS_SUCCESS;
+    }
 
     GMimeStream *stream_stdout;
     GMimeStream *stream_filter = NULL;
-    GMimeDataWrapper *wrapper;
 
     stream_stdout = g_mime_stream_file_new (stdout);
     g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
 
     stream_filter = g_mime_stream_filter_new (stream_stdout);
 
-    wrapper = g_mime_part_get_content_object (GMIME_PART (part));
+    if (GMIME_IS_PART (node->part)) {
+	/* For leaf parts, we emit only the transfer-decoded
+	 * body. */
+	GMimeDataWrapper *wrapper;
+	wrapper = g_mime_part_get_content_object (GMIME_PART (node->part));
 
-    if (wrapper && stream_filter)
-	g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
+	if (wrapper && stream_filter)
+	    g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
+    } else {
+	/* Write out the whole part.  For message parts (the root
+	 * part and embedded message parts), this will be the
+	 * message including its headers (but not the
+	 * encapsulating part's headers).  For multipart parts,
+	 * this will include the headers. */
+	if (stream_filter)
+	    g_mime_object_write_to_stream (node->part, stream_filter);
+    }
 
     if (stream_filter)
 	g_object_unref (stream_filter);
 
     if (stream_stdout)
 	g_object_unref(stream_stdout);
+
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 static notmuch_status_t
@@ -891,50 +911,7 @@ do_show_single (void *ctx,
 
     notmuch_message_set_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH, 1);
 
-    /* Special case for --format=raw of full single message, just cat out file */
-    if (params->raw && 0 == params->part) {
-
-	const char *filename;
-	FILE *file;
-	size_t size;
-	char buf[4096];
-
-	filename = notmuch_message_get_filename (message);
-	if (filename == NULL) {
-	    fprintf (stderr, "Error: Cannot message filename.\n");
-	    return 1;
-	}
-
-	file = fopen (filename, "r");
-	if (file == NULL) {
-	    fprintf (stderr, "Error: Cannot open file %s: %s\n", filename, strerror (errno));
-	    return 1;
-	}
-
-	while (!feof (file)) {
-	    size = fread (buf, 1, sizeof (buf), file);
-	    if (ferror (file)) {
-		fprintf (stderr, "Error: Read failed from %s\n", filename);
-		fclose (file);
-		return 1;
-	    }
-
-	    if (fwrite (buf, size, 1, stdout) != 1) {
-		fprintf (stderr, "Error: Write failed\n");
-		fclose (file);
-		return 1;
-	    }
-	}
-
-	fclose (file);
-
-	return 0;
-
-    } else {
-
-	return show_message (ctx, format, message, 0, params) != NOTMUCH_STATUS_SUCCESS;
-
-    }
+    return show_message (ctx, format, message, 0, params) != NOTMUCH_STATUS_SUCCESS;
 }
 
 /* Formatted output of threads */
diff --git a/test/multipart b/test/multipart
index 475c6a2..c1a5702 100755
--- a/test/multipart
+++ b/test/multipart
@@ -450,58 +450,80 @@ test_expect_equal_file OUTPUT "${MAIL_DIR}"/multipart
 
 test_begin_subtest "--format=raw --part=1, message body"
 notmuch show --format=raw --part=1 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
-# output should *not* include newline
-echo >>OUTPUT
-cat <<EOF >EXPECTED
-Subject: html message
-From: Carl Worth <cworth@cworth.org>
-To: cworth@cworth.org
-Date: Fri, 05 Jan 2001 15:42:57 +0000
-
-<p>This is an embedded message, with a multipart/alternative part.</p>
-This is an embedded message, with a multipart/alternative part.
-This is a text attachment.
-And this message is signed.
-
--Carl
------BEGIN PGP SIGNATURE-----
-Version: GnuPG v1.4.11 (GNU/Linux)
-
-iEYEARECAAYFAk3SA/gACgkQ6JDdNq8qSWj0sACghqVJEQJUs3yV8zbTzhgnSIcD
-W6cAmQE4dcYrx/LPLtYLZm1jsGauE5hE
-=zkga
------END PGP SIGNATURE-----
-EOF
-test_expect_equal_file OUTPUT EXPECTED
+test_expect_equal_file OUTPUT "${MAIL_DIR}"/multipart
 
 test_begin_subtest "--format=raw --part=2, multipart/mixed"
 notmuch show --format=raw --part=2 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
 cat <<EOF >EXPECTED
-Subject: html message
+Content-Type: multipart/mixed; boundary="=-=-="
+
+--=-=-=
+Content-Type: message/rfc822
+Content-Disposition: inline
+
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
+Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
+User-Agent: Notmuch/0.5 (http://notmuchmail.org) Emacs/23.3.1 (i486-pc-linux-gnu)
+Message-ID: <87liy5ap01.fsf@yoom.home.cworth.org>
+MIME-Version: 1.0
+Content-Type: multipart/alternative; boundary="==-=-=="
+
+--==-=-==
+Content-Type: text/html
 
 <p>This is an embedded message, with a multipart/alternative part.</p>
+
+--==-=-==
+Content-Type: text/plain
+
 This is an embedded message, with a multipart/alternative part.
+
+--==-=-==--
+
+--=-=-=
+Content-Disposition: attachment; filename=attachment
+
 This is a text attachment.
+
+--=-=-=
+
 And this message is signed.
 
 -Carl
+
+--=-=-=--
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "--format=raw --part=3, rfc822 part"
-test_subtest_known_broken
-
 notmuch show --format=raw --part=3 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
 test_expect_equal_file OUTPUT embedded_message
 
 test_begin_subtest "--format=raw --part=4, rfc822's multipart"
 notmuch show --format=raw --part=4 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
 cat <<EOF >EXPECTED
+From: Carl Worth <cworth@cworth.org>
+To: cworth@cworth.org
+Subject: html message
+Date: Fri, 05 Jan 2001 15:42:57 +0000
+User-Agent: Notmuch/0.5 (http://notmuchmail.org) Emacs/23.3.1 (i486-pc-linux-gnu)
+Message-ID: <87liy5ap01.fsf@yoom.home.cworth.org>
+MIME-Version: 1.0
+Content-Type: multipart/alternative; boundary="==-=-=="
+
+--==-=-==
+Content-Type: text/html
+
 <p>This is an embedded message, with a multipart/alternative part.</p>
+
+--==-=-==
+Content-Type: text/plain
+
 This is an embedded message, with a multipart/alternative part.
+
+--==-=-==--
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
-- 
1.7.7.3

[PATCH v2 8/8] man: Update raw format documentation

[Austin Clements]

---
 man/man1/notmuch-show.1 |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/man/man1/notmuch-show.1 b/man/man1/notmuch-show.1
index 4c5db94..9506367 100644
--- a/man/man1/notmuch-show.1
+++ b/man/man1/notmuch-show.1
@@ -84,12 +84,17 @@ http://homepage.ntlworld.com/jonathan.deboynepollard/FGA/mail-mbox-formats.html
 .TP 4
 .BR raw " (default for a single part, see \-\-part)"
 
-For a message, the original, raw content of the email message is
-output. Consumers of this format should expect to implement MIME
-decoding and similar functions.
+For a message or an attached message part, the original, raw content
+of the email message is output. Consumers of this format should expect
+to implement MIME decoding and similar functions.
 
 For a single part (\-\-part) the raw part content is output after
-performing any necessary MIME decoding.
+performing any necessary MIME decoding.  Note that messages with a
+simple body still have two parts: part 0 is the whole message and part
+1 is the body.
+
+For a multipart part, the part headers and body (including all child
+parts) is output.
 
 The raw format must only be used with search terms matching single
 message.
-- 
1.7.7.3

Re: [PATCH v2 3/8] show: Allow formatters to return errors

[Mark Walters]

On Tue,  6 Mar 2012 18:48:39 +0000, Austin Clements <amdragon@MIT.EDU> wrote:
> Formatter errors are propagated to the exit status of notmuch show.
> 
> This isn't used by the JSON or text formatters, but it will be useful
> for the raw format, which is pickier.

I am not very familiar with this part of the code but the whole series
looks fine to me.

My only minor comment is that I like Tom's suggestion (in
id:"m2399qrtat.fsf@guru.guru-group.fi") of having NOTMUCH_STATUS_FAILURE
(= 1) to make the error handling look cleaner. Alternatively (or
possibly as well) the functions do_show and do_show_messages could pass
the actual error back up to notmuch_show_command and then that function
could convert return 0/1 as appropriate.

As usual I am quite happy to be overruled!

Best wishes

Mark

Re: [PATCH v2 3/8] show: Allow formatters to return errors

[Tomi Ollila]

On Tue, 06 Mar 2012 21:22:55 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> On Tue,  6 Mar 2012 18:48:39 +0000, Austin Clements <amdragon@MIT.EDU> wrote:
> > Formatter errors are propagated to the exit status of notmuch show.
> > 
> > This isn't used by the JSON or text formatters, but it will be useful
> > for the raw format, which is pickier.
> 
> I am not very familiar with this part of the code but the whole series
> looks fine to me.

Looks good to me too.

> 
> My only minor comment is that I like Tom's suggestion (in
> id:"m2399qrtat.fsf@guru.guru-group.fi") of having NOTMUCH_STATUS_FAILURE
> (= 1) to make the error handling look cleaner. Alternatively (or
> possibly as well) the functions do_show and do_show_messages could pass
> the actual error back up to notmuch_show_command and then that function
> could convert return 0/1 as appropriate.
> 
> As usual I am quite happy to be overruled!

As usual I no longer know why I suggested that in the first place
(at least in the context of these patches ;D)

> Best wishes
> 
> Mark

Tomi

Re: [PATCH v2 0/8] Rewrite mbox and raw show formats

[Adam Wolfe Gordon]

On Tue, Mar 6, 2012 at 11:48, Austin Clements <amdragon@mit.edu> wrote:
> v2 makes the new raw format support interior, as well as root and leaf
> parts.
>
> Patches 1, 2, and 8 are new but simple.  Patches 3 through 6 have not
> changed.  Patch 7 has obviously changed quite a bit.

The whole series looks good to me.

Re: [PATCH v2 3/8] show: Allow formatters to return errors

[Austin Clements]

On Tue, 06 Mar 2012 21:22:55 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> On Tue,  6 Mar 2012 18:48:39 +0000, Austin Clements <amdragon@MIT.EDU> wrote:
> > Formatter errors are propagated to the exit status of notmuch show.
> > 
> > This isn't used by the JSON or text formatters, but it will be useful
> > for the raw format, which is pickier.
> 
> I am not very familiar with this part of the code but the whole series
> looks fine to me.
> 
> My only minor comment is that I like Tom's suggestion (in
> id:"m2399qrtat.fsf@guru.guru-group.fi") of having NOTMUCH_STATUS_FAILURE
> (= 1) to make the error handling look cleaner. Alternatively (or

We could introduce a generic failure, though it couldn't be equal to 1
without breaking binary compatibility.  I'm not sure a generic failure
would make things much better, though; as far as I can tell, error
handling in C is doomed to ugliness.

> possibly as well) the functions do_show and do_show_messages could pass
> the actual error back up to notmuch_show_command and then that function
> could convert return 0/1 as appropriate.

I would be happy to be convinced either way.  It's the way it is because
I didn't bother changing it and because I think it's reasonable to think
of do_show and do_show_messages as extensions of notmuch_show_command.

> As usual I am quite happy to be overruled!
> 
> Best wishes
> 
> Mark
> 

Re: [PATCH v2 0/8] Rewrite mbox and raw show formats

[David Bremner]

On Tue,  6 Mar 2012 18:48:36 +0000, Austin Clements <amdragon@MIT.EDU> wrote:
> v2 makes the new raw format support interior, as well as root and leaf
> parts.
> 
> Patches 1, 2, and 8 are new but simple.  Patches 3 through 6 have not
> changed.  Patch 7 has obviously changed quite a bit.

Pushed, 

d