90/33cdcfdc06d350855b183063bd7d705e251af3

   1 Return-Path: <m.walters@qmul.ac.uk>\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 0E8EA431FB6\r
   6         for <notmuch@notmuchmail.org>; Thu, 22 Nov 2012 04:23:05 -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: -1.098\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5\r
  12         tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001,\r
  13         NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] 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 eLgKnG8jceqo for <notmuch@notmuchmail.org>;\r
  17         Thu, 22 Nov 2012 04:23:03 -0800 (PST)\r
  18 Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6])\r
  19         (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))\r
  20         (No client certificate requested)\r
  21         by olra.theworths.org (Postfix) with ESMTPS id C2CE6431FAF\r
  22         for <notmuch@notmuchmail.org>; Thu, 22 Nov 2012 04:23:02 -0800 (PST)\r
  23 Received: from smtp.qmul.ac.uk ([138.37.6.40])\r
  24         by mail2.qmul.ac.uk with esmtp (Exim 4.71)\r
  25         (envelope-from <m.walters@qmul.ac.uk>)\r
  26         id 1TbVnu-0002GW-4k; Thu, 22 Nov 2012 12:22:52 +0000\r
  27 Received: from 93-97-24-31.zone5.bethere.co.uk ([93.97.24.31] helo=localhost)\r
  28         by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69)\r
  29         (envelope-from <m.walters@qmul.ac.uk>)\r
  30         id 1TbVnt-0004DL-Dc; Thu, 22 Nov 2012 12:22:50 +0000\r
  31 From: Mark Walters <markwalters1009@gmail.com>\r
  32 To: david@tethera.net, notmuch@notmuchmail.org\r
  33 Subject: Re: [PATCH 08/16] tag-util.[ch]: New files for common tagging\r
  34  routines\r
  35 In-Reply-To: <1353265498-3839-9-git-send-email-david@tethera.net>\r
  36 References: <1353265498-3839-1-git-send-email-david@tethera.net>\r
  37         <1353265498-3839-9-git-send-email-david@tethera.net>\r
  38 User-Agent: Notmuch/0.14+81~g9730584 (http://notmuchmail.org) Emacs/23.4.1\r
  39         (x86_64-pc-linux-gnu)\r
  40 Date: Thu, 22 Nov 2012 12:22:50 +0000\r
  41 Message-ID: <87k3tdltd1.fsf@qmul.ac.uk>\r
  42 MIME-Version: 1.0\r
  43 Content-Type: text/plain; charset=us-ascii\r
  44 X-Sender-Host-Address: 93.97.24.31\r
  45 X-QM-SPAM-Info: Sender has good ham record.  :)\r
  46 X-QM-Body-MD5: 00d652e871f3b7c3eb9a7da70df9034d (of first 20000 bytes)\r
  47 X-SpamAssassin-Score: -1.8\r
  48 X-SpamAssassin-SpamBar: -\r
  49 X-SpamAssassin-Report: The QM spam filters have analysed this message to\r
  50         determine if it is\r
  51         spam. We require at least 5.0 points to mark a message as spam.\r
  52         This message scored -1.8 points.\r
  53         Summary of the scoring: \r
  54         * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/,\r
  55         *      medium trust\r
  56         *      [138.37.6.40 listed in list.dnswl.org]\r
  57         * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\r
  58         provider *      (markwalters1009[at]gmail.com)\r
  59         *  0.5 AWL AWL: From: address is in the auto white-list\r
  60 X-QM-Scan-Virus: ClamAV says the message is clean\r
  61 Cc: David Bremner <bremner@debian.org>\r
  62 X-BeenThere: notmuch@notmuchmail.org\r
  63 X-Mailman-Version: 2.1.13\r
  64 Precedence: list\r
  65 List-Id: "Use and development of the notmuch mail system."\r
  66         <notmuch.notmuchmail.org>\r
  67 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  68         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  69 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  70 List-Post: <mailto:notmuch@notmuchmail.org>\r
  71 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  72 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  73         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  74 X-List-Received-Date: Thu, 22 Nov 2012 12:23:05 -0000\r
  75 \r
  76 \r
  77 Hi\r
  78 \r
  79 I am slowly working my way through this series. I have some queries\r
  80 comments and things that confused me in this patch.\r
  81 \r
  82 One longer term concern (but I have not got far enough to see if it\r
  83 could be a problem): could someone use dump and then not be able to\r
  84 restore because some of the tags are now banned (eg starting with -\r
  85 etc)? Obviously anything which fails currently is not a concern, only if\r
  86 some cases stop some currently working cases.\r
  87 \r
  88 \r
  89 One particular comment is that most of the functions do not have a\r
  90 comment defining them in tag-util.h. So for example (at this point in\r
  91 reading the series) I am not sure of why we need both parse_tag_line and\r
  92 tag_op_list_from_string.\r
  93 \r
  94 \r
  95 On Sun, 18 Nov 2012, david@tethera.net wrote:\r
  96 > From: David Bremner <bremner@debian.org>\r
  97 >\r
  98 > These are meant to be shared between notmuch-tag and notmuch-restore.\r
  99 >\r
 100 > The bulk of the routines implement a "tag operation list" abstract\r
 101 > data type act as a structured representation of a set of tag\r
 102 > operations (typically coming from a single tag command or line of\r
 103 > input).\r
 104 > ---\r
 105 >  Makefile.local |    1 +\r
 106 >  tag-util.c     |  273 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\r
 107 >  tag-util.h     |   71 +++++++++++++++\r
 108 >  3 files changed, 345 insertions(+)\r
 109 >  create mode 100644 tag-util.c\r
 110 >  create mode 100644 tag-util.h\r
 111 >\r
 112 > diff --git a/Makefile.local b/Makefile.local\r
 113 > index 2b91946..854867d 100644\r
 114 > --- a/Makefile.local\r
 115 > +++ b/Makefile.local\r
 116 > @@ -274,6 +274,7 @@ notmuch_client_srcs =             \\r
 117 >       query-string.c          \\r
 118 >       mime-node.c             \\r
 119 >       crypto.c                \\r
 120 > +     tag-util.c\r
 121 >  \r
 122 >  notmuch_client_modules = $(notmuch_client_srcs:.c=.o)\r
 123 >  \r
 124 > diff --git a/tag-util.c b/tag-util.c\r
 125 > new file mode 100644\r
 126 > index 0000000..5329b1f\r
 127 > --- /dev/null\r
 128 > +++ b/tag-util.c\r
 129 > @@ -0,0 +1,273 @@\r
 130 > +#include "tag-util.h"\r
 131 > +#include "hex-escape.h"\r
 132 > +\r
 133 > +static char *\r
 134 > +strtok_len (char *s, const char *delim, size_t *len)\r
 135 > +{\r
 136 > +    /* skip initial delims */\r
 137 > +    s += strspn (s, delim);\r
 138 > +\r
 139 > +    /* length of token */\r
 140 > +    *len = strcspn (s, delim);\r
 141 > +\r
 142 > +    return *len ? s : NULL;\r
 143 > +}\r
 144 > +\r
 145 > +/* Tag messages according to 'input', which must consist of lines of\r
 146 > + * the format:\r
 147 > + *\r
 148 > + * +<tag>|-<tag> [...] [--] <search-terms>\r
 149 > + *\r
 150 > + * Each line is interpreted similarly to "notmuch tag" command line\r
 151 > + * arguments. The delimiter is one or more spaces ' '. Any characters\r
 152 > + * in <tag> and <search-terms> MAY be hex encoded with %NN where NN is\r
 153 > + * the hexadecimal value of the character. Any ' ' and '%' characters\r
 154 > + * in <tag> and <search-terms> MUST be hex encoded (using %20 and %25,\r
 155 > + * respectively). Any characters that are not part of <tag> or\r
 156 > + * <search-terms> MUST NOT be hex encoded.\r
 157 > + *\r
 158 > + * Leading and trailing space ' ' is ignored. Empty lines and lines\r
 159 > + * beginning with '#' are ignored.\r
 160 > + */\r
 161 \r
 162 The comment says lines starting with '#' are ignored but I don't see\r
 163 anything actually ignoring them.\r
 164 \r
 165 > +int\r
 166 > +parse_tag_line (void *ctx, char *line,\r
 167 > +             char **query_string,\r
 168 > +             tag_op_list_t *tag_ops)\r
 169 > +{\r
 170 > +    char *tok = line;\r
 171 > +    size_t tok_len = 0;\r
 172 > +\r
 173 > +    chomp_newline (line);\r
 174 > +    tag_op_list_reset (tag_ops);\r
 175 > +\r
 176 > +    /* Parse tags. */\r
 177 > +    while ((tok = strtok_len (tok + tok_len, " ", &tok_len)) != NULL) {\r
 178 > +     notmuch_bool_t remove;\r
 179 > +     char *tag;\r
 180 > +\r
 181 > +     /* Optional explicit end of tags marker. */\r
 182 > +     if (strncmp (tok, "--", tok_len) == 0) {\r
 183 > +         tok = strtok_len (tok + tok_len, " ", &tok_len);\r
 184 > +         break;\r
 185 > +     }\r
 186 > +\r
 187 > +     /* Implicit end of tags. */\r
 188 > +     if (*tok != '-' && *tok != '+')\r
 189 > +         break;\r
 190 > +\r
 191 > +     /* If tag is terminated by NUL, there's no query string. */\r
 192 > +     if (*(tok + tok_len) == '\0') {\r
 193 > +         tok = NULL;\r
 194 > +         break;\r
 195 > +     }\r
 196 > +\r
 197 > +     /* Terminate, and start next token after terminator. */\r
 198 > +     *(tok + tok_len++) = '\0';\r
 199 > +\r
 200 > +     remove = (*tok == '-');\r
 201 > +     tag = tok + 1;\r
 202 > +\r
 203 > +     /* Refuse empty tags. */\r
 204 > +     if (*tag == '\0') {\r
 205 > +         tok = NULL;\r
 206 > +         break;\r
 207 > +     }\r
 208 > +\r
 209 > +     /* Decode tag. */\r
 210 > +     if (hex_decode_inplace (tag) != HEX_SUCCESS) {\r
 211 > +         tok = NULL;\r
 212 > +         break;\r
 213 > +     }\r
 214 > +\r
 215 > +     if (tag_op_list_append (ctx, tag_ops, tag, remove))\r
 216 > +         return -1;\r
 217 > +    }\r
 218 > +\r
 219 > +    if (tok == NULL || tag_ops->count == 0) {\r
 220 > +     /* FIXME: line has been modified! */\r
 221 > +     fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",\r
 222 > +              line);\r
 223 > +     return 1;\r
 224 > +    }\r
 225 > +\r
 226 > +    /* tok now points to the query string */\r
 227 > +    if (hex_decode_inplace (tok) != HEX_SUCCESS) {\r
 228 > +     /* FIXME: line has been modified! */\r
 229 > +     fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",\r
 230 > +              line);\r
 231 > +     return 1;\r
 232 > +    }\r
 233 > +\r
 234 > +    *query_string = tok;\r
 235 > +\r
 236 > +    return 0;\r
 237 > +}\r
 238 > +\r
 239 > +static inline void\r
 240 > +message_error (notmuch_message_t *message,\r
 241 > +            notmuch_status_t status,\r
 242 > +            const char *format, ...)\r
 243 > +{\r
 244 > +    va_list va_args;\r
 245 > +\r
 246 > +    va_start (va_args, format);\r
 247 > +\r
 248 > +    vfprintf (stderr, format, va_args);\r
 249 > +    fprintf (stderr, "Message-ID: %s\n", notmuch_message_get_message_id (message));\r
 250 > +    fprintf (stderr, "Status: %s\n", notmuch_status_to_string (status));\r
 251 > +}\r
 252 > +\r
 253 > +notmuch_status_t\r
 254 > +tag_op_list_apply (notmuch_message_t *message,\r
 255 > +                tag_op_list_t *list,\r
 256 > +                tag_op_flag_t flags)\r
 257 > +{\r
 258 > +    int i;\r
 259 > +\r
 260 > +    notmuch_status_t status = 0;\r
 261 > +    tag_operation_t *tag_ops = list->ops;\r
 262 > +\r
 263 > +    status = notmuch_message_freeze (message);\r
 264 > +    if (status) {\r
 265 > +     message_error (message, status, "freezing message");\r
 266 > +     return status;\r
 267 > +    }\r
 268 > +\r
 269 > +    if (flags & TAG_FLAG_REMOVE_ALL) {\r
 270 > +     status = notmuch_message_remove_all_tags (message);\r
 271 > +     if (status) {\r
 272 > +         message_error (message, status, "removing all tags" );\r
 273 > +         return status;\r
 274 > +     }\r
 275 > +    }\r
 276 > +\r
 277 > +    for (i = 0; i < list->count; i++) {\r
 278 > +     if (tag_ops[i].remove) {\r
 279 > +         status = notmuch_message_remove_tag (message, tag_ops[i].tag);\r
 280 > +         if (status) {\r
 281 > +             message_error (message, status, "removing tag %s", tag_ops[i].tag);\r
 282 > +             return status;\r
 283 > +         }\r
 284 > +     } else {\r
 285 > +         status = notmuch_message_add_tag (message, tag_ops[i].tag);\r
 286 > +         if (status) {\r
 287 > +             message_error (message, status, "adding tag %s", tag_ops[i].tag);\r
 288 > +             return status;\r
 289 > +         }\r
 290 > +\r
 291 > +     }\r
 292 > +    }\r
 293 > +\r
 294 > +    status = notmuch_message_thaw (message);\r
 295 > +    if (status) {\r
 296 > +     message_error (message, status, "thawing message");\r
 297 > +     return status;\r
 298 > +    }\r
 299 > +\r
 300 > +\r
 301 > +    if (flags & TAG_FLAG_MAILDIR_SYNC) {\r
 302 > +     status = notmuch_message_tags_to_maildir_flags (message);\r
 303 > +     if (status) {\r
 304 > +         message_error (message, status, "synching tags to maildir");\r
 305 > +         return status;\r
 306 > +     }\r
 307 > +    }\r
 308 > +\r
 309 > +    return NOTMUCH_STATUS_SUCCESS;\r
 310 > +\r
 311 > +}\r
 312 > +\r
 313 > +\r
 314 > +/* Array of tagging operations (add or remove), terminated with an\r
 315 > + * empty element. Size will be increased as necessary. */\r
 316 \r
 317 Is the list termination used (above you used list->count) or is it just\r
 318 that you can always safely write to the last element?\r
 319 \r
 320 > +\r
 321 > +tag_op_list_t *\r
 322 > +tag_op_list_create (void *ctx)\r
 323 > +{\r
 324 > +    tag_op_list_t *list;\r
 325 > +\r
 326 > +    list = talloc (ctx, tag_op_list_t);\r
 327 > +    if (list == NULL)\r
 328 > +     return NULL;\r
 329 > +\r
 330 > +    list->size = TAG_OP_LIST_INITIAL_SIZE;\r
 331 > +    list->count = 0;\r
 332 > +\r
 333 > +    list->ops = talloc_array (ctx, tag_operation_t, list->size);\r
 334 > +    if (list->ops == NULL)\r
 335 > +     return NULL;\r
 336 > +\r
 337 > +    return list;\r
 338 > +}\r
 339 > +\r
 340 > +\r
 341 > +int\r
 342 > +tag_op_list_append (void *ctx,\r
 343 > +                 tag_op_list_t *list,\r
 344 > +                 const char *tag,\r
 345 > +                 notmuch_bool_t remove)\r
 346 > +{\r
 347 > +    list->ops[list->count].tag = tag;\r
 348 > +    list->ops[list->count].remove = remove;\r
 349 > +    list->count++;\r
 350 > +\r
 351 > +    /* Make room for terminating empty element and potential\r
 352 > +     * new tags, if necessary. This should be a fairly rare\r
 353 > +     * case, considering the initial array size. */\r
 354 > +    if (list->count == list->size) {\r
 355 > +     list->size *= 2;\r
 356 > +     list->ops = talloc_realloc (ctx, list->ops, tag_operation_t,\r
 357 > +                                 list->size);\r
 358 > +     if (list->ops == NULL) {\r
 359 > +         fprintf (stderr, "Out of memory.\n");\r
 360 > +         return 1;\r
 361 > +     }\r
 362 > +    }\r
 363 > +\r
 364 > +    list->ops[list->count].tag = NULL;\r
 365 > +\r
 366 > +    return 0;\r
 367 > +}\r
 368 > +\r
 369 > +\r
 370 > +/* WARNING: modifies it's string argument */\r
 371 > +\r
 372 > +int\r
 373 > +tag_op_list_from_string (void *ctx,\r
 374 > +                      char *tag_string,\r
 375 > +                      notmuch_bool_t remove,\r
 376 > +                      tag_op_list_t *tag_ops)\r
 377 \r
 378 As mentioned above I wasn't sure why both this and parse_tag_line are\r
 379 needed. Also should one use the other? Otherwise we could get different\r
 380 allowed tags in different places?\r
 381 \r
 382 > +{\r
 383 > +    char *tok = tag_string;\r
 384 > +    size_t tok_len = 0;\r
 385 > +\r
 386 > +    tag_op_list_reset (tag_ops);\r
 387 > +\r
 388 > +/* Parse tags. */\r
 389 > +    while ((tok = strtok_len (tok + tok_len, " ", &tok_len)) != NULL) {\r
 390 > +\r
 391 > +     if (*(tok + tok_len) != '\0') {\r
 392 > +         *(tok + tok_len) = '\0';\r
 393 > +         tok_len++;\r
 394 > +     }\r
 395 > +\r
 396 > +     if (tag_op_list_append (ctx, tag_ops, tok, remove))\r
 397 > +         return -1;\r
 398 > +    }\r
 399 > +\r
 400 > +    return 0;\r
 401 > +\r
 402 > +}\r
 403 > +\r
 404 > +notmuch_bool_t\r
 405 > +tag_op_list_empty (tag_op_list_t *list)\r
 406 > +{\r
 407 > +    return (list->count == 0);\r
 408 > +}\r
 409 > +\r
 410 > +\r
 411 > +void\r
 412 > +tag_op_list_reset (tag_op_list_t *list)\r
 413 > +{\r
 414 > +    list->count = 0;\r
 415 > +}\r
 416 > diff --git a/tag-util.h b/tag-util.h\r
 417 > new file mode 100644\r
 418 > index 0000000..b381b8e\r
 419 > --- /dev/null\r
 420 > +++ b/tag-util.h\r
 421 > @@ -0,0 +1,71 @@\r
 422 > +#ifndef _TAG_UTIL_H\r
 423 > +#define _TAG_UTIL_H\r
 424 > +\r
 425 > +#include "notmuch-client.h"\r
 426 > +\r
 427 > +typedef struct {\r
 428 > +    const char *tag;\r
 429 > +    notmuch_bool_t remove;\r
 430 > +} tag_operation_t;\r
 431 > +\r
 432 > +#define TAG_OP_LIST_INITIAL_SIZE 10\r
 433 > +\r
 434 > +typedef struct {\r
 435 > +    tag_operation_t *ops;\r
 436 > +    int count;\r
 437 > +    int size;\r
 438 > +} tag_op_list_t;\r
 439 \r
 440 I thought it would be helpful to say something about *ops: e.g., "ops\r
 441 points to the first element of an array of count tag_operation_t's. This\r
 442 array should be empty terminated (if true)."\r
 443 \r
 444 One trivial comment for the next patch: the commit message says "notmuch\r
 445 new" rather than "notmuch tag".\r
 446 \r
 447 Best wishes\r
 448 \r
 449 Mark\r
 450  \r
 451 > +\r
 452 > +/* Use powers of 2 */\r
 453 > +typedef enum  { TAG_FLAG_NONE = 0,\r
 454 > +             TAG_FLAG_MAILDIR_SYNC = 1,\r
 455 > +             TAG_FLAG_REMOVE_ALL = 2 } tag_op_flag_t;\r
 456 > +\r
 457 > +\r
 458 > +typedef int (*tag_callback_t)(void *ctx,\r
 459 > +                           notmuch_database_t *notmuch,\r
 460 > +                           const char *query_string,\r
 461 > +                           tag_op_list_t *tag_ops,\r
 462 > +                           tag_op_flag_t flags);\r
 463 > +\r
 464 > +int\r
 465 > +parse_tag_stream (void *ctx,\r
 466 > +               notmuch_database_t *notmuch,\r
 467 > +               FILE *input,\r
 468 > +               tag_callback_t callback,\r
 469 > +               tag_op_flag_t flags,\r
 470 > +               volatile sig_atomic_t *interrupted);\r
 471 > +\r
 472 > +/*\r
 473 > + * Returns: 0 for OK, 1 for skipped line, -1 for fatal(ish) error.\r
 474 > + */\r
 475 > +int\r
 476 > +parse_tag_line (void *ctx, char *line, char **query_str, tag_op_list_t *ops);\r
 477 > +\r
 478 > +tag_op_list_t\r
 479 > +*tag_op_list_create (void *ctx);\r
 480 > +\r
 481 > +int\r
 482 > +tag_op_list_append (void *ctx,\r
 483 > +                 tag_op_list_t *list,\r
 484 > +                 const char *tag,\r
 485 > +                 notmuch_bool_t remove);\r
 486 > +\r
 487 > +notmuch_status_t\r
 488 > +tag_op_list_apply (notmuch_message_t *message,\r
 489 > +                tag_op_list_t *tag_ops,\r
 490 > +                tag_op_flag_t flags);\r
 491 > +\r
 492 > +int\r
 493 > +tag_op_list_from_string (void *ctx,\r
 494 > +                      char *tag_string,\r
 495 > +                      notmuch_bool_t remove,\r
 496 > +                      tag_op_list_t *tag_ops);\r
 497 > +\r
 498 > +notmuch_bool_t\r
 499 > +tag_op_list_empty (tag_op_list_t *list);\r
 500 > +\r
 501 > +void\r
 502 > +tag_op_list_reset (tag_op_list_t *list);\r
 503 > +\r
 504 > +#endif\r
 505 > -- \r
 506 > 1.7.10.4\r
 507 >\r
 508 > _______________________________________________\r
 509 > notmuch mailing list\r
 510 > notmuch@notmuchmail.org\r
 511 > http://notmuchmail.org/mailman/listinfo/notmuch\r