Further threading fixes

This builds on the earlier thread safety work to make it so that
setting the odb, index, refdb, or config for a repository is done
in a threadsafe manner with minimized locking time.  This is done
by adding a lock to the repository object and using it to guard
the assignment of the above listed pointers.  The lock is only
held to assign the pointer value.

This also contains some minor fixes to the other work with pack
files to reduce the time that locks are being held to and fix an
apparently memory leak.

8 files changed, 168 insertions, 102 deletions

diff --git a/src/global.c b/src/global.c
index b7fd8e257..a0571d127 100644
--- a/src/global.c
+++ b/src/global.c
@@ -135,6 +135,12 @@ int git_threads_init(void)
 
 void git_threads_shutdown(void)
 {
+	if (_tls_init) {
+		void *ptr = pthread_getspecific(_tls_key);
+		pthread_setspecific(_tls_key, NULL);
+		git__free(ptr);
+	}
+
 	pthread_key_delete(_tls_key);
 	_tls_init = 0;
 	git_mutex_free(&git__mwindow_mutex);
diff --git a/src/mwindow.c b/src/mwindow.c
index b35503d46..7e5fcdfbc 100644
--- a/src/mwindow.c
+++ b/src/mwindow.c
@@ -162,7 +162,7 @@ static git_mwindow *new_window(
 	git_mwindow *w;
 
 	w = git__malloc(sizeof(*w));
-	
+
 	if (w == NULL)
 		return NULL;
 
diff --git a/src/pack.c b/src/pack.c
index 1bffb4778..3a2e7fb5a 100644
--- a/src/pack.c
+++ b/src/pack.c
@@ -299,29 +299,27 @@ static int pack_index_open(struct git_pack_file *p)
 	int error = 0;
 	size_t name_len, base_len;
 
-	if ((error = git_mutex_lock(&p->lock)) < 0)
-		return error;
-
 	if (p->index_map.data)
-		goto done;
+		return 0;
 
 	name_len = strlen(p->pack_name);
 	assert(name_len > strlen(".pack")); /* checked by git_pack_file alloc */
 
-	if ((idx_name = git__malloc(name_len)) == NULL) {
-		error = -1;
-		goto done;
-	}
+	if ((idx_name = git__malloc(name_len)) == NULL)
+		return -1;
 
 	base_len = name_len - strlen(".pack");
 	memcpy(idx_name, p->pack_name, base_len);
 	memcpy(idx_name + base_len, ".idx", sizeof(".idx"));
 
-	error = pack_index_check(idx_name, p);
+	if ((error = git_mutex_lock(&p->lock)) < 0)
+		return error;
+
+	if (!p->index_map.data)
+		error = pack_index_check(idx_name, p);
 
 	git__free(idx_name);
 
-done:
 	git_mutex_unlock(&p->lock);
 
 	return error;
@@ -820,7 +818,6 @@ void git_packfile_free(struct git_pack_file *p)
 	cache_free(&p->bases);
 
 	git_mwindow_free_all(&p->mwf);
-	git_mwindow_file_deregister(&p->mwf);
 
 	if (p->mwf.fd != -1)
 		p_close(p->mwf.fd);
diff --git a/src/pack.h b/src/pack.h
index b734ac163..b8014b1ea 100644
--- a/src/pack.h
+++ b/src/pack.h
@@ -79,7 +79,7 @@ typedef struct {
 struct git_pack_file {
 	git_mwindow_file mwf;
 	git_map index_map;
-	git_mutex lock;
+	git_mutex lock; /* protect updates to mwf and index_map */
 
 	uint32_t num_objects;
 	uint32_t num_bad_objects;
diff --git a/src/refdb_fs.c b/src/refdb_fs.c
index 443871005..742ac6260 100644
--- a/src/refdb_fs.c
+++ b/src/refdb_fs.c
@@ -11,7 +11,6 @@
 #include "fileops.h"
 #include "pack.h"
 #include "reflog.h"
-#include "config.h"
 #include "refdb.h"
 #include "refdb_fs.h"
 
diff --git a/src/repository.c b/src/repository.c
index cda75b85f..8744b91e6 100644
--- a/src/repository.c
+++ b/src/repository.c
@@ -32,41 +32,43 @@
 
 #define GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
 
+#define repo_swap_ptr(repo,item_ptr,new_value) \
+	git__swap(&repo->lock, (void **)item_ptr, new_value)
+
 static void drop_odb(git_repository *repo)
 {
-	if (repo->_odb != NULL) {
-		GIT_REFCOUNT_OWN(repo->_odb, NULL);
-		git_odb_free(repo->_odb);
-		repo->_odb = NULL;
+	git_odb *dropme = repo_swap_ptr(repo, &repo->_odb, NULL);
+	if (dropme != NULL) {
+		GIT_REFCOUNT_OWN(dropme, NULL);
+		git_odb_free(dropme);
 	}
 }
 
 static void drop_refdb(git_repository *repo)
 {
-	if (repo->_refdb != NULL) {
-		GIT_REFCOUNT_OWN(repo->_refdb, NULL);
-		git_refdb_free(repo->_refdb);
-		repo->_refdb = NULL;
+	git_refdb *dropme = repo_swap_ptr(repo, &repo->_refdb, NULL);
+	if (dropme != NULL) {
+		GIT_REFCOUNT_OWN(dropme, NULL);
+		git_refdb_free(dropme);
 	}
 }
 
 static void drop_config(git_repository *repo)
 {
-	if (repo->_config != NULL) {
-		GIT_REFCOUNT_OWN(repo->_config, NULL);
-		git_config_free(repo->_config);
-		repo->_config = NULL;
+	git_config *dropme = repo_swap_ptr(repo, &repo->_config, NULL);
+	if (dropme != NULL) {
+		GIT_REFCOUNT_OWN(dropme, NULL);
+		git_config_free(dropme);
+		git_repository__cvar_cache_clear(repo);
 	}
-
-	git_repository__cvar_cache_clear(repo);
 }
 
 static void drop_index(git_repository *repo)
 {
-	if (repo->_index != NULL) {
-		GIT_REFCOUNT_OWN(repo->_index, NULL);
-		git_index_free(repo->_index);
-		repo->_index = NULL;
+	git_index *dropme = repo_swap_ptr(repo, &repo->_index, NULL);
+	if (dropme != NULL) {
+		GIT_REFCOUNT_OWN(dropme, NULL);
+		git_index_free(dropme);
 	}
 }
 
@@ -79,14 +81,15 @@ void git_repository_free(git_repository *repo)
 	git_attr_cache_flush(repo);
 	git_submodule_config_free(repo);
 
-	git__free(repo->path_repository);
-	git__free(repo->workdir);
-
 	drop_config(repo);
 	drop_index(repo);
 	drop_odb(repo);
 	drop_refdb(repo);
 
+	git__free(repo->path_repository);
+	git__free(repo->workdir);
+
+	git_mutex_free(&repo->lock);
 	git__free(repo);
 }
 
@@ -119,6 +122,8 @@ static git_repository *repository_alloc(void)
 
 	memset(repo, 0x0, sizeof(git_repository));
 
+	git_mutex_init(&repo->lock);
+
 	if (git_cache_init(&repo->objects) < 0) {
 		git__free(repo);
 		return NULL;
@@ -549,39 +554,47 @@ on_error:
 	return error;
 }
 
-int git_repository_config__weakptr(git_config **out, git_repository *repo)
+static const char *path_unless_empty(git_buf *buf)
 {
-	if (repo->_config == NULL) {
-		git_buf global_buf = GIT_BUF_INIT, xdg_buf = GIT_BUF_INIT, system_buf = GIT_BUF_INIT;
-		int res;
-
-		const char *global_config_path = NULL;
-		const char *xdg_config_path = NULL;
-		const char *system_config_path = NULL;
-
-		if (git_config_find_global_r(&global_buf) == 0)
-			global_config_path = global_buf.ptr;
+	return git_buf_len(buf) > 0 ? git_buf_cstr(buf) : NULL;
+}
 
-		if (git_config_find_xdg_r(&xdg_buf) == 0)
-			xdg_config_path = xdg_buf.ptr;
+int git_repository_config__weakptr(git_config **out, git_repository *repo)
+{
+	int error = 0;
 
-		if (git_config_find_system_r(&system_buf) == 0)
-			system_config_path = system_buf.ptr;
+	if (repo->_config == NULL) {
+		git_buf global_buf = GIT_BUF_INIT;
+		git_buf xdg_buf = GIT_BUF_INIT;
+		git_buf system_buf = GIT_BUF_INIT;
+		git_config *config;
 
-		res = load_config(&repo->_config, repo, global_config_path, xdg_config_path, system_config_path);
+		git_config_find_global_r(&global_buf);
+		git_config_find_xdg_r(&xdg_buf);
+		git_config_find_system_r(&system_buf);
+
+		error = load_config(
+			&config, repo,
+			path_unless_empty(&global_buf),
+			path_unless_empty(&xdg_buf),
+			path_unless_empty(&system_buf));
+		if (!error) {
+			GIT_REFCOUNT_OWN(config, repo);
+
+			config = repo_swap_ptr(repo, &repo->_config, config);
+			if (config != NULL) {
+				GIT_REFCOUNT_OWN(config, NULL);
+				git_config_free(config);
+			}
+		}
 
 		git_buf_free(&global_buf);
 		git_buf_free(&xdg_buf);
 		git_buf_free(&system_buf);
-
-		if (res < 0)
-			return -1;
-
-		GIT_REFCOUNT_OWN(repo->_config, repo);
 	}
 
 	*out = repo->_config;
-	return 0;
+	return error;
 }
 
 int git_repository_config(git_config **out, git_repository *repo)
@@ -597,35 +610,46 @@ void git_repository_set_config(git_repository *repo, git_config *config)
 {
 	assert(repo && config);
 
-	drop_config(repo);
+	GIT_REFCOUNT_OWN(config, repo);
+	GIT_REFCOUNT_INC(config);
+
+	config = repo_swap_ptr(repo, &repo->_config, config);
+	if (config != NULL) {
+		GIT_REFCOUNT_OWN(config, NULL);
+		git_config_free(config);
+	}
 
-	repo->_config = config;
-	GIT_REFCOUNT_OWN(repo->_config, repo);
-	GIT_REFCOUNT_INC(repo->_config);
+	git_repository__cvar_cache_clear(repo);
 }
 
 int git_repository_odb__weakptr(git_odb **out, git_repository *repo)
 {
+	int error = 0;
+
 	assert(repo && out);
 
 	if (repo->_odb == NULL) {
 		git_buf odb_path = GIT_BUF_INIT;
-		int res;
+		git_odb *odb;
 
-		if (git_buf_joinpath(&odb_path, repo->path_repository, GIT_OBJECTS_DIR) < 0)
-			return -1;
+		git_buf_joinpath(&odb_path, repo->path_repository, GIT_OBJECTS_DIR);
 
-		res = git_odb_open(&repo->_odb, odb_path.ptr);
-		git_buf_free(&odb_path); /* done with path */
+		error = git_odb_open(&odb, odb_path.ptr);
+		if (!error) {
+			GIT_REFCOUNT_OWN(odb, repo);
 
-		if (res < 0)
-			return -1;
+			odb = repo_swap_ptr(repo, &repo->_odb, odb);
+			if (odb != NULL) {
+				GIT_REFCOUNT_OWN(odb, NULL);
+				git_odb_free(odb);
+			}
+		}
 
-		GIT_REFCOUNT_OWN(repo->_odb, repo);
+		git_buf_free(&odb_path);
 	}
 
 	*out = repo->_odb;
-	return 0;
+	return error;
 }
 
 int git_repository_odb(git_odb **out, git_repository *repo)
@@ -641,30 +665,39 @@ void git_repository_set_odb(git_repository *repo, git_odb *odb)
 {
 	assert(repo && odb);
 
-	drop_odb(repo);
-
-	repo->_odb = odb;
-	GIT_REFCOUNT_OWN(repo->_odb, repo);
+	GIT_REFCOUNT_OWN(odb, repo);
 	GIT_REFCOUNT_INC(odb);
+
+	odb = repo_swap_ptr(repo, &repo->_odb, odb);
+	if (odb != NULL) {
+		GIT_REFCOUNT_OWN(odb, NULL);
+		git_odb_free(odb);
+	}
 }
 
 int git_repository_refdb__weakptr(git_refdb **out, git_repository *repo)
 {
+	int error = 0;
+
 	assert(out && repo);
 
 	if (repo->_refdb == NULL) {
-		int res;
+		git_refdb *refdb;
 
-		res = git_refdb_open(&repo->_refdb, repo);
-
-		if (res < 0)
-			return -1;
+		error = git_refdb_open(&refdb, repo);
+		if (!error) {
+			GIT_REFCOUNT_OWN(refdb, repo);
 
-		GIT_REFCOUNT_OWN(repo->_refdb, repo);
+			refdb = repo_swap_ptr(repo, &repo->_refdb, refdb);
+			if (refdb != NULL) {
+				GIT_REFCOUNT_OWN(refdb, NULL);
+				git_refdb_free(refdb);
+			}
+		}
 	}
 
 	*out = repo->_refdb;
-	return 0;
+	return error;
 }
 
 int git_repository_refdb(git_refdb **out, git_repository *repo)
@@ -678,40 +711,48 @@ int git_repository_refdb(git_refdb **out, git_repository *repo)
 
 void git_repository_set_refdb(git_repository *repo, git_refdb *refdb)
 {
-	assert (repo && refdb);
+	assert(repo && refdb);
 
-	drop_refdb(repo);
-
-	repo->_refdb = refdb;
-	GIT_REFCOUNT_OWN(repo->_refdb, repo);
+	GIT_REFCOUNT_OWN(refdb, repo);
 	GIT_REFCOUNT_INC(refdb);
+
+	refdb = repo_swap_ptr(repo, &repo->_refdb, refdb);
+	if (refdb != NULL) {
+		GIT_REFCOUNT_OWN(refdb, NULL);
+		git_refdb_free(refdb);
+	}
 }
 
 int git_repository_index__weakptr(git_index **out, git_repository *repo)
 {
+	int error = 0;
+
 	assert(out && repo);
 
 	if (repo->_index == NULL) {
-		int res;
 		git_buf index_path = GIT_BUF_INIT;
+		git_index *index;
 
-		if (git_buf_joinpath(&index_path, repo->path_repository, GIT_INDEX_FILE) < 0)
-			return -1;
+		git_buf_joinpath(&index_path, repo->path_repository, GIT_INDEX_FILE);
 
-		res = git_index_open(&repo->_index, index_path.ptr);
-		git_buf_free(&index_path); /* done with path */
+		error = git_index_open(&index, index_path.ptr);
+		if (!error) {
+			GIT_REFCOUNT_OWN(index, repo);
 
-		if (res < 0)
-			return -1;
+			index = repo_swap_ptr(repo, &repo->_index, index);
+			if (index != NULL) {
+				GIT_REFCOUNT_OWN(index, NULL);
+				git_index_free(index);
+			}
 
-		GIT_REFCOUNT_OWN(repo->_index, repo);
+			error = git_index_set_caps(repo->_index, GIT_INDEXCAP_FROM_OWNER);
+		}
 
-		if (git_index_set_caps(repo->_index, GIT_INDEXCAP_FROM_OWNER) < 0)
-			return -1;
+		git_buf_free(&index_path);
 	}
 
 	*out = repo->_index;
-	return 0;
+	return error;
 }
 
 int git_repository_index(git_index **out, git_repository *repo)
@@ -727,11 +768,14 @@ void git_repository_set_index(git_repository *repo, git_index *index)
 {
 	assert(repo && index);
 
-	drop_index(repo);
-
-	repo->_index = index;
-	GIT_REFCOUNT_OWN(repo->_index, repo);
+	GIT_REFCOUNT_OWN(index, repo);
 	GIT_REFCOUNT_INC(index);
+
+	index = repo_swap_ptr(repo, &repo->_index, index);
+	if (index != NULL) {
+		GIT_REFCOUNT_OWN(index, NULL);
+		git_index_free(index);
+	}
 }
 
 static int check_repositoryformatversion(git_config *config)
diff --git a/src/repository.h b/src/repository.h
index cc2f8c2b8..873498de0 100644
--- a/src/repository.h
+++ b/src/repository.h
@@ -83,6 +83,7 @@ struct git_repository {
 	git_refdb *_refdb;
 	git_config *_config;
 	git_index *_index;
+	git_mutex lock;
 
 	git_cache objects;
 	git_attr_cache attrcache;
diff --git a/src/util.h b/src/util.h
index af3ef0b46..a2233a7e8 100644
--- a/src/util.h
+++ b/src/util.h
@@ -306,11 +306,30 @@ int git__date_parse(git_time_t *out, const char *date);
 
 /*
  * Unescapes a string in-place.
- * 
+ *
  * Edge cases behavior:
  * - "jackie\" -> "jacky\"
  * - "chan\\" -> "chan\"
  */
 extern size_t git__unescape(char *str);
 
+/*
+ * Swap a pointer with thread safety, returning old value.
+ */
+GIT_INLINE(void *) git__swap(git_mutex *lock, void **ptr_ptr, void *new_ptr)
+{
+	void *old_ptr;
+
+	if (*ptr_ptr == new_ptr)
+		return NULL;
+	if (git_mutex_lock(lock) < 0)
+		return new_ptr;
+
+	old_ptr = *ptr_ptr;
+	*ptr_ptr = new_ptr;
+
+	git_mutex_unlock(lock);
+	return old_ptr;
+}
+
 #endif /* INCLUDE_util_h__ */