Re: [PATCH] attr: convert to new threadsafe API

Junio C Hamano [off-list ref] writes:

quoted

+	if (check)
+		return; /* already done */
 	check = git_attr_check_alloc();
 	while (*argv) {
 		struct git_attr *attr = git_attr(*argv);
 		git_attr_check_append(check, attr);

I thought you made git_attr() constructor unavailable, so
check_append() would just get a "const char *" instead?

quoted

 		argv++;
 	}
+	struct git_attr_result *result = git_attr_result_alloc(check);

This does not look like thread-safe.

I could understand it if the calling convention were like this,
though:

	if (git_attr_check_alloc(&check)) {
		while (*argv) {
	        	... append ...
		}
		git_attr_check_finished_appending(&check);
	}
	result = result_alloc();

In this variant, git_attr_check_alloc() is responsible for ensuring
that the "check" is allocated only once just like _initl() is, and
at the same time, it makes simultanous callers wait until the first
caller who appends to the singleton check instance declares that it
finished appending.  The return value signals if you are the first
caller (who is responsible for populating the check and for
declaring the check is ready to use at the end of appending).
Everybody else waits while the first caller is doing the if (...) {
} thing, and then receives false, at which time everybody (including
the first caller) goes on and allocating its own result and start
making queries.

Having said that, how flexible does the "alloc then append" side of
API have to be in the envisioned set of callers?  Is it expected
that it wouldn't be too hard to arrange them so that they have an
array of "const char *"s when they need to initialize/populate a
check?  If that is the case, instead of the "alloc and have others
wait" illustrated above, it may be a lot simpler to give _initv()
variant to them and be done with it, i.e. the above sample caller
would become just:

	git_attr_check_initv(&check, argv);
	result = git_attr_result_alloc(&check);

would that be too limiting?  The use of "alloc then append" pattern
in "git check-attr" done in your patch seems to fit the _initv()
well, and the "pathspec with attr match" that appears later in the
series also has all the attributes it needs to query available by
the time it wants to allocate and append to create an instance of
"check", I would think, so the _initv() might be sufficient as a
public API.