Merge pull request #3511 from ethomson/racy_fixes_2

Racy fixes for writing new indexes

src/index.c

@@ -18,6 +18,7 @@
 #include "ignore.h"
 #include "blob.h"
 #include "idxmap.h"
+#include "diff.h"
 
 #include "git2/odb.h"
 #include "git2/oid.h"
@@ -752,7 +753,9 @@ static int truncate_racily_clean(git_index *index)
 	int error;
 	git_index_entry *entry;
 	git_diff_options diff_opts = GIT_DIFF_OPTIONS_INIT;
-	git_diff *diff;
+	git_diff *diff = NULL;
+	git_vector paths = GIT_VECTOR_INIT;
+	git_diff_delta *delta;
 
 	/* Nothing to do if there's no repo to talk about */
 	if (!INDEX_OWNER(index))
@@ -764,22 +767,33 @@ static int truncate_racily_clean(git_index *index)
 
 	diff_opts.flags |= GIT_DIFF_INCLUDE_TYPECHANGE | GIT_DIFF_IGNORE_SUBMODULES | GIT_DIFF_DISABLE_PATHSPEC_MATCH;
 	git_vector_foreach(&index->entries, i, entry) {
-		if (!is_racy_timestamp(&index->stamp.mtime, entry))
+		if ((entry->flags_extended & GIT_IDXENTRY_UPTODATE) == 0 &&
-			continue;
+			is_racy_timestamp(&index->stamp.mtime, entry))
+			git_vector_insert(&paths, (char *)entry->path);
+	}
 
-		/* TODO: use the (non-fnmatching) filelist iterator */
+	if (paths.length == 0)
-		diff_opts.pathspec.count = 1;
+		goto done;
-		diff_opts.pathspec.strings = (char **) &entry->path;
+
+	diff_opts.pathspec.count = paths.length;
+	diff_opts.pathspec.strings = (char **)paths.contents;
 
 	if ((error = git_diff_index_to_workdir(&diff, INDEX_OWNER(index), index, &diff_opts)) < 0)
 		return error;
 
-		if (git_diff_num_deltas(diff) > 0)
+	git_vector_foreach(&diff->deltas, i, delta) {
-			entry->file_size = 0;
+		entry = (git_index_entry *)git_index_get_bypath(index, delta->old_file.path, 0);
 
-		git_diff_free(diff);
+		/* Ensure that we have a stage 0 for this file (ie, it's not a
+		 * conflict), otherwise smudging it is quite pointless.
+		 */
+		if (entry)
+			entry->file_size = 0;
 	}
 
+done:
+	git_diff_free(diff);
+	git_vector_free(&paths);
 	return 0;
 }
 
