Re: [PATCH] Reduce zlib deflate code duplication

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 31, 2010, 17:46 UTC
Message-ID: <7vpqwybros.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1283132815-3277-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

Show 11 quoted lines

> diff --git a/archive-zip.c b/archive-zip.c
> index cf28504..ed176ca 100644
> --- a/archive-zip.c
> +++ b/archive-zip.c
> @@ -87,39 +87,6 @@ static void copy_le32(unsigned char *dest, unsigned int n)
>  	dest[3] = 0xff & (n >> 030);
>  }
>  
> -static void *zlib_deflate(void *data, unsigned long size,
> -		int compression_level, unsigned long *compressed_size)
> -{

This name makes more sense than git_deflate() as it is about "deflating by calling zlib". For a common library routine "frotz", git_frotz tends to be used to name our own replacement for it, but with this patch we are not creating our own implementation of deflate algorithm.

> - ...
> -	deflateEnd(&stream);

We used not to check the return value of this call here; the new function in wrapper.c insists that this returns Z_OK. I think it is an improvement that deserves to be mentioned in the proposed log message.

Show 7 quoted lines

> @@ -164,8 +131,8 @@ static int write_zip_entry(struct archiver_args *args,
>  	}
>  
>  	if (method == 8) {
> -		deflated = zlib_deflate(buffer, size, args->compression_level,
> -				&compressed_size);
> +		compressed_size = size;

Isn't it awkward that you have to assign the size of the uncompressed input to a variable you are planning to use to hold the compressed size with your API? I do not see a compelling reason that we would want to use an in-out parameter here.

Show 16 quoted lines

> @@ -1708,12 +1684,19 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two, char
>  	unsigned long delta_size;
>  	unsigned long deflate_size;
>  	unsigned long data_size;
> +	int zlib_error;
>  
>  	/* We could do deflated delta, or we could do just deflated two,
>  	 * 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, &zlib_error);
> +	if (!deflated) {
> +		error("failed to compress for binary diff prefix %s, zlib error %d",
> +		      prefix, zlib_error);

What is this "prefix" about?

I think the callchain here is that builtin_diff() sets its local variable line_prefix according to the o->output_prefix (used by graphing cruft) and it is passed to emit_binary_diff(). It certainly shouldn't be in the middle of any line, and I don't think you want it in the error message in the first place. Likewise for another error message nearby.

Thanks.