Re: [PATCH] Reduce zlib deflate code duplication
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:49:25
Nguyễn Thái Ngọc Duy [off-list ref] writes:
quoted hunk
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.
quoted hunk
@@ -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.
quoted hunk
@@ -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.