Re: [PATCH 28/28] attr: convert to new threadsafe API
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 11, 2016, 17:40 UTC
- Message-ID
- <xmqqmviaaina.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20161011002115.23312-29-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 5 quoted lines
> This revamps the API of the attr subsystem to be thread safe. > Before we had the question and its results in one struct type. > The typical usage of the API was > > static struct git_attr_check;
I think you meant "*check" at the end, perhaps?
Show 13 quoted lines
>
> if (!check)
> check = git_attr_check_initl("text", NULL);
>
> git_check_attr(path, check);
> act_on(check->value[0]);
>
> * While the check for attributes to be questioned only need to
> be initalized once as that part will be read only after its
> initialisation, the answer may be different for each path.
> Because of that we need to decouple the check and the answer,
> such that each thread can obtain an answer for the path it
> is currently processing.Yes, it is good to separate questions and answers. I think answers should be owned by the caller, though. I do not think of a good reason why you want to make it impossible to do something like this:
struct git_attr_check_result *result_1 = ...allocate...; struct git_attr_check_result *result_2 = ...allocate...;
loop {
struct strbuf path = next_path();
git_check_attr(result_1, path.buf, check_1);
if (strbuf_strip_suffix(&path, ".c")) {
strbuf_addstr(&path, ".h");
git_check_attr(result_2, path.buf, check_2);
do something using result_1[] and result_2[];
} else {
do something using result_1[];
}
}Do we already have a design of the "result" thing that is concrete enough to require us to declare that the result is owned by the implementation and asking another question has to destroy the answer to the previous question? Otherwise, I'd rather not to see us make the API unnecessarily hard to use. While I do want us to avoid overengineering for excessive flexibility, I somehow feel "you cannot control the lifetime of the result, it is owned by the subsystem" falls the other side of the line.
Show 56 quoted lines
> diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt > index 92fc32a..2059aab 100644 > --- a/Documentation/technical/api-gitattributes.txt > +++ b/Documentation/technical/api-gitattributes.txt > @@ -8,6 +8,18 @@ attributes to set of paths. > Data Structure > -------------- > > +extern struct git_attr *git_attr(const char *); > + > +/* > + * Return the name of the attribute represented by the argument. The > + * return value is a pointer to a null-delimited string that is part > + * of the internal data structure; it should not be modified or freed. > + */ > +extern const char *git_attr_name(const struct git_attr *); > + > +extern int attr_name_valid(const char *name, size_t namelen); > +extern void invalid_attr_name_message(struct strbuf *, const char *, int); > + > `struct git_attr`:: > > An attribute is an opaque object that is identified by its name. > @@ -16,15 +28,17 @@ Data Structure > of no interest to the calling programs. The name of the > attribute can be retrieved by calling `git_attr_name()`. > > -`struct git_attr_check_elem`:: > - > - This structure represents one attribute and its value. > - > `struct git_attr_check`:: > > - This structure represents a collection of `git_attr_check_elem`. > + This structure represents a collection of `struct git_attrs`. > It is passed to `git_check_attr()` function, specifying the > - attributes to check, and receives their values. > + attributes to check, and receives their values into a corresponding > + `struct git_attr_result`. > + > +`struct git_attr_result`:: > + > + This structure represents a collection of results to its > + corresponding `struct git_attr_check`, that has the same order. > > > Attribute Values > @@ -56,16 +70,22 @@ Querying Specific Attributes > * Prepare `struct git_attr_check` using git_attr_check_initl() > function, enumerating the names of attributes whose values you are > interested in, terminated with a NULL pointer. Alternatively, an > - empty `struct git_attr_check` can be prepared by calling > - `git_attr_check_alloc()` function and then attributes you want to > - ask about can be added to it with `git_attr_check_append()` > - function. > + empty `struct git_attr_check` as alloced by git_attr_check_alloc()
"allocated", not "alloced".
> + can be prepared by calling `git_attr_check_alloc()` function and > + then attributes you want to ask about can be added to it with > + `git_attr_check_append()` function. > + git_attr_check_initl is thread safe, i.e. you can call it
Spell it `git_attr_check_initl()` for consistency.
> + from different threads at the same time; internally however only one > + call at a time is processed. If the calls from different threads have > + the same arguments, the returned `git_attr_check` may be the same.
I find this description a bit confusing. At least the way I envisioned was that when this piece of code is run by multiple people at the same time,
static struct git_attr_check *check = NULL; git_attr_check_initl(&check, ...);
we won't waste the "check" by allocated by the first one by overwriting it with another one allocated by the second one. So "the same arguments" does not come into the picture. A single variable is either
* already allocated and initialised by the an earlier call to initl() by somebody else, or
* originally NULL when you call initl(), and the implementation makes sure that other people wait while you allocate, initialise and assign it to the variable, or
* originally NULL when you call initl(), but the implementation notices that somebody else is doing the second bullet point above, and you wait until that somebody else finishes and then you return without doing anything (because by that time, "check" is filled by that other party doing the second bullet point above).
There is no need to check for "the same arguments".