34/54e3f03777935945abc1fa88f6000cc07f2ead

   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 3907B429E54\r
   6         for <notmuch@notmuchmail.org>; Mon, 23 Jan 2012 15:23:38 -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 tLT5nomUawmZ for <notmuch@notmuchmail.org>;\r
  16         Mon, 23 Jan 2012 15:23:37 -0800 (PST)\r
  17 Received: from dmz-mailsec-scanner-5.mit.edu (DMZ-MAILSEC-SCANNER-5.MIT.EDU\r
  18         [18.7.68.34])\r
  19         by olra.theworths.org (Postfix) with ESMTP id 7B709429E21\r
  20         for <notmuch@notmuchmail.org>; Mon, 23 Jan 2012 15:23:37 -0800 (PST)\r
  21 X-AuditID: 12074422-b7fd66d0000008f9-a1-4f1debf8bdab\r
  22 Received: from mailhub-auth-3.mit.edu ( [18.9.21.43])\r
  23         by dmz-mailsec-scanner-5.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id 73.14.02297.8FBED1F4; Mon, 23 Jan 2012 18:23:36 -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 q0NNNaMP001328; \r
  27         Mon, 23 Jan 2012 18:23:36 -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 q0NNNZZG023190\r
  32         (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
  33         Mon, 23 Jan 2012 18:23:36 -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 1RpTE9-0004Cu-Eu; Mon, 23 Jan 2012 18:23:05 -0500\r
  37 Date: Mon, 23 Jan 2012 18:23:05 -0500\r
  38 From: Austin Clements <amdragon@MIT.EDU>\r
  39 To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
  40 Subject: Re: [PATCH v2 3/3] show: Introduce mime_node formatter callback\r
  41 Message-ID: <20120123232305.GW16740@mit.edu>\r
  42 References: <1326918507-28033-1-git-send-email-amdragon@mit.edu>\r
  43         <1327285873-4713-1-git-send-email-amdragon@mit.edu>\r
  44         <1327285873-4713-4-git-send-email-amdragon@mit.edu>\r
  45         <8739b6niz9.fsf@gmail.com>\r
  46 MIME-Version: 1.0\r
  47 Content-Type: text/plain; charset=us-ascii\r
  48 Content-Disposition: inline\r
  49 In-Reply-To: <8739b6niz9.fsf@gmail.com>\r
  50 User-Agent: Mutt/1.5.21 (2010-09-15)\r
  51 X-Brightmail-Tracker:\r
  52  H4sIAAAAAAAAA+NgFmplleLIzCtJLcpLzFFi42IR4hTV1v3xWtbf4NNLC4urW/vZLa7fnMns\r
  53         wOSxc9Zddo9nq24xBzBFcdmkpOZklqUW6dslcGW8W3WFveCuSsW9LbuZGxhPSncxcnJICJhI\r
  54         nGvcwghhi0lcuLeerYuRi0NIYB+jxIQ3t5ghnA2MEi3b5rNAOCeZJC6s28sE4SxhlHi+9Rkz\r
  55         SD+LgKrExn0dbCA2m4CGxLb9y8HmiggYSty6+AqshllAWuLb72YmEFtYwEPiTtNZsDivgI7E\r
  56         uTUvWaE2MEpMmXmHESIhKHFy5hMWiGYtiRv/XgI1c4ANWv6PAyTMKaAu8f7CB7ByUQEViSkn\r
  57         t7FNYBSahaR7FpLuWQjdCxiZVzHKpuRW6eYmZuYUpybrFicn5uWlFuma6uVmluilppRuYgSF\r
  58         NruL0g7GnweVDjEKcDAq8fBKzJT1F2JNLCuuzD3EKMnBpCTK6wqMDCG+pPyUyozE4oz4otKc\r
  59         1OJDjBIczEoivGrngHK8KYmVValF+TApaQ4WJXFeda13fkIC6YklqdmpqQWpRTBZGQ4OJQne\r
  60         QJChgkWp6akVaZk5JQhpJg5OkOE8QMO9QGp4iwsSc4sz0yHypxgVpcR5vUESAiCJjNI8uF5Y\r
  61         6nnFKA70ijBvJEgVDzBtwXW/AhrMBDSYI08KZHBJIkJKqoFxxvrczU+KPgTO492fv/ZV6cGU\r
  62         lSdTkn/5sbJndRUI/qljK37NHebY8FlWLzDIksP37tmfm8It7wV+Y/ZJWrnw/yOT+fM2rPH7\r
  63         VjtfOP2vS6mCX2QGv0TGy1VVty/e5plse0h+y99uhg/7l/mt1lrvGLgrj1loQ8O08kuBlg+N\r
  64         vLc+rcrwMWlXYinOSDTUYi4qTgQAPacfwBgDAAA=\r
  65 Cc: notmuch@notmuchmail.org\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: Mon, 23 Jan 2012 23:23:38 -0000\r
  79 \r
  80 Thanks for the review.  New version coming shortly (v4, since the list\r
  81 ate v3 while everyone was still reading v2).\r
  82 \r
  83 Quoth Dmitry Kurochkin on Jan 24 at  2:37 am:\r
  84 > On Sun, 22 Jan 2012 21:31:13 -0500, Austin Clements <amdragon@MIT.EDU> wrote:\r
  85 > > This callback is the gateway to the new mime_node_t-based formatters.\r
  86 > > This maintains backwards compatibility so the formatters can be\r
  87 > > transitioned one at a time.  Once all formatters are converted, the\r
  88 > > formatter structure can be reduced to only message_set_{start,sep,end}\r
  89 > > and part, most of show_message can be deleted, and all of\r
  90 > > show-message.c can be deleted.\r
  91 > \r
  92 > Few comments below.\r
  93 > \r
  94 > > ---\r
  95 > >  notmuch-client.h |    6 ++++++\r
  96 > >  notmuch-reply.c  |    2 +-\r
  97 > >  notmuch-show.c   |   23 +++++++++++++++++++----\r
  98 > >  3 files changed, 26 insertions(+), 5 deletions(-)\r
  99 > > \r
 100 > > diff --git a/notmuch-client.h b/notmuch-client.h\r
 101 > > index abfe5d3..59606b4 100644\r
 102 > > --- a/notmuch-client.h\r
 103 > > +++ b/notmuch-client.h\r
 104 > > @@ -62,8 +62,14 @@\r
 105 > >  #define STRINGIFY(s) STRINGIFY_(s)\r
 106 > >  #define STRINGIFY_(s) #s\r
 107 > >  \r
 108 > > +struct mime_node;\r
 109 > > +struct notmuch_show_params;\r
 110 > > +\r
 111 > >  typedef struct notmuch_show_format {\r
 112 > >      const char *message_set_start;\r
 113 > > +    void (*part) (const void *ctx,\r
 114 > > +             struct mime_node *node, int indent,\r
 115 > > +             struct notmuch_show_params *params);\r
 116 > \r
 117 > Can we make the params parameter const?\r
 118 \r
 119 Apparently we can.\r
 120 \r
 121 > >      const char *message_start;\r
 122 > >      void (*message) (const void *ctx,\r
 123 > >                  notmuch_message_t *message,\r
 124 > > diff --git a/notmuch-reply.c b/notmuch-reply.c\r
 125 > > index bf67960..f55b1d2 100644\r
 126 > > --- a/notmuch-reply.c\r
 127 > > +++ b/notmuch-reply.c\r
 128 > > @@ -31,7 +31,7 @@ static void\r
 129 > >  reply_part_content (GMimeObject *part);\r
 130 > >  \r
 131 > >  static const notmuch_show_format_t format_reply = {\r
 132 > > -    "",\r
 133 > > +    "", NULL,\r
 134 > >     "", NULL,\r
 135 > >         "", NULL, reply_headers_message_part, ">\n",\r
 136 > >         "",\r
 137 > > diff --git a/notmuch-show.c b/notmuch-show.c\r
 138 > > index 682aa71..8185b02 100644\r
 139 > > --- a/notmuch-show.c\r
 140 > > +++ b/notmuch-show.c\r
 141 > > @@ -42,7 +42,7 @@ static void\r
 142 > >  format_part_end_text (GMimeObject *part);\r
 143 > >  \r
 144 > >  static const notmuch_show_format_t format_text = {\r
 145 > > -    "",\r
 146 > > +    "", NULL,\r
 147 > >     "\fmessage{ ", format_message_text,\r
 148 > >         "\fheader{\n", format_headers_text, format_headers_message_part_text, "\fheader}\n",\r
 149 > >         "\fbody{\n",\r
 150 > > @@ -89,7 +89,7 @@ static void\r
 151 > >  format_part_end_json (GMimeObject *part);\r
 152 > >  \r
 153 > >  static const notmuch_show_format_t format_json = {\r
 154 > > -    "[",\r
 155 > > +    "[", NULL,\r
 156 > >     "{", format_message_json,\r
 157 > >         "\"headers\": {", format_headers_json, format_headers_message_part_json, "}",\r
 158 > >         ", \"body\": [",\r
 159 > > @@ -110,7 +110,7 @@ format_message_mbox (const void *ctx,\r
 160 > >                  unused (int indent));\r
 161 > >  \r
 162 > >  static const notmuch_show_format_t format_mbox = {\r
 163 > > -    "",\r
 164 > > +    "", NULL,\r
 165 > >          "", format_message_mbox,\r
 166 > >              "", NULL, NULL, "",\r
 167 > >              "",\r
 168 > > @@ -129,7 +129,7 @@ static void\r
 169 > >  format_part_content_raw (GMimeObject *part);\r
 170 > >  \r
 171 > >  static const notmuch_show_format_t format_raw = {\r
 172 > > -    "",\r
 173 > > +    "", NULL,\r
 174 > >     "", NULL,\r
 175 > >         "", NULL, format_headers_message_part_text, "\n",\r
 176 > >              "",\r
 177 > > @@ -850,6 +850,21 @@ show_message (void *ctx,\r
 178 > >           int indent,\r
 179 > >           notmuch_show_params_t *params)\r
 180 > >  {\r
 181 > > +    if (format->part) {\r
 182 > > +   void *local = talloc_new (ctx);\r
 183 > > +   mime_node_t *root, *part;\r
 184 > > +\r
 185 > > +   if (mime_node_open (local, message, params->cryptoctx, params->decrypt,\r
 186 > > +                       &root) != NOTMUCH_STATUS_SUCCESS)\r
 187 > > +       goto DONE;\r
 188 > > +   part = mime_node_seek_dfs (root, params->part < 0 ? 0 : params->part);\r
 189 > \r
 190 > This should be done as a separate patch, but it looks like zero and\r
 191 > negative params->part is handled in the same way so we can change it to\r
 192 > always be non-negative.\r
 193 \r
 194 That's true here, but there are other places where the difference does\r
 195 matter.  (I would certainly *prefer* this not to be asymmetric, but\r
 196 that's a bigger issue involving show's inconsistent interface.)\r
 197 \r
 198 > > +   if (part)\r
 199 > > +       format->part (local, part, indent, params);\r
 200 > > +      DONE:\r
 201 > > +   talloc_free (local);\r
 202 > > +   return;\r
 203 > \r
 204 > Please move part assignment inside the if and remove the goto:\r
 205 > \r
 206 >   if (mime_node_open() == success && (part = seek()))\r
 207 >     format->part();\r
 208 \r
 209 Done.\r
 210 \r
 211 > Regards,\r
 212 >   Dmitry\r
 213 > \r
 214 > > +    }\r
 215 > > +\r
 216 > >      if (params->part <= 0) {\r
 217 > >     fputs (format->message_start, stdout);\r
 218 > >     if (format->message)\r
 219 > \r