From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 1/3] mime node: Record depth-first part numbers
In-Reply-To: <1327285873-4713-2-git-send-email-amdragon@mit.edu>
References: <1326918507-28033-1-git-send-email-amdragon@mit.edu>
	<1327285873-4713-1-git-send-email-amdragon@mit.edu>
	<1327285873-4713-2-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.11+100~gd650abf (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Tue, 24 Jan 2012 02:19:15 +0400
Message-ID: <8762g2njto.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

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