Stefan Beller <sbel...@google.com> writes:
> +* 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()
Need to drop "as allocated by git_attr_check_alloc()" here.
> + 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.
> + 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 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.
I have some comments on the example in the doc on the "alloc-append"
side. _initl() side looks OK.
> + static struct git_attr_check *check;
> + git_attr_check_initl(&check, "crlf", "ident", NULL);
OK.
> const char *path;
> + struct git_attr_result result[2];
>
> + git_check_attr(path, check, result);
OK. The above two may be easier to understand if they were a single
example, though.
> +. Act on `result.value[]`:
>
> ------------
> - const char *value = check->check[0].value;
> + const char *value = result.value[0];
OK.
> @@ -123,12 +135,15 @@ the first step in the above would be different.
> static struct git_attr_check *check;
> static void setup_check(const char **argv)
> {
> + if (check)
> + return; /* already done */
> check = git_attr_check_alloc();
> while (*argv) {
> struct git_attr *attr = git_attr(*argv);
> git_attr_check_append(check, attr);
> argv++;
> }
> + struct git_attr_result *result = git_attr_result_alloc(check);
This does not look like thread-safe.
I could understand it if the calling convention were like this,
though:
if (git_attr_check_alloc(&check)) {
while (*argv) {
... append ...
}
git_attr_check_finished_appending(&check);
}
result = result_alloc();
In this variant, git_attr_check_alloc() is responsible for ensuring
that the "check" is allocated only once just like _initl() is, and
at the same time, it makes simultanous callers wait until the first
caller who appends to the singleton check instance declares that it
finished appending. The return value signals if you are the first
caller (who is responsible for populating the check and for
declaring the check is ready to use at the end of appending).
Everybody else waits while the first caller is doing the if (...) {
} thing, and then receives false, at which time everybody (including
the first caller) goes on and allocating its own result and start
making queries.
> +* 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. 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);
> +------------
OK.
Thanks.
