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

Stefan Beller [off-list ref] writes:

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.

+  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

initialization?

+  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`

Grammo?  "either on the stack, or dynamically in 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.

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.

I think you are still missing "&" here.

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

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?

For the same reason, the size of the result is also unknown at the
callsite.

For these reasons, I thought git-all-attrs would need to use a
different calling convention from the usual "query for these
attributes -- get result for them" API.  Even if the same attr_check
and attr_result structures are used, they need to be initialized in
very different way from the normal calls, and this part of the doc
needs to explain how.  Perhaps it is sufficient to say "just declare
two pointer variables that point at these structures and a pair of
newly allocated structures of these types will be stored there" or
something.

It does not even have to use the usual "struct attr_check" "struct
attr_result" pair (I am not suggesting to use different convention
only to be different; I am merely pointing out that you have that
lattitude, as the usage pattern for all-attrs is so different from
the usual query).

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