@matheusbsbs / matheus-bagliano / commits / c30a574a0b

has_dir_name(): do not get confused by characters < '/'

There is a bug in directory/file ("D/F") conflict checking optimization: It assumes that such a conflict cannot happen if a newly added entry's path is lexicgraphically "greater than" the last already-existing index entry _and_ contains a directory separator that comes strictly after the common prefix (`len > len_eq_offset`). This assumption is incorrect, though: `a-` sorts _between_ `a` and `a/b`, their common prefix is `a`, the slash comes after the common prefix, and there is still a file/directory conflict. Let's re-design this logic, taking these facts into consideration: - It is impossible for a file to sort after another file with whose directory it conflicts because the trailing NUL byte is always smaller than any other character. - Since there are quite a number of ASCII characters that sort before the slash (e.g. `-`, `.`, the space character), looking at the last already-existing index entry is not enough to determine whether there is a D/F conflict when the first character different from the existing last index entry's path is a slash. If it is not a slash, there cannot be a file/directory conflict. And if the existing index entry's first different character is a slash, it also cannot be a file/directory conflict because the optimization requires the newly-added entry's path to sort _after_ the existing entry's, and the conflicting file's path would not. So let's fall back to the regular binary search whenever the newly-added item's path differs in a slash character. If it does not, and it sorts after the last index entry, there is no D/F conflict and the new index entry can be safely appended. This fix also nicely simplifies the logic and makes it much easier to reason about, while the impact on performance should be negligible: After this fix, the optimization will be skipped only when index entry's paths differ in a slash and a space, `!`, `"`, `#`, `$`, `%`, `&`, `'`, | ( `)`, `*`, `+`, `,`, `-`, or `.`, which should be a rare situation. Signed-off-by: Filip Hejsek <filip.hejsek@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Filip Hejsek committed Jan 28, 2024 at 04:30 UTC c30a574a0b50e64f26885f740dd49d2420b9bed7

2 files changed +47 -53

read-cache.c

+19 -53

@@ -1186,19 +1186,32 @@ static int has_dir_name(struct index_state *istate,

1186 istate->cache[istate->cache_nr - 1]->name,

1187 &len_eq_last);

1188 if (cmp_last > 0) {

1189 - if (len_eq_last == 0) {

1189 + if (name[len_eq_last] != '/') {

1190 /*

1191 * The entry sorts AFTER the last one in the

1192 - * index and their paths have no common prefix,

1193 - * so there cannot be a F/D conflict.

1192 + * index.

1193 + *

1194 + * If there were a conflict with "file", then our

1195 + * name would start with "file/" and the last index

1196 + * entry would start with "file" but not "file/".

1197 + *

1198 + * The next character after common prefix is

1199 + * not '/', so there can be no conflict.

1200 */

1201 return retval;

1202 } else {

1203 /*

1204 * The entry sorts AFTER the last one in the

1199 - * index, but has a common prefix. Fall through

1200 - * to the loop below to disect the entry's path

1201 - * and see where the difference is.

1205 + * index, and the next character after common

1206 + * prefix is '/'.

1207 + *

1208 + * Either the last index entry is a file in

1209 + * conflict with this entry, or it has a name

1210 + * which sorts between this entry and the

1211 + * potential conflicting file.

1212 + *

1213 + * In both cases, we fall through to the loop

1214 + * below and let the regular search code handle it.

1215 */

1216 }

1217 } else if (cmp_last == 0) {

@@ -1222,53 +1235,6 @@ static int has_dir_name(struct index_state *istate,

1235 }

1236 len = slash - name;

1237

1225 - if (cmp_last > 0) {

1226 - /*

1227 - * (len + 1) is a directory boundary (including

1228 - * the trailing slash). And since the loop is

1229 - * decrementing "slash", the first iteration is

1230 - * the longest directory prefix; subsequent

1231 - * iterations consider parent directories.

1232 - */

1233 -

1234 - if (len + 1 <= len_eq_last) {

1235 - /*

1236 - * The directory prefix (including the trailing

1237 - * slash) also appears as a prefix in the last

1238 - * entry, so the remainder cannot collide (because

1239 - * strcmp said the whole path was greater).

1240 - *

1241 - * EQ: last: xxx/A

1242 - * this: xxx/B

1243 - *

1244 - * LT: last: xxx/file_A

1245 - * this: xxx/file_B

1246 - */

1247 - return retval;

1248 - }

1249 -

1250 - if (len > len_eq_last) {

1251 - /*

1252 - * This part of the directory prefix (excluding

1253 - * the trailing slash) is longer than the known

1254 - * equal portions, so this sub-directory cannot

1255 - * collide with a file.

1256 - *

1257 - * GT: last: xxxA

1258 - * this: xxxB/file

1259 - */

1260 - return retval;

1261 - }

1262 -

1263 - /*

1264 - * This is a possible collision. Fall through and

1265 - * let the regular search code handle it.

1266 - *

1267 - * last: xxx

1268 - * this: xxx/file

1269 - */

1270 - }

1271 -

1238 pos = index_name_stage_pos(istate, name, len, stage, EXPAND_SPARSE);

1239 if (pos >= 0) {

1240 /*

t/t0000-basic.sh

+28

@@ -1200,6 +1200,34 @@ test_expect_success 'very long name in the index handled sanely' '

1200 test $len = 4098

1201 '

1202

1203 +# D/F conflict checking uses an optimization when adding to the end.

1204 +# make sure it does not get confused by `a-` sorting _between_

1205 +# `a` and `a/`.

1206 +test_expect_success 'more update-index D/F conflicts' '

1207 + # empty the index to make sure our entry is last

1208 + git read-tree --empty &&

1209 + cacheinfo=100644,$(test_oid empty_blob) &&

1210 + git update-index --add --cacheinfo $cacheinfo,path5/a &&

1211 +

1212 + test_must_fail git update-index --add --cacheinfo $cacheinfo,path5/a/file &&

1213 + test_must_fail git update-index --add --cacheinfo $cacheinfo,path5/a/b/file &&

1214 + test_must_fail git update-index --add --cacheinfo $cacheinfo,path5/a/b/c/file &&

1215 +

1216 + # "a-" sorts between "a" and "a/"

1217 + git update-index --add --cacheinfo $cacheinfo,path5/a- &&

1218 +

1219 + test_must_fail git update-index --add --cacheinfo $cacheinfo,path5/a/file &&

1220 + test_must_fail git update-index --add --cacheinfo $cacheinfo,path5/a/b/file &&

1221 + test_must_fail git update-index --add --cacheinfo $cacheinfo,path5/a/b/c/file &&

1222 +

1223 + cat >expected <<-\EOF &&

1224 + path5/a

1225 + path5/a-

1226 + EOF

1227 + git ls-files >actual &&

1228 + test_cmp expected actual

1229 +'

1230 +

1231 test_expect_success 'test_must_fail on a failing git command' '

1232 test_must_fail git notacommand

1233 '