From: Junio C Hamano <gitster@pobox•com>
To: Stefan Beller <sbeller@google•com>
Cc: git@vger•kernel.org, bmwill@google•com, pclouds@gmail•com,
Johannes.Schindelin@gmx•de, j6t@kdbg•org, peff@peff•net,
simon@ruderich•org
Subject: Re: [PATCH] attr: convert to new threadsafe API
Date: Fri, 28 Oct 2016 10:20:15 -0700 [thread overview]
Message-ID: <xmqqinsc1jcg.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20161027221550.14930-1-sbeller@google.com> (Stefan Beller's message of "Thu, 27 Oct 2016 15:15:50 -0700")
Stefan Beller <sbeller@google•com> writes:
> +* Prepare a `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.
> -
> -* Call `git_check_attr()` to check the attributes for the path.
> -
> -* Inspect `git_attr_check` structure to see how each of the
> - attribute in the array is defined for the path.
> -
> + empty `struct git_attr_check` as allocated by git_attr_check_alloc()
Need to drop "as allocated by git_attr_check_alloc()" here.
> + 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.
> + Both ways with `git_attr_check_initl()` as well as the
> + alloc and append route are thread safe, i.e. you can call it
> + from different threads at the same time; when check determines
> + the initialization is still needed, the threads will use a
> + single global mutex to perform the initialization just once, the
> + others will wait on the the thread to actually perform the
> + initialization.
I have some comments on the example in the doc on the "alloc-append"
side. _initl() side looks OK.
> + static struct git_attr_check *check;
> + git_attr_check_initl(&check, "crlf", "ident", NULL);
OK.
> const char *path;
> + struct git_attr_result result[2];
>
> + git_check_attr(path, check, result);
OK. The above two may be easier to understand if they were a single
example, though.
> +. Act on `result.value[]`:
>
> ------------
> - const char *value = check->check[0].value;
> + const char *value = result.value[0];
OK.
> @@ -123,12 +135,15 @@ the first step in the above would be different.
> static struct git_attr_check *check;
> static void setup_check(const char **argv)
> {
> + 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);
> 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.
> +* Setup a local variables for the question
> + `struct git_attr_check` as well as a pointer where the result
> + `struct git_attr_result` will be stored. Both should be initialized
> + to NULL.
> +
> +------------
> + struct git_attr_check *check = NULL;
> + struct git_attr_result *result = NULL;
> +------------
> +
> +* Call `git_all_attrs()`.
>
> +------------
> + git_all_attrs(full_path, &check, &result);
> +------------
OK.
Thanks.
next prev parent reply other threads:[~2016-10-28 17:20 UTC|newest]
Thread overview: 81+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-22 23:31 [PATCHv2 00/36] Revamping the attr subsystem! Stefan Beller
2016-10-22 23:31 ` [PATCH 01/36] commit.c: use strchrnul() to scan for one line Stefan Beller
2016-10-22 23:31 ` [PATCH 02/36] attr.c: " Stefan Beller
2016-10-22 23:31 ` [PATCH 03/36] attr.c: update a stale comment on "struct match_attr" Stefan Beller
2016-10-22 23:31 ` [PATCH 04/36] attr.c: explain the lack of attr-name syntax check in parse_attr() Stefan Beller
2016-10-22 23:31 ` [PATCH 05/36] attr.c: complete a sentence in a comment Stefan Beller
2016-10-22 23:31 ` [PATCH 06/36] attr.c: mark where #if DEBUG ends more clearly Stefan Beller
2016-10-22 23:31 ` [PATCH 07/36] attr.c: simplify macroexpand_one() Stefan Beller
2016-10-22 23:31 ` [PATCH 08/36] attr.c: tighten constness around "git_attr" structure Stefan Beller
2016-10-22 23:31 ` [PATCH 09/36] attr.c: plug small leak in parse_attr_line() Stefan Beller
2016-10-22 23:31 ` [PATCH 10/36] attr: rename function and struct related to checking attributes Stefan Beller
2016-10-22 23:32 ` [PATCH 11/36] attr: (re)introduce git_check_attr() and struct git_attr_check Stefan Beller
2016-10-22 23:32 ` [PATCH 12/36] attr: convert git_all_attrs() to use "struct git_attr_check" Stefan Beller
2016-10-22 23:32 ` [PATCH 13/36] attr: convert git_check_attrs() callers to use the new API Stefan Beller
2016-10-22 23:32 ` [PATCH 14/36] attr: retire git_check_attrs() API Stefan Beller
2016-10-22 23:32 ` [PATCH 15/36] attr: add counted string version of git_check_attr() Stefan Beller
2016-10-22 23:32 ` [PATCH 16/36] attr: add counted string version of git_attr() Stefan Beller
2016-10-22 23:32 ` [PATCH 17/36] attr: expose validity check for attribute names Stefan Beller
2016-10-23 15:07 ` Ramsay Jones
2016-10-24 21:07 ` Stefan Beller
2016-10-27 20:57 ` Stefan Beller
2016-10-26 21:20 ` [PATCH] attr: expose error reporting function for invalid " Stefan Beller
2016-10-22 23:32 ` [PATCH 18/36] attr: support quoting pathname patterns in C style Stefan Beller
2016-10-22 23:32 ` [PATCH 19/36] attr.c: add push_stack() helper Stefan Beller
2016-10-22 23:32 ` [PATCH 20/36] attr.c: pass struct git_attr_check down the callchain Stefan Beller
2016-10-22 23:32 ` [PATCH 21/36] attr.c: rename a local variable check Stefan Beller
2016-10-22 23:32 ` [PATCH 22/36] attr.c: correct ugly hack for git_all_attrs() Stefan Beller
2016-10-22 23:32 ` [PATCH 23/36] attr.c: introduce empty_attr_check_elems() Stefan Beller
2016-10-22 23:32 ` [PATCH 24/36] attr.c: always pass check[] to collect_some_attrs() Stefan Beller
2016-10-22 23:32 ` [PATCH 25/36] attr.c: outline the future plans by heavily commenting Stefan Beller
2016-10-22 23:32 ` [PATCH 26/36] attr: make git_check_attr_counted static Stefan Beller
2016-10-22 23:32 ` [PATCH 27/36] attr: convert to new threadsafe API Stefan Beller
2016-10-24 18:55 ` Junio C Hamano
2016-10-24 19:18 ` Stefan Beller
2016-10-26 14:06 ` Duy Nguyen
2016-10-26 8:52 ` Johannes Schindelin
2016-10-26 9:35 ` Simon Ruderich
2016-10-26 12:15 ` Jeff King
2016-10-26 19:51 ` Stefan Beller
2016-10-26 20:20 ` Jeff King
2016-10-26 20:25 ` Johannes Sixt
2016-10-26 20:26 ` Jeff King
2016-10-26 20:40 ` Johannes Sixt
2016-10-26 20:46 ` Stefan Beller
2016-10-26 22:41 ` [PATCHv2 1/2] " Stefan Beller
2016-10-26 23:14 ` Junio C Hamano
2016-10-27 0:08 ` Stefan Beller
2016-10-27 0:16 ` Junio C Hamano
2016-10-27 0:22 ` Stefan Beller
2016-10-27 0:50 ` Junio C Hamano
2016-10-27 0:49 ` Junio C Hamano
2016-10-27 2:19 ` Stefan Beller
2016-10-27 4:13 ` Junio C Hamano
2016-10-27 5:44 ` Junio C Hamano
2016-10-27 22:15 ` [PATCH] " Stefan Beller
2016-10-28 8:55 ` Johannes Schindelin
2016-10-28 17:20 ` Junio C Hamano [this message]
2016-10-28 17:30 ` Junio C Hamano
2016-10-28 18:16 ` Johannes Sixt
2016-10-26 20:43 ` [PATCH 27/36] " Stefan Beller
2016-10-26 13:52 ` Duy Nguyen
2016-10-22 23:32 ` [PATCH 28/36] attr: keep attr stack for each check Stefan Beller
2016-10-23 15:10 ` Ramsay Jones
2016-10-26 23:10 ` Stefan Beller
2016-10-24 19:07 ` Junio C Hamano
2016-10-24 19:32 ` Stefan Beller
2016-10-24 20:29 ` Junio C Hamano
2016-10-22 23:32 ` [PATCH 29/36] Documentation: fix a typo Stefan Beller
2016-10-22 23:32 ` [PATCH 30/36] pathspec: move long magic parsing out of prefix_pathspec Stefan Beller
2016-10-22 23:32 ` [PATCH 31/36] pathspec: move prefix check out of the inner loop Stefan Beller
2016-10-22 23:32 ` [PATCH 32/36] pathspec: allow querying for attributes Stefan Beller
2016-10-26 13:33 ` Duy Nguyen
2016-10-27 21:32 ` Stefan Beller
2016-10-27 18:29 ` Junio C Hamano
2016-11-09 9:45 ` Duy Nguyen
2016-11-09 18:08 ` Stefan Beller
2016-11-09 22:25 ` Junio C Hamano
2016-10-22 23:32 ` [PATCH 33/36] pathspec: allow escaped query values Stefan Beller
2016-10-22 23:32 ` [PATCH 34/36] submodule update: add `--init-default-path` switch Stefan Beller
2016-10-22 23:32 ` [PATCH 35/36] clone: add --init-submodule=<pathspec> switch Stefan Beller
2016-10-22 23:32 ` [PATCH 36/36] completion: clone can initialize specific submodules Stefan Beller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqinsc1jcg.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=Johannes.Schindelin@gmx$(echo .)de \
--cc=bmwill@google$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=j6t@kdbg$(echo .)org \
--cc=pclouds@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=sbeller@google$(echo .)com \
--cc=simon@ruderich$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox