[PATCH 0/3] Second step of 'show' rewrite

[PATCH 0/3] Second step of 'show' rewrite

[Austin Clements]

This adds support for self-recursive message formatters, while
maintaining backwards compatibility with old style formatters.  After
this, each format can be converted to the new style individually and,
once they're all converted, a bunch of code can be deleted.

These three patches are independent and can be pushed in any order.  I
put them in a series because pushing them before any formatter
rewrites will simplify dependencies between the individual formatter
rewrites.

[PATCH 1/3] mime node: Record depth-first part numbers

[Austin Clements]

This makes the part numbers readily accessible to formatters.
Hierarchical part numbering would be a more natural and efficient fit
for MIME and may be the way to go in the future, but depth-first
numbering maintains compatibility with what we currently do.
---
 mime-node.c      |   33 ++++++++++++++++++++++++++++++++-
 notmuch-client.h |   11 +++++++++++
 2 files changed, 43 insertions(+), 1 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index d26bb44..30b542f 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -104,6 +104,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
     root->nchildren = 1;
     root->ctx = mctx;
 
+    root->part_num = 0;
+    root->next_child = 0;
+    root->next_part_num = 1;
+
     *root_out = root;
     return NOTMUCH_STATUS_SUCCESS;
 
@@ -133,6 +137,8 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
 	talloc_free (node);
 	return NULL;
     }
+    node->parent = parent;
+    node->part_num = node->next_part_num = -1;
 
     /* Deal with the different types of parts */
     if (GMIME_IS_PART (part)) {
@@ -217,6 +223,7 @@ mime_node_t *
 mime_node_child (const mime_node_t *parent, int child)
 {
     GMimeObject *sub;
+    mime_node_t *node;
 
     if (!parent || child < 0 || child >= parent->nchildren)
 	return NULL;
@@ -234,7 +241,31 @@ mime_node_child (const mime_node_t *parent, int child)
 	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
 			g_type_name (G_OBJECT_TYPE (parent->part)));
     }
-    return _mime_node_create (parent, sub);
+    node = _mime_node_create (parent, sub);
+
+    if (child == parent->next_child && parent->next_part_num != -1) {
+	/* We're traversing in depth-first order.  Record the child's
+	 * depth-first numbering. */
+	node->part_num = parent->next_part_num;
+	node->next_part_num = node->part_num + 1;
+
+	/* Drop the const qualifier because these are internal fields
+	 * whose mutability doesn't affect the interface. */
+	((mime_node_t*)parent)->next_child++;
+	((mime_node_t*)parent)->next_part_num = -1;
+
+	if (node->nchildren == 0) {
+	    /* We've reached a leaf, so find the parent that has more
+	     * children and set it up to number its next child. */
+	    const mime_node_t *it = node;
+	    while (it && it->next_child == it->nchildren)
+		it = it->parent;
+	    if (it)
+		((mime_node_t*)it)->next_part_num = node->part_num + 1;
+	}
+    }
+
+    return node;
 }
 
 static mime_node_t *
diff --git a/notmuch-client.h b/notmuch-client.h
index 62ede28..b3dcb6b 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -281,6 +281,13 @@ typedef struct mime_node {
     /* The number of children of this part. */
     int nchildren;
 
+    /* The parent of this node or NULL if this is the root node. */
+    const struct mime_node *parent;
+
+    /* The depth-first part number of this child if the MIME tree is
+     * being traversed in depth-first order, or -1 otherwise. */
+    int part_num;
+
     /* True if decryption of this part was attempted. */
     notmuch_bool_t decrypt_attempted;
     /* True if decryption of this part's child succeeded.  In this
@@ -302,6 +309,10 @@ typedef struct mime_node {
     /* Internal: For successfully decrypted multipart parts, the
      * decrypted part to substitute for the second child. */
     GMimeObject *decrypted_child;
+
+    /* Internal: The next child for depth-first traversal and the part
+     * number to assign it (or -1 if unknown). */
+    int next_child, next_part_num;
 } mime_node_t;
 
 /* Construct a new MIME node pointing to the root message part of
-- 
1.7.7.3

[PATCH 2/3] show: Use consistent header ordering in the text format

[Austin Clements]

Previously, top-level message headers were printed as Subject, From,
To, Date, while embedded message headers were printed From, To,
Subject, Date.  This makes both cases use the former order and updates
the tests accordingly.

Strangely, the raw format also uses this function, so this also fixes
the two raw format tests affected by this change.
---
 notmuch-show.c |    2 +-
 test/multipart |   12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index d14dac9..ecadfa8 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -360,6 +360,7 @@ format_headers_message_part_text (GMimeMessage *message)
     InternetAddressList *recipients;
     const char *recipients_string;
 
+    printf ("Subject: %s\n", g_mime_message_get_subject (message));
     printf ("From: %s\n", g_mime_message_get_sender (message));
     recipients = g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_TO);
     recipients_string = internet_address_list_to_string (recipients, 0);
@@ -371,7 +372,6 @@ format_headers_message_part_text (GMimeMessage *message)
     if (recipients_string)
 	printf ("Cc: %s\n",
 		recipients_string);
-    printf ("Subject: %s\n", g_mime_message_get_subject (message));
     printf ("Date: %s\n", g_mime_message_get_date_as_string (message));
 }
 
diff --git a/test/multipart b/test/multipart
index f83526b..2dd73f5 100755
--- a/test/multipart
+++ b/test/multipart
@@ -121,9 +121,9 @@ Date: Fri, 05 Jan 2001 15:43:57 +0000
 \fpart{ ID: 2, Content-type: multipart/mixed
 \fpart{ ID: 3, Content-type: message/rfc822
 \fheader{
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 \fheader}
 \fbody{
@@ -162,9 +162,9 @@ cat <<EOF >EXPECTED
 \fpart{ ID: 2, Content-type: multipart/mixed
 \fpart{ ID: 3, Content-type: message/rfc822
 \fheader{
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 \fheader}
 \fbody{
@@ -200,9 +200,9 @@ cat <<EOF >EXPECTED
 \fpart{ ID: 2, Content-type: multipart/mixed
 \fpart{ ID: 3, Content-type: message/rfc822
 \fheader{
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 \fheader}
 \fbody{
@@ -233,9 +233,9 @@ notmuch show --format=text --part=3 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OU
 cat <<EOF >EXPECTED
 \fpart{ ID: 3, Content-type: message/rfc822
 \fheader{
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 \fheader}
 \fbody{
@@ -452,9 +452,9 @@ notmuch show --format=raw --part=1 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUT
 # output should *not* include newline
 echo >>OUTPUT
 cat <<EOF >EXPECTED
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 
 <p>This is an embedded message, with a multipart/alternative part.</p>
@@ -476,9 +476,9 @@ test_expect_equal_file OUTPUT EXPECTED
 test_begin_subtest "--format=raw --part=2, multipart/mixed"
 notmuch show --format=raw --part=2 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
 cat <<EOF >EXPECTED
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 
 <p>This is an embedded message, with a multipart/alternative part.</p>
-- 
1.7.7.3

[PATCH 3/3] show: Introduce mime_node formatter callback

[Austin Clements]

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)
+	    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

Re: [PATCH 0/3] Second step of 'show' rewrite

[Dmitry Kurochkin]

Hi Austin.

On Wed, 18 Jan 2012 15:28:24 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This adds support for self-recursive message formatters, while
> maintaining backwards compatibility with old style formatters.  After
> this, each format can be converted to the new style individually and,
> once they're all converted, a bunch of code can be deleted.
> 
> These three patches are independent and can be pushed in any order.  I
> put them in a series because pushing them before any formatter
> rewrites will simplify dependencies between the individual formatter
> rewrites.
> 

Thank you for the patches.  Sorry, I did not manage to review your
previous RFC series [1] yet (but it is on my TODO list).  These patches
are partially the same as in the RFC series.  I suppose I should start
with reviewing these new series first, right?  Does it still makes sense
to review the RFC series after that?

Regards,
  Dmitry

[1] id:"1326332973-30225-1-git-send-email-amdragon@mit.edu"

Re: [PATCH 0/3] Second step of 'show' rewrite

[Austin Clements]

Quoth Dmitry Kurochkin on Jan 19 at 12:35 am:
> Hi Austin.
> 
> On Wed, 18 Jan 2012 15:28:24 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > This adds support for self-recursive message formatters, while
> > maintaining backwards compatibility with old style formatters.  After
> > this, each format can be converted to the new style individually and,
> > once they're all converted, a bunch of code can be deleted.
> > 
> > These three patches are independent and can be pushed in any order.  I
> > put them in a series because pushing them before any formatter
> > rewrites will simplify dependencies between the individual formatter
> > rewrites.
> > 
> 
> Thank you for the patches.  Sorry, I did not manage to review your
> previous RFC series [1] yet (but it is on my TODO list).  These patches
> are partially the same as in the RFC series.  I suppose I should start
> with reviewing these new series first, right?  Does it still makes sense
> to review the RFC series after that?

No worries.  Reviewing these first would be best.  These patches are
very similar to the first few in the RFC series (maybe identical?  I
forget), so there's no point in reviewing those after these.  However,
the RFC series goes on to restructure the text formatter and I would
love to have someone skim over those before I send that part out for
real.

> Regards,
>   Dmitry
> 
> [1] id:"1326332973-30225-1-git-send-email-amdragon@mit.edu"

Re: [PATCH 1/3] mime node: Record depth-first part numbers

[Jani Nikula]

On Wed, 18 Jan 2012 15:28:25 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This makes the part numbers readily accessible to formatters.
> Hierarchical part numbering would be a more natural and efficient fit
> for MIME and may be the way to go in the future, but depth-first
> numbering maintains compatibility with what we currently do.

Hi, please find a few things to consider below. If you disagree after
considering, it's quite all right, as they're largely style matters. :)

BR,
Jani.


> ---
>  mime-node.c      |   33 ++++++++++++++++++++++++++++++++-
>  notmuch-client.h |   11 +++++++++++
>  2 files changed, 43 insertions(+), 1 deletions(-)
> 
> diff --git a/mime-node.c b/mime-node.c
> index d26bb44..30b542f 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -104,6 +104,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
>      root->nchildren = 1;
>      root->ctx = mctx;
>  
> +    root->part_num = 0;
> +    root->next_child = 0;
> +    root->next_part_num = 1;
> +
>      *root_out = root;
>      return NOTMUCH_STATUS_SUCCESS;
>  
> @@ -133,6 +137,8 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
>  	talloc_free (node);
>  	return NULL;
>      }
> +    node->parent = parent;
> +    node->part_num = node->next_part_num = -1;
>  
>      /* Deal with the different types of parts */
>      if (GMIME_IS_PART (part)) {
> @@ -217,6 +223,7 @@ mime_node_t *
>  mime_node_child (const mime_node_t *parent, int child)
>  {
>      GMimeObject *sub;
> +    mime_node_t *node;
>  
>      if (!parent || child < 0 || child >= parent->nchildren)
>  	return NULL;
> @@ -234,7 +241,31 @@ mime_node_child (const mime_node_t *parent, int child)
>  	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
>  			g_type_name (G_OBJECT_TYPE (parent->part)));
>      }
> -    return _mime_node_create (parent, sub);
> +    node = _mime_node_create (parent, sub);
> +
> +    if (child == parent->next_child && parent->next_part_num != -1) {
> +	/* We're traversing in depth-first order.  Record the child's
> +	 * depth-first numbering. */
> +	node->part_num = parent->next_part_num;
> +	node->next_part_num = node->part_num + 1;
> +
> +	/* Drop the const qualifier because these are internal fields
> +	 * whose mutability doesn't affect the interface. */

FWIW, I'm not a big fan of casting away const. Either it is const, or it
isn't. Not very many places would be affected if you dropped the const
qualifier from the related interface(s) altogether, and things would
look cleaner here. But I suppose this is a matter of taste.

> +	((mime_node_t*)parent)->next_child++;
> +	((mime_node_t*)parent)->next_part_num = -1;
> +
> +	if (node->nchildren == 0) {
> +	    /* We've reached a leaf, so find the parent that has more
> +	     * children and set it up to number its next child. */
> +	    const mime_node_t *it = node;
> +	    while (it && it->next_child == it->nchildren)
> +		it = it->parent;
> +	    if (it)
> +		((mime_node_t*)it)->next_part_num = node->part_num + 1;
> +	}
> +    }

