23/c6dc3b4fcd30c4a792bf7944f0beeb683eb1ca

   1 Return-Path: <amdragon@mit.edu>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id DDF6C431FAF\r
   6         for <notmuch@notmuchmail.org>; Wed, 18 Jan 2012 18:13:12 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id f6HiOqIWNB6m for <notmuch@notmuchmail.org>;\r
  16         Wed, 18 Jan 2012 18:13:12 -0800 (PST)\r
  17 Received: from dmz-mailsec-scanner-4.mit.edu (DMZ-MAILSEC-SCANNER-4.MIT.EDU\r
  18         [18.9.25.15])\r
  19         by olra.theworths.org (Postfix) with ESMTP id 14660431FAE\r
  20         for <notmuch@notmuchmail.org>; Wed, 18 Jan 2012 18:13:12 -0800 (PST)\r
  21 X-AuditID: 1209190f-b7f8a6d000000914-b2-4f177c3789c4\r
  22 Received: from mailhub-auth-4.mit.edu ( [18.7.62.39])\r
  23         by dmz-mailsec-scanner-4.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id 86.30.02324.73C771F4; Wed, 18 Jan 2012 21:13:11 -0500 (EST)\r
  25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
  26         by mailhub-auth-4.mit.edu (8.13.8/8.9.2) with ESMTP id q0J2DA4E013541; \r
  27         Wed, 18 Jan 2012 21:13:10 -0500\r
  28 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
  29         (authenticated bits=0)\r
  30         (User authenticated as amdragon@ATHENA.MIT.EDU)\r
  31         by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q0J2D9DQ002571\r
  32         (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
  33         Wed, 18 Jan 2012 21:13:09 -0500 (EST)\r
  34 Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77)\r
  35         (envelope-from <amdragon@mit.edu>)\r
  36         id 1RnhUk-00027Q-1C; Wed, 18 Jan 2012 21:12:54 -0500\r
  37 Date: Wed, 18 Jan 2012 21:12:54 -0500\r
  38 From: Austin Clements <amdragon@MIT.EDU>\r
  39 To: Jani Nikula <jani@nikula.org>\r
  40 Subject: Re: [PATCH 1/3] mime node: Record depth-first part numbers\r
  41 Message-ID: <20120119021254.GJ16740@mit.edu>\r
  42 References: <1326918507-28033-1-git-send-email-amdragon@mit.edu>\r
  43         <1326918507-28033-2-git-send-email-amdragon@mit.edu>\r
  44         <87zkdkbqc6.fsf@nikula.org>\r
  45 MIME-Version: 1.0\r
  46 Content-Type: text/plain; charset=us-ascii\r
  47 Content-Disposition: inline\r
  48 In-Reply-To: <87zkdkbqc6.fsf@nikula.org>\r
  49 User-Agent: Mutt/1.5.21 (2010-09-15)\r
  50 X-Brightmail-Tracker:\r
  51  H4sIAAAAAAAAA+NgFprNKsWRmVeSWpSXmKPExsUixG6nrmteI+5v8K1d1qK1+zOTxdWt/ewW\r
  52         TdOdLfbs87K4fnMmswOrx9nudlaPu6e5PHbOusvucev+a3aPZ6tuMQewRnHZpKTmZJalFunb\r
  53         JXBlzJ49n7lgi27F0ZeT2BoYDyh1MXJySAiYSJxcPYsNwhaTuHBvPZDNxSEksI9R4sn6q+wQ\r
  54         zgZGiel7G1ghnJNMEheXHgFrERJYwiix/CAviM0ioCpxcNZHZhCbTUBDYtv+5YwgtoiAosTm\r
  55         k/uBbA4OZoFiiZ6PVSCmsICzRN+XQBCTV0BHYtldN4iBUxklpq0PBbF5BQQlTs58wgJiMwto\r
  56         Sdz495IJYoi0xPJ/HCBhTqA9izfuB9spKqAiMeXkNrYJjEKzkHTPQtI9C6F7ASPzKkbZlNwq\r
  57         3dzEzJzi1GTd4uTEvLzUIl0TvdzMEr3UlNJNjKA44JTk38H47aDSIUYBDkYlHt5IEXF/IdbE\r
  58         suLK3EOMkhxMSqK8B6qBQnxJ+SmVGYnFGfFFpTmpxYcYJTiYlUR4d5oA5XhTEiurUovyYVLS\r
  59         HCxK4rxqWu/8hATSE0tSs1NTC1KLYLIyHBxKEryHQYYKFqWmp1akZeaUIKSZODhBhvMADZ8P\r
  60         UsNbXJCYW5yZDpE/xagoJc47AyQhAJLIKM2D64WlqVeM4kCvCPOuA6niAaY4uO5XQIOZgAZ7\r
  61         NImBDC5JREhJNTCms/CXHWY+NPmYdZ2EJcc6iWaWv+VLrf79n/Q/z+7k+lmvtpy5EdFk+tY2\r
  62         8Kq+n7UBy/OH/alP/obf/PTk5tGvBfVvdix5detG4n+fg4cOxC7dcMbMW1f7lzfL6f+Tl0ls\r
  63         lN3DX7iXf1HrndhPM66cq94zpWvH5QVHMtgLvT4cu9Du9sS2JP5nixJLcUaioRZzUXEiAHYT\r
  64         +G4uAwAA\r
  65 Cc: notmuch@notmuchmail.org, dkg@fifthhorseman.net\r
  66 X-BeenThere: notmuch@notmuchmail.org\r
  67 X-Mailman-Version: 2.1.13\r
  68 Precedence: list\r
  69 List-Id: "Use and development of the notmuch mail system."\r
  70         <notmuch.notmuchmail.org>\r
  71 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  72         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  73 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  74 List-Post: <mailto:notmuch@notmuchmail.org>\r
  75 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  76 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  77         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  78 X-List-Received-Date: Thu, 19 Jan 2012 02:13:13 -0000\r
  79 \r
  80 Quoth Jani Nikula on Jan 19 at 12:25 am:\r
  81 > On Wed, 18 Jan 2012 15:28:25 -0500, Austin Clements <amdragon@MIT.EDU> wrote:\r
  82 > > This makes the part numbers readily accessible to formatters.\r
  83 > > Hierarchical part numbering would be a more natural and efficient fit\r
  84 > > for MIME and may be the way to go in the future, but depth-first\r
  85 > > numbering maintains compatibility with what we currently do.\r
  86 > \r
  87 > Hi, please find a few things to consider below. If you disagree after\r
  88 > considering, it's quite all right, as they're largely style matters. :)\r
  89 > \r
  90 > BR,\r
  91 > Jani.\r
  92 > \r
  93 > \r
  94 > > ---\r
  95 > >  mime-node.c      |   33 ++++++++++++++++++++++++++++++++-\r
  96 > >  notmuch-client.h |   11 +++++++++++\r
  97 > >  2 files changed, 43 insertions(+), 1 deletions(-)\r
  98 > > \r
  99 > > diff --git a/mime-node.c b/mime-node.c\r
 100 > > index d26bb44..30b542f 100644\r
 101 > > --- a/mime-node.c\r
 102 > > +++ b/mime-node.c\r
 103 > > @@ -104,6 +104,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,\r
 104 > >      root->nchildren = 1;\r
 105 > >      root->ctx = mctx;\r
 106 > >  \r
 107 > > +    root->part_num = 0;\r
 108 > > +    root->next_child = 0;\r
 109 > > +    root->next_part_num = 1;\r
 110 > > +\r
 111 > >      *root_out = root;\r
 112 > >      return NOTMUCH_STATUS_SUCCESS;\r
 113 > >  \r
 114 > > @@ -133,6 +137,8 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)\r
 115 > >     talloc_free (node);\r
 116 > >     return NULL;\r
 117 > >      }\r
 118 > > +    node->parent = parent;\r
 119 > > +    node->part_num = node->next_part_num = -1;\r
 120 > >  \r
 121 > >      /* Deal with the different types of parts */\r
 122 > >      if (GMIME_IS_PART (part)) {\r
 123 > > @@ -217,6 +223,7 @@ mime_node_t *\r
 124 > >  mime_node_child (const mime_node_t *parent, int child)\r
 125 > >  {\r
 126 > >      GMimeObject *sub;\r
 127 > > +    mime_node_t *node;\r
 128 > >  \r
 129 > >      if (!parent || child < 0 || child >= parent->nchildren)\r
 130 > >     return NULL;\r
 131 > > @@ -234,7 +241,31 @@ mime_node_child (const mime_node_t *parent, int child)\r
 132 > >     INTERNAL_ERROR ("Unexpected GMimeObject type: %s",\r
 133 > >                     g_type_name (G_OBJECT_TYPE (parent->part)));\r
 134 > >      }\r
 135 > > -    return _mime_node_create (parent, sub);\r
 136 > > +    node = _mime_node_create (parent, sub);\r
 137 > > +\r
 138 > > +    if (child == parent->next_child && parent->next_part_num != -1) {\r
 139 > > +   /* We're traversing in depth-first order.  Record the child's\r
 140 > > +    * depth-first numbering. */\r
 141 > > +   node->part_num = parent->next_part_num;\r
 142 > > +   node->next_part_num = node->part_num + 1;\r
 143 > > +\r
 144 > > +   /* Drop the const qualifier because these are internal fields\r
 145 > > +    * whose mutability doesn't affect the interface. */\r
 146 > \r
 147 > FWIW, I'm not a big fan of casting away const. Either it is const, or it\r
 148 > isn't. Not very many places would be affected if you dropped the const\r
 149 > qualifier from the related interface(s) altogether, and things would\r
 150 > look cleaner here. But I suppose this is a matter of taste.\r
 151 \r
 152 I'm not particularly happy with this either.  Unfortunately, dropping\r
 153 the const here affects a surprising number of places, including the\r
 154 entire MIME node API.\r
 155 \r
 156 I think that, at a deep level, depth-first numbering simply doesn't\r
 157 resonate with an extremely hierarchical API like this and that\r
 158 dissonance is going to have to focus somewhere.  There have been\r
 159 discussions of switching to hierarchical part numbering before (in\r
 160 particular, because depth-first numbering is unstable with encrypted\r
 161 parts) and I'll probably restart those after all of this is done.\r
 162 \r
 163 > > +   ((mime_node_t*)parent)->next_child++;\r
 164 > > +   ((mime_node_t*)parent)->next_part_num = -1;\r
 165 > > +\r
 166 > > +   if (node->nchildren == 0) {\r
 167 > > +       /* We've reached a leaf, so find the parent that has more\r
 168 > > +        * children and set it up to number its next child. */\r
 169 > > +       const mime_node_t *it = node;\r
 170 > > +       while (it && it->next_child == it->nchildren)\r
 171 > > +           it = it->parent;\r
 172 > > +       if (it)\r
 173 > > +           ((mime_node_t*)it)->next_part_num = node->part_num + 1;\r
 174 > > +   }\r
 175 > > +    }\r
 176 > \r
 177 > Following the constness around made me wonder why the above if block\r
 178 > isn't within _mime_node_create(). It does have a feel of initialization\r
 179 > related to creation in it. (You'd have to pass more info to it though.)\r
 180 \r
 181 I considered this, but was dissuaded by having to pass redundant\r
 182 information to _mime_node_create.  I think it's okay here because it\r
 183 has as much to do with child fetching as with initialization, but I\r
 184 could easily be swayed.\r
 185 \r
 186 > > +\r
 187 > > +    return node;\r
 188 > >  }\r
 189 > >  \r
 190 > >  static mime_node_t *\r
 191 > > diff --git a/notmuch-client.h b/notmuch-client.h\r
 192 > > index 62ede28..b3dcb6b 100644\r
 193 > > --- a/notmuch-client.h\r
 194 > > +++ b/notmuch-client.h\r
 195 > > @@ -281,6 +281,13 @@ typedef struct mime_node {\r
 196 > >      /* The number of children of this part. */\r
 197 > >      int nchildren;\r
 198 > >  \r
 199 > > +    /* The parent of this node or NULL if this is the root node. */\r
 200 > > +    const struct mime_node *parent;\r
 201 > > +\r
 202 > > +    /* The depth-first part number of this child if the MIME tree is\r
 203 > > +     * being traversed in depth-first order, or -1 otherwise. */\r
 204 > > +    int part_num;\r
 205 > > +\r
 206 > >      /* True if decryption of this part was attempted. */\r
 207 > >      notmuch_bool_t decrypt_attempted;\r
 208 > >      /* True if decryption of this part's child succeeded.  In this\r
 209 > > @@ -302,6 +309,10 @@ typedef struct mime_node {\r
 210 > >      /* Internal: For successfully decrypted multipart parts, the\r
 211 > >       * decrypted part to substitute for the second child. */\r
 212 > >      GMimeObject *decrypted_child;\r
 213 > > +\r
 214 > > +    /* Internal: The next child for depth-first traversal and the part\r
 215 > > +     * number to assign it (or -1 if unknown). */\r
 216 > > +    int next_child, next_part_num;\r
 217 > \r
 218 > A matter of taste again, but I wouldn't use "int foo, bar" in struct\r
 219 > declarations.\r
 220 \r
 221 Fixed.\r
 222 \r
 223 > >  } mime_node_t;\r
 224 > >  \r
 225 > >  /* Construct a new MIME node pointing to the root message part of\r
 226 > \r