{"thread":{"id":"34747","subject":"[PATCH 0/2] fix t3010 failure when core.ignorecase=true","startedAt":"2013-08-23T04:29:13Z","lastAt":"2013-08-26T05:38:29Z","messageCount":7,"participants":["Eric Sunshine","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"225707","messageId":"1377232155-7300-1-git-send-email-sunshine@sunshineco.com","threadId":"34747","inReplyTo":null,"subject":"[PATCH 0/2] fix t3010 failure when core.ignorecase=true","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-23T04:29:13Z","receivedAt":"2013-08-23T04:29:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This series fixes a bug in dir.c which causes t3010 to fail [1] when\ncore.ignorecase is true. The problem is that\ndirectory_exists_in_index(dirname,len) and\ndirectory_exists_in_index_icase() behave differently if dirname[len] is\nnot a '/', even though this is beyond end-of-string. 2eac2a4cc4bdc8d7\n(ls-files -k: a directory only can be killed if the index has a\nnon-directory; 2013-08-15) adds a caller which neglects to ensure that\nthe the required '/' is present, hence the failure.\n\nI am not happy with the fix, which is too add a '/' after the last\ncharacter in dirname just at the call site introduced by\n2eac2a4cc4bdc8d7.\n\nThe reason for my unhappiness is that directory_exists_in_index_icase()\nmakes the assumption, not only that it can access the character beyond\nthe end-of-string, but also that that character will unconditionally be\n'/'. I presume that this was done for the sake of speed (existing\ncallers always had a '/' beyond end-of-string), but it feels like an\nugly wart. Since the required trailing '/' is purely an implementation\ndetail of directory_exists_in_index_icase(), and not of\ndirectory_exists_in_index(), a cleaner fix would be for\ndirectory_exists_in_index_icase() to add the '/' it needs, and not\nexpect the passed in dirname to have a '/' after its last character.\nUnfortunately, such a fix would probably negate any optimization benefit\ngained by the present implementation.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/232727\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 | 12 +++++++++---\n t/t3103-ls-tree-misc.sh | 15 +++++++++++++++\n 2 files changed, 24 insertions(+), 3 deletions(-)\n\n-- \n1.8.4.rc4.529.g78818d7\n"},{"id":"225708","messageId":"1377232155-7300-2-git-send-email-sunshine@sunshineco.com","threadId":"34747","inReplyTo":"1377232155-7300-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 1/2] t3103: demonstrate dir.c:treat_one_path() core.ignorecase failure","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-23T04:29:14Z","receivedAt":"2013-08-23T04:29:14Z","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 \n---\n t/t3103-ls-tree-misc.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh\nindex 09dcf04..fd95991 100755\n--- a/t/t3103-ls-tree-misc.sh\n+++ b/t/t3103-ls-tree-misc.sh\n@@ -4,6 +4,7 @@ test_description='\n Miscellaneous tests for git ls-tree.\n \n \t 1. git ls-tree fails in presence of tree damage.\n+\t 2. git ls-tree acts sanely with core.ignorecase.\n \n '\n \n@@ -21,4 +22,18 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' '\n \ttest_must_fail git ls-tree -r HEAD\n '\n \n+test_expect_failure 'ls-tree directory core.ignorecase' '\n+\tcat >expect <<-\\EOF &&\n+\td/e/f\n+\tEOF\n+\tmkdir d &&\n+\t>d/e &&\n+\tgit update-index --add -- d/e &&\n+\trm d/e &&\n+\tmkdir d/e &&\n+\t>d/e/f &&\n+\tgit -c core.ignorecase=true ls-files -k >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.4.rc4.529.g78818d7\n"},{"id":"225709","messageId":"1377232155-7300-3-git-send-email-sunshine@sunshineco.com","threadId":"34747","inReplyTo":"1377232155-7300-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 2/2] dir: test_one_path: fix inconsistent behavior due to missing '/'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-23T04:29:15Z","receivedAt":"2013-08-23T04:29:15Z","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. directory_exists_in_index(), on the other hand,\ndoes not assume nor care about a trailing '/' beyond end-of-string.\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 '/', thus leading to inconsistent behavior between\ndirectory_exists_in_index() and directory_exists_in_index_icase()\ndepending upon the setting of core.ignorecase. Fix this problem.\n\nThis also fixes an initially-unnoticed failure in a t3010 test added by\n3c56875176390eee (t3010: update to demonstrate \"ls-files -k\"\noptimization pitfalls; 2013-08-15) when core.ignorecase is true.\n\nSigned-off-by: Eric Sunshine \n---\n dir.c | 12 +++++++++---\n t/t3103-ls-tree-misc.sh | 2 +-\n 2 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex edd666a..a52c6f9 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1160,9 +1160,15 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,\n \t */\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\treturn path_none;\n+\t !has_path_in_index) {\n+\t\tstrbuf_addch(path, '/');\n+\t\tif (directory_exists_in_index(path->buf, path->len - 1) ==\n+\t\t index_nonexistent) {\n+\t\t\tstrbuf_setlen(path, path->len - 1);\n+\t\t\treturn path_none;\n+\t\t}\n+\t\tstrbuf_setlen(path, path->len - 1);\n+\t}\n \n \texclude = is_excluded(dir, path->buf, &dtype);\n \ndiff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh\nindex fd95991..9fb1706 100755\n--- a/t/t3103-ls-tree-misc.sh\n+++ b/t/t3103-ls-tree-misc.sh\n@@ -22,7 +22,7 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' '\n \ttest_must_fail git ls-tree -r HEAD\n '\n \n-test_expect_failure 'ls-tree directory core.ignorecase' '\n+test_expect_success 'ls-tree directory core.ignorecase' '\n \tcat >expect <<-\\EOF &&\n \td/e/f\n \tEOF\n-- \n1.8.4.rc4.529.g78818d7\n"},{"id":"225724","messageId":"xmqqfvu0pbkt.fsf@gitster.dls.corp.google.com","threadId":"34747","inReplyTo":"1377232155-7300-2-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH 1/2] t3103: demonstrate dir.c:treat_one_path() core.ignorecase failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-23T17:21:38Z","receivedAt":"2013-08-23T17:21:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> @@ -21,4 +22,18 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' '\n> \ttest_must_fail git ls-tree -r HEAD\n> '\n> \n> +test_expect_failure 'ls-tree directory core.ignorecase' '\n> +\tcat >expect <<-\\EOF &&\n> +\td/e/f\n> +\tEOF\n> +\tmkdir d &&\n> +\t>d/e &&\n> +\tgit update-index --add -- d/e &&\n> +\trm d/e &&\n> +\tmkdir d/e &&\n> +\t>d/e/f &&\n> +\tgit -c core.ignorecase=true ls-files -k >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nHmm. Wouldn't it be a clearer demonstration to add to t3010 another\ninvocation of \"ls-files -k\" that was added by 3c568751 (t3010:\nupdate to demonstrate \"ls-files -k\" optimization pitfalls,\n2013-08-15) but with core.ignorecase=true? Something like...\n\n t/t3010-ls-files-killed-modified.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh\nindex 6ea7ca8..f4783ed 100755\n--- a/t/t3010-ls-files-killed-modified.sh\n+++ b/t/t3010-ls-files-killed-modified.sh\n@@ -90,6 +90,17 @@ pathx/ju/nk\n EOF\n \n test_expect_success \\\n+ 'git ls-files -k to show killed files (with icase)' \\\n+ 'git -c core.ignorecase=true ls-files -k >.output'\n+cat >.expected < Although undocumented, directory_exists_in_index_icase(dirname,len)\n> unconditionally assumes the presence of a '/' at dirname[len] (despite\n> being past the end-of-string). Callers are expected to respect\n[...]\n> Fix this problem.\n\nSo, does this fix the problem by changing\ndirectory_exists_in_index_icase() to be more liberal in what it\naccepts, or callers to be more conservative in what they pass in?\n\nPlease forgive my laziness. I ask in order to save future readers the\ntime of digging into the code.\n\nThanks,\nJonathan\n"},{"id":"225843","messageId":"CAPig+cSY8oMeAaS2GRaPhwCkfO=eLa6-UPcJGRe98c0uUFbm8Q@mail.gmail.com","threadId":"34747","inReplyTo":"20130825060045.GP2882@elie.Belkin","subject":"Re: [PATCH 2/2] dir: test_one_path: fix inconsistent behavior due to missing '/'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-25T08:05:30Z","receivedAt":"2013-08-25T08:05:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 25, 2013 at 2:00 AM, Jonathan Nieder wrote:\n> Eric Sunshine wrote:\n>\n>> Although undocumented, directory_exists_in_index_icase(dirname,len)\n>> unconditionally assumes the presence of a '/' at dirname[len] (despite\n>> being past the end-of-string). Callers are expected to respect\n> [...]\n>> Fix this problem.\n>\n> So, does this fix the problem by changing\n> directory_exists_in_index_icase() to be more liberal in what it\n> accepts, or callers to be more conservative in what they pass in?\n\nIt places the onus upon the caller. As mentioned in the cover letter\n[1], I was not happy with this solution. Junio felt likewise. A\nfollow-up series [2] fixes both directory_exists_in_index() and\ndirectory_exists_in_index_icase() to be more liberal in what they\naccept, relieving the caller of the burden. By the time that series\nwas posted, however, Junio and Peff had decided that a fix at a more\nfundamental level would be better (a conclusion with which I agree,\nbut for which I do not yet have sufficient knowledge about git\ninternals to implement). In the meantime, as an interim bug fix, Junio\ndecided [3] to apply the patch to which you responded (but with an\nupdated commit message).\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/232796\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/232833\n[3]: http://thread.gmane.org/gmane.comp.version-control.git/232833/focus=232837\n\n> Please forgive my laziness. I ask in order to save future readers the\n> time of digging into the code.\n"},{"id":"225880","messageId":"xmqqsixxj9ka.fsf@gitster.dls.corp.google.com","threadId":"34747","inReplyTo":"CAPig+cSY8oMeAaS2GRaPhwCkfO=eLa6-UPcJGRe98c0uUFbm8Q@mail.gmail.com","subject":"Re: [PATCH 2/2] dir: test_one_path: fix inconsistent behavior due to missing '/'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-26T05:38:29Z","receivedAt":"2013-08-26T05:38:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> ... A\n> follow-up series [2] fixes both directory_exists_in_index() and\n> directory_exists_in_index_icase() to be more liberal in what they\n> accept, relieving the caller of the burden. By the time that series\n> was posted, however, Junio and Peff had decided that a fix at a more\n> fundamental level would be better (a conclusion with which I agree,\n\nThanks for a summary; the last \"would be better\" is a qualified one,\nthough.\n\nMy understanding is that we agreed that it would be better *if* we\ncan fix it at a more fundamental level. I looked at the codepaths\ninvolved just enough to make that off the cuff suggestion and no\ndeeper than that, and I suspect neither did Peff.\n\nIf any of us looked at the problem deep enough, we may realize that\nit would affect too many things, and $gmane/232833/focus=232835\n(your second round) may turn out to be a better solution. I think\nnone of us know yet.\n"}]}