On Tue, Oct 11, 2016 at 11:12 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Stefan Beller <sbel...@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.
