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

[Stefan Beller <sbeller@google.com>]

On Tue, Oct 11, 2016 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> I think this patch is the most interesting patch, so I'll refrain from
>> resending the other 27 patches, though I have adressed the review comments
>> locally. I'll resend everything once we are in agreement for this one.
>
> What is the primary purpose of this patch?  Is it to prepare callers
> so that the way they interact with the attr subsystem will not have to
> change when they become threaded and the attr subsystem becomes
> thread ready?
>
> 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.

>
>>       const char *path_without_prefix;
>>       int err;
>>
>> @@ -124,12 +125,15 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
>>               strbuf_addch(&path, '/');
>>       path_without_prefix = path.buf + args->baselen;
>>
>> -     if (!check)
>> -             check = git_attr_check_initl("export-ignore", "export-subst", NULL);
>> -     if (!git_check_attr(path_without_prefix, check)) {
>> -             if (ATTR_TRUE(check->check[0].value))
>> +     if (!check) {
>> +             git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
>> +             git_attr_result_init(&result, check);
>> +     }
>
> Are we assuming that storing and checking of a single pointer is
> atomic?  I would not expose that assumption to the callers.  On a
> platform where that assumption holds, "if check is not NULL,
> somebody must have done it already, so return without doing nothing"
> can be the first thing git_attr_check_initl()'s implementation does,
> though.  Or it may not hold anywhere without some barriers.  All
> that implementation details should be hidden inside _initl()'s
> implementation.  So this caller should instead just do an
> unconditional:
>
>         git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
>
> Also, as "result" should be per running thread, hence non-static,
> and because we do not want repeated heap allocations and releases
> but luckily most callers _know_ not just how many but what exact
> attributes they are interested in (I think there are only two
> callers that do not know it; check-all-attrs one, and your pathspec
> magic one that does not exist at this point in the series), I would
> think it is much more preferrable to allow the caller to prepare an
> on-stack array and call it "initialized already".
>
> 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.