{"thread":{"id":"33230","subject":"[regression?] trailing slash required in .gitattributes","startedAt":"2013-03-19T17:57:56Z","lastAt":"2013-03-30T01:40:24Z","messageCount":66,"participants":["Jeff King","Junio C Hamano","Duy Nguyen","Nguyễn Thái Ngọc Duy","Eric Sunshine","Torsten Bögershausen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"211695","messageId":"20130319175756.GA13760@sigill.intra.peff.net","threadId":"33230","inReplyTo":null,"subject":"[regression?] trailing slash required in .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-19T17:57:56Z","receivedAt":"2013-03-19T17:57:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Prior to v1.8.1.1, if I did this:\n\n git init\n echo content >foo &&\n mkdir subdir &&\n echo content >subdir/bar &&\n echo \"subdir export-ignore\" >.gitattributes\n git add . &&\n git commit -m one &&\n git archive HEAD | tar tf -\n\nmy archive would contain only \"foo\" and \".gitattributes\", not subdir. As\nof v1.8.1.1, the attribute on subdir is ignored unless it is written\nwith a trailing slash, like:\n\n subdir/ export-ignore\n\nThe issue bisects to 94bc671 (Add directory pattern matching to\nattributes, 2012-12-08). That commit actually tests not only that\n\"subdir/\" matches, but also that just \"subdir\" does not match.\n\nThe commit message there is vague about the reasoning, but my\nunderstanding is that it was meant to harmonize gitignore and\ngitattributes, the former of which can take \"dir/\". I don't have a\nproblem with offering \"dir/\" to match only directories, but what is the\npoint in disallowing just \"dir\" to match a directory?\n\nIt seems like a pointless regression to me, but I'm not clear whether it\nwas intentional or not (and if it was intentional, I think we would need\nto handle it with a proper transition period, not in a maint release).\n\n-Peff\n"},{"id":"211698","messageId":"7vsj3ri7lr.fsf@alter.siamese.dyndns.org","threadId":"33230","inReplyTo":"20130319175756.GA13760@sigill.intra.peff.net","subject":"Re: [regression?] trailing slash required in .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-19T18:10:08Z","receivedAt":"2013-03-19T18:10:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Prior to v1.8.1.1, if I did this:\n>\n> git init\n> echo content >foo &&\n> mkdir subdir &&\n> echo content >subdir/bar &&\n> echo \"subdir export-ignore\" >.gitattributes\n> git add . &&\n> git commit -m one &&\n> git archive HEAD | tar tf -\n>\n> my archive would contain only \"foo\" and \".gitattributes\", not subdir. As\n> of v1.8.1.1, the attribute on subdir is ignored unless it is written\n> with a trailing slash, like:\n>\n> subdir/ export-ignore\n>\n> The issue bisects to 94bc671 (Add directory pattern matching to\n> attributes, 2012-12-08). That commit actually tests not only that\n> \"subdir/\" matches, but also that just \"subdir\" does not match.\n>\n> The commit message there is vague about the reasoning, but my\n> understanding is that it was meant to harmonize gitignore and\n> gitattributes, the former of which can take \"dir/\". I don't have a\n> problem with offering \"dir/\" to match only directories, but what is the\n> point in disallowing just \"dir\" to match a directory?\n>\n> It seems like a pointless regression to me, but I'm not clear whether it\n> was intentional or not (and if it was intentional, I think we would need\n> to handle it with a proper transition period, not in a maint release).\n\nI agree that is a pointless regression.\n"},{"id":"211699","messageId":"20130319181042.GA14295@sigill.intra.peff.net","threadId":"33230","inReplyTo":"20130319175756.GA13760@sigill.intra.peff.net","subject":"Re: [regression?] trailing slash required in .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-19T18:10:42Z","receivedAt":"2013-03-19T18:10:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2013 at 01:57:56PM -0400, Jeff King wrote:\n\n> Prior to v1.8.1.1, if I did this:\n> \n> git init\n> echo content >foo &&\n> mkdir subdir &&\n> echo content >subdir/bar &&\n> echo \"subdir export-ignore\" >.gitattributes\n> git add . &&\n> git commit -m one &&\n> git archive HEAD | tar tf -\n> \n> my archive would contain only \"foo\" and \".gitattributes\", not subdir. As\n> of v1.8.1.1, the attribute on subdir is ignored unless it is written\n> with a trailing slash, like:\n> \n> subdir/ export-ignore\n> \n> The issue bisects to 94bc671 (Add directory pattern matching to\n> attributes, 2012-12-08). That commit actually tests not only that\n> \"subdir/\" matches, but also that just \"subdir\" does not match.\n\nSorry, I mis-read the tests. They are not testing that \"subdir\" does not\nwork, only that \"subdir/\" will match only a directory, not a regular\nfile. Which does make sense.\n\nSo I think the regression is accidental. And we would want tests like\nthis on top (which currently fail):\n\ndiff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh\nindex 0c847fb..3be809c 100755\n--- a/t/t5002-archive-attr-pattern.sh\n+++ b/t/t5002-archive-attr-pattern.sh\n@@ -27,6 +27,10 @@ test_expect_success 'setup' '\n \techo ignored-only-if-dir/ export-ignore >>.git/info/attributes &&\n \tgit add ignored-only-if-dir &&\n \n+\tmkdir -p ignored-without-slash &&\n+\techo ignored without slash >ignored-without-slash/foo &&\n+\tgit add ignored-without-slash/foo &&\n+\techo ignored-without-slash export-ignore >>.git/info/attributes &&\n \n \tmkdir -p one-level-lower/two-levels-lower/ignored-only-if-dir &&\n \techo ignored by ignored dir >one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir &&\n@@ -49,6 +53,8 @@ test_expect_missing\tarchive/ignored-ony-if-dir/ignored-by-ignored-dir\n test_expect_exists\tarchive/not-ignored-dir/\n test_expect_missing\tarchive/ignored-only-if-dir/\n test_expect_missing\tarchive/ignored-ony-if-dir/ignored-by-ignored-dir\n+test_expect_missing archive/ignored-without-slash/ &&\n+test_expect_missing archive/ignored-without-slash/foo &&\n test_expect_exists\tarchive/one-level-lower/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-only-if-dir/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-ony-if-dir/ignored-by-ignored-dir\n"},{"id":"211994","messageId":"20130322222438.GA13207@sigill.intra.peff.net","threadId":"33230","inReplyTo":"20130319181042.GA14295@sigill.intra.peff.net","subject":"Re: [regression?] trailing slash required in .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-22T22:24:39Z","receivedAt":"2013-03-22T22:24:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2013 at 02:10:42PM -0400, Jeff King wrote:\n\n> > The issue bisects to 94bc671 (Add directory pattern matching to\n> > attributes, 2012-12-08). That commit actually tests not only that\n> > \"subdir/\" matches, but also that just \"subdir\" does not match.\n> [...]\n> So I think the regression is accidental. And we would want tests like\n> this on top (which currently fail):\n> [...]\n\nI'm having trouble figuring out the right solution for this.\n\nThe problem is in path_matches, which used to receive just the unadorned\npathname, and now receives \"path/\" for directories. It now looks like\nthis:\n\n> static int path_matches(const char *pathname, int pathlen,\n> \t\t\tconst char *basename,\n> \t\t\tconst struct pattern *pat,\n> \t\t\tconst char *base, int baselen)\n> {\n> \tconst char *pattern = pat->pattern;\n> \tint prefix = pat->nowildcardlen;\n> \n> \tif ((pat->flags & EXC_FLAG_MUSTBEDIR) &&\n> \t ((!pathlen) || (pathname[pathlen-1] != '/')))\n> \t\treturn 0;\n\nThis first stanza checks that a pattern like \"foo/\" must be matched by a\nreal directory. Which is fine; that's the point of adding the \"/\" to the\npattern.\n\n> \tif (pat->flags & EXC_FLAG_NODIR) {\n> \t\treturn match_basename(basename,\n> \t\t\t\t pathlen - (basename - pathname),\n> \t\t\t\t pattern, prefix,\n> \t\t\t\t pat->patternlen, pat->flags);\n> \t}\n> \treturn match_pathname(pathname, pathlen,\n> \t\t\t base, baselen,\n> \t\t\t pattern, prefix, pat->patternlen, pat->flags);\n> }\n\nBut then here we'll end up feeding \"foo/\" to be compared with \"foo\",\nwhich we don't want. For a pattern \"foo\", we want to match _either_\n\"foo/\" or \"foo\". So you'd think something like:\n\n if (pathlen && pathname[pathlen-1] == '/')\n pathlen--;\n\nwould work. But it seems that match_basename, despite taking the length\nof all of the strings we pass it, will happily use NUL-terminated\nfunctions like strcmp or fnmatch. Converting the former to check lengths\nshould be pretty straightforward. But there is no version of fnmatch\nthat does what we want. I wonder if we using wildmatch can get around\nthis limitation.\n\n-Peff\n"},{"id":"212003","messageId":"7vzjxv3uef.fsf@alter.siamese.dyndns.org","threadId":"33230","inReplyTo":"20130322222438.GA13207@sigill.intra.peff.net","subject":"Re: [regression?] trailing slash required in .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-22T23:08:08Z","receivedAt":"2013-03-22T23:08:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> if (pathlen && pathname[pathlen-1] == '/')\n> pathlen--;\n>\n> would work. But it seems that match_basename, despite taking the length\n> of all of the strings we pass it, will happily use NUL-terminated\n> functions like strcmp or fnmatch. Converting the former to check lengths\n> should be pretty straightforward. But there is no version of fnmatch\n> that does what we want. I wonder if we using wildmatch can get around\n> this limitation.\n\nOr save away pathname[pathlen], temporarily NUL terminate and call\nthese functions?\n"},{"id":"212021","messageId":"20130323041824.GA11142@lanh","threadId":"33230","inReplyTo":"20130322222438.GA13207@sigill.intra.peff.net","subject":"Re: [regression?] trailing slash required in .gitattributes","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-23T04:18:24Z","receivedAt":"2013-03-23T04:18:24Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 22, 2013 at 06:24:39PM -0400, Jeff King wrote:\n> I'm having trouble figuring out the right solution for this.\n\nThanks for looking into this. It was on my todo list, but you beat me\nto it :)\n\n> But then here we'll end up feeding \"foo/\" to be compared with \"foo\",\n> which we don't want. For a pattern \"foo\", we want to match _either_\n> \"foo/\" or \"foo\". So you'd think something like:\n> \n> if (pathlen && pathname[pathlen-1] == '/')\n> pathlen--;\n> \n> would work. But it seems that match_basename, despite taking the length\n> of all of the strings we pass it, will happily use NUL-terminated\n> functions like strcmp or fnmatch. Converting the former to check lengths\n> should be pretty straightforward. But there is no version of fnmatch\n> that does what we want. I wonder if we using wildmatch can get around\n> this limitation.\n\nYou can use nwildmatch() from this patch. I tested it lightly with\nt3070-wildmatch.sh, feeding the strings with no terminating NUL. It\nseems to work ok.\n\n-- 8< --\nSubject: [PATCH] wildmatch: do not require \"text\" to be NUL-terminated\n\nThis may be helpful when we just want to match a part of \"text\".\nnwildmatch can be used for this purpose.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n wildmatch.c | 25 +++++++++++++------------\n wildmatch.h | 11 +++++++++--\n 2 files changed, 22 insertions(+), 14 deletions(-)\n\ndiff --git a/wildmatch.c b/wildmatch.c\nindex 7192bdc..f97ae2a 100644\n--- a/wildmatch.c\n+++ b/wildmatch.c\n@@ -52,7 +52,8 @@ typedef unsigned char uchar;\n #define ISXDIGIT(c) (ISASCII(c) && isxdigit(c))\n \n /* Match pattern \"p\" against \"text\" */\n-static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n+static int dowild(const uchar *p, const uchar *text,\n+\t\t const uchar *textend, unsigned int flags)\n {\n \tuchar p_ch;\n \tconst uchar *pattern = p;\n@@ -60,8 +61,9 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \tfor ( ; (p_ch = *p) != '\\0'; text++, p++) {\n \t\tint matched, match_slash, negated;\n \t\tuchar t_ch, prev_ch;\n-\t\tif ((t_ch = *text) == '\\0' && p_ch != '*')\n+\t\tif (text >= textend && p_ch != '*')\n \t\t\treturn WM_ABORT_ALL;\n+\t\tt_ch = *text;\n \t\tif ((flags & WM_CASEFOLD) && ISUPPER(t_ch))\n \t\t\tt_ch = tolower(t_ch);\n \t\tif ((flags & WM_CASEFOLD) && ISUPPER(p_ch))\n@@ -101,7 +103,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\t * both foo/bar and foo/a/bar.\n \t\t\t\t\t */\n \t\t\t\t\tif (p[0] == '/' &&\n-\t\t\t\t\t dowild(p + 1, text, flags) == WM_MATCH)\n+\t\t\t\t\t dowild(p + 1, text, textend, flags) == WM_MATCH)\n \t\t\t\t\t\treturn WM_MATCH;\n \t\t\t\t\tmatch_slash = 1;\n \t\t\t\t} else\n@@ -130,9 +132,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t/* the slash is consumed by the top-level for loop */\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\twhile (1) {\n-\t\t\t\tif (t_ch == '\\0')\n-\t\t\t\t\tbreak;\n+\t\t\twhile (text < textend) {\n \t\t\t\t/*\n \t\t\t\t * Try to advance faster when an asterisk is\n \t\t\t\t * followed by a literal. We know in this case\n@@ -145,18 +145,18 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\tp_ch = *p;\n \t\t\t\t\tif ((flags & WM_CASEFOLD) && ISUPPER(p_ch))\n \t\t\t\t\t\tp_ch = tolower(p_ch);\n-\t\t\t\t\twhile ((t_ch = *text) != '\\0' &&\n+\t\t\t\t\twhile (text < textend &&\n \t\t\t\t\t (match_slash || t_ch != '/')) {\n \t\t\t\t\t\tif ((flags & WM_CASEFOLD) && ISUPPER(t_ch))\n \t\t\t\t\t\t\tt_ch = tolower(t_ch);\n \t\t\t\t\t\tif (t_ch == p_ch)\n \t\t\t\t\t\t\tbreak;\n-\t\t\t\t\t\ttext++;\n+\t\t\t\t\t\tt_ch = *++text;\n \t\t\t\t\t}\n \t\t\t\t\tif (t_ch != p_ch)\n \t\t\t\t\t\treturn WM_NOMATCH;\n \t\t\t\t}\n-\t\t\t\tif ((matched = dowild(p, text, flags)) != WM_NOMATCH) {\n+\t\t\t\tif ((matched = dowild(p, text, textend, flags)) != WM_NOMATCH) {\n \t\t\t\t\tif (!match_slash || matched != WM_ABORT_TO_STARSTAR)\n \t\t\t\t\t\treturn matched;\n \t\t\t\t} else if (!match_slash && t_ch == '/')\n@@ -261,12 +261,13 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t}\n \t}\n \n-\treturn *text ? WM_NOMATCH : WM_MATCH;\n+\treturn text < textend ? WM_NOMATCH : WM_MATCH;\n }\n \n /* Match the \"pattern\" against the \"text\" string. */\n-int wildmatch(const char *pattern, const char *text,\n+int nwildmatch(const char *pattern, const char *text, int textlen,\n \t unsigned int flags, struct wildopts *wo)\n {\n-\treturn dowild((const uchar*)pattern, (const uchar*)text, flags);\n+\treturn dowild((const uchar*)pattern, (const uchar*)text,\n+\t\t (const uchar*)text + textlen, flags);\n }\ndiff --git a/wildmatch.h b/wildmatch.h\nindex 4090c8f..cdd7544 100644\n--- a/wildmatch.h\n+++ b/wildmatch.h\n@@ -12,7 +12,14 @@\n \n struct wildopts;\n \n-int wildmatch(const char *pattern, const char *text,\n-\t unsigned int flags,\n+int nwildmatch(const char *pattern, const char *text,\n+\t int len, unsigned int flags,\n \t struct wildopts *wo);\n+\n+/* Match the \"pattern\" against the \"text\" string. */\n+static inline int wildmatch(const char *pattern, const char *text,\n+\t unsigned int flags, struct wildopts *wo)\n+{\n+\treturn nwildmatch(pattern, text, strlen(text), flags, wo);\n+}\n #endif\n-- \n1.8.2.83.gc99314b\n\n-- 8< --\n"},{"id":"212022","messageId":"20130323044331.GA11256@lanh","threadId":"33230","inReplyTo":"20130323041824.GA11142@lanh","subject":"Re: [regression?] trailing slash required in .gitattributes","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-23T04:43:32Z","receivedAt":"2013-03-23T04:43:32Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 23, 2013 at 11:18:24AM +0700, Duy Nguyen wrote:\n> You can use nwildmatch() from this patch. I tested it lightly with\n> t3070-wildmatch.sh, feeding the strings with no terminating NUL. It\n> seems to work ok.\n\nAnd valgrind spotted my faults, especially for using strchr. You would\nneed this on top:\n\n-- 8< --\ndiff --git a/wildmatch.c b/wildmatch.c\nindex f97ae2a..939bac8 100644\n--- a/wildmatch.c\n+++ b/wildmatch.c\n@@ -61,9 +61,13 @@ static int dowild(const uchar *p, const uchar *text,\n \tfor ( ; (p_ch = *p) != '\\0'; text++, p++) {\n \t\tint matched, match_slash, negated;\n \t\tuchar t_ch, prev_ch;\n-\t\tif (text >= textend && p_ch != '*')\n-\t\t\treturn WM_ABORT_ALL;\n-\t\tt_ch = *text;\n+\t\tif (text >= textend) {\n+\t\t\tif (p_ch != '*')\n+\t\t\t\treturn WM_ABORT_ALL;\n+\t\t\telse\n+\t\t\t\tt_ch = '\\0';\n+\t\t} else\n+\t\t\tt_ch = *text;\n \t\tif ((flags & WM_CASEFOLD) && ISUPPER(t_ch))\n \t\t\tt_ch = tolower(t_ch);\n \t\tif ((flags & WM_CASEFOLD) && ISUPPER(p_ch))\n@@ -115,8 +119,9 @@ static int dowild(const uchar *p, const uchar *text,\n \t\t\t\t/* Trailing \"**\" matches everything. Trailing \"*\" matches\n \t\t\t\t * only if there are no more slash characters. */\n \t\t\t\tif (!match_slash) {\n-\t\t\t\t\tif (strchr((char*)text, '/') != NULL)\n-\t\t\t\t\t\treturn WM_NOMATCH;\n+\t\t\t\t\tfor (;text < textend; text++)\n+\t\t\t\t\t\tif (*text == '/')\n+\t\t\t\t\t\t\treturn WM_NOMATCH;\n \t\t\t\t}\n \t\t\t\treturn WM_MATCH;\n \t\t\t} else if (!match_slash && *p == '/') {\n@@ -125,10 +130,11 @@ static int dowild(const uchar *p, const uchar *text,\n \t\t\t\t * with WM_PATHNAME matches the next\n \t\t\t\t * directory\n \t\t\t\t */\n-\t\t\t\tconst char *slash = strchr((char*)text, '/');\n-\t\t\t\tif (!slash)\n+\t\t\t\tfor (;text < textend; text++)\n+\t\t\t\t\tif (*text == '/')\n+\t\t\t\t\t\tbreak;\n+\t\t\t\tif (text == textend)\n \t\t\t\t\treturn WM_NOMATCH;\n-\t\t\t\ttext = (const uchar*)slash;\n \t\t\t\t/* the slash is consumed by the top-level for loop */\n \t\t\t\tbreak;\n \t\t\t}\n@@ -151,7 +157,7 @@ static int dowild(const uchar *p, const uchar *text,\n \t\t\t\t\t\t\tt_ch = tolower(t_ch);\n \t\t\t\t\t\tif (t_ch == p_ch)\n \t\t\t\t\t\t\tbreak;\n-\t\t\t\t\t\tt_ch = *++text;\n+\t\t\t\t\t\tt_ch = ++text < textend ? *text : '\\0';\n \t\t\t\t\t}\n \t\t\t\t\tif (t_ch != p_ch)\n \t\t\t\t\t\treturn WM_NOMATCH;\n-- 8< --\n"},{"id":"212037","messageId":"20130323083927.GA25600@sigill.intra.peff.net","threadId":"33230","inReplyTo":"7vzjxv3uef.fsf@alter.siamese.dyndns.org","subject":"Re: [regression?] trailing slash required in .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-23T08:39:29Z","receivedAt":"2013-03-23T08:39:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 22, 2013 at 04:08:08PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > if (pathlen && pathname[pathlen-1] == '/')\n> > pathlen--;\n> >\n> > would work. But it seems that match_basename, despite taking the length\n> > of all of the strings we pass it, will happily use NUL-terminated\n> > functions like strcmp or fnmatch. Converting the former to check lengths\n> > should be pretty straightforward. But there is no version of fnmatch\n> > that does what we want. I wonder if we using wildmatch can get around\n> > this limitation.\n> \n> Or save away pathname[pathlen], temporarily NUL terminate and call\n> these functions?\n\nYeah, that is a possibility, though it involves casting away some\nconstness. Patch is below, which seems to work.\n\nIt still feels really ugly to me, and like match_basename is misdesigned\nand should respect the lengths we pass it. Also, if it does respect the\nlengths, it should be able to go much faster (e.g., in the common case,\nwe can drop a ton of strcmp_icase calls if we just check the lengths\nbeforehand). I feel like Duy was working on something like this\nrecently, but I don't see anything in pu.\n\n---\ndiff --git a/attr.c b/attr.c\nindex e2f9377..bd00a78 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -663,20 +663,58 @@ static int path_matches(const char *pathname, int pathlen,\n {\n \tconst char *pattern = pat->pattern;\n \tint prefix = pat->nowildcardlen;\n+\tchar path_munge = 0;\n+\tchar pattern_munge = 0;\n+\tint ret;\n \n \tif ((pat->flags & EXC_FLAG_MUSTBEDIR) &&\n \t ((!pathlen) || (pathname[pathlen-1] != '/')))\n \t\treturn 0;\n \n+\t/*\n+\t * Drop trailing slash from path, as we would want\n+\t * an unadorned pattern like \"foo\" to match both the\n+\t * file \"foo\" and the directory \"foo/\".\n+\t */\n+\tif (pathlen && pathname[pathlen-1] == '/') {\n+\t\tpathlen--;\n+\n+\t\t/*\n+\t\t * The match_* functions, despite taking a string length, will\n+\t\t * happily read all the way up to the NUL-terminating character.\n+\t\t * So we must not only shrink pathlen, but munge the buffer\n+\t\t * to NUL-terminate it.\n+\t\t */\n+\t\tpath_munge = pathname[pathlen];\n+\t\t((char *)pathname)[pathlen] = '\\0';\n+\t}\n+\n+\t/*\n+\t * The pattern up to patternlen will not include a\n+\t * trailing slash, but it may still be present in the string.\n+\t * And since the match_* functions will read up to the NUL,\n+\t * we need to terminate the buffer.\n+\t */\n+\tpattern_munge = pattern[pat->patternlen];\n+\t((char *)pattern)[pat->patternlen] = '\\0';\n+\n \tif (pat->flags & EXC_FLAG_NODIR) {\n-\t\treturn match_basename(basename,\n-\t\t\t\t pathlen - (basename - pathname),\n-\t\t\t\t pattern, prefix,\n-\t\t\t\t pat->patternlen, pat->flags);\n-\t}\n-\treturn match_pathname(pathname, pathlen,\n-\t\t\t base, baselen,\n-\t\t\t pattern, prefix, pat->patternlen, pat->flags);\n+\t\tret = match_basename(basename,\n+\t\t\t\t pathlen - (basename - pathname),\n+\t\t\t\t pattern, prefix,\n+\t\t\t\t pat->patternlen, pat->flags);\n+\t}\n+\telse {\n+\t\tret = match_pathname(pathname, pathlen,\n+\t\t\t\t base, baselen,\n+\t\t\t\t pattern, prefix,\n+\t\t\t\t pat->patternlen, pat->flags);\n+\t}\n+\n+\tif (path_munge)\n+\t\t((char *)pathname)[pathlen] = path_munge;\n+\t((char *)pattern)[pat->patternlen] = pattern_munge;\n+\treturn ret;\n }\n \n static int macroexpand_one(int attr_nr, int rem);\ndiff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh\nindex 0c847fb..3be809c 100755\n--- a/t/t5002-archive-attr-pattern.sh\n+++ b/t/t5002-archive-attr-pattern.sh\n@@ -27,6 +27,10 @@ test_expect_success 'setup' '\n \techo ignored-only-if-dir/ export-ignore >>.git/info/attributes &&\n \tgit add ignored-only-if-dir &&\n \n+\tmkdir -p ignored-without-slash &&\n+\techo ignored without slash >ignored-without-slash/foo &&\n+\tgit add ignored-without-slash/foo &&\n+\techo ignored-without-slash export-ignore >>.git/info/attributes &&\n \n \tmkdir -p one-level-lower/two-levels-lower/ignored-only-if-dir &&\n \techo ignored by ignored dir >one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir &&\n@@ -49,6 +53,8 @@ test_expect_missing\tarchive/ignored-ony-if-dir/ignored-by-ignored-dir\n test_expect_exists\tarchive/not-ignored-dir/\n test_expect_missing\tarchive/ignored-only-if-dir/\n test_expect_missing\tarchive/ignored-ony-if-dir/ignored-by-ignored-dir\n+test_expect_missing archive/ignored-without-slash/ &&\n+test_expect_missing archive/ignored-without-slash/foo &&\n test_expect_exists\tarchive/one-level-lower/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-only-if-dir/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-ony-if-dir/ignored-by-ignored-dir\n"},{"id":"212073","messageId":"7v8v5d4bed.fsf@alter.siamese.dyndns.org","threadId":"33230","inReplyTo":"20130323083927.GA25600@sigill.intra.peff.net","subject":"Re: [regression?] trailing slash required in .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-24T05:25:30Z","receivedAt":"2013-03-24T05:25:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, that is a possibility, though it involves casting away some\n> constness. Patch is below, which seems to work.\n\nHmm, because this was after I read this part:\n\n> ... match_basename, despite taking the length\n> of all of the strings we pass it, will happily use NUL-terminated\n> functions like strcmp or fnmatch.\n\nI actually meant to do that inside match_basename(), not in one\nparticular caller of it. Otherwise, new callers to match_basename()\nwill also suffer from this broken API that pretends as if it takes\ncounted strings but uses strings as NUL terminated, no?\n\n> It still feels really ugly to me, and like match_basename is misdesigned\n> and should respect the lengths we pass it.\n\nExactly.\n"},{"id":"212142","messageId":"1364191510-8900-1-git-send-email-pclouds@gmail.com","threadId":"33230","inReplyTo":"20130319175756.GA13760@sigill.intra.peff.net","subject":"[PATCH 0/4] attr directory matching regression","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-25T06:05:06Z","receivedAt":"2013-03-25T06:05:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"I think the fix is something like this. There is still one thing I'd\nlike to do: make this code not rely on NUL for terminating the\npatterns. That should remove the ugly \"p[len] = '\\0'\" in\nprepare_attr_stack() 4/4 and and the reallocation in add_exclude() (in\ncurrent code). But let's deal with the regression first.\n\nNguyễn Thái Ngọc Duy (4):\n wildmatch: do not require \"text\" to be NUL-terminated\n attr.c: fix pattern{,len} inconsistency in struct match_attr\n dir.c: make match_{base,path}name respect {basename,path}len\n attr.c: fix matching \"subdir\" without the trailing slash\n\n attr.c | 11 ++++++++++-\n dir.c | 13 ++++++++-----\n dir.h | 2 +-\n t/t5002-archive-attr-pattern.sh | 6 ++++++\n wildmatch.c | 43 ++++++++++++++++++++++++-----------------\n wildmatch.h | 11 +++++++++--\n 6 files changed, 59 insertions(+), 27 deletions(-)\n\n-- \n1.8.2.82.gc24b958\n"},{"id":"212143","messageId":"1364191510-8900-2-git-send-email-pclouds@gmail.com","threadId":"33230","inReplyTo":"1364191510-8900-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/4] wildmatch: do not require \"text\" to be NUL-terminated","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-25T06:05:07Z","receivedAt":"2013-03-25T06:05:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This may be helpful when we just want to match a part of \"text\".\nnwildmatch can be used for this purpose.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n wildmatch.c | 43 +++++++++++++++++++++++++------------------\n wildmatch.h | 11 +++++++++--\n 2 files changed, 34 insertions(+), 20 deletions(-)\n\ndiff --git a/wildmatch.c b/wildmatch.c\nindex 7192bdc..939bac8 100644\n--- a/wildmatch.c\n+++ b/wildmatch.c\n@@ -52,7 +52,8 @@ typedef unsigned char uchar;\n #define ISXDIGIT(c) (ISASCII(c) && isxdigit(c))\n \n /* Match pattern \"p\" against \"text\" */\n-static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n+static int dowild(const uchar *p, const uchar *text,\n+\t\t const uchar *textend, unsigned int flags)\n {\n \tuchar p_ch;\n \tconst uchar *pattern = p;\n@@ -60,8 +61,13 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \tfor ( ; (p_ch = *p) != '\\0'; text++, p++) {\n \t\tint matched, match_slash, negated;\n \t\tuchar t_ch, prev_ch;\n-\t\tif ((t_ch = *text) == '\\0' && p_ch != '*')\n-\t\t\treturn WM_ABORT_ALL;\n+\t\tif (text >= textend) {\n+\t\t\tif (p_ch != '*')\n+\t\t\t\treturn WM_ABORT_ALL;\n+\t\t\telse\n+\t\t\t\tt_ch = '\\0';\n+\t\t} else\n+\t\t\tt_ch = *text;\n \t\tif ((flags & WM_CASEFOLD) && ISUPPER(t_ch))\n \t\t\tt_ch = tolower(t_ch);\n \t\tif ((flags & WM_CASEFOLD) && ISUPPER(p_ch))\n@@ -101,7 +107,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\t * both foo/bar and foo/a/bar.\n \t\t\t\t\t */\n \t\t\t\t\tif (p[0] == '/' &&\n-\t\t\t\t\t dowild(p + 1, text, flags) == WM_MATCH)\n+\t\t\t\t\t dowild(p + 1, text, textend, flags) == WM_MATCH)\n \t\t\t\t\t\treturn WM_MATCH;\n \t\t\t\t\tmatch_slash = 1;\n \t\t\t\t} else\n@@ -113,8 +119,9 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t/* Trailing \"**\" matches everything. Trailing \"*\" matches\n \t\t\t\t * only if there are no more slash characters. */\n \t\t\t\tif (!match_slash) {\n-\t\t\t\t\tif (strchr((char*)text, '/') != NULL)\n-\t\t\t\t\t\treturn WM_NOMATCH;\n+\t\t\t\t\tfor (;text < textend; text++)\n+\t\t\t\t\t\tif (*text == '/')\n+\t\t\t\t\t\t\treturn WM_NOMATCH;\n \t\t\t\t}\n \t\t\t\treturn WM_MATCH;\n \t\t\t} else if (!match_slash && *p == '/') {\n@@ -123,16 +130,15 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t * with WM_PATHNAME matches the next\n \t\t\t\t * directory\n \t\t\t\t */\n-\t\t\t\tconst char *slash = strchr((char*)text, '/');\n-\t\t\t\tif (!slash)\n+\t\t\t\tfor (;text < textend; text++)\n+\t\t\t\t\tif (*text == '/')\n+\t\t\t\t\t\tbreak;\n+\t\t\t\tif (text == textend)\n \t\t\t\t\treturn WM_NOMATCH;\n-\t\t\t\ttext = (const uchar*)slash;\n \t\t\t\t/* the slash is consumed by the top-level for loop */\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\twhile (1) {\n-\t\t\t\tif (t_ch == '\\0')\n-\t\t\t\t\tbreak;\n+\t\t\twhile (text < textend) {\n \t\t\t\t/*\n \t\t\t\t * Try to advance faster when an asterisk is\n \t\t\t\t * followed by a literal. We know in this case\n@@ -145,18 +151,18 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\tp_ch = *p;\n \t\t\t\t\tif ((flags & WM_CASEFOLD) && ISUPPER(p_ch))\n \t\t\t\t\t\tp_ch = tolower(p_ch);\n-\t\t\t\t\twhile ((t_ch = *text) != '\\0' &&\n+\t\t\t\t\twhile (text < textend &&\n \t\t\t\t\t (match_slash || t_ch != '/')) {\n \t\t\t\t\t\tif ((flags & WM_CASEFOLD) && ISUPPER(t_ch))\n \t\t\t\t\t\t\tt_ch = tolower(t_ch);\n \t\t\t\t\t\tif (t_ch == p_ch)\n \t\t\t\t\t\t\tbreak;\n-\t\t\t\t\t\ttext++;\n+\t\t\t\t\t\tt_ch = ++text < textend ? *text : '\\0';\n \t\t\t\t\t}\n \t\t\t\t\tif (t_ch != p_ch)\n \t\t\t\t\t\treturn WM_NOMATCH;\n \t\t\t\t}\n-\t\t\t\tif ((matched = dowild(p, text, flags)) != WM_NOMATCH) {\n+\t\t\t\tif ((matched = dowild(p, text, textend, flags)) != WM_NOMATCH) {\n \t\t\t\t\tif (!match_slash || matched != WM_ABORT_TO_STARSTAR)\n \t\t\t\t\t\treturn matched;\n \t\t\t\t} else if (!match_slash && t_ch == '/')\n@@ -261,12 +267,13 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t}\n \t}\n \n-\treturn *text ? WM_NOMATCH : WM_MATCH;\n+\treturn text < textend ? WM_NOMATCH : WM_MATCH;\n }\n \n /* Match the \"pattern\" against the \"text\" string. */\n-int wildmatch(const char *pattern, const char *text,\n+int nwildmatch(const char *pattern, const char *text, int textlen,\n \t unsigned int flags, struct wildopts *wo)\n {\n-\treturn dowild((const uchar*)pattern, (const uchar*)text, flags);\n+\treturn dowild((const uchar*)pattern, (const uchar*)text,\n+\t\t (const uchar*)text + textlen, flags);\n }\ndiff --git a/wildmatch.h b/wildmatch.h\nindex 4090c8f..cdd7544 100644\n--- a/wildmatch.h\n+++ b/wildmatch.h\n@@ -12,7 +12,14 @@\n \n struct wildopts;\n \n-int wildmatch(const char *pattern, const char *text,\n-\t unsigned int flags,\n+int nwildmatch(const char *pattern, const char *text,\n+\t int len, unsigned int flags,\n \t struct wildopts *wo);\n+\n+/* Match the \"pattern\" against the \"text\" string. */\n+static inline int wildmatch(const char *pattern, const char *text,\n+\t unsigned int flags, struct wildopts *wo)\n+{\n+\treturn nwildmatch(pattern, text, strlen(text), flags, wo);\n+}\n #endif\n-- \n1.8.2.82.gc24b958\n"},{"id":"212144","messageId":"1364191510-8900-3-git-send-email-pclouds@gmail.com","threadId":"33230","inReplyTo":"1364191510-8900-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/4] attr.c: fix pattern{,len} inconsistency in struct match_attr","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-25T06:05:08Z","receivedAt":"2013-03-25T06:05:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When parse_exclude_pattern detects EXC_FLAG_MUSTBEDIR, it sets\npatternlen _not_ to include the trailing slash and expects the caller\nto trim it. Of the two callers, add_exclude() does, parse_attr_line()\ndoes not.\n\nBecause of that, after parse_attr_line() returns, we may have pattern\n\"foo/\" but its length is reported 3. Some functions do not care about\npatternlen and will see the pattern as \"foo/\" while others may see it\nas \"foo\". This patch makes patternlen reflect the true length of\npattern.\n\nThis is a bandage patch that's required for the next patch to pass the\ntest suite as that patch will rely on patternlen's correctness. The\ntrue fix comes in the patch after the next one.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n attr.c | 2 ++\n dir.h | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/attr.c b/attr.c\nindex e2f9377..1818ba5 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -255,6 +255,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\t\t\t &res->u.pat.patternlen,\n \t\t\t\t &res->u.pat.flags,\n \t\t\t\t &res->u.pat.nowildcardlen);\n+\t\tif (res->u.pat.flags & EXC_FLAG_MUSTBEDIR)\n+\t\t\tres->u.pat.patternlen++;\n \t\tif (res->u.pat.flags & EXC_FLAG_NEGATIVE) {\n \t\t\twarning(_(\"Negative patterns are ignored in git attributes\\n\"\n \t\t\t\t \"Use '\\\\!' for literal leading exclamation.\"));\ndiff --git a/dir.h b/dir.h\nindex c3eb4b5..dc63fc8 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -40,7 +40,7 @@ struct exclude_list {\n \t\tstruct exclude_list *el;\n \n \t\tconst char *pattern;\n-\t\tint patternlen;\n+\t\tint patternlen;\t/* must equal strlen(pattern) */\n \t\tint nowildcardlen;\n \t\tconst char *base;\n \t\tint baselen;\n-- \n1.8.2.82.gc24b958\n"},{"id":"212145","messageId":"1364191510-8900-4-git-send-email-pclouds@gmail.com","threadId":"33230","inReplyTo":"1364191510-8900-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/4] dir.c: make match_{base,path}name respect {basename,path}len","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-25T06:05:09Z","receivedAt":"2013-03-25T06:05:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When match_basename was split out of excluded_from_list in 593cb88\n(exclude: split basename matching code into a separate function -\n2012-10-15), it took basenamelen only as a hint. basename was required\nto be NUL-terminated at the given length.\n\nThis was fine until match_basename had a new caller (from attr.c) and\ntherefore was no longer excluded_from_list's internal business. Make\nmatch_basename stop relying on the NUL assumption above.\n\nDo the same for match_pathname. From now on, only pattern is required\nto be NUL-terminated at the specified patternlen for both\nmatch_{base,path}name.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n dir.c | 13 ++++++++-----\n 1 file changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 57394e4..30f5241 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -626,15 +626,18 @@ int match_basename(const char *basename, int basenamelen,\n \t\t int flags)\n {\n \tif (prefix == patternlen) {\n-\t\tif (!strcmp_icase(pattern, basename))\n+\t\tif (patternlen == basenamelen &&\n+\t\t !strncmp_icase(pattern, basename, basenamelen))\n \t\t\treturn 1;\n \t} else if (flags & EXC_FLAG_ENDSWITH) {\n \t\tif (patternlen - 1 <= basenamelen &&\n-\t\t !strcmp_icase(pattern + 1,\n-\t\t\t\t basename + basenamelen - patternlen + 1))\n+\t\t !strncmp_icase(pattern + 1,\n+\t\t\t\t basename + basenamelen - patternlen + 1,\n+\t\t\t\t patternlen - 1))\n \t\t\treturn 1;\n \t} else {\n-\t\tif (fnmatch_icase(pattern, basename, 0) == 0)\n+\t\tif (nwildmatch(pattern, basename, basenamelen,\n+\t\t\t ignore_case ? WM_CASEFOLD : 0, NULL) == 0)\n \t\t\treturn 1;\n \t}\n \treturn 0;\n@@ -684,7 +687,7 @@ int match_pathname(const char *pathname, int pathlen,\n \t\tnamelen -= prefix;\n \t}\n \n-\treturn wildmatch(pattern, name,\n+\treturn nwildmatch(pattern, name, namelen,\n \t\t\t WM_PATHNAME | (ignore_case ? WM_CASEFOLD : 0),\n \t\t\t NULL) == 0;\n }\n-- \n1.8.2.82.gc24b958\n"},{"id":"212146","messageId":"1364191510-8900-5-git-send-email-pclouds@gmail.com","threadId":"33230","inReplyTo":"1364191510-8900-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 4/4] attr.c: fix matching \"subdir\" without the trailing slash","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-25T06:05:10Z","receivedAt":"2013-03-25T06:05:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The story goes back to 94bc671 (Add directory pattern matching to\nattributes - 2012-12-08). Before this commit, directories are passed\nto path_matches without the trailing slash. This is fine for matching\npattern \"subdir\" with \"foo/subdir\".\n\nPatterns like \"subdir/\" (i.e. match _directory_ subdir) won't work\nthough. So paths are now passed to path_matches with the trailing\nslash (i.e. \"subdir/\"). The trailing slash is used as the directory\nindicator (similar to dtype in exclude case). This makes pattern\n\"subdir/\" match directory \"subdir/\". Pattern \"subdir\" no longer match\nsubdir, which is now \"subdir/\".\n\nAs the trailing slash in pathname is the directory indicator, we do\nnot need to keep it in the pathname for matching. The trailing slash\nshould be turned to dtype \"DT_DIR\" and stripped out of pathname. This\nkeeps the code pattern similar to exclude.\n\nThe same applies for the pattern \"subdir/\". The trailing slash is\nconverted to flag EXC_FLAG_MUSTBEDIR and should not remain in the\npattern, as noted in parse_exclude_pattern(). prepare_attr_stack()\nbreaks this and keeps the trailing slash anyway.\n\nTo sum up, both patterns and pathnames should never have the trailing\nslash when it comes to match_basename.\n\nReported-and-analyzed-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n attr.c | 11 +++++++++--\n t/t5002-archive-attr-pattern.sh | 6 ++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 1818ba5..a95c837 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -256,7 +256,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\t\t\t &res->u.pat.flags,\n \t\t\t\t &res->u.pat.nowildcardlen);\n \t\tif (res->u.pat.flags & EXC_FLAG_MUSTBEDIR)\n-\t\t\tres->u.pat.patternlen++;\n+\t\t\tp[res->u.pat.patternlen] = '\\0';\n \t\tif (res->u.pat.flags & EXC_FLAG_NEGATIVE) {\n \t\t\twarning(_(\"Negative patterns are ignored in git attributes\\n\"\n \t\t\t\t \"Use '\\\\!' for literal leading exclamation.\"));\n@@ -665,9 +665,16 @@ static int path_matches(const char *pathname, int pathlen,\n {\n \tconst char *pattern = pat->pattern;\n \tint prefix = pat->nowildcardlen;\n+\tint dtype;\n+\n+\tif (pathlen && pathname[pathlen-1] == '/') {\n+\t\tdtype = DT_DIR;\n+\t\tpathlen--;\n+\t} else\n+\t\tdtype = DT_REG;\n \n \tif ((pat->flags & EXC_FLAG_MUSTBEDIR) &&\n-\t ((!pathlen) || (pathname[pathlen-1] != '/')))\n+\t dtype != DT_DIR)\n \t\treturn 0;\n \n \tif (pat->flags & EXC_FLAG_NODIR) {\ndiff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh\nindex 0c847fb..98ccc3c 100755\n--- a/t/t5002-archive-attr-pattern.sh\n+++ b/t/t5002-archive-attr-pattern.sh\n@@ -27,6 +27,10 @@ test_expect_success 'setup' '\n \techo ignored-only-if-dir/ export-ignore >>.git/info/attributes &&\n \tgit add ignored-only-if-dir &&\n \n+\tmkdir -p ignored-without-slash &&\n+\techo ignored without slash >ignored-without-slash/foo &&\n+\tgit add ignored-without-slash/foo &&\n+\techo ignored-without-slash export-ignore >>.git/info/attributes &&\n \n \tmkdir -p one-level-lower/two-levels-lower/ignored-only-if-dir &&\n \techo ignored by ignored dir >one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir &&\n@@ -49,6 +53,8 @@ test_expect_exists\tarchive/not-ignored-dir/ignored-only-if-dir\n test_expect_exists\tarchive/not-ignored-dir/\n test_expect_missing\tarchive/ignored-only-if-dir/\n test_expect_missing\tarchive/ignored-ony-if-dir/ignored-by-ignored-dir\n+test_expect_missing\tarchive/ignored-without-slash/ &&\n+test_expect_missing\tarchive/ignored-without-slash/foo &&\n test_expect_exists\tarchive/one-level-lower/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-only-if-dir/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-ony-if-dir/ignored-by-ignored-dir\n-- \n1.8.2.82.gc24b958\n"},{"id":"212147","messageId":"CACsJy8CNjKP1y925mb8_t_hs0QCZwaY6mYTe6EMvbJk2pPifew@mail.gmail.com","threadId":"33230","inReplyTo":"1364191510-8900-5-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 4/4] attr.c: fix matching \"subdir\" without the trailing slash","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-25T07:20:31Z","receivedAt":"2013-03-25T07:20:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 25, 2013 at 1:05 PM, Nguyễn Thái Ngọc Duy wrote:\n> The story goes back to 94bc671 (Add directory pattern matching to\n> attributes - 2012-12-08). Before this commit, directories are passed\n> to path_matches without the trailing slash. This is fine for matching\n> pattern \"subdir\" with \"foo/subdir\".\n>\n> Patterns like \"subdir/\" (i.e. match _directory_ subdir) won't work\n> though. So paths are now passed to path_matches with the trailing\n> slash (i.e. \"subdir/\"). The trailing slash is used as the directory\n> indicator (similar to dtype in exclude case). This makes pattern\n> \"subdir/\" match directory \"subdir/\". Pattern \"subdir\" no longer match\n> subdir, which is now \"subdir/\".\n>\n> As the trailing slash in pathname is the directory indicator, we do\n> not need to keep it in the pathname for matching. The trailing slash\n> should be turned to dtype \"DT_DIR\" and stripped out of pathname. This\n> keeps the code pattern similar to exclude.\n\nOn second thought, maybe we should not pass path \"subdir/\" at all.\nInstead we create a fake dtype based on the trailing slash and pass it\ndown to attr.c:fill() -> path_matches(), just like how\nlast_exclude_matching_from_list() is called.\n-- \nDuy\n"},{"id":"212154","messageId":"20130325092424.GA17291@duynguyen-vnpc.dek-tpc.internal","threadId":"33230","inReplyTo":"CACsJy8CNjKP1y925mb8_t_hs0QCZwaY6mYTe6EMvbJk2pPifew@mail.gmail.com","subject":"Re: [PATCH 4/4] attr.c: fix matching \"subdir\" without the trailing slash","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-25T09:24:24Z","receivedAt":"2013-03-25T09:24:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 25, 2013 at 02:20:31PM +0700, Duy Nguyen wrote:\n> On second thought, maybe we should not pass path \"subdir/\" at all.\n> Instead we create a fake dtype based on the trailing slash and pass it\n> down to attr.c:fill() -> path_matches(), just like how\n> last_exclude_matching_from_list() is called.\n\nI was hoping to make a small patch, but as it turns out,\ncollect_all_attrs() takes a const path that contains the trailing\nslash, we still need to ignore it in match_{base,path}name so the\nwhole series is still required. The only difference is in the final\npatch, which is a bit longer:\n\n-- 8< --\ndiff --git a/attr.c b/attr.c\nindex 1818ba5..0b2b716 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -256,7 +256,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\t\t\t &res->u.pat.flags,\n \t\t\t\t &res->u.pat.nowildcardlen);\n \t\tif (res->u.pat.flags & EXC_FLAG_MUSTBEDIR)\n-\t\t\tres->u.pat.patternlen++;\n+\t\t\tp[res->u.pat.patternlen] = '\\0';\n \t\tif (res->u.pat.flags & EXC_FLAG_NEGATIVE) {\n \t\t\twarning(_(\"Negative patterns are ignored in git attributes\\n\"\n \t\t\t\t \"Use '\\\\!' for literal leading exclamation.\"));\n@@ -659,15 +659,14 @@ static void prepare_attr_stack(const char *path, int dirlen)\n }\n \n static int path_matches(const char *pathname, int pathlen,\n-\t\t\tconst char *basename,\n+\t\t\tconst char *basename, int dtype,\n \t\t\tconst struct pattern *pat,\n \t\t\tconst char *base, int baselen)\n {\n \tconst char *pattern = pat->pattern;\n \tint prefix = pat->nowildcardlen;\n \n-\tif ((pat->flags & EXC_FLAG_MUSTBEDIR) &&\n-\t ((!pathlen) || (pathname[pathlen-1] != '/')))\n+\tif ((pat->flags & EXC_FLAG_MUSTBEDIR) && dtype != DT_DIR)\n \t\treturn 0;\n \n \tif (pat->flags & EXC_FLAG_NODIR) {\n@@ -706,7 +705,7 @@ static int fill_one(const char *what, struct match_attr *a, int rem)\n }\n \n static int fill(const char *path, int pathlen, const char *basename,\n-\t\tstruct attr_stack *stk, int rem)\n+\t\tint dtype, struct attr_stack *stk, int rem)\n {\n \tint i;\n \tconst char *base = stk->origin ? stk->origin : \"\";\n@@ -715,7 +714,7 @@ static int fill(const char *path, int pathlen, const char *basename,\n \t\tstruct match_attr *a = stk->attrs[i];\n \t\tif (a->is_macro)\n \t\t\tcontinue;\n-\t\tif (path_matches(path, pathlen, basename,\n+\t\tif (path_matches(path, pathlen, basename, dtype,\n \t\t\t\t &a->u.pat, base, stk->originlen))\n \t\t\trem = fill_one(\"fill\", a, rem);\n \t}\n@@ -755,11 +754,17 @@ static void collect_all_attrs(const char *path)\n \tstruct attr_stack *stk;\n \tint i, pathlen, rem, dirlen;\n \tconst char *basename, *cp, *last_slash = NULL;\n+\tint dtype;\n \n \tfor (cp = path; *cp; cp++) {\n \t\tif (*cp == '/' && cp[1])\n \t\t\tlast_slash = cp;\n \t}\n+\tif (cp > path && cp[-1] == '/') {\n+\t\tdtype = DT_DIR;\n+\t\tcp--;\n+\t} else\n+\t\tdtype = DT_REG;\n \tpathlen = cp - path;\n \tif (last_slash) {\n \t\tbasename = last_slash + 1;\n@@ -775,7 +780,7 @@ static void collect_all_attrs(const char *path)\n \n \trem = attr_nr;\n \tfor (stk = attr_stack; 0 < rem && stk; stk = stk->prev)\n-\t\trem = fill(path, pathlen, basename, stk, rem);\n+\t\trem = fill(path, pathlen, basename, dtype, stk, rem);\n }\n \n int git_check_attr(const char *path, int num, struct git_attr_check *check)\ndiff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh\nindex 0c847fb..98ccc3c 100755\n--- a/t/t5002-archive-attr-pattern.sh\n+++ b/t/t5002-archive-attr-pattern.sh\n@@ -27,6 +27,10 @@ test_expect_success 'setup' '\n \techo ignored-only-if-dir/ export-ignore >>.git/info/attributes &&\n \tgit add ignored-only-if-dir &&\n \n+\tmkdir -p ignored-without-slash &&\n+\techo ignored without slash >ignored-without-slash/foo &&\n+\tgit add ignored-without-slash/foo &&\n+\techo ignored-without-slash export-ignore >>.git/info/attributes &&\n \n \tmkdir -p one-level-lower/two-levels-lower/ignored-only-if-dir &&\n \techo ignored by ignored dir >one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir &&\n@@ -49,6 +53,8 @@ test_expect_exists\tarchive/not-ignored-dir/ignored-only-if-dir\n test_expect_exists\tarchive/not-ignored-dir/\n test_expect_missing\tarchive/ignored-only-if-dir/\n test_expect_missing\tarchive/ignored-ony-if-dir/ignored-by-ignored-dir\n+test_expect_missing\tarchive/ignored-without-slash/ &&\n+test_expect_missing\tarchive/ignored-without-slash/foo &&\n test_expect_exists\tarchive/one-level-lower/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-only-if-dir/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-ony-if-dir/ignored-by-ignored-dir\n-- 8<--\n"},{"id":"212290","messageId":"7v38viurgz.fsf@alter.siamese.dyndns.org","threadId":"33230","inReplyTo":"1364191510-8900-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 0/4] attr directory matching regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-26T15:10:36Z","receivedAt":"2013-03-26T15:10:36Z","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> I think the fix is something like this. There is still one thing I'd\n> like to do: make this code not rely on NUL for terminating the\n> patterns. That should remove the ugly \"p[len] = '\\0'\" in\n> prepare_attr_stack() 4/4 and and the reallocation in add_exclude() (in\n> current code). But let's deal with the regression first.\n\nAs a regression fix, we would need a fix that applies to\nmaint-1.8.1, not 'next'.\n\n>\n> Nguyễn Thái Ngọc Duy (4):\n> wildmatch: do not require \"text\" to be NUL-terminated\n> attr.c: fix pattern{,len} inconsistency in struct match_attr\n> dir.c: make match_{base,path}name respect {basename,path}len\n> attr.c: fix matching \"subdir\" without the trailing slash\n>\n> attr.c | 11 ++++++++++-\n> dir.c | 13 ++++++++-----\n> dir.h | 2 +-\n> t/t5002-archive-attr-pattern.sh | 6 ++++++\n> wildmatch.c | 43 ++++++++++++++++++++++++-----------------\n> wildmatch.h | 11 +++++++++--\n> 6 files changed, 59 insertions(+), 27 deletions(-)\n"},{"id":"212304","messageId":"1364323171-20299-1-git-send-email-gitster@pobox.com","threadId":"33230","inReplyTo":"20130323083927.GA25600@sigill.intra.peff.net","subject":"[PATCH 0/4] attribute regression fix for maint-1.8.1 and upward","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-26T18:39:27Z","receivedAt":"2013-03-26T18:39:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"So here is an attempt to fix the unintended regression, on top of\n9db9eecfe5c2 (attr: avoid calling find_basename() twice per path,\n2013-01-16). It consists of four patches.\n\nThe first patch is not essential to the fix, but I think it\nclarifies what is going on in this codepath.\n\nThe second patch addresses the issue Jeff noticed; it appears as if\nmatch_basename() takes counted strings, but one of the strings was\nnot a counted string at all. Its length was given to the function\nbecause the caller already had one, so that we do not have to do\nstrlen() ourselves. And the other one was meant to be a counted\nstring, but the callee was not using it as such. The patch makes\nthem both counted strings and treat them as such.\n\nThe third patch is the main fix. As I said in the log message, I\ndidn't look at it very carefully, so extra sets of eyeballs are very\nmuch appreciated.\n\nThe last one is a test stolen from Jeff to seal the series. It\nneeds sign-off from Jeff.\n\nJeff King (1):\n make sure a pattern without trailing slash matches a directory\n\nJunio C Hamano (3):\n attr.c::path_matches(): the basename is part of the pathname\n dir.c::match_basename(): pay attention to the length of string\n parameters\n attr.c::path_matches(): special case paths that end with a slash\n\n attr.c | 23 ++++++++++++-----------\n dir.c | 31 +++++++++++++++++++++++++++----\n t/t5002-archive-attr-pattern.sh | 6 ++++++\n 3 files changed, 45 insertions(+), 15 deletions(-)\n\n-- \n1.8.2-350-g3df87a1\n"},{"id":"212305","messageId":"1364323171-20299-2-git-send-email-gitster@pobox.com","threadId":"33230","inReplyTo":"1364323171-20299-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/4] attr.c::path_matches(): the basename is part of the pathname","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-26T18:39:28Z","receivedAt":"2013-03-26T18:39:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The function takes two strings (pathname and basename) as if they\nare independent strings, but in reality, the latter is always\npointing into a substring in the former.\n\nClarify this relationship by expressing the latter as an offset into\nthe former.\n\nSigned-off-by: Junio C Hamano \n---\n attr.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex ab2aab2..4cfe0ee 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -655,7 +655,7 @@ static void prepare_attr_stack(const char *path, int dirlen)\n }\n \n static int path_matches(const char *pathname, int pathlen,\n-\t\t\tconst char *basename,\n+\t\t\tint basename_offset,\n \t\t\tconst struct pattern *pat,\n \t\t\tconst char *base, int baselen)\n {\n@@ -667,8 +667,8 @@ static int path_matches(const char *pathname, int pathlen,\n \t\treturn 0;\n \n \tif (pat->flags & EXC_FLAG_NODIR) {\n-\t\treturn match_basename(basename,\n-\t\t\t\t pathlen - (basename - pathname),\n+\t\treturn match_basename(pathname + basename_offset,\n+\t\t\t\t pathlen - basename_offset,\n \t\t\t\t pattern, prefix,\n \t\t\t\t pat->patternlen, pat->flags);\n \t}\n@@ -701,7 +701,7 @@ static int fill_one(const char *what, struct match_attr *a, int rem)\n \treturn rem;\n }\n \n-static int fill(const char *path, int pathlen, const char *basename,\n+static int fill(const char *path, int pathlen, int basename_offset,\n \t\tstruct attr_stack *stk, int rem)\n {\n \tint i;\n@@ -711,7 +711,7 @@ static int fill(const char *path, int pathlen, const char *basename,\n \t\tstruct match_attr *a = stk->attrs[i];\n \t\tif (a->is_macro)\n \t\t\tcontinue;\n-\t\tif (path_matches(path, pathlen, basename,\n+\t\tif (path_matches(path, pathlen, basename_offset,\n \t\t\t\t &a->u.pat, base, stk->originlen))\n \t\t\trem = fill_one(\"fill\", a, rem);\n \t}\n@@ -750,7 +750,8 @@ static void collect_all_attrs(const char *path)\n {\n \tstruct attr_stack *stk;\n \tint i, pathlen, rem, dirlen;\n-\tconst char *basename, *cp, *last_slash = NULL;\n+\tconst char *cp, *last_slash = NULL;\n+\tint basename_offset;\n \n \tfor (cp = path; *cp; cp++) {\n \t\tif (*cp == '/' && cp[1])\n@@ -758,10 +759,10 @@ static void collect_all_attrs(const char *path)\n \t}\n \tpathlen = cp - path;\n \tif (last_slash) {\n-\t\tbasename = last_slash + 1;\n+\t\tbasename_offset = last_slash + 1 - path;\n \t\tdirlen = last_slash - path;\n \t} else {\n-\t\tbasename = path;\n+\t\tbasename_offset = 0;\n \t\tdirlen = 0;\n \t}\n \n@@ -771,7 +772,7 @@ static void collect_all_attrs(const char *path)\n \n \trem = attr_nr;\n \tfor (stk = attr_stack; 0 < rem && stk; stk = stk->prev)\n-\t\trem = fill(path, pathlen, basename, stk, rem);\n+\t\trem = fill(path, pathlen, basename_offset, stk, rem);\n }\n \n int git_check_attr(const char *path, int num, struct git_attr_check *check)\n-- \n1.8.2-350-g3df87a1\n"},{"id":"212307","messageId":"1364323171-20299-3-git-send-email-gitster@pobox.com","threadId":"33230","inReplyTo":"1364323171-20299-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/4] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-26T18:39:29Z","receivedAt":"2013-03-26T18:39:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The function takes two counted strings ( and\n) as parameters, together with prefix (the\nlength of the prefix in pattern that is to be matched literally\nwithout globbing against the basename) and EXC_* flags that tells it\nhow to match the pattern against the basename.\n\nHowever, it did not pay attention to the length of these counted\nstrings. Update them to do the following:\n\n * When the entire pattern is to be matched literally, the pattern\n matches the basename only when the lengths of them are the same,\n and they match up to that length.\n\n * When the pattern is \"*\" followed by a string to be matched\n literally, make sure that the basenamelen is equal or longer than\n the \"literal\" part of the pattern, and the tail of the basename\n string matches that literal part.\n\n * Otherwise, make sure we use only the counted part of the strings\n when calling fnmatch_icase(). Because these counted strings are\n full strings most of the time, avoid unnecessary allocation.\n\nSigned-off-by: Junio C Hamano \n---\n dir.c | 31 +++++++++++++++++++++++++++----\n 1 file changed, 27 insertions(+), 4 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 5a83aa7..aa16303 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -537,15 +537,38 @@ int match_basename(const char *basename, int basenamelen,\n \t\t int flags)\n {\n \tif (prefix == patternlen) {\n-\t\tif (!strcmp_icase(pattern, basename))\n+\t\tif (patternlen == basenamelen &&\n+\t\t !strncmp_icase(pattern, basename, basenamelen))\n \t\t\treturn 1;\n \t} else if (flags & EXC_FLAG_ENDSWITH) {\n+\t\t/* \"*literal\" matching against \"fooliteral\" */\n \t\tif (patternlen - 1 <= basenamelen &&\n-\t\t !strcmp_icase(pattern + 1,\n-\t\t\t\t basename + basenamelen - patternlen + 1))\n+\t\t !strncmp_icase(pattern + 1,\n+\t\t\t\t basename + basenamelen - (patternlen - 1),\n+\t\t\t\t patternlen - 1))\n \t\t\treturn 1;\n \t} else {\n-\t\tif (fnmatch_icase(pattern, basename, 0) == 0)\n+\t\tint match_status;\n+\t\tstruct strbuf pat = STRBUF_INIT;\n+\t\tstruct strbuf base = STRBUF_INIT;\n+\t\tconst char *use_pat = pattern;\n+\t\tconst char *use_base = basename;\n+\n+\t\tif (pattern[patternlen]) {\n+\t\t\tstrbuf_add(&pat, pattern, patternlen);\n+\t\t\tuse_pat = pat.buf;\n+\t\t}\n+\t\tif (basename[basenamelen]) {\n+\t\t\tstrbuf_add(&base, basename, basenamelen);\n+\t\t\tuse_base = base.buf;\n+\t\t}\n+\t\tmatch_status = fnmatch_icase(use_pat, use_base, 0);\n+\t\tif (use_pat)\n+\t\t\tstrbuf_release(&pat);\n+\t\tif (use_base)\n+\t\t\tstrbuf_release(&base);\n+\n+\t\tif (match_status == 0)\n \t\t\treturn 1;\n \t}\n \treturn 0;\n-- \n1.8.2-350-g3df87a1\n"},{"id":"212306","messageId":"1364323171-20299-4-git-send-email-gitster@pobox.com","threadId":"33230","inReplyTo":"1364323171-20299-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/4] attr.c::path_matches(): special case paths that end with a slash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-26T18:39:30Z","receivedAt":"2013-03-26T18:39:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The function is given a string that ends with a slash to signal that\nthe path is a directory to make sure that a pattern that ends with a\nslash (i.e. MUSTBEDIR) can tell directories and non-directories\napart. However, the pattern itself (pat->pattern and\npat->patternlen) that came from such a MUSTBEDIR pattern is\nrepresented as a string that ends with a slash, but patternlen does\nnot count that trailing slash. A MUSTBEDIR pattern \"element/\" is\nrepresented as a counted string <\"element/\", 7> and this must match\nmatch pathname \"element/\".\n\nBecause match_basename() wants to see pathname \"element\" to match\nagainst the pattern <\"element/\", 7>, reduce the length of the path\nto exclude the trailing slash when calling match_basename().\n\nA similar adjustment for match_pathname() might be needed, but I\ndidn't look into it.\n\nSigned-off-by: Junio C Hamano \n---\n attr.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 4cfe0ee..00a0016 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -661,14 +661,14 @@ static int path_matches(const char *pathname, int pathlen,\n {\n \tconst char *pattern = pat->pattern;\n \tint prefix = pat->nowildcardlen;\n+\tint isdir = (pathlen && pathname[pathlen - 1] == '/');\n \n-\tif ((pat->flags & EXC_FLAG_MUSTBEDIR) &&\n-\t ((!pathlen) || (pathname[pathlen-1] != '/')))\n+\tif ((pat->flags & EXC_FLAG_MUSTBEDIR) && !isdir)\n \t\treturn 0;\n \n \tif (pat->flags & EXC_FLAG_NODIR) {\n \t\treturn match_basename(pathname + basename_offset,\n-\t\t\t\t pathlen - basename_offset,\n+\t\t\t\t pathlen - basename_offset - isdir,\n \t\t\t\t pattern, prefix,\n \t\t\t\t pat->patternlen, pat->flags);\n \t}\n-- \n1.8.2-350-g3df87a1\n"},{"id":"212308","messageId":"1364323171-20299-5-git-send-email-gitster@pobox.com","threadId":"33230","inReplyTo":"1364323171-20299-1-git-send-email-gitster@pobox.com","subject":"[PATCH 4/4] make sure a pattern without trailing slash matches a directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-26T18:39:31Z","receivedAt":"2013-03-26T18:39:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jeff King \n\nPrior to v1.8.1.1, with:\n\n git init\n echo content >foo &&\n mkdir subdir &&\n echo content >subdir/bar &&\n echo \"subdir export-ignore\" >.gitattributes\n git add . &&\n git commit -m one &&\n git archive HEAD | tar tf -\n\nthe resulting archive would contain only \"foo\" and \".gitattributes\",\nnot subdir. This was broken with a recent change that intended to\nallow \"subdir/ export-ignore\" to also exclude the directory, but\ninstead ended up _requiring_ the trailing slash by mistake.\n\nA pattern \"subdir\" should match any path \"subdir\", whether it is a\ndirectory or a non-diretory. A pattern \"subdir/\" insists that a\npath \"subdir\" must be a directory for it to match.\n\nSigned-off-by: Junio C Hamano \n---\n t/t5002-archive-attr-pattern.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh\nindex 0c847fb..3be809c 100755\n--- a/t/t5002-archive-attr-pattern.sh\n+++ b/t/t5002-archive-attr-pattern.sh\n@@ -27,6 +27,10 @@ test_expect_success 'setup' '\n \techo ignored-only-if-dir/ export-ignore >>.git/info/attributes &&\n \tgit add ignored-only-if-dir &&\n \n+\tmkdir -p ignored-without-slash &&\n+\techo ignored without slash >ignored-without-slash/foo &&\n+\tgit add ignored-without-slash/foo &&\n+\techo ignored-without-slash export-ignore >>.git/info/attributes &&\n \n \tmkdir -p one-level-lower/two-levels-lower/ignored-only-if-dir &&\n \techo ignored by ignored dir >one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir &&\n@@ -49,6 +53,8 @@ test_expect_exists\tarchive/not-ignored-dir/ignored-only-if-dir\n test_expect_exists\tarchive/not-ignored-dir/\n test_expect_missing\tarchive/ignored-only-if-dir/\n test_expect_missing\tarchive/ignored-ony-if-dir/ignored-by-ignored-dir\n+test_expect_missing archive/ignored-without-slash/ &&\n+test_expect_missing archive/ignored-without-slash/foo &&\n test_expect_exists\tarchive/one-level-lower/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-only-if-dir/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-ony-if-dir/ignored-by-ignored-dir\n-- \n1.8.2-350-g3df87a1\n"},{"id":"212309","messageId":"20130326184921.GA26462@sigill.intra.peff.net","threadId":"33230","inReplyTo":"1364323171-20299-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 1/4] attr.c::path_matches(): the basename is part of the pathname","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-26T18:49:22Z","receivedAt":"2013-03-26T18:49:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 11:39:28AM -0700, Junio C Hamano wrote:\n\n> The function takes two strings (pathname and basename) as if they\n> are independent strings, but in reality, the latter is always\n> pointing into a substring in the former.\n> \n> Clarify this relationship by expressing the latter as an offset into\n> the former.\n> \n> Signed-off-by: Junio C Hamano \n\nThis is a huge improvement in maintainability. My initial fix attempt\nwas to just xstrdup() the strings (knowing that the performance would be\nhorrible, but I was still investigating correctness issues at that\npoint). And of course I ran into this same issue as I tried to make a\ncopy of pathname.\n\nSo even without the rest of the fix, this is definitely a good idea. :)\n\n-Peff\n"},{"id":"212310","messageId":"20130326185559.GB26462@sigill.intra.peff.net","threadId":"33230","inReplyTo":"1364323171-20299-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 2/4] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-26T18:55:59Z","receivedAt":"2013-03-26T18:55:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 11:39:29AM -0700, Junio C Hamano wrote:\n\n> The function takes two counted strings ( and\n> ) as parameters, together with prefix (the\n> length of the prefix in pattern that is to be matched literally\n> without globbing against the basename) and EXC_* flags that tells it\n> how to match the pattern against the basename.\n> \n> However, it did not pay attention to the length of these counted\n> strings. Update them to do the following:\n> \n> * When the entire pattern is to be matched literally, the pattern\n> matches the basename only when the lengths of them are the same,\n> and they match up to that length.\n> \n> * When the pattern is \"*\" followed by a string to be matched\n> literally, make sure that the basenamelen is equal or longer than\n> the \"literal\" part of the pattern, and the tail of the basename\n> string matches that literal part.\n> \n> * Otherwise, make sure we use only the counted part of the strings\n> when calling fnmatch_icase(). Because these counted strings are\n> full strings most of the time, avoid unnecessary allocation.\n\nI think this is OK, with the intention that we would eventually drop the\nallocations from your third bullet point in favor of using a\nbyte-counted version of fnmatch (i.e., nwildmatch). But until then we're\ngoing to see a performance drop.\n\nThe pattern is usually going to be NUL-terminated at the length counter,\nbut every time we feed a directory, it's going to run into this\nallocation. And we do it once for _every_ directory against _every_\nwildcard gitignore pattern. So I think it is probably going to be\nmeasurable. I guess we can try measuring it on something like WebKit,\nwhich has plenty of both directories and gitattributes.\n\n-Peff\n"},{"id":"212311","messageId":"20130326190558.GC26462@sigill.intra.peff.net","threadId":"33230","inReplyTo":"1364323171-20299-4-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 3/4] attr.c::path_matches(): special case paths that end with a slash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-26T19:05:58Z","receivedAt":"2013-03-26T19:05:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 11:39:30AM -0700, Junio C Hamano wrote:\n\n> A similar adjustment for match_pathname() might be needed, but I\n> didn't look into it.\n\nI notice that match_pathname takes _two_ lengths for the pattern: the\nnowildcardlen (called \"prefix\", and the full patternlen). But the first\nthing it does is:\n\n if (*pattern == '/') {\n pattern++;\n prefix--;\n }\n\nwhich seems obviously wrong, as patternlen should be dropped, too. But\nwe do not seem to look at patternlen at all! I think we can drop the\nparameter totally.\n\nWe do seem to use strncmp_icase through the rest of the function,\nthough, which should be OK. The one exception is that we call fnmatch at\nthe end. Should the allocation hack from the previous patch make its way\ninto an \"fnmatch_icase_bytes()\" function, so we can use it here, too?\nAnd then when we have a more efficient solution, we can just plug it in\nthere.\n\n-Peff\n"},{"id":"212312","messageId":"20130326190801.GD26462@sigill.intra.peff.net","threadId":"33230","inReplyTo":"1364323171-20299-5-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 4/4] make sure a pattern without trailing slash matches a directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-26T19:08:01Z","receivedAt":"2013-03-26T19:08:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 11:39:31AM -0700, Junio C Hamano wrote:\n\n> From: Jeff King \n> \n> Prior to v1.8.1.1, with:\n> \n> git init\n> echo content >foo &&\n> mkdir subdir &&\n> echo content >subdir/bar &&\n> echo \"subdir export-ignore\" >.gitattributes\n> git add . &&\n> git commit -m one &&\n> git archive HEAD | tar tf -\n> \n> the resulting archive would contain only \"foo\" and \".gitattributes\",\n> not subdir. This was broken with a recent change that intended to\n> allow \"subdir/ export-ignore\" to also exclude the directory, but\n> instead ended up _requiring_ the trailing slash by mistake.\n\nYeah, I think that is fine. I'd just squash this test and description\ninto the previous patch, though (I do not care about dropping my commit\ncount by 1).\n\nAnd of course,\n\nSigned-off-by: Jeff King \n\n> A pattern \"subdir\" should match any path \"subdir\", whether it is a\n> directory or a non-diretory. A pattern \"subdir/\" insists that a\n> path \"subdir\" must be a directory for it to match.\n\ns/diretory/directory/\n\n-Peff\n"},{"id":"212330","messageId":"20130326203914.GA29167@sigill.intra.peff.net","threadId":"33230","inReplyTo":"20130326185559.GB26462@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-26T20:39:14Z","receivedAt":"2013-03-26T20:39:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 02:55:59PM -0400, Jeff King wrote:\n\n> > * Otherwise, make sure we use only the counted part of the strings\n> > when calling fnmatch_icase(). Because these counted strings are\n> > full strings most of the time, avoid unnecessary allocation.\n> \n> I think this is OK, with the intention that we would eventually drop the\n> allocations from your third bullet point in favor of using a\n> byte-counted version of fnmatch (i.e., nwildmatch). But until then we're\n> going to see a performance drop.\n> \n> The pattern is usually going to be NUL-terminated at the length counter,\n> but every time we feed a directory, it's going to run into this\n> allocation. And we do it once for _every_ directory against _every_\n> wildcard gitignore pattern. So I think it is probably going to be\n> measurable. I guess we can try measuring it on something like WebKit,\n> which has plenty of both directories and gitattributes.\n\nI timed this doing \"git archive HEAD\" on webkit.git before and after. It\nactually ended up not mattering much (I think because it is only the\ndirectories which are affected, not each individually path, so it's a\nmuch smaller number than you'd think). The best-of-five timing was\nslightly slower, but was within the noise.\n\nSo I do still think it would make sense to go to a byte-limited version\nof fnmatch eventually, just for code cleanliness and predictability of\nperformance, but this is really not a bad solution in the interim.\n\n-Peff\n"},{"id":"212332","messageId":"7v7gktubsp.fsf@alter.siamese.dyndns.org","threadId":"33230","inReplyTo":"20130326203914.GA29167@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-26T20:49:10Z","receivedAt":"2013-03-26T20:49:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I timed this doing \"git archive HEAD\" on webkit.git before and after. It\n> actually ended up not mattering much (I think because it is only the\n> directories which are affected, not each individually path, so it's a\n> much smaller number than you'd think). The best-of-five timing was\n> slightly slower, but was within the noise.\n\nInteresting. Because \"archive\" has to incur a large I/O cost\nanyway, I expected extra allocation for correctness for only the\ndirectory paths would be dwarfed in the noise.\n\nI actually care more about cases other than \"archive\", though. Do\nwe even feed directory paths to the machinery?\n\n> So I do still think it would make sense to go to a byte-limited version\n> of fnmatch eventually, just for code cleanliness and predictability of\n> performance, but this is really not a bad solution in the interim.\n\nYes, what we do with wildmatch is a separate issue for 'master' and\nupwards.\n"},{"id":"212338","messageId":"20130326212948.GA1340@sigill.intra.peff.net","threadId":"33230","inReplyTo":"7v7gktubsp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/4] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-26T21:29:48Z","receivedAt":"2013-03-26T21:29:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 01:49:10PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I timed this doing \"git archive HEAD\" on webkit.git before and after. It\n> > actually ended up not mattering much (I think because it is only the\n> > directories which are affected, not each individually path, so it's a\n> > much smaller number than you'd think). The best-of-five timing was\n> > slightly slower, but was within the noise.\n> \n> Interesting. Because \"archive\" has to incur a large I/O cost\n> anyway, I expected extra allocation for correctness for only the\n> directory paths would be dwarfed in the noise.\n> \n> I actually care more about cases other than \"archive\", though. Do\n> we even feed directory paths to the machinery?\n\nIn general, no, I don't think so. That's why I tested \"archive\", since I\nknew it did. In the normal case, we should just feed file paths, meaning\nwe only run into this code path when somebody has \"foo/\" in their\npattern. Testing like:\n\n git ls-files -z >files\n time git check-attr --stdin -z -a /dev/null\n\nshowed a difference well within the noise.\n\n> > So I do still think it would make sense to go to a byte-limited version\n> > of fnmatch eventually, just for code cleanliness and predictability of\n> > performance, but this is really not a bad solution in the interim.\n> \n> Yes, what we do with wildmatch is a separate issue for 'master' and\n> upwards.\n\nOh, agreed. I just wanted to see how much performance would be impacted\nfor the interim. But it seems that it's not.\n\nSo I think your series is the right direction, but we would want to\nfactor out the allocation code and use it from match_pathname, as well.\n\n-Peff\n"},{"id":"212340","messageId":"20130326213319.GA3058@sigill.intra.peff.net","threadId":"33230","inReplyTo":"20130326190558.GC26462@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] attr.c::path_matches(): special case paths that end with a slash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-26T21:33:19Z","receivedAt":"2013-03-26T21:33:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 03:05:58PM -0400, Jeff King wrote:\n\n> On Tue, Mar 26, 2013 at 11:39:30AM -0700, Junio C Hamano wrote:\n> \n> > A similar adjustment for match_pathname() might be needed, but I\n> > didn't look into it.\n> \n> I notice that match_pathname takes _two_ lengths for the pattern: the\n> nowildcardlen (called \"prefix\", and the full patternlen). But the first\n> thing it does is:\n> \n> if (*pattern == '/') {\n> pattern++;\n> prefix--;\n> }\n> \n> which seems obviously wrong, as patternlen should be dropped, too. But\n> we do not seem to look at patternlen at all! I think we can drop the\n> parameter totally.\n> \n> We do seem to use strncmp_icase through the rest of the function,\n> though, which should be OK. The one exception is that we call fnmatch at\n> the end. Should the allocation hack from the previous patch make its way\n> into an \"fnmatch_icase_bytes()\" function, so we can use it here, too?\n> And then when we have a more efficient solution, we can just plug it in\n> there.\n\nHmm. match_pathname does have this:\n\n /*\n * baselen does not count the trailing slash. base[] may or\n * may not end with a trailing slash though.\n */\n if (pathlen < baselen + 1 ||\n (baselen && pathname[baselen] != '/') ||\n strncmp_icase(pathname, base, baselen))\n return 0;\n\nwhich seems to imply that the trailing slash is important here, and that\nwe should not drop it when passing the path to match_pathname. I'm\nstill trying to figure out exactly what it is that the extra slash check\nis for, and whether it might not have the same problem.\n\n-Peff\n"},{"id":"212353","messageId":"7vd2ulsse3.fsf@alter.siamese.dyndns.org","threadId":"33230","inReplyTo":"20130326212948.GA1340@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-26T22:33:40Z","receivedAt":"2013-03-26T22:33:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I think your series is the right direction, but we would want to\n> factor out the allocation code and use it from match_pathname, as well.\n\nI am deep into today's integration cycle, so perhaps in the meantime\nyou can help with a follow-up patch ;-)?\n"},{"id":"212357","messageId":"20130327010429.GB18405@sigill.intra.peff.net","threadId":"33230","inReplyTo":"7vd2ulsse3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/4] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-27T01:04:29Z","receivedAt":"2013-03-27T01:04:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 03:33:40PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So I think your series is the right direction, but we would want to\n> > factor out the allocation code and use it from match_pathname, as well.\n> \n> I am deep into today's integration cycle, so perhaps in the meantime\n> you can help with a follow-up patch ;-)?\n\nYour afternoon integration is my dinner-time. :)\n\nI'll try to look at it tomorrow.\n\n-Peff\n"},{"id":"212358","messageId":"20130327011309.GA10946@duynguyen-vnpc.dek-tpc.internal","threadId":"33230","inReplyTo":"1364323171-20299-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 0/4] attribute regression fix for maint-1.8.1 and upward","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-27T01:13:09Z","receivedAt":"2013-03-27T01:13:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"qOn Tue, Mar 26, 2013 at 11:39:27AM -0700, Junio C Hamano wrote:\n> So here is an attempt to fix the unintended regression, on top of\n> 9db9eecfe5c2 (attr: avoid calling find_basename() twice per path,\n> 2013-01-16). It consists of four patches.\n\nNot that I disagree with this. Just wanted to see how far the \"dtype\"\nidea went. How about this? git_check_attr() now takes dtype as an\nargument and the caller must not add the trailing slash. This could be\nsplit into two patches, one for git_check_attr prototype change, and\nthe other the real meat.\n\n-- 8< --\ndiff --git a/archive.c b/archive.c\nindex 93e00bb..ab811b3 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -112,7 +112,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n \twrite_archive_entry_fn_t write_entry = c->write_entry;\n \tstruct git_attr_check check[2];\n \tconst char *path_without_prefix;\n-\tint err;\n+\tint err, dtype;\n \n \targs->convert = 0;\n \tstrbuf_reset(&path);\n@@ -120,18 +120,18 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n \tstrbuf_add(&path, args->base, args->baselen);\n \tstrbuf_add(&path, base, baselen);\n \tstrbuf_addstr(&path, filename);\n-\tif (S_ISDIR(mode) || S_ISGITLINK(mode))\n-\t\tstrbuf_addch(&path, '/');\n+\tdtype = S_ISDIR(mode) || S_ISGITLINK(mode) ? DT_DIR : DT_REG;\n \tpath_without_prefix = path.buf + args->baselen;\n \n \tsetup_archive_check(check);\n-\tif (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {\n+\tif (!git_check_attr(path_without_prefix, dtype, ARRAY_SIZE(check), check)) {\n \t\tif (ATTR_TRUE(check[0].value))\n \t\t\treturn 0;\n \t\targs->convert = ATTR_TRUE(check[1].value);\n \t}\n \n \tif (S_ISDIR(mode) || S_ISGITLINK(mode)) {\n+\t\tstrbuf_addch(&path, '/');\n \t\tif (args->verbose)\n \t\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n \t\terr = write_entry(args, sha1, path.buf, path.len, mode);\ndiff --git a/attr.c b/attr.c\nindex e2f9377..ab6422c 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -255,6 +255,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\t\t\t &res->u.pat.patternlen,\n \t\t\t\t &res->u.pat.flags,\n \t\t\t\t &res->u.pat.nowildcardlen);\n+\t\tif (res->u.pat.flags & EXC_FLAG_MUSTBEDIR)\n+\t\t\tp[res->u.pat.patternlen] = '\\0';\n \t\tif (res->u.pat.flags & EXC_FLAG_NEGATIVE) {\n \t\t\twarning(_(\"Negative patterns are ignored in git attributes\\n\"\n \t\t\t\t \"Use '\\\\!' for literal leading exclamation.\"));\n@@ -657,15 +659,14 @@ static void prepare_attr_stack(const char *path, int dirlen)\n }\n \n static int path_matches(const char *pathname, int pathlen,\n-\t\t\tconst char *basename,\n+\t\t\tconst char *basename, int dtype,\n \t\t\tconst struct pattern *pat,\n \t\t\tconst char *base, int baselen)\n {\n \tconst char *pattern = pat->pattern;\n \tint prefix = pat->nowildcardlen;\n \n-\tif ((pat->flags & EXC_FLAG_MUSTBEDIR) &&\n-\t ((!pathlen) || (pathname[pathlen-1] != '/')))\n+\tif ((pat->flags & EXC_FLAG_MUSTBEDIR) && dtype != DT_DIR)\n \t\treturn 0;\n \n \tif (pat->flags & EXC_FLAG_NODIR) {\n@@ -704,7 +705,7 @@ static int fill_one(const char *what, struct match_attr *a, int rem)\n }\n \n static int fill(const char *path, int pathlen, const char *basename,\n-\t\tstruct attr_stack *stk, int rem)\n+\t\tint dtype, struct attr_stack *stk, int rem)\n {\n \tint i;\n \tconst char *base = stk->origin ? stk->origin : \"\";\n@@ -713,7 +714,7 @@ static int fill(const char *path, int pathlen, const char *basename,\n \t\tstruct match_attr *a = stk->attrs[i];\n \t\tif (a->is_macro)\n \t\t\tcontinue;\n-\t\tif (path_matches(path, pathlen, basename,\n+\t\tif (path_matches(path, pathlen, basename, dtype,\n \t\t\t\t &a->u.pat, base, stk->originlen))\n \t\t\trem = fill_one(\"fill\", a, rem);\n \t}\n@@ -748,20 +749,17 @@ static int macroexpand_one(int attr_nr, int rem)\n * Collect all attributes for path into the array pointed to by\n * check_all_attr.\n */\n-static void collect_all_attrs(const char *path)\n+static void collect_all_attrs(const char *path, int dtype)\n {\n \tstruct attr_stack *stk;\n \tint i, pathlen, rem, dirlen;\n-\tconst char *basename, *cp, *last_slash = NULL;\n+\tconst char *basename, *cp;\n \n-\tfor (cp = path; *cp; cp++) {\n-\t\tif (*cp == '/' && cp[1])\n-\t\t\tlast_slash = cp;\n-\t}\n-\tpathlen = cp - path;\n-\tif (last_slash) {\n-\t\tbasename = last_slash + 1;\n-\t\tdirlen = last_slash - path;\n+\tcp = strrchr(path, '/');\n+\tpathlen = strlen(path);\n+\tif (cp) {\n+\t\tbasename = cp + 1;\n+\t\tdirlen = cp - path;\n \t} else {\n \t\tbasename = path;\n \t\tdirlen = 0;\n@@ -773,14 +771,14 @@ static void collect_all_attrs(const char *path)\n \n \trem = attr_nr;\n \tfor (stk = attr_stack; 0 < rem && stk; stk = stk->prev)\n-\t\trem = fill(path, pathlen, basename, stk, rem);\n+\t\trem = fill(path, pathlen, basename, dtype, stk, rem);\n }\n \n-int git_check_attr(const char *path, int num, struct git_attr_check *check)\n+int git_check_attr(const char *path, int dtype, int num, struct git_attr_check *check)\n {\n \tint i;\n \n-\tcollect_all_attrs(path);\n+\tcollect_all_attrs(path, dtype);\n \n \tfor (i = 0; i < num; i++) {\n \t\tconst char *value = check_all_attr[check[i].attr->attr_nr].value;\n@@ -796,7 +794,7 @@ int git_all_attrs(const char *path, int *num, struct git_attr_check **check)\n {\n \tint i, count, j;\n \n-\tcollect_all_attrs(path);\n+\tcollect_all_attrs(path, DT_REG);\n \n \t/* Count the number of attributes that are set. */\n \tcount = 0;\ndiff --git a/attr.h b/attr.h\nindex 8b08d33..ce39c9c 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -36,7 +36,7 @@ struct git_attr_check {\n */\n char *git_attr_name(struct git_attr *);\n \n-int git_check_attr(const char *path, int, struct git_attr_check *);\n+int git_check_attr(const char *path, int, int, struct git_attr_check *);\n \n /*\n * Retrieve all attributes that apply to the specified path. *num\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex 075d01d..261e57d 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -49,7 +49,7 @@ static void check_attr(const char *prefix, int cnt,\n \tchar *full_path =\n \t\tprefix_path(prefix, prefix ? strlen(prefix) : 0, file);\n \tif (check != NULL) {\n-\t\tif (git_check_attr(full_path, cnt, check))\n+\t\tif (git_check_attr(full_path, DT_REG, cnt, check))\n \t\t\tdie(\"git_check_attr died\");\n \t\toutput_attr(cnt, check, file);\n \t} else {\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f069462..7a77288 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -894,7 +894,7 @@ static int no_try_delta(const char *path)\n \tstruct git_attr_check check[1];\n \n \tsetup_delta_attr_check(check);\n-\tif (git_check_attr(path, ARRAY_SIZE(check), check))\n+\tif (git_check_attr(path, DT_REG, ARRAY_SIZE(check), check))\n \t\treturn 0;\n \tif (ATTR_FALSE(check->value))\n \t\treturn 1;\ndiff --git a/convert.c b/convert.c\nindex 3520252..3f09cbb 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -755,7 +755,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t\tgit_config(read_convert_config, NULL);\n \t}\n \n-\tif (!git_check_attr(path, NUM_CONV_ATTRS, ccheck)) {\n+\tif (!git_check_attr(path, DT_REG, NUM_CONV_ATTRS, ccheck)) {\n \t\tca->crlf_action = git_path_check_crlf(path, ccheck + 4);\n \t\tif (ca->crlf_action == CRLF_GUESS)\n \t\t\tca->crlf_action = git_path_check_crlf(path, ccheck + 0);\ndiff --git a/ll-merge.c b/ll-merge.c\nindex fb61ea6..1944392 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -342,7 +342,7 @@ static int git_path_check_merge(const char *path, struct git_attr_check check[2]\n \t\tcheck[0].attr = git_attr(\"merge\");\n \t\tcheck[1].attr = git_attr(\"conflict-marker-size\");\n \t}\n-\treturn git_check_attr(path, 2, check);\n+\treturn git_check_attr(path, DT_REG, 2, check);\n }\n \n static void normalize_file(mmfile_t *mm, const char *path)\n@@ -399,7 +399,7 @@ int ll_merge_marker_size(const char *path)\n \n \tif (!check.attr)\n \t\tcheck.attr = git_attr(\"conflict-marker-size\");\n-\tif (!git_check_attr(path, 1, &check) && check.value) {\n+\tif (!git_check_attr(path, DT_REG, 1, &check) && check.value) {\n \t\tmarker_size = atoi(check.value);\n \t\tif (marker_size <= 0)\n \t\t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\ndiff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh\nindex 0c847fb..98ccc3c 100755\n--- a/t/t5002-archive-attr-pattern.sh\n+++ b/t/t5002-archive-attr-pattern.sh\n@@ -27,6 +27,10 @@ test_expect_success 'setup' '\n \techo ignored-only-if-dir/ export-ignore >>.git/info/attributes &&\n \tgit add ignored-only-if-dir &&\n \n+\tmkdir -p ignored-without-slash &&\n+\techo ignored without slash >ignored-without-slash/foo &&\n+\tgit add ignored-without-slash/foo &&\n+\techo ignored-without-slash export-ignore >>.git/info/attributes &&\n \n \tmkdir -p one-level-lower/two-levels-lower/ignored-only-if-dir &&\n \techo ignored by ignored dir >one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir &&\n@@ -49,6 +53,8 @@ test_expect_exists\tarchive/not-ignored-dir/ignored-only-if-dir\n test_expect_exists\tarchive/not-ignored-dir/\n test_expect_missing\tarchive/ignored-only-if-dir/\n test_expect_missing\tarchive/ignored-ony-if-dir/ignored-by-ignored-dir\n+test_expect_missing\tarchive/ignored-without-slash/ &&\n+test_expect_missing\tarchive/ignored-without-slash/foo &&\n test_expect_exists\tarchive/one-level-lower/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-only-if-dir/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-ony-if-dir/ignored-by-ignored-dir\ndiff --git a/userdiff.c b/userdiff.c\nindex ea43a03..fd4f576 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -260,7 +260,7 @@ struct userdiff_driver *userdiff_find_by_path(const char *path)\n \n \tif (!path)\n \t\treturn NULL;\n-\tif (git_check_attr(path, 1, &check))\n+\tif (git_check_attr(path, DT_REG, 1, &check))\n \t\treturn NULL;\n \n \tif (ATTR_TRUE(check.value))\ndiff --git a/ws.c b/ws.c\nindex b498d75..34c2145 100644\n--- a/ws.c\n+++ b/ws.c\n@@ -88,7 +88,7 @@ unsigned whitespace_rule(const char *pathname)\n \tstruct git_attr_check attr_whitespace_rule;\n \n \tsetup_whitespace_attr_check(&attr_whitespace_rule);\n-\tif (!git_check_attr(pathname, 1, &attr_whitespace_rule)) {\n+\tif (!git_check_attr(pathname, DT_REG, 1, &attr_whitespace_rule)) {\n \t\tconst char *value;\n \n \t\tvalue = attr_whitespace_rule.value;\n-- 8< --\n"},{"id":"212360","messageId":"CACsJy8A8QNaKUoMUYri6uUs56suerUZ5n48Q3VDD-=OcruBqww@mail.gmail.com","threadId":"33230","inReplyTo":"20130326213319.GA3058@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] attr.c::path_matches(): special case paths that end with a slash","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-27T01:30:22Z","receivedAt":"2013-03-27T01:30:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 27, 2013 at 4:33 AM, Jeff King wrote:\n> Hmm. match_pathname does have this:\n>\n> /*\n> * baselen does not count the trailing slash. base[] may or\n> * may not end with a trailing slash though.\n> */\n> if (pathlen < baselen + 1 ||\n> (baselen && pathname[baselen] != '/') ||\n> strncmp_icase(pathname, base, baselen))\n> return 0;\n>\n> which seems to imply that the trailing slash is important here, and that\n> we should not drop it when passing the path to match_pathname. I'm\n> still trying to figure out exactly what it is that the extra slash check\n> is for, and whether it might not have the same problem.\n\nThe \"may not end with a trailing slash\" can only happen when baselen\n== 0. And that rule is documented in code, at this line in\nlast_exclude_matching_from_list:\n\nassert(x->baselen == 0 || x->base[x->baselen - 1] == '/');\n-- \nDuy\n"},{"id":"212361","messageId":"CACsJy8B63yeC2jTqq9aYU9ajvnaNBey1viQkw=aDJqnTwGn+1g@mail.gmail.com","threadId":"33230","inReplyTo":"20130326184921.GA26462@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] attr.c::path_matches(): the basename is part of the pathname","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-27T01:40:16Z","receivedAt":"2013-03-27T01:40:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 27, 2013 at 1:49 AM, Jeff King wrote:\n> On Tue, Mar 26, 2013 at 11:39:28AM -0700, Junio C Hamano wrote:\n>\n>> The function takes two strings (pathname and basename) as if they\n>> are independent strings, but in reality, the latter is always\n>> pointing into a substring in the former.\n>>\n>> Clarify this relationship by expressing the latter as an offset into\n>> the former.\n>>\n>> Signed-off-by: Junio C Hamano \n>\n> This is a huge improvement in maintainability. My initial fix attempt\n> was to just xstrdup() the strings (knowing that the performance would be\n> horrible, but I was still investigating correctness issues at that\n> point). And of course I ran into this same issue as I tried to make a\n> copy of pathname.\n>\n> So even without the rest of the fix, this is definitely a good idea. :)\n\nmatch_{base,path}name and their exclude callers do the same thing. I\nguess I'm used to it and did not see the maintainability issue. Maybe\nwe should do the same there too.\n-- \nDuy\n"},{"id":"212364","messageId":"7vmwtpqyui.fsf@alter.siamese.dyndns.org","threadId":"33230","inReplyTo":"20130327011309.GA10946@duynguyen-vnpc.dek-tpc.internal","subject":"Re: [PATCH 0/4] attribute regression fix for maint-1.8.1 and upward","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-27T03:57:09Z","receivedAt":"2013-03-27T03:57:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> How about this? git_check_attr() now takes dtype as an argument\n> and the caller must not add the trailing slash. This could be\n> split into two patches, one for git_check_attr prototype change,\n> and the other the real meat.\n\n\"git check-attr\" fundamentally cannot know, but aside from that do\nall the callsites know if the path in question is a directory or\nnot? My impression was that there are some cases you do not\nnecessarily know.\n\n\"Add slash when you _know_ it is a directory, but otherwise pass the\npath without trailing slash.\" is easier to understand than \"Pass\n040000 if you know it is a directory, but otherwise pass 100644\",\nexactly because \"otherwise\" in both of these instructions include\nthe case where the path in question _is_ a directory (you just do\nnot know what it is).\n\nI do not particularly like the \"trailing slash on the basename\"\napproach, but it feels less bad than passing dtype down.\n"},{"id":"212365","messageId":"CACsJy8BtmyPvU1Zrhdm9mEWTwk9GBuMEs-8ycpBHM79dGyCrMw@mail.gmail.com","threadId":"33230","inReplyTo":"7vmwtpqyui.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/4] attribute regression fix for maint-1.8.1 and upward","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-27T04:01:14Z","receivedAt":"2013-03-27T04:01:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 27, 2013 at 10:57 AM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> How about this? git_check_attr() now takes dtype as an argument\n>> and the caller must not add the trailing slash. This could be\n>> split into two patches, one for git_check_attr prototype change,\n>> and the other the real meat.\n>\n> \"git check-attr\" fundamentally cannot know, but aside from that do\n> all the callsites know if the path in question is a directory or\n> not? My impression was that there are some cases you do not\n> necessarily know.\n>\n> \"Add slash when you _know_ it is a directory, but otherwise pass the\n> path without trailing slash.\" is easier to understand than \"Pass\n> 040000 if you know it is a directory, but otherwise pass 100644\",\n> exactly because \"otherwise\" in both of these instructions include\n> the case where the path in question _is_ a directory (you just do\n> not know what it is).\n>\n> I do not particularly like the \"trailing slash on the basename\"\n> approach, but it feels less bad than passing dtype down.\n\nFair enough. I'll rebase my changes on top of yours as long term\ncleanup. Maybe I can make nwildmatch take patternlen too.\n-- \nDuy\n"},{"id":"212574","messageId":"20130328194924.GA21095@sigill.intra.peff.net","threadId":"33230","inReplyTo":"20130326190558.GC26462@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] attr.c::path_matches(): special case paths that end with a slash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-28T19:49:25Z","receivedAt":"2013-03-28T19:49:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 03:05:58PM -0400, Jeff King wrote:\n\n> On Tue, Mar 26, 2013 at 11:39:30AM -0700, Junio C Hamano wrote:\n> \n> > A similar adjustment for match_pathname() might be needed, but I\n> > didn't look into it.\n> [...]\n> We do seem to use strncmp_icase through the rest of the function,\n> though, which should be OK. The one exception is that we call fnmatch at\n> the end. Should the allocation hack from the previous patch make its way\n> into an \"fnmatch_icase_bytes()\" function, so we can use it here, too?\n> And then when we have a more efficient solution, we can just plug it in\n> there.\n\nHmm, yeah, there is more going on here than just that. If I add the\ntests below, the first one (a wildcard) passes, because you fixed the\nfnmatch code path. But the deep/ ones do not, as they should be going\nthrough match_pathname. I expected the deep/with/wildcard one to fail\n(because of the fnmatch problem I mentioned above), but not the\ndeep/and/slashless one, which should be using strncmp. I'll see if I can\ntrack down the cause.\n\n-Peff\n\n---\ndiff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh\nindex 3be809c..234a615 100755\n--- a/t/t5002-archive-attr-pattern.sh\n+++ b/t/t5002-archive-attr-pattern.sh\n@@ -32,6 +32,21 @@ test_expect_success 'setup' '\n \tgit add ignored-without-slash/foo &&\n \techo ignored-without-slash export-ignore >>.git/info/attributes &&\n \n+\tmkdir -p wildcard-without-slash &&\n+\techo \"ignored without slash\" >wildcard-without-slash/foo &&\n+\tgit add wildcard-without-slash/foo &&\n+\techo \"wild*-without-slash export-ignore\" >>.git/info/attributes &&\n+\n+\tmkdir -p deep/and/slashless &&\n+\techo \"ignored without slash\" >deep/and/slashless/foo &&\n+\tgit add deep/and/slashless/foo &&\n+\techo deep/and/slashless export-ignore >>.git/info/attributes &&\n+\n+\tmkdir -p deep/with/wildcard &&\n+\techo \"ignored without slash\" >deep/with/wildcard/foo &&\n+\tgit add deep/with/wildcard/foo &&\n+\techo \"deep/*t*/wildcard export-ignore\" >>.git/info/attributes &&\n+\n \tmkdir -p one-level-lower/two-levels-lower/ignored-only-if-dir &&\n \techo ignored by ignored dir >one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir &&\n \tgit add one-level-lower &&\n@@ -55,6 +70,12 @@ test_expect_missing archive/ignored-without-slash/foo &&\n test_expect_missing\tarchive/ignored-ony-if-dir/ignored-by-ignored-dir\n test_expect_missing archive/ignored-without-slash/ &&\n test_expect_missing archive/ignored-without-slash/foo &&\n+test_expect_missing archive/wildcard-without-slash/\n+test_expect_missing archive/wildcard-without-slash/foo &&\n+test_expect_missing archive/deep/and/slashless/ &&\n+test_expect_missing archive/deep/and/slashless/foo &&\n+test_expect_missing archive/deep/with/wildcard/ &&\n+test_expect_missing archive/deep/with/wildcard/foo &&\n test_expect_exists\tarchive/one-level-lower/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-only-if-dir/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-ony-if-dir/ignored-by-ignored-dir\n"},{"id":"212582","messageId":"20130328214358.GA10685@sigill.intra.peff.net","threadId":"33230","inReplyTo":"1364323171-20299-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 0/6] attribute regression fix for maint-1.8.1 and upward","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-28T21:43:58Z","receivedAt":"2013-03-28T21:43:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 11:39:27AM -0700, Junio C Hamano wrote:\n\n> So here is an attempt to fix the unintended regression, on top of\n> 9db9eecfe5c2 (attr: avoid calling find_basename() twice per path,\n> 2013-01-16). It consists of four patches.\n\nHere's my update to the series. I think this should fix all of the\nissues. And it should be very easy to drop in Duy's nwildmatch later on;\nit can just replace the fnmatch_icase_mem function added in patch 2\nbelow.\n\nThe main fix in this iteration is that match_pathname receives the same\ntreatment as match_basename, which is done in patches 3 and 4 (the\nissues were subtly different enough that I didn't want to squash it all\ntogether; plus, gotta keep that commit count up).\n\n [1/6]: attr.c::path_matches(): the basename is part of the pathname\n [2/6]: dir.c::match_basename(): pay attention to the length of string parameters\n [3/6]: dir.c::match_pathname(): adjust patternlen when shifting pattern\n [4/6]: dir.c::match_pathname(): pay attention to the length of string parameters\n [5/6]: attr.c::path_matches(): special case paths that end with a slash\n [6/6]: t: check that a pattern without trailing slash matches a directory\n\n-Peff\n\nPS I followed your subject-naming convention since I was adding into\n your series, but it seems quite long to me. I would have just said:\n \"match_basename: pay attention...\".\n"},{"id":"212583","messageId":"20130328214500.GA10936@sigill.intra.peff.net","threadId":"33230","inReplyTo":"20130328214358.GA10685@sigill.intra.peff.net","subject":"[PATCH 1/6] attr.c::path_matches(): the basename is part of the pathname","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-28T21:45:00Z","receivedAt":"2013-03-28T21:45:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Junio C Hamano \n\nThe function takes two strings (pathname and basename) as if they\nare independent strings, but in reality, the latter is always\npointing into a substring in the former.\n\nClarify this relationship by expressing the latter as an offset into\nthe former.\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Jeff King \n---\nThis is identical to the original 1/4.\n\n attr.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex ab2aab2..4cfe0ee 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -655,7 +655,7 @@ static int path_matches(const char *pathname, int pathlen,\n }\n \n static int path_matches(const char *pathname, int pathlen,\n-\t\t\tconst char *basename,\n+\t\t\tint basename_offset,\n \t\t\tconst struct pattern *pat,\n \t\t\tconst char *base, int baselen)\n {\n@@ -667,8 +667,8 @@ static int path_matches(const char *pathname, int pathlen,\n \t\treturn 0;\n \n \tif (pat->flags & EXC_FLAG_NODIR) {\n-\t\treturn match_basename(basename,\n-\t\t\t\t pathlen - (basename - pathname),\n+\t\treturn match_basename(pathname + basename_offset,\n+\t\t\t\t pathlen - basename_offset,\n \t\t\t\t pattern, prefix,\n \t\t\t\t pat->patternlen, pat->flags);\n \t}\n@@ -701,7 +701,7 @@ static int fill_one(const char *what, struct match_attr *a, int rem)\n \treturn rem;\n }\n \n-static int fill(const char *path, int pathlen, const char *basename,\n+static int fill(const char *path, int pathlen, int basename_offset,\n \t\tstruct attr_stack *stk, int rem)\n {\n \tint i;\n@@ -711,7 +711,7 @@ static int fill(const char *path, int pathlen, const char *basename,\n \t\tstruct match_attr *a = stk->attrs[i];\n \t\tif (a->is_macro)\n \t\t\tcontinue;\n-\t\tif (path_matches(path, pathlen, basename,\n+\t\tif (path_matches(path, pathlen, basename_offset,\n \t\t\t\t &a->u.pat, base, stk->originlen))\n \t\t\trem = fill_one(\"fill\", a, rem);\n \t}\n@@ -750,7 +750,8 @@ static void collect_all_attrs(const char *path)\n {\n \tstruct attr_stack *stk;\n \tint i, pathlen, rem, dirlen;\n-\tconst char *basename, *cp, *last_slash = NULL;\n+\tconst char *cp, *last_slash = NULL;\n+\tint basename_offset;\n \n \tfor (cp = path; *cp; cp++) {\n \t\tif (*cp == '/' && cp[1])\n@@ -758,10 +759,10 @@ static void collect_all_attrs(const char *path)\n \t}\n \tpathlen = cp - path;\n \tif (last_slash) {\n-\t\tbasename = last_slash + 1;\n+\t\tbasename_offset = last_slash + 1 - path;\n \t\tdirlen = last_slash - path;\n \t} else {\n-\t\tbasename = path;\n+\t\tbasename_offset = 0;\n \t\tdirlen = 0;\n \t}\n \n@@ -771,7 +772,7 @@ static void collect_all_attrs(const char *path)\n \n \trem = attr_nr;\n \tfor (stk = attr_stack; 0 < rem && stk; stk = stk->prev)\n-\t\trem = fill(path, pathlen, basename, stk, rem);\n+\t\trem = fill(path, pathlen, basename_offset, stk, rem);\n }\n \n int git_check_attr(const char *path, int num, struct git_attr_check *check)\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212584","messageId":"20130328214728.GB10936@sigill.intra.peff.net","threadId":"33230","inReplyTo":"20130328214358.GA10685@sigill.intra.peff.net","subject":"[PATCH 2/6] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-28T21:47:28Z","receivedAt":"2013-03-28T21:47:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Junio C Hamano \n\nThe function takes two counted strings ( and\n) as parameters, together with prefix (the\nlength of the prefix in pattern that is to be matched literally\nwithout globbing against the basename) and EXC_* flags that tells it\nhow to match the pattern against the basename.\n\nHowever, it did not pay attention to the length of these counted\nstrings. Update them to do the following:\n\n * When the entire pattern is to be matched literally, the pattern\n matches the basename only when the lengths of them are the same,\n and they match up to that length.\n\n * When the pattern is \"*\" followed by a string to be matched\n literally, make sure that the basenamelen is equal or longer than\n the \"literal\" part of the pattern, and the tail of the basename\n string matches that literal part.\n\n * Otherwise, use the new fnmatch_icase_mem helper to make\n sure we only lookmake sure we use only look at the\n counted part of the strings. Because these counted strings are\n full strings most of the time, we check for termination\n to avoid unnecessary allocation.\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Jeff King \n---\nCompared to v1:\n\n - This factors the fnmatch bits into a helper function so we can reuse it\n later. As a result, the variable names are changed a bit.\n\n - The original did:\n\n if (use_pat)\n strbuf_release(&pat);\n\n but AFAICT that was a useless conditional; use_pat always points to\n either the incoming buffer or the strbuf. But strbuf_release will\n handle both cases for us.\n\n dir.c | 40 ++++++++++++++++++++++++++++++++++++----\n 1 file changed, 36 insertions(+), 4 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 5a83aa7..fac82c1 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -34,6 +34,33 @@ int fnmatch_icase(const char *pattern, const char *string, int flags)\n \treturn fnmatch(pattern, string, flags | (ignore_case ? FNM_CASEFOLD : 0));\n }\n \n+static int fnmatch_icase_mem(const char *pattern, int patternlen,\n+\t\t\t const char *string, int stringlen,\n+\t\t\t int flags)\n+{\n+\tint match_status;\n+\tstruct strbuf pat_buf = STRBUF_INIT;\n+\tstruct strbuf str_buf = STRBUF_INIT;\n+\tconst char *use_pat = pattern;\n+\tconst char *use_str = string;\n+\n+\tif (pattern[patternlen]) {\n+\t\tstrbuf_add(&pat_buf, pattern, patternlen);\n+\t\tuse_pat = pat_buf.buf;\n+\t}\n+\tif (string[stringlen]) {\n+\t\tstrbuf_add(&str_buf, string, stringlen);\n+\t\tuse_str = str_buf.buf;\n+\t}\n+\n+\tmatch_status = fnmatch_icase(use_pat, use_str, 0);\n+\n+\tstrbuf_release(&pat_buf);\n+\tstrbuf_release(&str_buf);\n+\n+\treturn match_status;\n+}\n+\n static size_t common_prefix_len(const char **pathspec)\n {\n \tconst char *n, *first;\n@@ -537,15 +564,20 @@ int match_basename(const char *basename, int basenamelen,\n \t\t int flags)\n {\n \tif (prefix == patternlen) {\n-\t\tif (!strcmp_icase(pattern, basename))\n+\t\tif (patternlen == basenamelen &&\n+\t\t !strncmp_icase(pattern, basename, basenamelen))\n \t\t\treturn 1;\n \t} else if (flags & EXC_FLAG_ENDSWITH) {\n+\t\t/* \"*literal\" matching against \"fooliteral\" */\n \t\tif (patternlen - 1 <= basenamelen &&\n-\t\t !strcmp_icase(pattern + 1,\n-\t\t\t\t basename + basenamelen - patternlen + 1))\n+\t\t !strncmp_icase(pattern + 1,\n+\t\t\t\t basename + basenamelen - (patternlen - 1),\n+\t\t\t\t patternlen - 1))\n \t\t\treturn 1;\n \t} else {\n-\t\tif (fnmatch_icase(pattern, basename, 0) == 0)\n+\t\tif (fnmatch_icase_mem(pattern, patternlen,\n+\t\t\t\t basename, basenamelen,\n+\t\t\t\t 0) == 0)\n \t\t\treturn 1;\n \t}\n \treturn 0;\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212585","messageId":"20130328214747.GC10936@sigill.intra.peff.net","threadId":"33230","inReplyTo":"20130328214358.GA10685@sigill.intra.peff.net","subject":"[PATCH 3/6] dir.c::match_pathname(): adjust patternlen when shifting pattern","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-28T21:47:47Z","receivedAt":"2013-03-28T21:47:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we receive a pattern that starts with \"/\", we shift it\nforward to avoid looking at the \"/\" part. Since the prefix\nand patternlen parameters are counts of what is in the\npattern, we must decrement them as we increment the pointer.\n\nWe remembered to handle prefix, but not patternlen. This\ndidn't cause any bugs, though, because the patternlen\nparameter is not actually used. Since it will be used in\nfuture patches, let's correct this oversight.\n\nSigned-off-by: Jeff King \n---\nNew in this iteration.\n\n dir.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/dir.c b/dir.c\nindex fac82c1..cc4ce8b 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -597,6 +597,7 @@ int match_pathname(const char *pathname, int pathlen,\n \t */\n \tif (*pattern == '/') {\n \t\tpattern++;\n+\t\tpatternlen--;\n \t\tprefix--;\n \t}\n \n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212586","messageId":"20130328214821.GD10936@sigill.intra.peff.net","threadId":"33230","inReplyTo":"20130328214358.GA10685@sigill.intra.peff.net","subject":"[PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-28T21:48:21Z","receivedAt":"2013-03-28T21:48:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function takes two counted strings: a pair\nand a pair. But we end up feeding the result to\nfnmatch, which expects NUL-terminated strings.\n\nWe can fix this by calling the fnmatch_icase_mem function, which\nhandles re-allocating into a NUL-terminated string if necessary.\n\nWhile we're at it, we can avoid even calling fnmatch in some cases. In\naddition to patternlen, we get \"prefix\", the size of the pattern that\ncontains no wildcard characters. We do a straight match of the prefix\npart first, and then use fnmatch to cover the rest. But if there are\nno wildcards in the pattern at all, we do not even need to call\nfnmatch; we would simply be comparing two empty strings.\n\nSigned-off-by: Jeff King \n---\nNew in this iteration.\n\n dir.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex cc4ce8b..3ad44c3 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -624,11 +624,22 @@ int match_pathname(const char *pathname, int pathlen,\n \t\tif (strncmp_icase(pattern, name, prefix))\n \t\t\treturn 0;\n \t\tpattern += prefix;\n+\t\tpatternlen -= prefix;\n \t\tname += prefix;\n \t\tnamelen -= prefix;\n+\n+\t\t/*\n+\t\t * If the whole pattern did not have a wildcard,\n+\t\t * then our prefix match is all we need; we\n+\t\t * do not need to call fnmatch at all.\n+\t\t */\n+\t\tif (!patternlen && !namelen)\n+\t\t\treturn 1;\n \t}\n \n-\treturn fnmatch_icase(pattern, name, FNM_PATHNAME) == 0;\n+\treturn fnmatch_icase_mem(pattern, patternlen,\n+\t\t\t\t name, namelen,\n+\t\t\t\t FNM_PATHNAME) == 0;\n }\n \n /* Scan the list and let the last match determine the fate.\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212587","messageId":"20130328214913.GE10936@sigill.intra.peff.net","threadId":"33230","inReplyTo":"20130328214358.GA10685@sigill.intra.peff.net","subject":"[PATCH 5/6] attr.c::path_matches(): special case paths that end with a slash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-28T21:49:13Z","receivedAt":"2013-03-28T21:49:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Junio C Hamano \n\nThe function is given a string that ends with a slash to signal that\nthe path is a directory to make sure that a pattern that ends with a\nslash (i.e. MUSTBEDIR) can tell directories and non-directories\napart. However, the pattern itself (pat->pattern and\npat->patternlen) that came from such a MUSTBEDIR pattern is\nrepresented as a string that ends with a slash, but patternlen does\nnot count that trailing slash. A MUSTBEDIR pattern \"element/\" is\nrepresented as a counted string <\"element/\", 7> and this must match\nmatch pathname \"element/\".\n\nBecause match_basename() and match_pathname() want to see pathname\n\"element\" to match against the pattern <\"element/\", 7>, reduce the\nlength of the path to exclude the trailing slash when calling\nthese functions.\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Jeff King \n---\nTweaked since v1 to also drop the trailing slash when we pass the path\nto match_pathname.\n\n attr.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 4cfe0ee..4d620bc 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -661,18 +661,18 @@ static int path_matches(const char *pathname, int pathlen,\n {\n \tconst char *pattern = pat->pattern;\n \tint prefix = pat->nowildcardlen;\n+\tint isdir = (pathlen && pathname[pathlen - 1] == '/');\n \n-\tif ((pat->flags & EXC_FLAG_MUSTBEDIR) &&\n-\t ((!pathlen) || (pathname[pathlen-1] != '/')))\n+\tif ((pat->flags & EXC_FLAG_MUSTBEDIR) && !isdir)\n \t\treturn 0;\n \n \tif (pat->flags & EXC_FLAG_NODIR) {\n \t\treturn match_basename(pathname + basename_offset,\n-\t\t\t\t pathlen - basename_offset,\n+\t\t\t\t pathlen - basename_offset - isdir,\n \t\t\t\t pattern, prefix,\n \t\t\t\t pat->patternlen, pat->flags);\n \t}\n-\treturn match_pathname(pathname, pathlen,\n+\treturn match_pathname(pathname, pathlen - isdir,\n \t\t\t base, baselen,\n \t\t\t pattern, prefix, pat->patternlen, pat->flags);\n }\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212588","messageId":"20130328215004.GF10936@sigill.intra.peff.net","threadId":"33230","inReplyTo":"20130328214358.GA10685@sigill.intra.peff.net","subject":"[PATCH 6/6] t: check that a pattern without trailing slash matches a directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-28T21:50:04Z","receivedAt":"2013-03-28T21:50:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Prior to v1.8.1.1, with:\n\n git init\n echo content >foo &&\n mkdir subdir &&\n echo content >subdir/bar &&\n echo \"subdir export-ignore\" >.gitattributes\n git add . &&\n git commit -m one &&\n git archive HEAD | tar tf -\n\nthe resulting archive would contain only \"foo\" and\n\".gitattributes\", not subdir. This was broken with a recent\nchange that intended to allow \"subdir/ export-ignore\" to\nalso exclude the directory, but instead ended up _requiring_\nthe trailing slash by mistake.\n\nA pattern \"subdir\" should match any path \"subdir\", whether it is a\ndirectory or a non-diretory. A pattern \"subdir/\" insists that a\npath \"subdir\" must be a directory for it to match.\n\nThis patch adds test not just for this simple case, but also\nfor deeper cross-directory cases, as well as cases with\nwildcards.\n\nSigned-off-by: Jeff King \n---\nAdded new tests since v1 that handle the match_pathname code path.\n\n t/t5002-archive-attr-pattern.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh\nindex 0c847fb..6667d15 100755\n--- a/t/t5002-archive-attr-pattern.sh\n+++ b/t/t5002-archive-attr-pattern.sh\n@@ -27,6 +27,25 @@ test_expect_success 'setup' '\n \techo ignored-only-if-dir/ export-ignore >>.git/info/attributes &&\n \tgit add ignored-only-if-dir &&\n \n+\tmkdir -p ignored-without-slash &&\n+\techo \"ignored without slash\" >ignored-without-slash/foo &&\n+\tgit add ignored-without-slash/foo &&\n+\techo \"ignored-without-slash export-ignore\" >>.git/info/attributes &&\n+\n+\tmkdir -p wildcard-without-slash &&\n+\techo \"ignored without slash\" >wildcard-without-slash/foo &&\n+\tgit add wildcard-without-slash/foo &&\n+\techo \"wild*-without-slash export-ignore\" >>.git/info/attributes &&\n+\n+\tmkdir -p deep/and/slashless &&\n+\techo \"ignored without slash\" >deep/and/slashless/foo &&\n+\tgit add deep/and/slashless/foo &&\n+\techo \"deep/and/slashless export-ignore\" >>.git/info/attributes &&\n+\n+\tmkdir -p deep/with/wildcard &&\n+\techo \"ignored without slash\" >deep/with/wildcard/foo &&\n+\tgit add deep/with/wildcard/foo &&\n+\techo \"deep/*t*/wildcard export-ignore\" >>.git/info/attributes &&\n \n \tmkdir -p one-level-lower/two-levels-lower/ignored-only-if-dir &&\n \techo ignored by ignored dir >one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir &&\n@@ -49,6 +68,14 @@ test_expect_missing\tarchive/ignored-ony-if-dir/ignored-by-ignored-dir\n test_expect_exists\tarchive/not-ignored-dir/\n test_expect_missing\tarchive/ignored-only-if-dir/\n test_expect_missing\tarchive/ignored-ony-if-dir/ignored-by-ignored-dir\n+test_expect_missing\tarchive/ignored-without-slash/ &&\n+test_expect_missing\tarchive/ignored-without-slash/foo &&\n+test_expect_missing\tarchive/wildcard-without-slash/\n+test_expect_missing\tarchive/wildcard-without-slash/foo &&\n+test_expect_missing\tarchive/deep/and/slashless/ &&\n+test_expect_missing\tarchive/deep/and/slashless/foo &&\n+test_expect_missing\tarchive/deep/with/wildcard/ &&\n+test_expect_missing\tarchive/deep/with/wildcard/foo &&\n test_expect_exists\tarchive/one-level-lower/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-only-if-dir/\n test_expect_missing\tarchive/one-level-lower/two-levels-lower/ignored-ony-if-dir/ignored-by-ignored-dir\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212589","messageId":"CAPig+cRyPMg_icgK1+5-WdXeaAZ9ytYQgFLgbVohRJaC9qzZQA@mail.gmail.com","threadId":"33230","inReplyTo":"20130328215004.GF10936@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] t: check that a pattern without trailing slash matches a directory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-03-28T22:21:08Z","receivedAt":"2013-03-28T22:21:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 28, 2013 at 5:50 PM, Jeff King wrote:\n> A pattern \"subdir\" should match any path \"subdir\", whether it is a\n> directory or a non-diretory. A pattern \"subdir/\" insists that a\n\ns/diretory/directory/ [1]\n\n> path \"subdir\" must be a directory for it to match.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/219185/\n"},{"id":"212590","messageId":"20130328222251.GA15173@sigill.intra.peff.net","threadId":"33230","inReplyTo":"CAPig+cRyPMg_icgK1+5-WdXeaAZ9ytYQgFLgbVohRJaC9qzZQA@mail.gmail.com","subject":"Re: [PATCH 6/6] t: check that a pattern without trailing slash matches a directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-28T22:22:51Z","receivedAt":"2013-03-28T22:22:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 28, 2013 at 06:21:08PM -0400, Eric Sunshine wrote:\n\n> On Thu, Mar 28, 2013 at 5:50 PM, Jeff King wrote:\n> > A pattern \"subdir\" should match any path \"subdir\", whether it is a\n> > directory or a non-diretory. A pattern \"subdir/\" insists that a\n> \n> s/diretory/directory/ [1]\n\nI think I've taken proofreading to a new level by missing the error I\nalready corrected somebody else for. Thanks for noticing. :)\n\n-Peff\n"},{"id":"212591","messageId":"7vk3orrwbv.fsf@alter.siamese.dyndns.org","threadId":"33230","inReplyTo":"20130328214821.GD10936@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-28T22:30:44Z","receivedAt":"2013-03-28T22:30:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This function takes two counted strings: a pair\n> and a pair. But we end up feeding the result to\n> fnmatch, which expects NUL-terminated strings.\n>\n> We can fix this by calling the fnmatch_icase_mem function, which\n> handles re-allocating into a NUL-terminated string if necessary.\n>\n> While we're at it, we can avoid even calling fnmatch in some cases. In\n> addition to patternlen, we get \"prefix\", the size of the pattern that\n> contains no wildcard characters. We do a straight match of the prefix\n> part first, and then use fnmatch to cover the rest. But if there are\n> no wildcards in the pattern at all, we do not even need to call\n> fnmatch; we would simply be comparing two empty strings.\n\nIt is not a new issue, but it would be nicer to have a comment to\nwarn people that this part needs to be adjusted when they want to\nadd support for using FNM_PERIOD in our codebase.\n\nOther than that, looks sane to me. Thanks.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> New in this iteration.\n>\n> dir.c | 13 ++++++++++++-\n> 1 file changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/dir.c b/dir.c\n> index cc4ce8b..3ad44c3 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -624,11 +624,22 @@ int match_pathname(const char *pathname, int pathlen,\n> \t\tif (strncmp_icase(pattern, name, prefix))\n> \t\t\treturn 0;\n> \t\tpattern += prefix;\n> +\t\tpatternlen -= prefix;\n> \t\tname += prefix;\n> \t\tnamelen -= prefix;\n> +\n> +\t\t/*\n> +\t\t * If the whole pattern did not have a wildcard,\n> +\t\t * then our prefix match is all we need; we\n> +\t\t * do not need to call fnmatch at all.\n> +\t\t */\n> +\t\tif (!patternlen && !namelen)\n> +\t\t\treturn 1;\n> \t}\n> \n> -\treturn fnmatch_icase(pattern, name, FNM_PATHNAME) == 0;\n> +\treturn fnmatch_icase_mem(pattern, patternlen,\n> +\t\t\t\t name, namelen,\n> +\t\t\t\t FNM_PATHNAME) == 0;\n> }\n> \n> /* Scan the list and let the last match determine the fate.\n"},{"id":"212595","messageId":"20130328224027.GA9911@sigill.intra.peff.net","threadId":"33230","inReplyTo":"20130328214728.GB10936@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-28T22:40:27Z","receivedAt":"2013-03-28T22:40:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 28, 2013 at 05:47:28PM -0400, Jeff King wrote:\n\n> From: Junio C Hamano \n> \n> The function takes two counted strings ( and\n> ) as parameters, together with prefix (the\n> length of the prefix in pattern that is to be matched literally\n> without globbing against the basename) and EXC_* flags that tells it\n> how to match the pattern against the basename.\n> \n> However, it did not pay attention to the length of these counted\n> strings. Update them to do the following:\n> \n> * When the entire pattern is to be matched literally, the pattern\n> matches the basename only when the lengths of them are the same,\n> and they match up to that length.\n\nHrm. Though the tip of this series passes all tests, this one actually\nbreaks bisectability. What happens is that the existing code passes:\n\n path=foo/\n pathlen=4\n\n pattern=foo/\n patternlen=3\n\nmatch_basename is happy to compare \"foo/\" to \"foo/\" and realize they're\nequal. With this change, we compare \"foo\" to \"foo/\" and do not match. It\nisn't until the later patch where you start passing pathlen=3 that it\nworks again.\n\nI wonder if it is worth reordering the series to put the path_matches\nfix first.\n\n-Peff\n"},{"id":"212596","messageId":"20130328224928.GA20242@sigill.intra.peff.net","threadId":"33230","inReplyTo":"20130328224027.GA9911@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-28T22:49:28Z","receivedAt":"2013-03-28T22:49:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 28, 2013 at 06:40:27PM -0400, Jeff King wrote:\n\n> On Thu, Mar 28, 2013 at 05:47:28PM -0400, Jeff King wrote:\n> \n> > From: Junio C Hamano \n> > \n> > The function takes two counted strings ( and\n> > ) as parameters, together with prefix (the\n> > length of the prefix in pattern that is to be matched literally\n> > without globbing against the basename) and EXC_* flags that tells it\n> > how to match the pattern against the basename.\n> > \n> > However, it did not pay attention to the length of these counted\n> > strings. Update them to do the following:\n> > \n> > * When the entire pattern is to be matched literally, the pattern\n> > matches the basename only when the lengths of them are the same,\n> > and they match up to that length.\n> \n> Hrm. Though the tip of this series passes all tests, this one actually\n> breaks bisectability. What happens is that the existing code passes:\n\nUgh. That is a problem, but this series does _not_ pass all tests. I\nthink I failed to run the complete test suite on the correct tip.\n\nMy match_pathspec fix breaks at least t1011.\n\n-Peff\n"},{"id":"212597","messageId":"7vr4izqfwt.fsf@alter.siamese.dyndns.org","threadId":"33230","inReplyTo":"20130328224928.GA20242@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-28T23:10:42Z","receivedAt":"2013-03-28T23:10:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Mar 28, 2013 at 06:40:27PM -0400, Jeff King wrote:\n>\n>> On Thu, Mar 28, 2013 at 05:47:28PM -0400, Jeff King wrote:\n>> \n>> > From: Junio C Hamano \n>> > \n>> > The function takes two counted strings ( and\n>> > ) as parameters, together with prefix (the\n>> > length of the prefix in pattern that is to be matched literally\n>> > without globbing against the basename) and EXC_* flags that tells it\n>> > how to match the pattern against the basename.\n>> > \n>> > However, it did not pay attention to the length of these counted\n>> > strings. Update them to do the following:\n>> > \n>> > * When the entire pattern is to be matched literally, the pattern\n>> > matches the basename only when the lengths of them are the same,\n>> > and they match up to that length.\n>> \n>> Hrm. Though the tip of this series passes all tests, this one actually\n>> breaks bisectability. What happens is that the existing code passes:\n>\n> Ugh. That is a problem, but this series does _not_ pass all tests. I\n> think I failed to run the complete test suite on the correct tip.\n>\n> My match_pathspec fix breaks at least t1011.\n\nYeah, the tip of 'jch' (slightly ahead of 'next' that I use myself)\nhas 0003, 1011 and 3001 broken X-<.\n"},{"id":"212598","messageId":"CACsJy8BcRp7-6Zf6=6Snd1JO1kPaw8b-q=s1QF7TaYHPubJ-0A@mail.gmail.com","threadId":"33230","inReplyTo":"20130328224928.GA20242@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-28T23:40:30Z","receivedAt":"2013-03-28T23:40:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 29, 2013 at 5:49 AM, Jeff King wrote:\n> My match_pathspec fix breaks at least t1011.\n\nHaven't looked closely at the series, but I suspect you need this\n\nhttp://article.gmane.org/gmane.comp.version-control.git/219008\n-- \nDuy\n"},{"id":"212599","messageId":"CACsJy8CcTqkPeOZ7Xa=7J4BH4sSFD6X6hKjrLz3kf0J0E_J+cw@mail.gmail.com","threadId":"33230","inReplyTo":"20130328214728.GB10936@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-29T01:25:00Z","receivedAt":"2013-03-29T01:25:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 29, 2013 at 4:47 AM, Jeff King wrote:\n> +static int fnmatch_icase_mem(const char *pattern, int patternlen,\n> + const char *string, int stringlen,\n> + int flags)\n> +{\n> + int match_status;\n> + struct strbuf pat_buf = STRBUF_INIT;\n> + struct strbuf str_buf = STRBUF_INIT;\n> + const char *use_pat = pattern;\n> + const char *use_str = string;\n> +\n> + if (pattern[patternlen]) {\n> + strbuf_add(&pat_buf, pattern, patternlen);\n> + use_pat = pat_buf.buf;\n> + }\n> + if (string[stringlen]) {\n> + strbuf_add(&str_buf, string, stringlen);\n> + use_str = str_buf.buf;\n> + }\n> +\n> + match_status = fnmatch_icase(use_pat, use_str, 0);\n\nYou should pass flags in here instead of 0.\n-- \nDuy\n"},{"id":"212600","messageId":"20130329030217.GA1851@sigill.intra.peff.net","threadId":"33230","inReplyTo":"CACsJy8CcTqkPeOZ7Xa=7J4BH4sSFD6X6hKjrLz3kf0J0E_J+cw@mail.gmail.com","subject":"Re: [PATCH 2/6] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-29T03:02:17Z","receivedAt":"2013-03-29T03:02:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 29, 2013 at 08:25:00AM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> On Fri, Mar 29, 2013 at 4:47 AM, Jeff King wrote:\n> > +static int fnmatch_icase_mem(const char *pattern, int patternlen,\n> > + const char *string, int stringlen,\n> > + int flags)\n> > +{\n> > + int match_status;\n> > + struct strbuf pat_buf = STRBUF_INIT;\n> > + struct strbuf str_buf = STRBUF_INIT;\n> > + const char *use_pat = pattern;\n> > + const char *use_str = string;\n> > +\n> > + if (pattern[patternlen]) {\n> > + strbuf_add(&pat_buf, pattern, patternlen);\n> > + use_pat = pat_buf.buf;\n> > + }\n> > + if (string[stringlen]) {\n> > + strbuf_add(&str_buf, string, stringlen);\n> > + use_str = str_buf.buf;\n> > + }\n> > +\n> > + match_status = fnmatch_icase(use_pat, use_str, 0);\n> \n> You should pass flags in here instead of 0.\n\nEek, yeah, that's obviously wrong. Thanks for catching it. Fixing that\nclears up all of the test failures outside of t5002.\n\nAnd if you move patch 5 (\"special case paths that end with a slash\")\ninto position 2, it cleans up the mid-series failures of t5002, making\nthe series clean for later bisecting.\n\nThanks for looking it over.\n\n-Peff\n"},{"id":"212605","messageId":"7vy5d6px2u.fsf@alter.siamese.dyndns.org","threadId":"33230","inReplyTo":"20130329030217.GA1851@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] dir.c::match_basename(): pay attention to the length of string parameters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-29T05:57:29Z","receivedAt":"2013-03-29T05:57:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Eek, yeah, that's obviously wrong. Thanks for catching it. Fixing that\n> clears up all of the test failures outside of t5002.\n>\n> And if you move patch 5 (\"special case paths that end with a slash\")\n> into position 2, it cleans up the mid-series failures of t5002, making\n> the series clean for later bisecting.\n\nYeah, the tip becomes test-clean with that \"flags\" change, but it\nappears that there seems to be some mismerge (there is another topic\nor two in flight that want to touch dir.c for pathspecs) that breaks\n0003 and 3001 when merged to 'pu'.\n\nDuy, can you take another look?\n"},{"id":"212606","messageId":"CACsJy8DisE8UNZzqmOFxPqw=bmFiHgE5-ao83ciGNUV9Sc9-gA@mail.gmail.com","threadId":"33230","inReplyTo":"20130328214821.GD10936@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-29T08:45:35Z","receivedAt":"2013-03-29T08:45:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 29, 2013 at 4:48 AM, Jeff King wrote:\n> - return fnmatch_icase(pattern, name, FNM_PATHNAME) == 0;\n> + return fnmatch_icase_mem(pattern, patternlen,\n> + name, namelen,\n> + FNM_PATHNAME) == 0;\n> }\n\nI think you (or Junio) should rebase this on maint. Since c41244e\n(included in maint), this call is turned to wildmatch(WM_PATHNAME) and\nWM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like\n\"foo/**/bar\". A diff between next and pu shows me that WM_PATHNAME is\nincorrectly converted to FNM_PATHNAME. I hope that is the cause of all\nbreakages Junio found out on pu.\n-- \nDuy\n"},{"id":"212607","messageId":"CACsJy8CT8DQEEJVMwonW05bFZNb7HjQWBdEjy3tv332xabJ=Bw@mail.gmail.com","threadId":"33230","inReplyTo":"CACsJy8DisE8UNZzqmOFxPqw=bmFiHgE5-ao83ciGNUV9Sc9-gA@mail.gmail.com","subject":"Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-29T10:03:34Z","receivedAt":"2013-03-29T10:03:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 29, 2013 at 3:45 PM, Duy Nguyen wrote:\n> On Fri, Mar 29, 2013 at 4:48 AM, Jeff King wrote:\n>> - return fnmatch_icase(pattern, name, FNM_PATHNAME) == 0;\n>> + return fnmatch_icase_mem(pattern, patternlen,\n>> + name, namelen,\n>> + FNM_PATHNAME) == 0;\n>> }\n>\n> I think you (or Junio) should rebase this on maint. Since c41244e\n> (included in maint), this call is turned to wildmatch(WM_PATHNAME) and\n> WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like\n> \"foo/**/bar\". A diff between next and pu shows me that WM_PATHNAME is\n> incorrectly converted to FNM_PATHNAME. I hope that is the cause of all\n> breakages Junio found out on pu.\n\nJust tested. t0003 and t3001 on 'pu' work for me because I have\nUSE_WILDMATCH on (which turns FNM_PATHNAME to WM_PATHNAME). Both break\nwithout USE_WILDMATCH.\n-- \nDuy\n"},{"id":"212615","messageId":"51557BE5.8000305@web.de","threadId":"33230","inReplyTo":"CACsJy8CT8DQEEJVMwonW05bFZNb7HjQWBdEjy3tv332xabJ=Bw@mail.gmail.com","subject":"Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-03-29T11:32:53Z","receivedAt":"2013-03-29T11:32:53Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 29.03.13 11:03, Duy Nguyen wrote:\n> On Fri, Mar 29, 2013 at 3:45 PM, Duy Nguyen wrote:\n>> On Fri, Mar 29, 2013 at 4:48 AM, Jeff King wrote:\n>>> - return fnmatch_icase(pattern, name, FNM_PATHNAME) == 0;\n>>> + return fnmatch_icase_mem(pattern, patternlen,\n>>> + name, namelen,\n>>> + FNM_PATHNAME) == 0;\n>>> }\n>>\n>> I think you (or Junio) should rebase this on maint. Since c41244e\n>> (included in maint), this call is turned to wildmatch(WM_PATHNAME) and\n>> WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like\n>> \"foo/**/bar\". A diff between next and pu shows me that WM_PATHNAME is\n>> incorrectly converted to FNM_PATHNAME. I hope that is the cause of all\n>> breakages Junio found out on pu.\n> \n> Just tested. t0003 and t3001 on 'pu' work for me because I have\n> USE_WILDMATCH on (which turns FNM_PATHNAME to WM_PATHNAME). Both break\n> without USE_WILDMATCH.\n> \nHm, tested what?\n\ndiff --git a/dir.c b/dir.c\nindex 73a08af..0b63167 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -564,7 +564,7 @@ int match_pathname(const char *pathname, int pathlen,\n \n return fnmatch_icase_mem(pattern, patternlen,\n name, namelen,\n- FNM_PATHNAME) == 0;\n+ WM_PATHNAME) == 0;\n }\n \nGives only one breakage, so we are coming closer.\n\n\n*** t3001-ls-files-others-exclude.sh ***\n[snip]\nnot ok 17 - ls-files with \"**\" patterns\n"},{"id":"212616","messageId":"CACsJy8AwrcbLuTnzq-y=xbXnyA-M=QCqu_S+h1of4-aWYfcN2w@mail.gmail.com","threadId":"33230","inReplyTo":"51557BE5.8000305@web.de","subject":"Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-29T11:37:52Z","receivedAt":"2013-03-29T11:37:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 29, 2013 at 6:32 PM, Torsten Bögershausen wrote:\n>> Just tested. t0003 and t3001 on 'pu' work for me because I have\n>> USE_WILDMATCH on (which turns FNM_PATHNAME to WM_PATHNAME). Both break\n>> without USE_WILDMATCH.\n>>\n> Hm, tested what?\n\nTested t0003 and t3001 with and without USE_WILDMATCH, which is pretty\nmuch like you patch, except that wildmatch is used instead of fnmatch.\n\n> diff --git a/dir.c b/dir.c\n> index 73a08af..0b63167 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -564,7 +564,7 @@ int match_pathname(const char *pathname, int pathlen,\n>\n> return fnmatch_icase_mem(pattern, patternlen,\n> name, namelen,\n> - FNM_PATHNAME) == 0;\n> + WM_PATHNAME) == 0;\n> }\n>\n> Gives only one breakage, so we are coming closer.\n>\n>\n> *** t3001-ls-files-others-exclude.sh ***\n> [snip]\n> not ok 17 - ls-files with \"**\" patterns\n>\n>\n>\n\n\n\n-- \nDuy\n"},{"id":"212617","messageId":"20130329120539.GA20711@sigill.intra.peff.net","threadId":"33230","inReplyTo":"CACsJy8DisE8UNZzqmOFxPqw=bmFiHgE5-ao83ciGNUV9Sc9-gA@mail.gmail.com","subject":"Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-29T12:05:40Z","receivedAt":"2013-03-29T12:05:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 29, 2013 at 03:45:35PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> On Fri, Mar 29, 2013 at 4:48 AM, Jeff King wrote:\n> > - return fnmatch_icase(pattern, name, FNM_PATHNAME) == 0;\n> > + return fnmatch_icase_mem(pattern, patternlen,\n> > + name, namelen,\n> > + FNM_PATHNAME) == 0;\n> > }\n> \n> I think you (or Junio) should rebase this on maint. Since c41244e\n> (included in maint), this call is turned to wildmatch(WM_PATHNAME) and\n> WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like\n> \"foo/**/bar\". A diff between next and pu shows me that WM_PATHNAME is\n> incorrectly converted to FNM_PATHNAME. I hope that is the cause of all\n> breakages Junio found out on pu.\n\nI don't think we want to rebase; the regression is in the v1.8.1 series,\nand I suspected that Junio was planning to ship a v1.8.1.6 with the fix.\nThe wildmatch code comes in v1.8.2.\n\nSo we would want to do any adjustment to the fix when we merge up to\nmaint.\n\n-Peff\n"},{"id":"212618","messageId":"20130329130230.GA25861@lanh","threadId":"33230","inReplyTo":"20130329120539.GA20711@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-29T13:02:30Z","receivedAt":"2013-03-29T13:02:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 29, 2013 at 08:05:40AM -0400, Jeff King wrote:\n> On Fri, Mar 29, 2013 at 03:45:35PM +0700, Nguyen Thai Ngoc Duy wrote:\n> \n> > On Fri, Mar 29, 2013 at 4:48 AM, Jeff King wrote:\n> > > - return fnmatch_icase(pattern, name, FNM_PATHNAME) == 0;\n> > > + return fnmatch_icase_mem(pattern, patternlen,\n> > > + name, namelen,\n> > > + FNM_PATHNAME) == 0;\n> > > }\n> > \n> > I think you (or Junio) should rebase this on maint. Since c41244e\n> > (included in maint), this call is turned to wildmatch(WM_PATHNAME) and\n> > WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like\n> > \"foo/**/bar\". A diff between next and pu shows me that WM_PATHNAME is\n> > incorrectly converted to FNM_PATHNAME. I hope that is the cause of all\n> > breakages Junio found out on pu.\n> \n> I don't think we want to rebase; the regression is in the v1.8.1 series,\n> and I suspected that Junio was planning to ship a v1.8.1.6 with the fix.\n> The wildmatch code comes in v1.8.2.\n> \n> So we would want to do any adjustment to the fix when we merge up to\n> maint.\n\nOK. Then Junio, you may need to resolve the conflict with something\nlike this. Originally match_basename uses fnmatch, not wildmatch. But\nusing wildmatch there too should be fine, now that both\nmatch_{base,path}name share fnmatch_icase_mem().\n\n-- 8< --\ndiff --git a/dir.c b/dir.c\nindex 73a08af..84744df 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -81,7 +81,9 @@ static int fnmatch_icase_mem(const char *pattern, int patternlen,\n \t\tuse_str = str_buf.buf;\n \t}\n \n-\tmatch_status = fnmatch_icase(use_pat, use_str, flags);\n+\tif (ignore_case)\n+\t\tflags |= WM_CASEFOLD;\n+\tmatch_status = wildmatch(use_pat, use_str, flags, NULL);\n \n \tstrbuf_release(&pat_buf);\n \tstrbuf_release(&str_buf);\n@@ -564,7 +566,7 @@ int match_pathname(const char *pathname, int pathlen,\n \n \treturn fnmatch_icase_mem(pattern, patternlen,\n \t\t\t\t name, namelen,\n-\t\t\t\t FNM_PATHNAME) == 0;\n+\t\t\t\t WM_PATHNAME) == 0;\n }\n \n /*\n-- 8< --\n"},{"id":"212626","messageId":"7vli96p34f.fsf@alter.siamese.dyndns.org","threadId":"33230","inReplyTo":"20130329130230.GA25861@lanh","subject":"Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-29T16:44:32Z","receivedAt":"2013-03-29T16:44:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> So we would want to do any adjustment to the fix when we merge up to\n>> maint.\n>\n> OK. Then Junio, you may need to resolve the conflict with something\n> like this. Originally match_basename uses fnmatch, not wildmatch. But\n> using wildmatch there too should be fine, now that both\n> match_{base,path}name share fnmatch_icase_mem().\n\nThanks.\n\nThe result still smells somewhat funny, though.\n\nfnmatch_icase_mem() is meant to be a wrapper of fnmatch_icase() for\ncounted strings and its matching semantics should be the same as\nfnmatch_icase().\n\nWith the merge-fix, fnmatch_icase_mem() calls into wildmatch(), but\nfnmatch_icase() still calls into fnmatch().\n\nThe latter's flags are meant to be taken from FNM_* family, but the\nformer takes flags from WM_* family of bits, no?\n\nI think you are running with USE_WILDMATCH which may make the\ndifferences harder to notice, but the name fnmatch_icase_mem() that\nis not in the same family as fnmatch but is from the wildmatch()\nfamily smells like an accident waiting to happen.\n\nI tend to think in the longer term it may be a good idea to build\nwith USE_WILDMATCH unconditionally (we can lose compat/fnmatch), so\nin the end this may not matter that much, but before that happens,\nsoon after we merge the regression fix with this merge-fix, we may\nwant to update the codebase as if we applied a series that were\nbased on 'maint' as you suggested, i.e. using raw wildmatch()\nconsistently in the match_{base,path}name() codepath.\n\nOpinions?\n\n>\n> -- 8< --\n> diff --git a/dir.c b/dir.c\n> index 73a08af..84744df 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -81,7 +81,9 @@ static int fnmatch_icase_mem(const char *pattern, int patternlen,\n> \t\tuse_str = str_buf.buf;\n> \t}\n> \n> -\tmatch_status = fnmatch_icase(use_pat, use_str, flags);\n> +\tif (ignore_case)\n> +\t\tflags |= WM_CASEFOLD;\n> +\tmatch_status = wildmatch(use_pat, use_str, flags, NULL);\n> \n> \tstrbuf_release(&pat_buf);\n> \tstrbuf_release(&str_buf);\n> @@ -564,7 +566,7 @@ int match_pathname(const char *pathname, int pathlen,\n> \n> \treturn fnmatch_icase_mem(pattern, patternlen,\n> \t\t\t\t name, namelen,\n> -\t\t\t\t FNM_PATHNAME) == 0;\n> +\t\t\t\t WM_PATHNAME) == 0;\n> }\n> \n> /*\n> -- 8< --\n"},{"id":"212628","messageId":"20130329170459.GB3552@sigill.intra.peff.net","threadId":"33230","inReplyTo":"7vli96p34f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-29T17:04:59Z","receivedAt":"2013-03-29T17:04:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 29, 2013 at 09:44:32AM -0700, Junio C Hamano wrote:\n\n> Duy Nguyen writes:\n> \n> >> So we would want to do any adjustment to the fix when we merge up to\n> >> maint.\n> >\n> > OK. Then Junio, you may need to resolve the conflict with something\n> > like this. Originally match_basename uses fnmatch, not wildmatch. But\n> > using wildmatch there too should be fine, now that both\n> > match_{base,path}name share fnmatch_icase_mem().\n> \n> Thanks.\n> \n> The result still smells somewhat funny, though.\n> \n> fnmatch_icase_mem() is meant to be a wrapper of fnmatch_icase() for\n> counted strings and its matching semantics should be the same as\n> fnmatch_icase().\n> \n> With the merge-fix, fnmatch_icase_mem() calls into wildmatch(), but\n> fnmatch_icase() still calls into fnmatch().\n> \n> The latter's flags are meant to be taken from FNM_* family, but the\n> former takes flags from WM_* family of bits, no?\n\nYeah, that does not seem right. If match_pathname has learned to call\ninto wildmatch instead of fnmatch_icase in the interim, then the right\nresolution is to convert its call to fnmatch_icase_mem to a new\nwildmatch_mem. Presumably that can be done by either tweaking\nfnmatch_icase_mem, or, if wildmatch is ready to take counted strings,\ncalling into it with the right options.\n\n> I think you are running with USE_WILDMATCH which may make the\n> differences harder to notice, but the name fnmatch_icase_mem() that is\n> not in the same family as fnmatch but is from the wildmatch() family\n> smells like an accident waiting to happen.\n\nAgreed.\n\n> I tend to think in the longer term it may be a good idea to build with\n> USE_WILDMATCH unconditionally (we can lose compat/fnmatch), so in the\n> end this may not matter that much\n\nYeah, I think that is a sane long-term goal.\n\n-Peff\n"},{"id":"212634","messageId":"7vzjxmnm7e.fsf@alter.siamese.dyndns.org","threadId":"33230","inReplyTo":"20130329170459.GB3552@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-29T17:35:17Z","receivedAt":"2013-03-29T17:35:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Mar 29, 2013 at 09:44:32AM -0700, Junio C Hamano wrote:\n> ...\n>> With the merge-fix, fnmatch_icase_mem() calls into wildmatch(), but\n>> fnmatch_icase() still calls into fnmatch().\n>> \n>> The latter's flags are meant to be taken from FNM_* family, but the\n>> former takes flags from WM_* family of bits, no?\n>\n> Yeah, that does not seem right. If match_pathname has learned to call\n> into wildmatch instead of fnmatch_icase in the interim, then the right\n> resolution is to convert its call to fnmatch_icase_mem to a new\n> wildmatch_mem. Presumably that can be done by either tweaking\n> fnmatch_icase_mem, or, if wildmatch is ready to take counted strings,\n> calling into it with the right options.\n>\n>> I think you are running with USE_WILDMATCH which may make the\n>> differences harder to notice, but the name fnmatch_icase_mem() that is\n>> not in the same family as fnmatch but is from the wildmatch() family\n>> smells like an accident waiting to happen.\n>\n> Agreed.\n\nThis may be just the matter of naming.\n\nIt smelled wrong to me only because the \"fnmatch\" in the helper\nfnmatch_icase_mem() told me that it should forever use fnmatch\nsemantics. After reading its (only) two callsites, they are both\n\"the caller has transformed the inputs to this lowest level pathname\nvs pattern matching function in order to reduce the cost of\nmatching, and now it is time to exercise the matcher\". The only\nthing they care about is that they are calling \"the lowest level\npathname vs pattern matching function.\"\n\nIf we pronounce \"fnmatch_icase_mem()\" as \"match_path_with_pattern()\"\nor something in the original series, the problem may go away ;-)\n\nDoes any caller pass FNM_* bits to a callchain that reach the new *_mem()\nfunction?\n"},{"id":"212636","messageId":"20130329174455.GA13268@sigill.intra.peff.net","threadId":"33230","inReplyTo":"7vzjxmnm7e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-29T17:44:55Z","receivedAt":"2013-03-29T17:44:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 29, 2013 at 10:35:17AM -0700, Junio C Hamano wrote:\n\n> This may be just the matter of naming.\n> \n> It smelled wrong to me only because the \"fnmatch\" in the helper\n> fnmatch_icase_mem() told me that it should forever use fnmatch\n> semantics. After reading its (only) two callsites, they are both\n> \"the caller has transformed the inputs to this lowest level pathname\n> vs pattern matching function in order to reduce the cost of\n> matching, and now it is time to exercise the matcher\". The only\n> thing they care about is that they are calling \"the lowest level\n> pathname vs pattern matching function.\"\n> \n> If we pronounce \"fnmatch_icase_mem()\" as \"match_path_with_pattern()\"\n> or something in the original series, the problem may go away ;-)\n\nYes, since there are only the two new added callers, if they both want\nto switch to wildmatch, then it is fine for the helper function to\nswitch. The danger is if some other caller wants to start using it; I\nadded it with the name I did figuring that other spots might want to use\nit eventually. But if all of fnmatch is going to go away in favor of\nwildmatch eventually, then that helper is not all that useful (or it\nwould be even more useful as \"wildmatch_mem\" or similar).\n\n> Does any caller pass FNM_* bits to a callchain that reach the new *_mem()\n> function?\n\nThe series only adds two callers, and they provide constant flags;\nmatch_basename passes no flags, and should be OK. match_pathname uses\nFNM_PATHNAME, and would need to be converted to use WM_PATHNAME as part\nof the conflict resolution.\n\n-Peff\n"},{"id":"212668","messageId":"20130330014024.GA9025@lanh","threadId":"33230","inReplyTo":"7vli96p34f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-30T01:40:24Z","receivedAt":"2013-03-30T01:40:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 29, 2013 at 09:44:32AM -0700, Junio C Hamano wrote:\n> I tend to think in the longer term it may be a good idea to build\n> with USE_WILDMATCH unconditionally (we can lose compat/fnmatch), so\n> in the end this may not matter that much\n\nI was thinking about that yesterday. After all, it's the purpose of\nUSE_WILDMATCH and nd/retire-fnmatch series. So how about this patch to\nenable USE_WILDMATCH for a wider audience? I think it could get merged\ndown to master. If wildmatch's bug history is messy by the time we\nenter rc cycles, we could revert the patch and cut new releases with\nfnmatch. If not, after 1.9 is released, I'll submit a patch to remove\nfnmatch compatiblity later and we will be on wildmatch only.\n\n-- 8< --\ndiff --git a/Makefile b/Makefile\nindex 598d631..8a740f8 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -106,7 +106,7 @@ all::\n # Define NO_FNMATCH_CASEFOLD if your fnmatch function doesn't have the\n # FNM_CASEFOLD GNU extension.\n #\n-# Define USE_WILDMATCH if you want to use Git's wildmatch\n+# Define NO_USE_WILDMATCH if you do not want to use Git's wildmatch\n # implementation as fnmatch\n #\n # Define NO_GECOS_IN_PWENT if you don't have pw_gecos in struct passwd\n@@ -1255,7 +1255,7 @@ ifdef NO_FNMATCH_CASEFOLD\n \tCOMPAT_OBJS += compat/fnmatch/fnmatch.o\n endif\n endif\n-ifdef USE_WILDMATCH\n+ifndef NO_USE_WILDMATCH\n \tCOMPAT_CFLAGS += -DUSE_WILDMATCH\n endif\n ifdef NO_SETENV\n-- 8< --\n"}]}