From: Jani Nikula <jani@nikula.org>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH 3/3] show: Introduce mime_node formatter callback
In-Reply-To: <1326918507-28033-4-git-send-email-amdragon@mit.edu>
References: <1326918507-28033-1-git-send-email-amdragon@mit.edu>
	<1326918507-28033-4-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.11+76~g1de742d (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Thu, 19 Jan 2012 00:33:44 +0200
Message-ID: <87wr8obpyv.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: dkg@fifthhorseman.net
Precedence: list

On Wed, 18 Jan 2012 15:28:27 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This callback is the gateway to the new mime_node_t-based formatters.
> This maintains backwards compatibility so the formatters can be
> transitioned one at a time.  Once all formatters are converted, the
> formatter structure can be reduced to only message_set_{start,sep,end}
> and part, most of show_message can be deleted, and all of
> show-message.c can be deleted.
> ---
>  notmuch-client.h |    6 ++++++
>  notmuch-reply.c  |    2 +-
>  notmuch-show.c   |   22 ++++++++++++++++++----
>  3 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/notmuch-client.h b/notmuch-client.h
> index b3dcb6b..3ccdfad 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -54,8 +54,14 @@
>  #define STRINGIFY(s) STRINGIFY_(s)
>  #define STRINGIFY_(s) #s
>  
> +struct mime_node;
> +struct notmuch_show_params;
> +
>  typedef struct notmuch_show_format {
>      const char *message_set_start;
> +    void (*part) (const void *ctx,
> +		  struct mime_node *node, int indent,
> +		  struct notmuch_show_params *params);
>      const char *message_start;
>      void (*message) (const void *ctx,
>  		     notmuch_message_t *message,
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 0f682db..9a224e2 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -31,7 +31,7 @@ static void
>  reply_part_content (GMimeObject *part);
>  
>  static const notmuch_show_format_t format_reply = {
> -    "",
> +    "", NULL,
>  	"", NULL,
>  	    "", NULL, reply_headers_message_part, ">\n",
>  	    "",
> diff --git a/notmuch-show.c b/notmuch-show.c
> index ecadfa8..46eef44 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -42,7 +42,7 @@ static void
>  format_part_end_text (GMimeObject *part);
>  
>  static const notmuch_show_format_t format_text = {
> -    "",
> +    "", NULL,
>  	"\fmessage{ ", format_message_text,
>  	    "\fheader{\n", format_headers_text, format_headers_message_part_text, "\fheader}\n",
>  	    "\fbody{\n",
> @@ -85,7 +85,7 @@ static void
>  format_part_end_json (GMimeObject *part);
>  
>  static const notmuch_show_format_t format_json = {
> -    "[",
> +    "[", NULL,
>  	"{", format_message_json,
>  	    "\"headers\": {", format_headers_json, format_headers_message_part_json, "}",
>  	    ", \"body\": [",
> @@ -106,7 +106,7 @@ format_message_mbox (const void *ctx,
>  		     unused (int indent));
>  
>  static const notmuch_show_format_t format_mbox = {
> -    "",
> +    "", NULL,
>          "", format_message_mbox,
>              "", NULL, NULL, "",
>              "",
> @@ -125,7 +125,7 @@ static void
>  format_part_content_raw (GMimeObject *part);
>  
>  static const notmuch_show_format_t format_raw = {
> -    "",
> +    "", NULL,
>  	"", NULL,
>  	    "", NULL, format_headers_message_part_text, "\n",
>              "",
> @@ -762,6 +762,20 @@ show_message (void *ctx,
>  	      int indent,
>  	      notmuch_show_params_t *params)
>  {
> +    if (format->part) {
> +	void *local = talloc_new (ctx);
> +	mime_node_t *root, *part;
> +
> +	if (mime_node_open (local, message, params->cryptoctx, params->decrypt,
> +			    &root) != NOTMUCH_STATUS_SUCCESS)

I'm new to talloc, I think I like it, but I always find it confusing
when some code paths free the contexts, and some don't. Like here.

Are you not freeing the local context here because it's just an empty
context, and freeing below because it's no longer empty?

No need to change anything, I guess, just asking...

BR,
Jani.

> +	    return;
> +	part = mime_node_seek_dfs (root, params->part < 0 ? 0 : params->part);
> +	if (part)
> +	    format->part (local, part, indent, params);
> +	talloc_free (local);
> +	return;
> +    }
> +
>      if (params->part <= 0) {
>  	fputs (format->message_start, stdout);
>  	if (format->message)
> -- 
> 1.7.7.3
>