[PATCH/RFC 0/8] Remove various dependencies from wrapper.o

DORMANTno replies

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

[PATCH/RFC 0/8] Remove various dependencies from wrapper.o

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:59 

Hi,

Ram noticed that using any function from strbuf.c results in a larger
binary that requires -lz to link, which is pretty far from ideal.
This patch series moves code around to make that no longer the case.

Vaguely inspired by [1].

After applying these patches, you can write:

	#define SHA1_HEADER "block-sha1/sha1.h"
	#include "cache.h"
	int main(void)
	{
		volatile char *p = xmalloc(5);
		return 0;
	}

and build it with

	cc -Wall -W test.c wrapper.o usage.o

-- no libz needed, no sha1 library, no environment.c, small binary.

Of course, making the linker happy tends to make the code clearer,
too.

Patch 1 moves xmmap to sha1_file.c, where release_pack_memory()
is defined.  If xmmap were used by tiny programs, maybe it would
make sense to replace release_pack_memory() with a callback,
but this is simpler.

Patch 2 moves odb_mkstemp and odb_pack_keep to environment.c,
were they have access to the cached value of $GIT_OBJECT_DIRECTORY.

Patch 3 moves git_mkstemps_mode et al from path.c to wrapper.c,
so xmkstemp_mode does not have to depend on path.c.

Patch 4 moves some refname-related functions from strbuf.c to
sha1_name.c.

Patch 5 moves git_inflate et al to a new zlib.c, so wrapper.o does
not have to depend directly on libz.

Patch 6 is a missing "static" keyword.  It probably doesn't belong
in this series, but it fits okay.

Patch 7 moves try_to_free_builtin to sha1_file.c and adds a
declaration for it to git-compat-util.h.

Patch 8 stops referring to try_to_free_builtin from wrapper.c,
completing the project.

Thoughts?  Improvements?  Benchmarks?

[1] http://thread.gmane.org/gmane.comp.version-control.git/137686/focus=137737

Jonathan Nieder (8):
  wrapper: move xmmap() to sha1_file.c
  wrapper: move odb_* to environment.c
  path helpers: move git_mkstemp* to wrapper.c
  strbuf: move strbuf_branchname to sha1_name.c
  wrapper: give zlib wrappers their own translation unit
  pack-objects: mark file-local variable static
  wrapper: expose try_to_free_pack_memory()
  Remove pack file handling dependency from wrapper.o

 Makefile                                  |    1 +
 builtin/pack-objects.c                    |    2 +-
 check-racy.c                              |    1 +
 contrib/convert-objects/convert-objects.c |    1 +
 daemon.c                                  |    1 +
 environment.c                             |   37 ++++++
 fast-import.c                             |    1 +
 git-compat-util.h                         |    2 +
 git.c                                     |    2 +
 http-backend.c                            |    1 +
 http-fetch.c                              |    1 +
 http-push.c                               |    1 +
 imap-send.c                               |    1 +
 path.c                                    |  113 ----------------
 remote-curl.c                             |    1 +
 sha1_file.c                               |   20 +++
 sha1_name.c                               |   16 +++
 strbuf.c                                  |   16 ---
 test-dump-cache-tree.c                    |    4 +-
 test-match-trees.c                        |    1 +
 upload-pack.c                             |    1 +
 wrapper.c                                 |  198 ++++++++++++++--------------
 zlib.c                                    |   61 +++++++++
 23 files changed, 253 insertions(+), 230 deletions(-)
 create mode 100644 zlib.c

-- 
1.7.2.3.557.gab647.dirty

[PATCH 1/8] wrapper: move xmmap() to sha1_file.c

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:59 

wrapper.o depends on sha1_file.o for a number of reasons.  One is
release_pack_memory().

xmmap function calls mmap, discarding unused pack windows when
necessary to relieve memory pressure.  Simple git programs using
wrapper.o as a friendly libc do not need this functionality.
So move xmmap to sha1_file.o, where release_pack_memory() is.

Signed-off-by: Jonathan Nieder <redacted>
---
 sha1_file.c |   15 +++++++++++++++
 wrapper.c   |   15 ---------------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 0cd9435..8e299ae 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -576,6 +576,21 @@ void release_pack_memory(size_t need, int fd)
 		; /* nothing */
 }
 
