{"thread":{"id":"45334","subject":"[PATCH 0/2] bringing attributes to pathspecs","startedAt":"2017-03-09T21:08:15Z","lastAt":"2017-03-21T16:58:35Z","messageCount":23,"participants":["Brandon Williams","Stefan Beller","Jonathan Tan","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"313679","messageId":"20170309210756.105566-1-bmwill@google.com","threadId":"45334","inReplyTo":null,"subject":"[PATCH 0/2] bringing attributes to pathspecs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-09T21:07:54Z","receivedAt":"2017-03-09T21:08:15Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"This small series extends the pathspec magic to allow users to specify\nattributes that files must have in order for a pathspec to 'match' a file.\n\nOne potential use for this is to allow a repository to specify attributes for a\nset of files.  The user can then specify that attribute as a pathspec to\nperform various operations only on that set of files. One simple example:\n\n\tgit ls-files -- \":(attr:text)\"\n\ncan be used to list all of the files with the 'text' attribute.\n\nBrandon Williams (2):\n  pathspec: allow querying for attributes\n  pathspec: allow escaped query values\n\n Documentation/glossary-content.txt |  20 ++++\n attr.c                             |  17 ++++\n attr.h                             |   1 +\n dir.c                              |  43 ++++++++-\n pathspec.c                         | 165 ++++++++++++++++++++++++++++++--\n pathspec.h                         |  16 +++-\n t/t6135-pathspec-with-attrs.sh     | 190 +++++++++++++++++++++++++++++++++++++\n 7 files changed, 442 insertions(+), 10 deletions(-)\n create mode 100755 t/t6135-pathspec-with-attrs.sh\n\n-- \n2.12.0.246.ga2ecc84866-goog\n\n"},{"id":"313680","messageId":"20170309210756.105566-2-bmwill@google.com","threadId":"45334","inReplyTo":"20170309210756.105566-1-bmwill@google.com","subject":"[PATCH 1/2] pathspec: allow querying for attributes","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-09T21:07:55Z","receivedAt":"2017-03-09T21:08:18Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"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\nBased on a patch by Stefan Beller <sbeller@google.com>\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n Documentation/glossary-content.txt |  20 ++++\n attr.c                             |  17 ++++\n attr.h                             |   1 +\n dir.c                              |  43 ++++++++-\n pathspec.c                         | 119 +++++++++++++++++++++++-\n pathspec.h                         |  16 +++-\n t/t6135-pathspec-with-attrs.sh     | 181 +++++++++++++++++++++++++++++++++++++\n 7 files changed, 388 insertions(+), 9 deletions(-)\n create mode 100755 t/t6135-pathspec-with-attrs.sh\n\ndiff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\nindex fc9320e59..5c32d1905 100644\n--- a/Documentation/glossary-content.txt\n+++ b/Documentation/glossary-content.txt\n@@ -384,6 +384,26 @@ full pathname may have special meaning:\n +\n Glob magic is incompatible with literal magic.\n \n+attr;;\n+After `attr:` comes a space separated list of \"attribute\n+requirements\", all of which must be met in order for the\n+path to be considered a match; this is in addition to the\n+usual non-magic pathspec pattern matching.\n++\n+Each of the attribute requirements for the path takes one of\n+these forms:\n+\n+- \"`ATTR`\" requires that the attribute `ATTR` must be set.\n+\n+- \"`-ATTR`\" requires that the attribute `ATTR` must be unset.\n+\n+- \"`ATTR=VALUE`\" requires that the attribute `ATTR` must be\n+  set to the string `VALUE`.\n+\n+- \"`!ATTR`\" requires that the attribute `ATTR` must be\n+  unspecified.\n++\n+\n exclude;;\n \tAfter a path matches any non-exclude pathspec, it will be run\n \tthrough all exclude pathspec (magic signature: `!` or its\ndiff --git a/attr.c b/attr.c\nindex 5493bff22..7e2134471 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -603,6 +603,23 @@ struct attr_check *attr_check_initl(const char *one, ...)\n \treturn check;\n }\n \n+struct attr_check *attr_check_dup(const struct attr_check *check)\n+{\n+\tstruct attr_check *ret;\n+\n+\tif (!check)\n+\t\treturn NULL;\n+\n+\tret = attr_check_alloc();\n+\n+\tret->nr = check->nr;\n+\tret->alloc = check->alloc;\n+\tALLOC_ARRAY(ret->items, ret->nr);\n+\tCOPY_ARRAY(ret->items, check->items, ret->nr);\n+\n+\treturn ret;\n+}\n+\n struct attr_check_item *attr_check_append(struct attr_check *check,\n \t\t\t\t\t  const struct git_attr *attr)\n {\ndiff --git a/attr.h b/attr.h\nindex 48ab3e1c2..442d464db 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -44,6 +44,7 @@ struct attr_check {\n \n extern struct attr_check *attr_check_alloc(void);\n extern struct attr_check *attr_check_initl(const char *, ...);\n+extern struct attr_check *attr_check_dup(const struct attr_check *check);\n \n extern struct attr_check_item *attr_check_append(struct attr_check *check,\n \t\t\t\t\t\t const struct git_attr *attr);\ndiff --git a/dir.c b/dir.c\nindex 4541f9e14..2fe7acbcf 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@@ -134,7 +135,8 @@ static size_t common_prefix_len(const struct pathspec *pathspec)\n \t\t       PATHSPEC_LITERAL |\n \t\t       PATHSPEC_GLOB |\n \t\t       PATHSPEC_ICASE |\n-\t\t       PATHSPEC_EXCLUDE);\n+\t\t       PATHSPEC_EXCLUDE |\n+\t\t       PATHSPEC_ATTR);\n \n \tfor (n = 0; n < pathspec->nr; n++) {\n \t\tsize_t i = 0, len = 0, item_len;\n@@ -209,6 +211,36 @@ int within_depth(const char *name, int namelen,\n #define DO_MATCH_DIRECTORY (1<<1)\n #define DO_MATCH_SUBMODULE (1<<2)\n \n+static int match_attrs(const char *name, int namelen,\n+\t\t       const struct pathspec_item *item)\n+{\n+\tint i;\n+\n+\tgit_check_attr(name, item->attr_check);\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->items[i].value;\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\telse if (ATTR_FALSE(value))\n+\t\t\tmatched = (match_mode == MATCH_UNSET);\n+\t\telse if (ATTR_UNSET(value))\n+\t\t\tmatched = (match_mode == MATCH_UNSPECIFIED);\n+\t\telse\n+\t\t\tmatched = (match_mode == MATCH_VALUE &&\n+\t\t\t\t   !strcmp(item->attr_match[i].value, value));\n+\t\tif (!matched)\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n /*\n  * Does 'match' match the given name?\n  * A match is found if\n@@ -261,6 +293,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;\n@@ -339,7 +374,8 @@ static int do_match_pathspec(const struct pathspec *ps,\n \t\t       PATHSPEC_LITERAL |\n \t\t       PATHSPEC_GLOB |\n \t\t       PATHSPEC_ICASE |\n-\t\t       PATHSPEC_EXCLUDE);\n+\t\t       PATHSPEC_EXCLUDE |\n+\t\t       PATHSPEC_ATTR);\n \n \tif (!ps->nr) {\n \t\tif (!ps->recursive ||\n@@ -1361,7 +1397,8 @@ static int simplify_away(const char *path, int pathlen,\n \t\t       PATHSPEC_LITERAL |\n \t\t       PATHSPEC_GLOB |\n \t\t       PATHSPEC_ICASE |\n-\t\t       PATHSPEC_EXCLUDE);\n+\t\t       PATHSPEC_EXCLUDE |\n+\t\t       PATHSPEC_ATTR);\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tconst struct pathspec_item *item = &pathspec->items[i];\ndiff --git a/pathspec.c b/pathspec.c\nindex b961f00c8..583ed5208 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@@ -72,6 +73,7 @@ static struct pathspec_magic {\n \t{ PATHSPEC_GLOB,    '\\0', \"glob\" },\n \t{ PATHSPEC_ICASE,   '\\0', \"icase\" },\n \t{ PATHSPEC_EXCLUDE,  '!', \"exclude\" },\n+\t{ PATHSPEC_ATTR,    '\\0', \"attr\" },\n };\n \n static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n@@ -87,6 +89,72 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\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+\tif (item->attr_check)\n+\t\tdie(_(\"Only one 'attr:' specification is allowed.\"));\n+\n+\tif (!value || !strlen(value))\n+\t\tdie(_(\"attr spec must not be empty\"));\n+\n+\tstring_list_split(&list, value, ' ', -1);\n+\tstring_list_remove_empty_items(&list, 0);\n+\n+\titem->attr_check = attr_check_alloc();\n+\tALLOC_GROW(item->attr_match,\n+\t\t   item->attr_match_nr + list.nr,\n+\t\t   item->attr_match_alloc);\n+\n+\tfor_each_string_list_item(si, &list) {\n+\t\tsize_t attr_len;\n+\t\tchar *attr_name;\n+\t\tconst struct git_attr *a;\n+\n+\t\tint j = item->attr_match_nr++;\n+\t\tconst char *attr = si->string;\n+\t\tstruct attr_match *am = &item->attr_match[j];\n+\n+\t\tswitch (*attr) {\n+\t\tcase '!':\n+\t\t\tam->match_mode = MATCH_UNSPECIFIED;\n+\t\t\tattr++;\n+\t\t\tattr_len = strlen(attr);\n+\t\t\tbreak;\n+\t\tcase '-':\n+\t\t\tam->match_mode = MATCH_UNSET;\n+\t\t\tattr++;\n+\t\t\tattr_len = strlen(attr);\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tattr_len = strcspn(attr, \"=\");\n+\t\t\tif (attr[attr_len] != '=')\n+\t\t\t\tam->match_mode = MATCH_SET;\n+\t\t\telse {\n+\t\t\t\tam->match_mode = MATCH_VALUE;\n+\t\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n+\t\t\t\tif (strchr(am->value, '\\\\'))\n+\t\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n+\t\t\t}\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tattr_name = xmemdupz(attr, attr_len);\n+\t\ta = git_attr(attr_name);\n+\t\tif (!a)\n+\t\t\tdie(_(\"invalid attribute name %s\"), attr_name);\n+\n+\t\tattr_check_append(item->attr_check, a);\n+\n+\t\tfree(attr_name);\n+\t}\n+\n+\tstring_list_clear(&list, 0);\n+\treturn;\n+}\n+\n static inline int get_literal_global(void)\n {\n \tstatic int literal = -1;\n@@ -164,6 +232,7 @@ static int get_global_magic(int element_magic)\n  * returns the position in 'elem' after all magic has been parsed\n  */\n static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n+\t\t\t\t    struct pathspec_item *item,\n \t\t\t\t    const char *elem)\n {\n \tconst char *pos;\n@@ -189,6 +258,14 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (starts_with(pos, \"attr:\")) {\n+\t\t\tchar *attr_body = xmemdupz(pos + 5, len - 5);\n+\t\t\tparse_pathspec_attr_match(item, attr_body);\n+\t\t\t*magic |= PATHSPEC_ATTR;\n+\t\t\tfree(attr_body);\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, pos, len)) {\n@@ -252,13 +329,14 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)\n }\n \n static const char *parse_element_magic(unsigned *magic, int *prefix_len,\n+\t\t\t\t       struct pathspec_item *item,\n \t\t\t\t       const char *elem)\n {\n \tif (elem[0] != ':' || get_literal_global())\n \t\treturn elem; /* nothing to do */\n \telse if (elem[1] == '(')\n \t\t/* longhand */\n-\t\treturn parse_long_magic(magic, prefix_len, elem);\n+\t\treturn parse_long_magic(magic, prefix_len, item, elem);\n \telse\n \t\t/* shorthand */\n \t\treturn parse_short_magic(magic, elem);\n@@ -335,12 +413,18 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \tchar *match;\n \tint pathspec_prefix = -1;\n \n+\titem->attr_check = NULL;\n+\titem->attr_match = NULL;\n+\titem->attr_match_nr = 0;\n+\titem->attr_match_alloc = 0;\n+\n \t/* PATHSPEC_LITERAL_PATH ignores magic */\n \tif (flags & PATHSPEC_LITERAL_PATH) {\n \t\tmagic = PATHSPEC_LITERAL;\n \t} else {\n \t\tcopyfrom = parse_element_magic(&element_magic,\n \t\t\t\t\t       &pathspec_prefix,\n+\t\t\t\t\t       item,\n \t\t\t\t\t       elt);\n \t\tmagic |= element_magic;\n \t\tmagic |= get_global_magic(element_magic);\n@@ -544,6 +628,10 @@ 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_check &&\n+\t\t    item[i].attr_check->nr != item[i].attr_match_nr)\n+\t\t\tdie(\"BUG: should have same number of entries\");\n \t}\n \n \t/*\n@@ -565,26 +653,47 @@ void parse_pathspec(struct pathspec *pathspec,\n \n void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n {\n-\tint i;\n+\tint i, j;\n \n \t*dst = *src;\n \tALLOC_ARRAY(dst->items, dst->nr);\n \tCOPY_ARRAY(dst->items, src->items, dst->nr);\n \n \tfor (i = 0; i < dst->nr; i++) {\n-\t\tdst->items[i].match = xstrdup(src->items[i].match);\n-\t\tdst->items[i].original = xstrdup(src->items[i].original);\n+\t\tstruct pathspec_item *d = &dst->items[i];\n+\t\tstruct pathspec_item *s = &src->items[i];\n+\n+\t\td->match = xstrdup(s->match);\n+\t\td->original = xstrdup(s->original);\n+\n+\t\tALLOC_ARRAY(d->attr_match, d->attr_match_nr);\n+\t\tCOPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr);\n+\t\tfor (j = 0; j < d->attr_match_nr; j++) {\n+\t\t\tconst char *value = s->attr_match[j].value;\n+\t\t\tif (value)\n+\t\t\t\td->attr_match[j].value = xstrdup(value);\n+\t\t}\n+\n+\t\td->attr_check = attr_check_dup(s->attr_check);\n \t}\n }\n \n void clear_pathspec(struct pathspec *pathspec)\n {\n-\tint i;\n+\tint i, j;\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tfree(pathspec->items[i].match);\n \t\tfree(pathspec->items[i].original);\n+\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+\n+\t\tif (pathspec->items[i].attr_check)\n+\t\t\tattr_check_free(pathspec->items[i].attr_check);\n \t}\n+\n \tfree(pathspec->items);\n \tpathspec->items = NULL;\n \tpathspec->nr = 0;\ndiff --git a/pathspec.h b/pathspec.h\nindex 49fd823dd..83625f006 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -8,13 +8,15 @@\n #define PATHSPEC_GLOB\t\t(1<<3)\n #define PATHSPEC_ICASE\t\t(1<<4)\n #define PATHSPEC_EXCLUDE\t(1<<5)\n+#define PATHSPEC_ATTR\t\t(1<<6)\n #define PATHSPEC_ALL_MAGIC\t  \\\n \t(PATHSPEC_FROMTOP\t| \\\n \t PATHSPEC_MAXDEPTH\t| \\\n \t PATHSPEC_LITERAL\t| \\\n \t PATHSPEC_GLOB\t\t| \\\n \t PATHSPEC_ICASE\t\t| \\\n-\t PATHSPEC_EXCLUDE)\n+\t PATHSPEC_EXCLUDE\t| \\\n+\t PATHSPEC_ATTR)\n \n #define PATHSPEC_ONESTAR 1\t/* the pathspec pattern satisfies GFNM_ONESTAR */\n \n@@ -31,6 +33,18 @@ 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\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} match_mode;\n+\t\t} *attr_match;\n+\t\tstruct attr_check *attr_check;\n \t} *items;\n };\n \ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nnew file mode 100755\nindex 000000000..b5e5a0607\n--- /dev/null\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -0,0 +1,181 @@\n+#!/bin/sh\n+\n+test_description='test labels in pathspecs'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a tree' '\n+\tcat <<-EOF >expect &&\n+\tfileA\n+\tfileAB\n+\tfileAC\n+\tfileB\n+\tfileBC\n+\tfileC\n+\tfileNoLabel\n+\tfileSetLabel\n+\tfileUnsetLabel\n+\tfileValue\n+\tfileWrongLabel\n+\tsub/fileA\n+\tsub/fileAB\n+\tsub/fileAC\n+\tsub/fileB\n+\tsub/fileBC\n+\tsub/fileC\n+\tsub/fileNoLabel\n+\tsub/fileSetLabel\n+\tsub/fileUnsetLabel\n+\tsub/fileValue\n+\tsub/fileWrongLabel\n+\tEOF\n+\tmkdir sub &&\n+\twhile read path\n+\tdo\n+\t\t: >$path &&\n+\t\tgit add $path || return 1\n+\tdone <expect &&\n+\tgit commit -m \"initial commit\" &&\n+\tgit ls-files >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pathspec with no attr' '\n+\ttest_must_fail git ls-files \":(attr:)\"\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+\tfileA labelA\n+\tfileB labelB\n+\tfileC labelC\n+\tfileAB labelA labelB\n+\tfileAC labelA labelC\n+\tfileBC labelB labelC\n+\tfileUnsetLabel -label\n+\tfileSetLabel label\n+\tfileValue label=foo\n+\tfileWrongLabel label☺\n+\tEOF\n+\tgit add .gitattributes &&\n+\tgit commit -m \"add attributes\"\n+'\n+\n+test_expect_success 'check specific set attr' '\n+\tcat <<-EOF >expect &&\n+\tfileSetLabel\n+\tsub/fileSetLabel\n+\tEOF\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+\tfileUnsetLabel\n+\tsub/fileUnsetLabel\n+\tEOF\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+\tfileValue\n+\tsub/fileValue\n+\tEOF\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 unspecified attr' '\n+\tcat <<-EOF >expect &&\n+\t.gitattributes\n+\tfileA\n+\tfileAB\n+\tfileAC\n+\tfileB\n+\tfileBC\n+\tfileC\n+\tfileNoLabel\n+\tfileWrongLabel\n+\tsub/fileA\n+\tsub/fileAB\n+\tsub/fileAC\n+\tsub/fileB\n+\tsub/fileBC\n+\tsub/fileC\n+\tsub/fileNoLabel\n+\tsub/fileWrongLabel\n+\tEOF\n+\tgit ls-files \":(attr:!label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check multiple unspecified attr' '\n+\tcat <<-EOF >expect &&\n+\t.gitattributes\n+\tfileC\n+\tfileNoLabel\n+\tfileWrongLabel\n+\tsub/fileC\n+\tsub/fileNoLabel\n+\tsub/fileWrongLabel\n+\tEOF\n+\tgit ls-files \":(attr:!labelB !labelA !label)\" >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+\tfileAB\n+\tfileB\n+\tfileBC\n+\tEOF\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+\tfileAB\n+\tfileB\n+\tfileBC\n+\tsub/fileAB\n+\tsub/fileB\n+\tEOF\n+\tgit ls-files \":(attr:labelB)\" \":(exclude,attr:labelC)sub/\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n+\ttest_must_fail git ls-files . \":(attr:labelB,attr:labelC)\" 2>actual &&\n+\ttest_i18ngrep \"Only one\" actual\n+'\n+\n+test_expect_success 'fail if attr magic is used places not implemented' '\n+\t# The main purpose of this test is to check that we actually fail\n+\t# when you attempt to use attr magic in commands that do not implement\n+\t# attr magic. This test does not advocate git-add to stay that way,\n+\t# though, but git-add is convenient as it has its own internal pathspec\n+\t# parsing.\n+\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n+\ttest_i18ngrep \"unsupported magic\" actual\n+'\n+\n+test_expect_success 'abort on giving invalid label on the command line' '\n+\ttest_must_fail git ls-files . \":(attr:☺)\"\n+'\n+\n+test_expect_success 'abort on asking for wrong magic' '\n+\ttest_must_fail git ls-files . \":(attr:-label=foo)\" &&\n+\ttest_must_fail git ls-files . \":(attr:!label=foo)\"\n+'\n+\n+test_done\n-- \n2.12.0.246.ga2ecc84866-goog\n\n"},{"id":"313681","messageId":"20170309210756.105566-3-bmwill@google.com","threadId":"45334","inReplyTo":"20170309210756.105566-1-bmwill@google.com","subject":"[PATCH 2/2] pathspec: allow escaped query values","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-09T21:07:56Z","receivedAt":"2017-03-09T21:08:20Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"In our own .gitattributes file we have attributes such as:\n\n    *.[ch] whitespace=indent,trail,space\n\nWhen querying for attributes we want to be able to ask for the exact\nvalue, i.e.\n\n    git ls-files :(attr:whitespace=indent,trail,space)\n\nshould work, but the commas are used in the attr magic to introduce\nthe next attr, such that this query currently fails with\n\nfatal: Invalid pathspec magic 'trail' in ':(attr:whitespace=indent,trail,space)'\n\nThis change allows escaping characters by a backslash, such that the query\n\n    git ls-files :(attr:whitespace=indent\\,trail\\,space)\n\nwill match all path that have the value \"indent,trail,space\" for the\nwhitespace attribute. To accomplish this, we need to modify two places.\nFirst `parse_long_magic` needs to not stop early upon seeing a comma or\nclosing paren that is escaped. As a second step we need to remove any\nescaping from the attr value.\n\nBased on a patch by Stefan Beller <sbeller@google.com>\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n pathspec.c                     | 52 ++++++++++++++++++++++++++++++++++++++----\n t/t6135-pathspec-with-attrs.sh |  9 ++++++++\n 2 files changed, 57 insertions(+), 4 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 583ed5208..f89f84a83 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -89,6 +89,51 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+static size_t strcspn_escaped(const char *s, const char *stop)\n+{\n+\tconst char *i;\n+\n+\tfor (i = s; *i; i++) {\n+\t\t/* skip the escaped character */\n+\t\tif (i[0] == '\\\\' && i[1]) {\n+\t\t\ti++;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (strchr(stop, *i))\n+\t\t\tbreak;\n+\t}\n+\treturn i - s;\n+}\n+\n+static inline int invalid_value_char(const char ch)\n+{\n+\tif (isalnum(ch) || strchr(\",-_\", ch))\n+\t\treturn 0;\n+\treturn -1;\n+}\n+\n+static char *attr_value_unescape(const char *value)\n+{\n+\tconst char *src;\n+\tchar *dst, *ret;\n+\n+\tret = xmallocz(strlen(value));\n+\tfor (src = value, dst = ret; *src; src++, dst++) {\n+\t\tif (*src == '\\\\') {\n+\t\t\tif (!src[1])\n+\t\t\t\tdie(_(\"Escape character '\\\\' not allowed as \"\n+\t\t\t\t      \"last character in attr value\"));\n+\t\t\tsrc++;\n+\t\t}\n+\t\tif (invalid_value_char(*src))\n+\t\t\tdie(\"cannot use '%c' for value matching\", *src);\n+\t\t*dst = *src;\n+\t}\n+\t*dst = '\\0';\n+\treturn ret;\n+}\n+\n static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)\n {\n \tstruct string_list_item *si;\n@@ -133,10 +178,9 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va\n \t\t\tif (attr[attr_len] != '=')\n \t\t\t\tam->match_mode = MATCH_SET;\n \t\t\telse {\n+\t\t\t\tconst char *v = &attr[attr_len + 1];\n \t\t\t\tam->match_mode = MATCH_VALUE;\n-\t\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n-\t\t\t\tif (strchr(am->value, '\\\\'))\n-\t\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n+\t\t\t\tam->value = attr_value_unescape(v);\n \t\t\t}\n \t\t\tbreak;\n \t\t}\n@@ -239,7 +283,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n \tconst char *nextat;\n \n \tfor (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {\n-\t\tsize_t len = strcspn(pos, \",)\");\n+\t\tsize_t len = strcspn_escaped(pos, \",)\");\n \t\tint i;\n \n \t\tif (pos[len] == ',')\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex b5e5a0607..585d17bad 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -178,4 +178,13 @@ test_expect_success 'abort on asking for wrong magic' '\n \ttest_must_fail git ls-files . \":(attr:!label=foo)\"\n '\n \n+test_expect_success 'check attribute list' '\n+\tcat <<-EOF >>.gitattributes &&\n+\t* whitespace=indent,trail,space\n+\tEOF\n+\tgit ls-files \":(attr:whitespace=indent\\,trail\\,space)\" >actual &&\n+\tgit ls-files >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.12.0.246.ga2ecc84866-goog\n\n"},{"id":"313682","messageId":"CAGZ79ka=kV_QQo3ExHmieNUajCvzZejFfmDsKeHr9XR0QvEVmA@mail.gmail.com","threadId":"45334","inReplyTo":"20170309210756.105566-1-bmwill@google.com","subject":"Re: [PATCH 0/2] bringing attributes to pathspecs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-09T21:22:28Z","receivedAt":"2017-03-09T21:23:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 9, 2017 at 1:07 PM, Brandon Williams <bmwill@google.com> wrote:\n> This small series extends the pathspec magic to allow users to specify\n> attributes that files must have in order for a pathspec to 'match' a file.\n>\n> One potential use for this is to allow a repository to specify attributes for a\n> set of files.  The user can then specify that attribute as a pathspec to\n> perform various operations only on that set of files. One simple example:\n>\n>         git ls-files -- \":(attr:text)\"\n>\n> can be used to list all of the files with the 'text' attribute.\n\nThanks for reviving these patches.\n\nI have read through both of them and they look good to me.\n\nThanks,\nStefan\n"},{"id":"313706","messageId":"e8ef511f-563b-8b22-d8c6-daec2291f2aa@google.com","threadId":"45334","inReplyTo":"20170309210756.105566-2-bmwill@google.com","subject":"Re: [PATCH 1/2] pathspec: allow querying for attributes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-03-09T22:19:25Z","receivedAt":"2017-03-09T22:19:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 03/09/2017 01:07 PM, Brandon Williams wrote:\n> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\n> index fc9320e59..5c32d1905 100644\n> --- a/Documentation/glossary-content.txt\n> +++ b/Documentation/glossary-content.txt\n> @@ -384,6 +384,26 @@ full pathname may have special meaning:\n>  +\n>  Glob magic is incompatible with literal magic.\n>\n> +attr;;\n> +After `attr:` comes a space separated list of \"attribute\n> +requirements\", all of which must be met in order for the\n> +path to be considered a match; this is in addition to the\n> +usual non-magic pathspec pattern matching.\n> ++\n> +Each of the attribute requirements for the path takes one of\n> +these forms:\n> +\n> +- \"`ATTR`\" requires that the attribute `ATTR` must be set.\n\nAs a relative newcomer to attributes, I was confused by the fact that \n\"set\" and \"set to a value\" is different (and likewise \"unset\" and \n\"unspecified\"). Maybe it's worthwhile including a link to \n\"gitattributes\" to explain the different (exclusive) states that an \nattribute can be in.\n\n> +\n> +- \"`-ATTR`\" requires that the attribute `ATTR` must be unset.\n> +\n> +- \"`ATTR=VALUE`\" requires that the attribute `ATTR` must be\n> +  set to the string `VALUE`.\n> +\n> +- \"`!ATTR`\" requires that the attribute `ATTR` must be\n> +  unspecified.\n\nIt would read better to me if you omitted \"must\" in all 4 bullet points \n(and it is redundant anyway with \"requires\"), but I don't feel too \nstrongly about this.\n\n> diff --git a/pathspec.c b/pathspec.c\n> index b961f00c8..583ed5208 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -87,6 +89,72 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\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> +\tif (item->attr_check)\n> +\t\tdie(_(\"Only one 'attr:' specification is allowed.\"));\n> +\n> +\tif (!value || !strlen(value))\n\nYou can write `!*value` instead of `!strlen(value)`.\n\n> +\t\tdie(_(\"attr spec must not be empty\"));\n> +\n> +\tstring_list_split(&list, value, ' ', -1);\n\nYou could avoid some allocations by using the in-place variant (since \nvalue is a newly allocated string not used elsewhere) but it is probably \nnot that important in this argument parsing case.\n\n> +\tstring_list_remove_empty_items(&list, 0);\n> +\n> +\titem->attr_check = attr_check_alloc();\n> +\tALLOC_GROW(item->attr_match,\n> +\t\t   item->attr_match_nr + list.nr,\n> +\t\t   item->attr_match_alloc);\n\nIs there a time when this function is called while item->attr_match_nr \nis not zero?\n\n> +\n> +\tfor_each_string_list_item(si, &list) {\n> +\t\tsize_t attr_len;\n> +\t\tchar *attr_name;\n> +\t\tconst struct git_attr *a;\n> +\n> +\t\tint j = item->attr_match_nr++;\n> +\t\tconst char *attr = si->string;\n> +\t\tstruct attr_match *am = &item->attr_match[j];\n> +\n> +\t\tswitch (*attr) {\n> +\t\tcase '!':\n> +\t\t\tam->match_mode = MATCH_UNSPECIFIED;\n> +\t\t\tattr++;\n> +\t\t\tattr_len = strlen(attr);\n> +\t\t\tbreak;\n> +\t\tcase '-':\n> +\t\t\tam->match_mode = MATCH_UNSET;\n> +\t\t\tattr++;\n> +\t\t\tattr_len = strlen(attr);\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tattr_len = strcspn(attr, \"=\");\n> +\t\t\tif (attr[attr_len] != '=')\n> +\t\t\t\tam->match_mode = MATCH_SET;\n> +\t\t\telse {\n> +\t\t\t\tam->match_mode = MATCH_VALUE;\n> +\t\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n> +\t\t\t\tif (strchr(am->value, '\\\\'))\n> +\t\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n> +\t\t\t}\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tattr_name = xmemdupz(attr, attr_len);\n> +\t\ta = git_attr(attr_name);\n> +\t\tif (!a)\n> +\t\t\tdie(_(\"invalid attribute name %s\"), attr_name);\n> +\n> +\t\tattr_check_append(item->attr_check, a);\n> +\n> +\t\tfree(attr_name);\n> +\t}\n> +\n> +\tstring_list_clear(&list, 0);\n> +\treturn;\n\nRedundant return?\n\n> @@ -544,6 +628,10 @@ 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_check &&\n> +\t\t    item[i].attr_check->nr != item[i].attr_match_nr)\n> +\t\t\tdie(\"BUG: should have same number of entries\");\n\nI'm not sure if this check is giving us any benefit - I would expect \nthis type of code before some other code that assumed that the numbers \nmatched, and that will potentially segfault if not.\n"},{"id":"313719","messageId":"a81ad8b5-3b49-8232-1656-66b2e9d78569@google.com","threadId":"45334","inReplyTo":"20170309210756.105566-3-bmwill@google.com","subject":"Re: [PATCH 2/2] pathspec: allow escaped query values","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-03-09T22:31:59Z","receivedAt":"2017-03-09T22:32:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 03/09/2017 01:07 PM, Brandon Williams wrote:\n> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\n> index b5e5a0607..585d17bad 100755\n> --- a/t/t6135-pathspec-with-attrs.sh\n> +++ b/t/t6135-pathspec-with-attrs.sh\n> @@ -178,4 +178,13 @@ test_expect_success 'abort on asking for wrong magic' '\n>  \ttest_must_fail git ls-files . \":(attr:!label=foo)\"\n>  '\n>\n> +test_expect_success 'check attribute list' '\n> +\tcat <<-EOF >>.gitattributes &&\n> +\t* whitespace=indent,trail,space\n> +\tEOF\n> +\tgit ls-files \":(attr:whitespace=indent\\,trail\\,space)\" >actual &&\n> +\tgit ls-files >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n\nIs there a way to verify that `\\,` is not escaped by the shell into `,`?\n\nMaybe also add tests that show \\ as the last character and \\ escaping \nanother \\.\n"},{"id":"313765","messageId":"20170310182644.GB53198@google.com","threadId":"45334","inReplyTo":"e8ef511f-563b-8b22-d8c6-daec2291f2aa@google.com","subject":"Re: [PATCH 1/2] pathspec: allow querying for attributes","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-10T18:26:44Z","receivedAt":"2017-03-10T18:26:52Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/09, Jonathan Tan wrote:\n> On 03/09/2017 01:07 PM, Brandon Williams wrote:\n> >diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\n> >index fc9320e59..5c32d1905 100644\n> >--- a/Documentation/glossary-content.txt\n> >+++ b/Documentation/glossary-content.txt\n> >@@ -384,6 +384,26 @@ full pathname may have special meaning:\n> > +\n> > Glob magic is incompatible with literal magic.\n> >\n> >+attr;;\n> >+After `attr:` comes a space separated list of \"attribute\n> >+requirements\", all of which must be met in order for the\n> >+path to be considered a match; this is in addition to the\n> >+usual non-magic pathspec pattern matching.\n> >++\n> >+Each of the attribute requirements for the path takes one of\n> >+these forms:\n> >+\n> >+- \"`ATTR`\" requires that the attribute `ATTR` must be set.\n> \n> As a relative newcomer to attributes, I was confused by the fact\n> that \"set\" and \"set to a value\" is different (and likewise \"unset\"\n> and \"unspecified\"). Maybe it's worthwhile including a link to\n> \"gitattributes\" to explain the different (exclusive) states that an\n> attribute can be in.\n\nGood idea! I'll add in a link to gitattributes.\n> \n> >+\n> >+- \"`-ATTR`\" requires that the attribute `ATTR` must be unset.\n> >+\n> >+- \"`ATTR=VALUE`\" requires that the attribute `ATTR` must be\n> >+  set to the string `VALUE`.\n> >+\n> >+- \"`!ATTR`\" requires that the attribute `ATTR` must be\n> >+  unspecified.\n> \n> It would read better to me if you omitted \"must\" in all 4 bullet\n> points (and it is redundant anyway with \"requires\"), but I don't\n> feel too strongly about this.\n\nI agree, the first paragraph already says \"must\" so it reads better\nwithout repeating must over and over again.\n\n> \n> >diff --git a/pathspec.c b/pathspec.c\n> >index b961f00c8..583ed5208 100644\n> >--- a/pathspec.c\n> >+++ b/pathspec.c\n> >@@ -87,6 +89,72 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\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> >+\tif (item->attr_check)\n> >+\t\tdie(_(\"Only one 'attr:' specification is allowed.\"));\n> >+\n> >+\tif (!value || !strlen(value))\n> \n> You can write `!*value` instead of `!strlen(value)`.\n> \n\nDone.\n\n> >+\tstring_list_remove_empty_items(&list, 0);\n> >+\n> >+\titem->attr_check = attr_check_alloc();\n> >+\tALLOC_GROW(item->attr_match,\n> >+\t\t   item->attr_match_nr + list.nr,\n> >+\t\t   item->attr_match_alloc);\n> \n> Is there a time when this function is called while\n> item->attr_match_nr is not zero?\n\nNope, it pretty much has to be zero.  I'll change this to just use\nlist.nr.  item->attr_match_nr will be incremented up to list.nr over the\ncourse of the for loop and I'll move the equality check to the end of\nthis function.\n\n> >+\tstring_list_clear(&list, 0);\n> >+\treturn;\n> \n> Redundant return?\n\nI'll remove it.\n\n> \n> >@@ -544,6 +628,10 @@ 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_check &&\n> >+\t\t    item[i].attr_check->nr != item[i].attr_match_nr)\n> >+\t\t\tdie(\"BUG: should have same number of entries\");\n> \n> I'm not sure if this check is giving us any benefit - I would expect\n> this type of code before some other code that assumed that the\n> numbers matched, and that will potentially segfault if not.\n\nI'll push the check to right after the object creation (see comment\nabove).\n\n-- \nBrandon Williams\n"},{"id":"313768","messageId":"20170310185332.GC53198@google.com","threadId":"45334","inReplyTo":"a81ad8b5-3b49-8232-1656-66b2e9d78569@google.com","subject":"Re: [PATCH 2/2] pathspec: allow escaped query values","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-10T18:53:32Z","receivedAt":"2017-03-10T18:53:39Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/09, Jonathan Tan wrote:\n> On 03/09/2017 01:07 PM, Brandon Williams wrote:\n> >diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\n> >index b5e5a0607..585d17bad 100755\n> >--- a/t/t6135-pathspec-with-attrs.sh\n> >+++ b/t/t6135-pathspec-with-attrs.sh\n> >@@ -178,4 +178,13 @@ test_expect_success 'abort on asking for wrong magic' '\n> > \ttest_must_fail git ls-files . \":(attr:!label=foo)\"\n> > '\n> >\n> >+test_expect_success 'check attribute list' '\n> >+\tcat <<-EOF >>.gitattributes &&\n> >+\t* whitespace=indent,trail,space\n> >+\tEOF\n> >+\tgit ls-files \":(attr:whitespace=indent\\,trail\\,space)\" >actual &&\n> >+\tgit ls-files >expect &&\n> >+\ttest_cmp expect actual\n> >+'\n> >+\n> > test_done\n> \n> Is there a way to verify that `\\,` is not escaped by the shell into `,`?\n\nYou can run with GIT_TRACE=1 to see the actual string passed to git.\n'\\,' is indeed passed to git with no problems. \n\n> \n> Maybe also add tests that show \\ as the last character and \\\n> escaping another \\.\n\nDone\n\n-- \nBrandon Williams\n"},{"id":"313771","messageId":"20170310185908.171589-2-bmwill@google.com","threadId":"45334","inReplyTo":"20170310185908.171589-1-bmwill@google.com","subject":"[PATCH v2 1/2] pathspec: allow querying for attributes","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-10T18:59:07Z","receivedAt":"2017-03-10T18:59:48Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"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\nBased on a patch by Stefan Beller <sbeller@google.com>\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n Documentation/glossary-content.txt |  21 +++++\n attr.c                             |  17 ++++\n attr.h                             |   1 +\n dir.c                              |  43 ++++++++-\n pathspec.c                         | 117 +++++++++++++++++++++++-\n pathspec.h                         |  16 +++-\n t/t6135-pathspec-with-attrs.sh     | 181 +++++++++++++++++++++++++++++++++++++\n 7 files changed, 387 insertions(+), 9 deletions(-)\n create mode 100755 t/t6135-pathspec-with-attrs.sh\n\ndiff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\nindex fc9320e59..6e991c246 100644\n--- a/Documentation/glossary-content.txt\n+++ b/Documentation/glossary-content.txt\n@@ -384,6 +384,27 @@ full pathname may have special meaning:\n +\n Glob magic is incompatible with literal magic.\n \n+attr;;\n+After `attr:` comes a space separated list of \"attribute\n+requirements\", all of which must be met in order for the\n+path to be considered a match; this is in addition to the\n+usual non-magic pathspec pattern matching.\n+See linkgit:gitattributes[5].\n++\n+Each of the attribute requirements for the path takes one of\n+these forms:\n+\n+- \"`ATTR`\" requires that the attribute `ATTR` be set.\n+\n+- \"`-ATTR`\" requires that the attribute `ATTR` be unset.\n+\n+- \"`ATTR=VALUE`\" requires that the attribute `ATTR` be\n+  set to the string `VALUE`.\n+\n+- \"`!ATTR`\" requires that the attribute `ATTR` be\n+  unspecified.\n++\n+\n exclude;;\n \tAfter a path matches any non-exclude pathspec, it will be run\n \tthrough all exclude pathspec (magic signature: `!` or its\ndiff --git a/attr.c b/attr.c\nindex 5493bff22..7e2134471 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -603,6 +603,23 @@ struct attr_check *attr_check_initl(const char *one, ...)\n \treturn check;\n }\n \n+struct attr_check *attr_check_dup(const struct attr_check *check)\n+{\n+\tstruct attr_check *ret;\n+\n+\tif (!check)\n+\t\treturn NULL;\n+\n+\tret = attr_check_alloc();\n+\n+\tret->nr = check->nr;\n+\tret->alloc = check->alloc;\n+\tALLOC_ARRAY(ret->items, ret->nr);\n+\tCOPY_ARRAY(ret->items, check->items, ret->nr);\n+\n+\treturn ret;\n+}\n+\n struct attr_check_item *attr_check_append(struct attr_check *check,\n \t\t\t\t\t  const struct git_attr *attr)\n {\ndiff --git a/attr.h b/attr.h\nindex 48ab3e1c2..442d464db 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -44,6 +44,7 @@ struct attr_check {\n \n extern struct attr_check *attr_check_alloc(void);\n extern struct attr_check *attr_check_initl(const char *, ...);\n+extern struct attr_check *attr_check_dup(const struct attr_check *check);\n \n extern struct attr_check_item *attr_check_append(struct attr_check *check,\n \t\t\t\t\t\t const struct git_attr *attr);\ndiff --git a/dir.c b/dir.c\nindex 4541f9e14..2fe7acbcf 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@@ -134,7 +135,8 @@ static size_t common_prefix_len(const struct pathspec *pathspec)\n \t\t       PATHSPEC_LITERAL |\n \t\t       PATHSPEC_GLOB |\n \t\t       PATHSPEC_ICASE |\n-\t\t       PATHSPEC_EXCLUDE);\n+\t\t       PATHSPEC_EXCLUDE |\n+\t\t       PATHSPEC_ATTR);\n \n \tfor (n = 0; n < pathspec->nr; n++) {\n \t\tsize_t i = 0, len = 0, item_len;\n@@ -209,6 +211,36 @@ int within_depth(const char *name, int namelen,\n #define DO_MATCH_DIRECTORY (1<<1)\n #define DO_MATCH_SUBMODULE (1<<2)\n \n+static int match_attrs(const char *name, int namelen,\n+\t\t       const struct pathspec_item *item)\n+{\n+\tint i;\n+\n+\tgit_check_attr(name, item->attr_check);\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->items[i].value;\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\telse if (ATTR_FALSE(value))\n+\t\t\tmatched = (match_mode == MATCH_UNSET);\n+\t\telse if (ATTR_UNSET(value))\n+\t\t\tmatched = (match_mode == MATCH_UNSPECIFIED);\n+\t\telse\n+\t\t\tmatched = (match_mode == MATCH_VALUE &&\n+\t\t\t\t   !strcmp(item->attr_match[i].value, value));\n+\t\tif (!matched)\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n /*\n  * Does 'match' match the given name?\n  * A match is found if\n@@ -261,6 +293,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;\n@@ -339,7 +374,8 @@ static int do_match_pathspec(const struct pathspec *ps,\n \t\t       PATHSPEC_LITERAL |\n \t\t       PATHSPEC_GLOB |\n \t\t       PATHSPEC_ICASE |\n-\t\t       PATHSPEC_EXCLUDE);\n+\t\t       PATHSPEC_EXCLUDE |\n+\t\t       PATHSPEC_ATTR);\n \n \tif (!ps->nr) {\n \t\tif (!ps->recursive ||\n@@ -1361,7 +1397,8 @@ static int simplify_away(const char *path, int pathlen,\n \t\t       PATHSPEC_LITERAL |\n \t\t       PATHSPEC_GLOB |\n \t\t       PATHSPEC_ICASE |\n-\t\t       PATHSPEC_EXCLUDE);\n+\t\t       PATHSPEC_EXCLUDE |\n+\t\t       PATHSPEC_ATTR);\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tconst struct pathspec_item *item = &pathspec->items[i];\ndiff --git a/pathspec.c b/pathspec.c\nindex b961f00c8..7cd5f6e3d 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@@ -72,6 +73,7 @@ static struct pathspec_magic {\n \t{ PATHSPEC_GLOB,    '\\0', \"glob\" },\n \t{ PATHSPEC_ICASE,   '\\0', \"icase\" },\n \t{ PATHSPEC_EXCLUDE,  '!', \"exclude\" },\n+\t{ PATHSPEC_ATTR,    '\\0', \"attr\" },\n };\n \n static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n@@ -87,6 +89,74 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\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+\tif (item->attr_check)\n+\t\tdie(_(\"Only one 'attr:' specification is allowed.\"));\n+\n+\tif (!value || !*value)\n+\t\tdie(_(\"attr spec must not be empty\"));\n+\n+\tstring_list_split(&list, value, ' ', -1);\n+\tstring_list_remove_empty_items(&list, 0);\n+\n+\titem->attr_check = attr_check_alloc();\n+\tALLOC_GROW(item->attr_match,\n+\t\t   list.nr,\n+\t\t   item->attr_match_alloc);\n+\n+\tfor_each_string_list_item(si, &list) {\n+\t\tsize_t attr_len;\n+\t\tchar *attr_name;\n+\t\tconst struct git_attr *a;\n+\n+\t\tint j = item->attr_match_nr++;\n+\t\tconst char *attr = si->string;\n+\t\tstruct attr_match *am = &item->attr_match[j];\n+\n+\t\tswitch (*attr) {\n+\t\tcase '!':\n+\t\t\tam->match_mode = MATCH_UNSPECIFIED;\n+\t\t\tattr++;\n+\t\t\tattr_len = strlen(attr);\n+\t\t\tbreak;\n+\t\tcase '-':\n+\t\t\tam->match_mode = MATCH_UNSET;\n+\t\t\tattr++;\n+\t\t\tattr_len = strlen(attr);\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tattr_len = strcspn(attr, \"=\");\n+\t\t\tif (attr[attr_len] != '=')\n+\t\t\t\tam->match_mode = MATCH_SET;\n+\t\t\telse {\n+\t\t\t\tam->match_mode = MATCH_VALUE;\n+\t\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n+\t\t\t\tif (strchr(am->value, '\\\\'))\n+\t\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n+\t\t\t}\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tattr_name = xmemdupz(attr, attr_len);\n+\t\ta = git_attr(attr_name);\n+\t\tif (!a)\n+\t\t\tdie(_(\"invalid attribute name %s\"), attr_name);\n+\n+\t\tattr_check_append(item->attr_check, a);\n+\n+\t\tfree(attr_name);\n+\t}\n+\n+\tif (item->attr_check->nr != item->attr_match_nr)\n+\t\tdie(\"BUG: should have same number of entries\");\n+\n+\tstring_list_clear(&list, 0);\n+}\n+\n static inline int get_literal_global(void)\n {\n \tstatic int literal = -1;\n@@ -164,6 +234,7 @@ static int get_global_magic(int element_magic)\n  * returns the position in 'elem' after all magic has been parsed\n  */\n static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n+\t\t\t\t    struct pathspec_item *item,\n \t\t\t\t    const char *elem)\n {\n \tconst char *pos;\n@@ -189,6 +260,14 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (starts_with(pos, \"attr:\")) {\n+\t\t\tchar *attr_body = xmemdupz(pos + 5, len - 5);\n+\t\t\tparse_pathspec_attr_match(item, attr_body);\n+\t\t\t*magic |= PATHSPEC_ATTR;\n+\t\t\tfree(attr_body);\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, pos, len)) {\n@@ -252,13 +331,14 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)\n }\n \n static const char *parse_element_magic(unsigned *magic, int *prefix_len,\n+\t\t\t\t       struct pathspec_item *item,\n \t\t\t\t       const char *elem)\n {\n \tif (elem[0] != ':' || get_literal_global())\n \t\treturn elem; /* nothing to do */\n \telse if (elem[1] == '(')\n \t\t/* longhand */\n-\t\treturn parse_long_magic(magic, prefix_len, elem);\n+\t\treturn parse_long_magic(magic, prefix_len, item, elem);\n \telse\n \t\t/* shorthand */\n \t\treturn parse_short_magic(magic, elem);\n@@ -335,12 +415,18 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \tchar *match;\n \tint pathspec_prefix = -1;\n \n+\titem->attr_check = NULL;\n+\titem->attr_match = NULL;\n+\titem->attr_match_nr = 0;\n+\titem->attr_match_alloc = 0;\n+\n \t/* PATHSPEC_LITERAL_PATH ignores magic */\n \tif (flags & PATHSPEC_LITERAL_PATH) {\n \t\tmagic = PATHSPEC_LITERAL;\n \t} else {\n \t\tcopyfrom = parse_element_magic(&element_magic,\n \t\t\t\t\t       &pathspec_prefix,\n+\t\t\t\t\t       item,\n \t\t\t\t\t       elt);\n \t\tmagic |= element_magic;\n \t\tmagic |= get_global_magic(element_magic);\n@@ -565,26 +651,47 @@ void parse_pathspec(struct pathspec *pathspec,\n \n void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n {\n-\tint i;\n+\tint i, j;\n \n \t*dst = *src;\n \tALLOC_ARRAY(dst->items, dst->nr);\n \tCOPY_ARRAY(dst->items, src->items, dst->nr);\n \n \tfor (i = 0; i < dst->nr; i++) {\n-\t\tdst->items[i].match = xstrdup(src->items[i].match);\n-\t\tdst->items[i].original = xstrdup(src->items[i].original);\n+\t\tstruct pathspec_item *d = &dst->items[i];\n+\t\tstruct pathspec_item *s = &src->items[i];\n+\n+\t\td->match = xstrdup(s->match);\n+\t\td->original = xstrdup(s->original);\n+\n+\t\tALLOC_ARRAY(d->attr_match, d->attr_match_nr);\n+\t\tCOPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr);\n+\t\tfor (j = 0; j < d->attr_match_nr; j++) {\n+\t\t\tconst char *value = s->attr_match[j].value;\n+\t\t\tif (value)\n+\t\t\t\td->attr_match[j].value = xstrdup(value);\n+\t\t}\n+\n+\t\td->attr_check = attr_check_dup(s->attr_check);\n \t}\n }\n \n void clear_pathspec(struct pathspec *pathspec)\n {\n-\tint i;\n+\tint i, j;\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tfree(pathspec->items[i].match);\n \t\tfree(pathspec->items[i].original);\n+\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+\n+\t\tif (pathspec->items[i].attr_check)\n+\t\t\tattr_check_free(pathspec->items[i].attr_check);\n \t}\n+\n \tfree(pathspec->items);\n \tpathspec->items = NULL;\n \tpathspec->nr = 0;\ndiff --git a/pathspec.h b/pathspec.h\nindex 49fd823dd..83625f006 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -8,13 +8,15 @@\n #define PATHSPEC_GLOB\t\t(1<<3)\n #define PATHSPEC_ICASE\t\t(1<<4)\n #define PATHSPEC_EXCLUDE\t(1<<5)\n+#define PATHSPEC_ATTR\t\t(1<<6)\n #define PATHSPEC_ALL_MAGIC\t  \\\n \t(PATHSPEC_FROMTOP\t| \\\n \t PATHSPEC_MAXDEPTH\t| \\\n \t PATHSPEC_LITERAL\t| \\\n \t PATHSPEC_GLOB\t\t| \\\n \t PATHSPEC_ICASE\t\t| \\\n-\t PATHSPEC_EXCLUDE)\n+\t PATHSPEC_EXCLUDE\t| \\\n+\t PATHSPEC_ATTR)\n \n #define PATHSPEC_ONESTAR 1\t/* the pathspec pattern satisfies GFNM_ONESTAR */\n \n@@ -31,6 +33,18 @@ 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\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} match_mode;\n+\t\t} *attr_match;\n+\t\tstruct attr_check *attr_check;\n \t} *items;\n };\n \ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nnew file mode 100755\nindex 000000000..b5e5a0607\n--- /dev/null\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -0,0 +1,181 @@\n+#!/bin/sh\n+\n+test_description='test labels in pathspecs'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a tree' '\n+\tcat <<-EOF >expect &&\n+\tfileA\n+\tfileAB\n+\tfileAC\n+\tfileB\n+\tfileBC\n+\tfileC\n+\tfileNoLabel\n+\tfileSetLabel\n+\tfileUnsetLabel\n+\tfileValue\n+\tfileWrongLabel\n+\tsub/fileA\n+\tsub/fileAB\n+\tsub/fileAC\n+\tsub/fileB\n+\tsub/fileBC\n+\tsub/fileC\n+\tsub/fileNoLabel\n+\tsub/fileSetLabel\n+\tsub/fileUnsetLabel\n+\tsub/fileValue\n+\tsub/fileWrongLabel\n+\tEOF\n+\tmkdir sub &&\n+\twhile read path\n+\tdo\n+\t\t: >$path &&\n+\t\tgit add $path || return 1\n+\tdone <expect &&\n+\tgit commit -m \"initial commit\" &&\n+\tgit ls-files >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pathspec with no attr' '\n+\ttest_must_fail git ls-files \":(attr:)\"\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+\tfileA labelA\n+\tfileB labelB\n+\tfileC labelC\n+\tfileAB labelA labelB\n+\tfileAC labelA labelC\n+\tfileBC labelB labelC\n+\tfileUnsetLabel -label\n+\tfileSetLabel label\n+\tfileValue label=foo\n+\tfileWrongLabel label☺\n+\tEOF\n+\tgit add .gitattributes &&\n+\tgit commit -m \"add attributes\"\n+'\n+\n+test_expect_success 'check specific set attr' '\n+\tcat <<-EOF >expect &&\n+\tfileSetLabel\n+\tsub/fileSetLabel\n+\tEOF\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+\tfileUnsetLabel\n+\tsub/fileUnsetLabel\n+\tEOF\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+\tfileValue\n+\tsub/fileValue\n+\tEOF\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 unspecified attr' '\n+\tcat <<-EOF >expect &&\n+\t.gitattributes\n+\tfileA\n+\tfileAB\n+\tfileAC\n+\tfileB\n+\tfileBC\n+\tfileC\n+\tfileNoLabel\n+\tfileWrongLabel\n+\tsub/fileA\n+\tsub/fileAB\n+\tsub/fileAC\n+\tsub/fileB\n+\tsub/fileBC\n+\tsub/fileC\n+\tsub/fileNoLabel\n+\tsub/fileWrongLabel\n+\tEOF\n+\tgit ls-files \":(attr:!label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check multiple unspecified attr' '\n+\tcat <<-EOF >expect &&\n+\t.gitattributes\n+\tfileC\n+\tfileNoLabel\n+\tfileWrongLabel\n+\tsub/fileC\n+\tsub/fileNoLabel\n+\tsub/fileWrongLabel\n+\tEOF\n+\tgit ls-files \":(attr:!labelB !labelA !label)\" >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+\tfileAB\n+\tfileB\n+\tfileBC\n+\tEOF\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+\tfileAB\n+\tfileB\n+\tfileBC\n+\tsub/fileAB\n+\tsub/fileB\n+\tEOF\n+\tgit ls-files \":(attr:labelB)\" \":(exclude,attr:labelC)sub/\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n+\ttest_must_fail git ls-files . \":(attr:labelB,attr:labelC)\" 2>actual &&\n+\ttest_i18ngrep \"Only one\" actual\n+'\n+\n+test_expect_success 'fail if attr magic is used places not implemented' '\n+\t# The main purpose of this test is to check that we actually fail\n+\t# when you attempt to use attr magic in commands that do not implement\n+\t# attr magic. This test does not advocate git-add to stay that way,\n+\t# though, but git-add is convenient as it has its own internal pathspec\n+\t# parsing.\n+\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n+\ttest_i18ngrep \"unsupported magic\" actual\n+'\n+\n+test_expect_success 'abort on giving invalid label on the command line' '\n+\ttest_must_fail git ls-files . \":(attr:☺)\"\n+'\n+\n+test_expect_success 'abort on asking for wrong magic' '\n+\ttest_must_fail git ls-files . \":(attr:-label=foo)\" &&\n+\ttest_must_fail git ls-files . \":(attr:!label=foo)\"\n+'\n+\n+test_done\n-- \n2.12.0.246.ga2ecc84866-goog\n\n"},{"id":"313772","messageId":"20170310185908.171589-3-bmwill@google.com","threadId":"45334","inReplyTo":"20170310185908.171589-1-bmwill@google.com","subject":"[PATCH v2 2/2] pathspec: allow escaped query values","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-10T18:59:08Z","receivedAt":"2017-03-10T18:59:49Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"In our own .gitattributes file we have attributes such as:\n\n    *.[ch] whitespace=indent,trail,space\n\nWhen querying for attributes we want to be able to ask for the exact\nvalue, i.e.\n\n    git ls-files :(attr:whitespace=indent,trail,space)\n\nshould work, but the commas are used in the attr magic to introduce\nthe next attr, such that this query currently fails with\n\nfatal: Invalid pathspec magic 'trail' in ':(attr:whitespace=indent,trail,space)'\n\nThis change allows escaping characters by a backslash, such that the query\n\n    git ls-files :(attr:whitespace=indent\\,trail\\,space)\n\nwill match all path that have the value \"indent,trail,space\" for the\nwhitespace attribute. To accomplish this, we need to modify two places.\nFirst `parse_long_magic` needs to not stop early upon seeing a comma or\nclosing paren that is escaped. As a second step we need to remove any\nescaping from the attr value.\n\nBased on a patch by Stefan Beller <sbeller@google.com>\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n pathspec.c                     | 52 ++++++++++++++++++++++++++++++++++++++----\n t/t6135-pathspec-with-attrs.sh | 19 +++++++++++++++\n 2 files changed, 67 insertions(+), 4 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 7cd5f6e3d..d7956f6bf 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -89,6 +89,51 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+static size_t strcspn_escaped(const char *s, const char *stop)\n+{\n+\tconst char *i;\n+\n+\tfor (i = s; *i; i++) {\n+\t\t/* skip the escaped character */\n+\t\tif (i[0] == '\\\\' && i[1]) {\n+\t\t\ti++;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (strchr(stop, *i))\n+\t\t\tbreak;\n+\t}\n+\treturn i - s;\n+}\n+\n+static inline int invalid_value_char(const char ch)\n+{\n+\tif (isalnum(ch) || strchr(\",-_\", ch))\n+\t\treturn 0;\n+\treturn -1;\n+}\n+\n+static char *attr_value_unescape(const char *value)\n+{\n+\tconst char *src;\n+\tchar *dst, *ret;\n+\n+\tret = xmallocz(strlen(value));\n+\tfor (src = value, dst = ret; *src; src++, dst++) {\n+\t\tif (*src == '\\\\') {\n+\t\t\tif (!src[1])\n+\t\t\t\tdie(_(\"Escape character '\\\\' not allowed as \"\n+\t\t\t\t      \"last character in attr value\"));\n+\t\t\tsrc++;\n+\t\t}\n+\t\tif (invalid_value_char(*src))\n+\t\t\tdie(\"cannot use '%c' for value matching\", *src);\n+\t\t*dst = *src;\n+\t}\n+\t*dst = '\\0';\n+\treturn ret;\n+}\n+\n static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)\n {\n \tstruct string_list_item *si;\n@@ -133,10 +178,9 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va\n \t\t\tif (attr[attr_len] != '=')\n \t\t\t\tam->match_mode = MATCH_SET;\n \t\t\telse {\n+\t\t\t\tconst char *v = &attr[attr_len + 1];\n \t\t\t\tam->match_mode = MATCH_VALUE;\n-\t\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n-\t\t\t\tif (strchr(am->value, '\\\\'))\n-\t\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n+\t\t\t\tam->value = attr_value_unescape(v);\n \t\t\t}\n \t\t\tbreak;\n \t\t}\n@@ -241,7 +285,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n \tconst char *nextat;\n \n \tfor (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {\n-\t\tsize_t len = strcspn(pos, \",)\");\n+\t\tsize_t len = strcspn_escaped(pos, \",)\");\n \t\tint i;\n \n \t\tif (pos[len] == ',')\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex b5e5a0607..f60af29a9 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -178,4 +178,23 @@ test_expect_success 'abort on asking for wrong magic' '\n \ttest_must_fail git ls-files . \":(attr:!label=foo)\"\n '\n \n+test_expect_success 'check attribute list' '\n+\tcat <<-EOF >>.gitattributes &&\n+\t* whitespace=indent,trail,space\n+\tEOF\n+\tgit ls-files \":(attr:whitespace=indent\\,trail\\,space)\" >actual &&\n+\tgit ls-files >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'backslash cannot be the last character' '\n+\ttest_must_fail git ls-files \":(attr:label=foo\\\\ labelA=bar)\" 2>actual &&\n+\ttest_i18ngrep \"not allowed as last character in attr value\" actual\n+'\n+\n+test_expect_success 'backslash cannot be used as a value' '\n+\ttest_must_fail git ls-files \":(attr:label=f\\\\\\oo)\" 2>actual &&\n+\ttest_i18ngrep \"for value matching\" actual\n+'\n+\n test_done\n-- \n2.12.0.246.ga2ecc84866-goog\n\n"},{"id":"313773","messageId":"20170310185908.171589-1-bmwill@google.com","threadId":"45334","inReplyTo":"20170309210756.105566-1-bmwill@google.com","subject":"[PATCH v2 0/2] bringing attributes to pathspecs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-10T18:59:06Z","receivedAt":"2017-03-10T18:59:51Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"v2 addresses the comments made by Jonathan.\n\nBrandon Williams (2):\n  pathspec: allow querying for attributes\n  pathspec: allow escaped query values\n\n Documentation/glossary-content.txt |  21 ++++\n attr.c                             |  17 ++++\n attr.h                             |   1 +\n dir.c                              |  43 +++++++-\n pathspec.c                         | 163 ++++++++++++++++++++++++++++--\n pathspec.h                         |  16 ++-\n t/t6135-pathspec-with-attrs.sh     | 200 +++++++++++++++++++++++++++++++++++++\n 7 files changed, 451 insertions(+), 10 deletions(-)\n create mode 100755 t/t6135-pathspec-with-attrs.sh\n\n-- \n2.12.0.246.ga2ecc84866-goog\n\n"},{"id":"313787","messageId":"80fdf152-a4a1-7078-bb86-22f11763a4bb@google.com","threadId":"45334","inReplyTo":"20170310185908.171589-2-bmwill@google.com","subject":"Re: [PATCH v2 1/2] pathspec: allow querying for attributes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-03-10T19:56:05Z","receivedAt":"2017-03-10T19:56:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks - I don't think I have any more comments on this patch set after \nthese.\n\nOn 03/10/2017 10:59 AM, Brandon Williams wrote:\n> diff --git a/pathspec.c b/pathspec.c\n> index b961f00c8..7cd5f6e3d 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -87,6 +89,74 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\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> +\tif (item->attr_check)\n> +\t\tdie(_(\"Only one 'attr:' specification is allowed.\"));\n> +\n> +\tif (!value || !*value)\n> +\t\tdie(_(\"attr spec must not be empty\"));\n> +\n> +\tstring_list_split(&list, value, ' ', -1);\n> +\tstring_list_remove_empty_items(&list, 0);\n> +\n> +\titem->attr_check = attr_check_alloc();\n> +\tALLOC_GROW(item->attr_match,\n> +\t\t   list.nr,\n> +\t\t   item->attr_match_alloc);\n\nIf item->attr_match always starts empty, then I think an xmalloc or \nxcalloc suffices (and we don't need item->attr_match_alloc anymore).\n\nWe should probably also check item->attr_match above - that is, `if \n(item->attr_check || item->attr_match)`.\n\n> +\n> +\tfor_each_string_list_item(si, &list) {\n> +\t\tsize_t attr_len;\n> +\t\tchar *attr_name;\n> +\t\tconst struct git_attr *a;\n> +\n> +\t\tint j = item->attr_match_nr++;\n> +\t\tconst char *attr = si->string;\n> +\t\tstruct attr_match *am = &item->attr_match[j];\n> +\n> +\t\tswitch (*attr) {\n> +\t\tcase '!':\n> +\t\t\tam->match_mode = MATCH_UNSPECIFIED;\n> +\t\t\tattr++;\n> +\t\t\tattr_len = strlen(attr);\n> +\t\t\tbreak;\n> +\t\tcase '-':\n> +\t\t\tam->match_mode = MATCH_UNSET;\n> +\t\t\tattr++;\n> +\t\t\tattr_len = strlen(attr);\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tattr_len = strcspn(attr, \"=\");\n> +\t\t\tif (attr[attr_len] != '=')\n> +\t\t\t\tam->match_mode = MATCH_SET;\n> +\t\t\telse {\n> +\t\t\t\tam->match_mode = MATCH_VALUE;\n> +\t\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n> +\t\t\t\tif (strchr(am->value, '\\\\'))\n> +\t\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n> +\t\t\t}\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tattr_name = xmemdupz(attr, attr_len);\n> +\t\ta = git_attr(attr_name);\n> +\t\tif (!a)\n> +\t\t\tdie(_(\"invalid attribute name %s\"), attr_name);\n> +\n> +\t\tattr_check_append(item->attr_check, a);\n> +\n> +\t\tfree(attr_name);\n> +\t}\n> +\n> +\tif (item->attr_check->nr != item->attr_match_nr)\n> +\t\tdie(\"BUG: should have same number of entries\");\n\nI think such postcondition checks are usually not worth it, but others \nmight differ.\n\n> +\n> +\tstring_list_clear(&list, 0);\n> +}\n> +\n>  static inline int get_literal_global(void)\n>  {\n>  \tstatic int literal = -1;\n"},{"id":"313813","messageId":"20170311002821.GD53198@google.com","threadId":"45334","inReplyTo":"80fdf152-a4a1-7078-bb86-22f11763a4bb@google.com","subject":"Re: [PATCH v2 1/2] pathspec: allow querying for attributes","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-11T00:28:21Z","receivedAt":"2017-03-11T00:28:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/10, Jonathan Tan wrote:\n> Thanks - I don't think I have any more comments on this patch set\n> after these.\n> \n> On 03/10/2017 10:59 AM, Brandon Williams wrote:\n> >diff --git a/pathspec.c b/pathspec.c\n> >index b961f00c8..7cd5f6e3d 100644\n> >--- a/pathspec.c\n> >+++ b/pathspec.c\n> >@@ -87,6 +89,74 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\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> >+\tif (item->attr_check)\n> >+\t\tdie(_(\"Only one 'attr:' specification is allowed.\"));\n> >+\n> >+\tif (!value || !*value)\n> >+\t\tdie(_(\"attr spec must not be empty\"));\n> >+\n> >+\tstring_list_split(&list, value, ' ', -1);\n> >+\tstring_list_remove_empty_items(&list, 0);\n> >+\n> >+\titem->attr_check = attr_check_alloc();\n> >+\tALLOC_GROW(item->attr_match,\n> >+\t\t   list.nr,\n> >+\t\t   item->attr_match_alloc);\n> \n> If item->attr_match always starts empty, then I think an xmalloc or\n> xcalloc suffices (and we don't need item->attr_match_alloc anymore).\n> \n> We should probably also check item->attr_match above - that is, `if\n> (item->attr_check || item->attr_match)`.\n\nCorrect, I'll make these changes.\n\n> \n> >+\n> >+\tfor_each_string_list_item(si, &list) {\n> >+\t\tsize_t attr_len;\n> >+\t\tchar *attr_name;\n> >+\t\tconst struct git_attr *a;\n> >+\n> >+\t\tint j = item->attr_match_nr++;\n> >+\t\tconst char *attr = si->string;\n> >+\t\tstruct attr_match *am = &item->attr_match[j];\n> >+\n> >+\t\tswitch (*attr) {\n> >+\t\tcase '!':\n> >+\t\t\tam->match_mode = MATCH_UNSPECIFIED;\n> >+\t\t\tattr++;\n> >+\t\t\tattr_len = strlen(attr);\n> >+\t\t\tbreak;\n> >+\t\tcase '-':\n> >+\t\t\tam->match_mode = MATCH_UNSET;\n> >+\t\t\tattr++;\n> >+\t\t\tattr_len = strlen(attr);\n> >+\t\t\tbreak;\n> >+\t\tdefault:\n> >+\t\t\tattr_len = strcspn(attr, \"=\");\n> >+\t\t\tif (attr[attr_len] != '=')\n> >+\t\t\t\tam->match_mode = MATCH_SET;\n> >+\t\t\telse {\n> >+\t\t\t\tam->match_mode = MATCH_VALUE;\n> >+\t\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n> >+\t\t\t\tif (strchr(am->value, '\\\\'))\n> >+\t\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n> >+\t\t\t}\n> >+\t\t\tbreak;\n> >+\t\t}\n> >+\n> >+\t\tattr_name = xmemdupz(attr, attr_len);\n> >+\t\ta = git_attr(attr_name);\n> >+\t\tif (!a)\n> >+\t\t\tdie(_(\"invalid attribute name %s\"), attr_name);\n> >+\n> >+\t\tattr_check_append(item->attr_check, a);\n> >+\n> >+\t\tfree(attr_name);\n> >+\t}\n> >+\n> >+\tif (item->attr_check->nr != item->attr_match_nr)\n> >+\t\tdie(\"BUG: should have same number of entries\");\n> \n> I think such postcondition checks are usually not worth it, but\n> others might differ.\n\nyeah probably not, but its just an assert check for just in case so I'll\nleave it in.\n\n> \n> >+\n> >+\tstring_list_clear(&list, 0);\n> >+}\n> >+\n> > static inline int get_literal_global(void)\n> > {\n> > \tstatic int literal = -1;\n\n-- \nBrandon Williams\n"},{"id":"313873","messageId":"xmqqo9x5uc9u.fsf@gitster.mtv.corp.google.com","threadId":"45334","inReplyTo":"20170309210756.105566-2-bmwill@google.com","subject":"Re: [PATCH 1/2] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-13T02:43:41Z","receivedAt":"2017-03-13T02:43:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> +struct attr_check *attr_check_dup(const struct attr_check *check)\n> +{\n> +\tstruct attr_check *ret;\n> +\n> +\tif (!check)\n> +\t\treturn NULL;\n> +\n> +\tret = attr_check_alloc();\n> +\n> +\tret->nr = check->nr;\n> +\tret->alloc = check->alloc;\n> +\tALLOC_ARRAY(ret->items, ret->nr);\n> +\tCOPY_ARRAY(ret->items, check->items, ret->nr);\n> +\n> +\treturn ret;\n> +}\n\nBecause an attr_check instance cannot be shared and used by multiple\nthreads, we expect that the callers that go multi-thread to copy\npathspec to each worker, and preload_index(), which is an existing\nexample of such a caller, already does so with copy_pathspec().\n\nMakes sense.\n\n> @@ -565,26 +653,47 @@ void parse_pathspec(struct pathspec *pathspec,\n>  \n>  void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n>  {\n> -\tint i;\n> +\tint i, j;\n>  \n>  \t*dst = *src;\n>  \tALLOC_ARRAY(dst->items, dst->nr);\n>  \tCOPY_ARRAY(dst->items, src->items, dst->nr);\n>  \n>  \tfor (i = 0; i < dst->nr; i++) {\n> -\t\tdst->items[i].match = xstrdup(src->items[i].match);\n> -\t\tdst->items[i].original = xstrdup(src->items[i].original);\n> +\t\tstruct pathspec_item *d = &dst->items[i];\n> +\t\tstruct pathspec_item *s = &src->items[i];\n> +\n> +\t\td->match = xstrdup(s->match);\n> +\t\td->original = xstrdup(s->original);\n> +\n> +\t\tALLOC_ARRAY(d->attr_match, d->attr_match_nr);\n> +\t\tCOPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr);\n> +\t\tfor (j = 0; j < d->attr_match_nr; j++) {\n> +\t\t\tconst char *value = s->attr_match[j].value;\n> +\t\t\tif (value)\n> +\t\t\t\td->attr_match[j].value = xstrdup(value);\n\nWe have xstrdup_or_null(), which may help here.\n\n> +\t\t}\n> +\n> +\t\td->attr_check = attr_check_dup(s->attr_check);\n>  \t}\n>  }\n>  \n>  void clear_pathspec(struct pathspec *pathspec)\n>  {\n> -\tint i;\n> +\tint i, j;\n>  \n>  \tfor (i = 0; i < pathspec->nr; i++) {\n>  \t\tfree(pathspec->items[i].match);\n>  \t\tfree(pathspec->items[i].original);\n> +\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> +\n> +\t\tif (pathspec->items[i].attr_check)\n> +\t\t\tattr_check_free(pathspec->items[i].attr_check);\n>  \t}\n> +\n>  \tfree(pathspec->items);\n>  \tpathspec->items = NULL;\n>  \tpathspec->nr = 0;\n\nOK, makes sense.\n\n> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\n> new file mode 100755\n> index 000000000..b5e5a0607\n> --- /dev/null\n> +++ b/t/t6135-pathspec-with-attrs.sh\n> @@ -0,0 +1,181 @@\n> +#!/bin/sh\n> +\n> +test_description='test labels in pathspecs'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup a tree' '\n> +\tcat <<-EOF >expect &&\n\nMinor style nit. Quote the 'EOF' marker and you signal to readers\nthat what they'll see are literally the values, and they do not have\nto worry about $variable_interpolation.  I.e.\n\n\tcat <<-\\EOF >expect &&\n\n> +test_expect_success 'fail if attr magic is used places not implemented' '\n> +\t# The main purpose of this test is to check that we actually fail\n> +\t# when you attempt to use attr magic in commands that do not implement\n> +\t# attr magic. This test does not advocate git-add to stay that way,\n> +\t# though, but git-add is convenient as it has its own internal pathspec\n> +\t# parsing.\n\nThat's thought-provoking ;-) Would it help to add a test-pathspec\nhelper, similar to test-config helper, that serves as a vehicle to\ntest this?\n\n> +\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n> +\ttest_i18ngrep \"unsupported magic\" actual\n> +'\n\nThanks.\n"},{"id":"313915","messageId":"20170313182322.27539-1-bmwill@google.com","threadId":"45334","inReplyTo":"20170310185908.171589-1-bmwill@google.com","subject":"[PATCH v3 0/2] bringing attributes to pathspecs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-13T18:23:20Z","receivedAt":"2017-03-13T18:23:32Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"v3 fixes some nits in style in the test script (using <<-\\EOF instead of <<-EOF)\nas well as fixing a few other minor things reported by Junio and Jonathan.\n\nBrandon Williams (2):\n  pathspec: allow querying for attributes\n  pathspec: allow escaped query values\n\n Documentation/glossary-content.txt |  21 ++++\n attr.c                             |  17 ++++\n attr.h                             |   1 +\n dir.c                              |  43 +++++++-\n pathspec.c                         | 159 +++++++++++++++++++++++++++--\n pathspec.h                         |  15 ++-\n t/t6135-pathspec-with-attrs.sh     | 200 +++++++++++++++++++++++++++++++++++++\n 7 files changed, 446 insertions(+), 10 deletions(-)\n create mode 100755 t/t6135-pathspec-with-attrs.sh\n\n--- interdiff with bw/attr-pathspec\n\ndiff --git a/pathspec.c b/pathspec.c\nindex d7956f6bf..303efda83 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -139,7 +139,7 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va\n \tstruct string_list_item *si;\n \tstruct string_list list = STRING_LIST_INIT_DUP;\n \n-\tif (item->attr_check)\n+\tif (item->attr_check || item->attr_match)\n \t\tdie(_(\"Only one 'attr:' specification is allowed.\"));\n \n \tif (!value || !*value)\n@@ -149,9 +149,7 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va\n \tstring_list_remove_empty_items(&list, 0);\n \n \titem->attr_check = attr_check_alloc();\n-\tALLOC_GROW(item->attr_match,\n-\t\t   list.nr,\n-\t\t   item->attr_match_alloc);\n+\titem->attr_match = xcalloc(list.nr, sizeof(struct attr_match));\n \n \tfor_each_string_list_item(si, &list) {\n \t\tsize_t attr_len;\n@@ -462,7 +460,6 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \titem->attr_check = NULL;\n \titem->attr_match = NULL;\n \titem->attr_match_nr = 0;\n-\titem->attr_match_alloc = 0;\n \n \t/* PATHSPEC_LITERAL_PATH ignores magic */\n \tif (flags & PATHSPEC_LITERAL_PATH) {\n@@ -712,8 +709,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n \t\tCOPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr);\n \t\tfor (j = 0; j < d->attr_match_nr; j++) {\n \t\t\tconst char *value = s->attr_match[j].value;\n-\t\t\tif (value)\n-\t\t\t\td->attr_match[j].value = xstrdup(value);\n+\t\t\td->attr_match[j].value = xstrdup_or_null(value);\n \t\t}\n \n \t\td->attr_check = attr_check_dup(s->attr_check);\ndiff --git a/pathspec.h b/pathspec.h\nindex 83625f006..55e976972 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -34,7 +34,6 @@ struct pathspec {\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\tchar *value;\n \t\t\tenum attr_match_mode {\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex f60af29a9..77b8cef66 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -4,7 +4,7 @@ test_description='test labels in pathspecs'\n . ./test-lib.sh\n \n test_expect_success 'setup a tree' '\n-\tcat <<-EOF >expect &&\n+\tcat <<-\\EOF >expect &&\n \tfileA\n \tfileAB\n \tfileAC\n@@ -49,7 +49,7 @@ test_expect_success 'pathspec with labels and non existent .gitattributes' '\n '\n \n test_expect_success 'setup .gitattributes' '\n-\tcat <<-EOF >.gitattributes &&\n+\tcat <<-\\EOF >.gitattributes &&\n \tfileA labelA\n \tfileB labelB\n \tfileC labelC\n@@ -66,7 +66,7 @@ test_expect_success 'setup .gitattributes' '\n '\n \n test_expect_success 'check specific set attr' '\n-\tcat <<-EOF >expect &&\n+\tcat <<-\\EOF >expect &&\n \tfileSetLabel\n \tsub/fileSetLabel\n \tEOF\n@@ -75,7 +75,7 @@ test_expect_success 'check specific set attr' '\n '\n \n test_expect_success 'check specific unset attr' '\n-\tcat <<-EOF >expect &&\n+\tcat <<-\\EOF >expect &&\n \tfileUnsetLabel\n \tsub/fileUnsetLabel\n \tEOF\n@@ -84,7 +84,7 @@ test_expect_success 'check specific unset attr' '\n '\n \n test_expect_success 'check specific value attr' '\n-\tcat <<-EOF >expect &&\n+\tcat <<-\\EOF >expect &&\n \tfileValue\n \tsub/fileValue\n \tEOF\n@@ -95,7 +95,7 @@ test_expect_success 'check specific value attr' '\n '\n \n test_expect_success 'check unspecified attr' '\n-\tcat <<-EOF >expect &&\n+\tcat <<-\\EOF >expect &&\n \t.gitattributes\n \tfileA\n \tfileAB\n@@ -119,7 +119,7 @@ test_expect_success 'check unspecified attr' '\n '\n \n test_expect_success 'check multiple unspecified attr' '\n-\tcat <<-EOF >expect &&\n+\tcat <<-\\EOF >expect &&\n \t.gitattributes\n \tfileC\n \tfileNoLabel\n@@ -133,7 +133,7 @@ test_expect_success 'check multiple unspecified attr' '\n '\n \n test_expect_success 'check label with more labels but excluded path' '\n-\tcat <<-EOF >expect &&\n+\tcat <<-\\EOF >expect &&\n \tfileAB\n \tfileB\n \tfileBC\n@@ -143,7 +143,7 @@ test_expect_success 'check label with more labels but excluded path' '\n '\n \n test_expect_success 'check label excluding other labels' '\n-\tcat <<-EOF >expect &&\n+\tcat <<-\\EOF >expect &&\n \tfileAB\n \tfileB\n \tfileBC\n\n-- \n2.12.0.246.ga2ecc84866-goog\n\n"},{"id":"313916","messageId":"20170313182322.27539-2-bmwill@google.com","threadId":"45334","inReplyTo":"20170313182322.27539-1-bmwill@google.com","subject":"[PATCH v3 1/2] pathspec: allow querying for attributes","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-13T18:23:21Z","receivedAt":"2017-03-13T18:23:36Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"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\nBased on a patch by Stefan Beller <sbeller@google.com>\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n Documentation/glossary-content.txt |  21 +++++\n attr.c                             |  17 ++++\n attr.h                             |   1 +\n dir.c                              |  43 ++++++++-\n pathspec.c                         | 113 ++++++++++++++++++++++-\n pathspec.h                         |  15 ++-\n t/t6135-pathspec-with-attrs.sh     | 181 +++++++++++++++++++++++++++++++++++++\n 7 files changed, 382 insertions(+), 9 deletions(-)\n create mode 100755 t/t6135-pathspec-with-attrs.sh\n\ndiff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\nindex fc9320e59..6e991c246 100644\n--- a/Documentation/glossary-content.txt\n+++ b/Documentation/glossary-content.txt\n@@ -384,6 +384,27 @@ full pathname may have special meaning:\n +\n Glob magic is incompatible with literal magic.\n \n+attr;;\n+After `attr:` comes a space separated list of \"attribute\n+requirements\", all of which must be met in order for the\n+path to be considered a match; this is in addition to the\n+usual non-magic pathspec pattern matching.\n+See linkgit:gitattributes[5].\n++\n+Each of the attribute requirements for the path takes one of\n+these forms:\n+\n+- \"`ATTR`\" requires that the attribute `ATTR` be set.\n+\n+- \"`-ATTR`\" requires that the attribute `ATTR` be unset.\n+\n+- \"`ATTR=VALUE`\" requires that the attribute `ATTR` be\n+  set to the string `VALUE`.\n+\n+- \"`!ATTR`\" requires that the attribute `ATTR` be\n+  unspecified.\n++\n+\n exclude;;\n \tAfter a path matches any non-exclude pathspec, it will be run\n \tthrough all exclude pathspec (magic signature: `!` or its\ndiff --git a/attr.c b/attr.c\nindex 5493bff22..7e2134471 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -603,6 +603,23 @@ struct attr_check *attr_check_initl(const char *one, ...)\n \treturn check;\n }\n \n+struct attr_check *attr_check_dup(const struct attr_check *check)\n+{\n+\tstruct attr_check *ret;\n+\n+\tif (!check)\n+\t\treturn NULL;\n+\n+\tret = attr_check_alloc();\n+\n+\tret->nr = check->nr;\n+\tret->alloc = check->alloc;\n+\tALLOC_ARRAY(ret->items, ret->nr);\n+\tCOPY_ARRAY(ret->items, check->items, ret->nr);\n+\n+\treturn ret;\n+}\n+\n struct attr_check_item *attr_check_append(struct attr_check *check,\n \t\t\t\t\t  const struct git_attr *attr)\n {\ndiff --git a/attr.h b/attr.h\nindex 48ab3e1c2..442d464db 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -44,6 +44,7 @@ struct attr_check {\n \n extern struct attr_check *attr_check_alloc(void);\n extern struct attr_check *attr_check_initl(const char *, ...);\n+extern struct attr_check *attr_check_dup(const struct attr_check *check);\n \n extern struct attr_check_item *attr_check_append(struct attr_check *check,\n \t\t\t\t\t\t const struct git_attr *attr);\ndiff --git a/dir.c b/dir.c\nindex 4541f9e14..2fe7acbcf 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@@ -134,7 +135,8 @@ static size_t common_prefix_len(const struct pathspec *pathspec)\n \t\t       PATHSPEC_LITERAL |\n \t\t       PATHSPEC_GLOB |\n \t\t       PATHSPEC_ICASE |\n-\t\t       PATHSPEC_EXCLUDE);\n+\t\t       PATHSPEC_EXCLUDE |\n+\t\t       PATHSPEC_ATTR);\n \n \tfor (n = 0; n < pathspec->nr; n++) {\n \t\tsize_t i = 0, len = 0, item_len;\n@@ -209,6 +211,36 @@ int within_depth(const char *name, int namelen,\n #define DO_MATCH_DIRECTORY (1<<1)\n #define DO_MATCH_SUBMODULE (1<<2)\n \n+static int match_attrs(const char *name, int namelen,\n+\t\t       const struct pathspec_item *item)\n+{\n+\tint i;\n+\n+\tgit_check_attr(name, item->attr_check);\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->items[i].value;\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\telse if (ATTR_FALSE(value))\n+\t\t\tmatched = (match_mode == MATCH_UNSET);\n+\t\telse if (ATTR_UNSET(value))\n+\t\t\tmatched = (match_mode == MATCH_UNSPECIFIED);\n+\t\telse\n+\t\t\tmatched = (match_mode == MATCH_VALUE &&\n+\t\t\t\t   !strcmp(item->attr_match[i].value, value));\n+\t\tif (!matched)\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n /*\n  * Does 'match' match the given name?\n  * A match is found if\n@@ -261,6 +293,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;\n@@ -339,7 +374,8 @@ static int do_match_pathspec(const struct pathspec *ps,\n \t\t       PATHSPEC_LITERAL |\n \t\t       PATHSPEC_GLOB |\n \t\t       PATHSPEC_ICASE |\n-\t\t       PATHSPEC_EXCLUDE);\n+\t\t       PATHSPEC_EXCLUDE |\n+\t\t       PATHSPEC_ATTR);\n \n \tif (!ps->nr) {\n \t\tif (!ps->recursive ||\n@@ -1361,7 +1397,8 @@ static int simplify_away(const char *path, int pathlen,\n \t\t       PATHSPEC_LITERAL |\n \t\t       PATHSPEC_GLOB |\n \t\t       PATHSPEC_ICASE |\n-\t\t       PATHSPEC_EXCLUDE);\n+\t\t       PATHSPEC_EXCLUDE |\n+\t\t       PATHSPEC_ATTR);\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tconst struct pathspec_item *item = &pathspec->items[i];\ndiff --git a/pathspec.c b/pathspec.c\nindex b961f00c8..608511c52 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@@ -72,6 +73,7 @@ static struct pathspec_magic {\n \t{ PATHSPEC_GLOB,    '\\0', \"glob\" },\n \t{ PATHSPEC_ICASE,   '\\0', \"icase\" },\n \t{ PATHSPEC_EXCLUDE,  '!', \"exclude\" },\n+\t{ PATHSPEC_ATTR,    '\\0', \"attr\" },\n };\n \n static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n@@ -87,6 +89,72 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\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+\tif (item->attr_check || item->attr_match)\n+\t\tdie(_(\"Only one 'attr:' specification is allowed.\"));\n+\n+\tif (!value || !*value)\n+\t\tdie(_(\"attr spec must not be empty\"));\n+\n+\tstring_list_split(&list, value, ' ', -1);\n+\tstring_list_remove_empty_items(&list, 0);\n+\n+\titem->attr_check = attr_check_alloc();\n+\titem->attr_match = xcalloc(list.nr, sizeof(struct attr_match));\n+\n+\tfor_each_string_list_item(si, &list) {\n+\t\tsize_t attr_len;\n+\t\tchar *attr_name;\n+\t\tconst struct git_attr *a;\n+\n+\t\tint j = item->attr_match_nr++;\n+\t\tconst char *attr = si->string;\n+\t\tstruct attr_match *am = &item->attr_match[j];\n+\n+\t\tswitch (*attr) {\n+\t\tcase '!':\n+\t\t\tam->match_mode = MATCH_UNSPECIFIED;\n+\t\t\tattr++;\n+\t\t\tattr_len = strlen(attr);\n+\t\t\tbreak;\n+\t\tcase '-':\n+\t\t\tam->match_mode = MATCH_UNSET;\n+\t\t\tattr++;\n+\t\t\tattr_len = strlen(attr);\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tattr_len = strcspn(attr, \"=\");\n+\t\t\tif (attr[attr_len] != '=')\n+\t\t\t\tam->match_mode = MATCH_SET;\n+\t\t\telse {\n+\t\t\t\tam->match_mode = MATCH_VALUE;\n+\t\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n+\t\t\t\tif (strchr(am->value, '\\\\'))\n+\t\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n+\t\t\t}\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tattr_name = xmemdupz(attr, attr_len);\n+\t\ta = git_attr(attr_name);\n+\t\tif (!a)\n+\t\t\tdie(_(\"invalid attribute name %s\"), attr_name);\n+\n+\t\tattr_check_append(item->attr_check, a);\n+\n+\t\tfree(attr_name);\n+\t}\n+\n+\tif (item->attr_check->nr != item->attr_match_nr)\n+\t\tdie(\"BUG: should have same number of entries\");\n+\n+\tstring_list_clear(&list, 0);\n+}\n+\n static inline int get_literal_global(void)\n {\n \tstatic int literal = -1;\n@@ -164,6 +232,7 @@ static int get_global_magic(int element_magic)\n  * returns the position in 'elem' after all magic has been parsed\n  */\n static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n+\t\t\t\t    struct pathspec_item *item,\n \t\t\t\t    const char *elem)\n {\n \tconst char *pos;\n@@ -189,6 +258,14 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (starts_with(pos, \"attr:\")) {\n+\t\t\tchar *attr_body = xmemdupz(pos + 5, len - 5);\n+\t\t\tparse_pathspec_attr_match(item, attr_body);\n+\t\t\t*magic |= PATHSPEC_ATTR;\n+\t\t\tfree(attr_body);\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, pos, len)) {\n@@ -252,13 +329,14 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)\n }\n \n static const char *parse_element_magic(unsigned *magic, int *prefix_len,\n+\t\t\t\t       struct pathspec_item *item,\n \t\t\t\t       const char *elem)\n {\n \tif (elem[0] != ':' || get_literal_global())\n \t\treturn elem; /* nothing to do */\n \telse if (elem[1] == '(')\n \t\t/* longhand */\n-\t\treturn parse_long_magic(magic, prefix_len, elem);\n+\t\treturn parse_long_magic(magic, prefix_len, item, elem);\n \telse\n \t\t/* shorthand */\n \t\treturn parse_short_magic(magic, elem);\n@@ -335,12 +413,17 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \tchar *match;\n \tint pathspec_prefix = -1;\n \n+\titem->attr_check = NULL;\n+\titem->attr_match = NULL;\n+\titem->attr_match_nr = 0;\n+\n \t/* PATHSPEC_LITERAL_PATH ignores magic */\n \tif (flags & PATHSPEC_LITERAL_PATH) {\n \t\tmagic = PATHSPEC_LITERAL;\n \t} else {\n \t\tcopyfrom = parse_element_magic(&element_magic,\n \t\t\t\t\t       &pathspec_prefix,\n+\t\t\t\t\t       item,\n \t\t\t\t\t       elt);\n \t\tmagic |= element_magic;\n \t\tmagic |= get_global_magic(element_magic);\n@@ -565,26 +648,46 @@ void parse_pathspec(struct pathspec *pathspec,\n \n void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n {\n-\tint i;\n+\tint i, j;\n \n \t*dst = *src;\n \tALLOC_ARRAY(dst->items, dst->nr);\n \tCOPY_ARRAY(dst->items, src->items, dst->nr);\n \n \tfor (i = 0; i < dst->nr; i++) {\n-\t\tdst->items[i].match = xstrdup(src->items[i].match);\n-\t\tdst->items[i].original = xstrdup(src->items[i].original);\n+\t\tstruct pathspec_item *d = &dst->items[i];\n+\t\tstruct pathspec_item *s = &src->items[i];\n+\n+\t\td->match = xstrdup(s->match);\n+\t\td->original = xstrdup(s->original);\n+\n+\t\tALLOC_ARRAY(d->attr_match, d->attr_match_nr);\n+\t\tCOPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr);\n+\t\tfor (j = 0; j < d->attr_match_nr; j++) {\n+\t\t\tconst char *value = s->attr_match[j].value;\n+\t\t\td->attr_match[j].value = xstrdup_or_null(value);\n+\t\t}\n+\n+\t\td->attr_check = attr_check_dup(s->attr_check);\n \t}\n }\n \n void clear_pathspec(struct pathspec *pathspec)\n {\n-\tint i;\n+\tint i, j;\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tfree(pathspec->items[i].match);\n \t\tfree(pathspec->items[i].original);\n+\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+\n+\t\tif (pathspec->items[i].attr_check)\n+\t\t\tattr_check_free(pathspec->items[i].attr_check);\n \t}\n+\n \tfree(pathspec->items);\n \tpathspec->items = NULL;\n \tpathspec->nr = 0;\ndiff --git a/pathspec.h b/pathspec.h\nindex 49fd823dd..55e976972 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -8,13 +8,15 @@\n #define PATHSPEC_GLOB\t\t(1<<3)\n #define PATHSPEC_ICASE\t\t(1<<4)\n #define PATHSPEC_EXCLUDE\t(1<<5)\n+#define PATHSPEC_ATTR\t\t(1<<6)\n #define PATHSPEC_ALL_MAGIC\t  \\\n \t(PATHSPEC_FROMTOP\t| \\\n \t PATHSPEC_MAXDEPTH\t| \\\n \t PATHSPEC_LITERAL\t| \\\n \t PATHSPEC_GLOB\t\t| \\\n \t PATHSPEC_ICASE\t\t| \\\n-\t PATHSPEC_EXCLUDE)\n+\t PATHSPEC_EXCLUDE\t| \\\n+\t PATHSPEC_ATTR)\n \n #define PATHSPEC_ONESTAR 1\t/* the pathspec pattern satisfies GFNM_ONESTAR */\n \n@@ -31,6 +33,17 @@ struct pathspec {\n \t\tint len, prefix;\n \t\tint nowildcard_len;\n \t\tint flags;\n+\t\tint attr_match_nr;\n+\t\tstruct attr_match {\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} match_mode;\n+\t\t} *attr_match;\n+\t\tstruct attr_check *attr_check;\n \t} *items;\n };\n \ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nnew file mode 100755\nindex 000000000..3f06fcf8d\n--- /dev/null\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -0,0 +1,181 @@\n+#!/bin/sh\n+\n+test_description='test labels in pathspecs'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a tree' '\n+\tcat <<-\\EOF >expect &&\n+\tfileA\n+\tfileAB\n+\tfileAC\n+\tfileB\n+\tfileBC\n+\tfileC\n+\tfileNoLabel\n+\tfileSetLabel\n+\tfileUnsetLabel\n+\tfileValue\n+\tfileWrongLabel\n+\tsub/fileA\n+\tsub/fileAB\n+\tsub/fileAC\n+\tsub/fileB\n+\tsub/fileBC\n+\tsub/fileC\n+\tsub/fileNoLabel\n+\tsub/fileSetLabel\n+\tsub/fileUnsetLabel\n+\tsub/fileValue\n+\tsub/fileWrongLabel\n+\tEOF\n+\tmkdir sub &&\n+\twhile read path\n+\tdo\n+\t\t: >$path &&\n+\t\tgit add $path || return 1\n+\tdone <expect &&\n+\tgit commit -m \"initial commit\" &&\n+\tgit ls-files >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pathspec with no attr' '\n+\ttest_must_fail git ls-files \":(attr:)\"\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+\tfileA labelA\n+\tfileB labelB\n+\tfileC labelC\n+\tfileAB labelA labelB\n+\tfileAC labelA labelC\n+\tfileBC labelB labelC\n+\tfileUnsetLabel -label\n+\tfileSetLabel label\n+\tfileValue label=foo\n+\tfileWrongLabel label☺\n+\tEOF\n+\tgit add .gitattributes &&\n+\tgit commit -m \"add attributes\"\n+'\n+\n+test_expect_success 'check specific set attr' '\n+\tcat <<-\\EOF >expect &&\n+\tfileSetLabel\n+\tsub/fileSetLabel\n+\tEOF\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+\tfileUnsetLabel\n+\tsub/fileUnsetLabel\n+\tEOF\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+\tfileValue\n+\tsub/fileValue\n+\tEOF\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 unspecified attr' '\n+\tcat <<-\\EOF >expect &&\n+\t.gitattributes\n+\tfileA\n+\tfileAB\n+\tfileAC\n+\tfileB\n+\tfileBC\n+\tfileC\n+\tfileNoLabel\n+\tfileWrongLabel\n+\tsub/fileA\n+\tsub/fileAB\n+\tsub/fileAC\n+\tsub/fileB\n+\tsub/fileBC\n+\tsub/fileC\n+\tsub/fileNoLabel\n+\tsub/fileWrongLabel\n+\tEOF\n+\tgit ls-files \":(attr:!label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check multiple unspecified attr' '\n+\tcat <<-\\EOF >expect &&\n+\t.gitattributes\n+\tfileC\n+\tfileNoLabel\n+\tfileWrongLabel\n+\tsub/fileC\n+\tsub/fileNoLabel\n+\tsub/fileWrongLabel\n+\tEOF\n+\tgit ls-files \":(attr:!labelB !labelA !label)\" >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+\tfileAB\n+\tfileB\n+\tfileBC\n+\tEOF\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+\tfileAB\n+\tfileB\n+\tfileBC\n+\tsub/fileAB\n+\tsub/fileB\n+\tEOF\n+\tgit ls-files \":(attr:labelB)\" \":(exclude,attr:labelC)sub/\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n+\ttest_must_fail git ls-files . \":(attr:labelB,attr:labelC)\" 2>actual &&\n+\ttest_i18ngrep \"Only one\" actual\n+'\n+\n+test_expect_success 'fail if attr magic is used places not implemented' '\n+\t# The main purpose of this test is to check that we actually fail\n+\t# when you attempt to use attr magic in commands that do not implement\n+\t# attr magic. This test does not advocate git-add to stay that way,\n+\t# though, but git-add is convenient as it has its own internal pathspec\n+\t# parsing.\n+\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n+\ttest_i18ngrep \"unsupported magic\" actual\n+'\n+\n+test_expect_success 'abort on giving invalid label on the command line' '\n+\ttest_must_fail git ls-files . \":(attr:☺)\"\n+'\n+\n+test_expect_success 'abort on asking for wrong magic' '\n+\ttest_must_fail git ls-files . \":(attr:-label=foo)\" &&\n+\ttest_must_fail git ls-files . \":(attr:!label=foo)\"\n+'\n+\n+test_done\n-- \n2.12.0.246.ga2ecc84866-goog\n\n"},{"id":"313917","messageId":"20170313182322.27539-3-bmwill@google.com","threadId":"45334","inReplyTo":"20170313182322.27539-1-bmwill@google.com","subject":"[PATCH v3 2/2] pathspec: allow escaped query values","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-13T18:23:22Z","receivedAt":"2017-03-13T18:23:39Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"In our own .gitattributes file we have attributes such as:\n\n    *.[ch] whitespace=indent,trail,space\n\nWhen querying for attributes we want to be able to ask for the exact\nvalue, i.e.\n\n    git ls-files :(attr:whitespace=indent,trail,space)\n\nshould work, but the commas are used in the attr magic to introduce\nthe next attr, such that this query currently fails with\n\nfatal: Invalid pathspec magic 'trail' in ':(attr:whitespace=indent,trail,space)'\n\nThis change allows escaping characters by a backslash, such that the query\n\n    git ls-files :(attr:whitespace=indent\\,trail\\,space)\n\nwill match all path that have the value \"indent,trail,space\" for the\nwhitespace attribute. To accomplish this, we need to modify two places.\nFirst `parse_long_magic` needs to not stop early upon seeing a comma or\nclosing paren that is escaped. As a second step we need to remove any\nescaping from the attr value.\n\nBased on a patch by Stefan Beller <sbeller@google.com>\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n pathspec.c                     | 52 ++++++++++++++++++++++++++++++++++++++----\n t/t6135-pathspec-with-attrs.sh | 19 +++++++++++++++\n 2 files changed, 67 insertions(+), 4 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 608511c52..303efda83 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -89,6 +89,51 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+static size_t strcspn_escaped(const char *s, const char *stop)\n+{\n+\tconst char *i;\n+\n+\tfor (i = s; *i; i++) {\n+\t\t/* skip the escaped character */\n+\t\tif (i[0] == '\\\\' && i[1]) {\n+\t\t\ti++;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (strchr(stop, *i))\n+\t\t\tbreak;\n+\t}\n+\treturn i - s;\n+}\n+\n+static inline int invalid_value_char(const char ch)\n+{\n+\tif (isalnum(ch) || strchr(\",-_\", ch))\n+\t\treturn 0;\n+\treturn -1;\n+}\n+\n+static char *attr_value_unescape(const char *value)\n+{\n+\tconst char *src;\n+\tchar *dst, *ret;\n+\n+\tret = xmallocz(strlen(value));\n+\tfor (src = value, dst = ret; *src; src++, dst++) {\n+\t\tif (*src == '\\\\') {\n+\t\t\tif (!src[1])\n+\t\t\t\tdie(_(\"Escape character '\\\\' not allowed as \"\n+\t\t\t\t      \"last character in attr value\"));\n+\t\t\tsrc++;\n+\t\t}\n+\t\tif (invalid_value_char(*src))\n+\t\t\tdie(\"cannot use '%c' for value matching\", *src);\n+\t\t*dst = *src;\n+\t}\n+\t*dst = '\\0';\n+\treturn ret;\n+}\n+\n static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)\n {\n \tstruct string_list_item *si;\n@@ -131,10 +176,9 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va\n \t\t\tif (attr[attr_len] != '=')\n \t\t\t\tam->match_mode = MATCH_SET;\n \t\t\telse {\n+\t\t\t\tconst char *v = &attr[attr_len + 1];\n \t\t\t\tam->match_mode = MATCH_VALUE;\n-\t\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n-\t\t\t\tif (strchr(am->value, '\\\\'))\n-\t\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n+\t\t\t\tam->value = attr_value_unescape(v);\n \t\t\t}\n \t\t\tbreak;\n \t\t}\n@@ -239,7 +283,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n \tconst char *nextat;\n \n \tfor (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {\n-\t\tsize_t len = strcspn(pos, \",)\");\n+\t\tsize_t len = strcspn_escaped(pos, \",)\");\n \t\tint i;\n \n \t\tif (pos[len] == ',')\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex 3f06fcf8d..77b8cef66 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -178,4 +178,23 @@ test_expect_success 'abort on asking for wrong magic' '\n \ttest_must_fail git ls-files . \":(attr:!label=foo)\"\n '\n \n+test_expect_success 'check attribute list' '\n+\tcat <<-EOF >>.gitattributes &&\n+\t* whitespace=indent,trail,space\n+\tEOF\n+\tgit ls-files \":(attr:whitespace=indent\\,trail\\,space)\" >actual &&\n+\tgit ls-files >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'backslash cannot be the last character' '\n+\ttest_must_fail git ls-files \":(attr:label=foo\\\\ labelA=bar)\" 2>actual &&\n+\ttest_i18ngrep \"not allowed as last character in attr value\" actual\n+'\n+\n+test_expect_success 'backslash cannot be used as a value' '\n+\ttest_must_fail git ls-files \":(attr:label=f\\\\\\oo)\" 2>actual &&\n+\ttest_i18ngrep \"for value matching\" actual\n+'\n+\n test_done\n-- \n2.12.0.246.ga2ecc84866-goog\n\n"},{"id":"313919","messageId":"CAGZ79kYbkpjxJihZMh8W1sutjP1WtSE4FjXU3v-eFPYx0Pu_-g@mail.gmail.com","threadId":"45334","inReplyTo":"xmqqo9x5uc9u.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-13T18:30:26Z","receivedAt":"2017-03-13T18:30:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">> +test_expect_success 'fail if attr magic is used places not implemented' '\n>> +     # The main purpose of this test is to check that we actually fail\n>> +     # when you attempt to use attr magic in commands that do not implement\n>> +     # attr magic. This test does not advocate git-add to stay that way,\n>> +     # though, but git-add is convenient as it has its own internal pathspec\n>> +     # parsing.\n>\n> That's thought-provoking ;-) Would it help to add a test-pathspec\n> helper, similar to test-config helper, that serves as a vehicle to\n> test this?\n\nI think that is a very good idea; looking through files mentioned by\n\"ls t/ |grep pathspec\" most of the tests for pathspecs are done with\ngit-log currently, which also helps with DWIM checking, e.g. if the\npathspec part can be interpreted as a ref instead.\n\nSo I wonder if such a test-pathspec helper would first only cover this\ntest case of not supporting pathspecs, which sounds a bit counter intuitive.\n\nThanks,\nStefan\n"},{"id":"313996","messageId":"xmqqbmt4n71d.fsf@gitster.mtv.corp.google.com","threadId":"45334","inReplyTo":"20170313182322.27539-1-bmwill@google.com","subject":"Re: [PATCH v3 0/2] bringing attributes to pathspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-13T22:30:54Z","receivedAt":"2017-03-13T22:31:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> v3 fixes some nits in style in the test script (using <<-\\EOF instead of <<-EOF)\n> as well as fixing a few other minor things reported by Junio and Jonathan.\n\nThanks.  Will replace.\n\nI think this is ready for 'next', so let's ask reviewers to really\npay attention to this round, wait for a few days and merge it by the\nend of the week at the latest.\n\nThanks.\n"},{"id":"313999","messageId":"20170313223822.GB66601@google.com","threadId":"45334","inReplyTo":"xmqqbmt4n71d.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 0/2] bringing attributes to pathspecs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-13T22:38:22Z","receivedAt":"2017-03-13T22:38:31Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/13, Junio C Hamano wrote:\n> Brandon Williams <bmwill@google.com> writes:\n> \n> > v3 fixes some nits in style in the test script (using <<-\\EOF instead of <<-EOF)\n> > as well as fixing a few other minor things reported by Junio and Jonathan.\n> \n> Thanks.  Will replace.\n> \n> I think this is ready for 'next', so let's ask reviewers to really\n> pay attention to this round, wait for a few days and merge it by the\n> end of the week at the latest.\n\nSounds good to me, Thanks!\n\n-- \nBrandon Williams\n"},{"id":"314787","messageId":"CACsJy8BRTuH=xv_xvQPOkVaRcMXKnjgT77SXkwLkZ6aAvcTyHg@mail.gmail.com","threadId":"45334","inReplyTo":"20170313182322.27539-1-bmwill@google.com","subject":"Re: [PATCH v3 0/2] bringing attributes to pathspecs","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-21T10:51:29Z","receivedAt":"2017-03-21T10:52:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 14, 2017 at 1:23 AM, Brandon Williams <bmwill@google.com> wrote:\n> v3 fixes some nits in style in the test script (using <<-\\EOF instead of <<-EOF)\n> as well as fixing a few other minor things reported by Junio and Jonathan.\n\nI'm slowly digging through the pile of mails in the past weeks... I\nknow this has landed on 'master' (thanks!). Just wanted to check\nsomething.\n\nThe series updated match_pathspec(), but that's only one of two\npathspec filtering functions. The other is tree_entry_interesting()\n(e.g. for \"git grep <tree>\"). Do you have plans to support :(attr)\nthere too? \"No\" is a perfectly fine answer (and it will end up in my\nforever growing backlog).\n\nThe thing about tree_entry_interesting() is, we would want to stop\ntraversing subtrees as soon as possible. Naively implemented, we would\nneed to traverse all subtrees so we can call match_attrs(). That's not\ngreat. Oii I'm rambling.. I don't know yet how to implement this thing\nefficiently.\n-- \nDuy\n"},{"id":"314815","messageId":"xmqqr31qsk4s.fsf@gitster.mtv.corp.google.com","threadId":"45334","inReplyTo":"CACsJy8BRTuH=xv_xvQPOkVaRcMXKnjgT77SXkwLkZ6aAvcTyHg@mail.gmail.com","subject":"Re: [PATCH v3 0/2] bringing attributes to pathspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-21T15:51:47Z","receivedAt":"2017-03-21T15:52:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> The series updated match_pathspec(), but that's only one of two\n> pathspec filtering functions. The other is tree_entry_interesting()\n> (e.g. for \"git grep <tree>\"). Do you have plans to support :(attr)\n> there too? \"No\" is a perfectly fine answer (and it will end up in my\n> forever growing backlog).\n>\n> The thing about tree_entry_interesting() is, we would want to stop\n> traversing subtrees as soon as possible. Naively implemented, we would\n> need to traverse all subtrees so we can call match_attrs(). That's not\n> great. Oii I'm rambling.. I don't know yet how to implement this thing\n> efficiently.\n\nThanks for great insights.  \n\nIt indeed will become issue when an overly broad pathspec pattern is\ncombined with an attribute requirement, e.g. \".:(attr=X)\", and we\nmay have to devise a way to tell that there won't be any paths with\nthat satisfy the attribute requirement before descending into a tree\nas an optimization.\n"},{"id":"314820","messageId":"20170321165254.GA12937@google.com","threadId":"45334","inReplyTo":"CACsJy8BRTuH=xv_xvQPOkVaRcMXKnjgT77SXkwLkZ6aAvcTyHg@mail.gmail.com","subject":"Re: [PATCH v3 0/2] bringing attributes to pathspecs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-21T16:52:54Z","receivedAt":"2017-03-21T16:58:35Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/21, Duy Nguyen wrote:\n> On Tue, Mar 14, 2017 at 1:23 AM, Brandon Williams <bmwill@google.com> wrote:\n> > v3 fixes some nits in style in the test script (using <<-\\EOF instead of <<-EOF)\n> > as well as fixing a few other minor things reported by Junio and Jonathan.\n> \n> I'm slowly digging through the pile of mails in the past weeks... I\n> know this has landed on 'master' (thanks!). Just wanted to check\n> something.\n> \n> The series updated match_pathspec(), but that's only one of two\n> pathspec filtering functions. The other is tree_entry_interesting()\n> (e.g. for \"git grep <tree>\"). Do you have plans to support :(attr)\n> there too? \"No\" is a perfectly fine answer (and it will end up in my\n> forever growing backlog).\n> \n> The thing about tree_entry_interesting() is, we would want to stop\n> traversing subtrees as soon as possible. Naively implemented, we would\n> need to traverse all subtrees so we can call match_attrs(). That's not\n> great. Oii I'm rambling.. I don't know yet how to implement this thing\n> efficiently.\n\nI expect that this should be supported in tree_entry_interesting() at\nsome point, though I didn't have any immediate plans to do that yet.\nOne reason for that is I just haven't thought through all the cases to\nmake it performant (as you've pointed out).  So for now it'll probably\nbe appended to the backlog (yours, mine, or both) and if there ends up\nbeing a larger demand for it then we can increase the priority for it.\n\n-- \nBrandon Williams\n"}]}