[PATCH 01/11] Introduce stream compress helpers

Subsystems: kernel build + files below scripts/ (unless maintained elsewhere), the rest

STALE3736d

21 messages, 3 authors, 2016-06-15 · open the first message on its own page

[PATCH 01/11] Introduce stream compress helpers

From: Marco Costalba <hidden>
Date: 2016-06-15 22:44:09

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 <redacted>
---
 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

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

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

From: Marco Costalba <hidden>
Date: 2016-06-15 22:44:09

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 <redacted>
---
 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)
 {
-	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

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

From: Marco Costalba <hidden>
Date: 2016-06-15 22:44:09

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 <redacted>
---
 fast-import.c |   45 +++++++++++++++------------------------------
 1 files changed, 15 insertions(+), 30 deletions(-)

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

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

From: Marco Costalba <hidden>
Date: 2016-06-15 22:44:09

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

Signed-off-by: Marco Costalba <redacted>
---
 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

@@ -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

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

From: Marco Costalba <hidden>
Date: 2016-06-15 22:44:09

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

Signed-off-by: Marco Costalba <redacted>
---
 sha1_file.c |   41 ++++++++++++-----------------------------
 1 files changed, 12 insertions(+), 29 deletions(-)

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

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

From: Marco Costalba <hidden>
Date: 2016-06-15 22:44:09

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 <redacted>
---
 builtin-pack-objects.c   |   14 ++++++--------
 builtin-unpack-objects.c |   22 +++++++++-------------
 index-pack.c             |    4 +++-
 3 files changed, 18 insertions(+), 22 deletions(-)

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

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

From: Marco Costalba <hidden>
Date: 2016-06-15 22:44:09

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 <redacted>
---
 sha1_file.c |   71 +++++++++++++++++++++-------------------------------------
 1 files changed, 26 insertions(+), 45 deletions(-)

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

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

From: Marco Costalba <hidden>
Date: 2016-06-15 22:44:09

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 <redacted>
---
 http-push.c   |   21 +++++++++------------
 http-walker.c |   22 +++++++++-------------
 2 files changed, 18 insertions(+), 25 deletions(-)

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

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

From: Marco Costalba <hidden>
Date: 2016-06-15 22:44:09

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

Signed-off-by: Marco Costalba <redacted>
---
 compress.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

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

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

From: Marco Costalba <hidden>
Date: 2016-06-15 22:44:09

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

Signed-off-by: Marco Costalba <redacted>
---
 builtin-apply.c |   23 +++++++++--------------
 index-pack.c    |   30 +++++++-----------------------
 2 files changed, 16 insertions(+), 37 deletions(-)

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

[PATCH 07/11] Introduce stream decompress helpers

From: Marco Costalba <hidden>
Date: 2016-06-15 22:44:09

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 <redacted>
---
 compress.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 compress.h |   17 ++++++++++++-
 2 files changed, 97 insertions(+), 1 deletions(-)

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

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:10

Marco Costalba [off-list ref] 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.

quoted hunk

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.

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:10

Marco Costalba [off-list ref] 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 <redacted>
---
 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.

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:10

Marco Costalba [off-list ref] writes:

quoted hunk

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

Signed-off-by: Marco Costalba <redacted>
---
 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.

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:10

Marco Costalba [off-list ref] writes:

quoted hunk

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 <redacted>
---
 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?

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:10

Marco Costalba [off-list ref] 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.

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

From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:44:10

Marco Costalba [off-list ref] wrote:

quoted hunk

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.

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

From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:44:10

Junio C Hamano [off-list ref] wrote:

Marco Costalba [off-list ref] writes:

quoted

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 <redacted>
---
 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.

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:10

Overall, everything looks quite nicely done.

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

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:10

Marco Costalba [off-list ref] writes:

quoted hunk

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?

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:10

Marco Costalba [off-list ref] writes:

quoted hunk

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.

-	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".

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help