Following the constness around made me wonder why the above if block
isn't within _mime_node_create(). It does have a feel of initialization
related to creation in it. (You'd have to pass more info to it though.)

> +
> +    return node;
>  }
>  
>  static mime_node_t *
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 62ede28..b3dcb6b 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -281,6 +281,13 @@ typedef struct mime_node {
>      /* The number of children of this part. */
>      int nchildren;
>  
> +    /* The parent of this node or NULL if this is the root node. */
> +    const struct mime_node *parent;
> +
> +    /* The depth-first part number of this child if the MIME tree is
> +     * being traversed in depth-first order, or -1 otherwise. */
> +    int part_num;
> +
>      /* True if decryption of this part was attempted. */
>      notmuch_bool_t decrypt_attempted;
>      /* True if decryption of this part's child succeeded.  In this
> @@ -302,6 +309,10 @@ typedef struct mime_node {
>      /* Internal: For successfully decrypted multipart parts, the
>       * decrypted part to substitute for the second child. */
>      GMimeObject *decrypted_child;
> +
> +    /* Internal: The next child for depth-first traversal and the part
> +     * number to assign it (or -1 if unknown). */
> +    int next_child, next_part_num;

A matter of taste again, but I wouldn't use "int foo, bar" in struct
declarations.

>  } mime_node_t;
>  
>  /* Construct a new MIME node pointing to the root message part of
> -- 
> 1.7.7.3
> 

Re: [PATCH 3/3] show: Introduce mime_node formatter callback

[Jani Nikula]

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
> 

Re: [PATCH 1/3] mime node: Record depth-first part numbers

[Austin Clements]

Quoth Jani Nikula on Jan 19 at 12:25 am:
> On Wed, 18 Jan 2012 15:28:25 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > This makes the part numbers readily accessible to formatters.
> > Hierarchical part numbering would be a more natural and efficient fit
> > for MIME and may be the way to go in the future, but depth-first
> > numbering maintains compatibility with what we currently do.
> 
> Hi, please find a few things to consider below. If you disagree after
> considering, it's quite all right, as they're largely style matters. :)
> 
> BR,
> Jani.
> 
> 
> > ---
> >  mime-node.c      |   33 ++++++++++++++++++++++++++++++++-
> >  notmuch-client.h |   11 +++++++++++
> >  2 files changed, 43 insertions(+), 1 deletions(-)
> > 
> > diff --git a/mime-node.c b/mime-node.c
> > index d26bb44..30b542f 100644
> > --- a/mime-node.c
> > +++ b/mime-node.c
> > @@ -104,6 +104,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
> >      root->nchildren = 1;
> >      root->ctx = mctx;
> >  
> > +    root->part_num = 0;
> > +    root->next_child = 0;
> > +    root->next_part_num = 1;
> > +
> >      *root_out = root;
> >      return NOTMUCH_STATUS_SUCCESS;
> >  
> > @@ -133,6 +137,8 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> >  	talloc_free (node);
> >  	return NULL;
> >      }
> > +    node->parent = parent;
> > +    node->part_num = node->next_part_num = -1;
> >  
> >      /* Deal with the different types of parts */
> >      if (GMIME_IS_PART (part)) {
> > @@ -217,6 +223,7 @@ mime_node_t *
> >  mime_node_child (const mime_node_t *parent, int child)
> >  {
> >      GMimeObject *sub;
> > +    mime_node_t *node;
> >  
> >      if (!parent || child < 0 || child >= parent->nchildren)
> >  	return NULL;
> > @@ -234,7 +241,31 @@ mime_node_child (const mime_node_t *parent, int child)
> >  	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
> >  			g_type_name (G_OBJECT_TYPE (parent->part)));
> >      }
> > -    return _mime_node_create (parent, sub);
> > +    node = _mime_node_create (parent, sub);
> > +
> > +    if (child == parent->next_child && parent->next_part_num != -1) {
> > +	/* We're traversing in depth-first order.  Record the child's
> > +	 * depth-first numbering. */
> > +	node->part_num = parent->next_part_num;
> > +	node->next_part_num = node->part_num + 1;
> > +
> > +	/* Drop the const qualifier because these are internal fields
> > +	 * whose mutability doesn't affect the interface. */
> 
> FWIW, I'm not a big fan of casting away const. Either it is const, or it
> isn't. Not very many places would be affected if you dropped the const
> qualifier from the related interface(s) altogether, and things would
> look cleaner here. But I suppose this is a matter of taste.

I'm not particularly happy with this either.  Unfortunately, dropping
the const here affects a surprising number of places, including the
entire MIME node API.

I think that, at a deep level, depth-first numbering simply doesn't
resonate with an extremely hierarchical API like this and that
dissonance is going to have to focus somewhere.  There have been
discussions of switching to hierarchical part numbering before (in
particular, because depth-first numbering is unstable with encrypted
parts) and I'll probably restart those after all of this is done.

> > +	((mime_node_t*)parent)->next_child++;
> > +	((mime_node_t*)parent)->next_part_num = -1;
> > +
> > +	if (node->nchildren == 0) {
> > +	    /* We've reached a leaf, so find the parent that has more
> > +	     * children and set it up to number its next child. */
> > +	    const mime_node_t *it = node;
> > +	    while (it && it->next_child == it->nchildren)
> > +		it = it->parent;
> > +	    if (it)
> > +		((mime_node_t*)it)->next_part_num = node->part_num + 1;
> > +	}
> > +    }
> 
> Following the constness around made me wonder why the above if block
> isn't within _mime_node_create(). It does have a feel of initialization
> related to creation in it. (You'd have to pass more info to it though.)

I considered this, but was dissuaded by having to pass redundant
information to _mime_node_create.  I think it's okay here because it
has as much to do with child fetching as with initialization, but I
could easily be swayed.

> > +
> > +    return node;
> >  }
> >  
> >  static mime_node_t *
> > diff --git a/notmuch-client.h b/notmuch-client.h
> > index 62ede28..b3dcb6b 100644
> > --- a/notmuch-client.h
> > +++ b/notmuch-client.h
> > @@ -281,6 +281,13 @@ typedef struct mime_node {
> >      /* The number of children of this part. */
> >      int nchildren;
> >  
> > +    /* The parent of this node or NULL if this is the root node. */
> > +    const struct mime_node *parent;
> > +
> > +    /* The depth-first part number of this child if the MIME tree is
> > +     * being traversed in depth-first order, or -1 otherwise. */
> > +    int part_num;
> > +
> >      /* True if decryption of this part was attempted. */
> >      notmuch_bool_t decrypt_attempted;
> >      /* True if decryption of this part's child succeeded.  In this
> > @@ -302,6 +309,10 @@ typedef struct mime_node {
> >      /* Internal: For successfully decrypted multipart parts, the
> >       * decrypted part to substitute for the second child. */
> >      GMimeObject *decrypted_child;
> > +
> > +    /* Internal: The next child for depth-first traversal and the part
> > +     * number to assign it (or -1 if unknown). */
> > +    int next_child, next_part_num;
> 
> A matter of taste again, but I wouldn't use "int foo, bar" in struct
> declarations.

Fixed.

> >  } mime_node_t;
> >  
> >  /* Construct a new MIME node pointing to the root message part of
> 

Re: [PATCH 3/3] show: Introduce mime_node formatter callback

[Austin Clements]

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)

Re: [PATCH 2/3] show: Use consistent header ordering in the text format

[Tomi Ollila]

On Wed, 18 Jan 2012 15:28:26 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Previously, top-level message headers were printed as Subject, From,
> To, Date, while embedded message headers were printed From, To,
> Subject, Date.  This makes both cases use the former order and updates
> the tests accordingly.
> 
> Strangely, the raw format also uses this function, so this also fixes
> the two raw format tests affected by this change.
> ---

LGTM (this patch).

Tomi

Re: [PATCH 0/3] Second step of 'show' rewrite

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]

On Wed, 18 Jan 2012 15:28:24 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This adds support for self-recursive message formatters, while
> maintaining backwards compatibility with old style formatters.  After
> this, each format can be converted to the new style individually and,
> once they're all converted, a bunch of code can be deleted.
> 
> These three patches are independent and can be pushed in any order.  I
> put them in a series because pushing them before any formatter
> rewrites will simplify dependencies between the individual formatter
> rewrites.

Hey, Austin.  I just looked through these patches and tested them out
and they look good.  Jani's comments are more incisive than anything I
could come up with.  +1 on the series, and I'm looking forward to the
next phase.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 1/3] mime node: Record depth-first part numbers

[Austin Clements]

Quoth myself on Jan 18 at  9:12 pm:
> Quoth Jani Nikula on Jan 19 at 12:25 am:
> > FWIW, I'm not a big fan of casting away const. Either it is const, or it
> > isn't. Not very many places would be affected if you dropped the const
> > qualifier from the related interface(s) altogether, and things would
> > look cleaner here. But I suppose this is a matter of taste.
> 
> I'm not particularly happy with this either.  Unfortunately, dropping
> the const here affects a surprising number of places, including the
> entire MIME node API.

I've changed my mind and removed a few consts so that this funny cast
isn't necessary.  (It also turned out that when I tried this before,
I'd given up just a smidgen before removing enough consts to make it
work.)

> I think that, at a deep level, depth-first numbering simply doesn't
> resonate with an extremely hierarchical API like this and that
> dissonance is going to have to focus somewhere.  There have been
> discussions of switching to hierarchical part numbering before (in
> particular, because depth-first numbering is unstable with encrypted
> parts) and I'll probably restart those after all of this is done.

I have not, however, changed my mind about this.

[PATCH v2 0/3] Second step of 'show' rewrite

[Austin Clements]

This revision addresses Jani's comments.  It removes some
const-stripping casts (at the cost of dropping a const from the API),
fixes a delayed free, and cleans up some aesthetics.

[PATCH v2 1/3] mime node: Record depth-first part numbers

[Austin Clements]

This makes the part numbers readily accessible to formatters.
Hierarchical part numbering would be a more natural and efficient fit
for MIME and may be the way to go in the future, but depth-first
numbering maintains compatibility with what we currently do.
---
 mime-node.c      |   37 ++++++++++++++++++++++++++++++++++---
 notmuch-client.h |   14 +++++++++++++-
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 27077f7..025c537 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -112,6 +112,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
     root->nchildren = 1;
     root->ctx = mctx;
 
+    root->part_num = 0;
+    root->next_child = 0;
+    root->next_part_num = 1;
+
     *root_out = root;
     return NOTMUCH_STATUS_SUCCESS;
 
@@ -137,7 +141,7 @@ _signature_validity_free (GMimeSignatureValidity **proxy)
 #endif
 
 static mime_node_t *
-_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+_mime_node_create (mime_node_t *parent, GMimeObject *part)
 {
     mime_node_t *node = talloc_zero (parent, mime_node_t);
     GError *err = NULL;
@@ -150,6 +154,8 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
 	talloc_free (node);
 	return NULL;
     }
+    node->parent = parent;
+    node->part_num = node->next_part_num = -1;
 
     /* Deal with the different types of parts */
     if (GMIME_IS_PART (part)) {
@@ -267,9 +273,10 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
 }
 
 mime_node_t *
