Index locking and entry allocation changes

This makes the lock management on the index a little bit broader,
having a number of routines hold the lock across looking up the
item to be modified and actually making the modification.  Still
not true thread safety, but more pure index modifications are now
safe which allows the simple cases (such as starting up a diff
while index modifications are underway) safe enough to get the
snapshot without hitting allocation problems.

As part of this, I simplified the allocation of index entries to
use a flex array and just put the path at the end of the index
entry.  This makes every entry self-contained and makes it a
little easier to feel sure that pointers to strings aren't
being accidentally copied and freed while other references are
still being held.

src/common.h

@@ -46,6 +46,7 @@
 # include <unistd.h>
 # ifdef GIT_THREADS
 #	include <pthread.h>
+#	include <sched.h>
 # endif
 #define GIT_STDLIB_CALL
 

src/merge.c

@@ -1230,7 +1230,7 @@ done:
 	return error;
 }
 
-GIT_INLINE(int) index_entry_dup(
+GIT_INLINE(int) index_entry_dup_pool(
 	git_index_entry *out,
 	git_pool *pool,
 	const git_index_entry *src)
@@ -1276,9 +1276,9 @@ static git_merge_diff *merge_diff_from_index_entries(
 	if ((conflict = git_pool_malloc(pool, sizeof(git_merge_diff))) == NULL)
 		return NULL;
 
-	if (index_entry_dup(&conflict->ancestor_entry, pool, entries[TREE_IDX_ANCESTOR]) < 0 ||
-		index_entry_dup(&conflict->our_entry, pool, entries[TREE_IDX_OURS]) < 0 ||
-		index_entry_dup(&conflict->their_entry, pool, entries[TREE_IDX_THEIRS]) < 0)
+	if (index_entry_dup_pool(&conflict->ancestor_entry, pool, entries[TREE_IDX_ANCESTOR]) < 0 ||
+		index_entry_dup_pool(&conflict->our_entry, pool, entries[TREE_IDX_OURS]) < 0 ||
+		index_entry_dup_pool(&conflict->their_entry, pool, entries[TREE_IDX_THEIRS]) < 0)
 		return NULL;
 
 	conflict->our_status = merge_delta_type_from_index_entries(
@@ -1318,7 +1318,7 @@ static int merge_diff_list_insert_unmodified(
 	entry = git_pool_malloc(&diff_list->pool, sizeof(git_index_entry));
 	GITERR_CHECK_ALLOC(entry);
 
-	if ((error = index_entry_dup(entry, &diff_list->pool, tree_items[0])) >= 0)
+	if ((error = index_entry_dup_pool(entry, &diff_list->pool, tree_items[0])) >= 0)
 		error = git_vector_insert(&diff_list->staged, entry);
 
 	return error;

src/thread-utils.h

@@ -47,6 +47,12 @@ typedef git_atomic git_atomic_ssize;
 #define git_thread_exit(status)	pthread_exit(status)
 #define git_thread_join(id, status) pthread_join(id, status)
 
+#if defined(GIT_WIN32)
+#define git_thread_yield() Sleep(0)
+#else
+#define git_thread_yield() sched_yield()
+#endif
+
 /* Pthreads Mutex */
 #define git_mutex pthread_mutex_t
 #define git_mutex_init(a)	pthread_mutex_init(a, NULL)
@@ -176,6 +182,7 @@ GIT_INLINE(int64_t) git_atomic64_add(git_atomic64 *a, int64_t addend)
 #define git_thread_kill(thread) (void)0
 #define git_thread_exit(status) (void)0
 #define git_thread_join(id, status) (void)0
+#define git_thread_yield() (void)0
 
 /* Pthreads Mutex */
 #define git_mutex unsigned int

tests/diff/iterator.c

@@ -363,9 +363,8 @@ static void index_iterator_test(
 	git_index *index;
 	git_iterator *i;
 	const git_index_entry *entry;
-	int error, count = 0;
+	int error, count = 0, caps;
 	git_repository *repo = cl_git_sandbox_init(sandbox);
-	unsigned int caps;
 
 	cl_git_pass(git_repository_index(&index, repo));
 	caps = git_index_caps(index);

tests/threads/diff.c

 #include "clar_libgit2.h"
 #include "thread-utils.h"
 
-static git_repository *g_repo;
-static git_tree *a, *b;
-static git_atomic counts[4];
+static git_repository *_repo;
+static git_tree *_a, *_b;
+static git_atomic _counts[4];
+static int _check_counts;
 
 void test_threads_diff__cleanup(void)
 {
@@ -24,7 +25,7 @@ static void run_in_parallel(
 	cl_assert(id != NULL && th != NULL);
 
 	for (r = 0; r < repeats; ++r) {
-		g_repo = cl_git_sandbox_reopen(); /* reopen sandbox to flush caches */
+		_repo = cl_git_sandbox_reopen(); /* reopen sandbox to flush caches */
 
 		if (before_test) before_test();
 
@@ -53,24 +54,26 @@ static void run_in_parallel(
 static void setup_trees(void)
 {
 	cl_git_pass(git_revparse_single(
-		(git_object **)&a, g_repo, "0017bd4ab1^{tree}"));
+		(git_object **)&_a, _repo, "0017bd4ab1^{tree}"));
 	cl_git_pass(git_revparse_single(
-		(git_object **)&b, g_repo, "26a125ee1b^{tree}"));
+		(git_object **)&_b, _repo, "26a125ee1b^{tree}"));
 
-	memset(counts, 0, sizeof(counts));
+	memset(_counts, 0, sizeof(_counts));
 }
 
 #define THREADS 20
 
 static void free_trees(void)
 {
-	git_tree_free(a); a = NULL;
-	git_tree_free(b); b = NULL;
-
-	cl_assert_equal_i(288, git_atomic_get(&counts[0]));
-	cl_assert_equal_i(112, git_atomic_get(&counts[1]));
-	cl_assert_equal_i( 80, git_atomic_get(&counts[2]));
-	cl_assert_equal_i( 96, git_atomic_get(&counts[3]));
+	git_tree_free(_a); _a = NULL;
+	git_tree_free(_b); _b = NULL;
+
+	if (_check_counts) {
+		cl_assert_equal_i(288, git_atomic_get(&_counts[0]));
+		cl_assert_equal_i(112, git_atomic_get(&_counts[1]));
+		cl_assert_equal_i( 80, git_atomic_get(&_counts[2]));
+		cl_assert_equal_i( 96, git_atomic_get(&_counts[3]));
+	}
 }
 
 static void *run_index_diffs(void *arg)
@@ -81,48 +84,41 @@ static void *run_index_diffs(void *arg)
 	size_t i;
 	int exp[4] = { 0, 0, 0, 0 };
 
-//	fprintf(stderr, "%d >>>\n", thread);
-
 	switch (thread & 0x03) {
 	case 0: /* diff index to workdir */;
-		cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+		cl_git_pass(git_diff_index_to_workdir(&diff, _repo, NULL, &opts));
 		break;
 	case 1: /* diff tree 'a' to index */;
-		cl_git_pass(git_diff_tree_to_index(&diff, g_repo, a, NULL, &opts));
+		cl_git_pass(git_diff_tree_to_index(&diff, _repo, _a, NULL, &opts));
 		break;
 	case 2: /* diff tree 'b' to index */;
-		cl_git_pass(git_diff_tree_to_index(&diff, g_repo, b, NULL, &opts));
+		cl_git_pass(git_diff_tree_to_index(&diff, _repo, _b, NULL, &opts));
 		break;
 	case 3: /* diff index to workdir (explicit index) */;
 		{
 			git_index *idx;
-			cl_git_pass(git_repository_index(&idx, g_repo));
-			cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, idx, &opts));
+			cl_git_pass(git_repository_index(&idx, _repo));
+			cl_git_pass(git_diff_index_to_workdir(&diff, _repo, idx, &opts));
 			git_index_free(idx);
 			break;
 		}
 	}
 
-//	fprintf(stderr, "%d <<<\n", thread);
-
 	/* keep some diff stats to make sure results are as expected */
 
 	i = git_diff_num_deltas(diff);
-	git_atomic_add(&counts[0], (int32_t)i);
+	git_atomic_add(&_counts[0], (int32_t)i);
 	exp[0] = (int)i;
 
 	while (i > 0) {
 		switch (git_diff_get_delta(diff, --i)->status) {
-		case GIT_DELTA_MODIFIED: exp[1]++; git_atomic_inc(&counts[1]); break;
-		case GIT_DELTA_ADDED:    exp[2]++; git_atomic_inc(&counts[2]); break;
-		case GIT_DELTA_DELETED:  exp[3]++; git_atomic_inc(&counts[3]); break;
+		case GIT_DELTA_MODIFIED: exp[1]++; git_atomic_inc(&_counts[1]); break;
+		case GIT_DELTA_ADDED:    exp[2]++; git_atomic_inc(&_counts[2]); break;
+		case GIT_DELTA_DELETED:  exp[3]++; git_atomic_inc(&_counts[3]); break;
 		default: break;
 		}
 	}
 
-//	fprintf(stderr, "%2d: [%d] total %d (M %d A %d D %d)\n",
-//			thread, (int)(thread & 0x03), exp[0], exp[1], exp[2], exp[3]);
-
 	switch (thread & 0x03) {
 	case 0: case 3:
 		cl_assert_equal_i(8, exp[0]); cl_assert_equal_i(4, exp[1]);
@@ -145,8 +141,71 @@ static void *run_index_diffs(void *arg)
 
 void test_threads_diff__concurrent_diffs(void)
 {
-	g_repo = cl_git_sandbox_init("status");
+	_repo = cl_git_sandbox_init("status");
+	_check_counts = 1;
 
 	run_in_parallel(
 		20, 32, run_index_diffs, setup_trees, free_trees);
 }
+
+static void *run_index_diffs_with_modifier(void *arg)
+{
+	int thread = *(int *)arg;
+	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
+	git_diff *diff = NULL;
+	git_index *idx = NULL;
+
+	cl_git_pass(git_repository_index(&idx, _repo));
+
+	/* have first thread altering the index as we go */
+	if (thread == 0) {
+		int i;
+
+		for (i = 0; i < 300; ++i) {
+			switch (i & 0x03) {
+			case 0: (void)git_index_add_bypath(idx, "new_file"); break;
+			case 1: (void)git_index_remove_bypath(idx, "modified_file"); break;
+			case 2: (void)git_index_remove_bypath(idx, "new_file"); break;
+			case 3: (void)git_index_add_bypath(idx, "modified_file"); break;
+			}
+			git_thread_yield();
+		}
+
+		git_index_free(idx);
+		return arg;
+	}
+
+	/* only use explicit index in this test to prevent reloading */
+
+	switch (thread & 0x03) {
+	case 0: /* diff index to workdir */;
+		cl_git_pass(git_diff_index_to_workdir(&diff, _repo, idx, &opts));
+		break;
+	case 1: /* diff tree 'a' to index */;
+		cl_git_pass(git_diff_tree_to_index(&diff, _repo, _a, idx, &opts));
+		break;
+	case 2: /* diff tree 'b' to index */;
+		cl_git_pass(git_diff_tree_to_index(&diff, _repo, _b, idx, &opts));
+		break;
+	case 3: /* diff index to workdir reversed */;
+		opts.flags |= GIT_DIFF_REVERSE;
+		cl_git_pass(git_diff_index_to_workdir(&diff, _repo, idx, &opts));
+		break;
+	}
+
+	/* results will be unpredictable with index modifier thread running */
+
+	git_diff_free(diff);
+	git_index_free(idx);
+
+	return arg;
+}
+
+void test_threads_diff__with_concurrent_index_modified(void)
+{
+	_repo = cl_git_sandbox_init("status");
+	_check_counts = 0;
+
+	run_in_parallel(
+		20, 32, run_index_diffs_with_modifier, setup_trees, free_trees);
+}