Stefan Beller <sbeller@xxxxxxxxxx> writes:
> This revamps the API of the attr subsystem to be thread safe.
> Before we had the question and its results in one struct type.
> The typical usage of the API was
>
> static struct git_attr_check *check;
>
> if (!check)
> check = git_attr_check_initl("text", NULL);
>
> git_check_attr(path, check);
> act_on(check->value[0]);
>
> This has a couple of issues when it comes to thread safety:
>
> * the initialization is racy in this implementation. To make it
> thread safe, we need to acquire a mutex, such that only one
> thread is executing the code in git_attr_check_initl.
> As we do not want to introduce a mutex at each call site,
> this is best done in the attr code. However to do so, we need
> to have access to the `check` variable, i.e. the API has to
> look like
> git_attr_check_initl(struct git_attr_check*, ...);
Make that a double-asterisk. The same problem appears in an updated
example in technical/api-gitattributes.txt doc, but the example in
the commit log message (below) is correct.
> The usage of the new API will be:
>
> /*
> * The initl call will thread-safely check whether the
> * struct git_attr_check has been initialized. We only
> * want to do the initialization work once, hence we do
> * that work inside a thread safe environment.
> */
> static struct git_attr_check *check;
> git_attr_check_initl(&check, "text", NULL);
>
> /*
> * Obtain a pointer to a correctly sized result
> * statically allocated on the stack; this macro:
> */
> GIT_ATTR_RESULT_INIT_FOR(myresult, 1);
Are you sure about this? We've called attr_check_initl() already so
if this is declaring myresult, it would be decl-after-stmt.
> /* Perform the check and act on it: */
> git_check_attr(path, check, myresult);
> act_on(myresult->value[0]);
>
> /*
> * No need to free the check as it is static, hence doesn't leak
> * memory. The result is also static, so no need to free there either.
> */
The latter half is questionable. If it is "static" it wouldn't be
thread safe, no? I think the diff in this patch for archive.c shows
that we only expect
struct git_attr_result result[2];
upfront without RESULT_INIT_FOR(), and the reason why there is no
need to free the result[] is because it is on the stack. And each
element in result[] may point at a string, but the string belongs to
the attr subsystem and must not be freed.