Date: Wed, 18 Jan 2012 21:12:54 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [PATCH 1/3] mime node: Record depth-first part numbers
Message-ID: <20120119021254.GJ16740@mit.edu>
References: <1326918507-28033-1-git-send-email-amdragon@mit.edu>
	<1326918507-28033-2-git-send-email-amdragon@mit.edu>
	<87zkdkbqc6.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87zkdkbqc6.fsf@nikula.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org, dkg@fifthhorseman.net
Precedence: list

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
>