aa/3187df9577f5cd97d3ed3740dbccae5b707356

   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 DC388431FAF\r
   6         for <notmuch@notmuchmail.org>; Sat, 22 Dec 2012 15:36:09 -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 L+IhLMBCrkVs for <notmuch@notmuchmail.org>;\r
  16         Sat, 22 Dec 2012 15:36:09 -0800 (PST)\r
  17 Received: from mail-la0-f50.google.com (mail-la0-f50.google.com\r
  18         [209.85.215.50]) (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 E4263431FAE\r
  21         for <notmuch@notmuchmail.org>; Sat, 22 Dec 2012 15:36:08 -0800 (PST)\r
  22 Received: by mail-la0-f50.google.com with SMTP id c1so7069663lah.9\r
  23         for <notmuch@notmuchmail.org>; Sat, 22 Dec 2012 15:36:07 -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=x-received:from:to:cc:subject:in-reply-to:references:user-agent\r
  27         :date:message-id:mime-version:content-type:x-gm-message-state;\r
  28         bh=p3CaFoivDEW/ydAv352pxY83u8ZH7iZxZ0e7nU9Vzh0=;\r
  29         b=QD7wzSqcoEdWmwQWqQFevU9TFOHB+8/L3BDac4cq7U74MzoVflq0VA6GNlUnvcxa+9\r
  30         y5ITiW9v5ECEZDUQUIdZCO/krER4FttBjF7ZoFXNcuZCLtaWvkbQsdX79NIJKff/DbSU\r
  31         Y9WiFac7mcuWoLMgFXZFAnKNM7g1oGdCwfQe0KibjpYPJ4kcO9Kf4vqoCf1/ekPebvzp\r
  32         1fa5hXzDnTMe2ESmDDS6oT18P9GPj8b7LdHMNmf6VYyyyRXELyiaLVfXZ1wpi/y8lf38\r
  33         rFoVZFLCRzj7Paz4I8CrbqUU4BhLRWGgJvAiYna1a50CdAlVRvzSdfkMdAqJxc9nHZ09\r
  34         g0fw==\r
  35 X-Received: by 10.152.125.240 with SMTP id mt16mr16389477lab.17.1356219367368;\r
  36         Sat, 22 Dec 2012 15:36:07 -0800 (PST)\r
  37 Received: from localhost (dsl-hkibrasgw4-50df51-27.dhcp.inet.fi.\r
  38         [80.223.81.27])\r
  39         by mx.google.com with ESMTPS id bf3sm5949712lbb.16.2012.12.22.15.36.04\r
  40         (version=SSLv3 cipher=OTHER); Sat, 22 Dec 2012 15:36:06 -0800 (PST)\r
  41 From: Jani Nikula <jani@nikula.org>\r
  42 To: david@tethera.net, notmuch@notmuchmail.org\r
  43 Subject: Re: [Patch v8 07/18] unhex_and_quote: new function to quote\r
  44         hex-decoded queries\r
  45 In-Reply-To: <1356095307-22895-7-git-send-email-david@tethera.net>\r
  46 References: <1356095307-22895-1-git-send-email-david@tethera.net>\r
  47         <1356095307-22895-7-git-send-email-david@tethera.net>\r
  48 User-Agent: Notmuch/0.14+211~gc8d6546 (http://notmuchmail.org) Emacs/24.2.1\r
  49         (x86_64-pc-linux-gnu)\r
  50 Date: Sun, 23 Dec 2012 01:36:03 +0200\r
  51 Message-ID: <87txrdhd7g.fsf@oiva.home.nikula.org>\r
  52 MIME-Version: 1.0\r
  53 Content-Type: text/plain\r
  54 X-Gm-Message-State:\r
  55  ALoCoQnV28RTEqNv3tbjSScEVBiQ/d9VUnE1WsVGa9fVhxFHWy3cJ7W2XboSdPMnsi3QTfAvDfT+\r
  56 Cc: David Bremner <bremner@debian.org>\r
  57 X-BeenThere: notmuch@notmuchmail.org\r
  58 X-Mailman-Version: 2.1.13\r
  59 Precedence: list\r
  60 List-Id: "Use and development of the notmuch mail system."\r
  61         <notmuch.notmuchmail.org>\r
  62 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  63         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  64 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  65 List-Post: <mailto:notmuch@notmuchmail.org>\r
  66 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  67 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  68         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  69 X-List-Received-Date: Sat, 22 Dec 2012 23:36:10 -0000\r
  70 \r
  71 On Fri, 21 Dec 2012, david@tethera.net wrote:\r
  72 > From: David Bremner <bremner@debian.org>\r
  73 >\r
  74 > Space delimited tokens are hex decoded and then quoted according to\r
  75 > Xapian rules. Prefixes and '*' are passed through unquoted, as is\r
  76 > anything that hex-decoding would not change.\r
  77 > ---\r
  78 >  tag-util.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\r
  79 >  1 file changed, 81 insertions(+)\r
  80 >\r
  81 > diff --git a/tag-util.c b/tag-util.c\r
  82 > index f89669a..46aab4e 100644\r
  83 > --- a/tag-util.c\r
  84 > +++ b/tag-util.c\r
  85 > @@ -56,6 +56,87 @@ illegal_tag (const char *tag, notmuch_bool_t remove)\r
  86 >      return NULL;\r
  87 >  }\r
  88 >  \r
  89 > +/* Input is a hex encoded string, presumed to be a query for Xapian.\r
  90 > + *\r
  91 > + * Space delimited tokens are decoded and quoted, with '*' and prefixes\r
  92 > + * of the form "foo:" passed through unquoted.\r
  93 > + */\r
  94 > +static tag_parse_status_t\r
  95 > +unhex_and_quote (void *ctx, char *encoded, const char *line_for_error,\r
  96 > +              char **query_string)\r
  97 > +{\r
  98 > +    char *tok = encoded;\r
  99 > +    size_t tok_len = 0;\r
 100 > +    char *buf = NULL;\r
 101 > +    size_t buf_len = 0;\r
 102 > +    tag_parse_status_t ret = TAG_PARSE_SUCCESS;\r
 103 > +\r
 104 > +    *query_string = talloc_strdup (ctx, "");\r
 105 > +\r
 106 > +    while ((tok = strtok_len (tok + tok_len, " ", &tok_len)) != NULL) {\r
 107 > +\r
 108 > +     size_t prefix_len;\r
 109 > +     char delim = *(tok + tok_len);\r
 110 > +\r
 111 > +     *(tok + tok_len++) = '\0';\r
 112 \r
 113 You need to do tok_len++ to satisfy the next round of strtok_len, but I\r
 114 think for clarity of the code below you should move tok_len++ to the end\r
 115 of the while block. And review tok_len usage below.\r
 116 \r
 117 > +\r
 118 > +     prefix_len = hex_invariant (tok, tok_len);\r
 119 \r
 120 This, along with the doc comment "...initial segment of str that would\r
 121 not be changed by hex encoding..." for hex_invariant, was the hardest\r
 122 bit to understand. I don't follow how hex *encoding* matters here; the\r
 123 input is only affected by hex *decoding*, and that depends on %NN only.\r
 124 \r
 125 Should this be a function that counts the length of the initial segment\r
 126 of str that consists of valid Xapian prefix characters and does not\r
 127 contain % (I don't know if that's included in Xapian prefix\r
 128 characters). In the end, the contents of the function may be (I don't\r
 129 know) exactly the same as hex_invariant, but the function name would be\r
 130 more self explanatory.\r
 131 \r
 132 Does that make any sense to you...?\r
 133 \r
 134 > +\r
 135 > +     if ((strcmp (tok, "*") == 0) || prefix_len >= tok_len - 1) {\r
 136 \r
 137 With the tok_len++ at the end, I think this should have "prefix_len ==\r
 138 tok_len" for clarity.\r
 139 \r
 140 > +\r
 141 > +         /* pass some things through without quoting or decoding.\r
 142 > +          * Note for '*' this is mandatory.\r
 143 > +          */\r
 144 > +\r
 145 > +         if (! (*query_string = talloc_asprintf_append_buffer (\r
 146 > +                    *query_string, "%s%c", tok, delim))) {\r
 147 > +\r
 148 > +             ret = line_error (TAG_PARSE_OUT_OF_MEMORY,\r
 149 > +                               line_for_error, "aborting");\r
 150 > +             goto DONE;\r
 151 > +         }\r
 152 > +\r
 153 > +     } else {\r
 154 > +         /* potential prefix: one for ':', then something after */\r
 155 > +         if ((tok_len - prefix_len > 2) && *(tok + prefix_len) == ':') {\r
 156 \r
 157 I don't think this takes into account the tok_len++. So this should stay\r
 158 as it is if you move tok_len++ to the end.\r
 159 \r
 160 > +             if (! (*query_string = talloc_strndup_append (*query_string,\r
 161 > +                                                           tok,\r
 162 > +                                                           prefix_len + 1))) {\r
 163 > +                 ret = line_error (TAG_PARSE_OUT_OF_MEMORY,\r
 164 > +                                   line_for_error, "aborting");\r
 165 > +                 goto DONE;\r
 166 > +             }\r
 167 > +             tok += prefix_len + 1;\r
 168 > +             tok_len -= prefix_len + 1;\r
 169 > +         }\r
 170 > +\r
 171 > +         if (hex_decode_inplace (tok) != HEX_SUCCESS) {\r
 172 > +             ret = line_error (TAG_PARSE_INVALID, line_for_error,\r
 173 > +                               "hex decoding of token '%s' failed", tok);\r
 174 > +             goto DONE;\r
 175 > +         }\r
 176 > +\r
 177 > +         if (double_quote_str (ctx, tok, &buf, &buf_len)) {\r
 178 > +             ret = line_error (TAG_PARSE_OUT_OF_MEMORY,\r
 179 > +                               line_for_error, "aborting");\r
 180 > +             goto DONE;\r
 181 > +         }\r
 182 > +\r
 183 > +         if (! (*query_string = talloc_asprintf_append_buffer (\r
 184 > +                    *query_string, "%s%c", buf, delim))) {\r
 185 > +             ret = line_error (TAG_PARSE_OUT_OF_MEMORY,\r
 186 > +                               line_for_error, "aborting");\r
 187 > +             goto DONE;\r
 188 > +         }\r
 189 > +     }\r
 190 \r
 191 I think tok_len++ should be here.\r
 192 \r
 193 BR,\r
 194 Jani.\r
 195 \r
 196 > +    }\r
 197 > +\r
 198 > +  DONE:\r
 199 > +    if (ret != TAG_PARSE_SUCCESS && *query_string)\r
 200 > +     talloc_free (*query_string);\r
 201 > +    return ret;\r
 202 > +}\r
 203 > +\r
 204 >  tag_parse_status_t\r
 205 >  parse_tag_line (void *ctx, char *line,\r
 206 >               tag_op_flag_t flags,\r
 207 > -- \r
 208 > 1.7.10.4\r
 209 >\r
 210 > _______________________________________________\r
 211 > notmuch mailing list\r
 212 > notmuch@notmuchmail.org\r
 213 > http://notmuchmail.org/mailman/listinfo/notmuch\r