Re: [PATCHv2 27/36] attr: convert to new threadsafe API

[Johannes Sixt]

Am 29.10.2016 um 00:06 schrieb Junio C Hamano:

Probably this needs to be squashed in, now the MinGW discussion has
settled.


Yes, this looks good. Thank you very much, both of you.

As I said, I won't be able to test this until late next week.

-- Hannes



 attr.c | 2 +-
 common-main.c  | 2 ++
 compat/mingw.c | 4 
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/attr.c b/attr.c
index 082b5ed343..961218a0d5 100644
--- a/attr.c
+++ b/attr.c
@@ -50,7 +50,7 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);

 #ifndef NO_PTHREADS

-static pthread_mutex_t attr_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t attr_mutex;
 #define attr_lock()pthread_mutex_lock(_mutex)
 #define attr_unlock()  pthread_mutex_unlock(_mutex)
 void attr_start(void) { pthread_mutex_init(_mutex, NULL); }
diff --git a/common-main.c b/common-main.c
index 44a29e8b13..d4699cd404 100644
--- a/common-main.c
+++ b/common-main.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "exec_cmd.h"
+#include "attr.h"

 /*
  * Many parts of Git have subprograms communicate via pipe, expect the
@@ -32,6 +33,7 @@ int main(int argc, const char **argv)
sanitize_stdfds();

git_setup_gettext();
+   attr_start();

argv[0] = git_extract_argv0_path(argv[0]);

diff --git a/compat/mingw.c b/compat/mingw.c
index 51ed76326b..3fbfda5978 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -5,7 +5,6 @@
 #include "../strbuf.h"
 #include "../run-command.h"
 #include "../cache.h"
-#include "../attr.h"

 #define HCAST(type, handle) ((type)(intptr_t)handle)

@@ -2233,9 +2232,6 @@ void mingw_startup(void)
/* initialize critical section for waitpid pinfo_t list */
InitializeCriticalSection(_cs);

-   /* initialize critical sections in the attr code */
-   attr_start();
-
/* set up default file mode and file modes for stdin/out/err */
_fmode = _O_BINARY;
_setmode(_fileno(stdin), _O_BINARY);

Re: [PATCHv2 27/36] attr: convert to new threadsafe API

[Junio C Hamano]

Stefan Beller  writes:

> On Fri, Oct 28, 2016 at 3:06 PM, Junio C Hamano  wrote:
>> Probably this needs to be squashed in, now the MinGW discussion has
>> settled.
>
> I was about to propose this (and resend it non-rebased).
>
> So I do not resend, but rather ask you to squash this patch?

That's OK.  I've queued a "SQUASH???" separately for now and if we
need futher changes, you may want to resend, but I can locally
squash before merging it down to 'next' if it turns out that there
is no more changes necessary.

Thanks.

Re: [PATCHv2 27/36] attr: convert to new threadsafe API

[Stefan Beller]

On Fri, Oct 28, 2016 at 3:06 PM, Junio C Hamano  wrote:
> Probably this needs to be squashed in, now the MinGW discussion has
> settled.

I was about to propose this (and resend it non-rebased).

So I do not resend, but rather ask you to squash this patch?

Thanks,
Stefan

Re: [PATCHv2 27/36] attr: convert to new threadsafe API

[Junio C Hamano]

Probably this needs to be squashed in, now the MinGW discussion has
settled.

 attr.c | 2 +-
 common-main.c  | 2 ++
 compat/mingw.c | 4 
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/attr.c b/attr.c
index 082b5ed343..961218a0d5 100644
--- a/attr.c
+++ b/attr.c
@@ -50,7 +50,7 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);
 
 #ifndef NO_PTHREADS
 
-static pthread_mutex_t attr_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t attr_mutex;
 #define attr_lock()pthread_mutex_lock(_mutex)
 #define attr_unlock()  pthread_mutex_unlock(_mutex)
 void attr_start(void) { pthread_mutex_init(_mutex, NULL); }
