41/7d17054650b713c45e063d76a4b1a2db2834e1

   1 Return-Path: <jani@nikula.org>\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 31586429E25\r
   6         for <notmuch@notmuchmail.org>; Sat, 14 Jan 2012 12:59: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 xDU3DuOtJtmd for <notmuch@notmuchmail.org>;\r
  16         Sat, 14 Jan 2012 12:59:06 -0800 (PST)\r
  17 Received: from mail-ey0-f181.google.com (mail-ey0-f181.google.com\r
  18         [209.85.215.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  19         (No client certificate requested)\r
  20         by olra.theworths.org (Postfix) with ESMTPS id 9DB6E431FB6\r
  21         for <notmuch@notmuchmail.org>; Sat, 14 Jan 2012 12:59:05 -0800 (PST)\r
  22 Received: by eaah10 with SMTP id h10so828123eaa.26\r
  23         for <notmuch@notmuchmail.org>; Sat, 14 Jan 2012 12:59:04 -0800 (PST)\r
  24 Received: by 10.213.9.71 with SMTP id k7mr1032488ebk.71.1326574739515;\r
  25         Sat, 14 Jan 2012 12:58:59 -0800 (PST)\r
  26 Received: from localhost (dsl-hkibrasgw4-fe5cdc00-23.dhcp.inet.fi.\r
  27         [80.220.92.23])\r
  28         by mx.google.com with ESMTPS id a60sm48763810eeb.4.2012.01.14.12.58.56\r
  29         (version=SSLv3 cipher=OTHER); Sat, 14 Jan 2012 12:58:58 -0800 (PST)\r
  30 From: Jani Nikula <jani@nikula.org>\r
  31 To: Adam Wolfe Gordon <awg+notmuch@xvx.ca>, notmuch@notmuchmail.org,\r
  32  awg@xvx.ca\r
  33 Subject: Re: [PATCH 2/4] reply: Add a JSON reply format.\r
  34 In-Reply-To: <1326009162-19524-3-git-send-email-awg+notmuch@xvx.ca>\r
  35 References: <1326009162-19524-1-git-send-email-awg+notmuch@xvx.ca>\r
  36         <1326009162-19524-3-git-send-email-awg+notmuch@xvx.ca>\r
  37 User-Agent: Notmuch/0.11+59~g83cfe07 (http://notmuchmail.org) Emacs/23.3.1\r
  38         (i686-pc-linux-gnu)\r
  39 Date: Sat, 14 Jan 2012 22:58:55 +0200\r
  40 Message-ID: <87k44uatm8.fsf@nikula.org>\r
  41 MIME-Version: 1.0\r
  42 Content-Type: text/plain; charset=us-ascii\r
  43 X-BeenThere: notmuch@notmuchmail.org\r
  44 X-Mailman-Version: 2.1.13\r
  45 Precedence: list\r
  46 List-Id: "Use and development of the notmuch mail system."\r
  47         <notmuch.notmuchmail.org>\r
  48 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  49         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  50 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  51 List-Post: <mailto:notmuch@notmuchmail.org>\r
  52 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  53 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  54         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  55 X-List-Received-Date: Sat, 14 Jan 2012 20:59:07 -0000\r
  56 \r
  57 On Sun,  8 Jan 2012 00:52:40 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:\r
  58 > From: Adam Wolfe Gordon <awg@xvx.ca>\r
  59 > \r
  60 > This new JSON format for replies includes headers generated for a reply\r
  61 > message as well as the headers and all text parts of the original message.\r
  62 > Using this data, a client can intelligently create a reply. For example,\r
  63 > the emacs client will be able to create replies with quoted HTML parts by\r
  64 > parsing the HTML parts using w3m.\r
  65 \r
  66 Hi Adam, this is a drive-by-review on some things I spotted, but can't\r
  67 say I would've thought the whole thing through. I'm pretty ignorant\r
  68 about MIME parts etc. Please find comments inline.\r
  69 \r
  70 The reply-to-sender set was just merged. You'll have conflicts both here\r
  71 and in emacs code, but they should be trivial to sort out.\r
  72 \r
  73 \r
  74 BR,\r
  75 Jani.\r
  76 \r
  77 \r
  78 > ---\r
  79 >  notmuch-reply.c |  269 +++++++++++++++++++++++++++++++++++++++++++++++--------\r
  80 >  1 files changed, 230 insertions(+), 39 deletions(-)\r
  81 > \r
  82 > diff --git a/notmuch-reply.c b/notmuch-reply.c\r
  83 > index f8d5f64..82df396 100644\r
  84 > --- a/notmuch-reply.c\r
  85 > +++ b/notmuch-reply.c\r
  86 > @@ -30,6 +30,15 @@ reply_headers_message_part (GMimeMessage *message);\r
  87 >  static void\r
  88 >  reply_part_content (GMimeObject *part);\r
  89 >  \r
  90 > +static void\r
  91 > +reply_part_start_json (GMimeObject *part, int *part_count);\r
  92 > +\r
  93 > +static void\r
  94 > +reply_part_content_json (GMimeObject *part);\r
  95 > +\r
  96 > +static void\r
  97 > +reply_part_end_json (GMimeObject *part);\r
  98 > +\r
  99 \r
 100 I know there are existing forward declarations like this, but would it\r
 101 be much trouble to arrange the code so that they were not needed at all?\r
 102 \r
 103 >  static const notmuch_show_format_t format_reply = {\r
 104 >      "",\r
 105 >       "", NULL,\r
 106 > @@ -46,6 +55,22 @@ static const notmuch_show_format_t format_reply = {\r
 107 >      ""\r
 108 >  };\r
 109 >  \r
 110 > +static const notmuch_show_format_t format_json = {\r
 111 > +    "",\r
 112 > +     "", NULL,\r
 113 > +         "", NULL, NULL, "",\r
 114 > +         "",\r
 115 > +             reply_part_start_json,\r
 116 > +             NULL,\r
 117 > +             NULL,\r
 118 > +             reply_part_content_json,\r
 119 > +             reply_part_end_json,\r
 120 > +             "",\r
 121 > +         "",\r
 122 > +     "", "",\r
 123 > +    ""\r
 124 > +};\r
 125 > +\r
 126 >  static void\r
 127 >  show_reply_headers (GMimeMessage *message)\r
 128 >  {\r
 129 > @@ -147,6 +172,78 @@ reply_part_content (GMimeObject *part)\r
 130 >      }\r
 131 >  }\r
 132 >  \r
 133 > +static void\r
 134 > +reply_part_start_json (GMimeObject *part, unused(int *part_count))\r
 135 > +{\r
 136 > +    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));\r
 137 > +    GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (part);\r
 138 > +\r
 139 > +    if (g_mime_content_type_is_type (content_type, "text", "*") &&\r
 140 > +     (!disposition ||\r
 141 > +      strcmp (disposition->disposition, GMIME_DISPOSITION_INLINE) == 0))\r
 142 > +    {\r
 143 > +     printf("{ ");\r
 144 > +    }\r
 145 > +}\r
 146 > +\r
 147 > +static void\r
 148 > +reply_part_end_json (GMimeObject *part)\r
 149 > +{\r
 150 > +    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));\r
 151 > +    GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (part);\r
 152 > +\r
 153 > +    if (g_mime_content_type_is_type (content_type, "text", "*") &&\r
 154 > +     (!disposition ||\r
 155 > +      strcmp (disposition->disposition, GMIME_DISPOSITION_INLINE) == 0))\r
 156 > +     printf ("}, ");\r
 157 > +}\r
 158 \r
 159 The two functions above, while small, are almost identical. Please move\r
 160 the common stuff to a common helper, and you can have something like\r
 161 this:\r
 162 \r
 163         if (the_common_function (part))\r
 164                 printf ("}, ");\r
 165 \r
 166 > +\r
 167 > +static void\r
 168 > +reply_part_content_json (GMimeObject *part)\r
 169 > +{\r
 170 > +    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));\r
 171 > +    GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (part);\r
 172 > +\r
 173 > +    void *ctx = talloc_new (NULL);\r
 174 > +\r
 175 > +    /* We only care about inline text parts for reply purposes */\r
 176 > +    if (g_mime_content_type_is_type (content_type, "text", "*") &&\r
 177 > +     (!disposition ||\r
 178 > +      strcmp (disposition->disposition, GMIME_DISPOSITION_INLINE) == 0))\r
 179 \r
 180 Oh, you can use the common helper here too.\r
 181 \r
 182 > +    {\r
 183 > +     GMimeStream *stream_memory = NULL, *stream_filter = NULL;\r
 184 \r
 185 No need to initialize stream_memory here.\r
 186 \r
 187 > +     GMimeDataWrapper *wrapper;\r
 188 > +     GByteArray *part_content;\r
 189 > +     const char *charset;\r
 190 > +\r
 191 > +     printf("\"content-type\": %s, \"content\": ",\r
 192 > +            json_quote_str(ctx, g_mime_content_type_to_string(content_type)));\r
 193 \r
 194 The style in notmuch is to have a space before the opening brace in\r
 195 function calls. Check elsewhere also. I always forget that too. :)\r
 196 \r
 197 > +\r
 198 > +     charset = g_mime_object_get_content_type_parameter (part, "charset");\r
 199 \r
 200 AFAICT charset declaration and the above call could be moved inside "if\r
 201 (stream_memory)" below.\r
 202 \r
 203 > +     stream_memory = g_mime_stream_mem_new ();\r
 204 > +     if (stream_memory) {\r
 205 > +         stream_filter = g_mime_stream_filter_new(stream_memory);\r
 206 > +         if (charset) {\r
 207 > +             g_mime_stream_filter_add(GMIME_STREAM_FILTER(stream_filter),\r
 208 > +                                      g_mime_filter_charset_new(charset, "UTF-8"));\r
 209 > +         }\r
 210 > +     }\r
 211 > +     wrapper = g_mime_part_get_content_object (GMIME_PART (part));\r
 212 > +     if (wrapper && stream_filter)\r
 213 > +         g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);\r
 214 > +     part_content = g_mime_stream_mem_get_byte_array (GMIME_STREAM_MEM (stream_memory));\r
 215 \r
 216 You only need charset, stream_memory and stream_filter if wrapper is\r
 217 true. You could perhaps include all that stuff within "if (wrapper)".\r
 218 \r
 219 > +\r
 220 > +     printf("%s", json_quote_chararray(ctx, (char *) part_content->data, part_content->len));\r
 221 > +\r
 222 > +     if (stream_filter)\r
 223 > +         g_object_unref(stream_filter);\r
 224 > +     if (stream_memory)\r
 225 > +         g_object_unref(stream_memory);\r
 226 > +    }\r
 227 > +\r
 228 > +    talloc_free (ctx);\r
 229 > +}\r
 230 > +\r
 231 >  /* Is the given address configured as one of the user's "personal" or\r
 232 >   * "other" addresses. */\r
 233 >  static int\r
 234 > @@ -476,6 +573,59 @@ guess_from_received_header (notmuch_config_t *config, notmuch_message_t *message\r
 235 >      return NULL;\r
 236 >  }\r
 237 >  \r
 238 > +static GMimeMessage *\r
 239 > +create_reply_message(void *ctx,\r
 240 > +                  notmuch_config_t *config,\r
 241 > +                  notmuch_message_t *message)\r
 242 > +{\r
 243 > +    const char *subject, *from_addr = NULL;\r
 244 > +    const char *in_reply_to, *orig_references, *references;\r
 245 > +\r
 246 > +    /* The 1 means we want headers in a "pretty" order. */\r
 247 > +    GMimeMessage *reply = g_mime_message_new (1);\r
 248 > +    if (reply == NULL) {\r
 249 > +     fprintf (stderr, "Out of memory\n");\r
 250 > +     return NULL;\r
 251 > +    }\r
 252 > +\r
 253 > +    subject = notmuch_message_get_header (message, "subject");\r
 254 > +    if (subject) {\r
 255 > +     if (strncasecmp (subject, "Re:", 3))\r
 256 > +         subject = talloc_asprintf (ctx, "Re: %s", subject);\r
 257 > +     g_mime_message_set_subject (reply, subject);\r
 258 > +    }\r
 259 > +\r
 260 > +    from_addr = add_recipients_from_message (reply, config, message);\r
 261 > +\r
 262 > +    if (from_addr == NULL)\r
 263 > +     from_addr = guess_from_received_header (config, message);\r
 264 > +\r
 265 > +    if (from_addr == NULL)\r
 266 > +     from_addr = notmuch_config_get_user_primary_email (config);\r
 267 > +\r
 268 > +    from_addr = talloc_asprintf (ctx, "%s <%s>",\r
 269 > +                              notmuch_config_get_user_name (config),\r
 270 > +                              from_addr);\r
 271 > +    g_mime_object_set_header (GMIME_OBJECT (reply),\r
 272 > +                           "From", from_addr);\r
 273 > +\r
 274 > +    in_reply_to = talloc_asprintf (ctx, "<%s>",\r
 275 > +                                notmuch_message_get_message_id (message));\r
 276 > +\r
 277 > +    g_mime_object_set_header (GMIME_OBJECT (reply),\r
 278 > +                           "In-Reply-To", in_reply_to);\r
 279 > +\r
 280 > +    orig_references = notmuch_message_get_header (message, "references");\r
 281 > +    references = talloc_asprintf (ctx, "%s%s%s",\r
 282 > +                               orig_references ? orig_references : "",\r
 283 > +                               orig_references ? " " : "",\r
 284 > +                               in_reply_to);\r
 285 > +    g_mime_object_set_header (GMIME_OBJECT (reply),\r
 286 > +                           "References", references);\r
 287 > +\r
 288 > +    return reply;\r
 289 > +}\r
 290 \r
 291 If you want to, you could make review easier by first having a patch\r
 292 that abstracts the above in the original code, with no other\r
 293 modifications. Then you could introduce new functionality that utilizes\r
 294 it later. But this is no big deal in such a small abstraction.\r
 295 \r
 296 > +\r
 297 >  static int\r
 298 >  notmuch_reply_format_default(void *ctx,\r
 299 >                            notmuch_config_t *config,\r
 300 > @@ -485,8 +635,6 @@ notmuch_reply_format_default(void *ctx,\r
 301 >      GMimeMessage *reply;\r
 302 >      notmuch_messages_t *messages;\r
 303 >      notmuch_message_t *message;\r
 304 > -    const char *subject, *from_addr = NULL;\r
 305 > -    const char *in_reply_to, *orig_references, *references;\r
 306 >      const notmuch_show_format_t *format = &format_reply;\r
 307 >  \r
 308 >      for (messages = notmuch_query_search_messages (query);\r
 309 > @@ -495,61 +643,102 @@ notmuch_reply_format_default(void *ctx,\r
 310 >      {\r
 311 >       message = notmuch_messages_get (messages);\r
 312 >  \r
 313 > -     /* The 1 means we want headers in a "pretty" order. */\r
 314 > -     reply = g_mime_message_new (1);\r
 315 > -     if (reply == NULL) {\r
 316 > -         fprintf (stderr, "Out of memory\n");\r
 317 > -         return 1;\r
 318 > -     }\r
 319 > +     reply = create_reply_message(ctx, config, message);\r
 320 \r
 321 You should check the return value.\r
 322 \r
 323 >  \r
 324 > -     subject = notmuch_message_get_header (message, "subject");\r
 325 > -     if (subject) {\r
 326 > -         if (strncasecmp (subject, "Re:", 3))\r
 327 > -             subject = talloc_asprintf (ctx, "Re: %s", subject);\r
 328 > -         g_mime_message_set_subject (reply, subject);\r
 329 > -     }\r
 330 > +     show_reply_headers (reply);\r
 331 >  \r
 332 > -     from_addr = add_recipients_from_message (reply, config, message);\r
 333 > +     g_object_unref (G_OBJECT (reply));\r
 334 > +     reply = NULL;\r
 335 >  \r
 336 > -     if (from_addr == NULL)\r
 337 > -         from_addr = guess_from_received_header (config, message);\r
 338 > +     printf ("On %s, %s wrote:\n",\r
 339 > +             notmuch_message_get_header (message, "date"),\r
 340 > +             notmuch_message_get_header (message, "from"));\r
 341 >  \r
 342 > -     if (from_addr == NULL)\r
 343 > -         from_addr = notmuch_config_get_user_primary_email (config);\r
 344 > +     show_message_body (message, format, params);\r
 345 >  \r
 346 > -     from_addr = talloc_asprintf (ctx, "%s <%s>",\r
 347 > -                                  notmuch_config_get_user_name (config),\r
 348 > -                                  from_addr);\r
 349 > -     g_mime_object_set_header (GMIME_OBJECT (reply),\r
 350 > -                               "From", from_addr);\r
 351 > +     notmuch_message_destroy (message);\r
 352 > +    }\r
 353 > +    return 0;\r
 354 > +}\r
 355 >  \r
 356 > -     in_reply_to = talloc_asprintf (ctx, "<%s>",\r
 357 > -                          notmuch_message_get_message_id (message));\r
 358 > +static int\r
 359 > +notmuch_reply_format_json(void *ctx,\r
 360 > +                       notmuch_config_t *config,\r
 361 > +                       notmuch_query_t *query,\r
 362 > +                       unused (notmuch_show_params_t *params))\r
 363 > +{\r
 364 > +    GMimeMessage *reply;\r
 365 > +    notmuch_messages_t *messages;\r
 366 > +    notmuch_message_t *message;\r
 367 > +    const notmuch_show_format_t *format = &format_json;\r
 368 >  \r
 369 > -     g_mime_object_set_header (GMIME_OBJECT (reply),\r
 370 > -                               "In-Reply-To", in_reply_to);\r
 371 > +    const char *reply_headers[] = {"from", "to", "subject", "in-reply-to", "references"};\r
 372 > +    const int n_reply_headers = 5;\r
 373 \r
 374 Drop this, and use ARRAY_SIZE (reply_headers) instead.\r
 375 \r
 376 > +    const char *orig_headers[] = {"from", "to", "cc", "subject", "date", "in-reply-to", "references"};\r
 377 > +    const int n_orig_headers = 7;\r
 378 \r
 379 Ditto.\r
 380 \r
 381 > +    int hidx;\r
 382 >  \r
 383 > -     orig_references = notmuch_message_get_header (message, "references");\r
 384 > -     references = talloc_asprintf (ctx, "%s%s%s",\r
 385 > -                                   orig_references ? orig_references : "",\r
 386 > -                                   orig_references ? " " : "",\r
 387 > -                                   in_reply_to);\r
 388 > -     g_mime_object_set_header (GMIME_OBJECT (reply),\r
 389 > -                               "References", references);\r
 390 > +    /* Start array of reply objects */\r
 391 > +    printf ("[");\r
 392 >  \r
 393 > -     show_reply_headers (reply);\r
 394 > +    for (messages = notmuch_query_search_messages (query);\r
 395 > +      notmuch_messages_valid (messages);\r
 396 > +      notmuch_messages_move_to_next (messages))\r
 397 > +    {\r
 398 > +     /* Start a reply object */\r
 399 > +     printf ("{ \"reply\": { \"headers\": { ");\r
 400 > +\r
 401 > +     message = notmuch_messages_get (messages);\r
 402 > +\r
 403 > +     reply = create_reply_message(ctx, config, message);\r
 404 \r
 405 Check return value.\r
 406 \r
 407 > +\r
 408 > +     for (hidx = 0; hidx < n_reply_headers; hidx += 1)\r
 409 \r
 410 hidx++ is the pattern in such for loops.\r
 411 \r
 412 > +     {\r
 413 > +         if (hidx > 0) {\r
 414 > +             printf (", ");\r
 415 > +         }\r
 416 \r
 417 You could just compare "if (hidx)", and also drop the curly braces.\r
 418 \r
 419 > +\r
 420 > +         printf ("%s: %s", json_quote_str(ctx, reply_headers[hidx]),\r
 421 > +                 json_quote_str (ctx, g_mime_object_get_header (GMIME_OBJECT (reply), reply_headers[hidx])));\r
 422 > +     }\r
 423 >  \r
 424 >       g_object_unref (G_OBJECT (reply));\r
 425 >       reply = NULL;\r
 426 >  \r
 427 > -     printf ("On %s, %s wrote:\n",\r
 428 > -             notmuch_message_get_header (message, "date"),\r
 429 > -             notmuch_message_get_header (message, "from"));\r
 430 > +     /* Done the headers for the reply, which has no body parts */\r
 431 > +     printf ("} }");\r
 432 > +\r
 433 > +     /* Start the original */\r
 434 > +     printf (", \"original\": { \"headers\": { ");\r
 435 > +\r
 436 > +     for (hidx = 0; hidx < n_orig_headers; hidx += 1)\r
 437 \r
 438 hidx++\r
 439 \r
 440 > +     {\r
 441 > +         if (hidx > 0) {\r
 442 > +             printf (", ");\r
 443 > +         }\r
 444 \r
 445 Same as above.\r
 446 \r
 447 > +\r
 448 > +         printf ("%s: %s", json_quote_str(ctx, orig_headers[hidx]),\r
 449 > +                 json_quote_str (ctx, notmuch_message_get_header (message, orig_headers[hidx])));\r
 450 > +     }\r
 451 > +\r
 452 > +     /* End headers */\r
 453 > +     printf (" }, \"body\": [ ");\r
 454 >  \r
 455 > +     /* Show body parts */\r
 456 >       show_message_body (message, format, params);\r
 457 >  \r
 458 >       notmuch_message_destroy (message);\r
 459 > +\r
 460 > +     /* Done the original */\r
 461 > +     printf ("{} ] }");\r
 462 > +\r
 463 > +     /* End the reply object. */\r
 464 > +     printf (" }, ");\r
 465 >      }\r
 466 > +\r
 467 > +    /* End array of reply objects */\r
 468 > +    printf ("{} ]\n");\r
 469 > +\r
 470 >      return 0;\r
 471 >  }\r
 472 >  \r
 473 > @@ -640,6 +829,8 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])\r
 474 >               reply_format_func = notmuch_reply_format_default;\r
 475 >           } else if (strcmp (opt, "headers-only") == 0) {\r
 476 >               reply_format_func = notmuch_reply_format_headers_only;\r
 477 > +         } else if (strcmp (opt, "json") == 0) {\r
 478 > +             reply_format_func = notmuch_reply_format_json;\r
 479 >           } else {\r
 480 >               fprintf (stderr, "Invalid value for --format: %s\n", opt);\r
 481 >               return 1;\r
 482 > -- \r
 483 > 1.7.5.4\r
 484 > \r
 485 > _______________________________________________\r
 486 > notmuch mailing list\r
 487 > notmuch@notmuchmail.org\r
 488 > http://notmuchmail.org/mailman/listinfo/notmuch\r