b5/42d45a929de6c7c238c452c9a86dc0a048f2ef

   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 2B472429E34\r
   6         for <notmuch@notmuchmail.org>; Mon, 30 Jan 2012 15:38:48 -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 5MDRU5Uq9zDL for <notmuch@notmuchmail.org>;\r
  17         Mon, 30 Jan 2012 15:38:47 -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 C78D5431FBC\r
  22         for <notmuch@notmuchmail.org>; Mon, 30 Jan 2012 15:38:46 -0800 (PST)\r
  23 Received: by bke11 with SMTP id 11so1973525bke.26\r
  24         for <notmuch@notmuchmail.org>; Mon, 30 Jan 2012 15:38:45 -0800 (PST)\r
  25 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\r
  26         h=from:to:cc:subject:in-reply-to:references:user-agent:date\r
  27         :message-id:mime-version:content-type;\r
  28         bh=cV3X5UuScXPp4C0jZyMEgr2tieLSl1uzr6cz4ZtPFTo=;\r
  29         b=AJyvfCZPyQ5zzn68s1SVPEMTqxhsDezebdphGlkGi/oiTXEGx3u2Kob7bllhtd1KjD\r
  30         NUUttKnO2vXemBjt2XHn/oZrp3a3bv3ZkTOvjnkf8o62K+IDPPkXFypgaqsBf5P0SXfs\r
  31         DftvRJoM/6xOo7VusBcDsZBrjW5wLRHSOtD6U=\r
  32 Received: by 10.204.157.17 with SMTP id z17mr9378762bkw.37.1327966725499;\r
  33         Mon, 30 Jan 2012 15:38:45 -0800 (PST)\r
  34 Received: from localhost ([91.144.186.21])\r
  35         by mx.google.com with ESMTPS id 20sm30234379bkr.0.2012.01.30.15.38.43\r
  36         (version=TLSv1/SSLv3 cipher=OTHER);\r
  37         Mon, 30 Jan 2012 15:38:44 -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: [PATCH 2/2] show: Simplify new text formatter code\r
  41 In-Reply-To: <1327560926-22401-3-git-send-email-amdragon@mit.edu>\r
  42 References: <1327560926-22401-1-git-send-email-amdragon@mit.edu>\r
  43         <1327560926-22401-3-git-send-email-amdragon@mit.edu>\r
  44 User-Agent: Notmuch/0.11+139~gd9b7cab (http://notmuchmail.org) Emacs/23.3.1\r
  45         (x86_64-pc-linux-gnu)\r
  46 Date: Tue, 31 Jan 2012 03:37:33 +0400\r
  47 Message-ID: <87lioooj7m.fsf@gmail.com>\r
  48 MIME-Version: 1.0\r
  49 Content-Type: text/plain; charset=us-ascii\r
  50 Cc: tomi.ollila@iki.fi\r
  51 X-BeenThere: notmuch@notmuchmail.org\r
  52 X-Mailman-Version: 2.1.13\r
  53 Precedence: list\r
  54 List-Id: "Use and development of the notmuch mail system."\r
  55         <notmuch.notmuchmail.org>\r
  56 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  57         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  58 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  59 List-Post: <mailto:notmuch@notmuchmail.org>\r
  60 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  61 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  62         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  63 X-List-Received-Date: Mon, 30 Jan 2012 23:38:48 -0000\r
  64 \r
  65 On Thu, 26 Jan 2012 01:55:26 -0500, Austin Clements <amdragon@MIT.EDU> wrote:\r
  66 > This makes the text formatter take advantage of the new code\r
  67 > structure.  The previously duplicated header logic is now unified,\r
  68 > several things that we used to compute repeatedly across different\r
  69 > callbacks are now computed once, and the code is simpler overall and\r
  70 > 32% shorter.\r
  71 > \r
  72 > Unifying the header logic causes this to format some dates slightly\r
  73 > differently, so the two affected test cases are updated.\r
  74 > ---\r
  75 \r
  76 Looks good to me.  Two minor comments, which can be freely ignored, are\r
  77 below.\r
  78 \r
  79 >  notmuch-show.c     |   88 +++++++++++++--------------------------------------\r
  80 >  test/crypto        |    2 +-\r
  81 >  test/thread-naming |   16 +++++-----\r
  82 >  3 files changed, 32 insertions(+), 74 deletions(-)\r
  83 > \r
  84 > diff --git a/notmuch-show.c b/notmuch-show.c\r
  85 > index 6a890b2..30f6501 100644\r
  86 > --- a/notmuch-show.c\r
  87 > +++ b/notmuch-show.c\r
  88 > @@ -727,67 +727,48 @@ format_part_text (const void *ctx, mime_node_t *node,\r
  89 >      GMimeObject *meta = node->envelope_part ?\r
  90 >       GMIME_OBJECT (node->envelope_part) : node->part;\r
  91 >      GMimeContentType *content_type = g_mime_object_get_content_type (meta);\r
  92 > +    const notmuch_bool_t leaf = GMIME_IS_PART (node->part);\r
  93 > +    const char *part_type;\r
  94 >      int i;\r
  95 >  \r
  96 >      if (node->envelope_file) {\r
  97 >       notmuch_message_t *message = node->envelope_file;\r
  98 > -     const char *headers[] = {\r
  99 > -         "Subject", "From", "To", "Cc", "Bcc", "Date"\r
 100 > -     };\r
 101 > -     const char *name, *value;\r
 102 > -     unsigned int i;\r
 103 >  \r
 104 > -     printf ("\fmessage{ ");\r
 105 > -     printf ("id:%s depth:%d match:%d filename:%s\n",\r
 106 > +     part_type = "message";\r
 107 > +     printf ("\f%s{ id:%s depth:%d match:%d filename:%s\n",\r
 108 > +             part_type,\r
 109 >               notmuch_message_get_message_id (message),\r
 110 >               indent,\r
 111 >               notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH),\r
 112 >               notmuch_message_get_filename (message));\r
 113 > -\r
 114 > -     printf ("\fheader{\n");\r
 115 > -\r
 116 > -     printf ("%s\n", _get_one_line_summary (ctx, message));\r
 117 > -\r
 118 > -     for (i = 0; i < ARRAY_SIZE (headers); i++) {\r
 119 > -         name = headers[i];\r
 120 > -         value = notmuch_message_get_header (message, name);\r
 121 > -         if (value && strlen (value))\r
 122 > -             printf ("%s: %s\n", name, value);\r
 123 > -     }\r
 124 > -     printf ("\fheader}\n");\r
 125 >      } else {\r
 126 >       GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (meta);\r
 127 >       const char *cid = g_mime_object_get_content_id (meta);\r
 128 > +     const char *filename = leaf ?\r
 129 > +         g_mime_part_get_filename (GMIME_PART (node->part)) : NULL;\r
 130 >  \r
 131 >       if (disposition &&\r
 132 >           strcmp (disposition->disposition, GMIME_DISPOSITION_ATTACHMENT) == 0)\r
 133 > -     {\r
 134 > -         printf ("\fattachment{ ID: %d", node->part_num);\r
 135 > -\r
 136 > -     } else {\r
 137 > -\r
 138 > -         printf ("\fpart{ ID: %d", node->part_num);\r
 139 > -     }\r
 140 > -\r
 141 > -     if (GMIME_IS_PART (node->part))\r
 142 > -     {\r
 143 > -         const char *filename = g_mime_part_get_filename (GMIME_PART (node->part));\r
 144 > -         if (filename)\r
 145 > -             printf (", Filename: %s", filename);\r
 146 > -     }\r
 147 > +         part_type = "attachment";\r
 148 > +     else\r
 149 > +         part_type = "part";\r
 150 \r
 151 Others may disagree, but I would write this using the (? :) operator.\r
 152 Not important, feel free to leave it as is.\r
 153 \r
 154 >  \r
 155 > +     printf ("\f%s{ ID: %d", part_type, node->part_num);\r
 156 > +     if (filename)\r
 157 > +         printf (", Filename: %s", filename);\r
 158 >       if (cid)\r
 159 >           printf (", Content-id: %s", cid);\r
 160 > -\r
 161 >       printf (", Content-type: %s\n", g_mime_content_type_to_string (content_type));\r
 162 >      }\r
 163 >  \r
 164 > -    if (node->envelope_part) {\r
 165 > +    if (GMIME_IS_MESSAGE (node->part)) {\r
 166 >       GMimeMessage *message = GMIME_MESSAGE (node->part);\r
 167 >       InternetAddressList *recipients;\r
 168 >       const char *recipients_string;\r
 169 >  \r
 170 >       printf ("\fheader{\n");\r
 171 > +     if (node->envelope_file)\r
 172 > +         printf ("%s\n", _get_one_line_summary (ctx, node->envelope_file));\r
 173 >       printf ("Subject: %s\n", g_mime_message_get_subject (message));\r
 174 >       printf ("From: %s\n", g_mime_message_get_sender (message));\r
 175 >       recipients = g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_TO);\r
 176 > @@ -800,9 +781,11 @@ format_part_text (const void *ctx, mime_node_t *node,\r
 177 >           printf ("Cc: %s\n", recipients_string);\r
 178 >       printf ("Date: %s\n", g_mime_message_get_date_as_string (message));\r
 179 >       printf ("\fheader}\n");\r
 180 > +\r
 181 > +     printf ("\fbody{\n");\r
 182 >      }\r
 183 >  \r
 184 > -    if (!node->envelope_file) {\r
 185 > +    if (leaf) {\r
 186 >       if (g_mime_content_type_is_type (content_type, "text", "*") &&\r
 187 >           !g_mime_content_type_is_type (content_type, "text", "html"))\r
 188 >       {\r
 189 > @@ -810,45 +793,20 @@ format_part_text (const void *ctx, mime_node_t *node,\r
 190 >           g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);\r
 191 >           show_text_part_content (node->part, stream_stdout);\r
 192 >           g_object_unref(stream_stdout);\r
 193 > -     }\r
 194 > -     else if (g_mime_content_type_is_type (content_type, "multipart", "*") ||\r
 195 > -              g_mime_content_type_is_type (content_type, "message", "rfc822"))\r
 196 > -     {\r
 197 > -         /* Do nothing for multipart since its content will be printed\r
 198 > -          * when recursing. */\r
 199 > -     }\r
 200 > -     else\r
 201 > -     {\r
 202 > +     } else {\r
 203 >           printf ("Non-text part: %s\n",\r
 204 >                   g_mime_content_type_to_string (content_type));\r
 205 >       }\r
 206 >      }\r
 207 >  \r
 208 > -    if (GMIME_IS_MESSAGE (node->part))\r
 209 > -     printf ("\fbody{\n");\r
 210 > -\r
 211 >      for (i = 0; i < node->nchildren; i++)\r
 212 >       format_part_text (ctx, mime_node_child (node, i), indent, params);\r
 213 >  \r
 214 > -    if (GMIME_IS_MESSAGE (node->part))\r
 215 > +    if (GMIME_IS_MESSAGE (node->part)) {\r
 216 \r
 217 Since the if body is one line, I would prefer to remove the closing\r
 218 bracket rather than adding the opening one.\r
 219 \r
 220 Regards,\r
 221   Dmitry\r
 222 \r
 223 >       printf ("\fbody}\n");\r
 224 > -\r
 225 > -    if (node->envelope_file) {\r
 226 > -     printf ("\fmessage}\n");\r
 227 > -    } else {\r
 228 > -     GMimeContentDisposition *disposition;\r
 229 > -\r
 230 > -     disposition = g_mime_object_get_content_disposition (meta);\r
 231 > -     if (disposition &&\r
 232 > -         strcmp (disposition->disposition, GMIME_DISPOSITION_ATTACHMENT) == 0)\r
 233 > -     {\r
 234 > -         printf ("\fattachment}\n");\r
 235 > -     }\r
 236 > -     else\r
 237 > -     {\r
 238 > -         printf ("\fpart}\n");\r
 239 > -     }\r
 240 >      }\r
 241 > +\r
 242 > +    printf ("\f%s}\n", part_type);\r
 243 >  }\r
 244 >  \r
 245 >  static void\r
 246 > diff --git a/test/crypto b/test/crypto\r
 247 > index 446a58b..1dbb60a 100755\r
 248 > --- a/test/crypto\r
 249 > +++ b/test/crypto\r
 250 > @@ -159,7 +159,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2000-01-01) (encrypted inbox)\r
 251 >  Subject: test encrypted message 001\r
 252 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 253 >  To: test_suite@notmuchmail.org\r
 254 > -Date: 01 Jan 2000 12:00:00 -0000\r
 255 > +Date: Sat, 01 Jan 2000 12:00:00 +0000\r
 256 >  \f\r
 257 header}\r
 258 >  \f\r
 259 body{\r
 260 >  \f\r
 261 part{ ID: 1, Content-type: multipart/encrypted\r
 262 > diff --git a/test/thread-naming b/test/thread-naming\r
 263 > index 2ce9216..942e593 100755\r
 264 > --- a/test/thread-naming\r
 265 > +++ b/test/thread-naming\r
 266 > @@ -71,7 +71,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-05) (unread)\r
 267 >  Subject: thread-naming: Initial thread subject\r
 268 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 269 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 270 > -Date: Fri, 05 Jan 2001 15:43:56 -0000\r
 271 > +Date: Fri, 05 Jan 2001 15:43:56 +0000\r
 272 >  \f\r
 273 header}\r
 274 >  \f\r
 275 body{\r
 276 >  \f\r
 277 part{ ID: 1, Content-type: text/plain\r
 278 > @@ -85,7 +85,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-06) (inbox unread)\r
 279 >  Subject: thread-naming: Older changed subject\r
 280 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 281 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 282 > -Date: Sat, 06 Jan 2001 15:43:56 -0000\r
 283 > +Date: Sat, 06 Jan 2001 15:43:56 +0000\r
 284 >  \f\r
 285 header}\r
 286 >  \f\r
 287 body{\r
 288 >  \f\r
 289 part{ ID: 1, Content-type: text/plain\r
 290 > @@ -99,7 +99,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-07) (inbox unread)\r
 291 >  Subject: thread-naming: Newer changed subject\r
 292 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 293 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 294 > -Date: Sun, 07 Jan 2001 15:43:56 -0000\r
 295 > +Date: Sun, 07 Jan 2001 15:43:56 +0000\r
 296 >  \f\r
 297 header}\r
 298 >  \f\r
 299 body{\r
 300 >  \f\r
 301 part{ ID: 1, Content-type: text/plain\r
 302 > @@ -113,7 +113,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-08) (unread)\r
 303 >  Subject: thread-naming: Final thread subject\r
 304 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 305 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 306 > -Date: Mon, 08 Jan 2001 15:43:56 -0000\r
 307 > +Date: Mon, 08 Jan 2001 15:43:56 +0000\r
 308 >  \f\r
 309 header}\r
 310 >  \f\r
 311 body{\r
 312 >  \f\r
 313 part{ ID: 1, Content-type: text/plain\r
 314 > @@ -127,7 +127,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-09) (inbox unread)\r
 315 >  Subject: Re: thread-naming: Initial thread subject\r
 316 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 317 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 318 > -Date: Tue, 09 Jan 2001 15:43:45 -0000\r
 319 > +Date: Tue, 09 Jan 2001 15:43:45 +0000\r
 320 >  \f\r
 321 header}\r
 322 >  \f\r
 323 body{\r
 324 >  \f\r
 325 part{ ID: 1, Content-type: text/plain\r
 326 > @@ -141,7 +141,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-10) (inbox unread)\r
 327 >  Subject: Aw: thread-naming: Initial thread subject\r
 328 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 329 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 330 > -Date: Wed, 10 Jan 2001 15:43:45 -0000\r
 331 > +Date: Wed, 10 Jan 2001 15:43:45 +0000\r
 332 >  \f\r
 333 header}\r
 334 >  \f\r
 335 body{\r
 336 >  \f\r
 337 part{ ID: 1, Content-type: text/plain\r
 338 > @@ -155,7 +155,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-11) (inbox unread)\r
 339 >  Subject: Vs: thread-naming: Initial thread subject\r
 340 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 341 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 342 > -Date: Thu, 11 Jan 2001 15:43:45 -0000\r
 343 > +Date: Thu, 11 Jan 2001 15:43:45 +0000\r
 344 >  \f\r
 345 header}\r
 346 >  \f\r
 347 body{\r
 348 >  \f\r
 349 part{ ID: 1, Content-type: text/plain\r
 350 > @@ -169,7 +169,7 @@ Notmuch Test Suite <test_suite@notmuchmail.org> (2001-01-12) (inbox unread)\r
 351 >  Subject: Sv: thread-naming: Initial thread subject\r
 352 >  From: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 353 >  To: Notmuch Test Suite <test_suite@notmuchmail.org>\r
 354 > -Date: Fri, 12 Jan 2001 15:43:45 -0000\r
 355 > +Date: Fri, 12 Jan 2001 15:43:45 +0000\r
 356 >  \f\r
 357 header}\r
 358 >  \f\r
 359 body{\r
 360 >  \f\r
 361 part{ ID: 1, Content-type: text/plain\r
 362 > -- \r
 363 > 1.7.7.3\r
 364 > \r