Stefan Beller <[email protected]> writes:
>> The minimum that would future-proof us, that is still missing from
>> the series, would probably be to separate the query parameter
>> "struct git_attr_check" and the return values from git_check_attr().
>
> Not sure what you mean here with separating as a preparation for
> the thread safety. As I understand it we can still keep the thread local
> states in git_attr_check, we'd just have to route each thread to its
> own part of the memory in there?
For example, look at what you did in your pathspec-label topic.
static int match_attrs(const char *name, int namelen,
const struct pathspec_item *item)
{
int i;
git_check_attr_counted(name, namelen, item->attr_check);
for (i = 0; i < item->attr_match_nr; i++) {
const char *value;
int matched;
enum attr_match_mode match_mode;
value = item->attr_check->check[i].value;
match_mode = item->attr_match[i].match_mode;
Each pathspec item has an attr_check member that wants to see a
specific set of attributes for a path being matched. Each element
of the item->attr_check->check[] array is <attr, value> pair, where
<attr> is a constant for the purpose of the codepath (i.e. no matter
which thread is asking, and no matter for which path the question is
being asked, it asks for a fixed attribute that was computed when
the pathspec was parsed). But <value> is a slot to return the
finding back to the caller.
So you can never keep this code structure and have this function
called more than once, specifically, you cannot make
git_check_attr_counted() call from multiple threads, at one time.
Instead the calling convention needs to be updated to allow this
caller of git_check_attr_counted() to pass a separate array that is
on its stack, e.g.
const char *v[... some size ...];
git_check_attr_counted(name, namelen, item->attr_check, v);
for (i = 0; i < item->attr_match_nr; i++) {
const char *value;
value = v[i];
match_mode = item->attr_match[i].match_mode;
We could do that API update way before we make the attribute
subsystem's implementation thread-safe, and if we did so now,
then the caller will not have to change.
That is what I meant as "future-proofing", i.e. future-proofing the
callers.
And from that point of view, I think 0a5aadcce4 is not an ideal
place to stop. We'd want at least up to 079186123a but probably
even more, e.g. to 48d93f7f42, I would think.
