Add diff threading tests and attr file cache locks

This adds a basic test of doing simultaneous diffs on multiple
threads and adds basic locking for the attr file cache because
that was the immediate problem that arose from these tests.

10 files changed, 173 insertions, 102 deletions

diff --git a/src/attr.c b/src/attr.c
index d8a171d0f..f52a8a97b 100644
--- a/src/attr.c
+++ b/src/attr.c
@@ -33,6 +33,7 @@ static int collect_attr_files(
 	const char *path,
 	git_vector *files);
 
+static void release_attr_files(git_vector *files);
 
 int git_attr_get(
 	const char **value,
@@ -76,7 +77,7 @@ int git_attr_get(
 	}
 
 cleanup:
-	git_vector_free(&files);
+	release_attr_files(&files);
 	git_attr_path__free(&path);
 
 	return error;
@@ -152,7 +153,7 @@ int git_attr_get_many(
 	}
 
 cleanup:
-	git_vector_free(&files);
+	release_attr_files(&files);
 	git_attr_path__free(&path);
 	git__free(info);
 
@@ -181,12 +182,10 @@ int git_attr_foreach(
 	if (git_attr_path__init(&path, pathname, git_repository_workdir(repo)) < 0)
 		return -1;
 
-	if ((error = collect_attr_files(repo, flags, pathname, &files)) < 0)
+	if ((error = collect_attr_files(repo, flags, pathname, &files)) < 0 ||
+		(error = git_strmap_alloc(&seen)) < 0)
 		goto cleanup;
 
-	seen = git_strmap_alloc();
-	GITERR_CHECK_ALLOC(seen);
-
 	git_vector_foreach(&files, i, file) {
 
 		git_attr_file__foreach_matching_rule(file, &path, j, rule) {
@@ -211,7 +210,7 @@ int git_attr_foreach(
 
 cleanup:
 	git_strmap_free(seen);
-	git_vector_free(&files);
+	release_attr_files(&files);
 	git_attr_path__free(&path);
 
 	return error;
@@ -350,12 +349,21 @@ static int load_attr_from_cache(
 	if (git_buf_printf(&cache_key, "%d#%s", (int)source, relative_path) < 0)
 		return -1;
 
-	cache_pos = git_strmap_lookup_index(cache->files, cache_key.ptr);
+	if (git_mutex_lock(&cache->lock) < 0) {
+		giterr_set(GITERR_OS, "Could not get cache attr lock");
+		git_buf_free(&cache_key);
+		return -1;
+	}
 
-	git_buf_free(&cache_key);
+	cache_pos = git_strmap_lookup_index(cache->files, cache_key.ptr);
 
-	if (git_strmap_valid_index(cache->files, cache_pos))
+	if (git_strmap_valid_index(cache->files, cache_pos)) {
 		*file = git_strmap_value_at(cache->files, cache_pos);
+		GIT_REFCOUNT_INC(*file);
+	}
+
+	git_mutex_unlock(&cache->lock);
+	git_buf_free(&cache_key);
 
 	return 0;
 }
@@ -367,20 +375,26 @@ int git_attr_cache__internal_file(
 {
 	int error = 0;
 	git_attr_cache *cache = git_repository_attr_cache(repo);
-	khiter_t cache_pos = git_strmap_lookup_index(cache->files, filename);
+	khiter_t cache_pos;
+
+	if (git_mutex_lock(&cache->lock) < 0) {
+		giterr_set(GITERR_OS, "Unable to get attr cache lock");
+		return -1;
+	}
+
+	cache_pos = git_strmap_lookup_index(cache->files, filename);
 
 	if (git_strmap_valid_index(cache->files, cache_pos)) {
 		*file = git_strmap_value_at(cache->files, cache_pos);
-		return 0;
 	}
+	else if (!(error = git_attr_file__new(file, 0, filename, &cache->pool))) {
 
-	if (git_attr_file__new(file, 0, filename, &cache->pool) < 0)
-		return -1;
-
-	git_strmap_insert(cache->files, (*file)->key + 2, *file, error);
-	if (error > 0)
-		error = 0;
+		git_strmap_insert(cache->files, (*file)->key + 2, *file, error);
+		if (error > 0)
+			error = 0;
+	}
 
+	git_mutex_unlock(&cache->lock);
 	return error;
 }
 
@@ -452,9 +466,17 @@ int git_attr_cache__push_file(
 	if (parse && (error = parse(repo, parsedata, content, file)) < 0)
 		goto finish;
 
-	git_strmap_insert(cache->files, file->key, file, error); //-V595
-	if (error > 0)
-		error = 0;
+	if (git_mutex_lock(&cache->lock) < 0) {
+		giterr_set(GITERR_OS, "Unable to get attr cache lock");
+		error = -1;
+	} else {
+		git_strmap_insert(cache->files, file->key, file, error); /* -V595 */
+		if (error > 0) { /* > 0 means inserting for the first time */
+			error = 0;
+			GIT_REFCOUNT_INC(file);
+		}
+		git_mutex_unlock(&cache->lock);
+	}
 
 	/* remember "cache buster" file signature */
 	if (blob)
@@ -481,7 +503,8 @@ finish:
 }
 
 #define push_attr_file(R,S,B,F) \
-	git_attr_cache__push_file((R),(B),(F),GIT_ATTR_FILE_FROM_FILE,git_attr_file__parse_buffer,NULL,(S))
+	git_attr_cache__push_file											\
+	((R),(B),(F),GIT_ATTR_FILE_FROM_FILE,git_attr_file__parse_buffer,NULL,(S))
 
 typedef struct {
 	git_repository *repo;
@@ -535,6 +558,18 @@ static int push_one_attr(void *ref, git_buf *path)
 	return error;
 }
 
+static void release_attr_files(git_vector *files)
+{
+	size_t i;
+	git_attr_file *file;
+
+	git_vector_foreach(files, i, file) {
+		git_attr_file__free(file);
+		files->contents[i] = NULL;
+	}
+	git_vector_free(files);
+}
+
 static int collect_attr_files(
 	git_repository *repo,
 	uint32_t flags,
@@ -600,7 +635,7 @@ static int collect_attr_files(
 
  cleanup:
 	if (error < 0)
-		git_vector_free(files);
+		release_attr_files(files);
 	git_buf_free(&dir);
 
 	return error;
@@ -637,61 +672,11 @@ static int attr_cache__lookup_path(
 	return error;
 }
 
-int git_attr_cache__init(git_repository *repo)
-{
-	int ret;
-	git_attr_cache *cache = git_repository_attr_cache(repo);
-	git_config *cfg;
-
-	if (cache->initialized)
-		return 0;
-
-	/* cache config settings for attributes and ignores */
-	if (git_repository_config__weakptr(&cfg, repo) < 0)
-		return -1;
-
-	ret = attr_cache__lookup_path(
-		&cache->cfg_attr_file, cfg, GIT_ATTR_CONFIG, GIT_ATTR_FILE_XDG);
-	if (ret < 0)
-		return ret;
-
-	ret = attr_cache__lookup_path(
-		&cache->cfg_excl_file, cfg, GIT_IGNORE_CONFIG, GIT_IGNORE_FILE_XDG);
-	if (ret < 0)
-		return ret;
-
-	/* allocate hashtable for attribute and ignore file contents */
-	if (cache->files == NULL) {
-		cache->files = git_strmap_alloc();
-		GITERR_CHECK_ALLOC(cache->files);
-	}
-
-	/* allocate hashtable for attribute macros */
-	if (cache->macros == NULL) {
-		cache->macros = git_strmap_alloc();
-		GITERR_CHECK_ALLOC(cache->macros);
-	}
-
-	/* allocate string pool */
-	if (git_pool_init(&cache->pool, 1, 0) < 0)
-		return -1;
-
-	cache->initialized = 1;
-
-	/* insert default macros */
-	return git_attr_add_macro(repo, "binary", "-diff -crlf -text");
-}
-
-void git_attr_cache_flush(
-	git_repository *repo)
+static void attr_cache__free(git_attr_cache *cache)
 {
-	git_attr_cache *cache;
-
-	if (!repo)
+	if (!cache)
 		return;
 
-	cache = git_repository_attr_cache(repo);
-
 	if (cache->files != NULL) {
 		git_attr_file *file;
 
@@ -720,19 +705,93 @@ void git_attr_cache_flush(
 	git__free(cache->cfg_excl_file);
 	cache->cfg_excl_file = NULL;
 
-	cache->initialized = 0;
+	git_mutex_free(&cache->lock);
+
+	git__free(cache);
+}
+
+int git_attr_cache__init(git_repository *repo)
+{
+	int ret = 0;
+	git_attr_cache *cache = git_repository_attr_cache(repo);
+	git_config *cfg;
+
+	if (cache)
+		return 0;
+
+	if ((ret = git_repository_config__weakptr(&cfg, repo)) < 0)
+		return ret;
+
+	cache = git__calloc(1, sizeof(git_attr_cache));
+	GITERR_CHECK_ALLOC(cache);
+
+	/* set up lock */
+	if (git_mutex_init(&cache->lock) < 0) {
+		giterr_set(GITERR_OS, "Unable to initialize lock for attr cache");
+		git__free(cache);
+		return -1;
+	}
+
+	/* cache config settings for attributes and ignores */
+	ret = attr_cache__lookup_path(
+		&cache->cfg_attr_file, cfg, GIT_ATTR_CONFIG, GIT_ATTR_FILE_XDG);
+	if (ret < 0)
+		goto cancel;
+
+	ret = attr_cache__lookup_path(
+		&cache->cfg_excl_file, cfg, GIT_IGNORE_CONFIG, GIT_IGNORE_FILE_XDG);
+	if (ret < 0)
+		goto cancel;
+
+	/* allocate hashtable for attribute and ignore file contents,
+	 * hashtable for attribute macros, and string pool
+	 */
+	if ((ret = git_strmap_alloc(&cache->files)) < 0 ||
+		(ret = git_strmap_alloc(&cache->macros)) < 0 ||
+		(ret = git_pool_init(&cache->pool, 1, 0)) < 0)
+		goto cancel;
+
+	cache = git__compare_and_swap(&repo->attrcache, NULL, cache);
+	if (cache)
+		goto cancel; /* raced with another thread, free this but no error */
+
+	/* insert default macros */
+	return git_attr_add_macro(repo, "binary", "-diff -crlf -text");
+
+cancel:
+	attr_cache__free(cache);
+	return ret;
+}
+
+void git_attr_cache_flush(git_repository *repo)
+{
+	git_attr_cache *cache;
+
+	/* this could be done less expensively, but for now, we'll just free
+	 * the entire attrcache and let the next use reinitialize it...
+	 */
+	if (repo && (cache = git__swap(repo->attrcache, NULL)) != NULL)
+		attr_cache__free(cache);
 }
 
 int git_attr_cache__insert_macro(git_repository *repo, git_attr_rule *macro)
 {
-	git_strmap *macros = git_repository_attr_cache(repo)->macros;
+	git_attr_cache *cache = git_repository_attr_cache(repo);
+	git_strmap *macros = cache->macros;
 	int error;
 
 	/* TODO: generate warning log if (macro->assigns.length == 0) */
 	if (macro->assigns.length == 0)
 		return 0;
 
-	git_strmap_insert(macros, macro->match.pattern, macro, error);
+	if (git_mutex_lock(&cache->lock) < 0) {
+		giterr_set(GITERR_OS, "Unable to get attr cache lock");
+		error = -1;
+	} else {
+		git_strmap_insert(macros, macro->match.pattern, macro, error);
+		git_mutex_unlock(&cache->lock);
+	}
+
 	return (error < 0) ? -1 : 0;
 }
 
diff --git a/src/attr_file.c b/src/attr_file.c
index ea92336f7..695f661a8 100644
--- a/src/attr_file.c
+++ b/src/attr_file.c
@@ -23,6 +23,7 @@ int git_attr_file__new(
 
 	attrs = git__calloc(1, sizeof(git_attr_file));
 	GITERR_CHECK_ALLOC(attrs);
+	GIT_REFCOUNT_INC(attrs);
 
 	if (pool)
 		attrs->pool = pool;
@@ -152,11 +153,8 @@ void git_attr_file__clear_rules(git_attr_file *file)
 	git_vector_free(&file->rules);
 }
 
-void git_attr_file__free(git_attr_file *file)
+static void attr_file_free(git_attr_file *file)
 {
-	if (!file)
-		return;
-
 	git_attr_file__clear_rules(file);
 
 	if (file->pool_is_allocated) {
@@ -168,6 +166,13 @@ void git_attr_file__free(git_attr_file *file)
 	git__free(file);
 }
 
+void git_attr_file__free(git_attr_file *file)
+{
+	if (!file)
+		return;
+	GIT_REFCOUNT_DEC(file, attr_file_free);
+}
+
 uint32_t git_attr_file__name_hash(const char *name)
 {
 	uint32_t h = 5381;
diff --git a/src/attr_file.h b/src/attr_file.h
index 3bc7c6cb8..dbd6696c9 100644
--- a/src/attr_file.h
+++ b/src/attr_file.h
@@ -64,6 +64,7 @@ typedef struct {
 } git_attr_assignment;
 
 typedef struct {
+	git_refcount rc;
 	char *key;				/* cache "source#path" this was loaded from */
 	git_vector rules;		/* vector of <rule*> or <fnmatch*> */
 	git_pool *pool;
diff --git a/src/attrcache.h b/src/attrcache.h
index 077633b87..4f9cff6bb 100644
--- a/src/attrcache.h
+++ b/src/attrcache.h
@@ -11,12 +11,12 @@
 #include "strmap.h"
 
 typedef struct {
-	int initialized;
-	git_pool pool;
-	git_strmap *files;	 /* hash path to git_attr_file of rules */
-	git_strmap *macros;	 /* hash name to vector<git_attr_assignment> */
 	char *cfg_attr_file; /* cached value of core.attributesfile */
 	char *cfg_excl_file; /* cached value of core.excludesfile */
+	git_strmap *files;	 /* hash path to git_attr_file of rules */
+	git_strmap *macros;	 /* hash name to vector<git_attr_assignment> */
+	git_mutex lock;
+	git_pool  pool;
 } git_attr_cache;
 
 extern int git_attr_cache__init(git_repository *repo);
diff --git a/src/config_file.c b/src/config_file.c
index aedf2cb12..bb26aa8a3 100644
--- a/src/config_file.c
+++ b/src/config_file.c
@@ -180,11 +180,15 @@ static int config_open(git_config_backend *cfg, git_config_level_t level)
 
 	b->level = level;
 
-	b->values = git_strmap_alloc();
-	GITERR_CHECK_ALLOC(b->values);
+	if ((res = git_strmap_alloc(&b->values)) < 0)
+		return res;
 
 	git_array_init(b->readers);
 	reader = git_array_alloc(b->readers);
+	if (!reader) {
+		git_strmap_free(b->values);
+		return -1;
+	}
 	memset(reader, 0, sizeof(struct reader));
 
 	reader->file_path = git__strdup(b->file_path);
@@ -205,6 +209,7 @@ static int config_open(git_config_backend *cfg, git_config_level_t level)
 
 	reader = git_array_get(b->readers, 0);
 	git_buf_free(&reader->buffer);
+
 	return res;
 }
 
@@ -218,8 +223,10 @@ static int config_refresh(git_config_backend *cfg)
 
 	for (i = 0; i < git_array_size(b->readers); i++) {
 		reader = git_array_get(b->readers, i);
+
 		res = git_futils_readbuffer_updated(
-			&reader->buffer, reader->file_path, &reader->file_mtime, &reader->file_size, &updated);
+			&reader->buffer, reader->file_path,
+			&reader->file_mtime, &reader->file_size, &updated);
 
 		if (res < 0)
 			return (res == GIT_ENOTFOUND) ? 0 : res;
@@ -233,10 +240,9 @@ static int config_refresh(git_config_backend *cfg)
 
 	/* need to reload - store old values and prep for reload */
 	old_values = b->values;
-	b->values = git_strmap_alloc();
-	GITERR_CHECK_ALLOC(b->values);
-
-	if ((res = config_parse(b, reader, b->level, 0)) < 0) {
+	if ((res = git_strmap_alloc(&b->values)) < 0) {
+		b->values = old_values;
+	} else if ((res = config_parse(b, reader, b->level, 0)) < 0) {
 		free_vars(b->values);
 		b->values = old_values;
 	} else {
diff --git a/src/diff_driver.c b/src/diff_driver.c
index 4c9a0af65..8136e0dd9 100644
--- a/src/diff_driver.c
+++ b/src/diff_driver.c
@@ -66,7 +66,7 @@ git_diff_driver_registry *git_diff_driver_registry_new()
 	if (!reg)
 		return NULL;
 
-	if ((reg->drivers = git_strmap_alloc()) == NULL) {
+	if (git_strmap_alloc(&reg->drivers) < 0) {
 		git_diff_driver_registry_free(reg);
 		return NULL;
 	}
diff --git a/src/repository.h b/src/repository.h
index 99923b63b..86db488fd 100644
--- a/src/repository.h
+++ b/src/repository.h
@@ -108,7 +108,7 @@ struct git_repository {
 	git_submodule_cache *_submodules;
 
 	git_cache objects;
-	git_attr_cache attrcache;
+	git_attr_cache *attrcache;
 	git_diff_driver_registry *diff_drivers;
 
 	char *path_repository;
@@ -123,7 +123,7 @@ struct git_repository {
 
 GIT_INLINE(git_attr_cache *) git_repository_attr_cache(git_repository *repo)
 {
-	return &repo->attrcache;
+	return repo->attrcache;
 }
 
 int git_repository_head_tree(git_tree **tree, git_repository *repo);
diff --git a/src/sortedcache.c b/src/sortedcache.c
index 13f0921f1..625322034 100644
--- a/src/sortedcache.c
+++ b/src/sortedcache.c
@@ -20,7 +20,7 @@ int git_sortedcache_new(
 
 	if (git_pool_init(&sc->pool, 1, 0) < 0 ||
 		git_vector_init(&sc->items, 4, item_cmp) < 0 ||
-		(sc->map = git_strmap_alloc()) == NULL)
+		git_strmap_alloc(&sc->map) < 0)
 		goto fail;
 
 	if (git_rwlock_init(&sc->lock)) {
@@ -39,8 +39,7 @@ int git_sortedcache_new(
 	return 0;
 
 fail:
-	if (sc->map)
-		git_strmap_free(sc->map);
+	git_strmap_free(sc->map);
 	git_vector_free(&sc->items);
 	git_pool_clear(&sc->pool);
 	git__free(sc);
diff --git a/src/strmap.h b/src/strmap.h
index 8276ab468..8985aaf7e 100644
--- a/src/strmap.h
+++ b/src/strmap.h
@@ -22,7 +22,9 @@ typedef khiter_t git_strmap_iter;
 #define GIT__USE_STRMAP \
 	__KHASH_IMPL(str, static kh_inline, const char *, void *, 1, kh_str_hash_func, kh_str_hash_equal)
 
-#define git_strmap_alloc()  kh_init(str)
+#define git_strmap_alloc(hp) \
+	((*(hp) = kh_init(str)) == NULL) ? giterr_set_oom(), -1 : 0
+
 #define git_strmap_free(h)  kh_destroy(str, h), h = NULL
 #define git_strmap_clear(h) kh_clear(str, h)
 
diff --git a/src/submodule.c b/src/submodule.c
index bea096df5..95d3d0d9c 100644
--- a/src/submodule.c
+++ b/src/submodule.c
@@ -1638,8 +1638,7 @@ static int submodule_cache_alloc(
 		return -1;
 	}
 
-	cache->submodules = git_strmap_alloc();
-	if (!cache->submodules) {
+	if (git_strmap_alloc(&cache->submodules) < 0) {
 		submodule_cache_free(cache);
 		return -1;
 	}