{"thread":{"id":"34753","subject":"[PATCH v2 0/2] fix t3010 failure when core.ignorecase=true","startedAt":"2013-08-23T20:29:17Z","lastAt":"2013-08-23T21:04:42Z","messageCount":5,"participants":["Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"225743","messageId":"1377289759-46838-1-git-send-email-sunshine@sunshineco.com","threadId":"34753","inReplyTo":null,"subject":"[PATCH v2 0/2] fix t3010 failure when core.ignorecase=true","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-23T20:29:17Z","receivedAt":"2013-08-23T20:29:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This is a re-roll of [1] which fixes a bug in dir.c resulting in failure\nof a newly added test in t3010 when core.ignorecase=true.\n\nRecent mailing list discussion [2][3] suggests that changes at a more\nfundamental level (if they pan out) would be a more appropriate approach\nto resolving this bug.  Such an approach would (happily) make patch 2/2\nunnecessary, however, I'm posting this series for posterity since it was\nalready written before [2][3]. Also, since I'm learning the relevant git\ninternals as I work on these issues, any fix at the more fundamental\nlevel (if it comes from me) is going to be correspondingly delayed since\nthere is so much more to digest.\n\nChanges since v1:\n\npatch 1: place the new test in t3010 instead of t3103 [4]\n\npatch 2: make directory_exists_in_index() and\ndirectory_exists_in_index_icase() responsible for their own internal\nrequirements regarding presence or lack of trailing '/' [5]; also, pass\nin already-existing strbufs so that directory_exists_in_index_icase()\ndoesn't have to make its own copy of the incoming string in order to\nappend '/'\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/232796\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/232727/focus=232813\n[3]: http://thread.gmane.org/gmane.comp.version-control.git/232727/focus=232822\n[4]: http://thread.gmane.org/gmane.comp.version-control.git/232796/focus=232814\n[5]: http://thread.gmane.org/gmane.comp.version-control.git/232727/focus=232800\n\nEric Sunshine (2):\n  t3103: demonstrate dir.c:treat_one_path() core.ignorecase failure\n  dir: test_one_path: fix inconsistent behavior due to missing '/'\n\n dir.c                               | 42 +++++++++++++++++++++++--------------\n t/t3010-ls-files-killed-modified.sh |  8 +++++++\n 2 files changed, 34 insertions(+), 16 deletions(-)\n\n-- \n1.8.4.rc4.557.g46c5bb2\n"},{"id":"225744","messageId":"1377289759-46838-2-git-send-email-sunshine@sunshineco.com","threadId":"34753","inReplyTo":"1377289759-46838-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH v2 1/2] t3103: demonstrate dir.c:treat_one_path() core.ignorecase failure","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-23T20:29:18Z","receivedAt":"2013-08-23T20:29:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"2eac2a4cc4bdc8d7 (ls-files -k: a directory only can be killed if the\nindex has a non-directory; 2013-08-15) adds a caller of\ndirectory_exists_in_index(dirname,len) which forgets to satisfy the\nundocumented requirement that a '/' must be present at dirname[len]\n(despite being past the end-of-string). This oversight leads to\nincorrect behavior when core.ignorecase is true. Demonstrate this.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n t/t3010-ls-files-killed-modified.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh\nindex 3120efd..198d308 100755\n--- a/t/t3010-ls-files-killed-modified.sh\n+++ b/t/t3010-ls-files-killed-modified.sh\n@@ -103,6 +103,14 @@ test_expect_success 'validate git ls-files -k output.' '\n \ttest_cmp .expected .output\n '\n \n+test_expect_success 'git ls-files -k to show killed files (icase).' '\n+\tgit -c core.ignorecase=true ls-files -k >.output\n+'\n+\n+test_expect_failure 'validate git ls-files -k output (icase).' '\n+\ttest_cmp .expected .output\n+'\n+\n test_expect_success 'git ls-files -m to show modified files.' '\n \tgit ls-files -m >.output\n '\n-- \n1.8.4.rc4.557.g46c5bb2\n"},{"id":"225745","messageId":"1377289759-46838-3-git-send-email-sunshine@sunshineco.com","threadId":"34753","inReplyTo":"1377289759-46838-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH v2 2/2] dir: fix core.ignorecase inconsistency with missing '/'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-23T20:29:19Z","receivedAt":"2013-08-23T20:29:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Although undocumented, directory_exists_in_index_icase(dirname,len)\nunconditionally assumes the presence of a '/' at dirname[len], despite\nbeing past the end-of-string. Callers are expected to respect this\nassumption by ensuring that a '/' is present beyond the last character\nof the passed path.\n\ndirectory_exists_in_index(), on the other hand, expects no trailing '/'\n(and never looks beyond end-of-string). Callers are expected to respect\nthis by ensuring the absence of '/'.\n\n2eac2a4cc4bdc8d7 (ls-files -k: a directory only can be killed if the\nindex has a non-directory; 2013-08-15) adds a caller which forgets to\nensure the trailing '/' beyond end-of-string, which leads to\ninconsistent behavior between directory_exists_in_index() and\ndirectory_exists_in_index_icase(), depending upon the setting of\ncore.ignorecase.\n\nOne approach to fix this would be to augment the new caller (added by\n2eac2a4cc4bdc8d7) to add a '/' beyond end-of-string, but this places\nextra burden on the caller to account for an implementation detail of\ndirectory_exists_in_index_icase().\n\nAnother approach would be for directory_exists_in_index_icase() to take\nresponsibility for its own requirements by copying the incoming path and\nadding a trailing '/', but such copying can become expensive.\n\nThe approach taken by this patch is to pass the strbufs already used by\ntheir callers into directory_exists_in_index() and\ndirectory_exists_in_index_icase() rather than 'const char *' + 'size_t\nlen'. This allows both functions to satisfy their own internal\nrequirements -- by manipulating the strbuf to add or remove the trailing\n'/' -- without placing burden upon callers and without having to make\nexpensive copies of each incoming string.\n\nThis also fixes an initially-unnoticed failure, when core.ignorecase is\ntrue, in a t3010 test added by 3c56875176390eee (t3010: update to\ndemonstrate \"ls-files -k\" optimization pitfalls; 2013-08-15).\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n dir.c                               | 42 +++++++++++++++++++++++--------------\n t/t3010-ls-files-killed-modified.sh |  2 +-\n 2 files changed, 27 insertions(+), 17 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex edd666a..6f761a1 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -887,14 +887,21 @@ enum exist_status {\n  * the directory name; instead, use the case insensitive\n  * name hash.\n  */\n-static enum exist_status directory_exists_in_index_icase(const char *dirname, int len)\n+static enum exist_status directory_exists_in_index_icase(struct strbuf *dirname)\n {\n-\tconst struct cache_entry *ce = cache_name_exists(dirname, len + 1, ignore_case);\n+\tconst struct cache_entry *ce;\n \tunsigned char endchar;\n+\tsize_t len = dirname->len;\n+\tint need_slash = len && dirname->buf[len - 1] != '/';\n+\n+\tif (need_slash)\n+\t\tstrbuf_addch(dirname, '/');\n+\tce = cache_name_exists(dirname->buf, dirname->len, ignore_case);\n+\tstrbuf_setlen(dirname, len);\n \n \tif (!ce)\n \t\treturn index_nonexistent;\n-\tendchar = ce->name[len];\n+\tendchar = ce->name[need_slash ? len : len - 1];\n \n \t/*\n \t * The cache_entry structure returned will contain this dirname\n@@ -922,21 +929,25 @@ static enum exist_status directory_exists_in_index_icase(const char *dirname, in\n  * the files it contains) will sort with the '/' at the\n  * end.\n  */\n-static enum exist_status directory_exists_in_index(const char *dirname, int len)\n+static enum exist_status directory_exists_in_index(struct strbuf *dirname)\n {\n-\tint pos;\n+\tint pos, len;\n \n \tif (ignore_case)\n-\t\treturn directory_exists_in_index_icase(dirname, len);\n+\t\treturn directory_exists_in_index_icase(dirname);\n+\n+\tlen = dirname->len;\n+\tif (len && dirname->buf[len - 1] == '/')\n+\t\tlen--;\n \n-\tpos = cache_name_pos(dirname, len);\n+\tpos = cache_name_pos(dirname->buf, len);\n \tif (pos < 0)\n \t\tpos = -pos-1;\n \twhile (pos < active_nr) {\n \t\tconst struct cache_entry *ce = active_cache[pos++];\n \t\tunsigned char endchar;\n \n-\t\tif (strncmp(ce->name, dirname, len))\n+\t\tif (strncmp(ce->name, dirname->buf, len))\n \t\t\tbreak;\n \t\tendchar = ce->name[len];\n \t\tif (endchar > '/')\n@@ -983,11 +994,10 @@ static enum exist_status directory_exists_in_index(const char *dirname, int len)\n  *  (c) otherwise, we recurse into it.\n  */\n static enum path_treatment treat_directory(struct dir_struct *dir,\n-\tconst char *dirname, int len, int exclude,\n+\tstruct strbuf *dirname, int exclude,\n \tconst struct path_simplify *simplify)\n {\n-\t/* The \"len-1\" is to strip the final '/' */\n-\tswitch (directory_exists_in_index(dirname, len-1)) {\n+\tswitch (directory_exists_in_index(dirname)) {\n \tcase index_directory:\n \t\treturn path_recurse;\n \n@@ -999,7 +1009,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\t\tbreak;\n \t\tif (!(dir->flags & DIR_NO_GITLINKS)) {\n \t\t\tunsigned char sha1[20];\n-\t\t\tif (resolve_gitlink_ref(dirname, \"HEAD\", sha1) == 0)\n+\t\t\tif (!resolve_gitlink_ref(dirname->buf, \"HEAD\", sha1))\n \t\t\t\treturn path_untracked;\n \t\t}\n \t\treturn path_recurse;\n@@ -1010,7 +1020,8 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \tif (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))\n \t\treturn exclude ? path_excluded : path_untracked;\n \n-\treturn read_directory_recursive(dir, dirname, len, 1, simplify);\n+\treturn read_directory_recursive(dir, dirname->buf, dirname->len, 1,\n+\t\t\t\t\tsimplify);\n }\n \n /*\n@@ -1161,7 +1172,7 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,\n \tif ((dir->flags & DIR_COLLECT_KILLED_ONLY) &&\n \t    (dtype == DT_DIR) &&\n \t    !has_path_in_index &&\n-\t    (directory_exists_in_index(path->buf, path->len) == index_nonexistent))\n+\t    (directory_exists_in_index(path) == index_nonexistent))\n \t\treturn path_none;\n \n \texclude = is_excluded(dir, path->buf, &dtype);\n@@ -1178,8 +1189,7 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,\n \t\treturn path_none;\n \tcase DT_DIR:\n \t\tstrbuf_addch(path, '/');\n-\t\treturn treat_directory(dir, path->buf, path->len, exclude,\n-\t\t\tsimplify);\n+\t\treturn treat_directory(dir, path, exclude, simplify);\n \tcase DT_REG:\n \tcase DT_LNK:\n \t\treturn exclude ? path_excluded : path_untracked;\ndiff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh\nindex 198d308..78954bd 100755\n--- a/t/t3010-ls-files-killed-modified.sh\n+++ b/t/t3010-ls-files-killed-modified.sh\n@@ -107,7 +107,7 @@ test_expect_success 'git ls-files -k to show killed files (icase).' '\n \tgit -c core.ignorecase=true ls-files -k >.output\n '\n \n-test_expect_failure 'validate git ls-files -k output (icase).' '\n+test_expect_success 'validate git ls-files -k output (icase).' '\n \ttest_cmp .expected .output\n '\n \n-- \n1.8.4.rc4.557.g46c5bb2\n"},{"id":"225747","messageId":"xmqqob8onmw9.fsf@gitster.dls.corp.google.com","threadId":"34753","inReplyTo":"1377289759-46838-1-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH v2 0/2] fix t3010 failure when core.ignorecase=true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-23T21:00:06Z","receivedAt":"2013-08-23T21:00:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Recent mailing list discussion [2][3] suggests that changes at a more\n> fundamental level (if they pan out) would be a more appropriate approach\n> to resolving this bug.  Such an approach would (happily) make patch 2/2\n> unnecessary, however, I'm posting this series for posterity since it was\n> already written before [2][3].\n\nFair enough.\n\nIn the meantime, given the reaction from Peff, I am tempted to\nsquash your \"work around in the caller side\" as a fix to 2eac2a4c\n(ls-files -k: a directory only can be killed if the index has a\nnon-directory, 2013-08-15) and squash in the patch to run \"ls-files\n-k\" with \"-c core.ignorecase=true\" to 3c568751 (t3010: update to\ndemonstrate \"ls-files -k\" optimization pitfalls, 2013-08-15). That\nway, the eventual fix of not adding '/' at the end do not have to\nrevert the changes to the caller, and the tests added to t3010 by\nthe latter will be \"optimization pitfalls\" as before.\n\nThanks.\n"},{"id":"225748","messageId":"CAPig+cSYPLNcMUtJTH08BAipfLOn_662k2DBwPzs2Z5dhwUhng@mail.gmail.com","threadId":"34753","inReplyTo":"xmqqob8onmw9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 0/2] fix t3010 failure when core.ignorecase=true","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-23T21:04:42Z","receivedAt":"2013-08-23T21:04:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 23, 2013 at 5:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>\n>> Recent mailing list discussion [2][3] suggests that changes at a more\n>> fundamental level (if they pan out) would be a more appropriate approach\n>> to resolving this bug.  Such an approach would (happily) make patch 2/2\n>> unnecessary, however, I'm posting this series for posterity since it was\n>> already written before [2][3].\n>\n> Fair enough.\n>\n> In the meantime, given the reaction from Peff, I am tempted to\n> squash your \"work around in the caller side\" as a fix to 2eac2a4c\n> (ls-files -k: a directory only can be killed if the index has a\n> non-directory, 2013-08-15) and squash in the patch to run \"ls-files\n> -k\" with \"-c core.ignorecase=true\" to 3c568751 (t3010: update to\n> demonstrate \"ls-files -k\" optimization pitfalls, 2013-08-15). That\n> way, the eventual fix of not adding '/' at the end do not have to\n> revert the changes to the caller, and the tests added to t3010 by\n> the latter will be \"optimization pitfalls\" as before.\n\nThat sounds like a reasonable interim solution.\n"}]}