Re: [PATCHv2 1/2] attr: convert to new threadsafe API

On Wed, Oct 26, 2016 at 4:14 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

@@ -53,19 +57,32 @@ value of the attribute for the path.
 Querying Specific Attributes
 ----------------------------

-* Prepare `struct git_attr_check` using git_attr_check_initl()
+* Prepare a `struct git_attr_check` using `git_attr_check_initl()`
   function, enumerating the names of attributes whose values you are
   interested in, terminated with a NULL pointer.  Alternatively, an
-  empty `struct git_attr_check` can be prepared by calling
-  `git_attr_check_alloc()` function and then attributes you want to
-  ask about can be added to it with `git_attr_check_append()`
-  function.
-
-* Call `git_check_attr()` to check the attributes for the path.
-
-* Inspect `git_attr_check` structure to see how each of the
-  attribute in the array is defined for the path.
-
+  empty `struct git_attr_check` as allocated by git_attr_check_alloc()
+  can be prepared by calling `git_attr_check_alloc()` function and
+  then attributes you want to ask about can be added to it with
+  `git_attr_check_append()` function.

I think that my version that was discarded forbade appending once
you started to use the check for querying, because the check was
meant to be used as a key for an attr-stack and the check-specific
attr-stack was planned to keep only the attrs the check is
interested in (and appending is to change the set of attrs the check
is interested in, invalidating the attr-stack at that point).

If you lost that restriction, that is good (I didn't check the
implementation, though).  Otherwise we'd need to say something here.

That restriction still applies. Though I think mentioning it in the
paragraph where we describe querying makes more sense.

initialization?

done

Grammo?  "either on the stack, or dynamically in the heap"?

done

Having result defined statically is not thread safe for that
reason.  It is not clear what you mean by "The call to initialize
the result"; having it on the stack or have one dynamically on the
heap ought to be thread safe.

done

quoted

-}
+     static struct git_attr_check *check;
+     git_attr_check_initl(check, "crlf", "ident", NULL);

I think you are still missing "&" here.

done

quoted

+     if (check)
+             return; /* already done */
      check = git_attr_check_alloc();

You may want to say that this is thread-unsafe.

It is not; see the implementation:

void git_attr_check_append(struct git_attr_check *check,
               const struct git_attr *attr)
{
    int i;
    if (check->finalized)
        die("BUG: append after git_attr_check structure is finalized");
    if (!attr_check_is_dynamic(check))
        die("BUG: appending to a statically initialized git_attr_check");
    attr_lock();
    for (i = 0; i < check->check_nr; i++)
        if (check->attr[i] == attr)
            break;
    if (i == check->check_nr) {
        ALLOC_GROW(check->attr, check->check_nr + 1, check->check_alloc);
        check->attr[check->check_nr++] = attr;
    }
    attr_unlock();
}

quoted

+* Call `git_all_attrs()`.

Hmph, the caller does not know what attribute it is interested in,
and it is unclear "how" the former needs to be set up from this
description.  Should it prepare an empty one that can be appended?

Good point on clarifying this one. It is just needed to have
NULL pointers around:

    struct git_attr_check *check = NULL;
    struct git_attr_result *result = NULL;
    git_all_attrs(full_path, &local_check, &result);
    // proceed as in the case above

All comments from above yield the following diff:

diff --git a/Documentation/technical/api-gitattributes.txt

b/Documentation/technical/api-gitattributes.txt
index 4d8ef93..d221736 100644

--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt

@@ -67,19 +67,21 @@ Querying Specific Attributes
   Both ways with `git_attr_check_initl()` as well as the
   alloc and append route are thread safe, i.e. you can call it
   from different threads at the same time; when check determines
-  the initialzisation is still needed, the threads will use a
+  the initialization is still needed, the threads will use a
   single global mutex to perform the initialization just once, the
   others will wait on the the thread to actually perform the
   initialization.

-* Allocate an array of `struct git_attr_result` either statically on the
-  as a variable on the stack or dynamically via `git_attr_result_alloc`
-  when the result size is not known at compile time. The call to initialize
+* Allocate an array of `struct git_attr_result` either on the stack
+  or via `git_attr_result_alloc` on the heap when the result size
+  is not known at compile time. The call to initialize
   the result is not thread safe, because different threads need their
   own thread local result anyway.

 * Call `git_check_attr()` to check the attributes for the path,
   the given `git_attr_result` will be filled with the result.
+  You must not change the `struct git_attr_check` after calling
+  `git_check_attr()`.

 * Inspect each `git_attr_result` structure to see how
   each of the attribute in the array is defined for the path.

@@ -103,7 +105,7 @@ To see how attributes "crlf" and "ident" are set

for different paths.
         const char *path;
         struct git_attr_result result[2];

-        git_check_attr(path, check, result);
+        git_check_attr(path, &check, result);
 ------------

 . Act on `result.value[]`:

@@ -155,10 +157,20 @@ To get the values of all attributes associated

with a file:

 * Setup a local variables for the question
   `struct git_attr_check` as well as a pointer where the result
-  `struct git_attr_result` will be stored.
+  `struct git_attr_result` will be stored. Both should be initialized
+  to NULL.
+
+------------
+  struct git_attr_check *check = NULL;
+  struct git_attr_result *result = NULL;
+------------

 * Call `git_all_attrs()`.

+------------
+  git_all_attrs(full_path, &check, &result);
+------------
+
 * Iterate over the `git_attr_check.attr[]` array to examine the
   attribute names.  The name of the attribute described by a
   `git_attr_check.attr[]` object can be retrieved via