Commit 8b6e28958c99b4f9effbbedb84ae779b3b1b9d98
2018-09-21T15:18:03
index: fix adding index entries with conflicting files When adding an index entry "a/b/c" while an index entry "a/b" already exists, git will happily remove "a/b/c" and only add the new index entry: $ git init test Initialized empty Git repository in /tmp/test.repo/test/.git/ $ touch x $ git add x $ rm x $ mkdir x $ touch x/y $ git add x/y $ git status A x/y The other way round, adding an index entry "a/b" with an entry "a/b/c" already existing is equivalent, where git will remove "a/b/c" and add "a/b". In contrast, libgit2 will currently fail to add these properly and instead complain about the entry appearing as both a file and a directory. This is a programming error, though: our current code already tries to detect and, in the case of `git_index_add`, to automatically replace such index entries. Funnily enough, we already remove the conflicting index entries, but instead of adding the new entry we then bail out afterwards. This leaves callers with the worst of both worlds: we both remove the old entry but fail to add the new one. The root cause is weird semantics of the `has_file_name` and `has_dir_name` functions. While these functions only sound like they are responsible for detecting such conflicts, they will also already remove them in case where its `ok_to_replace` parameter is set. But even if we tell it to replace such entries, it will return an error code. Fix the error by returning success in case where the entries have been replaced. Fix an already existing test which tested for wrong behaviour. Note that the test didn't notice that the resulting tree had no entries. Thus it is fine to change existing behaviour here, as the previous result could've let to silently loosing data. Also add a new test that verifies behaviour in the reverse conflicting case.
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
diff --git a/src/index.c b/src/index.c
index 0e0dfe8..239a178 100644
--- a/src/index.c
+++ b/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;
diff --git a/tests/index/collision.c b/tests/index/collision.c
index 41eb7bf..699c985 100644
--- a/tests/index/collision.c
+++ b/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);
}