{"thread":{"id":"42352","subject":"[RFC-PATCHv6 3/4] pathspec: move prefix check out of the inner loop","startedAt":"2016-05-17T03:13:49Z","lastAt":"2016-05-18T15:39:06Z","messageCount":18,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"286758","messageId":"20160517031353.23707-1-sbeller@google.com","threadId":"42352","inReplyTo":null,"subject":"[RFC-PATCHv6 0/4] pathspec attrs [WAS pathspec labels [WAS submodule groups]]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T03:13:49Z","receivedAt":"2016-05-17T03:13:49Z","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 and is RFC as\n I did not write tests nor documentation, yet.\n I wanted to understand Junios series, so I built on top.\n \nThe meat is in the last patch, which allows for\n\n git ls-files :(attr:-text)path/pattern # (ATTR_FALSE)\n git ls-files :(attr:+text)path/pattern # (ATTR_TRUE)\n git ls-files :(attr:eol=input)path/pattern # values must match\n \n git ls-files :(attr:!text)path/pattern # find patterns with no \"text\" attribute, i.e. neither FALSE, TRUE or value\n git ls-files :(attr:text)path/pattern # opposite of !; find files which are TRUE, FALSE or value\n \nOf course you can chain them:\n\n git ls-files :(attr:text,attr:eol=lf)path/pattern # must match both attr specs.\n \nFeedback on the parsing and design welcome,\n\nThanks,\nStefan\n \n\nStefan Beller (4):\n Documentation: fix a typo\n pathspec: move long magic parsing out of prefix_pathspec\n pathspec: move prefix check out of the inner loop\n pathspec: allow querying for attributes\n\n Documentation/gitattributes.txt | 2 +-\n attr.c | 2 +-\n attr.h | 2 +\n dir.c | 49 ++++++++++++\n pathspec.c | 172 +++++++++++++++++++++++++++++++---------\n pathspec.h | 16 ++++\n 6 files changed, 203 insertions(+), 40 deletions(-)\n\n-- \n2.8.2.401.g9c0faef\n"},{"id":"286757","messageId":"20160517031353.23707-2-sbeller@google.com","threadId":"42352","inReplyTo":"20160517031353.23707-1-sbeller@google.com","subject":"[RFC-PATCHv6 1/4] Documentation: fix a typo","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T03:13:50Z","receivedAt":"2016-05-17T03:13:50Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \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.401.g9c0faef\n"},{"id":"286759","messageId":"20160517031353.23707-3-sbeller@google.com","threadId":"42352","inReplyTo":"20160517031353.23707-1-sbeller@google.com","subject":"[RFC-PATCHv6 2/4] pathspec: move long magic parsing out of prefix_pathspec","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T03:13:51Z","receivedAt":"2016-05-17T03:13:51Z","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 \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, ©from, &long_magic_end);\n \t} else {\n \t\t/* shorthand */\n \t\tfor (copyfrom = elt + 1;\n-- \n2.8.2.401.g9c0faef\n"},{"id":"286756","messageId":"20160517031353.23707-4-sbeller@google.com","threadId":"42352","inReplyTo":"20160517031353.23707-1-sbeller@google.com","subject":"[RFC-PATCHv6 3/4] pathspec: move prefix check out of the inner loop","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T03:13:52Z","receivedAt":"2016-05-17T03:13:52Z","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 \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.401.g9c0faef\n"},{"id":"286760","messageId":"20160517031353.23707-5-sbeller@google.com","threadId":"42352","inReplyTo":"20160517031353.23707-1-sbeller@google.com","subject":"[RFC-PATCHv6 4/4] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T03:13:53Z","receivedAt":"2016-05-17T03:13:53Z","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 \n---\n attr.c | 2 +-\n attr.h | 2 ++\n dir.c | 49 +++++++++++++++++++++++++++++++++\n pathspec.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---\n pathspec.h | 16 +++++++++++\n 5 files changed, 157 insertions(+), 5 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 5178eb8..2d68a3c 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -59,7 +59,7 @@ static unsigned hash_name(const char *name, int namelen)\n \treturn val;\n }\n \n-static int invalid_attr_name(const char *name, int namelen)\n+int invalid_attr_name(const char *name, int namelen)\n {\n \t/*\n \t * Attribute name cannot begin with '-' and must consist of\ndiff --git a/attr.h b/attr.h\nindex 7dc49f8..7fd8b90 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -45,6 +45,8 @@ extern void git_attr_check_append(struct git_attr_check *, const char *);\n extern void git_attr_check_clear(struct git_attr_check *);\n extern void git_attr_check_free(struct git_attr_check *);\n \n+extern int invalid_attr_name(const char *name, int namelen);\n+\n /*\n * Return the name of the attribute represented by the argument. The\n * return value is a pointer to a null-delimited string that is part\ndiff --git a/dir.c b/dir.c\nindex 996653b..540ea1f 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,51 @@ int within_depth(const char *name, int namelen,\n \treturn 1;\n }\n \n+static struct git_attr_check *check;\n+static int match_attrs(const char *name, int namelen,\n+\t\t const struct pathspec_item *item)\n+{\n+\tchar *path;\n+\tint i;\n+\n+\tif (!check) {\n+\t\tcheck = git_attr_check_alloc();\n+\t\tfor (i = 0; i < item->attr_nr; i++)\n+\t\t\tgit_attr_check_append(check, item->attrs[i].attr);\n+\t}\n+\n+\tpath = xmemdupz(name, namelen);\n+\tgit_all_attrs(path, check);\n+\n+\tfor (i = 0; i < item->attr_nr; i++) {\n+\t\tint matched;\n+\t\tconst char *value = check->check[i].value;\n+\n+\t\tif (ATTR_TRUE(value)) {\n+\t\t\tmatched = (item->attrs[i].mode == MATCH_SET ||\n+\t\t\t\t item->attrs[i].mode == MATCH_NOT_UNSPECIFIED);\n+\t\t} else if (ATTR_FALSE(value)) {\n+\t\t\tmatched = (item->attrs[i].mode == MATCH_UNSET ||\n+\t\t\t\t item->attrs[i].mode == MATCH_NOT_UNSPECIFIED);\n+\t\t} else if (ATTR_UNSET(value)) {\n+\t\t\tmatched = (item->attrs[i].mode == MATCH_UNSPECIFIED);\n+\t\t} else {\n+\t\t\tif (item->attrs[i].mode == MATCH_NOT_UNSPECIFIED) {\n+\t\t\t\tmatched = 1;\n+\t\t\t} else {\n+\t\t\t\t/* NEEDSWORK: better value matching */\n+\t\t\t\tmatched = !strcmp(item->attrs[i].value, value);\n+\t\t\t}\n+\t\t}\n+\t\tif (!matched)\n+\t\t\treturn 0;\n+\t}\n+\n+\tfree(path);\n+\n+\treturn 1;\n+}\n+\n #define DO_MATCH_EXCLUDE 1\n #define DO_MATCH_DIRECTORY 2\n \n@@ -270,6 +316,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_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..523ac8c 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,73 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+/*\n+ * Check if the given string is a good specification to search for attributes.\n+ * Accepted strings:\n+ * [+-!] ATTRIBUTE_NAME [= ]\n+ *\n+ * Examples:\n+ * attr:+val to find value set to true\n+ * attr:-val to find a value set to false\n+ * attr:!val to find a value that is not set\n+ * (i.e. it is neither set as \"val\", \"val=\", nor unset as \"-val\")\n+ * attr:val=value: to find value that have at least a and b set.\n+ *\n+ * This only checks one attr, multiple attrs need to be given as multiple\n+ * strings.\n+ */\n+\n+static void parse_attr_item(struct attr_item *attr, const char *value)\n+{\n+\tsize_t val_len;\n+\tconst char *val = value;\n+\n+\tif (!val || !strlen(val))\n+\t\tgoto out;\n+\n+\tif (val[0] == '+')\n+\t\tattr->mode = MATCH_SET;\n+\telse if (val[0] == '-')\n+\t\tattr->mode = MATCH_UNSET;\n+\telse if (val[0] == '!')\n+\t\tattr->mode = MATCH_UNSPECIFIED;\n+\telse\n+\t\tattr->mode = NOT_INIT;\n+\n+\tif (attr->mode != NOT_INIT)\n+\t\tval++;\n+\n+\tval_len = strcspn(val, \"=,)\");\n+\tif (val[val_len] == '=')\n+\t\tattr->mode = MATCH_VALUE;\n+\telse\n+\t\tattr->mode = MATCH_NOT_UNSPECIFIED;\n+\n+\tif (invalid_attr_name(val, val_len))\n+\t\tgoto out;\n+\n+\tattr->attr = xmemdupz(val, val_len);\n+\n+\tif (attr->mode == MATCH_VALUE) {\n+\t\tconst char *after_equal = val + val_len + 1;\n+\t\tsize_t after_equal_end = strcspn(after_equal, \",)\");\n+\t\tattr->value = xmemdupz(after_equal, after_equal_end);\n+\t} else\n+\t\tattr->value = NULL;\n+\treturn;\n+out:\n+\tattr->mode = INVALID_ATTR;\n+\twarning(_(\"attr spec '%s': attrs must not start with '-' and \"\n+\t\t \"be composed of [-A-Za-z0-9_.].\"), value);\n+}\n+\n static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \t\tunsigned *magic, int *pathspec_prefix,\n \t\tconst char **copyfrom_, const char **long_magic_end)\n {\n \tint i;\n \tconst char *copyfrom = *copyfrom_;\n+\tconst char *body;\n \t/* longhand */\n \tconst char *nextat;\n \tfor (copyfrom = elt + 2;\n@@ -108,15 +170,20 @@ 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\tALLOC_GROW(item->attrs, item->attr_nr + 1, item->attr_alloc);\n+\t\t\tparse_attr_item(&item->attrs[item->attr_nr++], 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 +492,9 @@ 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].attrs = NULL;\n+\t\titem[i].attr_nr = 0;\n+\t\titem[i].attr_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 +516,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_nr) {\n+\t\t\tint j;\n+\t\t\tfor (j = 0; j < item[i].attr_nr; j++)\n+\t\t\t\tif (item[i].attrs[j].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 +578,15 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n \n void free_pathspec(struct pathspec *pathspec)\n {\n+\tint i, j;\n+\tfor (i = 0; i < pathspec->nr; i++) {\n+\t\tfor (j = 0; j < pathspec->items[j].attr_nr; j++) {\n+\t\t\tfree(pathspec->items[i].attrs[j].attr);\n+\t\t\tfree(pathspec->items[i].attrs[j].value);\n+\t\t}\n+\t\tfree(pathspec->items[i].attrs);\n+\t}\n+\n \tfree(pathspec->items);\n \tpathspec->items = NULL;\n }\ndiff --git a/pathspec.h b/pathspec.h\nindex 0c11262..89d73db 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -32,6 +32,21 @@ struct pathspec {\n \t\tint len, prefix;\n \t\tint nowildcard_len;\n \t\tint flags;\n+\t\tint attr_nr;\n+\t\tint attr_alloc;\n+\t\tstruct attr_item {\n+\t\t\tchar *attr;\n+\t\t\tchar *value;\n+\t\t\tenum attr_match_mode {\n+\t\t\t\tNOT_INIT,\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\tINVALID_ATTR\n+\t\t\t} mode;\n+\t\t} *attrs;\n \t} *items;\n };\n \n@@ -98,5 +113,6 @@ extern char *find_pathspecs_matching_against_index(const struct pathspec *pathsp\n extern void add_pathspec_matches_against_index(const struct pathspec *pathspec, char *seen);\n extern const char *check_path_for_gitlink(const char *path);\n extern void die_if_path_beyond_symlink(const char *path, const char *prefix);\n+extern int validate_label_name(const char *label);\n \n #endif /* PATHSPEC_H */\n-- \n2.8.2.401.g9c0faef\n"},{"id":"286773","messageId":"xmqqeg91z5h2.fsf@gitster.mtv.corp.google.com","threadId":"42352","inReplyTo":"20160517031353.23707-1-sbeller@google.com","subject":"Re: [RFC-PATCHv6 0/4] pathspec attrs [WAS pathspec labels [WAS submodule groups]]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-17T04:14:01Z","receivedAt":"2016-05-17T04:14:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I wanted to understand Junios series, so I built on top.\n\nThanks.\n"},{"id":"286776","messageId":"xmqqa8jpz51m.fsf@gitster.mtv.corp.google.com","threadId":"42352","inReplyTo":"20160517031353.23707-5-sbeller@google.com","subject":"Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-17T04:23:17Z","receivedAt":"2016-05-17T04:23:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> + * attr:+val to find value set to true\n> + * attr:-val to find a value set to false\n> + * attr:!val to find a value that is not set\n> + * (i.e. it is neither set as \"val\", \"val=\", nor unset as \"-val\")\n> + * attr:val=value: to find value that have at least a and b set.\n\nI would have expected that there won't be \"attr:+val\", but it is\nspelled as \"attr:val\" instead.\n\n> +static void parse_attr_item(struct attr_item *attr, const char *value)\n\nPlease do not call something that is not part of the attribute\ninfrastructure as \"attr_item\"; I wasted time looking for the\nstructure definition for \"attr_item\" in .\n\n> +static int match_attrs(const char *name, int namelen,\n> +\t\t const struct pathspec_item *item)\n> +{\n> +\tchar *path;\n> +\tint i;\n> +\n> +\tif (!check) {\n> +\t\tcheck = git_attr_check_alloc();\n> +\t\tfor (i = 0; i < item->attr_nr; i++)\n> +\t\t\tgit_attr_check_append(check, item->attrs[i].attr);\n> +\t}\n> +\n> +\tpath = xmemdupz(name, namelen);\n> +\tgit_all_attrs(path, check);\n\nPLEASE DON'T. git_all_attrs() asks for all the attribute under the\nsun and has no hope to perform sensibly, especially at the very leaf\nlevel of the pathspec logic where one call to this function is made\nfor each and every path in the tree.\n\nInstead, have a pointer to \"struct git_attr_check\" in pathspec_item\nand make a call to git_check_attr(path, item->check) here.\n\nWhich means that you would need to prepare git_attr_check around ...\n\n> +\t\tif (skip_prefix(copyfrom, \"attr:\", &body)) {\n> +\t\t\tALLOC_GROW(item->attrs, item->attr_nr + 1, item->attr_alloc);\n> +\t\t\tparse_attr_item(&item->attrs[item->attr_nr++], body);\n\n... HERE.\n"},{"id":"286777","messageId":"xmqqvb2dxomo.fsf@gitster.mtv.corp.google.com","threadId":"42352","inReplyTo":"20160517031353.23707-5-sbeller@google.com","subject":"Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-17T05:03:11Z","receivedAt":"2016-05-17T05:03:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +static struct git_attr_check *check;\n> +static int match_attrs(const char *name, int namelen,\n> +\t\t const struct pathspec_item *item)\n> +{\n> +\tchar *path;\n> +\tint i;\n> +\n> +\tif (!check) {\n> +\t\tcheck = git_attr_check_alloc();\n> +\t\tfor (i = 0; i < item->attr_nr; i++)\n> +\t\t\tgit_attr_check_append(check, item->attrs[i].attr);\n\nThis is simply wrong; you may have two pathspec elements with\nattribute match magic, the first one may ask for one attribute while\nthe second one may ask for seven. The first time around you\nallocate and append one attribute. The second time around you don't\ndo anything useful, and send a git_attr_check with one element to\ndeal with 7 attributes.\n\n> +\t}\n> +\tpath = xmemdupz(name, namelen);\n> +\tgit_all_attrs(path, check);\n\nHowever, the above \"This is simply wrong\" bogosity is covered\nbecause git_all_attrs() is used here, ignoring what is in check.\n\nThe loop we see above is an expensive no-op, as the first thing\nall_attrs() does is to empty check() and instead stuff it with every\nattribute under the sun, not necessarily limited to attributes in\nitem->attrs[].\n\nBy the way, do not call an array as plural. item->attr[i] is a good\nname to call a single ith element in an array. item->attrs[i] isn't.\n\n> +\tfor (i = 0; i < item->attr_nr; i++) {\n> +\t\tint matched;\n> +\t\tconst char *value = check->check[i].value;\n\ncheck[i] has no relevance to item->attrs[i] here. I do not think\nthe code after this point is computing anything sensible.\n\n> diff --git a/pathspec.h b/pathspec.h\n> index 0c11262..89d73db 100644\n> --- a/pathspec.h\n> +++ b/pathspec.h\n> @@ -32,6 +32,21 @@ struct pathspec {\n> \t\tint len, prefix;\n> \t\tint nowildcard_len;\n> \t\tint flags;\n> +\t\tint attr_nr;\n> +\t\tint attr_alloc;\n> +\t\tstruct attr_item {\n> +\t\t\tchar *attr;\n> +\t\t\tchar *value;\n> +\t\t\tenum attr_match_mode {\n> +\t\t\t\tNOT_INIT,\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\tINVALID_ATTR\n> +\t\t\t} mode;\n> +\t\t} *attrs;\n\nI'd think the above addition that is in line with the updated API\nwould look more like this [*1*]:\n\n\tint attr_match_nr;\n int attr_match_alloc;\n struct attr_match {\n \tstruct git_attr *attr;\n char *value;\n enum attr_match_mode {\n \t...\n\t\t} match_mode;\n\t} *attr_match;\n\tstruct git_attr_check *attr_check;\n\nThen while parsing \":(attr:VAR1=VAL1 -VAR2 VAR3...)path/to/dir/\",\nyou would first do:\n\n\tp->attr_check = git_attr_check_alloc();\n\nonce, and then send each of VAR1=VAL2, -VAR2, VAR3... to your\nparse_one_item() helper function which would:\n\n * parse the match-mode like your code does;\n\n * parse out the attribute name (i.e. VAR1, VAR2 and VAR3), and\n instead of keeping it as a \"(const) char *\", call git_attr()\n to intern it (and keep it in local variable \"attr\"), and save\n it in p->attr_match[p->attr_nr].attr;\n\n * call git_attr_check_append(p->attr_check, git_attr_name(attr))\n\nAfter the above finishes, you would end up with something like:\n\n .attr_match = {\n { .attr = git_attr(\"VAR1\"), .value = \"VAL2\",\n .match_mode = MATCH_VALUE },\n { .attr = git_attr(\"VAR2\"), .value = ,\n .match_mode = MATCH_UNSET },\n ...\n\t},\n .attr_check = {\n\t .check = {\n \t{ .attr = git_attr(\"VAR1\"), .value = },\n \t{ .attr = git_attr(\"VAR2\"), .value = },\n \t{ .attr = git_attr(\"VAR3\"), .value = },\n }\n\t \nWhen matching (i.e. the match_attrs() function), you would instead\ndo\n\n\tpath = xmemdupz(name, namelen);\n\tgit_check_attr(path, item->attr_check);\n\nto grab values for only attributes that matter to you, instead of\ncalling git_all_attrs() [*2*].\n\nAfter git_check_attr() returns, item->attr_check.check[0].attr would\nbe git_attr(\"VAR1\") and item->attr_check.check[0].value would be\nwhatever setting the path has for the VAR1 attribute. You can use\nyour match_mode logic to compare it with the values .attr_match\nexpects.\n\nYou do not necessarily have to have the same number of elements in\n.attr_match and .attr_check.check by the way. .attr_match might say\n\n\tVAR1=VAL2 !VAR1 -VAR1\n\nwhich may be always false if these are ANDed together, but in order\nto evaluate it, you need only one git_attr_check_elem for VAR1.\n\n\n[Footnote]\n\n*1* With the old API, things would not be that much different.\n Instead of single structure .attr_check, you would make an\n array of git_attr_check structure, exactly like the array\n at .attr_check.check[] in the new API by hand. The new API\n makes this preparation simpler by managing the array on the API\n implementation side.\n\n*2* Please do not use that silly function especially in performance\n sensitive codepath.\n"},{"id":"286796","messageId":"CAGZ79kZSZQwnatExNFKCiWz8zgJfnS+8VWLkZcMaiHS1ESqBaQ@mail.gmail.com","threadId":"42352","inReplyTo":"xmqqa8jpz51m.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T16:45:01Z","receivedAt":"2016-05-17T16:45:01Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 16, 2016 at 9:23 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> + * attr:+val to find value set to true\n>> + * attr:-val to find a value set to false\n>> + * attr:!val to find a value that is not set\n>> + * (i.e. it is neither set as \"val\", \"val=\", nor unset as \"-val\")\n>> + * attr:val=value: to find value that have at least a and b set.\n>\n> I would have expected that there won't be \"attr:+val\", but it is\n> spelled as \"attr:val\" instead.\n\n\"val\" matches if the attr is not unspecified, i.e. one of {true, false, value}\n\"+val\" matches {true} only.\n\nMaybe we want to redo that to\n\n\"val\" matches {true} only.\n\"?val\" matches {true, false, value}. (I can leave this case out in the\nfirst series, too)\n\n>\n>> +static void parse_attr_item(struct attr_item *attr, const char *value)\n>\n> Please do not call something that is not part of the attribute\n> infrastructure as \"attr_item\"; I wasted time looking for the\n> structure definition for \"attr_item\" in .\n\nSo \"parse_pathspec_attr_match\" instead?\n\n>\n>> +static int match_attrs(const char *name, int namelen,\n>> + const struct pathspec_item *item)\n>> +{\n>> + char *path;\n>> + int i;\n>> +\n>> + if (!check) {\n>> + check = git_attr_check_alloc();\n>> + for (i = 0; i < item->attr_nr; i++)\n>> + git_attr_check_append(check, item->attrs[i].attr);\n>> + }\n>> +\n>> + path = xmemdupz(name, namelen);\n>> + git_all_attrs(path, check);\n>\n> PLEASE DON'T. git_all_attrs() asks for all the attribute under the\n> sun and has no hope to perform sensibly, especially at the very leaf\n> level of the pathspec logic where one call to this function is made\n> for each and every path in the tree.\n\nThis is executed only once, as check is static? From a users perception\nit doesn't matter if it is executed once just after parsing all pathspec\nitems or at the first path to check for a match, no?\n\nThe mistake is using the API wrong. So inside the '!check', after the\npreparation loop of git_attr_check_append, we'd need to\nhand over the \"check\" to git_check_attr instead?\n\n>\n> Instead, have a pointer to \"struct git_attr_check\" in pathspec_item\n> and make a call to git_check_attr(path, item->check) here.\n\nI see, then we have multiple `check` structs. Makes sense.\n\n>\n> Which means that you would need to prepare git_attr_check around ...\n>\n>> + if (skip_prefix(copyfrom, \"attr:\", &body)) {\n>> + ALLOC_GROW(item->attrs, item->attr_nr + 1, item->attr_alloc);\n>> + parse_attr_item(&item->attrs[item->attr_nr++], body);\n>\n> ... HERE.\n>\n"},{"id":"286799","messageId":"CAGZ79kbYB_4KO+XpYa0OhAcU63Q2M2kLWa03HcxeYS1HJOgfZw@mail.gmail.com","threadId":"42352","inReplyTo":"xmqqvb2dxomo.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T17:03:38Z","receivedAt":"2016-05-17T17:03:38Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 16, 2016 at 10:03 PM, Junio C Hamano wrote:\n>\n> int attr_match_nr;\n> int attr_match_alloc;\n> struct attr_match {\n> struct git_attr *attr;\n> char *value;\n> enum attr_match_mode {\n> ...\n> } match_mode;\n> } *attr_match;\n> struct git_attr_check *attr_check;\n\nok, I'll use that structure and go from here.\n\n\n>\n> Then while parsing \":(attr:VAR1=VAL1 -VAR2 VAR3...)path/to/dir/\",\n\nThis syntax is not pleasant to parse IMHO as it is not clear if the token\nafter white space (-VAR2 here) is another attribute or the next part of\nthe list of VAR1, i.e. this could also be:\n\n :(attr:VAR1=VAL1 VAL2)\n\nI wonder if we'd want to use the colon to separate different VARs:\n\n :(attr:VAR1=VAL1 VAL2:-VAR2:VAR3)\n\nwhich means:\n\n match all path, that\n * have VAR1 set to a value list containing at least\n VAL1 and VAL2\n * have VAR2 unset\n * have VAR3 set to true.\n"},{"id":"286804","messageId":"xmqq37pgy4fn.fsf@gitster.mtv.corp.google.com","threadId":"42352","inReplyTo":"CAGZ79kbYB_4KO+XpYa0OhAcU63Q2M2kLWa03HcxeYS1HJOgfZw@mail.gmail.com","subject":"Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-17T17:34:04Z","receivedAt":"2016-05-17T17:34:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> Then while parsing \":(attr:VAR1=VAL1 -VAR2 VAR3...)path/to/dir/\",\n>\n> This syntax is not pleasant to parse IMHO as it is not clear if the token\n> after white space (-VAR2 here) is another attribute or the next part of\n> the list of VAR1, ...\n\nRemove the ambiguity by declaring that the list is always whitespace\nseparated. No whitespace in var, no whitespace in val, no quoting.\n\nThe set of attributes with values expected to be used in the\npathspec \"attribute match\" magic, I do not think there is anything\nthat wants such a random arbitrary string. The value side of an\nattribute with value, e.g. \"eol=crlf\", \"conflict-marker-size=7\", is\ndesigned to be a token that our C code is prepared to parse.\n\nIn other words, if you match the parsing semantics of parse_attr()\nin attr.c, you are OK. The attribute subsystem will not give users\nanything that is more complex than what that routine is prepared to\nparse, and that is a \"whitespace separated list, no whitespace in\nattribute names, no whitespace in values, no quoting\".\n"},{"id":"286806","messageId":"CAGZ79kYbUTC7m-5kdTbvxmSkq__5BVz5x1UeieHhB4TVSqssHw@mail.gmail.com","threadId":"42352","inReplyTo":"xmqq37pgy4fn.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T17:45:31Z","receivedAt":"2016-05-17T17:45:31Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 17, 2016 at 10:34 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>>> Then while parsing \":(attr:VAR1=VAL1 -VAR2 VAR3...)path/to/dir/\",\n>>\n>> This syntax is not pleasant to parse IMHO as it is not clear if the token\n>> after white space (-VAR2 here) is another attribute or the next part of\n>> the list of VAR1, ...\n>\n> Remove the ambiguity by declaring that the list is always whitespace\n> separated. No whitespace in var, no whitespace in val, no quoting.\n>\n> The set of attributes with values expected to be used in the\n> pathspec \"attribute match\" magic, I do not think there is anything\n> that wants such a random arbitrary string. The value side of an\n> attribute with value, e.g. \"eol=crlf\", \"conflict-marker-size=7\", is\n> designed to be a token that our C code is prepared to parse.\n\nI am not talking about crazy stuff here, but consider our own\n.gitattributes file:\n\n * whitespace=!indent,trail,space\n *.[ch] whitespace=indent,trail,space\n *.sh whitespace=indent,trail,space\n\nNow I want to search for\n\n \"the whitespace attribute that is set to at least trail and space\"\n\nWe cannot use commas for the specification as they are used in .gitattributes,\nbecause that would make it even harder, so\nI would imagine this could be used:\n\n :(attr:whitespace=space trail)\n\nSee the whitespace separates the values, not the next variable.\n\nTo add another variable, I would suggest using a ':', such as\n\n :(attr:whitespace=space trail:text)\n\nmight a viable thing.\n\n\n\n>\n> In other words, if you match the parsing semantics of parse_attr()\n> in attr.c, you are OK. The attribute subsystem will not give users\n> anything that is more complex than what that routine is prepared to\n> parse, and that is a \"whitespace separated list, no whitespace in\n> attribute names, no whitespace in values, no quoting\".\n>\n"},{"id":"286809","messageId":"xmqqposkwofd.fsf@gitster.mtv.corp.google.com","threadId":"42352","inReplyTo":"CAGZ79kYbUTC7m-5kdTbvxmSkq__5BVz5x1UeieHhB4TVSqssHw@mail.gmail.com","subject":"Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-17T18:05:10Z","receivedAt":"2016-05-17T18:05:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I am not talking about crazy stuff here, but consider our own\n> .gitattributes file:\n>\n> * whitespace=!indent,trail,space\n> *.[ch] whitespace=indent,trail,space\n> *.sh whitespace=indent,trail,space\n>\n> Now I want to search for\n>\n> \"the whitespace attribute that is set to at least trail and space\"\n\nWith :(attr:VAR=VAL) syntax, you can only look for whitespace\nattribute exactly set to \"!indent,trail,space\", so you can't. So\nyou are talking about crazy stuff after all, aren't you? Are you\nnow extending it to do \"whitespace~=indent\" or something like that?\n\nYou do not even need VAR=VAL form for your main topic. You only\nneed group-doc group-code etc. to look for \"set to TRUE\", no?\n\nI do not want to see us wasting too much time over-engineering it\nwithout having a concrete and useful use case, and \"find path to\nwhom whitespace checks are set for 'trail' and 'space'\" is not.\nThese comma-separated tokens augment WS_DEFAULT_RULE which has\n'trail' already, so you do not look for 'trail' in the first place.\n\n\"I want submodules under subs/ that is in either group-doc or\ngroup-code\" is more reasonable and realistic use case, but that does\nnot need any crazy stuff. Either two separate pathspec elements,\n\n\t\":(attr:group-doc)subs/\" \":(attr:group-code)/subs/\"\n\nor if we are to do the \"ORed collection of ANDed attrs\", it would be\nsomething like:\n\n\t\":(attr:group-doc):(attr:group-code)/subs/\"\n\nno?\n"},{"id":"286811","messageId":"CAGZ79kasg6zQjNLbESrP0vG5OpZ=yqMJhXqGUDtuEFdDS6=W-w@mail.gmail.com","threadId":"42352","inReplyTo":"xmqqposkwofd.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T18:10:24Z","receivedAt":"2016-05-17T18:10:24Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 17, 2016 at 11:05 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> I am not talking about crazy stuff here, but consider our own\n>> .gitattributes file:\n>>\n>> * whitespace=!indent,trail,space\n>> *.[ch] whitespace=indent,trail,space\n>> *.sh whitespace=indent,trail,space\n>>\n>> Now I want to search for\n>>\n>> \"the whitespace attribute that is set to at least trail and space\"\n>\n> With :(attr:VAR=VAL) syntax, you can only look for whitespace\n> attribute exactly set to \"!indent,trail,space\", so you can't. So\n> you are talking about crazy stuff after all, aren't you?\n\nI think that stuck mentally with me from the \"label\" series,\nas then you had to have an exact specification of the value list.\n\nWe don't do that any more, so in a very first series I can omit the\nvalue parsing at all.\n\n> Are you\n> now extending it to do \"whitespace~=indent\" or something like that?\n>\n> You do not even need VAR=VAL form for your main topic. You only\n> need group-doc group-code etc. to look for \"set to TRUE\", no?\n>\n> I do not want to see us wasting too much time over-engineering it\n> without having a concrete and useful use case, and \"find path to\n> whom whitespace checks are set for 'trail' and 'space'\" is not.\n> These comma-separated tokens augment WS_DEFAULT_RULE which has\n> 'trail' already, so you do not look for 'trail' in the first place.\n>\n> \"I want submodules under subs/ that is in either group-doc or\n> group-code\" is more reasonable and realistic use case, but that does\n> not need any crazy stuff. Either two separate pathspec elements,\n>\n> \":(attr:group-doc)subs/\" \":(attr:group-code)/subs/\"\n>\n> or if we are to do the \"ORed collection of ANDed attrs\", it would be\n> something like:\n>\n> \":(attr:group-doc):(attr:group-code)/subs/\"\n>\n> no?\n\nThanks for bringing me back to realities. I'll just drop support for values\nin the first series.\n"},{"id":"286815","messageId":"xmqqeg90wnap.fsf@gitster.mtv.corp.google.com","threadId":"42352","inReplyTo":"CAGZ79kasg6zQjNLbESrP0vG5OpZ=yqMJhXqGUDtuEFdDS6=W-w@mail.gmail.com","subject":"Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-17T18:29:34Z","receivedAt":"2016-05-17T18:29:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I'll just drop support for values\n> in the first series.\n\nI do not think an exact string match to support :(attr:eol=crlf) is\nso bad. The \"crazy stuff\" aka over-engineering is when it goes\nbeyond that, e.g. 'eol is set to one of these values\"\n"},{"id":"286820","messageId":"CAGZ79kZ-xf167LiO+zY+B8BTYd-9h7u=dgpL=4tsEJDPgwq8CA@mail.gmail.com","threadId":"42352","inReplyTo":"xmqqvb2dxomo.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-17T19:23:33Z","receivedAt":"2016-05-17T19:23:33Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 16, 2016 at 10:03 PM, Junio C Hamano wrote:\n> When matching (i.e. the match_attrs() function), you would instead\n> do\n>\n> path = xmemdupz(name, namelen);\n> git_check_attr(path, item->attr_check);\n>\n> to grab values for only attributes that matter to you, instead of\n> calling git_all_attrs() [*2*].\n>\n> After git_check_attr() returns, item->attr_check.check[0].attr would\n> be git_attr(\"VAR1\") and item->attr_check.check[0].value would be\n> whatever setting the path has for the VAR1 attribute. You can use\n> your match_mode logic to compare it with the values .attr_match\n> expects.\n>\n> You do not necessarily have to have the same number of elements in\n> .attr_match and .attr_check.check by the way. .attr_match might say\n>\n> VAR1=VAL2 !VAR1 -VAR1\n>\n> which may be always false if these are ANDed together, but in order\n> to evaluate it, you need only one git_attr_check_elem for VAR1.\n\nSo for the matching we would need to get the order right, i.e.\n\n const char *inspect_name = git_attr_name(item.attr_match[i].attr);\n for (j=0; j < p.attr_check.check_nr; j++) {\n const char *cur_name = git_attr_name(p.attr_check.check[j].attr);\n if (!strcmp(inspect_name, cur_name))\n break;\n // now compare .attr_match[i] with attr_check.check[j]\n\nThis doesn't look cheap to me? Am I holding it wrong again?\n"},{"id":"286832","messageId":"xmqqposkv3c9.fsf@gitster.mtv.corp.google.com","threadId":"42352","inReplyTo":"CAGZ79kZ-xf167LiO+zY+B8BTYd-9h7u=dgpL=4tsEJDPgwq8CA@mail.gmail.com","subject":"Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-17T20:25:58Z","receivedAt":"2016-05-17T20:25:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Mon, May 16, 2016 at 10:03 PM, Junio C Hamano wrote:\n>> When matching (i.e. the match_attrs() function), you would instead\n>> do\n>>\n>> path = xmemdupz(name, namelen);\n>> git_check_attr(path, item->attr_check);\n>>\n>> to grab values for only attributes that matter to you, instead of\n>> calling git_all_attrs() [*2*].\n>>\n>> After git_check_attr() returns, item->attr_check.check[0].attr would\n>> be git_attr(\"VAR1\") and item->attr_check.check[0].value would be\n>> whatever setting the path has for the VAR1 attribute. You can use\n>> your match_mode logic to compare it with the values .attr_match\n>> expects.\n>>\n>> You do not necessarily have to have the same number of elements in\n>> .attr_match and .attr_check.check by the way. .attr_match might say\n>>\n>> VAR1=VAL2 !VAR1 -VAR1\n>>\n>> which may be always false if these are ANDed together, but in order\n>> to evaluate it, you need only one git_attr_check_elem for VAR1.\n>\n\nThe key phrase in the message you are reacting to is \"not\nnecessarily\". It is not a crime to ask for the same attribute twice\nin a git_attr_check structure.\n\n $ git check-attr text text text -- path\n\nwould stuff three instances of \"text\" in there and ask them for\n\"path\". The simple in-code callers that uses git_attr_check_initl()\ndo rely on the order of the attributes it placed in attr_check\nstructure (see e.g. how ll_merge() uses check[0].value and\ncheck[1].value to see the driver name and marker size), and that is\nperfectly kosher. Existing code is your friend.\n\nThe mention of the possibility is purely as a hint useful for a\npossible enhancement in the far future. If we ever want to support\nsomething like this:\n\n\t\":(attr-expression (VAR1=VAL1 | VAR1=VAL2) & VAR2)\"\n\nyou can remember that you can put VAR1 and VAR2 in attr_check to\ngrab values for VAR1 and VAR2 (even though VAR1 is mentioned twice\nin the expression), and use them in the evaluation you will perform.\n\n> So for the matching we would need to get the order right, i.e.\n>\n> const char *inspect_name = git_attr_name(item.attr_match[i].attr);\n> for (j=0; j < p.attr_check.check_nr; j++) {\n> const char *cur_name = git_attr_name(p.attr_check.check[j].attr);\n> if (!strcmp(inspect_name, cur_name))\n> break;\n\nYou do not strcmp() when you have attributes. They are interned so\nthat you can compare their addresses. That makes it somewhat\ncheaper.\n\nOnce you start \"expression over random attributes\", you'd need to\nmap attr => value somehow. The format attr_check structure gives\nyou, i.e. a list of , is aimed at compactness than\nrandom hashmap-like access. If the caller wants a hashmap-like\naccess for performance purposes, the caller does that itself.\n\nExisting users do not need a hashmap-like access, because they know\nat which index in attr_check they placed request for what attribute.\nAn array that can be indexed with a small integer is exactly what\nthey want.\n\n> This doesn't look cheap to me? Am I holding it wrong again?\n\nBy the way, I do not think during the entire discussion on this\ntopic, you have never been in a situation to deserve the \"holding it\nwrong\" label (which implies \"a ware is broken, but somehow the end\nuser is blamed for using it incorrectly\"). When you were wrong, you\nwere simply wrong.\n"},{"id":"286888","messageId":"xmqqa8jntlyd.fsf@gitster.mtv.corp.google.com","threadId":"42352","inReplyTo":"xmqqposkv3c9.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-18T15:39:06Z","receivedAt":"2016-05-18T15:39:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The mention of the possibility is purely as a hint useful for a\n> possible enhancement in the far future. If we ever want to support\n> something like this:\n>\n> \t\":(attr-expression (VAR1=VAL1 | VAR1=VAL2) & VAR2)\"\n>\n> you can remember that you can put VAR1 and VAR2 in attr_check to\n> grab values for VAR1 and VAR2 (even though VAR1 is mentioned twice\n> in the expression), and use them in the evaluation you will perform.\n>\n>> So for the matching we would need to get the order right, i.e.\n>>\n>> const char *inspect_name = git_attr_name(item.attr_match[i].attr);\n>> for (j=0; j < p.attr_check.check_nr; j++) {\n>> const char *cur_name = git_attr_name(p.attr_check.check[j].attr);\n>> if (!strcmp(inspect_name, cur_name))\n>> break;\n>\n> You do not strcmp() when you have attributes. They are interned so\n> that you can compare their addresses. That makes it somewhat\n> cheaper.\n>\n> Once you start \"expression over random attributes\", you'd need to\n> map attr => value somehow. The format attr_check structure gives\n> you, i.e. a list of , is aimed at compactness than\n> random hashmap-like access. If the caller wants a hashmap-like\n> access for performance purposes, the caller does that itself.\n\nTo expand this a bit, I actually do not think hashmap-like access is\nnecessary even in such an application.\n\nAn implementation of the evaluator, at least a production-quality\none, for the attr expression example shown above is unlikely to keep\nthe expression in a single string \"(VAR1=VAL1 | VAR1=VAL2) & VAR2\".\nInstead it would use a parse tree with nodes that represent\noperators (e.g. OR, AND, \"=\", etc.) and terms (e.g. attribute\nreference VAR1, VAR2, and constatnts \"VAL1\", \"VAL2\") as its internal\nrepresentation.\n\nAnd a node in such a parse tree that refers to an attribute VAR1\nwould unlikely to keep a \"const char *\" that is \"VAR1\". The node\nwould have a field that stores an index into the attr_check.check[]\narray when the textual expression is \"compiled\" into a parse tree\nand the set of attributes the expression uses (hence it needs to\nask the attributes API about, to prepare attr_check array) is\nidentified.\n\nWhen \"evaluating\" the parse-tree, a node that refers to an attribute\nhas an index into attr_check.check[] array, so there is no need for\na loop like the one shown above at all.\n\nWhen \"showing\" the expression (for debugging purposes), it would\ngrab the \"index into the attr_check.check[] array\" from the node,\nand the element in that array is an pair, so it can\nuse git_attr_name() to obtain the attribute name.\n"}]}