{"thread":{"id":"42563","subject":"[PATCH 0/5] A bit more \"attributes\"","startedAt":"2016-06-08T22:58:13Z","lastAt":"2016-06-16T02:19:50Z","messageCount":13,"participants":["Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"288773","messageId":"20160608225818.726-1-gitster@pobox.com","threadId":"42563","inReplyTo":null,"subject":"[PATCH 0/5] A bit more \"attributes\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-08T22:58:13Z","receivedAt":"2016-06-08T22:58:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The whole jc/attr topic is now depended on Stefan's \"pathspec label\"\ntopic, both of which are in 'next' and cannot be rewound until the\nnext cycle.  These five patches are to continue my recent\n\"attributes\" work while the tree is in frozen state.\n\nA few central data structures, namely \"attr_stack\" that keeps track\nof the attributes read from the .gitattributes files in the\ndirectory hierarchy of interest and \"check_all_attr[]\" that keeps\ntrack of the state of the attributes for the path being checked, are\nimplemented as file-scope global singletons that belong to the\nattribute subsystem (\"attr.c\").  The envisioned endgame is to make\nthem into per \"struct git_attr_check\" instance, so that the entire\nsystem can have multiple \"directory hierarchy of interest\" and \"path\nbeing checked\", i.e. allow multiple threads to keep their own\n\"git_attr_check\" and to use the attribute system simultaneously.\n\nTo further that plan, the first two patches pass the check[]\nstructure further down the callchain, and the last two patches\nremove a hack that prevented check[] from being passed in one corner\ncase and always pass it down the callchain.\n\nJunio C Hamano (5):\n  attr.c: add push_stack() helper\n  attr.c: pass struct git_attr_check down the callchain\n  fixup! d5ad6c13\n  attr.c: correct ugly hack for git_all_attrs()\n  attr.c: always pass check[] to collect_some_attrs()\n\n attr.c | 143 +++++++++++++++++++++++++++++++++--------------------------------\n attr.h |   2 +-\n 2 files changed, 74 insertions(+), 71 deletions(-)\n\n-- \n2.9.0-rc2-262-g9161bbf\n"},{"id":"288774","messageId":"20160608225818.726-2-gitster@pobox.com","threadId":"42563","inReplyTo":"20160608225818.726-1-gitster@pobox.com","subject":"[PATCH 1/5] attr.c: add push_stack() helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-08T22:58:14Z","receivedAt":"2016-06-08T22:58:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"There are too many repetitious \"I have this new attr_stack element;\npush it at the top of the stack\" sequence.  The new helper function\npush_stack() gives us a way to express what is going on at these\nplaces, and as a side effect, halves the number of times we mention\nthe attr_stack global variable.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n attr.c | 71 +++++++++++++++++++++++++++++++-----------------------------------\n 1 file changed, 33 insertions(+), 38 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex fc63712..26228ce 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -521,6 +521,18 @@ static int git_attr_system(void)\n \n static GIT_PATH_FUNC(git_path_info_attributes, INFOATTRIBUTES_FILE)\n \n+static void push_stack(struct attr_stack **attr_stack_p,\n+\t\t       struct attr_stack *elem, char *origin, size_t originlen)\n+{\n+\tif (elem) {\n+\t\telem->origin = origin;\n+\t\tif (origin)\n+\t\t\telem->originlen = originlen;\n+\t\telem->prev = *attr_stack_p;\n+\t\t*attr_stack_p = elem;\n+\t}\n+}\n+\n static void bootstrap_attr_stack(void)\n {\n \tstruct attr_stack *elem;\n@@ -528,52 +540,35 @@ static void bootstrap_attr_stack(void)\n \tif (attr_stack)\n \t\treturn;\n \n-\telem = read_attr_from_array(builtin_attr);\n-\telem->origin = NULL;\n-\telem->prev = attr_stack;\n-\tattr_stack = elem;\n-\n-\tif (git_attr_system()) {\n-\t\telem = read_attr_from_file(git_etc_gitattributes(), 1);\n-\t\tif (elem) {\n-\t\t\telem->origin = NULL;\n-\t\t\telem->prev = attr_stack;\n-\t\t\tattr_stack = elem;\n-\t\t}\n-\t}\n+\tpush_stack(&attr_stack, read_attr_from_array(builtin_attr), NULL, 0);\n+\n+\tif (git_attr_system())\n+\t\tpush_stack(&attr_stack,\n+\t\t\t   read_attr_from_file(git_etc_gitattributes(), 1),\n+\t\t\t   NULL, 0);\n \n \tif (!git_attributes_file)\n \t\tgit_attributes_file = xdg_config_home(\"attributes\");\n-\tif (git_attributes_file) {\n-\t\telem = read_attr_from_file(git_attributes_file, 1);\n-\t\tif (elem) {\n-\t\t\telem->origin = NULL;\n-\t\t\telem->prev = attr_stack;\n-\t\t\tattr_stack = elem;\n-\t\t}\n-\t}\n+\tif (git_attributes_file)\n+\t\tpush_stack(&attr_stack,\n+\t\t\t   read_attr_from_file(git_attributes_file, 1),\n+\t\t\t   NULL, 0);\n \n \tif (!is_bare_repository() || direction == GIT_ATTR_INDEX) {\n \t\telem = read_attr(GITATTRIBUTES_FILE, 1);\n-\t\telem->origin = xstrdup(\"\");\n-\t\telem->originlen = 0;\n-\t\telem->prev = attr_stack;\n-\t\tattr_stack = elem;\n+\t\tpush_stack(&attr_stack, elem, xstrdup(\"\"), 0);\n \t\tdebug_push(elem);\n \t}\n \n \telem = read_attr_from_file(git_path_info_attributes(), 1);\n \tif (!elem)\n \t\telem = xcalloc(1, sizeof(*elem));\n-\telem->origin = NULL;\n-\telem->prev = attr_stack;\n-\tattr_stack = elem;\n+\tpush_stack(&attr_stack, elem, NULL, 0);\n }\n \n static void prepare_attr_stack(const char *path, int dirlen)\n {\n \tstruct attr_stack *elem, *info;\n-\tint len;\n \tconst char *cp;\n \n \t/*\n@@ -633,20 +628,21 @@ static void prepare_attr_stack(const char *path, int dirlen)\n \n \t\tassert(attr_stack->origin);\n \t\twhile (1) {\n-\t\t\tlen = strlen(attr_stack->origin);\n+\t\t\tsize_t len = strlen(attr_stack->origin);\n+\t\t\tchar *origin;\n+\n \t\t\tif (dirlen <= len)\n \t\t\t\tbreak;\n \t\t\tcp = memchr(path + len + 1, '/', dirlen - len - 1);\n \t\t\tif (!cp)\n \t\t\t\tcp = path + dirlen;\n-\t\t\tstrbuf_add(&pathbuf, path, cp - path);\n-\t\t\tstrbuf_addch(&pathbuf, '/');\n-\t\t\tstrbuf_addstr(&pathbuf, GITATTRIBUTES_FILE);\n+\t\t\tstrbuf_addf(&pathbuf,\n+\t\t\t\t    \"%.*s/%s\", (int)(cp - path), path,\n+\t\t\t\t    GITATTRIBUTES_FILE);\n \t\t\telem = read_attr(pathbuf.buf, 0);\n \t\t\tstrbuf_setlen(&pathbuf, cp - path);\n-\t\t\telem->origin = strbuf_detach(&pathbuf, &elem->originlen);\n-\t\t\telem->prev = attr_stack;\n-\t\t\tattr_stack = elem;\n+\t\t\torigin = strbuf_detach(&pathbuf, &len);\n+\t\t\tpush_stack(&attr_stack, elem, origin, len);\n \t\t\tdebug_push(elem);\n \t\t}\n \n@@ -656,8 +652,7 @@ static void prepare_attr_stack(const char *path, int dirlen)\n \t/*\n \t * Finally push the \"info\" one at the top of the stack.\n \t */\n-\tinfo->prev = attr_stack;\n-\tattr_stack = info;\n+\tpush_stack(&attr_stack, info, NULL, 0);\n }\n \n static int path_matches(const char *pathname, int pathlen,\n-- \n2.9.0-rc2-262-g9161bbf\n"},{"id":"288777","messageId":"20160608225818.726-3-gitster@pobox.com","threadId":"42563","inReplyTo":"20160608225818.726-1-gitster@pobox.com","subject":"[PATCH 2/5] attr.c: pass struct git_attr_check down the callchain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-08T22:58:15Z","receivedAt":"2016-06-08T22:58:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The callchain that starts from git_check_attrs() down to\ncollect_some_attrs() used to take an array of git_attr_check_elem\nas their parameters.  Pass the enclosing git_attr_check instance\ninstead, so that they will have access to new fields we will add to\nthe data structure.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n attr.c | 36 ++++++++++++++++++++++++------------\n 1 file changed, 24 insertions(+), 12 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 26228ce..4e2172a 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -746,14 +746,25 @@ static int macroexpand_one(int nr, int rem)\n  * check_all_attr. If num is non-zero, only attributes in check[] are\n  * collected. Otherwise all attributes are collected.\n  */\n-static void collect_some_attrs(const char *path, int pathlen, int num,\n-\t\t\t       struct git_attr_check_elem *check)\n+static void collect_some_attrs(const char *path, int pathlen,\n+\t\t\t       struct git_attr_check *check)\n \n {\n \tstruct attr_stack *stk;\n \tint i, rem, dirlen;\n \tconst char *cp, *last_slash = NULL;\n \tint basename_offset;\n+\tint num;\n+\tstruct git_attr_check_elem *celem;\n+\n+\tif (!check) {\n+\t\t/* Yuck - ugly git_all_attrs() hack! */\n+\t\tcelem = NULL;\n+\t\tnum = 0;\n+\t} else {\n+\t\tcelem = check->check;\n+\t\tnum = check->check_nr;\n+\t}\n \n \tfor (cp = path; cp < path + pathlen; cp++) {\n \t\tif (*cp == '/' && cp[1])\n@@ -773,9 +784,9 @@ static void collect_some_attrs(const char *path, int pathlen, int num,\n \tif (num && !cannot_trust_maybe_real) {\n \t\trem = 0;\n \t\tfor (i = 0; i < num; i++) {\n-\t\t\tif (!check[i].attr->maybe_real) {\n+\t\t\tif (!celem[i].attr->maybe_real) {\n \t\t\t\tstruct git_attr_check_elem *c;\n-\t\t\t\tc = check_all_attr + check[i].attr->attr_nr;\n+\t\t\t\tc = check_all_attr + celem[i].attr->attr_nr;\n \t\t\t\tc->value = ATTR__UNSET;\n \t\t\t\trem++;\n \t\t\t}\n@@ -789,18 +800,19 @@ static void collect_some_attrs(const char *path, int pathlen, int num,\n \t\trem = fill(path, pathlen, basename_offset, stk, rem);\n }\n \n-static int git_check_attrs(const char *path, int pathlen, int num,\n-\t\t\t   struct git_attr_check_elem *check)\n+static int git_check_attrs(const char *path, int pathlen,\n+\t\t\t   struct git_attr_check *check)\n {\n \tint i;\n+\tstruct git_attr_check_elem *celem = check->check;\n \n-\tcollect_some_attrs(path, pathlen, num, check);\n+\tcollect_some_attrs(path, pathlen, check);\n \n-\tfor (i = 0; i < num; i++) {\n-\t\tconst char *value = check_all_attr[check[i].attr->attr_nr].value;\n+\tfor (i = 0; i < check->check_nr; i++) {\n+\t\tconst char *value = check_all_attr[celem[i].attr->attr_nr].value;\n \t\tif (value == ATTR__UNKNOWN)\n \t\t\tvalue = ATTR__UNSET;\n-\t\tcheck[i].value = value;\n+\t\tcelem[i].value = value;\n \t}\n \n \treturn 0;\n@@ -811,7 +823,7 @@ void git_all_attrs(const char *path, struct git_attr_check *check)\n \tint i;\n \n \tgit_attr_check_clear(check);\n-\tcollect_some_attrs(path, strlen(path), 0, NULL);\n+\tcollect_some_attrs(path, strlen(path), NULL);\n \n \tfor (i = 0; i < attr_nr; i++) {\n \t\tconst char *name = check_all_attr[i].attr->name;\n@@ -840,7 +852,7 @@ int git_check_attr_counted(const char *path, int pathlen,\n \t\t\t   struct git_attr_check *check)\n {\n \tcheck->finalized = 1;\n-\treturn git_check_attrs(path, pathlen, check->check_nr, check->check);\n+\treturn git_check_attrs(path, pathlen, check);\n }\n \n int git_check_attr(const char *path, struct git_attr_check *check)\n-- \n2.9.0-rc2-262-g9161bbf\n"},{"id":"288776","messageId":"20160608225818.726-4-gitster@pobox.com","threadId":"42563","inReplyTo":"20160608225818.726-1-gitster@pobox.com","subject":"[PATCH 3/5] fixup! d5ad6c13","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-08T22:58:16Z","receivedAt":"2016-06-08T22:58:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"As the topic is in 'next' already, I'll leave this floating near the\ntip for now, until we can rewind the topic after the next release.\n---\n attr.c | 9 ++++++---\n attr.h | 2 +-\n 2 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 4e2172a..0e61950 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -899,13 +899,16 @@ struct git_attr_check *git_attr_check_alloc(void)\n \treturn xcalloc(1, sizeof(struct git_attr_check));\n }\n \n-void git_attr_check_append(struct git_attr_check *check,\n-\t\t\t   const struct git_attr *attr)\n+struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *check,\n+\t\t\t\t\t\t  const struct git_attr *attr)\n {\n+\tstruct git_attr_check_elem *elem;\n \tif (check->finalized)\n \t\tdie(\"BUG: append after git_attr_check structure is finalized\");\n \tALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);\n-\tcheck->check[check->check_nr++].attr = attr;\n+\telem = &check->check[check->check_nr++];\n+\telem->attr = attr;\n+\treturn elem;\n }\n \n void git_attr_check_clear(struct git_attr_check *check)\ndiff --git a/attr.h b/attr.h\nindex fc72030..40abc16 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -47,7 +47,7 @@ extern int git_check_attr(const char *path, struct git_attr_check *);\n extern int git_check_attr_counted(const char *, int, struct git_attr_check *);\n \n extern struct git_attr_check *git_attr_check_alloc(void);\n-extern void git_attr_check_append(struct git_attr_check *, const struct git_attr *);\n+extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *, const struct git_attr *);\n \n extern void git_attr_check_clear(struct git_attr_check *);\n extern void git_attr_check_free(struct git_attr_check *);\n-- \n2.9.0-rc2-262-g9161bbf\n"},{"id":"288778","messageId":"20160608225818.726-5-gitster@pobox.com","threadId":"42563","inReplyTo":"20160608225818.726-1-gitster@pobox.com","subject":"[PATCH 4/5] attr.c: correct ugly hack for git_all_attrs()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-08T22:58:17Z","receivedAt":"2016-06-08T22:58:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The collect_some_attrs() function has an ugly hack since\n\n06a604e6 (attr: avoid heavy work when we know the specified attr is\nnot defined, 2014-12-28) added an optimization that relies on the\nfact that the caller knows what attributes it is interested in, so\nthat we can leave once we know the final answer for all the\nattributes the caller asked.\n\ngit_all_attrs() that asks \"what attributes are on this path?\"\nhowever does not know what attributes it is interested in, other\nthan the vague \"we are interested in all of them\", which is not a\nvery useful thing to say.  As a way to disable this optimization\nfor this caller, the said commit added a code to skip it when\nthe caller passes a NULL for the check structure.\n\nHowever, it skipped the optimization not when check is NULL, but\nwhen the number of attributes being checked is 0, which is\nunnecessarily pessimistic.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n attr.c | 24 ++++++++----------------\n 1 file changed, 8 insertions(+), 16 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 0e61950..cdf064e 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -743,8 +743,8 @@ static int macroexpand_one(int nr, int rem)\n \n /*\n  * Collect attributes for path into the array pointed to by\n- * check_all_attr. If num is non-zero, only attributes in check[] are\n- * collected. Otherwise all attributes are collected.\n+ * check_all_attr.  If check is not NULL, only attributes in\n+ * check[] are collected. Otherwise all attributes are collected.\n  */\n static void collect_some_attrs(const char *path, int pathlen,\n \t\t\t       struct git_attr_check *check)\n@@ -754,17 +754,6 @@ static void collect_some_attrs(const char *path, int pathlen,\n \tint i, rem, dirlen;\n \tconst char *cp, *last_slash = NULL;\n \tint basename_offset;\n-\tint num;\n-\tstruct git_attr_check_elem *celem;\n-\n-\tif (!check) {\n-\t\t/* Yuck - ugly git_all_attrs() hack! */\n-\t\tcelem = NULL;\n-\t\tnum = 0;\n-\t} else {\n-\t\tcelem = check->check;\n-\t\tnum = check->check_nr;\n-\t}\n \n \tfor (cp = path; cp < path + pathlen; cp++) {\n \t\tif (*cp == '/' && cp[1])\n@@ -781,9 +770,12 @@ static void collect_some_attrs(const char *path, int pathlen,\n \tprepare_attr_stack(path, dirlen);\n \tfor (i = 0; i < attr_nr; i++)\n \t\tcheck_all_attr[i].value = ATTR__UNKNOWN;\n-\tif (num && !cannot_trust_maybe_real) {\n+\n+\tif (check && !cannot_trust_maybe_real) {\n+\t\tstruct git_attr_check_elem *celem = check->check;\n+\n \t\trem = 0;\n-\t\tfor (i = 0; i < num; i++) {\n+\t\tfor (i = 0; i < check->check_nr; i++) {\n \t\t\tif (!celem[i].attr->maybe_real) {\n \t\t\t\tstruct git_attr_check_elem *c;\n \t\t\t\tc = check_all_attr + celem[i].attr->attr_nr;\n@@ -791,7 +783,7 @@ static void collect_some_attrs(const char *path, int pathlen,\n \t\t\t\trem++;\n \t\t\t}\n \t\t}\n-\t\tif (rem == num)\n+\t\tif (rem == check->check_nr)\n \t\t\treturn;\n \t}\n \n-- \n2.9.0-rc2-262-g9161bbf\n"},{"id":"288775","messageId":"20160608225818.726-6-gitster@pobox.com","threadId":"42563","inReplyTo":"20160608225818.726-1-gitster@pobox.com","subject":"[PATCH 5/5] attr.c: always pass check[] to collect_some_attrs()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-08T22:58:18Z","receivedAt":"2016-06-08T22:58:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This function used to be called with check=NULL to signal it to\ncollect all attributes in the global check_all_attr[] array.\n\nBecause the longer term plan is to allocate check_all_attr[] and\nattr_stack data structures per git_attr_check instance (i.e. \"check\"\nhere) to make the attr subsystem thread-safe, it is unacceptable.\n\nPass \"Are we grabbing all attributes defined in the system?\" bit as\na separate argument and pass it from the callers.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n attr.c | 37 +++++++++++++++++++------------------\n 1 file changed, 19 insertions(+), 18 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex cdf064e..95d2f77 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -743,11 +743,12 @@ static int macroexpand_one(int nr, int rem)\n \n /*\n  * Collect attributes for path into the array pointed to by\n- * check_all_attr.  If check is not NULL, only attributes in\n- * check[] are collected. Otherwise all attributes are collected.\n+ * check_all_attr.  If collect_all is zero, only attributes in\n+ * check[] are collected.  Otherwise, check[] is cleared and\n+ * any and all attributes that are visible are collected in it.\n  */\n static void collect_some_attrs(const char *path, int pathlen,\n-\t\t\t       struct git_attr_check *check)\n+\t\t\t       struct git_attr_check *check, int collect_all)\n \n {\n \tstruct attr_stack *stk;\n@@ -768,10 +769,11 @@ static void collect_some_attrs(const char *path, int pathlen,\n \t}\n \n \tprepare_attr_stack(path, dirlen);\n+\n \tfor (i = 0; i < attr_nr; i++)\n \t\tcheck_all_attr[i].value = ATTR__UNKNOWN;\n \n-\tif (check && !cannot_trust_maybe_real) {\n+\tif (!collect_all && !cannot_trust_maybe_real) {\n \t\tstruct git_attr_check_elem *celem = check->check;\n \n \t\trem = 0;\n@@ -790,6 +792,17 @@ static void collect_some_attrs(const char *path, int pathlen,\n \trem = attr_nr;\n \tfor (stk = attr_stack; 0 < rem && stk; stk = stk->prev)\n \t\trem = fill(path, pathlen, basename_offset, stk, rem);\n+\n+\tif (collect_all) {\n+\t\tgit_attr_check_clear(check);\n+\t\tfor (i = 0; i < attr_nr; i++) {\n+\t\t\tconst struct git_attr *attr = check_all_attr[i].attr;\n+\t\t\tconst char *value = check_all_attr[i].value;\n+\t\t\tif (value == ATTR__UNSET || value == ATTR__UNKNOWN)\n+\t\t\t\tcontinue;\n+\t\t\tgit_attr_check_append(check, attr)->value = value;\n+\t\t}\n+\t}\n }\n \n static int git_check_attrs(const char *path, int pathlen,\n@@ -798,7 +811,7 @@ static int git_check_attrs(const char *path, int pathlen,\n \tint i;\n \tstruct git_attr_check_elem *celem = check->check;\n \n-\tcollect_some_attrs(path, pathlen, check);\n+\tcollect_some_attrs(path, pathlen, check, 0);\n \n \tfor (i = 0; i < check->check_nr; i++) {\n \t\tconst char *value = check_all_attr[celem[i].attr->attr_nr].value;\n@@ -812,19 +825,7 @@ static int git_check_attrs(const char *path, int pathlen,\n \n void git_all_attrs(const char *path, struct git_attr_check *check)\n {\n-\tint i;\n-\n-\tgit_attr_check_clear(check);\n-\tcollect_some_attrs(path, strlen(path), NULL);\n-\n-\tfor (i = 0; i < attr_nr; i++) {\n-\t\tconst char *name = check_all_attr[i].attr->name;\n-\t\tconst char *value = check_all_attr[i].value;\n-\t\tif (value == ATTR__UNSET || value == ATTR__UNKNOWN)\n-\t\t\tcontinue;\n-\t\tgit_attr_check_append(check, git_attr(name));\n-\t\tcheck->check[check->check_nr - 1].value = value;\n-\t}\n+\tcollect_some_attrs(path, strlen(path), check, 1);\n }\n \n void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)\n-- \n2.9.0-rc2-262-g9161bbf\n"},{"id":"288780","messageId":"CAGZ79ka+BXOAMr7JxJXmhsmXTwTcdidJ72=9mbCkHiycj-t6AQ@mail.gmail.com","threadId":"42563","inReplyTo":"20160608225818.726-2-gitster@pobox.com","subject":"Re: [PATCH 1/5] attr.c: add push_stack() helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-08T23:43:44Z","receivedAt":"2016-06-08T23:43:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jun 8, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> +static void push_stack(struct attr_stack **attr_stack_p,\n> +                      struct attr_stack *elem, char *origin, size_t originlen)\n> +{\n> +       if (elem) {\n> +               elem->origin = origin;\n> +               if (origin)\n> +                       elem->originlen = originlen;\n\nWhy do we need to be conditional on origin for setting the originlen,\nin all occurrences below, we pass in a reasonable number (0),\nso I would leave out the condition here.\n\nWe make use of the `originlen` in `fill` only, so maybe we can\neven get rid of the length and when we need it compute\nit with strlen? (I am unsure on this tradeoff)\n\n\n>\n>         if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {\n>                 elem = read_attr(GITATTRIBUTES_FILE, 1);\n> -               elem->origin = xstrdup(\"\");\n> -               elem->originlen = 0;\n> -               elem->prev = attr_stack;\n> -               attr_stack = elem;\n> +               push_stack(&attr_stack, elem, xstrdup(\"\"), 0);\n\nI wonder why we need to pass an empty-but-not-null string here?\nIn `fill` we use\n\n  const char *base = stk->origin ? stk->origin : \"\";\n\nso there it would be the same. In prepare_stack we have\n       /*\n        * Pop the ones from directories that are not the prefix of\n        * the path we are checking. Break out of the loop when we see\n        * the root one (whose origin is an empty string \"\") or the builtin\n        * one (whose origin is NULL) without popping it.\n        */\n        while (attr_stack->origin) {\n\nwhich seems to answer my question  that we make a difference between\nempty and null `origin`. However I wonder if that could be made more clear?\n(By a well named bit flag in the attr_stack?)\n\n> -                       strbuf_add(&pathbuf, path, cp - path);\n> -                       strbuf_addch(&pathbuf, '/');\n> -                       strbuf_addstr(&pathbuf, GITATTRIBUTES_FILE);\n> +                       strbuf_addf(&pathbuf,\n> +                                   \"%.*s/%s\", (int)(cp - path), path,\n\nThis is neat way of handling strings that are not null terminated!\nI have the suspicion I could have used this before.\n\nAs this is meant as a refactoring patch, which doesn't want to change\nsemantics, it looks good to me, the questions are rather meant for followups.\n\nThanks,\nStefan\n"},{"id":"288782","messageId":"CAGZ79kZ0uSvTgc9YnVKg01PtRcG4vuhUE+TZoPadT8Xqeqi=Tg@mail.gmail.com","threadId":"42563","inReplyTo":"20160608225818.726-3-gitster@pobox.com","subject":"Re: [PATCH 2/5] attr.c: pass struct git_attr_check down the callchain","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-08T23:54:53Z","receivedAt":"2016-06-08T23:54:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jun 8, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> The callchain that starts from git_check_attrs() down to\n> collect_some_attrs() used to take an array of git_attr_check_elem\n> as their parameters.  Pass the enclosing git_attr_check instance\n> instead, so that they will have access to new fields we will add to\n> the data structure.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  attr.c | 36 ++++++++++++++++++++++++------------\n>  1 file changed, 24 insertions(+), 12 deletions(-)\n>\n> diff --git a/attr.c b/attr.c\n> index 26228ce..4e2172a 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -746,14 +746,25 @@ static int macroexpand_one(int nr, int rem)\n>   * check_all_attr. If num is non-zero, only attributes in check[] are\n>   * collected. Otherwise all attributes are collected.\n>   */\n> -static void collect_some_attrs(const char *path, int pathlen, int num,\n> -                              struct git_attr_check_elem *check)\n> +static void collect_some_attrs(const char *path, int pathlen,\n> +                              struct git_attr_check *check)\n>\n>  {\n>         struct attr_stack *stk;\n>         int i, rem, dirlen;\n>         const char *cp, *last_slash = NULL;\n>         int basename_offset;\n> +       int num;\n> +       struct git_attr_check_elem *celem;\n> +\n> +       if (!check) {\n> +               /* Yuck - ugly git_all_attrs() hack! */\n> +               celem = NULL;\n> +               num = 0;\n> +       } else {\n> +               celem = check->check;\n> +               num = check->check_nr;\n> +       }\n>\n>         for (cp = path; cp < path + pathlen; cp++) {\n>                 if (*cp == '/' && cp[1])\n> @@ -773,9 +784,9 @@ static void collect_some_attrs(const char *path, int pathlen, int num,\n>         if (num && !cannot_trust_maybe_real) {\n>                 rem = 0;\n>                 for (i = 0; i < num; i++) {\n> -                       if (!check[i].attr->maybe_real) {\n> +                       if (!celem[i].attr->maybe_real) {\n>                                 struct git_attr_check_elem *c;\n> -                               c = check_all_attr + check[i].attr->attr_nr;\n> +                               c = check_all_attr + celem[i].attr->attr_nr;\n>                                 c->value = ATTR__UNSET;\n>                                 rem++;\n>                         }\n> @@ -789,18 +800,19 @@ static void collect_some_attrs(const char *path, int pathlen, int num,\n>                 rem = fill(path, pathlen, basename_offset, stk, rem);\n>  }\n>\n> -static int git_check_attrs(const char *path, int pathlen, int num,\n> -                          struct git_attr_check_elem *check)\n> +static int git_check_attrs(const char *path, int pathlen,\n> +                          struct git_attr_check *check)\n>  {\n>         int i;\n> +       struct git_attr_check_elem *celem = check->check;\n\nWe don't need this outside the for loop; maybe we want to move\nthis in there including indexing?\n\nOr rather dropping the arrays and use pointers\nin there if we're concerned about performance?\n(That would be a slightly larger refactoring, roughly):\n\n    struct git_attr_check_elem *cur = check->check;\n    struct git_attr_check_elem *end = cur + check->check_nr;\n    while (cur < end) {\n        value = check_all_attr[cur->attr->attr_nr].value;\n        cur->value = (value == ATTR__UNKNOWN) ? ATTR__UNSET : value;\n        cur++;\n    }\n\n\nLooks correct as it is, though.\n\nThanks,\nStefan\n"},{"id":"288783","messageId":"CAGZ79kYjaWJ8=3rizFE+EZXS3P1LjX8bGD-6LwfmjG83cjgVJQ@mail.gmail.com","threadId":"42563","inReplyTo":"20160608225818.726-4-gitster@pobox.com","subject":"Re: [PATCH 3/5] fixup! d5ad6c13","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-09T00:00:20Z","receivedAt":"2016-06-09T00:00:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jun 8, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> As the topic is in 'next' already, I'll leave this floating near the\n> tip for now, until we can rewind the topic after the next release.\n\nI don't quite understand the motivation behind this commit.\n\n    We return the last element to allow succeeding operations\n    access to the bottom of the stack without needing to walk it?\n    This makes the follow up operations faster, because we expect the stack\n    to be larger than 5 elements.\n\n?\n\n\n> ---\n>  attr.c | 9 ++++++---\n>  attr.h | 2 +-\n>  2 files changed, 7 insertions(+), 4 deletions(-)\n>\n> diff --git a/attr.c b/attr.c\n> index 4e2172a..0e61950 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -899,13 +899,16 @@ struct git_attr_check *git_attr_check_alloc(void)\n>         return xcalloc(1, sizeof(struct git_attr_check));\n>  }\n>\n> -void git_attr_check_append(struct git_attr_check *check,\n> -                          const struct git_attr *attr)\n> +struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *check,\n> +                                                 const struct git_attr *attr)\n>  {\n> +       struct git_attr_check_elem *elem;\n>         if (check->finalized)\n>                 die(\"BUG: append after git_attr_check structure is finalized\");\n>         ALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);\n> -       check->check[check->check_nr++].attr = attr;\n> +       elem = &check->check[check->check_nr++];\n> +       elem->attr = attr;\n> +       return elem;\n>  }\n>\n>  void git_attr_check_clear(struct git_attr_check *check)\n> diff --git a/attr.h b/attr.h\n> index fc72030..40abc16 100644\n> --- a/attr.h\n> +++ b/attr.h\n> @@ -47,7 +47,7 @@ extern int git_check_attr(const char *path, struct git_attr_check *);\n>  extern int git_check_attr_counted(const char *, int, struct git_attr_check *);\n>\n>  extern struct git_attr_check *git_attr_check_alloc(void);\n> -extern void git_attr_check_append(struct git_attr_check *, const struct git_attr *);\n> +extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *, const struct git_attr *);\n>\n>  extern void git_attr_check_clear(struct git_attr_check *);\n>  extern void git_attr_check_free(struct git_attr_check *);\n> --\n> 2.9.0-rc2-262-g9161bbf\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"288785","messageId":"xmqqporrjk76.fsf@gitster.mtv.corp.google.com","threadId":"42563","inReplyTo":"CAGZ79kYjaWJ8=3rizFE+EZXS3P1LjX8bGD-6LwfmjG83cjgVJQ@mail.gmail.com","subject":"Re: [PATCH 3/5] fixup! d5ad6c13","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-09T00:10:37Z","receivedAt":"2016-06-09T00:10:37Z","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 Wed, Jun 8, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> As the topic is in 'next' already, I'll leave this floating near the\n>> tip for now, until we can rewind the topic after the next release.\n>\n> I don't quite understand the motivation behind this commit.\n>\n>     We return the last element to allow succeeding operations\n>     access to the bottom of the stack without needing to walk it?\n>     This makes the follow up operations faster, because we expect the stack\n>     to be larger than 5 elements.\n>\n> ?\n\nThink how you call _append() that returns void and then set the\nvalue to the last appended item.  YOu'd need to do something\nunwieldy like\n\n\tgit_attr_check_append(check, attr);\n\tcheck->check[check->check_nr-1].value = value\n"},{"id":"288786","messageId":"xmqqlh2fjjzg.fsf@gitster.mtv.corp.google.com","threadId":"42563","inReplyTo":"CAGZ79ka+BXOAMr7JxJXmhsmXTwTcdidJ72=9mbCkHiycj-t6AQ@mail.gmail.com","subject":"Re: [PATCH 1/5] attr.c: add push_stack() helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-09T00:15:15Z","receivedAt":"2016-06-09T00:15: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\nAll of your questions is about !origin vs !*origin.  This applies to\nthe code in 'master', not specific to jc/attr topic's improvements.\nIt is how the prepare_attr_stack() tells where the .gitattributes\nfor the top-level directory is when popping the elements for other\ndirectory hierarchy we have left (origin && !*origin is the root\nlevel). !origin denotes entries that did not come from in-tree files\n(i.e. $GIT_DIR/info/attributes, built-in ones, etc.).\n"},{"id":"288788","messageId":"CAGZ79kbWygHWVhcn9du8P15i2okvFgvt2VPdamHCf2Bvm-n0fg@mail.gmail.com","threadId":"42563","inReplyTo":"20160608225818.726-6-gitster@pobox.com","subject":"Re: [PATCH 5/5] attr.c: always pass check[] to collect_some_attrs()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-09T00:25:39Z","receivedAt":"2016-06-09T00:25:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jun 8, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> This function used to be called with check=NULL to signal it to\n> collect all attributes in the global check_all_attr[] array.\n>\n> Because the longer term plan is to allocate check_all_attr[] and\n> attr_stack data structures per git_attr_check instance (i.e. \"check\"\n> here) to make the attr subsystem thread-safe, it is unacceptable.\n>\n> Pass \"Are we grabbing all attributes defined in the system?\" bit as\n> a separate argument and pass it from the callers.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  attr.c | 37 +++++++++++++++++++------------------\n>  1 file changed, 19 insertions(+), 18 deletions(-)\n>\n> diff --git a/attr.c b/attr.c\n> index cdf064e..95d2f77 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -743,11 +743,12 @@ static int macroexpand_one(int nr, int rem)\n>\n>  /*\n>   * Collect attributes for path into the array pointed to by\n> - * check_all_attr.  If check is not NULL, only attributes in\n> - * check[] are collected. Otherwise all attributes are collected.\n> + * check_all_attr.  If collect_all is zero, only attributes in\n> + * check[] are collected.  Otherwise, check[] is cleared and\n> + * any and all attributes that are visible are collected in it.\n>   */\n>  static void collect_some_attrs(const char *path, int pathlen,\n> -                              struct git_attr_check *check)\n> +                              struct git_attr_check *check, int collect_all)\n\nI think it may be better to have a collect_all_attrs instead of a flag here,\nas more than half the executed code differs (the parts conditioned on\ncollect_all are rather large in the function)\n\n>\n>  {\n>         struct attr_stack *stk;\n> @@ -768,10 +769,11 @@ static void collect_some_attrs(const char *path, int pathlen,\n>         }\n>\n>         prepare_attr_stack(path, dirlen);\n> +\n\nstray empty line?\n"},{"id":"288832","messageId":"xmqq37omi5zt.fsf@gitster.mtv.corp.google.com","threadId":"42563","inReplyTo":"CAGZ79kbWygHWVhcn9du8P15i2okvFgvt2VPdamHCf2Bvm-n0fg@mail.gmail.com","subject":"Re: [PATCH 5/5] attr.c: always pass check[] to collect_some_attrs()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-09T18:15:02Z","receivedAt":"2016-06-16T02: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>>  static void collect_some_attrs(const char *path, int pathlen,\n>> -                              struct git_attr_check *check)\n>> +                              struct git_attr_check *check, int collect_all)\n>\n> I think it may be better to have a collect_all_attrs instead of a flag here,\n> as more than half the executed code differs (the parts conditioned on\n> collect_all are rather large in the function)\n\nIt is understandable if it appears to anybody who does not know the\nfuture that way ;-)\n\nThe plan is to remove that \"if (!collect_all)\" optimization block.\nOnce the attr_stack and check_all_attr becomes per git_attr_check\ninstance, there is no reason to keep record of all attribute\ndefinitions read from the .gitattribute files.  The entries that are\nknown not to matter (i.e. not listed in git_attr_check) can be\ndiscarded when they are read before it hits attr_stack.\n\nThe \"optimization\" in that block is to optimize one special case: an\nattribute may be defined in the git_attr_hash[] dictionary, and the\nones that appeared in various .gitattribute files are marked with a\nbit (an attribute may be in that dictionary only because the caller\nwho asks about the attribute filled it in check->check[] array, and\nmay not appear in the .gitattribute files prepare_attr_stack()\nread).  If the check->check[] wants to know about an attribute that\ndoes not appear in any of the .gitattribute files, we can answer\n\"that attribute is not set to anything\" without actually executing\nany matches with path patterns.\n\nThe \"an attr_stack that is per git_attr_check only has to contain\nwhat matters and can discard irrelevant records\" is a natural\nextension of the current optimization.  Instead of the current \"when\n0 rules refer to attribute, do not do any of the 47 path matches\ndefined in the attribute system, but even if one rule refers to it,\ndo all 47 path matches to check\", we would do \"among 47 path\npatterns, only attempt to match patterns that affect the attributes\nthat we are checking (which could be 0 rules).\n"}]}