Date: Mon, 23 Jan 2012 18:15:35 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
Subject: Re: [PATCH v2 1/3] mime node: Record depth-first part numbers
Message-ID: <20120123231535.GV16740@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>
	<8762g2njto.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8762g2njto.fsf@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

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.