Re: [PATCH] Reduce zlib deflate code duplication

[Jonathan Nieder <jrnieder@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.

> --- 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.

> --- 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);
[...]
> @@ -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(
[...]
> -	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(
[...]
> -			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.

> --- 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.

> -		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