Merge pull request libgit2#4818 from pks-t/pks/index-collision

Index collision fixes

Author: pks-t

src/index.c

@@ -1096,7 +1096,6 @@ static int index_entry_dup_nocache(
 static int has_file_name(git_index *index,
 	 const git_index_entry *entry, size_t pos, int ok_to_replace)
 {
-	int retval = 0;
 	size_t len = strlen(entry->path);
 	int stage = GIT_IDXENTRY_STAGE(entry);
 	const char *name = entry->path;
@@ -1112,14 +1111,13 @@ static int has_file_name(git_index *index,
 			continue;
 		if (p->path[len] != '/')
 			continue;
-		retval = -1;
 		if (!ok_to_replace)
-			break;
+			return -1;
 
 		if (index_remove_entry(index, --pos) < 0)
 			break;
 	}
-	return retval;
+	return 0;
 }
 
 /*
@@ -1129,7 +1127,6 @@ static int has_file_name(git_index *index,
 static int has_dir_name(git_index *index,
 		const git_index_entry *entry, int ok_to_replace)
 {
-	int retval = 0;
 	int stage = GIT_IDXENTRY_STAGE(entry);
 	const char *name = entry->path;
 	const char *slash = name + strlen(name);
@@ -1141,14 +1138,13 @@ static int has_dir_name(git_index *index,
 			if (*--slash == '/')
 				break;
 			if (slash <= entry->path)
-				return retval;
+				return 0;
 		}
 		len = slash - name;
 
 		if (!index_find(&pos, index, name, len, stage)) {
-			retval = -1;
 			if (!ok_to_replace)
-				break;
+				return -1;
 
 			if (index_remove_entry(index, pos) < 0)
 				break;
@@ -1169,20 +1165,18 @@ static int has_dir_name(git_index *index,
 				break; /* not our subdirectory */
 
 			if (GIT_IDXENTRY_STAGE(&p->entry) == stage)
-				return retval;
+				return 0;
 		}
 	}
 
