[PATCH v2 0/2] fix t3010 failure when core.ignorecase=true

[PATCH v2 0/2] fix t3010 failure when core.ignorecase=true

[Eric Sunshine]

This is a re-roll of [1] which fixes a bug in dir.c resulting in failure
of a newly added test in t3010 when core.ignorecase=true.

Recent mailing list discussion [2][3] suggests that changes at a more
fundamental level (if they pan out) would be a more appropriate approach
to resolving this bug.  Such an approach would (happily) make patch 2/2
unnecessary, however, I'm posting this series for posterity since it was
already written before [2][3]. Also, since I'm learning the relevant git
internals as I work on these issues, any fix at the more fundamental
level (if it comes from me) is going to be correspondingly delayed since
there is so much more to digest.

Changes since v1:

patch 1: place the new test in t3010 instead of t3103 [4]

patch 2: make directory_exists_in_index() and
directory_exists_in_index_icase() responsible for their own internal
requirements regarding presence or lack of trailing '/' [5]; also, pass
in already-existing strbufs so that directory_exists_in_index_icase()
doesn't have to make its own copy of the incoming string in order to
append '/'

[1]: http://thread.gmane.org/gmane.comp.version-control.git/232796
[2]: http://thread.gmane.org/gmane.comp.version-control.git/232727/focus=232813
[3]: http://thread.gmane.org/gmane.comp.version-control.git/232727/focus=232822
[4]: http://thread.gmane.org/gmane.comp.version-control.git/232796/focus=232814
[5]: http://thread.gmane.org/gmane.comp.version-control.git/232727/focus=232800

Eric Sunshine (2):
  t3103: demonstrate dir.c:treat_one_path() core.ignorecase failure
  dir: test_one_path: fix inconsistent behavior due to missing '/'

 dir.c                               | 42 +++++++++++++++++++++++--------------
 t/t3010-ls-files-killed-modified.sh |  8 +++++++
 2 files changed, 34 insertions(+), 16 deletions(-)

-- 
1.8.4.rc4.557.g46c5bb2

[PATCH v2 1/2] t3103: demonstrate dir.c:treat_one_path() core.ignorecase failure

[Eric Sunshine]

2eac2a4cc4bdc8d7 (ls-files -k: a directory only can be killed if the
index has a non-directory; 2013-08-15) adds a caller of
directory_exists_in_index(dirname,len) which forgets to satisfy the
undocumented requirement that a '/' must be present at dirname[len]
(despite being past the end-of-string). This oversight leads to
incorrect behavior when core.ignorecase is true. Demonstrate this.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t3010-ls-files-killed-modified.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh
index 3120efd..198d308 100755
--- a/t/t3010-ls-files-killed-modified.sh
+++ b/t/t3010-ls-files-killed-modified.sh
@@ -103,6 +103,14 @@ test_expect_success 'validate git ls-files -k output.' '
 	test_cmp .expected .output
 '
 
+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_cmp .expected .output
+'
+
 test_expect_success 'git ls-files -m to show modified files.' '
 	git ls-files -m >.output
 '
-- 
1.8.4.rc4.557.g46c5bb2

[PATCH v2 2/2] dir: fix core.ignorecase inconsistency with missing '/'

[Eric Sunshine]

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@sunshineco.com>
---
 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

Re: [PATCH v2 0/2] fix t3010 failure when core.ignorecase=true

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Recent mailing list discussion [2][3] suggests that changes at a more
> fundamental level (if they pan out) would be a more appropriate approach
> to resolving this bug.  Such an approach would (happily) make patch 2/2
> unnecessary, however, I'm posting this series for posterity since it was
> already written before [2][3].

Fair enough.

In the meantime, given the reaction from Peff, I am tempted to
squash your "work around in the caller side" as a fix to 2eac2a4c
(ls-files -k: a directory only can be killed if the index has a
non-directory, 2013-08-15) and squash in the patch to run "ls-files
-k" with "-c core.ignorecase=true" to 3c568751 (t3010: update to
demonstrate "ls-files -k" optimization pitfalls, 2013-08-15). That
way, the eventual fix of not adding '/' at the end do not have to
revert the changes to the caller, and the tests added to t3010 by
the latter will be "optimization pitfalls" as before.

Thanks.

Re: [PATCH v2 0/2] fix t3010 failure when core.ignorecase=true

[Eric Sunshine]

On Fri, Aug 23, 2013 at 5:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Recent mailing list discussion [2][3] suggests that changes at a more
>> fundamental level (if they pan out) would be a more appropriate approach
>> to resolving this bug.  Such an approach would (happily) make patch 2/2
>> unnecessary, however, I'm posting this series for posterity since it was
>> already written before [2][3].
>
> Fair enough.
>
> In the meantime, given the reaction from Peff, I am tempted to
> squash your "work around in the caller side" as a fix to 2eac2a4c
> (ls-files -k: a directory only can be killed if the index has a
> non-directory, 2013-08-15) and squash in the patch to run "ls-files
> -k" with "-c core.ignorecase=true" to 3c568751 (t3010: update to
> demonstrate "ls-files -k" optimization pitfalls, 2013-08-15). That
> way, the eventual fix of not adding '/' at the end do not have to
> revert the changes to the caller, and the tests added to t3010 by
> the latter will be "optimization pitfalls" as before.

That sounds like a reasonable interim solution.