Re: Re attr API further revamp

>
> In the original design, a codepath that wants to check attributes
> repeatedly for many paths (e.g. "convert" that wants to see what
> crlf, ident, filter, eol and text attributes are set to for paths
> that it is asked to munge the blob contents for) used to allocate
>
>         static struct git_attr_check {
>                 struct git_attr *attr;
>                 const char *value;
>         } ccheck[NUM_ATTRS];
>
> and populated the array when the first time the codepath was
> entered, e.g.
>
>         if (!ccheck[0].attr) {
>                 for (i = 0; i < NUM_ATTRS; i++)
>                         ccheck[i].attr = git_attr(...);
>         }
>
> and then make a call to the API to ask for these attributes in
> ccheck[] for a path, i.e.
>
>         git_attr_check(path, NUM_ATTRS, ccheck);
>
> The API function will fill in the ccheck[].value fields and the
> caller will read from there how each attribute is set for the path.
>
> This was updated with recent jc/attr topic so that attr_check
> structure can be enriched to keep _more_ state, such as the
> pre-parsed representation of all the contents of the relevant
> .gitattributes files, which currently is held in a program-side
> singleton instance called attr_stack, partly in preparation for
> Stefan's sb/pathspec-label topic that is expected to place a lot
> heavier load to the attribute subsystem.

>
> A caller of the API after that update would do more like
>
>         static struct git_attr_check *ccheck;
>
>         if (!ccheck)
>                 ccheck = git_attr_check_initl(...);
>         git_attr_check(path, ccheck);
>         for (i = 0; i < NUM_ATTRS; i++)
>                 if (ccheck->check[i].value == ATTR_UNSET)
>                         ... do the Unset thing ...

>
> This however is not thread-safe for obvious two reasons.
>
>  * Two threads can simultanously enter this section of the code and
>    end up initializing ccheck twice;
>
>  * Even though ccheck[].attr are constant for the purpose of this
>    codepath (i.e. all threads passing through are interested in
>    checking the same set of attributes), ccheck[].value fields
>    cannot be shared across simultaneous threads like the above
>    snippet does.
>
> So it appears that the final API visible from the callers needs to
> be updated further, perhaps something like:
>
>
>         static struct git_attr_check *ccheck;
>         const char *values[NUM_ATTRS];
>
>         git_attr_check_initl(&ccheck, "crlf", "ident", ..., NULL);
>         git_attr_check(path, ccheck, values);

>
> where git_attr_check_initl() would do the "if ccheck is NULL then
> initialize it to an instance" atomically in threading environment,
> and git_attr_check() returns its finding to values[] array the
> calling thread exclusively owns, while sharing the input ccheck
> and the list of attributes the call inquires among threads.
>
> Also unlike the earlier plan, attr_stack aka "where in the directory
> hierarchy are we asking attributes for?" will not be stored directly
> in git_attr_check structure.  It is likely that a thread-local
> structure will be introduced hidden behind this API (i.e. the
> callers do not have to be aware of it), and attr_stack will be
> registered in this thread-local structure keyed by &ccheck, so that
> <thread, callsite> pair can have attr_stack instance of its own.
>
> This is because a single attr_stack per ccheck will not work as an
> optimization mechanism when multi-threaded.  Two threads may be
> running the same convert() function, and they may be interested in
> the same set of attributes (e.g. "crlf", "ident", etc.) but they
> will be working on different parts of the tree, so having attr_stack
> per <thread, callsite> would make more sense.
>
> All of which will be quite a lot of work, though, so do not expect
> that it will appear by the end of this week ;-)
>
> Thanks.
>