From: Tomi Ollila <tomi.ollila@iki.fi>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Subject: Re: [PATCH] util/hex-escape.[ch]: encoding/decoding strings into
	restricted character set
In-Reply-To: <1323620384-16043-1-git-send-email-david@tethera.net>
References: <1323620384-16043-1-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.10.2+93~g631d290 (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Mon, 12 Dec 2011 13:29:45 +0200
Message-ID: <yf6ty56m3iu.fsf@taco2.nixu.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: David Bremner <bremner@debian.org>
Precedence: list

On Sun, 11 Dec 2011 12:19:44 -0400, David Bremner <david@tethera.net> wrote:
> From: David Bremner <bremner@debian.org>
> 
> The character set is chosen to be suitable for pathnames, and the same
> as that used by contrib/nmbug. The new encoded/decoded strings are
> allocated using talloc.
> ---
> This isn't urgent, but it is useful for a couple projects I have
> brewing (nmbug compatible dump/restore and tag logging), so I thought
> I would get some feedback on it.
> 
> 
>  util/Makefile.local |    4 +-
>  util/hex-escape.c   |  110 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  util/hex-escape.h   |   10 +++++
>  3 files changed, 122 insertions(+), 2 deletions(-)
>  create mode 100644 util/hex-escape.c
>  create mode 100644 util/hex-escape.h

Like Dmitry mentioned, Makefile.local change in separate patch after
hex-escape additions.

> diff --git a/util/hex-escape.c b/util/hex-escape.c
> new file mode 100644
> index 0000000..c294bb5
> --- /dev/null
> +++ b/util/hex-escape.c

[ ... snip ... ]
> +
> +static int
> +escapes_needed (const char *str){

Opening { in separate line, like in all other source files.

> +    int escapes = 0;
> +
> +    while (*str) {
> +	if (index (HEX_NO_ESCAPE, *str) == NULL)

strchr() instead of index()

And, like Dmitry mentioned, static const char _hex_no_escape[] = "...";

> +	    escapes++;
> +	str++;
> +    }
> +
> +   return escapes;
> +}
> +
> +char *
> +hex_encode (void *ctx, const char *str) {
> +    char *newstr = talloc_size (ctx, strlen (str)+3*escapes_needed (str)+1);

Consistent spacing, like Dmitry mentioned (I compared with
_optimize_tag_query () in notmuch-tag.c ).

> +
> +    char *out = newstr;
> +
> +    while (*str) {
> +	if (index (HEX_NO_ESCAPE, *str)) {

... if (strchr ( _hex_no_escape, *str) != NULL) { 

[ ... snip ... ]
> +
> +inline static int
> +_digit (char c) {

Maybe _hexdigit () ?

> +    if ('0' <= c && c <= '9')
> +	return c - '0';
> +
> +    if ('A' <= c && c <= 'F')
> +	return c - 'A';
> +
> +    if ('a' <= c && c <= 'f')
> +	return c - 'a';

Fix this (or change to sscanf) like Dmitry mentioned
(c - 'A' + 10 and c - 'a' + 10)

> +
> +    INTERNAL_ERROR ("Illegal hex digit %c", c);

Is this too heavy ? -- but there may not be alternative.

> +    /*NOTREACHED*/
> +    return 0;
> +}
[ ... snip ... ]

Tomi