Lock attribute file while reparsing data

I don't love this approach, but achieving thread-safety for
attribute and ignore data while reloading files would require a
larger rewrite in order to avoid this.  If an attribute or ignore
file is out of date, this holds a lock on the file while we are
reloading the data so that another thread won't try to reload the
data at the same time.

3 files changed, 49 insertions, 17 deletions

diff --git a/src/attr_file.c b/src/attr_file.c
index 66f71ad19..574de25bb 100644
--- a/src/attr_file.c
+++ b/src/attr_file.c
@@ -9,8 +9,13 @@
 
 static void attr_file_free(git_attr_file *file)
 {
-	git_attr_file__clear_rules(file);
+	bool unlock = !git_mutex_lock(&file->lock);
+	git_attr_file__clear_rules(file, false);
 	git_pool_clear(&file->pool);
+	if (unlock)
+		git_mutex_unlock(&file->lock);
+	git_mutex_free(&file->lock);
+
 	git__memzero(file, sizeof(*file));
 	git__free(file);
 }
@@ -23,6 +28,12 @@ int git_attr_file__new(
 	git_attr_file *attrs = git__calloc(1, sizeof(git_attr_file));
 	GITERR_CHECK_ALLOC(attrs);
 
+	if (git_mutex_init(&attrs->lock) < 0) {
+		giterr_set(GITERR_OS, "Failed to initialize lock");
+		git__free(attrs);
+		return -1;
+	}
+
 	if (git_pool_init(&attrs->pool, 1, 0) < 0) {
 		attr_file_free(attrs);
 		return -1;
@@ -35,14 +46,24 @@ int git_attr_file__new(
 	return 0;
 }
 
-void git_attr_file__clear_rules(git_attr_file *file)
+int git_attr_file__clear_rules(git_attr_file *file, bool need_lock)
 {
 	unsigned int i;
 	git_attr_rule *rule;
 
+	if (need_lock && git_mutex_lock(&file->lock) < 0) {
+		giterr_set(GITERR_OS, "Failed to lock attribute file");
+		return -1;
+	}
+
 	git_vector_foreach(&file->rules, i, rule)
 		git_attr_rule__free(rule);
 	git_vector_free(&file->rules);
+
+	if (need_lock)
+		git_mutex_unlock(&file->lock);
+
+	return 0;
 }
 
 void git_attr_file__free(git_attr_file *file)
@@ -162,6 +183,11 @@ int git_attr_file__parse_buffer(
 		!git__suffixcmp(attrs->ce->path, "/" GIT_ATTR_FILE))
 		context = attrs->ce->path;
 
+	if (git_mutex_lock(&attrs->lock) < 0) {
+		giterr_set(GITERR_OS, "Failed to lock attribute file");
+		return -1;
+	}
+
 	while (!error && *scan) {
 		/* allocate rule if needed */
 		if (!rule) {
@@ -198,6 +224,7 @@ int git_attr_file__parse_buffer(
 		}
 	}
 
+	git_mutex_unlock(&attrs->lock);
 	git_attr_rule__free(rule);
 
 	return error;
diff --git a/src/attr_file.h b/src/attr_file.h
index f92ce3c96..9b4b8724b 100644
--- a/src/attr_file.h
+++ b/src/attr_file.h
@@ -66,6 +66,7 @@ typedef struct {
 
 struct git_attr_file {
 	git_refcount rc;
+	git_mutex lock;
 	git_attr_cache_entry *ce;
 	git_attr_cache_source source;
 	git_vector rules;			/* vector of <rule*> or <fnmatch*> */
@@ -115,7 +116,8 @@ int git_attr_file__parse_buffer(
 	const char *data,
 	void *payload);
 
-void git_attr_file__clear_rules(git_attr_file *file);
+int git_attr_file__clear_rules(
+	git_attr_file *file, bool need_lock);
 
 int git_attr_file__lookup_one(
 	git_attr_file *file,
diff --git a/src/ignore.c b/src/ignore.c
index 1f9df8ab4..fbebd9ad3 100644
--- a/src/ignore.c
+++ b/src/ignore.c
@@ -32,6 +32,11 @@ static int parse_ignore_file(
 		!git__suffixcmp(attrs->ce->path, "/" GIT_IGNORE_FILE))
 		context = attrs->ce->path;
 
+	if (git_mutex_lock(&attrs->lock) < 0) {
+		giterr_set(GITERR_OS, "Failed to lock attribute file");
+		return -1;
+	}
+
 	while (!error && *scan) {
 		if (!match) {
 			match = git__calloc(1, sizeof(*match));
@@ -63,6 +68,7 @@ static int parse_ignore_file(
 		}
 	}
 
+	git_mutex_unlock(&attrs->lock);
 	git__free(match);
 
 	return error;
@@ -247,12 +253,12 @@ void git_ignore__free(git_ignores *ignores)
 }
 
 static bool ignore_lookup_in_rules(
-	git_vector *rules, git_attr_path *path, int *ignored)
+	git_attr_file *file, git_attr_path *path, int *ignored)
 {
 	size_t j;
 	git_attr_fnmatch *match;
 
-	git_vector_rforeach(rules, j, match) {
+	git_vector_rforeach(&file->rules, j, match) {
 		if (git_attr_fnmatch__match(match, path)) {
 			*ignored = ((match->flags & GIT_ATTR_FNMATCH_NEGATIVE) == 0);
 			return true;
@@ -274,19 +280,18 @@ int git_ignore__lookup(
 		return -1;
 
 	/* first process builtins - success means path was found */
-	if (ignore_lookup_in_rules(
-			&ignores->ign_internal->rules, &path, ignored))
+	if (ignore_lookup_in_rules(ignores->ign_internal, &path, ignored))
 		goto cleanup;
 
 	/* next process files in the path */
 	git_vector_foreach(&ignores->ign_path, i, file) {
-		if (ignore_lookup_in_rules(&file->rules, &path, ignored))
+		if (ignore_lookup_in_rules(file, &path, ignored))
 			goto cleanup;
 	}
 
 	/* last process global ignores */
 	git_vector_foreach(&ignores->ign_global, i, file) {
-		if (ignore_lookup_in_rules(&file->rules, &path, ignored))
+		if (ignore_lookup_in_rules(file, &path, ignored))
 			goto cleanup;
 	}
 
@@ -319,10 +324,9 @@ int git_ignore_clear_internal_rules(
 	git_attr_file *ign_internal;
 
 	if (!(error = get_internal_ignores(&ign_internal, repo))) {
-		git_attr_file__clear_rules(ign_internal);
-
-		error = parse_ignore_file(
-			repo, ign_internal, GIT_IGNORE_DEFAULT_RULES, NULL);
+		if (!(error = git_attr_file__clear_rules(ign_internal, true)))
+			error = parse_ignore_file(
+				repo, ign_internal, GIT_IGNORE_DEFAULT_RULES, NULL);
 
 		git_attr_file__free(ign_internal);
 	}
@@ -371,19 +375,18 @@ int git_ignore_path_is_ignored(
 			break;
 
 		/* first process builtins - success means path was found */
-		if (ignore_lookup_in_rules(
-				&ignores.ign_internal->rules, &path, ignored))
+		if (ignore_lookup_in_rules(ignores.ign_internal, &path, ignored))
 			goto cleanup;
 
 		/* next process files in the path */
 		git_vector_foreach(&ignores.ign_path, i, file) {
-			if (ignore_lookup_in_rules(&file->rules, &path, ignored))
+			if (ignore_lookup_in_rules(file, &path, ignored))
 				goto cleanup;
 		}
 
 		/* last process global ignores */
 		git_vector_foreach(&ignores.ign_global, i, file) {
-			if (ignore_lookup_in_rules(&file->rules, &path, ignored))
+			if (ignore_lookup_in_rules(file, &path, ignored))
 				goto cleanup;
 		}