-mime_node_child (const mime_node_t *parent, int child)
+mime_node_child (mime_node_t *parent, int child)
 {
     GMimeObject *sub;
+    mime_node_t *node;
 
     if (!parent || child < 0 || child >= parent->nchildren)
 	return NULL;
@@ -287,7 +294,31 @@ mime_node_child (const mime_node_t *parent, int child)
 	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
 			g_type_name (G_OBJECT_TYPE (parent->part)));
     }
-    return _mime_node_create (parent, sub);
+    node = _mime_node_create (parent, sub);
+
+    if (child == parent->next_child && parent->next_part_num != -1) {
+	/* We're traversing in depth-first order.  Record the child's
+	 * depth-first numbering. */
+	node->part_num = parent->next_part_num;
+	node->next_part_num = node->part_num + 1;
+
+	/* Drop the const qualifier because these are internal fields
+	 * whose mutability doesn't affect the interface. */
+	parent->next_child++;
+	parent->next_part_num = -1;
+
+	if (node->nchildren == 0) {
+	    /* We've reached a leaf, so find the parent that has more
+	     * children and set it up to number its next child. */
+	    mime_node_t *it = node;
+	    while (it && it->next_child == it->nchildren)
+		it = it->parent;
+	    if (it)
+		it->next_part_num = node->part_num + 1;
+	}
+    }
+
+    return node;
 }
 
 static mime_node_t *
diff --git a/notmuch-client.h b/notmuch-client.h
index 9c1d383..abfe5d3 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -297,6 +297,13 @@ typedef struct mime_node {
     /* The number of children of this part. */
     int nchildren;
 
+    /* The parent of this node or NULL if this is the root node. */
+    struct mime_node *parent;
+
+    /* The depth-first part number of this child if the MIME tree is
+     * being traversed in depth-first order, or -1 otherwise. */
+    int part_num;
+
     /* True if decryption of this part was attempted. */
     notmuch_bool_t decrypt_attempted;
     /* True if decryption of this part's child succeeded.  In this
@@ -324,6 +331,11 @@ typedef struct mime_node {
     /* Internal: For successfully decrypted multipart parts, the
      * decrypted part to substitute for the second child. */
     GMimeObject *decrypted_child;
+
+    /* Internal: The next child for depth-first traversal and the part
+     * number to assign it (or -1 if unknown). */
+    int next_child;
+    int next_part_num;
 } mime_node_t;
 
 /* Construct a new MIME node pointing to the root message part of
@@ -356,7 +368,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
  * an error message on stderr).
  */
 mime_node_t *
-mime_node_child (const mime_node_t *parent, int child);
+mime_node_child (mime_node_t *parent, int child);
 
 /* Return the nth child of node in a depth-first traversal.  If n is
  * 0, returns node itself.  Returns NULL if there is no such part. */
-- 
1.7.7.3

[PATCH v2 2/3] show: Use consistent header ordering in the text format

[Austin Clements]

Previously, top-level message headers were printed as Subject, From,
To, Date, while embedded message headers were printed From, To,
Subject, Date.  This makes both cases use the former order and updates
the tests accordingly.

