From: Jani Nikula <jani@nikula.org>
To: Adam Wolfe Gordon <awg+notmuch@xvx.ca>, notmuch@notmuchmail.org,
 awg@xvx.ca
Subject: Re: [PATCH 2/4] reply: Add a JSON reply format.
In-Reply-To: <1326009162-19524-3-git-send-email-awg+notmuch@xvx.ca>
References: <1326009162-19524-1-git-send-email-awg+notmuch@xvx.ca>
	<1326009162-19524-3-git-send-email-awg+notmuch@xvx.ca>
User-Agent: Notmuch/0.11+59~g83cfe07 (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Sat, 14 Jan 2012 22:58:55 +0200
Message-ID: <87k44uatm8.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Sun,  8 Jan 2012 00:52:40 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> From: Adam Wolfe Gordon <awg@xvx.ca>
> 
> 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 Adam, this is a drive-by-review on some things I spotted, but can't
say I would've thought the whole thing through. I'm pretty ignorant
about MIME parts etc. Please find comments inline.

The reply-to-sender set was just merged. You'll have conflicts both here
and in emacs code, but they should be trivial to sort out.


BR,
Jani.


> ---
>  notmuch-reply.c |  269 +++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 230 insertions(+), 39 deletions(-)
> 
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index f8d5f64..82df396 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);
> +

I know there are existing forward declarations like this, but would it
be much trouble to arrange the code so that they were not needed at all?

>  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)
>  {
> @@ -147,6 +172,78 @@ reply_part_content (GMimeObject *part)
>      }
>  }
>  
> +static void
> +reply_part_start_json (GMimeObject *part, unused(int *part_count))
> +{
> +    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
> +    GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (part);
> +
> +    if (g_mime_content_type_is_type (content_type, "text", "*") &&
> +	(!disposition ||
> +	 strcmp (disposition->disposition, GMIME_DISPOSITION_INLINE) == 0))
> +    {
> +	printf("{ ");
> +    }
> +}
> +
> +static void
> +reply_part_end_json (GMimeObject *part)
> +{
> +    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
> +    GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (part);
> +
> +    if (g_mime_content_type_is_type (content_type, "text", "*") &&
> +	(!disposition ||
> +	 strcmp (disposition->disposition, GMIME_DISPOSITION_INLINE) == 0))
> +	printf ("}, ");
> +}

The two functions above, while small, are almost identical. Please move
the common stuff to a common helper, and you can have something like
this:

	if (the_common_function (part))
		printf ("}, ");

> +
> +static void
> +reply_part_content_json (GMimeObject *part)
> +{
> +    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
> +    GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (part);
> +
> +    void *ctx = talloc_new (NULL);
> +
> +    /* We only care about inline text parts for reply purposes */
> +    if (g_mime_content_type_is_type (content_type, "text", "*") &&
> +	(!disposition ||
> +	 strcmp (disposition->disposition, GMIME_DISPOSITION_INLINE) == 0))

Oh, you can use the common helper here too.

> +    {
> +	GMimeStream *stream_memory = NULL, *stream_filter = NULL;

No need to initialize stream_memory here.

> +	GMimeDataWrapper *wrapper;
> +	GByteArray *part_content;
> +	const char *charset;
> +
> +	printf("\"content-type\": %s, \"content\": ",
> +	       json_quote_str(ctx, g_mime_content_type_to_string(content_type)));

The style in notmuch is to have a space before the opening brace in
function calls. Check elsewhere also. I always forget that too. :)

> +
> +	charset = g_mime_object_get_content_type_parameter (part, "charset");

AFAICT charset declaration and the above call could be moved inside "if
(stream_memory)" below.

> +	stream_memory = g_mime_stream_mem_new ();
> +	if (stream_memory) {
> +	    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"));
> +	    }
> +	}
> +	wrapper = g_mime_part_get_content_object (GMIME_PART (part));
> +	if (wrapper && 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));

You only need charset, stream_memory and stream_filter if wrapper is
true. You could perhaps include all that stuff within "if (wrapper)".

> +
> +	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
> @@ -476,6 +573,59 @@ 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)
> +{
> +    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);
> +
> +    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;
> +}

If you want to, you could make review easier by first having a patch
that abstracts the above in the original code, with no other
modifications. Then you could introduce new functionality that utilizes
it later. But this is no big deal in such a small abstraction.

> +
>  static int
>  notmuch_reply_format_default(void *ctx,
>  			     notmuch_config_t *config,
> @@ -485,8 +635,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);
> @@ -495,61 +643,102 @@ 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);

You should check the return value.

>  
> -	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);
> -	}
> +	show_reply_headers (reply);
>  
> -	from_addr = add_recipients_from_message (reply, config, message);
> +	g_object_unref (G_OBJECT (reply));
> +	reply = NULL;
>  
> -	if (from_addr == NULL)
> -	    from_addr = guess_from_received_header (config, message);
> +	printf ("On %s, %s wrote:\n",
> +		notmuch_message_get_header (message, "date"),
> +		notmuch_message_get_header (message, "from"));
>  
> -	if (from_addr == NULL)
> -	    from_addr = notmuch_config_get_user_primary_email (config);
> +	show_message_body (message, format, params);
>  
> -	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);
> +	notmuch_message_destroy (message);
> +    }
> +    return 0;
> +}
>  
> -	in_reply_to = talloc_asprintf (ctx, "<%s>",
> -			     notmuch_message_get_message_id (message));
> +static int
> +notmuch_reply_format_json(void *ctx,
> +			  notmuch_config_t *config,
> +			  notmuch_query_t *query,
> +			  unused (notmuch_show_params_t *params))
> +{
> +    GMimeMessage *reply;
> +    notmuch_messages_t *messages;
> +    notmuch_message_t *message;
> +    const notmuch_show_format_t *format = &format_json;
>  
> -	g_mime_object_set_header (GMIME_OBJECT (reply),
> -				  "In-Reply-To", in_reply_to);
> +    const char *reply_headers[] = {"from", "to", "subject", "in-reply-to", "references"};
> +    const int n_reply_headers = 5;

Drop this, and use ARRAY_SIZE (reply_headers) instead.

> +    const char *orig_headers[] = {"from", "to", "cc", "subject", "date", "in-reply-to", "references"};
> +    const int n_orig_headers = 7;

Ditto.

> +    int hidx;
>  
> -	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);
> +    /* Start array of reply objects */
> +    printf ("[");
>  
> -	show_reply_headers (reply);
> +    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);

Check return value.

> +
> +	for (hidx = 0; hidx < n_reply_headers; hidx += 1)

hidx++ is the pattern in such for loops.

> +	{
> +	    if (hidx > 0) {
> +		printf (", ");
> +	    }

You could just compare "if (hidx)", and also drop the curly braces.

> +
> +	    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 < n_orig_headers; hidx += 1)

hidx++

> +	{
> +	    if (hidx > 0) {
> +		printf (", ");
> +	    }

Same as above.

> +
> +	    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;
>  }
>  
> @@ -640,6 +829,8 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>  		reply_format_func = notmuch_reply_format_default;
>  	    } else if (strcmp (opt, "headers-only") == 0) {
>  		reply_format_func = notmuch_reply_format_headers_only;
> +	    } else if (strcmp (opt, "json") == 0) {
> +		reply_format_func = notmuch_reply_format_json;
>  	    } else {
>  		fprintf (stderr, "Invalid value for --format: %s\n", opt);
>  		return 1;
> -- 
> 1.7.5.4
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch