Convert sortedcache to use rwlock

This is the first use we have of pthread_rwlock_t in libgit2.
Hopefully it won't cause any serious portability problems.

5 files changed, 265 insertions, 185 deletions

diff --git a/src/refdb_fs.c b/src/refdb_fs.c
index 876e8458..04516a5b 100644
--- a/src/refdb_fs.c
+++ b/src/refdb_fs.c
@@ -172,7 +172,7 @@ static int packed_reload(refdb_fs_backend *backend)
 			ref->flags |= PACKREF_CANNOT_PEEL;
 	}
 
-	git_sortedcache_unlock(backend->refcache);
+	git_sortedcache_wunlock(backend->refcache);
 	git_buf_free(&packedrefs);
 
 	return 0;
@@ -181,7 +181,7 @@ parse_failed:
 	giterr_set(GITERR_REFERENCE, "Corrupted packed references file");
 
 	git_sortedcache_clear(backend->refcache, false);
-	git_sortedcache_unlock(backend->refcache);
+	git_sortedcache_wunlock(backend->refcache);
 	git_buf_free(&packedrefs);
 
 	return -1;
@@ -244,7 +244,7 @@ static int loose_lookup_to_packfile(refdb_fs_backend *backend, const char *name)
 	if ((error = loose_parse_oid(&oid, name, &ref_file)) < 0)
 		goto done;
 
-	git_sortedcache_lock(backend->refcache);
+	git_sortedcache_wlock(backend->refcache);
 
 	if (!(error = git_sortedcache_upsert(
 			(void **)&ref, backend->refcache, name))) {
@@ -253,7 +253,7 @@ static int loose_lookup_to_packfile(refdb_fs_backend *backend, const char *name)
 		ref->flags = PACKREF_WAS_LOOSE;
 	}
 
-	git_sortedcache_unlock(backend->refcache);
+	git_sortedcache_wunlock(backend->refcache);
 
 done:
 	git_buf_free(&ref_file);
@@ -317,7 +317,7 @@ static int refdb_fs_backend__exists(
 		return -1;
 
 	*exists = git_path_isfile(ref_path.ptr) ||
-		git_sortedcache_lookup(backend->refcache, ref_name);
+		(git_sortedcache_lookup(backend->refcache, ref_name) != NULL);
 
 	git_buf_free(&ref_path);
 	return 0;
@@ -395,7 +395,8 @@ static int packed_lookup(
 	if (packed_reload(backend) < 0)
 		return -1;
 
-	git_sortedcache_lock(backend->refcache);
+	if (git_sortedcache_rlock(backend->refcache) < 0)
+		return -1;
 
 	entry = git_sortedcache_lookup(backend->refcache, ref_name);
 	if (!entry) {
@@ -406,7 +407,8 @@ static int packed_lookup(
 			error = -1;
 	}
 
-	git_sortedcache_unlock(backend->refcache);
+	git_sortedcache_runlock(backend->refcache);
+
 	return error;
 }
 
@@ -486,11 +488,11 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
 			(iter->glob && p_fnmatch(iter->glob, ref_name, 0) != 0))
 			continue;
 
-		git_sortedcache_lock(backend->refcache);
+		git_sortedcache_rlock(backend->refcache);
 		ref = git_sortedcache_lookup(backend->refcache, ref_name);
 		if (ref)
 			ref->flags |= PACKREF_SHADOWED;
-		git_sortedcache_unlock(backend->refcache);
+		git_sortedcache_runlock(backend->refcache);
 
 		ref_dup = git_pool_strdup(&iter->pool, ref_name);
 		if (!ref_dup)
@@ -508,6 +510,7 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
 static int refdb_fs_backend__iterator_next(
 	git_reference **out, git_reference_iterator *_iter)
 {
+	int error = GIT_ITEROVER;
 	refdb_fs_iter *iter = (refdb_fs_iter *)_iter;
 	refdb_fs_backend *backend = (refdb_fs_backend *)iter->parent.db->backend;
 	struct packref *ref;
@@ -521,7 +524,7 @@ static int refdb_fs_backend__iterator_next(
 		giterr_clear();
 	}
 
-	git_sortedcache_lock(backend->refcache);
+	git_sortedcache_rlock(backend->refcache);
 
 	while (iter->packed_pos < git_sortedcache_entrycount(backend->refcache)) {
 		ref = git_sortedcache_entry(backend->refcache, iter->packed_pos++);
@@ -530,57 +533,56 @@ static int refdb_fs_backend__iterator_next(
 
 		if (ref->flags & PACKREF_SHADOWED)
 			continue;
-
 		if (iter->glob && p_fnmatch(iter->glob, ref->name, 0) != 0)
 			continue;
 
 		*out = git_reference__alloc(ref->name, &ref->oid, &ref->peel);
-		git_sortedcache_unlock(backend->refcache);
-		return (*out != NULL) ? 0 : -1;
+		error = (*out != NULL) ? 0 : -1;
+		break;
 	}
 
-	git_sortedcache_unlock(backend->refcache);
-	return GIT_ITEROVER;
+	git_sortedcache_runlock(backend->refcache);
+	return error;
 }
 
 static int refdb_fs_backend__iterator_next_name(
 	const char **out, git_reference_iterator *_iter)
 {
+	int error = GIT_ITEROVER;
 	refdb_fs_iter *iter = (refdb_fs_iter *)_iter;
 	refdb_fs_backend *backend = (refdb_fs_backend *)iter->parent.db->backend;
+	struct packref *ref;
 
 	while (iter->loose_pos < iter->loose.length) {
 		const char *path = git_vector_get(&iter->loose, iter->loose_pos++);
 
-		if (loose_lookup(NULL, backend, path) != 0) {
-			giterr_clear();
-			continue;
+		if (loose_lookup(NULL, backend, path) == 0) {
+			*out = path;
+			return 0;
 		}
 
-		*out = path;
-		return 0;
+		giterr_clear();
 	}
 
-	git_sortedcache_lock(backend->refcache);
+	git_sortedcache_rlock(backend->refcache);
 
 	while (iter->packed_pos < git_sortedcache_entrycount(backend->refcache)) {
-		struct packref *ref =
-			git_sortedcache_entry(backend->refcache, iter->packed_pos++);
+		ref = git_sortedcache_entry(backend->refcache, iter->packed_pos++);
+		if (!ref) /* stop now if another thread deleted refs and we past end */
+			break;
 
 		if (ref->flags & PACKREF_SHADOWED)
 			continue;
-
-		*out = ref->name;
-
-		if (iter->glob && p_fnmatch(iter->glob, *out, 0) != 0)
+		if (iter->glob && p_fnmatch(iter->glob, ref->name, 0) != 0)
 			continue;
 
-		git_sortedcache_unlock(backend->refcache);
-		return 0;
+		*out = ref->name;
+		error = 0;
+		break;
 	}
 
-	git_sortedcache_unlock(backend->refcache);
-	return GIT_ITEROVER;
+	git_sortedcache_runlock(backend->refcache);
+	return error;
 }
 
 static int refdb_fs_backend__iterator(
@@ -658,26 +660,25 @@ static int reference_path_available(
 		if (exists) {
 			giterr_set(GITERR_REFERENCE,
 				"Failed to write reference '%s': a reference with "
-				" that name already exists.", new_ref);
+				"that name already exists.", new_ref);
 			return GIT_EEXISTS;
 		}
 	}
 
-	git_sortedcache_lock(backend->refcache);
+	git_sortedcache_rlock(backend->refcache);
 
 	for (i = 0; i < git_sortedcache_entrycount(backend->refcache); ++i) {
-		struct packref *this_ref =
-			git_sortedcache_entry(backend->refcache, i);
+		struct packref *ref = git_sortedcache_entry(backend->refcache, i);
 
-		if (!ref_is_available(old_ref, new_ref, this_ref->name)) {
-			git_sortedcache_unlock(backend->refcache);
+		if (ref && !ref_is_available(old_ref, new_ref, ref->name)) {
+			git_sortedcache_runlock(backend->refcache);
 			giterr_set(GITERR_REFERENCE,
-				"The path to reference '%s' collides with an existing one", new_ref);
+				"Path to reference '%s' collides with existing one", new_ref);
 			return -1;
 		}
 	}
 
-	git_sortedcache_unlock(backend->refcache);
+	git_sortedcache_runlock(backend->refcache);
 	return 0;
 }
 
@@ -816,7 +817,7 @@ static int packed_remove_loose(refdb_fs_backend *backend)
 	for (i = 0; i < git_sortedcache_entrycount(backend->refcache); ++i) {
 		struct packref *ref = git_sortedcache_entry(backend->refcache, i);
 
-		if ((ref->flags & PACKREF_WAS_LOOSE) == 0)
+		if (!ref || !(ref->flags & PACKREF_WAS_LOOSE))
 			continue;
 
 		if (git_buf_joinpath(&full_path, backend->path, ref->name) < 0)
@@ -849,67 +850,53 @@ static int packed_remove_loose(refdb_fs_backend *backend)
  */
 static int packed_write(refdb_fs_backend *backend)
 {
+	git_sortedcache *refcache = backend->refcache;
 	git_filebuf pack_file = GIT_FILEBUF_INIT;
 	size_t i;
-	git_buf pack_file_path = GIT_BUF_INIT;
 
 	/* lock the cache to updates while we do this */
-	if (git_sortedcache_lock(backend->refcache) < 0)
+	if (git_sortedcache_wlock(refcache) < 0)
 		return -1;
 
 	/* Open the file! */
-	if (git_buf_joinpath(
-			&pack_file_path, backend->path, GIT_PACKEDREFS_FILE) < 0)
-		goto cleanup_memory;
-
-	if (git_filebuf_open(&pack_file, pack_file_path.ptr, 0) < 0)
-		goto cleanup_packfile;
+	if (git_filebuf_open(&pack_file, git_sortedcache_path(refcache), 0) < 0)
+		goto fail;
 
 	/* Packfiles have a header... apparently
 	 * This is in fact not required, but we might as well print it
 	 * just for kicks */
 	if (git_filebuf_printf(&pack_file, "%s\n", GIT_PACKEDREFS_HEADER) < 0)
-		goto cleanup_packfile;
+		goto fail;
 
-	for (i = 0; i < git_sortedcache_entrycount(backend->refcache); ++i) {
-		struct packref *ref =
-			git_sortedcache_entry(backend->refcache, i);
+	for (i = 0; i < git_sortedcache_entrycount(refcache); ++i) {
+		struct packref *ref = git_sortedcache_entry(refcache, i);
 
 		if (packed_find_peel(backend, ref) < 0)
-			goto cleanup_packfile;
+			goto fail;
 
 		if (packed_write_ref(ref, &pack_file) < 0)
-			goto cleanup_packfile;
+			goto fail;
 	}
 
 	/* if we've written all the references properly, we can commit
 	 * the packfile to make the changes effective */
 	if (git_filebuf_commit(&pack_file, GIT_PACKEDREFS_FILE_MODE) < 0)
-		goto cleanup_memory;
+		goto fail;
 
 	/* when and only when the packfile has been properly written,
 	 * we can go ahead and remove the loose refs */
 	if (packed_remove_loose(backend) < 0)
-		goto cleanup_memory;
-
-	git_sortedcache_unlock(backend->refcache);
-
-	 /* update filestamp to latest value */
-	if (git_futils_filestamp_check(
-			&backend->refcache->stamp, pack_file_path.ptr) < 0)
-		giterr_clear();
+		goto fail;
 
-	git_buf_free(&pack_file_path);
+	git_sortedcache_updated(refcache);
+	git_sortedcache_wunlock(refcache);
 
 	/* we're good now */
 	return 0;
 
-cleanup_packfile:
+fail:
 	git_filebuf_cleanup(&pack_file);
-
-cleanup_memory:
-	git_sortedcache_unlock(backend->refcache);
-	git_buf_free(&pack_file_path);
+	git_sortedcache_wunlock(refcache);
 
 	return -1;
 }
@@ -961,14 +948,14 @@ static int refdb_fs_backend__delete(
 		return -1;
 
 	/* If a packed reference exists, remove it from the packfile and repack */
-	if (git_sortedcache_lock(backend->refcache) < 0)
+	if (git_sortedcache_wlock(backend->refcache) < 0)
 		return -1;
 
 	if (!(error = git_sortedcache_lookup_index(
 			&pack_pos, backend->refcache, ref_name)))
-		error = git_sortedcache_remove(backend->refcache, pack_pos, false);
+		error = git_sortedcache_remove(backend->refcache, pack_pos);
 
-	git_sortedcache_unlock(backend->refcache);
+	git_sortedcache_wunlock(backend->refcache);
 
 	if (error == GIT_ENOTFOUND)
 		return loose_deleted ? 0 : ref_error_notfound(ref_name);
diff --git a/src/sortedcache.c b/src/sortedcache.c
index d0a8031c..c087dbbe 100644
--- a/src/sortedcache.c
+++ b/src/sortedcache.c
@@ -23,13 +23,13 @@ int git_sortedcache_new(
 		(sc->map = git_strmap_alloc()) == NULL)
 		goto fail;
 
-	if (git_mutex_init(&sc->lock)) {
-		giterr_set(GITERR_OS, "Failed to initialize mutex");
+	if (git_rwlock_init(&sc->lock)) {
+		giterr_set(GITERR_OS, "Failed to initialize lock");
 		goto fail;
 	}
 
-	sc->item_path_offset = item_path_offset;
-	sc->free_item = free_item;
+	sc->item_path_offset  = item_path_offset;
+	sc->free_item         = free_item;
 	sc->free_item_payload = free_item_payload;
 	GIT_REFCOUNT_INC(sc);
 	if (pathlen)
@@ -52,6 +52,11 @@ void git_sortedcache_incref(git_sortedcache *sc)
 	GIT_REFCOUNT_INC(sc);
 }
 
+const char *git_sortedcache_path(git_sortedcache *sc)
+{
+	return sc->path;
+}
+
 static void sortedcache_clear(git_sortedcache *sc)
 {
 	git_strmap_clear(sc->map);
@@ -72,19 +77,17 @@ static void sortedcache_clear(git_sortedcache *sc)
 
 static void sortedcache_free(git_sortedcache *sc)
 {
-	if (git_mutex_lock(&sc->lock) < 0) {
-		giterr_set(GITERR_OS, "Unable to acquire mutex lock for free");
+	/* acquire write lock to make sure everyone else is done */
+	if (git_sortedcache_wlock(sc) < 0)
 		return;
-	}
 
 	sortedcache_clear(sc);
-
 	git_vector_free(&sc->items);
 	git_strmap_free(sc->map);
 
-	git_mutex_unlock(&sc->lock);
-	git_mutex_free(&sc->lock);
+	git_sortedcache_wunlock(sc);
 
+	git_rwlock_free(&sc->lock);
 	git__free(sc);
 }
 
@@ -107,86 +110,86 @@ static int sortedcache_copy_item(void *payload, void *tgt_item, void *src_item)
 int git_sortedcache_copy(
 	git_sortedcache **out,
 	git_sortedcache *src,
+	bool wlock,
 	int (*copy_item)(void *payload, void *tgt_item, void *src_item),
 	void *payload)
 {
+	int error = 0;
 	git_sortedcache *tgt;
 	size_t i;
 	void *src_item, *tgt_item;
 
+	/* just use memcpy if no special copy fn is passed in */
 	if (!copy_item) {
 		copy_item = sortedcache_copy_item;
 		payload   = src;
 	}
 
-	if (git_sortedcache_new(
+	if ((error = git_sortedcache_new(
 			&tgt, src->item_path_offset,
 			src->free_item, src->free_item_payload,
-			src->items._cmp, src->path) < 0)
-		return -1;
+			src->items._cmp, src->path)) < 0)
+		return error;
 
-	if (git_sortedcache_lock(src) < 0) {
+	if (wlock && git_sortedcache_wlock(src) < 0) {
 		git_sortedcache_free(tgt);
 		return -1;
 	}
 
 	git_vector_foreach(&src->items, i, src_item) {
-		if (git_sortedcache_upsert(
-				&tgt_item, tgt, ((char *)src_item) + src->item_path_offset) < 0)
-			goto fail;
-		if (copy_item(payload, tgt_item, src_item) < 0)
-			goto fail;
+		char *path = ((char *)src_item) + src->item_path_offset;
+
+		if ((error = git_sortedcache_upsert(&tgt_item, tgt, path)) < 0 ||
+			(error = copy_item(payload, tgt_item, src_item)) < 0)
+			break;
 	}
 
-	git_sortedcache_unlock(src);
+	if (wlock)
+		git_sortedcache_wunlock(src);
+	if (error)
+		git_sortedcache_free(tgt);
 
-	*out = tgt;
-	return 0;
+	*out = !error ? tgt : NULL;
 
-fail:
-	git_sortedcache_unlock(src);
-	git_sortedcache_free(tgt);
-	return -1;
+	return error;
 }
 
-/* release all items in sorted cache */
-void git_sortedcache_clear(git_sortedcache *sc, bool lock)
+/* lock sortedcache while making modifications */
+int git_sortedcache_wlock(git_sortedcache *sc)
 {
-	if (lock && git_mutex_lock(&sc->lock) < 0) {
-		giterr_set(GITERR_OS, "Unable to acquire mutex lock for clear");
-		return;
-	}
+	GIT_UNUSED(sc); /* prevent warning when compiled w/o threads */
 
-	sortedcache_clear(sc);
-
-	if (lock)
-		git_mutex_unlock(&sc->lock);
+	if (git_rwlock_wrlock(&sc->lock) < 0) {
+		giterr_set(GITERR_OS, "Unable to acquire write lock on cache");
+		return -1;
+	}
+	return 0;
 }
 
-/* check file stamp to see if reload is required */
-bool git_sortedcache_out_of_date(git_sortedcache *sc)
+/* unlock sorted cache when done with modifications */
+void git_sortedcache_wunlock(git_sortedcache *sc)
 {
-	return (git_futils_filestamp_check(&sc->stamp, sc->path) != 0);
+	git_vector_sort(&sc->items);
+	git_rwlock_wrunlock(&sc->lock);
 }
 
-/* lock sortedcache while making modifications */
-int git_sortedcache_lock(git_sortedcache *sc)
+/* lock sortedcache for read */
+int git_sortedcache_rlock(git_sortedcache *sc)
 {
-	GIT_UNUSED(sc); /* to prevent warning when compiled w/o threads */
+	GIT_UNUSED(sc); /* prevent warning when compiled w/o threads */
 
-	if (git_mutex_lock(&sc->lock) < 0) {
-		giterr_set(GITERR_OS, "Unable to acquire mutex lock");
+	if (git_rwlock_rdlock(&sc->lock) < 0) {
+		giterr_set(GITERR_OS, "Unable to acquire read lock on cache");
 		return -1;
 	}
 	return 0;
 }
 
-/* unlock sorted cache when done with modifications */
-int git_sortedcache_unlock(git_sortedcache *sc)
+/* unlock sorted cache when done reading */
+void git_sortedcache_runlock(git_sortedcache *sc)
 {
-	git_vector_sort(&sc->items);
-	git_mutex_unlock(&sc->lock);
-	return 0;
+	GIT_UNUSED(sc); /* prevent warning when compiled w/o threads */
+	git_rwlock_rdunlock(&sc->lock);
 }
 
 /* if the file has changed, lock cache and load file contents into buf;
@@ -196,7 +199,7 @@ int git_sortedcache_lockandload(git_sortedcache *sc, git_buf *buf)
 {
 	int error, fd;
 
-	if ((error = git_sortedcache_lock(sc)) < 0)
+	if ((error = git_sortedcache_wlock(sc)) < 0)
 		return error;
 
 	if ((error = git_futils_filestamp_check(&sc->stamp, sc->path)) <= 0)
@@ -224,13 +227,33 @@ int git_sortedcache_lockandload(git_sortedcache *sc, git_buf *buf)
 	return 1; /* return 1 -> file needs reload and was successfully loaded */
 
 unlock:
-	git_sortedcache_unlock(sc);
+	git_sortedcache_wunlock(sc);
 	return error;
 }
 
+void git_sortedcache_updated(git_sortedcache *sc)
+{
+	 /* update filestamp to latest value */
+	if (git_futils_filestamp_check(&sc->stamp, sc->path) < 0)
+		giterr_clear();
+}
+
+/* release all items in sorted cache */
+int git_sortedcache_clear(git_sortedcache *sc, bool wlock)
+{
+	if (wlock && git_sortedcache_wlock(sc) < 0)
+		return -1;
+
+	sortedcache_clear(sc);
+
+	if (wlock)
+		git_sortedcache_wunlock(sc);
+
+	return 0;
+}
+
 /* find and/or insert item, returning pointer to item data */
-int git_sortedcache_upsert(
-	void **out, git_sortedcache *sc, const char *key)
+int git_sortedcache_upsert(void **out, git_sortedcache *sc, const char *key)
 {
 	int error = 0;
 	khiter_t pos;
@@ -246,7 +269,10 @@ int git_sortedcache_upsert(
 
 	keylen = strlen(key);
 	item = git_pool_mallocz(&sc->pool, sc->item_path_offset + keylen + 1);
-	GITERR_CHECK_ALLOC(item);
+	if (!item) { /* don't use GITERR_CHECK_ALLOC b/c of lock */
+		error = -1;
+		goto done;
+	}
 
 	/* one strange thing is that even if the vector or hash table insert
 	 * fail, there is no way to free the pool item so we just abandon it
@@ -289,11 +315,16 @@ size_t git_sortedcache_entrycount(const git_sortedcache *sc)
 }
 
 /* lookup item by index */
-void *git_sortedcache_entry(const git_sortedcache *sc, size_t pos)
+void *git_sortedcache_entry(git_sortedcache *sc, size_t pos)
 {
+	/* make sure the items are sorted so this gets the correct item */
+	if (!sc->items.sorted)
+		git_vector_sort(&sc->items);
+
 	return git_vector_get(&sc->items, pos);
 }
 
+/* helper struct so bsearch callback can know offset + key value for cmp */
 struct sortedcache_magic_key {
 	size_t offset;
 	const char *key;
@@ -319,23 +350,18 @@ int git_sortedcache_lookup_index(
 }
 
 /* remove entry from cache */
-int git_sortedcache_remove(git_sortedcache *sc, size_t pos, bool lock)
+int git_sortedcache_remove(git_sortedcache *sc, size_t pos)
 {
-	int error = 0;
 	char *item;
 	khiter_t mappos;
 
-	if (lock && git_sortedcache_lock(sc) < 0)
-		return -1;
-
 	/* because of pool allocation, this can't actually remove the item,
 	 * but we can remove it from the items vector and the hash table.
 	 */
 
 	if ((item = git_vector_get(&sc->items, pos)) == NULL) {
 		giterr_set(GITERR_INVALID, "Removing item out of range");
-		error = GIT_ENOTFOUND;
-		goto done;
+		return GIT_ENOTFOUND;
 	}
 
 	(void)git_vector_remove(&sc->items, pos);
@@ -346,9 +372,6 @@ int git_sortedcache_remove(git_sortedcache *sc, size_t pos, bool lock)
 	if (sc->free_item)
 		sc->free_item(sc->free_item_payload, item);
 
-done:
-	if (lock)
-		git_sortedcache_unlock(sc);
-	return error;
+	return 0;
 }
 
diff --git a/src/sortedcache.h b/src/sortedcache.h
index c1c9d134..7d1cd2f1 100644
--- a/src/sortedcache.h
+++ b/src/sortedcache.h
@@ -25,7 +25,7 @@ typedef void (*git_sortedcache_free_item_fn)(void *payload, void *item);
 
 typedef struct {
 	git_refcount rc;
-	git_mutex    lock;
+	git_rwlock   lock;
 	size_t       item_path_offset;
 	git_sortedcache_free_item_fn free_item;
 	void         *free_item_payload;
@@ -36,84 +36,120 @@ typedef struct {
 	char         path[GIT_FLEX_ARRAY];
 } git_sortedcache;
 
-/* create a new sortedcache
+/* Create a new sortedcache
  *
- * even though every sortedcache stores items with a GIT_FLEX_ARRAY at
+ * Even though every sortedcache stores items with a GIT_FLEX_ARRAY at
  * the end containing their key string, you have to provide the item_cmp
  * sorting function because the sorting function doesn't get a payload
  * and therefore can't know the offset to the item key string. :-(
  */
 int git_sortedcache_new(
 	git_sortedcache **out,
-	size_t item_path_offset, /* use offsetof() macro */
+	size_t item_path_offset, /* use offsetof(struct, path-field) macro */
 	git_sortedcache_free_item_fn free_item,
 	void *free_item_payload,
 	git_vector_cmp item_cmp,
 	const char *path);
 
-/* copy a sorted cache
+/* Copy a sorted cache
  *
- * - copy_item can be NULL to memcpy
- * - locks src while copying
+ * - `copy_item` can be NULL to just use memcpy
+ * - if `wlock`, grabs write lock on `src` during copy and releases after
  */
 int git_sortedcache_copy(
 	git_sortedcache **out,
 	git_sortedcache *src,
+	bool wlock,
 	int (*copy_item)(void *payload, void *tgt_item, void *src_item),
 	void *payload);
 
-/* free sorted cache (first calling free_item callbacks)
- * don't call on a locked collection - it may acquire a lock
+/* Free sorted cache (first calling `free_item` callbacks)
+ *
+ * Don't call on a locked collection - it may acquire a write lock
  */
 void git_sortedcache_free(git_sortedcache *sc);
 
-/* increment reference count - balance with call to free */
+/* Increment reference count - balance with call to free */
 void git_sortedcache_incref(git_sortedcache *sc);
 
-/* release all items in sorted cache - lock during clear if `lock` is true */
-void git_sortedcache_clear(git_sortedcache *sc, bool lock);
+/* Get the pathname associated with this cache at creation time */
+const char *git_sortedcache_path(git_sortedcache *sc);
 
-/* check file stamp to see if reload is required */
-bool git_sortedcache_out_of_date(git_sortedcache *sc);
+/*
+ * CACHE WRITE FUNCTIONS
+ *
+ * The following functions require you to have a writer lock to make the
+ * modification.  Some of the functions take a `wlock` parameter and
+ * will optionally lock and unlock for you if that is passed as true.
+ *
+ */
 
-/* lock sortedcache during access or modification */
-int git_sortedcache_lock(git_sortedcache *sc);
+/* Lock sortedcache for write */
+int git_sortedcache_wlock(git_sortedcache *sc);
 
-/* unlock sorted cache when done */
-int git_sortedcache_unlock(git_sortedcache *sc);
+/* Unlock sorted cache when done with write */
+void git_sortedcache_wunlock(git_sortedcache *sc);
 
-/* if the file has changed, lock cache and load file contents into buf;
+/* Lock cache and test if the file has changed.  If the file has changed,
+ * then load the contents into `buf` otherwise unlock and return 0.
+ *
  * @return 0 if up-to-date, 1 if out-of-date, <0 on error
  */
 int git_sortedcache_lockandload(git_sortedcache *sc, git_buf *buf);
 
-/* find and/or insert item, returning pointer to item data
- * should only call on locked collection
+/* Refresh file timestamp after write completes
+ * You should already be holding the write lock when you call this.
+ */
+void git_sortedcache_updated(git_sortedcache *sc);
+
+/* Release all items in sorted cache
+ *
+ * If `wlock` is true, grabs write lock and releases when done, otherwise
+ * you should already be holding a write lock when you call this.
+ */
+int git_sortedcache_clear(git_sortedcache *sc, bool wlock);
+
+/* Find and/or insert item, returning pointer to item data.
+ * You should already be holding the write lock when you call this.
  */
 int git_sortedcache_upsert(
 	void **out, git_sortedcache *sc, const char *key);
 
-/* lookup item by key
- * should only call on locked collection if return value will be used
+/* Removes entry at pos from cache
+ * You should already be holding the write lock when you call this.
+ */
+int git_sortedcache_remove(git_sortedcache *sc, size_t pos);
+
+/*
+ * CACHE READ FUNCTIONS
+ *
+ * The following functions access items in the cache.  To prevent the
+ * results from being invalidated before they can be used, you should be
+ * holding either a read lock or a write lock when using these functions.
+ *
  */
+
+/* Lock sortedcache for read */
+int git_sortedcache_rlock(git_sortedcache *sc);
+
+/* Unlock sorted cache when done with read */
+void git_sortedcache_runlock(git_sortedcache *sc);
+
+/* Lookup item by key - returns NULL if not found */
 void *git_sortedcache_lookup(const git_sortedcache *sc, const char *key);
 
-/* find out how many items are in the cache */
+/* Get how many items are in the cache
+ *
+ * You can call this function without holding a lock, but be aware
+ * that it may change before you use it.
+ */
 size_t git_sortedcache_entrycount(const git_sortedcache *sc);
 
-/* lookup item by index
- * should only call on locked collection if return value will be used
- */
-void *git_sortedcache_entry(const git_sortedcache *sc, size_t pos);
+/* Lookup item by index - returns NULL if out of range */
+void *git_sortedcache_entry(git_sortedcache *sc, size_t pos);
 
-/* lookup index of item by key
- * if collection is not locked, there is no guarantee the returned index
- * will be correct if it used to look up the item
- */
+/* Lookup index of item by key - returns GIT_ENOTFOUND if not found */
 int git_sortedcache_lookup_index(
 	size_t *out, git_sortedcache *sc, const char *key);
 
-/* remove entry from cache - lock during delete if `lock` is true */
-int git_sortedcache_remove(git_sortedcache *sc, size_t pos, bool lock);
-
 #endif
diff --git a/src/thread-utils.h b/src/thread-utils.h
index 04e02959..ffcdb4ab 100644
--- a/src/thread-utils.h
+++ b/src/thread-utils.h
@@ -41,7 +41,8 @@ typedef git_atomic git_atomic_ssize;
 #ifdef GIT_THREADS
 
 #define git_thread pthread_t
-#define git_thread_create(thread, attr, start_routine, arg) pthread_create(thread, attr, start_routine, arg)
+#define git_thread_create(thread, attr, start_routine, arg) \
+	pthread_create(thread, attr, start_routine, arg)
 #define git_thread_kill(thread) pthread_cancel(thread)
 #define git_thread_exit(status)	pthread_exit(status)
 #define git_thread_join(id, status) pthread_join(id, status)
@@ -61,6 +62,17 @@ typedef git_atomic git_atomic_ssize;
 #define git_cond_signal(c)	pthread_cond_signal(c)
 #define git_cond_broadcast(c)	pthread_cond_broadcast(c)
 
+/* Pthreads rwlock */
+#define git_rwlock pthread_rwlock_t
+#define git_rwlock_init(a)		pthread_rwlock_init(a, NULL)
+#define git_rwlock_rdlock(a)	pthread_rwlock_rdlock(a)
+#define git_rwlock_rdunlock(a)	pthread_rwlock_unlock(a)
+#define git_rwlock_wrlock(a)	pthread_rwlock_wrlock(a)
+#define git_rwlock_wrunlock(a)	pthread_rwlock_unlock(a)
+#define git_rwlock_free(a)		pthread_rwlock_destroy(a)
+#define GIT_RWLOCK_STATIC_INIT	PTHREAD_RWLOCK_INITIALIZER
+
+
 GIT_INLINE(void) git_atomic_set(git_atomic *a, int val)
 {
 #if defined(GIT_WIN32)
@@ -147,7 +159,7 @@ GIT_INLINE(int64_t) git_atomic64_add(git_atomic64 *a, int64_t addend)
 #else
 
 #define git_thread unsigned int
-#define git_thread_create(thread, attr, start_routine, arg) (void)0
+#define git_thread_create(thread, attr, start_routine, arg) 0
 #define git_thread_kill(thread) (void)0
 #define git_thread_exit(status) (void)0
 #define git_thread_join(id, status) (void)0
@@ -167,6 +179,17 @@ GIT_INLINE(int64_t) git_atomic64_add(git_atomic64 *a, int64_t addend)
 #define git_cond_signal(c) (void)0
 #define git_cond_broadcast(c) (void)0
 
+/* Pthreads rwlock */
+#define git_rwlock unsigned int
+#define git_rwlock_init(a)		0
+#define git_rwlock_rdlock(a)	0
+#define git_rwlock_rdunlock(a)	(void)0
+#define git_rwlock_wrlock(a)	0
+#define git_rwlock_wrunlock(a)	(void)0
+#define git_rwlock_free(a)		(void)0
+#define GIT_RWLOCK_STATIC_INIT	0
+
+
 GIT_INLINE(void) git_atomic_set(git_atomic *a, int val)
 {
 	a->val = val;
diff --git a/tests-clar/core/sortedcache.c b/tests-clar/core/sortedcache.c
index 91415943..509d0df6 100644
--- a/tests-clar/core/sortedcache.c
+++ b/tests-clar/core/sortedcache.c
@@ -15,13 +15,13 @@ void test_core_sortedcache__name_only(void)
 	cl_git_pass(git_sortedcache_new(
 		&sc, 0, NULL, NULL, name_only_cmp, NULL));
 
-	cl_git_pass(git_sortedcache_lock(sc));
+	cl_git_pass(git_sortedcache_wlock(sc));
 	cl_git_pass(git_sortedcache_upsert(&item, sc, "aaa"));
 	cl_git_pass(git_sortedcache_upsert(&item, sc, "bbb"));
 	cl_git_pass(git_sortedcache_upsert(&item, sc, "zzz"));
 	cl_git_pass(git_sortedcache_upsert(&item, sc, "mmm"));
 	cl_git_pass(git_sortedcache_upsert(&item, sc, "iii"));
-	cl_git_pass(git_sortedcache_unlock(sc));
+	git_sortedcache_wunlock(sc);
 
 	cl_assert_equal_sz(5, git_sortedcache_entrycount(sc));
 
@@ -95,7 +95,7 @@ void test_core_sortedcache__in_memory(void)
 		sortedcache_test_struct_free, &free_count,
 		sortedcache_test_struct_cmp, NULL));
 
-	cl_git_pass(git_sortedcache_lock(sc));
+	cl_git_pass(git_sortedcache_wlock(sc));
 	cl_git_pass(git_sortedcache_upsert((void **)&item, sc, "aaa"));
 	item->value = 10;
 	item->smaller_value = 1;
@@ -111,10 +111,12 @@ void test_core_sortedcache__in_memory(void)
 	cl_git_pass(git_sortedcache_upsert((void **)&item, sc, "iii"));
 	item->value = 50;
 	item->smaller_value = 9;
-	cl_git_pass(git_sortedcache_unlock(sc));
+	git_sortedcache_wunlock(sc);
 
 	cl_assert_equal_sz(5, git_sortedcache_entrycount(sc));
 
+	cl_git_pass(git_sortedcache_rlock(sc));
+
 	cl_assert((item = git_sortedcache_lookup(sc, "aaa")) != NULL);
 	cl_assert_equal_s("aaa", item->path);
 	cl_assert_equal_i(10, item->value);
@@ -126,6 +128,8 @@ void test_core_sortedcache__in_memory(void)
 	cl_assert_equal_i(30, item->value);
 	cl_assert(git_sortedcache_lookup(sc, "abc") == NULL);
 
+	cl_git_pass(git_sortedcache_rlock(sc)); /* grab more than one */
+
 	cl_assert((item = git_sortedcache_entry(sc, 0)) != NULL);
 	cl_assert_equal_s("aaa", item->path);
 	cl_assert_equal_i(10, item->value);
@@ -143,6 +147,9 @@ void test_core_sortedcache__in_memory(void)
 	cl_assert_equal_i(30, item->value);
 	cl_assert(git_sortedcache_entry(sc, 5) == NULL);
 
+	git_sortedcache_runlock(sc);
+	git_sortedcache_runlock(sc);
+
 	cl_assert_equal_i(0, free_count);
 
 	git_sortedcache_clear(sc, true);
@@ -156,7 +163,7 @@ void test_core_sortedcache__in_memory(void)
 
 	free_count = 0;
 
-	cl_git_pass(git_sortedcache_lock(sc));
+	cl_git_pass(git_sortedcache_wlock(sc));
 	cl_git_pass(git_sortedcache_upsert((void **)&item, sc, "testing"));
 	item->value = 10;
 	item->smaller_value = 3;
@@ -166,7 +173,7 @@ void test_core_sortedcache__in_memory(void)
 	cl_git_pass(git_sortedcache_upsert((void **)&item, sc, "final"));
 	item->value = 30;
 	item->smaller_value = 2;
-	cl_git_pass(git_sortedcache_unlock(sc));
+	git_sortedcache_wunlock(sc);
 
 	cl_assert_equal_sz(3, git_sortedcache_entrycount(sc));
 
@@ -195,9 +202,11 @@ void test_core_sortedcache__in_memory(void)
 	{
 		size_t pos;
 
+		cl_git_pass(git_sortedcache_wlock(sc));
+
 		cl_git_pass(git_sortedcache_lookup_index(&pos, sc, "again"));
 		cl_assert_equal_sz(0, pos);
-		cl_git_pass(git_sortedcache_remove(sc, pos, true));
+		cl_git_pass(git_sortedcache_remove(sc, pos));
 		cl_assert_equal_i(
 			GIT_ENOTFOUND, git_sortedcache_lookup_index(&pos, sc, "again"));
 
@@ -205,7 +214,7 @@ void test_core_sortedcache__in_memory(void)
 
 		cl_git_pass(git_sortedcache_lookup_index(&pos, sc, "testing"));
 		cl_assert_equal_sz(1, pos);
-		cl_git_pass(git_sortedcache_remove(sc, pos, true));
+		cl_git_pass(git_sortedcache_remove(sc, pos));
 		cl_assert_equal_i(
 			GIT_ENOTFOUND, git_sortedcache_lookup_index(&pos, sc, "testing"));
 
@@ -213,11 +222,13 @@ void test_core_sortedcache__in_memory(void)
 
 		cl_git_pass(git_sortedcache_lookup_index(&pos, sc, "final"));
 		cl_assert_equal_sz(0, pos);
-		cl_git_pass(git_sortedcache_remove(sc, pos, true));
+		cl_git_pass(git_sortedcache_remove(sc, pos));
 		cl_assert_equal_i(
 			GIT_ENOTFOUND, git_sortedcache_lookup_index(&pos, sc, "final"));
 
 		cl_assert_equal_sz(0, git_sortedcache_entrycount(sc));
+
+		git_sortedcache_wunlock(sc);
 	}
 
 	git_sortedcache_free(sc);
@@ -251,7 +262,7 @@ static void sortedcache_test_reload(git_sortedcache *sc)
 		item->smaller_value = (char)(count++);
 	}
 
-	cl_git_pass(git_sortedcache_unlock(sc));
+	git_sortedcache_wunlock(sc);
 
 	git_buf_free(&buf);
 }