{"thread":{"id":"44283","subject":"[PATCHv2] attr: convert to new threadsafe API","startedAt":"2016-10-12T00:08:23Z","lastAt":"2016-10-12T23:10:21Z","messageCount":16,"participants":["Stefan Beller","Junio C Hamano","Johannes Sixt","Jacob Keller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"303951","messageId":"20161011235951.8358-1-sbeller@google.com","threadId":"44283","inReplyTo":null,"subject":"[PATCHv2] attr: convert to new threadsafe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-11T23:59:51Z","receivedAt":"2016-10-12T00:08:23Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This revamps the API of the attr subsystem to be thread safe.\nBefore we had the question and its results in one struct type.\nThe typical usage of the API was\n\n    static struct git_attr_check check;\n\n    if (!check)\n        check = git_attr_check_initl(\"text\", NULL);\n\n    git_check_attr(path, check);\n    act_on(check->value[0]);\n\nThis has a couple of issues when it comes to thread safety:\n\n* the initialization is racy in this implementation. To make it\n  thread safe, we need to acquire a mutex, such that only one\n  thread is executing the code in git_attr_check_initl.\n  As we do not want to introduce a mutex at each call site,\n  this is best done in the attr code. However to do so, we need\n  to have access to the `check` variable, i.e. the API has to\n  look like\n    git_attr_check_initl(struct git_attr_check*, ...);\n  Then one of the threads calling git_attr_check_initl will\n  acquire the mutex and init the `check`, while all other threads\n  will wait on the mutex just to realize they're late to the\n  party and they'll return with no work done.\n\n* While the check for attributes to be questioned only need to\n  be initalized once as that part will be read only after its\n  initialisation, the answer may be different for each path.\n  Because of that we need to decouple the check and the answer,\n  such that each thread can obtain an answer for the path it\n  is currently processing.\n\nThis commit changes the API and adds locking in\ngit_attr_check_initl that provides the thread safety.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nI think this patch is the most interesting patch, so I'll refrain from\nresending the other 27 patches, though I have adressed the review comments\nlocally. I'll resend everything once we are in agreement for this one.\n\nThis version adds the actual thread safety,\nthat is promised in the documentation, however it doesn't add any optimization,\ni.e. it's a single global lock. But as we do not expect contention, that is fine.\n\nAlso there is no example of how to use the thread safe API for asking questions\nfrom multiple threads.\n\nThanks,\nStefan\n\n Documentation/technical/api-gitattributes.txt |  93 +++++++++++++------\n archive.c                                     |  14 ++-\n attr.c                                        | 128 +++++++++++++++++---------\n attr.h                                        |  80 ++++++++++------\n builtin/check-attr.c                          |  30 +++---\n builtin/pack-objects.c                        |  12 ++-\n convert.c                                     |  35 +++----\n ll-merge.c                                    |  27 ++++--\n userdiff.c                                    |  19 ++--\n ws.c                                          |  15 +--\n 10 files changed, 292 insertions(+), 161 deletions(-)\n\ndiff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt\nindex 92fc32a..ac97244 100644\n--- a/Documentation/technical/api-gitattributes.txt\n+++ b/Documentation/technical/api-gitattributes.txt\n@@ -8,6 +8,18 @@ attributes to set of paths.\n Data Structure\n --------------\n \n+extern struct git_attr *git_attr(const char *);\n+\n+/*\n+ * Return the name of the attribute represented by the argument.  The\n+ * return value is a pointer to a null-delimited string that is part\n+ * of the internal data structure; it should not be modified or freed.\n+ */\n+extern const char *git_attr_name(const struct git_attr *);\n+\n+extern int attr_name_valid(const char *name, size_t namelen);\n+extern void invalid_attr_name_message(struct strbuf *, const char *, int);\n+\n `struct git_attr`::\n \n \tAn attribute is an opaque object that is identified by its name.\n@@ -16,15 +28,17 @@ Data Structure\n \tof no interest to the calling programs.  The name of the\n \tattribute can be retrieved by calling `git_attr_name()`.\n \n-`struct git_attr_check_elem`::\n-\n-\tThis structure represents one attribute and its value.\n-\n `struct git_attr_check`::\n \n-\tThis structure represents a collection of `git_attr_check_elem`.\n+\tThis structure represents a collection of `struct git_attrs`.\n \tIt is passed to `git_check_attr()` function, specifying the\n-\tattributes to check, and receives their values.\n+\tattributes to check, and receives their values into a corresponding\n+\t`struct git_attr_result`.\n+\n+`struct git_attr_result`::\n+\n+\tThis structure represents a collection of results to its\n+\tcorresponding `struct git_attr_check`, that has the same order.\n \n \n Attribute Values\n@@ -53,19 +67,30 @@ value of the attribute for the path.\n Querying Specific Attributes\n ----------------------------\n \n-* Prepare `struct git_attr_check` using git_attr_check_initl()\n+* Prepare a `struct git_attr_check` using `git_attr_check_initl()`\n   function, enumerating the names of attributes whose values you are\n   interested in, terminated with a NULL pointer.  Alternatively, an\n-  empty `struct git_attr_check` can be prepared by calling\n-  `git_attr_check_alloc()` function and then attributes you want to\n-  ask about can be added to it with `git_attr_check_append()`\n-  function.\n-\n-* Call `git_check_attr()` to check the attributes for the path.\n-\n-* Inspect `git_attr_check` structure to see how each of the\n-  attribute in the array is defined for the path.\n-\n+  empty `struct git_attr_check` as allocated by git_attr_check_alloc()\n+  can be prepared by calling `git_attr_check_alloc()` function and\n+  then attributes you want to ask about can be added to it with\n+  `git_attr_check_append()` function.\n+  `git_attr_check_initl()` is thread safe, i.e. you can call it\n+  from different threads at the same time; when check determines\n+  the initialzisation is still needed, the threads will use a\n+  single global mutex to perform the initialization just once, the\n+  others will wait on the the thread to actually perform the\n+  initialization.\n+\n+* Prepare a `struct git_attr_result` using `git_attr_result_init()`\n+  for the check struct. The call to initialize the result is not thread\n+  safe, because different threads need their own thread local result\n+  anyway.\n+\n+* Call `git_check_attr()` to check the attributes for the path,\n+  the returned `git_attr_result` contains the result.\n+\n+* Inspect the returned `git_attr_result` structure to see how\n+  each of the attribute in the array is defined for the path.\n \n Example\n -------\n@@ -89,15 +114,20 @@ static void setup_check(void)\n \n ------------\n \tconst char *path;\n+\tstruct git_attr_result *result;\n \n \tsetup_check();\n-\tgit_check_attr(path, check);\n+\tresult = git_check_attr(path, check);\n ------------\n \n-. Act on `.value` member of the result, left in `check->check[]`:\n+The `result` must not be free'd as it is owned by the attr subsystem.\n+It is reused by the same thread, so a subsequent call to git_check_attr\n+in the same thread will overwrite the result.\n+\n+. Act on `result->value[]`:\n \n ------------\n-\tconst char *value = check->check[0].value;\n+\tconst char *value = result->value[0];\n \n \tif (ATTR_TRUE(value)) {\n \t\tThe attribute is Set, by listing only the name of the\n@@ -123,6 +153,8 @@ the first step in the above would be different.\n static struct git_attr_check *check;\n static void setup_check(const char **argv)\n {\n+\tif (check)\n+\t\treturn; /* already done */\n \tcheck = git_attr_check_alloc();\n \twhile (*argv) {\n \t\tstruct git_attr *attr = git_attr(*argv);\n@@ -138,17 +170,20 @@ Querying All Attributes\n \n To get the values of all attributes associated with a file:\n \n-* Prepare an empty `git_attr_check` structure by calling\n-  `git_attr_check_alloc()`.\n+* Setup a local variables on the stack for both the question\n+  `struct git_attr_check` as well as the result `struct git_attr_result`.\n+  Zero them out via their respective _INIT macro.\n \n-* Call `git_all_attrs()`, which populates the `git_attr_check`\n-  with the attributes attached to the path.\n+* Call `git_all_attrs()`\n \n-* Iterate over the `git_attr_check.check[]` array to examine\n-  the attribute names and values.  The name of the attribute\n-  described by a  `git_attr_check.check[]` object can be retrieved via\n-  `git_attr_name(check->check[i].attr)`.  (Please note that no items\n+* Iterate over the `git_attr_check.attr[]` array to examine the\n+  attribute names.  The name of the attribute described by a\n+  `git_attr_check.attr[]` object can be retrieved via\n+  `git_attr_name(check->attr[i])`.  (Please note that no items\n   will be returned for unset attributes, so `ATTR_UNSET()` will return\n   false for all returned `git_array_check` objects.)\n+  The respective value for an attribute can be found in the same\n+  index position in of `git_attr_result`.\n \n-* Free the `git_array_check` by calling `git_attr_check_free()`.\n+* Clear the local variables by calling `git_attr_check_clear()` and\n+  `git_attr_result_clear()`.\ndiff --git a/archive.c b/archive.c\nindex 11e3951..ae98df2 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n \tstruct archiver_args *args = c->args;\n \twrite_archive_entry_fn_t write_entry = c->write_entry;\n \tstatic struct git_attr_check *check;\n+\tstatic struct git_attr_result result;\n \tconst char *path_without_prefix;\n \tint err;\n \n@@ -124,12 +125,15 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n \t\tstrbuf_addch(&path, '/');\n \tpath_without_prefix = path.buf + args->baselen;\n \n-\tif (!check)\n-\t\tcheck = git_attr_check_initl(\"export-ignore\", \"export-subst\", NULL);\n-\tif (!git_check_attr(path_without_prefix, check)) {\n-\t\tif (ATTR_TRUE(check->check[0].value))\n+\tif (!check) {\n+\t\tgit_attr_check_initl(&check, \"export-ignore\", \"export-subst\", NULL);\n+\t\tgit_attr_result_init(&result, check);\n+\t}\n+\n+\tif (!git_check_attr(path_without_prefix, check, &result)) {\n+\t\tif (ATTR_TRUE(result.value[0]))\n \t\t\treturn 0;\n-\t\targs->convert = ATTR_TRUE(check->check[1].value);\n+\t\targs->convert = ATTR_TRUE(result.value[1]);\n \t}\n \n \tif (S_ISDIR(mode) || S_ISGITLINK(mode)) {\ndiff --git a/attr.c b/attr.c\nindex 0faa69f..bf01466 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -14,6 +14,7 @@\n #include \"dir.h\"\n #include \"utf8.h\"\n #include \"quote.h\"\n+#include \"thread-utils.h\"\n \n const char git_attr__true[] = \"(builtin)true\";\n const char git_attr__false[] = \"\\0(builtin)false\";\n@@ -46,6 +47,19 @@ struct git_attr {\n static int attr_nr;\n static struct git_attr *(git_attr_hash[HASHSIZE]);\n \n+#ifndef NO_PTHREADS\n+\n+static pthread_mutex_t attr_mutex;\n+#define attr_lock()\t\tpthread_mutex_lock(&attr_mutex)\n+#define attr_unlock()\t\tpthread_mutex_unlock(&attr_mutex)\n+\n+#else\n+\n+#define attr_lock()\t\t(void)0\n+#define attr_unlock()\t\t(void)0\n+\n+#endif /* NO_PTHREADS */\n+\n /*\n  * NEEDSWORK: maybe-real, maybe-macro are not property of\n  * an attribute, as it depends on what .gitattributes are\n@@ -55,6 +69,16 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);\n  */\n static int cannot_trust_maybe_real;\n \n+/*\n+ * Send one or more git_attr_check to git_check_attrs(), and\n+ * each 'value' member tells what its value is.\n+ * Unset one is returned as NULL.\n+ */\n+struct git_attr_check_elem {\n+\tconst struct git_attr *attr;\n+\tconst char *value;\n+};\n+\n /* NEEDSWORK: This will become per git_attr_check */\n static struct git_attr_check_elem *check_all_attr;\n \n@@ -781,7 +805,7 @@ static int macroexpand_one(int nr, int rem)\n \n static int attr_check_is_dynamic(const struct git_attr_check *check)\n {\n-\treturn (void *)(check->check) != (void *)(check + 1);\n+\treturn (void *)(check->attr) != (void *)(check + 1);\n }\n \n static void empty_attr_check_elems(struct git_attr_check *check)\n@@ -799,7 +823,8 @@ static void empty_attr_check_elems(struct git_attr_check *check)\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, int collect_all)\n+\t\t\t       struct git_attr_check *check,\n+\t\t\t       struct git_attr_result *result, int collect_all)\n \n {\n \tstruct attr_stack *stk;\n@@ -825,13 +850,11 @@ static void collect_some_attrs(const char *path, int pathlen,\n \t\tcheck_all_attr[i].value = ATTR__UNKNOWN;\n \n \tif (!collect_all && !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 < check->check_nr; i++) {\n-\t\t\tif (!celem[i].attr->maybe_real) {\n+\t\t\tif (!check->attr[i]->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+\t\t\t\tc = check_all_attr + check->attr[i]->attr_nr;\n \t\t\t\tc->value = ATTR__UNSET;\n \t\t\t\trem++;\n \t\t\t}\n@@ -845,38 +868,45 @@ static void collect_some_attrs(const char *path, int pathlen,\n \t\trem = fill(path, pathlen, basename_offset, stk, rem);\n \n \tif (collect_all) {\n-\t\tempty_attr_check_elems(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+\n+\t\t\tgit_attr_check_append(check, attr);\n+\n+\t\t\tALLOC_GROW(result->value,\n+\t\t\t\t   result->check_nr + 1,\n+\t\t\t\t   result->check_alloc);\n+\t\t\tresult->value[result->check_nr++] = value;\n \t\t}\n \t}\n }\n \n static int git_check_attrs(const char *path, int pathlen,\n-\t\t\t   struct git_attr_check *check)\n+\t\t\t   struct git_attr_check *check,\n+\t\t\t   struct git_attr_result *result)\n {\n \tint i;\n-\tstruct git_attr_check_elem *celem = check->check;\n \n-\tcollect_some_attrs(path, pathlen, check, 0);\n+\tcollect_some_attrs(path, pathlen, check, result, 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+\t\tconst char *value = check_all_attr[check->attr[i]->attr_nr].value;\n \t\tif (value == ATTR__UNKNOWN)\n \t\t\tvalue = ATTR__UNSET;\n-\t\tcelem[i].value = value;\n+\t\tresult->value[i] = value;\n \t}\n \n \treturn 0;\n }\n \n-void git_all_attrs(const char *path, struct git_attr_check *check)\n+void git_all_attrs(const char *path,\n+\t\t   struct git_attr_check *check,\n+\t\t   struct git_attr_result *result)\n {\n-\tcollect_some_attrs(path, strlen(path), check, 1);\n+\tcollect_some_attrs(path, strlen(path), check, result, 1);\n }\n \n void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)\n@@ -892,36 +922,45 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist\n \tuse_index = istate;\n }\n \n-static int git_check_attr_counted(const char *path, int pathlen,\n-\t\t\t\t  struct git_attr_check *check)\n+int git_check_attr(const char *path,\n+\t\t    struct git_attr_check *check,\n+\t\t    struct git_attr_result *result)\n {\n+\tif (result->check_nr != check->check_nr)\n+\t\tdie(\"BUG: git_attr_result is not prepared correctly\");\n \tcheck->finalized = 1;\n-\treturn git_check_attrs(path, pathlen, check);\n-}\n-\n-int git_check_attr(const char *path, struct git_attr_check *check)\n-{\n-\treturn git_check_attr_counted(path, strlen(path), check);\n+\treturn git_check_attrs(path, strlen(path), check, result);\n }\n \n-struct git_attr_check *git_attr_check_initl(const char *one, ...)\n+void git_attr_check_initl(struct git_attr_check **check_,\n+\t\t\t  const char *one, ...)\n {\n-\tstruct git_attr_check *check;\n \tint cnt;\n \tva_list params;\n \tconst char *param;\n+\tstruct git_attr_check *check;\n+\n+\tif (*check_)\n+\t\treturn;\n+\n+\tattr_lock();\n+\tif (*check_) {\n+\t\tattr_unlock();\n+\t\treturn;\n+\t}\n \n \tva_start(params, one);\n \tfor (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)\n \t\t;\n \tva_end(params);\n+\n \tcheck = xcalloc(1,\n-\t\t\tsizeof(*check) + cnt * sizeof(*(check->check)));\n+\t\t\tsizeof(*check) + cnt * sizeof(*(check->attr)));\n \tcheck->check_nr = cnt;\n \tcheck->finalized = 1;\n-\tcheck->check = (struct git_attr_check_elem *)(check + 1);\n+\tcheck->attr = (const struct git_attr **)(check + 1);\n \n-\tcheck->check[0].attr = git_attr(one);\n+\tcheck->attr[0] = git_attr(one);\n \tva_start(params, one);\n \tfor (cnt = 1; cnt < check->check_nr; cnt++) {\n \t\tstruct git_attr *attr;\n@@ -932,10 +971,11 @@ struct git_attr_check *git_attr_check_initl(const char *one, ...)\n \t\tattr = git_attr(param);\n \t\tif (!attr)\n \t\t\tdie(\"BUG: %s: not a valid attribute name\", param);\n-\t\tcheck->check[cnt].attr = attr;\n+\t\tcheck->attr[cnt] = attr;\n \t}\n \tva_end(params);\n-\treturn check;\n+\t*check_ = check;\n+\tattr_unlock();\n }\n \n struct git_attr_check *git_attr_check_alloc(void)\n@@ -943,18 +983,23 @@ struct git_attr_check *git_attr_check_alloc(void)\n \treturn xcalloc(1, sizeof(struct git_attr_check));\n }\n \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+void git_attr_result_init(struct git_attr_result *result,\n+\t\t\t  struct git_attr_check *check)\n+{\n+\tALLOC_GROW(result->value, check->check_nr, result->check_alloc);\n+\tresult->check_nr = check->check_nr;\n+}\n+\n+\n+void git_attr_check_append(struct git_attr_check *check,\n+\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 \tif (!attr_check_is_dynamic(check))\n \t\tdie(\"BUG: appending to a statically initialized git_attr_check\");\n-\tALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);\n-\telem = &check->check[check->check_nr++];\n-\telem->attr = attr;\n-\treturn elem;\n+\tALLOC_GROW(check->attr, check->check_nr + 1, check->check_alloc);\n+\tcheck->attr[check->check_nr++] = attr;\n }\n \n void git_attr_check_clear(struct git_attr_check *check)\n@@ -962,12 +1007,13 @@ void git_attr_check_clear(struct git_attr_check *check)\n \tempty_attr_check_elems(check);\n \tif (!attr_check_is_dynamic(check))\n \t\tdie(\"BUG: clearing a statically initialized git_attr_check\");\n-\tfree(check->check);\n+\tfree(check->attr);\n \tcheck->check_alloc = 0;\n }\n \n-void git_attr_check_free(struct git_attr_check *check)\n+void git_attr_result_clear(struct git_attr_result *result)\n {\n-\tgit_attr_check_clear(check);\n-\tfree(check);\n+\tfree(result->value);\n+\tresult->check_nr = 0;\n+\tresult->check_alloc = 0;\n }\ndiff --git a/attr.h b/attr.h\nindex 163fcd6..620f6f8 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -10,6 +10,13 @@ struct git_attr;\n  */\n extern struct git_attr *git_attr(const char *);\n \n+/*\n+ * Return the name of the attribute represented by the argument.  The\n+ * return value is a pointer to a null-delimited string that is part\n+ * of the internal data structure; it should not be modified or freed.\n+ */\n+extern const char *git_attr_name(const struct git_attr *);\n+\n extern int attr_name_valid(const char *name, size_t namelen);\n extern void invalid_attr_name_message(struct strbuf *, const char *, int);\n \n@@ -22,44 +29,65 @@ extern const char git_attr__false[];\n #define ATTR_FALSE(v) ((v) == git_attr__false)\n #define ATTR_UNSET(v) ((v) == NULL)\n \n-/*\n- * Send one or more git_attr_check to git_check_attrs(), and\n- * each 'value' member tells what its value is.\n- * Unset one is returned as NULL.\n- */\n-struct git_attr_check_elem {\n-\tconst struct git_attr *attr;\n-\tconst char *value;\n-};\n-\n struct git_attr_check {\n \tint finalized;\n \tint check_nr;\n \tint check_alloc;\n-\tstruct git_attr_check_elem *check;\n+\tconst struct git_attr **attr;\n };\n+#define GIT_ATTR_CHECK_INIT {0, 0, 0, NULL}\n \n-extern struct git_attr_check *git_attr_check_initl(const char *, ...);\n-extern int git_check_attr(const char *path, struct git_attr_check *);\n+struct git_attr_result {\n+\tint check_nr;\n+\tint check_alloc;\n+\tconst char **value;\n+};\n+#define GIT_ATTR_RESULT_INIT {0, 0, NULL}\n \n+/*\n+ * Initialize the `git_attr_check` via one of the following three functions:\n+ *\n+ * git_attr_check_alloc  allocates an empty check,\n+ * git_attr_check_append add an attribute to the given git_attr_check\n+ * git_attr_result_init  prepare the result struct for a given check struct\n+ *\n+ * git_all_attrs         allocates a check and fills in all attributes that\n+ *                       are set for the given path.\n+ * git_attr_check_initl  takes a pointer to where the check will be initialized,\n+ *                       followed by all attributes that are to be checked.\n+ *                       This makes it potentially thread safe as it could\n+ *                       internally have a mutex for that memory location.\n+ *                       Currently it is not thread safe!\n+ */\n extern struct git_attr_check *git_attr_check_alloc(void);\n-extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *, const struct git_attr *);\n+extern void git_attr_check_append(struct git_attr_check *,\n+\t\t\t\t  const struct git_attr *);\n+extern void git_attr_check_initl(struct git_attr_check **,\n+\t\t\t\t const char *, ...);\n \n-extern void git_attr_check_clear(struct git_attr_check *);\n-extern void git_attr_check_free(struct git_attr_check *);\n+extern void git_attr_result_init(struct git_attr_result *,\n+\t\t\t\t struct git_attr_check *);\n \n-/*\n- * Return the name of the attribute represented by the argument.  The\n- * return value is a pointer to a null-delimited string that is part\n- * of the internal data structure; it should not be modified or freed.\n- */\n-extern const char *git_attr_name(const struct git_attr *);\n+extern void git_all_attrs(const char *path,\n+\t\t\t  struct git_attr_check *,\n+\t\t\t  struct git_attr_result *);\n \n-/*\n- * Retrieve all attributes that apply to the specified path.\n- * check holds the attributes and their values.\n+/* Query a path for its attributes */\n+extern int git_check_attr(const char *path,\n+\t\t\t  struct git_attr_check *,\n+\t\t\t  struct git_attr_result *);\n+\n+\n+\n+/**\n+ * Free or clear the result struct. The underlying strings are not free'd\n+ * as they are globally known.\n  */\n-void git_all_attrs(const char *path, struct git_attr_check *check);\n+extern void git_attr_result_free(struct git_attr_result *);\n+extern void git_attr_result_clear(struct git_attr_result *);\n+\n+extern void git_attr_check_clear(struct git_attr_check *);\n+\n \n enum git_attr_direction {\n \tGIT_ATTR_CHECKIN,\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex ec61476..a3df756 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -24,13 +24,17 @@ static const struct option check_attr_options[] = {\n \tOPT_END()\n };\n \n-static void output_attr(struct git_attr_check *check, const char *file)\n+static void output_attr(struct git_attr_check *check,\n+\t\t\tstruct git_attr_result *result, const char *file)\n {\n \tint j;\n \tint cnt = check->check_nr;\n \n+\tif (check->check_nr != result->check_nr)\n+\t\tdie(\"BUG: confused check and result internally\");\n+\n \tfor (j = 0; j < cnt; j++) {\n-\t\tconst char *value = check->check[j].value;\n+\t\tconst char *value = result->value[j];\n \n \t\tif (ATTR_TRUE(value))\n \t\t\tvalue = \"set\";\n@@ -44,11 +48,11 @@ static void output_attr(struct git_attr_check *check, const char *file)\n \t\t\t       \"%s%c\" /* attrname */\n \t\t\t       \"%s%c\" /* attrvalue */,\n \t\t\t       file, 0,\n-\t\t\t       git_attr_name(check->check[j].attr), 0, value, 0);\n+\t\t\t       git_attr_name(check->attr[j]), 0, value, 0);\n \t\t} else {\n \t\t\tquote_c_style(file, NULL, stdout, 0);\n \t\t\tprintf(\": %s: %s\\n\",\n-\t\t\t       git_attr_name(check->check[j].attr), value);\n+\t\t\t       git_attr_name(check->attr[j]), value);\n \t\t}\n \t}\n }\n@@ -60,14 +64,18 @@ static void check_attr(const char *prefix,\n \tchar *full_path =\n \t\tprefix_path(prefix, prefix ? strlen(prefix) : 0, file);\n \tif (check != NULL) {\n-\t\tif (git_check_attr(full_path, check))\n-\t\t\tdie(\"git_check_attr died\");\n-\t\toutput_attr(check, file);\n+\t\tstruct git_attr_result result = GIT_ATTR_RESULT_INIT;\n+\t\tgit_attr_result_init(&result, check);\n+\t\tgit_check_attr(full_path, check, &result);\n+\t\toutput_attr(check, &result, file);\n+\t\tgit_attr_result_clear(&result);\n \t} else {\n-\t\tcheck = git_attr_check_alloc();\n-\t\tgit_all_attrs(full_path, check);\n-\t\toutput_attr(check, file);\n-\t\tgit_attr_check_free(check);\n+\t\tstruct git_attr_check check = GIT_ATTR_CHECK_INIT;\n+\t\tstruct git_attr_result result = GIT_ATTR_RESULT_INIT;\n+\t\tgit_all_attrs(full_path, &check, &result);\n+\t\toutput_attr(&check, &result, file);\n+\t\tgit_attr_result_clear(&result);\n+\t\tgit_attr_check_clear(&check);\n \t}\n \tfree(full_path);\n }\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 3918c07..962c87b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -899,12 +899,16 @@ static void write_pack_file(void)\n static int no_try_delta(const char *path)\n {\n \tstatic struct git_attr_check *check;\n+\tstatic struct git_attr_result result;\n \n-\tif (!check)\n-\t\tcheck = git_attr_check_initl(\"delta\", NULL);\n-\tif (git_check_attr(path, check))\n+\tif (!check) {\n+\t\tgit_attr_check_initl(&check, \"delta\", NULL);\n+\t\tgit_attr_result_init(&result, check);\n+\t}\n+\n+\tif (git_check_attr(path, check, &result))\n \t\treturn 0;\n-\tif (ATTR_FALSE(check->check[0].value))\n+\tif (ATTR_FALSE(result.value[0]))\n \t\treturn 1;\n \treturn 0;\n }\ndiff --git a/convert.c b/convert.c\nindex bb2435a..dd8d25b 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -718,10 +718,8 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,\n \treturn 1;\n }\n \n-static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)\n+static enum crlf_action git_path_check_crlf(const char *value)\n {\n-\tconst char *value = check->value;\n-\n \tif (ATTR_TRUE(value))\n \t\treturn CRLF_TEXT;\n \telse if (ATTR_FALSE(value))\n@@ -735,10 +733,8 @@ static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)\n \treturn CRLF_UNDEFINED;\n }\n \n-static enum eol git_path_check_eol(struct git_attr_check_elem *check)\n+static enum eol git_path_check_eol(const char *value)\n {\n-\tconst char *value = check->value;\n-\n \tif (ATTR_UNSET(value))\n \t\t;\n \telse if (!strcmp(value, \"lf\"))\n@@ -748,9 +744,8 @@ static enum eol git_path_check_eol(struct git_attr_check_elem *check)\n \treturn EOL_UNSET;\n }\n \n-static struct convert_driver *git_path_check_convert(struct git_attr_check_elem *check)\n+static struct convert_driver *git_path_check_convert(const char *value)\n {\n-\tconst char *value = check->value;\n \tstruct convert_driver *drv;\n \n \tif (ATTR_TRUE(value) || ATTR_FALSE(value) || ATTR_UNSET(value))\n@@ -761,10 +756,8 @@ static struct convert_driver *git_path_check_convert(struct git_attr_check_elem\n \treturn NULL;\n }\n \n-static int git_path_check_ident(struct git_attr_check_elem *check)\n+static int git_path_check_ident(const char *value)\n {\n-\tconst char *value = check->value;\n-\n \treturn !!ATTR_TRUE(value);\n }\n \n@@ -778,25 +771,25 @@ struct conv_attrs {\n static void convert_attrs(struct conv_attrs *ca, const char *path)\n {\n \tstatic struct git_attr_check *check;\n+\tstatic struct git_attr_result result;\n \n \tif (!check) {\n-\t\tcheck = git_attr_check_initl(\"crlf\", \"ident\",\n-\t\t\t\t\t     \"filter\", \"eol\", \"text\",\n-\t\t\t\t\t     NULL);\n+\t\tgit_attr_check_initl(&check, \"crlf\", \"ident\", \"filter\",\n+\t\t\t\t     \"eol\", \"text\", NULL);\n+\t\tgit_attr_result_init(&result, check);\n \t\tuser_convert_tail = &user_convert;\n \t\tgit_config(read_convert_config, NULL);\n \t}\n \n-\tif (!git_check_attr(path, check)) {\n-\t\tstruct git_attr_check_elem *ccheck = check->check;\n-\t\tca->crlf_action = git_path_check_crlf(ccheck + 4);\n+\tif (!git_check_attr(path, check, &result)) {\n+\t\tca->crlf_action = git_path_check_crlf(result.value[4]);\n \t\tif (ca->crlf_action == CRLF_UNDEFINED)\n-\t\t\tca->crlf_action = git_path_check_crlf(ccheck + 0);\n+\t\t\tca->crlf_action = git_path_check_crlf(result.value[0]);\n \t\tca->attr_action = ca->crlf_action;\n-\t\tca->ident = git_path_check_ident(ccheck + 1);\n-\t\tca->drv = git_path_check_convert(ccheck + 2);\n+\t\tca->ident = git_path_check_ident(result.value[1]);\n+\t\tca->drv = git_path_check_convert(result.value[2]);\n \t\tif (ca->crlf_action != CRLF_BINARY) {\n-\t\t\tenum eol eol_attr = git_path_check_eol(ccheck + 3);\n+\t\t\tenum eol eol_attr = git_path_check_eol(result.value[3]);\n \t\t\tif (ca->crlf_action == CRLF_AUTO && eol_attr == EOL_LF)\n \t\t\t\tca->crlf_action = CRLF_AUTO_INPUT;\n \t\t\telse if (ca->crlf_action == CRLF_AUTO && eol_attr == EOL_CRLF)\ndiff --git a/ll-merge.c b/ll-merge.c\nindex bc6479c..086f5c7 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -355,6 +355,7 @@ int ll_merge(mmbuffer_t *result_buf,\n {\n \tstatic struct git_attr_check *check;\n \tstatic const struct ll_merge_options default_opts;\n+\tstatic struct git_attr_result result;\n \tconst char *ll_driver_name = NULL;\n \tint marker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n \tconst struct ll_merge_driver *driver;\n@@ -368,13 +369,15 @@ int ll_merge(mmbuffer_t *result_buf,\n \t\tnormalize_file(theirs, path);\n \t}\n \n-\tif (!check)\n-\t\tcheck = git_attr_check_initl(\"merge\", \"conflict-marker-size\", NULL);\n+\tif (!check) {\n+\t\tgit_attr_check_initl(&check, \"merge\", \"conflict-marker-size\", NULL);\n+\t\tgit_attr_result_init(&result, check);\n+\t}\n \n-\tif (!git_check_attr(path, check)) {\n-\t\tll_driver_name = check->check[0].value;\n-\t\tif (check->check[1].value) {\n-\t\t\tmarker_size = atoi(check->check[1].value);\n+\tif (!git_check_attr(path, check, &result)) {\n+\t\tll_driver_name = result.value[0];\n+\t\tif (result.value[1]) {\n+\t\t\tmarker_size = atoi(result.value[1]);\n \t\t\tif (marker_size <= 0)\n \t\t\t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n \t\t}\n@@ -394,12 +397,16 @@ int ll_merge(mmbuffer_t *result_buf,\n int ll_merge_marker_size(const char *path)\n {\n \tstatic struct git_attr_check *check;\n+\tstatic struct git_attr_result result;\n \tint marker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n \n-\tif (!check)\n-\t\tcheck = git_attr_check_initl(\"conflict-marker-size\", NULL);\n-\tif (!git_check_attr(path, check) && check->check[0].value) {\n-\t\tmarker_size = atoi(check->check[0].value);\n+\tif (!check) {\n+\t\tgit_attr_check_initl(&check, \"conflict-marker-size\", NULL);\n+\t\tgit_attr_result_init(&result, check);\n+\t}\n+\n+\tif (!git_check_attr(path, check, &result) && result.value[0]) {\n+\t\tmarker_size = atoi(result.value[0]);\n \t\tif (marker_size <= 0)\n \t\t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n \t}\ndiff --git a/userdiff.c b/userdiff.c\nindex 46dfd32..e88568a 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -263,21 +263,26 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {\n struct userdiff_driver *userdiff_find_by_path(const char *path)\n {\n \tstatic struct git_attr_check *check;\n+\tstatic struct git_attr_result result;\n+\n+\tif (!check) {\n+\t\tgit_attr_check_initl(&check, \"diff\", NULL);\n+\t\tgit_attr_result_init(&result, check);\n+\t}\n \n-\tif (!check)\n-\t\tcheck = git_attr_check_initl(\"diff\", NULL);\n \tif (!path)\n \t\treturn NULL;\n-\tif (git_check_attr(path, check))\n+\n+\tif (git_check_attr(path, check, &result))\n \t\treturn NULL;\n \n-\tif (ATTR_TRUE(check->check[0].value))\n+\tif (ATTR_TRUE(result.value[0]))\n \t\treturn &driver_true;\n-\tif (ATTR_FALSE(check->check[0].value))\n+\tif (ATTR_FALSE(result.value[0]))\n \t\treturn &driver_false;\n-\tif (ATTR_UNSET(check->check[0].value))\n+\tif (ATTR_UNSET(result.value[0]))\n \t\treturn NULL;\n-\treturn userdiff_find_by_name(check->check[0].value);\n+\treturn userdiff_find_by_name(result.value[0]);\n }\n \n struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver)\ndiff --git a/ws.c b/ws.c\nindex bb3270c..8bdbbf7 100644\n--- a/ws.c\n+++ b/ws.c\n@@ -73,15 +73,16 @@ unsigned parse_whitespace_rule(const char *string)\n \n unsigned whitespace_rule(const char *pathname)\n {\n-\tstatic struct git_attr_check *attr_whitespace_rule;\n+\tstatic struct git_attr_check *check;\n+\tstatic struct git_attr_result result;\n \n-\tif (!attr_whitespace_rule)\n-\t\tattr_whitespace_rule = git_attr_check_initl(\"whitespace\", NULL);\n-\n-\tif (!git_check_attr(pathname, attr_whitespace_rule)) {\n-\t\tconst char *value;\n+\tif (!check) {\n+\t\tgit_attr_check_initl(&check, \"whitespace\", NULL);\n+\t\tgit_attr_result_init(&result, check);\n+\t}\n \n-\t\tvalue = attr_whitespace_rule->check[0].value;\n+\tif (!git_check_attr(pathname, check, &result)) {\n+\t\tconst char *value = result.value[0];\n \t\tif (ATTR_TRUE(value)) {\n \t\t\t/* true (whitespace) */\n \t\t\tunsigned all_rule = ws_tab_width(whitespace_rule_cfg);\n-- \n2.10.1.432.g8a36cd8.dirty\n\n"},{"id":"303963","messageId":"xmqqvawy5c4i.fsf@gitster.mtv.corp.google.com","threadId":"44283","inReplyTo":"20161011235951.8358-1-sbeller@google.com","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-12T06:12:45Z","receivedAt":"2016-10-12T06:13:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> I think this patch is the most interesting patch, so I'll refrain from\n> resending the other 27 patches, though I have adressed the review comments\n> locally. I'll resend everything once we are in agreement for this one.\n\nWhat is the primary purpose of this patch?  Is it to prepare callers\nso that the way they interact with the attr subsystem will not have to\nchange when they become threaded and the attr subsystem becomes\nthread ready?\n\nI am not sure if the updates to the callers fulfill that purpose.\nFor example, look at this hunk.\n\n> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>  \tstruct archiver_args *args = c->args;\n>  \twrite_archive_entry_fn_t write_entry = c->write_entry;\n>  \tstatic struct git_attr_check *check;\n> +\tstatic struct git_attr_result result;\n\nAs we discussed, this caller, even when threaded, will always want\nto ask for a fixed two attributes, so \"check\" being static and\nshared across threads is perfectly fine.  But we do not want to see\n\"result\" shared, do we?\n\n>  \tconst char *path_without_prefix;\n>  \tint err;\n>  \n> @@ -124,12 +125,15 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>  \t\tstrbuf_addch(&path, '/');\n>  \tpath_without_prefix = path.buf + args->baselen;\n>  \n> -\tif (!check)\n> -\t\tcheck = git_attr_check_initl(\"export-ignore\", \"export-subst\", NULL);\n> -\tif (!git_check_attr(path_without_prefix, check)) {\n> -\t\tif (ATTR_TRUE(check->check[0].value))\n> +\tif (!check) {\n> +\t\tgit_attr_check_initl(&check, \"export-ignore\", \"export-subst\", NULL);\n> +\t\tgit_attr_result_init(&result, check);\n> +\t}\n\nAre we assuming that storing and checking of a single pointer is\natomic?  I would not expose that assumption to the callers.  On a\nplatform where that assumption holds, \"if check is not NULL,\nsomebody must have done it already, so return without doing nothing\"\ncan be the first thing git_attr_check_initl()'s implementation does,\nthough.  Or it may not hold anywhere without some barriers.  All\nthat implementation details should be hidden inside _initl()'s\nimplementation.  So this caller should instead just do an\nunconditional:\n\n\tgit_attr_check_initl(&check, \"export-ignore\", \"export-subst\", NULL);\n\t\nAlso, as \"result\" should be per running thread, hence non-static,\nand because we do not want repeated heap allocations and releases\nbut luckily most callers _know_ not just how many but what exact\nattributes they are interested in (I think there are only two\ncallers that do not know it; check-all-attrs one, and your pathspec\nmagic one that does not exist at this point in the series), I would\nthink it is much more preferrable to allow the caller to prepare an\non-stack array and call it \"initialized already\".  \n\nIn other words, ideally, I think this part of the patch should\nrather read like this:\n\n\tstatic struct git_attr_check *check;\n\tstruct git_attr_result result[2];\n\n\t...\n\tgit_attr_check_initl(&check, \"export-ignore\", \"export-subst\", NULL);\n\tif (!git_check_attr(path_without_prefix, check, result)) {\n\t\t... use result[0] and result[1] ...\n\nFor sanity checking, it is OK to add ARRAY_SIZE(result) as the final\nand extra parameter to git_check_attr() so that the function can\nmake sure it matches (or exceeds) check->nr.\n"},{"id":"303966","messageId":"CAGZ79kZrNSmPAQ6SmBzFDJtSmdCbqKcgQu4KDLfoYVkSXvo-og@mail.gmail.com","threadId":"44283","inReplyTo":"xmqqvawy5c4i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-12T06:51:56Z","receivedAt":"2016-10-12T06:52:05Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 11, 2016 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> I think this patch is the most interesting patch, so I'll refrain from\n>> resending the other 27 patches, though I have adressed the review comments\n>> locally. I'll resend everything once we are in agreement for this one.\n>\n> What is the primary purpose of this patch?  Is it to prepare callers\n> so that the way they interact with the attr subsystem will not have to\n> change when they become threaded and the attr subsystem becomes\n> thread ready?\n>\n> I am not sure if the updates to the callers fulfill that purpose.\n> For example, look at this hunk.\n>\n>> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>>       struct archiver_args *args = c->args;\n>>       write_archive_entry_fn_t write_entry = c->write_entry;\n>>       static struct git_attr_check *check;\n>> +     static struct git_attr_result result;\n>\n> As we discussed, this caller, even when threaded, will always want\n> to ask for a fixed two attributes, so \"check\" being static and\n> shared across threads is perfectly fine.  But we do not want to see\n> \"result\" shared, do we?\n\nWell all of the hunks in the patch are not threaded, so they\ndon't follow a threading pattern, but the static pattern to not be\nmore expensive than needed.\n\n>\n>>       const char *path_without_prefix;\n>>       int err;\n>>\n>> @@ -124,12 +125,15 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>>               strbuf_addch(&path, '/');\n>>       path_without_prefix = path.buf + args->baselen;\n>>\n>> -     if (!check)\n>> -             check = git_attr_check_initl(\"export-ignore\", \"export-subst\", NULL);\n>> -     if (!git_check_attr(path_without_prefix, check)) {\n>> -             if (ATTR_TRUE(check->check[0].value))\n>> +     if (!check) {\n>> +             git_attr_check_initl(&check, \"export-ignore\", \"export-subst\", NULL);\n>> +             git_attr_result_init(&result, check);\n>> +     }\n>\n> Are we assuming that storing and checking of a single pointer is\n> atomic?  I would not expose that assumption to the callers.  On a\n> platform where that assumption holds, \"if check is not NULL,\n> somebody must have done it already, so return without doing nothing\"\n> can be the first thing git_attr_check_initl()'s implementation does,\n> though.  Or it may not hold anywhere without some barriers.  All\n> that implementation details should be hidden inside _initl()'s\n> implementation.  So this caller should instead just do an\n> unconditional:\n>\n>         git_attr_check_initl(&check, \"export-ignore\", \"export-subst\", NULL);\n>\n> Also, as \"result\" should be per running thread, hence non-static,\n> and because we do not want repeated heap allocations and releases\n> but luckily most callers _know_ not just how many but what exact\n> attributes they are interested in (I think there are only two\n> callers that do not know it; check-all-attrs one, and your pathspec\n> magic one that does not exist at this point in the series), I would\n> think it is much more preferrable to allow the caller to prepare an\n> on-stack array and call it \"initialized already\".\n>\n> In other words, ideally, I think this part of the patch should\n> rather read like this:\n>\n>         static struct git_attr_check *check;\n>         struct git_attr_result result[2];\n>\n>         ...\n>         git_attr_check_initl(&check, \"export-ignore\", \"export-subst\", NULL);\n>         if (!git_check_attr(path_without_prefix, check, result)) {\n>                 ... use result[0] and result[1] ...\n>\n> For sanity checking, it is OK to add ARRAY_SIZE(result) as the final\n> and extra parameter to git_check_attr() so that the function can\n> make sure it matches (or exceeds) check->nr.\n\nThat seems tempting from a callers perspective; I'll look into that.\n"},{"id":"304005","messageId":"xmqqfuo15yjm.fsf@gitster.mtv.corp.google.com","threadId":"44283","inReplyTo":"CAGZ79kZrNSmPAQ6SmBzFDJtSmdCbqKcgQu4KDLfoYVkSXvo-og@mail.gmail.com","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-12T16:20:45Z","receivedAt":"2016-10-12T16:20:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> I am not sure if the updates to the callers fulfill that purpose.\n>> For example, look at this hunk.\n>>\n>>> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>>>       struct archiver_args *args = c->args;\n>>>       write_archive_entry_fn_t write_entry = c->write_entry;\n>>>       static struct git_attr_check *check;\n>>> +     static struct git_attr_result result;\n>>\n>> As we discussed, this caller, even when threaded, will always want\n>> to ask for a fixed two attributes, so \"check\" being static and\n>> shared across threads is perfectly fine.  But we do not want to see\n>> \"result\" shared, do we?\n> \n> Well all of the hunks in the patch are not threaded, so they\n> don't follow a threading pattern, but the static pattern to not be\n> more expensive than needed.\n\nIs it too invasive a change to make them as if they are thread-ready\nusers of API that happen to know their callers are not threading?\nIt would be ideal if we can prepare them so that the way they\ninteract with the attr subsystem will not have to change after this\nstep.\n\n>> In other words, ideally, I think this part of the patch should\n>> rather read like this:\n>>\n>>         static struct git_attr_check *check;\n>>         struct git_attr_result result[2];\n>>\n>>         ...\n>>         git_attr_check_initl(&check, \"export-ignore\", \"export-subst\", NULL);\n>>         if (!git_check_attr(path_without_prefix, check, result)) {\n>>                 ... use result[0] and result[1] ...\n>>\n>> For sanity checking, it is OK to add ARRAY_SIZE(result) as the final\n>> and extra parameter to git_check_attr() so that the function can\n>> make sure it matches (or exceeds) check->nr.\n>\n> That seems tempting from a callers perspective; I'll look into that.\n\nFor callers that prepare \"check\" and \"result\" before asking\ncheck-attr about the attributes in \"check\" for many paths, it is OK\nto use your \"allocate with attr_result_init()\" pattern.  The \"result\"\nstill needs to be made non-static, though.\n\nBut many callers do not follow that; rather they do\n\n\tloop to iterate over paths {\n\t\tcall a helper func to learn attr X for path\n\t\tuse the value of attr X\n\t}\n\nusing a callchain that embeds a helper function deep inside, and\n\"check\" is kept in the helper, check-attr function is called from\nthere, and \"result\" is not passed from the caller to the helper\n(obviously, because it does not exist in the current API).  See the\ncallchain that leads down to convert.c::convert_attrs() for a\ntypical example.  When converted to the new API, it needs to have a\nnew \"result\" structure every time it is called, and cannot reuse the\none that was used in its previous call.\n"},{"id":"304023","messageId":"CAGZ79kaBhXHLDEK0XMLjm3QofmtaGZspA3EEx5x4-qCYY--wZA@mail.gmail.com","threadId":"44283","inReplyTo":"xmqqfuo15yjm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-12T20:02:40Z","receivedAt":"2016-10-12T20:02:46Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 12, 2016 at 9:20 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>> I am not sure if the updates to the callers fulfill that purpose.\n>>> For example, look at this hunk.\n>>>\n>>>> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>>>>       struct archiver_args *args = c->args;\n>>>>       write_archive_entry_fn_t write_entry = c->write_entry;\n>>>>       static struct git_attr_check *check;\n>>>> +     static struct git_attr_result result;\n>>>\n>>> As we discussed, this caller, even when threaded, will always want\n>>> to ask for a fixed two attributes, so \"check\" being static and\n>>> shared across threads is perfectly fine.  But we do not want to see\n>>> \"result\" shared, do we?\n>>\n>> Well all of the hunks in the patch are not threaded, so they\n>> don't follow a threading pattern, but the static pattern to not be\n>> more expensive than needed.\n>\n> Is it too invasive a change to make them as if they are thread-ready\n> users of API that happen to know their callers are not threading?\n> It would be ideal if we can prepare them so that the way they\n> interact with the attr subsystem will not have to change after this\n> step.\n\nAs far as I see the future, we do not need to change those in the future,\nunless we add the threading to the current callers, which is usually a very\ninvasive thing?\n\n>\n>>> In other words, ideally, I think this part of the patch should\n>>> rather read like this:\n>>>\n>>>         static struct git_attr_check *check;\n>>>         struct git_attr_result result[2];\n>>>\n>>>         ...\n>>>         git_attr_check_initl(&check, \"export-ignore\", \"export-subst\", NULL);\n>>>         if (!git_check_attr(path_without_prefix, check, result)) {\n>>>                 ... use result[0] and result[1] ...\n>>>\n>>> For sanity checking, it is OK to add ARRAY_SIZE(result) as the final\n>>> and extra parameter to git_check_attr() so that the function can\n>>> make sure it matches (or exceeds) check->nr.\n>>\n>> That seems tempting from a callers perspective; I'll look into that.\n>\n> For callers that prepare \"check\" and \"result\" before asking\n> check-attr about the attributes in \"check\" for many paths, it is OK\n> to use your \"allocate with attr_result_init()\" pattern.  The \"result\"\n> still needs to be made non-static, though.\n\nI do not see why we would want to have a non static result for\nnon threaded callers.\n\n>\n> But many callers do not follow that; rather they do\n>\n>         loop to iterate over paths {\n>                 call a helper func to learn attr X for path\n>                 use the value of attr X\n>         }\n>\n> using a callchain that embeds a helper function deep inside, and\n> \"check\" is kept in the helper, check-attr function is called from\n> there, and \"result\" is not passed from the caller to the helper\n> (obviously, because it does not exist in the current API).  See the\n> callchain that leads down to convert.c::convert_attrs() for a\n> typical example.  When converted to the new API, it needs to have a\n> new \"result\" structure every time it is called, and cannot reuse the\n> one that was used in its previous call.\n\nWhy would that be? i.e. I do not understand the reasoning/motivation\nas well as what you propose to change here. Do you think of the result\nbeing held at the top of the call chain (and there it may be just allocated\non the stack) and passed down all the way to convert_attrs, that writes\ninto that result?\n\nCurrently the static result in convert_attrs just works fine (as all tests pass)\nand that is not the place where we'd add threading to?\n\nThreading would be added in dir.c, a new place that will use attrs.\n\nMaybe instead of discussing the meaning of this patch further, I should\nrebase the \"pathspecs can ask for attributes\" patches to either experience\nthe problems myself or at least have a series that has a meaningful endgoal.\nI thought this preparatory series may be better than sending an even\nlarger series.\n\nThanks,\nStefan\n"},{"id":"304024","messageId":"44c554b8-7ac1-047d-59f0-b4d5331ed496@kdbg.org","threadId":"44283","inReplyTo":"20161011235951.8358-1-sbeller@google.com","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-10-12T20:07:03Z","receivedAt":"2016-10-12T20:08:56Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12.10.2016 um 01:59 schrieb Stefan Beller:\n> +void git_attr_check_initl(struct git_attr_check **check_,\n> +\t\t\t  const char *one, ...)\n>  {\n> -\tstruct git_attr_check *check;\n>  \tint cnt;\n>  \tva_list params;\n>  \tconst char *param;\n> +\tstruct git_attr_check *check;\n> +\n> +\tif (*check_)\n> +\t\treturn;\n> +\n> +\tattr_lock();\n> +\tif (*check_) {\n> +\t\tattr_unlock();\n> +\t\treturn;\n> +\t}\n> ...\n>  \tcheck = xcalloc(1,\n> -\t\t\tsizeof(*check) + cnt * sizeof(*(check->check)));\n> +\t\t\tsizeof(*check) + cnt * sizeof(*(check->attr)));\n> ...\n> +\t*check_ = check;\n> +\tattr_unlock();\n\nSigh. DCLP, the Double Checked Locking Pattern. These days, it should be \ncommon knowledge among professionals that this naïve version \n_does_not_work_ [1]!\n\nI suggest you go without it, then measure, and only *then* optimize if \nit is a bottleneck. Did I read \"we do not expect much contention\" somewhere?\n\n[1] http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf C++ \ncentric, but applies to C just as well\n\n-- Hannes\n\n"},{"id":"304030","messageId":"xmqqh98h2r5t.fsf@gitster.mtv.corp.google.com","threadId":"44283","inReplyTo":"20161011235951.8358-1-sbeller@google.com","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-12T21:28:30Z","receivedAt":"2016-10-12T21:28:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This version adds the actual thread safety,\n> that is promised in the documentation, however it doesn't add any optimization,\n> i.e. it's a single global lock. But as we do not expect contention, that is fine.\n\nBecause we have to start _somewhere_, I agree it is a good approach\nto first try the simplest implementation and then optimize later,\nbut is it an agreed consensus that we do not expect contention?\n\n"},{"id":"304032","messageId":"CAGZ79kaQGs_3dhd-yEzaq=CRXGQhbMuN_7FdFY=FeKZ4scmMeQ@mail.gmail.com","threadId":"44283","inReplyTo":"xmqqh98h2r5t.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-12T21:39:17Z","receivedAt":"2016-10-12T21:40:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 12, 2016 at 2:28 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> This version adds the actual thread safety,\n>> that is promised in the documentation, however it doesn't add any optimization,\n>> i.e. it's a single global lock. But as we do not expect contention, that is fine.\n>\n> Because we have to start _somewhere_, I agree it is a good approach\n> to first try the simplest implementation and then optimize later,\n> but is it an agreed consensus that we do not expect contention?\n>\n\nI agree on that. Did you mean this is obvious to the reader?  I just wanted\nto state how I'll do it, such that if in the future someone digs up\nthis code, they'll\nknow I did not look at performance in the first run.\n"},{"id":"304033","messageId":"CAGZ79kb-xKDNG-LC56i8Y-FAaYZwr8zzXuC9snj1PavnQ6cdCA@mail.gmail.com","threadId":"44283","inReplyTo":"xmqq8ttt2qpp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-12T21:42:37Z","receivedAt":"2016-10-12T21:42:43Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 12, 2016 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Johannes Sixt <j6t@kdbg.org> writes:\n>\n>> Sigh. DCLP, the Double Checked Locking Pattern. ...\n>> I suggest you go without it, then measure, and only *then* optimize if\n>> it is a bottleneck.\n>\n> That comes from me in earlier discussion before the patch, namely in\n> <xmqqeg3m8y6y.fsf@gitster.mtv.corp.google.com>, where I wondered if\n> a cheap check outside the lock may be a possible optimization\n> opportunity, as this is a classic singleton that will not be\n> deinitialized, and once the codepath gets exercised, we would be\n> taking the \"nothing to do\" route 100% of the time.\n>\n\nHaving followed that advice (and internally having a DCLP), I think\nwe have Triple Checked Locking Pattern in this patch.  Nobody wrote\na paper on how that would not work, yet. ;)\n\nIn the reroll I plan to reduce it to a Single Checked (inside a mutex)\nLocking Pattern, though I would expect that performance (or lack thereof)\nwill show then. But let's postpone measuring until we have a working patch.\n"},{"id":"304034","messageId":"xmqq8ttt2qpp.fsf@gitster.mtv.corp.google.com","threadId":"44283","inReplyTo":"44c554b8-7ac1-047d-59f0-b4d5331ed496@kdbg.org","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-12T21:38:10Z","receivedAt":"2016-10-12T21:44:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Sigh. DCLP, the Double Checked Locking Pattern. ...\n> I suggest you go without it, then measure, and only *then* optimize if\n> it is a bottleneck. \n\nThat comes from me in earlier discussion before the patch, namely in\n<xmqqeg3m8y6y.fsf@gitster.mtv.corp.google.com>, where I wondered if\na cheap check outside the lock may be a possible optimization\nopportunity, as this is a classic singleton that will not be\ndeinitialized, and once the codepath gets exercised, we would be\ntaking the \"nothing to do\" route 100% of the time.\n\n\n\n"},{"id":"304035","messageId":"xmqq4m4h2qef.fsf@gitster.mtv.corp.google.com","threadId":"44283","inReplyTo":"CAGZ79kaQGs_3dhd-yEzaq=CRXGQhbMuN_7FdFY=FeKZ4scmMeQ@mail.gmail.com","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-12T21:44:56Z","receivedAt":"2016-10-12T21:45:23Z","isPatch":false,"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, Oct 12, 2016 at 2:28 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> This version adds the actual thread safety,\n>>> that is promised in the documentation, however it doesn't add any optimization,\n>>> i.e. it's a single global lock. But as we do not expect contention, that is fine.\n>>\n>> Because we have to start _somewhere_, I agree it is a good approach\n>> to first try the simplest implementation and then optimize later,\n>> but is it an agreed consensus that we do not expect contention?\n>\n> I agree on that. Did you mean this is obvious to the reader?\n\nI meant to say that \"But as we do not expect\" sounded like a\njustification for the approach based on an unwarranted assumption\nthat is not even the list concensus.  I do not think it is obvious\nto the reader that there is no need to worry about contention.\n\nIt all is outside the log message, so as long as readers understand\nwhat we meant from this discussion, that is OK.\n\n"},{"id":"304036","messageId":"CA+P7+xqBUT3jUsxciVydO+nRoR+iJygWG=y_ARpiQSs+-kcH2A@mail.gmail.com","threadId":"44283","inReplyTo":"44c554b8-7ac1-047d-59f0-b4d5331ed496@kdbg.org","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-12T21:45:08Z","receivedAt":"2016-10-12T21:45:34Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Oct 12, 2016 at 1:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n>\n> Sigh. DCLP, the Double Checked Locking Pattern. These days, it should be\n> common knowledge among professionals that this naïve version _does_not_work_\n> [1]!\n>\n> I suggest you go without it, then measure, and only *then* optimize if it is\n> a bottleneck. Did I read \"we do not expect much contention\" somewhere?\n>\n> [1] http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf C++ centric,\n> but applies to C just as well\n>\n> -- Hannes\n>\n\n\nYou know, I always wondered why Linux Kernel code needed memory\nbarriers but userspace programs didn't seem to use them.. turns out\nthey actually *do* need them for the same exact types of problems...\n\nThanks,\nJake\n"},{"id":"304037","messageId":"xmqqpon52shy.fsf@gitster.mtv.corp.google.com","threadId":"44283","inReplyTo":"CAGZ79kaBhXHLDEK0XMLjm3QofmtaGZspA3EEx5x4-qCYY--wZA@mail.gmail.com","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-12T20:59:37Z","receivedAt":"2016-10-12T22:14:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>>> Well all of the hunks in the patch are not threaded, so they\n>>> don't follow a threading pattern, but the static pattern to not be\n>>> more expensive than needed.\n>>\n>> Is it too invasive a change to make them as if they are thread-ready\n>> users of API that happen to know their callers are not threading?\n>> It would be ideal if we can prepare them so that the way they\n>> interact with the attr subsystem will not have to change after this\n>> step.\n>\n> As far as I see the future, we do not need to change those in the future,\n> unless we add the threading to the current callers, which is usually a very\n> invasive thing?\n\nIt does not matter how invasive the thread set-up and teardown that\nhappens in the callers.\n\nI am talking about the part of _THIS_ code that you are updating,\nthat interacts with attr API.  The way they prepare \"check\" and\n\"result\", the way they ask questions by calling git_check_attr()\nfunction.\n\nThink of a thread-safe library function (like malloc()).  If you\nwrite \n\n\tfunc (...) {\n\t\tbuf = malloc(20);\n\t\t...\n\t\tfree(buf);\n\t}\n\nin a function that happens to be only called in a non-threaded\nprogram today, you do not have to update these calls to malloc(3)\nand free(3) when you update the callchain to threadable, right?\n\nThat kind of thread-preparedness is what I am trying to see if we\ncan achieve with this update.\n"},{"id":"304038","messageId":"xmqqwphd1bkc.fsf@gitster.mtv.corp.google.com","threadId":"44283","inReplyTo":"CAGZ79kb-xKDNG-LC56i8Y-FAaYZwr8zzXuC9snj1PavnQ6cdCA@mail.gmail.com","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-12T21:50:43Z","receivedAt":"2016-10-12T22:25:25Z","isPatch":false,"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, Oct 12, 2016 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Johannes Sixt <j6t@kdbg.org> writes:\n>>\n>>> Sigh. DCLP, the Double Checked Locking Pattern. ...\n>>> I suggest you go without it, then measure, and only *then* optimize if\n>>> it is a bottleneck.\n>>\n>> That comes from me in earlier discussion before the patch, namely in\n>> <xmqqeg3m8y6y.fsf@gitster.mtv.corp.google.com>, where I wondered if\n>> a cheap check outside the lock may be a possible optimization\n>> opportunity, as this is a classic singleton that will not be\n>> deinitialized, and once the codepath gets exercised, we would be\n>> taking the \"nothing to do\" route 100% of the time.\n>\n> Having followed that advice (and internally having a DCLP), I think\n> we have Triple Checked Locking Pattern in this patch.  Nobody wrote\n> a paper on how that would not work, yet. ;)\n>\n> In the reroll I plan to reduce it to a Single Checked (inside a mutex)\n> Locking Pattern, though I would expect that performance (or lack thereof)\n> will show then. But let's postpone measuring until we have a working patch.\n\nOh, that wasn't even an \"advice\" (read it again).  I fully agree\nthat starting simple would be the way to go.\n"},{"id":"304042","messageId":"CAGZ79ka5qkDri0gCCLQQcUgqFpOi=np+jm-x046Foc9NOpE6NA@mail.gmail.com","threadId":"44283","inReplyTo":"CA+P7+xqBUT3jUsxciVydO+nRoR+iJygWG=y_ARpiQSs+-kcH2A@mail.gmail.com","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-12T21:47:17Z","receivedAt":"2016-10-12T22:48:13Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 12, 2016 at 2:45 PM, Jacob Keller <jacob.keller@gmail.com> wrote:\n> On Wed, Oct 12, 2016 at 1:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n>>\n>> Sigh. DCLP, the Double Checked Locking Pattern. These days, it should be\n>> common knowledge among professionals that this naïve version _does_not_work_\n>> [1]!\n>>\n>> I suggest you go without it, then measure, and only *then* optimize if it is\n>> a bottleneck. Did I read \"we do not expect much contention\" somewhere?\n>>\n>> [1] http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf C++ centric,\n>> but applies to C just as well\n>>\n>> -- Hannes\n>>\n>\n>\n> You know, I always wondered why Linux Kernel code needed memory\n> barriers but userspace programs didn't seem to use them.. turns out\n> they actually *do* need them for the same exact types of problems...\n>\n> Thanks,\n> Jake\n\nIn a former job I made use of them, too. So I am kinda embarrassed.\n(I cannot claim I did not know about these patterns and memory\nfencing, it just escaped my consciousness).\n"},{"id":"304044","messageId":"xmqqlgxt18h0.fsf@gitster.mtv.corp.google.com","threadId":"44283","inReplyTo":"CAGZ79kaBhXHLDEK0XMLjm3QofmtaGZspA3EEx5x4-qCYY--wZA@mail.gmail.com","subject":"Re: [PATCHv2] attr: convert to new threadsafe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-12T22:57:31Z","receivedAt":"2016-10-12T23:10:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> But many callers do not follow that; rather they do\n>>\n>>         loop to iterate over paths {\n>>                 call a helper func to learn attr X for path\n>>                 use the value of attr X\n>>         }\n>>\n>> using a callchain that embeds a helper function deep inside, and\n>> \"check\" is kept in the helper, check-attr function is called from\n>> there, and \"result\" is not passed from the caller to the helper\n>> (obviously, because it does not exist in the current API).  See the\n>> callchain that leads down to convert.c::convert_attrs() for a\n>> typical example.  When converted to the new API, it needs to have a\n>> new \"result\" structure every time it is called, and cannot reuse the\n>> one that was used in its previous call.\n>\n> Why would that be? i.e. I do not understand the reasoning/motivation\n> as well as what you propose to change here.\n\nThe leaf function may look like\n\n\tcheck_eol(const char *path)\n\t{\n                static struct git_attr_check *check;\n\n\t\tinitl(&check, \"eol\", NULL);\n\t\tgit_check_attr(&check, path, result);\n\t\treturn nth_element_in_result(result, 0);\n\t}                \n\nand we want \"result\" to be thread-ready.  \n\nA naive and undesired way to put it on stack is like so:\n\n\tconst char *check_eol(const char *path)\n\t{\n                static struct git_attr_check *check;\n\t\tstruct git_attr_result result = RESULT_INIT;\n\t\tconst char *eol;\n\n\t\tinitl(&check, \"eol\", NULL);\n\t\tinit_result(&check, &result);\n\t\tgit_check_attr(&check, path, &result);\n\t\teol = nth_element_in_result(&result, 0);\n\t\tclear_result(&result);\n\t\treturn eol;\n\t}                \n\nwhere your \"struct git_attr_result\" has a fixed size, and the actual\nresult array is allocated via ALLOC_GROW() etc.  That's overhead\nthat we do not want.  Instead can't we do this?\n\n\tconst char *check_eol(const char *path)\n\t{\n                static struct git_attr_check *check;\n\t\t/* we know we only want one */\n\t\tstruct git_attr_result result[1];\n\t\tconst char *eol;\n\n\t\tinitl(&check, \"eol\", NULL);\n\t\tgit_check_attr(&check, path, result);\n\t\treturn result[0];\n\t}                \n\nThat way, we won't be doing ALLOC_GROW() in init_result() or free in\nclear_result().\n\nIf you use a structure that has pointer to an array (i.e. the \"naive\nand undesired way\" above), you cannot amortize the alloc/free by\nmaking result \"static\" if you want to be thread-ready.\n"}]}