{"thread":{"id":"24875","subject":"[PATCH] Reduce zlib deflate code duplication","startedAt":"2010-08-26T20:56:04Z","lastAt":"2010-08-31T17:46:59Z","messageCount":5,"participants":["Nguyễn Thái Ngọc Duy","Shawn Pearce","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"149090","messageId":"1282856164-5126-1-git-send-email-pclouds@gmail.com","threadId":"24875","inReplyTo":null,"subject":"[PATCH] Reduce zlib deflate code duplication","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-08-26T20:56:04Z","receivedAt":"2010-08-26T20:56:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Most of deflation code is simply \"given this buffer, just deflate\nit\". Make a common routine and reuse it instead. \n\nExceptions include index-pack, http-push, fast-import,\nwrite_sha1_file, which either make multiple zlib streams, or compress\nand calculate sha1/checksum in parallel.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n There is possibly a regression here. remote-curl.c/post_rpc()\n specifies more compression options, although I doubt it affects\n performance. It also reveals places where deflation is expected\n to be always successful.\n\n archive-zip.c          |   37 ++-----------------------------------\n builtin/pack-objects.c |   24 +++---------------------\n cache.h                |    1 +\n diff.c                 |   29 +++--------------------------\n fast-import.c          |   40 ++++++++++++----------------------------\n remote-curl.c          |   31 ++++---------------------------\n wrapper.c              |   30 ++++++++++++++++++++++++++++++\n 7 files changed, 55 insertions(+), 137 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex cf28504..64ec465 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -87,39 +87,6 @@ static void copy_le32(unsigned char *dest, unsigned int n)\n \tdest[3] = 0xff & (n >> 030);\n }\n \n-static void *zlib_deflate(void *data, unsigned long size,\n-\t\tint compression_level, unsigned long *compressed_size)\n-{\n-\tz_stream stream;\n-\tunsigned long maxsize;\n-\tvoid *buffer;\n-\tint result;\n-\n-\tmemset(&stream, 0, sizeof(stream));\n-\tdeflateInit(&stream, compression_level);\n-\tmaxsize = deflateBound(&stream, size);\n-\tbuffer = xmalloc(maxsize);\n-\n-\tstream.next_in = data;\n-\tstream.avail_in = size;\n-\tstream.next_out = buffer;\n-\tstream.avail_out = maxsize;\n-\n-\tdo {\n-\t\tresult = deflate(&stream, Z_FINISH);\n-\t} while (result == Z_OK);\n-\n-\tif (result != Z_STREAM_END) {\n-\t\tfree(buffer);\n-\t\treturn NULL;\n-\t}\n-\n-\tdeflateEnd(&stream);\n-\t*compressed_size = stream.total_out;\n-\n-\treturn buffer;\n-}\n-\n static int write_zip_entry(struct archiver_args *args,\n \t\tconst unsigned char *sha1, const char *path, size_t pathlen,\n \t\tunsigned int mode, void *buffer, unsigned long size)\n@@ -164,8 +131,8 @@ static int write_zip_entry(struct archiver_args *args,\n \t}\n \n \tif (method == 8) {\n-\t\tdeflated = zlib_deflate(buffer, size, args->compression_level,\n-\t\t\t\t&compressed_size);\n+\t\tcompressed_size = size;\n+\t\tdeflated = git_deflate(buffer, &compressed_size, args->compression_level);\n \t\tif (deflated && compressed_size - 6 < size) {\n \t\t\t/* ZLIB --> raw compressed data (see RFC 1950) */\n \t\t\t/* CMF and FLG ... */\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 0e81673..20e2b6e 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -130,28 +130,10 @@ static void *get_delta(struct object_entry *entry)\n \n static unsigned long do_compress(void **pptr, unsigned long size)\n {\n-\tz_stream stream;\n-\tvoid *in, *out;\n-\tunsigned long maxsize;\n-\n-\tmemset(&stream, 0, sizeof(stream));\n-\tdeflateInit(&stream, pack_compression_level);\n-\tmaxsize = deflateBound(&stream, size);\n-\n-\tin = *pptr;\n-\tout = xmalloc(maxsize);\n+\tvoid *out = git_deflate(*pptr, &size, pack_compression_level);\n+\tfree(*pptr);\n \t*pptr = out;\n-\n-\tstream.next_in = in;\n-\tstream.avail_in = size;\n-\tstream.next_out = out;\n-\tstream.avail_out = maxsize;\n-\twhile (deflate(&stream, Z_FINISH) == Z_OK)\n-\t\t; /* nothing */\n-\tdeflateEnd(&stream);\n-\n-\tfree(in);\n-\treturn stream.total_out;\n+\treturn size;\n }\n \n /*\ndiff --git a/cache.h b/cache.h\nindex eb77e1d..c8ab52d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -22,6 +22,7 @@\n void git_inflate_init(z_streamp strm);\n void git_inflate_end(z_streamp strm);\n int git_inflate(z_streamp strm, int flush);\n+void *git_deflate(void *buf, unsigned long *size, int compression_level);\n \n #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n #define DTYPE(de)\t((de)->d_type)\ndiff --git a/diff.c b/diff.c\nindex 6fb97d4..18f2f55 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1674,30 +1674,6 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)\n \t}\n }\n \n-static unsigned char *deflate_it(char *data,\n-\t\t\t\t unsigned long size,\n-\t\t\t\t unsigned long *result_size)\n-{\n-\tint bound;\n-\tunsigned char *deflated;\n-\tz_stream stream;\n-\n-\tmemset(&stream, 0, sizeof(stream));\n-\tdeflateInit(&stream, zlib_compression_level);\n-\tbound = deflateBound(&stream, size);\n-\tdeflated = xmalloc(bound);\n-\tstream.next_out = deflated;\n-\tstream.avail_out = bound;\n-\n-\tstream.next_in = (unsigned char *)data;\n-\tstream.avail_in = size;\n-\twhile (deflate(&stream, Z_FINISH) == Z_OK)\n-\t\t; /* nothing */\n-\tdeflateEnd(&stream);\n-\t*result_size = stream.total_out;\n-\treturn deflated;\n-}\n-\n static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two, char *prefix)\n {\n \tvoid *cp;\n@@ -1713,7 +1689,8 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two, char\n \t * whichever is smaller.\n \t */\n \tdelta = NULL;\n-\tdeflated = deflate_it(two->ptr, two->size, &deflate_size);\n+\tdeflate_size = two->size;\n+\tdeflated = git_deflate(two->ptr, &deflate_size, zlib_compression_level);\n \tif (one->size && two->size) {\n \t\tdelta = diff_delta(one->ptr, one->size,\n \t\t\t\t   two->ptr, two->size,\n@@ -1721,7 +1698,7 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two, char\n \t\tif (delta) {\n \t\t\tvoid *to_free = delta;\n \t\t\torig_size = delta_size;\n-\t\t\tdelta = deflate_it(delta, delta_size, &delta_size);\n+\t\t\tdelta = git_deflate(delta, &delta_size, zlib_compression_level);\n \t\t\tfree(to_free);\n \t\t}\n \t}\ndiff --git a/fast-import.c b/fast-import.c\nindex dd51ac4..28bac18 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -993,7 +993,7 @@ static int store_object(\n \tunsigned char sha1[20];\n \tunsigned long hdrlen, deltalen;\n \tgit_SHA_CTX c;\n-\tz_stream s;\n+\tunsigned long compressed_size;\n \n \thdrlen = sprintf((char *)hdr,\"%s %lu\", typename(type),\n \t\t(unsigned long)dat->len) + 1;\n@@ -1025,24 +1025,13 @@ static int store_object(\n \t} else\n \t\tdelta = NULL;\n \n-\tmemset(&s, 0, sizeof(s));\n-\tdeflateInit(&s, pack_compression_level);\n-\tif (delta) {\n-\t\ts.next_in = delta;\n-\t\ts.avail_in = deltalen;\n-\t} else {\n-\t\ts.next_in = (void *)dat->buf;\n-\t\ts.avail_in = dat->len;\n-\t}\n-\ts.avail_out = deflateBound(&s, s.avail_in);\n-\ts.next_out = out = xmalloc(s.avail_out);\n-\twhile (deflate(&s, Z_FINISH) == Z_OK)\n-\t\t/* nothing */;\n-\tdeflateEnd(&s);\n+\tcompressed_size = delta ? deltalen : dat->len;\n+\tout = git_deflate(delta ? delta : dat->buf, &compressed_size,\n+\t\t\t  pack_compression_level);\n \n \t/* Determine if we should auto-checkpoint. */\n-\tif ((max_packsize && (pack_size + 60 + s.total_out) > max_packsize)\n-\t\t|| (pack_size + 60 + s.total_out) < pack_size) {\n+\tif ((max_packsize && (pack_size + 60 + compressed_size) > max_packsize)\n+\t\t|| (pack_size + 60 + compressed_size) < pack_size) {\n \n \t\t/* This new object needs to *not* have the current pack_id. */\n \t\te->pack_id = pack_id + 1;\n@@ -1053,15 +1042,10 @@ static int store_object(\n \t\t\tfree(delta);\n \t\t\tdelta = NULL;\n \n-\t\t\tmemset(&s, 0, sizeof(s));\n-\t\t\tdeflateInit(&s, pack_compression_level);\n-\t\t\ts.next_in = (void *)dat->buf;\n-\t\t\ts.avail_in = dat->len;\n-\t\t\ts.avail_out = deflateBound(&s, s.avail_in);\n-\t\t\ts.next_out = out = xrealloc(out, s.avail_out);\n-\t\t\twhile (deflate(&s, Z_FINISH) == Z_OK)\n-\t\t\t\t/* nothing */;\n-\t\t\tdeflateEnd(&s);\n+\t\t\tfree(out);\n+\t\t\tcompressed_size = dat->len;\n+\t\t\tout = git_deflate(dat->buf, &compressed_size,\n+\t\t\t\t\t  pack_compression_level);\n \t\t}\n \t}\n \n@@ -1096,8 +1080,8 @@ static int store_object(\n \t\tpack_size += hdrlen;\n \t}\n \n-\tsha1write(pack_file, out, s.total_out);\n-\tpack_size += s.total_out;\n+\tsha1write(pack_file, out, compressed_size);\n+\tpack_size += compressed_size;\n \n \te->idx.crc32 = crc32_end(pack_file);\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex 04d4813..e0d07fc 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -420,33 +420,10 @@ static int post_rpc(struct rpc_state *rpc)\n \t\t * we can try to deflate it ourselves, this may save on.\n \t\t * the transfer time.\n \t\t */\n-\t\tsize_t size;\n-\t\tz_stream stream;\n-\t\tint ret;\n-\n-\t\tmemset(&stream, 0, sizeof(stream));\n-\t\tret = deflateInit2(&stream, Z_BEST_COMPRESSION,\n-\t\t\t\tZ_DEFLATED, (15 + 16),\n-\t\t\t\t8, Z_DEFAULT_STRATEGY);\n-\t\tif (ret != Z_OK)\n-\t\t\tdie(\"cannot deflate request; zlib init error %d\", ret);\n-\t\tsize = deflateBound(&stream, rpc->len);\n-\t\tgzip_body = xmalloc(size);\n-\n-\t\tstream.next_in = (unsigned char *)rpc->buf;\n-\t\tstream.avail_in = rpc->len;\n-\t\tstream.next_out = (unsigned char *)gzip_body;\n-\t\tstream.avail_out = size;\n-\n-\t\tret = deflate(&stream, Z_FINISH);\n-\t\tif (ret != Z_STREAM_END)\n-\t\t\tdie(\"cannot deflate request; zlib deflate error %d\", ret);\n-\n-\t\tret = deflateEnd(&stream);\n-\t\tif (ret != Z_OK)\n-\t\t\tdie(\"cannot deflate request; zlib end error %d\", ret);\n-\n-\t\tsize = stream.total_out;\n+\t\tunsigned long size = rpc->len;\n+\t\tgzip_body = git_deflate(rpc->buf, &size, Z_BEST_COMPRESSION);\n+\t\tif (!gzip_body)\n+\t\t\tdie(\"cannot deflate request; zlib deflate error\");\n \n \t\theaders = curl_slist_append(headers, \"Content-Encoding: gzip\");\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, gzip_body);\ndiff --git a/wrapper.c b/wrapper.c\nindex fd8ead3..fda7279 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -289,6 +289,36 @@ int git_inflate(z_streamp strm, int flush)\n \treturn ret;\n }\n \n+void *git_deflate(void *in, unsigned long *size, int compression_level)\n+{\n+\tz_stream stream;\n+\tvoid *out;\n+\tunsigned long maxsize;\n+\tint result;\n+\n+\tmemset(&stream, 0, sizeof(stream));\n+\tdeflateInit(&stream, compression_level);\n+\tmaxsize = deflateBound(&stream, *size);\n+\n+\tout = xmalloc(maxsize);\n+\tstream.next_in = in;\n+\tstream.avail_in = *size;\n+\tstream.next_out = out;\n+\tstream.avail_out = maxsize;\n+\twhile ((result = deflate(&stream, Z_FINISH)) == Z_OK)\n+\t\t; /* nothing */\n+\n+\tif (result != Z_STREAM_END) {\n+\t\t*size = 0;\n+\t\tfree(out);\n+\t\treturn NULL;\n+\t}\n+\n+\tdeflateEnd(&stream);\n+\t*size = stream.total_out;\n+\treturn out;\n+}\n+\n int odb_mkstemp(char *template, size_t limit, const char *pattern)\n {\n \tint fd;\n-- \n1.7.1.rc1.69.g24c2f7\n"},{"id":"149096","messageId":"AANLkTikxxxxZwty156XG13QsOoimzYzuxSuTcf5KonTE@mail.gmail.com","threadId":"24875","inReplyTo":"1282856164-5126-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Reduce zlib deflate code duplication","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-08-27T01:56:40Z","receivedAt":"2010-08-27T01:56:40Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"2010/8/26 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:\n> Most of deflation code is simply \"given this buffer, just deflate\n> it\". Make a common routine and reuse it instead.\n>\n> Exceptions include index-pack, http-push, fast-import,\n> write_sha1_file, which either make multiple zlib streams, or compress\n> and calculate sha1/checksum in parallel.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  There is possibly a regression here. remote-curl.c/post_rpc()\n>  specifies more compression options, although I doubt it affects\n>  performance. It also reveals places where deflation is expected\n>  to be always successful.\n\nIt is a regression.\n\nThe compression options in remote-curl.c are to setup a gzip style\nstream, because that is what the servers expect.  Your new common\nroutine is using the bare deflation stream, which isn't the same\nheader/footer formatting, and thus servers will choke with your change\nand smart HTTP payloads that are compressed by the client.\n\nSo yea, you can't get rid of all of those options for the remote-curl\ninit.  You'll need to still let remote-curl setup its options itself,\nand then pass you the zlib structure to deflate on.\n\n-- \nShawn.\n"},{"id":"149098","messageId":"20100827023754.GB23924@burratino","threadId":"24875","inReplyTo":"1282856164-5126-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Reduce zlib deflate code duplication","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-27T02:37:54Z","receivedAt":"2010-08-27T02:37:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n\n> Most of deflation code is simply \"given this buffer, just deflate\n> it\". Make a common routine and reuse it instead. \n\nI like this idea.  But:\n\n>  There is possibly a regression here.\n\nRight.\n\n> --- a/archive-zip.c\n> +++ b/archive-zip.c\n\nLooks good.\n\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -130,28 +130,10 @@ static void *get_delta(struct object_entry *entry)\n>  \n>  static unsigned long do_compress(void **pptr, unsigned long size)\n>  {\n> -\tz_stream stream;\n> -\tvoid *in, *out;\n> -\tunsigned long maxsize;\n> -\n> -\tmemset(&stream, 0, sizeof(stream));\n> -\tdeflateInit(&stream, pack_compression_level);\n> -\tmaxsize = deflateBound(&stream, size);\n> -\n> -\tin = *pptr;\n> -\tout = xmalloc(maxsize);\n> +\tvoid *out = git_deflate(*pptr, &size, pack_compression_level);\n> +\tfree(*pptr);\n>  \t*pptr = out;\n\nOn error, previously *pptr and size would reflect a truncated result,\nbut now *pptr is NULL and size is 0.  Both results are silly.\n\nIt would be nicer if the caller (or do_compress itself) could check\nfor errors and report them.\n\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1713,7 +1689,8 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two, char\n>  \t * whichever is smaller.\n>  \t */\n>  \tdelta = NULL;\n> -\tdeflated = deflate_it(two->ptr, two->size, &deflate_size);\n> +\tdeflate_size = two->size;\n> +\tdeflated = git_deflate(two->ptr, &deflate_size, zlib_compression_level);\n[...]\n> @@ -1721,7 +1698,7 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two, char\n>  \t\tif (delta) {\n>  \t\t\tvoid *to_free = delta;\n>  \t\t\torig_size = delta_size;\n> -\t\t\tdelta = deflate_it(delta, delta_size, &delta_size);\n> +\t\t\tdelta = git_deflate(delta, &delta_size, zlib_compression_level);\n[...]\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1025,24 +1025,13 @@ static int store_object(\n[...]\n> -\twhile (deflate(&s, Z_FINISH) == Z_OK)\n> -\t\t/* nothing */;\n> -\tdeflateEnd(&s);\n> +\tcompressed_size = delta ? deltalen : dat->len;\n> +\tout = git_deflate(delta ? delta : dat->buf, &compressed_size,\n> +\t\t\t  pack_compression_level);\n[...]\n> @@ -1053,15 +1042,10 @@ static int store_object(\n[...]\n> -\t\t\ts.next_out = out = xrealloc(out, s.avail_out);\n> -\t\t\twhile (deflate(&s, Z_FINISH) == Z_OK)\n> -\t\t\t\t/* nothing */;\n> -\t\t\tdeflateEnd(&s);\n> +\t\t\tfree(out);\n> +\t\t\tcompressed_size = dat->len;\n> +\t\t\tout = git_deflate(dat->buf, &compressed_size,\n> +\t\t\t\t\t  pack_compression_level);\n\nLikewise.\n\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -420,33 +420,10 @@ static int post_rpc(struct rpc_state *rpc)\n>  \t\t * we can try to deflate it ourselves, this may save on.\n>  \t\t * the transfer time.\n>  \t\t */\n> -\t\tsize_t size;\n> -\t\tz_stream stream;\n> -\t\tint ret;\n> -\n> -\t\tmemset(&stream, 0, sizeof(stream));\n> -\t\tret = deflateInit2(&stream, Z_BEST_COMPRESSION,\n> -\t\t\t\tZ_DEFLATED, (15 + 16),\n\nAs Shawn mentioned, this requests gzip encoding with the default\nwindow size.\n\n> -\t\tret = deflate(&stream, Z_FINISH);\n> -\t\tif (ret != Z_STREAM_END)\n> -\t\t\tdie(\"cannot deflate request; zlib deflate error %d\", ret);\n> -\n> -\t\tret = deflateEnd(&stream);\n> -\t\tif (ret != Z_OK)\n> -\t\t\tdie(\"cannot deflate request; zlib end error %d\", ret);\n> -\n> -\t\tsize = stream.total_out;\n> +\t\tunsigned long size = rpc->len;\n> +\t\tgzip_body = git_deflate(rpc->buf, &size, Z_BEST_COMPRESSION);\n> +\t\tif (!gzip_body)\n> +\t\t\tdie(\"cannot deflate request; zlib deflate error\");\n\nThe zlib error codes are very helpful for debugging, so I would be sad\nto see them go.\n\nThanks,\nJonathan\n"},{"id":"149262","messageId":"1283132815-3277-1-git-send-email-pclouds@gmail.com","threadId":"24875","inReplyTo":"1282856164-5126-1-git-send-email-pclouds@gmail.com","subject":"[PATCH] Reduce zlib deflate code duplication","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-08-30T01:46:55Z","receivedAt":"2010-08-30T01:46:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This patch make a common deflate routine and reuse that routine as\nmuch as possible.\n\nMost of deflation code is simply \"given this buffer, just deflate\nit\". Exceptions include index-pack, http-push, fast-import,\nwrite_sha1_file and remote-curl, which either make multiple zlib\nstreams, or compress and calculate sha1/checksum in parallel, or\ncreate gzip stream.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n remote-curl is now excluded from the patch.\n \n git_deflate() returns zlib error code and will be used to print\n something out. I'm not sure if it should die() inside diff.c.\n It error() onlys for now.\n\n archive-zip.c          |   37 ++-----------------------------------\n builtin/pack-objects.c |   27 ++++++---------------------\n cache.h                |    1 +\n diff.c                 |   41 +++++++++++++++--------------------------\n fast-import.c          |   45 +++++++++++++++++----------------------------\n wrapper.c              |   31 +++++++++++++++++++++++++++++++\n 6 files changed, 72 insertions(+), 110 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex cf28504..ed176ca 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -87,39 +87,6 @@ static void copy_le32(unsigned char *dest, unsigned int n)\n \tdest[3] = 0xff & (n >> 030);\n }\n \n-static void *zlib_deflate(void *data, unsigned long size,\n-\t\tint compression_level, unsigned long *compressed_size)\n-{\n-\tz_stream stream;\n-\tunsigned long maxsize;\n-\tvoid *buffer;\n-\tint result;\n-\n-\tmemset(&stream, 0, sizeof(stream));\n-\tdeflateInit(&stream, compression_level);\n-\tmaxsize = deflateBound(&stream, size);\n-\tbuffer = xmalloc(maxsize);\n-\n-\tstream.next_in = data;\n-\tstream.avail_in = size;\n-\tstream.next_out = buffer;\n-\tstream.avail_out = maxsize;\n-\n-\tdo {\n-\t\tresult = deflate(&stream, Z_FINISH);\n-\t} while (result == Z_OK);\n-\n-\tif (result != Z_STREAM_END) {\n-\t\tfree(buffer);\n-\t\treturn NULL;\n-\t}\n-\n-\tdeflateEnd(&stream);\n-\t*compressed_size = stream.total_out;\n-\n-\treturn buffer;\n-}\n-\n static int write_zip_entry(struct archiver_args *args,\n \t\tconst unsigned char *sha1, const char *path, size_t pathlen,\n \t\tunsigned int mode, void *buffer, unsigned long size)\n@@ -164,8 +131,8 @@ static int write_zip_entry(struct archiver_args *args,\n \t}\n \n \tif (method == 8) {\n-\t\tdeflated = zlib_deflate(buffer, size, args->compression_level,\n-\t\t\t\t&compressed_size);\n+\t\tcompressed_size = size;\n+\t\tdeflated = git_deflate(buffer, &compressed_size, args->compression_level, NULL);\n \t\tif (deflated && compressed_size - 6 < size) {\n \t\t\t/* ZLIB --> raw compressed data (see RFC 1950) */\n \t\t\t/* CMF and FLG ... */\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 0e81673..9e78e88 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -130,28 +130,13 @@ static void *get_delta(struct object_entry *entry)\n \n static unsigned long do_compress(void **pptr, unsigned long size)\n {\n-\tz_stream stream;\n-\tvoid *in, *out;\n-\tunsigned long maxsize;\n-\n-\tmemset(&stream, 0, sizeof(stream));\n-\tdeflateInit(&stream, pack_compression_level);\n-\tmaxsize = deflateBound(&stream, size);\n-\n-\tin = *pptr;\n-\tout = xmalloc(maxsize);\n+\tint zlib_error;\n+\tvoid *out = git_deflate(*pptr, &size, pack_compression_level, &zlib_error);\n+\tif (!out)\n+\t\tdie(\"failed to compress, zlib error %d\", zlib_error);\n+\tfree(*pptr);\n \t*pptr = out;\n-\n-\tstream.next_in = in;\n-\tstream.avail_in = size;\n-\tstream.next_out = out;\n-\tstream.avail_out = maxsize;\n-\twhile (deflate(&stream, Z_FINISH) == Z_OK)\n-\t\t; /* nothing */\n-\tdeflateEnd(&stream);\n-\n-\tfree(in);\n-\treturn stream.total_out;\n+\treturn size;\n }\n \n /*\ndiff --git a/cache.h b/cache.h\nindex eb77e1d..afca130 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -22,6 +22,7 @@\n void git_inflate_init(z_streamp strm);\n void git_inflate_end(z_streamp strm);\n int git_inflate(z_streamp strm, int flush);\n+void *git_deflate(void *buf, unsigned long *size, int compression_level, int *error);\n \n #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n #define DTYPE(de)\t((de)->d_type)\ndiff --git a/diff.c b/diff.c\nindex 6fb97d4..56ef7a0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1674,30 +1674,6 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)\n \t}\n }\n \n-static unsigned char *deflate_it(char *data,\n-\t\t\t\t unsigned long size,\n-\t\t\t\t unsigned long *result_size)\n-{\n-\tint bound;\n-\tunsigned char *deflated;\n-\tz_stream stream;\n-\n-\tmemset(&stream, 0, sizeof(stream));\n-\tdeflateInit(&stream, zlib_compression_level);\n-\tbound = deflateBound(&stream, size);\n-\tdeflated = xmalloc(bound);\n-\tstream.next_out = deflated;\n-\tstream.avail_out = bound;\n-\n-\tstream.next_in = (unsigned char *)data;\n-\tstream.avail_in = size;\n-\twhile (deflate(&stream, Z_FINISH) == Z_OK)\n-\t\t; /* nothing */\n-\tdeflateEnd(&stream);\n-\t*result_size = stream.total_out;\n-\treturn deflated;\n-}\n-\n static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two, char *prefix)\n {\n \tvoid *cp;\n@@ -1708,12 +1684,19 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two, char\n \tunsigned long delta_size;\n \tunsigned long deflate_size;\n \tunsigned long data_size;\n+\tint zlib_error;\n \n \t/* We could do deflated delta, or we could do just deflated two,\n \t * whichever is smaller.\n \t */\n \tdelta = NULL;\n-\tdeflated = deflate_it(two->ptr, two->size, &deflate_size);\n+\tdeflate_size = two->size;\n+\tdeflated = git_deflate(two->ptr, &deflate_size, zlib_compression_level, &zlib_error);\n+\tif (!deflated) {\n+\t\terror(\"failed to compress for binary diff prefix %s, zlib error %d\",\n+\t\t      prefix, zlib_error);\n+\t\treturn;\n+\t}\n \tif (one->size && two->size) {\n \t\tdelta = diff_delta(one->ptr, one->size,\n \t\t\t\t   two->ptr, two->size,\n@@ -1721,8 +1704,14 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two, char\n \t\tif (delta) {\n \t\t\tvoid *to_free = delta;\n \t\t\torig_size = delta_size;\n-\t\t\tdelta = deflate_it(delta, delta_size, &delta_size);\n+\t\t\tdelta = git_deflate(delta, &delta_size, zlib_compression_level, NULL);\n \t\t\tfree(to_free);\n+\t\t\tif (!delta) {\n+\t\t\t\tfree(deflated);\n+\t\t\t\terror(\"failed to compress for delta prefix %s, zlib error %d\",\n+\t\t\t\t      prefix, zlib_error);\n+\t\t\t\treturn;\n+\t\t\t}\n \t\t}\n \t}\n \ndiff --git a/fast-import.c b/fast-import.c\nindex dd51ac4..cbdab25 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -993,7 +993,8 @@ static int store_object(\n \tunsigned char sha1[20];\n \tunsigned long hdrlen, deltalen;\n \tgit_SHA_CTX c;\n-\tz_stream s;\n+\tunsigned long compressed_size;\n+\tint zlib_error;\n \n \thdrlen = sprintf((char *)hdr,\"%s %lu\", typename(type),\n \t\t(unsigned long)dat->len) + 1;\n@@ -1025,24 +1026,15 @@ static int store_object(\n \t} else\n \t\tdelta = NULL;\n \n-\tmemset(&s, 0, sizeof(s));\n-\tdeflateInit(&s, pack_compression_level);\n-\tif (delta) {\n-\t\ts.next_in = delta;\n-\t\ts.avail_in = deltalen;\n-\t} else {\n-\t\ts.next_in = (void *)dat->buf;\n-\t\ts.avail_in = dat->len;\n-\t}\n-\ts.avail_out = deflateBound(&s, s.avail_in);\n-\ts.next_out = out = xmalloc(s.avail_out);\n-\twhile (deflate(&s, Z_FINISH) == Z_OK)\n-\t\t/* nothing */;\n-\tdeflateEnd(&s);\n+\tcompressed_size = delta ? deltalen : dat->len;\n+\tout = git_deflate(delta ? delta : dat->buf, &compressed_size,\n+\t\t\t  pack_compression_level, &zlib_error);\n+\tif (!out)\n+\t\tdie(\"failed to compress, zlib error %d\", zlib_error);\n \n \t/* Determine if we should auto-checkpoint. */\n-\tif ((max_packsize && (pack_size + 60 + s.total_out) > max_packsize)\n-\t\t|| (pack_size + 60 + s.total_out) < pack_size) {\n+\tif ((max_packsize && (pack_size + 60 + compressed_size) > max_packsize)\n+\t\t|| (pack_size + 60 + compressed_size) < pack_size) {\n \n \t\t/* This new object needs to *not* have the current pack_id. */\n \t\te->pack_id = pack_id + 1;\n@@ -1053,15 +1045,12 @@ static int store_object(\n \t\t\tfree(delta);\n \t\t\tdelta = NULL;\n \n-\t\t\tmemset(&s, 0, sizeof(s));\n-\t\t\tdeflateInit(&s, pack_compression_level);\n-\t\t\ts.next_in = (void *)dat->buf;\n-\t\t\ts.avail_in = dat->len;\n-\t\t\ts.avail_out = deflateBound(&s, s.avail_in);\n-\t\t\ts.next_out = out = xrealloc(out, s.avail_out);\n-\t\t\twhile (deflate(&s, Z_FINISH) == Z_OK)\n-\t\t\t\t/* nothing */;\n-\t\t\tdeflateEnd(&s);\n+\t\t\tfree(out);\n+\t\t\tcompressed_size = dat->len;\n+\t\t\tout = git_deflate(dat->buf, &compressed_size,\n+\t\t\t\t\t  pack_compression_level, &zlib_error);\n+\t\t\tif (!out)\n+\t\t\t\tdie(\"failed to compress, zlib error %d\", zlib_error);\n \t\t}\n \t}\n \n@@ -1096,8 +1085,8 @@ static int store_object(\n \t\tpack_size += hdrlen;\n \t}\n \n-\tsha1write(pack_file, out, s.total_out);\n-\tpack_size += s.total_out;\n+\tsha1write(pack_file, out, compressed_size);\n+\tpack_size += compressed_size;\n \n \te->idx.crc32 = crc32_end(pack_file);\n \ndiff --git a/wrapper.c b/wrapper.c\nindex fd8ead3..d0d3de9 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -289,6 +289,37 @@ int git_inflate(z_streamp strm, int flush)\n \treturn ret;\n }\n \n+void *git_deflate(void *in, unsigned long *size, int compression_level, int *error)\n+{\n+\tz_stream stream;\n+\tvoid *out;\n+\tunsigned long maxsize;\n+\tint result;\n+\n+\tmemset(&stream, 0, sizeof(stream));\n+\tdeflateInit(&stream, compression_level);\n+\tmaxsize = deflateBound(&stream, *size);\n+\n+\tout = xmalloc(maxsize);\n+\tstream.next_in = in;\n+\tstream.avail_in = *size;\n+\tstream.next_out = out;\n+\tstream.avail_out = maxsize;\n+\twhile ((result = deflate(&stream, Z_FINISH)) == Z_OK)\n+\t\t; /* nothing */\n+\n+\tif (result != Z_STREAM_END ||\n+\t    (result = deflateEnd(&stream)) != Z_OK) {\n+\t\tif (error)\n+\t\t\t*error = result;\n+\t\t*size = 0;\n+\t\tfree(out);\n+\t\treturn NULL;\n+\t}\n+\t*size = stream.total_out;\n+\treturn out;\n+}\n+\n int odb_mkstemp(char *template, size_t limit, const char *pattern)\n {\n \tint fd;\n-- \n1.7.1.rc1.69.g24c2f7\n"},{"id":"149462","messageId":"7vpqwybros.fsf@alter.siamese.dyndns.org","threadId":"24875","inReplyTo":"1283132815-3277-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Reduce zlib deflate code duplication","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-31T17:46:59Z","receivedAt":"2010-08-31T17:46:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n> diff --git a/archive-zip.c b/archive-zip.c\n> index cf28504..ed176ca 100644\n> --- a/archive-zip.c\n> +++ b/archive-zip.c\n> @@ -87,39 +87,6 @@ static void copy_le32(unsigned char *dest, unsigned int n)\n>  \tdest[3] = 0xff & (n >> 030);\n>  }\n>  \n> -static void *zlib_deflate(void *data, unsigned long size,\n> -\t\tint compression_level, unsigned long *compressed_size)\n> -{\n\nThis name makes more sense than git_deflate() as it is about \"deflating by\ncalling zlib\".  For a common library routine \"frotz\", git_frotz tends to\nbe used to name our own replacement for it, but with this patch we are not\ncreating our own implementation of deflate algorithm.\n\n> - ...\n> -\tdeflateEnd(&stream);\n\nWe used not to check the return value of this call here; the new function\nin wrapper.c insists that this returns Z_OK.  I think it is an improvement\nthat deserves to be mentioned in the proposed log message.\n\n> @@ -164,8 +131,8 @@ static int write_zip_entry(struct archiver_args *args,\n>  \t}\n>  \n>  \tif (method == 8) {\n> -\t\tdeflated = zlib_deflate(buffer, size, args->compression_level,\n> -\t\t\t\t&compressed_size);\n> +\t\tcompressed_size = size;\n\nIsn't it awkward that you have to assign the size of the uncompressed\ninput to a variable you are planning to use to hold the compressed size\nwith your API?  I do not see a compelling reason that we would want to use\nan in-out parameter here.\n\n> @@ -1708,12 +1684,19 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two, char\n>  \tunsigned long delta_size;\n>  \tunsigned long deflate_size;\n>  \tunsigned long data_size;\n> +\tint zlib_error;\n>  \n>  \t/* We could do deflated delta, or we could do just deflated two,\n>  \t * whichever is smaller.\n>  \t */\n>  \tdelta = NULL;\n> -\tdeflated = deflate_it(two->ptr, two->size, &deflate_size);\n> +\tdeflate_size = two->size;\n> +\tdeflated = git_deflate(two->ptr, &deflate_size, zlib_compression_level, &zlib_error);\n> +\tif (!deflated) {\n> +\t\terror(\"failed to compress for binary diff prefix %s, zlib error %d\",\n> +\t\t      prefix, zlib_error);\n\nWhat is this \"prefix\" about?\n\nI think the callchain here is that builtin_diff() sets its local variable\nline_prefix according to the o->output_prefix (used by graphing cruft) and\nit is passed to emit_binary_diff().  It certainly shouldn't be in the\nmiddle of any line, and I don't think you want it in the error message in\nthe first place.  Likewise for another error message nearby.\n\nThanks.\n"}]}