Re: [PATCH 28/28] attr: convert to new threadsafe API

> Stefan Beller <sbeller@google.com> 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;
>
> I think you meant "*check" at the end, perhaps?
>
>>
>>     if (!check)
>>         check = git_attr_check_initl("text", NULL);
>>
>>     git_check_attr(path, check);
>>     act_on(check->value[0]);
>>
>> * While the check for attributes to be questioned only need to
>>   be initalized once as that part will be read only after its
>>   initialisation, the answer may be different for each path.
>>   Because of that we need to decouple the check and the answer,
>>   such that each thread can obtain an answer for the path it
>>   is currently processing.
>
> Yes, it is good to separate questions and answers.  I think answers
> should be owned by the caller, though.  I do not think of a good
> reason why you want to make it impossible to do something like this:
>
>         struct git_attr_check_result *result_1 = ...allocate...;
>         struct git_attr_check_result *result_2 = ...allocate...;
>
>         loop {
>                 struct strbuf path = next_path();
>                 git_check_attr(result_1, path.buf, check_1);
>                 if (strbuf_strip_suffix(&path, ".c")) {
>                         strbuf_addstr(&path, ".h");
>                         git_check_attr(result_2, path.buf, check_2);
>                         do something using result_1[] and result_2[];
>                 } else {
>                         do something using result_1[];
>                 }
>         }
>
> Do we already have a design of the "result" thing that is concrete
> enough to require us to declare that the result is owned by the
> implementation and asking another question has to destroy the answer
> to the previous question?  Otherwise, I'd rather not to see us make
> the API unnecessarily hard to use.  While I do want us to avoid
> overengineering for excessive flexibility, I somehow feel "you
> cannot control the lifetime of the result, it is owned by the
> subsystem" falls the other side of the line.

>
>> +  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.
>> +  git_attr_check_initl is thread safe, i.e. you can call it
>
> Spell it `git_attr_check_initl()` for consistency.

>
>> +  from different threads at the same time; internally however only one
>> +  call at a time is processed. If the calls from different threads have
>> +  the same arguments, the returned `git_attr_check` may be the same.
>
> I find this description a bit confusing.  At least the way I
> envisioned was that when this piece of code is run by multiple
> people at the same time,
>
>         static struct git_attr_check *check = NULL;
>         git_attr_check_initl(&check, ...);
>
> we won't waste the "check" by allocated by the first one by
> overwriting it with another one allocated by the second one.  So
> "the same arguments" does not come into the picture.  A single
> variable is either
>
>  * already allocated and initialised by the an earlier call to
>    initl() by somebody else, or
>
>  * originally NULL when you call initl(), and the implementation
>    makes sure that other people wait while you allocate, initialise
>    and assign it to the variable, or
>
>  * originally NULL when you call initl(), but the implementation
>    notices that somebody else is doing the second bullet point
>    above, and you wait until that somebody else finishes and then
>    you return without doing anything (because by that time, "check"
>    is filled by that other party doing the second bullet point
>    above).
>
> There is no need to check for "the same arguments".
>

> On 10/10, Stefan Beller wrote:
>>   be initalized once as that part will be read only after its
>>       initialized
>>   initialisation, the answer may be different for each path.
> should this be the US spelling 'initialization'?