On Wed, Oct 26, 2016 at 4:14 PM, Junio C Hamano <[email protected]> wrote:
> Stefan Beller <[email protected]> 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.
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
>> -}
>> + static struct git_attr_check *check;
>> + git_attr_check_initl(check, "crlf", "ident", NULL);
>
> I think you are still missing "&" here.
done
>> + 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();
}
>> +* 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
