{"thread":{"id":"42379","subject":"[PATCHv8 1/5] string list: improve comment","startedAt":"2016-05-19T01:09:30Z","lastAt":"2016-05-24T02:00:14Z","messageCount":22,"participants":["Stefan Beller","Junio C Hamano","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"286956","messageId":"20160519010935.27856-1-sbeller@google.com","threadId":"42379","inReplyTo":null,"subject":"[PATCHv8 0/5] pathspec magic extension to search for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T01:09:30Z","receivedAt":"2016-05-19T01:09:30Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This goes on top of origin/jc/attr, (396bf756f95, attr: expose validity check\nfor attribute names)\n\nPatches 1 and 2 are small fixes, which could go independently as well.\nPatches 3 and 4 are refactoring pathspec.c a little.\nThese did not change since v7\n\n\ndiff to v7:\ndiff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\nindex 866e8d8..aa9f220 100644\n--- a/Documentation/glossary-content.txt\n+++ b/Documentation/glossary-content.txt\n@@ -394,11 +394,9 @@ you can query each attribute for certain states. The \"`[mode]`\" is a special\n character to indicate which attribute states are looked for. The following\n modes are available:\n\n- - \"`+`\" the attribute must be set\n+ - an empty \"`[mode]`\" matches if the attribute is set\n - \"`-`\" the attribute must be unset\n- - \"`~`\" the attribute must be unspecified\n- - \"`?`\" the attribute must not be unspecified, i.e. set, unset or value matches\n- - an empty \"`[mode]`\" matches if the attribute is set or has a value\n+ - \"`!`\" the attribute must be unspecified\n - an empty \"`[mode]`\" combined with \"`[=value]`\" matches if the attribute has\n the given value.\n +\ndiff --git a/dir.c b/dir.c\nindex 3141a5a..f60a503 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -219,42 +219,31 @@ int within_depth(const char *name, int namelen,\n static int match_attrs(const char *name, int namelen,\n \t\t const struct pathspec_item *item)\n {\n-\tchar *path;\n \tint i;\n\n-\tpath = xmemdupz(name, namelen);\n-\tgit_check_attr(path, item->attr_check);\n-\n+\tgit_check_attr_counted(name, namelen, item->attr_check);\n \tfor (i = 0; i < item->attr_match_nr; i++) {\n \t\tconst char *value;\n \t\tint matched;\n \t\tenum attr_match_mode match_mode;\n\n \t\tvalue = item->attr_check->check[i].value;\n-\n \t\tmatch_mode = item->attr_match[i].match_mode;\n\n-\t\tif (ATTR_TRUE(value)) {\n-\t\t\tmatched = match_mode == MATCH_SET ||\n-\t\t\t\t match_mode == MATCH_SET_OR_VALUE ||\n-\t\t\t\t match_mode == MATCH_NOT_UNSPECIFIED;\n-\t\t} else if (ATTR_FALSE(value)) {\n-\t\t\tmatched = match_mode == MATCH_UNSET ||\n-\t\t\t\t match_mode == MATCH_NOT_UNSPECIFIED;\n-\t\t} else if (ATTR_UNSET(value)) {\n+\t\tif (ATTR_TRUE(value))\n+\t\t\tmatched = match_mode == MATCH_SET;\n+\t\telse if (ATTR_FALSE(value))\n+\t\t\tmatched = match_mode == MATCH_UNSET;\n+\t\telse if (ATTR_UNSET(value))\n \t\t\tmatched = match_mode == MATCH_UNSPECIFIED;\n-\t\t} else {\n-\t\t\tmatched = match_mode == MATCH_NOT_UNSPECIFIED ||\n-\t\t\t\t match_mode == MATCH_SET_OR_VALUE ||\n-\t\t\t\t (match_mode == MATCH_VALUE &&\n+\t\telse\n+\t\t\tmatched = (match_mode == MATCH_VALUE &&\n \t\t\t\t !strcmp(item->attr_match[i].value, value));\n-\t\t}\n+\n \t\tif (!matched)\n \t\t\treturn 0;\n \t}\n\n-\tfree(path);\n-\n \treturn 1;\n }\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 32fb6a8..b795a9c 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -96,66 +96,58 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va\n\n\n \tif (!value || !strlen(value))\n-\t\tgoto err;\n+\t\tdie(_(\"attr spec must not be empty\"));\n\n \tstring_list_split(&list, value, ' ', -1);\n \tstring_list_remove_empty_items(&list, 0);\n\n \tif (!item->attr_check)\n \t\titem->attr_check = git_attr_check_alloc();\n+\telse\n+\t\tdie(_(\"Only one 'attr:' specification is allowed.\"));\n\n \tALLOC_GROW(item->attr_match, item->attr_match_nr + list.nr, item->attr_match_alloc);\n\n \tfor_each_string_list_item(si, &list) {\n-\t\tsize_t val_len;\n+\t\tsize_t attr_len;\n\n \t\tint j = item->attr_match_nr++;\n-\t\tconst char *val = si->string;\n+\t\tconst char *attr = si->string;\n \t\tstruct attr_match *am = &item->attr_match[j];\n\n-\t\tif (val[0] == '?')\n-\t\t\tam->match_mode = MATCH_NOT_UNSPECIFIED;\n-\t\telse if (val[0] == '~')\n+\t\tif (attr[0] == '!')\n \t\t\tam->match_mode = MATCH_UNSPECIFIED;\n-\t\telse if (val[0] == '+')\n-\t\t\tam->match_mode = MATCH_SET;\n-\t\telse if (val[0] == '-')\n+\t\telse if (attr[0] == '-')\n \t\t\tam->match_mode = MATCH_UNSET;\n \t\telse\n-\t\t\tam->match_mode = MATCH_SET_OR_VALUE;\n+\t\t\tam->match_mode = MATCH_SET;\n\n-\t\tif (am->match_mode != MATCH_SET_OR_VALUE)\n+\t\tif (am->match_mode != MATCH_SET)\n \t\t\t/* skip first character */\n-\t\t\tval++;\n+\t\t\tattr++;\n\n-\t\tval_len = strcspn(val, \"=,)\");\n-\t\tif (val[val_len] == '=') {\n+\t\tattr_len = strcspn(attr, \"=\");\n+\t\tif (attr[attr_len] == '=') {\n \t\t\tam->match_mode = MATCH_VALUE;\n-\t\t\tam->value = xstrdup(&val[val_len + 1]);\n-\t\t\t/*\n-\t\t\t * NEEDSWORK:\n-\t\t\t * Do we want to allow escaped commas to search\n-\t\t\t * for comma separated values?\n-\t\t\t */\n+\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n \t\t\tif (strchr(am->value, '\\\\'))\n \t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n \t\t} else\n \t\t\tam->value = NULL;\n\n-\t\tif (invalid_attr_name(val, val_len)) {\n+\t\tif (!attr_name_valid(attr, attr_len)) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n \t\t\tam->match_mode = INVALID_ATTR;\n-\t\t\tgoto err;\n+\t\t\tinvalid_attr_name_message(&sb, attr, attr_len);\n+\t\t\tdie(_(\"invalid attribute in '%s': '%s'\"), value, sb.buf);\n \t\t}\n\n-\t\tam->attr = git_attr(xmemdupz(val, val_len));\n+\t\tam->attr = git_attr_counted(attr, attr_len);\n \t\tgit_attr_check_append(item->attr_check, am->attr);\n \t}\n\n \tstring_list_clear(&list, 0);\n \treturn;\n-err:\n-\tdie(_(\"attr spec '%s': attrs must not start with '-' and \"\n-\t \"be composed of [-A-Za-z0-9_.].\"), value);\n }\n\n static void eat_long_magic(struct pathspec_item *item, const char *elt,\n@@ -188,9 +180,9 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \t\t}\n\n \t\tif (skip_prefix(copyfrom, \"attr:\", &body)) {\n-\t\t\tchar *pass = xmemdupz(body, len - strlen(\"attr:\"));\n-\t\t\tparse_pathspec_attr_match(item, pass);\n-\t\t\tfree(pass);\n+\t\t\tchar *attr_body = xmemdupz(body, len - strlen(\"attr:\"));\n+\t\t\tparse_pathspec_attr_match(item, attr_body);\n+\t\t\tfree(attr_body);\n \t\t\tcontinue;\n \t\t}\n\n@@ -591,6 +583,8 @@ void free_pathspec(struct pathspec *pathspec)\n {\n \tint i, j;\n \tfor (i = 0; i < pathspec->nr; i++) {\n+\t\tif (!pathspec->items[i].attr_match_nr)\n+\t\t\tcontinue;\n \t\tfor (j = 0; j < pathspec->items[j].attr_match_nr; j++)\n \t\t\tfree(pathspec->items[i].attr_match[j].value);\n \t\tfree(pathspec->items[i].attr_match);\ndiff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh\nindex 35b3ab2..c0d8cda 100755\n--- a/t/t6134-pathspec-with-labels.sh\n+++ b/t/t6134-pathspec-with-labels.sh\n@@ -74,7 +74,7 @@ test_expect_success 'check specific set attr' '\n fileSetLabel\n sub/fileSetLabel\n EOF\n-\tgit ls-files \":(attr:+label)\" >actual &&\n+\tgit ls-files \":(attr:label)\" >actual &&\n \ttest_cmp expect actual\n '\n\n@@ -98,52 +98,41 @@ EOF\n \ttest_must_be_empty actual\n '\n\n-test_expect_success 'check set or value attr' '\n-\tcat <expect &&\n-fileSetLabel\n-fileValue\n-sub/fileSetLabel\n-sub/fileValue\n-EOF\n-\tgit ls-files \":(attr:label)\" >actual &&\n-\ttest_cmp expect actual\n-'\n-\n test_expect_success 'check unspecified attr' '\n \tcat <expect &&\n .gitattributes\n+fileA\n+fileAB\n+fileAC\n+fileB\n+fileBC\n fileC\n fileNoLabel\n fileWrongLabel\n+sub/fileA\n+sub/fileAB\n+sub/fileAC\n+sub/fileB\n+sub/fileBC\n sub/fileC\n sub/fileNoLabel\n sub/fileWrongLabel\n EOF\n-\tgit ls-files \":(attr:~label,attr:~labelA,attr:~labelB)\" >actual &&\n+\tgit ls-files :\\(attr:\\!label\\) >actual &&\n \ttest_cmp expect actual\n '\n\n-test_expect_success 'check not unspecified attr' '\n+test_expect_success 'check multiple unspecified attr' '\n \tcat <expect &&\n-fileSetLabel\n-fileUnsetLabel\n-fileValue\n-sub/fileSetLabel\n-sub/fileUnsetLabel\n-sub/fileValue\n-EOF\n-\tgit ls-files \":(attr:?label)\" >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'check label with 2 labels' '\n-\tcat <expect &&\n-fileAB\n-sub/fileAB\n+.gitattributes\n+fileC\n+fileNoLabel\n+fileWrongLabel\n+sub/fileC\n+sub/fileNoLabel\n+sub/fileWrongLabel\n EOF\n-\tgit ls-files \":(attr:labelA labelB)\" >actual &&\n-\ttest_cmp expect actual &&\n-\tgit ls-files \":(attr:labelA,attr:labelB)\" >actual &&\n+\tgit ls-files :\\(attr:\\!labelB\\ \\!labelA\\ \\!label\\) >actual &&\n \ttest_cmp expect actual\n '\n"},{"id":"286953","messageId":"20160519010935.27856-2-sbeller@google.com","threadId":"42379","inReplyTo":"20160519010935.27856-1-sbeller@google.com","subject":"[PATCHv8 1/5] string list: improve comment","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T01:09:31Z","receivedAt":"2016-05-19T01:09:31Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n string-list.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/string-list.h b/string-list.h\nindex d3809a1..465a1f0 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -106,7 +106,7 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_\n * list->strdup_strings must be set, as new memory needs to be\n * allocated to hold the substrings. If maxsplit is non-negative,\n * then split at most maxsplit times. Return the number of substrings\n- * appended to list.\n+ * appended to list. The list may be non-empty already.\n *\n * Examples:\n * string_list_split(l, \"foo:bar:baz\", ':', -1) -> [\"foo\", \"bar\", \"baz\"]\n-- \n2.8.2.123.g3bde101\n"},{"id":"286954","messageId":"20160519010935.27856-3-sbeller@google.com","threadId":"42379","inReplyTo":"20160519010935.27856-1-sbeller@google.com","subject":"[PATCHv8 2/5] Documentation: fix a typo","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T01:09:32Z","receivedAt":"2016-05-19T01:09:32Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n Documentation/gitattributes.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex e3b1de8..af2c682 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -86,7 +86,7 @@ is either not set or empty, $HOME/.config/git/attributes is used instead.\n Attributes for all users on a system should be placed in the\n `$(prefix)/etc/gitattributes` file.\n \n-Sometimes you would need to override an setting of an attribute\n+Sometimes you would need to override a setting of an attribute\n for a path to `Unspecified` state. This can be done by listing\n the name of the attribute prefixed with an exclamation point `!`.\n \n-- \n2.8.2.123.g3bde101\n"},{"id":"286955","messageId":"20160519010935.27856-4-sbeller@google.com","threadId":"42379","inReplyTo":"20160519010935.27856-1-sbeller@google.com","subject":"[PATCHv8 3/5] pathspec: move long magic parsing out of prefix_pathspec","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T01:09:33Z","receivedAt":"2016-05-19T01:09:33Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`prefix_pathspec` is quite a lengthy function and we plan on adding more.\nSplit it up for better readability. As we want to add code into the\ninner loop of the long magic parsing, we also benefit from lower\nindentation.\n\nSigned-off-by: Stefan Beller \n---\n pathspec.c | 84 +++++++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 47 insertions(+), 37 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex c9e9b6c..eba37c2 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -88,6 +88,52 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+static void eat_long_magic(struct pathspec_item *item, const char *elt,\n+\t\tunsigned *magic, int *pathspec_prefix,\n+\t\tconst char **copyfrom_, const char **long_magic_end)\n+{\n+\tint i;\n+\tconst char *copyfrom = *copyfrom_;\n+\t/* longhand */\n+\tconst char *nextat;\n+\tfor (copyfrom = elt + 2;\n+\t *copyfrom && *copyfrom != ')';\n+\t copyfrom = nextat) {\n+\t\tsize_t len = strcspn(copyfrom, \",)\");\n+\t\tif (copyfrom[len] == ',')\n+\t\t\tnextat = copyfrom + len + 1;\n+\t\telse\n+\t\t\t/* handle ')' and '\\0' */\n+\t\t\tnextat = copyfrom + len;\n+\t\tif (!len)\n+\t\t\tcontinue;\n+\t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n+\t\t\tif (strlen(pathspec_magic[i].name) == len &&\n+\t\t\t !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n+\t\t\t\t*magic |= pathspec_magic[i].bit;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tif (starts_with(copyfrom, \"prefix:\")) {\n+\t\t\t\tchar *endptr;\n+\t\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n+\t\t\t\t\t\t\t &endptr, 10);\n+\t\t\t\tif (endptr - copyfrom != len)\n+\t\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n+\t\t\t\t/* \"i\" would be wrong, but it does not matter */\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (ARRAY_SIZE(pathspec_magic) <= i)\n+\t\t\tdie(_(\"Invalid pathspec magic '%.*s' in '%s'\"),\n+\t\t\t (int) len, copyfrom, elt);\n+\t}\n+\tif (*copyfrom != ')')\n+\t\tdie(_(\"Missing ')' at the end of pathspec magic in '%s'\"), elt);\n+\t*long_magic_end = copyfrom;\n+\tcopyfrom++;\n+\t*copyfrom_ = copyfrom;\n+}\n+\n /*\n * Take an element of a pathspec and check for magic signatures.\n * Append the result to the prefix. Return the magic bitmap.\n@@ -150,43 +196,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \t (flags & PATHSPEC_LITERAL_PATH)) {\n \t\t; /* nothing to do */\n \t} else if (elt[1] == '(') {\n-\t\t/* longhand */\n-\t\tconst char *nextat;\n-\t\tfor (copyfrom = elt + 2;\n-\t\t *copyfrom && *copyfrom != ')';\n-\t\t copyfrom = nextat) {\n-\t\t\tsize_t len = strcspn(copyfrom, \",)\");\n-\t\t\tif (copyfrom[len] == ',')\n-\t\t\t\tnextat = copyfrom + len + 1;\n-\t\t\telse\n-\t\t\t\t/* handle ')' and '\\0' */\n-\t\t\t\tnextat = copyfrom + len;\n-\t\t\tif (!len)\n-\t\t\t\tcontinue;\n-\t\t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n-\t\t\t\tif (strlen(pathspec_magic[i].name) == len &&\n-\t\t\t\t !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n-\t\t\t\t\tmagic |= pathspec_magic[i].bit;\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t\tif (starts_with(copyfrom, \"prefix:\")) {\n-\t\t\t\t\tchar *endptr;\n-\t\t\t\t\tpathspec_prefix = strtol(copyfrom + 7,\n-\t\t\t\t\t\t\t\t &endptr, 10);\n-\t\t\t\t\tif (endptr - copyfrom != len)\n-\t\t\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n-\t\t\t\t\t/* \"i\" would be wrong, but it does not matter */\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t\tif (ARRAY_SIZE(pathspec_magic) <= i)\n-\t\t\t\tdie(_(\"Invalid pathspec magic '%.*s' in '%s'\"),\n-\t\t\t\t (int) len, copyfrom, elt);\n-\t\t}\n-\t\tif (*copyfrom != ')')\n-\t\t\tdie(_(\"Missing ')' at the end of pathspec magic in '%s'\"), elt);\n-\t\tlong_magic_end = copyfrom;\n-\t\tcopyfrom++;\n+\t\teat_long_magic(item, elt, &magic, &pathspec_prefix, ©from, &long_magic_end);\n \t} else {\n \t\t/* shorthand */\n \t\tfor (copyfrom = elt + 1;\n-- \n2.8.2.123.g3bde101\n"},{"id":"286957","messageId":"20160519010935.27856-5-sbeller@google.com","threadId":"42379","inReplyTo":"20160519010935.27856-1-sbeller@google.com","subject":"[PATCHv8 4/5] pathspec: move prefix check out of the inner loop","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T01:09:34Z","receivedAt":"2016-05-19T01:09:34Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The prefix check is not related the check of pathspec magic; also there\nis no code that is relevant after we'd break the loop on a match for\n\"prefix:\". So move the check before the loop and shortcircuit the outer\nloop.\n\nSigned-off-by: Stefan Beller \n---\n pathspec.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex eba37c2..4dff252 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -107,21 +107,22 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \t\t\tnextat = copyfrom + len;\n \t\tif (!len)\n \t\t\tcontinue;\n+\n+\t\tif (starts_with(copyfrom, \"prefix:\")) {\n+\t\t\tchar *endptr;\n+\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n+\t\t\t\t\t\t &endptr, 10);\n+\t\t\tif (endptr - copyfrom != len)\n+\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n \t\t\tif (strlen(pathspec_magic[i].name) == len &&\n \t\t\t !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n \t\t\t\t*magic |= pathspec_magic[i].bit;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tif (starts_with(copyfrom, \"prefix:\")) {\n-\t\t\t\tchar *endptr;\n-\t\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n-\t\t\t\t\t\t\t &endptr, 10);\n-\t\t\t\tif (endptr - copyfrom != len)\n-\t\t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n-\t\t\t\t/* \"i\" would be wrong, but it does not matter */\n-\t\t\t\tbreak;\n-\t\t\t}\n \t\t}\n \t\tif (ARRAY_SIZE(pathspec_magic) <= i)\n \t\t\tdie(_(\"Invalid pathspec magic '%.*s' in '%s'\"),\n-- \n2.8.2.123.g3bde101\n"},{"id":"286958","messageId":"20160519010935.27856-6-sbeller@google.com","threadId":"42379","inReplyTo":"20160519010935.27856-1-sbeller@google.com","subject":"[PATCHv8 5/5] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T01:09:35Z","receivedAt":"2016-05-19T01:09:35Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The pathspec mechanism is extended via the new\n\":(attr:eol=input)pattern/to/match\" syntax to filter paths so that it\nrequires paths to not just match the given pattern but also have the\nspecified attrs attached for them to be chosen.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/glossary-content.txt | 17 ++++\n dir.c | 35 ++++++++\n pathspec.c | 97 +++++++++++++++++++++-\n pathspec.h | 16 ++++\n t/t6134-pathspec-with-labels.sh | 166 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 327 insertions(+), 4 deletions(-)\n create mode 100755 t/t6134-pathspec-with-labels.sh\n\ndiff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\nindex cafc284..aa9f220 100644\n--- a/Documentation/glossary-content.txt\n+++ b/Documentation/glossary-content.txt\n@@ -384,6 +384,23 @@ full pathname may have special meaning:\n +\n Glob magic is incompatible with literal magic.\n \n+attr;;\n+\tAdditionally to matching the pathspec, the path must have the\n+\tattribute as specified. The syntax for specifying the required\n+\tattributes is \"`attr: [mode] [=value]`\"\n++\n+Attributes can have 4 states (Set, Unset, Set to a value, unspecified) and\n+you can query each attribute for certain states. The \"`[mode]`\" is a special\n+character to indicate which attribute states are looked for. The following\n+modes are available:\n+\n+ - an empty \"`[mode]`\" matches if the attribute is set\n+ - \"`-`\" the attribute must be unset\n+ - \"`!`\" the attribute must be unspecified\n+ - an empty \"`[mode]`\" combined with \"`[=value]`\" matches if the attribute has\n+ the given value.\n++\n+\n exclude;;\n \tAfter a path matches any non-exclude pathspec, it will be run\n \tthrough all exclude pathspec (magic signature: `!`). If it\ndiff --git a/dir.c b/dir.c\nindex 996653b..f60a503 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -9,6 +9,7 @@\n */\n #include \"cache.h\"\n #include \"dir.h\"\n+#include \"attr.h\"\n #include \"refs.h\"\n #include \"wildmatch.h\"\n #include \"pathspec.h\"\n@@ -215,6 +216,37 @@ int within_depth(const char *name, int namelen,\n \treturn 1;\n }\n \n+static int match_attrs(const char *name, int namelen,\n+\t\t const struct pathspec_item *item)\n+{\n+\tint i;\n+\n+\tgit_check_attr_counted(name, namelen, item->attr_check);\n+\tfor (i = 0; i < item->attr_match_nr; i++) {\n+\t\tconst char *value;\n+\t\tint matched;\n+\t\tenum attr_match_mode match_mode;\n+\n+\t\tvalue = item->attr_check->check[i].value;\n+\t\tmatch_mode = item->attr_match[i].match_mode;\n+\n+\t\tif (ATTR_TRUE(value))\n+\t\t\tmatched = match_mode == MATCH_SET;\n+\t\telse if (ATTR_FALSE(value))\n+\t\t\tmatched = match_mode == MATCH_UNSET;\n+\t\telse if (ATTR_UNSET(value))\n+\t\t\tmatched = match_mode == MATCH_UNSPECIFIED;\n+\t\telse\n+\t\t\tmatched = (match_mode == MATCH_VALUE &&\n+\t\t\t\t !strcmp(item->attr_match[i].value, value));\n+\n+\t\tif (!matched)\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n #define DO_MATCH_EXCLUDE 1\n #define DO_MATCH_DIRECTORY 2\n \n@@ -270,6 +302,9 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,\n \t strncmp(item->match, name - prefix, item->prefix))\n \t\treturn 0;\n \n+\tif (item->attr_match_nr && !match_attrs(name, namelen, item))\n+\t\treturn 0;\n+\n \t/* If the match was just the prefix, we matched */\n \tif (!*match)\n \t\treturn MATCHED_RECURSIVELY;\ndiff --git a/pathspec.c b/pathspec.c\nindex 4dff252..b795a9c 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"dir.h\"\n #include \"pathspec.h\"\n+#include \"attr.h\"\n \n /*\n * Finds which of the given pathspecs match items in the index.\n@@ -88,12 +89,74 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)\n+{\n+\tstruct string_list_item *si;\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\n+\n+\tif (!value || !strlen(value))\n+\t\tdie(_(\"attr spec must not be empty\"));\n+\n+\tstring_list_split(&list, value, ' ', -1);\n+\tstring_list_remove_empty_items(&list, 0);\n+\n+\tif (!item->attr_check)\n+\t\titem->attr_check = git_attr_check_alloc();\n+\telse\n+\t\tdie(_(\"Only one 'attr:' specification is allowed.\"));\n+\n+\tALLOC_GROW(item->attr_match, item->attr_match_nr + list.nr, item->attr_match_alloc);\n+\n+\tfor_each_string_list_item(si, &list) {\n+\t\tsize_t attr_len;\n+\n+\t\tint j = item->attr_match_nr++;\n+\t\tconst char *attr = si->string;\n+\t\tstruct attr_match *am = &item->attr_match[j];\n+\n+\t\tif (attr[0] == '!')\n+\t\t\tam->match_mode = MATCH_UNSPECIFIED;\n+\t\telse if (attr[0] == '-')\n+\t\t\tam->match_mode = MATCH_UNSET;\n+\t\telse\n+\t\t\tam->match_mode = MATCH_SET;\n+\n+\t\tif (am->match_mode != MATCH_SET)\n+\t\t\t/* skip first character */\n+\t\t\tattr++;\n+\n+\t\tattr_len = strcspn(attr, \"=\");\n+\t\tif (attr[attr_len] == '=') {\n+\t\t\tam->match_mode = MATCH_VALUE;\n+\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n+\t\t\tif (strchr(am->value, '\\\\'))\n+\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n+\t\t} else\n+\t\t\tam->value = NULL;\n+\n+\t\tif (!attr_name_valid(attr, attr_len)) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tam->match_mode = INVALID_ATTR;\n+\t\t\tinvalid_attr_name_message(&sb, attr, attr_len);\n+\t\t\tdie(_(\"invalid attribute in '%s': '%s'\"), value, sb.buf);\n+\t\t}\n+\n+\t\tam->attr = git_attr_counted(attr, attr_len);\n+\t\tgit_attr_check_append(item->attr_check, am->attr);\n+\t}\n+\n+\tstring_list_clear(&list, 0);\n+\treturn;\n+}\n+\n static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \t\tunsigned *magic, int *pathspec_prefix,\n \t\tconst char **copyfrom_, const char **long_magic_end)\n {\n \tint i;\n \tconst char *copyfrom = *copyfrom_;\n+\tconst char *body;\n \t/* longhand */\n \tconst char *nextat;\n \tfor (copyfrom = elt + 2;\n@@ -108,15 +171,21 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \t\tif (!len)\n \t\t\tcontinue;\n \n-\t\tif (starts_with(copyfrom, \"prefix:\")) {\n+\t\tif (skip_prefix(copyfrom, \"prefix:\", &body)) {\n \t\t\tchar *endptr;\n-\t\t\t*pathspec_prefix = strtol(copyfrom + 7,\n-\t\t\t\t\t\t &endptr, 10);\n+\t\t\t*pathspec_prefix = strtol(body, &endptr, 10);\n \t\t\tif (endptr - copyfrom != len)\n \t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (skip_prefix(copyfrom, \"attr:\", &body)) {\n+\t\t\tchar *attr_body = xmemdupz(body, len - strlen(\"attr:\"));\n+\t\t\tparse_pathspec_attr_match(item, attr_body);\n+\t\t\tfree(attr_body);\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n \t\t\tif (strlen(pathspec_magic[i].name) == len &&\n \t\t\t !strncmp(pathspec_magic[i].name, copyfrom, len)) {\n@@ -425,7 +494,10 @@ void parse_pathspec(struct pathspec *pathspec,\n \tfor (i = 0; i < n; i++) {\n \t\tunsigned short_magic;\n \t\tentry = argv[i];\n-\n+\t\titem[i].attr_check = NULL;\n+\t\titem[i].attr_match = NULL;\n+\t\titem[i].attr_match_nr = 0;\n+\t\titem[i].attr_match_alloc = 0;\n \t\titem[i].magic = prefix_pathspec(item + i, &short_magic,\n \t\t\t\t\t\targv + i, flags,\n \t\t\t\t\t\tprefix, prefixlen, entry);\n@@ -447,6 +519,13 @@ void parse_pathspec(struct pathspec *pathspec,\n \t\tif (item[i].nowildcard_len < item[i].len)\n \t\t\tpathspec->has_wildcard = 1;\n \t\tpathspec->magic |= item[i].magic;\n+\n+\t\tif (item[i].attr_match_nr) {\n+\t\t\tint j;\n+\t\t\tfor (j = 0; j < item[i].attr_match_nr; j++)\n+\t\t\t\tif (item[i].attr_match[j].match_mode == INVALID_ATTR)\n+\t\t\t\t\tdie(_(\"attribute spec in the wrong syntax are prohibited.\"));\n+\t\t}\n \t}\n \n \tif (nr_exclude == n)\n@@ -502,6 +581,16 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n \n void free_pathspec(struct pathspec *pathspec)\n {\n+\tint i, j;\n+\tfor (i = 0; i < pathspec->nr; i++) {\n+\t\tif (!pathspec->items[i].attr_match_nr)\n+\t\t\tcontinue;\n+\t\tfor (j = 0; j < pathspec->items[j].attr_match_nr; j++)\n+\t\t\tfree(pathspec->items[i].attr_match[j].value);\n+\t\tfree(pathspec->items[i].attr_match);\n+\t\tgit_attr_check_free(pathspec->items[i].attr_check);\n+\t}\n+\n \tfree(pathspec->items);\n \tpathspec->items = NULL;\n }\ndiff --git a/pathspec.h b/pathspec.h\nindex 0c11262..5308137 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -32,6 +32,22 @@ struct pathspec {\n \t\tint len, prefix;\n \t\tint nowildcard_len;\n \t\tint flags;\n+\t\tint attr_match_nr;\n+\t\tint attr_match_alloc;\n+\t\tstruct attr_match {\n+\t\t\tstruct git_attr *attr;\n+\t\t\tchar *value;\n+\t\t\tenum attr_match_mode {\n+\t\t\t\tMATCH_SET,\n+\t\t\t\tMATCH_UNSET,\n+\t\t\t\tMATCH_VALUE,\n+\t\t\t\tMATCH_UNSPECIFIED,\n+\t\t\t\tMATCH_NOT_UNSPECIFIED,\n+\t\t\t\tMATCH_SET_OR_VALUE,\n+\t\t\t\tINVALID_ATTR\n+\t\t\t} match_mode;\n+\t\t} *attr_match;\n+\t\tstruct git_attr_check *attr_check;\n \t} *items;\n };\n \ndiff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh\nnew file mode 100755\nindex 0000000..c0d8cda\n--- /dev/null\n+++ b/t/t6134-pathspec-with-labels.sh\n@@ -0,0 +1,166 @@\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 fileValue fileWrongLabel; do\n+\t\t: >$p &&\n+\t\tgit add $p &&\n+\t\t: >sub/$p\n+\t\tgit add sub/$p\n+\tdone &&\n+\tgit commit -m $p &&\n+\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+fileValue\n+fileWrongLabel\n+sub/fileA\n+sub/fileAB\n+sub/fileAC\n+sub/fileB\n+sub/fileBC\n+sub/fileC\n+sub/fileNoLabel\n+sub/fileSetLabel\n+sub/fileUnsetLabel\n+sub/fileValue\n+sub/fileWrongLabel\n+EOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pathspec with no attr' '\n+\ttest_must_fail git ls-files \":(attr:)\" 2>actual &&\n+\ttest_i18ngrep fatal actual\n+'\n+\n+test_expect_success 'pathspec with labels and non existent .gitattributes' '\n+\tgit ls-files \":(attr:label)\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'setup .gitattributes' '\n+\tcat <.gitattributes &&\n+fileA labelA\n+fileB labelB\n+fileC labelC\n+fileAB labelA labelB\n+fileAC labelA labelC\n+fileBC labelB labelC\n+fileUnsetLabel -label\n+fileSetLabel label\n+fileValue label=foo\n+fileWrongLabel label☺\n+EOF\n+\tgit add .gitattributes &&\n+\tgit commit -m \"add attributes\"\n+'\n+\n+sq=\"'\"\n+\n+test_expect_success 'check specific set attr' '\n+\tcat <expect &&\n+fileSetLabel\n+sub/fileSetLabel\n+EOF\n+\tgit ls-files \":(attr:label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check specific unset attr' '\n+\tcat <expect &&\n+fileUnsetLabel\n+sub/fileUnsetLabel\n+EOF\n+\tgit ls-files \":(attr:-label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check specific value attr' '\n+\tcat <expect &&\n+fileValue\n+sub/fileValue\n+EOF\n+\tgit ls-files \":(attr:label=foo)\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files \":(attr:label=bar)\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'check unspecified attr' '\n+\tcat <expect &&\n+.gitattributes\n+fileA\n+fileAB\n+fileAC\n+fileB\n+fileBC\n+fileC\n+fileNoLabel\n+fileWrongLabel\n+sub/fileA\n+sub/fileAB\n+sub/fileAC\n+sub/fileB\n+sub/fileBC\n+sub/fileC\n+sub/fileNoLabel\n+sub/fileWrongLabel\n+EOF\n+\tgit ls-files :\\(attr:\\!label\\) >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check multiple unspecified attr' '\n+\tcat <expect &&\n+.gitattributes\n+fileC\n+fileNoLabel\n+fileWrongLabel\n+sub/fileC\n+sub/fileNoLabel\n+sub/fileWrongLabel\n+EOF\n+\tgit ls-files :\\(attr:\\!labelB\\ \\!labelA\\ \\!label\\) >actual &&\n+\ttest_cmp expect 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+EOF\n+\tgit ls-files \":(attr:labelB)\" \":(exclude)sub/\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check label excluding other labels' '\n+\tcat <expect &&\n+fileAB\n+fileB\n+fileBC\n+sub/fileAB\n+sub/fileB\n+EOF\n+\tgit ls-files \":(attr:labelB)\" \":(exclude,attr:labelC)sub/\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'abort on giving invalid label on the command line' '\n+\ttest_must_fail git ls-files . \":(attr:☺)\" 2>actual &&\n+\ttest_i18ngrep \"fatal\" actual\n+'\n+\n+test_done\n-- \n2.8.2.123.g3bde101\n"},{"id":"287006","messageId":"xmqq60u9kjjh.fsf@gitster.mtv.corp.google.com","threadId":"42379","inReplyTo":"20160519010935.27856-2-sbeller@google.com","subject":"Re: [PATCHv8 1/5] string list: improve comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-19T18:08:18Z","receivedAt":"2016-05-19T18:08:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Signed-off-by: Stefan Beller \n> ---\n> string-list.h | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/string-list.h b/string-list.h\n> index d3809a1..465a1f0 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -106,7 +106,7 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_\n> * list->strdup_strings must be set, as new memory needs to be\n> * allocated to hold the substrings. If maxsplit is non-negative,\n> * then split at most maxsplit times. Return the number of substrings\n> - * appended to list.\n> + * appended to list. The list may be non-empty already.\n\nI personally find that the original comment is clear enough, though.\n\nWhen somebody says \"resulting elements of the split are appended to\nthe list\" without saying either:\n\n a. \"the list MUST be empty in the beginning\", or\n b. \"the list will be emptied first before the split result are appended\",\n\nwouldn't it be natural to take it as \"you can append them to any\nlist, be it empty or not, and they are _appended_, not replaced\"?\n\nSo while this is not incorrect per-se, I am not sure if it adds much\nvalue. If somebody needs this additional clarification, because the\noriginal did not say a. above, she would certainly need more\nclarification because the original did not say b. above, either.\n\n\"The list may be non-empty already\", but she would keep wondering if\nthe existing contents would be discarded before the result gets\nappended to it.\n\nYou may say \"No, there won't be such a confusion, because we say\n'append'; empty and then append is 'replace'\". But then the same\nlogic would say \"There cannot be a requirement for the list to be\nempty in the first place, because we say 'append'\".\n\nSo...\n"},{"id":"287007","messageId":"CAGZ79kZ9bsuBU+CKuw+kZ+f6JZEPHvYFM3R0rre51jChVxHAfQ@mail.gmail.com","threadId":"42379","inReplyTo":"xmqq60u9kjjh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv8 1/5] string list: improve comment","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T18:12:36Z","receivedAt":"2016-05-19T18:12:36Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 19, 2016 at 11:08 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> string-list.h | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/string-list.h b/string-list.h\n>> index d3809a1..465a1f0 100644\n>> --- a/string-list.h\n>> +++ b/string-list.h\n>> @@ -106,7 +106,7 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_\n>> * list->strdup_strings must be set, as new memory needs to be\n>> * allocated to hold the substrings. If maxsplit is non-negative,\n>> * then split at most maxsplit times. Return the number of substrings\n>> - * appended to list.\n>> + * appended to list. The list may be non-empty already.\n>\n> I personally find that the original comment is clear enough, though.\n>\n> When somebody says \"resulting elements of the split are appended to\n> the list\" without saying either:\n>\n> a. \"the list MUST be empty in the beginning\", or\n> b. \"the list will be emptied first before the split result are appended\",\n>\n> wouldn't it be natural to take it as \"you can append them to any\n> list, be it empty or not, and they are _appended_, not replaced\"?\n\nThat is true. I missed that though when reading the documentation and\nread the source code to be clear.\n\n>\n> So while this is not incorrect per-se, I am not sure if it adds much\n> value. If somebody needs this additional clarification, because the\n> original did not say a. above, she would certainly need more\n> clarification because the original did not say b. above, either.\n>\n> \"The list may be non-empty already\", but she would keep wondering if\n> the existing contents would be discarded before the result gets\n> appended to it.\n>\n> You may say \"No, there won't be such a confusion, because we say\n> 'append'; empty and then append is 'replace'\". But then the same\n> logic would say \"There cannot be a requirement for the list to be\n> empty in the first place, because we say 'append'\".\n>\n> So...\n\nSo, please drop?\n\nI do not feel strongly about this patch. I basically wrote to for myself\nafter I consulted the source.\n"},{"id":"287010","messageId":"xmqqmvnlj2vf.fsf@gitster.mtv.corp.google.com","threadId":"42379","inReplyTo":"20160519010935.27856-6-sbeller@google.com","subject":"Re: [PATCHv8 5/5] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-19T18:53:40Z","receivedAt":"2016-05-19T18:53:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\n> index cafc284..aa9f220 100644\n> --- a/Documentation/glossary-content.txt\n> +++ b/Documentation/glossary-content.txt\n> @@ -384,6 +384,23 @@ full pathname may have special meaning:\n> +\n> Glob magic is incompatible with literal magic.\n> \n> +attr;;\n> +\tAdditionally to matching the pathspec, the path must have the\n> +\tattribute as specified. The syntax for specifying the required\n> +\tattributes is \"`attr: [mode] [=value]`\"\n> ++\n> +Attributes can have 4 states (Set, Unset, Set to a value, unspecified) and\n> +you can query each attribute for certain states. The \"`[mode]`\" is a special\n> +character to indicate which attribute states are looked for. The following\n> +modes are available:\n> +\n> + - an empty \"`[mode]`\" matches if the attribute is set\n> + - \"`-`\" the attribute must be unset\n> + - \"`!`\" the attribute must be unspecified\n> + - an empty \"`[mode]`\" combined with \"`[=value]`\" matches if the attribute has\n> + the given value.\n> ++\n\nAs an initial design, I find this much more agreeable than the\nprevious rounds. I however find the phrasing of the above harder\nthan necessary to understand, for a few reasons.\n\n * Mixed use of \"X matches if ...\" and \"... must be Y\" makes it\n unclear if they are talking about different kind of things, or\n the same kind of things in merely different ways.\n\n * It does not make it clear \"=value\" is only meaningful when [mode]\n is empty.\n\nPerhaps dropping the '[mode]' thing altogether and instead saying\n\n After `attr:` comes a space separated list of \"attribute\n \trequirements\", all of which must be met in order for the\n \tpath to be considered a match; this is in addition to the\n \tusual non-magic pathspec pattern matching.\n\n\tEach of the attribute requirements for the path takes one of\n\tthese forms:\n\n\t- \"`ATTR`\" requires that the attribute `ATTR` must be set.\n\n\t- \"`-ATTR`\" requires that the attribute `ATTR` must be unset.\n\n - \"`ATTR=VALUE`\" requires that the attribute `ATTR` must be\n set to the string `VALUE`.\n\n - \"`!ATTR`\" requires that the attribute `ATTR` must be\n unspecified.\n\nwould make the resulting text easier to read?\n\n> +static int match_attrs(const char *name, int namelen,\n> +\t\t const struct pathspec_item *item)\n> +{\n> +\tint i;\n> +\n> +\tgit_check_attr_counted(name, namelen, item->attr_check);\n> +\tfor (i = 0; i < item->attr_match_nr; i++) {\n> +\t\tconst char *value;\n> +\t\tint matched;\n> +\t\tenum attr_match_mode match_mode;\n> +\n> +\t\tvalue = item->attr_check->check[i].value;\n> +\t\tmatch_mode = item->attr_match[i].match_mode;\n> +\n> +\t\tif (ATTR_TRUE(value))\n> +\t\t\tmatched = match_mode == MATCH_SET;\n> +\t\telse if (ATTR_FALSE(value))\n> +\t\t\tmatched = match_mode == MATCH_UNSET;\n> +\t\telse if (ATTR_UNSET(value))\n> +\t\t\tmatched = match_mode == MATCH_UNSPECIFIED;\n\nreadability nit:\n\n\tmatched = (match_mode == MATCH_WHATEVER);\n\nwould be easier to view\n\n> +\t\telse\n> +\t\t\tmatched = (match_mode == MATCH_VALUE &&\n> +\t\t\t\t !strcmp(item->attr_match[i].value, value));\n\nand would match the last case above better.\n\n> +static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)\n> +{\n> +\tstruct string_list_item *si;\n> +\tstruct string_list list = STRING_LIST_INIT_DUP;\n> +\n> +\n> +\tif (!value || !strlen(value))\n> +\t\tdie(_(\"attr spec must not be empty\"));\n> +\n> +\tstring_list_split(&list, value, ' ', -1);\n> +\tstring_list_remove_empty_items(&list, 0);\n> +\n> +\tif (!item->attr_check)\n> +\t\titem->attr_check = git_attr_check_alloc();\n> +\telse\n> +\t\tdie(_(\"Only one 'attr:' specification is allowed.\"));\n> +\n> +\tALLOC_GROW(item->attr_match, item->attr_match_nr + list.nr, item->attr_match_alloc);\n> +\n> +\tfor_each_string_list_item(si, &list) {\n> +\t\tsize_t attr_len;\n> +\n> +\t\tint j = item->attr_match_nr++;\n> +\t\tconst char *attr = si->string;\n> +\t\tstruct attr_match *am = &item->attr_match[j];\n> +\n> +\t\tif (attr[0] == '!')\n> +\t\t\tam->match_mode = MATCH_UNSPECIFIED;\n> +\t\telse if (attr[0] == '-')\n> +\t\t\tam->match_mode = MATCH_UNSET;\n> +\t\telse\n> +\t\t\tam->match_mode = MATCH_SET;\n> +\n> +\t\tif (am->match_mode != MATCH_SET)\n> +\t\t\t/* skip first character */\n> +\t\t\tattr++;\n> +\t\tattr_len = strcspn(attr, \"=\");\n> +\t\tif (attr[attr_len] == '=') {\n> +\t\t\tam->match_mode = MATCH_VALUE;\n> +\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n> +\t\t\tif (strchr(am->value, '\\\\'))\n> +\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n> +\t\t} else\n> +\t\t\tam->value = NULL;\n> +\n\nLet me try a variant to see if we can improve it (thinking aloud).\n\n\tswitch (*attr) {\n case '!':\n \tam->match_mode = MATCH_UNSPECIFIED;\n\t\tattr++;\n break;\n case '-':\n \tam->match_mode = MATCH_UNSET;\n\t\tattr++;\n break;\n\tdefault:\n\t\tattr_len = strcspn(attr, \"=\");\n\t\tif (attr[attr_len] != '=')\n\t\t\tam->match_mode = MATCH_SET;\n\t\telse {\n\t\t\tam->match_mode = MATCH_VALUE;\n\t am->value = xstrdup(...);\n\t\t}\n break;\n\t}\n\nUsing switch/case does not save line counts at all but it may still\nmake the result easier to follow. More importantly, it would help\nyou catch \"!ATTR=VAR\" as an \"invalid attribute name 'ATTR=VAR' was\nrequested\" error by arranging the code to make sure that scanning\nfor '=' would not happen in !/- case (in other words, while thiking\naloud with an alternative way to write the same thing, I spotted a\nbug in the original ;-).\n\n> +\t\tif (!attr_name_valid(attr, attr_len)) {\n> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\t\tam->match_mode = INVALID_ATTR;\n> +\t\t\tinvalid_attr_name_message(&sb, attr, attr_len);\n> +\t\t\tdie(_(\"invalid attribute in '%s': '%s'\"), value, sb.buf);\n> +\t\t}\n> +\n> +\t\tam->attr = git_attr_counted(attr, attr_len);\n\nI'd do this the other order, i.e.\n\n\tam->attr = git_attr_counted(...);\n if (!am->attr) {\n\t\t...\n die(...);\n\t}\n\nIt is wasteful to call attr_name_valid() yourself before seeing an\nerror from git_attr_counted().\n\n> diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh\n> new file mode 100755\n> index 0000000..c0d8cda\n> --- /dev/null\n> +++ b/t/t6134-pathspec-with-labels.sh\n> @@ -0,0 +1,166 @@\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 fileValue fileWrongLabel; do\n> +\t\t: >$p &&\n> +\t\tgit add $p &&\n> +\t\t: >sub/$p\n> +\t\tgit add sub/$p\n> +\tdone &&\n> +\tgit commit -m $p &&\n> +\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> +fileValue\n> +fileWrongLabel\n> +sub/fileA\n> +sub/fileAB\n> +sub/fileAC\n> +sub/fileB\n> +sub/fileBC\n> +sub/fileC\n> +sub/fileNoLabel\n> +sub/fileSetLabel\n> +sub/fileUnsetLabel\n> +sub/fileValue\n> +sub/fileWrongLabel\n> +EOF\n\n\tcat <<-\\EOF >expect &&\n fileA\n ...\n EOF\n\nto indent the whole thing?\n\n> +test_expect_success 'setup .gitattributes' '\n> +\tcat <.gitattributes &&\n\nLikewise.\n\n> +fileWrongLabel label☺\n> +EOF\n> +\tgit add .gitattributes &&\n> +\tgit commit -m \"add attributes\"\n> +'\n> +\n> +sq=\"'\"\n\nI do not think you use this $sq variable below, but I may have\noverlooked its use.\n\n> +\n> +test_expect_success 'check specific set attr' '\n> +\tcat <expect &&\n> +fileSetLabel\n> +sub/fileSetLabel\n> +EOF\n> +\tgit ls-files \":(attr:label)\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'check specific unset attr' '\n> +\tcat <expect &&\n> +fileUnsetLabel\n> +sub/fileUnsetLabel\n> +EOF\n> +\tgit ls-files \":(attr:-label)\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'check specific value attr' '\n> +\tcat <expect &&\n> +fileValue\n> +sub/fileValue\n> +EOF\n> +\tgit ls-files \":(attr:label=foo)\" >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit ls-files \":(attr:label=bar)\" >actual &&\n> +\ttest_must_be_empty actual\n> +'\n> +\n> +test_expect_success 'check unspecified attr' '\n> +\tcat <expect &&\n> +.gitattributes\n> +...\n> +EOF\n> +\tgit ls-files :\\(attr:\\!label\\) >actual &&\n\nWhy not the more normal-looking \":(attr:!label)\"? I do not think\nhistory substitution would trigger in an script.\n\nAnd no, \"I may cut and paste into my interactive shell while\ndebugging\" is not a good reason to make the end-result (i.e. script)\nless readable.\n\n> +test_expect_success 'check multiple unspecified attr' '\n> +\tcat <expect &&\n> +.gitattributes\n> +fileC\n> +fileNoLabel\n> +fileWrongLabel\n> +sub/fileC\n> +sub/fileNoLabel\n> +sub/fileWrongLabel\n> +EOF\n> +\tgit ls-files :\\(attr:\\!labelB\\ \\!labelA\\ \\!label\\) >actual &&\n> +\ttest_cmp expect 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> +EOF\n> +\tgit ls-files \":(attr:labelB)\" \":(exclude)sub/\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'check label excluding other labels' '\n> +\tcat <expect &&\n> +fileAB\n> +fileB\n> +fileBC\n> +sub/fileAB\n> +sub/fileB\n> +EOF\n> +\tgit ls-files \":(attr:labelB)\" \":(exclude,attr:labelC)sub/\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'abort on giving invalid label on the command line' '\n> +\ttest_must_fail git ls-files . \":(attr:☺)\" 2>actual &&\n> +\ttest_i18ngrep \"fatal\" actual\n> +'\n> +\n> +test_done\n"},{"id":"287011","messageId":"xmqqiny9j2sc.fsf@gitster.mtv.corp.google.com","threadId":"42379","inReplyTo":"20160519010935.27856-1-sbeller@google.com","subject":"Re: [PATCHv8 0/5] pathspec magic extension to search for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-19T18:55:31Z","receivedAt":"2016-05-19T18:55:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I think this round is 99% there. The next step would be to answer\n\"does the feature set we have here meet your needs that you wanted\nto fill with the submodule labels originally?\" and I am hoping it is\n\"yes\".\n\nThanks.\n"},{"id":"287014","messageId":"xmqqa8jlj0tn.fsf@gitster.mtv.corp.google.com","threadId":"42379","inReplyTo":"20160519010935.27856-6-sbeller@google.com","subject":"Re: [PATCHv8 5/5] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-19T19:37:56Z","receivedAt":"2016-05-19T19:37:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +test_expect_success 'setup a tree' '\n> +\tmkdir sub &&\n> +\tfor p in fileA fileB fileC fileAB fileAC fileBC fileNoLabel fileUnsetLabel fileSetLabel fileValue fileWrongLabel; do\n> +\t\t: >$p &&\n> +\t\tgit add $p &&\n> +\t\t: >sub/$p\n> +\t\tgit add sub/$p\n> +\tdone &&\n> +\tgit commit -m $p &&\n\nWhat does this $p refer to?\n\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> +fileValue\n> +fileWrongLabel\n> +sub/fileA\n> +sub/fileAB\n> +sub/fileAC\n> +sub/fileB\n> +sub/fileBC\n> +sub/fileC\n> +sub/fileNoLabel\n> +sub/fileSetLabel\n> +sub/fileUnsetLabel\n> +sub/fileValue\n> +sub/fileWrongLabel\n> +EOF\n> +\ttest_cmp expect actual\n> +'\n\nIf I were doing this, I'd prepare the list of paths (i.e. expect)\nfirst and then create these paths using that list, i.e.\n\ntest_expect_success 'setup a tree' '\n\tcat <<-\\EOF >expect &&\n\tfileA\n\tfileAB\n\t...\n\tsub/fileWrongLabel\n\tEOF\n\tmkdir sub &&\n\twhile read path\n\tdo\n\t\t: >$path &&\n\t\tgit add $path || return 1\n\tdone actual &&\n\ttest_cmp expect actual\n'\n"},{"id":"287020","messageId":"CAGZ79kaZ=0XdHEkocP-LpHT2+ybHd44KCbfONzYj77UTEPb0FA@mail.gmail.com","threadId":"42379","inReplyTo":"xmqqmvnlj2vf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv8 5/5] pathspec: allow querying for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T20:42:19Z","receivedAt":"2016-05-19T20:42:19Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 19, 2016 at 11:53 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\n>> index cafc284..aa9f220 100644\n>> --- a/Documentation/glossary-content.txt\n>> +++ b/Documentation/glossary-content.txt\n>> @@ -384,6 +384,23 @@ full pathname may have special meaning:\n>> +\n>> Glob magic is incompatible with literal magic.\n>>\n>> +attr;;\n>> + Additionally to matching the pathspec, the path must have the\n>> + attribute as specified. The syntax for specifying the required\n>> + attributes is \"`attr: [mode] [=value]`\"\n>> ++\n>> +Attributes can have 4 states (Set, Unset, Set to a value, unspecified) and\n>> +you can query each attribute for certain states. The \"`[mode]`\" is a special\n>> +character to indicate which attribute states are looked for. The following\n>> +modes are available:\n>> +\n>> + - an empty \"`[mode]`\" matches if the attribute is set\n>> + - \"`-`\" the attribute must be unset\n>> + - \"`!`\" the attribute must be unspecified\n>> + - an empty \"`[mode]`\" combined with \"`[=value]`\" matches if the attribute has\n>> + the given value.\n>> ++\n>\n> As an initial design, I find this much more agreeable than the\n> previous rounds. I however find the phrasing of the above harder\n> than necessary to understand, for a few reasons.\n>\n> * Mixed use of \"X matches if ...\" and \"... must be Y\" makes it\n> unclear if they are talking about different kind of things, or\n> the same kind of things in merely different ways.\n>\n> * It does not make it clear \"=value\" is only meaningful when [mode]\n> is empty.\n>\n> Perhaps dropping the '[mode]' thing altogether and instead saying\n>\n> After `attr:` comes a space separated list of \"attribute\n> requirements\", all of which must be met in order for the\n> path to be considered a match; this is in addition to the\n> usual non-magic pathspec pattern matching.\n>\n> Each of the attribute requirements for the path takes one of\n> these forms:\n>\n> - \"`ATTR`\" requires that the attribute `ATTR` must be set.\n>\n> - \"`-ATTR`\" requires that the attribute `ATTR` must be unset.\n>\n> - \"`ATTR=VALUE`\" requires that the attribute `ATTR` must be\n> set to the string `VALUE`.\n>\n> - \"`!ATTR`\" requires that the attribute `ATTR` must be\n> unspecified.\n>\n> would make the resulting text easier to read?\n\nThat is way better!\n\n>\n>> +static int match_attrs(const char *name, int namelen,\n>> + const struct pathspec_item *item)\n>> +{\n>> + int i;\n>> +\n>> + git_check_attr_counted(name, namelen, item->attr_check);\n>> + for (i = 0; i < item->attr_match_nr; i++) {\n>> + const char *value;\n>> + int matched;\n>> + enum attr_match_mode match_mode;\n>> +\n>> + value = item->attr_check->check[i].value;\n>> + match_mode = item->attr_match[i].match_mode;\n>> +\n>> + if (ATTR_TRUE(value))\n>> + matched = match_mode == MATCH_SET;\n>> + else if (ATTR_FALSE(value))\n>> + matched = match_mode == MATCH_UNSET;\n>> + else if (ATTR_UNSET(value))\n>> + matched = match_mode == MATCH_UNSPECIFIED;\n>\n> readability nit:\n>\n> matched = (match_mode == MATCH_WHATEVER);\n>\n> would be easier to view\n\nok.\n\n>\n>> + else\n>> + matched = (match_mode == MATCH_VALUE &&\n>> + !strcmp(item->attr_match[i].value, value));\n>\n> and would match the last case above better.\n>\n>> +static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)\n>> +{\n>> + struct string_list_item *si;\n>> + struct string_list list = STRING_LIST_INIT_DUP;\n>> +\n>> +\n>> + if (!value || !strlen(value))\n>> + die(_(\"attr spec must not be empty\"));\n>> +\n>> + string_list_split(&list, value, ' ', -1);\n>> + string_list_remove_empty_items(&list, 0);\n>> +\n>> + if (!item->attr_check)\n>> + item->attr_check = git_attr_check_alloc();\n>> + else\n>> + die(_(\"Only one 'attr:' specification is allowed.\"));\n>> +\n>> + ALLOC_GROW(item->attr_match, item->attr_match_nr + list.nr, item->attr_match_alloc);\n>> +\n>> + for_each_string_list_item(si, &list) {\n>> + size_t attr_len;\n>> +\n>> + int j = item->attr_match_nr++;\n>> + const char *attr = si->string;\n>> + struct attr_match *am = &item->attr_match[j];\n>> +\n>> + if (attr[0] == '!')\n>> + am->match_mode = MATCH_UNSPECIFIED;\n>> + else if (attr[0] == '-')\n>> + am->match_mode = MATCH_UNSET;\n>> + else\n>> + am->match_mode = MATCH_SET;\n>> +\n>> + if (am->match_mode != MATCH_SET)\n>> + /* skip first character */\n>> + attr++;\n>> + attr_len = strcspn(attr, \"=\");\n>> + if (attr[attr_len] == '=') {\n>> + am->match_mode = MATCH_VALUE;\n>> + am->value = xstrdup(&attr[attr_len + 1]);\n>> + if (strchr(am->value, '\\\\'))\n>> + die(_(\"attr spec values must not contain backslashes\"));\n>> + } else\n>> + am->value = NULL;\n>> +\n>\n> Let me try a variant to see if we can improve it (thinking aloud).\n>\n> switch (*attr) {\n> case '!':\n> am->match_mode = MATCH_UNSPECIFIED;\n> attr++;\n> break;\n> case '-':\n> am->match_mode = MATCH_UNSET;\n> attr++;\n> break;\n> default:\n> attr_len = strcspn(attr, \"=\");\n> if (attr[attr_len] != '=')\n> am->match_mode = MATCH_SET;\n> else {\n> am->match_mode = MATCH_VALUE;\n> am->value = xstrdup(...);\n> }\n> break;\n> }\n>\n> Using switch/case does not save line counts at all but it may still\n> make the result easier to follow. More importantly, it would help\n> you catch \"!ATTR=VAR\" as an \"invalid attribute name 'ATTR=VAR' was\n> requested\" error by arranging the code to make sure that scanning\n> for '=' would not happen in !/- case (in other words, while thiking\n> aloud with an alternative way to write the same thing, I spotted a\n> bug in the original ;-).\n\nThat makes sense.\n\n>\n>> + if (!attr_name_valid(attr, attr_len)) {\n>> + struct strbuf sb = STRBUF_INIT;\n>> + am->match_mode = INVALID_ATTR;\n>> + invalid_attr_name_message(&sb, attr, attr_len);\n>> + die(_(\"invalid attribute in '%s': '%s'\"), value, sb.buf);\n>> + }\n>> +\n>> + am->attr = git_attr_counted(attr, attr_len);\n>\n> I'd do this the other order, i.e.\n>\n> am->attr = git_attr_counted(...);\n> if (!am->attr) {\n> ...\n> die(...);\n> }\n>\n> It is wasteful to call attr_name_valid() yourself before seeing an\n> error from git_attr_counted().\n>\n>> diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh\n>> new file mode 100755\n>> index 0000000..c0d8cda\n>> --- /dev/null\n>> +++ b/t/t6134-pathspec-with-labels.sh\n>> @@ -0,0 +1,166 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='test labels in pathspecs'\n>> +. ./test-lib.sh\n>> +\n>> +test_expect_success 'setup a tree' '\n>> + mkdir sub &&\n>> + for p in fileA fileB fileC fileAB fileAC fileBC fileNoLabel fileUnsetLabel fileSetLabel fileValue fileWrongLabel; do\n>> + : >$p &&\n>> + git add $p &&\n>> + : >sub/$p\n>> + git add sub/$p\n>> + done &&\n>> + git commit -m $p &&\n>> + git ls-files >actual &&\n>> + cat <expect &&\n>> +fileA\n>> +fileAB\n>> +fileAC\n>> +fileB\n>> +fileBC\n>> +fileC\n>> +fileNoLabel\n>> +fileSetLabel\n>> +fileUnsetLabel\n>> +fileValue\n>> +fileWrongLabel\n>> +sub/fileA\n>> +sub/fileAB\n>> +sub/fileAC\n>> +sub/fileB\n>> +sub/fileBC\n>> +sub/fileC\n>> +sub/fileNoLabel\n>> +sub/fileSetLabel\n>> +sub/fileUnsetLabel\n>> +sub/fileValue\n>> +sub/fileWrongLabel\n>> +EOF\n>\n> cat <<-\\EOF >expect &&\n> fileA\n> ...\n> EOF\n>\n> to indent the whole thing?\n\n$ grep -r \"cat\" |grep \"<<-\"|wc -l\n915\n$ grep -r \"cat\" |grep \"<<\"|grep -v \"<<-\"| wc -l\n1329\n\nI was undecided what the prevailing style is, some did indent,\nothers did not.\n\nOk, will indent.\n\n>> +sq=\"'\"\n>\n> I do not think you use this $sq variable below, but I may have\n> overlooked its use.\n>\n\nok\n\n>> + git ls-files :\\(attr:\\!label\\) >actual &&\n>\n> Why not the more normal-looking \":(attr:!label)\"? I do not think\n> history substitution would trigger in an script.\n>\n> And no, \"I may cut and paste into my interactive shell while\n> debugging\" is not a good reason to make the end-result (i.e. script)\n> less readable.\n\nok\n"},{"id":"287021","messageId":"CAGZ79kZvZQxPUkECupvqk0KRbq-pRK6y=GksOiYn_zz+TM=dBA@mail.gmail.com","threadId":"42379","inReplyTo":"xmqqiny9j2sc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv8 0/5] pathspec magic extension to search for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T21:00:31Z","receivedAt":"2016-05-19T21:00:31Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 19, 2016 at 11:55 AM, Junio C Hamano wrote:\n> I think this round is 99% there. The next step would be to answer\n> \"does the feature set we have here meet your needs that you wanted\n> to fill with the submodule labels originally?\" and I am hoping it is\n> \"yes\".\n\nBut it's no. (...not quite / not yet)\n\nI thought about this question since sending out the\nseries and I think we would want to improve submodules more.\n\nIn the original patch series \"submodule groups\" I tried to achieve two goals:\n (A) a grouping scheme for submodules\n (B) some sort of persistent configuration for such a group\n\n(A) will be mostly solved now by the pathspec attrs. It may be a bit\nconfusing for users, as any other submodule related configuration is done\nin .gitmodules. Also when moving a submodule (changing its path on the file\nsystem, not the name in the config), any configuration still applies\nexcept for the grouping scheme, which may not match any more.\n\n(B) requires some thought though. Here is my vision:\n\n 1) Allow pathspecs for sparse checkout.\n\n I wonder if we just add support for that in .git/info-sparse-checkout\n or if we add a new file that is for pathspecs only, or we have a config\n option whether sparse-checkout follows pathspecs or gitignore patterns\n\n 2) Teach `git clone` a new option `--sparse-checkout `\n When that option is set the pathspec is written into the new file from\n (1) and core.sparsecheckout is set to true\n\n 3) Advertise to do a `git clone --sparse-checkout\n:(attr:default_submodules)`\n\nGoing this way we would help making submodules not different but integrate more\ninto other concepts of Git. As a downside this would require touching\nsparse checkout which may be more time consuming than just adding a\n`git clone --init-submodules-by-label` which stores the labels and only upddates\nthose submodules.\n\nOr are there other ideas how to go from here?\n\nThanks,\nStefan\n\n\n\n>\n> Thanks.\n"},{"id":"287022","messageId":"xmqqwpmphifb.fsf@gitster.mtv.corp.google.com","threadId":"42379","inReplyTo":"CAGZ79kaZ=0XdHEkocP-LpHT2+ybHd44KCbfONzYj77UTEPb0FA@mail.gmail.com","subject":"Re: [PATCHv8 5/5] pathspec: allow querying for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-19T21:00:40Z","receivedAt":"2016-05-19T21:00:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> $ grep -r \"cat\" |grep \"<<-\"|wc -l\n> 915\n> $ grep -r \"cat\" |grep \"<<\"|grep -v \"<<-\"| wc -l\n> 1329\n>\n> I was undecided what the prevailing style is, some did indent,\n> others did not.\n\nFWIW, newer ones tend to use \"<<-\"; just FYI.\n"},{"id":"287023","messageId":"xmqqshxdhi76.fsf@gitster.mtv.corp.google.com","threadId":"42379","inReplyTo":"CAGZ79kZvZQxPUkECupvqk0KRbq-pRK6y=GksOiYn_zz+TM=dBA@mail.gmail.com","subject":"Re: [PATCHv8 0/5] pathspec magic extension to search for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-19T21:05:33Z","receivedAt":"2016-05-19T21:05:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> (B) requires some thought though. Here is my vision:\n>\n> 1) Allow pathspecs for sparse checkout.\n>\n> I wonder if we just add support for that in .git/info-sparse-checkout\n> or if we add a new file that is for pathspecs only, or we have a config\n> option whether sparse-checkout follows pathspecs or gitignore patterns\n>\n> 2) Teach `git clone` a new option `--sparse-checkout `\n> When that option is set the pathspec is written into the new file from\n> (1) and core.sparsecheckout is set to true\n>\n> 3) Advertise to do a `git clone --sparse-checkout\n> :(attr:default_submodules)`\n>\n> Going this way we would help making submodules not different but integrate more\n> into other concepts of Git. As a downside this would require touching\n> sparse checkout which may be more time consuming than just adding a\n> `git clone --init-submodules-by-label` which stores the labels and only upddates\n> those submodules.\n>\n> Or are there other ideas how to go from here?\n\nMy take is to pretend sparse checkout does not exist at all and then\ngo from there ;-) It is a poorly designed and implemented \"concept\"\nthat we do not want to spread around.\n\nYou were going to add defaultGroup and teach 'clone' (and other\ncommands) about it, and have clone find submodules in that Group,\nright? Isn't the pathspec magic just another way to introduce\nhow you express the \"defaultGroup\", i.e. not with the \"label\" thing\nin .gitmodules, but with pathspec elements with attribute magic?\n"},{"id":"287024","messageId":"CAGZ79kYx_pX81bmpyXCvJSVsZW=t3VpBizUPQ90Wz9=HdG42UA@mail.gmail.com","threadId":"42379","inReplyTo":"xmqqshxdhi76.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv8 0/5] pathspec magic extension to search for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-19T21:25:52Z","receivedAt":"2016-05-19T21:25:52Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 19, 2016 at 2:05 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> (B) requires some thought though. Here is my vision:\n>>\n>> 1) Allow pathspecs for sparse checkout.\n>>\n>> I wonder if we just add support for that in .git/info-sparse-checkout\n>> or if we add a new file that is for pathspecs only, or we have a config\n>> option whether sparse-checkout follows pathspecs or gitignore patterns\n>>\n>> 2) Teach `git clone` a new option `--sparse-checkout `\n>> When that option is set the pathspec is written into the new file from\n>> (1) and core.sparsecheckout is set to true\n>>\n>> 3) Advertise to do a `git clone --sparse-checkout\n>> :(attr:default_submodules)`\n>>\n>> Going this way we would help making submodules not different but integrate more\n>> into other concepts of Git. As a downside this would require touching\n>> sparse checkout which may be more time consuming than just adding a\n>> `git clone --init-submodules-by-label` which stores the labels and only upddates\n>> those submodules.\n>>\n>> Or are there other ideas how to go from here?\n>\n> My take is to pretend sparse checkout does not exist at all and then\n> go from there ;-) It is a poorly designed and implemented \"concept\"\n> that we do not want to spread around.\n>\n> You were going to add defaultGroup and teach 'clone' (and other\n> commands) about it, and have clone find submodules in that Group,\n> right?\n\nRight. But upon finding the new name for clone, I wondered why\nthis has to be submodule specific. The attr pathspecs are also working\nwith any other files. So if you don't use submodules, I think it would be\npretty cool to have a\n\n git clone --sparse-checkout=Documentation/ ...\n\n> Isn't the pathspec magic just another way to introduce\n> how you express the \"defaultGroup\", i.e. not with the \"label\" thing\n> in .gitmodules, but with pathspec elements with attribute magic?\n\nRight. So instead I could do a\n\n git clone --recursive --restrict-submodules-to= --save-restriction\n\nand then later on\n\n git submodule update --use-restriction-saved-by-clone\n\nI think we'd not want more than that for some first real tests\nby some engineers.\n\nHowever after thinking about this for a while I think it's too\nsubmodule centric? The more special sauce we add for submodules\nthe harder they will be to maintain/support as they grow into their own\nthing down the road.\n\nUsing these pathspec attrs are a good example for why we would\nwant to make it more generic. Imagine a future, where more attributes\ncan be codified into pathspecs and this is one of the possibilities:\n\n git clone --sparse=\":(exclude,size>5MB,binary)\n\nwhich would not checkout files that are large binary files. We could\nalso extend the protocol (v2 with the capabilities in client speaks first)\nto transmit such a requirement to the server.\n\nWhy is sparseness considered bad?\n(I did find only limited resources on the net, those bogs I found are\nadvertising it as the last remainder Git needed to be superior to svn in\nany discipline; I did not find a lot of negative feedback on it. So I guess\nusability and confusion?)\n\nIf I wanted to just do the submodule only thing, this would be a smaller\nseries, I would guess.\n\nThanks,\nStefan\n"},{"id":"287107","messageId":"xmqq1t4wfyv4.fsf@gitster.mtv.corp.google.com","threadId":"42379","inReplyTo":"CAGZ79kYx_pX81bmpyXCvJSVsZW=t3VpBizUPQ90Wz9=HdG42UA@mail.gmail.com","subject":"Re: [PATCHv8 0/5] pathspec magic extension to search for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-20T17:00:47Z","receivedAt":"2016-05-20T17:00:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Right. But upon finding the new name for clone, I wondered why\n> this has to be submodule specific. The attr pathspecs are also working\n> with any other files. So if you don't use submodules, I think it would be\n> pretty cool to have a\n>\n> git clone --sparse-checkout=Documentation/ ...\n\nIt would be cool, but arent' \"sparse\" and the various existing\nstatus \"submodule\" has very different things?\n\n - A submodule can be uninitialized, in which case you do get an empty\n directory but you do not see .git in it.\n\n - A path can be excluded by the sparse checkout mechanism, in which\n case you do not get _anything_ in the filesystem.\n\nSo \"git clone --sparse-exclude=Documentation/\" that does not waste\ndiskspace for Documentation/ directory may be an interesting thing\nto have, and \"git clone --sparse-exclude=submodule-dir/\" that does\nnot even create submodule-dir/ directory may also be, but the latter\nis quite different from a submodule that is not initialied in a\nsuperproject that does not use any \"sparse\" mechanism.\n\nBesides, I think (improved) submodule mechanism would be a good way\nforward for scalability, and \"sparse\" hack is not (primarily because\nit still populates the index fully with 5 million entries even when\nyour attention is narrowed only to a handful of directories with\n2000 leaf entries; this misdesign requires other ugly hacks to be\npiled on, like untracked cache and split index).\n\nI do not think we want \"submodule\" to be tied to and dependent on\nthe latter.\n"},{"id":"287117","messageId":"CAGZ79kaBjHXGNxR_6WNWMtmBm0H_QCpHGciM7B1yY1yJ3S23wA@mail.gmail.com","threadId":"42379","inReplyTo":"xmqq1t4wfyv4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv8 0/5] pathspec magic extension to search for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-20T18:12:27Z","receivedAt":"2016-05-20T18:12:27Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 20, 2016 at 10:00 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Right. But upon finding the new name for clone, I wondered why\n>> this has to be submodule specific. The attr pathspecs are also working\n>> with any other files. So if you don't use submodules, I think it would be\n>> pretty cool to have a\n>>\n>> git clone --sparse-checkout=Documentation/ ...\n>\n> It would be cool, but arent' \"sparse\" and the various existing\n> status \"submodule\" has very different things?\n\nYes they are. In one of the various \"submodule groups\" series I\nproposed a \"defaultGroup\" which allows commands to ignore\nsome submodules. That was conceptually the very same as a\n\"sparse checkout, just for submodules\", i.e. the submodule is\ninitialized and has a directory as a place holder, but most commands\nignore its existence.\n\nWe decided that was a bad thing, so now I think of a light weight\n\"submodule.updateGroup\" which holds a pathspec and is only\nused for \"submodule update\" commands that have no explicit\npathspec given. (That setting would be set via \"git clone\n--submodule-pathspec \")\n\n>\n> - A submodule can be uninitialized, in which case you do get an empty\n> directory but you do not see .git in it.\n>\n> - A path can be excluded by the sparse checkout mechanism, in which\n> case you do not get _anything_ in the filesystem.\n\nYes, but isn't that one of the minor issues?\n\n>\n> So \"git clone --sparse-exclude=Documentation/\" that does not waste\n> diskspace for Documentation/ directory may be an interesting thing\n> to have, and \"git clone --sparse-exclude=submodule-dir/\" that does\n> not even create submodule-dir/ directory may also be, but the latter\n> is quite different from a submodule that is not initialied in a\n> superproject that does not use any \"sparse\" mechanism.\n>\n> Besides, I think (improved) submodule mechanism would be a good way\n> forward for scalability, and \"sparse\" hack is not (primarily because\n> it still populates the index fully with 5 million entries even when\n> your attention is narrowed only to a handful of directories with\n> 2000 leaf entries; this misdesign requires other ugly hacks to be\n> piled on, like untracked cache and split index).\n>\n> I do not think we want \"submodule\" to be tied to and dependent on\n> the latter.\n\nOk I just wanted to probe how much resistance I get here as an\nindicator of how much more work that would be.\n\nBesides I think (improved) sparse mechanism would be a good way\nto not confuse users between submodule scalability and single\nrepo scalability. ;)\n\nWe don't have to keep 5 million things in the index there, but we can\nstop on the tree/directory level, i.e. if a whole directory is excluded\nThat's all we'd need to keep a record of, no?\n\nAs a user I'd prefer to be exposed to as few concepts as possible,\nand adding yet another concept of sparseness is not a good thing\nIMHO, so I'll try to keep it simple there.\n\nThanks,\nStefan\n"},{"id":"287119","messageId":"xmqq8tz4ego4.fsf@gitster.mtv.corp.google.com","threadId":"42379","inReplyTo":"CAGZ79kaBjHXGNxR_6WNWMtmBm0H_QCpHGciM7B1yY1yJ3S23wA@mail.gmail.com","subject":"Re: [PATCHv8 0/5] pathspec magic extension to search for attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-20T18:19:07Z","receivedAt":"2016-05-20T18:19:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> As a user I'd prefer to be exposed to as few concepts as possible,\n> and adding yet another concept of sparseness is not a good thing\n> IMHO, so I'll try to keep it simple there.\n\nYes, and as a user, I'd prefer if I (and all the users) do not have\nto even worry about the \"sparse\" hack.\n\nIf you are using submodules, it was in its design from day one that\nit is not unsual for many of your submodules to be uninitialized\n(but still present). And that is a pretty normal state. I do not\nthink we want to expose users to the \"sparse\" hack only to use\nsubmodules effectively.\n"},{"id":"287205","messageId":"CACsJy8CpYmp9t01RSeVY_UAw=zPmhGTY1TZ_p2E2zLuu7LpXhA@mail.gmail.com","threadId":"42379","inReplyTo":"CAGZ79kYx_pX81bmpyXCvJSVsZW=t3VpBizUPQ90Wz9=HdG42UA@mail.gmail.com","subject":"Re: [PATCHv8 0/5] pathspec magic extension to search for attributes","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-22T11:45:38Z","receivedAt":"2016-05-22T11:45:38Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 20, 2016 at 4:25 AM, Stefan Beller wrote:\n>> My take is to pretend sparse checkout does not exist at all and then\n>> go from there ;-)\n\nHehe.. shameless plug, narrow checkout [1] should be its great\nsuccessor where everything is done right (famous last words). Maybe I\ncan convince Stefan to finish that off then I'll finally bring narrow\nclone!\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/289112\n\n> Using these pathspec attrs are a good example for why we would\n> want to make it more generic. Imagine a future, where more attributes\n> can be codified into pathspecs and this is one of the possibilities:\n>\n> git clone --sparse=\":(exclude,size>5MB,binary)\n>\n> which would not checkout files that are large binary files. We could\n> also extend the protocol (v2 with the capabilities in client speaks first)\n> to transmit such a requirement to the server.\n\nI think you need narrow clone there ;-) It's the first step to have a\nclone with missing directories. I think then we can extend it further\nto exclude (large) files.\n\n> Why is sparseness considered bad?\n\nIt does not scale well when the worktree gets bigger. It can be slow\n(but this is just a technical problem I haven't spent time on fixing).\nAnd it does not enable narrow clone (not with lots of hacks, I think I\ndid just that in some early iterations).\n\n> If I wanted to just do the submodule only thing, this would be a smaller\n> series, I would guess.\n\nNo no no. Do more. Start with narrow checkout. I'll help ;-)\n-- \nDuy\n"},{"id":"287269","messageId":"CAGZ79kZPPdUK1=GB6fnHP0cJyoM_VYmDj3wizApN5iHdCQCUyg@mail.gmail.com","threadId":"42379","inReplyTo":"CACsJy8CpYmp9t01RSeVY_UAw=zPmhGTY1TZ_p2E2zLuu7LpXhA@mail.gmail.com","subject":"Re: [PATCHv8 0/5] pathspec magic extension to search for attributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-23T18:49:20Z","receivedAt":"2016-05-23T18:49:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, May 22, 2016 at 4:45 AM, Duy Nguyen wrote:\n> On Fri, May 20, 2016 at 4:25 AM, Stefan Beller wrote:\n>>> My take is to pretend sparse checkout does not exist at all and then\n>>> go from there ;-)\n>\n> Hehe.. shameless plug, narrow checkout [1] should be its great\n> successor where everything is done right (famous last words). Maybe I\n> can convince Stefan to finish that off then I'll finally bring narrow\n> clone!\n\nI started reviewing that (see separate email).\n\nSo conceptually the narrow checkout is \"sparse checkout just for\ndirectories\" IIUC.\n\nA submodule is treated as a files (sometimes, e.g. diff) and sometimes\nas a directory (e.g. in the working tree)\n\n>\n> [1] http://article.gmane.org/gmane.comp.version-control.git/289112\n>\n>> Using these pathspec attrs are a good example for why we would\n>> want to make it more generic. Imagine a future, where more attributes\n>> can be codified into pathspecs and this is one of the possibilities:\n>>\n>> git clone --sparse=\":(exclude,size>5MB,binary)\n>>\n>> which would not checkout files that are large binary files. We could\n>> also extend the protocol (v2 with the capabilities in client speaks first)\n>> to transmit such a requirement to the server.\n>\n> I think you need narrow clone there ;-) It's the first step to have a\n> clone with missing directories. I think then we can extend it further\n> to exclude (large) files.\n\nBut if we only exclude some files we are not having a binary\ndecision for a directory, so narrow checkout doesn't work here?\n\n git clone --sparse=\":(exclude,size>5MB,binary)\n\nwould maybe lead to have\n dir/path.c #included\n dir/binary-asset.img # excluded\n\nso it is more a sparse thing than a narrow thing?\n\n>\n>> Why is sparseness considered bad?\n>\n> It does not scale well when the worktree gets bigger. It can be slow\n> (but this is just a technical problem I haven't spent time on fixing).\n> And it does not enable narrow clone (not with lots of hacks, I think I\n> did just that in some early iterations).\n>\n>> If I wanted to just do the submodule only thing, this would be a smaller\n>> series, I would guess.\n>\n> No no no. Do more. Start with narrow checkout. I'll help ;-)\n\nThanks for the encouragement!\nHow is the interface going to look like for the narrow checkout/clone ?\nIf the UI is supposed to be very similar, we can merge the two concepts\nand make it one thing.\n\nBut if the UI is different enough, we may want to keep it separate as\nit is not as confusing to the user as we'd think?\n\nWhat I imagine UI-wise, see\nhttp://thread.gmane.org/gmane.comp.version-control.git/295221\n\n\nThanks,\nStefan\n\n> --\n> Duy\n"},{"id":"287332","messageId":"CACsJy8CqxYVJt=311xmm3+js_1vx01by4=SH5_0fNw0t2bEzvw@mail.gmail.com","threadId":"42379","inReplyTo":"CAGZ79kZPPdUK1=GB6fnHP0cJyoM_VYmDj3wizApN5iHdCQCUyg@mail.gmail.com","subject":"Re: [PATCHv8 0/5] pathspec magic extension to search for attributes","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-24T02:00:14Z","receivedAt":"2016-05-24T02:00:14Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 24, 2016 at 1:49 AM, Stefan Beller wrote:\n> On Sun, May 22, 2016 at 4:45 AM, Duy Nguyen wrote:\n>> On Fri, May 20, 2016 at 4:25 AM, Stefan Beller wrote:\n>>>> My take is to pretend sparse checkout does not exist at all and then\n>>>> go from there ;-)\n>>\n>> Hehe.. shameless plug, narrow checkout [1] should be its great\n>> successor where everything is done right (famous last words). Maybe I\n>> can convince Stefan to finish that off then I'll finally bring narrow\n>> clone!\n>\n> I started reviewing that (see separate email).\n>\n> So conceptually the narrow checkout is \"sparse checkout just for\n> directories\" IIUC.\n\nYep.\n\n> A submodule is treated as a files (sometimes, e.g. diff) and sometimes\n> as a directory (e.g. in the working tree)\n\nIt's the same here. Submodules have a special mode in the index\n(S_IFDIR + S_IF) while dirs just have S_IFDIR. It's up to\nthe index user to treat it as a \"file\" or a directory (maybe only\nunpack-trees)\n\n>>> Using these pathspec attrs are a good example for why we would\n>>> want to make it more generic. Imagine a future, where more attributes\n>>> can be codified into pathspecs and this is one of the possibilities:\n>>>\n>>> git clone --sparse=\":(exclude,size>5MB,binary)\n>>>\n>>> which would not checkout files that are large binary files. We could\n>>> also extend the protocol (v2 with the capabilities in client speaks first)\n>>> to transmit such a requirement to the server.\n>>\n>> I think you need narrow clone there ;-) It's the first step to have a\n>> clone with missing directories. I think then we can extend it further\n>> to exclude (large) files.\n>\n> But if we only exclude some files we are not having a binary\n> decision for a directory, so narrow checkout doesn't work here?\n>\n> git clone --sparse=\":(exclude,size>5MB,binary)\n>\n> would maybe lead to have\n> dir/path.c #included\n> dir/binary-asset.img # excluded\n>\n> so it is more a sparse thing than a narrow thing?\n\nI wrote two paragraphs, only to realized that you were right. In the\nend we just need a way to exchange \"sparse\" (not narrow) information\nabout this. Narrow clone may need to exchange more side information\n(for git-replace) but I don't see how sparse files fit exactly in this\nwhole exchange yet.\n\n>>> Why is sparseness considered bad?\n>>\n>> It does not scale well when the worktree gets bigger. It can be slow\n>> (but this is just a technical problem I haven't spent time on fixing).\n>> And it does not enable narrow clone (not with lots of hacks, I think I\n>> did just that in some early iterations).\n>>\n>>> If I wanted to just do the submodule only thing, this would be a smaller\n>>> series, I would guess.\n>>\n>> No no no. Do more. Start with narrow checkout. I'll help ;-)\n>\n> Thanks for the encouragement!\n> How is the interface going to look like for the narrow checkout/clone ?\n> If the UI is supposed to be very similar, we can merge the two concepts\n> and make it one thing.\n>\n> But if the UI is different enough, we may want to keep it separate as\n> it is not as confusing to the user as we'd think?\n\nFor narrow clone, I imagine you need to specify what directories, say\nwith --narrow-path option or simply --path, to clone, and that's it.\nIn future you may add --path-filter option that does\n\"exclude,size>5MB,binary\" above.\n\nFor narrow checkout, we have two more operations: fold and unfold\ndirectories. At plumbing level, maybe update-index can support these\noperations explicitly. At porcelain level, checkout probably learns\nabout --narrow-path and submodule can make use of it if it wants.\n\n> What I imagine UI-wise, see\n> http://thread.gmane.org/gmane.comp.version-control.git/295221\n>\n>\n> Thanks,\n> Stefan\n>\n>> --\n>> Duy\n\n\n\n-- \nDuy\n"}]}