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

Stefan Beller [off-list ref] writes:

+* 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.

Do you want to keep the last sentence?  "The call to initialize the
result is not thread safe..."?  Is that true?

What's the point of this change?  Isn't check typically a pointer
already?

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

Stefan Beller [off-list ref] writes:

quoted

+* 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.

Do you want to keep the last sentence?  "The call to initialize the
result is not thread safe..."?  Is that true?

I'll drop that sentence, as it overstates the situation.

To explain, you can either have:
    struct git_attr_result result[2];
or
    struct git_attr_result *result = git_attr_result_alloc(check);
and both are running just fine in a thread. However you should not
make that variable static. But maybe that is too much common sense
and hence confusing.

quoted

@@ -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);

What's the point of this change?  Isn't check typically a pointer
already?

This ought to go to

    git_attr_check_initl(&check, "crlf", "ident", NULL);

instead.