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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 12, 2016, 16:20 UTC
Message-ID: <xmqqfuo15yjm.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kZrNSmPAQ6SmBzFDJtSmdCbqKcgQu4KDLfoYVkSXvo-og@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 17 quoted lines

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

Show 16 quoted lines

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