@@ -989,20 +1003,11 @@ static int index_entry_reuc_init(git_index_reuc_entry **reuc_out,
 
 static void index_entry_cpy(
 	git_index_entry *tgt,
-	git_index *index,
+	const git_index_entry *src)
-	const git_index_entry *src,
-	bool update_path)
 {
 	const char *tgt_path = tgt->path;
 	memcpy(tgt, src, sizeof(*tgt));
-
-	/* keep the existing path buffer, but update the path to the one
-	 * given by the caller, if we trust it.
-	 */
 	tgt->path = tgt_path;
-
-	if (index->ignore_case && update_path)
-		memcpy((char *)tgt->path, src->path, strlen(tgt->path));
 }
 
 static int index_entry_dup(
@@ -1010,18 +1015,32 @@ static int index_entry_dup(
 	git_index *index,
 	const git_index_entry *src)
 {
-	git_index_entry *entry;
+	if (index_entry_create(out, INDEX_OWNER(index), src->path) < 0)
-
-	if (!src) {
-		*out = NULL;
-		return 0;
-	}
-
-	if (index_entry_create(&entry, INDEX_OWNER(index), src->path) < 0)
 		return -1;
 
-	index_entry_cpy(entry, index, src, false);
+	index_entry_cpy(*out, src);
-	*out = entry;
+	return 0;
+}
+
+static void index_entry_cpy_nocache(
+	git_index_entry *tgt,
+	const git_index_entry *src)
+{
+	git_oid_cpy(&tgt->id, &src->id);
+	tgt->mode = src->mode;
+	tgt->flags = src->flags;
+	tgt->flags_extended = (src->flags_extended & GIT_IDXENTRY_EXTENDED_FLAGS);
+}
+
+static int index_entry_dup_nocache(
+	git_index_entry **out,
+	git_index *index,
+	const git_index_entry *src)
+{
+	if (index_entry_create(out, INDEX_OWNER(index), src->path) < 0)
+		return -1;
+
+	index_entry_cpy_nocache(*out, src);
 	return 0;
 }
 
@@ -1282,6 +1301,9 @@ static int index_insert(
 	else
 		entry->flags |= GIT_IDXENTRY_NAMEMASK;
 
+	/* this entry is now up-to-date and should not be checked for raciness */
+	entry->flags_extended |= GIT_IDXENTRY_UPTODATE;
+
 	if (git_mutex_lock(&index->lock) < 0) {
 		giterr_set(GITERR_OS, "Unable to acquire index lock");
 		return -1;
@@ -1314,8 +1336,13 @@ static int index_insert(
 	 * and return it in place of the passed in one.
 	 */
 	else if (existing) {
-		if (replace)
+		if (replace) {
-			index_entry_cpy(existing, index, entry, trust_path);
+			index_entry_cpy(existing, entry);
+
+			if (trust_path)
+				memcpy((char *)existing->path, entry->path, strlen(entry->path));
+		}
+
 		index_entry_free(entry);
 		*entry_ptr = entry = existing;
 	}
@@ -1692,9 +1719,12 @@ int git_index_conflict_add(git_index *index,
 
 	assert (index);
 
-	if ((ret = index_entry_dup(&entries[0], index, ancestor_entry)) < 0 ||
+	if ((ancestor_entry &&
-		(ret = index_entry_dup(&entries[1], index, our_entry)) < 0 ||
+			(ret = index_entry_dup(&entries[0], index, ancestor_entry)) < 0) ||
-		(ret = index_entry_dup(&entries[2], index, their_entry)) < 0)
+		(our_entry &&
+			(ret = index_entry_dup(&entries[1], index, our_entry)) < 0) ||
+		(their_entry &&
+			(ret = index_entry_dup(&entries[2], index, their_entry)) < 0))
 		goto on_error;
 
 	/* Validate entries */
@@ -2545,7 +2575,8 @@ static int write_disk_entry(git_filebuf *file, git_index_entry *entry)
 	if (entry->flags & GIT_IDXENTRY_EXTENDED) {
 		struct entry_long *ondisk_ext;
 		ondisk_ext = (struct entry_long *)ondisk;
-		ondisk_ext->flags_extended = htons(entry->flags_extended);
+		ondisk_ext->flags_extended = htons(entry->flags_extended &
+			GIT_IDXENTRY_EXTENDED_FLAGS);
 		path = ondisk_ext->path;
 	}
 	else
@@ -2728,6 +2759,15 @@ static int write_tree_extension(git_index *index, git_filebuf *file)
 	return error;
 }
 
+static void clear_uptodate(git_index *index)
+{
+	git_index_entry *entry;
+	size_t i;
+
+	git_vector_foreach(&index->entries, i, entry)
+		entry->flags_extended &= ~GIT_IDXENTRY_UPTODATE;
+}
+
 static int write_index(git_oid *checksum, git_index *index, git_filebuf *file)
 {
 	git_oid hash_final;
@@ -2767,7 +2807,13 @@ static int write_index(git_oid *checksum, git_index *index, git_filebuf *file)
 	git_oid_cpy(checksum, &hash_final);
 
 	/* write it at the end of the file */
-	return git_filebuf_write(file, hash_final.id, GIT_OID_RAWSZ);
+	if (git_filebuf_write(file, hash_final.id, GIT_OID_RAWSZ) < 0)
+		return -1;
+
+	/* file entries are no longer up to date */
+	clear_uptodate(index);
+
+	return 0;
 }
 
 int git_index_entry_stage(const git_index_entry *entry)
@@ -2816,7 +2862,7 @@ static int read_tree_cb(
 		entry->mode == old_entry->mode &&
 		git_oid_equal(&entry->id, &old_entry->id))
 	{
-		index_entry_cpy(entry, data->index, old_entry, false);
+		index_entry_cpy(entry, old_entry);
 		entry->flags_extended = 0;
 	}
 
@@ -2941,7 +2987,10 @@ int git_index_read_index(
 		goto done;
 
 	while (true) {
-		git_index_entry *add_entry = NULL, *remove_entry = NULL;
+		git_index_entry
+			*dup_entry = NULL,
+			*add_entry = NULL,
+			*remove_entry = NULL;
 		int diff;
 
 		if (old_entry && new_entry)
@@ -2956,8 +3005,7 @@ int git_index_read_index(
 		if (diff < 0) {
 			remove_entry = (git_index_entry *)old_entry;
 		} else if (diff > 0) {
-			if ((error = index_entry_dup(&add_entry, index, new_entry)) < 0)
+			dup_entry = (git_index_entry *)new_entry;
-				goto done;
 		} else {
 			/* Path and stage are equal, if the OID is equal, keep it to
 			 * keep the stat cache data.
@@ -2965,13 +3013,16 @@ int git_index_read_index(
 			if (git_oid_equal(&old_entry->id, &new_entry->id)) {
 				add_entry = (git_index_entry *)old_entry;
 			} else {
-				if ((error = index_entry_dup(&add_entry, index, new_entry)) < 0)
+				dup_entry = (git_index_entry *)new_entry;
-					goto done;
-
 				remove_entry = (git_index_entry *)old_entry;
 			}
 		}
 
+		if (dup_entry) {
+			if ((error = index_entry_dup_nocache(&add_entry, index, dup_entry)) < 0)
+				goto done;
+		}
+
 		if (add_entry) {
 			if ((error = git_vector_insert(&new_entries, add_entry)) == 0)
 				INSERT_IN_MAP_EX(index, new_entries_map, add_entry, error);

tests/checkout/crlf.c

@@ -278,6 +278,7 @@ void test_checkout_crlf__autocrlf_true_index_size_is_filtered_size(void)
 void test_checkout_crlf__with_ident(void)
 {
 	git_index *index;
+	git_index_entry *entry;
 	git_blob *blob;
 	git_checkout_options opts = GIT_CHECKOUT_OPTIONS_INIT;
 	opts.checkout_strategy = GIT_CHECKOUT_FORCE;
@@ -310,14 +311,14 @@ void test_checkout_crlf__with_ident(void)
 
 	/* check that blobs have $Id$ */
 
-	cl_git_pass(git_blob_lookup(&blob, g_repo,
+	cl_assert((entry = git_index_get_bypath(index, "lf.ident", 0)));
-		& git_index_get_bypath(index, "lf.ident", 0)->id));
+	cl_git_pass(git_blob_lookup(&blob, g_repo, &entry->id));
 	cl_assert_equal_s(
 		ALL_LF_TEXT_RAW "\n$Id$\n", git_blob_rawcontent(blob));
 	git_blob_free(blob);
 
-	cl_git_pass(git_blob_lookup(&blob, g_repo,
+	cl_assert((entry = git_index_get_bypath(index, "more2.identcrlf", 0)));
-		& git_index_get_bypath(index, "more2.identcrlf", 0)->id));
+	cl_git_pass(git_blob_lookup(&blob, g_repo, &entry->id));
 	cl_assert_equal_s(
 		"\n$Id$\n" MORE_CRLF_TEXT_AS_LF, git_blob_rawcontent(blob));
 	git_blob_free(blob);

tests/index/racy.c

@@ -100,13 +100,13 @@ void test_index_racy__write_index_just_after_file(void)
 	git_index_free(index);
 }
 
-void test_index_racy__empty_file_after_smudge(void)
+
+static void setup_race(void)
 {
-	git_index *index;
-	git_diff *diff;
 	git_buf path = GIT_BUF_INIT;
-	int i, found_race = 0;
+	git_index *index;
 	const git_index_entry *entry;
+	int i, found_race = 0;
 
 	/* Make sure we do have a timestamp */
 	cl_git_pass(git_repository_index__weakptr(&index, g_repo));
@@ -131,17 +131,198 @@ void test_index_racy__empty_file_after_smudge(void)
 			found_race = 1;
 			break;
 		}
-
 	}
 
 	if (!found_race)
 		cl_fail("failed to find race after 10 attempts");
 
+	git_buf_free(&path);
+}
+
+void test_index_racy__smudges_index_entry_on_save(void)
+{
+	git_index *index;
+	const git_index_entry *entry;
+
+	setup_race();
+
+	/* write the index, which will smudge anything that had the same timestamp
+	 * as the index when the index was loaded.  that way future loads of the
+	 * index (with the new timestamp) will know that these files were not
+	 * clean.
+	 */
+
+	cl_git_pass(git_repository_index__weakptr(&index, g_repo));
+	cl_git_pass(git_index_write(index));
+
+	cl_assert(entry = git_index_get_bypath(index, "A", 0));
 	cl_assert_equal_i(0, entry->file_size);
+}
+
+void test_index_racy__detects_diff_of_change_in_identical_timestamp(void)
+{
+	git_index *index;
+	git_diff *diff;
+
+	cl_git_pass(git_repository_index__weakptr(&index, g_repo));
+
+	setup_race();
 
 	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, index, NULL));
 	cl_assert_equal_i(1, git_diff_num_deltas(diff));
 
-	git_buf_free(&path);
 	git_diff_free(diff);
 }
+
+static void setup_uptodate_files(void)
+{
+	git_buf path = GIT_BUF_INIT;
+	git_index *index;
+	git_index_entry new_entry = {0};
+
+	cl_git_pass(git_repository_index(&index, g_repo));
+
+	cl_git_pass(git_buf_joinpath(&path, git_repository_workdir(g_repo), "A"));
+	cl_git_mkfile(path.ptr, "A");
+
+	/* Put 'A' into the index */
+	cl_git_pass(git_index_add_bypath(index, "A"));
+
+	/* Put 'B' into the index */
+	new_entry.path = "B";
+	new_entry.mode = GIT_FILEMODE_BLOB;
+	cl_git_pass(git_index_add(index, &new_entry));
+
+	/* Put 'C' into the index */
+	new_entry.path = "C";
+	new_entry.mode = GIT_FILEMODE_BLOB;
+	cl_git_pass(git_index_add_frombuffer(index, &new_entry, "hello!\n", 7));
+
+	git_index_free(index);
+	git_buf_free(&path);
+}
+
+void test_index_racy__adding_to_index_is_uptodate(void)
+{
+	git_index *index;
+	const git_index_entry *entry;
+
+	setup_uptodate_files();
+
+	cl_git_pass(git_repository_index(&index, g_repo));
+
+	/* ensure that they're all uptodate */
+	cl_assert((entry = git_index_get_bypath(index, "A", 0)));
+	cl_assert_equal_i(GIT_IDXENTRY_UPTODATE, (entry->flags_extended & GIT_IDXENTRY_UPTODATE));
+
+	cl_assert((entry = git_index_get_bypath(index, "B", 0)));
+	cl_assert_equal_i(GIT_IDXENTRY_UPTODATE, (entry->flags_extended & GIT_IDXENTRY_UPTODATE));
+
+	cl_assert((entry = git_index_get_bypath(index, "C", 0)));
+	cl_assert_equal_i(GIT_IDXENTRY_UPTODATE, (entry->flags_extended & GIT_IDXENTRY_UPTODATE));
+
+	cl_git_pass(git_index_write(index));
+
+	git_index_free(index);
+}
+
+void test_index_racy__reading_clears_uptodate_bit(void)
+{
+	git_index *index;
+	const git_index_entry *entry;
+
+	setup_uptodate_files();
+
+	cl_git_pass(git_repository_index(&index, g_repo));
+	cl_git_pass(git_index_write(index));
+
+	cl_git_pass(git_index_read(index, true));
+
+	/* ensure that no files are uptodate */
+	cl_assert((entry = git_index_get_bypath(index, "A", 0)));
+	cl_assert_equal_i(0, (entry->flags_extended & GIT_IDXENTRY_UPTODATE));
+
+	cl_assert((entry = git_index_get_bypath(index, "B", 0)));
+	cl_assert_equal_i(0, (entry->flags_extended & GIT_IDXENTRY_UPTODATE));
+
+	cl_assert((entry = git_index_get_bypath(index, "C", 0)));
+	cl_assert_equal_i(0, (entry->flags_extended & GIT_IDXENTRY_UPTODATE));
+
+	git_index_free(index);
+}
+
+void test_index_racy__read_tree_clears_uptodate_bit(void)
+{
+	git_index *index;
+	git_tree *tree;
+	const git_index_entry *entry;
+	git_oid id;
+
+	setup_uptodate_files();
+
+	cl_git_pass(git_repository_index(&index, g_repo));
+	cl_git_pass(git_index_write_tree_to(&id, index, g_repo));
+	cl_git_pass(git_tree_lookup(&tree, g_repo, &id));
+	cl_git_pass(git_index_read_tree(index, tree));
+
+	/* ensure that no files are uptodate */
+	cl_assert((entry = git_index_get_bypath(index, "A", 0)));
+	cl_assert_equal_i(0, (entry->flags_extended & GIT_IDXENTRY_UPTODATE));
+
+	cl_assert((entry = git_index_get_bypath(index, "B", 0)));
+	cl_assert_equal_i(0, (entry->flags_extended & GIT_IDXENTRY_UPTODATE));
+
+	cl_assert((entry = git_index_get_bypath(index, "C", 0)));
+	cl_assert_equal_i(0, (entry->flags_extended & GIT_IDXENTRY_UPTODATE));
+
+	git_tree_free(tree);
+	git_index_free(index);
+}
+
+void test_index_racy__read_index_smudges(void)
+{
+	git_index *index, *newindex;
+	const git_index_entry *entry;
+
+	/* if we are reading an index into our new index, ensure that any
+	 * racy entries in the index that we're reading are smudged so that
+	 * we don't propagate their timestamps without further investigation.
+	 */
+	setup_race();
+
+	cl_git_pass(git_repository_index(&index, g_repo));
+	cl_git_pass(git_index_new(&newindex));
+	cl_git_pass(git_index_read_index(newindex, index));
+
+	cl_assert(entry = git_index_get_bypath(newindex, "A", 0));
+	cl_assert_equal_i(0, entry->file_size);
+
+	git_index_free(index);
+	git_index_free(newindex);
+}
+
+void test_index_racy__read_index_clears_uptodate_bit(void)
+{
+	git_index *index, *newindex;
+	const git_index_entry *entry;
+	git_oid id;
+
+	setup_uptodate_files();
+
+	cl_git_pass(git_repository_index(&index, g_repo));
+	cl_git_pass(git_index_new(&newindex));
+	cl_git_pass(git_index_read_index(newindex, index));
+
+	/* ensure that files brought in from the other index are not uptodate */
+	cl_assert((entry = git_index_get_bypath(newindex, "A", 0)));
+	cl_assert_equal_i(0, (entry->flags_extended & GIT_IDXENTRY_UPTODATE));
+
+	cl_assert((entry = git_index_get_bypath(newindex, "B", 0)));
+	cl_assert_equal_i(0, (entry->flags_extended & GIT_IDXENTRY_UPTODATE));
+
+	cl_assert((entry = git_index_get_bypath(newindex, "C", 0)));
+	cl_assert_equal_i(0, (entry->flags_extended & GIT_IDXENTRY_UPTODATE));
+
+	git_index_free(index);
+	git_index_free(newindex);
+}