Re: [PATCHv2 1/2] attr: convert to new threadsafe API

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 27, 2016, 00:49 UTC
Message-ID: <xmqqbmy6aa6b.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161026224104.31844-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

>  extern struct git_attr *git_attr(const char *);
> ...
> +extern void git_attr_check_append(struct git_attr_check *,
> +				  const struct git_attr *);

Another thing. Do we still need to expose git_attr() to the outside callers? If we change git_attr_check_append() to take "const char *" as its second parameter, can we retire it from the public interface?

It being an "intern" function, by definition it is not thread-safe. Its use from prepare_attr_stack() inside git_check_attr() that takes the Big Attribute Subsystem Lock is safe, but the callers that do

	struct git_attr_check *check = ...;
	struct git_attr *text_attr = git_attr("text");

	git_attr_check_append(check, text_attr);

would risk racing to register the same entry to the "all the attributes in the known universe" table.

If the attribute API does not have to expose git_attr(const char *) to the external callers at all, that would be ideal. Otherwise, we would need to rename the current git_attr() that is used internally under the Big Lock to

    static struct git_attr *git_attr_locked(const char*)

that is defined inside attr.c, and then provide the external version as a thin wrapper that calls it under the Big Lock.