Strangely, the raw format also uses this function, so this also fixes
the two raw format tests affected by this change.
---
 notmuch-show.c |    2 +-
 test/multipart |   12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 7b40568..682aa71 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -364,6 +364,7 @@ format_headers_message_part_text (GMimeMessage *message)
     InternetAddressList *recipients;
     const char *recipients_string;
 
+    printf ("Subject: %s\n", g_mime_message_get_subject (message));
     printf ("From: %s\n", g_mime_message_get_sender (message));
     recipients = g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_TO);
     recipients_string = internet_address_list_to_string (recipients, 0);
@@ -375,7 +376,6 @@ format_headers_message_part_text (GMimeMessage *message)
     if (recipients_string)
 	printf ("Cc: %s\n",
 		recipients_string);
-    printf ("Subject: %s\n", g_mime_message_get_subject (message));
     printf ("Date: %s\n", g_mime_message_get_date_as_string (message));
 }
 
diff --git a/test/multipart b/test/multipart
index f83526b..2dd73f5 100755
--- a/test/multipart
+++ b/test/multipart
@@ -121,9 +121,9 @@ Date: Fri, 05 Jan 2001 15:43:57 +0000
 \fpart{ ID: 2, Content-type: multipart/mixed
 \fpart{ ID: 3, Content-type: message/rfc822
 \fheader{
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 \fheader}
 \fbody{
@@ -162,9 +162,9 @@ cat <<EOF >EXPECTED
 \fpart{ ID: 2, Content-type: multipart/mixed
 \fpart{ ID: 3, Content-type: message/rfc822
 \fheader{
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 \fheader}
 \fbody{
@@ -200,9 +200,9 @@ cat <<EOF >EXPECTED
 \fpart{ ID: 2, Content-type: multipart/mixed
 \fpart{ ID: 3, Content-type: message/rfc822
 \fheader{
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 \fheader}
 \fbody{
@@ -233,9 +233,9 @@ notmuch show --format=text --part=3 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OU
 cat <<EOF >EXPECTED
 \fpart{ ID: 3, Content-type: message/rfc822
 \fheader{
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 \fheader}
 \fbody{
@@ -452,9 +452,9 @@ notmuch show --format=raw --part=1 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUT
 # output should *not* include newline
 echo >>OUTPUT
 cat <<EOF >EXPECTED
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 
 <p>This is an embedded message, with a multipart/alternative part.</p>
@@ -476,9 +476,9 @@ test_expect_equal_file OUTPUT EXPECTED
 test_begin_subtest "--format=raw --part=2, multipart/mixed"
 notmuch show --format=raw --part=2 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
 cat <<EOF >EXPECTED
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 
 <p>This is an embedded message, with a multipart/alternative part.</p>
-- 
1.7.7.3

[PATCH v2 3/3] show: Introduce mime_node formatter callback

[Austin Clements]

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   |   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);
     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);
+	if (part)
+	    format->part (local, part, indent, params);
+      DONE:
+	talloc_free (local);
+	return;
+    }
+
     if (params->part <= 0) {
 	fputs (format->message_start, stdout);
 	if (format->message)
-- 
1.7.7.3

Re: [PATCH v2 1/3] mime node: Record depth-first part numbers

[Jani Nikula]

On Sun, 22 Jan 2012 21:31:11 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This makes the part numbers readily accessible to formatters.
> Hierarchical part numbering would be a more natural and efficient fit
> for MIME and may be the way to go in the future, but depth-first
> numbering maintains compatibility with what we currently do.
> ---
>  mime-node.c      |   37 ++++++++++++++++++++++++++++++++++---
>  notmuch-client.h |   14 +++++++++++++-
>  2 files changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/mime-node.c b/mime-node.c
> index 27077f7..025c537 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -112,6 +112,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
>      root->nchildren = 1;
>      root->ctx = mctx;
>  
> +    root->part_num = 0;
> +    root->next_child = 0;
> +    root->next_part_num = 1;
> +
>      *root_out = root;
>      return NOTMUCH_STATUS_SUCCESS;
>  
> @@ -137,7 +141,7 @@ _signature_validity_free (GMimeSignatureValidity **proxy)
>  #endif
>  
>  static mime_node_t *
> -_mime_node_create (const mime_node_t *parent, GMimeObject *part)
> +_mime_node_create (mime_node_t *parent, GMimeObject *part)
>  {
>      mime_node_t *node = talloc_zero (parent, mime_node_t);
>      GError *err = NULL;
> @@ -150,6 +154,8 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
>  	talloc_free (node);
>  	return NULL;
>      }
> +    node->parent = parent;
> +    node->part_num = node->next_part_num = -1;
>  
>      /* Deal with the different types of parts */
>      if (GMIME_IS_PART (part)) {
> @@ -267,9 +273,10 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
>  }
>  
>  mime_node_t *
> -mime_node_child (const mime_node_t *parent, int child)
> +mime_node_child (mime_node_t *parent, int child)
>  {
>      GMimeObject *sub;
> +    mime_node_t *node;
>  
>      if (!parent || child < 0 || child >= parent->nchildren)
>  	return NULL;
> @@ -287,7 +294,31 @@ mime_node_child (const mime_node_t *parent, int child)
>  	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
>  			g_type_name (G_OBJECT_TYPE (parent->part)));
>      }
> -    return _mime_node_create (parent, sub);
> +    node = _mime_node_create (parent, sub);
> +
> +    if (child == parent->next_child && parent->next_part_num != -1) {
> +	/* We're traversing in depth-first order.  Record the child's
> +	 * depth-first numbering. */
> +	node->part_num = parent->next_part_num;
> +	node->next_part_num = node->part_num + 1;
> +
> +	/* Drop the const qualifier because these are internal fields
> +	 * whose mutability doesn't affect the interface. */

Leftover comment from the old version.

Otherwise, the code looks good, with the disclaimer that I don't know
much about MIME or gmime.

BR,
Jani.


> +	parent->next_child++;
> +	parent->next_part_num = -1;
> +
> +	if (node->nchildren == 0) {
> +	    /* We've reached a leaf, so find the parent that has more
> +	     * children and set it up to number its next child. */
> +	    mime_node_t *it = node;
> +	    while (it && it->next_child == it->nchildren)
> +		it = it->parent;
> +	    if (it)
> +		it->next_part_num = node->part_num + 1;
> +	}
> +    }
> +
> +    return node;
>  }
>  
>  static mime_node_t *
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 9c1d383..abfe5d3 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -297,6 +297,13 @@ typedef struct mime_node {
>      /* The number of children of this part. */
>      int nchildren;
>  
> +    /* The parent of this node or NULL if this is the root node. */
> +    struct mime_node *parent;
> +
> +    /* The depth-first part number of this child if the MIME tree is
> +     * being traversed in depth-first order, or -1 otherwise. */
> +    int part_num;
> +
>      /* True if decryption of this part was attempted. */
>      notmuch_bool_t decrypt_attempted;
>      /* True if decryption of this part's child succeeded.  In this
> @@ -324,6 +331,11 @@ typedef struct mime_node {
>      /* Internal: For successfully decrypted multipart parts, the
>       * decrypted part to substitute for the second child. */
>      GMimeObject *decrypted_child;
> +
> +    /* Internal: The next child for depth-first traversal and the part
> +     * number to assign it (or -1 if unknown). */
> +    int next_child;
> +    int next_part_num;
>  } mime_node_t;
>  
>  /* Construct a new MIME node pointing to the root message part of
> @@ -356,7 +368,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
>   * an error message on stderr).
>   */
>  mime_node_t *
> -mime_node_child (const mime_node_t *parent, int child);
> +mime_node_child (mime_node_t *parent, int child);
>  
>  /* Return the nth child of node in a depth-first traversal.  If n is
>   * 0, returns node itself.  Returns NULL if there is no such part. */
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH v3 0/2] Second step of 'show' rewrite

[Austin Clements]

Fix a silly comment mistake pointed out by Jani

[PATCH v3 1/2] mime node: Record depth-first part numbers

[Austin Clements]

This makes the part numbers readily accessible to formatters.
Hierarchical part numbering would be a more natural and efficient fit
for MIME and may be the way to go in the future, but depth-first
numbering maintains compatibility with what we currently do.
---
 mime-node.c      |   36 +++++++++++++++++++++++++++++++++---
 notmuch-client.h |   14 +++++++++++++-
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 27077f7..c073c7e 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -112,6 +112,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
     root->nchildren = 1;
     root->ctx = mctx;
 
+    root->part_num = 0;
+    root->next_child = 0;
+    root->next_part_num = 1;
+
     *root_out = root;
     return NOTMUCH_STATUS_SUCCESS;
 
@@ -137,7 +141,7 @@ _signature_validity_free (GMimeSignatureValidity **proxy)
 #endif
 
 static mime_node_t *
-_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+_mime_node_create (mime_node_t *parent, GMimeObject *part)
 {
     mime_node_t *node = talloc_zero (parent, mime_node_t);
     GError *err = NULL;
@@ -150,6 +154,8 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
 	talloc_free (node);
 	return NULL;
     }
+    node->parent = parent;
+    node->part_num = node->next_part_num = -1;
 
     /* Deal with the different types of parts */
     if (GMIME_IS_PART (part)) {
@@ -267,9 +273,10 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
 }
 
 mime_node_t *
-mime_node_child (const mime_node_t *parent, int child)
+mime_node_child (mime_node_t *parent, int child)
 {
     GMimeObject *sub;
+    mime_node_t *node;
 
     if (!parent || child < 0 || child >= parent->nchildren)
 	return NULL;
@@ -287,7 +294,30 @@ mime_node_child (const mime_node_t *parent, int child)
 	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
 			g_type_name (G_OBJECT_TYPE (parent->part)));
     }
-    return _mime_node_create (parent, sub);
+    node = _mime_node_create (parent, sub);
+
+    if (child == parent->next_child && parent->next_part_num != -1) {
+	/* We're traversing in depth-first order.  Record the child's
+	 * depth-first numbering. */
+	node->part_num = parent->next_part_num;
+	node->next_part_num = node->part_num + 1;
+
+	/* Prepare the parent for its next depth-first child. */
+	parent->next_child++;
+	parent->next_part_num = -1;
+
+	if (node->nchildren == 0) {
+	    /* We've reached a leaf, so find the parent that has more
+	     * children and set it up to number its next child. */
+	    mime_node_t *it = node;
+	    while (it && it->next_child == it->nchildren)
+		it = it->parent;
+	    if (it)
+		it->next_part_num = node->part_num + 1;
+	}
+    }
+
+    return node;
 }
 
 static mime_node_t *
diff --git a/notmuch-client.h b/notmuch-client.h
index 9c1d383..abfe5d3 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -297,6 +297,13 @@ typedef struct mime_node {
     /* The number of children of this part. */
     int nchildren;
 
+    /* The parent of this node or NULL if this is the root node. */
+    struct mime_node *parent;
+
+    /* The depth-first part number of this child if the MIME tree is
+     * being traversed in depth-first order, or -1 otherwise. */
+    int part_num;
+
     /* True if decryption of this part was attempted. */
     notmuch_bool_t decrypt_attempted;
     /* True if decryption of this part's child succeeded.  In this
@@ -324,6 +331,11 @@ typedef struct mime_node {
     /* Internal: For successfully decrypted multipart parts, the
      * decrypted part to substitute for the second child. */
     GMimeObject *decrypted_child;
+
+    /* Internal: The next child for depth-first traversal and the part
+     * number to assign it (or -1 if unknown). */
+    int next_child;
+    int next_part_num;
 } mime_node_t;
 
 /* Construct a new MIME node pointing to the root message part of
@@ -356,7 +368,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
  * an error message on stderr).
  */
 mime_node_t *
-mime_node_child (const mime_node_t *parent, int child);
+mime_node_child (mime_node_t *parent, int child);
 
 /* Return the nth child of node in a depth-first traversal.  If n is
  * 0, returns node itself.  Returns NULL if there is no such part. */
-- 
1.7.7.3

[PATCH v3 2/2] show: Introduce mime_node formatter callback

[Austin Clements]

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   |   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);
     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);
+	if (part)
+	    format->part (local, part, indent, params);
+      DONE:
+	talloc_free (local);
+	return;
+    }
+
     if (params->part <= 0) {
 	fputs (format->message_start, stdout);
 	if (format->message)
-- 
1.7.7.3

Re: [PATCH v2 3/3] show: Introduce mime_node formatter callback

[Tomi Ollila]

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.
> ---

LGTM.

Tomi

Re: [PATCH v2 1/3] mime node: Record depth-first part numbers

[Tomi Ollila]

On Sun, 22 Jan 2012 21:31:11 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This makes the part numbers readily accessible to formatters.
> Hierarchical part numbering would be a more natural and efficient fit
> for MIME and may be the way to go in the future, but depth-first
> numbering maintains compatibility with what we currently do.
> ---

LGTM. I did not understand the logic so after a while I concentrated
on code robustness (i.e. this doesn't break anything). Future
work depends on this.


Tomi

Re: [PATCH v2 1/3] mime node: Record depth-first part numbers

[Dmitry Kurochkin]

On Sun, 22 Jan 2012 21:31:11 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This makes the part numbers readily accessible to formatters.
> Hierarchical part numbering would be a more natural and efficient fit
> for MIME and may be the way to go in the future, but depth-first
> numbering maintains compatibility with what we currently do.

The patch looks good except for few minor comments below.  I do not
think that we need another review for the next patch version.

> ---
>  mime-node.c      |   37 ++++++++++++++++++++++++++++++++++---
>  notmuch-client.h |   14 +++++++++++++-
>  2 files changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/mime-node.c b/mime-node.c
> index 27077f7..025c537 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -112,6 +112,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
>      root->nchildren = 1;
>      root->ctx = mctx;
>  
> +    root->part_num = 0;
> +    root->next_child = 0;
> +    root->next_part_num = 1;
> +

For consistency, we should either initialize root->parent to NULL
explicitly or remove part_num and next_child initialization.

>      *root_out = root;
>      return NOTMUCH_STATUS_SUCCESS;
>  
> @@ -137,7 +141,7 @@ _signature_validity_free (GMimeSignatureValidity **proxy)
>  #endif
>  
>  static mime_node_t *
> -_mime_node_create (const mime_node_t *parent, GMimeObject *part)
> +_mime_node_create (mime_node_t *parent, GMimeObject *part)
>  {
>      mime_node_t *node = talloc_zero (parent, mime_node_t);
>      GError *err = NULL;
> @@ -150,6 +154,8 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
>  	talloc_free (node);
>  	return NULL;
>      }
> +    node->parent = parent;
> +    node->part_num = node->next_part_num = -1;

Same here: if we initialize next_child to zero explicitly in
mime_node_open(), we should do it here as well.

>  
>      /* Deal with the different types of parts */
>      if (GMIME_IS_PART (part)) {
> @@ -267,9 +273,10 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
>  }
>  
>  mime_node_t *
> -mime_node_child (const mime_node_t *parent, int child)
> +mime_node_child (mime_node_t *parent, int child)
>  {
>      GMimeObject *sub;
> +    mime_node_t *node;
>  
>      if (!parent || child < 0 || child >= parent->nchildren)
>  	return NULL;
> @@ -287,7 +294,31 @@ mime_node_child (const mime_node_t *parent, int child)
>  	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
>  			g_type_name (G_OBJECT_TYPE (parent->part)));
>      }
> -    return _mime_node_create (parent, sub);
> +    node = _mime_node_create (parent, sub);
> +
> +    if (child == parent->next_child && parent->next_part_num != -1) {
> +	/* We're traversing in depth-first order.  Record the child's
> +	 * depth-first numbering. */
> +	node->part_num = parent->next_part_num;
> +	node->next_part_num = node->part_num + 1;
> +
> +	/* Drop the const qualifier because these are internal fields
> +	 * whose mutability doesn't affect the interface. */

The comment is no longer relevant, please remove.

> +	parent->next_child++;
> +	parent->next_part_num = -1;
> +
> +	if (node->nchildren == 0) {
> +	    /* We've reached a leaf, so find the parent that has more
> +	     * children and set it up to number its next child. */
> +	    mime_node_t *it = node;
> +	    while (it && it->next_child == it->nchildren)

It seems that it should be initialized to node->parent, because
node->next_child is always 0.

Just curious, does "it" stands for "iterator"?  I would prefer just "i"
or "iter" :)

> +		it = it->parent;
> +	    if (it)
> +		it->next_part_num = node->part_num + 1;
> +	}
> +    }

Austin, I trust you that this code works :)  Though I hope we will get
to hierarchical part numbering one day and it would simplify this code.

