Re: [PATCH] Reduce zlib deflate code duplication
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Aug 27, 2010, 02:37 UTC
- Message-ID
- <20100827023754.GB23924@burratino>
- In-Reply-To
- <1282856164-5126-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy wrote:
> Most of deflation code is simply "given this buffer, just deflate > it". Make a common routine and reuse it instead.
I like this idea. But:
> There is possibly a regression here.
Right.
> --- a/archive-zip.c > +++ b/archive-zip.c
Looks good.
Show 19 quoted lines
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -130,28 +130,10 @@ static void *get_delta(struct object_entry *entry)
>
> static unsigned long do_compress(void **pptr, unsigned long size)
> {
> - z_stream stream;
> - void *in, *out;
> - unsigned long maxsize;
> -
> - memset(&stream, 0, sizeof(stream));
> - deflateInit(&stream, pack_compression_level);
> - maxsize = deflateBound(&stream, size);
> -
> - in = *pptr;
> - out = xmalloc(maxsize);
> + void *out = git_deflate(*pptr, &size, pack_compression_level);
> + free(*pptr);
> *pptr = out;On error, previously *pptr and size would reflect a truncated result, but now *pptr is NULL and size is 0. Both results are silly.
It would be nicer if the caller (or do_compress itself) could check for errors and report them.
Show 9 quoted lines
> --- a/diff.c > +++ b/diff.c > @@ -1713,7 +1689,8 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two, char > * whichever is smaller. > */ > delta = NULL; > - deflated = deflate_it(two->ptr, two->size, &deflate_size); > + deflate_size = two->size; > + deflated = git_deflate(two->ptr, &deflate_size, zlib_compression_level);
[...]
Show 6 quoted lines
> @@ -1721,7 +1698,7 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two, char
> if (delta) {
> void *to_free = delta;
> orig_size = delta_size;
> - delta = deflate_it(delta, delta_size, &delta_size);
> + delta = git_deflate(delta, &delta_size, zlib_compression_level);[...]
> --- a/fast-import.c > +++ b/fast-import.c > @@ -1025,24 +1025,13 @@ static int store_object(
[...]
Show 6 quoted lines
> - while (deflate(&s, Z_FINISH) == Z_OK) > - /* nothing */; > - deflateEnd(&s); > + compressed_size = delta ? deltalen : dat->len; > + out = git_deflate(delta ? delta : dat->buf, &compressed_size, > + pack_compression_level);
[...]
> @@ -1053,15 +1042,10 @@ static int store_object(
[...]
Show 8 quoted lines
> - s.next_out = out = xrealloc(out, s.avail_out); > - while (deflate(&s, Z_FINISH) == Z_OK) > - /* nothing */; > - deflateEnd(&s); > + free(out); > + compressed_size = dat->len; > + out = git_deflate(dat->buf, &compressed_size, > + pack_compression_level);
Likewise.
Show 13 quoted lines
> --- a/remote-curl.c > +++ b/remote-curl.c > @@ -420,33 +420,10 @@ static int post_rpc(struct rpc_state *rpc) > * we can try to deflate it ourselves, this may save on. > * the transfer time. > */ > - size_t size; > - z_stream stream; > - int ret; > - > - memset(&stream, 0, sizeof(stream)); > - ret = deflateInit2(&stream, Z_BEST_COMPRESSION, > - Z_DEFLATED, (15 + 16),
As Shawn mentioned, this requests gzip encoding with the default window size.
Show 13 quoted lines
> - ret = deflate(&stream, Z_FINISH);
> - if (ret != Z_STREAM_END)
> - die("cannot deflate request; zlib deflate error %d", ret);
> -
> - ret = deflateEnd(&stream);
> - if (ret != Z_OK)
> - die("cannot deflate request; zlib end error %d", ret);
> -
> - size = stream.total_out;
> + unsigned long size = rpc->len;
> + gzip_body = git_deflate(rpc->buf, &size, Z_BEST_COMPRESSION);
> + if (!gzip_body)
> + die("cannot deflate request; zlib deflate error");The zlib error codes are very helpful for debugging, so I would be sad to see them go.
Thanks, Jonathan