{"thread":{"id":"31762","subject":"[PATCH nd/attr-match-optim-more 1/2] gitignore: make pattern parsing code a separate function","startedAt":"2012-10-09T02:24:04Z","lastAt":"2012-10-13T04:32:59Z","messageCount":14,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Johannes Sixt","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"200801","messageId":"1349749445-25397-1-git-send-email-pclouds@gmail.com","threadId":"31762","inReplyTo":null,"subject":"[PATCH nd/attr-match-optim-more 1/2] gitignore: make pattern parsing code a separate function","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-09T02:24:04Z","receivedAt":"2012-10-09T02:24:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This function can later be reused by attr.c. Also turn to_exclude\nfield into a flag.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n No changes.\n\n dir.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++---------------------\n dir.h | 2 +-\n 2 files changed, 50 insertions(+), 23 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 0015cc5..48aed85 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -308,42 +308,69 @@ static int no_wildcard(const char *string)\n \treturn string[simple_length(string)] == '\\0';\n }\n \n+static void parse_exclude_pattern(const char **pattern,\n+\t\t\t\t int *patternlen,\n+\t\t\t\t int *flags,\n+\t\t\t\t int *nowildcardlen)\n+{\n+\tconst char *p = *pattern;\n+\tsize_t i, len;\n+\n+\t*flags = 0;\n+\tif (*p == '!') {\n+\t\t*flags |= EXC_FLAG_NEGATIVE;\n+\t\tp++;\n+\t}\n+\tlen = strlen(p);\n+\tif (len && p[len - 1] == '/') {\n+\t\tlen--;\n+\t\t*flags |= EXC_FLAG_MUSTBEDIR;\n+\t}\n+\tfor (i = 0; i < len; i++) {\n+\t\tif (p[i] == '/')\n+\t\t\tbreak;\n+\t}\n+\tif (i == len)\n+\t\t*flags |= EXC_FLAG_NODIR;\n+\t*nowildcardlen = simple_length(p);\n+\t/*\n+\t * we should have excluded the trailing slash from 'p' too,\n+\t * but that's one more allocation. Instead just make sure\n+\t * nowildcardlen does not exceed real patternlen\n+\t */\n+\tif (*nowildcardlen > len)\n+\t\t*nowildcardlen = len;\n+\tif (*p == '*' && no_wildcard(p + 1))\n+\t\t*flags |= EXC_FLAG_ENDSWITH;\n+\t*pattern = p;\n+\t*patternlen = len;\n+}\n+\n void add_exclude(const char *string, const char *base,\n \t\t int baselen, struct exclude_list *which)\n {\n \tstruct exclude *x;\n-\tsize_t len;\n-\tint to_exclude = 1;\n-\tint flags = 0;\n+\tint patternlen;\n+\tint flags;\n+\tint nowildcardlen;\n \n-\tif (*string == '!') {\n-\t\tto_exclude = 0;\n-\t\tstring++;\n-\t}\n-\tlen = strlen(string);\n-\tif (len && string[len - 1] == '/') {\n+\tparse_exclude_pattern(&string, &patternlen, &flags, &nowildcardlen);\n+\tif (flags & EXC_FLAG_MUSTBEDIR) {\n \t\tchar *s;\n-\t\tx = xmalloc(sizeof(*x) + len);\n+\t\tx = xmalloc(sizeof(*x) + patternlen + 1);\n \t\ts = (char *)(x+1);\n-\t\tmemcpy(s, string, len - 1);\n-\t\ts[len - 1] = '\\0';\n-\t\tstring = s;\n+\t\tmemcpy(s, string, patternlen);\n+\t\ts[patternlen] = '\\0';\n \t\tx->pattern = s;\n-\t\tflags = EXC_FLAG_MUSTBEDIR;\n \t} else {\n \t\tx = xmalloc(sizeof(*x));\n \t\tx->pattern = string;\n \t}\n-\tx->to_exclude = to_exclude;\n-\tx->patternlen = strlen(string);\n+\tx->patternlen = patternlen;\n+\tx->nowildcardlen = nowildcardlen;\n \tx->base = base;\n \tx->baselen = baselen;\n \tx->flags = flags;\n-\tif (!strchr(string, '/'))\n-\t\tx->flags |= EXC_FLAG_NODIR;\n-\tx->nowildcardlen = simple_length(string);\n-\tif (*string == '*' && no_wildcard(string+1))\n-\t\tx->flags |= EXC_FLAG_ENDSWITH;\n \tALLOC_GROW(which->excludes, which->nr + 1, which->alloc);\n \twhich->excludes[which->nr++] = x;\n }\n@@ -518,7 +545,7 @@ int excluded_from_list(const char *pathname,\n \tfor (i = el->nr - 1; 0 <= i; i--) {\n \t\tstruct exclude *x = el->excludes[i];\n \t\tconst char *name, *exclude = x->pattern;\n-\t\tint to_exclude = x->to_exclude;\n+\t\tint to_exclude = x->flags & EXC_FLAG_NEGATIVE ? 0 : 1;\n \t\tint namelen, prefix = x->nowildcardlen;\n \n \t\tif (x->flags & EXC_FLAG_MUSTBEDIR) {\ndiff --git a/dir.h b/dir.h\nindex 893465a..41ea32d 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -11,6 +11,7 @@ struct dir_entry {\n #define EXC_FLAG_NODIR 1\n #define EXC_FLAG_ENDSWITH 4\n #define EXC_FLAG_MUSTBEDIR 8\n+#define EXC_FLAG_NEGATIVE 16\n \n struct exclude_list {\n \tint nr;\n@@ -21,7 +22,6 @@ struct exclude_list {\n \t\tint nowildcardlen;\n \t\tconst char *base;\n \t\tint baselen;\n-\t\tint to_exclude;\n \t\tint flags;\n \t} **excludes;\n };\n-- \n1.8.0.rc0.29.g1fdd78f\n"},{"id":"200802","messageId":"1349749445-25397-2-git-send-email-pclouds@gmail.com","threadId":"31762","inReplyTo":"1349749445-25397-1-git-send-email-pclouds@gmail.com","subject":"[PATCH nd/attr-match-optim-more 2/2] attr: more matching optimizations from .gitignore","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-09T02:24:05Z","receivedAt":"2012-10-09T02:24:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":".gitattributes and .gitignore share the same pattern syntax but has\nseparate matching implementation. Over the years, ignore's\nimplementation accumulates more optimizations while attr's stays the\nsame.\n\nThis patch adds those optimizations to attr. Basically it tries to\navoid fnmatch as much as possible in favor of strncmp.\n\nA few notes from this work is put in the documentation:\n\n* \"!pattern\" syntax is not supported in .gitattributes. Negative\n patterns may work well for a single attribute like .gitignore. It's\n confusing in .gitattributes are many attributes can be\n set/unset/undefined at using the same pattern.\n\n* we support attaching attributes to directories at the syntax\n level, but we do not really attach attributes on directory or use\n them.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n No more die() for path/ syntax. Rephrase \"!syntax\" messages from not\n supported to forbidden. A small test to verify that we actually\n forbid it.\n\n Documentation/gitattributes.txt | 3 +-\n attr.c | 67 ++++++++++++++++++++++++++++++++++-------\n dir.c | 8 ++---\n dir.h | 1 +\n t/t0003-attributes.sh | 14 +++++++++\n 5 files changed, 77 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 80120ea..cc2ff1d 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -23,7 +23,7 @@ Each line in `gitattributes` file is of form:\n That is, a pattern followed by an attributes list,\n separated by whitespaces. When the pattern matches the\n path in question, the attributes listed on the line are given to\n-the path.\n+the path. Only files can be attached attributes to.\n \n Each attribute can be in one of these states for a given path:\n \n@@ -56,6 +56,7 @@ When more than one pattern matches the path, a later line\n overrides an earlier line. This overriding is done per\n attribute. The rules how the pattern matches paths are the\n same as in `.gitignore` files; see linkgit:gitignore[5].\n+Unlike `.gitignore`, negative patterns are forbidden.\n \n When deciding what attributes are assigned to a path, git\n consults `$GIT_DIR/info/attributes` file (which has the highest\ndiff --git a/attr.c b/attr.c\nindex e7caee4..7e85f82 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -115,6 +115,13 @@ struct attr_state {\n \tconst char *setto;\n };\n \n+struct pattern {\n+\tconst char *pattern;\n+\tint patternlen;\n+\tint nowildcardlen;\n+\tint flags;\t\t/* EXC_FLAG_* */\n+};\n+\n /*\n * One rule, as from a .gitattributes file.\n *\n@@ -131,7 +138,7 @@ struct attr_state {\n */\n struct match_attr {\n \tunion {\n-\t\tchar *pattern;\n+\t\tstruct pattern pat;\n \t\tstruct git_attr *attr;\n \t} u;\n \tchar is_macro;\n@@ -241,9 +248,17 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \tif (is_macro)\n \t\tres->u.attr = git_attr_internal(name, namelen);\n \telse {\n-\t\tres->u.pattern = (char *)&(res->state[num_attr]);\n-\t\tmemcpy(res->u.pattern, name, namelen);\n-\t\tres->u.pattern[namelen] = 0;\n+\t\tchar *p = (char *)&(res->state[num_attr]);\n+\t\tmemcpy(p, name, namelen);\n+\t\tp[namelen] = 0;\n+\t\tres->u.pat.pattern = p;\n+\t\tparse_exclude_pattern(&res->u.pat.pattern,\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_NEGATIVE)\n+\t\t\tdie(_(\"Negative patterns are forbidden in git attributes\\n\"\n+\t\t\t \"Use '\\\\!' for literal leading exclamation.\"));\n \t}\n \tres->is_macro = is_macro;\n \tres->num_attr = num_attr;\n@@ -640,25 +655,55 @@ static void prepare_attr_stack(const char *path)\n \n static int path_matches(const char *pathname, int pathlen,\n \t\t\tconst char *basename,\n-\t\t\tconst char *pattern,\n+\t\t\tconst struct pattern *pat,\n \t\t\tconst char *base, int baselen)\n {\n-\tif (!strchr(pattern, '/')) {\n+\tconst char *pattern = pat->pattern;\n+\tint prefix = pat->nowildcardlen;\n+\tconst char *name;\n+\tint namelen;\n+\n+\tif (pat->flags & EXC_FLAG_NODIR) {\n+\t\tif (prefix == pat->patternlen &&\n+\t\t !strcmp_icase(pattern, basename))\n+\t\t\treturn 1;\n+\n+\t\tif (pat->flags & EXC_FLAG_ENDSWITH &&\n+\t\t pat->patternlen - 1 <= pathlen &&\n+\t\t !strcmp_icase(pattern + 1, pathname +\n+\t\t\t\t pathlen - pat->patternlen + 1))\n+\t\t\treturn 1;\n+\n \t\treturn (fnmatch_icase(pattern, basename, 0) == 0);\n \t}\n \t/*\n \t * match with FNM_PATHNAME; the pattern has base implicitly\n \t * in front of it.\n \t */\n-\tif (*pattern == '/')\n+\tif (*pattern == '/') {\n \t\tpattern++;\n+\t\tprefix--;\n+\t}\n+\n+\t/*\n+\t * note: unlike excluded_from_list, baselen here does not\n+\t * contain the trailing slash\n+\t */\n+\n \tif (pathlen < baselen ||\n \t (baselen && pathname[baselen] != '/') ||\n \t strncmp(pathname, base, baselen))\n \t\treturn 0;\n-\tif (baselen != 0)\n-\t\tbaselen++;\n-\treturn fnmatch_icase(pattern, pathname + baselen, FNM_PATHNAME) == 0;\n+\n+\tnamelen = baselen ? pathlen - baselen - 1 : pathlen;\n+\tname = pathname + pathlen - namelen;\n+\n+\t/* if the non-wildcard part is longer than the remaining\n+\t pathname, surely it cannot match */\n+\tif (!namelen || prefix > namelen)\n+\t\treturn 0;\n+\n+\treturn fnmatch_icase(pattern, name, FNM_PATHNAME) == 0;\n }\n \n static int macroexpand_one(int attr_nr, int rem);\n@@ -696,7 +741,7 @@ static int fill(const char *path, int pathlen, const char *basename,\n \t\tif (a->is_macro)\n \t\t\tcontinue;\n \t\tif (path_matches(path, pathlen, basename,\n-\t\t\t\t a->u.pattern, base, stk->originlen))\n+\t\t\t\t &a->u.pat, base, stk->originlen))\n \t\t\trem = fill_one(\"fill\", a, rem);\n \t}\n \treturn rem;\ndiff --git a/dir.c b/dir.c\nindex 48aed85..cddf043 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -308,10 +308,10 @@ static int no_wildcard(const char *string)\n \treturn string[simple_length(string)] == '\\0';\n }\n \n-static void parse_exclude_pattern(const char **pattern,\n-\t\t\t\t int *patternlen,\n-\t\t\t\t int *flags,\n-\t\t\t\t int *nowildcardlen)\n+void parse_exclude_pattern(const char **pattern,\n+\t\t\t int *patternlen,\n+\t\t\t int *flags,\n+\t\t\t int *nowildcardlen)\n {\n \tconst char *p = *pattern;\n \tsize_t i, len;\ndiff --git a/dir.h b/dir.h\nindex 41ea32d..fd5c2aa 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -97,6 +97,7 @@ extern int path_excluded(struct path_exclude_check *, const char *, int namelen,\n extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,\n \t\t\t\t\t char **buf_p, struct exclude_list *which, int check_index);\n extern void add_excludes_from_file(struct dir_struct *, const char *fname);\n+extern void parse_exclude_pattern(const char **string, int *patternlen, int *flags, int *nowildcardlen);\n extern void add_exclude(const char *string, const char *base,\n \t\t\tint baselen, struct exclude_list *which);\n extern void free_excludes(struct exclude_list *el);\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 51f3045..4a1402f 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -242,4 +242,18 @@ test_expect_success 'bare repository: test info/attributes' '\n \tattr_check subdir/a/i unspecified\n '\n \n+test_expect_success 'leave bare' '\n+\tcd ..\n+'\n+\n+test_expect_success 'negative patterns' '\n+\techo \"!f test=bar\" >.gitattributes &&\n+\ttest_must_fail git check-attr test -- f\n+'\n+\n+test_expect_success 'patterns starting with exclamation' '\n+\techo \"\\!f test=foo\" >.gitattributes &&\n+\tattr_check \"!f\" foo\n+'\n+\n test_done\n-- \n1.8.0.rc0.29.g1fdd78f\n"},{"id":"200813","messageId":"7vd30si665.fsf@alter.siamese.dyndns.org","threadId":"31762","inReplyTo":"1349749445-25397-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH nd/attr-match-optim-more 2/2] attr: more matching optimizations from .gitignore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-09T05:08:18Z","receivedAt":"2012-10-09T05:08:18Z","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> .gitattributes and .gitignore share the same pattern syntax but\n> has separate matching implementation. Over the years, ignore's\n> implementation accumulates more optimizations while attr's stays\n> the same.\n>\n> This patch adds those optimizations to attr. Basically it tries to\n> avoid fnmatch as much as possible in favor of strncmp.\n>\n> A few notes from this work is put in the documentation:\n>\n> * \"!pattern\" syntax is not supported in .gitattributes. Negative\n> patterns may work well for a single attribute like .gitignore. It's\n> confusing in .gitattributes are many attributes can be\n> set/unset/undefined at using the same pattern.\n\nI think the above misses the point.\n\nImagine if we allowed only one attribute per line, instead of\nmultiple attributes on one line.\n \n - If you want to unset the attribute, you would write \"path -attr\".\n\n - If you want to reset the attribute to unspecified, you would\n write \"path !attr\".\n\nBoth are used in conjunction with some other (typically more\ngeneric) pattern that sets, sets to a value, and/or unsets the\nattribute, to countermand its effect.\n\nIf you were to allow \"!path attr\", what does it mean? It obviously\nis not about setting the attr to true or to a string value, but is\nit countermanding an earlier set and telling us to unset the attr,\nor make the attr unspecified?\n\nThat is the ambiguity issue \"!pattern\" syntax would introduce if it\nwere to be allowed in the attributes. I think \"multiple attributes\non the same line\" is a red herring.\n\n> * we support attaching attributes to directories at the syntax\n> level, but we do not really attach attributes on directory or use\n> them.\n\nI would say \"... but we do not currently use attributes on\ndirectories.\"\n\n> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n> index 80120ea..cc2ff1d 100644\n> --- a/Documentation/gitattributes.txt\n> +++ b/Documentation/gitattributes.txt\n> @@ -23,7 +23,7 @@ Each line in `gitattributes` file is of form:\n> That is, a pattern followed by an attributes list,\n> separated by whitespaces. When the pattern matches the\n> path in question, the attributes listed on the line are given to\n> -the path.\n> +the path. Only files can be attached attributes to.\n\nSymbolic links?\n\nI would strongly suggest dropping \"Only ... can be...\". You can\nspecify attributes to anything and check with \"check-attr\". It is\njust that core part does not have anything that pays attention to\nattributes given to directories in the current codebase.\n\n> @@ -56,6 +56,7 @@ When more than one pattern matches the path, a later line\n> overrides an earlier line. This overriding is done per\n> attribute. The rules how the pattern matches paths are the\n> same as in `.gitignore` files; see linkgit:gitignore[5].\n> +Unlike `.gitignore`, negative patterns are forbidden.\n\nOK (I am debating myself if it helps the readers if we said why it\nis forbidden to write such).\n\n> diff --git a/attr.c b/attr.c\n> index e7caee4..7e85f82 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -115,6 +115,13 @@ struct attr_state {\n> \tconst char *setto;\n> };\n> \n> +struct pattern {\n> +\tconst char *pattern;\n> +\tint patternlen;\n> +\tint nowildcardlen;\n> +\tint flags;\t\t/* EXC_FLAG_* */\n> +};\n> +\n> /*\n> * One rule, as from a .gitattributes file.\n> *\n> @@ -131,7 +138,7 @@ struct attr_state {\n> */\n> struct match_attr {\n> \tunion {\n> -\t\tchar *pattern;\n> +\t\tstruct pattern pat;\n> \t\tstruct git_attr *attr;\n> \t} u;\n> \tchar is_macro;\n> @@ -241,9 +248,17 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n> \tif (is_macro)\n> \t\tres->u.attr = git_attr_internal(name, namelen);\n> \telse {\n> -\t\tres->u.pattern = (char *)&(res->state[num_attr]);\n> -\t\tmemcpy(res->u.pattern, name, namelen);\n> -\t\tres->u.pattern[namelen] = 0;\n> +\t\tchar *p = (char *)&(res->state[num_attr]);\n> +\t\tmemcpy(p, name, namelen);\n> +\t\tp[namelen] = 0;\n\nThis is inherited from the original code, but *res is calloc(3)ed\nso the above memcpy() automatically NUL terminates this string.\n\n> +\t\tres->u.pat.pattern = p;\n> +\t\tparse_exclude_pattern(&res->u.pat.pattern,\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_NEGATIVE)\n> +\t\t\tdie(_(\"Negative patterns are forbidden in git attributes\\n\"\n> +\t\t\t \"Use '\\\\!' for literal leading exclamation.\"));\n> \t}\n> \tres->is_macro = is_macro;\n> \tres->num_attr = num_attr;\n> @@ -640,25 +655,55 @@ static void prepare_attr_stack(const char *path)\n> \n> static int path_matches(const char *pathname, int pathlen,\n> \t\t\tconst char *basename,\n> -\t\t\tconst char *pattern,\n> +\t\t\tconst struct pattern *pat,\n> \t\t\tconst char *base, int baselen)\n> {\n> -\tif (!strchr(pattern, '/')) {\n> +\tconst char *pattern = pat->pattern;\n> +\tint prefix = pat->nowildcardlen;\n> +\tconst char *name;\n> +\tint namelen;\n> +\n> +\tif (pat->flags & EXC_FLAG_NODIR) {\n> +\t\tif (prefix == pat->patternlen &&\n> +\t\t !strcmp_icase(pattern, basename))\n> +\t\t\treturn 1;\n\nAt some point, we should rename strcmp_icase and strncmp_icase to\nmake it clear that\n\n (1) they are not about \"strings\"; and\n (2) icase is not always in effect.\n\nThey are about comparing pathnames and that is the reason why\ndepending on core.ignorecase settings we sometimes do _icase()\ncomparison. The same issue is shared with fnmatch_icase() but\n\"fnmatch_\" prefix hints that the helper is not about matching\ngeneral strings so it is with lessor problem compared with other\ntwo.\n\n> +\t\tif (pat->flags & EXC_FLAG_ENDSWITH &&\n> +\t\t pat->patternlen - 1 <= pathlen &&\n> +\t\t !strcmp_icase(pattern + 1, pathname +\n> +\t\t\t\t pathlen - pat->patternlen + 1))\n> +\t\t\treturn 1;\n> +\n> \t\treturn (fnmatch_icase(pattern, basename, 0) == 0);\n> \t}\n> \t/*\n> \t * match with FNM_PATHNAME; the pattern has base implicitly\n> \t * in front of it.\n> \t */\n> -\tif (*pattern == '/')\n> +\tif (*pattern == '/') {\n> \t\tpattern++;\n> +\t\tprefix--;\n> +\t}\n> +\n> +\t/*\n> +\t * note: unlike excluded_from_list, baselen here does not\n> +\t * contain the trailing slash\n> +\t */\n> +\n> \tif (pathlen < baselen ||\n> \t (baselen && pathname[baselen] != '/') ||\n> \t strncmp(pathname, base, baselen))\n\nThis is probably strncmp_icase(), which is to be renamed to\nsomething more sensible.\n\n> \t\treturn 0;\n> -\tif (baselen != 0)\n> -\t\tbaselen++;\n> -\treturn fnmatch_icase(pattern, pathname + baselen, FNM_PATHNAME) == 0;\n> +\n> +\tnamelen = baselen ? pathlen - baselen - 1 : pathlen;\n\nI think this \"- 1\" is what the above \"note: unlike excluded_from...\"\nrefers to, but then isn't a same adjustment necessary to the \"if\"\ncondition we see above that compares pathlen and baselen???\n\n> +\tname = pathname + pathlen - namelen;\n> +\n> +\t/* if the non-wildcard part is longer than the remaining\n> +\t pathname, surely it cannot match */\n\nStyle.\n\n> +\tif (!namelen || prefix > namelen)\n> +\t\treturn 0;\n> +\n> +\treturn fnmatch_icase(pattern, name, FNM_PATHNAME) == 0;\n> }\n"},{"id":"200816","messageId":"5073BFDE.60707@viscovery.net","threadId":"31762","inReplyTo":"7vd30si665.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH nd/attr-match-optim-more 2/2] attr: more matching optimizations from .gitignore","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-09T06:10:38Z","receivedAt":"2012-10-09T06:10:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/9/2012 7:08, schrieb Junio C Hamano:\n> Imagine if we allowed only one attribute per line, instead of\n> multiple attributes on one line.\n> \n> - If you want to unset the attribute, you would write \"path -attr\".\n> \n> - If you want to reset the attribute to unspecified, you would\n> write \"path !attr\".\n> \n> Both are used in conjunction with some other (typically more\n> generic) pattern that sets, sets to a value, and/or unsets the\n> attribute, to countermand its effect.\n> \n> If you were to allow \"!path attr\", what does it mean? It obviously\n> is not about setting the attr to true or to a string value, but is\n> it countermanding an earlier set and telling us to unset the attr,\n> or make the attr unspecified?\n\nIf I have at the toplevel:\n\n *.txt whitespace=tabwidth=4\n\nand in a subdirectory\n\n *.txt whitespace=tabwidth=8\n !README.txt\n\nit could be interpreted as \"do not apply *.txt to REAME.txt in this\nsubdirectory\". That is, it does not countermand some _particular_\nattribute setting, but says \"use the attributes collected elsewhere\".\n\n-- Hannes\n"},{"id":"200819","messageId":"7vvcekgn0h.fsf@alter.siamese.dyndns.org","threadId":"31762","inReplyTo":"5073BFDE.60707@viscovery.net","subject":"Re: [PATCH nd/attr-match-optim-more 2/2] attr: more matching optimizations from .gitignore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-09T06:47:26Z","receivedAt":"2012-10-09T06:47:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 10/9/2012 7:08, schrieb Junio C Hamano:\n>> Imagine if we allowed only one attribute per line, instead of\n>> multiple attributes on one line.\n>> \n>> - If you want to unset the attribute, you would write \"path -attr\".\n>> \n>> - If you want to reset the attribute to unspecified, you would\n>> write \"path !attr\".\n>> \n>> Both are used in conjunction with some other (typically more\n>> generic) pattern that sets, sets to a value, and/or unsets the\n>> attribute, to countermand its effect.\n>> \n>> If you were to allow \"!path attr\", what does it mean? It obviously\n>> is not about setting the attr to true or to a string value, but is\n>> it countermanding an earlier set and telling us to unset the attr,\n>> or make the attr unspecified?\n>\n> If I have at the toplevel:\n>\n> *.txt whitespace=tabwidth=4\n>\n> and in a subdirectory\n>\n> *.txt whitespace=tabwidth=8\n> !README.txt\n>\n> it could be interpreted as \"do not apply *.txt to REAME.txt in this\n> subdirectory\". That is, it does not countermand some _particular_\n> attribute setting, but says \"use the attributes collected elsewhere\".\n\nIt makes it unclear what \"elsewhere\" means, though (besides, it does\nnot match the way the matching logic works at all).\n"},{"id":"200840","messageId":"7vhaq3ha4r.fsf@alter.siamese.dyndns.org","threadId":"31762","inReplyTo":"7vvcekgn0h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH nd/attr-match-optim-more 2/2] attr: more matching optimizations from .gitignore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-09T16:40:20Z","receivedAt":"2012-10-09T16:40:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Sixt writes:\n>\n>> Am 10/9/2012 7:08, schrieb Junio C Hamano:\n>>> Imagine if we allowed only one attribute per line, instead of\n>>> multiple attributes on one line.\n>>> \n>>> - If you want to unset the attribute, you would write \"path -attr\".\n>>> \n>>> - If you want to reset the attribute to unspecified, you would\n>>> write \"path !attr\".\n>>> \n>>> Both are used in conjunction with some other (typically more\n>>> generic) pattern that sets, sets to a value, and/or unsets the\n>>> attribute, to countermand its effect.\n>>> \n>>> If you were to allow \"!path attr\", what does it mean? It obviously\n>>> is not about setting the attr to true or to a string value, but is\n>>> it countermanding an earlier set and telling us to unset the attr,\n>>> or make the attr unspecified?\n>>\n>> If I have at the toplevel:\n>>\n>> *.txt whitespace=tabwidth=4\n>>\n>> and in a subdirectory\n>>\n>> *.txt whitespace=tabwidth=8\n>> !README.txt\n>>\n>> it could be interpreted as \"do not apply *.txt to REAME.txt in this\n>> subdirectory\". That is, it does not countermand some _particular_\n>> attribute setting, but says \"use the attributes collected elsewhere\".\n>\n> It makes it unclear what \"elsewhere\" means, though (besides, it does\n> not match the way the matching logic works at all).\n\nIgnoring the current implementation, I find the suggested semantics\nsomewhat intriguing. It is something we may want to look into in\nthe future.\n"},{"id":"200895","messageId":"1349864466-28289-1-git-send-email-pclouds@gmail.com","threadId":"31762","inReplyTo":"7vd30si665.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2 2/2] attr: more matching optimizations from .gitignore","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-10T10:21:06Z","receivedAt":"2012-10-10T10:21:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":".gitattributes and .gitignore share the same pattern syntax but has\nseparate matching implementation. Over the years, ignore's\nimplementation accumulates more optimizations while attr's stays the\nsame.\n\nThis patch adds those optimizations to attr. Basically it tries to\navoid fnmatch as much as possible in favor of strncmp.\n\nA few notes from this work is put in the documentation:\n\n* \"!pattern\" syntax is not supported in .gitattributes as it's not\n clear what it means (e.g. \"!path attr\" is about unsetting attr, or\n undefining it..)\n\n* patterns applying to directories\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n How about this? Diff from the previous version:\n\n diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n index cc2ff1d..9a0ed19 100644\n --- a/Documentation/gitattributes.txt\n +++ b/Documentation/gitattributes.txt\n @@ -23,7 +23,7 @@ Each line in `gitattributes` file is of form:\n That is, a pattern followed by an attributes list,\n separated by whitespaces. When the pattern matches the\n path in question, the attributes listed on the line are given to\n -the path. Only files can be attached attributes to.\n +the path.\n \n Each attribute can be in one of these states for a given path:\n \n @@ -58,6 +58,13 @@ attribute. The rules how the pattern matches paths are the\n same as in `.gitignore` files; see linkgit:gitignore[5].\n Unlike `.gitignore`, negative patterns are not supported.\n \n +Note that if a .gitignore rule matches a directory, the directory\n +is ignored, which may be seen as assigning \"ignore\" attribute the\n +directory and all files and directories inside. However, if a\n +.gitattributes rule matches a directory, it manipulates\n +attributes on that directory only, not files and directories\n +inside.\n +\n When deciding what attributes are assigned to a path, git\n consults `$GIT_DIR/info/attributes` file (which has the highest\n precedence), `.gitattributes` file in the same directory as the\n diff --git a/attr.c b/attr.c\n index 7e85f82..4faf1ff 100644\n --- a/attr.c\n +++ b/attr.c\n @@ -250,7 +250,6 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \telse {\n \t\tchar *p = (char *)&(res->state[num_attr]);\n \t\tmemcpy(p, name, namelen);\n -\t\tp[namelen] = 0;\n \t\tres->u.pat.pattern = p;\n \t\tparse_exclude_pattern(&res->u.pat.pattern,\n \t\t\t\t &res->u.pat.patternlen,\n @@ -690,16 +689,18 @@ static int path_matches(const char *pathname, int pathlen,\n \t * contain the trailing slash\n \t */\n \n -\tif (pathlen < baselen ||\n +\tif (pathlen < baselen + 1 ||\n \t (baselen && pathname[baselen] != '/') ||\n -\t strncmp(pathname, base, baselen))\n +\t strncmp_icase(pathname, base, baselen))\n \t\treturn 0;\n \n \tnamelen = baselen ? pathlen - baselen - 1 : pathlen;\n \tname = pathname + pathlen - namelen;\n \n -\t/* if the non-wildcard part is longer than the remaining\n -\t pathname, surely it cannot match */\n +\t/*\n +\t * if the non-wildcard part is longer than the remaining\n +\t * pathname, surely it cannot match\n +\t */\n \tif (!namelen || prefix > namelen)\n \t\treturn 0;\n \n\n Documentation/gitattributes.txt | 8 +++++\n attr.c | 72 +++++++++++++++++++++++++++++++++--------\n dir.c | 8 ++---\n dir.h | 1 +\n t/t0003-attributes.sh | 14 ++++++++\n 5 files changed, 86 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 80120ea..9a0ed19 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -56,6 +56,14 @@ When more than one pattern matches the path, a later line\n overrides an earlier line. This overriding is done per\n attribute. The rules how the pattern matches paths are the\n same as in `.gitignore` files; see linkgit:gitignore[5].\n+Unlike `.gitignore`, negative patterns are not supported.\n+\n+Note that if a .gitignore rule matches a directory, the directory\n+is ignored, which may be seen as assigning \"ignore\" attribute the\n+directory and all files and directories inside. However, if a\n+.gitattributes rule matches a directory, it manipulates\n+attributes on that directory only, not files and directories\n+inside.\n \n When deciding what attributes are assigned to a path, git\n consults `$GIT_DIR/info/attributes` file (which has the highest\ndiff --git a/attr.c b/attr.c\nindex e7caee4..4faf1ff 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -115,6 +115,13 @@ struct attr_state {\n \tconst char *setto;\n };\n \n+struct pattern {\n+\tconst char *pattern;\n+\tint patternlen;\n+\tint nowildcardlen;\n+\tint flags;\t\t/* EXC_FLAG_* */\n+};\n+\n /*\n * One rule, as from a .gitattributes file.\n *\n@@ -131,7 +138,7 @@ struct attr_state {\n */\n struct match_attr {\n \tunion {\n-\t\tchar *pattern;\n+\t\tstruct pattern pat;\n \t\tstruct git_attr *attr;\n \t} u;\n \tchar is_macro;\n@@ -241,9 +248,16 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \tif (is_macro)\n \t\tres->u.attr = git_attr_internal(name, namelen);\n \telse {\n-\t\tres->u.pattern = (char *)&(res->state[num_attr]);\n-\t\tmemcpy(res->u.pattern, name, namelen);\n-\t\tres->u.pattern[namelen] = 0;\n+\t\tchar *p = (char *)&(res->state[num_attr]);\n+\t\tmemcpy(p, name, namelen);\n+\t\tres->u.pat.pattern = p;\n+\t\tparse_exclude_pattern(&res->u.pat.pattern,\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_NEGATIVE)\n+\t\t\tdie(_(\"Negative patterns are forbidden in git attributes\\n\"\n+\t\t\t \"Use '\\\\!' for literal leading exclamation.\"));\n \t}\n \tres->is_macro = is_macro;\n \tres->num_attr = num_attr;\n@@ -640,25 +654,57 @@ static void prepare_attr_stack(const char *path)\n \n static int path_matches(const char *pathname, int pathlen,\n \t\t\tconst char *basename,\n-\t\t\tconst char *pattern,\n+\t\t\tconst struct pattern *pat,\n \t\t\tconst char *base, int baselen)\n {\n-\tif (!strchr(pattern, '/')) {\n+\tconst char *pattern = pat->pattern;\n+\tint prefix = pat->nowildcardlen;\n+\tconst char *name;\n+\tint namelen;\n+\n+\tif (pat->flags & EXC_FLAG_NODIR) {\n+\t\tif (prefix == pat->patternlen &&\n+\t\t !strcmp_icase(pattern, basename))\n+\t\t\treturn 1;\n+\n+\t\tif (pat->flags & EXC_FLAG_ENDSWITH &&\n+\t\t pat->patternlen - 1 <= pathlen &&\n+\t\t !strcmp_icase(pattern + 1, pathname +\n+\t\t\t\t pathlen - pat->patternlen + 1))\n+\t\t\treturn 1;\n+\n \t\treturn (fnmatch_icase(pattern, basename, 0) == 0);\n \t}\n \t/*\n \t * match with FNM_PATHNAME; the pattern has base implicitly\n \t * in front of it.\n \t */\n-\tif (*pattern == '/')\n+\tif (*pattern == '/') {\n \t\tpattern++;\n-\tif (pathlen < baselen ||\n+\t\tprefix--;\n+\t}\n+\n+\t/*\n+\t * note: unlike excluded_from_list, baselen here does not\n+\t * contain the trailing slash\n+\t */\n+\n+\tif (pathlen < baselen + 1 ||\n \t (baselen && pathname[baselen] != '/') ||\n-\t strncmp(pathname, base, baselen))\n+\t strncmp_icase(pathname, base, baselen))\n+\t\treturn 0;\n+\n+\tnamelen = baselen ? pathlen - baselen - 1 : pathlen;\n+\tname = pathname + pathlen - namelen;\n+\n+\t/*\n+\t * if the non-wildcard part is longer than the remaining\n+\t * pathname, surely it cannot match\n+\t */\n+\tif (!namelen || prefix > namelen)\n \t\treturn 0;\n-\tif (baselen != 0)\n-\t\tbaselen++;\n-\treturn fnmatch_icase(pattern, pathname + baselen, FNM_PATHNAME) == 0;\n+\n+\treturn fnmatch_icase(pattern, name, FNM_PATHNAME) == 0;\n }\n \n static int macroexpand_one(int attr_nr, int rem);\n@@ -696,7 +742,7 @@ static int fill(const char *path, int pathlen, const char *basename,\n \t\tif (a->is_macro)\n \t\t\tcontinue;\n \t\tif (path_matches(path, pathlen, basename,\n-\t\t\t\t a->u.pattern, base, stk->originlen))\n+\t\t\t\t &a->u.pat, base, stk->originlen))\n \t\t\trem = fill_one(\"fill\", a, rem);\n \t}\n \treturn rem;\ndiff --git a/dir.c b/dir.c\nindex 48aed85..cddf043 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -308,10 +308,10 @@ static int no_wildcard(const char *string)\n \treturn string[simple_length(string)] == '\\0';\n }\n \n-static void parse_exclude_pattern(const char **pattern,\n-\t\t\t\t int *patternlen,\n-\t\t\t\t int *flags,\n-\t\t\t\t int *nowildcardlen)\n+void parse_exclude_pattern(const char **pattern,\n+\t\t\t int *patternlen,\n+\t\t\t int *flags,\n+\t\t\t int *nowildcardlen)\n {\n \tconst char *p = *pattern;\n \tsize_t i, len;\ndiff --git a/dir.h b/dir.h\nindex 41ea32d..fd5c2aa 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -97,6 +97,7 @@ extern int path_excluded(struct path_exclude_check *, const char *, int namelen,\n extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,\n \t\t\t\t\t char **buf_p, struct exclude_list *which, int check_index);\n extern void add_excludes_from_file(struct dir_struct *, const char *fname);\n+extern void parse_exclude_pattern(const char **string, int *patternlen, int *flags, int *nowildcardlen);\n extern void add_exclude(const char *string, const char *base,\n \t\t\tint baselen, struct exclude_list *which);\n extern void free_excludes(struct exclude_list *el);\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 51f3045..4a1402f 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -242,4 +242,18 @@ test_expect_success 'bare repository: test info/attributes' '\n \tattr_check subdir/a/i unspecified\n '\n \n+test_expect_success 'leave bare' '\n+\tcd ..\n+'\n+\n+test_expect_success 'negative patterns' '\n+\techo \"!f test=bar\" >.gitattributes &&\n+\ttest_must_fail git check-attr test -- f\n+'\n+\n+test_expect_success 'patterns starting with exclamation' '\n+\techo \"\\!f test=foo\" >.gitattributes &&\n+\tattr_check \"!f\" foo\n+'\n+\n test_done\n-- \n1.7.12.1.406.g6ab07c4\n"},{"id":"200947","messageId":"7vpq4qaycn.fsf@alter.siamese.dyndns.org","threadId":"31762","inReplyTo":"1349864466-28289-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 2/2] attr: more matching optimizations from .gitignore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-10T20:03:36Z","receivedAt":"2012-10-10T20:03: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> .gitattributes and .gitignore share the same pattern syntax but has\n> separate matching implementation. Over the years, ignore's\n> implementation accumulates more optimizations while attr's stays the\n> same.\n>\n> This patch adds those optimizations to attr. Basically it tries to\n> avoid fnmatch as much as possible in favor of strncmp.\n>\n> A few notes from this work is put in the documentation:\n>\n> * \"!pattern\" syntax is not supported in .gitattributes as it's not\n\ns/not supported/forbidden/;\n\nA reader can take \"not supported\" as \"silently ignored\", which is\nnot the case. An explicit \"forbidden\" does not have such a\nmisinterpretation.\n\nIt would save time from both of us if you can check what is queued\non 'pu'. I do not think I touched the code for off-by-one bugs\nthere, though.\n\n> clear what it means (e.g. \"!path attr\" is about unsetting attr, or\n> undefining it..)\n>\n> * patterns applying to directories\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> How about this? Diff from the previous version:\n>\n> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n> index cc2ff1d..9a0ed19 100644\n> --- a/Documentation/gitattributes.txt\n> +++ b/Documentation/gitattributes.txt\n> @@ -23,7 +23,7 @@ Each line in `gitattributes` file is of form:\n> That is, a pattern followed by an attributes list,\n> separated by whitespaces. When the pattern matches the\n> path in question, the attributes listed on the line are given to\n> -the path. Only files can be attached attributes to.\n> +the path.\n> \n> Each attribute can be in one of these states for a given path:\n> \n> @@ -58,6 +58,13 @@ attribute. The rules how the pattern matches paths are the\n> same as in `.gitignore` files; see linkgit:gitignore[5].\n> Unlike `.gitignore`, negative patterns are not supported.\n> \n> +Note that if a .gitignore rule matches a directory, the directory\n> +is ignored, which may be seen as assigning \"ignore\" attribute the\n> +directory and all files and directories inside. However, if a\n> +.gitattributes rule matches a directory, it manipulates\n> +attributes on that directory only, not files and directories\n> +inside.\n\nWhy do you even need to mention .gitignore in gitattributes manual\nwhere it is irrelevant from the reader's point of view?\n\nBesides, the interpretation the \"may be seen as\" suggests is\nactively wrong. It is assigning \"ignore-this-and-below\" attribute\nto the directory, and there is no inconsistency between the two.\n\nAgain, I'd suggest dropping this addition.\n\n> diff --git a/attr.c b/attr.c\n> index 7e85f82..4faf1ff 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -250,7 +250,6 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n> \telse {\n> \t\tchar *p = (char *)&(res->state[num_attr]);\n> \t\tmemcpy(p, name, namelen);\n> -\t\tp[namelen] = 0;\n> \t\tres->u.pat.pattern = p;\n> \t\tparse_exclude_pattern(&res->u.pat.pattern,\n> \t\t\t\t &res->u.pat.patternlen,\n> @@ -690,16 +689,18 @@ static int path_matches(const char *pathname, int pathlen,\n> \t * contain the trailing slash\n> \t */\n> \n> -\tif (pathlen < baselen ||\n> +\tif (pathlen < baselen + 1 ||\n> \t (baselen && pathname[baselen] != '/') ||\n> -\t strncmp(pathname, base, baselen))\n> +\t strncmp_icase(pathname, base, baselen))\n> \t\treturn 0;\n> \n> \tnamelen = baselen ? pathlen - baselen - 1 : pathlen;\n> \tname = pathname + pathlen - namelen;\n> \n> -\t/* if the non-wildcard part is longer than the remaining\n> -\t pathname, surely it cannot match */\n> +\t/*\n> +\t * if the non-wildcard part is longer than the remaining\n> +\t * pathname, surely it cannot match\n> +\t */\n> \tif (!namelen || prefix > namelen)\n> \t\treturn 0;\n"},{"id":"200956","messageId":"7v626iatt9.fsf@alter.siamese.dyndns.org","threadId":"31762","inReplyTo":"1349864466-28289-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 2/2] attr: more matching optimizations from .gitignore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-10T21:41:38Z","receivedAt":"2012-10-10T21:41:38Z","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> @@ -690,16 +689,18 @@ static int path_matches(const char *pathname, int pathlen,\n> \t * contain the trailing slash\n> \t */\n> \n> -\tif (pathlen < baselen ||\n> +\tif (pathlen < baselen + 1 ||\n> \t (baselen && pathname[baselen] != '/') ||\n> -\t strncmp(pathname, base, baselen))\n> +\t strncmp_icase(pathname, base, baselen))\n\nShouldn't the last comparison be\n\n\tstrncmp_icase(pathname, base, baselen + 1)\n\ninstead, if you are trying to match this part from dir.c where\nbaselen does count the trailing slash?\n\n\t\tif (pathlen < x->baselen ||\n\t\t (x->baselen && pathname[x->baselen-1] != '/') ||\n\t\t strncmp_icase(pathname, x->base, x->baselen))\n\t\t\tcontinue;\n\nIn other words, relative to what was queued to 'pu', something like\nthis instead....\n\n-- >8 --\nSubject: [PATCH] fixup: matching path_matches() in attr.c to that of dir.c\n\nIn this function, baselen does not count the trailing slash that\nshould come after the directory name held in \"basename\" variable, so\nwhenever the corresponding code in dir.c:excluded_from_list() refers\nto x->baselen, we would need to use \"baselen+1\" consistently.\n\nAlso remove unnecessary NUL-termination of a buffer obtained from\ncalloc().\n\nSigned-off-by: Junio C Hamano \n---\n attr.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 7e85f82..a9c04a8 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -250,7 +250,6 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \telse {\n \t\tchar *p = (char *)&(res->state[num_attr]);\n \t\tmemcpy(p, name, namelen);\n-\t\tp[namelen] = 0;\n \t\tres->u.pat.pattern = p;\n \t\tparse_exclude_pattern(&res->u.pat.pattern,\n \t\t\t\t &res->u.pat.patternlen,\n@@ -687,19 +686,21 @@ static int path_matches(const char *pathname, int pathlen,\n \n \t/*\n \t * note: unlike excluded_from_list, baselen here does not\n-\t * contain the trailing slash\n+\t * count the trailing slash.\n \t */\n \n-\tif (pathlen < baselen ||\n+\tif (pathlen < baselen + 1 ||\n \t (baselen && pathname[baselen] != '/') ||\n-\t strncmp(pathname, base, baselen))\n+\t strncmp_icase(pathname, base, baselen + 1))\n \t\treturn 0;\n \n \tnamelen = baselen ? pathlen - baselen - 1 : pathlen;\n \tname = pathname + pathlen - namelen;\n \n-\t/* if the non-wildcard part is longer than the remaining\n-\t pathname, surely it cannot match */\n+\t/*\n+\t * if the non-wildcard part is longer than the remaining\n+\t * pathname, surely it cannot match.\n+\t */\n \tif (!namelen || prefix > namelen)\n \t\treturn 0;\n \n-- \n1.8.0.rc1.76.g5a375e6\n"},{"id":"200959","messageId":"7vwqyy9etf.fsf@alter.siamese.dyndns.org","threadId":"31762","inReplyTo":"7v626iatt9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] attr: more matching optimizations from .gitignore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-10T21:50:52Z","receivedAt":"2012-10-10T21:50:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> @@ -690,16 +689,18 @@ static int path_matches(const char *pathname, int pathlen,\n>> \t * contain the trailing slash\n>> \t */\n>> \n>> -\tif (pathlen < baselen ||\n>> +\tif (pathlen < baselen + 1 ||\n>> \t (baselen && pathname[baselen] != '/') ||\n>> -\t strncmp(pathname, base, baselen))\n>> +\t strncmp_icase(pathname, base, baselen))\n>\n> Shouldn't the last comparison be\n>\n> \tstrncmp_icase(pathname, base, baselen + 1)\n>\n> instead, if you are trying to match this part from dir.c where\n> baselen does count the trailing slash?\n>\n> \t\tif (pathlen < x->baselen ||\n> \t\t (x->baselen && pathname[x->baselen-1] != '/') ||\n> \t\t strncmp_icase(pathname, x->base, x->baselen))\n> \t\t\tcontinue;\n>\n> In other words, relative to what was queued to 'pu', something like\n> this instead....\n\nAaaaand,... it doesn't work and breaks t0003.sh. Sigh...\n"},{"id":"200965","messageId":"CACsJy8CdCN0hBU2WwffNvHOwcC87v2OVJj=SYH4BYYveBpFiQw@mail.gmail.com","threadId":"31762","inReplyTo":"7v626iatt9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] attr: more matching optimizations from .gitignore","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-11T01:36:50Z","receivedAt":"2012-10-11T01:36:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 11, 2012 at 4:41 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> @@ -690,16 +689,18 @@ static int path_matches(const char *pathname, int pathlen,\n>> * contain the trailing slash\n>> */\n>>\n>> - if (pathlen < baselen ||\n>> + if (pathlen < baselen + 1 ||\n>> (baselen && pathname[baselen] != '/') ||\n>> - strncmp(pathname, base, baselen))\n>> + strncmp_icase(pathname, base, baselen))\n>\n> Shouldn't the last comparison be\n>\n> strncmp_icase(pathname, base, baselen + 1)\n>\n> instead,\n\n\"base\" does not contain the trailing slash, so it can only match up to\nbase[baselen-1], then fail at base[baselen], which is '\\0'. The \"no\ntrailing slash\" business in this function is tricky :(\n\n> if you are trying to match this part from dir.c where\n> baselen does count the trailing slash?\n>\n> if (pathlen < x->baselen ||\n> (x->baselen && pathname[x->baselen-1] != '/') ||\n> strncmp_icase(pathname, x->base, x->baselen))\n> continue;\n\nstrncmp_icase() here just needs to compare x->baselen-1 chars (i.e. no\ntrailing slash) as the trailing slash is explicitly checked just above\nstrncmp_icase. But it does not hurt to compare an extra character so I\nleave it unchanged. But obviously it causes confusion when we try to\nmatch this function and the one in attr.c\n-- \nDuy\n"},{"id":"201051","messageId":"CACsJy8DkF2XgwPbW=FVTi46hnhk5UmpeHy6ETotomo3e+-Huww@mail.gmail.com","threadId":"31762","inReplyTo":"7vpq4qaycn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] attr: more matching optimizations from .gitignore","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-12T10:13:26Z","receivedAt":"2012-10-12T10:13:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 11, 2012 at 3:03 AM, Junio C Hamano wrote:\n> It would save time from both of us if you can check what is queued\n> on 'pu'. I do not think I touched the code for off-by-one bugs\n> there, though.\n\n'pu' looks good.\n-- \nDuy\n"},{"id":"201072","messageId":"7vy5jb33sq.fsf@alter.siamese.dyndns.org","threadId":"31762","inReplyTo":"1349864466-28289-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 2/2] attr: more matching optimizations from .gitignore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-12T19:09:57Z","receivedAt":"2012-10-12T19:09:57Z","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> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\n> index 51f3045..4a1402f 100755\n> --- a/t/t0003-attributes.sh\n> +++ b/t/t0003-attributes.sh\n> @@ -242,4 +242,18 @@ test_expect_success 'bare repository: test info/attributes' '\n> \tattr_check subdir/a/i unspecified\n> '\n> \n> +test_expect_success 'leave bare' '\n> +\tcd ..\n> +'\n> +\n> +test_expect_success 'negative patterns' '\n> +\techo \"!f test=bar\" >.gitattributes &&\n> +\ttest_must_fail git check-attr test -- f\n> +'\n> +\n> +test_expect_success 'patterns starting with exclamation' '\n> +\techo \"\\!f test=foo\" >.gitattributes &&\n> +\tattr_check \"!f\" foo\n> +'\n> +\n> test_done\n\nThis is not entirely your fault, but please don't do that \"cd ..\".\n\nThe original test had \"cd bare\", made an assumption that step will\nnever fail (which is mostly correct), and ran everything afterward\nin that subdirectory.\n\nAdding \"Do a 'cd ..' to come back\" is a horrible way to build on\nit. Imagine what happens when another person also did the same\nthing, and both changes need to be merged. You will end up going up\ntwo levels, which is not what you want.\n\nI think the right fix is to make each of the test that wants to run\nin \"bare\" chdir in its own subshell, or not append these new tests\nthat do not run in the \"bare\" to the end of this file, but before\nthe execution goes down to \"bare\".\n"},{"id":"201080","messageId":"20121013043259.GA7011@do","threadId":"31762","inReplyTo":"7vy5jb33sq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] attr: more matching optimizations from .gitignore","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-13T04:32:59Z","receivedAt":"2012-10-13T04:32:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Oct 12, 2012 at 12:09:57PM -0700, Junio C Hamano wrote:\n> This is not entirely your fault, but please don't do that \"cd ..\".\n> \n> The original test had \"cd bare\", made an assumption that step will\n> never fail (which is mostly correct), and ran everything afterward\n> in that subdirectory.\n> \n> Adding \"Do a 'cd ..' to come back\" is a horrible way to build on\n> it. Imagine what happens when another person also did the same\n> thing, and both changes need to be merged. You will end up going up\n> two levels, which is not what you want.\n> \n> I think the right fix is to make each of the test that wants to run\n> in \"bare\" chdir in its own subshell, or not append these new tests\n> that do not run in the \"bare\" to the end of this file, but before\n> the execution goes down to \"bare\".\n> \n\nThe reason I put these tests at the end was because I destroy\n.gitattributes and it might affect the following tests. But obviously\nthe bare tests run in its own repository (and I have not committed\nanything till the repo is cloned) so my .gitattributes changes can't\naffect them.\n\nPlease squash this in\n\n-- 8< --\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 4a1402f..f6c21ea 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -206,6 +206,16 @@ test_expect_success 'root subdir attribute test' '\n \tattr_check subdir/a/i unspecified\n '\n \n+test_expect_success 'negative patterns' '\n+\techo \"!f test=bar\" >.gitattributes &&\n+\ttest_must_fail git check-attr test -- f\n+'\n+\n+test_expect_success 'patterns starting with exclamation' '\n+\techo \"\\!f test=foo\" >.gitattributes &&\n+\tattr_check \"!f\" foo\n+'\n+\n test_expect_success 'setup bare' '\n \tgit clone --bare . bare.git &&\n \tcd bare.git\n@@ -242,18 +252,4 @@ test_expect_success 'bare repository: test info/attributes' '\n \tattr_check subdir/a/i unspecified\n '\n \n-test_expect_success 'leave bare' '\n-\tcd ..\n-'\n-\n-test_expect_success 'negative patterns' '\n-\techo \"!f test=bar\" >.gitattributes &&\n-\ttest_must_fail git check-attr test -- f\n-'\n-\n-test_expect_success 'patterns starting with exclamation' '\n-\techo \"\\!f test=foo\" >.gitattributes &&\n-\tattr_check \"!f\" foo\n-'\n-\n test_done\n-- 8< --\n-- \nDuy\n"}]}