[PATCH v5.2 0/7] Reply enhancements, tidied up

[PATCH v5.2 0/7] Reply enhancements, tidied up

[Adam Wolfe Gordon]

Hi all,

As indicated by the odd version number, this is a very minor change to
the patchset I sent last night [1]:

* Split the refactoring into separate patches, both for the reply patch
  and the emacs patch. The first reply patch should now be applicable
  by itself, as requested by Austin to ease his work on rewriting the
  default reply formatter.

* Fix a bug in my emacs code that caused replies to be created with two
  From headers, owing to capitalization descrepencies.

* Split the emacs tests into their own patch, since they're pretty big.

[1] id:"1329282027-29457-1-git-send-email-awg+notmuch@xvx.ca"

Adam Wolfe Gordon (7):
  test: Add broken test for the new JSON reply format.
  reply: Factor out reply creation
  reply: Add a JSON reply format.
  man: Update notmuch-reply man page for JSON format.
  emacs: Factor out useful functions into notmuch-lib
  test: Add broken tests for new emacs reply functionality
  emacs: Use the new JSON reply format and message-cite-original

 emacs/notmuch-lib.el     |   39 +++++++++
 emacs/notmuch-mua.el     |  127 ++++++++++++++++++++---------
 emacs/notmuch-show.el    |   24 +-----
 man/man1/notmuch-reply.1 |    5 +
 notmuch-client.h         |    3 +
 notmuch-reply.c          |  202 +++++++++++++++++++++++++++++++++++-----------
 notmuch-show.c           |    2 +-
 test/emacs               |  101 ++++++++++++++++++++++-
 test/multipart           |   51 ++++++++++++
 9 files changed, 443 insertions(+), 111 deletions(-)

-- 
1.7.5.4

[PATCH v5.2 1/7] test: Add broken test for the new JSON reply format.

[Adam Wolfe Gordon]

---
 test/multipart |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/test/multipart b/test/multipart
