Re: [PATCH 01/11] Introduce stream compress helpers

[Junio C Hamano <gitster@pobox.com>]

Marco Costalba <mcostalba@gmail.com> writes:

> When decompressing a zlib stream use this
> helpers instead of calling low level zlib
> function.
>
> This patch introduces the necessary framework,
> still no code change.
>
> This is the first step in generalizing compress and
> decompress functions avoiding zlib directly calls.

(Cosmetic)

Your log message lines are wrapped a bit too short, while some
of the patch lines are too long.

> diff --git a/compress.c b/compress.c
> new file mode 100644
> index 0000000..f6986c3
> --- /dev/null
> +++ b/compress.c
> @@ -0,0 +1,58 @@
> +#include "cache.h"
> +#include "compress.h"
> +
> +unsigned long compress_alloc(z_stream *stream, int level, unsigned long size)
> +{
> +	memset(stream, 0, sizeof(*stream));
> +	deflateInit(stream, level);
> +	return deflateBound(stream, size);
> +}

(Naming)

This is not about "allocation", but about "setup".

> +int compress_start(z_stream *stream,
> +                   unsigned char *in, unsigned long in_size,
> +                   unsigned char *out, unsigned long out_size)
> +{
> +	stream->next_out = (out ? out : xmalloc(out_size));
> +	stream->avail_out = out_size;
> +	stream->next_in = in;
> +	stream->avail_in = in_size;
> +	return Z_OK;
> +}

This returns Z_OK unconditionally and most callers do not even
bother checking the return value.  Shouldn't this be of type
void?

Especially the use of this in if() conditional, after [06/11]
changes its use in compress_all(), looks quite ugly.

> +unsigned long compress_free(z_stream *stream)
> +{
> +	deflateEnd(stream);
> +	return stream->total_out;
> +}

Eventually, this should check errors from deflateEnd() and
propagate that to the caller.