Regards,
  Dmitry

> +
> +    return node;
>  }
>  
>  static mime_node_t *
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 9c1d383..abfe5d3 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -297,6 +297,13 @@ typedef struct mime_node {
>      /* The number of children of this part. */
>      int nchildren;
>  
> +    /* The parent of this node or NULL if this is the root node. */
> +    struct mime_node *parent;
> +
> +    /* The depth-first part number of this child if the MIME tree is
> +     * being traversed in depth-first order, or -1 otherwise. */
> +    int part_num;
> +
>      /* True if decryption of this part was attempted. */
>      notmuch_bool_t decrypt_attempted;
>      /* True if decryption of this part's child succeeded.  In this
> @@ -324,6 +331,11 @@ typedef struct mime_node {
>      /* Internal: For successfully decrypted multipart parts, the
>       * decrypted part to substitute for the second child. */
>      GMimeObject *decrypted_child;
> +
> +    /* Internal: The next child for depth-first traversal and the part
> +     * number to assign it (or -1 if unknown). */
> +    int next_child;
> +    int next_part_num;
>  } mime_node_t;
>  
>  /* Construct a new MIME node pointing to the root message part of
> @@ -356,7 +368,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
>   * an error message on stderr).
>   */
>  mime_node_t *
> -mime_node_child (const mime_node_t *parent, int child);
> +mime_node_child (mime_node_t *parent, int child);
>  
>  /* Return the nth child of node in a depth-first traversal.  If n is
>   * 0, returns node itself.  Returns NULL if there is no such part. */
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2 3/3] show: Introduce mime_node formatter callback

