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

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

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.