+void *xmmap(void *start, size_t length,
+	int prot, int flags, int fd, off_t offset)
+{
+	void *ret = mmap(start, length, prot, flags, fd, offset);
+	if (ret == MAP_FAILED) {
+		if (!length)
+			return NULL;
+		release_pack_memory(length, fd);
+		ret = mmap(start, length, prot, flags, fd, offset);
+		if (ret == MAP_FAILED)
+			die_errno("Out of memory? mmap failed");
+	}
+	return ret;
+}
+
 void close_pack_windows(struct packed_git *p)
 {
 	while (p->windows) {
diff --git a/wrapper.c b/wrapper.c
index fd8ead3..3195ef3 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -108,21 +108,6 @@ void *xcalloc(size_t nmemb, size_t size)
 	return ret;
 }
 
-void *xmmap(void *start, size_t length,
-	int prot, int flags, int fd, off_t offset)
-{
-	void *ret = mmap(start, length, prot, flags, fd, offset);
-	if (ret == MAP_FAILED) {
-		if (!length)
-			return NULL;
-		release_pack_memory(length, fd);
-		ret = mmap(start, length, prot, flags, fd, offset);
-		if (ret == MAP_FAILED)
-			die_errno("Out of memory? mmap failed");
-	}
-	return ret;
-}
-
 /*
  * xread() is the same a read(), but it automatically restarts read()
  * operations with a recoverable error (EAGAIN and EINTR). xread()
-- 
1.7.2.3.557.gab647.dirty

[PATCH 2/8] wrapper: move odb_* to environment.c

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:59 

The odb_mkstemp and odb_pack_keep functions open files under the
$GIT_OBJECT_DIRECTORY directory.  This requires access to the git
configuration which very simple programs do not need.

Move these functions to environment.o, closer to their dependencies.
This should make it easier for programs to link to wrapper.o without
linking to environment.o.

Signed-off-by: Jonathan Nieder <redacted>
---
 environment.c     |   37 +++++++++++++++++++++++++++++++++++++
 git-compat-util.h |    1 +
 wrapper.c         |   37 -------------------------------------
 3 files changed, 38 insertions(+), 37 deletions(-)

diff --git a/environment.c b/environment.c
index de5581f..95777f4 100644
--- a/environment.c
+++ b/environment.c
@@ -171,6 +171,43 @@ char *get_object_directory(void)
 	return git_object_dir;
 }
 
+int odb_mkstemp(char *template, size_t limit, const char *pattern)
+{
+	int fd;
+	/*
+	 * we let the umask do its job, don't try to be more
+	 * restrictive except to remove write permission.
+	 */
+	int mode = 0444;
+	snprintf(template, limit, "%s/%s",
+		 get_object_directory(), pattern);
+	fd = git_mkstemp_mode(template, mode);
+	if (0 <= fd)
+		return fd;
+
+	/* slow path */
+	/* some mkstemp implementations erase template on failure */
+	snprintf(template, limit, "%s/%s",
+		 get_object_directory(), pattern);
+	safe_create_leading_directories(template);
+	return xmkstemp_mode(template, mode);
+}
+
+int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1)
+{
+	int fd;
+
+	snprintf(name, namesz, "%s/pack/pack-%s.keep",
+		 get_object_directory(), sha1_to_hex(sha1));
+	fd = open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
+	if (0 <= fd)
+		return fd;
+
+	/* slow path */
+	safe_create_leading_directories(name);
+	return open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
+}
+
 char *get_index_file(void)
 {
 	if (!git_index_file)
diff --git a/git-compat-util.h b/git-compat-util.h
index 2af8d3e..029162e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -404,6 +404,7 @@ extern ssize_t xwrite(int fd, const void *buf, size_t len);
 extern int xdup(int fd);
 extern FILE *xfdopen(int fd, const char *mode);
 extern int xmkstemp(char *template);
+extern int xmkstemp_mode(char *template, int mode);
 extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
 extern int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1);
 
diff --git a/wrapper.c b/wrapper.c
index 3195ef3..e67b70a 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -274,43 +274,6 @@ int git_inflate(z_streamp strm, int flush)
 	return ret;
 }
 
-int odb_mkstemp(char *template, size_t limit, const char *pattern)
-{
-	int fd;
-	/*
-	 * we let the umask do its job, don't try to be more
-	 * restrictive except to remove write permission.
-	 */
-	int mode = 0444;
-	snprintf(template, limit, "%s/%s",
-		 get_object_directory(), pattern);
-	fd = git_mkstemp_mode(template, mode);
-	if (0 <= fd)
-		return fd;
-
-	/* slow path */
-	/* some mkstemp implementations erase template on failure */
-	snprintf(template, limit, "%s/%s",
-		 get_object_directory(), pattern);
-	safe_create_leading_directories(template);
-	return xmkstemp_mode(template, mode);
-}
-
-int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1)
-{
-	int fd;
-
-	snprintf(name, namesz, "%s/pack/pack-%s.keep",
-		 get_object_directory(), sha1_to_hex(sha1));
-	fd = open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
-	if (0 <= fd)
-		return fd;
-
-	/* slow path */
-	safe_create_leading_directories(name);
-	return open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
-}
-
 static int warn_if_unremovable(const char *op, const char *file, int rc)
 {
 	if (rc < 0) {
-- 
1.7.2.3.557.gab647.dirty

[PATCH 3/8] path helpers: move git_mkstemp* to wrapper.c

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:59 

git_mkstemp_mode and related functions do not require access to
specialized git machinery, unlike some other functions from
path.c (like set_shared_perm()).  Move them to wrapper.c where
the wrapper xmkstemp_mode is defined.

This eliminates a dependency of wrapper.o on environment.o via
path.o.  With typical linkers (e.g., gcc), that dependency makes
programs that use functions from wrapper.o and not environment.o
or path.o larger than they need to be.

Signed-off-by: Jonathan Nieder <redacted>
---
 path.c    |  113 -------------------------------------------------------------
 wrapper.c |  113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+), 113 deletions(-)

diff --git a/path.c b/path.c
index a2c9d1e..8951333 100644
--- a/path.c
+++ b/path.c
@@ -161,119 +161,6 @@ char *git_path_submodule(const char *path, const char *fmt, ...)
 	return cleanup_path(pathname);
 }
 
