Re: [PATCHv2] attr: convert to new threadsafe API

> Stefan Beller <sbeller@google.com> writes:
>
>>> I am not sure if the updates to the callers fulfill that purpose.
>>> For example, look at this hunk.
>>>
>>>> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
>>>>       struct archiver_args *args = c->args;
>>>>       write_archive_entry_fn_t write_entry = c->write_entry;
>>>>       static struct git_attr_check *check;
>>>> +     static struct git_attr_result result;
>>>
>>> As we discussed, this caller, even when threaded, will always want
>>> to ask for a fixed two attributes, so "check" being static and
>>> shared across threads is perfectly fine.  But we do not want to see
>>> "result" shared, do we?
>>
>> Well all of the hunks in the patch are not threaded, so they
>> don't follow a threading pattern, but the static pattern to not be
>> more expensive than needed.
>
> Is it too invasive a change to make them as if they are thread-ready
> users of API that happen to know their callers are not threading?
> It would be ideal if we can prepare them so that the way they
> interact with the attr subsystem will not have to change after this
> step.

>
>>> In other words, ideally, I think this part of the patch should
>>> rather read like this:
>>>
>>>         static struct git_attr_check *check;
>>>         struct git_attr_result result[2];
>>>
>>>         ...
>>>         git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
>>>         if (!git_check_attr(path_without_prefix, check, result)) {
>>>                 ... use result[0] and result[1] ...
>>>
>>> For sanity checking, it is OK to add ARRAY_SIZE(result) as the final
>>> and extra parameter to git_check_attr() so that the function can
>>> make sure it matches (or exceeds) check->nr.
>>
>> That seems tempting from a callers perspective; I'll look into that.
>
> For callers that prepare "check" and "result" before asking
> check-attr about the attributes in "check" for many paths, it is OK
> to use your "allocate with attr_result_init()" pattern.  The "result"
> still needs to be made non-static, though.

>
> But many callers do not follow that; rather they do
>
>         loop to iterate over paths {
>                 call a helper func to learn attr X for path
>                 use the value of attr X
>         }
>
> using a callchain that embeds a helper function deep inside, and
> "check" is kept in the helper, check-attr function is called from
> there, and "result" is not passed from the caller to the helper
> (obviously, because it does not exist in the current API).  See the
> callchain that leads down to convert.c::convert_attrs() for a
> typical example.  When converted to the new API, it needs to have a
> new "result" structure every time it is called, and cannot reuse the
> one that was used in its previous call.