-	return retval;
+	return 0;
 }
 
 static int check_file_directory_collision(git_index *index,
 		git_index_entry *entry, size_t pos, int ok_to_replace)
 {
-	int retval = has_file_name(index, entry, pos, ok_to_replace);
-	retval = retval + has_dir_name(index, entry, ok_to_replace);
-
-	if (retval) {
+	if (has_file_name(index, entry, pos, ok_to_replace) < 0 ||
+	    has_dir_name(index, entry, ok_to_replace) < 0) {
 		giterr_set(GITERR_INDEX,
 			"'%s' appears as both a file and a directory", entry->path);
 		return -1;
@@ -1337,57 +1331,58 @@ static int index_insert(
 	bool trust_mode,
 	bool trust_id)
 {
-	int error = 0;
-	size_t path_length, position;
 	git_index_entry *existing, *best, *entry;
+	size_t path_length, position;
+	int error;
 
 	assert(index && entry_ptr);
 
 	entry = *entry_ptr;
 
-	/* make sure that the path length flag is correct */
+	/* Make sure that the path length flag is correct */
 	path_length = ((struct entry_internal *)entry)->pathlen;
 	index_entry_adjust_namemask(entry, path_length);
 
-	/* this entry is now up-to-date and should not be checked for raciness */
+	/* This entry is now up-to-date and should not be checked for raciness */
 	entry->flags_extended |= GIT_IDXENTRY_UPTODATE;
 
 	git_vector_sort(&index->entries);
 
-	/* look if an entry with this path already exists, either staged, or (if
+	/*
+	 * Look if an entry with this path already exists, either staged, or (if
 	 * this entry is a regular staged item) as the "ours" side of a conflict.
 	 */
 	index_existing_and_best(&existing, &position, &best, index, entry);
 
-	/* update the file mode */
+	/* Update the file mode */
 	entry->mode = trust_mode ?
 		git_index__create_mode(entry->mode) :
 		index_merge_mode(index, best, entry->mode);
 
-	/* canonicalize the directory name */
-	if (!trust_path)
-		error = canonicalize_directory_path(index, entry, best);
+	/* Canonicalize the directory name */
+	if (!trust_path && (error = canonicalize_directory_path(index, entry, best)) < 0)
+		goto out;
 
-	/* ensure that the given id exists (unless it's a submodule) */
-	if (!error && !trust_id && INDEX_OWNER(index) &&
-		(entry->mode & GIT_FILEMODE_COMMIT) != GIT_FILEMODE_COMMIT) {
+	/* Ensure that the given id exists (unless it's a submodule) */
+	if (!trust_id && INDEX_OWNER(index) &&
+	    (entry->mode & GIT_FILEMODE_COMMIT) != GIT_FILEMODE_COMMIT) {
 
 		if (!git_object__is_valid(INDEX_OWNER(index), &entry->id,
-			git_object__type_from_filemode(entry->mode)))
+					  git_object__type_from_filemode(entry->mode))) {
 			error = -1;
+			goto out;
+		}
 	}
 
-	/* look for tree / blob name collisions, removing conflicts if requested */
-	if (!error)
-		error = check_file_directory_collision(index, entry, position, replace);
-
-	if (error < 0)
-		/* skip changes */;
+	/* Look for tree / blob name collisions, removing conflicts if requested */
+	if ((error = check_file_directory_collision(index, entry, position, replace)) < 0)
+		goto out;
 
-	/* if we are replacing an existing item, overwrite the existing entry
+	/*
+	 * If we are replacing an existing item, overwrite the existing entry
 	 * and return it in place of the passed in one.
 	 */
-	else if (existing) {
+	if (existing) {
 		if (replace) {
 			index_entry_cpy(existing, entry);
 
@@ -1396,25 +1391,25 @@ static int index_insert(
 		}
 
 		index_entry_free(entry);
-		*entry_ptr = entry = existing;
-	}
-	else {
-		/* if replace is not requested or no existing entry exists, insert
+		*entry_ptr = existing;
+	} else {
+		/*
+		 * 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_sorted(&index->entries, entry, index_no_dups);
+		if ((error = git_vector_insert_sorted(&index->entries, entry, index_no_dups)) < 0)
+			goto out;
 
-		if (error == 0) {
-			INSERT_IN_MAP(index, entry, &error);
-		}
+		INSERT_IN_MAP(index, entry, &error);
 	}
 
+	index->dirty = 1;
+
+out:
 	if (error < 0) {
 		index_entry_free(*entry_ptr);
 		*entry_ptr = NULL;
-	} else {
-		index->dirty = 1;
 	}
 
 	return error;

tests/index/collision.c

@@ -23,9 +23,10 @@ void test_index_collision__cleanup(void)
 	cl_git_sandbox_cleanup();
 }
 
-void test_index_collision__add(void)
+void test_index_collision__add_blob_with_conflicting_file(void)
 {
 	git_index_entry entry;
+	git_tree_entry *tentry;
 	git_oid tree_id;
 	git_tree *tree;
 
@@ -39,13 +40,59 @@ void test_index_collision__add(void)
 	entry.path = "a/b";
 	cl_git_pass(git_index_add(g_index, &entry));
 
+	/* Check a/b exists here */
+	cl_git_pass(git_index_write_tree(&tree_id, g_index));
+	cl_git_pass(git_tree_lookup(&tree, g_repo, &tree_id));
+	cl_git_pass(git_tree_entry_bypath(&tentry, tree, "a/b"));
+	git_tree_entry_free(tentry);
+	git_tree_free(tree);
+
 	/* create a tree/blob collision */
 	entry.path = "a/b/c";
-	cl_git_fail(git_index_add(g_index, &entry));
+	cl_git_pass(git_index_add(g_index, &entry));
 
+	/* a/b should now be a tree and a/b/c a blob */
 	cl_git_pass(git_index_write_tree(&tree_id, g_index));
 	cl_git_pass(git_tree_lookup(&tree, g_repo, &tree_id));
+	cl_git_pass(git_tree_entry_bypath(&tentry, tree, "a/b/c"));
+	git_tree_entry_free(tentry);
+	git_tree_free(tree);
+}
+
+void test_index_collision__add_blob_with_conflicting_dir(void)
+{
+	git_index_entry entry;
+	git_tree_entry *tentry;
+	git_oid tree_id;
+	git_tree *tree;
 
+	memset(&entry, 0, sizeof(entry));
+	entry.ctime.seconds = 12346789;
+	entry.mtime.seconds = 12346789;
+	entry.mode  = 0100644;
+	entry.file_size = 0;
+	git_oid_cpy(&entry.id, &g_empty_id);
+
+	entry.path = "a/b/c";
+	cl_git_pass(git_index_add(g_index, &entry));
+
+	/* Check a/b/c exists here */
+	cl_git_pass(git_index_write_tree(&tree_id, g_index));
+	cl_git_pass(git_tree_lookup(&tree, g_repo, &tree_id));
+	cl_git_pass(git_tree_entry_bypath(&tentry, tree, "a/b/c"));
+	git_tree_entry_free(tentry);
+	git_tree_free(tree);
+
+	/* create a blob/tree collision */
+	entry.path = "a/b";
+	cl_git_pass(git_index_add(g_index, &entry));
+
+	/* a/b should now be a tree and a/b/c a blob */
+	cl_git_pass(git_index_write_tree(&tree_id, g_index));
+	cl_git_pass(git_tree_lookup(&tree, g_repo, &tree_id));
+	cl_git_pass(git_tree_entry_bypath(&tentry, tree, "a/b"));
+	cl_git_fail(git_tree_entry_bypath(&tentry, tree, "a/b/c"));
+	git_tree_entry_free(tentry);
 	git_tree_free(tree);
 }