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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 3, 2016, 20:17 UTC
Message-ID: <xmqq1szx6vch.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79ka8dO1AHJftKAqD6LvxJSP+8yGGa7Citcdxxrnc5DMeYg@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 8 quoted lines

>> 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.