Test cancel from indexer progress callback

This adds tests that try canceling an indexer operation from within the progress callback. After writing the tests, I wanted to run this under valgrind and had a number of errors in that situation because mmap wasn't working. I added a CMake option to force emulation of mmap and consolidated the Amiga-specific code into that new place (so we don't actually need separate Amiga code now, just have to turn on -DNO_MMAP). Additionally, I made the indexer code propagate error codes more reliably than it used to.
libgit2 · vmg · Dec 13, 2013 · Nov 25, 2013 · Dec 4, 2013 · Dec 5, 2013
commit 7697e54176ccab22ed6d4597d7256e9a1e6ff202
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -33,6 +33,7 @@ OPTION( ANDROID				"Build for android NDK"	 				OFF )
 
 OPTION( USE_ICONV			"Link with and use iconv library" 		OFF )
 OPTION( USE_SSH				"Link with libssh to enable SSH support" ON )
+OPTION( VALGRIND			"Configure build for valgrind"			OFF )
 
 IF(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
 	SET( USE_ICONV ON )
@@ -340,9 +341,11 @@ IF (WIN32 AND NOT CYGWIN)
 	ADD_DEFINITIONS(-DWIN32 -D_WIN32_WINNT=0x0501)
 	FILE(GLOB SRC_OS src/win32/*.c src/win32/*.h)
 ELSEIF (AMIGA)
-	ADD_DEFINITIONS(-DNO_ADDRINFO -DNO_READDIR_R)
-	FILE(GLOB SRC_OS src/amiga/*.c src/amiga/*.h)
+	ADD_DEFINITIONS(-DNO_ADDRINFO -DNO_READDIR_R -DNO_MMAP)
 ELSE()
+	IF (VALGRIND)
+		ADD_DEFINITIONS(-DNO_MMAP)
+	ENDIF()
 	FILE(GLOB SRC_OS src/unix/*.c src/unix/*.h)
 ENDIF()
 FILE(GLOB SRC_GIT2 src/*.c src/*.h src/transports/*.c src/transports/*.h src/xdiff/*.c src/xdiff/*.h)

diff --git a/include/git2/pack.h b/include/git2/pack.h
@@ -52,7 +52,7 @@ typedef enum {
 	GIT_PACKBUILDER_ADDING_OBJECTS = 0,
 	GIT_PACKBUILDER_DELTAFICATION = 1,
 } git_packbuilder_stage_t;
-	
+
 /**
  * Initialize a new packbuilder
  *
@@ -143,6 +143,7 @@ GIT_EXTERN(int) git_packbuilder_write(
 GIT_EXTERN(const git_oid *) git_packbuilder_hash(git_packbuilder *pb);
 
 typedef int (*git_packbuilder_foreach_cb)(void *buf, size_t size, void *payload);
+
 /**
  * Create the new pack and pass each object to the callback
  *

diff --git a/src/amiga/map.c b/src/amiga/map.c
diff --git a/src/indexer.c b/src/indexer.c
@@ -441,21 +441,21 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran
 
 	processed = stats->indexed_objects;
 
-	if (git_filebuf_write(&idx->pack_file, data, size) < 0)
-		return -1;
+	if ((error = git_filebuf_write(&idx->pack_file, data, size)) < 0)
+		return error;
 
 	hash_partially(idx, data, (int)size);
 
 	/* Make sure we set the new size of the pack */
 	if (idx->opened_pack) {
 		idx->pack->mwf.size += size;
 	} else {
-		if (open_pack(&idx->pack, idx->pack_file.path_lock) < 0)
-			return -1;
+		if ((error = open_pack(&idx->pack, idx->pack_file.path_lock)) < 0)
+			return error;
 		idx->opened_pack = 1;
 		mwf = &idx->pack->mwf;
-		if (git_mwindow_file_register(&idx->pack->mwf) < 0)
-			return -1;
+		if ((error = git_mwindow_file_register(&idx->pack->mwf)) < 0)
+			return error;
 	}
 
 	if (!idx->parsed_header) {
@@ -464,8 +464,8 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran
 		if ((unsigned)idx->pack->mwf.size < sizeof(struct git_pack_header))
 			return 0;
 
-		if (parse_header(&idx->hdr, idx->pack) < 0)
-			return -1;
+		if ((error = parse_header(&idx->hdr, idx->pack)) < 0)
+			return error;
 
 		idx->parsed_header = 1;
 		idx->nr_objects = ntohl(hdr->hdr_entries);
@@ -503,6 +503,7 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran
 
 	/* As the file grows any windows we try to use will be out of date */
 	git_mwindow_free_all(mwf);
+
 	while (processed < idx->nr_objects) {
 		git_packfile_stream *stream = &idx->stream;
 		git_off_t entry_start = idx->off;
@@ -520,7 +521,7 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran
 				return 0;
 			}
 			if (error < 0)
-				return error;
+				goto on_error;
 
 			git_mwindow_close(&w);
 			idx->entry_start = entry_start;
@@ -533,7 +534,7 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran
 					return 0;
 				}
 				if (error < 0)
-					return error;
+					goto on_error;
 
 				idx->have_delta = 1;
 			} else {
@@ -542,9 +543,10 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran
 			}
 
 			idx->have_stream = 1;
-			if (git_packfile_stream_open(stream, idx->pack, idx->off) < 0)
-				goto on_error;
 
+			error = git_packfile_stream_open(stream, idx->pack, idx->off);
+			if (error < 0)
+				goto on_error;
 		}
 
 		if (idx->have_delta) {
@@ -858,7 +860,7 @@ int git_indexer_commit(git_indexer *idx, git_transfer_progress *stats)
 
 	/* Test for this before resolve_deltas(), as it plays with idx->off */
 	if (idx->off < idx->pack->mwf.size - 20) {
-		giterr_set(GITERR_INDEXER, "unexpected data at the end of the pack");
+		giterr_set(GITERR_INDEXER, "Unexpected data at the end of the pack");
 		return -1;
 	}
 

diff --git a/src/posix.c b/src/posix.c
@@ -203,4 +203,40 @@ int p_write(git_file fd, const void *buf, size_t cnt)
 	return 0;
 }
 