-/* git_mkstemp() - create tmp file honoring TMPDIR variable */
-int git_mkstemp(char *path, size_t len, const char *template)
-{
-	const char *tmp;
-	size_t n;
-
-	tmp = getenv("TMPDIR");
-	if (!tmp)
-		tmp = "/tmp";
-	n = snprintf(path, len, "%s/%s", tmp, template);
-	if (len <= n) {
-		errno = ENAMETOOLONG;
-		return -1;
-	}
-	return mkstemp(path);
-}
-
-/* git_mkstemps() - create tmp file with suffix honoring TMPDIR variable. */
-int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)
-{
-	const char *tmp;
-	size_t n;
-
-	tmp = getenv("TMPDIR");
-	if (!tmp)
-		tmp = "/tmp";
-	n = snprintf(path, len, "%s/%s", tmp, template);
-	if (len <= n) {
-		errno = ENAMETOOLONG;
-		return -1;
-	}
-	return mkstemps(path, suffix_len);
-}
-
-/* Adapted from libiberty's mkstemp.c. */
-
-#undef TMP_MAX
-#define TMP_MAX 16384
-
-int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
-{
-	static const char letters[] =
-		"abcdefghijklmnopqrstuvwxyz"
-		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-		"0123456789";
-	static const int num_letters = 62;
-	uint64_t value;
-	struct timeval tv;
-	char *template;
-	size_t len;
-	int fd, count;
-
-	len = strlen(pattern);
-
-	if (len < 6 + suffix_len) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	/*
-	 * Replace pattern's XXXXXX characters with randomness.
-	 * Try TMP_MAX different filenames.
-	 */
-	gettimeofday(&tv, NULL);
-	value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();
-	template = &pattern[len - 6 - suffix_len];
-	for (count = 0; count < TMP_MAX; ++count) {
-		uint64_t v = value;
-		/* Fill in the random bits. */
-		template[0] = letters[v % num_letters]; v /= num_letters;
-		template[1] = letters[v % num_letters]; v /= num_letters;
-		template[2] = letters[v % num_letters]; v /= num_letters;
-		template[3] = letters[v % num_letters]; v /= num_letters;
-		template[4] = letters[v % num_letters]; v /= num_letters;
-		template[5] = letters[v % num_letters]; v /= num_letters;
-
-		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
-		if (fd > 0)
-			return fd;
-		/*
-		 * Fatal error (EPERM, ENOSPC etc).
-		 * It doesn't make sense to loop.
-		 */
-		if (errno != EEXIST)
-			break;
-		/*
-		 * This is a random value.  It is only necessary that
-		 * the next TMP_MAX values generated by adding 7777 to
-		 * VALUE are different with (module 2^32).
-		 */
-		value += 7777;
-	}
-	/* We return the null string if we can't find a unique file name.  */
-	pattern[0] = '\0';
-	return -1;
-}
-
-int git_mkstemp_mode(char *pattern, int mode)
-{
-	/* mkstemp is just mkstemps with no suffix */
-	return git_mkstemps_mode(pattern, 0, mode);
-}
-
-int gitmkstemps(char *pattern, int suffix_len)
-{
-	return git_mkstemps_mode(pattern, suffix_len, 0600);
-}
-
 int validate_headref(const char *path)
 {
 	struct stat st;
diff --git a/wrapper.c b/wrapper.c
index e67b70a..b3efefb 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -204,6 +204,119 @@ int xmkstemp(char *template)
 	return fd;
 }
 
