From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 3/3] show: Introduce mime_node formatter callback
In-Reply-To: <1327285873-4713-4-git-send-email-amdragon@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>
User-Agent: Notmuch/0.11+100~gd650abf (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Tue, 24 Jan 2012 02:37:30 +0400
Message-ID: <8739b6niz9.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

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?

>      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.

> +	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();

Regards,
  Dmitry

> +    }
> +
>      if (params->part <= 0) {
>  	fputs (format->message_start, stdout);
>  	if (format->message)
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch