{"thread":{"id":"42373","subject":"[PATCHv7 2/5] Documentation: fix a typo","startedAt":"2016-05-18T19:02:17Z","lastAt":"2016-05-18T22:59:20Z","messageCount":17,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"286911","messageId":"20160518190222.28105-1-sbeller@google.com","threadId":"42373","inReplyTo":null,"subject":"[PATCHv7 0/5] pathspec magic extension to search for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-18T19:02:17Z","receivedAt":"2016-05-18T19:02:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This goes on top of origin/jc/attr, (8195c5cde01, SQUASH???, 2016-05-17 14:08:50)\n\nPatches 1 and 2 are small fixes, which could go independently as well.\n(1 is new compared to v6)\n\nPatches 3 and 4 are refactoring pathspec.c a little.\n\nThe last patch is the most exciting patch as it adds the new feature.\nQuoting from the documentation update:\n\n        attr;;\n                Additionally to matching the pathspec, the path must have the\n                attribute as specified. The syntax for specifying the required\n                attributes is \"`attr: [mode] <attribute name> [=value]`\"\n        +\n        Attributes can have 4 states (Set, Unset, Set to a value, unspecified) and\n        you can query each attribute for certain states. The \"`[mode]`\" is a special\n        character to indicate which attribute states are looked for. The following\n        modes are available:\n        \n        - \"`+`\" the attribute must be set\n        - \"`-`\" the attribute must be unset\n        - \"`~`\" the attribute must be unspecified\n        - \"`?`\" the attribute must not be unspecified, i.e. set, unset or value matches\n        - an empty \"`[mode]`\" matches if the attribute is set or has a value\n        - an empty \"`[mode]`\" combined with \"`[=value]`\" matches if the attribute has\n          the given value.\n        +\n\nWhile I followed Junios suggestion of the data layout in pathspec.h,\nI am not quite happy with it yet as I think it will not be easy in the future\nto enhance search for specific attribute values. Quoting from the tests:\n\n        test_expect_success 'check label with 2 labels' '\n                cat <<EOF >expect &&\n        fileAB     # has both labels\n        sub/fileAB # has both labels\n        EOF\n                git ls-files \":(attr:labelA labelB)\" >actual &&\n                test_cmp expect actual &&\n                git ls-files \":(attr:labelA,attr:labelB)\" >actual &&\n                test_cmp expect actual\n        '\n\nYou can see that two notations are possible to narrow the search down when\nasking for 2 attributes to be set. Either you can do 2 \"attr:\" magic strings\nor you can have one \"attr:\" and then a space separated list of attributes.\n\nThe problem arises once you'd want to have a better search for values, as\ncurrently only \":(attr:label=value) is supported with \"value\" being the\nexact match, e.g. a path having more than just value would not match as it is\nnot exact. E.g. in git.git we have\n\n        $ cat .gitattributes \n        * whitespace=!indent,trail,space\n        *.[ch] whitespace=indent,trail,space\n        *.sh whitespace=indent,trail,space\n        $ git ls-files \":(attr:whitespace=indent)\"\n        $ # no match!\n\nSo instead of matching exactly we'd want to just look for the value being\none part of the comma separated list as setup in .gitattributes.\nHowever searching for more than one value will be complicated, as we'd then\nneed to escape commas. My vision was to use white spaces instead, but they\nare now taken to separate different attributes.\n\nSo in the future I would imagine to have escaping like \n\n        git ls-files \":(attr:label=value\\,value2 labelA=value3,exclude)\n        \nor yet another separator like\n\n        git ls-files \":(attr:label=value1;value2 labelA=value3,exclude)\n\nThanks,\nStefan\n\n        \nStefan Beller (5):\n  string list: improve comment\n  Documentation: fix a typo\n  pathspec: move long magic parsing out of prefix_pathspec\n  pathspec: move prefix check out of the inner loop\n  pathspec: allow querying for attributes\n\n Documentation/gitattributes.txt    |   2 +-\n Documentation/glossary-content.txt |  19 ++++\n attr.c                             |   2 +-\n attr.h                             |   2 +\n dir.c                              |  46 ++++++++++\n pathspec.c                         | 182 +++++++++++++++++++++++++++++--------\n pathspec.h                         |  16 ++++\n string-list.h                      |   2 +-\n t/t6134-pathspec-with-labels.sh    | 177 ++++++++++++++++++++++++++++++++++++\n 9 files changed, 407 insertions(+), 41 deletions(-)\n create mode 100755 t/t6134-pathspec-with-labels.sh\n\n-- \n2.8.2.121.ga97fb08\n"},{"id":"286910","messageId":"20160518190222.28105-2-sbeller@google.com","threadId":"42373","inReplyTo":"20160518190222.28105-1-sbeller@google.com","subject":"[PATCHv7 1/5] string list: improve comment","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-18T19:02:18Z","receivedAt":"2016-05-18T19:02:18Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n string-list.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/string-list.h b/string-list.h\nindex d3809a1..465a1f0 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -106,7 +106,7 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_\n  * list->strdup_strings must be set, as new memory needs to be\n  * allocated to hold the substrings.  If maxsplit is non-negative,\n  * then split at most maxsplit times.  Return the number of substrings\n- * appended to list.\n+ * appended to list. The list may be non-empty already.\n  *\n  * Examples:\n  *   string_list_split(l, \"foo:bar:baz\", ':', -1) -> [\"foo\", \"bar\", \"baz\"]\n-- \n2.8.2.121.ga97fb08\n"},{"id":"286909","messageId":"20160518190222.28105-3-sbeller@google.com","threadId":"42373","inReplyTo":"20160518190222.28105-1-sbeller@google.com","subject":"[PATCHv7 2/5] Documentation: fix a typo","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-18T19:02:19Z","receivedAt":"2016-05-18T19:02:19Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/gitattributes.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex e3b1de8..af2c682 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -86,7 +86,7 @@ is either not set or empty, $HOME/.config/git/attributes is used instead.\n Attributes for all users on a system should be placed in the\n `$(prefix)/etc/gitattributes` file.\n \n-Sometimes you would need to override an setting of an attribute\n+Sometimes you would need to override a setting of an attribute\n for a path to `Unspecified` state.  This can be done by listing\n the name of the attribute prefixed with an exclamation point `!`.\n \n-- \n2.8.2.121.ga97fb08\n"},{"id":"286913","messageId":"20160518190222.28105-4-sbeller@google.com","threadId":"42373","inReplyTo":"20160518190222.28105-1-sbeller@google.com","subject":"[PATCHv7 3/5] pathspec: move long magic parsing out of prefix_pathspec","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-18T19:02:20Z","receivedAt":"2016-05-18T19:02:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`prefix_pathspec` is quite a lengthy function and we plan on adding more.\nSplit it up for better readability. As we want to add code into the\ninner loop of the long magic parsing, we also benefit from lower\nindentation.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n pathspec.c | 84 +++++++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 47 insertions(+), 37 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex c9e9b6c..eba37c2 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -88,6 +88,52 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+static void eat_long_magic(struct pathspec_item *item, const char *elt,\n+\t\tunsigned *magic, int *pathspec_prefix,\n+\t\tconst char **copyfrom_, const char **long_magic_end)\n+{\n+\tint i;\n+\tconst char *copyfrom = *copyfrom_;\n+\t/* longhand */\n+\tconst char *nextat;\n+\tfor (copyfrom = elt + 2;\n+\t     *copyfrom && *copyfrom != ')';\n+\t     copyfrom = nextat) {\n+\t\tsize_t len = strcspn(copyfrom, \",)\");\n+\t\tif (copyfrom[len] == ',')\n+\t\t\tnextat = copyfrom + len + 1;\n+\t\telse\n+\t\t\t/* handle ')' and '\\0' */\n+\t\t\tnextat = copyfrom + len;\n+\t\tif (!len)\n+\t\t\tcontinue;\n+\t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n+\t\t\tif (strlen(pathspec_magic[i].name) == len &&\n+\t\t\t    !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n+\t\t\t\t*magic |= pathspec_magic[i].bit;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tif (starts_with(copyfrom, \"prefix:\")) {\n+\t\t\t\tchar *endptr;\n+\t\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n+\t\t\t\t\t\t\t  &endptr, 10);\n+\t\t\t\tif (endptr - copyfrom != len)\n+\t\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n+\t\t\t\t/* \"i\" would be wrong, but it does not matter */\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (ARRAY_SIZE(pathspec_magic) <= i)\n+\t\t\tdie(_(\"Invalid pathspec magic '%.*s' in '%s'\"),\n+\t\t\t    (int) len, copyfrom, elt);\n+\t}\n+\tif (*copyfrom != ')')\n+\t\tdie(_(\"Missing ')' at the end of pathspec magic in '%s'\"), elt);\n+\t*long_magic_end = copyfrom;\n+\tcopyfrom++;\n+\t*copyfrom_ = copyfrom;\n+}\n+\n /*\n  * Take an element of a pathspec and check for magic signatures.\n  * Append the result to the prefix. Return the magic bitmap.\n@@ -150,43 +196,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \t    (flags & PATHSPEC_LITERAL_PATH)) {\n \t\t; /* nothing to do */\n \t} else if (elt[1] == '(') {\n-\t\t/* longhand */\n-\t\tconst char *nextat;\n-\t\tfor (copyfrom = elt + 2;\n-\t\t     *copyfrom && *copyfrom != ')';\n-\t\t     copyfrom = nextat) {\n-\t\t\tsize_t len = strcspn(copyfrom, \",)\");\n-\t\t\tif (copyfrom[len] == ',')\n-\t\t\t\tnextat = copyfrom + len + 1;\n-\t\t\telse\n-\t\t\t\t/* handle ')' and '\\0' */\n-\t\t\t\tnextat = copyfrom + len;\n-\t\t\tif (!len)\n-\t\t\t\tcontinue;\n-\t\t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n-\t\t\t\tif (strlen(pathspec_magic[i].name) == len &&\n-\t\t\t\t    !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n-\t\t\t\t\tmagic |= pathspec_magic[i].bit;\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t\tif (starts_with(copyfrom, \"prefix:\")) {\n-\t\t\t\t\tchar *endptr;\n-\t\t\t\t\tpathspec_prefix = strtol(copyfrom + 7,\n-\t\t\t\t\t\t\t\t &endptr, 10);\n-\t\t\t\t\tif (endptr - copyfrom != len)\n-\t\t\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n-\t\t\t\t\t/* \"i\" would be wrong, but it does not matter */\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t\tif (ARRAY_SIZE(pathspec_magic) <= i)\n-\t\t\t\tdie(_(\"Invalid pathspec magic '%.*s' in '%s'\"),\n-\t\t\t\t    (int) len, copyfrom, elt);\n-\t\t}\n-\t\tif (*copyfrom != ')')\n-\t\t\tdie(_(\"Missing ')' at the end of pathspec magic in '%s'\"), elt);\n-\t\tlong_magic_end = copyfrom;\n-\t\tcopyfrom++;\n+\t\teat_long_magic(item, elt, &magic, &pathspec_prefix, &copyfrom, &long_magic_end);\n \t} else {\n \t\t/* shorthand */\n \t\tfor (copyfrom = elt + 1;\n-- \n2.8.2.121.ga97fb08\n"},{"id":"286914","messageId":"20160518190222.28105-5-sbeller@google.com","threadId":"42373","inReplyTo":"20160518190222.28105-1-sbeller@google.com","subject":"[PATCHv7 4/5] pathspec: move prefix check out of the inner loop","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-18T19:02:21Z","receivedAt":"2016-05-18T19:02:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The prefix check is not related the check of pathspec magic; also there\nis no code that is relevant after we'd break the loop on a match for\n\"prefix:\". So move the check before the loop and shortcircuit the outer\nloop.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n pathspec.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex eba37c2..4dff252 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -107,21 +107,22 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \t\t\tnextat = copyfrom + len;\n \t\tif (!len)\n \t\t\tcontinue;\n+\n+\t\tif (starts_with(copyfrom, \"prefix:\")) {\n+\t\t\tchar *endptr;\n+\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n+\t\t\t\t\t\t  &endptr, 10);\n+\t\t\tif (endptr - copyfrom != len)\n+\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n \t\t\tif (strlen(pathspec_magic[i].name) == len &&\n \t\t\t    !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n \t\t\t\t*magic |= pathspec_magic[i].bit;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tif (starts_with(copyfrom, \"prefix:\")) {\n-\t\t\t\tchar *endptr;\n-\t\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n-\t\t\t\t\t\t\t  &endptr, 10);\n-\t\t\t\tif (endptr - copyfrom != len)\n-\t\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n-\t\t\t\t/* \"i\" would be wrong, but it does not matter */\n-\t\t\t\tbreak;\n-\t\t\t}\n \t\t}\n \t\tif (ARRAY_SIZE(pathspec_magic) <= i)\n \t\t\tdie(_(\"Invalid pathspec magic '%.*s' in '%s'\"),\n-- \n2.8.2.121.ga97fb08\n"},{"id":"286912","messageId":"20160518190222.28105-6-sbeller@google.com","threadId":"42373","inReplyTo":"20160518190222.28105-1-sbeller@google.com","subject":"[PATCHv7 5/5] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-18T19:02:22Z","receivedAt":"2016-05-18T19:02:22Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The pathspec mechanism is extended via the new\n\":(attr:eol=input)pattern/to/match\" syntax to filter paths so that it\nrequires paths to not just match the given pattern but also have the\nspecified attrs attached for them to be chosen.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/glossary-content.txt |  19 ++++\n attr.c                             |   2 +-\n attr.h                             |   2 +\n dir.c                              |  46 ++++++++++\n pathspec.c                         | 103 ++++++++++++++++++++-\n pathspec.h                         |  16 ++++\n t/t6134-pathspec-with-labels.sh    | 177 +++++++++++++++++++++++++++++++++++++\n 7 files changed, 360 insertions(+), 5 deletions(-)\n create mode 100755 t/t6134-pathspec-with-labels.sh\n\ndiff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\nindex cafc284..866e8d8 100644\n--- a/Documentation/glossary-content.txt\n+++ b/Documentation/glossary-content.txt\n@@ -384,6 +384,25 @@ full pathname may have special meaning:\n +\n Glob magic is incompatible with literal magic.\n \n+attr;;\n+\tAdditionally to matching the pathspec, the path must have the\n+\tattribute as specified. The syntax for specifying the required\n+\tattributes is \"`attr: [mode] <attribute name> [=value]`\"\n++\n+Attributes can have 4 states (Set, Unset, Set to a value, unspecified) and\n+you can query each attribute for certain states. The \"`[mode]`\" is a special\n+character to indicate which attribute states are looked for. The following\n+modes are available:\n+\n+ - \"`+`\" the attribute must be set\n+ - \"`-`\" the attribute must be unset\n+ - \"`~`\" the attribute must be unspecified\n+ - \"`?`\" the attribute must not be unspecified, i.e. set, unset or value matches\n+ - an empty \"`[mode]`\" matches if the attribute is set or has a value\n+ - an empty \"`[mode]`\" combined with \"`[=value]`\" matches if the attribute has\n+   the given value.\n++\n+\n exclude;;\n \tAfter a path matches any non-exclude pathspec, it will be run\n \tthrough all exclude pathspec (magic signature: `!`). If it\ndiff --git a/attr.c b/attr.c\nindex e0f7965..65cffd8 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -59,7 +59,7 @@ static unsigned hash_name(const char *name, int namelen)\n \treturn val;\n }\n \n-static int invalid_attr_name(const char *name, int namelen)\n+int invalid_attr_name(const char *name, int namelen)\n {\n \t/*\n \t * Attribute name cannot begin with '-' and must consist of\ndiff --git a/attr.h b/attr.h\nindex 51ca36d..4bb4848 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -45,6 +45,8 @@ extern void git_attr_check_append(struct git_attr_check *, const struct git_attr\n extern void git_attr_check_clear(struct git_attr_check *);\n extern void git_attr_check_free(struct git_attr_check *);\n \n+extern int invalid_attr_name(const char *name, int namelen);\n+\n /*\n  * Return the name of the attribute represented by the argument.  The\n  * return value is a pointer to a null-delimited string that is part\ndiff --git a/dir.c b/dir.c\nindex 996653b..3141a5a 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -9,6 +9,7 @@\n  */\n #include \"cache.h\"\n #include \"dir.h\"\n+#include \"attr.h\"\n #include \"refs.h\"\n #include \"wildmatch.h\"\n #include \"pathspec.h\"\n@@ -215,6 +216,48 @@ int within_depth(const char *name, int namelen,\n \treturn 1;\n }\n \n+static int match_attrs(const char *name, int namelen,\n+\t\t       const struct pathspec_item *item)\n+{\n+\tchar *path;\n+\tint i;\n+\n+\tpath = xmemdupz(name, namelen);\n+\tgit_check_attr(path, item->attr_check);\n+\n+\tfor (i = 0; i < item->attr_match_nr; i++) {\n+\t\tconst char *value;\n+\t\tint matched;\n+\t\tenum attr_match_mode match_mode;\n+\n+\t\tvalue = item->attr_check->check[i].value;\n+\n+\t\tmatch_mode = item->attr_match[i].match_mode;\n+\n+\t\tif (ATTR_TRUE(value)) {\n+\t\t\tmatched = match_mode == MATCH_SET ||\n+\t\t\t\t  match_mode == MATCH_SET_OR_VALUE ||\n+\t\t\t\t  match_mode == MATCH_NOT_UNSPECIFIED;\n+\t\t} else if (ATTR_FALSE(value)) {\n+\t\t\tmatched = match_mode == MATCH_UNSET ||\n+\t\t\t\t  match_mode == MATCH_NOT_UNSPECIFIED;\n+\t\t} else if (ATTR_UNSET(value)) {\n+\t\t\tmatched = match_mode == MATCH_UNSPECIFIED;\n+\t\t} else {\n+\t\t\tmatched = match_mode == MATCH_NOT_UNSPECIFIED ||\n+\t\t\t\t  match_mode == MATCH_SET_OR_VALUE ||\n+\t\t\t\t  (match_mode == MATCH_VALUE &&\n+\t\t\t\t   !strcmp(item->attr_match[i].value, value));\n+\t\t}\n+\t\tif (!matched)\n+\t\t\treturn 0;\n+\t}\n+\n+\tfree(path);\n+\n+\treturn 1;\n+}\n+\n #define DO_MATCH_EXCLUDE   1\n #define DO_MATCH_DIRECTORY 2\n \n@@ -270,6 +313,9 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,\n \t    strncmp(item->match, name - prefix, item->prefix))\n \t\treturn 0;\n \n+\tif (item->attr_match_nr && !match_attrs(name, namelen, item))\n+\t\treturn 0;\n+\n \t/* If the match was just the prefix, we matched */\n \tif (!*match)\n \t\treturn MATCHED_RECURSIVELY;\ndiff --git a/pathspec.c b/pathspec.c\nindex 4dff252..32fb6a8 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"dir.h\"\n #include \"pathspec.h\"\n+#include \"attr.h\"\n \n /*\n  * Finds which of the given pathspecs match items in the index.\n@@ -88,12 +89,82 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)\n+{\n+\tstruct string_list_item *si;\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\n+\n+\tif (!value || !strlen(value))\n+\t\tgoto err;\n+\n+\tstring_list_split(&list, value, ' ', -1);\n+\tstring_list_remove_empty_items(&list, 0);\n+\n+\tif (!item->attr_check)\n+\t\titem->attr_check = git_attr_check_alloc();\n+\n+\tALLOC_GROW(item->attr_match, item->attr_match_nr + list.nr, item->attr_match_alloc);\n+\n+\tfor_each_string_list_item(si, &list) {\n+\t\tsize_t val_len;\n+\n+\t\tint j = item->attr_match_nr++;\n+\t\tconst char *val = si->string;\n+\t\tstruct attr_match *am = &item->attr_match[j];\n+\n+\t\tif (val[0] == '?')\n+\t\t\tam->match_mode = MATCH_NOT_UNSPECIFIED;\n+\t\telse if (val[0] == '~')\n+\t\t\tam->match_mode = MATCH_UNSPECIFIED;\n+\t\telse if (val[0] == '+')\n+\t\t\tam->match_mode = MATCH_SET;\n+\t\telse if (val[0] == '-')\n+\t\t\tam->match_mode = MATCH_UNSET;\n+\t\telse\n+\t\t\tam->match_mode = MATCH_SET_OR_VALUE;\n+\n+\t\tif (am->match_mode != MATCH_SET_OR_VALUE)\n+\t\t\t/* skip first character */\n+\t\t\tval++;\n+\n+\t\tval_len = strcspn(val, \"=,)\");\n+\t\tif (val[val_len] == '=') {\n+\t\t\tam->match_mode = MATCH_VALUE;\n+\t\t\tam->value = xstrdup(&val[val_len + 1]);\n+\t\t\t/*\n+\t\t\t * NEEDSWORK:\n+\t\t\t * Do we want to allow escaped commas to search\n+\t\t\t * for comma separated values?\n+\t\t\t */\n+\t\t\tif (strchr(am->value, '\\\\'))\n+\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n+\t\t} else\n+\t\t\tam->value = NULL;\n+\n+\t\tif (invalid_attr_name(val, val_len)) {\n+\t\t\tam->match_mode = INVALID_ATTR;\n+\t\t\tgoto err;\n+\t\t}\n+\n+\t\tam->attr = git_attr(xmemdupz(val, val_len));\n+\t\tgit_attr_check_append(item->attr_check, am->attr);\n+\t}\n+\n+\tstring_list_clear(&list, 0);\n+\treturn;\n+err:\n+\tdie(_(\"attr spec '%s': attrs must not start with '-' and \"\n+\t      \"be composed of [-A-Za-z0-9_.].\"), value);\n+}\n+\n static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \t\tunsigned *magic, int *pathspec_prefix,\n \t\tconst char **copyfrom_, const char **long_magic_end)\n {\n \tint i;\n \tconst char *copyfrom = *copyfrom_;\n+\tconst char *body;\n \t/* longhand */\n \tconst char *nextat;\n \tfor (copyfrom = elt + 2;\n@@ -108,15 +179,21 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \t\tif (!len)\n \t\t\tcontinue;\n \n-\t\tif (starts_with(copyfrom, \"prefix:\")) {\n+\t\tif (skip_prefix(copyfrom, \"prefix:\", &body)) {\n \t\t\tchar *endptr;\n-\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n-\t\t\t\t\t\t  &endptr, 10);\n+\t\t\t*pathspec_prefix = strtol(body, &endptr, 10);\n \t\t\tif (endptr - copyfrom != len)\n \t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (skip_prefix(copyfrom, \"attr:\", &body)) {\n+\t\t\tchar *pass = xmemdupz(body, len - strlen(\"attr:\"));\n+\t\t\tparse_pathspec_attr_match(item, pass);\n+\t\t\tfree(pass);\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n \t\t\tif (strlen(pathspec_magic[i].name) == len &&\n \t\t\t    !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n@@ -425,7 +502,10 @@ void parse_pathspec(struct pathspec *pathspec,\n \tfor (i = 0; i < n; i++) {\n \t\tunsigned short_magic;\n \t\tentry = argv[i];\n-\n+\t\titem[i].attr_check = NULL;\n+\t\titem[i].attr_match = NULL;\n+\t\titem[i].attr_match_nr = 0;\n+\t\titem[i].attr_match_alloc = 0;\n \t\titem[i].magic = prefix_pathspec(item + i, &short_magic,\n \t\t\t\t\t\targv + i, flags,\n \t\t\t\t\t\tprefix, prefixlen, entry);\n@@ -447,6 +527,13 @@ void parse_pathspec(struct pathspec *pathspec,\n \t\tif (item[i].nowildcard_len < item[i].len)\n \t\t\tpathspec->has_wildcard = 1;\n \t\tpathspec->magic |= item[i].magic;\n+\n+\t\tif (item[i].attr_match_nr) {\n+\t\t\tint j;\n+\t\t\tfor (j = 0; j < item[i].attr_match_nr; j++)\n+\t\t\t\tif (item[i].attr_match[j].match_mode == INVALID_ATTR)\n+\t\t\t\t\tdie(_(\"attribute spec in the wrong syntax are prohibited.\"));\n+\t\t}\n \t}\n \n \tif (nr_exclude == n)\n@@ -502,6 +589,14 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n \n void free_pathspec(struct pathspec *pathspec)\n {\n+\tint i, j;\n+\tfor (i = 0; i < pathspec->nr; i++) {\n+\t\tfor (j = 0; j < pathspec->items[j].attr_match_nr; j++)\n+\t\t\tfree(pathspec->items[i].attr_match[j].value);\n+\t\tfree(pathspec->items[i].attr_match);\n+\t\tgit_attr_check_free(pathspec->items[i].attr_check);\n+\t}\n+\n \tfree(pathspec->items);\n \tpathspec->items = NULL;\n }\ndiff --git a/pathspec.h b/pathspec.h\nindex 0c11262..5308137 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -32,6 +32,22 @@ struct pathspec {\n \t\tint len, prefix;\n \t\tint nowildcard_len;\n \t\tint flags;\n+\t\tint attr_match_nr;\n+\t\tint attr_match_alloc;\n+\t\tstruct attr_match {\n+\t\t\tstruct git_attr *attr;\n+\t\t\tchar *value;\n+\t\t\tenum attr_match_mode {\n+\t\t\t\tMATCH_SET,\n+\t\t\t\tMATCH_UNSET,\n+\t\t\t\tMATCH_VALUE,\n+\t\t\t\tMATCH_UNSPECIFIED,\n+\t\t\t\tMATCH_NOT_UNSPECIFIED,\n+\t\t\t\tMATCH_SET_OR_VALUE,\n+\t\t\t\tINVALID_ATTR\n+\t\t\t} match_mode;\n+\t\t} *attr_match;\n+\t\tstruct git_attr_check *attr_check;\n \t} *items;\n };\n \ndiff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh\nnew file mode 100755\nindex 0000000..35b3ab2\n--- /dev/null\n+++ b/t/t6134-pathspec-with-labels.sh\n@@ -0,0 +1,177 @@\n+#!/bin/sh\n+\n+test_description='test labels in pathspecs'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a tree' '\n+\tmkdir sub &&\n+\tfor p in fileA fileB fileC fileAB fileAC fileBC fileNoLabel fileUnsetLabel fileSetLabel fileValue fileWrongLabel; do\n+\t\t: >$p &&\n+\t\tgit add $p &&\n+\t\t: >sub/$p\n+\t\tgit add sub/$p\n+\tdone &&\n+\tgit commit -m $p &&\n+\tgit ls-files >actual &&\n+\tcat <<EOF >expect &&\n+fileA\n+fileAB\n+fileAC\n+fileB\n+fileBC\n+fileC\n+fileNoLabel\n+fileSetLabel\n+fileUnsetLabel\n+fileValue\n+fileWrongLabel\n+sub/fileA\n+sub/fileAB\n+sub/fileAC\n+sub/fileB\n+sub/fileBC\n+sub/fileC\n+sub/fileNoLabel\n+sub/fileSetLabel\n+sub/fileUnsetLabel\n+sub/fileValue\n+sub/fileWrongLabel\n+EOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pathspec with no attr' '\n+\ttest_must_fail git ls-files \":(attr:)\" 2>actual &&\n+\ttest_i18ngrep fatal actual\n+'\n+\n+test_expect_success 'pathspec with labels and non existent .gitattributes' '\n+\tgit ls-files \":(attr:label)\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'setup .gitattributes' '\n+\tcat <<EOF >.gitattributes &&\n+fileA labelA\n+fileB labelB\n+fileC labelC\n+fileAB labelA labelB\n+fileAC labelA labelC\n+fileBC labelB labelC\n+fileUnsetLabel -label\n+fileSetLabel label\n+fileValue label=foo\n+fileWrongLabel label☺\n+EOF\n+\tgit add .gitattributes &&\n+\tgit commit -m \"add attributes\"\n+'\n+\n+sq=\"'\"\n+\n+test_expect_success 'check specific set attr' '\n+\tcat <<EOF >expect &&\n+fileSetLabel\n+sub/fileSetLabel\n+EOF\n+\tgit ls-files \":(attr:+label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check specific unset attr' '\n+\tcat <<EOF >expect &&\n+fileUnsetLabel\n+sub/fileUnsetLabel\n+EOF\n+\tgit ls-files \":(attr:-label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check specific value attr' '\n+\tcat <<EOF >expect &&\n+fileValue\n+sub/fileValue\n+EOF\n+\tgit ls-files \":(attr:label=foo)\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files \":(attr:label=bar)\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'check set or value attr' '\n+\tcat <<EOF >expect &&\n+fileSetLabel\n+fileValue\n+sub/fileSetLabel\n+sub/fileValue\n+EOF\n+\tgit ls-files \":(attr:label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check unspecified attr' '\n+\tcat <<EOF >expect &&\n+.gitattributes\n+fileC\n+fileNoLabel\n+fileWrongLabel\n+sub/fileC\n+sub/fileNoLabel\n+sub/fileWrongLabel\n+EOF\n+\tgit ls-files \":(attr:~label,attr:~labelA,attr:~labelB)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check not unspecified attr' '\n+\tcat <<EOF >expect &&\n+fileSetLabel\n+fileUnsetLabel\n+fileValue\n+sub/fileSetLabel\n+sub/fileUnsetLabel\n+sub/fileValue\n+EOF\n+\tgit ls-files \":(attr:?label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check label with 2 labels' '\n+\tcat <<EOF >expect &&\n+fileAB\n+sub/fileAB\n+EOF\n+\tgit ls-files \":(attr:labelA labelB)\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files \":(attr:labelA,attr:labelB)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check label with more labels but excluded path' '\n+\tcat <<EOF >expect &&\n+fileAB\n+fileB\n+fileBC\n+EOF\n+\tgit ls-files \":(attr:labelB)\" \":(exclude)sub/\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check label excluding other labels' '\n+\tcat <<EOF >expect &&\n+fileAB\n+fileB\n+fileBC\n+sub/fileAB\n+sub/fileB\n+EOF\n+\tgit ls-files \":(attr:labelB)\" \":(exclude,attr:labelC)sub/\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'abort on giving invalid label on the command line' '\n+\ttest_must_fail git ls-files . \":(attr:☺)\" 2>actual &&\n+\ttest_i18ngrep \"fatal\" actual\n+'\n+\n+test_done\n-- \n2.8.2.121.ga97fb08\n"},{"id":"286918","messageId":"CAGZ79kaMU=H7x7Gxfk39No42kxNdrqykMyx+uD7gjDsK087m1g@mail.gmail.com","threadId":"42373","inReplyTo":"20160518190222.28105-6-sbeller@google.com","subject":"Re: [PATCHv7 5/5] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-18T19:42:00Z","receivedAt":"2016-05-18T19:42:00Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 18, 2016 at 12:02 PM, Stefan Beller <sbeller@google.com> wrote:\n>  void free_pathspec(struct pathspec *pathspec)\n>  {\n> +       int i, j;\n> +       for (i = 0; i < pathspec->nr; i++) {\n> +               for (j = 0; j < pathspec->items[j].attr_match_nr; j++)\n> +                       free(pathspec->items[i].attr_match[j].value);\n> +               free(pathspec->items[i].attr_match);\n> +               git_attr_check_free(pathspec->items[i].attr_check);\n\nThis is faulty as may be NULL and git_attr_check_free assumes its\nargument is not\nNULL. So I'll add a guard here in a reoll.\n"},{"id":"286919","messageId":"xmqq7ferrvvd.fsf@gitster.mtv.corp.google.com","threadId":"42373","inReplyTo":"20160518190222.28105-6-sbeller@google.com","subject":"Re: [PATCHv7 5/5] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-18T19:47:50Z","receivedAt":"2016-05-18T19:47:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> + - \"`+`\" the attribute must be set\n> + - \"`-`\" the attribute must be unset\n> + - \"`~`\" the attribute must be unspecified\n\nI think using these three, not the way how .gitattributes specify\nthem, is highly confusing to the users.\n\n> + - \"`?`\" the attribute must not be unspecified, i.e. set, unset or value matches\n\nThe choice of '?' is OK, as there is no reason .gitattributes wants\nto say something fuzzy like \"this is set or unset or has value\".\nThe last part \"set, unset or value matches\" does not make sense,\nthough.  Did you mean \"set, unset or set to value\"?\n\n> + - an empty \"`[mode]`\" matches if the attribute is set or has a value\n\nThe same comment as +/-/~ above applies.\n\n> + - an empty \"`[mode]`\" combined with \"`[=value]`\" matches if the attribute has\n> +   the given value.\n\nI.e. \"eol=crlf\" matches only for paths whose eol attribute is set to\n\"crlf\".  Makes perfect sense.\n\n\n> diff --git a/dir.c b/dir.c\n> index 996653b..3141a5a 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -9,6 +9,7 @@\n>   */\n>  #include \"cache.h\"\n>  #include \"dir.h\"\n> +#include \"attr.h\"\n>  #include \"refs.h\"\n>  #include \"wildmatch.h\"\n>  #include \"pathspec.h\"\n> @@ -215,6 +216,48 @@ int within_depth(const char *name, int namelen,\n>  \treturn 1;\n>  }\n>  \n> +static int match_attrs(const char *name, int namelen,\n> +\t\t       const struct pathspec_item *item)\n> +{\n> +\tchar *path;\n> +\tint i;\n> +\n> +\tpath = xmemdupz(name, namelen);\n> +\tgit_check_attr(path, item->attr_check);\n> +\n> +\tfor (i = 0; i < item->attr_match_nr; i++) {\n> +\t\tconst char *value;\n> +\t\tint matched;\n> +\t\tenum attr_match_mode match_mode;\n> +\n> +\t\tvalue = item->attr_check->check[i].value;\n> +\n> +\t\tmatch_mode = item->attr_match[i].match_mode;\n\nMental note: there is a one-to-one correspondence between\nattr_check->check[] and attr_match[].\n\n> +\t\tif (!matched)\n> +\t\t\treturn 0;\n\nSo this ANDs together.  OK.\n\n> +\t}\n> +\n> +\tfree(path);\n\nLet me see how involved a change would be to allow passing a counted\nstring to git_check_attr().\n\n> diff --git a/pathspec.c b/pathspec.c\n> index 4dff252..32fb6a8 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -1,6 +1,7 @@\n>  #include \"cache.h\"\n>  #include \"dir.h\"\n>  #include \"pathspec.h\"\n> +#include \"attr.h\"\n>  \n>  /*\n>   * Finds which of the given pathspecs match items in the index.\n> @@ -88,12 +89,82 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n>  \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n>  }\n>  \n\nMental note after skiming the caller:\n\nThe \"value\" here is like \"VAR1=VAL1 VAR2 !VAR3\" in a pathspec\n\":(attr:VAR1=VAL1 VAR2 !VAR3)path/to/limit\", i.e. the source string\nto be compiled into a list of conditionals that will be evaluated by\nmatch_attr() in dir.c\n\n> +static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)\n> +{\n> +\tstruct string_list_item *si;\n> +\tstruct string_list list = STRING_LIST_INIT_DUP;\n> +\n> +\n> +\tif (!value || !strlen(value))\n> +\t\tgoto err;\n> +\n> +\tstring_list_split(&list, value, ' ', -1);\n> +\tstring_list_remove_empty_items(&list, 0);\n\nAt this point, we got \"VAR1=VAL1\", \"VAR2\", \"!VAR3\" as elements in\nthis list.\n\n> +\tif (!item->attr_check)\n> +\t\titem->attr_check = git_attr_check_alloc();\n\nGiven more than one \"attr\" magic, e.g. \":(attr:A=a,attr:B)/path\",\nthe check may not be empty when we process the second one; we just\nextend it without losing the existing contents.\n\n> +\tALLOC_GROW(item->attr_match, item->attr_match_nr + list.nr, item->attr_match_alloc);\n\nLikewise, we extend attr_match[] array.\n\n> +\tfor_each_string_list_item(si, &list) {\n> +\t\tsize_t val_len;\n> +\n> +\t\tint j = item->attr_match_nr++;\n> +\t\tconst char *val = si->string;\n> +\t\tstruct attr_match *am = &item->attr_match[j];\n> +\n> +\t\tif (val[0] == '?')\n> +\t\t\tam->match_mode = MATCH_NOT_UNSPECIFIED;\n> +\t\telse if (val[0] == '~')\n> +\t\t\tam->match_mode = MATCH_UNSPECIFIED;\n> +\t\telse if (val[0] == '+')\n> +\t\t\tam->match_mode = MATCH_SET;\n> +\t\telse if (val[0] == '-')\n> +\t\t\tam->match_mode = MATCH_UNSET;\n> +\t\telse\n> +\t\t\tam->match_mode = MATCH_SET_OR_VALUE;\n> +\n> +\t\tif (am->match_mode != MATCH_SET_OR_VALUE)\n> +\t\t\t/* skip first character */\n> +\t\t\tval++;\n> +\n> +\t\tval_len = strcspn(val, \"=,)\");\n\nI understand \"=\", but can \",\" and \")\" appear here?\n\n> +\t\tif (val[val_len] == '=') {\n> +\t\t\tam->match_mode = MATCH_VALUE;\n> +\t\t\tam->value = xstrdup(&val[val_len + 1]);\n> +\t\t\t/*\n> +\t\t\t * NEEDSWORK:\n> +\t\t\t * Do we want to allow escaped commas to search\n> +\t\t\t * for comma separated values?\n> +\t\t\t */\n\nNo, we don't ;-).\n\n> +\t\t\tif (strchr(am->value, '\\\\'))\n> +\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n\nBut this is a good escape hatch to reserve for our own use, until we\ndecide what the quoting mechanism will be (or if it is necessary).\n\n> +\t\t} else\n> +\t\t\tam->value = NULL;\n> +\n> +\t\tif (invalid_attr_name(val, val_len)) {\n> +\t\t\tam->match_mode = INVALID_ATTR;\n> +\t\t\tgoto err;\n> +\t\t}\n> +\n> +\t\tam->attr = git_attr(xmemdupz(val, val_len));\n> +\t\tgit_attr_check_append(item->attr_check, am->attr);\n\nGOOD!\n\nI think val and val_len should be renamed to attr and attr_len (in\nthe VARIABLE=VALUE context, these two identifiers are about parsing\nthe variable side, not the value side).\n\n> +\t}\n> +\n> +\tstring_list_clear(&list, 0);\n> +\treturn;\n> +err:\n> +\tdie(_(\"attr spec '%s': attrs must not start with '-' and \"\n> +\t      \"be composed of [-A-Za-z0-9_.].\"), value);\n\nWhat is \"value\" at this point?  If you failed to parse the second\nelement in \"VAR1=VAL1 FR*TZ=VAL2 !VAR3\", I think you would want to\nsay \"'FR*TZ' is malformed\".\n\nExisting users of the function seems to say this:\n\n\t\tif (invalid_attr_name(cp, len)) {\n\t\t\tfprintf(stderr,\n\t\t\t\t\"%.*s is not a valid attribute name: %s:%d\\n\",\n\t\t\t\tlen, cp, src, lineno);\n\t\t\treturn NULL;\n\t\t}\n\nwhen parsing .gitattribute file.  The source file:line reference\ndoes not apply to this context, but it would be better to unify the\nmessage.  I do not mind spelling the rules out explicitly like you\ndid, but I do not want to see it spread across many places (which\nforces us to update them when we have to change the rule later).\n\nPerhaps we want a helper function in attr.c side that takes the\nattribute name string (val, val_len in your code above, which I\nsuggest to be renamed to attr, attr_len) and formats the error\nmessage into a strbuf, or something?\n\n\tvoid\n\tattr_name_error(struct strbuf *err, const char *name, size_t len)\n        {\n\t\tstrbuf_addf(_(\"invalid attribute name: '%.*s'\"), len, name);\n\t}\n\nthen this one can say\n\n\terr: {\n        \tstruct strbuf err = STRBUF_INIT;\n\t\tattr_name_error(&err, attr, attr_len);\n                die(err.buf);\n\t}\n\nwhile the error routines can do a similar thing using cp and len,\nand then append \"%s:%d\" % (src, lineno) at the end themselves.\n\n>  static void eat_long_magic(struct pathspec_item *item, const char *elt,\n>  \t\tunsigned *magic, int *pathspec_prefix,\n>  \t\tconst char **copyfrom_, const char **long_magic_end)\n>  {\n>  \tint i;\n>  \tconst char *copyfrom = *copyfrom_;\n> +\tconst char *body;\n>  \t/* longhand */\n>  \tconst char *nextat;\n>  \tfor (copyfrom = elt + 2;\n> @@ -108,15 +179,21 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n>  \t\tif (!len)\n>  \t\t\tcontinue;\n>  \n> -\t\tif (starts_with(copyfrom, \"prefix:\")) {\n> +\t\tif (skip_prefix(copyfrom, \"prefix:\", &body)) {\n>  \t\t\tchar *endptr;\n> -\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n> -\t\t\t\t\t\t  &endptr, 10);\n> +\t\t\t*pathspec_prefix = strtol(body, &endptr, 10);\n>  \t\t\tif (endptr - copyfrom != len)\n>  \t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n>  \t\t\tcontinue;\n>  \t\t}\n>  \n> +\t\tif (skip_prefix(copyfrom, \"attr:\", &body)) {\n> +\t\t\tchar *pass = xmemdupz(body, len - strlen(\"attr:\"));\n> +\t\t\tparse_pathspec_attr_match(item, pass);\n> +\t\t\tfree(pass);\n> +\t\t\tcontinue;\n\nMakes me wonder what \"pass\" stands for.  From the look of xmemdupz(),\ngiven \":(attr:VAR1=VAL1 VAR2 !VAR3)path/to/limit\" to parse, len\npoints at \")\" and xmemdupz() gives \"VAR1=VAL1 VAR2 !VAR3\" to it.\n\n> @@ -502,6 +589,14 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n>  \n>  void free_pathspec(struct pathspec *pathspec)\n>  {\n> +\tint i, j;\n> +\tfor (i = 0; i < pathspec->nr; i++) {\n> +\t\tfor (j = 0; j < pathspec->items[j].attr_match_nr; j++)\n> +\t\t\tfree(pathspec->items[i].attr_match[j].value);\n> +\t\tfree(pathspec->items[i].attr_match);\n> +\t\tgit_attr_check_free(pathspec->items[i].attr_check);\n> +\t}\n\nOK.\n\nOverall very nicely done (modulo nits mentioned above, none of which\nis unfixable).\n\nThanks.\n"},{"id":"286920","messageId":"xmqq37pfrv9y.fsf@gitster.mtv.corp.google.com","threadId":"42373","inReplyTo":"xmqq7ferrvvd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv7 5/5] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-18T20:00:41Z","receivedAt":"2016-05-18T20:00:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\nJust this part:\n\n> The last part \"set, unset or value matches\" does not make sense,\n> though.\n\nI re-read the line and I think you meant\n\n    ?VAR makes a path match if VAR attribute is set, set to false,\n    or set to value for the path\n\nand I shouldn't have read it as a 3-tuple (\"set\", \"unset\", \"value matches\"),\nbut as a 3-tuple (\"set\", \"unset\", \"value\").  That is,\n\n    A path (whose attribute state is one of these three) matches.\n\nSo I retract \"does not make sense\"; it still is \"hard to grok\", though.\n"},{"id":"286927","messageId":"xmqqy477qfr2.fsf@gitster.mtv.corp.google.com","threadId":"42373","inReplyTo":"xmqq7ferrvvd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv7 5/5] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-18T20:21:21Z","receivedAt":"2016-05-18T20:21:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> +static int match_attrs(const char *name, int namelen,\n>> +\t\t       const struct pathspec_item *item)\n>> +{\n>> +\tchar *path;\n>> +\tint i;\n>> +\n>> +\tpath = xmemdupz(name, namelen);\n>> +\tgit_check_attr(path, item->attr_check);\n> ...\n>> +\t}\n>> +\n>> +\tfree(path);\n>\n> Let me see how involved a change would be to allow passing a counted\n> string to git_check_attr().\n\nPerhaps the attached is sufficient, and you can avoid copying the\npaths in this codepath.\n\n attr.c | 23 ++++++++++++++---------\n attr.h |  1 +\n 2 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex e0f7965..eeb29e6 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -725,20 +725,19 @@ static int macroexpand_one(int nr, int rem)\n  * check_all_attr. If num is non-zero, only attributes in check[] are\n  * collected. Otherwise all attributes are collected.\n  */\n-static void collect_some_attrs(const char *path, int num,\n+static void collect_some_attrs(const char *path, int pathlen, int num,\n \t\t\t       struct git_attr_check_elem *check)\n \n {\n \tstruct attr_stack *stk;\n-\tint i, pathlen, rem, dirlen;\n+\tint i, rem, dirlen;\n \tconst char *cp, *last_slash = NULL;\n \tint basename_offset;\n \n-\tfor (cp = path; *cp; cp++) {\n+\tfor (cp = path; cp < path + pathlen; 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_offset = last_slash + 1 - path;\n \t\tdirlen = last_slash - path;\n@@ -769,12 +768,12 @@ static void collect_some_attrs(const char *path, int num,\n \t\trem = fill(path, pathlen, basename_offset, stk, rem);\n }\n \n-static int git_check_attrs(const char *path, int num,\n+static int git_check_attrs(const char *path, int pathlen, int num,\n \t\t\t   struct git_attr_check_elem *check)\n {\n \tint i;\n \n-\tcollect_some_attrs(path, num, check);\n+\tcollect_some_attrs(path, pathlen, num, check);\n \n \tfor (i = 0; i < num; i++) {\n \t\tconst char *value = check_all_attr[check[i].attr->attr_nr].value;\n@@ -791,7 +790,7 @@ void git_all_attrs(const char *path, struct git_attr_check *check)\n \tint i;\n \n \tgit_attr_check_clear(check);\n-\tcollect_some_attrs(path, 0, NULL);\n+\tcollect_some_attrs(path, strlen(path), 0, NULL);\n \n \tfor (i = 0; i < attr_nr; i++) {\n \t\tconst char *name = check_all_attr[i].attr->name;\n@@ -816,10 +815,16 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist\n \tuse_index = istate;\n }\n \n-int git_check_attr(const char *path, struct git_attr_check *check)\n+int git_check_attr_counted(const char *path, int pathlen,\n+\t\t\t   struct git_attr_check *check)\n {\n \tcheck->finalized = 1;\n-\treturn git_check_attrs(path, check->check_nr, check->check);\n+\treturn git_check_attrs(path, pathlen, check->check_nr, check->check);\n+}\n+\n+int git_check_attr(const char *path, struct git_attr_check *check)\n+{\n+\treturn git_check_attr_counted(path, strlen(path), check);\n }\n \n struct git_attr_check *git_attr_check_initl(const char *one, ...)\ndiff --git a/attr.h b/attr.h\nindex 51ca36d..4a4ac76 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -38,6 +38,7 @@ struct git_attr_check {\n \n extern struct git_attr_check *git_attr_check_initl(const char *, ...);\n extern int git_check_attr(const char *path, struct git_attr_check *);\n+extern int git_check_attr_counted(const char *, int, struct git_attr_check *);\n \n extern struct git_attr_check *git_attr_check_alloc(void);\n extern void git_attr_check_append(struct git_attr_check *, const struct git_attr *);\n"},{"id":"286929","messageId":"xmqqshxfqdw6.fsf_-_@gitster.mtv.corp.google.com","threadId":"42373","inReplyTo":"xmqqy477qfr2.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] attr: add counted string version of git_check_attr()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-18T21:01:29Z","receivedAt":"2016-05-18T21:01:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Often a potential caller has <path, pathlen> pair that\nrepresents the path it wants to ask attributes for; when\npath[pathlen] is not NUL, the caller has to xmemdupz()\nonly to call git_check_attr().\n\nAdd git_check_attr_counted() that takes such a counted\nstring instead of \"const char *path\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * This time with a log message\n\n attr.c | 23 ++++++++++++++---------\n attr.h |  1 +\n 2 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex e0f7965..eeb29e6 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -725,20 +725,19 @@ static int macroexpand_one(int nr, int rem)\n  * check_all_attr. If num is non-zero, only attributes in check[] are\n  * collected. Otherwise all attributes are collected.\n  */\n-static void collect_some_attrs(const char *path, int num,\n+static void collect_some_attrs(const char *path, int pathlen, int num,\n \t\t\t       struct git_attr_check_elem *check)\n \n {\n \tstruct attr_stack *stk;\n-\tint i, pathlen, rem, dirlen;\n+\tint i, rem, dirlen;\n \tconst char *cp, *last_slash = NULL;\n \tint basename_offset;\n \n-\tfor (cp = path; *cp; cp++) {\n+\tfor (cp = path; cp < path + pathlen; 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_offset = last_slash + 1 - path;\n \t\tdirlen = last_slash - path;\n@@ -769,12 +768,12 @@ static void collect_some_attrs(const char *path, int num,\n \t\trem = fill(path, pathlen, basename_offset, stk, rem);\n }\n \n-static int git_check_attrs(const char *path, int num,\n+static int git_check_attrs(const char *path, int pathlen, int num,\n \t\t\t   struct git_attr_check_elem *check)\n {\n \tint i;\n \n-\tcollect_some_attrs(path, num, check);\n+\tcollect_some_attrs(path, pathlen, num, check);\n \n \tfor (i = 0; i < num; i++) {\n \t\tconst char *value = check_all_attr[check[i].attr->attr_nr].value;\n@@ -791,7 +790,7 @@ void git_all_attrs(const char *path, struct git_attr_check *check)\n \tint i;\n \n \tgit_attr_check_clear(check);\n-\tcollect_some_attrs(path, 0, NULL);\n+\tcollect_some_attrs(path, strlen(path), 0, NULL);\n \n \tfor (i = 0; i < attr_nr; i++) {\n \t\tconst char *name = check_all_attr[i].attr->name;\n@@ -816,10 +815,16 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist\n \tuse_index = istate;\n }\n \n-int git_check_attr(const char *path, struct git_attr_check *check)\n+int git_check_attr_counted(const char *path, int pathlen,\n+\t\t\t   struct git_attr_check *check)\n {\n \tcheck->finalized = 1;\n-\treturn git_check_attrs(path, check->check_nr, check->check);\n+\treturn git_check_attrs(path, pathlen, check->check_nr, check->check);\n+}\n+\n+int git_check_attr(const char *path, struct git_attr_check *check)\n+{\n+\treturn git_check_attr_counted(path, strlen(path), check);\n }\n \n struct git_attr_check *git_attr_check_initl(const char *one, ...)\ndiff --git a/attr.h b/attr.h\nindex 51ca36d..4a4ac76 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -38,6 +38,7 @@ struct git_attr_check {\n \n extern struct git_attr_check *git_attr_check_initl(const char *, ...);\n extern int git_check_attr(const char *path, struct git_attr_check *);\n+extern int git_check_attr_counted(const char *, int, struct git_attr_check *);\n \n extern struct git_attr_check *git_attr_check_alloc(void);\n extern void git_attr_check_append(struct git_attr_check *, const struct git_attr *);\n-- \n2.8.2-759-geb611ab\n"},{"id":"286931","messageId":"xmqqmvnnqdtd.fsf_-_@gitster.mtv.corp.google.com","threadId":"42373","inReplyTo":"xmqqy477qfr2.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] attr: add counted string version of git_attr()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-18T21:03:10Z","receivedAt":"2016-05-18T21:03:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Often a potential caller has <name, namelen> pair that\nrepresents the name it wants to create an attribute out of.\n\nWhen name[namelen] is not NUL, the caller has to xmemdupz()\nonly to call git_attr().\n\nAdd git_attr_counted() that takes such a counted string instead of\n\"const char *name\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * The same for git_attr()\n\n attr.c | 8 ++++----\n attr.h | 5 ++++-\n 2 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex eeb29e6..7f20e21 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -78,7 +78,7 @@ static int invalid_attr_name(const char *name, int namelen)\n \treturn 0;\n }\n \n-static struct git_attr *git_attr_internal(const char *name, int len)\n+struct git_attr *git_attr_counted(const char *name, size_t len)\n {\n \tunsigned hval = hash_name(name, len);\n \tunsigned pos = hval % HASHSIZE;\n@@ -109,7 +109,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)\n \n struct git_attr *git_attr(const char *name)\n {\n-\treturn git_attr_internal(name, strlen(name));\n+\treturn git_attr_counted(name, strlen(name));\n }\n \n /* What does a matched pattern decide? */\n@@ -199,7 +199,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,\n \t\telse {\n \t\t\te->setto = xmemdupz(equals + 1, ep - equals - 1);\n \t\t}\n-\t\te->attr = git_attr_internal(cp, len);\n+\t\te->attr = git_attr_counted(cp, len);\n \t}\n \treturn ep + strspn(ep, blank);\n }\n@@ -254,7 +254,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\t      sizeof(struct attr_state) * num_attr +\n \t\t      (is_macro ? 0 : namelen + 1));\n \tif (is_macro) {\n-\t\tres->u.attr = git_attr_internal(name, namelen);\n+\t\tres->u.attr = git_attr_counted(name, namelen);\n \t\tres->u.attr->maybe_macro = 1;\n \t} else {\n \t\tchar *p = (char *)&(res->state[num_attr]);\ndiff --git a/attr.h b/attr.h\nindex 4a4ac76..78d6d5a 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -8,7 +8,10 @@ struct git_attr;\n  * Given a string, return the gitattribute object that\n  * corresponds to it.\n  */\n-struct git_attr *git_attr(const char *);\n+extern struct git_attr *git_attr(const char *);\n+\n+/* The same, but with counted string */\n+extern struct git_attr *git_attr_counted(const char *, size_t);\n \n /* Internal use */\n extern const char git_attr__true[];\n-- \n2.8.2-759-geb611ab\n"},{"id":"286932","messageId":"xmqqh9dvqdoy.fsf_-_@gitster.mtv.corp.google.com","threadId":"42373","inReplyTo":"xmqqy477qfr2.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] attr: expose validity check for attribute names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-18T21:05:49Z","receivedAt":"2016-05-18T21:05:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Export attr_name_valid() function, and a helper function that\nreturns the message to be given when a given <name, len> pair\nis not a good name for an attribute.\n\nWe could later update the message to exactly spell out what the\nrules for a good attribute name are, etc.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * And then finally the error message one.  I didn't change\n   the message itself to spell out the rules here, though.\n\n attr.c | 39 +++++++++++++++++++++++++--------------\n attr.h |  3 +++\n 2 files changed, 28 insertions(+), 14 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 7f20e21..8f54871 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -59,23 +59,38 @@ static unsigned hash_name(const char *name, int namelen)\n \treturn val;\n }\n \n-static int invalid_attr_name(const char *name, int namelen)\n+int attr_name_valid(const char *name, size_t namelen)\n {\n \t/*\n \t * Attribute name cannot begin with '-' and must consist of\n \t * characters from [-A-Za-z0-9_.].\n \t */\n \tif (namelen <= 0 || *name == '-')\n-\t\treturn -1;\n+\t\treturn 0;\n \twhile (namelen--) {\n \t\tchar ch = *name++;\n \t\tif (! (ch == '-' || ch == '.' || ch == '_' ||\n \t\t       ('0' <= ch && ch <= '9') ||\n \t\t       ('a' <= ch && ch <= 'z') ||\n \t\t       ('A' <= ch && ch <= 'Z')) )\n-\t\t\treturn -1;\n+\t\t\treturn 0;\n \t}\n-\treturn 0;\n+\treturn -1;\n+}\n+\n+void invalid_attr_name_message(struct strbuf *err, const char *name, int len)\n+{\n+\tstrbuf_addf(err, _(\"%.*s is not a valid attribute name\"),\n+\t\t    len, name);\n+}\n+\n+static void report_invalid_attr(const char *name, size_t len,\n+\t\t\t\tconst char *src, int lineno)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tinvalid_attr_name_message(&err, name, len);\n+\tfprintf(stderr, \"%s: %s:%d\\n\", err.buf, src, lineno);\n+\tstrbuf_release(&err);\n }\n \n struct git_attr *git_attr_counted(const char *name, size_t len)\n@@ -90,7 +105,7 @@ struct git_attr *git_attr_counted(const char *name, size_t len)\n \t\t\treturn a;\n \t}\n \n-\tif (invalid_attr_name(name, len))\n+\tif (!attr_name_valid(name, len))\n \t\treturn NULL;\n \n \tFLEX_ALLOC_MEM(a, name, name, len);\n@@ -176,17 +191,15 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,\n \t\t\tcp++;\n \t\t\tlen--;\n \t\t}\n-\t\tif (invalid_attr_name(cp, len)) {\n-\t\t\tfprintf(stderr,\n-\t\t\t\t\"%.*s is not a valid attribute name: %s:%d\\n\",\n-\t\t\t\tlen, cp, src, lineno);\n+\t\tif (!attr_name_valid(cp, len)) {\n+\t\t\treport_invalid_attr(cp, len, src, lineno);\n \t\t\treturn NULL;\n \t\t}\n \t} else {\n \t\t/*\n \t\t * As this function is always called twice, once with\n \t\t * e == NULL in the first pass and then e != NULL in\n-\t\t * the second pass, no need for invalid_attr_name()\n+\t\t * the second pass, no need for attr_name_valid()\n \t\t * check here.\n \t\t */\n \t\tif (*cp == '-' || *cp == '!') {\n@@ -229,10 +242,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\tname += strlen(ATTRIBUTE_MACRO_PREFIX);\n \t\tname += strspn(name, blank);\n \t\tnamelen = strcspn(name, blank);\n-\t\tif (invalid_attr_name(name, namelen)) {\n-\t\t\tfprintf(stderr,\n-\t\t\t\t\"%.*s is not a valid attribute name: %s:%d\\n\",\n-\t\t\t\tnamelen, name, src, lineno);\n+\t\tif (!attr_name_valid(name, namelen)) {\n+\t\t\treport_invalid_attr(name, namelen, src, lineno);\n \t\t\treturn NULL;\n \t\t}\n \t}\ndiff --git a/attr.h b/attr.h\nindex 78d6d5a..fc72030 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -13,6 +13,9 @@ extern struct git_attr *git_attr(const char *);\n /* The same, but with counted string */\n extern struct git_attr *git_attr_counted(const char *, size_t);\n \n+extern int attr_name_valid(const char *name, size_t namelen);\n+extern void invalid_attr_name_message(struct strbuf *, const char *, int);\n+\n /* Internal use */\n extern const char git_attr__true[];\n extern const char git_attr__false[];\n-- \n2.8.2-759-geb611ab\n"},{"id":"286934","messageId":"CAGZ79kbMsitKNeb-D3gmTFwrRS-6ZiCZcmu1MP42UCd5AHcHbA@mail.gmail.com","threadId":"42373","inReplyTo":"xmqqmvnnqdtd.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] attr: add counted string version of git_attr()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-18T21:36:46Z","receivedAt":"2016-05-18T21:36:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 18, 2016 at 2:03 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Often a potential caller has <name, namelen> pair that\n> represents the name it wants to create an attribute out of.\n>\n> When name[namelen] is not NUL, the caller has to xmemdupz()\n> only to call git_attr().\n>\n> Add git_attr_counted() that takes such a counted string instead of\n> \"const char *name\".\n\ns/ \"const char *name\"/a standard C string that is null terminated/ maybe?\n\n\"const char *name\" is not a strong hint that it is not counted?\n"},{"id":"286935","messageId":"CAGZ79kYkeF-YTYqsLSsESWLcXmX5Dx-NdCnzmeBiRRnotZ49dA@mail.gmail.com","threadId":"42373","inReplyTo":"xmqqh9dvqdoy.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] attr: expose validity check for attribute names","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-18T21:42:07Z","receivedAt":"2016-05-18T21:42:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 18, 2016 at 2:05 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Export attr_name_valid() function, and a helper function that\n> returns the message to be given when a given <name, len> pair\n> is not a good name for an attribute.\n>\n> We could later update the message to exactly spell out what the\n> rules for a good attribute name are, etc.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  * And then finally the error message one.  I didn't change\n>    the message itself to spell out the rules here, though.\n>\n>  attr.c | 39 +++++++++++++++++++++++++--------------\n>  attr.h |  3 +++\n>  2 files changed, 28 insertions(+), 14 deletions(-)\n>\n> diff --git a/attr.c b/attr.c\n> index 7f20e21..8f54871 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -59,23 +59,38 @@ static unsigned hash_name(const char *name, int namelen)\n>         return val;\n>  }\n>\n> -static int invalid_attr_name(const char *name, int namelen)\n> +int attr_name_valid(const char *name, size_t namelen)\n\nMental note:\nThe patch series with the pathspec attrs on top of this series makes this\nnon-static and exposes the invalid_attr_name for wider use, so I'll resolve\nthe merge conflict accordingly.\n\n>  {\n>         /*\n>          * Attribute name cannot begin with '-' and must consist of\n>          * characters from [-A-Za-z0-9_.].\n>          */\n>         if (namelen <= 0 || *name == '-')\n> -               return -1;\n> +               return 0;\n>         while (namelen--) {\n>                 char ch = *name++;\n>                 if (! (ch == '-' || ch == '.' || ch == '_' ||\n>                        ('0' <= ch && ch <= '9') ||\n>                        ('a' <= ch && ch <= 'z') ||\n>                        ('A' <= ch && ch <= 'Z')) )\n> -                       return -1;\n> +                       return 0;\n>         }\n> -       return 0;\n> +       return -1;\n> +}\n> +\n> +void invalid_attr_name_message(struct strbuf *err, const char *name, int len)\n> +{\n> +       strbuf_addf(err, _(\"%.*s is not a valid attribute name\"),\n> +                   len, name);\n> +}\n> +\n> +static void report_invalid_attr(const char *name, size_t len,\n> +                               const char *src, int lineno)\n> +{\n> +       struct strbuf err = STRBUF_INIT;\n> +       invalid_attr_name_message(&err, name, len);\n> +       fprintf(stderr, \"%s: %s:%d\\n\", err.buf, src, lineno);\n> +       strbuf_release(&err);\n>  }\n>\n>  struct git_attr *git_attr_counted(const char *name, size_t len)\n> @@ -90,7 +105,7 @@ struct git_attr *git_attr_counted(const char *name, size_t len)\n>                         return a;\n>         }\n>\n> -       if (invalid_attr_name(name, len))\n> +       if (!attr_name_valid(name, len))\n>                 return NULL;\n>\n>         FLEX_ALLOC_MEM(a, name, name, len);\n> @@ -176,17 +191,15 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,\n>                         cp++;\n>                         len--;\n>                 }\n> -               if (invalid_attr_name(cp, len)) {\n> -                       fprintf(stderr,\n> -                               \"%.*s is not a valid attribute name: %s:%d\\n\",\n> -                               len, cp, src, lineno);\n> +               if (!attr_name_valid(cp, len)) {\n> +                       report_invalid_attr(cp, len, src, lineno);\n>                         return NULL;\n>                 }\n>         } else {\n>                 /*\n>                  * As this function is always called twice, once with\n>                  * e == NULL in the first pass and then e != NULL in\n> -                * the second pass, no need for invalid_attr_name()\n> +                * the second pass, no need for attr_name_valid()\n>                  * check here.\n>                  */\n>                 if (*cp == '-' || *cp == '!') {\n> @@ -229,10 +242,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n>                 name += strlen(ATTRIBUTE_MACRO_PREFIX);\n>                 name += strspn(name, blank);\n>                 namelen = strcspn(name, blank);\n> -               if (invalid_attr_name(name, namelen)) {\n> -                       fprintf(stderr,\n> -                               \"%.*s is not a valid attribute name: %s:%d\\n\",\n> -                               namelen, name, src, lineno);\n> +               if (!attr_name_valid(name, namelen)) {\n> +                       report_invalid_attr(name, namelen, src, lineno);\n>                         return NULL;\n>                 }\n>         }\n> diff --git a/attr.h b/attr.h\n> index 78d6d5a..fc72030 100644\n> --- a/attr.h\n> +++ b/attr.h\n> @@ -13,6 +13,9 @@ extern struct git_attr *git_attr(const char *);\n>  /* The same, but with counted string */\n>  extern struct git_attr *git_attr_counted(const char *, size_t);\n>\n> +extern int attr_name_valid(const char *name, size_t namelen);\n\nok, I agree with this one.\n\n> +extern void invalid_attr_name_message(struct strbuf *, const char *, int);\n\nThis makes sense, too, so the caller can just collect the error message and\nattach its own flair to it, i.e. another message to explain the context about\nwrong attributes.\n\nThanks,\nStefan\n\n> +\n>  /* Internal use */\n>  extern const char git_attr__true[];\n>  extern const char git_attr__false[];\n> --\n> 2.8.2-759-geb611ab\n>\n"},{"id":"286937","messageId":"CAGZ79ka0Of6S3AGx24jrvso=AwMsxQOnWVFZA-XWy3590JbVgA@mail.gmail.com","threadId":"42373","inReplyTo":"xmqq7ferrvvd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv7 5/5] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-18T22:31:45Z","receivedAt":"2016-05-18T22:31:45Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 18, 2016 at 12:47 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> + - \"`+`\" the attribute must be set\n>> + - \"`-`\" the attribute must be unset\n>> + - \"`~`\" the attribute must be unspecified\n\nWell the '-' is in line, at least.\nSo you're proposing empty string for set?\n\ngitattributes documentation:\n> Sometimes you would need to override an setting of an attribute for a\n> path to Unspecified state. This can be done by listing the name of the\n> attribute prefixed with an exclamation point !.\n\nSo the unspecified should be '!' to match Git consistency.\n\n[I dislike the exclamation mark because of bash.\n    git ls-files \":(attr:!text)\"\n    bash: !text: event not found\n    git ls-files \":(attr:\\!text)\"\n    fatal: attr spec '\\!text': attrs must not start with '-' and be\ncomposed of [-A-Za-z0-9_.].\n    # we error out because of the \\ in there\n    git ls-files :\\(attr:\\!text\\)\n    ...\n    # that actually works.\n]\n\n>\n> I think using these three, not the way how .gitattributes specify\n> them, is highly confusing to the users.\n\nOk I'll align those.\n\n>\n>> + - \"`?`\" the attribute must not be unspecified, i.e. set, unset or value matches\n>\n> The choice of '?' is OK, as there is no reason .gitattributes wants\n> to say something fuzzy like \"this is set or unset or has value\".\n> The last part \"set, unset or value matches\" does not make sense,\n> though.  Did you mean \"set, unset or set to value\"?\n\nActually I meant \"this is set or unset or has [any] value\".\n\n>\n>> + - an empty \"`[mode]`\" matches if the attribute is set or has a value\n>\n> The same comment as +/-/~ above applies.\n\nSo consistency would ask for\n\n - an empty \"`[mode]`\" matches if the attribute is set\n - \"`-`\" the attribute must be unset\n - \"`!`\" the attribute must be unspecified\n - an empty \"`[mode]`\" combined with \"`[=value]`\" matches if the attribute has\n   the given value.\n\nand those are not yet codified, but for discussion then:\n\n - \"`?`\" the attribute must not be unspecified, i.e. set, unset or has any value\n - \"`+`\" the attribute must be set or has any value\n\n>> +\n>> +             match_mode = item->attr_match[i].match_mode;\n>\n> Mental note: there is a one-to-one correspondence between\n> attr_check->check[] and attr_match[].\n\nRight. I guess I should add a comment for that. Or better yet a test\nwith some weird pathspec making use of that\n\n    git ls-files :(attr:labelA !labelB +labelA)\n\nThat way we would see if there will be confusion between the modes\nand attr names in the future.\n\n>\n>> +             if (!matched)\n>> +                     return 0;\n>\n> So this ANDs together.  OK.\n\nSure, (it's clear to me, maybe I should document that as well),\nbecause multiple pathspecs are OR.\n\n    git ls-files :(attr:labelA) :(attr:labelB)\n\n>\n> Mental note after skiming the caller:\n>\n> The \"value\" here is like \"VAR1=VAL1 VAR2 !VAR3\" in a pathspec\n> \":(attr:VAR1=VAL1 VAR2 !VAR3)path/to/limit\", i.e. the source string\n> to be compiled into a list of conditionals that will be evaluated by\n> match_attr() in dir.c\n\nRight.\n\n>> +     string_list_split(&list, value, ' ', -1);\n>> +     string_list_remove_empty_items(&list, 0);\n\nRight.\n\n>\n> At this point, we got \"VAR1=VAL1\", \"VAR2\", \"!VAR3\" as elements in\n> this list.\n\nRight.\n\n>\n>> +     if (!item->attr_check)\n>> +             item->attr_check = git_attr_check_alloc();\n>\n> Given more than one \"attr\" magic, e.g. \":(attr:A=a,attr:B)/path\",\n> the check may not be empty when we process the second one; we just\n> extend it without losing the existing contents.\n\nThat is why I am not super happy with it though.\n\n    \":(attr:A=a,attr:B)/path\",\n    \":(attr:A=a B)/path\",\n\nare the same for the user as well as in the internal data structures.\nThis \"wastes\" the white space as a possible convenient separator\ncharacter, e.g. for multiple values. On the other hand it will be easier\nto type, specially for many attrs (read submodule groups).\n\n>\n>> +     ALLOC_GROW(item->attr_match, item->attr_match_nr + list.nr, item->attr_match_alloc);\n>\n> Likewise, we extend attr_match[] array.\n\n>> +\n>> +             val_len = strcspn(val, \"=,)\");\n>\n> I understand \"=\", but can \",\" and \")\" appear here?\n\nThis was overly caution from some intermediate state, where the caller\nhanded in more than required.\n\n    if (skip_prefix(copyfrom, \"attr:\", &body)) {\n        char *pass = xmemdupz(body, len - strlen(\"attr:\"));\n        parse_pathspec_attr_match(item, pass);\n        free(pass);\n        continue;\n    }\n\nWhen given \":(attr:VAR1=VAL1 VAR2 !VAR3)path/to/limit\"\nthis only passes in \"VAR1=VAL1 VAR2 !VAR3\".\n\nI contemplated write the caller as\n\n    if (skip_prefix(copyfrom, \"attr:\", &body)) {\n        parse_pathspec_attr_match(item, body);\n        continue;\n    }\n\nbut that would include further characters, i.e. ',' or ')'.\n(but not a lot more)\n\nI wanted to pass in an uncounted string though, as\nstring_list_separate doesn't offer a variant for counted\nstrings.\n\n>\n>> +             if (val[val_len] == '=') {\n>> +                     am->match_mode = MATCH_VALUE;\n>> +                     am->value = xstrdup(&val[val_len + 1]);\n>> +                     /*\n>> +                      * NEEDSWORK:\n>> +                      * Do we want to allow escaped commas to search\n>> +                      * for comma separated values?\n>> +                      */\n>\n> No, we don't ;-).\n\nok will drop comment, ...\n\n>\n>> +                     if (strchr(am->value, '\\\\'))\n>> +                             die(_(\"attr spec values must not contain backslashes\"));\n>\n> But this is a good escape hatch to reserve for our own use, until we\n> decide what the quoting mechanism will be (or if it is necessary).\n\nbut keep the code here\n\n>\n>> +             } else\n>> +                     am->value = NULL;\n>> +\n>> +             if (invalid_attr_name(val, val_len)) {\n>> +                     am->match_mode = INVALID_ATTR;\n>> +                     goto err;\n>> +             }\n>> +\n>> +             am->attr = git_attr(xmemdupz(val, val_len));\n>> +             git_attr_check_append(item->attr_check, am->attr);\n>\n> GOOD!\n\nexcept for the memory leak of xmemdupz. git_attr makes another\ninternal copy with FLEX_ALLOC_MEM. That ill be fixed once we use\ngit_attr_counted though.\n\n\n\n>\n> I think val and val_len should be renamed to attr and attr_len (in\n> the VARIABLE=VALUE context, these two identifiers are about parsing\n> the variable side, not the value side).\n\nok\n\n>\n>> +     }\n>> +\n>> +     string_list_clear(&list, 0);\n>> +     return;\n>> +err:\n>> +     die(_(\"attr spec '%s': attrs must not start with '-' and \"\n>> +           \"be composed of [-A-Za-z0-9_.].\"), value);\n>\n> What is \"value\" at this point?  If you failed to parse the second\n> element in \"VAR1=VAL1 FR*TZ=VAL2 !VAR3\", I think you would want to\n> say \"'FR*TZ' is malformed\".\n>\n> Existing users of the function seems to say this:\n>\n>                 if (invalid_attr_name(cp, len)) {\n>                         fprintf(stderr,\n>                                 \"%.*s is not a valid attribute name: %s:%d\\n\",\n>                                 len, cp, src, lineno);\n>                         return NULL;\n>                 }\n>\n> when parsing .gitattribute file.  The source file:line reference\n> does not apply to this context, but it would be better to unify the\n> message.  I do not mind spelling the rules out explicitly like you\n> did, but I do not want to see it spread across many places (which\n> forces us to update them when we have to change the rule later).\n>\n> Perhaps we want a helper function in attr.c side that takes the\n> attribute name string (val, val_len in your code above, which I\n> suggest to be renamed to attr, attr_len) and formats the error\n> message into a strbuf, or something?\n\nThat's why you added the reporting function to attrs, I see.\n\n>\n> Makes me wonder what \"pass\" stands for.  From the look of xmemdupz(),\n> given \":(attr:VAR1=VAL1 VAR2 !VAR3)path/to/limit\" to parse, len\n> points at \")\" and xmemdupz() gives \"VAR1=VAL1 VAR2 !VAR3\" to it.\n\n\"what we _pass_ to the parse_pathspec_attr_match\"... Maybe attr_body?\n\nThanks,\nStefan\n"},{"id":"286948","messageId":"xmqq4m9vq8fr.fsf@gitster.mtv.corp.google.com","threadId":"42373","inReplyTo":"CAGZ79ka0Of6S3AGx24jrvso=AwMsxQOnWVFZA-XWy3590JbVgA@mail.gmail.com","subject":"Re: [PATCHv7 5/5] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-18T22:59:20Z","receivedAt":"2016-05-18T22:59:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> and those are not yet codified, but for discussion then:\n>\n>  - \"`?`\" the attribute must not be unspecified, i.e. set, unset or has any value\n>  - \"`+`\" the attribute must be set or has any value\n\nI'd suggest not to support these until we see any concrete and\nconvincing example use case to tell us why this is useful.\n\n> That is why I am not super happy with it though.\n>\n>     \":(attr:A=a,attr:B)/path\",\n>     \":(attr:A=a B)/path\",\n>\n> are the same for the user as well as in the internal data structures.\n\nYou can interpret the former as ORed :(attr:A=a) and :(attr:B) if\nyou really wanted to.  You left the door open for such a future\nextension by explicitly rejecting multiple attributes added to a\nsingle pathspec element in earlier round, which was good.  You could\ndo the same here if you do not want to code that ORed ANDs, so that\nit (or some other semantics) can be introduced later without breaking\nthe mental model the users would form with the initial implementation.\n\n>>> +             val_len = strcspn(val, \"=,)\");\n>>\n>> I understand \"=\", but can \",\" and \")\" appear here?\n>\n> This was overly caution from some intermediate state, where the caller\n> handed in more than required.\n\nIs that being overly cautious?\n\nIt looks to me more like being sloppy and sweeping bugs in callers\nthat you could have diagnosed here.\n\nIf you didn't have \",)\" in the cspn set above, at least you would\nsee the effect caused by a broken caller.  E.g. \":(attr:FOO,icase)\"\nwould be given to you as \"FOO,icase)\" by a broken caller, you would\nsplit \"FOO,icase\" as one of the SP separated items, and try to parse\nit as an attribute name.  You can notice the breakage of the caller\nat that time.  With \",)\" in cspn set, you silently pass the parameter\ngiven by a broken caller.\n"}]}