+/* git_mkstemp() - create tmp file honoring TMPDIR variable */
+int git_mkstemp(char *path, size_t len, const char *template)
+{
+	const char *tmp;
+	size_t n;
+
+	tmp = getenv("TMPDIR");
+	if (!tmp)
+		tmp = "/tmp";
+	n = snprintf(path, len, "%s/%s", tmp, template);
+	if (len <= n) {
+		errno = ENAMETOOLONG;
+		return -1;
+	}
+	return mkstemp(path);
+}
+
+/* git_mkstemps() - create tmp file with suffix honoring TMPDIR variable. */
+int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)
+{
+	const char *tmp;
+	size_t n;
+
+	tmp = getenv("TMPDIR");
+	if (!tmp)
+		tmp = "/tmp";
+	n = snprintf(path, len, "%s/%s", tmp, template);
+	if (len <= n) {
+		errno = ENAMETOOLONG;
+		return -1;
+	}
+	return mkstemps(path, suffix_len);
+}
+
+/* Adapted from libiberty's mkstemp.c. */
+
+#undef TMP_MAX
+#define TMP_MAX 16384
+
+int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
+{
+	static const char letters[] =
+		"abcdefghijklmnopqrstuvwxyz"
+		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+		"0123456789";
+	static const int num_letters = 62;
+	uint64_t value;
+	struct timeval tv;
+	char *template;
+	size_t len;
+	int fd, count;
+
+	len = strlen(pattern);
+
+	if (len < 6 + suffix_len) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	/*
+	 * Replace pattern's XXXXXX characters with randomness.
+	 * Try TMP_MAX different filenames.
+	 */
+	gettimeofday(&tv, NULL);
+	value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();
+	template = &pattern[len - 6 - suffix_len];
+	for (count = 0; count < TMP_MAX; ++count) {
+		uint64_t v = value;
+		/* Fill in the random bits. */
+		template[0] = letters[v % num_letters]; v /= num_letters;
+		template[1] = letters[v % num_letters]; v /= num_letters;
+		template[2] = letters[v % num_letters]; v /= num_letters;
+		template[3] = letters[v % num_letters]; v /= num_letters;
+		template[4] = letters[v % num_letters]; v /= num_letters;
+		template[5] = letters[v % num_letters]; v /= num_letters;
+
+		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
+		if (fd > 0)
+			return fd;
+		/*
+		 * Fatal error (EPERM, ENOSPC etc).
+		 * It doesn't make sense to loop.
+		 */
+		if (errno != EEXIST)
+			break;
+		/*
+		 * This is a random value.  It is only necessary that
+		 * the next TMP_MAX values generated by adding 7777 to
+		 * VALUE are different with (module 2^32).
+		 */
+		value += 7777;
+	}
+	/* We return the null string if we can't find a unique file name.  */
+	pattern[0] = '\0';
+	return -1;
+}
+
+int git_mkstemp_mode(char *pattern, int mode)
+{
+	/* mkstemp is just mkstemps with no suffix */
+	return git_mkstemps_mode(pattern, 0, mode);
+}
+
+int gitmkstemps(char *pattern, int suffix_len)
+{
+	return git_mkstemps_mode(pattern, suffix_len, 0600);
+}
+
 int xmkstemp_mode(char *template, int mode)
 {
 	int fd;
-- 
1.7.2.3.557.gab647.dirty

[PATCH 4/8] strbuf: move strbuf_branchname to sha1_name.c

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:59 

strbuf_branchname is a thin wrapper around interpret_branch_name
from sha1_name.o.  Most strbuf.o users do not need it.

Signed-off-by: Jonathan Nieder <redacted>
---
 sha1_name.c |   16 ++++++++++++++++
 strbuf.c    |   16 ----------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 484081d..1beebe9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -934,6 +934,22 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 	return len;
 }
 
+int strbuf_branchname(struct strbuf *sb, const char *name)
+{
+	int len = strlen(name);
+	if (interpret_branch_name(name, sb) == len)
+		return 0;
+	strbuf_add(sb, name, len);
+	return len;
+}
+
+int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
+{
+	strbuf_branchname(sb, name);
+	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
+	return check_ref_format(sb->buf);
+}
+
 /*
  * This is like "get_sha1_basic()", except it allows "sha1 expressions",
  * notably "xyz^" for "parent of xyz"
diff --git a/strbuf.c b/strbuf.c
index bc3a080..9b3c445 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -386,19 +386,3 @@ int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
 
 	return len;
 }
-
-int strbuf_branchname(struct strbuf *sb, const char *name)
-{
-	int len = strlen(name);
-	if (interpret_branch_name(name, sb) == len)
-		return 0;
-	strbuf_add(sb, name, len);
-	return len;
-}
-
-int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
-{
-	strbuf_branchname(sb, name);
-	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
-	return check_ref_format(sb->buf);
-}
-- 
1.7.2.3.557.gab647.dirty

[PATCH 5/8] wrapper: give zlib wrappers their own translation unit

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:59 

Programs using xmalloc() but not git_inflate() require -lz on the
linker command line because git_inflate() is in the same translation
unit as xmalloc().

Signed-off-by: Jonathan Nieder <redacted>
---
 Makefile  |    1 +
 wrapper.c |   60 ------------------------------------------------------------
 zlib.c    |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 60 deletions(-)
 create mode 100644 zlib.c

diff --git a/Makefile b/Makefile
index d3dcfb1..45d03f8 100644
--- a/Makefile
+++ b/Makefile
@@ -659,6 +659,7 @@ LIB_OBJS += write_or_die.o
 LIB_OBJS += ws.o
 LIB_OBJS += wt-status.o
 LIB_OBJS += xdiff-interface.o
+LIB_OBJS += zlib.o
 
 BUILTIN_OBJS += builtin/add.o
 BUILTIN_OBJS += builtin/annotate.o
diff --git a/wrapper.c b/wrapper.c
index b3efefb..185dfbc 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -327,66 +327,6 @@ int xmkstemp_mode(char *template, int mode)
 	return fd;
 }
 
-/*
- * zlib wrappers to make sure we don't silently miss errors
- * at init time.
- */
-void git_inflate_init(z_streamp strm)
-{
-	const char *err;
-
-	switch (inflateInit(strm)) {
-	case Z_OK:
-		return;
-
-	case Z_MEM_ERROR:
-		err = "out of memory";
-		break;
-	case Z_VERSION_ERROR:
-		err = "wrong version";
-		break;
-	default:
-		err = "error";
-	}
-	die("inflateInit: %s (%s)", err, strm->msg ? strm->msg : "no message");
-}
-
-void git_inflate_end(z_streamp strm)
-{
-	if (inflateEnd(strm) != Z_OK)
-		error("inflateEnd: %s", strm->msg ? strm->msg : "failed");
-}
-
-int git_inflate(z_streamp strm, int flush)
-{
-	int ret = inflate(strm, flush);
-	const char *err;
-
-	switch (ret) {
-	/* Out of memory is fatal. */
-	case Z_MEM_ERROR:
-		die("inflate: out of memory");
-
-	/* Data corruption errors: we may want to recover from them (fsck) */
-	case Z_NEED_DICT:
-		err = "needs dictionary"; break;
-	case Z_DATA_ERROR:
-		err = "data stream error"; break;
-	case Z_STREAM_ERROR:
-		err = "stream consistency error"; break;
-	default:
-		err = "unknown error"; break;
-
-	/* Z_BUF_ERROR: normal, needs more space in the output buffer */
-	case Z_BUF_ERROR:
-	case Z_OK:
-	case Z_STREAM_END:
-		return ret;
-	}
-	error("inflate: %s (%s)", err, strm->msg ? strm->msg : "no message");
-	return ret;
-}
-
 static int warn_if_unremovable(const char *op, const char *file, int rc)
 {
 	if (rc < 0) {
diff --git a/zlib.c b/zlib.c
new file mode 100644
index 0000000..c4d58da
--- /dev/null
+++ b/zlib.c
@@ -0,0 +1,61 @@
+/*
+ * zlib wrappers to make sure we don't silently miss errors
+ * at init time.
+ */
+#include "cache.h"
+
+void git_inflate_init(z_streamp strm)
+{
+	const char *err;
+
+	switch (inflateInit(strm)) {
+	case Z_OK:
+		return;
+
+	case Z_MEM_ERROR:
+		err = "out of memory";
+		break;
+	case Z_VERSION_ERROR:
+		err = "wrong version";
+		break;
+	default:
+		err = "error";
+	}
+	die("inflateInit: %s (%s)", err, strm->msg ? strm->msg : "no message");
+}
+
+void git_inflate_end(z_streamp strm)
+{
+	if (inflateEnd(strm) != Z_OK)
+		error("inflateEnd: %s", strm->msg ? strm->msg : "failed");
+}
+
+int git_inflate(z_streamp strm, int flush)
+{
+	int ret = inflate(strm, flush);
+	const char *err;
+
+	switch (ret) {
+	/* Out of memory is fatal. */
+	case Z_MEM_ERROR:
+		die("inflate: out of memory");
+
+	/* Data corruption errors: we may want to recover from them (fsck) */
+	case Z_NEED_DICT:
+		err = "needs dictionary"; break;
+	case Z_DATA_ERROR:
+		err = "data stream error"; break;
+	case Z_STREAM_ERROR:
+		err = "stream consistency error"; break;
+	default:
+		err = "unknown error"; break;
+
+	/* Z_BUF_ERROR: normal, needs more space in the output buffer */
+	case Z_BUF_ERROR:
+	case Z_OK:
+	case Z_STREAM_END:
+		return ret;
+	}
+	error("inflate: %s (%s)", err, strm->msg ? strm->msg : "no message");
+	return ret;
+}
-- 
1.7.2.3.557.gab647.dirty

[PATCH 6/8] pack-objects: mark file-local variable static

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:59 

old_try_to_free_routine is not meant for use from other files.

Signed-off-by: Jonathan Nieder <redacted>
---
 builtin/pack-objects.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f8eba53..7471c92 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1529,7 +1529,7 @@ static void try_to_free_from_threads(size_t size)
 	read_unlock();
 }
 
-try_to_free_t old_try_to_free_routine;
+static try_to_free_t old_try_to_free_routine;
 
 /*
  * The main thread waits on the condition that (at least) one of the workers
-- 
1.7.2.3.557.gab647.dirty

[PATCH 7/8] wrapper: expose try_to_free_pack_memory()

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:59 

As v1.7.0-rc0~43 (slim down "git show-index", 2010-01-21) explains,
xmalloc() is dependent on zlib, the sha1 lib, and the rest of git's
object file access machinery via try_to_free_pack_memory.  That is
overkill when xmalloc() is only being used to print a message and exit
on memory exhaustion.

So let each caller decide whether the "discard pack windows" logic is
needed.  Programs using the sha1_file library should request that
xmalloc discard stale windows when an allocation fails, by calling

	set_try_to_free_handler(try_to_free_pack_memory);

A later patch will stop setting up that handler except in response to
such explicit requests, allowing wrapper.o to be used without adding a
sha1_file.o dependency by programs that do not access the object db.

Signed-off-by: Jonathan Nieder <redacted>
---
 git-compat-util.h |    1 +
 sha1_file.c       |    5 +++++
 wrapper.c         |    7 +------
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 029162e..07ee75e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -387,6 +387,7 @@ static inline void *gitmempcpy(void *dest, const void *src, size_t n)
 #endif
 
 extern void release_pack_memory(size_t, int);
+extern void try_to_free_pack_memory(size_t size);
 
 typedef void (*try_to_free_t)(size_t);
 extern try_to_free_t set_try_to_free_routine(try_to_free_t);
diff --git a/sha1_file.c b/sha1_file.c
index 8e299ae..c48baaa 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -576,6 +576,11 @@ void release_pack_memory(size_t need, int fd)
 		; /* nothing */
 }
 
