7b/85e383730fdfc21c7f17e81ab8f6d829ec1ca2

   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 62D99431FAF\r
   6         for <notmuch@notmuchmail.org>; Mon, 10 Dec 2012 13:18:29 -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 Qxf9TPvV2lYT for <notmuch@notmuchmail.org>;\r
  16         Mon, 10 Dec 2012 13:18:28 -0800 (PST)\r
  17 Received: from mail-lb0-f181.google.com (mail-lb0-f181.google.com\r
  18         [209.85.217.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 13A45431FAE\r
  21         for <notmuch@notmuchmail.org>; Mon, 10 Dec 2012 13:18:27 -0800 (PST)\r
  22 Received: by mail-lb0-f181.google.com with SMTP id ge1so2511251lbb.26\r
  23         for <notmuch@notmuchmail.org>; Mon, 10 Dec 2012 13:18:26 -0800 (PST)\r
  24 X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;\r
  25         d=google.com; s=20120113;\r
  26         h=from:to:cc:subject:in-reply-to:references:user-agent:date\r
  27         :message-id:mime-version:content-type:x-gm-message-state;\r
  28         bh=cjSpCj1bMz4SZsWtxB48Hws1Jx/as9QlO6fvs/Fz77I=;\r
  29         b=XLHLy1ogxHhfQ2S0n/OoFS86/DelMkIctWGTmd0qn5+gEzHhjy6VqYna7TWwLoMcfY\r
  30         6jPZPUvODses/TcgzebOrmDEgJnfGYQxVraBlNUs/ykYfo/kjvmtaMACrsZRQwrjoiKR\r
  31         uPvdb64NGAubziYViHHsgHPS0xPyqIcli1UIFRWW/anJoFcWwLxf9BtCRvyKK6iPD5YB\r
  32         ZWq2X+WDdZA9T0etMeg8hf0ZRGnWhYXMCGDs7r4nqC8uDlM9SXClHtdXVSQCs+yDeueA\r
  33         Xm8/T9STp0DKbWxpJtBI7OM9LFYgyypYH4VBBdla8KzLPu4WMNO/TGS+w/6blbeXzOL1\r
  34         wm5w==\r
  35 Received: by 10.152.145.8 with SMTP id sq8mr2807284lab.21.1355174306457;\r
  36         Mon, 10 Dec 2012 13:18:26 -0800 (PST)\r
  37 Received: from localhost (dsl-hkibrasgw4-fe51df00-27.dhcp.inet.fi.\r
  38         [80.223.81.27])\r
  39         by mx.google.com with ESMTPS id n7sm6956435lbg.3.2012.12.10.13.18.24\r
  40         (version=SSLv3 cipher=OTHER); Mon, 10 Dec 2012 13:18:25 -0800 (PST)\r
  41 From: Jani Nikula <jani@nikula.org>\r
  42 To: david@tethera.net, notmuch@notmuchmail.org\r
  43 Subject: Re: [Patch v6 2/6] notmuch-tag.c: convert to use tag-utils\r
  44 In-Reply-To: <1355096008-4544-3-git-send-email-david@tethera.net>\r
  45 References: <1355096008-4544-1-git-send-email-david@tethera.net>\r
  46         <1355096008-4544-3-git-send-email-david@tethera.net>\r
  47 User-Agent: Notmuch/0.14+138~g7041c56 (http://notmuchmail.org) Emacs/23.4.1\r
  48         (i686-pc-linux-gnu)\r
  49 Date: Mon, 10 Dec 2012 23:18:23 +0200\r
  50 Message-ID: <87k3spa9nk.fsf@nikula.org>\r
  51 MIME-Version: 1.0\r
  52 Content-Type: text/plain; charset=us-ascii\r
  53 X-Gm-Message-State:\r
  54  ALoCoQmlLRuvKARHO/ijmQebMyOhVl12V3/SJv2L4spVBq+IaoJLE8oAGRPWWtlEiAnvUrrIm6Rx\r
  55 Cc: David Bremner <bremner@debian.org>\r
  56 X-BeenThere: notmuch@notmuchmail.org\r
  57 X-Mailman-Version: 2.1.13\r
  58 Precedence: list\r
  59 List-Id: "Use and development of the notmuch mail system."\r
  60         <notmuch.notmuchmail.org>\r
  61 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  62         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  63 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  64 List-Post: <mailto:notmuch@notmuchmail.org>\r
  65 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  66 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  67         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  68 X-List-Received-Date: Mon, 10 Dec 2012 21:18:29 -0000\r
  69 \r
  70 On Mon, 10 Dec 2012, david@tethera.net wrote:\r
  71 > From: David Bremner <bremner@debian.org>\r
  72 >\r
  73 > Command line parsing is factored out into a function\r
  74 > parse_tag_command_line in tag-utils.c.\r
  75 >\r
  76 > There is some duplicated code eliminated in tag_query, and a bunch of\r
  77 > translation from using the bare tag_op structs to using that tag-utils\r
  78 > API.\r
  79 \r
  80 Functionally LGTM, nitpicks below.\r
  81 \r
  82 BR,\r
  83 Jani.\r
  84 \r
  85 > ---\r
  86 >  notmuch-tag.c |  104 +++++++++++++--------------------------------------------\r
  87 >  tag-util.c    |   48 ++++++++++++++++++++++++++\r
  88 >  tag-util.h    |   16 +++++++++\r
  89 >  3 files changed, 87 insertions(+), 81 deletions(-)\r
  90 >\r
  91 > diff --git a/notmuch-tag.c b/notmuch-tag.c\r
  92 > index 88d559b..b5cd578 100644\r
  93 > --- a/notmuch-tag.c\r
  94 > +++ b/notmuch-tag.c\r
  95 > @@ -19,6 +19,7 @@\r
  96 >   */\r
  97 >  \r
  98 >  #include "notmuch-client.h"\r
  99 > +#include "tag-util.h"\r
 100 >  \r
 101 >  static volatile sig_atomic_t interrupted;\r
 102 >  \r
 103 > @@ -54,14 +55,9 @@ _escape_tag (char *buf, const char *tag)\r
 104 >      return buf;\r
 105 >  }\r
 106 >  \r
 107 > -typedef struct {\r
 108 > -    const char *tag;\r
 109 > -    notmuch_bool_t remove;\r
 110 > -} tag_operation_t;\r
 111 > -\r
 112 >  static char *\r
 113 >  _optimize_tag_query (void *ctx, const char *orig_query_string,\r
 114 > -                  const tag_operation_t *tag_ops)\r
 115 > +                  const tag_op_list_t *list)\r
 116 >  {\r
 117 >      /* This is subtler than it looks.  Xapian ignores the '-' operator\r
 118 >       * at the beginning both queries and parenthesized groups and,\r
 119 > @@ -73,19 +69,20 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,\r
 120 >  \r
 121 >      char *escaped, *query_string;\r
 122 >      const char *join = "";\r
 123 > -    int i;\r
 124 > +    size_t i;\r
 125 >      unsigned int max_tag_len = 0;\r
 126 >  \r
 127 >      /* Don't optimize if there are no tag changes. */\r
 128 > -    if (tag_ops[0].tag == NULL)\r
 129 > +    if (tag_op_list_size (list) == 0)\r
 130 >       return talloc_strdup (ctx, orig_query_string);\r
 131 >  \r
 132 >      /* Allocate a buffer for escaping tags.  This is large enough to\r
 133 >       * hold a fully escaped tag with every character doubled plus\r
 134 >       * enclosing quotes and a NUL. */\r
 135 > -    for (i = 0; tag_ops[i].tag; i++)\r
 136 > -     if (strlen (tag_ops[i].tag) > max_tag_len)\r
 137 > -         max_tag_len = strlen (tag_ops[i].tag);\r
 138 > +    for (i = 0; i < tag_op_list_size (list); i++)\r
 139 > +     if (strlen (tag_op_list_tag (list, i)) > max_tag_len)\r
 140 > +         max_tag_len = strlen (tag_op_list_tag (list, i));\r
 141 > +\r
 142 >      escaped = talloc_array (ctx, char, max_tag_len * 2 + 3);\r
 143 >      if (! escaped)\r
 144 >       return NULL;\r
 145 > @@ -96,11 +93,11 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,\r
 146 >      else\r
 147 >       query_string = talloc_asprintf (ctx, "( %s ) and (", orig_query_string);\r
 148 >  \r
 149 > -    for (i = 0; tag_ops[i].tag && query_string; i++) {\r
 150 > +    for (i = 0; i < tag_op_list_size (list) && query_string; i++) {\r
 151 >       query_string = talloc_asprintf_append_buffer (\r
 152 >           query_string, "%s%stag:%s", join,\r
 153 > -         tag_ops[i].remove ? "" : "not ",\r
 154 > -         _escape_tag (escaped, tag_ops[i].tag));\r
 155 > +         tag_op_list_isremove (list, i) ? "" : "not ",\r
 156 > +         _escape_tag (escaped, tag_op_list_tag (list, i)));\r
 157 >       join = " or ";\r
 158 >      }\r
 159 >  \r
 160 > @@ -116,12 +113,11 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,\r
 161 >   * element. */\r
 162 >  static int\r
 163 >  tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,\r
 164 > -        tag_operation_t *tag_ops, notmuch_bool_t synchronize_flags)\r
 165 > +        tag_op_list_t *tag_ops, tag_op_flag_t flags)\r
 166 >  {\r
 167 >      notmuch_query_t *query;\r
 168 >      notmuch_messages_t *messages;\r
 169 >      notmuch_message_t *message;\r
 170 > -    int i;\r
 171 >  \r
 172 >      /* Optimize the query so it excludes messages that already have\r
 173 >       * the specified set of tags. */\r
 174 > @@ -144,21 +140,7 @@ tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,\r
 175 >        notmuch_messages_valid (messages) && ! interrupted;\r
 176 >        notmuch_messages_move_to_next (messages)) {\r
 177 >       message = notmuch_messages_get (messages);\r
 178 > -\r
 179 > -     notmuch_message_freeze (message);\r
 180 > -\r
 181 > -     for (i = 0; tag_ops[i].tag; i++) {\r
 182 > -         if (tag_ops[i].remove)\r
 183 > -             notmuch_message_remove_tag (message, tag_ops[i].tag);\r
 184 > -         else\r
 185 > -             notmuch_message_add_tag (message, tag_ops[i].tag);\r
 186 > -     }\r
 187 > -\r
 188 > -     notmuch_message_thaw (message);\r
 189 > -\r
 190 > -     if (synchronize_flags)\r
 191 > -         notmuch_message_tags_to_maildir_flags (message);\r
 192 > -\r
 193 > +     tag_op_list_apply (message, tag_ops, flags | TAG_FLAG_PRE_OPTIMIZED);\r
 194 >       notmuch_message_destroy (message);\r
 195 >      }\r
 196 >  \r
 197 > @@ -170,15 +152,13 @@ tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,\r
 198 >  int\r
 199 >  notmuch_tag_command (void *ctx, int argc, char *argv[])\r
 200 >  {\r
 201 > -    tag_operation_t *tag_ops;\r
 202 > -    int tag_ops_count = 0;\r
 203 > -    char *query_string;\r
 204 > +    tag_op_list_t *tag_ops = NULL;\r
 205 > +    char *query_string = NULL;\r
 206 >      notmuch_config_t *config;\r
 207 >      notmuch_database_t *notmuch;\r
 208 >      struct sigaction action;\r
 209 > -    notmuch_bool_t synchronize_flags;\r
 210 > -    int i;\r
 211 > -    int ret;\r
 212 > +    tag_op_flag_t tag_flags = TAG_FLAG_NONE;\r
 213 > +    int ret = 0;\r
 214 >  \r
 215 >      /* Setup our handler for SIGINT */\r
 216 >      memset (&action, 0, sizeof (struct sigaction));\r
 217 > @@ -187,54 +167,15 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])\r
 218 >      action.sa_flags = SA_RESTART;\r
 219 >      sigaction (SIGINT, &action, NULL);\r
 220 >  \r
 221 > -    argc--; argv++; /* skip subcommand argument */\r
 222 > -\r
 223 > -    /* Array of tagging operations (add or remove), terminated with an\r
 224 > -     * empty element. */\r
 225 > -    tag_ops = talloc_array (ctx, tag_operation_t, argc + 1);\r
 226 > +    tag_ops = tag_op_list_create (ctx);\r
 227 >      if (tag_ops == NULL) {\r
 228 >       fprintf (stderr, "Out of memory.\n");\r
 229 >       return 1;\r
 230 >      }\r
 231 >  \r
 232 > -    for (i = 0; i < argc; i++) {\r
 233 > -     if (strcmp (argv[i], "--") == 0) {\r
 234 > -         i++;\r
 235 > -         break;\r
 236 > -     }\r
 237 > -     if (argv[i][0] == '+' || argv[i][0] == '-') {\r
 238 > -         if (argv[i][0] == '+' && argv[i][1] == '\0') {\r
 239 > -             fprintf (stderr, "Error: tag names cannot be empty.\n");\r
 240 > -             return 1;\r
 241 > -         }\r
 242 > -         if (argv[i][0] == '+' && argv[i][1] == '-') {\r
 243 > -             /* This disallows adding the non-removable tag "-" and\r
 244 > -              * enables notmuch tag to take long options in the\r
 245 > -              * future. */\r
 246 > -             fprintf (stderr, "Error: tag names must not start with '-'.\n");\r
 247 > -             return 1;\r
 248 > -         }\r
 249 > -         tag_ops[tag_ops_count].tag = argv[i] + 1;\r
 250 > -         tag_ops[tag_ops_count].remove = (argv[i][0] == '-');\r
 251 > -         tag_ops_count++;\r
 252 > -     } else {\r
 253 > -         break;\r
 254 > -     }\r
 255 > -    }\r
 256 > -\r
 257 > -    tag_ops[tag_ops_count].tag = NULL;\r
 258 > -\r
 259 > -    if (tag_ops_count == 0) {\r
 260 > -     fprintf (stderr, "Error: 'notmuch tag' requires at least one tag to add or remove.\n");\r
 261 > -     return 1;\r
 262 > -    }\r
 263 > -\r
 264 > -    query_string = query_string_from_args (ctx, argc - i, &argv[i]);\r
 265 > -\r
 266 > -    if (*query_string == '\0') {\r
 267 > -     fprintf (stderr, "Error: notmuch tag requires at least one search term.\n");\r
 268 > +    if (parse_tag_command_line (ctx, argc - 1, argv + 1,\r
 269 > +                             0, &query_string, tag_ops))\r
 270 >       return 1;\r
 271 > -    }\r
 272 >  \r
 273 >      config = notmuch_config_open (ctx, NULL, NULL);\r
 274 >      if (config == NULL)\r
 275 > @@ -244,9 +185,10 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])\r
 276 >                              NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))\r
 277 >       return 1;\r
 278 >  \r
 279 > -    synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);\r
 280 > +    if (notmuch_config_get_maildir_synchronize_flags (config))\r
 281 > +     tag_flags |= TAG_FLAG_MAILDIR_SYNC;\r
 282 >  \r
 283 > -    ret = tag_query (ctx, notmuch, query_string, tag_ops, synchronize_flags);\r
 284 > +    ret = tag_query (ctx, notmuch, query_string, tag_ops, tag_flags);\r
 285 >  \r
 286 >      notmuch_database_destroy (notmuch);\r
 287 >  \r
 288 > diff --git a/tag-util.c b/tag-util.c\r
 289 > index c071ea8..6e3c069 100644\r
 290 > --- a/tag-util.c\r
 291 > +++ b/tag-util.c\r
 292 > @@ -162,6 +162,54 @@ parse_tag_line (void *ctx, char *line,\r
 293 >      return ret;\r
 294 >  }\r
 295 >  \r
 296 > +int\r
 297 > +parse_tag_command_line (void *ctx, int argc, char **argv,\r
 298 > +                     unused(tag_op_flag_t flags),\r
 299 \r
 300 Flags aren't used in the whole series; do you have something in mind for\r
 301 the future?\r
 302 \r
 303 > +                     char **query_str, tag_op_list_t *tag_ops){\r
 304 \r
 305 Add \n before {.\r
 306 \r
 307 > +\r
 308 > +    int i;\r
 309 > +\r
 310 > +    tag_op_list_reset(tag_ops);\r
 311 \r
 312 Space before (.\r
 313 \r
 314 > +\r
 315 > +    for (i = 0; i < argc; i++) {\r
 316 > +     if (strcmp (argv[i], "--") == 0) {\r
 317 > +         i++;\r
 318 > +         break;\r
 319 > +     }\r
 320 > +\r
 321 > +     if (argv[i][0] == '+' || argv[i][0] == '-') {\r
 322 \r
 323 Could also have\r
 324 \r
 325         if (argv[i][0] != '+' && argv[i][0] != '-')\r
 326                 break;\r
 327 \r
 328 here and take the stuff below out of the if block. Would get rid of the\r
 329 lone break in the else block. But that's just bikeshedding...\r
 330 \r
 331 > +         notmuch_bool_t is_remove = argv[i][0] == '-';\r
 332 > +         const char *msg;\r
 333 > +\r
 334 > +         msg = illegal_tag(argv[i]+1, is_remove);\r
 335 \r
 336 Spaces around +.\r
 337 \r
 338 > +         if (msg) {\r
 339 > +             fprintf (stderr, "Error: %s", msg);\r
 340 > +             return 1;\r
 341 > +         }\r
 342 > +\r
 343 > +         tag_op_list_append (ctx, tag_ops,\r
 344 > +                             argv[i] + 1, (argv[i][0] == '-'));\r
 345 \r
 346 You have is_remove you can use as the last parameter.\r
 347 \r
 348 > +     } else {\r
 349 > +         break;\r
 350 > +     }\r
 351 > +    }\r
 352 > +\r
 353 > +    if (tag_op_list_size (tag_ops) == 0) {\r
 354 > +     fprintf (stderr, "Error: 'notmuch tag' requires at least one tag to add or remove.\n");\r
 355 > +     return 1;\r
 356 > +    }\r
 357 > +\r
 358 > +    *query_str = query_string_from_args (ctx, argc - i, &argv[i]);\r
 359 > +\r
 360 \r
 361 Pedantically you should check if (*query_string == NULL) here (though\r
 362 this check didn't exist previously either).\r
 363 \r
 364 > +    if (**query_str == '\0') {\r
 365 > +     fprintf (stderr, "Error: notmuch tag requires at least one search term.\n");\r
 366 > +     return 1;\r
 367 > +    }\r
 368 > +\r
 369 > +    return 0;\r
 370 > +}\r
 371 > +\r
 372 > +\r
 373 >  static inline void\r
 374 >  message_error (notmuch_message_t *message,\r
 375 >              notmuch_status_t status,\r
 376 > diff --git a/tag-util.h b/tag-util.h\r
 377 > index 99b0fa0..4956725 100644\r
 378 > --- a/tag-util.h\r
 379 > +++ b/tag-util.h\r
 380 > @@ -72,6 +72,22 @@ parse_tag_line (void *ctx, char *line,\r
 381 >               tag_op_flag_t flags,\r
 382 >               char **query_str, tag_op_list_t *ops);\r
 383 >  \r
 384 > +\r
 385 > +\r
 386 > +/* Parse a command line of the following format:\r
 387 > + *\r
 388 > + * +<tag>|-<tag> [...] [--] <search-terms>\r
 389 > + *\r
 390 > + * Output Parameters:\r
 391 > + *   ops     contains a list of tag operations\r
 392 > + *   query_str the search terms.\r
 393 > + */\r
 394 > +\r
 395 > +tag_parse_status_t\r
 396 \r
 397 Either make this an int, or change the type in .c to tag_parse_status_t\r
 398 too and use the enums in the code.\r
 399 \r
 400 > +parse_tag_command_line (void *ctx, int argc, char **argv,\r
 401 > +                     tag_op_flag_t flags,\r
 402 > +                     char **query_str, tag_op_list_t *ops);\r
 403 > +\r
 404 >  /*\r
 405 >   * Create an empty list of tag operations\r
 406 >   *\r
 407 > -- \r
 408 > 1.7.10.4\r
 409 >\r
 410 > _______________________________________________\r
 411 > notmuch mailing list\r
 412 > notmuch@notmuchmail.org\r
 413 > http://notmuchmail.org/mailman/listinfo/notmuch\r