threads / patch / 11839

patch, 11 partsIntroduce stream compress helpers

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

## tl;dr

21 messages between Feb 2, 2008 and Feb 4, 2008. Diffs are folded; open one to read it.

replies: 20people: 3as markdown or json

Marco Costalba· Feb 2, 2008, 11:35 UTC · lore

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.

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 Makefile   |    4 ++--
 compress.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 compress.h |   12 ++++++++++++
 3 files changed, 72 insertions(+), 2 deletions(-)
 create mode 100644 compress.c
 create mode 100644 compress.h
Show changes to 3 files +72 −2

Makefile, compress.c, compress.h

diff --git a/Makefile b/Makefile
index 91a460f..f70f995 100644
--- a/Makefile
+++ b/Makefile
@@ -292,7 +292,7 @@ LIB_FILE=libgit.a
 XDIFF_LIB=xdiff/lib.a
 
 LIB_H = \
-	archive.h blob.h cache.h cache-tree.h commit.h csum-file.h delta.h grep.h \
+	archive.h blob.h cache.h cache-tree.h commit.h compress.h csum-file.h delta.h grep.h \
 	diff.h object.h pack.h pkt-line.h quote.h refs.h list-objects.h sideband.h \
 	run-command.h strbuf.h tag.h tree.h git-compat-util.h revision.h \
 	tree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h \
@@ -305,7 +305,7 @@ DIFF_OBJS = \
 	diffcore-delta.o log-tree.o
 
 LIB_OBJS = \
-	blob.o commit.o connect.o csum-file.o cache-tree.o base85.o \
+	blob.o commit.o compress.o connect.o csum-file.o cache-tree.o base85.o \
 	date.o diff-delta.o entry.o exec_cmd.o ident.o \
 	pretty.o interpolate.o hash.o \
 	lockfile.o \
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);
+}
+
+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;
+}
+
+int compress_next(z_stream *stream, int flush)
+{
+	int result;
+
+	do {
+		result = deflate(stream, flush);
+	} while (result == Z_OK);
+
+	return result;
+}
+
+unsigned long compress_free(z_stream *stream)
+{
+	deflateEnd(stream);
+	return stream->total_out;
+}
+
+unsigned long compress_all(int level, unsigned char *data,
+                           unsigned long size, unsigned char **out)
+{
+	int bound, result;
+	z_stream stream;
+
+	bound = compress_alloc(&stream, level, size);
+	compress_start(&stream, data, size, NULL, bound);
+
+	*out = stream.next_out;
+	result = compress_next(&stream, Z_FINISH);
+
+	if (result != Z_STREAM_END) {
+		compress_free(&stream);
+		free(*out);
+		*out = NULL;
+		return 0;
+	}
+	return compress_free(&stream);
+}
diff --git a/compress.h b/compress.h
new file mode 100644
index 0000000..d73c365
--- /dev/null
+++ b/compress.h
@@ -0,0 +1,12 @@
+#ifndef COMPRESS_H
+#define COMPRESS_H
+
+extern unsigned long compress_alloc(z_stream *stream, int level, unsigned long size);
+extern int compress_start(z_stream *stream, unsigned char *in, unsigned long in_size,
+                           unsigned char *out, unsigned long out_size);
+extern int compress_next(z_stream *stream, int flush);
+extern unsigned long compress_free(z_stream *stream);
+extern unsigned long compress_all(int level, unsigned char *data, unsigned long size,
+                                  unsigned char **out);
+
+#endif
-- 
1.5.4.rc4.39.g524a
Marco Costalba· Feb 2, 2008, 11:35 UTC · re: Marco Costalba · lore

[PATCH 02/11] Use new compress helpers in git files

These are the 'easy' ones, where a signgle step compression is requested so that we can use only one call to compress_all()

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 archive-zip.c          |   28 +++-------------------------
 builtin-pack-objects.c |   19 +++----------------
 diff.c                 |   22 +++++-----------------
 index-pack.c           |   20 +++-----------------
 4 files changed, 14 insertions(+), 75 deletions(-)
Show changes to 4 files +14 −75

archive-zip.c, builtin-pack-objects.c, diff.c, index-pack.c

diff --git a/archive-zip.c b/archive-zip.c
index 74e30f6..9071b86 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -3,6 +3,7 @@
  */
 #include "cache.h"
 #include "commit.h"
+#include "compress.h"
 #include "blob.h"
 #include "tree.h"
 #include "quote.h"
@@ -97,33 +98,10 @@ static void copy_le32(unsigned char *dest, unsigned int n)
 static void *zlib_deflate(void *data, unsigned long size,
                           unsigned long *compressed_size)
 {
-	z_stream stream;
-	unsigned long maxsize;
-	void *buffer;
-	int result;
-
-	memset(&stream, 0, sizeof(stream));
-	deflateInit(&stream, zlib_compression_level);
-	maxsize = deflateBound(&stream, size);
-	buffer = xmalloc(maxsize);
-
-	stream.next_in = data;
-	stream.avail_in = size;
-	stream.next_out = buffer;
-	stream.avail_out = maxsize;
-
-	do {
-		result = deflate(&stream, Z_FINISH);
-	} while (result == Z_OK);
-
-	if (result != Z_STREAM_END) {
-		free(buffer);
-		return NULL;
-	}
 
-	deflateEnd(&stream);
-	*compressed_size = stream.total_out;
+	unsigned char *buffer = NULL;
 
+	*compressed_size = compress_all(zlib_compression_level, data, size, &buffer);
 	return buffer;
 }
 
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index d3efeff..991a30f 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "compress.h"
 #include "attr.h"
 #include "object.h"
 #include "blob.h"
