{"thread":{"id":"42317","subject":"[PATCH 2/5] Documentation: correct typo in example for querying attributes","startedAt":"2016-05-13T23:13:21Z","lastAt":"2016-05-16T19:27:33Z","messageCount":17,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"286516","messageId":"20160513231326.8994-1-sbeller@google.com","threadId":"42317","inReplyTo":null,"subject":"[PATCHv2 0/5] pathspec labels [WAS: submodule groups]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-13T23:13:21Z","receivedAt":"2016-05-13T23:13:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I am happy with the results of the label experiments. (Mainly writing the tests)\n\n>> Isn't the former be \"label=\"\n\n> I do not know what you mean by the latter. I would understand\n>\"pretend this has all the labels under the sun\", though.\n\nSee the new tests what I mean. It cleared up for me as well when writing them.\n\n> I am wondering where we should sanity check (and die, pointing out\n> an error in .gitattributes file). Is this function a good place to\n> reject TRUE and FALSE?\n\nTRUE and FALSE are solved issues in this series. However we still want\nfunny characters not be used. What if we want to use them as \nspecial characters in the future? (e.g. 'label*' for 'labelA' or 'labelB'\nor label{A,B} for the same)\n\nI think it is too late in the matching, so ideally we would check label\nnames when they are assigned (i.e. while editing the .gitattributes file)\nThat is not in our control, though.\n\nAnd during use of match_pathspec it is already to late to die(...)\nfor bad labels, because the labels may not be in control for the user\n(She just cloned the repository and wants to inspect certain files,\nwhich are labeled \"$*&^\".) So for now we just output a warning for\nthese cases, in the hope that content creatos upstream respect the\nrecommendation for labels.\n\nFeedback welcome!\n\nThanks,\nStefan\n\nStefan Beller (5):\n Documentation: fix a typo\n Documentation: correct typo in example for querying attributes\n pathspec: move long magic parsing out of prefix_pathspec\n pathspec: move prefix check out of the inner loop\n pathspec: record labels\n\n Documentation/gitattributes.txt | 14 ++-\n Documentation/glossary-content.txt | 4 +\n Documentation/technical/api-gitattributes.txt | 2 +-\n dir.c | 50 ++++++++\n pathspec.c | 110 +++++++++++------\n pathspec.h | 1 +\n t/t6134-pathspec-with-labels.sh | 164 ++++++++++++++++++++++++++\n 7 files changed, 305 insertions(+), 40 deletions(-)\n create mode 100755 t/t6134-pathspec-with-labels.sh\n\n-- \n2.8.2.401.gb7348ac.dirty\n"},{"id":"286515","messageId":"20160513231326.8994-2-sbeller@google.com","threadId":"42317","inReplyTo":"20160513231326.8994-1-sbeller@google.com","subject":"[PATCH 1/5] Documentation: fix a typo","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-13T23:13:22Z","receivedAt":"2016-05-13T23:13:22Z","isPatch":true,"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.g6647087.dirty\n"},{"id":"286514","messageId":"20160513231326.8994-3-sbeller@google.com","threadId":"42317","inReplyTo":"20160513231326.8994-1-sbeller@google.com","subject":"[PATCH 2/5] Documentation: correct typo in example for querying attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-13T23:13:23Z","receivedAt":"2016-05-13T23:13:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The introductory text of the example has a typo in the\nspecification which makes it harder to follow that example.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/technical/api-gitattributes.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt\nindex 2602668..36fc9af 100644\n--- a/Documentation/technical/api-gitattributes.txt\n+++ b/Documentation/technical/api-gitattributes.txt\n@@ -61,7 +61,7 @@ Querying Specific Attributes\n Example\n -------\n \n-To see how attributes \"crlf\" and \"indent\" are set for different paths.\n+To see how attributes \"crlf\" and \"ident\" are set for different paths.\n \n . Prepare an array of `struct git_attr_check` with two elements (because\n we are checking two attributes). Initialize their `attr` member with\n-- \n2.8.2.401.g6647087.dirty\n"},{"id":"286517","messageId":"20160513231326.8994-4-sbeller@google.com","threadId":"42317","inReplyTo":"20160513231326.8994-1-sbeller@google.com","subject":"[PATCH 3/5] pathspec: move long magic parsing out of prefix_pathspec","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-13T23:13:24Z","receivedAt":"2016-05-13T23:13:24Z","isPatch":true,"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.g6647087.dirty\n"},{"id":"286518","messageId":"20160513231326.8994-5-sbeller@google.com","threadId":"42317","inReplyTo":"20160513231326.8994-1-sbeller@google.com","subject":"[PATCH 4/5] pathspec: move prefix check out of the inner loop","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-13T23:13:25Z","receivedAt":"2016-05-13T23:13:25Z","isPatch":true,"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.g6647087.dirty\n"},{"id":"286519","messageId":"20160513231326.8994-6-sbeller@google.com","threadId":"42317","inReplyTo":"20160513231326.8994-1-sbeller@google.com","subject":"[PATCH 5/5] pathspec: record labels","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-13T23:13:26Z","receivedAt":"2016-05-13T23:13:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Labels were originally designed to manage large amount of\nsubmodules, the discussion steered this in a more general\ndirection, such that other files can be labeled as well.\n\nLabels are meant to describe arbitrary set of files, which\nis not described via the tree layout.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/gitattributes.txt | 12 +++\n Documentation/glossary-content.txt | 4 +\n dir.c | 50 +++++++++++\n pathspec.c | 64 ++++++++++++++-\n pathspec.h | 1 +\n t/t6134-pathspec-with-labels.sh | 164 +++++++++++++++++++++++++++++++++++++\n 6 files changed, 291 insertions(+), 4 deletions(-)\n create mode 100755 t/t6134-pathspec-with-labels.sh\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex af2c682..7fd764f 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -906,6 +906,18 @@ If this attribute is not set or has an invalid value, the value of the\n (See linkgit:git-config[1]).\n \n \n+\n+Attaching labels to path\n+~~~~~~~~~~~~~~~~~~~~~~~~\n+\n+`label`\n+^^^^^^^^^^\n+By the value of this attribute you can specify a list of arbitrary strings\n+to be attached to a path as labels. These labels can be used for\n+easier paths matching using pathspecs (linkgit:gitglossary[1]).\n+It is recommended to use only alphanumeric characters, dashes and\n+underscores for the labels.\n+\n USING MACRO ATTRIBUTES\n ----------------------\n \ndiff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\nindex 8ad29e6..f990017 100644\n--- a/Documentation/glossary-content.txt\n+++ b/Documentation/glossary-content.txt\n@@ -384,6 +384,10 @@ full pathname may have special meaning:\n +\n Glob magic is incompatible with literal magic.\n \n+label:;;\n+\tAdditionally to matching the pathspec, the path must habe a 'label'\n+\tattribute having set all labels listed here.\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 656f272..a86e3ff 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@@ -208,6 +209,52 @@ int within_depth(const char *name, int namelen,\n \treturn 1;\n }\n \n+static int has_all_labels(const char *name, int namelen,\n+\t\t\t const struct string_list *required_labels)\n+{\n+\tstatic struct git_attr *attr;\n+\n+\tstruct git_attr_check check;\n+\tchar *path;\n+\tint ret;\n+\n+\tif (!attr)\n+\t\tattr = git_attr(\"label\");\n+\tcheck.attr = attr;\n+\n+\tpath = xmemdupz(name, namelen);\n+\tif (git_check_attr(path, 1, &check))\n+\t\tdie(\"internal bug: git_check_attr died.\");\n+\tfree(path);\n+\n+\tif (ATTR_TRUE(check.value))\n+\t\tret = 1; /* has all the labels */\n+\telse if (ATTR_FALSE(check.value))\n+\t\tret = 0; /* has no labels */\n+\telse if (ATTR_UNSET(check.value))\n+\t\t/*\n+\t\t * If no label was specified this matches, otherwise\n+\t\t * there is a missing label.\n+\t\t */\n+\t\tret = (required_labels->nr > 0) ? 0 : 1;\n+\telse {\n+\t\tstruct string_list_item *si;\n+\t\tstruct string_list attr_labels = STRING_LIST_INIT_DUP;\n+\t\tstring_list_split(&attr_labels, check.value, ',', -1);\n+\t\tstring_list_sort(&attr_labels);\n+\t\tret = 1;\n+\t\tfor_each_string_list_item(si, required_labels) {\n+\t\t\tif (!string_list_has_string(&attr_labels, si->string)) {\n+\t\t\t\tret = 0;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tstring_list_clear(&attr_labels, 0);\n+\t}\n+\n+\treturn ret;\n+}\n+\n #define DO_MATCH_EXCLUDE 1\n #define DO_MATCH_DIRECTORY 2\n \n@@ -263,6 +310,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->labels && !has_all_labels(name, namelen, item->labels))\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..c65428a 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -88,12 +88,40 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+static int check_valid_label_name(const char *label)\n+{\n+\tif (!label || !strlen(label))\n+\t\treturn -1;\n+\n+\tif (!isalpha(*label))\n+\t\treturn -1;\n+\n+\twhile (*label) {\n+\t\tif (!(isalnum(*label) ||\n+\t\t\t*label == '-' ||\n+\t\t\t*label == '_'))\n+\t\t\treturn -1;\n+\t\tlabel++;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static void validate_label_name(const char *label)\n+{\n+\tif (check_valid_label_name(label))\n+\t\twarning(_(\"Label '%s' discouraged. Recommended label names start \"\n+\t\t\t\"with an alphabetic character and use only alphanumeric \"\n+\t\t\t\"characters, dashes and underscores.\"), label);\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 +136,30 @@ 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\t/* The label token may have no argument, so no trailing ':' */\n+\t\tif (skip_prefix(copyfrom, \"label\", &body)) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tskip_prefix(body, \":\", &body);\n+\t\t\tstrbuf_add(&sb, body, nextat - body);\n+\t\t\tif (!item->labels) {\n+\t\t\t\titem->labels = xmalloc(sizeof(*item->labels));\n+\t\t\t\tstring_list_init(item->labels, 1);\n+\t\t\t} else\n+\t\t\t\tdie(_(\"multiple labels not supported in pathspec magic\"));\n+\t\t\tstring_list_split(item->labels, sb.buf, ' ', -1);\n+\t\t\tstring_list_remove_empty_items(item->labels, 0);\n+\t\t\tstrbuf_release(&sb);\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 +468,7 @@ 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].labels = NULL;\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 +490,12 @@ 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].labels) {\n+\t\t\tstruct string_list_item *si;\n+\t\t\tfor_each_string_list_item(si, item[i].labels)\n+\t\t\t\tvalidate_label_name(si->string);\n+\t\t}\n \t}\n \n \tif (nr_exclude == n)\n@@ -502,6 +551,13 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n \n void free_pathspec(struct pathspec *pathspec)\n {\n+\tint i;\n+\tfor (i = 0; i < pathspec->nr; i++) {\n+\t\tif (pathspec->items[i].labels)\n+\t\t\tstring_list_clear(pathspec->items[i].labels, 1);\n+\t\tfree(pathspec->items[i].labels);\n+\t}\n+\n \tfree(pathspec->items);\n \tpathspec->items = NULL;\n }\ndiff --git a/pathspec.h b/pathspec.h\nindex 0c11262..12ed2b8 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -32,6 +32,7 @@ struct pathspec {\n \t\tint len, prefix;\n \t\tint nowildcard_len;\n \t\tint flags;\n+\t\tstruct string_list *labels;\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..1fe5ed7\n--- /dev/null\n+++ b/t/t6134-pathspec-with-labels.sh\n@@ -0,0 +1,164 @@\n+#!/bin/sh\n+\n+test_description='test labels in pathspecs'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a tree' '\n+\tmkdir sub &&\n+\tfor p in fileA fileB fileC fileAB fileAC fileBC fileNoLabel fileUnsetLabel fileSetLabel; do\n+\t\t: >$p &&\n+\t\tgit add $p &&\n+\t\t: >sub/$p\n+\t\tgit add sub/$p\n+\tdone &&\n+\tgit commit -m $p &&\n+\tgit ls-files >actual &&\n+\tcat <expect &&\n+fileA\n+fileAB\n+fileAC\n+fileB\n+fileBC\n+fileC\n+fileNoLabel\n+fileSetLabel\n+fileUnsetLabel\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+EOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pathspec with labels and non existent .gitattributes' '\n+\tgit ls-files \":(label)\" >actual &&\n+\tgit ls-files >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pathspec with labels and non existent .gitattributes' '\n+\tgit ls-files \":(label:a)\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'setup .gitattributes' '\n+\tcat <.gitattributes &&\n+fileA label=labelA\n+fileB label=labelB\n+fileC label=labelC\n+fileAB label=labelA,labelB\n+fileAC label=labelA,labelC\n+fileBC label=labelB,labelC\n+fileUnsetLabel -label\n+fileSetLabel label\n+EOF\n+\tgit add .gitattributes &&\n+\tgit commit -m \"add attributes\"\n+'\n+\n+test_expect_success 'check for any label' '\n+\tcat <expect &&\n+.gitattributes\n+fileA\n+fileAB\n+fileAC\n+fileB\n+fileBC\n+fileC\n+fileNoLabel\n+fileSetLabel\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+EOF\n+\tgit ls-files \":(label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check specific label' '\n+\tcat <expect &&\n+fileA\n+fileAB\n+fileAC\n+fileSetLabel\n+sub/fileA\n+sub/fileAB\n+sub/fileAC\n+sub/fileSetLabel\n+EOF\n+\tgit ls-files \":(label:labelA)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check label with 2 labels' '\n+\tcat <expect &&\n+fileAB\n+fileSetLabel\n+sub/fileAB\n+sub/fileSetLabel\n+EOF\n+\tgit ls-files \":(label:labelA labelB)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check label with more labels' '\n+\ttest_must_fail git ls-files \":(label:labelA,label:labelB)\" 2>actual &&\n+\ttest_i18ngrep \"not supported\" actual\n+'\n+\n+test_expect_success 'check label with more labels but excluded path' '\n+\tcat <expect &&\n+fileAB\n+fileB\n+fileBC\n+fileSetLabel\n+EOF\n+\tgit ls-files \":(label:labelB)\" \":(exclude)sub/\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check label excluding other labels' '\n+\tcat <expect &&\n+fileAB\n+fileB\n+fileBC\n+fileSetLabel\n+sub/fileAB\n+sub/fileB\n+EOF\n+\tgit ls-files \":(label:labelB)\" \":(exclude,label:labelC)sub/\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n+test_expect_success 'check for explicit unlabeled paths' '\n+\tcat <expect &&\n+fileUnsetLabel\n+sub/fileUnsetLabel\n+EOF\n+\tgit ls-files . \":(exclude,label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check for paths that have no label' '\n+\tcat <expect &&\n+.gitattributes\n+fileNoLabel\n+sub/fileNoLabel\n+EOF\n+\tgit ls-files \":(label)\" \":(exclude,label:labelA)\" \":(exclude,label:labelB)\" \":(exclude,label:labelC)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.8.2.401.g6647087.dirty\n"},{"id":"286547","messageId":"xmqqk2iw78aq.fsf@gitster.mtv.corp.google.com","threadId":"42317","inReplyTo":"20160513231326.8994-6-sbeller@google.com","subject":"Re: [PATCH 5/5] pathspec: record labels","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-14T19:23:41Z","receivedAt":"2016-05-14T19:23:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Labels were originally designed to manage large amount of\n> submodules, the discussion steered this in a more general\n> direction, such that other files can be labeled as well.\n\ns/other files/any path/.\n\n> Labels are meant to describe arbitrary set of files, which\n> is not described via the tree layout.\n\nLet's avoid \"are meant to\", which is merely to give you an excuse to\nsay \"it was meant to ... but the implementation did not quite achieve\nthat goal\".\n\n The \"label\" attribute can be attached to paths, and the pathspec\n mechanism is extended via the new \":(label=X)pattern/to/match\"\n syntax to filter paths so that it requires paths to not just\n match the given pattern but also have the specified labels\n attached for them to be chosen.\n\nor something?\n\n> +Attaching labels to path\n> +~~~~~~~~~~~~~~~~~~~~~~~~\n> +\n> +`label`\n> +^^^^^^^^^^\n> +By the value of this attribute you can specify a list of arbitrary strings\n> +to be attached to a path as labels. These labels can be used for\n> +easier paths matching using pathspecs (linkgit:gitglossary[1]).\n> +It is recommended to use only alphanumeric characters, dashes and\n> +underscores for the labels.\n\nMake this recomendation stronger to requirement. It is always\npossible to loosen it later, but once we allow random things even\nwith a warning, it gets impossible to take them back. Users will say\n\"Even though we got a warning, it used to correctly filter; now Git\nis broken and my setup does not work.\"\n\n> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\n> index 8ad29e6..f990017 100644\n> --- a/Documentation/glossary-content.txt\n> +++ b/Documentation/glossary-content.txt\n> @@ -384,6 +384,10 @@ full pathname may have special meaning:\n> +\n> Glob magic is incompatible with literal magic.\n> \n> +label:;;\n> +\tAdditionally to matching the pathspec, the path must habe a 'label'\n> +\tattribute having set all labels listed here.\n\ns/habe/have/\n\n> +\tif (ATTR_TRUE(check.value))\n> +\t\tret = 1; /* has all the labels */\n\nSo this is \"pretend that I have all the labels under the sun\".\n\n> +\telse if (ATTR_FALSE(check.value))\n> +\t\tret = 0; /* has no labels */\n\nI do not see a value in this. What difference does it have compared\nto having a \"label=\" that explicitly says \"I do not have one\"? What\nis your answer to an end-user question \"When should I say 'label='\nand when should I say '!label'?\"\n\nJust forbid it for now; we may find a better use for it later.\n\n> +\telse if (ATTR_UNSET(check.value))\n> +\t\t/*\n> +\t\t * If no label was specified this matches, otherwise\n> +\t\t * there is a missing label.\n> +\t\t */\n> +\t\tret = (required_labels->nr > 0) ? 0 : 1;\n\nHmm, I can see that this is making things more complicated to\nexplain and understand, but I cannot see what the expected use case\nis.\n\nUnless there is any compelling use case, I'd say we should forbid it\nfor now.\n\n> +\telse {\n> +\t\tstruct string_list_item *si;\n> +\t\tstruct string_list attr_labels = STRING_LIST_INIT_DUP;\n> +\t\tstring_list_split(&attr_labels, check.value, ',', -1);\n> +\t\tstring_list_sort(&attr_labels);\n\nFilter out a non-compliant label values here, so that they are\nignored from day one. That way you would not have to deal with \"I\nknow I got the warning, but it used to work and you broke it\" later.\n\n> +\t\tret = 1;\n> +\t\tfor_each_string_list_item(si, required_labels) {\n> +\t\t\tif (!string_list_has_string(&attr_labels, si->string)) {\n> +\t\t\t\tret = 0;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t\tstring_list_clear(&attr_labels, 0);\n> +\t}\n> +\n> +\treturn ret;\n> +}\n\n> +static void validate_label_name(const char *label)\n> +{\n> +\tif (check_valid_label_name(label))\n> +\t\twarning(_(\"Label '%s' discouraged. Recommended label names start \"\n> +\t\t\t\"with an alphabetic character and use only alphanumeric \"\n> +\t\t\t\"characters, dashes and underscores.\"), label);\n> +}\n\nInstead of returning void, parrot the return value from\ncheck_valid_label_name().\n\n> +\t\t/* The label token may have no argument, so no trailing ':' */\n\nWhy close the door for future pathspec magic \"labelfoo\" by being\nunnecessarily[*1*] lenient?\n\n [*1*] Does \":(label)\" mean anything useful, and is there a good\n reason why it should behave differently from \":(label:)\"?\n\nUnless there is a good reason why users would want \":(label)\", I'd\nsay we should make it a syntax error.\n\n> +\t\tif (skip_prefix(copyfrom, \"label\", &body)) {\n> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\t\tskip_prefix(body, \":\", &body);\n> +\t\t\tstrbuf_add(&sb, body, nextat - body);\n> +\t\t\tif (!item->labels) {\n> +\t\t\t\titem->labels = xmalloc(sizeof(*item->labels));\n> +\t\t\t\tstring_list_init(item->labels, 1);\n> +\t\t\t} else\n> +\t\t\t\tdie(_(\"multiple labels not supported in pathspec magic\"));\n\nI am NOT suggesting to make this enhancement in the prototype to\nallow us experiment with submodule selection use case, but this is\nan obvious place to allow\n\n\t:(label=A B):(label=C D)\n\nto mean ((A & B) | (C & D)) by making item->labels an array of set\nof labels.\n\nAnd no, I do not think arbitrary boolean expression is too complex\nto understand to the end-users, especially if we have to stay within\nthe pathspec magic syntax. And my gut feeling is that it is not\nworth it to support anything more complex than \"OR of these ANDed\nones\".\n\n> +\t\t\tstring_list_split(item->labels, sb.buf, ' ', -1);\n> +\t\t\tstring_list_remove_empty_items(item->labels, 0);\n> +\t\t\tstrbuf_release(&sb);\n> +\t\t\tcontinue;\n\nThe data structure to record the \"required labels\" is shared\nknowledge between this function and the has_all_labels() and nobody\nelse knows it is done with string_list, so I think this is a good\nbalance between expediency and future optimization possibilities (I\nam anticipating that linear search of string list would be found as\nperformance bottleneck).\n\n> @@ -447,6 +490,12 @@ 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].labels) {\n> +\t\t\tstruct string_list_item *si;\n> +\t\t\tfor_each_string_list_item(si, item[i].labels)\n> +\t\t\t\tvalidate_label_name(si->string);\n\nReject a request to use a label that does not validate here.\n\nThe series is surprisingly simple and was a pleasant read (I no\nlonger recall how this compares with the earlier change to do this\nas [submodule \"x\"] label=X). Especially, given that the first two\npatches are unrelated clean-ups that can be split out, this now\nconsists of merely 3 patches, each of which is an easily digestable\nsize.\n"},{"id":"286678","messageId":"CAGZ79kZsVH3mE0zXu9wSWgit3QZ_CiKZqg-TQdyWnZw6D9=Cgw@mail.gmail.com","threadId":"42317","inReplyTo":"xmqqk2iw78aq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] pathspec: record labels","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-16T17:16:55Z","receivedAt":"2016-05-16T17:16:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, May 14, 2016 at 12:23 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Labels were originally designed to manage large amount of\n>> submodules, the discussion steered this in a more general\n>> direction, such that other files can be labeled as well.\n>\n> s/other files/any path/.\n>\n>> Labels are meant to describe arbitrary set of files, which\n>> is not described via the tree layout.\n>\n> Let's avoid \"are meant to\", which is merely to give you an excuse to\n> say \"it was meant to ... but the implementation did not quite achieve\n> that goal\".\n>\n> The \"label\" attribute can be attached to paths, and the pathspec\n> mechanism is extended via the new \":(label=X)pattern/to/match\"\n\nI wasn't sure about whether we want to have '=' or ':' as the separator\nof \"label\" and its values. The prefix which is passed around uses\na colon instead of equal sign, so I thought consistency would be neat\nthere. But as that is only internal, we can be inconsistent to a new\npublic feature, so '=' seems better for the labeling system.\n\n> syntax to filter paths so that it requires paths to not just\n> match the given pattern but also have the specified labels\n> attached for them to be chosen.\n>\n> or something?\n>\n>> +Attaching labels to path\n>> +~~~~~~~~~~~~~~~~~~~~~~~~\n>> +\n>> +`label`\n>> +^^^^^^^^^^\n>> +By the value of this attribute you can specify a list of arbitrary strings\n>> +to be attached to a path as labels. These labels can be used for\n>> +easier paths matching using pathspecs (linkgit:gitglossary[1]).\n>> +It is recommended to use only alphanumeric characters, dashes and\n>> +underscores for the labels.\n>\n> Make this recomendation stronger to requirement. It is always\n> possible to loosen it later, but once we allow random things even\n> with a warning, it gets impossible to take them back. Users will say\n> \"Even though we got a warning, it used to correctly filter; now Git\n> is broken and my setup does not work.\"\n\nI'll change the text here with s/recommended/required/ and error out\nfor that.\n\nBut still we do not enforce it early enough. Some crazy upstream may\nadd some labels which do not follow the requirements and then\ntell downstream to run a command like `git foo \"(label=!@#$)\".\nand then downstream curses at upstream as well as Git.\n\n>\n> s/habe/have/\n\ndone\n\n>\n>> + if (ATTR_TRUE(check.value))\n>> + ret = 1; /* has all the labels */\n>\n> So this is \"pretend that I have all the labels under the sun\".\n>\n>> + else if (ATTR_FALSE(check.value))\n>> + ret = 0; /* has no labels */\n>\n> I do not see a value in this. What difference does it have compared\n> to having a \"label=\" that explicitly says \"I do not have one\"? What\n> is your answer to an end-user question \"When should I say 'label='\n> and when should I say '!label'?\"\n>\n> Just forbid it for now; we may find a better use for it later.\n\nI don't think we want to die in that code as it is in the data already.\nIf we were to die in that case, you cannot query any path with\nlabels any more if asking for any kind of label.\n\n>\n>> + else if (ATTR_UNSET(check.value))\n>> + /*\n>> + * If no label was specified this matches, otherwise\n>> + * there is a missing label.\n>> + */\n>> + ret = (required_labels->nr > 0) ? 0 : 1;\n>\n> Hmm, I can see that this is making things more complicated to\n> explain and understand, but I cannot see what the expected use case\n> is.\n>\n> Unless there is any compelling use case, I'd say we should forbid it\n> for now.\n\nWe need to allow the UNSET case, as otherwise you'd need to label\nany path if using labels? In the next iteration I'll go with\n\n if (ATTR_TRUE(check.value))\n ret = 1; /* has all the labels */\n else if (ATTR_FALSE(check.value))\n die(_(\"Label attributes must not be unset\"));\n else if (ATTR_UNSET(check.value))\n ret = 0; /* has no labels */\n else {\n ...\n>\n>> + else {\n>> + struct string_list_item *si;\n>> + struct string_list attr_labels = STRING_LIST_INIT_DUP;\n>> + string_list_split(&attr_labels, check.value, ',', -1);\n>> + string_list_sort(&attr_labels);\n>\n> Filter out a non-compliant label values here, so that they are\n> ignored from day one. That way you would not have to deal with \"I\n> know I got the warning, but it used to work and you broke it\" later.\n\nSo you're saying we should not die(...) but just ignore those labels?\n\n>\n>> + ret = 1;\n>> + for_each_string_list_item(si, required_labels) {\n>> + if (!string_list_has_string(&attr_labels, si->string)) {\n>> + ret = 0;\n>> + break;\n>> + }\n>> + }\n>> + string_list_clear(&attr_labels, 0);\n>> + }\n>> +\n>> + return ret;\n>> +}\n>\n>> +static void validate_label_name(const char *label)\n>> +{\n>> + if (check_valid_label_name(label))\n>> + warning(_(\"Label '%s' discouraged. Recommended label names start \"\n>> + \"with an alphabetic character and use only alphanumeric \"\n>> + \"characters, dashes and underscores.\"), label);\n>> +}\n>\n> Instead of returning void, parrot the return value from\n> check_valid_label_name().\n>\n>> + /* The label token may have no argument, so no trailing ':' */\n>\n> Why close the door for future pathspec magic \"labelfoo\" by being\n> unnecessarily[*1*] lenient?\n>\n> [*1*] Does \":(label)\" mean anything useful, and is there a good\n> reason why it should behave differently from \":(label:)\"?\n\nok fixed.\n\n>\n> Unless there is a good reason why users would want \":(label)\", I'd\n> say we should make it a syntax error.\n>\n>> + if (skip_prefix(copyfrom, \"label\", &body)) {\n>> + struct strbuf sb = STRBUF_INIT;\n>> + skip_prefix(body, \":\", &body);\n>> + strbuf_add(&sb, body, nextat - body);\n>> + if (!item->labels) {\n>> + item->labels = xmalloc(sizeof(*item->labels));\n>> + string_list_init(item->labels, 1);\n>> + } else\n>> + die(_(\"multiple labels not supported in pathspec magic\"));\n>\n> I am NOT suggesting to make this enhancement in the prototype to\n> allow us experiment with submodule selection use case, but this is\n> an obvious place to allow\n>\n> :(label=A B):(label=C D)\n>\n> to mean ((A & B) | (C & D)) by making item->labels an array of set\n> of labels.\n>\n> And no, I do not think arbitrary boolean expression is too complex\n> to understand to the end-users, especially if we have to stay within\n> the pathspec magic syntax. And my gut feeling is that it is not\n> worth it to support anything more complex than \"OR of these ANDed\n> ones\".\n\nThat makes sense.\n\n>\n>> + string_list_split(item->labels, sb.buf, ' ', -1);\n>> + string_list_remove_empty_items(item->labels, 0);\n>> + strbuf_release(&sb);\n>> + continue;\n>\n> The data structure to record the \"required labels\" is shared\n> knowledge between this function and the has_all_labels() and nobody\n> else knows it is done with string_list, so I think this is a good\n> balance between expediency and future optimization possibilities (I\n> am anticipating that linear search of string list would be found as\n> performance bottleneck).\n>\n>> @@ -447,6 +490,12 @@ void parse_pathspec(struct pathspec *pathspec,\n>> if (item[i].nowildcard_len < item[i].len)\n>> pathspec->has_wildcard = 1;\n>> pathspec->magic |= item[i].magic;\n>> +\n>> + if (item[i].labels) {\n>> + struct string_list_item *si;\n>> + for_each_string_list_item(si, item[i].labels)\n>> + validate_label_name(si->string);\n>\n> Reject a request to use a label that does not validate here.\n\ndone.\n"},{"id":"286681","messageId":"xmqqlh393nuv.fsf@gitster.mtv.corp.google.com","threadId":"42317","inReplyTo":"CAGZ79kZsVH3mE0zXu9wSWgit3QZ_CiKZqg-TQdyWnZw6D9=Cgw@mail.gmail.com","subject":"Re: [PATCH 5/5] pathspec: record labels","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T17:38:00Z","receivedAt":"2016-05-16T17:38:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> Let's avoid \"are meant to\", which is merely to give you an excuse to\n>> say \"it was meant to ... but the implementation did not quite achieve\n>> that goal\".\n>>\n>> The \"label\" attribute can be attached to paths, and the pathspec\n>> mechanism is extended via the new \":(label=X)pattern/to/match\"\n>\n> I wasn't sure about whether we want to have '=' or ':' as the separator\n> of \"label\" and its values. ...\n\nOh, sorry, the above wasn't a suggestion to change : to = at all. I\nwas merely being sloppy in the details that is irrelevant to my main\npoint (which is, it is better spend the bits you spent for \"meant\nto\" instead for saying clearly what it does).\n\n> ... But as that is only internal, we can be inconsistent to a new\n> public feature, so '=' seems better for the labeling system.\n\nI can buy that, too. Good that my sloppy wording helped you think\nabout it further ;-).\n\n> But still we do not enforce it early enough. Some crazy upstream may\n> add some labels which do not follow the requirements and then\n> tell downstream to run a command like `git foo \"(label=!@#$)\".\n> and then downstream curses at upstream as well as Git.\n\nThat is why it is \"warn and ignore\", not \"die\".\n\n>>> + if (ATTR_TRUE(check.value))\n>>> + ret = 1; /* has all the labels */\n>>\n>> So this is \"pretend that I have all the labels under the sun\".\n>>\n>>> + else if (ATTR_FALSE(check.value))\n>>> + ret = 0; /* has no labels */\n>>\n>> I do not see a value in this. What difference does it have compared\n>> to having a \"label=\" that explicitly says \"I do not have one\"? What\n>> is your answer to an end-user question \"When should I say 'label='\n>> and when should I say '!label'?\"\n>>\n>> Just forbid it for now; we may find a better use for it later.\n>\n> I don't think we want to die in that code as it is in the data already.\n\nIs it? I think this is coming from the command line pathspec magic,\nnot from .gitattributes file recorded in the tree.\n\n> We need to allow the UNSET case, as otherwise you'd need to label\n> any path if using labels?\n\nYou do need UNSET (roughly, \"no label is mentioned for the path\").\n\nIf I want to say \"Everything under Documentation/ and also things\nwith label=doc\", I'd say\n\n\tgit cmd \"Documentation/\" \":(label=doc)\"\n\nand no path in Documentation/ need any label, i.e. for them, the\nlabels are unset. They will not be caught with \":(label=doc)\"\nbecause they are not set, but that is OK.\n\nFALSE is different from UNSET. It needs an explicit mention, i.e.\n\n\t*.doc\tdoc\n false.doc\t-doc\n\nWhat's the difference between saying \"-doc\" and not saying anything?\nIf you really want to explicitly say \"doc attribute is unset for\nthis path (perhaps because you may have other patterns that set the\ndoc elsewhere), you would say \"!doc\", and you already have code for\nthat.\n\n> if (ATTR_TRUE(check.value))\n> ret = 1; /* has all the labels */\n> else if (ATTR_FALSE(check.value))\n> die(_(\"Label attributes must not be unset\"));\n\nThe message is wrong. You are dying because the user explicitly set\nit to false, not because the user made it unset.\n\n> else if (ATTR_UNSET(check.value))\n> ret = 0; /* has no labels */\n\n>>> + struct string_list_item *si;\n>>> + struct string_list attr_labels = STRING_LIST_INIT_DUP;\n>>> + string_list_split(&attr_labels, check.value, ',', -1);\n>>> + string_list_sort(&attr_labels);\n>>\n>> Filter out a non-compliant label values here, so that they are\n>> ignored from day one. That way you would not have to deal with \"I\n>> know I got the warning, but it used to work and you broke it\" later.\n>\n> So you're saying we should not die(...) but just ignore those labels?\n\nDo not die() but warn-and-ignore when you see funnies in\n.gitattributes; do die() if you see funnies in pathspec magic.\n\n>> I am NOT suggesting to make this enhancement in the prototype to\n>> allow us experiment with submodule selection use case, but this is\n>> an obvious place to allow\n>>\n>> :(label=A B):(label=C D)\n>>\n>> to mean ((A & B) | (C & D)) by making item->labels an array of set\n>> of labels.\n>>\n>> And no, I do not think arbitrary boolean expression is too complex\n\ns/do not/do/\n\n>> to understand to the end-users, especially if we have to stay within\n>> the pathspec magic syntax. And my gut feeling is that it is not\n>> worth it to support anything more complex than \"OR of these ANDed\n>> ones\".\n>\n> That makes sense.\n\nThanks.\n"},{"id":"286684","messageId":"CAGZ79kbN_pzAVeAa-St_KRs60SbURTfKKP0v+do_+MK7orTkTg@mail.gmail.com","threadId":"42317","inReplyTo":"xmqqlh393nuv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] pathspec: record labels","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-16T17:46:00Z","receivedAt":"2016-05-16T17:46:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 16, 2016 at 10:38 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>>> Let's avoid \"are meant to\", which is merely to give you an excuse to\n>>> say \"it was meant to ... but the implementation did not quite achieve\n>>> that goal\".\n>>>\n>>> The \"label\" attribute can be attached to paths, and the pathspec\n>>> mechanism is extended via the new \":(label=X)pattern/to/match\"\n>>\n>> I wasn't sure about whether we want to have '=' or ':' as the separator\n>> of \"label\" and its values. ...\n>\n> Oh, sorry, the above wasn't a suggestion to change : to = at all. I\n> was merely being sloppy in the details that is irrelevant to my main\n> point (which is, it is better spend the bits you spent for \"meant\n> to\" instead for saying clearly what it does).\n>\n>> ... But as that is only internal, we can be inconsistent to a new\n>> public feature, so '=' seems better for the labeling system.\n>\n> I can buy that, too. Good that my sloppy wording helped you think\n> about it further ;-).\n>\n>> But still we do not enforce it early enough. Some crazy upstream may\n>> add some labels which do not follow the requirements and then\n>> tell downstream to run a command like `git foo \"(label=!@#$)\".\n>> and then downstream curses at upstream as well as Git.\n>\n> That is why it is \"warn and ignore\", not \"die\".\n\nSo \"warn and ignore\" for data from .gitattributes and die for\ncommandline arguments? That makes sense.\n\n>\n>>>> + if (ATTR_TRUE(check.value))\n>>>> + ret = 1; /* has all the labels */\n>>>\n>>> So this is \"pretend that I have all the labels under the sun\".\n>>>\n>>>> + else if (ATTR_FALSE(check.value))\n>>>> + ret = 0; /* has no labels */\n>>>\n>>> I do not see a value in this. What difference does it have compared\n>>> to having a \"label=\" that explicitly says \"I do not have one\"? What\n>>> is your answer to an end-user question \"When should I say 'label='\n>>> and when should I say '!label'?\"\n>>>\n>>> Just forbid it for now; we may find a better use for it later.\n>>\n>> I don't think we want to die in that code as it is in the data already.\n>\n> Is it? I think this is coming from the command line pathspec magic,\n> not from .gitattributes file recorded in the tree.\n\nThe 'check.value' is coming from the .gitattributes.\n\nThe 'required_labels' string list comes from the command line.\n(\"The command line requires these labels\")\n\n>\n>> We need to allow the UNSET case, as otherwise you'd need to label\n>> any path if using labels?\n>\n> You do need UNSET (roughly, \"no label is mentioned for the path\").\n>\n> If I want to say \"Everything under Documentation/ and also things\n> with label=doc\", I'd say\n>\n> git cmd \"Documentation/\" \":(label=doc)\"\n>\n> and no path in Documentation/ need any label, i.e. for them, the\n> labels are unset. They will not be caught with \":(label=doc)\"\n> because they are not set, but that is OK.\n\nright. that is what the end user should be able to do. That also means,\nwe need to handle files with UNSET labels (all under Documentation/)\n\n>\n> FALSE is different from UNSET. It needs an explicit mention, i.e.\n>\n> *.doc doc\n> false.doc -doc\n\nand this is in the .gitattribues, so warn-and-ignore and by ignore\nwe mean treat it as UNSET?\n\n>\n> What's the difference between saying \"-doc\" and not saying anything?\n> If you really want to explicitly say \"doc attribute is unset for\n> this path (perhaps because you may have other patterns that set the\n> doc elsewhere), you would say \"!doc\", and you already have code for\n> that.\n>\n>> if (ATTR_TRUE(check.value))\n>> ret = 1; /* has all the labels */\n>> else if (ATTR_FALSE(check.value))\n>> die(_(\"Label attributes must not be unset\"));\n>\n> The message is wrong. You are dying because the user explicitly set\n> it to false, not because the user made it unset.\n\nOk, so here is the warn-and-ignore code:\n\n\n if (ATTR_TRUE(check.value))\n ret = 1; /* has all the labels */\n else if (ATTR_FALSE(check.value)) {\n warning(_(\"Path '%s': Label must not be false. Treat\nas if no label was set\"), path);\n ret = 0;\n else if (ATTR_UNSET(check.value))\n ret = 0; /* has no labels */\n else {\n\nThanks,\nStefan\n"},{"id":"286686","messageId":"CAGZ79kbHW+qzQjoVu9gRYC0FBqpkq5bPPLU3=BdEFKeGc=U6sA@mail.gmail.com","threadId":"42317","inReplyTo":"xmqqk2iw78aq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] pathspec: record labels","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-16T17:55:56Z","receivedAt":"2016-05-16T17:55:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> I am NOT suggesting to make this enhancement in the prototype to\n> allow us experiment with submodule selection use case, but this is\n> an obvious place to allow\n>\n> :(label=A B):(label=C D)\n>\n> to mean ((A & B) | (C & D)) by making item->labels an array of set\n> of labels.\n\nThis is what already works with the series. Or rather:\n\n \":(label=A B)\" \":(label=C D)\"\n\nworks as you would expect for (A&B) | (C&D).\n\nSo I am a bit hesitant to replace the string list by an array\nof stringlists or such, as the future enhancement can also do that?\n\nThe enhancement may bring in more expressions into the label string,\nso it may even parse that string into a tree for Lexicographical order\ninstead of just using an array of lists.\n\n>\n> And no, I do not think arbitrary boolean expression is too complex\n> to understand to the end-users, especially if we have to stay within\n> the pathspec magic syntax. And my gut feeling is that it is not\n> worth it to support anything more complex than \"OR of these ANDed\n> ones\".\n>\n>> + string_list_split(item->labels, sb.buf, ' ', -1);\n>> + string_list_remove_empty_items(item->labels, 0);\n>> + strbuf_release(&sb);\n>> + continue;\n>\n> The data structure to record the \"required labels\" is shared\n> knowledge between this function and the has_all_labels() and nobody\n> else knows it is done with string_list, so I think this is a good\n> balance between expediency and future optimization possibilities (I\n> am anticipating that linear search of string list would be found as\n> performance bottleneck).\n>\n"},{"id":"286697","messageId":"xmqqd1ol3ku7.fsf@gitster.mtv.corp.google.com","threadId":"42317","inReplyTo":"CAGZ79kbHW+qzQjoVu9gRYC0FBqpkq5bPPLU3=BdEFKeGc=U6sA@mail.gmail.com","subject":"Re: [PATCH 5/5] pathspec: record labels","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T18:43:12Z","receivedAt":"2016-05-16T18:43:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> I am NOT suggesting to make this enhancement in the prototype to\n>> allow us experiment with submodule selection use case, but this is\n>> an obvious place to allow\n>>\n>> :(label=A B):(label=C D)\n>>\n>> to mean ((A & B) | (C & D)) by making item->labels an array of set\n>> of labels.\n>\n> This is what already works with the series. Or rather:\n>\n> \":(label=A B)\" \":(label=C D)\"\n>\n> works as you would expect for (A&B) | (C&D).\n\nThat is \"duplicationg path\". I was envisioning a single\n\n\t\":(label=A B):(label=C D)tediously/long/path/because/java/\"\n\na shorter and sweeter way to say your two pathspec variant, i.e.\n\n\t\":(label=A B)tediously/long/path/because/java/\" \\\n \":(label=C D)tediously/long/path/because/java/\"\n"},{"id":"286698","messageId":"xmqq8tz93kf9.fsf@gitster.mtv.corp.google.com","threadId":"42317","inReplyTo":"CAGZ79kbN_pzAVeAa-St_KRs60SbURTfKKP0v+do_+MK7orTkTg@mail.gmail.com","subject":"Re: [PATCH 5/5] pathspec: record labels","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T18:52:10Z","receivedAt":"2016-05-16T18:52:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So \"warn and ignore\" for data from .gitattributes and die for\n> commandline arguments? That makes sense.\n\nYes.\n\nOn the \"command line\" front, because we may want to give different\nmeanings to these two entries in the future:\n\n :(label=-doc)Documentation/\n\t:(label=!doc)Documentation/\n\nwe should diagnose -doc (FALSE) as an error, not treating it as the\nsame as !doc (UNSET). And we should warn and ignore -doc (FALSE) in\n.gitattributes. Yes, ignoring it would be more or less equivalent\nto treating it as UNSET, but because we may use -doc (FALSE) for a\nbetter purpose later, we should still warn.\n\n> Ok, so here is the warn-and-ignore code:\n>\n>\n> if (ATTR_TRUE(check.value))\n> ret = 1; /* has all the labels */\n> else if (ATTR_FALSE(check.value)) {\n> warning(_(\"Path '%s': Label must not be false. Treat\n> as if no label was set\"), path);\n> ret = 0;\n\ns/Treat as if .../The -label may be used differently in future\nversions of Git, so do not use it/;\n\nBut if we are going in the direction of :(attr:crlf=auto), all this\ndiscussion is moot, isn't it? I haven't formed a firm opinion on\nthis, but it sure does sound tempting, doesn't it?\n"},{"id":"286700","messageId":"CAGZ79kaV+hQRn6ZraZcG9ZRp1XwPGrEFibBEX5qFJdWDCg0uxQ@mail.gmail.com","threadId":"42317","inReplyTo":"xmqq8tz93kf9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] pathspec: record labels","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-16T19:03:06Z","receivedAt":"2016-05-16T19:03:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 16, 2016 at 11:52 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> So \"warn and ignore\" for data from .gitattributes and die for\n>> commandline arguments? That makes sense.\n>\n> Yes.\n>\n> On the \"command line\" front, because we may want to give different\n> meanings to these two entries in the future:\n>\n> :(label=-doc)Documentation/\n> :(label=!doc)Documentation/\n\nYes but both of these already errors out with:\n\n Label -doc': Label names must start with an\n alphabetic character and use only alphanumeric\n characters, dashes and underscores.\n fatal: Labels in the wrong syntax are prohibited.\n\nThe command line arguments are not parsed via the attr system.\nOnly data from .gitattributes are parsed via the attr system.\n\nI can add tests for those to fail though.\n\n>\n> we should diagnose -doc (FALSE) as an error, not treating it as the\n> same as !doc (UNSET). And we should warn and ignore -doc (FALSE) in\n> .gitattributes. Yes, ignoring it would be more or less equivalent\n> to treating it as UNSET, but because we may use -doc (FALSE) for a\n> better purpose later, we should still warn.\n>\n>> Ok, so here is the warn-and-ignore code:\n>>\n>>\n>> if (ATTR_TRUE(check.value))\n>> ret = 1; /* has all the labels */\n>> else if (ATTR_FALSE(check.value)) {\n>> warning(_(\"Path '%s': Label must not be false. Treat\n>> as if no label was set\"), path);\n>> ret = 0;\n>\n> s/Treat as if .../The -label may be used differently in future\n> versions of Git, so do not use it/;\n\n\"... but for now Git treats it as if it is not set at all\" is a valuable\ninformation to the user, still?\n\nThat code path is only used for data coming from .gitattributes,\nso we can bike shed the best warning message.\n\n>\n> But if we are going in the direction of :(attr:crlf=auto), all this\n> discussion is moot, isn't it? I haven't formed a firm opinion on\n> this, but it sure does sound tempting, doesn't it?\n>\n\nThat direction sounds scary as people may use any sort of labels, so\nwe can no longer add labels later on sanely.\n\n :(attr:random-attr=foo)\n\nshould also fall into the \"warn and ignore\" space. We only want to allow\n\n :(attr:known-attr)\n :(attr:label=..)\n\ninstead of label= we may want to allow a little bit more, such as\nabbreviated 'l='\nor 'group=', but not any sort of labeling.\n"},{"id":"286701","messageId":"xmqqzirp253d.fsf@gitster.mtv.corp.google.com","threadId":"42317","inReplyTo":"CAGZ79kaV+hQRn6ZraZcG9ZRp1XwPGrEFibBEX5qFJdWDCg0uxQ@mail.gmail.com","subject":"Re: [PATCH 5/5] pathspec: record labels","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T19:08:38Z","receivedAt":"2016-05-16T19:08:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> \"... but for now Git treats it as if it is not set at all\" is a valuable\n> information to the user, still?\n\nNot at all. \"What you are using is wrong and there is no guarantee\nwhat behaviour you would get\" is the message we need to convey.\n"},{"id":"286702","messageId":"CAGZ79kbLcRgM5PoTp7DA-c5DneAWJOxNwZVSMxU0LdVtuBfiRA@mail.gmail.com","threadId":"42317","inReplyTo":"xmqqzirp253d.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] pathspec: record labels","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-16T19:12:58Z","receivedAt":"2016-05-16T19:12:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 16, 2016 at 12:08 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> \"... but for now Git treats it as if it is not set at all\" is a valuable\n>> information to the user, still?\n>\n> Not at all. \"What you are using is wrong and there is no guarantee\n> what behaviour you would get\" is the message we need to convey.\n\n \"Path '%s': Ignoring label set to false; This may behave\ndifferently in future versions of Git.\"\n\nMaybe even with s/may/will/\n\nI think that conveys both the message about the future as well as\n\"what happened just now in that command\"\n"},{"id":"286705","messageId":"xmqqvb2d247u.fsf@gitster.mtv.corp.google.com","threadId":"42317","inReplyTo":"CAGZ79kbLcRgM5PoTp7DA-c5DneAWJOxNwZVSMxU0LdVtuBfiRA@mail.gmail.com","subject":"Re: [PATCH 5/5] pathspec: record labels","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T19:27:33Z","receivedAt":"2016-05-16T19:27:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> \"Path '%s': Ignoring label set to false; This may behave\n> differently in future versions of Git.\"\n\nI agree that mentioning \"ignoring\" is good enough. I think our\nrecent messages begin with lowercase, though.\n"}]}