index a3036b4..e7abcc2 100755
--- a/test/multipart
+++ b/test/multipart
@@ -589,6 +589,57 @@ Non-text part: text/html
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
+test_begin_subtest "'notmuch reply' to a multipart message with json format"
+notmuch reply --format=json 'id:87liy5ap00.fsf@yoom.home.cworth.org' | notmuch_json_show_sanitize >OUTPUT
+cat <<EOF >EXPECTED
+{"reply-headers": {"Subject": "Re: Multipart message",
+ "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
+ "To": "Carl Worth <cworth@cworth.org>,
+ cworth@cworth.org",
+ "In-reply-to": "<87liy5ap00.fsf@yoom.home.cworth.org>",
+ "References": " <87liy5ap00.fsf@yoom.home.cworth.org>"},
+ "original": {"id": "XXXXX",
+ "match": false,
+ "filename": "YYYYY",
+ "timestamp": 978709437,
+ "date_relative": "2001-01-05",
+ "tags": ["attachment","inbox","signed","unread"],
+ "headers": {"Subject": "Multipart message",
+ "From": "Carl Worth <cworth@cworth.org>",
+ "To": "cworth@cworth.org",
+ "Date": "Fri,
+ 05 Jan 2001 15:43:57 +0000"},
+ "body": [{"id": 1,
+ "content-type": "multipart/signed",
+ "content": [{"id": 2,
+ "content-type": "multipart/mixed",
+ "content": [{"id": 3,
+ "content-type": "message/rfc822",
+ "content": [{"headers": {"Subject": "html message",
+ "From": "Carl Worth <cworth@cworth.org>",
+ "To": "cworth@cworth.org",
+ "Date": "Fri,
+ 05 Jan 2001 15:42:57 +0000"},
+ "body": [{"id": 4,
+ "content-type": "multipart/alternative",
+ "content": [{"id": 5,
+ "content-type": "text/html"},
+ {"id": 6,
+ "content-type": "text/plain",
+ "content": "This is an embedded message,
+ with a multipart/alternative part.\n"}]}]}]},
+ {"id": 7,
+ "content-type": "text/plain",
+ "filename": "YYYYY",
+ "content": "This is a text attachment.\n"},
+ {"id": 8,
+ "content-type": "text/plain",
+ "content": "And this message is signed.\n\n-Carl\n"}]},
+ {"id": 9,
+ "content-type": "application/pgp-signature"}]}]}}
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
 test_begin_subtest "'notmuch show --part' does not corrupt a part with CRLF pair"
 notmuch show --format=raw --part=3 id:base64-part-with-crlf > crlf.out
 echo -n -e "\xEF\x0D\x0A" > crlf.expected
-- 
1.7.5.4

Re: [PATCH v5.2 1/7] test: Add broken test for the new JSON reply format.

[Austin Clements]

One nit, actually.  It would good if this test were marked
test_subtest_known_broken by this patch and then unmarked broken by
the patch that adds the JSON reply format so that the test doesn't
outright fail at any point in the history.

Quoth Adam Wolfe Gordon on Feb 15 at  8:12 pm:
> ---
>  test/multipart |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 51 insertions(+), 0 deletions(-)
> 
> diff --git a/test/multipart b/test/multipart
> index a3036b4..e7abcc2 100755
> --- a/test/multipart
> +++ b/test/multipart
> @@ -589,6 +589,57 @@ Non-text part: text/html
>  EOF
>  test_expect_equal_file OUTPUT EXPECTED
>  
> +test_begin_subtest "'notmuch reply' to a multipart message with json format"
> +notmuch reply --format=json 'id:87liy5ap00.fsf@yoom.home.cworth.org' | notmuch_json_show_sanitize >OUTPUT
> +cat <<EOF >EXPECTED
> +{"reply-headers": {"Subject": "Re: Multipart message",
> + "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
> + "To": "Carl Worth <cworth@cworth.org>,
> + cworth@cworth.org",
> + "In-reply-to": "<87liy5ap00.fsf@yoom.home.cworth.org>",
> + "References": " <87liy5ap00.fsf@yoom.home.cworth.org>"},
> + "original": {"id": "XXXXX",
> + "match": false,
> + "filename": "YYYYY",
> + "timestamp": 978709437,
> + "date_relative": "2001-01-05",
> + "tags": ["attachment","inbox","signed","unread"],
> + "headers": {"Subject": "Multipart message",
> + "From": "Carl Worth <cworth@cworth.org>",
> + "To": "cworth@cworth.org",
> + "Date": "Fri,
> + 05 Jan 2001 15:43:57 +0000"},
> + "body": [{"id": 1,
> + "content-type": "multipart/signed",
> + "content": [{"id": 2,
> + "content-type": "multipart/mixed",
> + "content": [{"id": 3,
> + "content-type": "message/rfc822",
> + "content": [{"headers": {"Subject": "html message",
> + "From": "Carl Worth <cworth@cworth.org>",
> + "To": "cworth@cworth.org",
> + "Date": "Fri,
> + 05 Jan 2001 15:42:57 +0000"},
> + "body": [{"id": 4,
> + "content-type": "multipart/alternative",
> + "content": [{"id": 5,
> + "content-type": "text/html"},
> + {"id": 6,
> + "content-type": "text/plain",
> + "content": "This is an embedded message,
> + with a multipart/alternative part.\n"}]}]}]},
> + {"id": 7,
> + "content-type": "text/plain",
> + "filename": "YYYYY",
> + "content": "This is a text attachment.\n"},
> + {"id": 8,
> + "content-type": "text/plain",
> + "content": "And this message is signed.\n\n-Carl\n"}]},
> + {"id": 9,
> + "content-type": "application/pgp-signature"}]}]}}
> +EOF
> +test_expect_equal_file OUTPUT EXPECTED
> +
>  test_begin_subtest "'notmuch show --part' does not corrupt a part with CRLF pair"
>  notmuch show --format=raw --part=3 id:base64-part-with-crlf > crlf.out
>  echo -n -e "\xEF\x0D\x0A" > crlf.expected

[PATCH v5.2 2/7] reply: Factor out reply creation

[Adam Wolfe Gordon]

Factor out the creation of a reply message based on an original
message so it can be shared by different reply formats.
---
 notmuch-reply.c |  101 +++++++++++++++++++++++++++++++-----------------------
 1 files changed, 58 insertions(+), 43 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index 6b244e6..8e56245 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -505,6 +505,61 @@ guess_from_received_header (notmuch_config_t *config, notmuch_message_t *message
     return NULL;
 }
 
+static GMimeMessage *
+create_reply_message(void *ctx,
+		     notmuch_config_t *config,
+		     notmuch_message_t *message,
+		     notmuch_bool_t reply_all)
+{
+    const char *subject, *from_addr = NULL;
+    const char *in_reply_to, *orig_references, *references;
+
+    /* The 1 means we want headers in a "pretty" order. */
+    GMimeMessage *reply = g_mime_message_new (1);
+    if (reply == NULL) {
+	fprintf (stderr, "Out of memory\n");
+	return NULL;
+    }
+
+    subject = notmuch_message_get_header (message, "subject");
+    if (subject) {
+	if (strncasecmp (subject, "Re:", 3))
+	    subject = talloc_asprintf (ctx, "Re: %s", subject);
+	g_mime_message_set_subject (reply, subject);
+    }
+
+    from_addr = add_recipients_from_message (reply, config,
+					     message, reply_all);
+
+    if (from_addr == NULL)
+	from_addr = guess_from_received_header (config, message);
+
+    if (from_addr == NULL)
+	from_addr = notmuch_config_get_user_primary_email (config);
+
+    from_addr = talloc_asprintf (ctx, "%s <%s>",
+				 notmuch_config_get_user_name (config),
+				 from_addr);
+    g_mime_object_set_header (GMIME_OBJECT (reply),
+			      "From", from_addr);
+
+    in_reply_to = talloc_asprintf (ctx, "<%s>",
+				   notmuch_message_get_message_id (message));
+
+    g_mime_object_set_header (GMIME_OBJECT (reply),
+			      "In-Reply-To", in_reply_to);
+
+    orig_references = notmuch_message_get_header (message, "references");
+    references = talloc_asprintf (ctx, "%s%s%s",
+				  orig_references ? orig_references : "",
+				  orig_references ? " " : "",
+				  in_reply_to);
+    g_mime_object_set_header (GMIME_OBJECT (reply),
+			      "References", references);
+
+    return reply;
+}
+
 static int
 notmuch_reply_format_default(void *ctx,
 			     notmuch_config_t *config,
@@ -515,8 +570,6 @@ notmuch_reply_format_default(void *ctx,
     GMimeMessage *reply;
     notmuch_messages_t *messages;
     notmuch_message_t *message;
-    const char *subject, *from_addr = NULL;
-    const char *in_reply_to, *orig_references, *references;
     const notmuch_show_format_t *format = &format_reply;
 
     for (messages = notmuch_query_search_messages (query);
@@ -525,48 +578,10 @@ notmuch_reply_format_default(void *ctx,
     {
 	message = notmuch_messages_get (messages);
 
-	/* The 1 means we want headers in a "pretty" order. */
-	reply = g_mime_message_new (1);
-	if (reply == NULL) {
-	    fprintf (stderr, "Out of memory\n");
-	    return 1;
-	}
-
-	subject = notmuch_message_get_header (message, "subject");
-	if (subject) {
-	    if (strncasecmp (subject, "Re:", 3))
-		subject = talloc_asprintf (ctx, "Re: %s", subject);
-	    g_mime_message_set_subject (reply, subject);
-	}
-
-	from_addr = add_recipients_from_message (reply, config, message,
-						 reply_all);
+	reply = create_reply_message (ctx, config, message, reply_all);
 
-	if (from_addr == NULL)
-	    from_addr = guess_from_received_header (config, message);
-
-	if (from_addr == NULL)
-	    from_addr = notmuch_config_get_user_primary_email (config);
-
-	from_addr = talloc_asprintf (ctx, "%s <%s>",
-				     notmuch_config_get_user_name (config),
-				     from_addr);
-	g_mime_object_set_header (GMIME_OBJECT (reply),
-				  "From", from_addr);
-
-	in_reply_to = talloc_asprintf (ctx, "<%s>",
-			     notmuch_message_get_message_id (message));
-
-	g_mime_object_set_header (GMIME_OBJECT (reply),
-				  "In-Reply-To", in_reply_to);
-
-	orig_references = notmuch_message_get_header (message, "references");
-	references = talloc_asprintf (ctx, "%s%s%s",
-				      orig_references ? orig_references : "",
-				      orig_references ? " " : "",
-				      in_reply_to);
-	g_mime_object_set_header (GMIME_OBJECT (reply),
-				  "References", references);
+	if (!reply)
+	    continue;
 
 	show_reply_headers (reply);
 
-- 
1.7.5.4

[PATCH v5.2 3/7] reply: Add a JSON reply format.

[Adam Wolfe Gordon]

This new JSON format for replies includes headers generated for a
reply message as well as the headers of the original message.  Using
this data, a client can intelligently create a reply. For example, the
emacs client will be able to create replies with quoted HTML parts by
parsing the HTML parts.

Reply now enforces that only one message is returned, as the semantics
of replying to multiple messages are not well-defined.
---
 notmuch-client.h |    3 +
 notmuch-reply.c  |  121 ++++++++++++++++++++++++++++++++++++++++++++++-------
 notmuch-show.c   |    2 +-
 3 files changed, 109 insertions(+), 17 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 60828aa..d28ea07 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -344,6 +344,9 @@ typedef struct mime_node {
     int next_part_num;
 } mime_node_t;
 
+void
+format_part_json (const void *ctx, mime_node_t *node, notmuch_bool_t first);
+
 /* 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
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 8e56245..6670288 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -572,30 +572,115 @@ notmuch_reply_format_default(void *ctx,
     notmuch_message_t *message;
     const notmuch_show_format_t *format = &format_reply;
 
-    for (messages = notmuch_query_search_messages (query);
-	 notmuch_messages_valid (messages);
-	 notmuch_messages_move_to_next (messages))
-    {
-	message = notmuch_messages_get (messages);
+    if (notmuch_query_count_messages (query) != 1) {
+	fprintf (stderr, "Error: search term did not match precisely one message.\n");
+	return 1;
+    }
 
-	reply = create_reply_message (ctx, config, message, reply_all);
+    messages = notmuch_query_search_messages (query);
+    message = notmuch_messages_get (messages);
 
-	if (!reply)
-	    continue;
+    reply = create_reply_message (ctx, config, message, reply_all);
 
-	show_reply_headers (reply);
+    if (!reply)
+	return 1;
 
-	g_object_unref (G_OBJECT (reply));
-	reply = NULL;
+    show_reply_headers (reply);
 
-	printf ("On %s, %s wrote:\n",
-		notmuch_message_get_header (message, "date"),
-		notmuch_message_get_header (message, "from"));
+    g_object_unref (G_OBJECT (reply));
+    reply = NULL;
 
-	show_message_body (message, format, params);
+    printf ("On %s, %s wrote:\n",
+	    notmuch_message_get_header (message, "date"),
+	    notmuch_message_get_header (message, "from"));
 
-	notmuch_message_destroy (message);
+    show_message_body (message, format, params);
+
+    notmuch_message_destroy (message);
+
+    return 0;
+}
+
+static void
+format_reply_headers_json (const void *ctx, GMimeMessage *message)
+{
+    void *local = talloc_new (ctx);
+    InternetAddressList *recipients;
+    const char *recipients_string;
+
+    printf ("{%s: %s",
+	    json_quote_str (local, "Subject"),
+	    json_quote_str (local, g_mime_message_get_subject (message)));
+    printf (", %s: %s",
+	    json_quote_str (local, "From"),
+	    json_quote_str (local, 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 (", %s: %s",
+		json_quote_str (local, "To"),
+		json_quote_str (local, 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 (", %s: %s",
+		json_quote_str (local, "Cc"),
+		json_quote_str (local, recipients_string));
+
+    printf (", %s: %s",
+	    json_quote_str (local, "In-reply-to"),
+	    json_quote_str (local, g_mime_object_get_header (GMIME_OBJECT (message), "In-reply-to")));
+
+    printf (", %s: %s",
+	    json_quote_str (local, "References"),
+	    json_quote_str (local, g_mime_object_get_header (GMIME_OBJECT (message), "References")));
+
+    talloc_free (local);
+}
+
+static int
+notmuch_reply_format_json(void *ctx,
+			  notmuch_config_t *config,
+			  notmuch_query_t *query,
+			  notmuch_show_params_t *params,
+			  notmuch_bool_t reply_all)
+{
+    GMimeMessage *reply;
+    notmuch_messages_t *messages;
+    notmuch_message_t *message;
+    mime_node_t *node;
+
+    if (notmuch_query_count_messages (query) != 1) {
+	fprintf (stderr, "Error: search term did not match precisely one message.\n");
+	return 1;
     }
+
+    messages = notmuch_query_search_messages (query);
+    message = notmuch_messages_get (messages);
+    if (mime_node_open (ctx, message, params->cryptoctx, params->decrypt,
+			&node) != NOTMUCH_STATUS_SUCCESS)
+	return 1;
+
+    reply = create_reply_message (ctx, config, message, reply_all);
+    if (!reply)
+	return 1;
+
+    /* The headers of the reply message we've created */
+    printf ("{\"reply-headers\": ");
+    format_reply_headers_json (ctx, reply);
+    printf ("}");
+    g_object_unref (G_OBJECT (reply));
+    reply = NULL;
+
+    /* Start the original */
+    printf (", \"original\": ");
+
+    format_part_json (ctx, node, TRUE);
+
+    /* End */
+    printf ("}\n");
+    notmuch_message_destroy (message);
+
     return 0;
 }
 
@@ -661,6 +746,7 @@ notmuch_reply_format_headers_only(void *ctx,
 
 enum {
     FORMAT_DEFAULT,
+    FORMAT_JSON,
     FORMAT_HEADERS_ONLY,
 };
 
@@ -680,6 +766,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_KEYWORD, &format, "format", 'f',
 	  (notmuch_keyword_t []){ { "default", FORMAT_DEFAULT },
+				  { "json", FORMAT_JSON },
 				  { "headers-only", FORMAT_HEADERS_ONLY },
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_KEYWORD, &reply_all, "reply-to", 'r',
@@ -698,6 +785,8 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
 
     if (format == FORMAT_HEADERS_ONLY)
 	reply_format_func = notmuch_reply_format_headers_only;
+    else if (format == FORMAT_JSON)
+	reply_format_func = notmuch_reply_format_json;
     else
 	reply_format_func = notmuch_reply_format_default;
 
diff --git a/notmuch-show.c b/notmuch-show.c
index 6a171a4..c570a16 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -652,7 +652,7 @@ format_part_text (const void *ctx, mime_node_t *node,
     printf ("\f%s}\n", part_type);
 }
 
-static void
+void
 format_part_json (const void *ctx, mime_node_t *node, notmuch_bool_t first)
 {
     /* Any changes to the JSON format should be reflected in the file
-- 
1.7.5.4

Re: [PATCH v5.2 3/7] reply: Add a JSON reply format.

[Austin Clements]

The first two patches LGTM.  A few nits in this one.

Quoth Adam Wolfe Gordon on Feb 15 at  8:12 pm:
> This new JSON format for replies includes headers generated for a
> reply message as well as the headers of the original message.  Using
> this data, a client can intelligently create a reply. For example, the
> emacs client will be able to create replies with quoted HTML parts by
> parsing the HTML parts.
> 
> Reply now enforces that only one message is returned, as the semantics
> of replying to multiple messages are not well-defined.
> ---
>  notmuch-client.h |    3 +
>  notmuch-reply.c  |  121 ++++++++++++++++++++++++++++++++++++++++++++++-------
>  notmuch-show.c   |    2 +-
>  3 files changed, 109 insertions(+), 17 deletions(-)
> 
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 60828aa..d28ea07 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -344,6 +344,9 @@ typedef struct mime_node {
>      int next_part_num;
>  } mime_node_t;
>  
> +void
> +format_part_json (const void *ctx, mime_node_t *node, notmuch_bool_t first);
> +

This is the wrong place for this declaration, since it is not part of
the MIME node abstraction.  It should go somewhere above the /*
notmuch-config.c */ comment.  Above that, it's a bit of a jumble.  I'd
probably put it right after notmuch_show_command.

>  /* 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
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 8e56245..6670288 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -572,30 +572,115 @@ notmuch_reply_format_default(void *ctx,
>      notmuch_message_t *message;
>      const notmuch_show_format_t *format = &format_reply;
>  
> -    for (messages = notmuch_query_search_messages (query);
> -	 notmuch_messages_valid (messages);
> -	 notmuch_messages_move_to_next (messages))
> -    {
> -	message = notmuch_messages_get (messages);
> +    if (notmuch_query_count_messages (query) != 1) {
> +	fprintf (stderr, "Error: search term did not match precisely one message.\n");
> +	return 1;
> +    }

Technically count_messages does not have to be accurate, but since
this is the same thing notmuch-show does, it's probably fine for now.

Perhaps we should add proper handling of multi-message replies to
devel/TODO?

>  
> -	reply = create_reply_message (ctx, config, message, reply_all);
> +    messages = notmuch_query_search_messages (query);
> +    message = notmuch_messages_get (messages);
>  
> -	if (!reply)
> -	    continue;
> +    reply = create_reply_message (ctx, config, message, reply_all);
>  
> -	show_reply_headers (reply);
> +    if (!reply)
> +	return 1;
>  
> -	g_object_unref (G_OBJECT (reply));
> -	reply = NULL;
> +    show_reply_headers (reply);
>  
> -	printf ("On %s, %s wrote:\n",
> -		notmuch_message_get_header (message, "date"),
> -		notmuch_message_get_header (message, "from"));
> +    g_object_unref (G_OBJECT (reply));
> +    reply = NULL;
>  
> -	show_message_body (message, format, params);
> +    printf ("On %s, %s wrote:\n",
> +	    notmuch_message_get_header (message, "date"),
> +	    notmuch_message_get_header (message, "from"));
>  
> -	notmuch_message_destroy (message);
> +    show_message_body (message, format, params);
> +
> +    notmuch_message_destroy (message);
> +
> +    return 0;
> +}

The above change could be separated out in to a separate patch, since
it has nothing to do with JSON.

> +
> +static void
> +format_reply_headers_json (const void *ctx, GMimeMessage *message)
> +{
> +    void *local = talloc_new (ctx);
> +    InternetAddressList *recipients;
> +    const char *recipients_string;
> +
> +    printf ("{%s: %s",
> +	    json_quote_str (local, "Subject"),
> +	    json_quote_str (local, g_mime_message_get_subject (message)));
> +    printf (", %s: %s",
> +	    json_quote_str (local, "From"),
> +	    json_quote_str (local, 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 (", %s: %s",
> +		json_quote_str (local, "To"),
> +		json_quote_str (local, 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 (", %s: %s",
> +		json_quote_str (local, "Cc"),
> +		json_quote_str (local, recipients_string));
> +
> +    printf (", %s: %s",
> +	    json_quote_str (local, "In-reply-to"),
> +	    json_quote_str (local, g_mime_object_get_header (GMIME_OBJECT (message), "In-reply-to")));
> +
> +    printf (", %s: %s",

There should be a close brace in this string, rather than opening the
object in this function and closing it in the caller.

> +	    json_quote_str (local, "References"),
> +	    json_quote_str (local, g_mime_object_get_header (GMIME_OBJECT (message), "References")));
> +
> +    talloc_free (local);
> +}

I would much rather see format_headers_json in notmuch-show.c gain a
parameter that tells it whether or not to include in-reply-to and
references.

> +
> +static int
> +notmuch_reply_format_json(void *ctx,
> +			  notmuch_config_t *config,
> +			  notmuch_query_t *query,
> +			  notmuch_show_params_t *params,
> +			  notmuch_bool_t reply_all)
> +{
> +    GMimeMessage *reply;
> +    notmuch_messages_t *messages;
> +    notmuch_message_t *message;
> +    mime_node_t *node;
> +
> +    if (notmuch_query_count_messages (query) != 1) {
> +	fprintf (stderr, "Error: search term did not match precisely one message.\n");
> +	return 1;
>      }
> +
> +    messages = notmuch_query_search_messages (query);
> +    message = notmuch_messages_get (messages);
> +    if (mime_node_open (ctx, message, params->cryptoctx, params->decrypt,
> +			&node) != NOTMUCH_STATUS_SUCCESS)
> +	return 1;
> +
> +    reply = create_reply_message (ctx, config, message, reply_all);
> +    if (!reply)
> +	return 1;
> +
> +    /* The headers of the reply message we've created */
> +    printf ("{\"reply-headers\": ");
> +    format_reply_headers_json (ctx, reply);
> +    printf ("}");
> +    g_object_unref (G_OBJECT (reply));
> +    reply = NULL;
> +
> +    /* Start the original */
> +    printf (", \"original\": ");
> +
> +    format_part_json (ctx, node, TRUE);
> +
> +    /* End */
> +    printf ("}\n");
> +    notmuch_message_destroy (message);
> +
>      return 0;
>  }
>  
> @@ -661,6 +746,7 @@ notmuch_reply_format_headers_only(void *ctx,
>  
>  enum {
>      FORMAT_DEFAULT,
> +    FORMAT_JSON,
>      FORMAT_HEADERS_ONLY,
>  };
>  
> @@ -680,6 +766,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>      notmuch_opt_desc_t options[] = {
>  	{ NOTMUCH_OPT_KEYWORD, &format, "format", 'f',
>  	  (notmuch_keyword_t []){ { "default", FORMAT_DEFAULT },
> +				  { "json", FORMAT_JSON },
>  				  { "headers-only", FORMAT_HEADERS_ONLY },
>  				  { 0, 0 } } },
>  	{ NOTMUCH_OPT_KEYWORD, &reply_all, "reply-to", 'r',
> @@ -698,6 +785,8 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>  
>      if (format == FORMAT_HEADERS_ONLY)
>  	reply_format_func = notmuch_reply_format_headers_only;
> +    else if (format == FORMAT_JSON)
> +	reply_format_func = notmuch_reply_format_json;
>      else
>  	reply_format_func = notmuch_reply_format_default;
>  
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 6a171a4..c570a16 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -652,7 +652,7 @@ format_part_text (const void *ctx, mime_node_t *node,
>      printf ("\f%s}\n", part_type);
>  }
>  
> -static void
> +void
>  format_part_json (const void *ctx, mime_node_t *node, notmuch_bool_t first)
>  {
>      /* Any changes to the JSON format should be reflected in the file

Re: [PATCH v5.2 3/7] reply: Add a JSON reply format.

[Adam Wolfe Gordon]

On Fri, Feb 17, 2012 at 10:04, Austin Clements <amdragon@mit.edu> wrote:
> The first two patches LGTM.  A few nits in this one.

Thanks for the review. A couple of points to discuss below; everything
else I'll change for the next version.

>> +void
>> +format_part_json (const void *ctx, mime_node_t *node, notmuch_bool_t first);
>> +
>
> This is the wrong place for this declaration, since it is not part of
> the MIME node abstraction.  It should go somewhere above the /*
> notmuch-config.c */ comment.  Above that, it's a bit of a jumble.  I'd
> probably put it right after notmuch_show_command.

Agreed. I initially had it earlier in the file (with the other
show-related functions), but moved it down since it requires the
mime_node_t declaration. There are a couple of options: put in a
pre-declaration of mime_node_t early in the file, or move the
mime_node stuff to a separate header file and include it in
notmuch-client.h. I lean toward the latter, since notmuch-client.h is
getting very big as it is. Thoughts?

>> +    if (notmuch_query_count_messages (query) != 1) {
>> +     fprintf (stderr, "Error: search term did not match precisely one message.\n");
>> +     return 1;
>> +    }
>
> Technically count_messages does not have to be accurate, but since
> this is the same thing notmuch-show does, it's probably fine for now.

Ah, I didn't realize this. I just followed the show example.

> Perhaps we should add proper handling of multi-message replies to
> devel/TODO?

Probably a good idea, although it means defining what proper handling
of multi-message replies in the CLI means. Personally, I don't think
it makes much sense to reply to multiple messages. The only place that
functionality is actually used (AFAIK) is in notmuch-search.el, which,
with my patches, throws an error if you try to reply to a thread
containing multiple messages. In my mind, the correct behavior in that
specific case is to create a reply to the last message in the thread,
which is better handled in the emacs code than the CLI anyway.

Re: [PATCH v5.2 3/7] reply: Add a JSON reply format.

[Austin Clements]

Quoth Adam Wolfe Gordon on Feb 17 at  7:06 pm:
> On Fri, Feb 17, 2012 at 10:04, Austin Clements <amdragon@mit.edu> wrote:
> > The first two patches LGTM.  A few nits in this one.
> 
> Thanks for the review. A couple of points to discuss below; everything
> else I'll change for the next version.
> 
> >> +void
> >> +format_part_json (const void *ctx, mime_node_t *node, notmuch_bool_t first);
> >> +
> >
> > This is the wrong place for this declaration, since it is not part of
> > the MIME node abstraction.  It should go somewhere above the /*
> > notmuch-config.c */ comment.  Above that, it's a bit of a jumble.  I'd
> > probably put it right after notmuch_show_command.
> 
> Agreed. I initially had it earlier in the file (with the other
> show-related functions), but moved it down since it requires the
> mime_node_t declaration. There are a couple of options: put in a
> pre-declaration of mime_node_t early in the file, or move the
> mime_node stuff to a separate header file and include it in
> notmuch-client.h. I lean toward the latter, since notmuch-client.h is
> getting very big as it is. Thoughts?

struct mime_node is already declared at the top of notmuch-client.h.
That should probably just be replaced with
 typedef struct mime_node mime_node_t;
(and notmuch_show_format.part can be updated to take a mime_node_t *).
Alternatively, you could change your declaration to take a struct
mime_node, but it's nicer if the declaration and the definition match
literally and not just logically.  Moving mime_node_t into its own
header isn't a bad idea on its own (in fact, I specifically wrote it
so it could live in util/ if we wanted), but seems like overkill for
this.

> >> +    if (notmuch_query_count_messages (query) != 1) {
> >> +     fprintf (stderr, "Error: search term did not match precisely one message.\n");
> >> +     return 1;
> >> +    }
> >
> > Technically count_messages does not have to be accurate, but since
> > this is the same thing notmuch-show does, it's probably fine for now.
> 
> Ah, I didn't realize this. I just followed the show example.

I think it's fine as is.  Probably as a later, independent patch, we
should update both places to just check the iterator after the call to
notmuch_messages_get.  Or you could update it as an extra minipatch in
your series if you want.

> > Perhaps we should add proper handling of multi-message replies to
> > devel/TODO?
> 
> Probably a good idea, although it means defining what proper handling
> of multi-message replies in the CLI means. Personally, I don't think
> it makes much sense to reply to multiple messages. The only place that
> functionality is actually used (AFAIK) is in notmuch-search.el, which,
> with my patches, throws an error if you try to reply to a thread
> containing multiple messages. In my mind, the correct behavior in that
> specific case is to create a reply to the last message in the thread,
> which is better handled in the emacs code than the CLI anyway.

*Doing* it requires defining what it means, but devel/TODO is a fine
place for arbitrarily fantastical and under-specified desires.  That
said, I don't think defining it is that hard.  We could just do
whatever mutt does.  The body of a multi-message reply in mutt is the
concatenation of the bodies that would be generated for individual
replies and I suspect the headers are the gathered up to/cc addresses,
an in-reply-to that lists all of the replied to message IDs, and a
subject and references header derived from the first message replied
to.

[PATCH v5.2 4/7] man: Update notmuch-reply man page for JSON format.

[Adam Wolfe Gordon]

---
 man/man1/notmuch-reply.1 |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/man/man1/notmuch-reply.1 b/man/man1/notmuch-reply.1
index 5160ece..307abee 100644
--- a/man/man1/notmuch-reply.1
+++ b/man/man1/notmuch-reply.1
@@ -43,6 +43,11 @@ include
 .BR default
 Includes subject and quoted message body.
 .TP
+.BR json
+Produces JSON output containing headers for a reply message and the
+contents of the original message. This output can be used by a client
+to create a reply message intelligently.
+.TP
 .BR headers\-only
 Only produces In\-Reply\-To, References, To, Cc, and Bcc headers.
 .RE
-- 
1.7.5.4

Re: [PATCH v5.2 4/7] man: Update notmuch-reply man page for JSON format.

[Austin Clements]

The JSON reply format should also get added to devel/schemata (since
you're building on my JSON show rewrite patches anyway).

Quoth Adam Wolfe Gordon on Feb 15 at  8:12 pm:
> ---
>  man/man1/notmuch-reply.1 |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/man/man1/notmuch-reply.1 b/man/man1/notmuch-reply.1
> index 5160ece..307abee 100644
> --- a/man/man1/notmuch-reply.1
> +++ b/man/man1/notmuch-reply.1
> @@ -43,6 +43,11 @@ include
>  .BR default
>  Includes subject and quoted message body.
>  .TP
> +.BR json
> +Produces JSON output containing headers for a reply message and the
> +contents of the original message. This output can be used by a client
> +to create a reply message intelligently.
> +.TP
>  .BR headers\-only
>  Only produces In\-Reply\-To, References, To, Cc, and Bcc headers.
>  .RE

[PATCH v5.2 5/7] emacs: Factor out useful functions into notmuch-lib

[Adam Wolfe Gordon]

Move a few functions related to handling multipart/alternative parts
into notmuch-lib.el, so they can be used by future reply code.
---
 emacs/notmuch-lib.el  |   33 +++++++++++++++++++++++++++++++++
 emacs/notmuch-show.el |   24 ++----------------------
 2 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index d315f76..7e3f110 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -21,6 +21,8 @@
 
 ;; This is an part of an emacs-based interface to the notmuch mail system.
 
+(eval-when-compile (require 'cl))
+
 (defvar notmuch-command "notmuch"
   "Command to run the notmuch binary.")
 
@@ -173,6 +175,37 @@ the user hasn't set this variable with the old or new value."
   (list 'when (< emacs-major-version 23)
 	form))
 
+(defun notmuch-split-content-type (content-type)
+  "Split content/type into 'content' and 'type'"
+  (split-string content-type "/"))
+
+(defun notmuch-match-content-type (t1 t2)
+  "Return t if t1 and t2 are matching content types, taking wildcards into account"
+  (let ((st1 (notmuch-split-content-type t1))
+	(st2 (notmuch-split-content-type t2)))
+    (if (or (string= (cadr st1) "*")
+	    (string= (cadr st2) "*"))
+	(string= (car st1) (car st2))
+      (string= t1 t2))))
+
+(defvar notmuch-multipart/alternative-discouraged
+  '(
+    ;; Avoid HTML parts.
+    "text/html"
+    ;; multipart/related usually contain a text/html part and some associated graphics.
+    "multipart/related"
+    ))
+
+(defun notmuch-multipart/alternative-choose (types)
+  "Return a list of preferred types from the given list of types"
+  ;; Based on `mm-preferred-alternative-precedence'.
+  (let ((seq types))
+    (dolist (pref (reverse notmuch-multipart/alternative-discouraged))
+      (dolist (elem (copy-sequence seq))
+	(when (string-match pref elem)
+	  (setq seq (nconc (delete elem seq) (list elem))))))
+    seq))
+
 ;; Compatibility functions for versions of emacs before emacs 23.
 ;;
 ;; Both functions here were copied from emacs 23 with the following copyright:
diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 43408d9..90cdd38 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -513,30 +513,13 @@ current buffer, if possible."
 	    (mm-display-part handle)
 	    t))))))
 
-(defvar notmuch-show-multipart/alternative-discouraged
-  '(
-    ;; Avoid HTML parts.
-    "text/html"
-    ;; multipart/related usually contain a text/html part and some associated graphics.
-    "multipart/related"
-    ))
-
 (defun notmuch-show-multipart/*-to-list (part)
   (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
 	  (plist-get part :content)))
 
-(defun notmuch-show-multipart/alternative-choose (types)
-  ;; Based on `mm-preferred-alternative-precedence'.
-  (let ((seq types))
-    (dolist (pref (reverse notmuch-show-multipart/alternative-discouraged))
-      (dolist (elem (copy-sequence seq))
-	(when (string-match pref elem)
-	  (setq seq (nconc (delete elem seq) (list elem))))))
-    seq))
-
 (defun notmuch-show-insert-part-multipart/alternative (msg part content-type nth depth declared-type)
   (notmuch-show-insert-part-header nth declared-type content-type nil)
-  (let ((chosen-type (car (notmuch-show-multipart/alternative-choose (notmuch-show-multipart/*-to-list part))))
+  (let ((chosen-type (car (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part))))
 	(inner-parts (plist-get part :content))
 	(start (point)))
     ;; This inserts all parts of the chosen type rather than just one,
@@ -775,9 +758,6 @@ current buffer, if possible."
 
 ;; Functions for determining how to handle MIME parts.
 
-(defun notmuch-show-split-content-type (content-type)
-  (split-string content-type "/"))
-
 (defun notmuch-show-handlers-for (content-type)
   "Return a list of content handlers for a part of type CONTENT-TYPE."
   (let (result)
@@ -788,7 +768,7 @@ current buffer, if possible."
 	  (list (intern (concat "notmuch-show-insert-part-*/*"))
 		(intern (concat
 			 "notmuch-show-insert-part-"
-			 (car (notmuch-show-split-content-type content-type))
+			 (car (notmuch-split-content-type content-type))
 			 "/*"))
 		(intern (concat "notmuch-show-insert-part-" content-type))))
     result))
-- 
1.7.5.4

[PATCH v5.2 6/7] test: Add broken tests for new emacs reply functionality

[Adam Wolfe Gordon]

Add tests for creating nice replies to multipart messages, including
those with HTML parts. These tests are expected to fail for now.
---
 test/emacs |   97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 97 insertions(+), 0 deletions(-)

diff --git a/test/emacs b/test/emacs
index d4a8d30..c3a75e9 100755
--- a/test/emacs
+++ b/test/emacs
@@ -273,6 +273,103 @@ On 01 Jan 2000 12:00:00 -0000, Notmuch Test Suite <test_suite@notmuchmail.org> w
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
+test_begin_subtest "Reply within emacs to a multipart/mixed message"
+test_subtest_known_broken
+test_emacs '(notmuch-show "id:20091118002059.067214ed@hikari")
+		(notmuch-show-reply)
+		(test-output)'
+cat <<EOF >EXPECTED
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+To: Adrian Perez de Castro <aperez@igalia.com>, notmuch@notmuchmail.org
+Subject: Re: [notmuch] Introducing myself
+In-Reply-To: <20091118002059.067214ed@hikari>
+Fcc: ${MAIL_DIR}/sent
+--text follows this line--
+Adrian Perez de Castro <aperez@igalia.com> writes:
+
+> Hello to all,
+>
+> I have just heard about Not Much today in some random Linux-related news
+> site (LWN?), my name is Adrian Perez and I work as systems administrator
+> (although I can do some code as well :P). I have always thought that the
+> ideas behind Sup were great, but after some time using it, I got tired of
+> the oddities that it has. I also do not like doing things like having to
+> install Ruby just for reading and sorting mails. Some time ago I thought
+> about doing something like Not Much and in fact I played a bit with the
+> Python+Xapian and the Python+Whoosh combinations, because I find relaxing
+> to code things in Python when I am not working and also it is installed
+> by default on most distribution. I got to have some mailboxes indexed and
+> basic searching working a couple of months ago. Lately I have been very
+> busy and had no time for coding, and them... boom! Not Much appears -- and
+> it is almost exactly what I was trying to do, but faster. I have been
+> playing a bit with Not Much today, and I think it has potential.
+>
+> Also, I would like to share one idea I had in mind, that you might find
+> interesting: One thing I have found very annoying is having to re-tag my
+> mail when the indexes get b0rked (it happened a couple of times to me while
+> using Sup), so I was planning to mails as read/unread and adding the tags
+> not just to the index, but to the mail text itself, e.g. by adding a
+> "X-Tags" header field or by reusing the "Keywords" one. This way, the index
+> could be totally recreated by re-reading the mail directories, and this
+> would also allow to a tools like OfflineIMAP [1] to get the mails into a
+> local maildir, tagging and indexing the mails with the e-mail reader and
+> then syncing back the messages with the "X-Tags" header to the IMAP server.
+> This would allow to use the mail reader from a different computer and still
+> have everything tagged finely.
+>
+> Best regards,
+>
+>
+> ---
+> [1] http://software.complete.org/software/projects/show/offlineimap
+>
+> -- 
+> Adrian Perez de Castro <aperez@igalia.com>
+> Igalia - Free Software Engineering
+> _______________________________________________
+> notmuch mailing list
+> notmuch@notmuchmail.org
+> http://notmuchmail.org/mailman/listinfo/notmuch
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
+test_begin_subtest "Reply within emacs to a multipart/alternative message"
+test_subtest_known_broken
+test_emacs '(notmuch-show "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com")
+		(notmuch-show-reply)
+		(test-output)'
+cat <<EOF >EXPECTED
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+To: Alex Botero-Lowry <alex.boterolowry@gmail.com>, notmuch@notmuchmail.org
+Subject: Re: [notmuch] preliminary FreeBSD support
+In-Reply-To: <cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com>
+Fcc: ${MAIL_DIR}/sent
+--text follows this line--
+Alex Botero-Lowry <alex.boterolowry@gmail.com> writes:
+
+> I saw the announcement this morning, and was very excited, as I had been
+> hoping sup would be turned into a library,
+> since I like the concept more than the UI (I'd rather an emacs interface).
+>
+> I did a preliminary compile which worked out fine, but
+> sysconf(_SC_SC_GETPW_R_SIZE_MAX) returns -1 on
+> FreeBSD, so notmuch_config_open segfaulted.
+>
+> Attached is a patch that supplies a default buffer size of 64 in cases where
+> -1 is returned.
+>
+> http://www.opengroup.org/austin/docs/austin_328.txt - seems to indicate this
+> is acceptable behavior,
+> and http://mail-index.netbsd.org/pkgsrc-bugs/2006/06/07/msg016808.htmlspecifically
+> uses 64 as the
+> buffer size.
+> _______________________________________________
+> notmuch mailing list
+> notmuch@notmuchmail.org
+> http://notmuchmail.org/mailman/listinfo/notmuch
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
 test_begin_subtest "Quote MML tags in reply"
 message_id='test-emacs-mml-quoting@message.id'
 add_message [id]="$message_id" \
-- 
1.7.5.4

[PATCH v5.2 7/7] emacs: Use the new JSON reply format and message-cite-original

[Adam Wolfe Gordon]

Use the new JSON reply format to create replies in emacs. Quote HTML
parts nicely by using mm-display-part to turn them into displayable
text, then quoting them with message-cite-original. This is very
useful for users who regularly receive HTML-only email.

Use message-mode's message-cite-original function to create the
quoted body for reply messages. In order to make this act like the
existing notmuch defaults, you will need to set the following in
your emacs configuration:

message-citation-line-format "On %a, %d %b %Y, %f wrote:"
message-citation-line-function 'message-insert-formatted-citation-line

The tests have been updated to reflect the (ugly) emacs default.
---
 emacs/notmuch-lib.el |    6 ++
 emacs/notmuch-mua.el |  127 +++++++++++++++++++++++++++++++++++---------------
 test/emacs           |    8 ++--
 3 files changed, 100 insertions(+), 41 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 7e3f110..3fc7aff 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -206,6 +206,12 @@ the user hasn't set this variable with the old or new value."
 	  (setq seq (nconc (delete elem seq) (list elem))))))
     seq))
 
+(defun notmuch-parts-filter-by-type (parts type)
+  "Given a vector of message parts, return a vector containing the ones matching the given type."
+  (loop for part across parts
+	if (notmuch-match-content-type (cdr (assq 'content-type part)) type)
+	vconcat (list part)))
+
 ;; Compatibility functions for versions of emacs before emacs 23.
 ;;
 ;; Both functions here were copied from emacs 23 with the following copyright:
diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 4be7c13..7d43821 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -19,11 +19,15 @@
 ;;
 ;; Authors: David Edmondson <dme@dme.org>
 
+(require 'json)
 (require 'message)
+(require 'format-spec)
 
 (require 'notmuch-lib)
 (require 'notmuch-address)
 
+(eval-when-compile (require 'cl))
+
 ;;
 
 (defcustom notmuch-mua-send-hook '(notmuch-mua-message-send-hook)
@@ -72,56 +76,105 @@ list."
 	    (push header message-hidden-headers)))
 	notmuch-mua-hidden-headers))
 
+(defun notmuch-mua-get-displayed-part (part query-string)
+  (with-temp-buffer
+    (if (assq 'content part)
+	(insert (cdr (assq 'content part)))
+      (call-process notmuch-command nil t nil "show" "--format=raw"
+		    (format "--part=%s" (cdr (assq 'id part)))
+		    query-string))
+
+    (let ((handle (mm-make-handle (current-buffer) (list (cdr (assq 'content-type part)))))
+	  (end-of-orig (point-max)))
+      (mm-display-part handle)
+      (delete-region (point-min) end-of-orig)
+      (buffer-substring (point-min) (point-max)))))
+
+(defun notmuch-mua-multipart/*-to-list (parts)
+  (loop for part across parts
+	collect (cdr (assq 'content-type part))))
+
+(defun notmuch-mua-get-quotable-parts (parts)
+  (loop for part across parts
+	if (notmuch-match-content-type (cdr (assq 'content-type part)) "multipart/alternative")
+	  append (let* ((subparts (cdr (assq 'content part)))
+			(types (notmuch-mua-multipart/*-to-list subparts))
+			(chosen-type (car (notmuch-multipart/alternative-choose types))))
+		   (notmuch-mua-get-quotable-parts (notmuch-parts-filter-by-type subparts chosen-type)))
+	else if (notmuch-match-content-type (cdr (assq 'content-type part)) "multipart/*")
+	  append (notmuch-mua-get-quotable-parts (cdr (assq 'content part)))
+	else if (notmuch-match-content-type (cdr (assq 'content-type part)) "text/*")
+	  collect part))
+
 (defun notmuch-mua-reply (query-string &optional sender reply-all)
-  (let (headers
-	body
-	(args '("reply")))
-    (if notmuch-show-process-crypto
-	(setq args (append args '("--decrypt"))))
+  (let ((args '("reply" "--format=json"))
+	reply
+	original)
+    (when notmuch-show-process-crypto
+      (setq args (append args '("--decrypt"))))
+
     (if reply-all
 	(setq args (append args '("--reply-to=all")))
       (setq args (append args '("--reply-to=sender"))))
     (setq args (append args (list query-string)))
-    ;; This make assumptions about the output of `notmuch reply', but
-    ;; really only that the headers come first followed by a blank
-    ;; line and then the body.
+
+    ;; Get the reply object as JSON, and parse it into an elisp object.
     (with-temp-buffer
       (apply 'call-process (append (list notmuch-command nil (list t t) nil) args))
       (goto-char (point-min))
-      (if (re-search-forward "^$" nil t)
-	  (save-excursion
-	    (save-restriction
-	      (narrow-to-region (point-min) (point))
-	      (goto-char (point-min))
-	      (setq headers (mail-header-extract)))))
-      (forward-line 1)
-      (setq body (buffer-substring (point) (point-max))))
-    ;; If sender is non-nil, set the From: header to its value.
-    (when sender
-      (mail-header-set 'from sender headers))
-    (let
-	;; Overlay the composition window on that being used to read
-	;; the original message.
-	((same-window-regexps '("\\*mail .*")))
-      (notmuch-mua-mail (mail-header 'to headers)
-			(mail-header 'subject headers)
-			(message-headers-to-generate headers t '(to subject))))
-    ;; insert the message body - but put it in front of the signature
-    ;; if one is present
-    (goto-char (point-max))
-    (if (re-search-backward message-signature-separator nil t)
+      (setq reply (json-read)))
+
+    ;; Extract the original message to simplify the following code.
+    (setq original (cdr (assq 'original reply)))
+
+    ;; Extract the headers of both the reply and the original message.
+    (let* ((original-headers (cdr (assq 'headers original)))
+	   (reply-headers (cdr (assq 'reply-headers reply))))
+
+      ;; If sender is non-nil, set the From: header to its value.
+      (when sender
+	(mail-header-set 'From sender reply-headers))
+      (let
+	  ;; Overlay the composition window on that being used to read
+	  ;; the original message.
+	  ((same-window-regexps '("\\*mail .*")))
+	(notmuch-mua-mail (mail-header 'To reply-headers)
+			  (mail-header 'Subject reply-headers)
+			  (message-headers-to-generate reply-headers t '(To Subject))))
+      ;; Insert the message body - but put it in front of the signature
+      ;; if one is present
+      (goto-char (point-max))
+      (if (re-search-backward message-signature-separator nil t)
 	  (forward-line -1)
-      (goto-char (point-max)))
-    (insert body)
-    (push-mark))
-  (set-buffer-modified-p nil)
+	(goto-char (point-max)))
+
+      (let ((from (cdr (assq 'From original-headers)))
+	    (date (cdr (assq 'Date original-headers)))
+	    (start (point)))
+
+	(insert "From: " from "\n")
+	(insert "Date: " date "\n\n")
+
+	;; Get the parts of the original message that should be quoted; this includes
+	;; all the text parts, except the non-preferred ones in a multipart/alternative.
+	(let ((quotable-parts (notmuch-mua-get-quotable-parts (cdr (assq 'body original)))))
+	  (mapc (lambda (part)
+		  (insert (notmuch-mua-get-displayed-part part query-string)))
+		quotable-parts))
+
+	(push-mark)
+	(goto-char start)
+	;; Quote the original message according to the user's configured style.
+	(message-cite-original))))
 
+  (push-mark)
   (message-goto-body)
   ;; Original message may contain (malicious) MML tags.  We must
   ;; properly quote them in the reply.  Note that using `point-max'
   ;; instead of `mark' here is wrong.  The buffer may include user's
   ;; signature which should not be MML-quoted.
-  (mml-quote-region (point) (mark)))
+  (mml-quote-region (point) (mark))
+  (set-buffer-modified-p nil))
 
 (defun notmuch-mua-forward-message ()
   (message-forward)
@@ -147,7 +200,7 @@ OTHER-ARGS are passed through to `message-mail'."
       (when (not (string= "" user-agent))
 	(push (cons "User-Agent" user-agent) other-headers))))
 
-  (unless (mail-header 'from other-headers)
+  (unless (mail-header 'From other-headers)
     (push (cons "From" (concat
 			(notmuch-user-name) " <" (notmuch-user-primary-email) ">")) other-headers))
 
@@ -210,7 +263,7 @@ the From: address first."
   (interactive "P")
   (let ((other-headers
 	 (when (or prompt-for-sender notmuch-always-prompt-for-sender)
-	   (list (cons 'from (notmuch-mua-prompt-for-sender))))))
+	   (list (cons 'From (notmuch-mua-prompt-for-sender))))))
     (notmuch-mua-mail nil nil other-headers)))
 
 (defun notmuch-mua-new-forward-message (&optional prompt-for-sender)
diff --git a/test/emacs b/test/emacs
index c3a75e9..a6786d4 100755
--- a/test/emacs
+++ b/test/emacs
@@ -268,13 +268,13 @@ Subject: Re: Testing message sent via SMTP
 In-Reply-To: <XXX>
 Fcc: $(pwd)/mail/sent
 --text follows this line--
-On 01 Jan 2000 12:00:00 -0000, Notmuch Test Suite <test_suite@notmuchmail.org> wrote:
+Notmuch Test Suite <test_suite@notmuchmail.org> writes:
+
 > This is a test that messages are sent via SMTP
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs to a multipart/mixed message"
-test_subtest_known_broken
 test_emacs '(notmuch-show "id:20091118002059.067214ed@hikari")
 		(notmuch-show-reply)
 		(test-output)'
@@ -334,7 +334,6 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Reply within emacs to a multipart/alternative message"
-test_subtest_known_broken
 test_emacs '(notmuch-show "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com")
 		(notmuch-show-reply)
 		(test-output)'
@@ -385,7 +384,8 @@ Subject: Re: Quote MML tags in reply
 In-Reply-To: <test-emacs-mml-quoting@message.id>
 Fcc: ${MAIL_DIR}/sent
 --text follows this line--
-On Fri, 05 Jan 2001 15:43:57 +0000, Notmuch Test Suite <test_suite@notmuchmail.org> wrote:
+Notmuch Test Suite <test_suite@notmuchmail.org> writes:
+
 > <#!part disposition=inline>
 EOF
 test_expect_equal_file OUTPUT EXPECTED
-- 
1.7.5.4

Re: [PATCH v5.2 7/7] emacs: Use the new JSON reply format and message-cite-original

[Austin Clements]

Quoth Adam Wolfe Gordon on Feb 15 at  8:12 pm:
> Use the new JSON reply format to create replies in emacs. Quote HTML
> parts nicely by using mm-display-part to turn them into displayable
> text, then quoting them with message-cite-original. This is very
> useful for users who regularly receive HTML-only email.
> 
> Use message-mode's message-cite-original function to create the
> quoted body for reply messages. In order to make this act like the
> existing notmuch defaults, you will need to set the following in
> your emacs configuration:
> 
> message-citation-line-format "On %a, %d %b %Y, %f wrote:"
> message-citation-line-function 'message-insert-formatted-citation-line
> 
> The tests have been updated to reflect the (ugly) emacs default.

One general comment that affects a lot of things in this patch: I
think you should use the same JSON parsing settings that
notmuch-query-get-threads uses.  Besides consistency and more
opportunities for code reuse, using lists instead of vectors for JSON
arrays will simplify a lot of this code without any drawbacks.

> ---
>  emacs/notmuch-lib.el |    6 ++
>  emacs/notmuch-mua.el |  127 +++++++++++++++++++++++++++++++++++---------------
>  test/emacs           |    8 ++--
>  3 files changed, 100 insertions(+), 41 deletions(-)
> 
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 7e3f110..3fc7aff 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -206,6 +206,12 @@ the user hasn't set this variable with the old or new value."
>  	  (setq seq (nconc (delete elem seq) (list elem))))))
>      seq))
>  
> +(defun notmuch-parts-filter-by-type (parts type)
> +  "Given a vector of message parts, return a vector containing the ones matching the given type."

Wrap at 72.

> +  (loop for part across parts
> +	if (notmuch-match-content-type (cdr (assq 'content-type part)) type)
> +	vconcat (list part)))

With lists, (and since we've decided it's okay to use cl):

  (remove-if-not
   (lambda (part) (notmuch-match-content-type (cdr (assq 'content-type part)) type))
   parts)

> +
>  ;; Compatibility functions for versions of emacs before emacs 23.
>  ;;
>  ;; Both functions here were copied from emacs 23 with the following copyright:
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 4be7c13..7d43821 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -19,11 +19,15 @@
>  ;;
>  ;; Authors: David Edmondson <dme@dme.org>
>  
> +(require 'json)
>  (require 'message)
> +(require 'format-spec)
>  
>  (require 'notmuch-lib)
>  (require 'notmuch-address)
>  
> +(eval-when-compile (require 'cl))
> +
>  ;;
>  
>  (defcustom notmuch-mua-send-hook '(notmuch-mua-message-send-hook)
> @@ -72,56 +76,105 @@ list."
>  	    (push header message-hidden-headers)))
>  	notmuch-mua-hidden-headers))
>  
> +(defun notmuch-mua-get-displayed-part (part query-string)
> +  (with-temp-buffer
> +    (if (assq 'content part)
> +	(insert (cdr (assq 'content part)))
> +      (call-process notmuch-command nil t nil "show" "--format=raw"
> +		    (format "--part=%s" (cdr (assq 'id part)))
> +		    query-string))
> +
> +    (let ((handle (mm-make-handle (current-buffer) (list (cdr (assq 'content-type part)))))
> +	  (end-of-orig (point-max)))
> +      (mm-display-part handle)
> +      (delete-region (point-min) end-of-orig)
> +      (buffer-substring (point-min) (point-max)))))

One of the biggest wins of using consistent JSON parsing settings is
that this can be replaced with notmuch-show-mm-display-part-inline,
which, as far as I can tell, accomplishes the same thing, but handles
a lot of corner-cases that this doesn't (like crypto and charset
conversion).

> +
> +(defun notmuch-mua-multipart/*-to-list (parts)

This name isn't particularly informative to me (though, for reasons
below, I don't think this even needs to be a function).

> +  (loop for part across parts
> +	collect (cdr (assq 'content-type part))))

With lists,
  (map (lambda (part) (cdr (assq 'content-type part))) parts)

Actually, with lists and plists,
  (map (lambda (part) (plist-get part 'content-type)) parts)
which I think is short enough and self-explanatory enough that it
doesn't even need to go in a function.

> +
> +(defun notmuch-mua-get-quotable-parts (parts)
> +  (loop for part across parts
> +	if (notmuch-match-content-type (cdr (assq 'content-type part)) "multipart/alternative")
> +	  append (let* ((subparts (cdr (assq 'content part)))
> +			(types (notmuch-mua-multipart/*-to-list subparts))
> +			(chosen-type (car (notmuch-multipart/alternative-choose types))))
> +		   (notmuch-mua-get-quotable-parts (notmuch-parts-filter-by-type subparts chosen-type)))

This seems roundabout.  The point of multipart/alternative is that the
subparts are equivalent representations provided in order of
preference by the sender and that the client is supposed to choose
*one* of the alternates.  Even if multiple subparts have the same
content-type, they're still alternates, so we should insert only one
of them (and, since content-type is our only criteria for choosing
between alternates, we should use the last one of acceptable type,
since it was considered more preferential by the sender).

> +	else if (notmuch-match-content-type (cdr (assq 'content-type part)) "multipart/*")
> +	  append (notmuch-mua-get-quotable-parts (cdr (assq 'content part)))
> +	else if (notmuch-match-content-type (cdr (assq 'content-type part)) "text/*")
> +	  collect part))
> +
>  (defun notmuch-mua-reply (query-string &optional sender reply-all)
> -  (let (headers
> -	body
> -	(args '("reply")))
> -    (if notmuch-show-process-crypto
> -	(setq args (append args '("--decrypt"))))
> +  (let ((args '("reply" "--format=json"))
> +	reply
> +	original)
> +    (when notmuch-show-process-crypto
> +      (setq args (append args '("--decrypt"))))

No need to change the last two lines above (though there's obviously
no harm in doing so).

> +
>      (if reply-all
>  	(setq args (append args '("--reply-to=all")))
>        (setq args (append args '("--reply-to=sender"))))
>      (setq args (append args (list query-string)))
> -    ;; This make assumptions about the output of `notmuch reply', but
> -    ;; really only that the headers come first followed by a blank
> -    ;; line and then the body.
> +
> +    ;; Get the reply object as JSON, and parse it into an elisp object.
>      (with-temp-buffer
>        (apply 'call-process (append (list notmuch-command nil (list t t) nil) args))
>        (goto-char (point-min))
> -      (if (re-search-forward "^$" nil t)
> -	  (save-excursion
> -	    (save-restriction
> -	      (narrow-to-region (point-min) (point))
> -	      (goto-char (point-min))
> -	      (setq headers (mail-header-extract)))))
> -      (forward-line 1)
> -      (setq body (buffer-substring (point) (point-max))))
> -    ;; If sender is non-nil, set the From: header to its value.
> -    (when sender
> -      (mail-header-set 'from sender headers))
> -    (let
> -	;; Overlay the composition window on that being used to read
> -	;; the original message.
> -	((same-window-regexps '("\\*mail .*")))
> -      (notmuch-mua-mail (mail-header 'to headers)
> -			(mail-header 'subject headers)
> -			(message-headers-to-generate headers t '(to subject))))
> -    ;; insert the message body - but put it in front of the signature
> -    ;; if one is present
> -    (goto-char (point-max))
> -    (if (re-search-backward message-signature-separator nil t)
> +      (setq reply (json-read)))
> +
> +    ;; Extract the original message to simplify the following code.
> +    (setq original (cdr (assq 'original reply)))
> +
> +    ;; Extract the headers of both the reply and the original message.
> +    (let* ((original-headers (cdr (assq 'headers original)))
> +	   (reply-headers (cdr (assq 'reply-headers reply))))

This is the one place where using the JSON parsing settings from
notmuch-query-get-threads is slightly annoying, since the mail-*
functions expect alists.  

OTOH, the mail-* functions seem kind of pointless here; plist-set
could replace mail-header-set and plist-get could replace mail-header.
The only non-trivial function that expects an alist is
message-headers-to-generate (and, by extension, notmuch-mua-mail).

> +
> +      ;; If sender is non-nil, set the From: header to its value.
> +      (when sender
> +	(mail-header-set 'From sender reply-headers))
> +      (let
> +	  ;; Overlay the composition window on that being used to read
> +	  ;; the original message.
> +	  ((same-window-regexps '("\\*mail .*")))
> +	(notmuch-mua-mail (mail-header 'To reply-headers)
> +			  (mail-header 'Subject reply-headers)
> +			  (message-headers-to-generate reply-headers t '(To Subject))))
> +      ;; Insert the message body - but put it in front of the signature
> +      ;; if one is present
> +      (goto-char (point-max))
> +      (if (re-search-backward message-signature-separator nil t)
>  	  (forward-line -1)
> -      (goto-char (point-max)))
> -    (insert body)
> -    (push-mark))
> -  (set-buffer-modified-p nil)
> +	(goto-char (point-max)))
> +
> +      (let ((from (cdr (assq 'From original-headers)))
> +	    (date (cdr (assq 'Date original-headers)))
> +	    (start (point)))
> +
> +	(insert "From: " from "\n")
> +	(insert "Date: " date "\n\n")

Sorry; I'm having trouble following the diff.  What are the inserts
for?

> +
> +	;; Get the parts of the original message that should be quoted; this includes
> +	;; all the text parts, except the non-preferred ones in a multipart/alternative.
> +	(let ((quotable-parts (notmuch-mua-get-quotable-parts (cdr (assq 'body original)))))
> +	  (mapc (lambda (part)
> +		  (insert (notmuch-mua-get-displayed-part part query-string)))
> +		quotable-parts))

Alternatively, notmuch-mua-get-quotable-parts could be
notmuch-mua-insert-quotable-parts, which would probably simplify
things a bit.  Your call.

> +
> +	(push-mark)

It's unfortunate that message-cite-original depends on the mark.
Since you're about to push the mark for the user anyway, maybe this
should be a set-mark so that only one mark gets pushed?

> +	(goto-char start)
> +	;; Quote the original message according to the user's configured style.
> +	(message-cite-original))))

message-cite-original-without-signature?

>  
> +  (push-mark)

Is message-cite-original guaranteed to leave point in a reasonable
place for this or should we create our own marker above (probably
after the if re-search-backward..) and use it here to get point to the
right place?

>    (message-goto-body)
>    ;; Original message may contain (malicious) MML tags.  We must
>    ;; properly quote them in the reply.  Note that using `point-max'
>    ;; instead of `mark' here is wrong.  The buffer may include user's
>    ;; signature which should not be MML-quoted.
> -  (mml-quote-region (point) (mark)))
> +  (mml-quote-region (point) (mark))
> +  (set-buffer-modified-p nil))
>  
>  (defun notmuch-mua-forward-message ()
>    (message-forward)
> @@ -147,7 +200,7 @@ OTHER-ARGS are passed through to `message-mail'."
>        (when (not (string= "" user-agent))
>  	(push (cons "User-Agent" user-agent) other-headers))))
>  
> -  (unless (mail-header 'from other-headers)
> +  (unless (mail-header 'From other-headers)
>      (push (cons "From" (concat
>  			(notmuch-user-name) " <" (notmuch-user-primary-email) ">")) other-headers))
>  
> @@ -210,7 +263,7 @@ the From: address first."
>    (interactive "P")
>    (let ((other-headers
>  	 (when (or prompt-for-sender notmuch-always-prompt-for-sender)
> -	   (list (cons 'from (notmuch-mua-prompt-for-sender))))))
> +	   (list (cons 'From (notmuch-mua-prompt-for-sender))))))
>      (notmuch-mua-mail nil nil other-headers)))
>  
>  (defun notmuch-mua-new-forward-message (&optional prompt-for-sender)
> diff --git a/test/emacs b/test/emacs
> index c3a75e9..a6786d4 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -268,13 +268,13 @@ Subject: Re: Testing message sent via SMTP
>  In-Reply-To: <XXX>
>  Fcc: $(pwd)/mail/sent
>  --text follows this line--
> -On 01 Jan 2000 12:00:00 -0000, Notmuch Test Suite <test_suite@notmuchmail.org> wrote:
> +Notmuch Test Suite <test_suite@notmuchmail.org> writes:
> +
>  > This is a test that messages are sent via SMTP
>  EOF
>  test_expect_equal_file OUTPUT EXPECTED
>  
>  test_begin_subtest "Reply within emacs to a multipart/mixed message"
> -test_subtest_known_broken
>  test_emacs '(notmuch-show "id:20091118002059.067214ed@hikari")
>  		(notmuch-show-reply)
>  		(test-output)'
> @@ -334,7 +334,6 @@ EOF
>  test_expect_equal_file OUTPUT EXPECTED
>  
>  test_begin_subtest "Reply within emacs to a multipart/alternative message"
> -test_subtest_known_broken
>  test_emacs '(notmuch-show "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com")
>  		(notmuch-show-reply)
>  		(test-output)'
> @@ -385,7 +384,8 @@ Subject: Re: Quote MML tags in reply
>  In-Reply-To: <test-emacs-mml-quoting@message.id>
>  Fcc: ${MAIL_DIR}/sent
>  --text follows this line--
> -On Fri, 05 Jan 2001 15:43:57 +0000, Notmuch Test Suite <test_suite@notmuchmail.org> wrote:
> +Notmuch Test Suite <test_suite@notmuchmail.org> writes:
> +
>  > <#!part disposition=inline>
>  EOF
>  test_expect_equal_file OUTPUT EXPECTED

Re: [PATCH v5.2 7/7] emacs: Use the new JSON reply format and message-cite-original

[Adam Wolfe Gordon]

On Fri, Feb 17, 2012 at 13:00, Austin Clements <amdragon@mit.edu> wrote:
> One general comment that affects a lot of things in this patch: I
> think you should use the same JSON parsing settings that
> notmuch-query-get-threads uses.  Besides consistency and more
> opportunities for code reuse, using lists instead of vectors for JSON
> arrays will simplify a lot of this code without any drawbacks.

I pretty much agree. The only reason I stuck with alists was, as you
mention below, to be compatible with certain mail functions. Given the
things you've pointed out, I think the small hassle of making those
work with plists is worthwhile, so I'll give it a go.

Clarification on a couple of things follow, otherwise I'll make all
these changes for the next version.

>> +     (goto-char (point-max)))
>> +
>> +      (let ((from (cdr (assq 'From original-headers)))
>> +         (date (cdr (assq 'Date original-headers)))
>> +         (start (point)))
>> +
>> +     (insert "From: " from "\n")
>> +     (insert "Date: " date "\n\n")
>
> Sorry; I'm having trouble following the diff.  What are the inserts
> for?

The function message-cite-original cites an original message, which is
in the marked region. It assumes the headers of the original message
will be part of the marked region, but the only ones it actually uses
are From and Date.

This could probably use a comment in the code.

>> +     (push-mark)
>
> It's unfortunate that message-cite-original depends on the mark.
> Since you're about to push the mark for the user anyway, maybe this
> should be a set-mark so that only one mark gets pushed?

Probably the right thing to do.

>> +     (goto-char start)
>> +     ;; Quote the original message according to the user's configured style.
>> +     (message-cite-original))))
>
> message-cite-original-without-signature?

Perhaps it should be configurable (notmuch-reply-cite-function or
somesuch). I believe message-cite-original matches the behavior of the
old reply, which didn't strip signatures, but I don't have strong
feelings either way.

>> +  (push-mark)
>
> Is message-cite-original guaranteed to leave point in a reasonable
> place for this or should we create our own marker above (probably
> after the if re-search-backward..) and use it here to get point to the
> right place?

In my experience, it leaves the point at the end of the quoted region,
but the documentation doesn't make any guarantees. Probably safer to
set a marker.

Re: [PATCH v5.2 7/7] emacs: Use the new JSON reply format and message-cite-original

[Austin Clements]

Quoth Adam Wolfe Gordon on Feb 17 at  7:22 pm:
> On Fri, Feb 17, 2012 at 13:00, Austin Clements <amdragon@mit.edu> wrote:
> > One general comment that affects a lot of things in this patch: I
> > think you should use the same JSON parsing settings that
> > notmuch-query-get-threads uses.  Besides consistency and more
> > opportunities for code reuse, using lists instead of vectors for JSON
> > arrays will simplify a lot of this code without any drawbacks.
> 
> I pretty much agree. The only reason I stuck with alists was, as you
> mention below, to be compatible with certain mail functions. Given the
> things you've pointed out, I think the small hassle of making those
> work with plists is worthwhile, so I'll give it a go.
> 
> Clarification on a couple of things follow, otherwise I'll make all
> these changes for the next version.
> 
> >> +     (goto-char (point-max)))
> >> +
> >> +      (let ((from (cdr (assq 'From original-headers)))
> >> +         (date (cdr (assq 'Date original-headers)))
> >> +         (start (point)))
> >> +
> >> +     (insert "From: " from "\n")
> >> +     (insert "Date: " date "\n\n")
> >
> > Sorry; I'm having trouble following the diff.  What are the inserts
> > for?
> 
> The function message-cite-original cites an original message, which is
> in the marked region. It assumes the headers of the original message
> will be part of the marked region, but the only ones it actually uses
> are From and Date.
> 
> This could probably use a comment in the code.

Ah, okay.  Is this how it generates the citation line?

Could definitely do with a comment.

> >> +     (push-mark)
> >
> > It's unfortunate that message-cite-original depends on the mark.
> > Since you're about to push the mark for the user anyway, maybe this
> > should be a set-mark so that only one mark gets pushed?
> 
> Probably the right thing to do.
> 
> >> +     (goto-char start)
> >> +     ;; Quote the original message according to the user's configured style.
> >> +     (message-cite-original))))
> >
> > message-cite-original-without-signature?
> 
> Perhaps it should be configurable (notmuch-reply-cite-function or
> somesuch). I believe message-cite-original matches the behavior of the
> old reply, which didn't strip signatures, but I don't have strong
> feelings either way.

We should probably stick with the original behavior, at least for now.

I just noticed that message-cite-original calls mml-quote-region.  How
is it that we don't wind up double-quoting MML tags with this change?

Re: [PATCH v5.2 7/7] emacs: Use the new JSON reply format and message-cite-original

[Adam Wolfe Gordon]

Hi Austin,

A couple of quick notes below, just before I send the new version:

On Fri, Feb 17, 2012 at 13:00, Austin Clements <amdragon@mit.edu> wrote:
> One of the biggest wins of using consistent JSON parsing settings is
> that this can be replaced with notmuch-show-mm-display-part-inline,
> which, as far as I can tell, accomplishes the same thing, but handles
> a lot of corner-cases that this doesn't (like crypto and charset
> conversion).

This would be very desirable indeed, but after taking a stab at it,
I've discovered it's a really significant amount of work. The show
code relies on a bunch of buffer-local variables regarding crypto and
such, which are tricky to factor out into notmuch-lib.

Do you think it's acceptable to leave this as future work?

> Alternatively, notmuch-mua-get-quotable-parts could be
> notmuch-mua-insert-quotable-parts, which would probably simplify
> things a bit.  Your call.

I've decided to leave this as get-quotable-parts. This is mostly my
personal preference to have a single function do the insertions when
possible, which I think makes the flow more obvious.

> Is message-cite-original guaranteed to leave point in a reasonable
> place for this or should we create our own marker above (probably
> after the if re-search-backward..) and use it here to get point to the
> right place?

It turns out it actually wasn't, which is why MML tags weren't getting
escaped twice. I've fixed this.

Re: [PATCH v5.2 7/7] emacs: Use the new JSON reply format and message-cite-original

[Austin Clements]

Quoth Adam Wolfe Gordon on Feb 15 at  8:12 pm:
> Use the new JSON reply format to create replies in emacs. Quote HTML
> parts nicely by using mm-display-part to turn them into displayable
> text, then quoting them with message-cite-original. This is very
> useful for users who regularly receive HTML-only email.
> 
> Use message-mode's message-cite-original function to create the
> quoted body for reply messages. In order to make this act like the
> existing notmuch defaults, you will need to set the following in
> your emacs configuration:
> 
> message-citation-line-format "On %a, %d %b %Y, %f wrote:"
> message-citation-line-function 'message-insert-formatted-citation-line

I've been thinking about this more.  message-mode's default citation
line is really unfortunate and quite possibly insane ("writes" isn't
even the right tense and what's up with that extra line break?).  The
option to change this is also well hidden (as an experiment, I tried
navigating to it through customize and couldn't figure out where it
was, even though I knew what I was looking for).  In general, I'm a
fan of inheriting as many options from Emacs as possible, but people
*are* going to ask how to change this and the default setting *is*
going to turn people off of notmuch ("What mail client do you use that
generates those quirky citation lines?"  "I use notmuch!"  "Is that,
like, from the 80's?").

So, what about adding a notmuch customize option for selecting the
citation line format?  It could offer a few const choices, including a
default, sane format, plus the option to enter your own or to fall
back to whatever message-mode is configured to do.  If we do this,
it's probably best done in a follow-up series, but this seemed like an
appropriate place to bring it up.

> The tests have been updated to reflect the (ugly) emacs default.
> ---
>  emacs/notmuch-lib.el |    6 ++
>  emacs/notmuch-mua.el |  127 +++++++++++++++++++++++++++++++++++---------------
>  test/emacs           |    8 ++--
>  3 files changed, 100 insertions(+), 41 deletions(-)

Re: [PATCH v5.2 7/7] emacs: Use the new JSON reply format and message-cite-original

[Adam Wolfe Gordon]

On Mon, Feb 20, 2012 at 22:59, Austin Clements <amdragon@mit.edu> wrote:
> I've been thinking about this more.  message-mode's default citation
> line is really unfortunate and quite possibly insane ("writes" isn't
> even the right tense and what's up with that extra line break?).  The
> option to change this is also well hidden (as an experiment, I tried
> navigating to it through customize and couldn't figure out where it
> was, even though I knew what I was looking for).  In general, I'm a
> fan of inheriting as many options from Emacs as possible, but people
> *are* going to ask how to change this and the default setting *is*
> going to turn people off of notmuch ("What mail client do you use that
> generates those quirky citation lines?"  "I use notmuch!"  "Is that,
> like, from the 80's?").

Agreed. It's a really unfortunate default.

> So, what about adding a notmuch customize option for selecting the
> citation line format?  It could offer a few const choices, including a
> default, sane format, plus the option to enter your own or to fall
> back to whatever message-mode is configured to do.  If we do this,
> it's probably best done in a follow-up series, but this seemed like an
> appropriate place to bring it up.

I think there are two options, which have been discussed a bit before [1]:

1) Wrap the citation format with a notmuch customization variable,
notmuch-citation-line-format or somesuch. Then set the
message-citation-line-format before calling message-cite-original.

2) Have notmuch load a user config file (~/.notmuch.el or something)
on startup, and provide a default file that sets nice defaults for
things like message-citation-line-format. The default file could even
be constructed on first run, such that if the user has already
customized some things (like message-citation-line-format) we can keep
their settings.

Option 2 is obviously more work, but I think it's the right way to go,
at least in the long run. In addition to giving a place to provide
nice defaults for non-notmuch variables, it gives the user a nice
place to specify notmuch-specific config. For example, I use
completely separate init files for notmuch and other emacs usage, and
having a notmuch config file would let me get away from this slightly
kludgey setup.

In either case, this can probably come as a separate patch series, but
it is good to start discussing it here.

[1] id:"m2mx9i3onw.fsf@wal122.wireless-pennnet.upenn.edu"

Re: [PATCH v5.2 7/7] emacs: Use the new JSON reply format and message-cite-original

[Austin Clements]

Quoth Adam Wolfe Gordon on Feb 21 at  9:49 am:
> On Mon, Feb 20, 2012 at 22:59, Austin Clements <amdragon@mit.edu> wrote:
> > I've been thinking about this more.  message-mode's default citation
> > line is really unfortunate and quite possibly insane ("writes" isn't
> > even the right tense and what's up with that extra line break?).  The
> > option to change this is also well hidden (as an experiment, I tried
> > navigating to it through customize and couldn't figure out where it
> > was, even though I knew what I was looking for).  In general, I'm a
> > fan of inheriting as many options from Emacs as possible, but people
> > *are* going to ask how to change this and the default setting *is*
> > going to turn people off of notmuch ("What mail client do you use that
> > generates those quirky citation lines?"  "I use notmuch!"  "Is that,
> > like, from the 80's?").
> 
> Agreed. It's a really unfortunate default.
> 
> > So, what about adding a notmuch customize option for selecting the
> > citation line format?  It could offer a few const choices, including a
> > default, sane format, plus the option to enter your own or to fall
> > back to whatever message-mode is configured to do.  If we do this,
> > it's probably best done in a follow-up series, but this seemed like an
> > appropriate place to bring it up.
> 
> I think there are two options, which have been discussed a bit before [1]:

Ah, interesting.  I hadn't been following this series at that point.

> 1) Wrap the citation format with a notmuch customization variable,
> notmuch-citation-line-format or somesuch. Then set the
> message-citation-line-format before calling message-cite-original.
> 
> 2) Have notmuch load a user config file (~/.notmuch.el or something)
> on startup, and provide a default file that sets nice defaults for
> things like message-citation-line-format. The default file could even
> be constructed on first run, such that if the user has already
> customized some things (like message-citation-line-format) we can keep
> their settings.
> 
> Option 2 is obviously more work, but I think it's the right way to go,
> at least in the long run. In addition to giving a place to provide
> nice defaults for non-notmuch variables, it gives the user a nice
> place to specify notmuch-specific config. For example, I use
> completely separate init files for notmuch and other emacs usage, and
> having a notmuch config file would let me get away from this slightly
> kludgey setup.

That is an intriguing idea.  My main concern would be
forwards-compatibility.  When we find some other variable that we want
to add to this file, what do we do with already-generated
.notmuch.els?

I wouldn't dismiss option 1, though.  From a user's perspective, it is
visible that they're using message-mode to edit message drafts and
hence natural that they would customize message-mode to control
editing behavior, but they create a reply by pressing 'r' in a
*notmuch* buffer, which naturally associates the operation with
notmuch, not message-mode.  I would go so far as to say reply's use of
message-mode is an implementation detail.  Hence, I would never expect
an uninformed user to guess that they have to go to message-mode to
customize the citation format.

> In either case, this can probably come as a separate patch series, but
> it is good to start discussing it here.
> 
> [1] id:"m2mx9i3onw.fsf@wal122.wireless-pennnet.upenn.edu"