22/cbc9810ed1d781dfba2610e4e5e00e061d6d30

   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 BC6CC431FAF\r
   6         for <notmuch@notmuchmail.org>; Wed, 18 Jan 2012 18:24:07 -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 CTxMdQQHg9Mg for <notmuch@notmuchmail.org>;\r
  16         Wed, 18 Jan 2012 18:24:07 -0800 (PST)\r
  17 Received: from dmz-mailsec-scanner-8.mit.edu (DMZ-MAILSEC-SCANNER-8.MIT.EDU\r
  18         [18.7.68.37])\r
  19         by olra.theworths.org (Postfix) with ESMTP id F307D431FAE\r
  20         for <notmuch@notmuchmail.org>; Wed, 18 Jan 2012 18:24:06 -0800 (PST)\r
  21 X-AuditID: 12074425-b7f4a6d0000008e0-ac-4f177ec508ca\r
  22 Received: from mailhub-auth-3.mit.edu ( [18.9.21.43])\r
  23         by dmz-mailsec-scanner-8.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id EF.DA.02272.5CE771F4; Wed, 18 Jan 2012 21:24:05 -0500 (EST)\r
  25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
  26         by mailhub-auth-3.mit.edu (8.13.8/8.9.2) with ESMTP id q0J2O4Xu023561; \r
  27         Wed, 18 Jan 2012 21:24:04 -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 q0J2O3Ie004223\r
  32         (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
  33         Wed, 18 Jan 2012 21:24:04 -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 1RnhfI-00028F-2x; Wed, 18 Jan 2012 21:23:48 -0500\r
  37 Date: Wed, 18 Jan 2012 21:23:48 -0500\r
  38 From: Austin Clements <amdragon@MIT.EDU>\r
  39 To: Jani Nikula <jani@nikula.org>\r
  40 Subject: Re: [PATCH 3/3] show: Introduce mime_node formatter callback\r
  41 Message-ID: <20120119022348.GK16740@mit.edu>\r
  42 References: <1326918507-28033-1-git-send-email-amdragon@mit.edu>\r
  43         <1326918507-28033-4-git-send-email-amdragon@mit.edu>\r
  44         <87wr8obpyv.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: <87wr8obpyv.fsf@nikula.org>\r
  49 User-Agent: Mutt/1.5.21 (2010-09-15)\r
  50 X-Brightmail-Tracker:\r
  51  H4sIAAAAAAAAA+NgFprHKsWRmVeSWpSXmKPExsUixCmqrXu0TtzfoP2xpkVr92cmi6tb+9kt\r
  52         mqY7W+zZ52Vx/eZMZgdWj7Pd7awed09zeeycdZfd49b91+wez1bdYg5gjeKySUnNySxLLdK3\r
  53         S+DK6P/Wxl6wRaXi4vufTA2Me6S7GDk5JARMJG4/X8oGYYtJXLi3Hsjm4hAS2McosazlLQuE\r
  54         s4FR4syGXlYI5ySTxNYXpxghnCWMEutv3mMC6WcRUJVoaZ/EDmKzCWhIbNu/nBHEFhFQlNh8\r
  55         cj+QzcHBLFAs0fOxCsQUFnCVWPtBFaSCV0BHYs+clewQI6cyStz4d4INIiEocXLmExYQm1lA\r
  56         Cyj+kglijLTE8n8cIGFOoE0bvr9kBrFFBVQkppzcxjaBUWgWku5ZSLpnIXQvYGRexSibklul\r
  57         m5uYmVOcmqxbnJyYl5dapGuhl5tZopeaUrqJERQN7C6qOxgnHFI6xCjAwajEwxshIu4vxJpY\r
  58         VlyZe4hRkoNJSZT3QDVQiC8pP6UyI7E4I76oNCe1+BCjBAezkgjvThOgHG9KYmVValE+TEqa\r
  59         g0VJnFdT652fkEB6YklqdmpqQWoRTFaGg0NJgreiBqhRsCg1PbUiLTOnBCHNxMEJMpwHaPgd\r
  60         kMW8xQWJucWZ6RD5U4yKUuK8VsB0IyQAksgozYPrhSWrV4ziQK8I856oBariASY6uO5XQIOZ\r
  61         gAZ7NImBDC5JREhJNTAuzl3vmiz8z0bR/ZZsbfLKsg2v/+8LV9J36zOMnvX/e9eOBY2Tjefx\r
  62         SM5+9HHi+Zm76iVtwwrnVUVuP1F8SnE/g+2vV1MVpynPX3Wc81KB+AfXQxOvdHDqpmd7RaVN\r
  63         1yu08RXXtv5rZvlto6asj9hsV1vFFKuPAWdWsk6edvNNbo1Ajeo8179KLMUZiYZazEXFiQDQ\r
  64         aoPjMQMAAA==\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:24:07 -0000\r
  79 \r
  80 Quoth Jani Nikula on Jan 19 at 12:33 am:\r
  81 > On Wed, 18 Jan 2012 15:28:27 -0500, Austin Clements <amdragon@MIT.EDU> wrote:\r
  82 > > This callback is the gateway to the new mime_node_t-based formatters.\r
  83 > > This maintains backwards compatibility so the formatters can be\r
  84 > > transitioned one at a time.  Once all formatters are converted, the\r
  85 > > formatter structure can be reduced to only message_set_{start,sep,end}\r
  86 > > and part, most of show_message can be deleted, and all of\r
  87 > > show-message.c can be deleted.\r
  88 > > ---\r
  89 > >  notmuch-client.h |    6 ++++++\r
  90 > >  notmuch-reply.c  |    2 +-\r
  91 > >  notmuch-show.c   |   22 ++++++++++++++++++----\r
  92 > >  3 files changed, 25 insertions(+), 5 deletions(-)\r
  93 > > \r
  94 > > diff --git a/notmuch-client.h b/notmuch-client.h\r
  95 > > index b3dcb6b..3ccdfad 100644\r
  96 > > --- a/notmuch-client.h\r
  97 > > +++ b/notmuch-client.h\r
  98 > > @@ -54,8 +54,14 @@\r
  99 > >  #define STRINGIFY(s) STRINGIFY_(s)\r
 100 > >  #define STRINGIFY_(s) #s\r
 101 > >  \r
 102 > > +struct mime_node;\r
 103 > > +struct notmuch_show_params;\r
 104 > > +\r
 105 > >  typedef struct notmuch_show_format {\r
 106 > >      const char *message_set_start;\r
 107 > > +    void (*part) (const void *ctx,\r
 108 > > +             struct mime_node *node, int indent,\r
 109 > > +             struct notmuch_show_params *params);\r
 110 > >      const char *message_start;\r
 111 > >      void (*message) (const void *ctx,\r
 112 > >                  notmuch_message_t *message,\r
 113 > > diff --git a/notmuch-reply.c b/notmuch-reply.c\r
 114 > > index 0f682db..9a224e2 100644\r
 115 > > --- a/notmuch-reply.c\r
 116 > > +++ b/notmuch-reply.c\r
 117 > > @@ -31,7 +31,7 @@ static void\r
 118 > >  reply_part_content (GMimeObject *part);\r
 119 > >  \r
 120 > >  static const notmuch_show_format_t format_reply = {\r
 121 > > -    "",\r
 122 > > +    "", NULL,\r
 123 > >     "", NULL,\r
 124 > >         "", NULL, reply_headers_message_part, ">\n",\r
 125 > >         "",\r
 126 > > diff --git a/notmuch-show.c b/notmuch-show.c\r
 127 > > index ecadfa8..46eef44 100644\r
 128 > > --- a/notmuch-show.c\r
 129 > > +++ b/notmuch-show.c\r
 130 > > @@ -42,7 +42,7 @@ static void\r
 131 > >  format_part_end_text (GMimeObject *part);\r
 132 > >  \r
 133 > >  static const notmuch_show_format_t format_text = {\r
 134 > > -    "",\r
 135 > > +    "", NULL,\r
 136 > >     "\fmessage{ ", format_message_text,\r
 137 > >         "\fheader{\n", format_headers_text, format_headers_message_part_text, "\fheader}\n",\r
 138 > >         "\fbody{\n",\r
 139 > > @@ -85,7 +85,7 @@ static void\r
 140 > >  format_part_end_json (GMimeObject *part);\r
 141 > >  \r
 142 > >  static const notmuch_show_format_t format_json = {\r
 143 > > -    "[",\r
 144 > > +    "[", NULL,\r
 145 > >     "{", format_message_json,\r
 146 > >         "\"headers\": {", format_headers_json, format_headers_message_part_json, "}",\r
 147 > >         ", \"body\": [",\r
 148 > > @@ -106,7 +106,7 @@ format_message_mbox (const void *ctx,\r
 149 > >                  unused (int indent));\r
 150 > >  \r
 151 > >  static const notmuch_show_format_t format_mbox = {\r
 152 > > -    "",\r
 153 > > +    "", NULL,\r
 154 > >          "", format_message_mbox,\r
 155 > >              "", NULL, NULL, "",\r
 156 > >              "",\r
 157 > > @@ -125,7 +125,7 @@ static void\r
 158 > >  format_part_content_raw (GMimeObject *part);\r
 159 > >  \r
 160 > >  static const notmuch_show_format_t format_raw = {\r
 161 > > -    "",\r
 162 > > +    "", NULL,\r
 163 > >     "", NULL,\r
 164 > >         "", NULL, format_headers_message_part_text, "\n",\r
 165 > >              "",\r
 166 > > @@ -762,6 +762,20 @@ show_message (void *ctx,\r
 167 > >           int indent,\r
 168 > >           notmuch_show_params_t *params)\r
 169 > >  {\r
 170 > > +    if (format->part) {\r
 171 > > +   void *local = talloc_new (ctx);\r
 172 > > +   mime_node_t *root, *part;\r
 173 > > +\r
 174 > > +   if (mime_node_open (local, message, params->cryptoctx, params->decrypt,\r
 175 > > +                       &root) != NOTMUCH_STATUS_SUCCESS)\r
 176 > \r
 177 > I'm new to talloc, I think I like it, but I always find it confusing\r
 178 > when some code paths free the contexts, and some don't. Like here.\r
 179 > \r
 180 > Are you not freeing the local context here because it's just an empty\r
 181 > context, and freeing below because it's no longer empty?\r
 182 \r
 183 No, that's just a bug.  In practice it probably doesn't matter much\r
 184 because, as you pointed out, the local context consumes very little\r
 185 memory and since the caller will eventually free ctx, local will get\r
 186 cleaned up, too.  So, while this isn't strictly a memory leak, in\r
 187 principle this could add up before ctx gets freed.  Fixed.\r
 188 \r
 189 > No need to change anything, I guess, just asking...\r
 190 \r
 191 I wait a little for other comments and then send a new version.\r
 192 \r
 193 Thanks for the review!\r
 194 \r
 195 > BR,\r
 196 > Jani.\r
 197 > \r
 198 > > +       return;\r
 199 > > +   part = mime_node_seek_dfs (root, params->part < 0 ? 0 : params->part);\r
 200 > > +   if (part)\r
 201 > > +       format->part (local, part, indent, params);\r
 202 > > +   talloc_free (local);\r
 203 > > +   return;\r
 204 > > +    }\r
 205 > > +\r
 206 > >      if (params->part <= 0) {\r
 207 > >     fputs (format->message_start, stdout);\r
 208 > >     if (format->message)\r