9b/2d3d39be49638751ec9eca49d80a82b8829e5c

   1 Return-Path: <dmitry.kurochkin@gmail.com>\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 8EE17429E25\r
   6         for <notmuch@notmuchmail.org>; Sun, 11 Dec 2011 09:57:15 -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.798\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.798 tagged_above=-999 required=5\r
  12         tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
  13         FREEMAIL_FROM=0.001, NORMAL_HTTP_TO_IP=0.001, RCVD_IN_DNSWL_LOW=-0.7]\r
  14         autolearn=disabled\r
  15 Received: from olra.theworths.org ([127.0.0.1])\r
  16         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  17         with ESMTP id YP7IQp4Uatn6 for <notmuch@notmuchmail.org>;\r
  18         Sun, 11 Dec 2011 09:57:13 -0800 (PST)\r
  19 Received: from mail-bw0-f53.google.com (mail-bw0-f53.google.com\r
  20         [209.85.214.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  21         (No client certificate requested)\r
  22         by olra.theworths.org (Postfix) with ESMTPS id 32A9A431FB6\r
  23         for <notmuch@notmuchmail.org>; Sun, 11 Dec 2011 09:57:13 -0800 (PST)\r
  24 Received: by bkat8 with SMTP id t8so4950674bka.26\r
  25         for <notmuch@notmuchmail.org>; Sun, 11 Dec 2011 09:57:11 -0800 (PST)\r
  26 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\r
  27         h=from:to:cc:subject:in-reply-to:references:user-agent:date\r
  28         :message-id:mime-version:content-type;\r
  29         bh=0Q991LE8Zo1JtOERL1PRro9ClRC4brapR+tjW8kzLMM=;\r
  30         b=Bob/aiwGH46v1k4oxJU1FQuxcGImdT6D6vnYo6CiIUPwMjKzEuVZudq8VyPLS8I50M\r
  31         MzCDRt+dTAfEDOqCTHpa1AcHqvoTi8neaS7D2F2761JH8WIJH4SO92i/pFXFGFTFnINq\r
  32         g5/zXBTq1wPdeHlQKn8fQtEOAEbB0EnUvUnxY=\r
  33 Received: by 10.204.148.67 with SMTP id o3mr8466298bkv.130.1323626231818;\r
  34         Sun, 11 Dec 2011 09:57:11 -0800 (PST)\r
  35 Received: from localhost ([91.144.186.21])\r
  36         by mx.google.com with ESMTPS id cc2sm25390088bkb.8.2011.12.11.09.57.10\r
  37         (version=TLSv1/SSLv3 cipher=OTHER);\r
  38         Sun, 11 Dec 2011 09:57:11 -0800 (PST)\r
  39 From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
  40 To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org\r
  41 Subject: Re: [PATCH] util/hex-escape.[ch]: encoding/decoding strings into\r
  42         restricted character set\r
  43 In-Reply-To: <1323620384-16043-1-git-send-email-david@tethera.net>\r
  44 References: <1323620384-16043-1-git-send-email-david@tethera.net>\r
  45 User-Agent: Notmuch/0.10.2+82~g96a629c (http://notmuchmail.org) Emacs/23.3.1\r
  46         (x86_64-pc-linux-gnu)\r
  47 Date: Sun, 11 Dec 2011 21:56:36 +0400\r
  48 Message-ID: <87zkez56wb.fsf@gmail.com>\r
  49 MIME-Version: 1.0\r
  50 Content-Type: text/plain; charset=us-ascii\r
  51 Cc: David Bremner <bremner@debian.org>\r
  52 X-BeenThere: notmuch@notmuchmail.org\r
  53 X-Mailman-Version: 2.1.13\r
  54 Precedence: list\r
  55 List-Id: "Use and development of the notmuch mail system."\r
  56         <notmuch.notmuchmail.org>\r
  57 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  58         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  59 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  60 List-Post: <mailto:notmuch@notmuchmail.org>\r
  61 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  62 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  63         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  64 X-List-Received-Date: Sun, 11 Dec 2011 17:57:15 -0000\r
  65 \r
  66 On Sun, 11 Dec 2011 12:19:44 -0400, David Bremner <david@tethera.net> wrote:\r
  67 > From: David Bremner <bremner@debian.org>\r
  68 > \r
  69 > The character set is chosen to be suitable for pathnames, and the same\r
  70 > as that used by contrib/nmbug. The new encoded/decoded strings are\r
  71 > allocated using talloc.\r
  72 > ---\r
  73 > This isn't urgent, but it is useful for a couple projects I have\r
  74 > brewing (nmbug compatible dump/restore and tag logging), so I thought\r
  75 > I would get some feedback on it.\r
  76 > \r
  77 \r
  78 I have some free time to spend on notmuch reviews today.  So here it is\r
  79 comes :)\r
  80 \r
  81 > \r
  82 >  util/Makefile.local |    4 +-\r
  83 >  util/hex-escape.c   |  110 +++++++++++++++++++++++++++++++++++++++++++++++++++\r
  84 >  util/hex-escape.h   |   10 +++++\r
  85 >  3 files changed, 122 insertions(+), 2 deletions(-)\r
  86 >  create mode 100644 util/hex-escape.c\r
  87 >  create mode 100644 util/hex-escape.h\r
  88 > \r
  89 > diff --git a/util/Makefile.local b/util/Makefile.local\r
  90 > index 0340899..2e63932 100644\r
  91 > --- a/util/Makefile.local\r
  92 > +++ b/util/Makefile.local\r
  93 > @@ -3,11 +3,11 @@\r
  94 >  dir := util\r
  95 >  extra_cflags += -I$(srcdir)/$(dir)\r
  96 >  \r
  97 > -libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c\r
  98 > +libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c\r
  99 >  \r
 100 >  libutil_modules := $(libutil_c_srcs:.c=.o)\r
 101 >  \r
 102 >  $(dir)/libutil.a: $(libutil_modules)\r
 103 >       $(call quiet,AR) rcs $@ $^\r
 104 >  \r
 105 > -CLEAN := $(CLEAN) $(dir)/xutil.o $(dir)/error_util.o $(dir)/libutil.a\r
 106 > +CLEAN := $(CLEAN) $(libutil_modules) $(dir)/libutil.a\r
 107 \r
 108 IMO this should be pushed as a separate patch (that does not need a\r
 109 review :)).\r
 110 \r
 111 > diff --git a/util/hex-escape.c b/util/hex-escape.c\r
 112 > new file mode 100644\r
 113 > index 0000000..c294bb5\r
 114 > --- /dev/null\r
 115 > +++ b/util/hex-escape.c\r
 116 > @@ -0,0 +1,110 @@\r
 117 > +/* hex-escape.c -  Manage encoding and decoding of byte strings into\r
 118 > + *              a restricted character set.\r
 119 > + *\r
 120 > + * Copyright (c) 2011 David Bremner\r
 121 > + *\r
 122 > + * This program is free software: you can redistribute it and/or modify\r
 123 > + * it under the terms of the GNU General Public License as published by\r
 124 > + * the Free Software Foundation, either version 3 of the License, or\r
 125 > + * (at your option) any later version.\r
 126 > + *\r
 127 > + * This program is distributed in the hope that it will be useful,\r
 128 > + * but WITHOUT ANY WARRANTY; without even the implied warranty of\r
 129 > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the\r
 130 > + * GNU General Public License for more details.\r
 131 > + *\r
 132 > + * You should have received a copy of the GNU General Public License\r
 133 > + * along with this program.  If not, see http://www.gnu.org/licenses/ .\r
 134 > + *\r
 135 > + * Author: David Bremner <david@tethera.net>\r
 136 > + */\r
 137 > +\r
 138 > +#include <string.h>\r
 139 > +#include <talloc.h>\r
 140 > +#include "error_util.h"\r
 141 > +#include "hex-escape.h"\r
 142 > +\r
 143 > +static int\r
 144 > +escapes_needed (const char *str){\r
 145 \r
 146 The name suggests that the function returns a boolean (needed or not\r
 147 needed).  Consider renaming to escapes_count, count_escapes or similar.\r
 148 \r
 149 Also there is a space missing before the {.\r
 150 \r
 151 > +    int escapes = 0;\r
 152 > +\r
 153 > +    while (*str) {\r
 154 > +     if (index (HEX_NO_ESCAPE, *str) == NULL)\r
 155 \r
 156 Consider adding a function (bool escape_needed(const char c)) (similar\r
 157 to isalpha() and friends) which would call index() and use it here and\r
 158 in hex_encode.  (This comment would not be actual if you decide to\r
 159 introduce a general char counting function.)\r
 160 \r
 161 > +         escapes++;\r
 162 > +     str++;\r
 163 > +    }\r
 164 > +\r
 165 > +   return escapes;\r
 166 \r
 167 Can we replace this function with a two-line for loop similar to the one\r
 168 in hex_decode?\r
 169 \r
 170 I think we should either use inline loops for counting chars in both\r
 171 hex_encode and hex_decode, or introduce a more general function that\r
 172 counts the number of given characters and use it in both hex_encode and\r
 173 hex_decode.\r
 174 \r
 175 > +}\r
 176 > +\r
 177 > +char *\r
 178 > +hex_encode (void *ctx, const char *str) {\r
 179 > +    char *newstr = talloc_size (ctx, strlen (str)+3*escapes_needed (str)+1);\r
 180 \r
 181 Do we need only strlen(str) + 2*escaped_count + 1 here?\r
 182 \r
 183 IMO it is very weird that we have spaces after a function name, but do\r
 184 not have spaces around +...\r
 185 \r
 186 > +\r
 187 > +    char *out = newstr;\r
 188 > +\r
 189 \r
 190 Why do we need both out and newstr variables?\r
 191 \r
 192 > +    while (*str) {\r
 193 > +     if (index (HEX_NO_ESCAPE, *str)) {\r
 194 > +         *out++ = *str++;\r
 195 > +     } else {\r
 196 > +         sprintf (out, "%%%02x", *str);\r
 197 > +         str++;\r
 198 \r
 199 Use *str++ as sprintf() parameter instead of doing it on a separate\r
 200 line?\r
 201 \r
 202 > +         out += 3;\r
 203 > +     }\r
 204 > +    }\r
 205 > +    *out = 0;\r
 206 \r
 207 I would prefer '\0' here.\r
 208 \r
 209 > +    return newstr;\r
 210 > +}\r
 211 > +\r
 212 > +inline static int\r
 213 > +_digit (char c) {\r
 214 \r
 215 Perhaps rename to hex_digit?  Other static function names do not start\r
 216 with underscore.  Let's be consistent.\r
 217 \r
 218 > +    if ('0' <= c && c <= '9')\r
 219 > +     return c - '0';\r
 220 > +\r
 221 > +    if ('A' <= c && c <= 'F')\r
 222 > +     return c - 'A';\r
 223 > +\r
 224 > +    if ('a' <= c && c <= 'f')\r
 225 > +     return c - 'a';\r
 226 > +\r
 227 \r
 228 Does this really work as expected?  'B' - 'A' would be 1, while it seems\r
 229 that we expect 10.  Perhaps we should use sscanf(3) instead?  That may\r
 230 make the code simpler and allow to convert both chars at once.\r
 231 \r
 232 > +    INTERNAL_ERROR ("Illegal hex digit %c", c);\r
 233 > +    /*NOTREACHED*/\r
 234 > +    return 0;\r
 235 > +}\r
 236 > +\r
 237 > +char *hex_decode (void *ctx, const char *str) {\r
 238 > +\r
 239 > +    int len = strlen(str);\r
 240 > +\r
 241 > +    const char *p;\r
 242 > +    char *q;\r
 243 > +    char *newstr;\r
 244 \r
 245 If you decide to use "out" variable in hex_encode() instead of "newstr",\r
 246 please rename it here as well.\r
 247 \r
 248 > +    int escapes = 0;\r
 249 > +\r
 250 > +    for (p = str; *p; p++)\r
 251 > +     escapes += (*p == HEX_ESCAPE_CHAR);\r
 252 > +\r
 253 > +    newstr = talloc_size (ctx, len - escapes*2 + 1);\r
 254 > +\r
 255 > +    p = str;\r
 256 > +    q = newstr;\r
 257 > +\r
 258 > +    while (*p) {\r
 259 > +\r
 260 > +     if (*p == HEX_ESCAPE_CHAR) {\r
 261 > +\r
 262 > +         if (len < 3) INTERNAL_ERROR ("Syntax error decoding %s", str);\r
 263 > +\r
 264 > +         *q = _digit(p[1]) * 16;\r
 265 > +         *q += _digit(p[2]);\r
 266 > +\r
 267 > +         len -= 3;\r
 268 > +         p += 3;\r
 269 > +         q++;\r
 270 > +     } else {\r
 271 > +         *q++ = *p++;\r
 272 > +     }\r
 273 > +    }\r
 274 > +\r
 275 > +    return newstr;\r
 276 > +}\r
 277 > diff --git a/util/hex-escape.h b/util/hex-escape.h\r
 278 > new file mode 100644\r
 279 > index 0000000..7caff15\r
 280 > --- /dev/null\r
 281 > +++ b/util/hex-escape.h\r
 282 > @@ -0,0 +1,10 @@\r
 283 > +#ifndef _HEX_ESCAPE_H\r
 284 > +#define _HEX_ESCAPE_H\r
 285 > +\r
 286 > +#define HEX_ESCAPE_CHAR '%'\r
 287 > +#define HEX_NO_ESCAPE        "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" \\r
 288 > +                     "0123456789+-_@=.:,"\r
 289 > +\r
 290 \r
 291 Can we make these proper constants?\r
 292 \r
 293 Are these macros supposed to be used elsewhere?  If no, we should move\r
 294 them inside hex-escape.c and probably even make local to functions that\r
 295 need them.\r
 296 \r
 297 Regards,\r
 298   Dmitry\r
 299 \r
 300 > +char *hex_encode (void *talloc_ctx, const char *string);\r
 301 > +char *hex_decode (void *talloc_ctx, const char *hex);\r
 302 > +#endif\r
 303 > -- \r
 304 > 1.7.7.3\r
 305 > \r
 306 > _______________________________________________\r
 307 > notmuch mailing list\r
 308 > notmuch@notmuchmail.org\r
 309 > http://notmuchmail.org/mailman/listinfo/notmuch\r