+#ifdef NO_MMAP
 
+#include "map.h"
+
+int p_mmap(git_map *out, size_t len, int prot, int flags, int fd, git_off_t offset)
+{
+	GIT_MMAP_VALIDATE(out, len, prot, flags);
+
+	out->data = NULL;
+	out->len = 0;
+
+	if ((prot & GIT_PROT_WRITE) && ((flags & GIT_MAP_TYPE) == GIT_MAP_SHARED)) {
+		giterr_set(GITERR_OS, "Trying to map shared-writeable");
+		return -1;
+	}
+
+	out->data = malloc(len);
+	GITERR_CHECK_ALLOC(out->data);
+
+	if ((p_lseek(fd, offset, SEEK_SET) < 0) || ((size_t)p_read(fd, out->data, len) != len)) {
+		giterr_set(GITERR_OS, "mmap emulation failed");
+		return -1;
+	}
+
+	out->len = len;
+	return 0;
+}
+
+int p_munmap(git_map *map)
+{
+	assert(map != NULL);
+	free(map->data);
+
+	return 0;
+}
+
+#endif
diff --git a/src/unix/map.c b/src/unix/map.c
@@ -6,7 +6,7 @@
  */
 #include <git2/common.h>
 
-#ifndef GIT_WIN32
+#if !defined(GIT_WIN32) && !defined(NO_MMAP)
 
 #include "map.h"
 #include <sys/mman.h>

diff --git a/src/win32/map.c b/src/win32/map.c
@@ -8,6 +8,7 @@
 #include "map.h"
 #include <errno.h>
 
+#ifndef NO_MMAP
 
 static DWORD get_page_size(void)
 {
@@ -112,4 +113,4 @@ int p_munmap(git_map *map)
 	return error;
 }
 
-
+#endif
diff --git a/tests/pack/indexer.c b/tests/pack/indexer.c
@@ -11,7 +11,7 @@
  * This is a packfile with three objects. The second is a delta which
  * depends on the third, which is also a delta.
  */
