{"thread":{"id":"47785","subject":"[PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()","startedAt":"2018-02-08T00:42:22Z","lastAt":"2018-02-09T05:43:48Z","messageCount":8,"participants":["Ben Peart","David Turner","Torsten Bögershausen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"338684","messageId":"20180208004156.36224-1-benpeart@microsoft.com","threadId":"47785","inReplyTo":null,"subject":"[PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()","fromName":"Ben Peart","fromEmail":"benpeart@microsoft.com","sentAt":"2018-02-08T00:41:56Z","receivedAt":"2018-02-08T00:42:22Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Correct the pointer arithmetic in adjust_dirname_case() so that it calls\nfind_dir_entry() with the correct string length.  Previously passing in\n\"dir1/foo\" would pass a length of 6 instead of the correct 4.  This resulted in\nfind_dir_entry() never finding the entry and so the subsequent memcpy that would\nfold the name to the version with the correct case never executed.\n\nAdd a test to validate the corrected behavior with name folding of directories.\n\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n\nNotes:\n    Base Ref: v2.16.1\n    Web-Diff: https://github.com/benpeart/git/commit/2944970f4e\n    Checkout: git fetch https://github.com/benpeart/git adjust_dirname-v1 && git checkout 2944970f4e\n\n name-hash.c           |  6 +++---\n t/t0050-filesystem.sh | 12 +++++++++++-\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/name-hash.c b/name-hash.c\nindex 45c98db0a0..2ddbb72647 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -696,12 +696,12 @@ void adjust_dirname_case(struct index_state *istate, char *name)\n \t\tif (*ptr == '/') {\n \t\t\tstruct dir_entry *dir;\n \n-\t\t\tptr++;\n-\t\t\tdir = find_dir_entry(istate, name, ptr - name + 1);\n+\t\t\tdir = find_dir_entry(istate, name, ptr - name);\n \t\t\tif (dir) {\n \t\t\t\tmemcpy((void *)startPtr, dir->name + (startPtr - name), ptr - startPtr);\n-\t\t\t\tstartPtr = ptr;\n+\t\t\t\tstartPtr = ptr + 1;\n \t\t\t}\n+\t\t\tptr++;\n \t\t}\n \t}\n }\ndiff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\nindex b29d749bb7..219c96594c 100755\n--- a/t/t0050-filesystem.sh\n+++ b/t/t0050-filesystem.sh\n@@ -80,7 +80,17 @@ test_expect_success 'merge (case change)' '\n \tgit merge topic\n '\n \n-\n+test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '\n+\tgit reset --hard initial &&\n+\tmkdir -p dir1 &&\n+\tmkdir -p dir1/dir2 &&\n+\ttouch dir1/dir2/a &&\n+\ttouch dir1/dir2/b &&\n+\tgit add dir1/dir2/a &&\n+\tgit add dir1/DIR2/b &&\n+\tcamel=$(git ls-files | grep dir2) &&\n+\ttest $(echo \"$camel\" | wc -l) = 2\n+'\n \n test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '\n \tgit reset --hard initial &&\n\nbase-commit: 8279ed033f703d4115bee620dccd32a9ec94d9aa\n-- \n2.15.0.windows.1\n\n"},{"id":"338685","messageId":"1518051751.15704.476.camel@novalis.org","threadId":"47785","inReplyTo":"20180208004156.36224-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2018-02-08T01:02:31Z","receivedAt":"2018-02-08T01:02:39Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2018-02-07 at 19:41 -0500, Ben Peart wrote:\n> Correct the pointer arithmetic in adjust_dirname_case() so that it\n> calls\n> find_dir_entry() with the correct string length.  Previously passing\n> in\n> \"dir1/foo\" would pass a length of 6 instead of the correct 4.  This\n> resulted in\n> find_dir_entry() never finding the entry and so the subsequent memcpy\n> that would\n> fold the name to the version with the correct case never executed.\n> \n> Add a test to validate the corrected behavior with name folding of\n> directories.\n\nLooks good to me.\n"},{"id":"338741","messageId":"20180208172153.GA30760@tor.lan","threadId":"47785","inReplyTo":"20180208004156.36224-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-02-08T17:21:53Z","receivedAt":"2018-02-08T17:22:10Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Wed, Feb 07, 2018 at 07:41:56PM -0500, Ben Peart wrote:\n[]\n\n> diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\n> index b29d749bb7..219c96594c 100755\n> --- a/t/t0050-filesystem.sh\n> +++ b/t/t0050-filesystem.sh\n> @@ -80,7 +80,17 @@ test_expect_success 'merge (case change)' '\n>  \tgit merge topic\n>  '\n>  \n> -\n> +test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '\n> +\tgit reset --hard initial &&\n> +\tmkdir -p dir1 &&\n> +\tmkdir -p dir1/dir2 &&\n> +\ttouch dir1/dir2/a &&\n> +\ttouch dir1/dir2/b &&\n> +\tgit add dir1/dir2/a &&\n> +\tgit add dir1/DIR2/b &&\n> +\tcamel=$(git ls-files | grep dir2) &&\n> +\ttest $(echo \"$camel\" | wc -l) = 2\n> +'\n>  \n\nThere is nothing wrong with with \"wc -l\", but:\na more new-style would probably use test_line_count() here.\n\nMy personal favorite would be to spell out what we expect and run a diff.\nWhen it fails, we can see what fails, and the function would look\nlike this:\n\n\ntest_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '\n\tgit reset --hard initial &&\n\tmkdir -p dir1 &&\n\tmkdir -p dir1/dir2 &&\n\ttouch dir1/dir2/a &&\n\ttouch dir1/dir2/b &&\n\tgit add dir1/dir2/a &&\n\tgit add dir1/DIR2/b &&\n\tgit ls-files | grep dir2 | sort >actual &&\n\tcat >expected <<-\\EOF &&\n\tdir1/dir2/a\n\tdir1/dir2/b\n\tEOF\n\ttest_cmp expected actual\n'\n\n\n\n"},{"id":"338742","messageId":"8a6b704f-409d-9a29-2229-d792d0f503ae@gmail.com","threadId":"47785","inReplyTo":"20180208172153.GA30760@tor.lan","subject":"Re: [PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-02-08T17:45:13Z","receivedAt":"2018-02-08T17:45:26Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 2/8/2018 12:21 PM, Torsten Bögershausen wrote:\n> On Wed, Feb 07, 2018 at 07:41:56PM -0500, Ben Peart wrote:\n> []\n> \n>> diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\n>> index b29d749bb7..219c96594c 100755\n>> --- a/t/t0050-filesystem.sh\n>> +++ b/t/t0050-filesystem.sh\n>> @@ -80,7 +80,17 @@ test_expect_success 'merge (case change)' '\n>>   \tgit merge topic\n>>   '\n>>   \n>> -\n>> +test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '\n>> +\tgit reset --hard initial &&\n>> +\tmkdir -p dir1 &&\n>> +\tmkdir -p dir1/dir2 &&\n>> +\ttouch dir1/dir2/a &&\n>> +\ttouch dir1/dir2/b &&\n>> +\tgit add dir1/dir2/a &&\n>> +\tgit add dir1/DIR2/b &&\n>> +\tcamel=$(git ls-files | grep dir2) &&\n>> +\ttest $(echo \"$camel\" | wc -l) = 2\n>> +'\n>>   \n> \n> There is nothing wrong with with \"wc -l\", but:\n> a more new-style would probably use test_line_count() here.\n> \n> My personal favorite would be to spell out what we expect and run a diff.\n> When it fails, we can see what fails, and the function would look\n> like this:\n> \n\nI agree with you completely that this is a better format and is easier \nto read.  All the new tests I've been writing follow this same pattern.\n\nIn this particular test file, I opted (for better and for worse) to \nstick with the style of all the other tests rather than 1) have this one \ntest be very different than all the others or 2) rewriting all the \nexisting tests in the new style.\n\n> \n> test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '\n> \tgit reset --hard initial &&\n> \tmkdir -p dir1 &&\n> \tmkdir -p dir1/dir2 &&\n> \ttouch dir1/dir2/a &&\n> \ttouch dir1/dir2/b &&\n> \tgit add dir1/dir2/a &&\n> \tgit add dir1/DIR2/b &&\n> \tgit ls-files | grep dir2 | sort >actual &&\n> \tcat >expected <<-\\EOF &&\n> \tdir1/dir2/a\n> \tdir1/dir2/b\n> \tEOF\n> \ttest_cmp expected actual\n> '\n> \n> \n> \n"},{"id":"338744","messageId":"xmqqlgg35on4.fsf@gitster-ct.c.googlers.com","threadId":"47785","inReplyTo":"20180208004156.36224-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-08T18:20:47Z","receivedAt":"2018-02-08T18:20:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart <benpeart@microsoft.com> writes:\n\n> Correct the pointer arithmetic in adjust_dirname_case() so that it calls\n> find_dir_entry() with the correct string length.  Previously passing in\n> \"dir1/foo\" would pass a length of 6 instead of the correct 4.  This resulted in\n> find_dir_entry() never finding the entry and so the subsequent memcpy that would\n> fold the name to the version with the correct case never executed.\n>\n> Add a test to validate the corrected behavior with name folding of directories.\n>\n> Signed-off-by: Ben Peart <benpeart@microsoft.com>\n> ---\n\nThanks.\n\nIt appears that this codepath has been miscounting ever since it was\nintroduced in 41284eb0 (\"name-hash: don't reuse cache_entry in\ndir_entry\", 2015-10-21).\n\n> diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\n> index b29d749bb7..219c96594c 100755\n> --- a/t/t0050-filesystem.sh\n> +++ b/t/t0050-filesystem.sh\n> @@ -80,7 +80,17 @@ test_expect_success 'merge (case change)' '\n> ...\n> +test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '\n> +\tgit reset --hard initial &&\n> +\tmkdir -p dir1 &&\n> +\tmkdir -p dir1/dir2 &&\n\nA single \"mkdir -p dir1/dir2\" should be sufficient, thanks to \"-p\" ;-)\n\n> +\ttouch dir1/dir2/a &&\n> +\ttouch dir1/dir2/b &&\n\nHmph.\n"},{"id":"338747","messageId":"xmqqfu6b5ogv.fsf@gitster-ct.c.googlers.com","threadId":"47785","inReplyTo":"20180208172153.GA30760@tor.lan","subject":"Re: [PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-08T18:24:32Z","receivedAt":"2018-02-08T18:24:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> My personal favorite would be to spell out what we expect and run a diff.\n> When it fails, we can see what fails, and the function would look\n> like this:\n\nI'd rather not to have the \"sort\" there; output from ls-files is\nmeant to be stable; passing it through sort would miss breakages.  I\nagree that comparison between \"expect\" and \"actual\" is a good idea\nnevertheless.\n\nSpeaking of styles, I'd prefer to reserve use of \"touch\" to cases\nwhere resulting timestamp matters, and not \"make sure it exists\".\n\nThanks.\n\n> test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '\n> \tgit reset --hard initial &&\n> \tmkdir -p dir1 &&\n> \tmkdir -p dir1/dir2 &&\n> \ttouch dir1/dir2/a &&\n> \ttouch dir1/dir2/b &&\n> \tgit add dir1/dir2/a &&\n> \tgit add dir1/DIR2/b &&\n> \tgit ls-files | grep dir2 | sort >actual &&\n> \tcat >expected <<-\\EOF &&\n> \tdir1/dir2/a\n> \tdir1/dir2/b\n> \tEOF\n> \ttest_cmp expected actual\n> '\n"},{"id":"338753","messageId":"20180208192333.7436-1-benpeart@microsoft.com","threadId":"47785","inReplyTo":"20180208004156.36224-1-benpeart@microsoft.com","subject":"[PATCH v2] name-hash: properly fold directory names in adjust_dirname_case()","fromName":"Ben Peart","fromEmail":"benpeart@microsoft.com","sentAt":"2018-02-08T19:23:33Z","receivedAt":"2018-02-08T19:24:05Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Correct the pointer arithmetic in adjust_dirname_case() so that it calls\nfind_dir_entry() with the correct string length.  Previously passing in\n\"dir1/foo\" would pass a length of 6 instead of the correct 4.  This resulted in\nfind_dir_entry() never finding the entry and so the subsequent memcpy that would\nfold the name to the version with the correct case never executed.\n\nAdd a test to validate the corrected behavior with name folding of directories.\n\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n\nNotes:\n    Base Ref: v2.16.1\n    Web-Diff: https://github.com/benpeart/git/commit/477da4602c\n    Checkout: git fetch https://github.com/benpeart/git adjust_dirname-v2 && git checkout 477da4602c\n    \n    ### Interdiff (v1..v2):\n    \n    diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\n    index 219c96594c..0e4e51b79a 100755\n    --- a/t/t0050-filesystem.sh\n    +++ b/t/t0050-filesystem.sh\n    @@ -82,14 +82,18 @@ test_expect_success 'merge (case change)' '\n    \n     test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '\n     \tgit reset --hard initial &&\n    -\tmkdir -p dir1 &&\n     \tmkdir -p dir1/dir2 &&\n    -\ttouch dir1/dir2/a &&\n    -\ttouch dir1/dir2/b &&\n    +\techo > dir1/dir2/a &&\n    +\techo > dir1/dir2/b &&\n     \tgit add dir1/dir2/a &&\n     \tgit add dir1/DIR2/b &&\n    -\tcamel=$(git ls-files | grep dir2) &&\n    -\ttest $(echo \"$camel\" | wc -l) = 2\n    +\tgit ls-files >actual &&\n    +\tcat >expected <<-\\EOF &&\n    +\t\tcamelcase\n    +\t\tdir1/dir2/a\n    +\t\tdir1/dir2/b\n    +\tEOF\n    +\ttest_cmp expected actual\n     '\n    \n     test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '\n    \n    ### Patches\n\n name-hash.c           |  6 +++---\n t/t0050-filesystem.sh | 16 +++++++++++++++-\n 2 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/name-hash.c b/name-hash.c\nindex 45c98db0a0..2ddbb72647 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -696,12 +696,12 @@ void adjust_dirname_case(struct index_state *istate, char *name)\n \t\tif (*ptr == '/') {\n \t\t\tstruct dir_entry *dir;\n \n-\t\t\tptr++;\n-\t\t\tdir = find_dir_entry(istate, name, ptr - name + 1);\n+\t\t\tdir = find_dir_entry(istate, name, ptr - name);\n \t\t\tif (dir) {\n \t\t\t\tmemcpy((void *)startPtr, dir->name + (startPtr - name), ptr - startPtr);\n-\t\t\t\tstartPtr = ptr;\n+\t\t\t\tstartPtr = ptr + 1;\n \t\t\t}\n+\t\t\tptr++;\n \t\t}\n \t}\n }\ndiff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\nindex b29d749bb7..0e4e51b79a 100755\n--- a/t/t0050-filesystem.sh\n+++ b/t/t0050-filesystem.sh\n@@ -80,7 +80,21 @@ test_expect_success 'merge (case change)' '\n \tgit merge topic\n '\n \n-\n+test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '\n+\tgit reset --hard initial &&\n+\tmkdir -p dir1/dir2 &&\n+\techo > dir1/dir2/a &&\n+\techo > dir1/dir2/b &&\n+\tgit add dir1/dir2/a &&\n+\tgit add dir1/DIR2/b &&\n+\tgit ls-files >actual &&\n+\tcat >expected <<-\\EOF &&\n+\t\tcamelcase\n+\t\tdir1/dir2/a\n+\t\tdir1/dir2/b\n+\tEOF\n+\ttest_cmp expected actual\n+'\n \n test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '\n \tgit reset --hard initial &&\n\nbase-commit: 8279ed033f703d4115bee620dccd32a9ec94d9aa\n-- \n2.15.0.windows.1\n\n"},{"id":"338809","messageId":"20180209054331.GA8337@tor.lan","threadId":"47785","inReplyTo":"20180208192333.7436-1-benpeart@microsoft.com","subject":"Re: [PATCH v2] name-hash: properly fold directory names in adjust_dirname_case()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-02-09T05:43:31Z","receivedAt":"2018-02-09T05:43:48Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Thu, Feb 08, 2018 at 02:23:33PM -0500, Ben Peart wrote:\n\n[]\n\n> -\n> +test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '\n> +\tgit reset --hard initial &&\n> +\tmkdir -p dir1/dir2 &&\n> +\techo > dir1/dir2/a &&\n> +\techo > dir1/dir2/b &&\n\nThanks for working on this-\nOne and a half style nits:\nThe \">\" should be close to the file name:\n\techo >dir1/dir2/a &&\nBut then we don't even look into the file, so that the \"\\\" produced by echo is not needed.\nA simple \n\t>dir1/dir2/a &&\nis all that is needed.\n"}]}