Re: What's cooking in git.git (Sep 2016, #08; Tue, 27)

Stefan Beller [off-list ref] writes:

quoted

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.