Date: Mon, 23 Jan 2012 18:23:05 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
Subject: Re: [PATCH v2 3/3] show: Introduce mime_node formatter callback
Message-ID: <20120123232305.GW16740@mit.edu>
References: <1326918507-28033-1-git-send-email-amdragon@mit.edu>
	<1327285873-4713-1-git-send-email-amdragon@mit.edu>
	<1327285873-4713-4-git-send-email-amdragon@mit.edu>
	<8739b6niz9.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8739b6niz9.fsf@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Thanks for the review.  New version coming shortly (v4, since the list
ate v3 while everyone was still reading v2).

Quoth Dmitry Kurochkin on Jan 24 at  2:37 am:
> On Sun, 22 Jan 2012 21:31:13 -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.
> 
> Few comments below.
> 
> > ---
> >  notmuch-client.h |    6 ++++++
> >  notmuch-reply.c  |    2 +-
> >  notmuch-show.c   |   23 +++++++++++++++++++----
> >  3 files changed, 26 insertions(+), 5 deletions(-)
> > 
> > diff --git a/notmuch-client.h b/notmuch-client.h
> > index abfe5d3..59606b4 100644
> > --- a/notmuch-client.h
> > +++ b/notmuch-client.h
> > @@ -62,8 +62,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);
> 
> Can we make the params parameter const?

Apparently we can.

> >      const char *message_start;
> >      void (*message) (const void *ctx,
> >  		     notmuch_message_t *message,
> > diff --git a/notmuch-reply.c b/notmuch-reply.c
> > index bf67960..f55b1d2 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 682aa71..8185b02 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",
> > @@ -89,7 +89,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\": [",
> > @@ -110,7 +110,7 @@ format_message_mbox (const void *ctx,
> >  		     unused (int indent));
> >  
> >  static const notmuch_show_format_t format_mbox = {
> > -    "",
> > +    "", NULL,
> >          "", format_message_mbox,
> >              "", NULL, NULL, "",
> >              "",
> > @@ -129,7 +129,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",
> >              "",
> > @@ -850,6 +850,21 @@ 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)
> > +	    goto DONE;
> > +	part = mime_node_seek_dfs (root, params->part < 0 ? 0 : params->part);
> 
> This should be done as a separate patch, but it looks like zero and
> negative params->part is handled in the same way so we can change it to
> always be non-negative.

That's true here, but there are other places where the difference does
matter.  (I would certainly *prefer* this not to be asymmetric, but
that's a bigger issue involving show's inconsistent interface.)

> > +	if (part)
> > +	    format->part (local, part, indent, params);
> > +      DONE:
> > +	talloc_free (local);
> > +	return;
> 
> Please move part assignment inside the if and remove the goto:
> 
>   if (mime_node_open() == success && (part = seek()))
>     format->part();

Done.

> Regards,
>   Dmitry
> 
> > +    }
> > +
> >      if (params->part <= 0) {
> >  	fputs (format->message_start, stdout);
> >  	if (format->message)
>