{"thread":{"id":"42297","subject":"[PATCH 2/4] pathspec: move long magic parsing out of prefix_pathspec","startedAt":"2016-05-13T00:19:32Z","lastAt":"2016-05-16T22:19:50Z","messageCount":25,"participants":["Stefan Beller","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"286416","messageId":"20160513001936.7623-1-sbeller@google.com","threadId":"42297","inReplyTo":null,"subject":"[RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-13T00:19:32Z","receivedAt":"2016-05-13T00:19:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"After some fruitful discussion[1] in which Junio suggested trying a very\ndifferent route[2] that is more general and not submodule related, I considered\ndoing a mock for this.\n\nThis lets you label arbitrary pathspecs, e.g. in git.git we may want to have:\n\n    t/t[0-9]*.sh label=tests\n    \nsuch that\n\n    $ git log --author=Beller \":(label=tests)\"\n    \nwould show all commits in which I touched tests.\n\nThis has suprisingly few lines of code, as the first 3 patches are refactoring.\nThe actual new feature is in the last patch.\n\nThis would solve the submodule issues I want to solve, as I can produce a\n.gitattributes like:\n\n    ./submodule1 label=default\n    ./submodule2 label=default\n    ./submodule3 label=optional-feature1\n    \nand then I'd instruct the users to clone like this:\n\n    git clone .. superproject\n    cd superproject\n    git submodule update --init :(label:default)\n    \nThe second part of the submodule series to collapse these three commands\nwill come as an extra series later, then.\n\nWhat annoys me here:\nAttributes can only be set once at the moment, there is no way to collect all\nattributes. If we'd have such a collection feature we would be able to\nconfigure this:\n\n    *.[ch] label=C,code\n    contrib/** label=oldstuff\n    \nand run this:\n\n    git status \":(label:C oldstuff)\"\n    \nwhich would be the equivalent to\n\n    contrib/**.[ch]\n    \nas in this proposed implementation the labels which are given within one\npathspec item are logical AND. To get the logical OR, you'd have to give multiple\npathspec items, i.e. \":(label:C)\" \":(label:oldstuff)\"\n\nFeedback welcome!\n\nThanks,\nStefan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/294212\n[2] http://thread.gmane.org/gmane.comp.version-control.git/294212/focus=294391\n\nStefan Beller (4):\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/glossary-content.txt            |   5 ++\n Documentation/technical/api-gitattributes.txt |   2 +-\n attr.h                                        |   1 +\n dir.c                                         |  31 ++++++++\n pathspec.c                                    | 109 +++++++++++++++++---------\n pathspec.h                                    |   1 +\n t/t6134-pathspec-with-labels.sh               |  91 +++++++++++++++++++++\n 7 files changed, 201 insertions(+), 39 deletions(-)\n create mode 100755 t/t6134-pathspec-with-labels.sh\n\n-- \n2.8.2.400.g66c4903.dirty\n"},{"id":"286415","messageId":"20160513001936.7623-2-sbeller@google.com","threadId":"42297","inReplyTo":"20160513001936.7623-1-sbeller@google.com","subject":"[PATCH 1/4] Documentation: correct typo in example for querying attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-13T00:19:33Z","receivedAt":"2016-05-13T00:19:33Z","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 <sbeller@google.com>\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.400.g66c4903.dirty\n"},{"id":"286414","messageId":"20160513001936.7623-3-sbeller@google.com","threadId":"42297","inReplyTo":"20160513001936.7623-1-sbeller@google.com","subject":"[PATCH 2/4] pathspec: move long magic parsing out of prefix_pathspec","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-13T00:19:34Z","receivedAt":"2016-05-13T00:19:34Z","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 <sbeller@google.com>\n---\n pathspec.c | 84 +++++++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 47 insertions(+), 37 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex c9e9b6c..eba37c2 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -88,6 +88,52 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+static void eat_long_magic(struct pathspec_item *item, const char *elt,\n+\t\tunsigned *magic, int *pathspec_prefix,\n+\t\tconst char **copyfrom_, const char **long_magic_end)\n+{\n+\tint i;\n+\tconst char *copyfrom = *copyfrom_;\n+\t/* longhand */\n+\tconst char *nextat;\n+\tfor (copyfrom = elt + 2;\n+\t     *copyfrom && *copyfrom != ')';\n+\t     copyfrom = nextat) {\n+\t\tsize_t len = strcspn(copyfrom, \",)\");\n+\t\tif (copyfrom[len] == ',')\n+\t\t\tnextat = copyfrom + len + 1;\n+\t\telse\n+\t\t\t/* handle ')' and '\\0' */\n+\t\t\tnextat = copyfrom + len;\n+\t\tif (!len)\n+\t\t\tcontinue;\n+\t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n+\t\t\tif (strlen(pathspec_magic[i].name) == len &&\n+\t\t\t    !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n+\t\t\t\t*magic |= pathspec_magic[i].bit;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tif (starts_with(copyfrom, \"prefix:\")) {\n+\t\t\t\tchar *endptr;\n+\t\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n+\t\t\t\t\t\t\t  &endptr, 10);\n+\t\t\t\tif (endptr - copyfrom != len)\n+\t\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n+\t\t\t\t/* \"i\" would be wrong, but it does not matter */\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (ARRAY_SIZE(pathspec_magic) <= i)\n+\t\t\tdie(_(\"Invalid pathspec magic '%.*s' in '%s'\"),\n+\t\t\t    (int) len, copyfrom, elt);\n+\t}\n+\tif (*copyfrom != ')')\n+\t\tdie(_(\"Missing ')' at the end of pathspec magic in '%s'\"), elt);\n+\t*long_magic_end = copyfrom;\n+\tcopyfrom++;\n+\t*copyfrom_ = copyfrom;\n+}\n+\n /*\n  * Take an element of a pathspec and check for magic signatures.\n  * Append the result to the prefix. Return the magic bitmap.\n@@ -150,43 +196,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \t    (flags & PATHSPEC_LITERAL_PATH)) {\n \t\t; /* nothing to do */\n \t} else if (elt[1] == '(') {\n-\t\t/* longhand */\n-\t\tconst char *nextat;\n-\t\tfor (copyfrom = elt + 2;\n-\t\t     *copyfrom && *copyfrom != ')';\n-\t\t     copyfrom = nextat) {\n-\t\t\tsize_t len = strcspn(copyfrom, \",)\");\n-\t\t\tif (copyfrom[len] == ',')\n-\t\t\t\tnextat = copyfrom + len + 1;\n-\t\t\telse\n-\t\t\t\t/* handle ')' and '\\0' */\n-\t\t\t\tnextat = copyfrom + len;\n-\t\t\tif (!len)\n-\t\t\t\tcontinue;\n-\t\t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n-\t\t\t\tif (strlen(pathspec_magic[i].name) == len &&\n-\t\t\t\t    !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n-\t\t\t\t\tmagic |= pathspec_magic[i].bit;\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t\tif (starts_with(copyfrom, \"prefix:\")) {\n-\t\t\t\t\tchar *endptr;\n-\t\t\t\t\tpathspec_prefix = strtol(copyfrom + 7,\n-\t\t\t\t\t\t\t\t &endptr, 10);\n-\t\t\t\t\tif (endptr - copyfrom != len)\n-\t\t\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n-\t\t\t\t\t/* \"i\" would be wrong, but it does not matter */\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t\tif (ARRAY_SIZE(pathspec_magic) <= i)\n-\t\t\t\tdie(_(\"Invalid pathspec magic '%.*s' in '%s'\"),\n-\t\t\t\t    (int) len, copyfrom, elt);\n-\t\t}\n-\t\tif (*copyfrom != ')')\n-\t\t\tdie(_(\"Missing ')' at the end of pathspec magic in '%s'\"), elt);\n-\t\tlong_magic_end = copyfrom;\n-\t\tcopyfrom++;\n+\t\teat_long_magic(item, elt, &magic, &pathspec_prefix, &copyfrom, &long_magic_end);\n \t} else {\n \t\t/* shorthand */\n \t\tfor (copyfrom = elt + 1;\n-- \n2.8.2.400.g66c4903.dirty\n"},{"id":"286417","messageId":"20160513001936.7623-4-sbeller@google.com","threadId":"42297","inReplyTo":"20160513001936.7623-1-sbeller@google.com","subject":"[PATCH 3/4] pathspec: move prefix check out of the inner loop","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-13T00:19:35Z","receivedAt":"2016-05-13T00:19:35Z","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 <sbeller@google.com>\n---\n pathspec.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex eba37c2..4dff252 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -107,21 +107,22 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \t\t\tnextat = copyfrom + len;\n \t\tif (!len)\n \t\t\tcontinue;\n+\n+\t\tif (starts_with(copyfrom, \"prefix:\")) {\n+\t\t\tchar *endptr;\n+\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n+\t\t\t\t\t\t  &endptr, 10);\n+\t\t\tif (endptr - copyfrom != len)\n+\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n \t\t\tif (strlen(pathspec_magic[i].name) == len &&\n \t\t\t    !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n \t\t\t\t*magic |= pathspec_magic[i].bit;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tif (starts_with(copyfrom, \"prefix:\")) {\n-\t\t\t\tchar *endptr;\n-\t\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n-\t\t\t\t\t\t\t  &endptr, 10);\n-\t\t\t\tif (endptr - copyfrom != len)\n-\t\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n-\t\t\t\t/* \"i\" would be wrong, but it does not matter */\n-\t\t\t\tbreak;\n-\t\t\t}\n \t\t}\n \t\tif (ARRAY_SIZE(pathspec_magic) <= i)\n \t\t\tdie(_(\"Invalid pathspec magic '%.*s' in '%s'\"),\n-- \n2.8.2.400.g66c4903.dirty\n"},{"id":"286418","messageId":"20160513001936.7623-5-sbeller@google.com","threadId":"42297","inReplyTo":"20160513001936.7623-1-sbeller@google.com","subject":"[PATCH 4/4] pathspec: record labels","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-13T00:19:36Z","receivedAt":"2016-05-13T00:19:36Z","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 <sbeller@google.com>\n---\n Documentation/glossary-content.txt |  5 +++\n attr.h                             |  1 +\n dir.c                              | 31 +++++++++++++\n pathspec.c                         | 24 +++++++++-\n pathspec.h                         |  1 +\n t/t6134-pathspec-with-labels.sh    | 91 ++++++++++++++++++++++++++++++++++++++\n 6 files changed, 152 insertions(+), 1 deletion(-)\n create mode 100755 t/t6134-pathspec-with-labels.sh\n\ndiff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\nindex 8ad29e6..a1fc9e0 100644\n--- a/Documentation/glossary-content.txt\n+++ b/Documentation/glossary-content.txt\n@@ -362,6 +362,11 @@ glob;;\n \tFor example, \"Documentation/{asterisk}.html\" matches\n \t\"Documentation/git.html\" but not \"Documentation/ppc/ppc.html\"\n \tor \"tools/perf/Documentation/perf.html\".\n+\n+label:<white space separated list>;;\n+\tLabels can be assigned to pathspecs in the .gitattributes file.\n+\tBy specifying a list of labels the pattern will match only\n+\tfiles which have all of the listed labels.\n +\n Two consecutive asterisks (\"`**`\") in patterns matched against\n full pathname may have special meaning:\ndiff --git a/attr.h b/attr.h\nindex 8b08d33..f6fc7c3 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -18,6 +18,7 @@ extern const char git_attr__false[];\n #define ATTR_TRUE(v) ((v) == git_attr__true)\n #define ATTR_FALSE(v) ((v) == git_attr__false)\n #define ATTR_UNSET(v) ((v) == NULL)\n+#define ATTR_CUSTOM(v) (!(ATTR_UNSET(v) || ATTR_FALSE(v) || ATTR_TRUE(v)))\n \n /*\n  * Send one or more git_attr_check to git_check_attr(), and\ndiff --git a/dir.c b/dir.c\nindex 656f272..51d5965 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,27 @@ int within_depth(const char *name, int namelen,\n \treturn 1;\n }\n \n+void load_labels(const char *name, int namelen, struct string_list *list)\n+{\n+\tstatic struct git_attr *attr;\n+\tstruct git_attr_check check;\n+\tchar *path = xmemdupz(name, namelen);\n+\n+\tif (!attr)\n+\t\tattr = git_attr(\"label\");\n+\tcheck.attr = attr;\n+\n+\tif (git_check_attr(path, 1, &check))\n+\t\tdie(\"git_check_attr died\");\n+\n+\tif (ATTR_CUSTOM(check.value)) {\n+\t\tstring_list_split(list, check.value, ',', -1);\n+\t\tstring_list_sort(list);\n+\t}\n+\n+\tfree(path);\n+}\n+\n #define DO_MATCH_EXCLUDE   1\n #define DO_MATCH_DIRECTORY 2\n \n@@ -263,6 +285,15 @@ 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->group) {\n+\t\tstruct string_list has_labels = STRING_LIST_INIT_DUP;\n+\t\tstruct string_list_item *si;\n+\t\tload_labels(name, namelen, &has_labels);\n+\t\tfor_each_string_list_item(si, item->group)\n+\t\t\tif (!string_list_has_string(&has_labels, si->string))\n+\t\t\t\treturn 0;\n+\t}\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..c227c25 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -94,6 +94,7 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n {\n \tint i;\n \tconst char *copyfrom = *copyfrom_;\n+\tconst char *out;\n \t/* longhand */\n \tconst char *nextat;\n \tfor (copyfrom = elt + 2;\n@@ -117,6 +118,20 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (skip_prefix(copyfrom, \"label:\", &out)) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tsize_t l = nextat - out;\n+\t\t\tstrbuf_add(&sb, out, l);\n+\t\t\tif (!item->group) {\n+\t\t\t\titem->group = xmalloc(sizeof(*item->group));\n+\t\t\t\tstring_list_init(item->group, 1);\n+\t\t\t}\n+\t\t\tstring_list_split(item->group, sb.buf, ' ', -1);\n+\t\t\tstring_list_remove_empty_items(item->group, 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 +440,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].group = 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@@ -502,6 +517,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].group)\n+\t\t\tstring_list_clear(pathspec->items[i].group, 1);\n+\t\tfree(pathspec->items[i].group);\n+\t}\n+\n \tfree(pathspec->items);\n \tpathspec->items = NULL;\n }\ndiff --git a/pathspec.h b/pathspec.h\nindex 0c11262..e3f7ebf 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 *group;\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..0c061ce\n--- /dev/null\n+++ b/t/t6134-pathspec-with-labels.sh\n@@ -0,0 +1,91 @@\n+#!/bin/sh\n+\n+test_description='test labels in pathspecs'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a tree' '\n+\tfor p in file sub/file sub/sub/file sub/file2 sub/sub/sub/file sub2/file; do\n+\t\tif echo $p | grep /; then\n+\t\t\tmkdir -p $(dirname $p)\n+\t\tfi &&\n+\t\t: >$p &&\n+\t\tgit add $p &&\n+\t\tgit commit -m $p\n+\tdone &&\n+\tgit log --oneline --format=%s >actual &&\n+\tcat <<EOF >expect &&\n+sub2/file\n+sub/sub/sub/file\n+sub/file2\n+sub/sub/file\n+sub/file\n+file\n+EOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pathspec with labels and no .gitattributes exists' '\n+\tgit ls-files \":(label:a)\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'setup .gitattributes' '\n+\tcat <<-EOF >.gitattributes &&\n+\t/file label=b\n+\tsub/file label=a\n+\tsub/sub/* label=b,c\n+\tEOF\n+\tgit add .gitattributes &&\n+\tgit commit -m \"add attributes\"\n+'\n+\n+test_expect_success 'check label' '\n+\tcat <<-EOF >expect &&\n+\tsub/file\n+\tEOF\n+\tgit ls-files \":(label:a)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check label from label list' '\n+\tcat <<-EOF >expect &&\n+\tsub/sub/file\n+\tEOF\n+\tgit ls-files \":(label:c)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check label with more labels' '\n+\tcat <<-EOF >expect &&\n+\tfile\n+\tsub/sub/file\n+\tEOF\n+\tgit ls-files \":(label:b)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check label with more labels but excluded path' '\n+\tcat <<-EOF >expect &&\n+\tsub/sub/file\n+\tEOF\n+\tgit ls-files \":(label:b)\" \":(exclude)./file\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check label specifying more labels' '\n+\tcat <<-EOF >expect &&\n+\tsub/sub/file\n+\tEOF\n+\tgit ls-files \":(label:b c)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check label specifying more labels' '\n+\tcat <<-EOF >expect &&\n+\tsub/file\n+\tsub/sub/file\n+\tEOF\n+\tgit ls-files \":(label:b c)\" \":(label:a)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+test_done\n-- \n2.8.2.400.g66c4903.dirty\n"},{"id":"286420","messageId":"xmqqfutmbmsv.fsf@gitster.mtv.corp.google.com","threadId":"42297","inReplyTo":"20160513001936.7623-5-sbeller@google.com","subject":"Re: [PATCH 4/4] pathspec: record labels","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-13T04:32:16Z","receivedAt":"2016-05-13T04:32:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\n> index 8ad29e6..a1fc9e0 100644\n> --- a/Documentation/glossary-content.txt\n> +++ b/Documentation/glossary-content.txt\n> @@ -362,6 +362,11 @@ glob;;\n>  \tFor example, \"Documentation/{asterisk}.html\" matches\n>  \t\"Documentation/git.html\" but not \"Documentation/ppc/ppc.html\"\n>  \tor \"tools/perf/Documentation/perf.html\".\n> +\n> +label:<white space separated list>;;\n> +\tLabels can be assigned to pathspecs in the .gitattributes file.\n> +\tBy specifying a list of labels the pattern will match only\n> +\tfiles which have all of the listed labels.\n>  +\n>  Two consecutive asterisks (\"`**`\") in patterns matched against\n>  full pathname may have special meaning:\n\nI think the new text is stuffed in the middle of the \"glob;;\" entry.\n"},{"id":"286421","messageId":"xmqqbn4abmak.fsf@gitster.mtv.corp.google.com","threadId":"42297","inReplyTo":"20160513001936.7623-4-sbeller@google.com","subject":"Re: [PATCH 3/4] pathspec: move prefix check out of the inner loop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-13T04:43:15Z","receivedAt":"2016-05-13T04:43:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The prefix check is not related the check of pathspec magic; also there\n> is 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\n> loop.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nWhat were we thinking back when we added this in the middle of the\nloop at 233c3e6c (parse_pathspec: preserve prefix length via\nPATHSPEC_PREFIX_ORIGIN, 2013-07-14)?  I am a bit embarrassed.\n\n>  pathspec.c | 19 ++++++++++---------\n>  1 file changed, 10 insertions(+), 9 deletions(-)\n>\n> diff --git a/pathspec.c b/pathspec.c\n> index 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"},{"id":"286424","messageId":"CAGZ79kaqDzrJDDdrr4OmH0nO4nSnB3ZLKC3FU-sLt5doHAhmhQ@mail.gmail.com","threadId":"42297","inReplyTo":"xmqqfutmbmsv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4] pathspec: record labels","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-13T05:26:00Z","receivedAt":"2016-05-13T05:26:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 12, 2016 at 9:32 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\n>> index 8ad29e6..a1fc9e0 100644\n>> --- a/Documentation/glossary-content.txt\n>> +++ b/Documentation/glossary-content.txt\n>> @@ -362,6 +362,11 @@ glob;;\n>>       For example, \"Documentation/{asterisk}.html\" matches\n>>       \"Documentation/git.html\" but not \"Documentation/ppc/ppc.html\"\n>>       or \"tools/perf/Documentation/perf.html\".\n>> +\n>> +label:<white space separated list>;;\n>> +     Labels can be assigned to pathspecs in the .gitattributes file.\n>> +     By specifying a list of labels the pattern will match only\n>> +     files which have all of the listed labels.\n>>  +\n>>  Two consecutive asterisks (\"`**`\") in patterns matched against\n>>  full pathname may have special meaning:\n>\n> I think the new text is stuffed in the middle of the \"glob;;\" entry.\n\nIt is. Will fix in a reroll.\n\nThanks,\nStefan\n"},{"id":"286425","messageId":"xmqqy47ea5ql.fsf@gitster.mtv.corp.google.com","threadId":"42297","inReplyTo":"20160513001936.7623-5-sbeller@google.com","subject":"Re: [PATCH 4/4] pathspec: record labels","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-13T05:26:10Z","receivedAt":"2016-05-13T05:26:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> +\n> +label:<white space separated list>;;\n> +\tLabels can be assigned to pathspecs in the .gitattributes file.\n> +\tBy specifying a list of labels the pattern will match only\n> +\tfiles which have all of the listed labels.\n>  +\n\nAttributes are given to paths, not pathspecs.  You specify which paths\nthe entry applies to by matching the pattern (which is at the beginning\nof the line) against each path, and take the matching entries.\n\nIn any case, what the first sentence tries to explain applies to\neach and every attribute .gitattributes file can define, and\nexplaining it should be better left for the first sentence in the\nDESCRIPTION section.\n\nAs to the second sentence, I would say \"When specifying the paths to\nwork on to various Git commands, the :(label=<label>,...)  magic\npathspec can be used to select paths that have all the labels given\nby the 'label' attribute.\", or something like that, to clarify where\nthat \"specifying\" and \"match\" happens (they do not happen here, but\nhappen when using magic pathspec).\n\n> +void load_labels(const char *name, int namelen, struct string_list *list)\n\nThis must be file scope static, no?\n\n> +{\n> +\tstatic struct git_attr *attr;\n> +\tstruct git_attr_check check;\n> +\tchar *path = xmemdupz(name, namelen);\n> +\n> +\tif (!attr)\n> +\t\tattr = git_attr(\"label\");\n> +\tcheck.attr = attr;\n> +\n> +\tif (git_check_attr(path, 1, &check))\n> +\t\tdie(\"git_check_attr died\");\n> +\n> +\tif (ATTR_CUSTOM(check.value)) {\n> +\t\tstring_list_split(list, check.value, ',', -1);\n> +\t\tstring_list_sort(list);\n> +\t}\n> +\n> +\tfree(path);\n> +}\n\nI am wondering where we should sanity check (and die, pointing out\nan error in .gitattributes file).  Is this function a good place to\nreject TRUE and FALSE?\n\nBy the way, ATTR_CUSTOM() is probably a bad name.  gitattributes.txt\ncalls them Set, Unset, Set to a value and Unspecified, and this is\ntrying to single out \"Set to a value\" case.  ATTR_STRING() may be\nmore appropriate.\n\n> @@ -263,6 +285,15 @@ 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->group) {\n> +\t\tstruct string_list has_labels = STRING_LIST_INIT_DUP;\n> +\t\tstruct string_list_item *si;\n> +\t\tload_labels(name, namelen, &has_labels);\n> +\t\tfor_each_string_list_item(si, item->group)\n> +\t\t\tif (!string_list_has_string(&has_labels, si->string))\n> +\t\t\t\treturn 0;\n> +\t}\n> +\n\nIs this the right place, before we even check if the prefix already\ncovered everything?\n\nWe are looking at one element in the pathspec here.  If that element\nis known to be a \"group\", and the path has all the required labels,\nis it correct to fall through?\n\n    Ahh, you are making \":(label=...)makefile\" to say \"paths must\n    match the string 'makefile' in some way, and further the paths\n    must have all these labels?  Then falling through is correct.\n    The placement before the \"prefix covered\" looks still a bit\n    strange, but understandable.\n\nIs this code leaking has_labels every time it is called?\n\nBy the way, we should pick one between label and group and stick to\nit, no?  Perhaps call the new field \"item->label\"?\n\n>  \t/* If the match was just the prefix, we matched */\n>  \tif (!*match)\n>  \t\treturn MATCHED_RECURSIVELY;\n> diff --git a/pathspec.c b/pathspec.c\n> index 4dff252..c227c25 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -94,6 +94,7 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n>  {\n>  \tint i;\n>  \tconst char *copyfrom = *copyfrom_;\n> +\tconst char *out;\n\nIt probably is meant as \"output\", but it looks more like the \"body\" or\nthe \"contents\" of the named magic to me.\n\n>  \t/* longhand */\n>  \tconst char *nextat;\n>  \tfor (copyfrom = elt + 2;\n> @@ -117,6 +118,20 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n>  \t\t\tcontinue;\n>  \t\t}\n>  \n> +\t\tif (skip_prefix(copyfrom, \"label:\", &out)) {\n\nThis is good, but can you fix the \"prefix:\" one we see a few lines\nabove, too, using this to lose \"copyfrom + 7\" there?\n"},{"id":"286426","messageId":"CAGZ79kbaqw9UvYrCWcLtAXaiw5-aSev9gn858zz5Ju0MVfzepQ@mail.gmail.com","threadId":"42297","inReplyTo":"xmqqy47ea5ql.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4] pathspec: record labels","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-13T05:41:13Z","receivedAt":"2016-05-13T05:41:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 12, 2016 at 10:26 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> +\n>> +label:<white space separated list>;;\n>> +     Labels can be assigned to pathspecs in the .gitattributes file.\n>> +     By specifying a list of labels the pattern will match only\n>> +     files which have all of the listed labels.\n>>  +\n>\n> Attributes are given to paths, not pathspecs.  You specify which paths\n> the entry applies to by matching the pattern (which is at the beginning\n> of the line) against each path, and take the matching entries.\n>\n> In any case, what the first sentence tries to explain applies to\n> each and every attribute .gitattributes file can define, and\n> explaining it should be better left for the first sentence in the\n> DESCRIPTION section.\n>\n> As to the second sentence, I would say \"When specifying the paths to\n> work on to various Git commands, the :(label=<label>,...)  magic\n> pathspec can be used to select paths that have all the labels given\n> by the 'label' attribute.\", or something like that, to clarify where\n> that \"specifying\" and \"match\" happens (they do not happen here, but\n> happen when using magic pathspec).\n\nReminder for myself: add a test for\n\n    \":(label=a) :(exclude,label=b)\"\n\n>\n>> +void load_labels(const char *name, int namelen, struct string_list *list)\n>\n> This must be file scope static, no?\n\nYes. It seems like I forget these statics often. :(\n\n>\n>> +{\n>> +     static struct git_attr *attr;\n>> +     struct git_attr_check check;\n>> +     char *path = xmemdupz(name, namelen);\n>> +\n>> +     if (!attr)\n>> +             attr = git_attr(\"label\");\n>> +     check.attr = attr;\n>> +\n>> +     if (git_check_attr(path, 1, &check))\n>> +             die(\"git_check_attr died\");\n>> +\n>> +     if (ATTR_CUSTOM(check.value)) {\n>> +             string_list_split(list, check.value, ',', -1);\n>> +             string_list_sort(list);\n>> +     }\n>> +\n>> +     free(path);\n>> +}\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\nWould it make sense to mark a file as\n\n    \"follows the labeling system, but has no label\" (TRUE)\n    \"doesn't follow the labeling system at all\" (FALSE)\n\nThis may be a useful addition later on to say\n\n    \"I want to talk only about labeled files, but not label 'a'\"\n\nI would think.\n\n\n>\n> By the way, ATTR_CUSTOM() is probably a bad name.  gitattributes.txt\n> calls them Set, Unset, Set to a value and Unspecified, and this is\n> trying to single out \"Set to a value\" case.  ATTR_STRING() may be\n> more appropriate.\n\nOk.\n\n>\n>> @@ -263,6 +285,15 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,\n>>           strncmp(item->match, name - prefix, item->prefix))\n>>               return 0;\n>>\n>> +     if (item->group) {\n>> +             struct string_list has_labels = STRING_LIST_INIT_DUP;\n>> +             struct string_list_item *si;\n>> +             load_labels(name, namelen, &has_labels);\n>> +             for_each_string_list_item(si, item->group)\n>> +                     if (!string_list_has_string(&has_labels, si->string))\n>> +                             return 0;\n>> +     }\n>> +\n>\n> Is this the right place, before we even check if the prefix already\n> covered everything?\n>\n> We are looking at one element in the pathspec here.  If that element\n> is known to be a \"group\", and the path has all the required labels,\n> is it correct to fall through?\n>\n>     Ahh, you are making \":(label=...)makefile\" to say \"paths must\n>     match the string 'makefile' in some way, and further the paths\n>     must have all these labels?  Then falling through is correct.\n\nThis is how I understood your initial design idea.\n\n    :(label=C_code)contrib/\n\ngives all the retired C programs.\n\n>     The placement before the \"prefix covered\" looks still a bit\n>     strange, but understandable.\n>\n> Is this code leaking has_labels every time it is called?\n\nIt does.\n\n>\n> By the way, we should pick one between label and group and stick to\n> it, no?  Perhaps call the new field \"item->label\"?\n\nwill do.\n\n>\n>>       /* If the match was just the prefix, we matched */\n>>       if (!*match)\n>>               return MATCHED_RECURSIVELY;\n>> diff --git a/pathspec.c b/pathspec.c\n>> index 4dff252..c227c25 100644\n>> --- a/pathspec.c\n>> +++ b/pathspec.c\n>> @@ -94,6 +94,7 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n>>  {\n>>       int i;\n>>       const char *copyfrom = *copyfrom_;\n>> +     const char *out;\n>\n> It probably is meant as \"output\", but it looks more like the \"body\" or\n> the \"contents\" of the named magic to me.\n\nok.\n\n>\n>>       /* longhand */\n>>       const char *nextat;\n>>       for (copyfrom = elt + 2;\n>> @@ -117,6 +118,20 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n>>                       continue;\n>>               }\n>>\n>> +             if (skip_prefix(copyfrom, \"label:\", &out)) {\n>\n> This is good, but can you fix the \"prefix:\" one we see a few lines\n> above, too, using this to lose \"copyfrom + 7\" there?\n\nI can do that. Though I did not want to clutter this patch with it.\n\nI wonder that you focus on the details already, but not on the grand\ndesign of things. \"Is it actually a sane thing I am proposing here?\"\nThough you may be biased as the the high level idea came from\nyou. :)\n\nOne of the things I switched last minute and tried to address in the\ncover letter is the semantics of ORing or ANDing the labels given\nwithin one pathspec item.\n\nThanks,\nStefan\n"},{"id":"286429","messageId":"xmqqtwi2a2v6.fsf@gitster.mtv.corp.google.com","threadId":"42297","inReplyTo":"CAGZ79kbaqw9UvYrCWcLtAXaiw5-aSev9gn858zz5Ju0MVfzepQ@mail.gmail.com","subject":"Re: [PATCH 4/4] pathspec: record labels","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-13T06:28:13Z","receivedAt":"2016-05-13T06:28:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Would it make sense to mark a file as\n>\n>     \"follows the labeling system, but has no label\" (TRUE)\n>     \"doesn't follow the labeling system at all\" (FALSE)\n\nIsn't the former be \"label=\"\n\nI do not know what you mean by the latter.  I would understand\n\"pretend this has all the labels under the sun\", though.\n\n>>     Ahh, you are making \":(label=...)makefile\" to say \"paths must\n>>     match the string 'makefile' in some way, and further the paths\n>>     must have all these labels?  Then falling through is correct.\n>\n> This is how I understood your initial design idea.\n>\n>     :(label=C_code)contrib/\n>\n> gives all the retired C programs.\n\nOK, you probably meant s/C program/shell script/ with a different\nlabel, but I think I got the idea.\n\n> I wonder that you focus on the details already, but not on the grand\n> design of things. \"Is it actually a sane thing I am proposing here?\"\n> Though you may be biased as the the high level idea came from\n> you. :)\n\nBiased is the primary reason ;-) I trust that other reviewers can\nstop me and correct course when I veer off the deep end by myself.\n\nJust like you called this as a \"mock\", I am treating this as a\ntestbed to let you try out the \"defaultGroup\" thing to see how\nflexibly you can express common wishes the end users have, i.e.\n\"does it give us an expressive enough system to replace repo?\"\nis the question I want this code to help answering.  And that is\nwhy I was trying to make sure it is good enough quickly.\n\n> One of the things I switched last minute and tried to address in\n> the cover letter is the semantics of ORing or ANDing the labels\n> given within one pathspec item.\n\nThat is something we may find out that the other way is more useful,\nor your final choice is better, by seeing how easy to express common\npatterns of submodule selections.  Perhaps we might end up wanting\nboth, but as you said, OR can be given by listing the same path with\ndiffernt required-labels so I think what you have is good enough for\nus to start experimenting with the higher layer.\n\nOne worry I have is that historically pathspec matching and\nattribute matching have both been very hard to make it perform well.\nThe placement of string_list_has_string() and load_labels() at the\nleaf level of the pathspec matching logic cannot be helped, but\nthese calls might turn out to be an unacceptable performance\nbottleneck.  But it is a bit too early to worry about that, I think.\n"},{"id":"286569","messageId":"CACsJy8BK-u2VV3kkq3ANHCanYqMwphqgxZmooQfewA_J7e8MPw@mail.gmail.com","threadId":"42297","inReplyTo":"20160513001936.7623-1-sbeller@google.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-15T10:06:22Z","receivedAt":"2016-05-15T10:06:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 13, 2016 at 7:19 AM, Stefan Beller <sbeller@google.com> wrote:\n> After some fruitful discussion[1] in which Junio suggested trying a very\n> different route[2] that is more general and not submodule related, I considered\n> doing a mock for this.\n>\n> This lets you label arbitrary pathspecs, e.g. in git.git we may want to have:\n>\n>     t/t[0-9]*.sh label=tests\n>\n> such that\n>\n>     $ git log --author=Beller \":(label=tests)\"\n>\n> would show all commits in which I touched tests.\n>\n> This has suprisingly few lines of code, as the first 3 patches are refactoring.\n> The actual new feature is in the last patch.\n>\n> This would solve the submodule issues I want to solve, as I can produce a\n> .gitattributes like:\n>\n>     ./submodule1 label=default\n>     ./submodule2 label=default\n>     ./submodule3 label=optional-feature1\n>\n> and then I'd instruct the users to clone like this:\n>\n>     git clone .. superproject\n>     cd superproject\n>     git submodule update --init :(label:default)\n>\n> The second part of the submodule series to collapse these three commands\n> will come as an extra series later, then.\n\nYeah. I can see the use of this, even without submodules.\n\n> What annoys me here:\n> Attributes can only be set once at the moment, there is no way to collect all\n> attributes. If we'd have such a collection feature we would be able to\n> configure this:\n>\n>     *.[ch] label=C,code\n>     contrib/** label=oldstuff\n\nInstead of putting everything in under the same attribute name\n\"label\", make one attribute per label? Would this work?\n\n*.[ch] c-group code-group\n\nAnd the pathspec magic name can be simply \"attr\", any git attribute\ncan be used with it, e.g. :(attr:c-group)\n\n> and run this:\n>\n>     git status \":(label:C oldstuff)\"\n>\n> which would be the equivalent to\n>\n>     contrib/**.[ch]\n>\n> as in this proposed implementation the labels which are given within one\n> pathspec item are logical AND. To get the logical OR, you'd have to give multiple\n> pathspec items, i.e. \":(label:C)\" \":(label:oldstuff)\"\n\nIt's even better if pathspec does not have to deal with combination\nlogic. Can attr macros deal with that (or if it can't at the moment,\ncan we improve it)?\n-- \nDuy\n"},{"id":"286596","messageId":"xmqq60uf6v5d.fsf@gitster.mtv.corp.google.com","threadId":"42297","inReplyTo":"CACsJy8BK-u2VV3kkq3ANHCanYqMwphqgxZmooQfewA_J7e8MPw@mail.gmail.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-15T18:19:58Z","receivedAt":"2016-05-15T18:19:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> Instead of putting everything in under the same attribute name\n> \"label\", make one attribute per label? Would this work?\n>\n> *.[ch] c-group code-group\n\nThe attribute subsystem expects that there will not be unbounded\nlarge number of attributes, so this is not a good direction to go.\n"},{"id":"286603","messageId":"xmqqfutj5d73.fsf@gitster.mtv.corp.google.com","threadId":"42297","inReplyTo":"xmqq60uf6v5d.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-15T19:33:04Z","receivedAt":"2016-05-15T19:33:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> Instead of putting everything in under the same attribute name\n>> \"label\", make one attribute per label? Would this work?\n>>\n>> *.[ch] c-group code-group\n>\n> The attribute subsystem expects that there will not be unbounded\n> large number of attributes, so this is not a good direction to go.\n\nHaving said that, I do not mind too much if it turns out that it is\nnecessary to use an unbounded set of random attributes to solve a\nspecific problem, if the use case is good.\n\nBut even then, in order to avoid confusion and name clashes, I'd\nprefer to see more like\n\n\t*.[ch] group-c group-code\n\nthat is matched by\n\n\tgit cmd ':(group:c code)\n\ni.e. reserve a single prefix that is not and will not be used for\nother purposes.\n"},{"id":"286618","messageId":"CACsJy8BU-zym6Nkmbs-jk66af1LwHaasybHBvBUJscU=eBOkQA@mail.gmail.com","threadId":"42297","inReplyTo":"xmqqfutj5d73.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-16T00:03:50Z","receivedAt":"2016-05-16T00:03:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, May 16, 2016 at 2:33 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Duy Nguyen <pclouds@gmail.com> writes:\n>>\n>>> Instead of putting everything in under the same attribute name\n>>> \"label\", make one attribute per label? Would this work?\n>>>\n>>> *.[ch] c-group code-group\n>>\n>> The attribute subsystem expects that there will not be unbounded\n>> large number of attributes, so this is not a good direction to go.\n>\n> Having said that, I do not mind too much if it turns out that it is\n> necessary to use an unbounded set of random attributes to solve a\n> specific problem, if the use case is good.\n\nI only have a vague idea so far, it seems to me a good idea to be able\nto specify \"binary-marked paths\" or \"filter attached paths\" from\npathspec. We can already do this with scripts, but we need to be very\ncareful about quoting. And if it's pathspec it can be combined with\nother magic (most likely negation)\n\n> But even then, in order to avoid confusion and name clashes, I'd\n> prefer to see more like\n>\n>         *.[ch] group-c group-code\n>\n> that is matched by\n>\n>         git cmd ':(group:c code)\n>\n> i.e. reserve a single prefix that is not and will not be used for\n> other purposes.\n\nFor my above use case, i can still define macro group-binary that is\nan alias of binary to get around this. So it's ok.\n-- \nDuy\n"},{"id":"286680","messageId":"CAGZ79kacKnBjzVFSShRido4rKa3-Zg465d2320M6w1qXEkOZqw@mail.gmail.com","threadId":"42297","inReplyTo":"CACsJy8BK-u2VV3kkq3ANHCanYqMwphqgxZmooQfewA_J7e8MPw@mail.gmail.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-16T17:20:52Z","receivedAt":"2016-05-16T17:20:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, May 15, 2016 at 3:06 AM, Duy Nguyen <pclouds@gmail.com> wrote:\n> Instead of putting everything in under the same attribute name\n> \"label\", make one attribute per label? Would this work?\n>\n> *.[ch] c-group code-group\n>\n> And the pathspec magic name can be simply \"attr\", any git attribute\n> can be used with it, e.g. :(attr:c-group)\n\nSo you want to be able to query something like:\n\n    git ls-files \":(crlf=auto)\"\n\nas well?\n"},{"id":"286683","messageId":"xmqqh9dx3nsf.fsf@gitster.mtv.corp.google.com","threadId":"42297","inReplyTo":"CAGZ79kacKnBjzVFSShRido4rKa3-Zg465d2320M6w1qXEkOZqw@mail.gmail.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T17:39:28Z","receivedAt":"2016-05-16T17:39:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Sun, May 15, 2016 at 3:06 AM, Duy Nguyen <pclouds@gmail.com> wrote:\n>> Instead of putting everything in under the same attribute name\n>> \"label\", make one attribute per label? Would this work?\n>>\n>> *.[ch] c-group code-group\n>>\n>> And the pathspec magic name can be simply \"attr\", any git attribute\n>> can be used with it, e.g. :(attr:c-group)\n>\n> So you want to be able to query something like:\n>\n>     git ls-files \":(crlf=auto)\"\n>\n> as well?\n\nIt would be more like\n\n\tgit ls-files \":(attr:crlf=auto)\"\n\nIt certainly sounds tempting, even though I do not want to keep what\nthis initial chunk of the series needs to do to the minimum.\n"},{"id":"286685","messageId":"CAGZ79kZyPvAFG1TjSm76LuDLpch=7iuL_-8LYqysSESFQj-YjQ@mail.gmail.com","threadId":"42297","inReplyTo":"xmqqh9dx3nsf.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-16T17:48:40Z","receivedAt":"2016-05-16T17:48:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 16, 2016 at 10:39 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> On Sun, May 15, 2016 at 3:06 AM, Duy Nguyen <pclouds@gmail.com> wrote:\n>>> Instead of putting everything in under the same attribute name\n>>> \"label\", make one attribute per label? Would this work?\n>>>\n>>> *.[ch] c-group code-group\n>>>\n>>> And the pathspec magic name can be simply \"attr\", any git attribute\n>>> can be used with it, e.g. :(attr:c-group)\n>>\n>> So you want to be able to query something like:\n>>\n>>     git ls-files \":(crlf=auto)\"\n>>\n>> as well?\n>\n> It would be more like\n>\n>         git ls-files \":(attr:crlf=auto)\"\n>\n> It certainly sounds tempting, even though I do not want to keep what\n> this initial chunk of the series needs to do to the minimum.\n\nThis is another case for using ':' instead of '='.\nSo I think ':' is better for this future enhancement.\n\nAlso this future enhancement may ask for\n\n      git ls-files \":(attr:label=foo)\"\n\nor\n\n      git ls-files \":(attr:-label)\"\n\nso the user can circumvent the warn and ignore strategy. :(\n"},{"id":"286720","messageId":"xmqqlh391z2v.fsf@gitster.mtv.corp.google.com","threadId":"42297","inReplyTo":"CAGZ79kZyPvAFG1TjSm76LuDLpch=7iuL_-8LYqysSESFQj-YjQ@mail.gmail.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T21:18:32Z","receivedAt":"2016-05-16T21:18:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This is another case for using ':' instead of '='.\n> So I think ':' is better for this future enhancement.\n>\n> Also this future enhancement may ask for\n>\n>       git ls-files \":(attr:label=foo)\"\n>\n> or\n>\n>       git ls-files \":(attr:-label)\"\n>\n> so the user can circumvent the warn and ignore strategy. :(\n\nThat is exactly what we want, I would say, if we want to accept\n\"arbitrary set of attributes with their states\".\n\nThe \"warn and ignore\" comes only from \"with '(:label=X Y Z)', we\ninspect attributes X, Y and Z and insist them to be set to true; it\nis a configuration error to set the label to anything but a string\",\nand accepting \"arbitrary set of attributes with their states\" makes\nit moot (as I said earlier in $gmane/294776).\n\nSo are we leaning towards \":(attr:<state>)\", where <state> is one of\n\n    var=value\n    var\n    -var\n    !var\n\nnow?  It makes the pathspec magic parser a bit more complex (and I\nam not sure if we have to worry too much about quoting \"value\" part\nto allow arbitrary sequence of bytes), though.\n"},{"id":"286722","messageId":"CAGZ79kZuMdWdw2kcfs_P-_Ax1zXpKNHqMjNL80hBj3=SSp2zSQ@mail.gmail.com","threadId":"42297","inReplyTo":"xmqqlh391z2v.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-16T21:36:28Z","receivedAt":"2016-05-16T21:36:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 16, 2016 at 2:18 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> This is another case for using ':' instead of '='.\n>> So I think ':' is better for this future enhancement.\n>>\n>> Also this future enhancement may ask for\n>>\n>>       git ls-files \":(attr:label=foo)\"\n>>\n>> or\n>>\n>>       git ls-files \":(attr:-label)\"\n>>\n>> so the user can circumvent the warn and ignore strategy. :(\n>\n> That is exactly what we want, I would say, if we want to accept\n> \"arbitrary set of attributes with their states\".\n>\n> The \"warn and ignore\" comes only from \"with '(:label=X Y Z)', we\n> inspect attributes X, Y and Z and insist them to be set to true; it\n> is a configuration error to set the label to anything but a string\",\n> and accepting \"arbitrary set of attributes with their states\" makes\n> it moot (as I said earlier in $gmane/294776).\n>\n> So are we leaning towards \":(attr:<state>)\", where <state> is one of\n>\n>     var=value\n>     var\n>     -var\n>     !var\n>\n> now?  It makes the pathspec magic parser a bit more complex (and I\n> am not sure if we have to worry too much about quoting \"value\" part\n> to allow arbitrary sequence of bytes), though.\n\nAnd we want to have both \"label=A B C\" and attr:label=A B C\" or *just* the\nattr query?\n\nWe should not allow the user to add arbitrary attributes (i.e. labels).\nInstead each of the \"attr for labeling purposes\" needs to follow a clear scheme\nthat allows us to add attributes later on that are outside of that scheme.\n\nIn a very first implementation we could require the attribute to start with\n\"label=\". ;)\n"},{"id":"286723","messageId":"xmqqh9dx1xku.fsf@gitster.mtv.corp.google.com","threadId":"42297","inReplyTo":"CAGZ79kZuMdWdw2kcfs_P-_Ax1zXpKNHqMjNL80hBj3=SSp2zSQ@mail.gmail.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T21:50:57Z","receivedAt":"2016-05-16T21:50:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> And we want to have both \"label=A B C\" and attr:label=A B C\" or *just* the\n> attr query?\n\nI think the choice at this point is between supporting just \"label=A\nB C\" or supporting just \"attr:eol=crlf text=auto !diff\".\n\nI think \"attr:label=A\" is merely a degenerated case of the latter.\n\n> We should not allow the user to add arbitrary attributes (i.e. labels).\n\nHmph, why not?\n\n> Instead each of the \"attr for labeling purposes\" needs to follow a clear scheme\n> that allows us to add attributes later on that are outside of that scheme.\n\nThat was my initial reaction when I saw Duy's \"attr:crlf=auto\" (by\nthe way, there is no such setting; crlf should be one of TRUE, UNSET\nor set to string \"input\") idea.  But I do not think of a good\nargument to justify that arbitrary attributes are not allowed.\n"},{"id":"286725","messageId":"CAGZ79kbJxf6qnj1vqkbFzXBU+6Cfh6RMvs2NqqeASZAPLFXu8Q@mail.gmail.com","threadId":"42297","inReplyTo":"xmqqh9dx1xku.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-16T22:00:09Z","receivedAt":"2016-05-16T22:00:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 16, 2016 at 2:50 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> And we want to have both \"label=A B C\" and attr:label=A B C\" or *just* the\n>> attr query?\n>\n> I think the choice at this point is between supporting just \"label=A\n> B C\" or supporting just \"attr:eol=crlf text=auto !diff\".\n>\n> I think \"attr:label=A\" is merely a degenerated case of the latter.\n>\n>> We should not allow the user to add arbitrary attributes (i.e. labels).\n\nBecause we cannot extend our attributes any further from that?\n\nConsider a user starts using attr <foo> for their labeling purposes.\nLater (in 2 years) we decide that <foo> is an attribute we want to\nuse to mark files as foo-ish. so we add meaning to that attribute\n(just like eol.crlf does an arbitrary thing, foo would do another arbitrary\nthing).\n\nThen the user picks up the new version of Git and expects <foo> to\nstill be a \"I use it for labeling purposes only\" thing. They would not\nexpect to all files labeled as <foo> to start behaving <foo-ish> ?\n\n> Hmph, why not?\n\nWe need a namespace for which\n* we can guarantee that it is for labeling purposes only (even in the future)\n* is obvious to the user to be a labeling name space\n\nStarting with \"label\" offers both?\n\n>\n>> Instead each of the \"attr for labeling purposes\" needs to follow a clear scheme\n>> that allows us to add attributes later on that are outside of that scheme.\n>\n> That was my initial reaction when I saw Duy's \"attr:crlf=auto\" (by\n> the way, there is no such setting; crlf should be one of TRUE, UNSET\n> or set to string \"input\") idea.  But I do not think of a good\n> argument to justify that arbitrary attributes are not allowed.\n\nI agree that querying for attr:eol or attr:diff is a good idea.\n\nI do however want to point out that all labels for \"labeling purposes\"\nMUST be a clear from the beginning, otherwise we'll have the maintenance\nproblem down the road?\n"},{"id":"286726","messageId":"xmqq8tz91x1b.fsf@gitster.mtv.corp.google.com","threadId":"42297","inReplyTo":"CAGZ79kbJxf6qnj1vqkbFzXBU+6Cfh6RMvs2NqqeASZAPLFXu8Q@mail.gmail.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T22:02:40Z","receivedAt":"2016-05-16T22:02:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> Hmph, why not?\n>\n> We need a namespace for which\n> * we can guarantee that it is for labeling purposes only (even in the future)\n> * is obvious to the user to be a labeling name space\n>\n> Starting with \"label\" offers both?\n\nAh, of course.  I thought that you were trying to limit \":(attr:<attribute>)\"\nmagic only to attributes that begin with \"label-\", which is where my\n\"why not?\" comes from.\n"},{"id":"286727","messageId":"CAGZ79kaP7rPu64ORXrGcONyoM2jA+gfPpy3LFkbp0Axff9dsHg@mail.gmail.com","threadId":"42297","inReplyTo":"xmqq8tz91x1b.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-16T22:09:53Z","receivedAt":"2016-05-16T22:09:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 16, 2016 at 3:02 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>> Hmph, why not?\n>>\n>> We need a namespace for which\n>> * we can guarantee that it is for labeling purposes only (even in the future)\n>> * is obvious to the user to be a labeling name space\n>>\n>> Starting with \"label\" offers both?\n>\n> Ah, of course.  I thought that you were trying to limit \":(attr:<attribute>)\"\n> magic only to attributes that begin with \"label-\", which is where my\n> \"why not?\" comes from.\n\nAnd going by the logic you presented before, we would\nneed to error out for the given pathspec \":(<string>)\" if\n\n* either the string is not well known (e.g. diif, eol )\n* or is outside of the labeling namespace.\n\nSo we don't want to see users complaining about\n\"bug attr:foo worked as a label, now it is a feature; you broke my code\"\n\nWe would need to ignore data from .gitattributes as it may be crafted from\na newer version of Git, but the command line argument still needs to die\nfor unknown arguments?\n\nSo asking for :(foo) would yield a\n\n    fatal: attr 'foo' is not known to Git, nor is it in the labeling name space\n\nI guess what I am asking is if there is a nice way to query \"do we know\nthis attribute?\"\n\nStefan\n"},{"id":"286729","messageId":"xmqqzirpzlvd.fsf@gitster.mtv.corp.google.com","threadId":"42297","inReplyTo":"CAGZ79kaP7rPu64ORXrGcONyoM2jA+gfPpy3LFkbp0Axff9dsHg@mail.gmail.com","subject":"Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T22:19:50Z","receivedAt":"2016-05-16T22:19:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> Ah, of course.  I thought that you were trying to limit \":(attr:<attribute>)\"\n>> magic only to attributes that begin with \"label-\", which is where my\n>> \"why not?\" comes from.\n>\n> And going by the logic you presented before, we would\n> need to error out for the given pathspec \":(<string>)\" if\n>\n> * either the string is not well known (e.g. diif, eol )\n> * or is outside of the labeling namespace.\n\nI do not think that follows _my_ line of thought.  What is \"well\nknown\"?  Doesn't that change over time?\n\nIf we are to do the \"attribute match\", there is no useful\nenforcement point.  An arbitrary version of Git cannot differentiate\na random cruft users will write in their .gitattributes from a\nlegitimate attribute that will be introduced in the future, so both\n\"data in .gitattributes\" and \"pathspec magic that referes to attribute\"\ncannot be limited by us.\n\nSo if we are going to take the arbitrary \":(attr:<attribute>)\"\nroute, \"label-\" prefix would be limitation on the \"core Git\" side\nand does not limit what end-user does.  We will promise that we\nwon't use names that begin with the prefix ourselves and leave them\nup to the projects.  If the end user uses an attribute \"foo\", which\ndoes not begin with \"label-\", the end user is risking to be broken\nby future versions of Git.\n\nIf you want to compile an authoritative list of attributes used by\ncore Git and maintain it forever, you are welcome to add warning,\nbut I wouldn't bother if I were doing this series, at least at the\nbeginning.\n"}]}