Junio C Hamano <gitster@xxxxxxxxx> writes:
> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>
>> Yeah, I can make it work without exposing struct git_attr.
>
> You completely misunderstood me. "struct git_attr" MUST be visible
> to the users so that they can ask for the name in git_check.attr[0].
>
> What would be nice to hide if you can is the function to intern a
> string into a pointer to struct git_attr, i.e. git_attr() function.
Even though that was not the primary point of my suggestion, I
actually think it is OK to make "struct git_attr" a structure that
is opaque to the users of the API if you wanted to. The attr_check
structure will have an array of pointers to "struct git_attr", and
the structure definition may be visible to the public attr.h header,
but the API users won't have to be able to dereference the pointer
"struct git_attr *". git_check.attr[0] would be a pointer to an
opaque structure from API users' point of view, that can be passed
to API function git_attr_name() to read its string.
What is nice to hide is the constructor of the structure. What it,
i.e. "struct git_attr *git_attr(const char *)", needs to do is to
(1) see if the attribute object with the same name already exists in
the table of "all known attributes in the universe", and if there
is, return that instance, (2) otherwise create a new attribute
object, register it to the table and return it. And it needs to do
it in a way that is thread-safe.
If we have to give access to it to the API users, then we'd need to
acquire and release the Big Attr Lock per each call.
The calls to git_attr() you need to make in your implementation will
be made from two codepaths:
* check_initl() acquires the Big Attr Lock, creates a check struct,
makes multiple calls to git_attr() to construct the necessary
git_attr instances to fill the array and then releases the lock,
so the git_attr() constructor does not have to be protected for
concurrent access.
* check_attr() acquires the Big Attr Lock, calls down to
prepare_attr_stack() as necessary to parse .gitattributes files
found in the directory hierarchy, which makes calls to git_attr()
to record the attributes found in the file. Then it does the
matching to fill results[] array and releases the lock. Again,
git_attr() constructors are called under the lock, so there is no
need for a separate lock.
If these are the only callpaths that reach git_attr() to construct
new attribute objects, it would mean that you can make this private
to attr subsystem and hide it from the users of the API.
Otherwise, you would need to rename the git_attr() constructor that
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, i.e.
struct git_attr *git_attr(const char *s)
{
struct git_attr *attr;
take_big_attr_lock();
attr = git_attr_locked(s);
release_big_attr_lock();
return attr;
}
That will have to make the big attr lock busier, and it would be
good if we can avoid it. That is where my "can we hide git_attr()
constructor?" comes from.