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

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> @@ -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.

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

> +* Call `git_check_attr()` to check the attributes for the path,
> +  the given `git_attr_result` will be filled with the result.
> +
> +* Inspect each `git_attr_result` structure to see how
> +  each of the attribute in the array is defined for the path.
>  
>  Example
>  -------
> @@ -76,28 +93,23 @@ To see how attributes "crlf" and "ident" are set for different paths.
>    we are checking two attributes):
>  
>  ------------
> -static struct git_attr_check *check;
> -static void setup_check(void)
> -{
> -	if (check)
> -		return; /* already done */
> -	check = git_attr_check_initl("crlf", "ident", NULL);
> -}
> +	static struct git_attr_check *check;
> +	git_attr_check_initl(check, "crlf", "ident", NULL);

I think you are still missing "&" here.

>  ------------
>  
>  . Call `git_check_attr()` with the prepared `struct git_attr_check`:
>  
>  ------------
>  	const char *path;
> +	struct git_attr_result result[2];
>  
> -	setup_check();
> -	git_check_attr(path, check);
> +	git_check_attr(path, check, result);
>  ------------
>  
> -. Act on `.value` member of the result, left in `check->check[]`:
> +. Act on `result.value[]`:
>  
>  ------------
> -	const char *value = check->check[0].value;
> +	const char *value = result.value[0];
>  
>  	if (ATTR_TRUE(value)) {
>  		The attribute is Set, by listing only the name of the
> @@ -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();

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

>  	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);
>  }
>  ------------
>  
> @@ -138,17 +153,20 @@ Querying All Attributes
>  
>  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.
> +* 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?

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).