From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Subject: Re: [Alpha Patch 2/2] test: update dump-restore tests for 8 bit
	characters.
In-Reply-To: <1323893641-4677-2-git-send-email-david@tethera.net>
References: <1323808075-7417-7-git-send-email-david@tethera.net>
	<1323893641-4677-1-git-send-email-david@tethera.net>
	<1323893641-4677-2-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.10.2+96~g74e5ae5 (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Thu, 15 Dec 2011 00:36:38 +0400
Message-ID: <87pqfqubzd.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: David Bremner <bremner@debian.org>, pere@hungry.com
Precedence: list

On Wed, 14 Dec 2011 16:14:01 -0400, David Bremner <david@tethera.net> wrote:
> From: David Bremner <bremner@debian.org>
> 
> hex-escape: fix for handling of 8 bit chars
> 
> The low level problem was passing negative numbers to sprintf(s,"%x");
> we fix this and clarify the api for hex_(decode|encode) by making
> encode go from (unsigned char *) (i.e. 8bit) to (char *) and decode
> vise-versa.

I did not do a proper review.  But I think the encoder and decoder
should accept and return the same type, either char* or unsigned char*.
The decision should be based on what type strings (that would be fed to
the encoder and decoder) have in notmuch code.  I guess it is char*, so
the encoder and decoder should take and return char*.  Internally we
would cast char* to unsigned char*.

Also, I do not like the _octet typedef in hex-escape.c.  Having
different function parameters in header and .c is confusing.  IMO we
should either move the typedef to some header, or just use unsigned
char.

Regards,
  Dmitry

> ---
>  test/dump-restore |    2 --
>  test/hex-escaping |    1 -
>  util/hex-escape.c |   26 +++++++++++++++-----------
>  util/hex-escape.h |    6 ++++--
>  4 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/test/dump-restore b/test/dump-restore
> index eee1773..c5b2e86 100755
> --- a/test/dump-restore
> +++ b/test/dump-restore
> @@ -114,7 +114,6 @@ notmuch dump --format=notmuch > BACKUP
>  notmuch tag +"$tag1" +"$tag2" +"$tag3" -inbox -unread "*"
>  
>  test_begin_subtest 'format=notmuch, round trip with strange tags'
> -   test_subtest_known_broken
>     notmuch dump --format=notmuch > EXPECTED.$test_count
>     notmuch dump --format=notmuch | notmuch restore --format=notmuch
>     notmuch dump --format=notmuch > OUTPUT.$test_count
> @@ -122,7 +121,6 @@ test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
>  
>  
>  test_begin_subtest 'format=notmuch, checking encoded output'
> -    test_subtest_known_broken
>      cp /dev/null EXPECTED.$test_count
>      notmuch dump --format=notmuch -- from:cworth |\
>  	 awk "{ print \$1 \" $enc1 $enc2 $enc3\" }" > EXPECTED.$test_count
> diff --git a/test/hex-escaping b/test/hex-escaping
> index 2053fb0..daa6446 100755
> --- a/test/hex-escaping
> +++ b/test/hex-escaping
> @@ -19,7 +19,6 @@ $TEST_DIRECTORY/hex-xcode e  < EXPECTED.$test_count |\
>  test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
>  
>  test_begin_subtest "round trip 8bit chars"
> -test_subtest_known_broken
>  echo '%c3%91%c3%a5%c3%b0%c3%a3%c3%a5%c3%a9-%c3%8f%c3%8a' > EXPECTED.$test_count
>  $TEST_DIRECTORY/hex-xcode d  < EXPECTED.$test_count |\
>  	$TEST_DIRECTORY/hex-xcode e > OUTPUT.$test_count
> diff --git a/util/hex-escape.c b/util/hex-escape.c
> index dcf87cf..565ae99 100644
> --- a/util/hex-escape.c
> +++ b/util/hex-escape.c
> @@ -28,23 +28,24 @@ static const size_t default_buf_size=1024;
>  static const char* output_charset=
>      "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+-_@=.:,";
>  
> -static const char escape_char='%';
> +static const int escape_char = '%';
>  
>  static int
>  is_output (char c) {
>      return (strchr (output_charset, c) != NULL);
>  }
>  
> +typedef unsigned char _octet;
>  
>  static int
> -maybe_realloc(void *ctx, size_t needed, char **out, size_t *out_size)
> +maybe_realloc(void *ctx, size_t needed, _octet **out, size_t *out_size)
>  {
>      if (*out_size < needed) {
>  
>  	if (*out == NULL)
>  	    *out = talloc_size(ctx,needed);
>  	else
> -	    *out = talloc_realloc(ctx,*out,char,needed);
> +	    *out = talloc_realloc(ctx, *out, _octet, needed);
>  
>  	if (*out == NULL)
>  	    return 0;
> @@ -56,24 +57,27 @@ maybe_realloc(void *ctx, size_t needed, char **out, size_t *out_size)
>  
>  
>  hex_status_t
> -hex_encode (void *ctx, const char *in, char **out, size_t *out_size)
> +hex_encode (void *ctx, const _octet *in, char **out, size_t *out_size)
>  {
>  
> -    const char *p;
> +    const _octet *p;
>      char *q;
>  
> -    int escape_count=0;
> +    size_t escape_count = 0;
> +    size_t len = 0;
>      size_t needed;
>  
> -    for  (p = in; *p; p++)
> +    for  (p = in; *p; p++) {
>  	escape_count += (! is_output (*p));
> +	len++;
> +    }
>  
> -    needed = strlen (in) + 2*escape_count + 1;
> +    needed = len + 2*escape_count + 1;
>  
>      if (*out == NULL)
>  	*out_size=0;
>  
> -    if (!maybe_realloc (ctx, needed, out, out_size))
> +    if (!maybe_realloc (ctx, needed, (_octet**)out, out_size))
>  	return HEX_OUT_OF_MEMORY;
>  
>      q = *out;
> @@ -94,12 +98,12 @@ hex_encode (void *ctx, const char *in, char **out, size_t *out_size)
>  
>  
>  hex_status_t
> -hex_decode (void *ctx, const char *in, char **out, size_t *out_size) {
> +hex_decode (void *ctx, const char *in, _octet **out, size_t *out_size) {
>  
>      char buf[3];
>  
>      const char *p;
> -    char *q;
> +    _octet *q;
>  
>      size_t escape_count = 0;
>      size_t needed = 0;
> diff --git a/util/hex-escape.h b/util/hex-escape.h
> index 98ecbe0..e04aff5 100644
> --- a/util/hex-escape.h
> +++ b/util/hex-escape.h
> @@ -8,8 +8,10 @@ typedef enum hex_status {
>  } hex_status_t;
>  
>  hex_status_t
> -hex_encode (void *talloc_ctx, const char *in, char **out, size_t *out_size);
> +hex_encode (void *talloc_ctx, const unsigned char *in, char **out,
> +	    size_t *out_size);
>  
>  hex_status_t
> -hex_decode (void *talloc_ctx, const char *in, char **out, size_t *out_size);
> +hex_decode (void *talloc_ctx, const char *in, unsigned char **out,
> +	    size_t *out_size);
>  #endif
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch