{"thread":{"id":"44221","subject":"[RFC/PATCH] attr: Document a new possible thread safe API","startedAt":"2016-10-04T22:14:45Z","lastAt":"2016-10-05T22:10:05Z","messageCount":6,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"303323","messageId":"20161004221433.23747-1-sbeller@google.com","threadId":"44221","inReplyTo":null,"subject":"[RFC/PATCH] attr: Document a new possible thread safe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-04T22:14:33Z","receivedAt":"2016-10-04T22:14:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is what we want to see at the end of the refactoring session\nto enable the attr subsystem to be thread safe.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nJunio wrote:  \n>> So how would we go about git_all_attrs then?\n>\n> I think you arrived the same conclusion, but ...\n  \nI think the changes as proposed are minimal to be able to use attrs in a\nmultithreaded environment. \n  \nThis patch applies on top of jc/attr-more.\nA complete diff between origin/master..HEAD for the documentation is\nappended below.\n  \nThanks,\nStefan\n\n Documentation/technical/api-gitattributes.txt | 22 ++++++++++++++--------\n 1 file changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt\nindex 92fc32a..940617e 100644\n--- a/Documentation/technical/api-gitattributes.txt\n+++ b/Documentation/technical/api-gitattributes.txt\n@@ -59,7 +59,10 @@ Querying Specific Attributes\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+  function. git_attr_check_initl is thread safe, i.e. you can call it\n+  from different threads at the same time; internally however only one\n+  call at a time is processed. If the calls from different threads have\n+  the same arguments, the returned `git_attr_check` may be the same.\n \n * Call `git_check_attr()` to check the attributes for the path.\n \n@@ -89,15 +92,21 @@ static void setup_check(void)\n \n ------------\n \tconst char *path;\n+\tstruct git_attr_check *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 may be the same as `check` (in a single threaded application),\n+but generally assume it is not. The `result` must not be free'd as it is\n+owned by the attr subsystem. It is reused by the same thread, so a subsequent\n+call to git_check_attr in the same thread will overwrite the result.\n+\n+. Act on `.value` member of the `result->check[]`:\n \n ------------\n-\tconst char *value = check->check[0].value;\n+\tconst char *value = result->check[0].value;\n \n \tif (ATTR_TRUE(value)) {\n \t\tThe attribute is Set, by listing only the name of the\n@@ -138,10 +147,7 @@ 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-\n-* Call `git_all_attrs()`, which populates the `git_attr_check`\n+* Call `git_all_attrs()`, which returns a `git_attr_check`\n   with the attributes attached to the path.\n \n * Iterate over the `git_attr_check.check[]` array to examine\n-- \n2.10.0.129.g35f6318\n\n\n\n\n\n\nThe following is `git diff origin/master Documentation/technical/api-gitattributes.txt`:\n\ndiff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt\nindex 2602668..940617e 100644\n--- a/Documentation/technical/api-gitattributes.txt\n+++ b/Documentation/technical/api-gitattributes.txt\n@@ -16,10 +16,15 @@ 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 set of attributes to check in a call\n-\tto `git_check_attr()` function, and receives the results.\n+\tThis structure represents a collection of `git_attr_check_elem`.\n+\tIt is passed to `git_check_attr()` function, specifying the\n+\tattributes to check, and receives their values.\n \n \n Attribute Values\n@@ -48,49 +53,60 @@ value of the attribute for the path.\n Querying Specific Attributes\n ----------------------------\n \n-* Prepare an array of `struct git_attr_check` to define the list of\n-  attributes you would want to check.  To populate this array, you would\n-  need to define necessary attributes by calling `git_attr()` function.\n+* Prepare `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. git_attr_check_initl is thread safe, i.e. you can call it\n+  from different threads at the same time; internally however only one\n+  call at a time is processed. If the calls from different threads have\n+  the same arguments, the returned `git_attr_check` may be the same.\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 attribute in\n-  the array is defined for the path.\n+* Inspect `git_attr_check` structure to see how each of the\n+  attribute in the array is defined for the path.\n \n \n Example\n -------\n \n-To see how attributes \"crlf\" and \"indent\" are set for different paths.\n+To see how attributes \"crlf\" and \"ident\" are set for different paths.\n \n-. Prepare an array of `struct git_attr_check` with two elements (because\n-  we are checking two attributes).  Initialize their `attr` member with\n-  pointers to `struct git_attr` obtained by calling `git_attr()`:\n+. Prepare a `struct git_attr_check` with two elements (because\n+  we are checking two attributes):\n \n ------------\n-static struct git_attr_check check[2];\n+static struct git_attr_check *check;\n static void setup_check(void)\n {\n-\tif (check[0].attr)\n+\tif (check)\n \t\treturn; /* already done */\n-\tcheck[0].attr = git_attr(\"crlf\");\n-\tcheck[1].attr = git_attr(\"ident\");\n+\tcheck = git_attr_check_initl(\"crlf\", \"ident\", NULL);\n }\n ------------\n \n-. Call `git_check_attr()` with the prepared array of `struct git_attr_check`:\n+. Call `git_check_attr()` with the prepared `struct git_attr_check`:\n \n ------------\n \tconst char *path;\n+\tstruct git_attr_check *result;\n \n \tsetup_check();\n-\tgit_check_attr(path, ARRAY_SIZE(check), check);\n+\tresult = git_check_attr(path, check);\n ------------\n \n-. Act on `.value` member of the result, left in `check[]`:\n+The result may be the same as `check` (in a single threaded application),\n+but generally assume it is not. The `result` must not be free'd as it is\n+owned by the attr subsystem. It is reused by the same thread, so a subsequent\n+call to git_check_attr in the same thread will overwrite the result.\n+\n+. Act on `.value` member of the `result->check[]`:\n \n ------------\n-\tconst char *value = check[0].value;\n+\tconst char *value = result->check[0].value;\n \n \tif (ATTR_TRUE(value)) {\n \t\tThe attribute is Set, by listing only the name of the\n@@ -109,20 +125,36 @@ static void setup_check(void)\n \t}\n ------------\n \n+To see how attributes in argv[] are set for different paths, only\n+the first step in the above would be different.\n+\n+------------\n+static struct git_attr_check *check;\n+static void setup_check(const char **argv)\n+{\n+\tcheck = git_attr_check_alloc();\n+\twhile (*argv) {\n+\t\tstruct git_attr *attr = git_attr(*argv);\n+\t\tgit_attr_check_append(check, attr);\n+\t\targv++;\n+\t}\n+}\n+------------\n+\n \n Querying All Attributes\n -----------------------\n \n To get the values of all attributes associated with a file:\n \n-* Call `git_all_attrs()`, which returns an array of `git_attr_check`\n-  structures.\n+* Call `git_all_attrs()`, which returns a `git_attr_check`\n+  with the attributes attached to the path.\n \n-* Iterate over the `git_attr_check` array to examine the attribute\n-  names and values.  The name of the attribute described by a\n-  `git_attr_check` object can be retrieved via\n-  `git_attr_name(check[i].attr)`.  (Please note that no items will be\n-  returned for unset attributes, so `ATTR_UNSET()` will return false\n-  for all returned `git_array_check` objects.)\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+  will be returned for unset attributes, so `ATTR_UNSET()` will return\n+  false for all returned `git_array_check` objects.)\n \n-* Free the `git_array_check` array.\n+* Free the `git_array_check` by calling `git_attr_check_free()`.\n\n\n\n\n"},{"id":"303334","messageId":"xmqqtwcrr9l6.fsf@gitster.mtv.corp.google.com","threadId":"44221","inReplyTo":"20161004221433.23747-1-sbeller@google.com","subject":"Re: [RFC/PATCH] attr: Document a new possible thread safe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T23:13:57Z","receivedAt":"2016-10-04T23:14:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt\n> index 92fc32a..940617e 100644\n> --- a/Documentation/technical/api-gitattributes.txt\n> +++ b/Documentation/technical/api-gitattributes.txt\n> @@ -59,7 +59,10 @@ Querying Specific Attributes\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> +  function. git_attr_check_initl is thread safe, i.e. you can call it\n> +  from different threads at the same time; internally however only one\n> +  call at a time is processed. If the calls from different threads have\n> +  the same arguments, the returned `git_attr_check` may be the same.\n\nI do not think this is enough.  Look at the example for _initl() and\nnotice that it keeps the \"singleton static check that is initialized\nby the very first caller if the caller notices it is NULL\" pattern.\n\nOne way to hide that may be to pass the address of that singleton\npointer to _initl(), so that it can do the \"has it been initialized?\nIf not, let's prepare the thing\" under lock.\n\n> @@ -89,15 +92,21 @@ static void setup_check(void)\n>  \n>  ------------\n>  \tconst char *path;\n> +\tstruct git_attr_check *result;\n>  \n>  \tsetup_check();\n> -\tgit_check_attr(path, check);\n> +\tresult = git_check_attr(path, check);\n\nI haven't formed a firm opinion, but I suspect your thinking might\nbe clouded by too much staring at the current implementation that\nhas <attr>,<value> pairs inside git_attr_check.  Traditionally, the\nattr subsystem used the same type for the query question and the\nquery answer the same type, but it does not have to stay to be the\ncase at all.  Have you considered that we are allowed to make these\ntwo types distinct?  A caller can share the same question instance\n(i.e. the set of interned <attr>, in git_attr_check) with other\nthreads as that is a read-only thing, but each of the callers would\nwant to have the result array on its own stack if possible\n(e.g. when asking for a known set of attributes, which is the\nmajority of the case) to avoid allocation cost.  I'd expect the most\ntypical caller to be\n\n\tstatic struct git_attr_check *check;\n\tstruct git_attr_result result[2]; /* we want two */\n\t\n\tgit_attr_check_initl(&check, \"crlf\", \"ident\", NULL);\n\tgit_check_attr(path, check, result);\n\t/* result[0] has \"crlf\", result[1] has \"ident\" */\n\nor something like that.\n"},{"id":"303335","messageId":"CAGZ79kaFx++yipGmq=D2EPN4Gw9JhRXf_i4pFivTnkJmJXjFfg@mail.gmail.com","threadId":"44221","inReplyTo":"xmqqtwcrr9l6.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] attr: Document a new possible thread safe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-04T23:25:27Z","receivedAt":"2016-10-04T23:28:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 4, 2016 at 4:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt\n>> index 92fc32a..940617e 100644\n>> --- a/Documentation/technical/api-gitattributes.txt\n>> +++ b/Documentation/technical/api-gitattributes.txt\n>> @@ -59,7 +59,10 @@ Querying Specific Attributes\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>> +  function. git_attr_check_initl is thread safe, i.e. you can call it\n>> +  from different threads at the same time; internally however only one\n>> +  call at a time is processed. If the calls from different threads have\n>> +  the same arguments, the returned `git_attr_check` may be the same.\n>\n> I do not think this is enough.  Look at the example for _initl() and\n> notice that it keeps the \"singleton static check that is initialized\n> by the very first caller if the caller notices it is NULL\" pattern.\n>\n> One way to hide that may be to pass the address of that singleton\n> pointer to _initl(), so that it can do the \"has it been initialized?\n> If not, let's prepare the thing\" under lock.\n\nOh, I see. Yeah that makes sense.\n\n>\n>> @@ -89,15 +92,21 @@ static void setup_check(void)\n>>\n>>  ------------\n>>       const char *path;\n>> +     struct git_attr_check *result;\n>>\n>>       setup_check();\n>> -     git_check_attr(path, check);\n>> +     result = git_check_attr(path, check);\n>\n> I haven't formed a firm opinion, but I suspect your thinking might\n> be clouded by too much staring at the current implementation that\n> has <attr>,<value> pairs inside git_attr_check.  Traditionally, the\n> attr subsystem used the same type for the query question and the\n> query answer the same type, but it does not have to stay to be the\n> case at all.  Have you considered that we are allowed to make these\n> two types distinct?\n\nI thought about that, but as I concluded that the get_all_attrs doesn't need\nconversion to a threading environment, we can keep it as is.\n\nWhen keeping the get_all_attrs as is, we need to keep the data structures\nas is, (i.e. key,value pair inside git_check_attr), so introducing a new\ndata type seemed not useful for the threaded part.\n\n>  A caller can share the same question instance\n> (i.e. the set of interned <attr>, in git_attr_check) with other\n> threads as that is a read-only thing, but each of the callers would\n> want to have the result array on its own stack if possible\n> (e.g. when asking for a known set of attributes, which is the\n> majority of the case) to avoid allocation cost.  I'd expect the most\n> typical caller to be\n>\n>         static struct git_attr_check *check;\n>         struct git_attr_result result[2]; /* we want two */\n>\n>         git_attr_check_initl(&check, \"crlf\", \"ident\", NULL);\n>         git_check_attr(path, check, result);\n>         /* result[0] has \"crlf\", result[1] has \"ident\" */\n>\n> or something like that.\n\nI see, that seems to be a clean API. So git_attr_check_initl\nwill lock the mutex and once it got the mutex it can either\n* return early as someone else did the work\n* needs to do the actual work\nand then unlock. In any case the work was done.\n\ngit_check_attr, which runs in all threads points to the same check,\nbut gets the different results.\n\nOk, I'll go in that direction then.\n\nThanks,\nStefan\n"},{"id":"303391","messageId":"xmqq8tu2ras4.fsf@gitster.mtv.corp.google.com","threadId":"44221","inReplyTo":"CAGZ79kaFx++yipGmq=D2EPN4Gw9JhRXf_i4pFivTnkJmJXjFfg@mail.gmail.com","subject":"Re: [RFC/PATCH] attr: Document a new possible thread safe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-05T17:00:27Z","receivedAt":"2016-10-05T17:00:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> I thought about that, but as I concluded that the get_all_attrs doesn't need\n> conversion to a threading environment, we can keep it as is.\n\nI agree that it is OK for get_all_attrs() to use its own way to ask\na question and receive an answer to it, that is different from how\ngit_check_attr() asks its question.  The threading-support for it is\nan unrelated issue, though (not that I think it needs to be run from\na multi-threaded caller).\n\n>> ...  I'd expect the most\n>> typical caller to be\n>>\n>>         static struct git_attr_check *check;\n>>         struct git_attr_result result[2]; /* we want two */\n>>\n>>         git_attr_check_initl(&check, \"crlf\", \"ident\", NULL);\n>>         git_check_attr(path, check, result);\n>>         /* result[0] has \"crlf\", result[1] has \"ident\" */\n>>\n>> or something like that.\n>\n> I see, that seems to be a clean API. So git_attr_check_initl\n> will lock the mutex and once it got the mutex it can either\n> * return early as someone else did the work\n> * needs to do the actual work\n> and then unlock. In any case the work was done.\n>\n> git_check_attr, which runs in all threads points to the same check,\n> but gets the different results.\n\nYeah, something along that line.  It seems that we are now on the\nsame page?\n\nThanks.\n\n\n"},{"id":"303393","messageId":"CAGZ79kZ7zNkXBVUFq5aLSup9xsdCbZpy_x8z5=uAQqftmOq26A@mail.gmail.com","threadId":"44221","inReplyTo":"xmqq8tu2ras4.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] attr: Document a new possible thread safe API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-05T17:04:43Z","receivedAt":"2016-10-05T17:04:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 5, 2016 at 10:00 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> I thought about that, but as I concluded that the get_all_attrs doesn't need\n>> conversion to a threading environment, we can keep it as is.\n>\n> I agree that it is OK for get_all_attrs() to use its own way to ask\n> a question and receive an answer to it, that is different from how\n> git_check_attr() asks its question.  The threading-support for it is\n> an unrelated issue, though (not that I think it needs to be run from\n> a multi-threaded caller).\n>\n>>> ...  I'd expect the most\n>>> typical caller to be\n>>>\n>>>         static struct git_attr_check *check;\n>>>         struct git_attr_result result[2]; /* we want two */\n>>>\n>>>         git_attr_check_initl(&check, \"crlf\", \"ident\", NULL);\n>>>         git_check_attr(path, check, result);\n>>>         /* result[0] has \"crlf\", result[1] has \"ident\" */\n>>>\n>>> or something like that.\n>>\n>> I see, that seems to be a clean API. So git_attr_check_initl\n>> will lock the mutex and once it got the mutex it can either\n>> * return early as someone else did the work\n>> * needs to do the actual work\n>> and then unlock. In any case the work was done.\n>>\n>> git_check_attr, which runs in all threads points to the same check,\n>> but gets the different results.\n>\n> Yeah, something along that line.  It seems that we are now on the\n> same page?\n>\n\nI think so, instead of resending the documentation, maybe the header\nfile shows that we're on the same page, I converted everything except\nattr.c to follow this header attr.h:\n\n---8<---\n#ifndef ATTR_H\n#define ATTR_H\n\n/* An attribute is a pointer to this opaque structure */\nstruct git_attr;\n\n/*\n * Given a string, return the gitattribute object that\n * corresponds to it.\n */\nextern 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 */\nextern const char *git_attr_name(const struct git_attr *);\n\nextern int attr_name_valid(const char *name, size_t namelen);\nextern void invalid_attr_name_message(struct strbuf *, const char *, int);\n\n/* Internal use */\nextern const char git_attr__true[];\nextern const char git_attr__false[];\n\n/* For public to check git_attr_check results */\n#define ATTR_TRUE(v) ((v) == git_attr__true)\n#define ATTR_FALSE(v) ((v) == git_attr__false)\n#define ATTR_UNSET(v) ((v) == NULL)\n\nstruct git_attr_check {\n        int finalized;\n        int check_nr;\n        int check_alloc;\n        struct git_attr **attr;\n};\n\nstruct git_attr_result {\n        int check_nr;\n        int check_alloc;\n        const char **value;\n};\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, add more attributes via\n *                      git_attr_check_append.\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 */\nextern struct git_attr_check *git_attr_check_alloc(void);\nextern void git_attr_check_append(struct git_attr_check *, const\nstruct git_attr *);\nextern struct git_attr_check *git_all_attrs(const char *path);\nextern void git_attr_check_initl(struct git_attr_check **, const char *, ...);\n\n/* Query a path for its attributes */\nextern struct git_attr_result *git_check_attr(const char *path,\n                                             struct git_attr_check *);\n\n/* internal? */\nextern void git_attr_check_clear(struct git_attr_check *);\n\n/* After use free the check */\nextern void git_attr_check_free(struct git_attr_check *);\n\nenum git_attr_direction {\n        GIT_ATTR_CHECKIN,\n        GIT_ATTR_CHECKOUT,\n        GIT_ATTR_INDEX\n};\nvoid git_attr_set_direction(enum git_attr_direction, struct index_state *);\n\n#endif /* ATTR_H */\n"},{"id":"303449","messageId":"xmqq1szuo3bm.fsf@gitster.mtv.corp.google.com","threadId":"44221","inReplyTo":"CAGZ79kZ7zNkXBVUFq5aLSup9xsdCbZpy_x8z5=uAQqftmOq26A@mail.gmail.com","subject":"Re: [RFC/PATCH] attr: Document a new possible thread safe API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-05T22:09:49Z","receivedAt":"2016-10-05T22:10:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> I think so, instead of resending the documentation, maybe the header\n> file shows that we're on the same page, I converted everything except\n> attr.c to follow this header attr.h:\n\nOK.  The function signature of git_check_attr() looks suspect (it\ndoes not match the \"typical case\" illustration in the message you\nare responding to), but other than that I think this matches my\nexpectation.\n\nThanks for taking this over.\n"}]}