21/4b9f59cb903c2c52b85d27e53de18b7db9ca11

   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 BA664431FAF\r
   6         for <notmuch@notmuchmail.org>; Sat, 24 Nov 2012 16:23:34 -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 JDXLZN2cXbGF for <notmuch@notmuchmail.org>;\r
  16         Sat, 24 Nov 2012 16:23:30 -0800 (PST)\r
  17 Received: from dmz-mailsec-scanner-4.mit.edu (DMZ-MAILSEC-SCANNER-4.MIT.EDU\r
  18         [18.9.25.15])\r
  19         by olra.theworths.org (Postfix) with ESMTP id 89EDC431FAE\r
  20         for <notmuch@notmuchmail.org>; Sat, 24 Nov 2012 16:23:30 -0800 (PST)\r
  21 X-AuditID: 1209190f-b7f636d00000095b-b6-50b16501721c\r
  22 Received: from mailhub-auth-4.mit.edu ( [18.7.62.39])\r
  23         by dmz-mailsec-scanner-4.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id CA.96.02395.10561B05; Sat, 24 Nov 2012 19:23:29 -0500 (EST)\r
  25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
  26         by mailhub-auth-4.mit.edu (8.13.8/8.9.2) with ESMTP id qAP0NSwK027407; \r
  27         Sat, 24 Nov 2012 19:23:29 -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 qAP0NQPG005608\r
  32         (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT);\r
  33         Sat, 24 Nov 2012 19:23:27 -0500 (EST)\r
  34 Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.80)\r
  35         (envelope-from <amdragon@mit.edu>)\r
  36         id 1TcQ0M-0006XZ-GL; Sat, 24 Nov 2012 19:23:26 -0500\r
  37 Date: Sat, 24 Nov 2012 19:23:26 -0500\r
  38 From: Austin Clements <amdragon@MIT.EDU>\r
  39 To: markwalters1009 <markwalters1009@gmail.com>\r
  40 Subject: Re: [PATCH v2 6/7] cli: allow search mode to include msg-ids with\r
  41         JSON output\r
  42 Message-ID: <20121125002326.GJ4562@mit.edu>\r
  43 References: <1353763256-32336-1-git-send-email-markwalters1009@gmail.com>\r
  44         <1353763256-32336-7-git-send-email-markwalters1009@gmail.com>\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: <1353763256-32336-7-git-send-email-markwalters1009@gmail.com>\r
  49 User-Agent: Mutt/1.5.21 (2010-09-15)\r
  50 X-Brightmail-Tracker:\r
  51  H4sIAAAAAAAAA+NgFprAKsWRmVeSWpSXmKPExsUixG6nrsuYujHA4PMrbovVc3ksrt+cyezA\r
  52         5LFz1l12j2erbjEHMEVx2aSk5mSWpRbp2yVwZfw7som5YLNfxaql65gbGCdYdzFyckgImEj8\r
  53         3NfHDGGLSVy4t56ti5GLQ0hgH6PEkm8PmCCcDYwS16ZvY4VwLjJJPJj6jgXCWcIoseLNHUaQ\r
  54         fhYBVYkTZ2+wgNhsAhoS2/YvB4uLCOhL7Flxmw3EZhaQlvj2u5kJxBYWiJD4/fwUWA2vgLZE\r
  55         y8UORoihnYwSf4/thkoISpyc+YQFollL4sa/l0DNHGCDlv/jAAlzCnhJHH3Qyw5iiwqoSEw5\r
  56         uY1tAqPQLCTds5B0z0LoXsDIvIpRNiW3Sjc3MTOnODVZtzg5MS8vtUjXRC83s0QvNaV0EyMo\r
  57         sDkl+XcwfjuodIhRgINRiYf3RuLGACHWxLLiytxDjJIcTEqivFOSgUJ8SfkplRmJxRnxRaU5\r
  58         qcWHGCU4mJVEeK1VgXK8KYmVValF+TApaQ4WJXHeqyk3/YUE0hNLUrNTUwtSi2CyMhwcShK8\r
  59         00GGChalpqdWpGXmlCCkmTg4QYbzAA1vAKnhLS5IzC3OTIfIn2LU5Zgzs/0JoxBLXn5eqpQ4\r
  60         71GQIgGQoozSPLg5sIT0ilEc6C1h3osgVTzAZAY36RXQEiagJU9nrwNZUpKIkJJqYJzvJH1j\r
  61         3ZIkuXMFt7N7lryxuj6ja+IuhrDtL/823TON/b/oOfvJxNOTjUyZds1ed2NuFU+G5XnmhN9B\r
  62         Ptb9PRxv5QW56qetlbl7XK54DbPE62PvJJ793HhpzcEFmY08t4wdZ+s55+Y2bG9s1JZ4wurJ\r
  63         1npe5+0nDq2JbWcWeHjwa0QkaU5+eFaJpTgj0VCLuag4EQCVq5jlIwMAAA==\r
  64 Cc: notmuch@notmuchmail.org\r
  65 X-BeenThere: notmuch@notmuchmail.org\r
  66 X-Mailman-Version: 2.1.13\r
  67 Precedence: list\r
  68 List-Id: "Use and development of the notmuch mail system."\r
  69         <notmuch.notmuchmail.org>\r
  70 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  71         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  72 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  73 List-Post: <mailto:notmuch@notmuchmail.org>\r
  74 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  75 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  76         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  77 X-List-Received-Date: Sun, 25 Nov 2012 00:23:34 -0000\r
  78 \r
  79 Quoth markwalters1009 on Nov 24 at  1:20 pm:\r
  80 > From: Mark Walters <markwalters1009@gmail.com>\r
  81 > \r
  82 > This adds a --queries=true option which modifies the summary output of\r
  83 > notmuch search by including two extra query strings with each result:\r
  84 > one query string specifies all matching messages and one query string\r
  85 > all non-matching messages. Currently these are just lists of message\r
  86 > ids joined with " or " but that could change in future.\r
  87 > \r
  88 > Currently this is not implemented for text format.\r
  89 > ---\r
  90 >  notmuch-search.c |   95 ++++++++++++++++++++++++++++++++++++++++++++++++++---\r
  91 >  1 files changed, 89 insertions(+), 6 deletions(-)\r
  92 > \r
  93 > diff --git a/notmuch-search.c b/notmuch-search.c\r
  94 > index 830c4e4..c8fc9a6 100644\r
  95 > --- a/notmuch-search.c\r
  96 > +++ b/notmuch-search.c\r
  97 > @@ -26,7 +26,8 @@ typedef enum {\r
  98 >      OUTPUT_THREADS,\r
  99 >      OUTPUT_MESSAGES,\r
 100 >      OUTPUT_FILES,\r
 101 > -    OUTPUT_TAGS\r
 102 > +    OUTPUT_TAGS,\r
 103 > +    OUTPUT_SUMMARY_WITH_QUERIES\r
 104 >  } output_t;\r
 105 >  \r
 106 >  static char *\r
 107 > @@ -46,6 +47,57 @@ sanitize_string (const void *ctx, const char *str)\r
 108 >      return out;\r
 109 >  }\r
 110 >  \r
 111 > +/* This function takes a message id and returns an escaped string\r
 112 > + * which can be used as a Xapian query. This involves prefixing with\r
 113 > + * `id:', putting the id inside double quotes, and doubling any\r
 114 > + * occurence of a double quote in the message id itself.*/\r
 115 > +static char *\r
 116 > +xapian_escape_id (const void *ctx,\r
 117 > +        const char *msg_id)\r
 118 > +{\r
 119 > +    const char *c;\r
 120 > +    char *escaped_msg_id;\r
 121 > +    escaped_msg_id = talloc_strdup (ctx, "id:\"");\r
 122 \r
 123 talloc_strdup can fail.\r
 124 \r
 125 > +    for (c=msg_id; *c; c++)\r
 126 \r
 127 Missing spaces around =.\r
 128 \r
 129 > +     if (*c == '"')\r
 130 > +         escaped_msg_id = talloc_asprintf_append (escaped_msg_id, "\"\"");\r
 131 > +     else\r
 132 > +         escaped_msg_id = talloc_asprintf_append (escaped_msg_id, "%c", *c);\r
 133 \r
 134 .. as can talloc_asprintf_append.\r
 135 \r
 136 > +    escaped_msg_id = talloc_asprintf_append (escaped_msg_id, "\"");\r
 137 > +    return escaped_msg_id;\r
 138 > +}\r
 139 \r
 140 Unfortunately, this approach will cause reallocation and copying for\r
 141 every character in msg_id (as well as requiring talloc_asprintf_append\r
 142 to re-scan escaped_msg_id to find the end on every iteration).  How\r
 143 about pre-allocating a large enough buffer and keeping your position\r
 144 in it, like\r
 145 \r
 146 /* This function takes a message id and returns an escaped string\r
 147  * which can be used as a Xapian query. This involves prefixing with\r
 148  * `id:', putting the id inside double quotes, and doubling any\r
 149  * occurence of a double quote in the message id itself. Returns NULL\r
 150  * if memory allocation fails. */\r
 151 static char *\r
 152 xapian_escape_id (const void *ctx,\r
 153                   const char *msg_id)\r
 154 {\r
 155     const char *in;\r
 156     char *out;\r
 157     char *escaped_msg_id = talloc_array (ctx, char, 6 + strlen (msg_id) * 2);\r
 158     if (!escaped_msg_id)\r
 159         return NULL;\r
 160     strcpy (escaped_msg_id, "id:\"");\r
 161     out = escaped_msg_id + 4;\r
 162     for (in = msg_id; *in; ++in) {\r
 163         if (*in == '"')\r
 164             *(out++) = '"';\r
 165         *(out++) = *in;\r
 166     }\r
 167     strcpy(out, "\"");\r
 168     return escaped_msg_id;\r
 169 }\r
 170 \r
 171 > +\r
 172 > +static char *\r
 173 > +output_msg_query (const void *ctx,\r
 174 > +             sprinter_t *format,\r
 175 > +             notmuch_bool_t matching,\r
 176 > +             notmuch_bool_t first,\r
 177 > +             notmuch_messages_t *messages)\r
 178 > +{\r
 179 > +    notmuch_message_t *message;\r
 180 > +    char *query, *escaped_msg_id;\r
 181 > +    query = talloc_strdup (ctx, "");\r
 182 > +    for (;\r
 183 > +      notmuch_messages_valid (messages);\r
 184 > +      notmuch_messages_move_to_next (messages))\r
 185 > +    {\r
 186 > +     message = notmuch_messages_get (messages);\r
 187 > +     if (notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH) == matching) {\r
 188 > +         escaped_msg_id = xapian_escape_id (ctx, notmuch_message_get_message_id (message));\r
 189 \r
 190 Two long lines.\r
 191 \r
 192 > +         if (first) {\r
 193 > +             query = talloc_asprintf_append (query, "%s", escaped_msg_id);\r
 194 > +             first = FALSE;\r
 195 > +         }\r
 196 > +         else\r
 197 \r
 198 "} else".\r
 199 \r
 200 > +             query = talloc_asprintf_append (query, " or %s", escaped_msg_id);\r
 201 \r
 202 The "or" is unnecessary, since id is registered with the query parser\r
 203 as an exclusive boolean term.\r
 204 \r
 205 You could simplify this to\r
 206 \r
 207   query = talloc_asprintf_append (query, "%s%s", first ? "" : " ", \r
 208                                   escaped_msg_id);\r
 209 \r
 210 Technically this loop has the same O(n^2) problem as xapian_escape_id\r
 211 and query_string_from_args, but given that threads rarely have more\r
 212 than a few dozen messages in them, perhaps it doesn't matter.  OTOH,\r
 213 this may deal poorly with pathological threads (autogenerated messages\r
 214 and such).\r
 215 \r
 216 I wonder if we should have some simple linear-time talloc string\r
 217 accumulation abstraction in util/...\r
 218 \r
 219 > +         talloc_free (escaped_msg_id);\r
 220 > +     }\r
 221 > +     /* output_msg_query already starts with an ` or' */\r
 222 > +     query = talloc_asprintf_append (query, "%s", output_msg_query (ctx, format, matching, first, notmuch_message_get_replies (message)));\r
 223 \r
 224 Oof, how unfortunate.  I've got a patch that adds an iterator over all\r
 225 of the messages in a thread, which would make this much simpler (I\r
 226 don't know how we've gotten this far without such an API).  I'll clean\r
 227 that up and send it.  This shouldn't block this patch, but whichever\r
 228 goes in second should clean this up.\r
 229 \r
 230 Also, long line.\r
 231 \r
 232 > +    }\r
 233 > +    return query;\r
 234 > +}\r
 235 > +\r
 236 >  static int\r
 237 >  do_search_threads (sprinter_t *format,\r
 238 >                  notmuch_query_t *query,\r
 239 > @@ -88,7 +140,7 @@ do_search_threads (sprinter_t *format,\r
 240 >           format->string (format,\r
 241 >                           notmuch_thread_get_thread_id (thread));\r
 242 >           format->separator (format);\r
 243 > -     } else { /* output == OUTPUT_SUMMARY */\r
 244 > +     } else { /* output == OUTPUT_SUMMARY or OUTPUT_SUMMARY_WITH_QUERIES */\r
 245 >           void *ctx_quote = talloc_new (thread);\r
 246 >           const char *authors = notmuch_thread_get_authors (thread);\r
 247 >           const char *subject = notmuch_thread_get_subject (thread);\r
 248 > @@ -108,7 +160,7 @@ do_search_threads (sprinter_t *format,\r
 249 >           relative_date = notmuch_time_relative_date (ctx_quote, date);\r
 250 >  \r
 251 >           if (format->is_text_printer) {\r
 252 > -                /* Special case for the text formatter */\r
 253 > +               /* Special case for the text formatter */\r
 254 \r
 255 Unintentional whitespace change?  (Actually, this line isn't indented\r
 256 with tabs either before or after this change; how'd that happen?)\r
 257 \r
 258 >               printf ("thread:%s %12s [%d/%d] %s; %s (",\r
 259 >                       thread_id,\r
 260 >                       relative_date,\r
 261 > @@ -133,8 +185,6 @@ do_search_threads (sprinter_t *format,\r
 262 >               format->string (format, subject);\r
 263 >           }\r
 264 >  \r
 265 > -         talloc_free (ctx_quote);\r
 266 > -\r
 267 >           format->map_key (format, "tags");\r
 268 >           format->begin_list (format);\r
 269 >  \r
 270 > @@ -145,7 +195,7 @@ do_search_threads (sprinter_t *format,\r
 271 >               const char *tag = notmuch_tags_get (tags);\r
 272 >  \r
 273 >               if (format->is_text_printer) {\r
 274 > -                  /* Special case for the text formatter */\r
 275 > +                 /* Special case for the text formatter */\r
 276 \r
 277 Same?  Looks like here you converted it to tabs.\r
 278 \r
 279 >                   if (first_tag)\r
 280 >                       first_tag = FALSE;\r
 281 >                   else\r
 282 > @@ -160,8 +210,25 @@ do_search_threads (sprinter_t *format,\r
 283 >               printf (")");\r
 284 >  \r
 285 >           format->end (format);\r
 286 > +\r
 287 > +         if (output == OUTPUT_SUMMARY_WITH_QUERIES) {\r
 288 > +             char *query;\r
 289 > +             query = output_msg_query (ctx_quote, format, TRUE, TRUE, notmuch_thread_get_toplevel_messages (thread));\r
 290 > +             if (strlen (query)) {\r
 291 > +                 format->map_key (format, "matching_msg_query");\r
 292 \r
 293 Maybe just matching_query?\r
 294 \r
 295 > +                 format->string (format, query);\r
 296 > +             }\r
 297 > +             query = output_msg_query (ctx_quote, format, FALSE, TRUE, notmuch_thread_get_toplevel_messages (thread));\r
 298 > +             if (strlen (query)) {\r
 299 > +                 format->map_key (format, "nonmatching_msg_query");\r
 300 \r
 301 nonmatching_query?\r
 302 \r
 303 Also, don't forget to update devel/schema.\r
 304 \r
 305 > +                 format->string (format, query);\r
 306 > +             }\r
 307 > +         }\r
 308 > +\r
 309 >           format->end (format);\r
 310 >           format->separator (format);\r
 311 > +\r
 312 > +         talloc_free (ctx_quote);\r
 313 >       }\r
 314 >  \r
 315 >       notmuch_thread_destroy (thread);\r
 316 > @@ -303,6 +370,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])\r
 317 >      int offset = 0;\r
 318 >      int limit = -1; /* unlimited */\r
 319 >      int exclude = EXCLUDE_TRUE;\r
 320 > +    notmuch_bool_t with_queries = FALSE;\r
 321 >      unsigned int i;\r
 322 >  \r
 323 >      enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }\r
 324 > @@ -323,12 +391,14 @@ notmuch_search_command (void *ctx, int argc, char *argv[])\r
 325 >                                 { "messages", OUTPUT_MESSAGES },\r
 326 >                                 { "files", OUTPUT_FILES },\r
 327 >                                 { "tags", OUTPUT_TAGS },\r
 328 > +                               { "with-queries", OUTPUT_SUMMARY_WITH_QUERIES },\r
 329 \r
 330 Was this intentional?\r
 331 \r
 332 >                                 { 0, 0 } } },\r
 333 >          { NOTMUCH_OPT_KEYWORD, &exclude, "exclude", 'x',\r
 334 >            (notmuch_keyword_t []){ { "true", EXCLUDE_TRUE },\r
 335 >                                    { "false", EXCLUDE_FALSE },\r
 336 >                                    { "flag", EXCLUDE_FLAG },\r
 337 >                                    { 0, 0 } } },\r
 338 > +        { NOTMUCH_OPT_BOOLEAN, &with_queries, "queries", 'b', 0 },\r
 339 \r
 340 Wrong indentation (since the next line is indented correctly you can\r
 341 be both locally and globally consistent).\r
 342 \r
 343 >       { NOTMUCH_OPT_INT, &offset, "offset", 'O', 0 },\r
 344 >       { NOTMUCH_OPT_INT, &limit, "limit", 'L', 0  },\r
 345 >       { 0, 0, 0, 0, 0 }\r
 346 > @@ -398,6 +468,19 @@ notmuch_search_command (void *ctx, int argc, char *argv[])\r
 347 >           notmuch_query_set_omit_excluded (query, FALSE);\r
 348 >      }\r
 349 >  \r
 350 > +    if (with_queries) {\r
 351 > +     if (format_sel == NOTMUCH_FORMAT_TEXT) {\r
 352 > +         fprintf (stderr, "Warning: --queries=true not implemented for text format.\n");\r
 353 > +         with_queries = FALSE;\r
 354 > +     }\r
 355 > +     if (output != OUTPUT_SUMMARY) {\r
 356 > +         fprintf (stderr, "Warning: --queries=true only implemented for --output=summary.\n");\r
 357 > +         with_queries = FALSE;\r
 358 > +     }\r
 359 > +    }\r
 360 > +\r
 361 > +    if (with_queries) output = OUTPUT_SUMMARY_WITH_QUERIES;\r
 362 \r
 363 Out of curiosity, why do this as a separate output type, rather than\r
 364 just passing a with_queries flag into do_search_threads?\r
 365 \r
 366 > +\r
 367 >      switch (output) {\r
 368 >      default:\r
 369 >      case OUTPUT_SUMMARY:\r