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.

1 files changed, 90 insertions, 31 deletions

diff --git a/tests/threads/diff.c b/tests/threads/diff.c
index 33afc58ac..7f10a699d 100644
--- a/tests/threads/diff.c
+++ b/tests/threads/diff.c
@@ -1,9 +1,10 @@
 #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);
+}