diff --git a/common-main.c b/common-main.c
index 44a29e8b13..d4699cd404 100644
--- a/common-main.c
+++ b/common-main.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "exec_cmd.h"
+#include "attr.h"
 
 /*
  * Many parts of Git have subprograms communicate via pipe, expect the
@@ -32,6 +33,7 @@ int main(int argc, const char **argv)
sanitize_stdfds();
 
git_setup_gettext();
+   attr_start();
 
argv[0] = git_extract_argv0_path(argv[0]);
 
diff --git a/compat/mingw.c b/compat/mingw.c
index 51ed76326b..3fbfda5978 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -5,7 +5,6 @@
 #include "../strbuf.h"
 #include "../run-command.h"
 #include "../cache.h"
-#include "../attr.h"
 
 #define HCAST(type, handle) ((type)(intptr_t)handle)
 
@@ -2233,9 +2232,6 @@ void mingw_startup(void)
/* initialize critical section for waitpid pinfo_t list */
InitializeCriticalSection(_cs);
 
-   /* initialize critical sections in the attr code */
-   attr_start();
-
/* set up default file mode and file modes for stdin/out/err */
_fmode = _O_BINARY;
_setmode(_fileno(stdin), _O_BINARY);

[PATCHv2 27/36] attr: convert to new threadsafe API

[Stefan Beller]

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 *check;

if (!check)
check = git_attr_check_initl("text", NULL);

git_check_attr(path, check);
act_on(check->value[0]);

This has a couple of issues when it comes to thread safety:

* the initialization is racy in this implementation. To make it
  thread safe, we need to acquire a mutex, such that only one
  thread is executing the code in git_attr_check_initl.
  As we do not want to introduce a mutex at each call site,
  this is best done in the attr code. However to do so, we need
  to have access to the `check` variable, i.e. the API has to
  look like
git_attr_check_initl(struct git_attr_check**, ...);
  Then one of the threads calling git_attr_check_initl will
  acquire the mutex and init the `check`, while all other threads
  will wait on the mutex just to realize they're late to the
  party and they'll return with no work done.

* 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.

This commit changes the API and adds locking in
git_attr_check_initl that provides the thread safety for constructing
`struct git_attr_check`.

The usage of the new API will be:

/*
 * The initl call will thread-safely check whether the
 * struct git_attr_check has been initialized. We only
 * want to do the initialization work once, hence we do
 * that work inside a thread safe environment.
 */
static struct git_attr_check *check;
git_attr_check_initl(, "text", NULL);

/* We're just asking for one attribute "text". */
git_attr_result myresult[1];

/* Perform the check and act on it: */
git_check_attr(path, check, myresult);
act_on(myresult[0].value);

/*
 * No need to free the check as it is static, hence doesn't leak
 * memory. The result is also static, so no need to free there either.
 */

Signed-off-by: Stefan Beller 
---
 Documentation/technical/api-gitattributes.txt | 136 +++-
 archive.c |  11 +-
 attr.c| 172 +-
 attr.h|  73 +--
 builtin/check-attr.c  |  50 
 builtin/pack-objects.c|  16 +--
 compat/mingw.c|   4 +
 convert.c |  40 +++---
 ll-merge.c|  24 ++--
 userdiff.c|  16 +--
 ws.c  |   8 +-
 11 files changed, 324 insertions(+), 226 deletions(-)

diff --git a/Documentation/technical/api-gitattributes.txt 
b/Documentation/technical/api-gitattributes.txt
index 92fc32a0ff..7dd6616a24 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -10,21 +10,23 @@ Data Structure
 
 `struct git_attr`::
 
-   An attribute is an opaque object that is identified by its name.
-   Pass the name to `git_attr()` function to obtain the object of
-   this type.  The internal representation of this structure is
+   The internal representation of this structure is
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 one results for a check, such that an
+   array of `struct git_attr_results` corresponds to a
+   `struct git_attr_check`. The answers given in that array are in
+   the the same order as the check struct.
 
 
 Attribute Values
@@ -32,7 +34,7 @@ Attribute Values
 
 An attribute for a path can be in one of four states: Set, Unset,
 Unspecified or set to a string, and `.value` member of `struct
-git_attr_check` records it.  There are three macros to check these:
+git_attr_result` records it.  There are three macros to check these:
 
 `ATTR_TRUE()`::
 
@@ -53,19 +55,28 @@ value of the attribute for the path.
 Querying Specific