Date: Fri, 17 Feb 2012 12:04:57 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Adam Wolfe Gordon <awg+notmuch@xvx.ca>
Subject: Re: [PATCH v5.2 3/7] reply: Add a JSON reply format.
Message-ID: <20120217170457.GE5991@mit.edu>
References: <1329361957-28493-1-git-send-email-awg+notmuch@xvx.ca>
	<1329361957-28493-4-git-send-email-awg+notmuch@xvx.ca>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1329361957-28493-4-git-send-email-awg+notmuch@xvx.ca>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

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