Fix race checking for existing index items In the threading tests, I was still seeing a race condition where the same item could end up being inserted multiple times into the index. Preserving the sorted-ness of the index outside of the `index_insert` call fixes the issue.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173
diff --git a/src/index.c b/src/index.c
index d7d937f..083c01f 100644
--- a/src/index.c
+++ b/src/index.c
@@ -292,6 +292,7 @@ static void index_entry_reuc_free(git_index_reuc_entry *reuc)
static void index_entry_free(git_index_entry *entry)
{
+ memset(&entry->id, 0, sizeof(entry->id));
git__free(entry);
}
@@ -415,9 +416,9 @@ int git_index_open(git_index **index_out, const char *index_path)
}
if (git_vector_init(&index->entries, 32, git_index_entry_cmp) < 0 ||
- git_vector_init(&index->names, 32, conflict_name_cmp) < 0 ||
- git_vector_init(&index->reuc, 32, reuc_cmp) < 0 ||
- git_vector_init(&index->deleted, 2, git_index_entry_cmp) < 0)
+ git_vector_init(&index->names, 8, conflict_name_cmp) < 0 ||
+ git_vector_init(&index->reuc, 8, reuc_cmp) < 0 ||
+ git_vector_init(&index->deleted, 8, git_index_entry_cmp) < 0)
goto fail;
index->entries_cmp_path = git__strcmp_cb;
@@ -477,7 +478,7 @@ static void index_free_deleted(git_index *index)
int readers = (int)git_atomic_get(&index->readers);
size_t i;
- if (readers > 0)
+ if (readers > 0 || !index->deleted.length)
return;
for (i = 0; i < index->deleted.length; ++i) {
@@ -500,12 +501,11 @@ static int index_remove_entry(git_index *index, size_t pos)
error = git_vector_remove(&index->entries, pos);
if (!error) {
- int readers = (int)git_atomic_get(&index->readers);
-
- if (readers > 0)
+ if (git_atomic_get(&index->readers) > 0) {
error = git_vector_insert(&index->deleted, entry);
- else
+ } else {
index_entry_free(entry);
+ }
}
return error;
@@ -529,13 +529,13 @@ int git_index_clear(git_index *index)
error = index_remove_entry(index, index->entries.length - 1);
index_free_deleted(index);
- git_mutex_unlock(&index->lock);
-
git_index_reuc_clear(index);
git_index_name_clear(index);
git_futils_filestamp_set(&index->stamp, NULL);
+ git_mutex_unlock(&index->lock);
+
return error;
}
@@ -860,6 +860,7 @@ static int index_entry_dup(git_index_entry **out, const git_index_entry *src)
GITERR_CHECK_ALLOC(entry);
index_entry_cpy(entry, src);
+
return 0;
}
@@ -961,6 +962,15 @@ static int check_file_directory_collision(git_index *index,
return 0;
}
+static int index_no_dups(void **old, void *new)
+{
+ const git_index_entry *entry = new;
+ GIT_UNUSED(old);
+ giterr_set(GITERR_INDEX, "'%s' appears multiple times at stage %d",
+ entry->path, GIT_IDXENTRY_STAGE(entry));
+ return GIT_EEXISTS;
+}
+
/* index_insert takes ownership of the new entry - if it can't insert
* it, then it will return an error **and also free the entry**. When
* it replaces an existing entry, it will update the entry_ptr with the
@@ -987,9 +997,9 @@ static int index_insert(
else
entry->flags |= GIT_IDXENTRY_NAMEMASK;
- if ((error = git_mutex_lock(&index->lock)) < 0) {
+ if (git_mutex_lock(&index->lock) < 0) {
giterr_set(GITERR_OS, "Unable to acquire index lock");
- return error;
+ return -1;
}
git_vector_sort(&index->entries);
@@ -1010,25 +1020,27 @@ static int index_insert(
/* if we are replacing an existing item, overwrite the existing entry
* and return it in place of the passed in one.
*/
- else if (existing && replace) {
- index_entry_cpy(existing, entry);
+ else if (existing) {
+ if (replace)
+ index_entry_cpy(existing, entry);
index_entry_free(entry);
- *entry_ptr = existing;
+ *entry_ptr = entry = existing;
}
else {
- /* if replacing is not requested or no existing entry exists, just
- * insert entry at the end; the index is no longer sorted
+ /* if replace is not requested or no existing entry exists, insert
+ * at the sorted position. (Since we re-sort after each insert to
+ * check for dups, this is actually cheaper in the long run.)
*/
- error = git_vector_insert(&index->entries, entry);
+ error = git_vector_insert_sorted(&index->entries, entry, index_no_dups);
}
- git_mutex_unlock(&index->lock);
-
if (error < 0) {
index_entry_free(*entry_ptr);
*entry_ptr = NULL;
}
+ git_mutex_unlock(&index->lock);
+
return error;
}
diff --git a/tests/threads/diff.c b/tests/threads/diff.c
index 562eec7..d33e75f 100644
--- a/tests/threads/diff.c
+++ b/tests/threads/diff.c
@@ -15,8 +15,14 @@ void test_threads_diff__cleanup(void)
static void setup_trees(void)
{
+ git_index *idx;
+
_repo = cl_git_sandbox_reopen(); /* reopen sandbox to flush caches */
+ /* avoid competing to load initial index */
+ cl_git_pass(git_repository_index(&idx, _repo));
+ git_index_free(idx);
+
cl_git_pass(git_revparse_single(
(git_object **)&_a, _repo, "0017bd4ab1^{tree}"));
cl_git_pass(git_revparse_single(
@@ -107,7 +113,7 @@ void test_threads_diff__concurrent_diffs(void)
_check_counts = 1;
run_in_parallel(
- 20, 32, run_index_diffs, setup_trees, free_trees);
+ 5, 32, run_index_diffs, setup_trees, free_trees);
}
static void *run_index_diffs_with_modifier(void *arg)
@@ -169,5 +175,5 @@ void test_threads_diff__with_concurrent_index_modified(void)
_check_counts = 0;
run_in_parallel(
- 20, 32, run_index_diffs_with_modifier, setup_trees, free_trees);
+ 5, 16, run_index_diffs_with_modifier, setup_trees, free_trees);
}