Although undocumented, directory_exists_in_index_icase(dirname,len)
unconditionally assumes the presence of a '/' at dirname[len], despite
being past the end-of-string. Callers are expected to respect this
assumption by ensuring that a '/' is present beyond the last character
of the passed path.
directory_exists_in_index(), on the other hand, expects no trailing '/'
(and never looks beyond end-of-string). Callers are expected to respect
this by ensuring the absence of '/'.
2eac2a4cc4bdc8d7 (ls-files -k: a directory only can be killed if the
index has a non-directory; 2013-08-15) adds a caller which forgets to
ensure the trailing '/' beyond end-of-string, which leads to
inconsistent behavior between directory_exists_in_index() and
directory_exists_in_index_icase(), depending upon the setting of
core.ignorecase.
One approach to fix this would be to augment the new caller (added by
2eac2a4cc4bdc8d7) to add a '/' beyond end-of-string, but this places
extra burden on the caller to account for an implementation detail of
directory_exists_in_index_icase().
Another approach would be for directory_exists_in_index_icase() to take
responsibility for its own requirements by copying the incoming path and
adding a trailing '/', but such copying can become expensive.
The approach taken by this patch is to pass the strbufs already used by
their callers into directory_exists_in_index() and
directory_exists_in_index_icase() rather than 'const char *' + 'size_t
len'. This allows both functions to satisfy their own internal
requirements -- by manipulating the strbuf to add or remove the trailing
'/' -- without placing burden upon callers and without having to make
expensive copies of each incoming string.
This also fixes an initially-unnoticed failure, when core.ignorecase is
true, in a t3010 test added by 3c56875176390eee (t3010: update to
demonstrate "ls-files -k" optimization pitfalls; 2013-08-15).
Signed-off-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
---
dir.c | 42 +++++++++++++++++++++++--------------
t/t3010-ls-files-killed-modified.sh | 2 +-
2 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/dir.c b/dir.c
index edd666a..6f761a1 100644
--- a/dir.c
+++ b/dir.c
@@ -887,14 +887,21 @@ enum exist_status {
* the directory name; instead, use the case insensitive
* name hash.
*/
-static enum exist_status directory_exists_in_index_icase(const char *dirname, int len)
+static enum exist_status directory_exists_in_index_icase(struct strbuf *dirname)
{
- const struct cache_entry *ce = cache_name_exists(dirname, len + 1, ignore_case);
+ const struct cache_entry *ce;
unsigned char endchar;
+ size_t len = dirname->len;
+ int need_slash = len && dirname->buf[len - 1] != '/';
+
+ if (need_slash)
+ strbuf_addch(dirname, '/');
+ ce = cache_name_exists(dirname->buf, dirname->len, ignore_case);
+ strbuf_setlen(dirname, len);
if (!ce)
return index_nonexistent;
- endchar = ce->name[len];
+ endchar = ce->name[need_slash ? len : len - 1];
/*
* The cache_entry structure returned will contain this dirname
@@ -922,21 +929,25 @@ static enum exist_status directory_exists_in_index_icase(const char *dirname, in
* the files it contains) will sort with the '/' at the
* end.
*/
-static enum exist_status directory_exists_in_index(const char *dirname, int len)
+static enum exist_status directory_exists_in_index(struct strbuf *dirname)
{
- int pos;
+ int pos, len;
if (ignore_case)
- return directory_exists_in_index_icase(dirname, len);
+ return directory_exists_in_index_icase(dirname);
+
+ len = dirname->len;
+ if (len && dirname->buf[len - 1] == '/')
+ len--;
- pos = cache_name_pos(dirname, len);
+ pos = cache_name_pos(dirname->buf, len);
if (pos < 0)
pos = -pos-1;
while (pos < active_nr) {
const struct cache_entry *ce = active_cache[pos++];
unsigned char endchar;
- if (strncmp(ce->name, dirname, len))
+ if (strncmp(ce->name, dirname->buf, len))
break;
endchar = ce->name[len];
if (endchar > '/')
@@ -983,11 +994,10 @@ static enum exist_status directory_exists_in_index(const char *dirname, int len)
* (c) otherwise, we recurse into it.
*/
static enum path_treatment treat_directory(struct dir_struct *dir,
- const char *dirname, int len, int exclude,
+ struct strbuf *dirname, int exclude,
const struct path_simplify *simplify)
{
- /* The "len-1" is to strip the final '/' */
- switch (directory_exists_in_index(dirname, len-1)) {
+ switch (directory_exists_in_index(dirname)) {
case index_directory:
return path_recurse;
@@ -999,7 +1009,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
break;
if (!(dir->flags & DIR_NO_GITLINKS)) {
unsigned char sha1[20];
- if (resolve_gitlink_ref(dirname, "HEAD", sha1) == 0)
+ if (!resolve_gitlink_ref(dirname->buf, "HEAD", sha1))
return path_untracked;
}
return path_recurse;
@@ -1010,7 +1020,8 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
if (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
return exclude ? path_excluded : path_untracked;
- return read_directory_recursive(dir, dirname, len, 1, simplify);
+ return read_directory_recursive(dir, dirname->buf, dirname->len, 1,
+ simplify);
}
/*
@@ -1161,7 +1172,7 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
if ((dir->flags & DIR_COLLECT_KILLED_ONLY) &&
(dtype == DT_DIR) &&
!has_path_in_index &&
- (directory_exists_in_index(path->buf, path->len) == index_nonexistent))
+ (directory_exists_in_index(path) == index_nonexistent))
return path_none;
exclude = is_excluded(dir, path->buf, &dtype);
@@ -1178,8 +1189,7 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
return path_none;
case DT_DIR:
strbuf_addch(path, '/');
- return treat_directory(dir, path->buf, path->len, exclude,
- simplify);
+ return treat_directory(dir, path, exclude, simplify);
case DT_REG:
case DT_LNK:
return exclude ? path_excluded : path_untracked;
diff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh
index 198d308..78954bd 100755
--- a/t/t3010-ls-files-killed-modified.sh
+++ b/t/t3010-ls-files-killed-modified.sh
@@ -107,7 +107,7 @@ test_expect_success 'git ls-files -k to show killed files (icase).' '
git -c core.ignorecase=true ls-files -k >.output
'
-test_expect_failure 'validate git ls-files -k output (icase).' '
+test_expect_success 'validate git ls-files -k output (icase).' '
test_cmp .expected .output
'
--
1.8.4.rc4.557.g46c5bb2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html