{"thread":{"id":"31812","subject":"[PATCH] exclude: fix a bug in prefix comparison optimization","startedAt":"2012-10-14T11:35:22Z","lastAt":"2012-10-15T04:28:48Z","messageCount":5,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"201161","messageId":"1350214522-3242-1-git-send-email-pclouds@gmail.com","threadId":"31812","inReplyTo":null,"subject":"[PATCH] exclude: fix a bug in prefix comparison optimization","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-14T11:35:22Z","receivedAt":"2012-10-14T11:35:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When \"namelen\" becomes zero at this stage, we have matched the fixed\npart, but whether it actually matches the pattern still depends on the\npattern in \"exclude\". As demonstrated in t3001, path \"three/a.3\"\nexists and it matches the \"three/a.3\" part in pattern \"three/a.3[abc]\",\nbut that does not mean a true match.\n\nDon't be too optimistic and let fnmatch() do the job.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n dir.c | 2 +-\n t/t3001-ls-files-others-exclude.sh | 6 ++++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex 4868339..90bf3a4 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -575,7 +575,7 @@ int excluded_from_list(const char *pathname,\n \t\t\tnamelen -= prefix;\n \t\t}\n \n-\t\tif (!namelen || !fnmatch_icase(exclude, name, FNM_PATHNAME))\n+\t\tif (!fnmatch_icase(exclude, name, FNM_PATHNAME))\n \t\t\treturn to_exclude;\n \t}\n \treturn -1; /* undecided */\ndiff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh\nindex c8fe978..dc2f045 100755\n--- a/t/t3001-ls-files-others-exclude.sh\n+++ b/t/t3001-ls-files-others-exclude.sh\n@@ -214,4 +214,10 @@ test_expect_success 'subdirectory ignore (l1)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pattern matches prefix completely' '\n+\t: >expect &&\n+\tgit ls-files -i -o --exclude \"/three/a.3[abc]\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.0.rc2.11.g2b79d01\n"},{"id":"201187","messageId":"7v8vb9yn09.fsf@alter.siamese.dyndns.org","threadId":"31812","inReplyTo":"1350214522-3242-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] exclude: fix a bug in prefix comparison optimization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-14T17:36:06Z","receivedAt":"2012-10-14T17:36:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> When \"namelen\" becomes zero at this stage, we have matched the fixed\n> part, but whether it actually matches the pattern still depends on the\n> pattern in \"exclude\". As demonstrated in t3001, path \"three/a.3\"\n> exists and it matches the \"three/a.3\" part in pattern \"three/a.3[abc]\",\n> but that does not mean a true match.\n>\n> Don't be too optimistic and let fnmatch() do the job.\n\nYeah, the existing code is correct _only_ if the pattern part can\nmatch an empty string (e.g. \"three/a.3*\") and this is a correct fix.\n\nWith your \"teach attr.c match the same optimization as dir.c\"\nseries, you would need something like this\n\ndiff --git i/attr.c w/attr.c\nindex 6d39406..528e935 100644\n--- i/attr.c\n+++ w/attr.c\n@@ -710,7 +710,7 @@ static int path_matches(const char *pathname, int pathlen,\n \t * if the non-wildcard part is longer than the remaining\n \t * pathname, surely it cannot match.\n \t */\n-\tif (!namelen || prefix > namelen)\n+\tif (prefix > namelen)\n \t\treturn 0;\n \tif (baselen != 0)\n \t\tbaselen++;\n\nComparing the corresponding code in dir.c, there is no \"compare the\nliteral prefix part with strcmp() before doing the fnmatch()\"\noptimization. Intended?\n\n(warning: I haven't had my caffeine yet)\n"},{"id":"201191","messageId":"7vobk4ykpf.fsf@alter.siamese.dyndns.org","threadId":"31812","inReplyTo":"7v8vb9yn09.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] exclude: fix a bug in prefix comparison optimization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-14T18:25:48Z","receivedAt":"2012-10-14T18:25:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Comparing the corresponding code in dir.c, there is no \"compare the\n> literal prefix part with strcmp() before doing the fnmatch()\"\n> optimization. Intended?\n>\n> (warning: I haven't had my caffeine yet)\n\nAnd it turns out that at the point I wrote the response, I still had\nthe attr-match-optim-more updates to be read in my mailbox ;-)\n \n"},{"id":"201197","messageId":"CACsJy8DEqkPob+KjFS_Y3BraS40CSP7snS3aB41hg0Ng0DuJDg@mail.gmail.com","threadId":"31812","inReplyTo":"7v8vb9yn09.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] exclude: fix a bug in prefix comparison optimization","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-15T04:09:26Z","receivedAt":"2012-10-15T04:09:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Oct 15, 2012 at 12:36 AM, Junio C Hamano wrote:\n> With your \"teach attr.c match the same optimization as dir.c\"\n> series, you would need something like this\n>\n> diff --git i/attr.c w/attr.c\n> index 6d39406..528e935 100644\n> --- i/attr.c\n> +++ w/attr.c\n> @@ -710,7 +710,7 @@ static int path_matches(const char *pathname, int pathlen,\n> * if the non-wildcard part is longer than the remaining\n> * pathname, surely it cannot match.\n> */\n> - if (!namelen || prefix > namelen)\n> + if (prefix > namelen)\n> return 0;\n> if (baselen != 0)\n> baselen++;\n\nIf there's still a chance to rewrite attr-match-optim-more series (I\nsee it's in next now), then I could reorder the patches so that\nexcluded_from_list code refactoring goes first, then rewrite \"teach\nattr.c match... as dir.c\" patch makes use of the new functions without\ncode duplication. The end result would be the same, except that we\nwon't see this bug in attr.c's history. Not much value so if it may\ntake a lot of your time, don't bother.\n--\nDuy\n"},{"id":"201198","messageId":"7vk3usxssf.fsf@alter.siamese.dyndns.org","threadId":"31812","inReplyTo":"CACsJy8DEqkPob+KjFS_Y3BraS40CSP7snS3aB41hg0Ng0DuJDg@mail.gmail.com","subject":"Re: [PATCH] exclude: fix a bug in prefix comparison optimization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-15T04:28:48Z","receivedAt":"2012-10-15T04:28:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> On Mon, Oct 15, 2012 at 12:36 AM, Junio C Hamano wrote:\n>> With your \"teach attr.c match the same optimization as dir.c\"\n>> series, you would need something like this\n>>\n>> diff --git i/attr.c w/attr.c\n>> index 6d39406..528e935 100644\n>> --- i/attr.c\n>> +++ w/attr.c\n>> @@ -710,7 +710,7 @@ static int path_matches(const char *pathname, int pathlen,\n>> * if the non-wildcard part is longer than the remaining\n>> * pathname, surely it cannot match.\n>> */\n>> - if (!namelen || prefix > namelen)\n>> + if (prefix > namelen)\n>> return 0;\n>> if (baselen != 0)\n>> baselen++;\n>\n> If there's still a chance to rewrite attr-match-optim-more series (I\n> see it's in next now), then I could reorder the patches so that\n> excluded_from_list code refactoring goes first, then rewrite \"teach\n> attr.c match... as dir.c\" patch makes use of the new functions without\n> code duplication. The end result would be the same, except that we\n> won't see this bug in attr.c's history. Not much value so if it may\n> take a lot of your time, don't bother.\n\nActually it started to become nuisance to deal with conflicts among\nas/check-ignore and the attr/dir/wildmatch series, and I've been\nwondering if it makes sense to eject all of these out of 'next' (as\nthey won't advance beyond 'next' until the next release anyway).\n\nSo please go ahead and reroll the whole thing if you think the end\nresult would be a history with better organization.\n"}]}