Date: Wed, 18 Jan 2012 21:23:48 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [PATCH 3/3] show: Introduce mime_node formatter callback
Message-ID: <20120119022348.GK16740@mit.edu>
References: <1326918507-28033-1-git-send-email-amdragon@mit.edu>
	<1326918507-28033-4-git-send-email-amdragon@mit.edu>
	<87wr8obpyv.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87wr8obpyv.fsf@nikula.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org, dkg@fifthhorseman.net
Precedence: list

Quoth Jani Nikula on Jan 19 at 12:33 am:
> 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, that's just a bug.  In practice it probably doesn't matter much
because, as you pointed out, the local context consumes very little
memory and since the caller will eventually free ctx, local will get
cleaned up, too.  So, while this isn't strictly a memory leak, in
principle this could add up before ctx gets freed.  Fixed.

> No need to change anything, I guess, just asking...

I wait a little for other comments and then send a new version.

Thanks for the review!

> 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)