{"thread":{"id":"42391","subject":"[PATCHv9 1/4] Documentation: fix a typo","startedAt":"2016-05-19T23:23:19Z","lastAt":"2016-05-20T18:31:13Z","messageCount":9,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"287052","messageId":"20160519232323.12775-1-sbeller@google.com","threadId":"42391","inReplyTo":null,"subject":"[PATCHv9 0/4] pathspec magic extension to search for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T23:23:19Z","receivedAt":"2016-05-19T23:23:19Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This goes on top of origin/jc/attr, (396bf756f95, attr: expose validity check\nfor attribute names)\n\nPatches 1 is a small fix, which could go independently as well.\nI dropped the patch \"string list: improve comment\"\nPatches 3 and 4 are refactoring pathspec.c a little.\nThese did not change since v7\n\nPatch 5 contains all of Junios suggestions.\n\nThanks,\nStefan\n\ndiff to v8:\ndiff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\nindex aa9f220..e06520b 100644\n--- a/Documentation/glossary-content.txt\n+++ b/Documentation/glossary-content.txt\n@@ -385,20 +385,23 @@ full pathname may have special meaning:\n Glob magic is incompatible with literal magic.\n \n attr;;\n-\tAdditionally to matching the pathspec, the path must have the\n-\tattribute as specified. The syntax for specifying the required\n-\tattributes is \"`attr: [mode] <attribute name> [=value]`\"\n-+\n-Attributes can have 4 states (Set, Unset, Set to a value, unspecified) and\n-you can query each attribute for certain states. The \"`[mode]`\" is a special\n-character to indicate which attribute states are looked for. The following\n-modes are available:\n-\n- - an empty \"`[mode]`\" matches if the attribute is set\n- - \"`-`\" the attribute must be unset\n- - \"`!`\" the attribute must be unspecified\n- - an empty \"`[mode]`\" combined with \"`[=value]`\" matches if the attribute has\n-   the given value.\n+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;;\ndiff --git a/dir.c b/dir.c\nindex f60a503..fc071af 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -231,11 +231,11 @@ static int match_attrs(const char *name, int namelen,\n \t\tmatch_mode = item->attr_match[i].match_mode;\n \n \t\tif (ATTR_TRUE(value))\n-\t\t\tmatched = match_mode == MATCH_SET;\n+\t\t\tmatched = (match_mode == MATCH_SET);\n \t\telse if (ATTR_FALSE(value))\n-\t\t\tmatched = match_mode == MATCH_UNSET;\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\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));\ndiff --git a/pathspec.c b/pathspec.c\nindex b795a9c..693a5e7 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -115,34 +115,38 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va\n \t\tconst char *attr = si->string;\n \t\tstruct attr_match *am = &item->attr_match[j];\n \n-\t\tif (attr[0] == '!')\n+\t\tattr_len = strcspn(attr, \"=\");\n+\t\tswitch (*attr) {\n+\t\tcase '!':\n \t\t\tam->match_mode = MATCH_UNSPECIFIED;\n-\t\telse if (attr[0] == '-')\n+\t\t\tattr++;\n+\t\t\tattr_len--;\n+\t\t\tbreak;\n+\t\tcase '-':\n \t\t\tam->match_mode = MATCH_UNSET;\n-\t\telse\n-\t\t\tam->match_mode = MATCH_SET;\n-\n-\t\tif (am->match_mode != MATCH_SET)\n-\t\t\t/* skip first character */\n \t\t\tattr++;\n+\t\t\tattr_len--;\n+\t\t\tbreak;\n+\t\tdefault:\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_len = strcspn(attr, \"=\");\n-\t\tif (attr[attr_len] == '=') {\n-\t\t\tam->match_mode = MATCH_VALUE;\n-\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n-\t\t\tif (strchr(am->value, '\\\\'))\n-\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n-\t\t} else\n-\t\t\tam->value = NULL;\n-\n-\t\tif (!attr_name_valid(attr, attr_len)) {\n+\t\tam->attr = git_attr_counted(attr, attr_len);\n+\t\tif (!am->attr) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n \t\t\tam->match_mode = INVALID_ATTR;\n \t\t\tinvalid_attr_name_message(&sb, attr, attr_len);\n \t\t\tdie(_(\"invalid attribute in '%s': '%s'\"), value, sb.buf);\n \t\t}\n \n-\t\tam->attr = git_attr_counted(attr, attr_len);\n \t\tgit_attr_check_append(item->attr_check, am->attr);\n \t}\n \ndiff --git a/string-list.h b/string-list.h\nindex 465a1f0..d3809a1 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -106,7 +106,7 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_\n  * list->strdup_strings must be set, as new memory needs to be\n  * allocated to hold the substrings.  If maxsplit is non-negative,\n  * then split at most maxsplit times.  Return the number of substrings\n- * appended to list. The list may be non-empty already.\n+ * appended to list.\n  *\n  * Examples:\n  *   string_list_split(l, \"foo:bar:baz\", ':', -1) -> [\"foo\", \"bar\", \"baz\"]\ndiff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh\nindex c0d8cda..5da1a63 100755\n--- a/t/t6134-pathspec-with-labels.sh\n+++ b/t/t6134-pathspec-with-labels.sh\n@@ -4,39 +4,38 @@ 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-\tfor p in fileA fileB fileC fileAB fileAC fileBC fileNoLabel fileUnsetLabel fileSetLabel fileValue fileWrongLabel; do\n-\t\t: >$p &&\n-\t\tgit add $p &&\n-\t\t: >sub/$p\n-\t\tgit add sub/$p\n-\tdone &&\n-\tgit commit -m $p &&\n+\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-\tcat <<EOF >expect &&\n-fileA\n-fileAB\n-fileAC\n-fileB\n-fileBC\n-fileC\n-fileNoLabel\n-fileSetLabel\n-fileUnsetLabel\n-fileValue\n-fileWrongLabel\n-sub/fileA\n-sub/fileAB\n-sub/fileAC\n-sub/fileB\n-sub/fileBC\n-sub/fileC\n-sub/fileNoLabel\n-sub/fileSetLabel\n-sub/fileUnsetLabel\n-sub/fileValue\n-sub/fileWrongLabel\n-EOF\n \ttest_cmp expect actual\n '\n \n@@ -51,47 +50,45 @@ test_expect_success 'pathspec with labels and non existent .gitattributes' '\n '\n \n test_expect_success 'setup .gitattributes' '\n-\tcat <<EOF >.gitattributes &&\n-fileA labelA\n-fileB labelB\n-fileC labelC\n-fileAB labelA labelB\n-fileAC labelA labelC\n-fileBC labelB labelC\n-fileUnsetLabel -label\n-fileSetLabel label\n-fileValue label=foo\n-fileWrongLabel label☺\n-EOF\n+\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-sq=\"'\"\n-\n test_expect_success 'check specific set attr' '\n-\tcat <<EOF >expect &&\n-fileSetLabel\n-sub/fileSetLabel\n-EOF\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-fileUnsetLabel\n-sub/fileUnsetLabel\n-EOF\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-fileValue\n-sub/fileValue\n-EOF\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@@ -99,61 +96,61 @@ EOF\n '\n \n test_expect_success 'check unspecified attr' '\n-\tcat <<EOF >expect &&\n-.gitattributes\n-fileA\n-fileAB\n-fileAC\n-fileB\n-fileBC\n-fileC\n-fileNoLabel\n-fileWrongLabel\n-sub/fileA\n-sub/fileAB\n-sub/fileAC\n-sub/fileB\n-sub/fileBC\n-sub/fileC\n-sub/fileNoLabel\n-sub/fileWrongLabel\n-EOF\n-\tgit ls-files :\\(attr:\\!label\\) >actual &&\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-.gitattributes\n-fileC\n-fileNoLabel\n-fileWrongLabel\n-sub/fileC\n-sub/fileNoLabel\n-sub/fileWrongLabel\n-EOF\n-\tgit ls-files :\\(attr:\\!labelB\\ \\!labelA\\ \\!label\\) >actual &&\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-fileAB\n-fileB\n-fileBC\n-EOF\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-fileAB\n-fileB\n-fileBC\n-sub/fileAB\n-sub/fileB\n-EOF\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"},{"id":"287049","messageId":"20160519232323.12775-2-sbeller@google.com","threadId":"42391","inReplyTo":"20160519232323.12775-1-sbeller@google.com","subject":"[PATCHv9 1/4] Documentation: fix a typo","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T23:23:20Z","receivedAt":"2016-05-19T23:23:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/gitattributes.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex e3b1de8..af2c682 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -86,7 +86,7 @@ is either not set or empty, $HOME/.config/git/attributes is used instead.\n Attributes for all users on a system should be placed in the\n `$(prefix)/etc/gitattributes` file.\n \n-Sometimes you would need to override an setting of an attribute\n+Sometimes you would need to override a setting of an attribute\n for a path to `Unspecified` state.  This can be done by listing\n the name of the attribute prefixed with an exclamation point `!`.\n \n-- \n2.8.2.123.gb4ad9b6.dirty\n"},{"id":"287051","messageId":"20160519232323.12775-3-sbeller@google.com","threadId":"42391","inReplyTo":"20160519232323.12775-1-sbeller@google.com","subject":"[PATCHv9 2/4] pathspec: move long magic parsing out of prefix_pathspec","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T23:23:21Z","receivedAt":"2016-05-19T23:23:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`prefix_pathspec` is quite a lengthy function and we plan on adding more.\nSplit it up for better readability. As we want to add code into the\ninner loop of the long magic parsing, we also benefit from lower\nindentation.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n pathspec.c | 84 +++++++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 47 insertions(+), 37 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex c9e9b6c..eba37c2 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -88,6 +88,52 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+static void eat_long_magic(struct pathspec_item *item, const char *elt,\n+\t\tunsigned *magic, int *pathspec_prefix,\n+\t\tconst char **copyfrom_, const char **long_magic_end)\n+{\n+\tint i;\n+\tconst char *copyfrom = *copyfrom_;\n+\t/* longhand */\n+\tconst char *nextat;\n+\tfor (copyfrom = elt + 2;\n+\t     *copyfrom && *copyfrom != ')';\n+\t     copyfrom = nextat) {\n+\t\tsize_t len = strcspn(copyfrom, \",)\");\n+\t\tif (copyfrom[len] == ',')\n+\t\t\tnextat = copyfrom + len + 1;\n+\t\telse\n+\t\t\t/* handle ')' and '\\0' */\n+\t\t\tnextat = copyfrom + len;\n+\t\tif (!len)\n+\t\t\tcontinue;\n+\t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n+\t\t\tif (strlen(pathspec_magic[i].name) == len &&\n+\t\t\t    !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n+\t\t\t\t*magic |= pathspec_magic[i].bit;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tif (starts_with(copyfrom, \"prefix:\")) {\n+\t\t\t\tchar *endptr;\n+\t\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n+\t\t\t\t\t\t\t  &endptr, 10);\n+\t\t\t\tif (endptr - copyfrom != len)\n+\t\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n+\t\t\t\t/* \"i\" would be wrong, but it does not matter */\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (ARRAY_SIZE(pathspec_magic) <= i)\n+\t\t\tdie(_(\"Invalid pathspec magic '%.*s' in '%s'\"),\n+\t\t\t    (int) len, copyfrom, elt);\n+\t}\n+\tif (*copyfrom != ')')\n+\t\tdie(_(\"Missing ')' at the end of pathspec magic in '%s'\"), elt);\n+\t*long_magic_end = copyfrom;\n+\tcopyfrom++;\n+\t*copyfrom_ = copyfrom;\n+}\n+\n /*\n  * Take an element of a pathspec and check for magic signatures.\n  * Append the result to the prefix. Return the magic bitmap.\n@@ -150,43 +196,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \t    (flags & PATHSPEC_LITERAL_PATH)) {\n \t\t; /* nothing to do */\n \t} else if (elt[1] == '(') {\n-\t\t/* longhand */\n-\t\tconst char *nextat;\n-\t\tfor (copyfrom = elt + 2;\n-\t\t     *copyfrom && *copyfrom != ')';\n-\t\t     copyfrom = nextat) {\n-\t\t\tsize_t len = strcspn(copyfrom, \",)\");\n-\t\t\tif (copyfrom[len] == ',')\n-\t\t\t\tnextat = copyfrom + len + 1;\n-\t\t\telse\n-\t\t\t\t/* handle ')' and '\\0' */\n-\t\t\t\tnextat = copyfrom + len;\n-\t\t\tif (!len)\n-\t\t\t\tcontinue;\n-\t\t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n-\t\t\t\tif (strlen(pathspec_magic[i].name) == len &&\n-\t\t\t\t    !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n-\t\t\t\t\tmagic |= pathspec_magic[i].bit;\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t\tif (starts_with(copyfrom, \"prefix:\")) {\n-\t\t\t\t\tchar *endptr;\n-\t\t\t\t\tpathspec_prefix = strtol(copyfrom + 7,\n-\t\t\t\t\t\t\t\t &endptr, 10);\n-\t\t\t\t\tif (endptr - copyfrom != len)\n-\t\t\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n-\t\t\t\t\t/* \"i\" would be wrong, but it does not matter */\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t\tif (ARRAY_SIZE(pathspec_magic) <= i)\n-\t\t\t\tdie(_(\"Invalid pathspec magic '%.*s' in '%s'\"),\n-\t\t\t\t    (int) len, copyfrom, elt);\n-\t\t}\n-\t\tif (*copyfrom != ')')\n-\t\t\tdie(_(\"Missing ')' at the end of pathspec magic in '%s'\"), elt);\n-\t\tlong_magic_end = copyfrom;\n-\t\tcopyfrom++;\n+\t\teat_long_magic(item, elt, &magic, &pathspec_prefix, &copyfrom, &long_magic_end);\n \t} else {\n \t\t/* shorthand */\n \t\tfor (copyfrom = elt + 1;\n-- \n2.8.2.123.gb4ad9b6.dirty\n"},{"id":"287050","messageId":"20160519232323.12775-4-sbeller@google.com","threadId":"42391","inReplyTo":"20160519232323.12775-1-sbeller@google.com","subject":"[PATCHv9 3/4] pathspec: move prefix check out of the inner loop","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T23:23:22Z","receivedAt":"2016-05-19T23:23:22Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The prefix check is not related the check of pathspec magic; also there\nis no code that is relevant after we'd break the loop on a match for\n\"prefix:\". So move the check before the loop and shortcircuit the outer\nloop.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n pathspec.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex eba37c2..4dff252 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -107,21 +107,22 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \t\t\tnextat = copyfrom + len;\n \t\tif (!len)\n \t\t\tcontinue;\n+\n+\t\tif (starts_with(copyfrom, \"prefix:\")) {\n+\t\t\tchar *endptr;\n+\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n+\t\t\t\t\t\t  &endptr, 10);\n+\t\t\tif (endptr - copyfrom != len)\n+\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n \t\t\tif (strlen(pathspec_magic[i].name) == len &&\n \t\t\t    !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n \t\t\t\t*magic |= pathspec_magic[i].bit;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tif (starts_with(copyfrom, \"prefix:\")) {\n-\t\t\t\tchar *endptr;\n-\t\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n-\t\t\t\t\t\t\t  &endptr, 10);\n-\t\t\t\tif (endptr - copyfrom != len)\n-\t\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n-\t\t\t\t/* \"i\" would be wrong, but it does not matter */\n-\t\t\t\tbreak;\n-\t\t\t}\n \t\t}\n \t\tif (ARRAY_SIZE(pathspec_magic) <= i)\n \t\t\tdie(_(\"Invalid pathspec magic '%.*s' in '%s'\"),\n-- \n2.8.2.123.gb4ad9b6.dirty\n"},{"id":"287053","messageId":"20160519232323.12775-5-sbeller@google.com","threadId":"42391","inReplyTo":"20160519232323.12775-1-sbeller@google.com","subject":"[PATCHv9 4/4] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T23:23:23Z","receivedAt":"2016-05-19T23:23:23Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The pathspec mechanism is extended via the new\n\":(attr:eol=input)pattern/to/match\" syntax to filter paths so that it\nrequires paths to not just match the given pattern but also have the\nspecified attrs attached for them to be chosen.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/glossary-content.txt |  20 +++++\n dir.c                              |  35 ++++++++\n pathspec.c                         | 101 ++++++++++++++++++++++-\n pathspec.h                         |  16 ++++\n t/t6134-pathspec-with-labels.sh    | 163 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 331 insertions(+), 4 deletions(-)\n create mode 100755 t/t6134-pathspec-with-labels.sh\n\ndiff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\nindex cafc284..e06520b 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: `!`). If it\ndiff --git a/dir.c b/dir.c\nindex 996653b..fc071af 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -9,6 +9,7 @@\n  */\n #include \"cache.h\"\n #include \"dir.h\"\n+#include \"attr.h\"\n #include \"refs.h\"\n #include \"wildmatch.h\"\n #include \"pathspec.h\"\n@@ -215,6 +216,37 @@ int within_depth(const char *name, int namelen,\n \treturn 1;\n }\n \n+static int match_attrs(const char *name, int namelen,\n+\t\t       const struct pathspec_item *item)\n+{\n+\tint i;\n+\n+\tgit_check_attr_counted(name, namelen, 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->check[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+\n+\t\tif (!matched)\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n #define DO_MATCH_EXCLUDE   1\n #define DO_MATCH_DIRECTORY 2\n \n@@ -270,6 +302,9 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,\n \t    strncmp(item->match, name - prefix, item->prefix))\n \t\treturn 0;\n \n+\tif (item->attr_match_nr && !match_attrs(name, namelen, item))\n+\t\treturn 0;\n+\n \t/* If the match was just the prefix, we matched */\n \tif (!*match)\n \t\treturn MATCHED_RECURSIVELY;\ndiff --git a/pathspec.c b/pathspec.c\nindex 4dff252..693a5e7 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"dir.h\"\n #include \"pathspec.h\"\n+#include \"attr.h\"\n \n /*\n  * Finds which of the given pathspecs match items in the index.\n@@ -88,12 +89,78 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)\n+{\n+\tstruct string_list_item *si;\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\n+\n+\tif (!value || !strlen(value))\n+\t\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+\tif (!item->attr_check)\n+\t\titem->attr_check = git_attr_check_alloc();\n+\telse\n+\t\tdie(_(\"Only one 'attr:' specification is allowed.\"));\n+\n+\tALLOC_GROW(item->attr_match, item->attr_match_nr + list.nr, item->attr_match_alloc);\n+\n+\tfor_each_string_list_item(si, &list) {\n+\t\tsize_t attr_len;\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\tattr_len = strcspn(attr, \"=\");\n+\t\tswitch (*attr) {\n+\t\tcase '!':\n+\t\t\tam->match_mode = MATCH_UNSPECIFIED;\n+\t\t\tattr++;\n+\t\t\tattr_len--;\n+\t\t\tbreak;\n+\t\tcase '-':\n+\t\t\tam->match_mode = MATCH_UNSET;\n+\t\t\tattr++;\n+\t\t\tattr_len--;\n+\t\t\tbreak;\n+\t\tdefault:\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\tam->attr = git_attr_counted(attr, attr_len);\n+\t\tif (!am->attr) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tam->match_mode = INVALID_ATTR;\n+\t\t\tinvalid_attr_name_message(&sb, attr, attr_len);\n+\t\t\tdie(_(\"invalid attribute in '%s': '%s'\"), value, sb.buf);\n+\t\t}\n+\n+\t\tgit_attr_check_append(item->attr_check, am->attr);\n+\t}\n+\n+\tstring_list_clear(&list, 0);\n+\treturn;\n+}\n+\n static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \t\tunsigned *magic, int *pathspec_prefix,\n \t\tconst char **copyfrom_, const char **long_magic_end)\n {\n \tint i;\n \tconst char *copyfrom = *copyfrom_;\n+\tconst char *body;\n \t/* longhand */\n \tconst char *nextat;\n \tfor (copyfrom = elt + 2;\n@@ -108,15 +175,21 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \t\tif (!len)\n \t\t\tcontinue;\n \n-\t\tif (starts_with(copyfrom, \"prefix:\")) {\n+\t\tif (skip_prefix(copyfrom, \"prefix:\", &body)) {\n \t\t\tchar *endptr;\n-\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n-\t\t\t\t\t\t  &endptr, 10);\n+\t\t\t*pathspec_prefix = strtol(body, &endptr, 10);\n \t\t\tif (endptr - copyfrom != len)\n \t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (skip_prefix(copyfrom, \"attr:\", &body)) {\n+\t\t\tchar *attr_body = xmemdupz(body, len - strlen(\"attr:\"));\n+\t\t\tparse_pathspec_attr_match(item, attr_body);\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, copyfrom, len)) {\n@@ -425,7 +498,10 @@ void parse_pathspec(struct pathspec *pathspec,\n \tfor (i = 0; i < n; i++) {\n \t\tunsigned short_magic;\n \t\tentry = argv[i];\n-\n+\t\titem[i].attr_check = NULL;\n+\t\titem[i].attr_match = NULL;\n+\t\titem[i].attr_match_nr = 0;\n+\t\titem[i].attr_match_alloc = 0;\n \t\titem[i].magic = prefix_pathspec(item + i, &short_magic,\n \t\t\t\t\t\targv + i, flags,\n \t\t\t\t\t\tprefix, prefixlen, entry);\n@@ -447,6 +523,13 @@ void parse_pathspec(struct pathspec *pathspec,\n \t\tif (item[i].nowildcard_len < item[i].len)\n \t\t\tpathspec->has_wildcard = 1;\n \t\tpathspec->magic |= item[i].magic;\n+\n+\t\tif (item[i].attr_match_nr) {\n+\t\t\tint j;\n+\t\t\tfor (j = 0; j < item[i].attr_match_nr; j++)\n+\t\t\t\tif (item[i].attr_match[j].match_mode == INVALID_ATTR)\n+\t\t\t\t\tdie(_(\"attribute spec in the wrong syntax are prohibited.\"));\n+\t\t}\n \t}\n \n \tif (nr_exclude == n)\n@@ -502,6 +585,16 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n \n void free_pathspec(struct pathspec *pathspec)\n {\n+\tint i, j;\n+\tfor (i = 0; i < pathspec->nr; i++) {\n+\t\tif (!pathspec->items[i].attr_match_nr)\n+\t\t\tcontinue;\n+\t\tfor (j = 0; j < pathspec->items[j].attr_match_nr; j++)\n+\t\t\tfree(pathspec->items[i].attr_match[j].value);\n+\t\tfree(pathspec->items[i].attr_match);\n+\t\tgit_attr_check_free(pathspec->items[i].attr_check);\n+\t}\n+\n \tfree(pathspec->items);\n \tpathspec->items = NULL;\n }\ndiff --git a/pathspec.h b/pathspec.h\nindex 0c11262..5308137 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -32,6 +32,22 @@ struct pathspec {\n \t\tint len, prefix;\n \t\tint nowildcard_len;\n \t\tint flags;\n+\t\tint attr_match_nr;\n+\t\tint attr_match_alloc;\n+\t\tstruct attr_match {\n+\t\t\tstruct git_attr *attr;\n+\t\t\tchar *value;\n+\t\t\tenum attr_match_mode {\n+\t\t\t\tMATCH_SET,\n+\t\t\t\tMATCH_UNSET,\n+\t\t\t\tMATCH_VALUE,\n+\t\t\t\tMATCH_UNSPECIFIED,\n+\t\t\t\tMATCH_NOT_UNSPECIFIED,\n+\t\t\t\tMATCH_SET_OR_VALUE,\n+\t\t\t\tINVALID_ATTR\n+\t\t\t} match_mode;\n+\t\t} *attr_match;\n+\t\tstruct git_attr_check *attr_check;\n \t} *items;\n };\n \ndiff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh\nnew file mode 100755\nindex 0000000..5da1a63\n--- /dev/null\n+++ b/t/t6134-pathspec-with-labels.sh\n@@ -0,0 +1,163 @@\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:)\" 2>actual &&\n+\ttest_i18ngrep fatal actual\n+'\n+\n+test_expect_success 'pathspec with labels and non existent .gitattributes' '\n+\tgit ls-files \":(attr:label)\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'setup .gitattributes' '\n+\tcat <<-EOF >.gitattributes &&\n+\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 'abort on giving invalid label on the command line' '\n+\ttest_must_fail git ls-files . \":(attr:☺)\" 2>actual &&\n+\ttest_i18ngrep \"fatal\" actual\n+'\n+\n+test_done\n-- \n2.8.2.123.gb4ad9b6.dirty\n"},{"id":"287055","messageId":"xmqqbn41hbeq.fsf@gitster.mtv.corp.google.com","threadId":"42391","inReplyTo":"20160519232323.12775-5-sbeller@google.com","subject":"Re: [PATCHv9 4/4] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-19T23:32:13Z","receivedAt":"2016-05-19T23:32:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> +attr;;\n> +After `attr:` comes a space separated list of \"attribute\n> +...\n> ++\n\nThe text looks OK, but does it format well?\n\n> +\t\tattr_len = strcspn(attr, \"=\");\n\nScanning for '=' here retains the same bug from the previous\niteration where you take !VAR=VAL and silently ignore =VAL part\nwithout diagnosing the error, doesn't it?\n\nPerhaps strlen(attr) here, and...\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--;\n> +\t\t\tbreak;\n> +\t\tcase '-':\n> +\t\t\tam->match_mode = MATCH_UNSET;\n> +\t\t\tattr++;\n> +\t\t\tattr_len--;\n> +\t\t\tbreak;\n> +\t\tdefault:\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... doing strcspn() only in default: part would be a quick fix.\n"},{"id":"287118","messageId":"xmqqd1ogegtn.fsf@gitster.mtv.corp.google.com","threadId":"42391","inReplyTo":"xmqqbn41hbeq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv9 4/4] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-20T18:15:48Z","receivedAt":"2016-05-20T18:15:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> +attr;;\n>> +After `attr:` comes a space separated list of \"attribute\n>> +...\n>> ++\n>\n> The text looks OK, but does it format well?\n\nI didn't check this, but the remainder would look like this\nsquashable patch.\n\nYou seem to i18ngrep for \"fatal\" but we are using test_must_fail for\nthe exit status, so I am not sure if that adds much value, so the\nadditional tests here do nto use that pattern.\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 693a5e7..0a02255 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -115,19 +115,19 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va\n \t\tconst char *attr = si->string;\n \t\tstruct attr_match *am = &item->attr_match[j];\n \n-\t\tattr_len = strcspn(attr, \"=\");\n \t\tswitch (*attr) {\n \t\tcase '!':\n \t\t\tam->match_mode = MATCH_UNSPECIFIED;\n \t\t\tattr++;\n-\t\t\tattr_len--;\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--;\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 {\ndiff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh\nindex 5da1a63..060047a 100755\n--- a/t/t6134-pathspec-with-labels.sh\n+++ b/t/t6134-pathspec-with-labels.sh\n@@ -160,4 +160,9 @@ test_expect_success 'abort on giving invalid label on the command line' '\n \ttest_i18ngrep \"fatal\" actual\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"},{"id":"287120","messageId":"CAGZ79kZOmo6hh_trBJ_H5QRWgAxbP4JUu_KBUKeeKL1XsAwiOQ@mail.gmail.com","threadId":"42391","inReplyTo":"xmqqd1ogegtn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv9 4/4] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-20T18:21:40Z","receivedAt":"2016-05-20T18:21:40Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 20, 2016 at 11:15 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> +attr;;\n>>> +After `attr:` comes a space separated list of \"attribute\n>>> +...\n>>> ++\n>>\n>> The text looks OK, but does it format well?\n>\n> I didn't check this, but the remainder would look like this\n> squashable patch.\n\nI checked and it looks wrong. the \"exclude\" section is indented below\nthe new attr section\n\nfix is:\n--8<--\ndiff --git a/Documentation/glossary-content.txt\nb/Documentation/glossary-content.txt\nindex e06520b..181f52e 100644\n--- a/Documentation/glossary-content.txt\n+++ b/Documentation/glossary-content.txt\n@@ -389,7 +389,7 @@ 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++\n Each of the attribute requirements for the path takes one of\n these forms:\n--8<--\n\nI can resend with your proposed fixes as well.\n\nThanks,\nStefan\n\n\n\n>\n> You seem to i18ngrep for \"fatal\" but we are using test_must_fail for\n> the exit status, so I am not sure if that adds much value, so the\n> additional tests here do nto use that pattern.\n>\n> diff --git a/pathspec.c b/pathspec.c\n> index 693a5e7..0a02255 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -115,19 +115,19 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va\n>                 const char *attr = si->string;\n>                 struct attr_match *am = &item->attr_match[j];\n>\n> -               attr_len = strcspn(attr, \"=\");\n>                 switch (*attr) {\n>                 case '!':\n>                         am->match_mode = MATCH_UNSPECIFIED;\n>                         attr++;\n> -                       attr_len--;\n> +                       attr_len = strlen(attr);\n>                         break;\n>                 case '-':\n>                         am->match_mode = MATCH_UNSET;\n>                         attr++;\n> -                       attr_len--;\n> +                       attr_len = strlen(attr);\n>                         break;\n>                 default:\n> +                       attr_len = strcspn(attr, \"=\");\n>                         if (attr[attr_len] != '=')\n>                                 am->match_mode = MATCH_SET;\n>                         else {\n> diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh\n> index 5da1a63..060047a 100755\n> --- a/t/t6134-pathspec-with-labels.sh\n> +++ b/t/t6134-pathspec-with-labels.sh\n> @@ -160,4 +160,9 @@ test_expect_success 'abort on giving invalid label on the command line' '\n>         test_i18ngrep \"fatal\" actual\n>  '\n>\n> +test_expect_success 'abort on asking for wrong magic' '\n> +       test_must_fail git ls-files . \":(attr:-label=foo)\" &&\n> +       test_must_fail git ls-files . \":(attr:!label=foo)\"\n> +'\n> +\n>  test_done\n"},{"id":"287123","messageId":"xmqq4m9seg3y.fsf@gitster.mtv.corp.google.com","threadId":"42391","inReplyTo":"CAGZ79kZOmo6hh_trBJ_H5QRWgAxbP4JUu_KBUKeeKL1XsAwiOQ@mail.gmail.com","subject":"Re: [PATCHv9 4/4] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-20T18:31:13Z","receivedAt":"2016-05-20T18:31:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> I checked and it looks wrong. the \"exclude\" section is indented below\n> the new attr section\n> ...\n> I can resend with your proposed fixes as well.\n\nIf you do so, please make sure that the way tests check for error\ncondition are consistent.  I personally do not think it adds any\nvalue to grep for \"fatal\", but I do not mind if you adjusted them to\ngo the same test_i18ngrep route if you think it does (and if you\nagree that grepping is not necessary, please do not forget to update\nthe tests you had that use test_i18ngrep).\n\nThanks.\n"}]}