-unsigned char out_of_order_pack[] = {
+static const unsigned char out_of_order_pack[] = {
   0x50, 0x41, 0x43, 0x4b, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x03,
   0x32, 0x78, 0x9c, 0x63, 0x67, 0x00, 0x00, 0x00, 0x10, 0x00, 0x08, 0x76,
   0xe6, 0x8f, 0xe8, 0x12, 0x9b, 0x54, 0x6b, 0x10, 0x1a, 0xee, 0x95, 0x10,
@@ -23,13 +23,13 @@ unsigned char out_of_order_pack[] = {
   0x19, 0x87, 0x58, 0x80, 0x61, 0x09, 0x9a, 0x33, 0xca, 0x7a, 0x31, 0x92,
   0x6f, 0xae, 0x66, 0x75
 };
-unsigned int out_of_order_pack_len = 112;
+static const unsigned int out_of_order_pack_len = 112;
 
 /*
  * Packfile with two objects. The second is a delta against an objec
EF57
t
  * which is not in the packfile
  */
-unsigned char thin_pack[] = {
+static const unsigned char thin_pack[] = {
   0x50, 0x41, 0x43, 0x4b, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x02,
   0x32, 0x78, 0x9c, 0x63, 0x67, 0x00, 0x00, 0x00, 0x10, 0x00, 0x08, 0x76,
   0xe6, 0x8f, 0xe8, 0x12, 0x9b, 0x54, 0x6b, 0x10, 0x1a, 0xee, 0x95, 0x10,
@@ -38,18 +38,19 @@ unsigned char thin_pack[] = {
   0x3a, 0x6f, 0x39, 0xd1, 0xfe, 0x66, 0x68, 0x6b, 0xa5, 0xe5, 0xe2, 0x97,
   0xac, 0x94, 0x6c, 0x76, 0x0b, 0x04
 };
-unsigned int thin_pack_len = 78;
+static const unsigned int thin_pack_len = 78;
 
-unsigned char base_obj[] = { 07, 076 };
-unsigned int base_obj_len = 2;
+static const unsigned char base_obj[] = { 07, 076 };
+static const unsigned int base_obj_len = 2;
 
 void test_pack_indexer__out_of_order(void)
 {
-	git_indexer *idx;
-	git_transfer_progress stats;
+	git_indexer *idx = 0;
+	git_transfer_progress stats = { 0 };
 
 	cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL));
-	cl_git_pass(git_indexer_append(idx, out_of_order_pack, out_of_order_pack_len, &stats));
+	cl_git_pass(git_indexer_append(
+		idx, out_of_order_pack, out_of_order_pack_len, &stats));
 	cl_git_pass(git_indexer_commit(idx, &stats));
 
 	cl_assert_equal_i(stats.total_objects, 3);
@@ -61,8 +62,8 @@ void test_pack_indexer__out_of_order(void)
 
 void test_pack_indexer__fix_thin(void)
 {
-	git_indexer *idx;
-	git_transfer_progress stats;
+	git_indexer *idx = NULL;
+	git_transfer_progress stats = { 0 };
 	git_repository *repo;
 	git_odb *odb;
 	git_oid id, should_id;

diff --git a/tests/pack/packbuilder.c b/tests/pack/packbuilder.c
@@ -12,6 +12,7 @@ static git_packbuilder *_packbuilder;
 static git_indexer *_indexer;
 static git_vector _commits;
 static int _commits_is_initialized;
+static git_transfer_progress _stats;
 
 void test_pack_packbuilder__initialize(void)
 {
@@ -20,6 +21,7 @@ void test_pack_packbuilder__initialize(void)
 	cl_git_pass(git_packbuilder_new(&_packbuilder, _repo));
 	cl_git_pass(git_vector_init(&_commits, 0, NULL));
 	_commits_is_initialized = 1;
+	memset(&_stats, 0, sizeof(_stats));
 }
 
 void test_pack_packbuilder__cleanup(void)
@@ -184,11 +186,10 @@ void test_pack_packbuilder__permissions_readwrite(void)
 	test_write_pack_permission(0666, 0666);
 }
 
-static git_transfer_progress stats;
 static int foreach_cb(void *buf, size_t len, void *payload)
 {
 	git_indexer *idx = (git_indexer *) payload;
-	cl_git_pass(git_indexer_append(idx, buf, len, &stats));
+	cl_git_pass(git_indexer_append(idx, buf, len, &_stats));
    	return 0;
 }
 
@@ -199,6 +200,24 @@ void test_pack_packbuilder__foreach(void)
 	seed_packbuilder();
 	cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL));
 	cl_git_pass(git_packbuilder_foreach(_packbuilder, foreach_cb, idx));
-	cl_git_pass(git_indexer_commit(idx, &stats));
+	cl_git_pass(git_indexer_commit(idx, &_stats));
+	git_indexer_free(idx);
+}
+
+static int foreach_cancel_cb(void *buf, size_t len, void *payload)
+{
+	git_indexer *idx = (git_indexer *)payload;
+	cl_git_pass(git_indexer_append(idx, buf, len, &_stats));
+	return (_stats.total_objects > 2) ? -1111 : 0;
+}
+
+void test_pack_packbuilder__foreach_with_cancel(void)
+{
+	git_indexer *idx;
+
+	seed_packbuilder();
+	cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL));
+	cl_git_fail_with(
+		git_packbuilder_foreach(_packbuilder, foreach_cancel_cb, idx), -1111);
 	git_indexer_free(idx);
 }
diff --git a/tests/valgrind-supp-mac.txt b/tests/valgrind-supp-mac.txt
@@ -102,14 +102,6 @@
 	...
 	fun:ssl23_connect
 }
-{
-	mac-ssl-uninitialized-4
-	Memcheck:Param
-	...
-	obj:/usr/lib/libcrypto.0.9.8.dylib
-	...
-	fun:ssl23_connect
-}
 {
 	mac-ssl-leak-1
 	Memcheck:Leak