[Dmitry Kurochkin]

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

Re: [PATCH v2 1/3] mime node: Record depth-first part numbers

[Austin Clements]

Quoth Dmitry Kurochkin on Jan 24 at  2:19 am:
> On Sun, 22 Jan 2012 21:31:11 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > This makes the part numbers readily accessible to formatters.
> > Hierarchical part numbering would be a more natural and efficient fit
> > for MIME and may be the way to go in the future, but depth-first
> > numbering maintains compatibility with what we currently do.
> 
> The patch looks good except for few minor comments below.  I do not
> think that we need another review for the next patch version.
> 
> > ---
> >  mime-node.c      |   37 ++++++++++++++++++++++++++++++++++---
> >  notmuch-client.h |   14 +++++++++++++-
> >  2 files changed, 47 insertions(+), 4 deletions(-)
> > 
> > diff --git a/mime-node.c b/mime-node.c
> > index 27077f7..025c537 100644
> > --- a/mime-node.c
> > +++ b/mime-node.c
> > @@ -112,6 +112,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
> >      root->nchildren = 1;
> >      root->ctx = mctx;
> >  
> > +    root->part_num = 0;
> > +    root->next_child = 0;
> > +    root->next_part_num = 1;
> > +
> 
> For consistency, we should either initialize root->parent to NULL
> explicitly or remove part_num and next_child initialization.
> 
> >      *root_out = root;
> >      return NOTMUCH_STATUS_SUCCESS;
> >  
> > @@ -137,7 +141,7 @@ _signature_validity_free (GMimeSignatureValidity **proxy)
> >  #endif
> >  
> >  static mime_node_t *
> > -_mime_node_create (const mime_node_t *parent, GMimeObject *part)
> > +_mime_node_create (mime_node_t *parent, GMimeObject *part)
> >  {
> >      mime_node_t *node = talloc_zero (parent, mime_node_t);
> >      GError *err = NULL;
> > @@ -150,6 +154,8 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> >  	talloc_free (node);
> >  	return NULL;
> >      }
> > +    node->parent = parent;
> > +    node->part_num = node->next_part_num = -1;
> 
> Same here: if we initialize next_child to zero explicitly in
> mime_node_open(), we should do it here as well.

Both done.

> >  
> >      /* Deal with the different types of parts */
> >      if (GMIME_IS_PART (part)) {
> > @@ -267,9 +273,10 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> >  }
> >  
> >  mime_node_t *
> > -mime_node_child (const mime_node_t *parent, int child)
> > +mime_node_child (mime_node_t *parent, int child)
> >  {
> >      GMimeObject *sub;
> > +    mime_node_t *node;
> >  
> >      if (!parent || child < 0 || child >= parent->nchildren)
> >  	return NULL;
> > @@ -287,7 +294,31 @@ mime_node_child (const mime_node_t *parent, int child)
> >  	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
> >  			g_type_name (G_OBJECT_TYPE (parent->part)));
> >      }
> > -    return _mime_node_create (parent, sub);
> > +    node = _mime_node_create (parent, sub);
> > +
> > +    if (child == parent->next_child && parent->next_part_num != -1) {
> > +	/* We're traversing in depth-first order.  Record the child's
> > +	 * depth-first numbering. */
> > +	node->part_num = parent->next_part_num;
> > +	node->next_part_num = node->part_num + 1;
> > +
> > +	/* Drop the const qualifier because these are internal fields
> > +	 * whose mutability doesn't affect the interface. */
> 
> The comment is no longer relevant, please remove.
> 
> > +	parent->next_child++;
> > +	parent->next_part_num = -1;
> > +
> > +	if (node->nchildren == 0) {
> > +	    /* We've reached a leaf, so find the parent that has more
> > +	     * children and set it up to number its next child. */
> > +	    mime_node_t *it = node;
> > +	    while (it && it->next_child == it->nchildren)
> 
> It seems that it should be initialized to node->parent, because
> node->next_child is always 0.

Either works.  I started at node because it seemed like a more
fundamental base case, but perhaps that just makes this code even more
obtuse.

> Just curious, does "it" stands for "iterator"?  I would prefer just "i"
> or "iter" :)

"it" is a C++ habit.  I changed it to "iter".

> > +		it = it->parent;
> > +	    if (it)
> > +		it->next_part_num = node->part_num + 1;
> > +	}
> > +    }
> 
> Austin, I trust you that this code works :)  Though I hope we will get
> to hierarchical part numbering one day and it would simplify this code.

It would simplify it down to one line, in fact.  Code simplification
aside, I think hierarchical numbering is the right thing to do.  But
that's for another day.

> Regards,
>   Dmitry
> 
> > +
> > +    return node;
> >  }
> >  
> >  static mime_node_t *
> > diff --git a/notmuch-client.h b/notmuch-client.h
> > index 9c1d383..abfe5d3 100644
> > --- a/notmuch-client.h
> > +++ b/notmuch-client.h
> > @@ -297,6 +297,13 @@ typedef struct mime_node {
> >      /* The number of children of this part. */
> >      int nchildren;
> >  
> > +    /* The parent of this node or NULL if this is the root node. */
> > +    struct mime_node *parent;
> > +
> > +    /* The depth-first part number of this child if the MIME tree is
> > +     * being traversed in depth-first order, or -1 otherwise. */
> > +    int part_num;
> > +
> >      /* True if decryption of this part was attempted. */
> >      notmuch_bool_t decrypt_attempted;
> >      /* True if decryption of this part's child succeeded.  In this
> > @@ -324,6 +331,11 @@ typedef struct mime_node {
> >      /* Internal: For successfully decrypted multipart parts, the
> >       * decrypted part to substitute for the second child. */
> >      GMimeObject *decrypted_child;
> > +
> > +    /* Internal: The next child for depth-first traversal and the part
> > +     * number to assign it (or -1 if unknown). */
> > +    int next_child;
> > +    int next_part_num;
> >  } mime_node_t;
> >  
> >  /* Construct a new MIME node pointing to the root message part of
> > @@ -356,7 +368,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
> >   * an error message on stderr).
> >   */
> >  mime_node_t *
> > -mime_node_child (const mime_node_t *parent, int child);
> > +mime_node_child (mime_node_t *parent, int child);
> >  
> >  /* Return the nth child of node in a depth-first traversal.  If n is
> >   * 0, returns node itself.  Returns NULL if there is no such part. */
> 

-- 
Austin Clements                                      MIT/'06/PhD/CSAIL
amdragon@mit.edu                           http://web.mit.edu/amdragon
       Somewhere in the dream we call reality you will find me,
              searching for the reality we call dreams.

Re: [PATCH v2 3/3] show: Introduce mime_node formatter callback

[Austin Clements]

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)
> 

[PATCH v4 0/3] Second step of 'show' rewrite

[Austin Clements]

This version addresses Dmitry's review.

[PATCH v4 1/3] mime node: Record depth-first part numbers

[Austin Clements]

This makes the part numbers readily accessible to formatters.
Hierarchical part numbering would be a more natural and efficient fit
for MIME and may be the way to go in the future, but depth-first
numbering maintains compatibility with what we currently do.
---
 mime-node.c      |   37 ++++++++++++++++++++++++++++++++++---
 notmuch-client.h |   14 +++++++++++++-
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 27077f7..025c537 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -112,6 +112,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
     root->nchildren = 1;
     root->ctx = mctx;
 
+    root->part_num = 0;
+    root->next_child = 0;
+    root->next_part_num = 1;
+
     *root_out = root;
     return NOTMUCH_STATUS_SUCCESS;
 
@@ -137,7 +141,7 @@ _signature_validity_free (GMimeSignatureValidity **proxy)
 #endif
 
 static mime_node_t *
-_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+_mime_node_create (mime_node_t *parent, GMimeObject *part)
 {
     mime_node_t *node = talloc_zero (parent, mime_node_t);
     GError *err = NULL;
@@ -150,6 +154,8 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
 	talloc_free (node);
 	return NULL;
     }
+    node->parent = parent;
+    node->part_num = node->next_part_num = -1;
 
     /* Deal with the different types of parts */
     if (GMIME_IS_PART (part)) {
@@ -267,9 +273,10 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
 }
 
 mime_node_t *
-mime_node_child (const mime_node_t *parent, int child)
+mime_node_child (mime_node_t *parent, int child)
 {
     GMimeObject *sub;
+    mime_node_t *node;
 
     if (!parent || child < 0 || child >= parent->nchildren)
 	return NULL;
@@ -287,7 +294,31 @@ mime_node_child (const mime_node_t *parent, int child)
 	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
 			g_type_name (G_OBJECT_TYPE (parent->part)));
     }
