From: Jani Nikula <jani@nikula.org>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH 1/3] mime node: Record depth-first part numbers
In-Reply-To: <1326918507-28033-2-git-send-email-amdragon@mit.edu>
References: <1326918507-28033-1-git-send-email-amdragon@mit.edu>
	<1326918507-28033-2-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.11+76~g1de742d (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Thu, 19 Jan 2012 00:25:45 +0200
Message-ID: <87zkdkbqc6.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: dkg@fifthhorseman.net
Precedence: list

On Wed, 18 Jan 2012 15:28: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
>