@@ -409,9 +410,7 @@ static unsigned long write_object(struct sha1file *f,
 				 */
 
 	if (!to_reuse) {
-		z_stream stream;
-		unsigned long maxsize;
-		void *out;
+		unsigned char *out = NULL;
 		if (!usable_delta) {
 			buf = read_sha1_file(entry->idx.sha1, &obj_type, &size);
 			if (!buf)
@@ -432,19 +431,7 @@ static unsigned long write_object(struct sha1file *f,
 				OBJ_OFS_DELTA : OBJ_REF_DELTA;
 		}
 		/* compress the data to store and put compressed length in datalen */
-		memset(&stream, 0, sizeof(stream));
-		deflateInit(&stream, pack_compression_level);
-		maxsize = deflateBound(&stream, size);
-		out = xmalloc(maxsize);
-		/* Compress it */
-		stream.next_in = buf;
-		stream.avail_in = size;
-		stream.next_out = out;
-		stream.avail_out = maxsize;
-		while (deflate(&stream, Z_FINISH) == Z_OK)
-			/* nothing */;
-		deflateEnd(&stream);
-		datalen = stream.total_out;
+		datalen = compress_all(pack_compression_level, buf, size, &out);
 
 		/*
 		 * The object header is a byte of 'type' followed by zero or
diff --git a/diff.c b/diff.c
index d464fe3..9eb9672 100644
--- a/diff.c
+++ b/diff.c
@@ -2,6 +2,7 @@
  * Copyright (C) 2005 Junio C Hamano
  */
 #include "cache.h"
+#include "compress.h"
 #include "quote.h"
 #include "diff.h"
 #include "diffcore.h"
@@ -1037,23 +1038,10 @@ static unsigned char *deflate_it(char *data,
 				 unsigned long size,
 				 unsigned long *result_size)
 {
-	int bound;
-	unsigned char *deflated;
-	z_stream stream;
-
-	memset(&stream, 0, sizeof(stream));
-	deflateInit(&stream, zlib_compression_level);
-	bound = deflateBound(&stream, size);
-	deflated = xmalloc(bound);
-	stream.next_out = deflated;
-	stream.avail_out = bound;
-
-	stream.next_in = (unsigned char *)data;
-	stream.avail_in = size;
-	while (deflate(&stream, Z_FINISH) == Z_OK)
-		; /* nothing */
-	deflateEnd(&stream);
-	*result_size = stream.total_out;
+	unsigned char *deflated = NULL;
+
+	*result_size = compress_all(zlib_compression_level,
+                                   (unsigned char *)data, size, &deflated);
 	return deflated;
 }
 
diff --git a/index-pack.c b/index-pack.c
index 9fd6982..880088e 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "compress.h"
 #include "delta.h"
 #include "pack.h"
 #include "csum-file.h"
@@ -494,24 +495,9 @@ static void parse_pack_objects(unsigned char *sha1)
 
 static int write_compressed(int fd, void *in, unsigned int size, uint32_t *obj_crc)
 {
-	z_stream stream;
-	unsigned long maxsize;
-	void *out;
+	unsigned char *out = NULL;
 
-	memset(&stream, 0, sizeof(stream));
-	deflateInit(&stream, zlib_compression_level);
-	maxsize = deflateBound(&stream, size);
-	out = xmalloc(maxsize);
-
-	/* Compress it */
-	stream.next_in = in;
-	stream.avail_in = size;
-	stream.next_out = out;
-	stream.avail_out = maxsize;
-	while (deflate(&stream, Z_FINISH) == Z_OK);
-	deflateEnd(&stream);
-
-	size = stream.total_out;
+	size = compress_all(zlib_compression_level, in, size, &out);
 	write_or_die(fd, out, size);
 	*obj_crc = crc32(*obj_crc, out, size);
 	free(out);
-- 
1.5.4.rc4.39.g524a
Marco Costalba· Feb 2, 2008, 11:35 UTC · re: Marco Costalba · lore

[PATCH 03/11] Use new compress helpers in fast-import

Here is slightly more difficult, in particular a xrealloc() has been substituted with a free() + xmalloc() to keep the code simple.

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 fast-import.c |   45 +++++++++++++++------------------------------
 1 files changed, 15 insertions(+), 30 deletions(-)
Show changes to fast-import.c +15 −30
diff --git a/fast-import.c b/fast-import.c
index a523b17..b6bb84c 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -141,6 +141,7 @@ Format of STDIN stream:
 
 #include "builtin.h"
 #include "cache.h"
+#include "compress.h"
 #include "object.h"
 #include "blob.h"
 #include "tree.h"
@@ -997,13 +998,13 @@ static int store_object(
 	unsigned char *sha1out,
 	uintmax_t mark)
 {
-	void *out, *delta;
+	unsigned char *out, *delta;
 	struct object_entry *e;
 	unsigned char hdr[96];
 	unsigned char sha1[20];
 	unsigned long hdrlen, deltalen;
 	SHA_CTX c;
-	z_stream s;
+	int out_size;
 
 	hdrlen = sprintf((char*)hdr,"%s %lu", typename(type),
 		(unsigned long)dat->len) + 1;
@@ -1039,24 +1040,15 @@ static int store_object(
 	} else
 		delta = NULL;
 
-	memset(&s, 0, sizeof(s));
-	deflateInit(&s, pack_compression_level);
-	if (delta) {
-		s.next_in = delta;
-		s.avail_in = deltalen;
-	} else {
-		s.next_in = (void *)dat->buf;
-		s.avail_in = dat->len;
-	}
-	s.avail_out = deflateBound(&s, s.avail_in);
-	s.next_out = out = xmalloc(s.avail_out);
-	while (deflate(&s, Z_FINISH) == Z_OK)
-		/* nothing */;
-	deflateEnd(&s);
+	if (delta)
+		out_size = compress_all(pack_compression_level, delta, deltalen, &out);
+	else
+		out_size = compress_all(pack_compression_level,
+                                       (unsigned char *)dat->buf, dat->len, &out);
 
 	/* Determine if we should auto-checkpoint. */
-	if ((pack_size + 60 + s.total_out) > max_packsize
-		|| (pack_size + 60 + s.total_out) < pack_size) {
+	if ((pack_size + 60 + out_size) > max_packsize
+		|| (pack_size + 60 + out_size) < pack_size) {
 
 		/* This new object needs to *not* have the current pack_id. */
 		e->pack_id = pack_id + 1;
@@ -1066,16 +1058,9 @@ static int store_object(
 		if (delta) {
 			free(delta);
 			delta = NULL;
-
-			memset(&s, 0, sizeof(s));
-			deflateInit(&s, pack_compression_level);
-			s.next_in = (void *)dat->buf;
-			s.avail_in = dat->len;
-			s.avail_out = deflateBound(&s, s.avail_in);
-			s.next_out = out = xrealloc(out, s.avail_out);
-			while (deflate(&s, Z_FINISH) == Z_OK)
-				/* nothing */;
-			deflateEnd(&s);
+			free(out);
+			out_size = compress_all(pack_compression_level,
+                                               (unsigned char *)dat->buf, dat->len, &out);
 		}
 	}
 
@@ -1108,8 +1093,8 @@ static int store_object(
 		pack_size += hdrlen;
 	}
 
-	write_or_die(pack_data->pack_fd, out, s.total_out);
-	pack_size += s.total_out;
+	write_or_die(pack_data->pack_fd, out, out_size);
+	pack_size += out_size;
 
 	free(out);
 	free(delta);
-- 
1.5.4.rc4.39.g524a
Marco Costalba· Feb 2, 2008, 11:35 UTC · re: Marco Costalba · lore

[PATCH 04/11] Use new compress helpers in http-push.c

A multistep compress is required here, so we need the full arsenal of compress helpers.

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 http-push.c |   22 ++++++++--------------
 1 files changed, 8 insertions(+), 14 deletions(-)
Show changes to http-push.c +8 −14
diff --git a/http-push.c b/http-push.c
index b2b410d..a7997ec 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "commit.h"
+#include "compress.h"
 #include "pack.h"
 #include "tag.h"
 #include "blob.h"
@@ -491,31 +492,24 @@ static void start_put(struct transfer_request *request)
 	hdrlen = sprintf(hdr, "%s %lu", typename(type), len) + 1;
 
 	/* Set it up */
-	memset(&stream, 0, sizeof(stream));
-	deflateInit(&stream, zlib_compression_level);
-	size = deflateBound(&stream, len + hdrlen);
+	size = compress_alloc(&stream, zlib_compression_level, len + hdrlen);
 	strbuf_init(&request->buffer.buf, size);
 	request->buffer.posn = 0;
 
 	/* Compress it */
-	stream.next_out = (unsigned char *)request->buffer.buf.buf;
-	stream.avail_out = size;
+	compress_start(&stream, (void *)hdr, hdrlen,
+                      (unsigned char *)request->buffer.buf.buf, size);
 
 	/* First header.. */
-	stream.next_in = (void *)hdr;
-	stream.avail_in = hdrlen;
-	while (deflate(&stream, 0) == Z_OK)
-		/* nothing */;
+	compress_next(&stream, Z_NO_FLUSH);
 
 	/* Then the data itself.. */
 	stream.next_in = unpacked;
 	stream.avail_in = len;
-	while (deflate(&stream, Z_FINISH) == Z_OK)
-		/* nothing */;
-	deflateEnd(&stream);
-	free(unpacked);
+	compress_next(&stream, Z_FINISH);
 
-	request->buffer.buf.len = stream.total_out;
+	request->buffer.buf.len = compress_free(&stream);
+	free(unpacked);
 
 	request->url = xmalloc(strlen(remote->url) +
 			       strlen(request->lock->token) + 51);
-- 
1.5.4.rc4.39.g524a
Marco Costalba· Feb 2, 2008, 11:35 UTC · re: Marco Costalba · lore

[PATCH 05/11] Use new compress helpers in sha1_file.c

A multistep compress is required here, so we need the full arsenal of compress helpers.

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 sha1_file.c |   41 ++++++++++++-----------------------------
 1 files changed, 12 insertions(+), 29 deletions(-)
Show changes to sha1_file.c +12 −29
diff --git a/sha1_file.c b/sha1_file.c
index 66a4e00..f48ad04 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -7,6 +7,7 @@
  * creation etc.
  */
 #include "cache.h"
+#include "compress.h"
 #include "delta.h"
 #include "pack.h"
 #include "blob.h"
@@ -2102,33 +2103,23 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha
 	}
 
 	/* Set it up */
-	memset(&stream, 0, sizeof(stream));
-	deflateInit(&stream, zlib_compression_level);
-	size = 8 + deflateBound(&stream, len+hdrlen);
+	size = 8 + compress_alloc(&stream, zlib_compression_level, len+hdrlen);
 	compressed = xmalloc(size);
 
 	/* Compress it */
-	stream.next_out = compressed;
-	stream.avail_out = size;
+	compress_start(&stream, (unsigned char *)hdr, hdrlen, compressed, size);
 
 	/* First header.. */
-	stream.next_in = (unsigned char *)hdr;
-	stream.avail_in = hdrlen;
-	while (deflate(&stream, 0) == Z_OK)
-		/* nothing */;
+	compress_next(&stream, Z_NO_FLUSH);
 
 	/* Then the data itself.. */
 	stream.next_in = buf;
 	stream.avail_in = len;
-	ret = deflate(&stream, Z_FINISH);
+	ret = compress_next(&stream, Z_FINISH);
 	if (ret != Z_STREAM_END)
 		die("unable to deflate new object %s (%d)", sha1_to_hex(sha1), ret);
 
-	ret = deflateEnd(&stream);
-	if (ret != Z_OK)
-		die("deflateEnd on object %s failed (%d)", sha1_to_hex(sha1), ret);
-
-	size = stream.total_out;
+	size = compress_free(&stream);
 
 	if (write_buffer(fd, compressed, size) < 0)
 		die("unable to write sha1 file");
@@ -2163,30 +2154,22 @@ static void *repack_object(const unsigned char *sha1, unsigned long *objsize)
 	hdrlen = sprintf(hdr, "%s %lu", typename(type), len) + 1;
 
 	/* Set it up */
-	memset(&stream, 0, sizeof(stream));
-	deflateInit(&stream, zlib_compression_level);
-	size = deflateBound(&stream, len + hdrlen);
+	size = compress_alloc(&stream, zlib_compression_level, len + hdrlen);
 	buf = xmalloc(size);
 
 	/* Compress it */
-	stream.next_out = buf;
-	stream.avail_out = size;
+	compress_start(&stream, (unsigned char *)hdr, hdrlen, buf, size);
 
 	/* First header.. */
-	stream.next_in = (void *)hdr;
-	stream.avail_in = hdrlen;
-	while (deflate(&stream, 0) == Z_OK)
-		/* nothing */;
+	compress_next(&stream, Z_NO_FLUSH);
 
 	/* Then the data itself.. */
 	stream.next_in = unpacked;
 	stream.avail_in = len;
-	while (deflate(&stream, Z_FINISH) == Z_OK)
-		/* nothing */;
-	deflateEnd(&stream);
-	free(unpacked);
+	compress_next(&stream, Z_FINISH);
 
-	*objsize = stream.total_out;
+	*objsize = compress_free(&stream);
+	free(unpacked);
 	return buf;
 }
 
-- 
1.5.4.rc4.39.g524a
Marco Costalba· Feb 2, 2008, 11:35 UTC · re: Marco Costalba · lore

[PATCH 06/11] Better error handling in compress_all()

Also let the caller to xmalloc() the buffer int compress_start()

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 compress.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)
Show changes to compress.c +9 −10
diff --git a/compress.c b/compress.c
index f6986c3..0d0b9d9 100644
--- a/compress.c
+++ b/compress.c
@@ -12,7 +12,7 @@ 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->next_out = out;
 	stream->avail_out = out_size;
 	stream->next_in = in;
 	stream->avail_in = in_size;
@@ -36,19 +36,18 @@ unsigned long compress_free(z_stream *stream)
 	return stream->total_out;
 }
 
-unsigned long compress_all(int level, unsigned char *data,
-                           unsigned long size, unsigned char **out)
+unsigned long compress_all(int level, unsigned char *in,
+                           unsigned long in_size, unsigned char **out)
 {
-	int bound, result;
+	unsigned long out_size;
 	z_stream stream;
 
-	bound = compress_alloc(&stream, level, size);
-	compress_start(&stream, data, size, NULL, bound);
+	out_size = compress_alloc(&stream, level, in_size);
+	*out = xmalloc(out_size);
 
-	*out = stream.next_out;
-	result = compress_next(&stream, Z_FINISH);
-
-	if (result != Z_STREAM_END) {
+	if (   compress_start(&stream, in, in_size, *out, out_size) != Z_OK
+	    || compress_next(&stream, Z_FINISH) != Z_STREAM_END)
+	{
 		compress_free(&stream);
 		free(*out);
 		*out = NULL;
-- 
1.5.4.rc4.39.g524a
Marco Costalba· Feb 2, 2008, 11:35 UTC · re: Marco Costalba · lore

[PATCH 07/11] Introduce stream decompress helpers

Decompressing turns out to be more difficult then comrpessing.

Helpers are more because more are the way zlib deflate() is used in git.

This patch just introduces the helpers, still no code change.

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 compress.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 compress.h |   17 ++++++++++++-
 2 files changed, 97 insertions(+), 1 deletions(-)
Show changes to 2 files +97 −1

compress.c, compress.h

diff --git a/compress.c b/compress.c
index 0d0b9d9..cf9d5ca 100644
--- a/compress.c
+++ b/compress.c
@@ -1,6 +1,10 @@
 #include "cache.h"
 #include "compress.h"
 
+/*
+ *     Compression helpers
+ */
+
 unsigned long compress_alloc(z_stream *stream, int level, unsigned long size)
 {
 	memset(stream, 0, sizeof(*stream));
@@ -55,3 +59,80 @@ unsigned long compress_all(int level, unsigned char *in,
 	}
 	return compress_free(&stream);
 }
+
+
+/*
+ *     Decompression helpers
+ */
+
+int decompress_alloc(z_stream *stream)
+{
+	memset(stream, 0, sizeof(*stream));
+	return inflateInit(stream);
+}
+
+int decompress_from(z_stream *stream, unsigned char *in, unsigned long in_size)
+{
+	stream->next_in = in;
+	stream->avail_in = in_size;
+	return Z_OK;
+}
+
+int decompress_into(z_stream *stream, unsigned char *out, unsigned long out_size)
+{
+	stream->next_out = out;
+	stream->avail_out = out_size;
+	return Z_OK;
+}
+
+int decompress_next(z_stream *stream, int flush)
+{
+	return inflate(stream, flush);
+}
+
+int decompress_next_from(z_stream *stream, unsigned char *in, unsigned long in_size, int flush)
+{
+	decompress_from(stream, in, in_size);
+	return inflate(stream, flush);
+}
+
+int decompress_next_into(z_stream *stream, unsigned char *out, unsigned long out_size, int flush)
+{
+	decompress_into(stream, out, out_size);
+	return inflate(stream, flush);
+}
+
+unsigned long decompress_free(z_stream *stream)
+{
+	inflateEnd(stream);
+	return stream->total_out;
+}
+
+unsigned long decompress_all(unsigned char *in, unsigned long in_size,
+                             unsigned char *out, unsigned long out_size)
+{
+/* caller should check for return value != 0 */
+
+	z_stream stream;
+	int st;
+
+	if (decompress_alloc(&stream) != Z_OK)
+		return 0;
+
+	if (   decompress_from(&stream, in, in_size) != Z_OK
+	    || decompress_into(&stream, out, out_size) != Z_OK)
+		goto fail;
+
+	do {
+		st = decompress_next(&stream, Z_FINISH);
+	} while (st == Z_OK);
+
+	if (st != Z_STREAM_END)
+		goto fail;
+
+	return decompress_free(&stream);
+
+fail:
+	decompress_free(&stream);
+	return 0;
+}
diff --git a/compress.h b/compress.h
index d73c365..30cc80f 100644
--- a/compress.h
+++ b/compress.h
@@ -6,7 +6,22 @@ extern int compress_start(z_stream *stream, unsigned char *in, unsigned long in_
                            unsigned char *out, unsigned long out_size);
 extern int compress_next(z_stream *stream, int flush);
 extern unsigned long compress_free(z_stream *stream);
-extern unsigned long compress_all(int level, unsigned char *data, unsigned long size,
+extern unsigned long compress_all(int level, unsigned char *in, unsigned long in_size,
                                   unsigned char **out);
 
+
+extern int decompress_alloc(z_stream *stream);
+
+extern int decompress_from(z_stream *stream, unsigned char *in, unsigned long in_size);
+extern int decompress_into(z_stream *stream, unsigned char *out, unsigned long out_size);
+
+extern int decompress_next(z_stream *stream, int flush);
+extern int decompress_next_from(z_stream *stream, unsigned char *in, unsigned long in_size, int flush);
+extern int decompress_next_into(z_stream *stream, unsigned char *out, unsigned long out_size, int flush);
+
+extern unsigned long decompress_free(z_stream *stream);
+
+extern unsigned long decompress_all(unsigned char *in, unsigned long in_size,
+                                    unsigned char *out, unsigned long out_size);
+
 #endif
-- 
1.5.4.rc4.39.g524a
Marco Costalba· Feb 2, 2008, 11:35 UTC · re: Marco Costalba · lore

[PATCH 08/11] Use new decompress_all() helper in git

Only in two places is possible to really simplify deflate code with the all_in_one decompress_all()

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 builtin-apply.c |   23 +++++++++--------------
 index-pack.c    |   30 +++++++-----------------------
 2 files changed, 16 insertions(+), 37 deletions(-)
Show changes to 2 files +14 −35

builtin-apply.c, index-pack.c

diff --git a/builtin-apply.c b/builtin-apply.c
index 30d86f2..fa589e6 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -8,6 +8,7 @@
  */
 #include "cache.h"
 #include "cache-tree.h"
+#include "compress.h"
 #include "quote.h"
 #include "blob.h"
 #include "delta.h"
@@ -1105,23 +1106,17 @@ static inline int metadata_changes(struct patch *patch)
 static char *inflate_it(const void *data, unsigned long size,
 			unsigned long inflated_size)
 {
-	z_stream stream;
-	void *out;
-	int st;
+	unsigned char *out;
+	unsigned long out_size;
 
-	memset(&stream, 0, sizeof(stream));
+	out = xmalloc(inflated_size);
+	out_size = decompress_all((unsigned char *)data, size, out, inflated_size);
 
-	stream.next_in = (unsigned char *)data;
-	stream.avail_in = size;
-	stream.next_out = out = xmalloc(inflated_size);
-	stream.avail_out = inflated_size;
-	inflateInit(&stream);
-	st = inflate(&stream, Z_FINISH);
-	if ((st != Z_STREAM_END) || stream.total_out != inflated_size) {
+	if (out_size != inflated_size) {
 		free(out);
 		return NULL;
 	}
-	return out;
+	return (char *)out;
 }
 
 static struct fragment *parse_binary_hunk(char **buf_p,
diff --git a/index-pack.c b/index-pack.c
index 880088e..30d7837 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -169,24 +169,18 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size)
 	z_stream stream;
 	void *buf = xmalloc(size);
 
-	memset(&stream, 0, sizeof(stream));
-	stream.next_out = buf;
-	stream.avail_out = size;
-	stream.next_in = fill(1);
-	stream.avail_in = input_len;
-	inflateInit(&stream);
+	decompress_alloc(&stream);
+	decompress_into(&stream, buf, size);
 
 	for (;;) {
-		int ret = inflate(&stream, 0);
+		int ret = decompress_next_from(&stream, fill(1), input_len, Z_NO_FLUSH);
 		use(input_len - stream.avail_in);
 		if (stream.total_out == size && ret == Z_STREAM_END)
 			break;
 		if (ret != Z_OK)
-			bad_object(offset, "inflate returned %d", ret);
-		stream.next_in = fill(1);
-		stream.avail_in = input_len;
+			bad_object(offset, "decompress returned %d", ret);
 	}
-	inflateEnd(&stream);
+	decompress_free(&stream);
 	return buf;
 }
 
@@ -261,8 +255,6 @@ static void *get_data_from_pack(struct object_entry *obj)
 	unsigned long len = obj[1].idx.offset - from;
 	unsigned long rdy = 0;
 	unsigned char *src, *data;
-	z_stream stream;
-	int st;
 
 	src = xmalloc(len);
 	data = src;
@@ -273,16 +265,8 @@ static void *get_data_from_pack(struct object_entry *obj)
 		rdy += n;
 	} while (rdy < len);
 	data = xmalloc(obj->size);
-	memset(&stream, 0, sizeof(stream));
-	stream.next_out = data;
-	stream.avail_out = obj->size;
-	stream.next_in = src;
-	stream.avail_in = len;
-	inflateInit(&stream);
-	while ((st = inflate(&stream, Z_FINISH)) == Z_OK);
-	inflateEnd(&stream);
-	if (st != Z_STREAM_END || stream.total_out != obj->size)
-		die("serious inflate inconsistency");
+	if (decompress_all(src, len, data, obj->size) != obj->size)
+		die("serious decompress inconsistency");
 	free(src);
 	return data;
 }
-- 
1.5.4.rc4.39.g524a
Marco Costalba· Feb 2, 2008, 11:35 UTC · re: Marco Costalba · lore

[PATCH 09/11] Convert http-push.c and http-walker.c

Conversion for both files is very similar and in both cases I have added a FIXME where I would have added an additional decompress_free()

The corresponding deflateEnd() call is not present in the original code, so I left the line commented out.

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 http-push.c   |   21 +++++++++------------
 http-walker.c |   22 +++++++++-------------
 2 files changed, 18 insertions(+), 25 deletions(-)
Show changes to 2 files +18 −25

http-push.c, http-walker.c

diff --git a/http-push.c b/http-push.c
index a7997ec..0920640 100644
--- a/http-push.c
+++ b/http-push.c
@@ -203,12 +203,11 @@ static size_t fwrite_sha1_file(void *ptr, size_t eltsize, size_t nmemb,
 		posn += retval;
 	} while (posn < size);
 
-	request->stream.avail_in = size;
-	request->stream.next_in = ptr;
+	decompress_from(&request->stream, ptr, size);
+
 	do {
-		request->stream.next_out = expn;
-		request->stream.avail_out = sizeof(expn);
-		request->zret = inflate(&request->stream, Z_SYNC_FLUSH);
+		request->zret = decompress_next_into(&request->stream, expn,
+		                                     sizeof(expn), Z_SYNC_FLUSH);
 		SHA1_Update(&request->c, expn,
 			    sizeof(expn) - request->stream.avail_out);
 	} while (request->stream.avail_in && request->zret == Z_OK);
@@ -266,9 +265,7 @@ static void start_fetch_loose(struct transfer_request *request)
 		return;
 	}
 
-	memset(&request->stream, 0, sizeof(request->stream));
-
-	inflateInit(&request->stream);
+	decompress_alloc(&request->stream);
 
 	SHA1_Init(&request->c);
 
@@ -305,11 +302,11 @@ static void start_fetch_loose(struct transfer_request *request)
 	}
 	unlink(prevfile);
 
-	/* Reset inflate/SHA1 if there was an error reading the previous temp
+	/* Reset decompress/SHA1 if there was an error reading the previous temp
 	   file; also rewind to the beginning of the local file. */
 	if (prev_read == -1) {
-		memset(&request->stream, 0, sizeof(request->stream));
-		inflateInit(&request->stream);
+		// FIXME should we need decompress_free() here?
+		decompress_alloc(&request->stream);
 		SHA1_Init(&request->c);
 		if (prev_posn>0) {
 			prev_posn = 0;
@@ -735,7 +732,7 @@ static void finish_request(struct transfer_request *request)
 			if (request->http_code == 416)
 				fprintf(stderr, "Warning: requested range invalid; we may already have all the data.\n");
 
-			inflateEnd(&request->stream);
+			decompress_free(&request->stream);
 			SHA1_Final(request->real_sha1, &request->c);
 			if (request->zret != Z_STREAM_END) {
 				unlink(request->tmpfile);
diff --git a/http-walker.c b/http-walker.c
index 2c37868..b1d2a28 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "commit.h"
+#include "compress.h"
 #include "pack.h"
 #include "walker.h"
 #include "http.h"
@@ -77,12 +78,10 @@ static size_t fwrite_sha1_file(void *ptr, size_t eltsize, size_t nmemb,
 		posn += retval;
 	} while (posn < size);
 
-	obj_req->stream.avail_in = size;
-	obj_req->stream.next_in = ptr;
+	decompress_from(&obj_req->stream, ptr, size);
 	do {
-		obj_req->stream.next_out = expn;
-		obj_req->stream.avail_out = sizeof(expn);
-		obj_req->zret = inflate(&obj_req->stream, Z_SYNC_FLUSH);
+		obj_req->zret = decompress_next_into(&obj_req->stream, expn,
+                                             sizeof(expn), Z_SYNC_FLUSH);
 		SHA1_Update(&obj_req->c, expn,
 			    sizeof(expn) - obj_req->stream.avail_out);
 	} while (obj_req->stream.avail_in && obj_req->zret == Z_OK);
@@ -140,10 +139,7 @@ static void start_object_request(struct walker *walker,
 		return;
 	}
 
-	memset(&obj_req->stream, 0, sizeof(obj_req->stream));
-
-	inflateInit(&obj_req->stream);
-
+	decompress_alloc(&obj_req->stream);
 	SHA1_Init(&obj_req->c);
 
 	url = xmalloc(strlen(obj_req->repo->base) + 51);
@@ -179,11 +175,11 @@ static void start_object_request(struct walker *walker,
 	}
 	unlink(prevfile);
 
-	/* Reset inflate/SHA1 if there was an error reading the previous temp
+	/* Reset decompress/SHA1 if there was an error reading the previous temp
 	   file; also rewind to the beginning of the local file. */
 	if (prev_read == -1) {
-		memset(&obj_req->stream, 0, sizeof(obj_req->stream));
-		inflateInit(&obj_req->stream);
+		// FIXME should we need decompress_free() here?
+		decompress_alloc(&obj_req->stream);
 		SHA1_Init(&obj_req->c);
 		if (prev_posn>0) {
 			prev_posn = 0;
@@ -243,7 +239,7 @@ static void finish_object_request(struct object_request *obj_req)
 		return;
 	}
 
-	inflateEnd(&obj_req->stream);
+	decompress_free(&obj_req->stream);
 	SHA1_Final(obj_req->real_sha1, &obj_req->c);
 	if (obj_req->zret != Z_STREAM_END) {
 		unlink(obj_req->tmpfile);
-- 
1.5.4.rc4.39.g524a
Marco Costalba· Feb 2, 2008, 11:35 UTC · re: Marco Costalba · lore

[PATCH 10/11] Convert builtin-pack/unpack

In this case decompression helper conversion is quite similar and not too complex, so they go togheter.

Also in index-pack.c pass correct arguments to decompress_next_from().

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 builtin-pack-objects.c   |   14 ++++++--------
 builtin-unpack-objects.c |   22 +++++++++-------------
 index-pack.c             |    4 +++-
 3 files changed, 18 insertions(+), 22 deletions(-)
Show changes to 3 files +18 −22

builtin-pack-objects.c, builtin-unpack-objects.c, index-pack.c

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 991a30f..43614ce 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -302,19 +302,17 @@ static int check_pack_inflate(struct packed_git *p,
 {
 	z_stream stream;
 	unsigned char fakebuf[4096], *in;
+	unsigned int in_size = 0;
 	int st;
 
-	memset(&stream, 0, sizeof(stream));
-	inflateInit(&stream);
+	decompress_alloc(&stream);
 	do {
-		in = use_pack(p, w_curs, offset, &stream.avail_in);
-		stream.next_in = in;
-		stream.next_out = fakebuf;
-		stream.avail_out = sizeof(fakebuf);
-		st = inflate(&stream, Z_FINISH);
+		decompress_into(&stream, fakebuf, sizeof(fakebuf));
+		in = use_pack(p, w_curs, offset, &in_size);
+		st = decompress_next_from(&stream, in, in_size, Z_FINISH);
 		offset += stream.next_in - in;
 	} while (st == Z_OK || st == Z_BUF_ERROR);
-	inflateEnd(&stream);
+	decompress_free(&stream);
 	return (st == Z_STREAM_END &&
 		stream.total_out == expect &&
 		stream.total_in == len) ? 0 : -1;
diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
index 1e51865..c996560 100644
--- a/builtin-unpack-objects.c
+++ b/builtin-unpack-objects.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "compress.h"
 #include "object.h"
 #include "delta.h"
 #include "pack.h"
@@ -61,23 +62,20 @@ static void use(int bytes)
 static void *get_data(unsigned long size)
 {
 	z_stream stream;
-	void *buf = xmalloc(size);
+	unsigned char *buf = xmalloc(size);;
 
-	memset(&stream, 0, sizeof(stream));
-
-	stream.next_out = buf;
-	stream.avail_out = size;
-	stream.next_in = fill(1);
-	stream.avail_in = len;
-	inflateInit(&stream);
+	decompress_alloc(&stream);
+	decompress_into(&stream, buf, size);
 
 	for (;;) {
-		int ret = inflate(&stream, 0);
+		/* fill() modifies len, so be sure is evaluated as first */
+		void* tmp = fill(1);
+		int ret = decompress_next_from(&stream, tmp, len, Z_NO_FLUSH);
 		use(len - stream.avail_in);
 		if (stream.total_out == size && ret == Z_STREAM_END)
 			break;
 		if (ret != Z_OK) {
-			error("inflate returned %d\n", ret);
+			error("decompress returned %d\n", ret);
 			free(buf);
 			buf = NULL;
 			if (!recover)
@@ -85,10 +83,8 @@ static void *get_data(unsigned long size)
 			has_errors = 1;
 			break;
 		}
-		stream.next_in = fill(1);
-		stream.avail_in = len;
 	}
-	inflateEnd(&stream);
+	decompress_free(&stream);
 	return buf;
 }
 
diff --git a/index-pack.c b/index-pack.c
index 30d7837..929de39 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -173,7 +173,9 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size)
 	decompress_into(&stream, buf, size);
 
 	for (;;) {
-		int ret = decompress_next_from(&stream, fill(1), input_len, Z_NO_FLUSH);
+		/* fill() modifies len, so be sure is evaluated as first */
+		void* tmp = fill(1);
+		int ret = decompress_next_from(&stream, tmp, input_len, Z_NO_FLUSH);
 		use(input_len - stream.avail_in);
 		if (stream.total_out == size && ret == Z_STREAM_END)
 			break;
-- 
1.5.4.rc4.39.g524a
Marco Costalba· Feb 2, 2008, 11:35 UTC · re: Marco Costalba · lore

[PATCH 11/11] Convert sha1_file.c to use decompress helpers

This is "The King".

It is the most difficult file to convert and some decompression functions have been created just for it.

Anyhow the lines of code removed (45) far surpass the ones added (26).

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 sha1_file.c |   71 +++++++++++++++++++++-------------------------------------
 1 files changed, 26 insertions(+), 45 deletions(-)
Show changes to sha1_file.c +26 −45
diff --git a/sha1_file.c b/sha1_file.c
index f48ad04..6500871 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1079,16 +1079,11 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon
 	enum object_type type;
 
 	/* Get the data stream */
-	memset(stream, 0, sizeof(*stream));
-	stream->next_in = map;
-	stream->avail_in = mapsize;
-	stream->next_out = buffer;
-	stream->avail_out = bufsiz;
+	decompress_alloc(stream);
+	decompress_into(stream, buffer, bufsiz);
 
-	if (legacy_loose_object(map)) {
-		inflateInit(stream);
-		return inflate(stream, 0);
-	}
+	if (legacy_loose_object(map))
+		return decompress_next_from(stream, map, mapsize, Z_NO_FLUSH);
 
 
 	/*
@@ -1105,9 +1100,7 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon
 	mapsize -= used;
 
 	/* Set up the stream for the rest.. */
-	stream->next_in = map;
-	stream->avail_in = mapsize;
-	inflateInit(stream);
+	decompress_from(stream, map, mapsize);
 
 	/* And generate the fake traditional header */
 	stream->total_out = 1 + snprintf(buffer, bufsiz, "%s %lu",
@@ -1141,14 +1134,13 @@ static void *unpack_sha1_rest(z_stream *stream, void *buffer, unsigned long size
 		 * we also want to check that zlib tells us that all
 		 * went well with status == Z_STREAM_END at the end.
 		 */
-		stream->next_out = buf + bytes;
-		stream->avail_out = size - bytes;
+		decompress_into(stream, buf + bytes, size - bytes);
 		while (status == Z_OK)
-			status = inflate(stream, Z_FINISH);
+			status = decompress_next(stream, Z_FINISH);
 	}
 	buf[size] = 0;
 	if (status == Z_STREAM_END && !stream->avail_in) {
-		inflateEnd(stream);
+		decompress_free(stream);
 		return buf;
 	}
 
@@ -1233,20 +1225,18 @@ unsigned long get_size_from_delta(struct packed_git *p,
 	unsigned char delta_head[20], *in;
 	z_stream stream;
 	int st;
+	unsigned int in_size = 0;
 
-	memset(&stream, 0, sizeof(stream));
-	stream.next_out = delta_head;
-	stream.avail_out = sizeof(delta_head);
+	decompress_alloc(&stream);
+	decompress_into(&stream, delta_head, sizeof(delta_head));
 
-	inflateInit(&stream);
 	do {
-		in = use_pack(p, w_curs, curpos, &stream.avail_in);
-		stream.next_in = in;
-		st = inflate(&stream, Z_FINISH);
+		in = use_pack(p, w_curs, curpos, &in_size);
+		st = decompress_next_from(&stream, in, in_size, Z_FINISH);
 		curpos += stream.next_in - in;
 	} while ((st == Z_OK || st == Z_BUF_ERROR) &&
 		 stream.total_out < sizeof(delta_head));
-	inflateEnd(&stream);
+	decompress_free(&stream);
 	if ((st != Z_STREAM_END) && stream.total_out != sizeof(delta_head))
 		die("delta data unpack-initial failed");
 
@@ -1323,7 +1313,7 @@ static int packed_delta_info(struct packed_git *p,
 	/* We choose to only get the type of the base object and
 	 * ignore potentially corrupt pack file that expects the delta
 	 * based on a base with a wrong size.  This saves tons of
-	 * inflate() calls.
+	 * decompress() calls.
 	 */
 	if (sizep)
 		*sizep = get_size_from_delta(p, w_curs, curpos);
@@ -1444,21 +1434,18 @@ static void *unpack_compressed_entry(struct packed_git *p,
 	int st;
 	z_stream stream;
 	unsigned char *buffer, *in;
+	unsigned int in_size = 0;
 
 	buffer = xmalloc(size + 1);
 	buffer[size] = 0;
-	memset(&stream, 0, sizeof(stream));
-	stream.next_out = buffer;
-	stream.avail_out = size;
-
-	inflateInit(&stream);
+	decompress_alloc(&stream);
+	decompress_into(&stream, buffer, size);
 	do {
-		in = use_pack(p, w_curs, curpos, &stream.avail_in);
-		stream.next_in = in;
-		st = inflate(&stream, Z_FINISH);
+		in = use_pack(p, w_curs, curpos, &in_size);
+		st = decompress_next_from(&stream, in, in_size, Z_FINISH);
 		curpos += stream.next_in - in;
 	} while (st == Z_OK || st == Z_BUF_ERROR);
-	inflateEnd(&stream);
+	decompress_free(&stream);
 	if ((st != Z_STREAM_END) || stream.total_out != size) {
 		free(buffer);
 		return NULL;
@@ -1804,7 +1791,7 @@ static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *size
 		status = error("unable to parse %s header", sha1_to_hex(sha1));
 	else if (sizep)
 		*sizep = size;
-	inflateEnd(&stream);
+	decompress_free(&stream);
 	munmap(map, mapsize);
 	return status;
 }
@@ -2212,21 +2199,15 @@ int write_sha1_from_fd(const unsigned char *sha1, int fd, char *buffer,
 			return error("unable to create temporary sha1 filename %s: %s\n", tmpfile, strerror(errno));
 	}
 
-	memset(&stream, 0, sizeof(stream));
-
-	inflateInit(&stream);
-
+	decompress_alloc(&stream);
 	SHA1_Init(&c);
 
 	do {
 		ssize_t size;
 		if (*bufposn) {
-			stream.avail_in = *bufposn;
-			stream.next_in = (unsigned char *) buffer;
+			decompress_from(&stream, (unsigned char *) buffer, *bufposn);
 			do {
-				stream.next_out = discard;
-				stream.avail_out = sizeof(discard);
-				ret = inflate(&stream, Z_SYNC_FLUSH);
+				ret = decompress_next_into(&stream, discard, sizeof(discard), Z_SYNC_FLUSH);
 				SHA1_Update(&c, discard, sizeof(discard) -
 					    stream.avail_out);
 			} while (stream.avail_in && ret == Z_OK);
@@ -2249,7 +2230,7 @@ int write_sha1_from_fd(const unsigned char *sha1, int fd, char *buffer,
 		}
 		*bufposn += size;
 	} while (1);
-	inflateEnd(&stream);
+	decompress_free(&stream);
 
 	fchmod(local, 0444);
 	if (close(local) != 0)
-- 
1.5.4.rc4.39.g524a
Junio C Hamano· Feb 4, 2008, 02:08 UTC · re: Marco Costalba · lore

Re: [PATCH 10/11] Convert builtin-pack/unpack

Marco Costalba <mcostalba@gmail.com> writes:
Show 17 quoted lines
> diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
> index 1e51865..c996560 100644
> --- a/builtin-unpack-objects.c
> +++ b/builtin-unpack-objects.c
> @@ -1,5 +1,6 @@
>  #include "builtin.h"
>  #include "cache.h"
> +#include "compress.h"
>  #include "object.h"
>  #include "delta.h"
>  #include "pack.h"
> @@ -61,23 +62,20 @@ static void use(int bytes)
>  static void *get_data(unsigned long size)
>  {
>  	z_stream stream;
> -	void *buf = xmalloc(size);
> +	unsigned char *buf = xmalloc(size);;

Why? Your other changes (e.g. unpack_entry_data()::index-pack.c in [08/11]) left the type of buf as it was, and I think the same should be done here.

Show 15 quoted lines
> -	memset(&stream, 0, sizeof(stream));
> -
> -	stream.next_out = buf;
> -	stream.avail_out = size;
> -	stream.next_in = fill(1);
> -	stream.avail_in = len;
> -	inflateInit(&stream);
> +	decompress_alloc(&stream);
> +	decompress_into(&stream, buf, size);
>  
>  	for (;;) {
> -		int ret = inflate(&stream, 0);
> +		/* fill() modifies len, so be sure is evaluated as first */
> +		void* tmp = fill(1);
> +		int ret = decompress_next_from(&stream, tmp, len, Z_NO_FLUSH);
(Style) that's "void *tmp".
Junio C Hamano· Feb 4, 2008, 02:07 UTC · re: Marco Costalba · lore

Re: [PATCH 08/11] Use new decompress_all() helper in git

Marco Costalba <mcostalba@gmail.com> writes:
Show 21 quoted lines
> Only in two places is possible to really simplify
> diff --git a/index-pack.c b/index-pack.c
> index 880088e..30d7837 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -169,24 +169,18 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size)
>  	z_stream stream;
>  	void *buf = xmalloc(size);
>  
> -	memset(&stream, 0, sizeof(stream));
> -	stream.next_out = buf;
> -	stream.avail_out = size;
> -	stream.next_in = fill(1);
> -	stream.avail_in = input_len;
> -	inflateInit(&stream);
> +	decompress_alloc(&stream);
> +	decompress_into(&stream, buf, size);
>  
>  	for (;;) {
> -		int ret = inflate(&stream, 0);
> +		int ret = decompress_next_from(&stream, fill(1), input_len, Z_NO_FLUSH);

The input_len variable is changed as a side effect of calling the fill() function. Don't you have the same issue that you handle with [10/11] here?

Junio C Hamano· Feb 4, 2008, 02:07 UTC · re: Marco Costalba · lore

Re: [PATCH 07/11] Introduce stream decompress helpers

Overall, everything looks quite nicely done.

The same comments as the compression side to "*_alloc()" apply. Perhaps call them "setup" and "finalize" or something?

Junio C Hamano· Feb 3, 2008, 22:54 UTC · re: Marco Costalba · lore

Re: [PATCH 06/11] Better error handling in compress_all()

Marco Costalba <mcostalba@gmail.com> writes:
> Also let the caller to xmalloc() the buffer
> int compress_start()

This is meant to be an improvement for [01/11] and I think should be done from the beginning by squashing into it.

Haven't looked at the decompression side yet. Help in reviewing this series from others are appreciated.

Junio C Hamano· Feb 3, 2008, 22:53 UTC · re: Marco Costalba · lore

Re: [PATCH 04/11] Use new compress helpers in http-push.c

Marco Costalba <mcostalba@gmail.com> writes:
Show 19 quoted lines
> A multistep compress is required here, so
> we need the full arsenal of compress helpers.
>
> Signed-off-by: Marco Costalba <mcostalba@gmail.com>
> ---
>  http-push.c |   22 ++++++++--------------
>  1 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/http-push.c b/http-push.c
> index b2b410d..a7997ec 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -491,31 +492,24 @@ static void start_put(struct transfer_request *request)
> ...
> -	stream.next_in = (void *)hdr;
> -	stream.avail_in = hdrlen;
> -	while (deflate(&stream, 0) == Z_OK)
> -		/* nothing */;
> +	compress_next(&stream, Z_NO_FLUSH);

Although the original does not bother to, the return value from compress_next() should be checked in later rounds.

Junio C Hamano· Feb 3, 2008, 22:53 UTC · re: Marco Costalba · lore

Re: [PATCH 03/11] Use new compress helpers in fast-import

Marco Costalba <mcostalba@gmail.com> writes:
Show 8 quoted lines
> Here is slightly more difficult, in particular
> a xrealloc() has been substituted with a
> free() + xmalloc() to keep the code simple.
>
> Signed-off-by: Marco Costalba <mcostalba@gmail.com>
> ---
>  fast-import.c |   45 +++++++++++++++------------------------------
>  1 files changed, 15 insertions(+), 30 deletions(-)

I'll let Shawn comment on this. The realloc() does not seem to be using the contents in the buffer from the previous round, so I suspect that a free() followed by an independent alloc() would be an improvement when the later call uses much larger buffer than the previous one, but would be a waste if the later one needs smaller buffer.

Shawn O. Pearce· Feb 4, 2008, 01:48 UTC · re: Junio C Hamano · lore

Re: [PATCH 03/11] Use new compress helpers in fast-import

Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> Marco Costalba <mcostalba@gmail.com> writes:
> 
> > Here is slightly more difficult, in particular
> > a xrealloc() has been substituted with a
> > free() + xmalloc() to keep the code simple.
> >
> > Signed-off-by: Marco Costalba <mcostalba@gmail.com>
> > ---
> >  fast-import.c |   45 +++++++++++++++------------------------------
> >  1 files changed, 15 insertions(+), 30 deletions(-)
> 
> I'll let Shawn comment on this.  The realloc() does not seem to
> be using the contents in the buffer from the previous round, so
> I suspect that a free() followed by an independent alloc() would
> be an improvement when the later call uses much larger buffer
> than the previous one, but would be a waste if the later one
> needs smaller buffer.

Junio is correct, that xrealloc isn't using the contents of the buffer from the last round, which makes any memcpy it might do internally due to movement to a larger buffer an utter waste.

In this new version we are probably always free'ing a buffer of a much smaller size than we are then later allocating (or in the old version xrealloc'ing to) because we are switching from a delta to full content. Its most likely the delta is way smaller, so I'd guess the malloc implementation is mostly going to another buffer. In short, Marco's change will most likely do better.

But this is all academic wanking. We're talking about this xrealloc (or free/xmalloc pair) happening only when we switch packfiles, which in fast-import is usually every 4 GiB of output. That's a *lot* of data to write. Who cares how many extra microseconds we spend to perform this buffer change; we probably hit it only once every 15-30 minutes, depending on how fast your system is able to transfer 4 GiB of data out of the source and into a packfile.

-- 
Shawn.
Shawn O. Pearce· Feb 4, 2008, 01:41 UTC · re: Marco Costalba · lore

Re: [PATCH 03/11] Use new compress helpers in fast-import

Marco Costalba <mcostalba@gmail.com> wrote:
Show 17 quoted lines
> diff --git a/fast-import.c b/fast-import.c
> index a523b17..b6bb84c 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -997,13 +998,13 @@ static int store_object(
>  	unsigned char *sha1out,
>  	uintmax_t mark)
>  {
> -	void *out, *delta;
> +	unsigned char *out, *delta;
>  	struct object_entry *e;
>  	unsigned char hdr[96];
>  	unsigned char sha1[20];
>  	unsigned long hdrlen, deltalen;
>  	SHA_CTX c;
> -	z_stream s;
> +	int out_size;
This really should be a size_t, shouldn't it?
  
-- 
Shawn.
Junio C Hamano· Feb 3, 2008, 22:54 UTC · re: Marco Costalba · lore

Re: [PATCH 02/11] Use new compress helpers in git files

Marco Costalba <mcostalba@gmail.com> writes:
Show 41 quoted lines
> These are the 'easy' ones, where a signgle step
> compression is requested so that we can use only
> one call to compress_all()
>
> Signed-off-by: Marco Costalba <mcostalba@gmail.com>
> ---
>  archive-zip.c          |   28 +++-------------------------
>  builtin-pack-objects.c |   19 +++----------------
>  diff.c                 |   22 +++++-----------------
>  index-pack.c           |   20 +++-----------------
>  4 files changed, 14 insertions(+), 75 deletions(-)
>
> diff --git a/archive-zip.c b/archive-zip.c
> index 74e30f6..9071b86 100644
> --- a/archive-zip.c
> +++ b/archive-zip.c
> @@ -3,6 +3,7 @@
>   */
>  #include "cache.h"
>  #include "commit.h"
> +#include "compress.h"
>  #include "blob.h"
>  #include "tree.h"
>  #include "quote.h"
> @@ -97,33 +98,10 @@ static void copy_le32(unsigned char *dest, unsigned int n)
>  static void *zlib_deflate(void *data, unsigned long size,
>                            unsigned long *compressed_size)
>  {
> ...
> -	if (result != Z_STREAM_END) {
> -		free(buffer);
> -		return NULL;
> -	}
>  
> -	deflateEnd(&stream);
> -	*compressed_size = stream.total_out;
> +	unsigned char *buffer = NULL;
>  
> +	*compressed_size = compress_all(zlib_compression_level, data, size, &buffer);
>  	return buffer;
>  }

This used to leave *compressed_size untouched upon deflate failure but now it sets it to zero. Is this change in behaviour safe?

Junio C Hamano· Feb 3, 2008, 22:53 UTC · re: Marco Costalba · lore

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

Marco Costalba <mcostalba@gmail.com> writes:
Show 9 quoted lines
> 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.

Show 15 quoted lines
> 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".
Show 10 quoted lines
> +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.

Show 5 quoted lines
> +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.

← back to recent threads