{"thread":{"id":"44293","subject":"[PATCHv3] attr: convert to new threadsafe API","startedAt":"2016-10-12T22:42:05Z","lastAt":"2016-10-19T00:40:43Z","messageCount":9,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"304040","messageId":"20161012224109.23410-1-sbeller@google.com","threadId":"44293","inReplyTo":null,"subject":"[PATCHv3] attr: convert to new threadsafe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-12T22:41:09Z","receivedAt":"2016-10-12T22:42:05Z","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\nThe usage of the new API will be:\n\n    /*\n     * The initl call will thread-safely check whether the\n     * struct git_attr_check has been initialized. We only\n     * want to do the initialization work once, hence we do\n     * that work inside a thread safe environment.\n     */\n    static struct git_attr_check *check;\n    git_attr_check_initl(&check, \"text\", NULL);\n\n    /*\n     * Resize internal data structures in the result to match\n     * the size of `check`:\n     */\n    struct git_attr_result *result = GIT_ATTR_RESULT_INIT;\n    git_attr_result_init(&result, check);\n\n    /* Perform the check and act on it: */\n    git_check_attr(path, check, &result);\n    act_on(check->value[0]);\n\n    /*\n     * Free the result.\n     * The check is static, so doesn't require free'ing\n     */\n    git_attr_result_clear(&result);\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n * Do not use the Double Check Locking Pattern\n * The callers are all thread safe now (i.e. no static result)\n * So this is roughly what we want for the first version of the conversion?\n \n Thanks,\n Stefan\n\n Documentation/technical/api-gitattributes.txt |  93 +++++++++++++------\n archive.c                                     |  18 ++--\n attr.c                                        | 125 +++++++++++++++++---------\n attr.h                                        |  80 +++++++++++------\n builtin/check-attr.c                          |  31 ++++---\n builtin/pack-objects.c                        |  20 +++--\n convert.c                                     |  43 +++++----\n ll-merge.c                                    |  28 +++---\n userdiff.c                                    |  24 +++--\n ws.c                                          |  17 ++--\n 10 files changed, 314 insertions(+), 165 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..15849a8 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -107,10 +107,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n \t\tvoid *context)\n {\n \tstatic struct strbuf path = STRBUF_INIT;\n+\tstatic struct git_attr_check *check;\n+\n \tstruct archiver_context *c = context;\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+\tstruct git_attr_result result = GIT_ATTR_RESULT_INIT;\n \tconst char *path_without_prefix;\n \tint err;\n \n@@ -124,12 +126,16 @@ 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+\tgit_attr_check_initl(&check, \"export-ignore\", \"export-subst\", NULL);\n+\tgit_attr_result_init(&result, check);\n+\n+\tif (!git_check_attr(path_without_prefix, check, &result)) {\n+\t\tif (ATTR_TRUE(result.value[0])) {\n+\t\t\tgit_attr_result_clear(&result);\n \t\t\treturn 0;\n-\t\targs->convert = ATTR_TRUE(check->check[1].value);\n+\t\t}\n+\t\targs->convert = ATTR_TRUE(result.value[1]);\n+\t\tgit_attr_result_clear(&result);\n \t}\n \n \tif (S_ISDIR(mode) || S_ISGITLINK(mode)) {\ndiff --git a/attr.c b/attr.c\nindex 0faa69f..648c4f9 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,42 @@ 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+\treturn git_check_attrs(path, strlen(path), check, result);\n }\n \n-int git_check_attr(const char *path, struct git_attr_check *check)\n+void git_attr_check_initl(struct git_attr_check **check_,\n+\t\t\t  const char *one, ...)\n {\n-\treturn git_check_attr_counted(path, strlen(path), check);\n-}\n-\n-struct git_attr_check *git_attr_check_initl(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+\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 +968,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 +980,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 +1004,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..5e78d5d 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@@ -59,16 +63,21 @@ static void check_attr(const char *prefix,\n {\n \tchar *full_path =\n \t\tprefix_path(prefix, prefix ? strlen(prefix) : 0, file);\n+\tstruct git_attr_result result = GIT_ATTR_RESULT_INIT;\n+\tstruct git_attr_check local_check = GIT_ATTR_CHECK_INIT;\n+\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\tgit_attr_result_init(&result, check);\n+\t\tgit_check_attr(full_path, check, &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\tgit_all_attrs(full_path, &local_check, &result);\n+\t\tcheck = &local_check;\n \t}\n+\n+\toutput_attr(check, &result, file);\n+\n+\tgit_attr_result_clear(&result);\n+\tgit_attr_check_clear(&local_check);\n \tfree(full_path);\n }\n \ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 3918c07..14b764a 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -900,13 +900,19 @@ static int no_try_delta(const char *path)\n {\n \tstatic struct git_attr_check *check;\n \n-\tif (!check)\n-\t\tcheck = git_attr_check_initl(\"delta\", NULL);\n-\tif (git_check_attr(path, check))\n-\t\treturn 0;\n-\tif (ATTR_FALSE(check->check[0].value))\n-\t\treturn 1;\n-\treturn 0;\n+\tstruct git_attr_result result = GIT_ATTR_RESULT_INIT;\n+\tint ret = 0;\n+\n+\tgit_attr_check_initl(&check, \"delta\", NULL);\n+\tgit_attr_result_init(&result, check);\n+\n+\tif (!git_check_attr(path, check, &result)) {\n+\t\tif (ATTR_FALSE(result.value[0]))\n+\t\t\tret = 1;\n+\t}\n+\n+\tgit_attr_result_clear(&result);\n+\treturn ret;\n }\n \n /*\ndiff --git a/convert.c b/convert.c\nindex bb2435a..01de8ef 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,29 @@ 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 int init_user_convert_tail;\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+\tstruct git_attr_result result = GIT_ATTR_RESULT_INIT;\n+\n+\tgit_attr_check_initl(&check, \"crlf\", \"ident\", \"filter\",\n+\t\t\t     \"eol\", \"text\", NULL);\n+\tgit_attr_result_init(&result, check);\n+\n+\tif (!init_user_convert_tail) {\n \t\tuser_convert_tail = &user_convert;\n \t\tgit_config(read_convert_config, NULL);\n+\t\tinit_user_convert_tail = 1;\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)\n@@ -820,6 +817,8 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t\tca->crlf_action = CRLF_AUTO_CRLF;\n \tif (ca->crlf_action == CRLF_UNDEFINED && auto_crlf == AUTO_CRLF_INPUT)\n \t\tca->crlf_action = CRLF_AUTO_INPUT;\n+\n+\tgit_attr_result_clear(&result);\n }\n \n int would_convert_to_git_filter_fd(const char *path)\ndiff --git a/ll-merge.c b/ll-merge.c\nindex bc6479c..3abc77a 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,14 +397,19 @@ 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+\n+\tstruct git_attr_result result = GIT_ATTR_RESULT_INIT;\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+\tgit_attr_check_initl(&check, \"conflict-marker-size\", NULL);\n+\tgit_attr_result_init(&result, check);\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}\n+\tgit_attr_result_clear(&result);\n+\n \treturn marker_size;\n }\ndiff --git a/userdiff.c b/userdiff.c\nindex 46dfd32..62b1ed6 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -264,20 +264,30 @@ struct userdiff_driver *userdiff_find_by_path(const char *path)\n {\n \tstatic struct git_attr_check *check;\n \n-\tif (!check)\n-\t\tcheck = git_attr_check_initl(\"diff\", NULL);\n+\tstruct git_attr_result result = GIT_ATTR_RESULT_INIT;\n+\tconst char *value;\n+\n \tif (!path)\n \t\treturn NULL;\n-\tif (git_check_attr(path, check))\n+\n+\tgit_attr_check_initl(&check, \"diff\", NULL);\n+\tgit_attr_result_init(&result, check);\n+\n+\tif (git_check_attr(path, check, &result)) {\n+\t\tgit_attr_result_clear(&result);\n \t\treturn NULL;\n+\t}\n+\n+\tvalue = result.value[0];\n+\tgit_attr_result_clear(&result);\n \n-\tif (ATTR_TRUE(check->check[0].value))\n+\tif (ATTR_TRUE(value))\n \t\treturn &driver_true;\n-\tif (ATTR_FALSE(check->check[0].value))\n+\tif (ATTR_FALSE(value))\n \t\treturn &driver_false;\n-\tif (ATTR_UNSET(check->check[0].value))\n+\tif (ATTR_UNSET(value))\n \t\treturn NULL;\n-\treturn userdiff_find_by_name(check->check[0].value);\n+\treturn userdiff_find_by_name(value);\n }\n \n struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver)\ndiff --git a/ws.c b/ws.c\nindex bb3270c..5e30fef 100644\n--- a/ws.c\n+++ b/ws.c\n@@ -73,15 +73,20 @@ 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 \n-\tif (!attr_whitespace_rule)\n-\t\tattr_whitespace_rule = git_attr_check_initl(\"whitespace\", NULL);\n+\tstruct git_attr_result result = GIT_ATTR_RESULT_INIT;\n+\tconst char *value;\n+\tint r;\n \n-\tif (!git_check_attr(pathname, attr_whitespace_rule)) {\n-\t\tconst char *value;\n+\tgit_attr_check_initl(&check, \"whitespace\", NULL);\n+\tgit_attr_result_init(&result, check);\n \n-\t\tvalue = attr_whitespace_rule->check[0].value;\n+\tr = git_check_attr(pathname, check, &result);\n+\tvalue = result.value[0];\n+\tgit_attr_result_clear(&result);\n+\n+\tif (!r) {\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":"304048","messageId":"xmqqfuo116t0.fsf@gitster.mtv.corp.google.com","threadId":"44293","inReplyTo":"20161012224109.23410-1-sbeller@google.com","subject":"Re: [PATCHv3] attr: convert to new threadsafe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-12T23:33:31Z","receivedAt":"2016-10-12T23:40:22Z","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> @@ -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\nThis looks stale by a few revisions of the other parts of the patch?\n\n> diff --git a/archive.c b/archive.c\n> index 11e3951..15849a8 100644\n> --- a/archive.c\n> +++ b/archive.c\n> @@ -107,10 +107,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>  \t\tvoid *context)\n>  {\n>  \tstatic struct strbuf path = STRBUF_INIT;\n> +\tstatic struct git_attr_check *check;\n> +\n>  \tstruct archiver_context *c = context;\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> +\tstruct git_attr_result result = GIT_ATTR_RESULT_INIT;\n>  \tconst char *path_without_prefix;\n>  \tint err;\n>  \n> @@ -124,12 +126,16 @@ 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> +\tgit_attr_check_initl(&check, \"export-ignore\", \"export-subst\", NULL);\n> +\tgit_attr_result_init(&result, check);\n> +\n> +\tif (!git_check_attr(path_without_prefix, check, &result)) {\n> +\t\tif (ATTR_TRUE(result.value[0])) {\n> +\t\t\tgit_attr_result_clear(&result);\n>  \t\t\treturn 0;\n> -\t\targs->convert = ATTR_TRUE(check->check[1].value);\n> +\t\t}\n> +\t\targs->convert = ATTR_TRUE(result.value[1]);\n> +\t\tgit_attr_result_clear(&result);\n>  \t}\n\nThis is exactly what I meant by \"can we avoid alloc/free of result\nin leaf function when we _know_ how many attributes we are\ninterested in already, which is the majority of the case?\".\n\nStarting with a simple but unoptimized internal implementation of\nthe attr subsystem is one thing (which is good).  Exposing an API that\ncannot be optimally implemented later without changing the callers\nis another (which is bad).\n\nBy encapsulating each element into \"struct git_attr_result\", we can\nextend the API without changing the API user [*1*].  \n\nBut I do not think of a way to allow an efficient implementation\nlater unless the source of the API user somehow says \"this user is\nonly interested in this many attributes\", like having this\n\n\tstruct git_attr_result result[2];\n\n(because this caller only wants \"ignore\" and \"subst\") on the API\nuser's side [*2*].  Without such a clue (like the patch above, that\nonly says \"there is a structure called 'result'\"), I do not think of\na way to avoid dynamic allocation on the API implementation side.\n\nAll the other callers in the patch (pack-objects, convert, ll-merge,\netc.) seem to share the exact same pattern.  Each of the leaf\nfunctions knows a fixed set of attributes it is interested in, the\ncaller iterates over many paths and makes calls to these leaf\nfunctions, and it is a waste to pay alloc/free overhead for each and\nevery iteration when we know how many elements result needs to\nstore.\n\n\n[Footnote]\n\n*1* Would we need a wrapping struct around the array of results?  If\n    that is the case, we may need something ugly like this on the\n    API user side:\n\n\tGIT_ATTR_RESULT_TYPE(2) result = {2,};\n\n    with something like the following on the API implementation\n    side:\n\n        #define GIT_ATTR_RESULT_TYPE(n) \\\n            struct { \\\n                    int num_slots; \\\n                    const char *value[n]; \\\n            }\n\n        struct git_attr_result {\n                int num_slots;\n                const char *value[FLEX_ARRAY];\n        };\n        git_attr_result_init(void *result_, struct git_attr_check *check)\n        {\n                struct git_attr_result *result = result_;\n\n                assert(result->num_slots, check->num_attrs);\n                ...\n        }                \n        git_check_attr(const char *path,\n                       struct git_attr_check *check,\n                       void *result_)\n        {                       \n                struct git_attr_result *result = result_;\n\n                assert(result->num_slots, check->num_attrs);\n                for (i = 0; i < check_num_attrs; i++)\n                        result->value[i] = ... found value ...;\n        }\n\n\n*2* Or the uglier\n\n\tGIT_ATTR_RESULT_TYPE(2) result = {2,};\n\n    I can see why the \"check\" side would benefit from a structure\n    that contains an array, but I do not see why \"result\" side would\n    want to, so I am hoping that we won't have to do this uglier\n    variant and just go with the simple \"array of resulting values\".\n"},{"id":"304090","messageId":"CAGZ79kYEefzKJT5TLXaGV0ZYoW=OUzrRBPTOovDG0ofO8-i5Jg@mail.gmail.com","threadId":"44293","inReplyTo":"xmqqfuo116t0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] attr: convert to new threadsafe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-13T18:42:18Z","receivedAt":"2016-10-13T18:48:24Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 12, 2016 at 4:33 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> @@ -89,15 +114,20 @@ static void setup_check(void)\n>>\n>>  ------------\n>>       const char *path;\n>> +     struct git_attr_result *result;\n>>\n>>       setup_check();\n>> -     git_check_attr(path, check);\n>> +     result = git_check_attr(path, check);\n>\n> This looks stale by a few revisions of the other parts of the patch?\n\nI'll update the documentation.\n\n>\n>> diff --git a/archive.c b/archive.c\n>> index 11e3951..15849a8 100644\n>> --- a/archive.c\n>> +++ b/archive.c\n>> @@ -107,10 +107,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>>               void *context)\n>>  {\n>>       static struct strbuf path = STRBUF_INIT;\n>> +     static struct git_attr_check *check;\n>> +\n>>       struct archiver_context *c = context;\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>> +     struct git_attr_result result = GIT_ATTR_RESULT_INIT;\n>>       const char *path_without_prefix;\n>>       int err;\n>>\n>> @@ -124,12 +126,16 @@ 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>> +     git_attr_check_initl(&check, \"export-ignore\", \"export-subst\", NULL);\n>> +     git_attr_result_init(&result, check);\n>> +\n>> +     if (!git_check_attr(path_without_prefix, check, &result)) {\n>> +             if (ATTR_TRUE(result.value[0])) {\n>> +                     git_attr_result_clear(&result);\n>>                       return 0;\n>> -             args->convert = ATTR_TRUE(check->check[1].value);\n>> +             }\n>> +             args->convert = ATTR_TRUE(result.value[1]);\n>> +             git_attr_result_clear(&result);\n>>       }\n>\n> This is exactly what I meant by \"can we avoid alloc/free of result\n> in leaf function when we _know_ how many attributes we are\n> interested in already, which is the majority of the case?\".\n\nWe can avoid that. For now I settled with an implementation that\nhas no \"answer\" type, but uses a bare \"const char *result[FLEX_ARRAY];\",\nor rather a const char **.\n\n>\n> Starting with a simple but unoptimized internal implementation of\n> the attr subsystem is one thing (which is good).  Exposing an API that\n> cannot be optimally implemented later without changing the callers\n> is another (which is bad).\n>\n> By encapsulating each element into \"struct git_attr_result\", we can\n> extend the API without changing the API user [*1*].\n\nOh I see.\n\nSo instead of a raw string we want to have\n\nstruct git_attr_result {\n    const char *value;\n};\n\njust to have it extensible. Makes sense. I'll redo that.\n\n\n>\n> But I do not think of a way to allow an efficient implementation\n> later unless the source of the API user somehow says \"this user is\n> only interested in this many attributes\", like having this\n>\n>         struct git_attr_result result[2];\n\n    const char *result[2] = {NULL, NULL};\n\nas of now would be\n\n    struct git_attr_result result[2];\n\nbut we'd lose the ability to set them to NULL easily. Probably not needed.\n\n>\n> (because this caller only wants \"ignore\" and \"subst\") on the API\n> user's side [*2*].  Without such a clue (like the patch above, that\n> only says \"there is a structure called 'result'\"), I do not think of\n> a way to avoid dynamic allocation on the API implementation side.\n>\n> All the other callers in the patch (pack-objects, convert, ll-merge,\n> etc.) seem to share the exact same pattern.  Each of the leaf\n> functions knows a fixed set of attributes it is interested in, the\n> caller iterates over many paths and makes calls to these leaf\n> functions, and it is a waste to pay alloc/free overhead for each and\n> every iteration when we know how many elements result needs to\n> store.\n>\n\nRight.\n\n>\n> [Footnote]\n>\n> *1* Would we need a wrapping struct around the array of results?  If\n>     that is the case, we may need something ugly like this on the\n>     API user side:\n>\n>         GIT_ATTR_RESULT_TYPE(2) result = {2,};\n>\n>     with something like the following on the API implementation\n>     side:\n>\n>         #define GIT_ATTR_RESULT_TYPE(n) \\\n>             struct { \\\n>                     int num_slots; \\\n>                     const char *value[n]; \\\n>             }\n>\n>         struct git_attr_result {\n>                 int num_slots;\n>                 const char *value[FLEX_ARRAY];\n>         };\n>         git_attr_result_init(void *result_, struct git_attr_check *check)\n>         {\n>                 struct git_attr_result *result = result_;\n>\n>                 assert(result->num_slots, check->num_attrs);\n>                 ...\n>         }\n>         git_check_attr(const char *path,\n>                        struct git_attr_check *check,\n>                        void *result_)\n>         {\n>                 struct git_attr_result *result = result_;\n>\n>                 assert(result->num_slots, check->num_attrs);\n>                 for (i = 0; i < check_num_attrs; i++)\n>                         result->value[i] = ... found value ...;\n>         }\n>\n>\n> *2* Or the uglier\n>\n>         GIT_ATTR_RESULT_TYPE(2) result = {2,};\n>\n>     I can see why the \"check\" side would benefit from a structure\n>     that contains an array, but I do not see why \"result\" side would\n>     want to, so I am hoping that we won't have to do this uglier\n>     variant and just go with the simple \"array of resulting values\".\n\nSo I currently have the \"array of resulting values\", but not wrapped.\n\nDo we expect to get more than the values out of the attr system?\n"},{"id":"304096","messageId":"CAGZ79kasCEWijdUe24mTy9n--BX2=ar4V-hvjXHqEm2sVzjk+g@mail.gmail.com","threadId":"44293","inReplyTo":"CAGZ79kYEefzKJT5TLXaGV0ZYoW=OUzrRBPTOovDG0ofO8-i5Jg@mail.gmail.com","subject":"Re: [PATCHv3] attr: convert to new threadsafe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-13T22:08:35Z","receivedAt":"2016-10-13T22:16:42Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> On Wed, Oct 12, 2016 at 4:33 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> so I am hoping that we won't have to do this uglier variant\n\n---8<--- attr.h:\n...\nstruct git_attr_result {\n    int check_nr;\n    /* Whether is was statically allocated and cannot be resized. */\n    int static_alloc;\n    const char *value[FLEX_ARRAY];\n};\n\n/* create a pointer pointing to a git_attr_result with a given size: */\n#define GIT_ATTR_RESULT_INIT_FOR(name, size) \\\n    struct { \\\n        int check_nr; \\\n        int static_alloc; \\\n        const char *value[size]; \\\n    } local_##name; \\\n    struct git_attr_result *name = \\\n        (struct git_attr_result *)&(local_##name); \\\n    local_##name.static_alloc = 1;\n...\n---8<--- e.g. ws.c:\n\n    static struct git_attr_check *attr_whitespace_rule;\n    GIT_ATTR_RESULT_INIT_FOR(result, 1);\n\n    git_attr_check_initl(&attr_whitespace_rule, \"whitespace\", NULL);\n\n    if (!git_check_attr(pathname, attr_whitespace_rule, result)) {\n        if (ATTR_TRUE(result->value[0])) {\n            ...\n        } else if (ATTR_FALSE(result->value[0])) {\n            ...\n"},{"id":"304168","messageId":"xmqq8ttr0wny.fsf@gitster.mtv.corp.google.com","threadId":"44293","inReplyTo":"xmqqfuo116t0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] attr: convert to new threadsafe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-14T15:37:05Z","receivedAt":"2016-10-14T15:38:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> *1* Would we need a wrapping struct around the array of results?\n\nBy the way, I do see a merit on the \"check\" side (tl;dr: but I do\nnot think \"result\" needs it, hence I do not see the need for the\n\"ugly\" variants).\n\nTake \"archive\" for example.  For each path, it wants to see the\nattribute \"export-ignore\" to decide if it is to be omitted.  In\naddition, the usual set of attributes used to smudge blobs into the\nworking tree representation are inspected by the convert.c API as\npart of its implementation of convert_to_working_tree().  This\nprogram has at least two sets of <\"check\", \"result\"> that are used\nby two git_check_attr() callsites that are unaware of each other.\n\nOne of the optimizations we discussed is to trim down the attr-stack\n(which caches the attributes read from .gitattributes files that are\nin effect for the \"last\" directory that has the path for which\nattrbiutes are queried for) by reading/keeping only the entries that\naffect the attributes the caller is interested in.  But when there\nare multiple callsites that are interested in different sets of\nattributes, we obviously cannot do such an optimization without\ntaking too much cache-invalidation hit.  Because these callsites are\nnot unaware of each other, I do not think we can say \"keep the\nentries that affects the union of all active callsites\" very easily,\neven if it were possible.\n\nBut we could tie this cache to \"check\", which keeps a constant\nsubset of attributes that the caller is interested in (i.e. each\ncallsite would keep its own cache that is useful for its query).\nWhile we are single-threaded, \"struct git_attr_check\" being a\nwrapping struct around the array of \"what attributes are of\ninterest?\" is a good place to add that per-check attr-stack cache.\nWhen we go multi-threaded, the attr-stack cache must become\nper-thread, and needs to be moved to per-thread storage, and such a\nper-thread storage would have multiple attr-stack, one per \"check\"\ninstance (i.e. looking up the attr-stack may have to say \"who/what\nthread am I?\" to first go to the thread-local storage for the\ncurrent thread, where a table of pointers to attr-stacks is kept and\nfrom there, index into that table to find the attr-stack that\ncorresponds to the particular \"check\").  We could use the address of\n\"check\" as the key into this table, but \"struct git_attr_check\" that\nwraps the array gives us another option to allocate a small\nconsecutive integer every time initl() creates a new \"check\" and use\nit as the index into that attr-stack table, as that integer index\ncan be in the struct that wraps the array of wanted attributes.\n\n\tNote. none of the above is a suggestion to do the attr\n\tcaching the way exactly described.  The above is primarily\n\tto illustrate how a wrapping struct may give us future\n\tflexibility without affecting a single line of code in the\n\tuser of API.\n\nIt may turn out that we do not need to have anything other than the\narray of wanted attributes in the \"check\" struct, but unlike\n\"result\", \"check\" is shared across threads, and do not have to live\ndirectly on the stack, so we can prepare for flexibility.\n\nI do not foresee a similar need for wrapping struct for \"result\",\nand given that we do want to keep the option of having them directly\non the stack, I am inclined to say we shouldn't introduce one.\n\nIf we were still to do the wrapping for result, I would say that\nbasing it around the FLEX_ARRAY idiom, i.e.\n\n>         struct git_attr_result {\n>                 int num_slots;\n>                 const char *value[FLEX_ARRAY];\n>         };\n\nis a horrible idea.  It would be less horrible if it were\n\n\tstruct git_attr_result {\n\t\tint num_slots;\n\t\tconst char **value;\n\t};\n\nthen make the API user write via a convenience macro something like\nthis\n\n\tconst char *result_values[NUM_ATTRS_OF_INTEREST];\n\tstruct git_attr_result result = {\n\t\tARRAY_SIZE(result_values), &result_values\n\t};                \n\ninstead.  That way, at least the side that implements git_check_attr()\nwould not have to be type-unsafe like the example of ugliness in the\nmessage I am following-up on.\n"},{"id":"304367","messageId":"CAGZ79kbS4mP7sVTCM+QJXTwKsgZ40xvVDng-F3igZnJWLYek0A@mail.gmail.com","threadId":"44293","inReplyTo":"xmqq8ttr0wny.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] attr: convert to new threadsafe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-18T23:52:51Z","receivedAt":"2016-10-18T23:52:58Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Oct 14, 2016 at 8:37 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> *1* Would we need a wrapping struct around the array of results?\n>\n> By the way, I do see a merit on the \"check\" side (tl;dr: but I do\n> not think \"result\" needs it, hence I do not see the need for the\n> \"ugly\" variants).\n\nSo we'd rather go with const char **result instead of our own new struct there.\nOk, got it.\n\n>\n> Take \"archive\" for example.  For each path, it wants to see the\n> attribute \"export-ignore\" to decide if it is to be omitted.  In\n> addition, the usual set of attributes used to smudge blobs into the\n> working tree representation are inspected by the convert.c API as\n> part of its implementation of convert_to_working_tree().  This\n> program has at least two sets of <\"check\", \"result\"> that are used\n> by two git_check_attr() callsites that are unaware of each other.\n>\n> One of the optimizations we discussed is to trim down the attr-stack\n> (which caches the attributes read from .gitattributes files that are\n> in effect for the \"last\" directory that has the path for which\n> attrbiutes are queried for) by reading/keeping only the entries that\n> affect the attributes the caller is interested in.  But when there\n> are multiple callsites that are interested in different sets of\n> attributes, we obviously cannot do such an optimization without\n> taking too much cache-invalidation hit.  Because these callsites are\n> not unaware of each other, I do not think we can say \"keep the\n> entries that affects the union of all active callsites\" very easily,\n> even if it were possible.\n>\n> But we could tie this cache to \"check\", which keeps a constant\n> subset of attributes that the caller is interested in (i.e. each\n> callsite would keep its own cache that is useful for its query).\n> While we are single-threaded, \"struct git_attr_check\" being a\n> wrapping struct around the array of \"what attributes are of\n> interest?\" is a good place to add that per-check attr-stack cache.\n> When we go multi-threaded, the attr-stack cache must become\n> per-thread, and needs to be moved to per-thread storage, and such a\n> per-thread storage would have multiple attr-stack, one per \"check\"\n> instance (i.e. looking up the attr-stack may have to say \"who/what\n> thread am I?\" to first go to the thread-local storage for the\n> current thread, where a table of pointers to attr-stacks is kept and\n> from there, index into that table to find the attr-stack that\n> corresponds to the particular \"check\").  We could use the address of\n> \"check\" as the key into this table, but \"struct git_attr_check\" that\n> wraps the array gives us another option to allocate a small\n> consecutive integer every time initl() creates a new \"check\" and use\n> it as the index into that attr-stack table, as that integer index\n> can be in the struct that wraps the array of wanted attributes.\n>\n>         Note. none of the above is a suggestion to do the attr\n>         caching the way exactly described.  The above is primarily\n>         to illustrate how a wrapping struct may give us future\n>         flexibility without affecting a single line of code in the\n>         user of API.\n>\n> It may turn out that we do not need to have anything other than the\n> array of wanted attributes in the \"check\" struct, but unlike\n> \"result\", \"check\" is shared across threads, and do not have to live\n> directly on the stack, so we can prepare for flexibility.\n>\n> I do not foresee a similar need for wrapping struct for \"result\",\n> and given that we do want to keep the option of having them directly\n> on the stack, I am inclined to say we shouldn't introduce one.\n>\n> If we were still to do the wrapping for result, I would say that\n> basing it around the FLEX_ARRAY idiom, i.e.\n>\n>>         struct git_attr_result {\n>>                 int num_slots;\n>>                 const char *value[FLEX_ARRAY];\n>>         };\n>\n> is a horrible idea.  It would be less horrible if it were\n>\n>         struct git_attr_result {\n>                 int num_slots;\n>                 const char **value;\n>         };\n\nSo const char** but with an additional number of slots, all we do\nwould be to compare this number of slots to the checks number of slots and\ndie(\"BUG:...\"),  which is just a burden and no help.\n\n>\n> then make the API user write via a convenience macro something like\n> this\n>\n>         const char *result_values[NUM_ATTRS_OF_INTEREST];\n>         struct git_attr_result result = {\n>                 ARRAY_SIZE(result_values), &result_values\n>         };\n>\n> instead.  That way, at least the side that implements git_check_attr()\n> would not have to be type-unsafe like the example of ugliness in the\n> message I am following-up on.\n\nOk I will reroll with the const char** instead of the macro stuff that\nI came up with,\n(that would be type safe though uglier than the pure variant).\n"},{"id":"304368","messageId":"CAPc5daVF2HBD2DfP=w2oG-2PMWQ+guXP_onQX+foSgVYH4N1Qg@mail.gmail.com","threadId":"44293","inReplyTo":"CAGZ79kbS4mP7sVTCM+QJXTwKsgZ40xvVDng-F3igZnJWLYek0A@mail.gmail.com","subject":"Re: [PATCHv3] attr: convert to new threadsafe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-19T00:06:55Z","receivedAt":"2016-10-19T00:07:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Oct 18, 2016 at 4:52 PM, Stefan Beller <sbeller@google.com> wrote:\n>\n> >\n> > By the way, I do see a merit on the \"check\" side (tl;dr: but I do\n> > not think \"result\" needs it, hence I do not see the need for the\n> > \"ugly\" variants).\n>\n> So we'd rather go with const char **result instead of our own new struct there.\n> Ok, got it.\n\nI do not think you got it. I am talking about wrapping struct around\nan array of element,\nnot each element in the array. IOW\n\n> > If we were still to do the wrapping for result, I would say that\n> > basing it around the FLEX_ARRAY idiom, i.e.\n> >\n> >>         struct git_attr_result {\n> >>                 int num_slots;\n> >>                 const char *value[FLEX_ARRAY];\n> >>         };\n\nthe structure around the array of elements (value) that allows us to have\nsomething other than value[] in it. That is what I said \"I do not see\nthe need for\".\n\nIt is perfectly fine future-proofing to have\n\nstruct git_attr_result_value {\n  const char *value;\n};\n\nand have the users of API declare\n\nstruct git_attr_result value[5];\n\nor whatever. That way we could fatten the structure later if we wanted\nto without having to update the users of API, and there is no downside.\n\nHaving wrapping strut around the array does have a huge downside,\nand that is what I said I see no need for.\n"},{"id":"304369","messageId":"CAGZ79kbq7m6s5s8OCXFwyonUdiaL3nVNCoGS40xkH2qKCkYzag@mail.gmail.com","threadId":"44293","inReplyTo":"CAPc5daVF2HBD2DfP=w2oG-2PMWQ+guXP_onQX+foSgVYH4N1Qg@mail.gmail.com","subject":"Re: [PATCHv3] attr: convert to new threadsafe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-19T00:20:53Z","receivedAt":"2016-10-19T00:21:00Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 18, 2016 at 5:06 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> On Tue, Oct 18, 2016 at 4:52 PM, Stefan Beller <sbeller@google.com> wrote:\n>>\n>> >\n>> > By the way, I do see a merit on the \"check\" side (tl;dr: but I do\n>> > not think \"result\" needs it, hence I do not see the need for the\n>> > \"ugly\" variants).\n>>\n>> So we'd rather go with const char **result instead of our own new struct there.\n>> Ok, got it.\n>\n> I do not think you got it. I am talking about wrapping struct around\n> an array of element,\n> not each element in the array. IOW\n>\n>> > If we were still to do the wrapping for result, I would say that\n>> > basing it around the FLEX_ARRAY idiom, i.e.\n>> >\n>> >>         struct git_attr_result {\n>> >>                 int num_slots;\n>> >>                 const char *value[FLEX_ARRAY];\n>> >>         };\n>\n> the structure around the array of elements (value) that allows us to have\n> something other than value[] in it. That is what I said \"I do not see\n> the need for\".\n>\n> It is perfectly fine future-proofing to have\n>\n> struct git_attr_result_value {\n>   const char *value;\n> };\n>\n> and have the users of API declare\n>\n> struct git_attr_result value[5];\n>\n> or whatever. That way we could fatten the structure later if we wanted\n> to without having to update the users of API, and there is no downside.\n>\n> Having wrapping strut around the array does have a huge downside,\n> and that is what I said I see no need for.\n\nI am not sure if I see the upside on wrapping a single value except for\nits future proofness, i.e. what if we want to transport information that\nis valid for all values, e.g. an error code or that the result check was done\nlazily (for non lazy you would need to do X) or ...\n\nIOW I would expect there to be more use cases for information regarding\nall the values and not each value enhanced by a thing.\n\nWe could just repeat the thing in each of the 5 values in\n> struct git_attr_result value[5];\nthough.\n\nI'll go with that.\n"},{"id":"304370","messageId":"xmqqbmyhnpby.fsf@gitster.mtv.corp.google.com","threadId":"44293","inReplyTo":"CAGZ79kbq7m6s5s8OCXFwyonUdiaL3nVNCoGS40xkH2qKCkYzag@mail.gmail.com","subject":"Re: [PATCHv3] attr: convert to new threadsafe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-19T00:40:33Z","receivedAt":"2016-10-19T00:40:43Z","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 I see the upside on wrapping a single value except for\n> its future proofness,\n\nI do not see anything other than future-proofing, either.  If we\nneed to touch all the code that uses the attributes to update the\nAPI, I'd prefer to avoid having to do that again in the future.\n"}]}