From: Jani Nikula <jani@nikula.org>
To: Adam Wolfe Gordon <awg+notmuch@xvx.ca>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 2/4] reply: Add a JSON reply format.
In-Reply-To: <1326737603-21166-3-git-send-email-awg+notmuch@xvx.ca>
References: <1326737603-21166-1-git-send-email-awg+notmuch@xvx.ca>
	<1326737603-21166-3-git-send-email-awg+notmuch@xvx.ca>
User-Agent: Notmuch/0.11+76~g1de742d (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Thu, 19 Jan 2012 01:07:02 +0200
Message-ID: <87sjjcbofd.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Mon, 16 Jan 2012 11:13:21 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> This new JSON format for replies includes headers generated for a reply
> message as well as the headers and all text parts 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 using w3m.

Hi, admittedly not a very thorough review, but please find a couple of
comments below.

BR,
Jani.


> ---
>  notmuch-reply.c |  313 ++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 253 insertions(+), 60 deletions(-)
> 
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index da3acce..f5a5dcf 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -30,6 +30,15 @@ reply_headers_message_part (GMimeMessage *message);
>  static void
>  reply_part_content (GMimeObject *part);
>  
> +static void
> +reply_part_start_json (GMimeObject *part, int *part_count);
> +
> +static void
> +reply_part_content_json (GMimeObject *part);
> +
> +static void
> +reply_part_end_json (GMimeObject *part);
> +
>  static const notmuch_show_format_t format_reply = {
>      "",
>  	"", NULL,
> @@ -46,6 +55,22 @@ static const notmuch_show_format_t format_reply = {
>      ""
>  };
>  
> +static const notmuch_show_format_t format_json = {
> +    "",
> +	"", NULL,
> +	    "", NULL, NULL, "",
> +	    "",
> +	        reply_part_start_json,
> +	        NULL,
> +	        NULL,
> +	        reply_part_content_json,
> +	        reply_part_end_json,
> +	        "",
> +	    "",
> +	"", "",
> +    ""
> +};
> +
>  static void
>  show_reply_headers (GMimeMessage *message)
>  {
> @@ -54,14 +79,14 @@ show_reply_headers (GMimeMessage *message)
>      stream_stdout = g_mime_stream_file_new (stdout);
>      if (stream_stdout) {
>  	g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
> -	stream_filter = g_mime_stream_filter_new(stream_stdout);
> +	stream_filter = g_mime_stream_filter_new (stream_stdout);
>  	if (stream_filter) {
> -		g_mime_stream_filter_add(GMIME_STREAM_FILTER(stream_filter),
> -					 g_mime_filter_headers_new());
> -		g_mime_object_write_to_stream(GMIME_OBJECT(message), stream_filter);
> -		g_object_unref(stream_filter);
> +		g_mime_stream_filter_add (GMIME_STREAM_FILTER(stream_filter),
> +					  g_mime_filter_headers_new());
> +		g_mime_object_write_to_stream (GMIME_OBJECT(message), stream_filter);
> +		g_object_unref (stream_filter);
>  	}
> -	g_object_unref(stream_stdout);
> +	g_object_unref (stream_stdout);

I know I asked you to adhere to notmuch coding style like above, but I
meant in the context of your patch, not elsewhere. Cleanups like this
should really be separate patches. Sorry if I was ambiguous.

>      }
>  }
>  
> @@ -86,6 +111,17 @@ reply_headers_message_part (GMimeMessage *message)
>      printf ("> Date: %s\n", g_mime_message_get_date_as_string (message));
>  }
>  
> +static notmuch_bool_t
> +reply_check_part_type (GMimeObject *part, const char *type, const char *subtype,
> +		       const char *disposition)
> +{
> +    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
> +    GMimeContentDisposition *part_disposition = g_mime_object_get_content_disposition (part);
> +
> +    return (g_mime_content_type_is_type (content_type, type, subtype) &&
> +	    (!part_disposition ||
> +	     strcmp (part_disposition->disposition, disposition) == 0));
> +}
>  
>  static void
>  reply_part_content (GMimeObject *part)
> @@ -108,32 +144,29 @@ reply_part_content (GMimeObject *part)
>      {
>  	GMimeStream *stream_stdout = NULL, *stream_filter = NULL;
>  	GMimeDataWrapper *wrapper;
> -	const char *charset;
> -
> -	charset = g_mime_object_get_content_type_parameter (part, "charset");
>  	stream_stdout = g_mime_stream_file_new (stdout);
>  	if (stream_stdout) {
>  	    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
> -	    stream_filter = g_mime_stream_filter_new(stream_stdout);
> -	    if (charset) {
> -		g_mime_stream_filter_add(GMIME_STREAM_FILTER(stream_filter),
> -					 g_mime_filter_charset_new(charset, "UTF-8"));
> -	    }
> +	    stream_filter = g_mime_stream_filter_new (stream_stdout);
> +
> +	    const char *charset = g_mime_object_get_content_type_parameter (part, "charset");
> +	    if (charset)
> +		g_mime_stream_filter_add(GMIME_STREAM_FILTER (stream_filter),
> +					 g_mime_filter_charset_new (charset, "UTF-8"));
>  	}
> -	g_mime_stream_filter_add(GMIME_STREAM_FILTER(stream_filter),
> -				 g_mime_filter_reply_new(TRUE));
> +	g_mime_stream_filter_add (GMIME_STREAM_FILTER (stream_filter),
> +				  g_mime_filter_reply_new (TRUE));
>  	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);
> +	    g_object_unref (stream_filter);
>  	if (stream_stdout)
> -	    g_object_unref(stream_stdout);
> +	    g_object_unref (stream_stdout);
>      }
>      else
>      {
> -	if (disposition &&
> -	    strcmp (disposition->disposition, GMIME_DISPOSITION_ATTACHMENT) == 0)
> +	if (disposition && strcmp (disposition->disposition, GMIME_DISPOSITION_ATTACHMENT) == 0)

This is also a change not related to your patch.

>  	{
>  	    const char *filename = g_mime_part_get_filename (GMIME_PART (part));
>  	    printf ("Attachment: %s (%s)\n", filename,
> @@ -147,6 +180,67 @@ reply_part_content (GMimeObject *part)
>      }
>  }
>  
> +static void
> +reply_part_start_json (GMimeObject *part, unused (int *part_count))
> +{
> +    if (reply_check_part_type (part, "text", "*", GMIME_DISPOSITION_INLINE))
> +	printf ("{ ");
> +}
> +
> +static void
> +reply_part_end_json (GMimeObject *part)
> +{
> +    if (reply_check_part_type (part, "text", "*", GMIME_DISPOSITION_INLINE))
> +	printf ("}, ");
> +}
> +
> +static void
> +reply_part_content_json (GMimeObject *part)
> +{
> +    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
> +    void *ctx = talloc_new (NULL);
> +
> +    /* We only care about inline text parts for reply purposes */
> +    if (reply_check_part_type (part, "text", "*", GMIME_DISPOSITION_INLINE))
> +    {

The style in notmuch is to put the opening brace in the end of the if
line. Same applies below.

> +	GMimeDataWrapper *wrapper;
> +	GByteArray *part_content;
> +
> +	printf ("\"content-type\": %s, \"content\": ",
> +	       json_quote_str (ctx, g_mime_content_type_to_string (content_type)));
> +
> +	wrapper = g_mime_part_get_content_object (GMIME_PART (part));
> +	if (wrapper)
> +	{
> +	    const char *charset = g_mime_object_get_content_type_parameter (part, "charset");
> +	    GMimeStream *stream_memory = g_mime_stream_mem_new ();
> +	    if (stream_memory) {
> +		GMimeStream *stream_filter = NULL;
> +		stream_filter = g_mime_stream_filter_new (stream_memory);
> +		if (charset) {
> +		    g_mime_stream_filter_add (GMIME_STREAM_FILTER (stream_filter),
> +					      g_mime_filter_charset_new (charset, "UTF-8"));
> +		}
> +
> +		if (stream_filter)
> +		{
> +		    g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
> +		    part_content = g_mime_stream_mem_get_byte_array (GMIME_STREAM_MEM (stream_memory));
> +
> +		    printf ("%s", json_quote_chararray (ctx, (char *) part_content->data, part_content->len));
> +		}
> +		if (stream_filter)
> +		    g_object_unref (stream_filter);
> +	    }
> +
> +	    if (stream_memory)
> +		g_object_unref (stream_memory);
> +	}
> +    }
> +
> +    talloc_free (ctx);
> +}
> +
>  /* Is the given address configured as one of the user's "personal" or
>   * "other" addresses. */
>  static int
> @@ -505,6 +599,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 +664,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,62 +672,104 @@ 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;
> -	}
> +	reply = create_reply_message (ctx, config, message, reply_all);
>  
> -	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);
> -	}
> +	if (!reply)
> +	    continue;
>  
> -	from_addr = add_recipients_from_message (reply, config, message,
> -						 reply_all);
> +	show_reply_headers (reply);
>  
> -	if (from_addr == NULL)
> -	    from_addr = guess_from_received_header (config, message);
> +	g_object_unref (G_OBJECT (reply));
> +	reply = NULL;
>  
> -	if (from_addr == NULL)
> -	    from_addr = notmuch_config_get_user_primary_email (config);
> +	printf ("On %s, %s wrote:\n",
> +		notmuch_message_get_header (message, "date"),
> +		notmuch_message_get_header (message, "from"));
>  
> -	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);
> +	show_message_body (message, format, params);
>  
> -	in_reply_to = talloc_asprintf (ctx, "<%s>",
> -			     notmuch_message_get_message_id (message));
> +	notmuch_message_destroy (message);
> +    }
> +    return 0;
> +}
>  
> -	g_mime_object_set_header (GMIME_OBJECT (reply),
> -				  "In-Reply-To", in_reply_to);
> +static int
> +notmuch_reply_format_json(void *ctx,
> +			  notmuch_config_t *config,
> +			  notmuch_query_t *query,
> +			  unused (notmuch_show_params_t *params),
> +			  notmuch_bool_t reply_all)
> +{
> +    GMimeMessage *reply;
> +    notmuch_messages_t *messages;
> +    notmuch_message_t *message;
> +    const notmuch_show_format_t *format = &format_json;
>  
> -	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);
> +    const char *reply_headers[] = {"from", "to", "subject", "in-reply-to", "references"};
> +    const char *orig_headers[] = {"from", "to", "cc", "subject", "date", "in-reply-to", "references"};
> +    unsigned int hidx;
>  
> -	show_reply_headers (reply);
> +    /* Start array of reply objects */
> +    printf ("[");
> +
> +    for (messages = notmuch_query_search_messages (query);
> +	 notmuch_messages_valid (messages);
> +	 notmuch_messages_move_to_next (messages))
> +    {
> +	/* Start a reply object */
> +	printf ("{ \"reply\": { \"headers\": { ");
> +
> +	message = notmuch_messages_get (messages);
> +
> +	reply = create_reply_message (ctx, config, message, reply_all);
> +	if (!reply)
> +	    continue;

If this occurs, it will create broken JSON.

> +
> +	for (hidx = 0; hidx < ARRAY_SIZE (reply_headers); hidx++)
> +	{
> +	    if (hidx)
> +		printf (", ");
> +
> +	    printf ("%s: %s", json_quote_str (ctx, reply_headers[hidx]),
> +		    json_quote_str (ctx, g_mime_object_get_header (GMIME_OBJECT (reply), reply_headers[hidx])));
> +	}
>  
>  	g_object_unref (G_OBJECT (reply));
>  	reply = NULL;
>  
> -	printf ("On %s, %s wrote:\n",
> -		notmuch_message_get_header (message, "date"),
> -		notmuch_message_get_header (message, "from"));
> +	/* Done the headers for the reply, which has no body parts */
> +	printf ("} }");
> +
> +	/* Start the original */
> +	printf (", \"original\": { \"headers\": { ");
> +
> +	for (hidx = 0; hidx < ARRAY_SIZE (orig_headers); hidx++)
> +	{
> +	    if (hidx)
> +		printf (", ");
> +
> +	    printf ("%s: %s", json_quote_str (ctx, orig_headers[hidx]),
> +		    json_quote_str (ctx, notmuch_message_get_header (message, orig_headers[hidx])));
> +	}
> +
> +	/* End headers */
> +	printf (" }, \"body\": [ ");
>  
> +	/* Show body parts */
>  	show_message_body (message, format, params);
>  
>  	notmuch_message_destroy (message);
> +
> +	/* Done the original */
> +	printf ("{} ] }");
> +
> +	/* End the reply object. */
> +	printf (" }, ");
>      }
> +
> +    /* End array of reply objects */
> +    printf ("{} ]\n");
> +
>      return 0;
>  }
>  
> @@ -646,6 +835,7 @@ notmuch_reply_format_headers_only(void *ctx,
>  
>  enum {
>      FORMAT_DEFAULT,
> +    FORMAT_JSON,
>      FORMAT_HEADERS_ONLY,
>  };
>  
> @@ -666,6 +856,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',
> @@ -684,6 +875,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;
>  
> -- 
> 1.7.5.4
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch