[PATCH 2/7] Switch git_mmap to use pread.

[PATCH 2/7] Switch git_mmap to use pread.

[Shawn O. Pearce]

Now that Git depends on pread in index-pack its safe to say we can
also depend on it within the git_mmap emulation we activate when
NO_MMAP is set.  On most systems pread should be slightly faster
than an lseek/read/lseek sequence as its one system call vs. three
system calls.

We also now honor EAGAIN and EINTR error codes from pread and
restart the prior read.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 compat/mmap.c |   17 ++++-------------
 1 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/compat/mmap.c b/compat/mmap.c
index bb34c7e..98056f0 100644
--- a/compat/mmap.c
+++ b/compat/mmap.c
@@ -2,17 +2,11 @@
 
 void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset)
 {
-	int n = 0;
-	off_t current_offset = lseek(fd, 0, SEEK_CUR);
+	size_t n = 0;
 
 	if (start != NULL || !(flags & MAP_PRIVATE))
 		die("Invalid usage of mmap when built with NO_MMAP");
 
-	if (lseek(fd, offset, SEEK_SET) < 0) {
-		errno = EINVAL;
-		return MAP_FAILED;
-	}
-
 	start = xmalloc(length);
 	if (start == NULL) {
 		errno = ENOMEM;
@@ -20,7 +14,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
 	}
 
 	while (n < length) {
-		int count = read(fd, start+n, length-n);
+		ssize_t count = pread(fd, start + n, length - n, offset + n);
 
 		if (count == 0) {
 			memset(start+n, 0, length-n);
@@ -28,6 +22,8 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
 		}
 
 		if (count < 0) {
+			if (errno == EAGAIN || errno == EINTR)
+				continue;
 			free(start);
 			errno = EACCES;
 			return MAP_FAILED;
@@ -36,11 +32,6 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
 		n += count;
 	}
 
-	if (current_offset != lseek(fd, current_offset, SEEK_SET)) {
-		errno = EINVAL;
-		return MAP_FAILED;
-	}
-
 	return start;
 }
 
-- 
1.4.4.3.g2e63

Re: [PATCH 2/7] Switch git_mmap to use pread.

[Johannes Schindelin]

Hi,

On Sun, 24 Dec 2006, Shawn O. Pearce wrote:

> Now that Git depends on pread in index-pack its safe to say we can
> also depend on it within the git_mmap emulation we activate when
> NO_MMAP is set.  On most systems pread should be slightly faster
> than an lseek/read/lseek sequence as its one system call vs. three
> system calls.

I don't think it matters much. The _only_ platform we really use NO_MMAP 
(other than for testing) is Windows, and AFAICT it does not have pread(), 
so it is emulated by lseek/read/lseek anyway.

But it's a cleanup, and it deletes more lines than it adds, so Ack from 
me.

Ciao,
Dscho

Re: [PATCH 2/7] Switch git_mmap to use pread.

[Alon Ziv]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

On Sun, 2006-12-24 at 14:09 +0100, Johannes Schindelin wrote:
> I don't think it matters much. The _only_ platform we really use NO_MMAP 
> (other than for testing) is Windows, and AFAICT it does not have pread(), 
> so it is emulated by lseek/read/lseek anyway.
> 

Well, Win32 does have a pread()-like behavior in its generic ReadFile()
system call (using a curiously-named "lpOverlapped" parameter).  I would
have expected Cygwin to use it, but unfortunately from the CVS sources
it appears not to :(

	-az

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2156 bytes --]

Re: [PATCH 2/7] Switch git_mmap to use pread.

[Shawn Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Sun, 24 Dec 2006, Shawn O. Pearce wrote:
> > Now that Git depends on pread in index-pack its safe to say we can
> > also depend on it within the git_mmap emulation we activate when
> > NO_MMAP is set.  On most systems pread should be slightly faster
> > than an lseek/read/lseek sequence as its one system call vs. three
> > system calls.
> 
> I don't think it matters much. The _only_ platform we really use NO_MMAP 
> (other than for testing) is Windows, and AFAICT it does not have pread(), 
> so it is emulated by lseek/read/lseek anyway.
> 
> But it's a cleanup, and it deletes more lines than it adds, so Ack from 
> me.

Right - that's why I did it.  I was already in there doing other work
and discovered that we had a lot of lines just to avoid using pread.
Initially that may have been because we were trying to avoid using
it, but now that one of our more important tools (index-pack)
depends on it... ;-)

I'm actually thinking of doing a NO_PACK_MMAP, especially for systems
like Mac OS X where the mmap() of small chunks appears to perform
so poorly.  Setting NO_PACK_MMAP and just using smaller windows
(1 MiB) might be worthwhile.  Though in that case we may just want
to compile with NO_MMAP.

-- 
Shawn.

Re: [PATCH 2/7] Switch git_mmap to use pread.

[Linus Torvalds]

On Sun, 24 Dec 2006, Johannes Schindelin wrote:
> 
> I don't think it matters much. The _only_ platform we really use NO_MMAP 
> (other than for testing) is Windows, and AFAICT it does not have pread(), 
> so it is emulated by lseek/read/lseek anyway.

Windows definitely has pread(). It is, of course, possible that Cygwin 
emulates it some other way (including doing a "lseek/read/lseek" 
combination), but pread() should still be better - because maybe some 
future cygwin release ends up using the native interfaces.

		Linus

[PATCH 3/7] Ensure packed_git.next is initialized to NULL.

[Shawn O. Pearce]

Junio noticed while reviewing this patch series that I removed the
initialization of packed_git.next = NULL in 88078baa.  That removal
was not intended so I'm restoring the initialization where necessary.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 sha1_file.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 1a87f95..8de8ce0 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -676,6 +676,7 @@ struct packed_git *add_packed_git(char *path, int path_len, int local)
 	p->index_size = idx_size;
 	p->pack_size = st.st_size;
 	p->index_base = idx_map;
+	p->next = NULL;
 	p->windows = NULL;
 	p->pack_fd = -1;
 	p->pack_local = local;
@@ -707,6 +708,7 @@ struct packed_git *parse_pack_index_file(const unsigned char *sha1, char *idx_pa
 	p->index_size = idx_size;
 	p->pack_size = 0;
 	p->index_base = idx_map;
+	p->next = NULL;
 	p->windows = NULL;
 	p->pack_fd = -1;
 	hashcpy(p->sha1, sha1);
-- 
1.4.4.3.g2e63

[PATCH 4/7] Default core.packdGitWindowSize to 1 MiB if NO_MMAP.

[Shawn O. Pearce]

If the compiler has asked us to disable use of mmap() on their
platform then we are forced to use git_mmap and its emulation via
pread.  In this case large (e.g. 32 MiB) windows for pack access
are simply too big as a command will wind up reading a lot more
data than it will ever need, significantly reducing response time.

To prevent a high latency when NO_MMAP has been selected we now
use a default of 1 MiB for core.packedGitWindowSize.  Credit goes
to Linus and Junio for recommending this more reasonable setting.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 environment.c     |    2 +-
 git-compat-util.h |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/environment.c b/environment.c
index 289fc84..e89aab4 100644
--- a/environment.c
+++ b/environment.c
@@ -22,7 +22,7 @@ char git_commit_encoding[MAX_ENCODING_LENGTH] = "utf-8";
 int shared_repository = PERM_UMASK;
 const char *apply_default_whitespace;
 int zlib_compression_level = Z_DEFAULT_COMPRESSION;
-size_t packed_git_window_size = 32 * 1024 * 1024;
+size_t packed_git_window_size = DEFAULT_packed_git_window_size;
 size_t packed_git_limit = 256 * 1024 * 1024;
 int pager_in_use;
 int pager_use_color = 1;
diff --git a/git-compat-util.h b/git-compat-util.h
index 5d9eb26..e056339 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -87,6 +87,7 @@ extern void set_warn_routine(void (*routine)(const char *warn, va_list params));
 #define MAP_FAILED ((void*)-1)
 #endif
 
+#define DEFAULT_packed_git_window_size (1 * 1024 * 1024)
 #define mmap git_mmap
 #define munmap git_munmap
 extern void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
@@ -95,6 +96,7 @@ extern int git_munmap(void *start, size_t length);
 #else /* NO_MMAP */
 
 #include <sys/mman.h>
+#define DEFAULT_packed_git_window_size (32 * 1024 * 1024)
 
 #endif /* NO_MMAP */
 
-- 
1.4.4.3.g2e63

[PATCH 5/7] Don't exit successfully on EPIPE in read_or_die.

[Shawn O. Pearce]

Junio pointed out to me that read() won't return EPIPE like write()
does, so there is no reason for us to exit successfully if we get
an EPIPE error while in read_or_die.

Instead we should just die if we get any error from read() while
in read_or_die as there is no reasonable recovery.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 write_or_die.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/write_or_die.c b/write_or_die.c
index a56d992..8cf6486 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -9,11 +9,8 @@ void read_or_die(int fd, void *buf, size_t count)
 		loaded = xread(fd, p, count);
 		if (loaded == 0)
 			die("unexpected end of file");
-		else if (loaded < 0) {
-			if (errno == EPIPE)
-				exit(0);
+		else if (loaded < 0)
 			die("read error (%s)", strerror(errno));
-		}
 		count -= loaded;
 		p += loaded;
 	}
-- 
1.4.4.3.g2e63

[PATCH 6/7] Release pack windows before reporting out of memory.

[Shawn O. Pearce]

If we are about to fail because this process has run out of memory we
should first try to automatically control our appetite for address
space by releasing enough least-recently-used pack windows to gain
back enough memory such that we might actually be able to meet the
current allocation request.

This should help users who have fairly large repositories but are
working on systems with relatively small virtual address space.
Many times we see reports on the mailing list of these users running
out of memory during various Git operations.  Dynamically decreasing
the amount of pack memory used when the demand for heap memory is
increasing is an intelligent solution to this problem.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 git-compat-util.h |   40 ++++++++++++++++++++++++++++++++--------
 sha1_file.c       |    7 +++++++
 2 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index e056339..4a417be 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -120,11 +120,17 @@ extern char *gitstrcasestr(const char *haystack, const char *needle);
 extern size_t gitstrlcpy(char *, const char *, size_t);
 #endif
 
+extern void release_pack_memory(size_t);
+
 static inline char* xstrdup(const char *str)
 {
 	char *ret = strdup(str);
-	if (!ret)
-		die("Out of memory, strdup failed");
+	if (!ret) {
+		release_pack_memory(strlen(str) + 1);
+		ret = strdup(str);
+		if (!ret)
+			die("Out of memory, strdup failed");
+	}
 	return ret;
 }
 
@@ -133,8 +139,14 @@ static inline void *xmalloc(size_t size)
 	void *ret = malloc(size);
 	if (!ret && !size)
 		ret = malloc(1);
-	if (!ret)
-		die("Out of memory, malloc failed");
+	if (!ret) {
+		release_pack_memory(size);
+		ret = malloc(size);
+		if (!ret && !size)
+			ret = malloc(1);
+		if (!ret)
+			die("Out of memory, malloc failed");
+	}
 #ifdef XMALLOC_POISON
 	memset(ret, 0xA5, size);
 #endif
@@ -146,8 +158,14 @@ static inline void *xrealloc(void *ptr, size_t size)
 	void *ret = realloc(ptr, size);
 	if (!ret && !size)
 		ret = realloc(ptr, 1);
-	if (!ret)
-		die("Out of memory, realloc failed");
+	if (!ret) {
+		release_pack_memory(size);
+		ret = realloc(ptr, size);
+		if (!ret && !size)
+			ret = realloc(ptr, 1);
+		if (!ret)
+			die("Out of memory, realloc failed");
+	}
 	return ret;
 }
 
@@ -156,8 +174,14 @@ static inline void *xcalloc(size_t nmemb, size_t size)
 	void *ret = calloc(nmemb, size);
 	if (!ret && (!nmemb || !size))
 		ret = calloc(1, 1);
-	if (!ret)
-		die("Out of memory, calloc failed");
+	if (!ret) {
+		release_pack_memory(nmemb * size);
+		ret = calloc(nmemb, size);
+		if (!ret && (!nmemb || !size))
+			ret = calloc(1, 1);
+		if (!ret)
+			die("Out of memory, calloc failed");
+	}
 	return ret;
 }
 
diff --git a/sha1_file.c b/sha1_file.c
index 8de8ce0..fb1032b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -522,6 +522,13 @@ static int unuse_one_window(struct packed_git *current)
 	return 0;
 }
 
+void release_pack_memory(size_t need)
+{
+	size_t cur = pack_mapped;
+	while (need >= (cur - pack_mapped) && unuse_one_window(NULL))
+		; /* nothing */
+}
+
 void unuse_pack(struct pack_window **w_cursor)
 {
 	struct pack_window *w = *w_cursor;
-- 
1.4.4.3.g2e63

Re: [PATCH 6/7] Release pack windows before reporting out of memory.

[Johannes Schindelin]

Hi,

a very cute idea that having x*alloc() in place you can just pluck in some 
garbage collection. I like it.

Ciao,
Dscho

[PATCH 7/7] Replace mmap with xmmap, better handling MAP_FAILED.

[Shawn O. Pearce]

In some cases we did not even bother to check the return value of
mmap() and just assume it worked.  This is bad, because if we are
out of virtual address space the kernel returned MAP_FAILED and we
would attempt to dereference that address, segfaulting without any
real error output to the user.

We are replacing all calls to mmap() with xmmap() and moving all
MAP_FAILED checking into that single location.  If a mmap call
fails we try to release enough least-recently-used pack windows
to possibly succeed, then retry the mmap() attempt.  If we cannot
mmap even after releasing pack memory then we die() as none of our
callers have any reasonable recovery strategy for a failed mmap.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 config.c          |    2 +-
 diff.c            |    4 +---
 git-compat-util.h |   13 +++++++++++++
 read-cache.c      |    2 +-
 refs.c            |    2 +-
 sha1_file.c       |   18 +++++-------------
 6 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/config.c b/config.c
index edc42f4..410d5e9 100644
--- a/config.c
+++ b/config.c
@@ -698,7 +698,7 @@ int git_config_set_multivar(const char* key, const char* value,
 		}
 
 		fstat(in_fd, &st);
-		contents = mmap(NULL, st.st_size, PROT_READ,
+		contents = xmmap(NULL, st.st_size, PROT_READ,
 			MAP_PRIVATE, in_fd, 0);
 		close(in_fd);
 
diff --git a/diff.c b/diff.c
index f14288b..244292a 100644
--- a/diff.c
+++ b/diff.c
@@ -1341,10 +1341,8 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		fd = open(s->path, O_RDONLY);
 		if (fd < 0)
 			goto err_empty;
-		s->data = mmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
+		s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
 		close(fd);
-		if (s->data == MAP_FAILED)
-			goto err_empty;
 		s->should_munmap = 1;
 	}
 	else {
diff --git a/git-compat-util.h b/git-compat-util.h
index 4a417be..51e8d7a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -185,6 +185,19 @@ static inline void *xcalloc(size_t nmemb, size_t size)
 	return ret;
 }
 
+static inline 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) {
+		release_pack_memory(length);
+		ret = mmap(start, length, prot, flags, fd, offset);
+		if (ret == MAP_FAILED)
+			die("Out of memory? mmap failed: %s", strerror(errno));
+	}
+	return ret;
+}
+
 static inline ssize_t xread(int fd, void *buf, size_t len)
 {
 	ssize_t nr;
diff --git a/read-cache.c b/read-cache.c
index b8d83cc..ca3efbb 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -798,7 +798,7 @@ int read_cache_from(const char *path)
 		cache_mmap_size = st.st_size;
 		errno = EINVAL;
 		if (cache_mmap_size >= sizeof(struct cache_header) + 20)
-			cache_mmap = mmap(NULL, cache_mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+			cache_mmap = xmmap(NULL, cache_mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
 	}
 	close(fd);
 	if (cache_mmap == MAP_FAILED)
diff --git a/refs.c b/refs.c
index a101ff3..286ae45 100644
--- a/refs.c
+++ b/refs.c
@@ -1025,7 +1025,7 @@ int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *
 	fstat(logfd, &st);
 	if (!st.st_size)
 		die("Log %s is empty.", logfile);
-	logdata = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, logfd, 0);
+	logdata = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, logfd, 0);
 	close(logfd);
 
 	lastrec = NULL;
diff --git a/sha1_file.c b/sha1_file.c
index fb1032b..84037fe 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -355,10 +355,8 @@ static void read_info_alternates(const char * relative_base, int depth)
 		close(fd);
 		return;
 	}
-	map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	map = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if (map == MAP_FAILED)
-		return;
 
 	link_alt_odb_entries(map, map + st.st_size, '\n', relative_base, depth);
 
@@ -442,10 +440,8 @@ static int check_packed_git_idx(const char *path, unsigned long *idx_size_,
 		return -1;
 	}
 	idx_size = st.st_size;
-	idx_map = mmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	idx_map = xmmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if (idx_map == MAP_FAILED)
-		return -1;
 
 	index = idx_map;
 	*idx_map_ = idx_map;
@@ -630,7 +626,7 @@ unsigned char* use_pack(struct packed_git *p,
 			while (packed_git_limit < pack_mapped
 				&& unuse_one_window(p))
 				; /* nothing */
-			win->base = mmap(NULL, win->len,
+			win->base = xmmap(NULL, win->len,
 				PROT_READ, MAP_PRIVATE,
 				p->pack_fd, win->offset);
 			if (win->base == MAP_FAILED)
@@ -828,10 +824,8 @@ void *map_sha1_file(const unsigned char *sha1, unsigned long *size)
 		 */
 		sha1_file_open_flag = 0;
 	}
-	map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	map = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if (map == MAP_FAILED)
-		return NULL;
 	*size = st.st_size;
 	return map;
 }
