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