-    return _mime_node_create (parent, sub);
+    node = _mime_node_create (parent, sub);
+
+    if (child == parent->next_child && parent->next_part_num != -1) {
+	/* We're traversing in depth-first order.  Record the child's
+	 * depth-first numbering. */
+	node->part_num = parent->next_part_num;
+	node->next_part_num = node->part_num + 1;
+
+	/* Drop the const qualifier because these are internal fields
+	 * whose mutability doesn't affect the interface. */
+	parent->next_child++;
+	parent->next_part_num = -1;
+
+	if (node->nchildren == 0) {
+	    /* We've reached a leaf, so find the parent that has more
+	     * children and set it up to number its next child. */
+	    mime_node_t *it = node;
+	    while (it && it->next_child == it->nchildren)
+		it = it->parent;
+	    if (it)
+		it->next_part_num = node->part_num + 1;
+	}
+    }
+
+    return node;
 }
 
 static mime_node_t *
diff --git a/notmuch-client.h b/notmuch-client.h
index 9c1d383..abfe5d3 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -297,6 +297,13 @@ typedef struct mime_node {
     /* The number of children of this part. */
     int nchildren;
 
+    /* The parent of this node or NULL if this is the root node. */
+    struct mime_node *parent;
+
+    /* The depth-first part number of this child if the MIME tree is
+     * being traversed in depth-first order, or -1 otherwise. */
+    int part_num;
+
     /* True if decryption of this part was attempted. */
     notmuch_bool_t decrypt_attempted;
     /* True if decryption of this part's child succeeded.  In this
@@ -324,6 +331,11 @@ typedef struct mime_node {
     /* Internal: For successfully decrypted multipart parts, the
      * decrypted part to substitute for the second child. */
     GMimeObject *decrypted_child;
+
+    /* Internal: The next child for depth-first traversal and the part
+     * number to assign it (or -1 if unknown). */
+    int next_child;
+    int next_part_num;
 } mime_node_t;
 
 /* Construct a new MIME node pointing to the root message part of
@@ -356,7 +368,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
  * an error message on stderr).
  */
 mime_node_t *
-mime_node_child (const mime_node_t *parent, int child);
+mime_node_child (mime_node_t *parent, int child);
 
 /* Return the nth child of node in a depth-first traversal.  If n is
  * 0, returns node itself.  Returns NULL if there is no such part. */
-- 
1.7.7.3

[PATCH v4 2/3] show: Use consistent header ordering in the text format

[Austin Clements]

Previously, top-level message headers were printed as Subject, From,
To, Date, while embedded message headers were printed From, To,
Subject, Date.  This makes both cases use the former order and updates
the tests accordingly.

Strangely, the raw format also uses this function, so this also fixes
the two raw format tests affected by this change.
---
 notmuch-show.c |    2 +-
 test/multipart |   12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 7b40568..682aa71 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -364,6 +364,7 @@ format_headers_message_part_text (GMimeMessage *message)
     InternetAddressList *recipients;
     const char *recipients_string;
 
+    printf ("Subject: %s\n", g_mime_message_get_subject (message));
     printf ("From: %s\n", g_mime_message_get_sender (message));
     recipients = g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_TO);
     recipients_string = internet_address_list_to_string (recipients, 0);
@@ -375,7 +376,6 @@ format_headers_message_part_text (GMimeMessage *message)
     if (recipients_string)
 	printf ("Cc: %s\n",
 		recipients_string);
-    printf ("Subject: %s\n", g_mime_message_get_subject (message));
     printf ("Date: %s\n", g_mime_message_get_date_as_string (message));
 }
 
