Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> 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.
> @@ -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.
> @@ -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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html