@@ -1987,10 +1981,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, con
 
 	buf = "";
 	if (size)
-		buf = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
+		buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if (buf == MAP_FAILED)
-		return -1;
 
 	if (!type)
 		type = blob_type;
-- 
1.4.4.3.g2e63

Re: [PATCH 7/7] Replace mmap with xmmap, better handling MAP_FAILED.

[Johannes Schindelin]

Hi,

On Sun, 24 Dec 2006, Shawn O. Pearce wrote:

> diff --git a/diff.c b/diff.c
> index f14288b..244292a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1341,10 +1341,8 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
>  		fd = open(s->path, O_RDONLY);
>  		if (fd < 0)
>  			goto err_empty;
> -		s->data = mmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
> +		s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
>  		close(fd);
> -		if (s->data == MAP_FAILED)
> -			goto err_empty;

The only gripe I have here is that the old code could actually say where 
the problem occurred ("cannot read data blob for <blabla>"), but that 
probably does not matter so much, now that we can hardly run out of memory 
on a decent machine, even using big packfiles.

> diff --git a/read-cache.c b/read-cache.c
> index b8d83cc..ca3efbb 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -798,7 +798,7 @@ int read_cache_from(const char *path)
>  		cache_mmap_size = st.st_size;
>  		errno = EINVAL;
>  		if (cache_mmap_size >= sizeof(struct cache_header) + 20)
> -			cache_mmap = mmap(NULL, cache_mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
> +			cache_mmap = xmmap(NULL, cache_mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
>  	}
>  	close(fd);
>  	if (cache_mmap == MAP_FAILED)

This MAP_FAILED no longer has anything to do with MAP_FAILED, but rather 
with fstat failed, so you probably want to move that into an else 
construct just before "close(fd);".

All in all it is a good change -- for the builtin programs.

But it is less good for the libification. Maybe it is time for a 
discussion about the possible strategies to avoid dying in libgit.a?

Ciao,
Dscho

Re: [PATCH 7/7] Replace mmap with xmmap, better handling MAP_FAILED.

[Shawn Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> All in all it is a good change -- for the builtin programs.
> 
> But it is less good for the libification. Maybe it is time for a 
> discussion about the possible strategies to avoid dying in libgit.a?

Well we have the same problem with xmalloc.  All I've done is move
the MAP_FAILED cases which tend to wind up die()'ing later anyway
into the same scope of area where the xmalloc issue is.

We die() all over the place.  ~1312 times according to 'git grep die'.
Git isn't a program for the living.  :-)

To properly libify we have a few issues:

  - we cannot just exit this process when we run into an error;

  - routines need to cleanup temporary resources (memory, file
    descriptors) when returning an error;

  - static variables like environment.c need to be reorganized to
    support multiple repositories in the same program;

  - objects need to be able to be deallocated, especially
    if the revision walking machinary has done rewriting

Though the die() is the largest issue.

-- 
Shawn.