diff --git a/test/multipart b/test/multipart
index f83526b..2dd73f5 100755
--- a/test/multipart
+++ b/test/multipart
@@ -121,9 +121,9 @@ Date: Fri, 05 Jan 2001 15:43:57 +0000
 \fpart{ ID: 2, Content-type: multipart/mixed
 \fpart{ ID: 3, Content-type: message/rfc822
 \fheader{
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 \fheader}
 \fbody{
@@ -162,9 +162,9 @@ cat <<EOF >EXPECTED
 \fpart{ ID: 2, Content-type: multipart/mixed
 \fpart{ ID: 3, Content-type: message/rfc822
 \fheader{
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 \fheader}
 \fbody{
@@ -200,9 +200,9 @@ cat <<EOF >EXPECTED
 \fpart{ ID: 2, Content-type: multipart/mixed
 \fpart{ ID: 3, Content-type: message/rfc822
 \fheader{
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 \fheader}
 \fbody{
@@ -233,9 +233,9 @@ notmuch show --format=text --part=3 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OU
 cat <<EOF >EXPECTED
 \fpart{ ID: 3, Content-type: message/rfc822
 \fheader{
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 \fheader}
 \fbody{
@@ -452,9 +452,9 @@ notmuch show --format=raw --part=1 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUT
 # output should *not* include newline
 echo >>OUTPUT
 cat <<EOF >EXPECTED
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 
 <p>This is an embedded message, with a multipart/alternative part.</p>
@@ -476,9 +476,9 @@ test_expect_equal_file OUTPUT EXPECTED
 test_begin_subtest "--format=raw --part=2, multipart/mixed"
 notmuch show --format=raw --part=2 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
 cat <<EOF >EXPECTED
+Subject: html message
 From: Carl Worth <cworth@cworth.org>
 To: cworth@cworth.org
-Subject: html message
 Date: Fri, 05 Jan 2001 15:42:57 +0000
 
 <p>This is an embedded message, with a multipart/alternative part.</p>
-- 
1.7.7.3

[PATCH v4 3/3] show: Introduce mime_node formatter callback

[Austin Clements]

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   |   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);
     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);
+	if (part)
+	    format->part (local, part, indent, params);
+      DONE:
+	talloc_free (local);
+	return;
+    }
+
     if (params->part <= 0) {
 	fputs (format->message_start, stdout);
 	if (format->message)
-- 
1.7.7.3

[PATCH v5 0/2] Second step of 'show' rewrite

[Austin Clements]

Arrg, sorry for the spam.  I grabbed the wrong commit ID when sending
v4.

[PATCH v5 1/2] mime node: Record depth-first part numbers

[Austin Clements]

This makes the part numbers readily accessible to formatters.
Hierarchical part numbering would be a more natural and efficient fit
for MIME and may be the way to go in the future, but depth-first
numbering maintains compatibility with what we currently do.
---
 mime-node.c      |   38 +++++++++++++++++++++++++++++++++++---
 notmuch-client.h |   14 +++++++++++++-
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 27077f7..d6b4506 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -112,6 +112,11 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
     root->nchildren = 1;
     root->ctx = mctx;
 
+    root->parent = NULL;
+    root->part_num = 0;
+    root->next_child = 0;
+    root->next_part_num = 1;
+
     *root_out = root;
     return NOTMUCH_STATUS_SUCCESS;
 
@@ -137,7 +142,7 @@ _signature_validity_free (GMimeSignatureValidity **proxy)
 #endif
 
 static mime_node_t *
-_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+_mime_node_create (mime_node_t *parent, GMimeObject *part)
 {
     mime_node_t *node = talloc_zero (parent, mime_node_t);
     GError *err = NULL;
@@ -150,6 +155,9 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
 	talloc_free (node);
 	return NULL;
     }
+    node->parent = parent;
+    node->part_num = node->next_part_num = -1;
+    node->next_child = 0;
 
     /* Deal with the different types of parts */
     if (GMIME_IS_PART (part)) {
@@ -267,9 +275,10 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
 }
 
 mime_node_t *
-mime_node_child (const mime_node_t *parent, int child)
+mime_node_child (mime_node_t *parent, int child)
 {
     GMimeObject *sub;
+    mime_node_t *node;
 
     if (!parent || child < 0 || child >= parent->nchildren)
 	return NULL;
@@ -287,7 +296,30 @@ mime_node_child (const mime_node_t *parent, int child)
 	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
 			g_type_name (G_OBJECT_TYPE (parent->part)));
     }
-    return _mime_node_create (parent, sub);
+    node = _mime_node_create (parent, sub);
+
+    if (child == parent->next_child && parent->next_part_num != -1) {
+	/* We're traversing in depth-first order.  Record the child's
+	 * depth-first numbering. */
+	node->part_num = parent->next_part_num;
+	node->next_part_num = node->part_num + 1;
+
+	/* Prepare the parent for its next depth-first child. */
+	parent->next_child++;
+	parent->next_part_num = -1;
+
+	if (node->nchildren == 0) {
+	    /* We've reached a leaf, so find the parent that has more
+	     * children and set it up to number its next child. */
+	    mime_node_t *iter = node->parent;
+	    while (iter && iter->next_child == iter->nchildren)
+		iter = iter->parent;
+	    if (iter)
+		iter->next_part_num = node->part_num + 1;
+	}
+    }
+
+    return node;
 }
 
 static mime_node_t *
diff --git a/notmuch-client.h b/notmuch-client.h
index 9c1d383..abfe5d3 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -297,6 +297,13 @@ typedef struct mime_node {
     /* The number of children of this part. */
     int nchildren;
 
+    /* The parent of this node or NULL if this is the root node. */
+    struct mime_node *parent;
+
+    /* The depth-first part number of this child if the MIME tree is
+     * being traversed in depth-first order, or -1 otherwise. */
+    int part_num;
+
     /* True if decryption of this part was attempted. */
     notmuch_bool_t decrypt_attempted;
     /* True if decryption of this part's child succeeded.  In this
@@ -324,6 +331,11 @@ typedef struct mime_node {
     /* Internal: For successfully decrypted multipart parts, the
      * decrypted part to substitute for the second child. */
     GMimeObject *decrypted_child;
+
+    /* Internal: The next child for depth-first traversal and the part
+     * number to assign it (or -1 if unknown). */
+    int next_child;
+    int next_part_num;
 } mime_node_t;
 
 /* Construct a new MIME node pointing to the root message part of
@@ -356,7 +368,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
  * an error message on stderr).
  */
 mime_node_t *
-mime_node_child (const mime_node_t *parent, int child);
+mime_node_child (mime_node_t *parent, int child);
 
 /* Return the nth child of node in a depth-first traversal.  If n is
  * 0, returns node itself.  Returns NULL if there is no such part. */
-- 
1.7.7.3

[PATCH v5 2/2] show: Introduce mime_node formatter callback

[Austin Clements]

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   |   21 +++++++++++++++++----
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index abfe5d3..093c789 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,
+		  const 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 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..dec799c 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,19 @@ 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 &&
+	    (part = mime_node_seek_dfs (root, (params->part < 0 ?
+					       0 : params->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

Re: [PATCH v5 1/2] mime node: Record depth-first part numbers

[Dmitry Kurochkin]

On Mon, 23 Jan 2012 18:33:09 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This makes the part numbers readily accessible to formatters.
> Hierarchical part numbering would be a more natural and efficient fit
> for MIME and may be the way to go in the future, but depth-first
> numbering maintains compatibility with what we currently do.

LGTM

Regards,
  Dmitry

> ---
>  mime-node.c      |   38 +++++++++++++++++++++++++++++++++++---
>  notmuch-client.h |   14 +++++++++++++-
>  2 files changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/mime-node.c b/mime-node.c
> index 27077f7..d6b4506 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -112,6 +112,11 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
>      root->nchildren = 1;
>      root->ctx = mctx;
>  
> +    root->parent = NULL;
> +    root->part_num = 0;
> +    root->next_child = 0;
> +    root->next_part_num = 1;
> +
>      *root_out = root;
>      return NOTMUCH_STATUS_SUCCESS;
>  
> @@ -137,7 +142,7 @@ _signature_validity_free (GMimeSignatureValidity **proxy)
>  #endif
>  
>  static mime_node_t *
> -_mime_node_create (const mime_node_t *parent, GMimeObject *part)
> +_mime_node_create (mime_node_t *parent, GMimeObject *part)
>  {
>      mime_node_t *node = talloc_zero (parent, mime_node_t);
>      GError *err = NULL;
> @@ -150,6 +155,9 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
>  	talloc_free (node);
>  	return NULL;
>      }
> +    node->parent = parent;
> +    node->part_num = node->next_part_num = -1;
> +    node->next_child = 0;
>  
>      /* Deal with the different types of parts */
>      if (GMIME_IS_PART (part)) {
> @@ -267,9 +275,10 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
>  }
>  
>  mime_node_t *
> -mime_node_child (const mime_node_t *parent, int child)
> +mime_node_child (mime_node_t *parent, int child)
>  {
>      GMimeObject *sub;
> +    mime_node_t *node;
>  
>      if (!parent || child < 0 || child >= parent->nchildren)
>  	return NULL;
> @@ -287,7 +296,30 @@ mime_node_child (const mime_node_t *parent, int child)
>  	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
>  			g_type_name (G_OBJECT_TYPE (parent->part)));
>      }
> -    return _mime_node_create (parent, sub);
> +    node = _mime_node_create (parent, sub);
> +
> +    if (child == parent->next_child && parent->next_part_num != -1) {
> +	/* We're traversing in depth-first order.  Record the child's
> +	 * depth-first numbering. */
> +	node->part_num = parent->next_part_num;
> +	node->next_part_num = node->part_num + 1;
> +
> +	/* Prepare the parent for its next depth-first child. */
> +	parent->next_child++;
> +	parent->next_part_num = -1;
> +
> +	if (node->nchildren == 0) {
> +	    /* We've reached a leaf, so find the parent that has more
> +	     * children and set it up to number its next child. */
> +	    mime_node_t *iter = node->parent;
> +	    while (iter && iter->next_child == iter->nchildren)
> +		iter = iter->parent;
> +	    if (iter)
> +		iter->next_part_num = node->part_num + 1;
> +	}
> +    }
> +
> +    return node;
>  }
>  
>  static mime_node_t *
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 9c1d383..abfe5d3 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -297,6 +297,13 @@ typedef struct mime_node {
>      /* The number of children of this part. */
>      int nchildren;
>  
> +    /* The parent of this node or NULL if this is the root node. */
> +    struct mime_node *parent;
> +
> +    /* The depth-first part number of this child if the MIME tree is
> +     * being traversed in depth-first order, or -1 otherwise. */
> +    int part_num;
> +
>      /* True if decryption of this part was attempted. */
>      notmuch_bool_t decrypt_attempted;
>      /* True if decryption of this part's child succeeded.  In this
> @@ -324,6 +331,11 @@ typedef struct mime_node {
>      /* Internal: For successfully decrypted multipart parts, the
>       * decrypted part to substitute for the second child. */
>      GMimeObject *decrypted_child;
> +
> +    /* Internal: The next child for depth-first traversal and the part
> +     * number to assign it (or -1 if unknown). */
> +    int next_child;
> +    int next_part_num;
>  } mime_node_t;
>  
>  /* Construct a new MIME node pointing to the root message part of
> @@ -356,7 +368,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
>   * an error message on stderr).
>   */
>  mime_node_t *
> -mime_node_child (const mime_node_t *parent, int child);
> +mime_node_child (mime_node_t *parent, int child);
>  
>  /* Return the nth child of node in a depth-first traversal.  If n is
>   * 0, returns node itself.  Returns NULL if there is no such part. */
> -- 
> 1.7.7.3
> 

Re: [PATCH v5 2/2] show: Introduce mime_node formatter callback

[Dmitry Kurochkin]

On Mon, 23 Jan 2012 18:33:10 -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.

LGTM

Regards,
  Dmitry

> ---
>  notmuch-client.h |    6 ++++++
>  notmuch-reply.c  |    2 +-
>  notmuch-show.c   |   21 +++++++++++++++++----
>  3 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/notmuch-client.h b/notmuch-client.h
> index abfe5d3..093c789 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,
> +		  const 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 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..dec799c 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,19 @@ 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 &&
> +	    (part = mime_node_seek_dfs (root, (params->part < 0 ?
> +					       0 : params->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
> 

Re: [PATCH v5 1/2] mime node: Record depth-first part numbers

[Tomi Ollila]

On Mon, 23 Jan 2012 18:33:09 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This makes the part numbers readily accessible to formatters.
> Hierarchical part numbering would be a more natural and efficient fit
> for MIME and may be the way to go in the future, but depth-first
> numbering maintains compatibility with what we currently do.
> ---

LGTM. Good that the comment about dropping const is gone.

Tomi

Re: [PATCH v5 2/2] show: Introduce mime_node formatter callback

[Tomi Ollila]

On Mon, 23 Jan 2012 18:33:10 -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.
> ---

LGTM.

Tomi

Re: [PATCH v5 2/2] show: Introduce mime_node formatter callback

[Jani Nikula]

On Mon, 23 Jan 2012 18:33:10 -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   |   21 +++++++++++++++++----
>  3 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/notmuch-client.h b/notmuch-client.h
> index abfe5d3..093c789 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,
> +		  const 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 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..dec799c 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,19 @@ 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 &&
> +	    (part = mime_node_seek_dfs (root, (params->part < 0 ?
> +					       0 : params->part))))
> +	    format->part (local, part, indent, params);

I know you did this because Dmitry asked to remove the goto, but I'd
argue this is *much* less readable than the original with goto. There's
just way too much going on in the if. Two calls, an assignment, a
ternary operator. KISS.

BR,
Jani.





> +	talloc_free (local);
> +	return;
> +    }
> +
>      if (params->part <= 0) {
>  	fputs (format->message_start, stdout);
>  	if (format->message)
> -- 
> 1.7.7.3
> 

Re: [PATCH v5 0/2] Second step of 'show' rewrite

[David Bremner]

On Mon, 23 Jan 2012 18:33:08 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Arrg, sorry for the spam.  I grabbed the wrong commit ID when sending
> v4.
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

v5 pushed