bc/fd56549f891cfd0dfc9def64db1a1dae949c7d

   1 Return-Path: <dmitry.kurochkin@gmail.com>\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 B36E1429E54\r
   6         for <notmuch@notmuchmail.org>; Mon, 23 Jan 2012 17:51:00 -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.799\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5\r
  12         tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
  13         FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  14 Received: from olra.theworths.org ([127.0.0.1])\r
  15         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  16         with ESMTP id Gr+qhg5Y68Bz for <notmuch@notmuchmail.org>;\r
  17         Mon, 23 Jan 2012 17:50:59 -0800 (PST)\r
  18 Received: from mail-bk0-f53.google.com (mail-bk0-f53.google.com\r
  19         [209.85.214.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  20         (No client certificate requested)\r
  21         by olra.theworths.org (Postfix) with ESMTPS id 4DA2F429E21\r
  22         for <notmuch@notmuchmail.org>; Mon, 23 Jan 2012 17:50:59 -0800 (PST)\r
  23 Received: by bkbzt19 with SMTP id zt19so1896327bkb.26\r
  24         for <notmuch@notmuchmail.org>; Mon, 23 Jan 2012 17:50:58 -0800 (PST)\r
  25 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\r
  26         h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
  27         :mime-version:content-type;\r
  28         bh=5jWYBeucQbpa1RlAUVSLOpewmKI3iKEDiVT2MHb0onc=;\r
  29         b=b7SAjdHeh0bZwnBTLEFaSl86EhZ0bArnRkzoO4wRCHCpWLB4m0F6XvT5TWTSPcbLBP\r
  30         usjUjAHcK+xRvgtURHDU/eLdheIHExrSyUvdloEBqHq4kzoxjxifKxG9sshEyR61VV2O\r
  31         8Oey3txhvCCwkBMOO6cCSiW9Kqj/yD7Jz7CWw=\r
  32 Received: by 10.204.153.15 with SMTP id i15mr4054374bkw.43.1327369857902;\r
  33         Mon, 23 Jan 2012 17:50:57 -0800 (PST)\r
  34 Received: from localhost ([91.144.186.21])\r
  35         by mx.google.com with ESMTPS id ez5sm22032505bkc.15.2012.01.23.17.50.56\r
  36         (version=TLSv1/SSLv3 cipher=OTHER);\r
  37         Mon, 23 Jan 2012 17:50:57 -0800 (PST)\r
  38 From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
  39 To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org\r
  40 Subject: Re: [RFC PATCH 5/5] show: Simplify new text formatter code\r
  41 In-Reply-To: <1326332973-30225-6-git-send-email-amdragon@mit.edu>\r
  42 References: <1326332973-30225-1-git-send-email-amdragon@mit.edu>\r
  43         <1326332973-30225-6-git-send-email-amdragon@mit.edu>\r
  44 User-Agent: Notmuch/0.11+100~gd650abf (http://notmuchmail.org) Emacs/23.3.1\r
  45         (x86_64-pc-linux-gnu)\r
  46 Date: Tue, 24 Jan 2012 05:49:52 +0400\r
  47 Message-ID: <87lioxna2n.fsf@gmail.com>\r
  48 MIME-Version: 1.0\r
  49 Content-Type: text/plain; charset=us-ascii\r
  50 X-BeenThere: notmuch@notmuchmail.org\r
  51 X-Mailman-Version: 2.1.13\r
  52 Precedence: list\r
  53 List-Id: "Use and development of the notmuch mail system."\r
  54         <notmuch.notmuchmail.org>\r
  55 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  56         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  57 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  58 List-Post: <mailto:notmuch@notmuchmail.org>\r
  59 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  60 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  61         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  62 X-List-Received-Date: Tue, 24 Jan 2012 01:51:00 -0000\r
  63 \r
  64 On Wed, 11 Jan 2012 20:49:33 -0500, Austin Clements <amdragon@MIT.EDU> wrote:\r
  65 > This makes the text formatter take advantage of the new code\r
  66 > structure.  The previously duplicated header logic is now unified,\r
  67 > several things that we used to compute repeatedly across different\r
  68 > callbacks are now computed once, and the code is generally simplified.\r
  69 > \r
  70 > Unifying the header logic causes this to format some dates slightly\r
  71 > differently, so the two affected test cases are updated.\r
  72 \r
  73 Thanks for these patches, Austin.  They are a definite improvement for\r
  74 notmuch show code.  I hope it would soon get pushed to master.  And I\r
  75 hope more patches would follow :)\r
  76 \r
  77 Few minor comments below.\r
  78 \r
  79 > ---\r
  80 >  notmuch-show.c     |   84 ++++++++++++----------------------------------------\r
  81 >  test/crypto        |    2 +-\r
  82 >  test/thread-naming |   16 +++++-----\r
  83 >  3 files changed, 28 insertions(+), 74 deletions(-)\r
  84 > \r
  85 > diff --git a/notmuch-show.c b/notmuch-show.c\r
  86 > index 3241965..1689222 100644\r
  87 > --- a/notmuch-show.c\r
  88 > +++ b/notmuch-show.c\r
  89 > @@ -175,67 +175,42 @@ format_part_text (const void *ctx, mime_node_t *node,\r
  90 >      GMimeObject *meta = node->envelope_part ?\r
  91 >       GMIME_OBJECT (node->envelope_part) : node->part;\r
  92 >      GMimeContentType *content_type = g_mime_object_get_content_type (meta);\r
  93 > +    GMimeContentDisposition *disposition =\r
  94 > +     g_mime_object_get_content_disposition (meta);\r
  95 > +    notmuch_bool_t attachment = disposition &&\r
  96 > +     strcmp (disposition->disposition, GMIME_DISPOSITION_ATTACHMENT) == 0;\r
  97 \r
  98 If I did not miss anything, attachment is used only as following:\r
  99 \r
 100   attachment ? "attachment" : "part"\r
 101 \r
 102 Please make it const char[] and set to "attachment" or "part".\r
 103 \r
 104 > +    notmuch_bool_t leaf = GMIME_IS_PART (node->part);\r
 105 \r
 106 Please add const where possible to local variables (e.g. attachment, leaf).\r
 107 \r
 108 >      int i;\r
 109 >  \r
 110 >      if (node->envelope_file) {\r
 111 >       notmuch_message_t *message = node->envelope_file;\r
 112 > -     const char *headers[] = {\r
 113 > -         "Subject", "From", "To", "Cc", "Bcc", "Date"\r
 114 > -     };\r
 115 > -     const char *name, *value;\r
 116 > -     unsigned int i;\r
 117 >  \r
 118 > -     printf ("\fmessage{ ");\r
 119 > -     printf ("id:%s depth:%d match:%d filename:%s\n",\r
 120 > +     printf ("\fmessage{ id:%s depth:%d match:%d filename:%s\n",\r
 121 >               notmuch_message_get_message_id (message),\r
 122 >               indent,\r
 123 >               notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH),\r
 124 >               notmuch_message_get_filename (message));\r
 125 > -\r
 126 > -     printf ("\fheader{\n");\r
 127 > -\r
 128 > -     printf ("%s\n", _get_one_line_summary (ctx, message));\r
 129 > -\r
 130 > -     for (i = 0; i < ARRAY_SIZE (headers); i++) {\r
 131 > -         name = headers[i];\r
 132 > -         value = notmuch_message_get_header (message, name);\r
 133 > -         if (value && strlen (value))\r
 134 > -             printf ("%s: %s\n", name, value);\r
 135 > -     }\r
 136 > -     printf ("\fheader}\n");\r
 137 \r
 138 Yay!  Only one header-printing code left :)\r
 139 \r
 140 >      } else {\r
 141 > -     GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (meta);\r
 142 >       const char *cid = g_mime_object_get_content_id (meta);\r
 143 > +     const char *filename = leaf ?\r
 144 > +         g_mime_part_get_filename (GMIME_PART (node->part)) : NULL;\r
 145 >  \r
 146 > -     if (disposition &&\r
 147 > -         strcmp (disposition->disposition, GMIME_DISPOSITION_ATTACHMENT) == 0)\r
 148 > -     {\r
 149 > -         printf ("\fattachment{ ID: %d", node->part_num);\r
 150 > -\r
 151 > -     } else {\r
 152 > -\r
 153 > -         printf ("\fpart{ ID: %d", node->part_num);\r
 154 > -     }\r
 155 > -\r
 156 > -     if (GMIME_IS_PART (node->part))\r
 157 > -     {\r
 158 > -         const char *filename = g_mime_part_get_filename (GMIME_PART (node->part));\r
 159 > -         if (filename)\r
 160 > -             printf (", Filename: %s", filename);\r
 161 > -     }\r
 162 > -\r
 163 > +     printf ("\f%s{ ID: %d", attachment ? "attachment" : "part", node->part_num);\r
 164 > +     if (filename)\r
 165 \r
 166 I always forget about it, can we declare variables inside if condition\r
 167 like in C++?  I.e.:\r
 168 \r
 169   if (const char *filename = leaf ? g_mime_part_get_filename (GMIME_PART (node->part)) : NULL)\r
 170 \r
 171 If yes, I would prefer to use this style where possible.\r
 172 \r
 173 > +         printf (", Filename: %s", filename);\r
 174 >       if (cid)\r
 175 >           printf (", Content-id: %s", cid);\r
 176 > -\r
 177 \r
 178 I would revert blank line changes.  But I do not insist :)\r
 179 \r
 180 >       printf (", Content-type: %s\n", g_mime_content_type_to_string (content_type));\r
 181 >      }\r
 182 >  \r
 183 > -    if (node->envelope_part) {\r
 184 > +    if (GMIME_IS_MESSAGE (node->part)) {\r
 185 \r
 186 This condition is repeated at least twice.  Please consider moving the\r
 187 message variable below to the top level and using it in the conditions.\r
 188 \r
 189 Regards,\r
 190   Dmitry\r
 191 \r
 192 >       GMimeMessage *message = GMIME_MESSAGE (node->part);\r
 193 >       InternetAddressList *recipients;\r
 194 >       const char *recipients_string;\r
 195 >  \r
 196 >       printf ("\fheader{\n");\r
 197 > +     if (node->envelope_file)\r
 198 > +         printf ("%s\n", _get_one_line_summary (ctx, node->envelope_file));\r
 199 >       printf ("Subject: %s\n", g_mime_message_get_subject (message));\r
 200 >       printf ("From: %s\n", g_mime_message_get_sender (message));\r
 201 >       recipients = g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_TO);\r
 202 > @@ -248,9 +223,11 @@ format_part_text (const void *ctx, mime_node_t *node,\r
 203 >           printf ("Cc: %s\n", recipients_string);\r
 204 >       printf ("Date: %s\n", g_mime_message_get_date_as_string (message));\r
 205 >       printf ("\fheader}\n");\r
 206 > +\r
 207 > +     printf ("\fbody{\n");\r
 208 >      }\r
 209 >  \r
 210 > -    if (!node->envelope_file) {\r
 211 > +    if (leaf) {\r
 212 >       if (g_mime_content_type_is_type (content_type, "text", "*") &&\r
 213 >           !g_mime_content_type_is_type (content_type, "text", "html"))\r
 214 >       {\r
 215 > @@ -258,24 +235,12 @@ format_part_text (const void *ctx, mime_node_t *node,\r
 216 >           g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);\r
 217 >           show_text_part_content (node->part, stream_stdout);\r
 218 >           g_object_unref(stream_stdout);\r
 219 > -     }\r
 220 > -     else if (g_mime_content_type_is_type (content_type, "multipart", "*") ||\r
 221 > -              g_mime_content_type_is_type (content_type, "message", "rfc822"))\r
 222 > -     {\r
 223 > -         /* Do nothing for multipart since its content will be printed\r
 224 > -          * when recursing. */\r
 225 > -     }\r
 226 > -     else\r
 227 > -     {\r
 228 > +     } else {\r
 229 >           printf ("Non-text part: %s\n",\r
 230 >                   g_mime_content_type_to_string (content_type));\r
 231 >       }\r
 232 >      }\r
 233 >  \r
 234 > -    if (GMIME_IS_MESSAGE (node->part)) {\r
 235 > -     printf ("\fbody{\n");\r
 236 > -    }\r
 237 > -\r
 238 >      for (i = 0; i < node->nchildren; i++)\r
 239 >       format_part_text (ctx, mime_node_child (node, i), indent, params);\r
 240 >  \r
 241 > @@ -286,18 +251,7 @@ format_part_text (const void *ctx, mime_node_t *node,\r
 242 >      if (node->envelope_file) {\r
 243 >       printf ("\fmessage}\n");\r
 244 >      } else {\r
 245 > -     GMimeContentDisposition *disposition;\r
 246 > -\r
 247 > -     disposition = g_mime_object_get_content_disposition (meta);\r
 248 > -     if (disposition &&\r
 249 > -         strcmp (disposition->disposition, GMIME_DISPOSITION_ATTACHMENT) == 0)\r
 250 > -     {\r
 251 > -         printf ("\fattachment}\n");\r
 252 > -     }\r
 253 > -     else\r
 254 > -     {\r
 255 > -         printf ("\fpart}\n");\r
 256 > -     }\r
 257 > +     printf ("\f%s}\n", attachment ? "attachment" : "part");\r
 258 >      }\r
 259 >  }\r
 260 >  \r
 261 > diff --git a/test/crypto b/test/crypto\r
 262 > index 0af4aa8..6723ef8 100755\r
 263 > --- a/test/crypto\r
 264 > +++ b/test/crypto\r
 265 > @@ -157,7 +157,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2000-01-01) (encrypted inbox)\r
 266 >  Subject: test encrypted message 001\r
 267 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 268 >  To: test_suite@notmuchmail.org\r
 269 > -Date: 01 Jan 2000 12:00:00 -0000\r
 270 > +Date: Sat, 01 Jan 2000 12:00:00 +0000\r
 271 >  \f\r
 272 header}\r
 273 >  \f\r
 274 body{\r
 275 >  \f\r
 276 part{ ID: 1, Content-type: multipart/encrypted\r
 277 > diff --git a/test/thread-naming b/test/thread-naming\r
 278 > index 41b97d9..b7c96f2 100755\r
 279 > --- a/test/thread-naming\r
 280 > +++ b/test/thread-naming\r
 281 > @@ -71,7 +71,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-05) (unread)\r
 282 >  Subject: thread-naming: Initial thread subject\r
 283 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 284 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 285 > -Date: Fri, 05 Jan 2001 15:43:56 -0000\r
 286 > +Date: Fri, 05 Jan 2001 15:43:56 +0000\r
 287 >  \f\r
 288 header}\r
 289 >  \f\r
 290 body{\r
 291 >  \f\r
 292 part{ ID: 1, Content-type: text/plain\r
 293 > @@ -85,7 +85,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-06) (inbox unread)\r
 294 >  Subject: thread-naming: Older changed subject\r
 295 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 296 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 297 > -Date: Sat, 06 Jan 2001 15:43:56 -0000\r
 298 > +Date: Sat, 06 Jan 2001 15:43:56 +0000\r
 299 >  \f\r
 300 header}\r
 301 >  \f\r
 302 body{\r
 303 >  \f\r
 304 part{ ID: 1, Content-type: text/plain\r
 305 > @@ -99,7 +99,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-07) (inbox unread)\r
 306 >  Subject: thread-naming: Newer changed subject\r
 307 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 308 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 309 > -Date: Sun, 07 Jan 2001 15:43:56 -0000\r
 310 > +Date: Sun, 07 Jan 2001 15:43:56 +0000\r
 311 >  \f\r
 312 header}\r
 313 >  \f\r
 314 body{\r
 315 >  \f\r
 316 part{ ID: 1, Content-type: text/plain\r
 317 > @@ -113,7 +113,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-08) (unread)\r
 318 >  Subject: thread-naming: Final thread subject\r
 319 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 320 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 321 > -Date: Mon, 08 Jan 2001 15:43:56 -0000\r
 322 > +Date: Mon, 08 Jan 2001 15:43:56 +0000\r
 323 >  \f\r
 324 header}\r
 325 >  \f\r
 326 body{\r
 327 >  \f\r
 328 part{ ID: 1, Content-type: text/plain\r
 329 > @@ -127,7 +127,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-09) (inbox unread)\r
 330 >  Subject: Re: thread-naming: Initial thread subject\r
 331 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 332 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 333 > -Date: Tue, 09 Jan 2001 15:43:45 -0000\r
 334 > +Date: Tue, 09 Jan 2001 15:43:45 +0000\r
 335 >  \f\r
 336 header}\r
 337 >  \f\r
 338 body{\r
 339 >  \f\r
 340 part{ ID: 1, Content-type: text/plain\r
 341 > @@ -141,7 +141,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-10) (inbox unread)\r
 342 >  Subject: Aw: thread-naming: Initial thread subject\r
 343 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 344 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 345 > -Date: Wed, 10 Jan 2001 15:43:45 -0000\r
 346 > +Date: Wed, 10 Jan 2001 15:43:45 +0000\r
 347 >  \f\r
 348 header}\r
 349 >  \f\r
 350 body{\r
 351 >  \f\r
 352 part{ ID: 1, Content-type: text/plain\r
 353 > @@ -155,7 +155,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-11) (inbox unread)\r
 354 >  Subject: Vs: thread-naming: Initial thread subject\r
 355 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 356 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 357 > -Date: Thu, 11 Jan 2001 15:43:45 -0000\r
 358 > +Date: Thu, 11 Jan 2001 15:43:45 +0000\r
 359 >  \f\r
 360 header}\r
 361 >  \f\r
 362 body{\r
 363 >  \f\r
 364 part{ ID: 1, Content-type: text/plain\r
 365 > @@ -169,7 +169,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-12) (inbox unread)\r
 366 >  Subject: Sv: thread-naming: Initial thread subject\r
 367 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 368 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 369 > -Date: Fri, 12 Jan 2001 15:43:45 -0000\r
 370 > +Date: Fri, 12 Jan 2001 15:43:45 +0000\r
 371 >  \f\r
 372 header}\r
 373 >  \f\r
 374 body{\r
 375 >  \f\r
 376 part{ ID: 1, Content-type: text/plain\r
 377 > -- \r
 378 > 1.7.7.3\r
 379 > \r
 380 > _______________________________________________\r
 381 > notmuch mailing list\r
 382 > notmuch@notmuchmail.org\r
 383 > http://notmuchmail.org/mailman/listinfo/notmuch\r