+void try_to_free_pack_memory(size_t size)
+{
+	release_pack_memory(size, -1);
+}
+
 void *xmmap(void *start, size_t length,
 	int prot, int flags, int fd, off_t offset)
 {
diff --git a/wrapper.c b/wrapper.c
index 185dfbc..6c6579b 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -3,12 +3,7 @@
  */
 #include "cache.h"
 
-static void try_to_free_builtin(size_t size)
-{
-	release_pack_memory(size, -1);
-}
-
-static void (*try_to_free_routine)(size_t size) = try_to_free_builtin;
+static void (*try_to_free_routine)(size_t size) = try_to_free_pack_memory;
 
 try_to_free_t set_try_to_free_routine(try_to_free_t routine)
 {
-- 
1.7.2.3.557.gab647.dirty

[PATCH 8/8] Remove pack file handling dependency from wrapper.o

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:59 

Commands that might access a git repository should discard pack
windows when memory is tight, but helpers like show-index do not need
to.

So stop setting try_to_free_pack_memory as the default
try_to_free_routine and instead set up the try_to_free handler
explicitly in main() for callers that require it.

After this change, a simple program using xmalloc() and no other
functions will not pull in any code from libgit.a aside from wrapper.o
and usage.o.

Signed-off-by: Jonathan Nieder <redacted>
---
Thanks for reading.  I hope it was not too dull.

Hopefully this will make svn-fe small again (it got big when the
vcs-svn lib started using strbufs).

Good night,
Jonathan

 check-racy.c                              |    1 +
 contrib/convert-objects/convert-objects.c |    1 +
 daemon.c                                  |    1 +
 fast-import.c                             |    1 +
 git.c                                     |    2 ++
 http-backend.c                            |    1 +
 http-fetch.c                              |    1 +
 http-push.c                               |    1 +
 imap-send.c                               |    1 +
 remote-curl.c                             |    1 +
 test-dump-cache-tree.c                    |    4 +++-
 test-match-trees.c                        |    1 +
 upload-pack.c                             |    1 +
 wrapper.c                                 |    6 +++++-
 14 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/check-racy.c b/check-racy.c
index 00d92a1..d4b2557 100644
--- a/check-racy.c
+++ b/check-racy.c
@@ -6,6 +6,7 @@ int main(int ac, char **av)
 	int dirty, clean, racy;
 
 	dirty = clean = racy = 0;
+	set_try_to_free_routine(try_to_free_pack_memory);
 	read_cache();
 	for (i = 0; i < active_nr; i++) {
 		struct cache_entry *ce = active_cache[i];
diff --git a/contrib/convert-objects/convert-objects.c b/contrib/convert-objects/convert-objects.c
index f3b57bf..983b917 100644
--- a/contrib/convert-objects/convert-objects.c
+++ b/contrib/convert-objects/convert-objects.c
@@ -317,6 +317,7 @@ int main(int argc, char **argv)
 	struct entry *entry;
 
 	setup_git_directory();
+	set_try_to_free_routine(try_to_free_pack_memory);
 
 	if (argc != 2)
 		usage("git-convert-objects <sha1>");
diff --git a/daemon.c b/daemon.c
index 9326d3a..ccf8960 100644
--- a/daemon.c
+++ b/daemon.c
@@ -976,6 +976,7 @@ int main(int argc, char **argv)
 	int i;
 
 	git_extract_argv0_path(argv[0]);
+	set_try_to_free_routine(try_to_free_pack_memory);
 
 	for (i = 1; i < argc; i++) {
 		char *arg = argv[i];
diff --git a/fast-import.c b/fast-import.c
index eab68d5..ccf81b1 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2917,6 +2917,7 @@ int main(int argc, const char **argv)
 	unsigned int i;
 
 	git_extract_argv0_path(argv[0]);
+	set_try_to_free_routine(try_to_free_pack_memory);
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(fast_import_usage);
diff --git a/git.c b/git.c
index 0409ac9..6386404 100644
--- a/git.c
+++ b/git.c
@@ -272,6 +272,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 
 	trace_argv_printf(argv, "trace: built-in: git");
 
+	set_try_to_free_routine(try_to_free_pack_memory);
+
 	status = p->fn(argc, argv, prefix);
 	if (status)
 		return status;
diff --git a/http-backend.c b/http-backend.c
index 14c90c2..7d02e55 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -551,6 +551,7 @@ int main(int argc, char **argv)
 	int i;
 
 	git_extract_argv0_path(argv[0]);
+	set_try_to_free_routine(try_to_free_pack_memory);
 	set_die_routine(die_webcgi);
 
 	if (!method)
diff --git a/http-fetch.c b/http-fetch.c
index 762c750..98cefc7 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -25,6 +25,7 @@ int main(int argc, const char **argv)
 	int get_recover = 0;
 
 	git_extract_argv0_path(argv[0]);
+	set_try_to_free_routine(try_to_free_pack_memory);
 
 	while (arg < argc && argv[arg][0] == '-') {
 		if (argv[arg][1] == 't') {
diff --git a/http-push.c b/http-push.c
index c9bcd11..fc25aeb 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1792,6 +1792,7 @@ int main(int argc, char **argv)
 	char *rewritten_url = NULL;
 
 	git_extract_argv0_path(argv[0]);
+	set_try_to_free_routine(try_to_free_pack_memory);
 
 	repo = xcalloc(sizeof(*repo), 1);
 
diff --git a/imap-send.c b/imap-send.c
index 71506a8..8056144 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1538,6 +1538,7 @@ int main(int argc, char **argv)
 	int nongit_ok;
 
 	git_extract_argv0_path(argv[0]);
+	set_try_to_free_routine(try_to_free_pack_memory);
 
 	if (argc != 1)
 		usage(imap_send_usage);
diff --git a/remote-curl.c b/remote-curl.c
index 04d4813..4c2b03a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -788,6 +788,7 @@ int main(int argc, const char **argv)
 	int nongit;
 
 	git_extract_argv0_path(argv[0]);
+	set_try_to_free_routine(try_to_free_pack_memory);
 	setup_git_directory_gently(&nongit);
 	if (argc < 2) {
 		fprintf(stderr, "Remote needed\n");
diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
index 1f73f1e..a6faa89 100644
--- a/test-dump-cache-tree.c
+++ b/test-dump-cache-tree.c
@@ -56,7 +56,9 @@ static int dump_cache_tree(struct cache_tree *it,
 
 int main(int ac, char **av)
 {
-	struct cache_tree *another = cache_tree();
+	struct cache_tree *another;
+	set_try_to_free_routine(try_to_free_pack_memory);
+	another = cache_tree();
 	if (read_cache() < 0)
 		die("unable to read index file");
 	cache_tree_update(another, active_cache, active_nr, 0, 1);
diff --git a/test-match-trees.c b/test-match-trees.c
index a3c4688..c543fee 100644
--- a/test-match-trees.c
+++ b/test-match-trees.c
@@ -6,6 +6,7 @@ int main(int ac, char **av)
 	unsigned char hash1[20], hash2[20], shifted[20];
 	struct tree *one, *two;
 
+	set_try_to_free_routine(try_to_free_pack_memory);
 	if (get_sha1(av[1], hash1))
 		die("cannot parse %s as an object name", av[1]);
 	if (get_sha1(av[2], hash2))
diff --git a/upload-pack.c b/upload-pack.c
index f05e422..d654e8b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -683,6 +683,7 @@ int main(int argc, char **argv)
 	int strict = 0;
 
 	git_extract_argv0_path(argv[0]);
+	set_try_to_free_routine(try_to_free_pack_memory);
 	read_replace_refs = 0;
 
 	for (i = 1; i < argc; i++) {
diff --git a/wrapper.c b/wrapper.c
index 6c6579b..4c1639f 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -3,7 +3,11 @@
  */
 #include "cache.h"
 
-static void (*try_to_free_routine)(size_t size) = try_to_free_pack_memory;
+static void do_nothing(size_t size)
+{
+}
+
+static void (*try_to_free_routine)(size_t size) = do_nothing;
 
 try_to_free_t set_try_to_free_routine(try_to_free_t routine)
 {
-- 
1.7.2.3.557.gab647.dirty

Re: [PATCH 8/8] Remove pack file handling dependency from wrapper.o

From: René Scharfe <hidden>
Date: 2016-06-15 22:49:59 

Am 06.11.2010 12:52, schrieb Jonathan Nieder:
Commands that might access a git repository should discard pack
windows when memory is tight, but helpers like show-index do not need
to.

So stop setting try_to_free_pack_memory as the default
try_to_free_routine and instead set up the try_to_free handler
explicitly in main() for callers that require it.
Ugh.  Having to remember setting this handler is tedious.

Can it be set automatically once the first pack is loaded?  A quick look
suggests that use_pack() would be the right place to do it.

René

Re: [PATCH 8/8] Remove pack file handling dependency from wrapper.o

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:59 

René Scharfe wrote:

Ugh.  Having to remember setting this handler is tedious.

Can it be set automatically once the first pack is loaded?  A quick look
suggests that use_pack() would be the right place to do it.
Maybe add_packed_git()?

use_pack() is called by:

 pack-objects::check_pack_inflate
 pack-objects::copy_pack_data
 pack-objects::check_object
 check_pack_crc
 verify_pack
 get_size_from_delta
 sha1_file::get_delta_base
 sha1_file::unpack_object_header
 packed_object_info_detail
 sha1_file::unpack_compressed_entry

some of which would make me worry about thread-safety.

Re: [PATCH 8/8] Remove pack file handling dependency from wrapper.o

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:59 

René Scharfe wrote:

Can it be set automatically once the first pack is loaded?  A quick look
suggests that use_pack() would be the right place to do it.
How about this?  Replaces patches 7 and 8.

--8 <--
Subject: Remove pack file handling dependency from wrapper.o

As v1.7.0-rc0~43 (slim down "git show-index", 2010-01-21) explains,
use of xmalloc() brings in a dependency on zlib, the sha1 lib, and the
rest of git's object file access machinery via try_to_free_pack_memory.
That is overkill when xmalloc is just being used as a convenience
wrapper to exit when no memory is available.

So defer setting try_to_free_pack_memory as try_to_free_routine until
the first packfile is opened in add_packed_git().

After this change, a simple program using xmalloc() and no other
functions will not pull in any code from libgit.a aside from wrapper.o
and usage.o.

Improved-by: René Scharfe [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
 sha1_file.c |   11 +++++++++++
 wrapper.c   |    5 ++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 8e299ae..e0d2496 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -818,11 +818,22 @@ static struct packed_git *alloc_packed_git(int extra)
 	return p;
 }
 
+static void try_to_free_pack_memory(size_t size)
+{
+	release_pack_memory(size, -1);
+}
+
 struct packed_git *add_packed_git(const char *path, int path_len, int local)
 {
+	static int have_set_try_to_free_routine;
 	struct stat st;
 	struct packed_git *p = alloc_packed_git(path_len + 2);
 
+	if (!have_set_try_to_free_routine) {
+		have_set_try_to_free_routine = 1;
+		set_try_to_free_routine(try_to_free_pack_memory);
+	}
+
 	/*
 	 * Make sure a corresponding .pack file exists and that
 	 * the index looks sane.
diff --git a/wrapper.c b/wrapper.c
index 185dfbc..4c1639f 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -3,12 +3,11 @@
  */
 #include "cache.h"
 
-static void try_to_free_builtin(size_t size)
+static void do_nothing(size_t size)
 {
-	release_pack_memory(size, -1);
 }
 
-static void (*try_to_free_routine)(size_t size) = try_to_free_builtin;
+static void (*try_to_free_routine)(size_t size) = do_nothing;
 
 try_to_free_t set_try_to_free_routine(try_to_free_t routine)
 {
-- 
1.7.2.3.557.gab647.dirty

Re: [PATCH 8/8] Remove pack file handling dependency from wrapper.o

From: René Scharfe <hidden>
Date: 2016-06-15 22:49:59 

Am 06.11.2010 19:42, schrieb Jonathan Nieder:
René Scharfe wrote:
quoted
Ugh.  Having to remember setting this handler is tedious.

Can it be set automatically once the first pack is loaded?  A quick look
suggests that use_pack() would be the right place to do it.
Maybe add_packed_git()?

use_pack() is called by:

 pack-objects::check_pack_inflate
 pack-objects::copy_pack_data
 pack-objects::check_object
 check_pack_crc
 verify_pack
 get_size_from_delta
 sha1_file::get_delta_base
 sha1_file::unpack_object_header
 packed_object_info_detail
 sha1_file::unpack_compressed_entry

some of which would make me worry about thread-safety.
Possibly, but if that's the case then we have a thread-safety issue
already: use_pack() updates pack_mapped, which is used by
release_pack_memory() to see how much can be freed.  Just saying.

René

Re: [PATCH 8/8] Remove pack file handling dependency from wrapper.o

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:59 

René Scharfe wrote:
Am 06.11.2010 19:42, schrieb Jonathan Nieder:
quoted
Maybe add_packed_git()?

use_pack() is called by:
[...]
quoted
some of which would make me worry about thread-safety.
Possibly, but if that's the case then we have a thread-safety issue
already: use_pack() updates pack_mapped, which is used by
release_pack_memory() to see how much can be freed.
Good point.  The threaded callers are probably protected by read_lock.

My other worry would be that try_to_free_pack_memory should not
override try_to_free_from_threads.  I guess some time documenting
these code paths will be needed. :(

Thanks for a sanity check,
Jonathan




    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help