Revamping the attr subsystem!
v2 cover Revamping the attr subsystem!
previous discussion:
http://public-inbox.org/git/[email protected]
http://public-inbox.org/git/[email protected]/
This implements the discarded series':
jc/attr
jc/attr-more
sb/pathspec-label
sb/submodule-default-paths
* I rebase to origin master (no merge conflicts)
* I implemented the thread safe attr API in patch 27 (attr: convert to new threadsafe API)
* patch 28 (attr: keep attr stack for each check) makes it actually possible to run in a multithreaded environment.
* I added a test for the multithreaded when it is introduced in patch 32 (pathspec: allow querying for attributes)
Thanks,
Stefan
Junio C Hamano (24):
commit.c: use strchrnul() to scan for one line
attr.c: use strchrnul() to scan for one line
attr.c: update a stale comment on "struct match_attr" attr.c: explain the lack of attr-name syntax check in parse_attr() attr.c: complete a sentence in a comment
attr.c: mark where #if DEBUG ends more clearly
attr.c: simplify macroexpand_one()
attr.c: tighten constness around "git_attr" structure attr.c: plug small leak in parse_attr_line()
attr: rename function and struct related to checking attributes
attr: (re)introduce git_check_attr() and struct git_attr_check
attr: convert git_all_attrs() to use "struct git_attr_check"
attr: convert git_check_attrs() callers to use the new API
attr: retire git_check_attrs() API
attr: add counted string version of git_check_attr()
attr: add counted string version of git_attr()
attr: expose validity check for attribute names
attr.c: add push_stack() helper
attr.c: pass struct git_attr_check down the callchain attr.c: rename a local variable check
attr.c: correct ugly hack for git_all_attrs()
attr.c: introduce empty_attr_check_elems()
attr.c: always pass check[] to collect_some_attrs() attr.c: outline the future plans by heavily commenting
Nguyễn Thái Ngọc Duy (1):
attr: support quoting pathname patterns in C style
Stefan Beller (11):
attr: make git_check_attr_counted static
attr: convert to new threadsafe API
attr: keep attr stack for each check
Documentation: fix a typo
pathspec: move long magic parsing out of prefix_pathspec
pathspec: move prefix check out of the inner loop
pathspec: allow querying for attributes
pathspec: allow escaped query values
submodule update: add --init-default-path switch
clone: add --init-submodule=<pathspec> switch
completion: clone can initialize specific submodules
Documentation/config.txt | 5 + Documentation/git-clone.txt | 23 +- Documentation/git-submodule.txt | 17 +- Documentation/gitattributes.txt | 10 +- Documentation/glossary-content.txt | 20 + Documentation/technical/api-gitattributes.txt | 117 ++++-- archive.c | 26 +- attr.c | 530 ++++++++++++++++++-------- attr.h | 74 +++- builtin/check-attr.c | 65 ++-- builtin/clone.c | 36 +- builtin/pack-objects.c | 27 +- commit.c | 3 +- contrib/completion/git-completion.bash | 1 + convert.c | 45 +-- dir.c | 35 ++ git-submodule.sh | 21 +- hashmap.h | 2 + ll-merge.c | 36 +- pathspec.c | 225 +++++++++-- pathspec.h | 16 + t/t0003-attributes.sh | 26 ++ t/t6134-pathspec-with-labels.sh | 180 +++++++++ t/t7400-submodule-basic.sh | 134 +++++++ userdiff.c | 21 +- ws.c | 26 +- 26 files changed, 1313 insertions(+), 408 deletions(-) create mode 100755 t/t6134-pathspec-with-labels.sh
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 1/36 commit.c: use strchrnul() to scan for one line
From: Junio C Hamano <[email protected]>
--- commit.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
commit.c +1 -2
@@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject) p++;if (*p) { p =skip_blank_lines (p +2 ); -for (eol = p; *eol && *eol !='\n' ; eol++) - ;/* do nothing */ + eol =strchrnul (p,'\n' ); }else eol = p;
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 2/36 attr.c: use strchrnul() to scan for one line
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
--- attr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
attr.c +2 -2
@@ -402,8 +402,8 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)for (sp = buf; *sp; ) {char *ep;int more; -for (ep = sp; *ep && *ep !='\n' ; ep++) - ; + + ep =strchrnul (sp,'\n' ); more = (*ep =='\n' ); *ep ='\0' ;handle_attr_line (res, sp, path, ++lineno, macro_ok);
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 3/36 attr.c: update a stale comment on "struct match_attr"
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
When 82dce998 (attr: more matching optimizations from .gitignore, 2012-10-15) changed a pointer to a string "*pattern" into an embedded "struct pattern" in struct match_attr, it forgot to update the comment that describes the structure.
--- attr.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
attr.c +2 -3
@@ -131,9 +131,8 @@ struct pattern {* If is_macro is true, then u.attr is a pointer to the git_attr being * defined. * -* If is_macro is false, then u.pattern points at the filename pattern -* to which the rule applies. (The memory pointed to is part of the -* memory block allocated for the match_attr instance.) +* If is_macro is false, then u.pat is the filename pattern to which the +* rule applies. * * In either case, num_attr is the number of attributes affected by * this rule, and state is an array listing them. The attributes are
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 4/36 attr.c: explain the lack of attr-name syntax check in parse_attr()
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
--- attr.c | 6 ++++++ 1 file changed, 6 insertions(+)
attr.c +6 -0
@@ -183,6 +183,12 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,return NULL ; } }else { +/* +* As this function is always called twice, once with +* e == NULL in the first pass and then e != NULL in +* the second pass, no need for invalid_attr_name() +* check here. +*/ if (*cp =='-' || *cp =='!' ) { e->setto = (*cp =='-' ) ?ATTR__FALSE :ATTR__UNSET ; cp++;
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 5/36 attr.c: complete a sentence in a comment
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
--- attr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
attr.c +1 -1
@@ -300,7 +300,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,* directory (again, reading the file from top to bottom) down to the * current directory, and then scan the list backwards to find the first match. * This is exactly the same as what is_excluded() does in dir.c to deal with -* .gitignore +* .gitignore file and info/excludes file as a fallback. */ static struct attr_stack {
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 6/36 attr.c: mark where #if DEBUG ends more clearly
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
--- attr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
attr.c +1 -1
@@ -469,7 +469,7 @@ static void debug_set(const char *what, const char *match, struct git_attr *attr#define debug_push (a) do { ; } while (0)#define debug_pop (a) do { ; } while (0)#define debug_set (a,b,c,d) do { ; } while (0) -#endif +#endif /* DEBUG_ATTR */ static void drop_attr_stack (void ) {
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 7/36 attr.c: simplify macroexpand_one()
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
The double-loop wants to do an early return immediately when one matching macro is found. Eliminate the extra variable 'a' used for that purpose and rewrite the "assign the found item to 'a' to make it non-NULL and force the loop(s) to terminate" with a direct return from there.
--- attr.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
attr.c +4 -7
@@ -701,24 +701,21 @@ static int fill(const char *path, int pathlen, int basename_offset,static int macroexpand_one (int nr,int rem) {struct attr_stack *stk; -struct match_attr *a =NULL ;int i;if (check_all_attr[nr].value !=ATTR__TRUE || !check_all_attr[nr].attr->maybe_macro)return rem; -for (stk = attr_stack; !a && stk; stk = stk->prev) -for (i = stk->num_matches -1 ; !a &&0 <= i; i--) { +for (stk = attr_stack; stk; stk = stk->prev) { +for (i = stk->num_matches -1 ;0 <= i; i--) {struct match_attr *ma = stk->attrs[i];if (!ma->is_macro)continue ;if (ma->u.attr->attr_nr == nr) - a = ma; +return fill_one ("expand" , ma, rem); } - -if (a) - rem =fill_one ("expand" , a, rem); + }return rem; }
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 8/36 attr.c: tighten constness around "git_attr" structure
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
It holds an interned string, and git_attr_name() is a way to peek into it. Make sure the involved pointer types are pointer-to-const.
--- attr.c | 2 +- attr.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
attr.c +1 -1
@@ -43,7 +43,7 @@ static int cannot_trust_maybe_real;static struct git_attr_check *check_all_attr;static struct git_attr *(git_attr_hash[HASHSIZE ]); -char *git_attr_name (struct git_attr *attr) +const char *git_attr_name (const struct git_attr *attr) {return attr->name; }
attr.h +2 -2
@@ -25,7 +25,7 @@ extern const char git_attr__false[];* Unset one is returned as NULL. */ struct git_attr_check { -struct git_attr *attr; +const struct git_attr *attr;const char *value; };
@@ -34,7 +34,7 @@ struct git_attr_check {* 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. */ -char *git_attr_name (struct git_attr *); +extern const char *git_attr_name (const struct git_attr *);int git_check_attr (const char *path,int ,struct git_attr_check *);
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 9/36 attr.c: plug small leak in parse_attr_line()
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
If any error is noticed after the match_attr structure is allocated, we shouldn't just return NULL from this function.
Add a fail_return label that frees the allocated structure and returns NULL, and consistently jump there when we want to return NULL after cleaning up.
--- attr.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
attr.c +8 -4
@@ -223,7 +223,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,if (!macro_ok) {fprintf (stderr,"%s not allowed: %s:%d\n" , name, src, lineno); -return NULL ; + goto fail_return; } is_macro =1 ; name +=strlen (ATTRIBUTE_MACRO_PREFIX );
@@ -233,7 +233,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,fprintf (stderr,"%.*s is not a valid attribute name: %s:%d\n" , namelen, name, src, lineno); -return NULL ; + goto fail_return; } }else
@@ -246,7 +246,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,for (cp = states, num_attr =0 ; *cp; num_attr++) { cp =parse_attr (src, lineno, cp,NULL );if (!cp) -return NULL ; + goto fail_return; } res = xcalloc(1,
@@ -267,7 +267,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,if (res->u.pat.flags &EXC_FLAG_NEGATIVE ) {warning (_ ("Negative patterns are ignored in git attributes\n" "Use '\\!' for literal leading exclamation." )); -return NULL ; + goto fail_return; } } res->is_macro = is_macro;
@@ -283,6 +283,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src, }return res; + +fail_return: +free (res); +return NULL ; }/*
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 10/36 attr: rename function and struct related to checking attributes
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
The traditional API to check attributes is to prepare an N-element array of "struct git_attr_check" and pass N and the array to the function "git_check_attr()" as arguments.
In preparation to revamp the API to pass a single structure, in which these N elements are held, rename the type used for these individual array elements to "struct git_attr_check_elem" and rename the function to "git_check_attrs()".
--- archive.c | 6 +++--- attr.c | 12 ++++++------ attr.h | 8 ++++---- builtin/check-attr.c | 19 ++++++++++--------- builtin/pack-objects.c | 6 +++--- convert.c | 12 ++++++------ ll-merge.c | 10 +++++----- userdiff.c | 4 ++-- ws.c | 6 +++--- 9 files changed, 42 insertions(+), 41 deletions(-)
archive.c +3 -3
@@ -87,7 +87,7 @@ void *sha1_file_to_archive(const struct archiver_args *args,return buffer; } -static void setup_archive_check (struct git_attr_check *check) +static void setup_archive_check (struct git_attr_check_elem *check) {static struct git_attr *attr_export_ignore;static struct git_attr *attr_export_subst;
@@ -123,7 +123,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,struct archiver_context *c = context;struct archiver_args *args = c->args;write_archive_entry_fn_t write_entry = c->write_entry; -struct git_attr_check check[2 ]; +struct git_attr_check_elem check[2 ];const char *path_without_prefix;int err;
@@ -138,7 +138,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base, path_without_prefix = path.buf + args->baselen;setup_archive_check (check); -if (!git_check_attr (path_without_prefix,ARRAY_SIZE (check), check)) { +if (!git_check_attrs (path_without_prefix,ARRAY_SIZE (check), check)) {if (ATTR_TRUE (check[0 ].value))return 0 ; args->convert =ATTR_TRUE (check[1 ].value);
attr.c +6 -6
@@ -40,7 +40,7 @@ struct git_attr {static int attr_nr;static int cannot_trust_maybe_real; -static struct git_attr_check *check_all_attr; +static struct git_attr_check_elem *check_all_attr;static struct git_attr *(git_attr_hash[HASHSIZE ]);const char *git_attr_name (const struct git_attr *attr)
@@ -665,7 +665,7 @@ static int macroexpand_one(int attr_nr, int rem);static int fill_one (const char *what,struct match_attr *a,int rem) { -struct git_attr_check *check = check_all_attr; +struct git_attr_check_elem *check = check_all_attr;int i;for (i = a->num_attr -1 ;0 < rem &&0 <= i; i--) {
@@ -730,7 +730,7 @@ static int macroexpand_one(int nr, int rem)* collected. Otherwise all attributes are collected. */ static void collect_some_attrs (const char *path,int num, -struct git_attr_check *check) +struct git_attr_check_elem *check) {struct attr_stack *stk;
@@ -758,7 +758,7 @@ static void collect_some_attrs(const char *path, int num, rem =0 ;for (i =0 ; i < num; i++) {if (!check[i].attr->maybe_real) { -struct git_attr_check *c; +struct git_attr_check_elem *c; c = check_all_attr + check[i].attr->attr_nr; c->value =ATTR__UNSET ; rem++;
@@ -773,7 +773,7 @@ static void collect_some_attrs(const char *path, int num, rem =fill (path, pathlen, basename_offset, stk, rem); } -int git_check_attr (const char *path,int num,struct git_attr_check *check) +int git_check_attrs (const char *path,int num,struct git_attr_check_elem *check) {int i;
@@ -789,7 +789,7 @@ int git_check_attr(const char *path, int num, struct git_attr_check *check)return 0 ; } -int git_all_attrs (const char *path,int *num,struct git_attr_check **check) +int git_all_attrs (const char *path,int *num,struct git_attr_check_elem **check) {int i, count, j;
attr.h +4 -4
@@ -20,11 +20,11 @@ extern const char git_attr__false[];#define ATTR_UNSET (v) ((v) == NULL)/* -* Send one or more git_attr_check to git_check_attr(), and +* Send one or more git_attr_check to git_check_attrs(), and * each 'value' member tells what its value is. * Unset one is returned as NULL. */ -struct git_attr_check { +struct git_attr_check_elem {const struct git_attr *attr;const char *value; };
@@ -36,7 +36,7 @@ struct git_attr_check {*/ extern const char *git_attr_name (const struct git_attr *); -int git_check_attr (const char *path,int ,struct git_attr_check *); +int git_check_attrs (const char *path,int ,struct git_attr_check_elem *);/* * Retrieve all attributes that apply to the specified path. *num
@@ -45,7 +45,7 @@ int git_check_attr(const char *path, int, struct git_attr_check *);* objects describing the attributes and their values. *check must be * free()ed by the caller. */ -int git_all_attrs (const char *path,int *num,struct git_attr_check **check); +int git_all_attrs (const char *path,int *num,struct git_attr_check_elem **check);enum git_attr_direction {GIT_ATTR_CHECKIN ,
builtin/check-attr.c +10 -9
@@ -24,8 +24,8 @@ static const struct option check_attr_options[] = { OPT_END() }; -static void output_attr (int cnt,struct git_attr_check *check, -const char *file) +static void output_attr (int cnt,struct git_attr_check_elem *check, +const char *file) {int j;for (j =0 ; j < cnt; j++) {
@@ -51,14 +51,15 @@ static void output_attr(int cnt, struct git_attr_check *check, } } -static void check_attr (const char *prefix,int cnt, -struct git_attr_check *check,const char *file) +static void check_attr (const char *prefix, +int cnt,struct git_attr_check_elem *check, +const char *file) {char *full_path =prefix_path (prefix, prefix ?strlen (prefix) :0 , file);if (check !=NULL ) { -if (git_check_attr (full_path, cnt, check)) -die ("git_check_attr died" ); +if (git_check_attrs (full_path, cnt, check)) +die ("git_check_attrs died" );output_attr (cnt, check, file); }else {if (git_all_attrs (full_path, &cnt, &check))
@@ -69,8 +70,8 @@ static void check_attr(const char *prefix, int cnt,free (full_path); } -static void check_attr_stdin_paths (const char *prefix,int cnt, -struct git_attr_check *check) +static void check_attr_stdin_paths (const char *prefix, +int cnt,struct git_attr_check_elem *check) {struct strbuf buf =STRBUF_INIT ;struct strbuf unquoted =STRBUF_INIT ;
@@ -99,7 +100,7 @@ static NORETURN void error_with_usage(const char *msg)int cmd_check_attr (int argc,const char **argv,const char *prefix) { -struct git_attr_check *check; +struct git_attr_check_elem *check;int cnt, i, doubledash, filei;if (!is_bare_repository ())
builtin/pack-objects.c +3 -3
@@ -896,7 +896,7 @@ static void write_pack_file(void) written, nr_result); } -static void setup_delta_attr_check (struct git_attr_check *check) +static void setup_delta_attr_check (struct git_attr_check_elem *check) {static struct git_attr *attr_delta;
@@ -908,10 +908,10 @@ static void setup_delta_attr_check(struct git_attr_check *check)static int no_try_delta (const char *path) { -struct git_attr_check check[1 ]; +struct git_attr_check_elem check[1 ];setup_delta_attr_check (check); -if (git_check_attr (path,ARRAY_SIZE (check), check)) +if (git_check_attrs (path,ARRAY_SIZE (check), check))return 0 ;if (ATTR_FALSE (check->value))return 1 ;
convert.c +6 -6
@@ -718,7 +718,7 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,return 1 ; } -static enum crlf_action git_path_check_crlf (struct git_attr_check *check) +static enum crlf_action git_path_check_crlf (struct git_attr_check_elem *check) {const char *value = check->value;
@@ -735,7 +735,7 @@ static enum crlf_action git_path_check_crlf(struct git_attr_check *check)return CRLF_UNDEFINED ; } -static enum eol git_path_check_eol (struct git_attr_check *check) +static enum eol git_path_check_eol (struct git_attr_check_elem *check) {const char *value = check->value;
@@ -748,7 +748,7 @@ static enum eol git_path_check_eol(struct git_attr_check *check)return EOL_UNSET ; } -static struct convert_driver *git_path_check_convert (struct git_attr_check *check) +static struct convert_driver *git_path_check_convert (struct git_attr_check_elem *check) {const char *value = check->value;struct convert_driver *drv;
@@ -761,7 +761,7 @@ static struct convert_driver *git_path_check_convert(struct git_attr_check *checreturn NULL ; } -static int git_path_check_ident (struct git_attr_check *check) +static int git_path_check_ident (struct git_attr_check_elem *check) {const char *value = check->value;
@@ -783,7 +783,7 @@ static const char *conv_attr_name[] = {static void convert_attrs (struct conv_attrs *ca,const char *path) {int i; -static struct git_attr_check ccheck[NUM_CONV_ATTRS ]; +static struct git_attr_check_elem ccheck[NUM_CONV_ATTRS ];if (!ccheck[0 ].attr) {for (i =0 ; i <NUM_CONV_ATTRS ; i++)
@@ -792,7 +792,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)git_config (read_convert_config,NULL ); } -if (!git_check_attr (path,NUM_CONV_ATTRS , ccheck)) { +if (!git_check_attrs (path,NUM_CONV_ATTRS , ccheck)) { ca->crlf_action =git_path_check_crlf (ccheck +4 );if (ca->crlf_action ==CRLF_UNDEFINED ) ca->crlf_action =git_path_check_crlf (ccheck +0 );
ll-merge.c +5 -5
@@ -336,13 +336,13 @@ static const struct ll_merge_driver *find_ll_merge_driver(const char *merge_attrreturn &ll_merge_drv[LL_TEXT_MERGE ]; } -static int git_path_check_merge (const char *path,struct git_attr_check check[2 ]) +static int git_path_check_merge (const char *path,struct git_attr_check_elem check[2 ]) {if (!check[0 ].attr) { check[0 ].attr =git_attr ("merge" ); check[1 ].attr =git_attr ("conflict-marker-size" ); } -return git_check_attr (path,2 , check); +return git_check_attrs (path,2 , check); }static void normalize_file (mmfile_t *mm,const char *path)
@@ -362,7 +362,7 @@ int ll_merge(mmbuffer_t *result_buf, mmfile_t *theirs, const char *their_label, const struct ll_merge_options *opts) { - static struct git_attr_check check[2]; + static struct git_attr_check_elem check[2];static const struct ll_merge_options default_opts;const char *ll_driver_name =NULL ;int marker_size =DEFAULT_CONFLICT_MARKER_SIZE ;
@@ -398,12 +398,12 @@ int ll_merge(mmbuffer_t *result_buf,int ll_merge_marker_size (const char *path) { -static struct git_attr_check check; +static struct git_attr_check_elem check;int marker_size =DEFAULT_CONFLICT_MARKER_SIZE ;if (!check.attr) check.attr =git_attr ("conflict-marker-size" ); -if (!git_check_attr (path,1 , &check) && check.value) { +if (!git_check_attrs (path,1 , &check) && check.value) { marker_size =atoi (check.value);if (marker_size <=0 ) marker_size =DEFAULT_CONFLICT_MARKER_SIZE ;
userdiff.c +2 -2
@@ -263,7 +263,7 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {struct userdiff_driver *userdiff_find_by_path (const char *path) {static struct git_attr *attr; -struct git_attr_check check; +struct git_attr_check_elem check;if (!attr) attr =git_attr ("diff" );
@@ -271,7 +271,7 @@ struct userdiff_driver *userdiff_find_by_path(const char *path)if (!path)return NULL ; -if (git_check_attr (path,1 , &check)) +if (git_check_attrs (path,1 , &check))return NULL ;if (ATTR_TRUE (check.value))
ws.c +3 -3
@@ -71,7 +71,7 @@ unsigned parse_whitespace_rule(const char *string)return rule; } -static void setup_whitespace_attr_check (struct git_attr_check *check) +static void setup_whitespace_attr_check (struct git_attr_check_elem *check) {static struct git_attr *attr_whitespace;
@@ -82,10 +82,10 @@ static void setup_whitespace_attr_check(struct git_attr_check *check)unsigned whitespace_rule (const char *pathname) { -struct git_attr_check attr_whitespace_rule; +struct git_attr_check_elem attr_whitespace_rule;setup_whitespace_attr_check (&attr_whitespace_rule); -if (!git_check_attr (pathname,1 , &attr_whitespace_rule)) { +if (!git_check_attrs (pathname,1 , &attr_whitespace_rule)) {const char *value; value = attr_whitespace_rule.value;
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 11/36 attr: (re)introduce git_check_attr() and struct git_attr_check
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
A common pattern to check N attributes for many paths is to
(1) prepare an array A of N git_attr_check_elem items;
(2) call git_attr() to intern the N attribute names and fill A;
(3) repeatedly call git_check_attrs() for path with N and A;
A look-up for these N attributes for a single path P scans the entire attr_stack, starting from the .git/info/attributes file and then .gitattributes file in the directory the path P is in, going upwards to find .gitattributes file found in parent directories.
An earlier commit 06a604e6 (attr: avoid heavy work when we know the specified attr is not defined, 2014-12-28) tried to optimize out this scanning for one trivial special case: when the attribute being sought is known not to exist, we do not have to scan for it. While this may be a cheap and effective heuristic, it would not work well when N is (much) more than 1.
What we would want is a more customized way to skip irrelevant entries in the attribute stack, and the definition of irrelevance is tied to the set of attributes passed to git_check_attrs() call, i.e. the set of attributes being sought. The data necessary for this optimization needs to live alongside the set of attributes, but a simple array of git_attr_check_elem simply does not have any place for that.
Introduce "struct git_attr_check" that contains N, the number of attributes being sought, and A, the array that holds N git_attr_check_elem items, and a function git_check_attr() that takes a path P and this structure as its parameters. This structure can later be extended to hold extra data necessary for optimization.
Also, to make it easier to write the first two steps in common cases, introduce git_attr_check_initl() helper function, which takes a NULL-terminated list of attribute names and initialize this structure.
As an illustration of this new API, convert archive.c that asks for export-subst and export-ignore attributes for each paths.
--- archive.c | 24 ++++++------------------ attr.c | 34 ++++++++++++++++++++++++++++++++++ attr.h | 9 +++++++++ 3 files changed, 49 insertions(+), 18 deletions(-)
archive.c +6 -18
@@ -87,19 +87,6 @@ void *sha1_file_to_archive(const struct archiver_args *args,return buffer; } -static void setup_archive_check (struct git_attr_check_elem *check) -{ -static struct git_attr *attr_export_ignore; -static struct git_attr *attr_export_subst; - -if (!attr_export_ignore) { - attr_export_ignore =git_attr ("export-ignore" ); - attr_export_subst =git_attr ("export-subst" ); - } - check[0 ].attr = attr_export_ignore; - check[1 ].attr = attr_export_subst; -} -struct directory {struct directory *up;struct object_id oid;
@@ -123,7 +110,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,struct archiver_context *c = context;struct archiver_args *args = c->args;write_archive_entry_fn_t write_entry = c->write_entry; -struct git_attr_check_elem check[2 ]; +static struct git_attr_check *check;const char *path_without_prefix;int err;
@@ -137,11 +124,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,strbuf_addch (&path,'/' ); path_without_prefix = path.buf + args->baselen; -setup_archive_check (check); -if (!git_check_attrs (path_without_prefix,ARRAY_SIZE (check), check)) { -if (ATTR_TRUE (check[0 ].value)) +if (!check) + check =git_attr_check_initl ("export-ignore" ,"export-subst" ,NULL ); +if (!git_check_attr (path_without_prefix, check)) { +if (ATTR_TRUE (check->check[0 ].value))return 0 ; - args->convert =ATTR_TRUE (check[1 ].value); + args->convert =ATTR_TRUE (check->check[1 ].value); }if (S_ISDIR (mode) ||S_ISGITLINK (mode)) {
attr.c +34 -0
@@ -829,3 +829,37 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *istdrop_attr_stack (); use_index = istate; } + +int git_check_attr (const char *path,struct git_attr_check *check) +{ +return git_check_attrs (path, check->check_nr, check->check); +} + +struct git_attr_check *git_attr_check_initl (const char *one, ...) +{ +struct git_attr_check *check; +int cnt; +va_list params; +const char *param; + +va_start (params, one); + for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++) + ; +va_end (params); + check =xcalloc (1 , +sizeof (*check) + cnt *sizeof (*(check->check))); + check->check_nr = cnt; + check->check = (struct git_attr_check_elem *)(check +1 ); + + check->check[0 ].attr =git_attr (one); +va_start (params, one); +for (cnt =1 ; cnt < check->check_nr; cnt++) { + param = va_arg(params, const char *); +if (!param) +die ("BUG: counted %d != ended at %d" , + check->check_nr, cnt); + check->check[cnt].attr =git_attr (param); + } +va_end (params); +return check; +}
attr.h +9 -0
@@ -29,6 +29,15 @@ struct git_attr_check_elem {const char *value; }; +struct git_attr_check { +int check_nr; +int check_alloc; +struct git_attr_check_elem *check; +}; + +extern struct git_attr_check *git_attr_check_initl (const char *, ...); +extern int git_check_attr (const char *path,struct git_attr_check *); +/* * Return the name of the attribute represented by the argument. The * return value is a pointer to a null-delimited string that is part
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 12/36 attr: convert git_all_attrs() to use "struct git_attr_check"
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
This updates the other two ways the attribute check is done via an array of "struct git_attr_check_elem" elements. These two niches appear only in "git check-attr".
* The caller does not know offhand what attributes it wants to ask about and cannot use git_attr_check_initl() to prepare the git_attr_check structure.
* The caller may not know what attributes it wants to ask at all, and instead wants to learn everything that the given path has.
Such a caller can call git_attr_check_alloc() to allocate an empty git_attr_check, and then call git_attr_check_append() to add attribute names one by one. A new attribute can be appended until git_attr_check structure is "finalized", which happens when it is used to ask for attributes for any path by calling git_check_attr() or git_all_attrs(). A git_attr_check structure that is initialized by git_attr_check_initl() is already finalized when it is returned.
I am not at all happy with the way git_all_attrs() API turned out to be, but it is only to support one niche caller ("check-attr --all"), so I'll stop here for now.
--- attr.c | 75 ++++++++++++++++++++++++++++++++++++++-------------- attr.h | 16 ++++++----- builtin/check-attr.c | 51 ++++++++++++++++++----------------- 3 files changed, 90 insertions(+), 52 deletions(-)
attr.c +55 -20
@@ -724,6 +724,11 @@ static int macroexpand_one(int nr, int rem)return rem; } +static int attr_check_is_dynamic (const struct git_attr_check *check) +{ +return (void *)(check->check) != (void *)(check +1 ); +} +/* * Collect attributes for path into the array pointed to by * check_all_attr. If num is non-zero, only attributes in check[] are
@@ -789,32 +794,21 @@ int git_check_attrs(const char *path, int num, struct git_attr_check_elem *checkreturn 0 ; } -int git_all_attrs (const char *path,int *num,struct git_attr_check_elem **check) +void git_all_attrs (const char *path,struct git_attr_check *check) { -int i, count, j; +int i; +git_attr_check_clear (check);collect_some_attrs (path,0 ,NULL ); -/* Count the number of attributes that are set. */ - count =0 ; -for (i =0 ; i < attr_nr; i++) { -const char *value = check_all_attr[i].value; -if (value !=ATTR__UNSET && value !=ATTR__UNKNOWN ) - ++count; - } - *num = count; -ALLOC_ARRAY (*check, count); - j =0 ;for (i =0 ; i < attr_nr; i++) { +const char *name = check_all_attr[i].attr->name;const char *value = check_all_attr[i].value; -if (value !=ATTR__UNSET && value !=ATTR__UNKNOWN ) { - (*check)[j].attr = check_all_attr[i].attr; - (*check)[j].value = value; - ++j; - } +if (value ==ATTR__UNSET || value ==ATTR__UNKNOWN ) +continue ; +git_attr_check_append (check,git_attr (name)); + check->check[check->check_nr -1 ].value = value; } - -return 0 ; }void git_attr_set_direction (enum git_attr_direction new,struct index_state *istate)
@@ -832,6 +826,7 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *istint git_check_attr (const char *path,struct git_attr_check *check) { + check->finalized =1 ;return git_check_attrs (path, check->check_nr, check->check); }
@@ -849,17 +844,57 @@ struct git_attr_check *git_attr_check_initl(const char *one, ...) check =xcalloc (1 ,sizeof (*check) + cnt *sizeof (*(check->check))); check->check_nr = cnt; + check->finalized =1 ; check->check = (struct git_attr_check_elem *)(check +1 ); check->check[0 ].attr =git_attr (one);va_start (params, one);for (cnt =1 ; cnt < check->check_nr; cnt++) { +struct git_attr *attr; param = va_arg(params, const char *);if (!param)die ("BUG: counted %d != ended at %d" , check->check_nr, cnt); - check->check[cnt].attr =git_attr (param); + attr =git_attr (param); +if (!attr) +die ("BUG: %s: not a valid attribute name" , param); + check->check[cnt].attr = attr; }va_end (params);return check; } + +struct git_attr_check *git_attr_check_alloc (void ) +{ +return xcalloc (1 ,sizeof (struct git_attr_check )); +} + +struct git_attr_check_elem *git_attr_check_append (struct git_attr_check *check, +const struct git_attr *attr) +{ +struct git_attr_check_elem *elem; +if (check->finalized) +die ("BUG: append after git_attr_check structure is finalized" ); +if (!attr_check_is_dynamic (check)) +die ("BUG: appending to a statically initialized git_attr_check" ); +ALLOC_GROW (check->check, check->check_nr +1 , check->check_alloc); + elem = &check->check[check->check_nr++]; + elem->attr = attr; +return elem; +} + +void git_attr_check_clear (struct git_attr_check *check) +{ +if (!attr_check_is_dynamic (check)) +die ("BUG: clearing a statically initialized git_attr_check" ); +free (check->check); + check->check_nr =0 ; + check->check_alloc =0 ; + check->finalized =0 ; +} + +void git_attr_check_free (struct git_attr_check *check) +{ +git_attr_check_clear (check); +free (check); +}
attr.h +10 -6
@@ -30,6 +30,7 @@ struct git_attr_check_elem { };struct git_attr_check { +int finalized;int check_nr;int check_alloc;struct git_attr_check_elem *check;
@@ -38,6 +39,12 @@ struct git_attr_check {extern struct git_attr_check *git_attr_check_initl (const char *, ...);extern int git_check_attr (const char *path,struct git_attr_check *); +extern struct git_attr_check *git_attr_check_alloc (void ); +extern struct git_attr_check_elem *git_attr_check_append (struct git_attr_check *,const struct git_attr *); + +extern void git_attr_check_clear (struct git_attr_check *); +extern void git_attr_check_free (struct git_attr_check *); +/* * Return the name of the attribute represented by the argument. The * return value is a pointer to a null-delimited string that is part
@@ -48,13 +55,10 @@ extern const char *git_attr_name(const struct git_attr *);int git_check_attrs (const char *path,int ,struct git_attr_check_elem *);/* -* Retrieve all attributes that apply to the specified path. *num -* will be set to the number of attributes on the path; **check will -* be set to point at a newly-allocated array of git_attr_check -* objects describing the attributes and their values. *check must be -* free()ed by the caller. +* Retrieve all attributes that apply to the specified path. +* check holds the attributes and their values. */ -int git_all_attrs (const char *path,int *num,struct git_attr_check_elem **check); +void git_all_attrs (const char *path,struct git_attr_check *check);enum git_attr_direction {GIT_ATTR_CHECKIN ,
builtin/check-attr.c +25 -26
111 lines not on this page · raw message
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 13/36 attr: convert git_check_attrs() callers to use the new API
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
The remaining callers are all simple "I have N attributes I am interested in. I'll ask about them with various paths one by one".
After this step, no caller to git_check_attrs() remains. After removing it, we can extend "struct git_attr_check" struct with data that can be used in optimizing the query for the specific N attributes it contains.
--- builtin/pack-objects.c | 19 +++++-------------- convert.c | 18 +++++++----------- ll-merge.c | 33 ++++++++++++++------------------- userdiff.c | 19 ++++++++----------- ws.c | 19 ++++++------------- 5 files changed, 40 insertions(+), 68 deletions(-)
builtin/pack-objects.c +5 -14
@@ -896,24 +896,15 @@ static void write_pack_file(void) written, nr_result); } -static void setup_delta_attr_check (struct git_attr_check_elem *check) -{ -static struct git_attr *attr_delta; - -if (!attr_delta) - attr_delta =git_attr ("delta" ); - - check[0 ].attr = attr_delta; -} -static int no_try_delta (const char *path) { -struct git_attr_check_elem check[1 ]; +static struct git_attr_check *check; -setup_delta_attr_check (check); -if (git_check_attrs (path,ARRAY_SIZE (check), check)) +if (!check) + check =git_attr_check_initl ("delta" ,NULL ); +if (git_check_attr (path, check))return 0 ; -if (ATTR_FALSE (check->value)) +if (ATTR_FALSE (check->check[0 ].value))return 1 ;return 0 ; }
convert.c +7 -11
@@ -775,24 +775,20 @@ struct conv_attrs {int ident; }; -static const char *conv_attr_name[] = { -"crlf" ,"ident" ,"filter" ,"eol" ,"text" , -}; -#define NUM_CONV_ATTRS ARRAY_SIZE(conv_attr_name) -static void convert_attrs (struct conv_attrs *ca,const char *path) { -int i; -static struct git_attr_check_elem ccheck[NUM_CONV_ATTRS ]; +static struct git_attr_check *check; -if (!ccheck[0 ].attr) { -for (i =0 ; i <NUM_CONV_ATTRS ; i++) - ccheck[i].attr =git_attr (conv_attr_name[i]); +if (!check) { + check =git_attr_check_initl ("crlf" ,"ident" , +"filter" ,"eol" ,"text" , +NULL ); user_convert_tail = &user_convert;git_config (read_convert_config,NULL ); } -if (!git_check_attrs (path,NUM_CONV_ATTRS , ccheck)) { +if (!git_check_attr (path, check)) { +struct git_attr_check_elem *ccheck = check->check; ca->crlf_action =git_path_check_crlf (ccheck +4 );if (ca->crlf_action ==CRLF_UNDEFINED ) ca->crlf_action =git_path_check_crlf (ccheck +0 );
ll-merge.c +14 -19
@@ -336,15 +336,6 @@ static const struct ll_merge_driver *find_ll_merge_driver(const char *merge_attrreturn &ll_merge_drv[LL_TEXT_MERGE ]; } -static int git_path_check_merge (const char *path,struct git_attr_check_elem check[2 ]) -{ -if (!check[0 ].attr) { - check[0 ].attr =git_attr ("merge" ); - check[1 ].attr =git_attr ("conflict-marker-size" ); - } -return git_check_attrs (path,2 , check); -} -static void normalize_file (mmfile_t *mm,const char *path) {struct strbuf strbuf =STRBUF_INIT ;
@@ -362,7 +353,7 @@ int ll_merge(mmbuffer_t *result_buf, mmfile_t *theirs, const char *their_label, const struct ll_merge_options *opts) { - static struct git_attr_check_elem check[2]; + static struct git_attr_check *check;static const struct ll_merge_options default_opts;const char *ll_driver_name =NULL ;int marker_size =DEFAULT_CONFLICT_MARKER_SIZE ;
@@ -376,10 +367,14 @@ int ll_merge(mmbuffer_t *result_buf,normalize_file (ours, path);normalize_file (theirs, path); } -if (!git_path_check_merge (path, check)) { - ll_driver_name = check[0 ].value; -if (check[1 ].value) { - marker_size =atoi (check[1 ].value); + +if (!check) + check =git_attr_check_initl ("merge" ,"conflict-marker-size" ,NULL ); + +if (!git_check_attr (path, check)) { + ll_driver_name = check->check[0 ].value; +if (check->check[1 ].value) { + marker_size =atoi (check->check[1 ].value);if (marker_size <=0 ) marker_size =DEFAULT_CONFLICT_MARKER_SIZE ; }
@@ -398,13 +393,13 @@ int ll_merge(mmbuffer_t *result_buf,int ll_merge_marker_size (const char *path) { -static struct git_attr_check_elem check; +static struct git_attr_check *check;int marker_size =DEFAULT_CONFLICT_MARKER_SIZE ; -if (!check.attr) - check.attr =git_attr ("conflict-marker-size" ); -if (!git_check_attrs (path,1 , &check) && check.value) { - marker_size =atoi (check.value); +if (!check) + check =git_attr_check_initl ("conflict-marker-size" ,NULL ); +if (!git_check_attr (path, check) && check->check[0 ].value) { + marker_size =atoi (check->check[0 ].value);if (marker_size <=0 ) marker_size =DEFAULT_CONFLICT_MARKER_SIZE ; }
userdiff.c +8 -11
@@ -262,25 +262,22 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {struct userdiff_driver *userdiff_find_by_path (const char *path) { -static struct git_attr *attr; -struct git_attr_check_elem check; - -if (!attr) - attr =git_attr ("diff" ); - check.attr = attr; +static struct git_attr_check *check; +if (!check) + check =git_attr_check_initl ("diff" ,NULL );if (!path)return NULL ; -if (git_check_attrs (path,1 , &check)) +if (git_check_attr (path, check))return NULL ; -if (ATTR_TRUE (check.value)) +if (ATTR_TRUE (check->check[0 ].value))return &driver_true; -if (ATTR_FALSE (check.value)) +if (ATTR_FALSE (check->check[0 ].value))return &driver_false; -if (ATTR_UNSET (check.value)) +if (ATTR_UNSET (check->check[0 ].value))return NULL ; -return userdiff_find_by_name (check.value); +return userdiff_find_by_name (check->check[0 ].value); }struct userdiff_driver *userdiff_get_textconv (struct userdiff_driver *driver)
ws.c +6 -13
@@ -71,24 +71,17 @@ unsigned parse_whitespace_rule(const char *string)return rule; } -static void setup_whitespace_attr_check (struct git_attr_check_elem *check) -{ -static struct git_attr *attr_whitespace; - -if (!attr_whitespace) - attr_whitespace =git_attr ("whitespace" ); - check[0 ].attr = attr_whitespace; -} -unsigned whitespace_rule (const char *pathname) { -struct git_attr_check_elem attr_whitespace_rule; +static struct git_attr_check *attr_whitespace_rule; + +if (!attr_whitespace_rule) + attr_whitespace_rule =git_attr_check_initl ("whitespace" ,NULL ); -setup_whitespace_attr_check (&attr_whitespace_rule); -if (!git_check_attrs (pathname,1 , &attr_whitespace_rule)) { +if (!git_check_attr (pathname, attr_whitespace_rule)) {const char *value; - value = attr_whitespace_rule.value; + value = attr_whitespace_rule->check[0 ].value;if (ATTR_TRUE (value)) {/* true (whitespace) */ unsigned all_rule =ws_tab_width (whitespace_rule_cfg);
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 14/36 attr: retire git_check_attrs() API
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
Since nobody uses the old API, make it file-scope static, and update the documentation to describe the new API.
--- Documentation/technical/api-gitattributes.txt | 82 ++++++++++++++++++--------- attr.c | 3 +- attr.h | 2 - 3 files changed, 56 insertions(+), 31 deletions(-)
Documentation/technical/api-gitattributes.txt +54 -28
@@ -16,10 +16,15 @@ 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 set of attributes to check in a call - to `git_check_attr()` function, and receives the results. + This structure represents a collection of `git_attr_check_elem`. + It is passed to `git_check_attr()` function, specifying the + attributes to check, and receives their values. Attribute Values
@@ -48,49 +53,51 @@ value of the attribute for the path. Querying Specific Attributes ---------------------------- -* Prepare an array of `struct git_attr_check` to define the list of - attributes you would want to check. To populate this array, you would - need to define necessary attributes by calling `git_attr()` function. +* 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. * 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. +* Inspect `git_attr_check` structure to see how each of the + attribute in the array is defined for the path. Example ------- -To see how attributes "crlf" and "indent" are set for different paths. +To see how attributes "crlf" and "ident" are set for different paths. -. Prepare an array of `struct git_attr_check` with two elements (because - we are checking two attributes). Initialize their `attr` member with - pointers to `struct git_attr` obtained by calling `git_attr()`: +. Prepare a `struct git_attr_check` with two elements (because + we are checking two attributes): ------------ -static struct git_attr_check check[2]; +static struct git_attr_check *check; static void setup_check(void) { - if (check[0].attr) + if (check) return; /* already done */ - check[0].attr = git_attr("crlf"); - check[1].attr = git_attr("ident"); + check = git_attr_check_initl("crlf", "ident", NULL); } ------------ -. Call `git_check_attr()` with the prepared array of `struct git_attr_check`: +. Call `git_check_attr()` with the prepared `struct git_attr_check`: ------------ const char *path; setup_check(); - git_check_attr(path, ARRAY_SIZE(check), check); + git_check_attr(path, check); ------------ -. Act on `.value` member of the result, left in `check[]`: +. Act on `.value` member of the result, left in `check->check[]`: ------------ - const char *value = check[0].value; + const char *value = check->check[0].value; if (ATTR_TRUE(value)) { The attribute is Set, by listing only the name of the
@@ -109,20 +116,39 @@ static void setup_check(void) } ------------ +To see how attributes in argv[] are set for different paths, only +the first step in the above would be different. + +------------ +static struct git_attr_check *check; +static void setup_check(const char **argv) +{ + check = git_attr_check_alloc(); + while (*argv) { + struct git_attr *attr = git_attr(*argv); + git_attr_check_append(check, attr); + argv++; + } +} +------------ + Querying All Attributes ----------------------- To get the values of all attributes associated with a file: -* Call `git_all_attrs()`, which returns an array of `git_attr_check` - structures. +* Prepare an empty `git_attr_check` structure by calling + `git_attr_check_alloc()`. + +* Call `git_all_attrs()`, which populates the `git_attr_check` + with the attributes attached to the path. -* Iterate over the `git_attr_check` array to examine the attribute - names and values. The name of the attribute described by a - `git_attr_check` object can be retrieved via - `git_attr_name(check[i].attr)`. (Please note that no items will be - returned for unset attributes, so `ATTR_UNSET()` will return false - for all returned `git_array_check` objects.) +* Iterate over the `git_attr_check.check[]` array to examine + the attribute names and values. The name of the attribute + described by a `git_attr_check.check[]` object can be retrieved via + `git_attr_name(check->check[i].attr)`. (Please note that no items + will be returned for unset attributes, so `ATTR_UNSET()` will return + false for all returned `git_array_check` objects.) -* Free the `git_array_check` array. +* Free the `git_array_check` by calling `git_attr_check_free()`.
attr.c +2 -1
@@ -778,7 +778,8 @@ static void collect_some_attrs(const char *path, int num, rem =fill (path, pathlen, basename_offset, stk, rem); } -int git_check_attrs (const char *path,int num,struct git_attr_check_elem *check) +static int git_check_attrs (const char *path,int num, +struct git_attr_check_elem *check) {int i;
attr.h +0 -2
@@ -52,8 +52,6 @@ extern void git_attr_check_free(struct git_attr_check *);*/ extern const char *git_attr_name (const struct git_attr *); -int git_check_attrs (const char *path,int ,struct git_attr_check_elem *); -/* * Retrieve all attributes that apply to the specified path. * check holds the attributes and their values.
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 15/36 attr: add counted string version of git_check_attr()
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
Often a potential caller has <path, pathlen> pair that represents the path it wants to ask attributes for; when path[pathlen] is not NUL, the caller has to xmemdupz() only to call git_check_attr().
Add git_check_attr_counted() that takes such a counted string instead of "const char *path".
--- attr.c | 23 ++++++++++++++--------- attr.h | 1 + 2 files changed, 15 insertions(+), 9 deletions(-)
attr.c +14 -9
@@ -734,20 +734,19 @@ static int attr_check_is_dynamic(const struct git_attr_check *check)* check_all_attr. If num is non-zero, only attributes in check[] are * collected. Otherwise all attributes are collected. */ -static void collect_some_attrs (const char *path,int num, +static void collect_some_attrs (const char *path,int pathlen,int num,struct git_attr_check_elem *check) {struct attr_stack *stk; -int i, pathlen, rem, dirlen; +int i, rem, dirlen;const char *cp, *last_slash =NULL ;int basename_offset; -for (cp = path; *cp; cp++) { +for (cp = path; cp < path + pathlen; cp++) {if (*cp =='/' && cp[1 ]) last_slash = cp; } - pathlen = cp - path;if (last_slash) { basename_offset = last_slash +1 - path; dirlen = last_slash - path;
@@ -778,12 +777,12 @@ static void collect_some_attrs(const char *path, int num, rem =fill (path, pathlen, basename_offset, stk, rem); } -static int git_check_attrs (const char *path,int num, +static int git_check_attrs (const char *path,int pathlen,int num,struct git_attr_check_elem *check) {int i; -collect_some_attrs (path, num, check); +collect_some_attrs (path, pathlen, num, check);for (i =0 ; i < num; i++) {const char *value = check_all_attr[check[i].attr->attr_nr].value;
@@ -800,7 +799,7 @@ void git_all_attrs(const char *path, struct git_attr_check *check)int i;git_attr_check_clear (check); -collect_some_attrs (path,0 ,NULL ); +collect_some_attrs (path,strlen (path),0 ,NULL );for (i =0 ; i < attr_nr; i++) {const char *name = check_all_attr[i].attr->name;
@@ -825,10 +824,16 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist use_index = istate; } -int git_check_attr (const char *path,struct git_attr_check *check) +int git_check_attr_counted (const char *path,int pathlen, +struct git_attr_check *check) { check->finalized =1 ; -return git_check_attrs (path, check->check_nr, check->check); +return git_check_attrs (path, pathlen, check->check_nr, check->check); +} + +int git_check_attr (const char *path,struct git_attr_check *check) +{ +return git_check_attr_counted (path,strlen (path), check); }struct git_attr_check *git_attr_check_initl (const char *one, ...)
attr.h +1 -0
@@ -38,6 +38,7 @@ struct git_attr_check {extern struct git_attr_check *git_attr_check_initl (const char *, ...);extern int git_check_attr (const char *path,struct git_attr_check *); +extern int git_check_attr_counted (const char *,int ,struct git_attr_check *);extern struct git_attr_check *git_attr_check_alloc (void );extern struct git_attr_check_elem *git_attr_check_append (struct git_attr_check *,const struct git_attr *);
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 16/36 attr: add counted string version of git_attr()
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
Often a potential caller has <name, namelen> pair that represents the name it wants to create an attribute out of.
When name[namelen] is not NUL, the caller has to xmemdupz() only to call git_attr().
Add git_attr_counted() that takes such a counted string instead of "const char *name".
--- attr.c | 8 ++++---- attr.h | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-)
attr.c +4 -4
@@ -78,7 +78,7 @@ static int invalid_attr_name(const char *name, int namelen)return 0 ; } -static struct git_attr *git_attr_internal (const char *name,int len) +struct git_attr *git_attr_counted (const char *name,size_t len) {unsigned hval =hash_name (name, len);unsigned pos = hval %HASHSIZE ;
@@ -109,7 +109,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)struct git_attr *git_attr (const char *name) { -return git_attr_internal (name,strlen (name)); +return git_attr_counted (name,strlen (name)); }/* What does a matched pattern decide? */
@@ -199,7 +199,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,else { e->setto =xmemdupz (equals +1 , ep - equals -1 ); } - e->attr =git_attr_internal (cp, len); + e->attr =git_attr_counted (cp, len); }return ep +strspn (ep, blank); }
@@ -254,7 +254,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,sizeof (struct attr_state ) * num_attr + (is_macro ? 0 : namelen + 1));if (is_macro) { - res->u.attr =git_attr_internal (name, namelen); + res->u.attr =git_attr_counted (name, namelen); res->u.attr->maybe_macro =1 ; }else {char *p = (char *)&(res->state[num_attr]);
attr.h +4 -1
@@ -8,7 +8,10 @@ struct git_attr;* Given a string, return the gitattribute object that * corresponds to it. */ -struct git_attr *git_attr (const char *); +extern struct git_attr *git_attr (const char *); + +/* The same, but with counted string */ +extern struct git_attr *git_attr_counted (const char *,size_t );/* Internal use */ extern const char git_attr__true[];
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 17/36 attr: expose validity check for attribute names
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
Export attr_name_valid() function, and a helper function that returns the message to be given when a given <name, len> pair is not a good name for an attribute.
We could later update the message to exactly spell out what the rules for a good attribute name are, etc.
--- attr.c | 39 +++++++++++++++++++++++++-------------- attr.h | 3 +++ 2 files changed, 28 insertions(+), 14 deletions(-)
attr.c +25 -14
@@ -59,23 +59,38 @@ static unsigned hash_name(const char *name, int namelen)return val; } -static int invalid_attr_name (const char *name,int namelen) +int attr_name_valid (const char *name,size_t namelen) {/* * Attribute name cannot begin with '-' and must consist of * characters from [-A-Za-z0-9_.]. */ if (namelen <=0 || *name =='-' ) -return -1 ; +return 0 ;while (namelen--) {char ch = *name++;if (! (ch =='-' || ch =='.' || ch =='_' || ('0' <= ch && ch <='9' ) || ('a' <= ch && ch <='z' ) || ('A' <= ch && ch <='Z' )) ) -return -1 ; +return 0 ; } -return 0 ; +return 1 ; +} + +void invalid_attr_name_message (struct strbuf *err,const char *name,int len) +{ +strbuf_addf (err,_ ("%.*s is not a valid attribute name" ), + len, name); +} + +static void report_invalid_attr (const char *name,size_t len, +const char *src,int lineno) +{ +struct strbuf err =STRBUF_INIT ; +invalid_attr_name_message (&err, name, len); +fprintf (stderr,"%s: %s:%d\n" , err.buf, src, lineno); +strbuf_release (&err); }struct git_attr *git_attr_counted (const char *name,size_t len)
@@ -90,7 +105,7 @@ struct git_attr *git_attr_counted(const char *name, size_t len)return a; } -if (invalid_attr_name (name, len)) +if (!attr_name_valid (name, len))return NULL ;FLEX_ALLOC_MEM (a, name, name, len);
@@ -176,17 +191,15 @@ static const char *parse_attr(const char *src, int lineno, const char *cp, cp++; len--; } -if (invalid_attr_name (cp, len)) { -fprintf (stderr, -"%.*s is not a valid attribute name: %s:%d\n" , - len, cp, src, lineno); +if (!attr_name_valid (cp, len)) { +report_invalid_attr (cp, len, src, lineno);return NULL ; } }else {/* * As this function is always called twice, once with * e == NULL in the first pass and then e != NULL in -* the second pass, no need for invalid_attr_name() +* the second pass, no need for attr_name_valid() * check here. */ if (*cp =='-' || *cp =='!' ) {
@@ -229,10 +242,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src, name +=strlen (ATTRIBUTE_MACRO_PREFIX ); name +=strspn (name, blank); namelen =strcspn (name, blank); -if (invalid_attr_name (name, namelen)) { -fprintf (stderr, -"%.*s is not a valid attribute name: %s:%d\n" , - namelen, name, src, lineno); +if (!attr_name_valid (name, namelen)) { +report_invalid_attr (name, namelen, src, lineno); goto fail_return; } }
attr.h +3 -0
@@ -13,6 +13,9 @@ extern struct git_attr *git_attr(const char *);/* The same, but with counted string */ extern struct git_attr *git_attr_counted (const char *,size_t ); +extern int attr_name_valid (const char *name,size_t namelen); +extern void invalid_attr_name_message (struct strbuf *,const char *,int ); +/* Internal use */ extern const char git_attr__true[];extern const char git_attr__false[];
Signature · 1 line
2.10.1.508.g6572022
Re: [PATCH 17/36] attr: expose validity check for attribute names
Quoted 12 lines · Stefan Beller
From: Junio C Hamano <[email protected]>
Export
attr_name_valid()function, and a helper function that returns the message to be given when a given <name, len> pair is not a good name for an attribute.We could later update the message to exactly spell out what the rules for a good attribute name are, etc.
Signed-off-by Junio C Hamano <[email protected]>Signed-off-by Stefan Beller <[email protected]>---
[snip]
attr.h extern struct git_attr *git_attr(const char *); · line 18
Quoted 3 lines
attr.h +3
-0+extern int attr_name_valid (const char *name,size_t namelen); +extern void invalid_attr_name_message (struct strbuf *,const char *,int ); +
The symbol 'attr_name_valid()' is not used outside of attr.c, even by the end of this series. Do you expect this function to be used in any future series? (The export is deliberate and it certainly seems like it should be part of the public interface, but ...)
In contrast, the 'invalid_attr_name_message()' function is called from code in pathspec.c, which relies on 'git_attr_counted()' to call 'attr_name_valid()' internally to check for validity. :-D
ATB,
Ramsay Jones
Re: [PATCH 17/36] attr: expose validity check for attribute names
Quoted 27 lines · Ramsay Jones
Quoted 12 lines · Stefan Beller
From: Junio C Hamano <[email protected]>
Export
attr_name_valid()function, and a helper function that returns the message to be given when a given <name, len> pair is not a good name for an attribute.We could later update the message to exactly spell out what the rules for a good attribute name are, etc.
Signed-off-by Junio C Hamano <[email protected]>Signed-off-by Stefan Beller <[email protected]>---
[snip]
attr.h extern struct git_attr *git_attr(const char *); · line 18
Quoted 3 lines
attr.h +3
-0+extern int attr_name_valid (const char *name,size_t namelen); +extern void invalid_attr_name_message (struct strbuf *,const char *,int ); +The symbol '
attr_name_valid()' is not used outside ofattr.c, even by the end of this series. Do you expect this function to be used in any future series? (The export is deliberate and it certainly seems like it should be part of the public interface, but ...)
Quoted 3 lines
In contrast, the '
invalid_attr_name_message()' function is called from code inpathspec.c, which relies on 'git_attr_counted()' to call 'attr_name_valid()' internally to check for validity. :-D
Yeah, I am taking over Junios patches and do not quite implement what Junio thought I would. ;) So I guess it is a communication mismatch.
git_attr_counted is a wrapper around attr_name_valid in the way that it either returns NULL when the attr name is invalid or it does extra work and returns a pointer to an attr.
So I think for API completeness we'd want to keep attr_name_valid around, as otherwise the API looks strange. But that doesn't seem like a compelling reason, so I'll drop it from the header file and make it static.
Thanks,
Stefan
Re: [PATCH 17/36] attr: expose validity check for attribute names
Quoted 20 lines · Stefan Beller
Quoted 18 lines · Ramsay Jones
Quoted 12 lines · Stefan Beller
From: Junio C Hamano <[email protected]>
Export
attr_name_valid()function, and a helper function that returns the message to be given when a given <name, len> pair is not a good name for an attribute.We could later update the message to exactly spell out what the rules for a good attribute name are, etc.
Signed-off-by Junio C Hamano <[email protected]>Signed-off-by Stefan Beller <[email protected]>---
[snip]
Quoted 9 lines
Quoted 9 lines
attr.h extern struct git_attr *git_attr(const char *); · line 18
Quoted 3 lines
attr.h +3
-0+extern int attr_name_valid (const char *name,size_t namelen); +extern void invalid_attr_name_message (struct strbuf *,const char *,int ); +The symbol '
attr_name_valid()' is not used outside ofattr.c, even by the end of this series. Do you expect this function to be used in any future series? (The export is deliberate and it certainly seems like it should be part of the public interface, but ...)
refactoring this series again, I will make use of attr_name_valid outside of attr.c, so I'll keep this patch.
PATCH 1/1 attr: expose error reporting function for invalid attribute names
In reply to Ramsay JonesFrom: Junio C Hamano <[email protected]>
Export invalid_attr_name_message() function that returns the message to be given when a given <name, len> pair is not a good name for an attribute.
We could later update the message to exactly spell out what the rules for a good attribute name are, etc.
We do not need to export the validity check 'attr_name_valid()' itself as we will learn about the validity indirectly in a later patch via calling 'git_attr_counted()'.
---
Ramsay,
I intend to replace the previous
[PATCH 17/36] attr: expose validity check for attribute names
by this one in a reroll.
Thanks,
Stefan
attr.c | 39 +++++++++++++++++++++++++-------------- attr.h | 2 ++ 2 files changed, 27 insertions(+), 14 deletions(-)
attr.c +25 -14
@@ -59,23 +59,38 @@ static unsigned hash_name(const char *name, int namelen)return val; } -static int invalid_attr_name (const char *name,int namelen) +static int attr_name_valid (const char *name,size_t namelen) {/* * Attribute name cannot begin with '-' and must consist of * characters from [-A-Za-z0-9_.]. */ if (namelen <=0 || *name =='-' ) -return -1 ; +return 0 ;while (namelen--) {char ch = *name++;if (! (ch =='-' || ch =='.' || ch =='_' || ('0' <= ch && ch <='9' ) || ('a' <= ch && ch <='z' ) || ('A' <= ch && ch <='Z' )) ) -return -1 ; +return 0 ; } -return 0 ; +return 1 ; +} + +void invalid_attr_name_message (struct strbuf *err,const char *name,int len) +{ +strbuf_addf (err,_ ("%.*s is not a valid attribute name" ), + len, name); +} + +static void report_invalid_attr (const char *name,size_t len, +const char *src,int lineno) +{ +struct strbuf err =STRBUF_INIT ; +invalid_attr_name_message (&err, name, len); +fprintf (stderr,"%s: %s:%d\n" , err.buf, src, lineno); +strbuf_release (&err); }struct git_attr *git_attr_counted (const char *name,size_t len)
@@ -90,7 +105,7 @@ struct git_attr *git_attr_counted(const char *name, size_t len)return a; } -if (invalid_attr_name (name, len)) +if (!attr_name_valid (name, len))return NULL ;FLEX_ALLOC_MEM (a, name, name, len);
@@ -176,17 +191,15 @@ static const char *parse_attr(const char *src, int lineno, const char *cp, cp++; len--; } -if (invalid_attr_name (cp, len)) { -fprintf (stderr, -"%.*s is not a valid attribute name: %s:%d\n" , - len, cp, src, lineno); +if (!attr_name_valid (cp, len)) { +report_invalid_attr (cp, len, src, lineno);return NULL ; } }else {/* * As this function is always called twice, once with * e == NULL in the first pass and then e != NULL in -* the second pass, no need for invalid_attr_name() +* the second pass, no need for attr_name_valid() * check here. */ if (*cp =='-' || *cp =='!' ) {
@@ -229,10 +242,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src, name +=strlen (ATTRIBUTE_MACRO_PREFIX ); name +=strspn (name, blank); namelen =strcspn (name, blank); -if (invalid_attr_name (name, namelen)) { -fprintf (stderr, -"%.*s is not a valid attribute name: %s:%d\n" , - namelen, name, src, lineno); +if (!attr_name_valid (name, namelen)) { +report_invalid_attr (name, namelen, src, lineno); goto fail_return; } }
attr.h +2 -0
@@ -13,6 +13,8 @@ extern struct git_attr *git_attr(const char *);/* The same, but with counted string */ extern struct git_attr *git_attr_counted (const char *,size_t ); +extern void invalid_attr_name_message (struct strbuf *,const char *,int ); +/* Internal use */ extern const char git_attr__true[];extern const char git_attr__false[];
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 18/36 attr: support quoting pathname patterns in C style
In reply to Stefan BellerFrom: Nguyễn Thái Ngọc Duy <[email protected]>
Full pattern must be quoted. So 'pat"t"ern attr' will give exactly 'pat"t"ern', not 'pattern'. Also clarify that leading whitespaces are not part of the pattern and document comment syntax.
--- Documentation/gitattributes.txt | 8 +++++--- attr.c | 15 +++++++++++++-- t/t0003-attributes.sh | 26 ++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 5 deletions(-)
Documentation/gitattributes.txt +5 -3
@@ -21,9 +21,11 @@ Each line in `gitattributes` file is of form: pattern attr1 attr2 ... That is, a pattern followed by an attributes list, -separated by whitespaces. When the pattern matches the -path in question, the attributes listed on the line are given to -the path. +separated by whitespaces. Leading and trailing whitespaces are +ignored. Lines that begin with '#' are ignored. Patterns +that begin with a double quote are quoted in C style. +When the pattern matches the path in question, the attributes +listed on the line are given to the path. Each attribute can be in one of these states for a given path:
attr.c +13 -2
@@ -13,6 +13,7 @@#include "attr.h" #include "dir.h" #include "utf8.h" +#include "quote.h" const char git_attr__true[] ="(builtin)true" ;const char git_attr__false[] ="\0(builtin)false" ;
@@ -225,12 +226,21 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,const char *cp, *name, *states;struct match_attr *res =NULL ;int is_macro; +struct strbuf pattern =STRBUF_INIT ; cp = line +strspn (line, blank);if (!*cp || *cp =='#' )return NULL ; name = cp; - namelen =strcspn (name, blank); + +if (*cp =='"' && !unquote_c_style (&pattern, name, &states)) { + name = pattern.buf; + namelen = pattern.len; + }else { + namelen =strcspn (name, blank); + states = name + namelen; + } +if (strlen (ATTRIBUTE_MACRO_PREFIX ) < namelen &&starts_with (name,ATTRIBUTE_MACRO_PREFIX )) {if (!macro_ok) {
@@ -250,7 +260,6 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,else is_macro =0 ; - states = name + namelen; states +=strspn (states, blank);/* First pass to count the attr_states */
@@ -293,9 +302,11 @@ static struct match_attr *parse_attr_line(const char *line, const char *src, cannot_trust_maybe_real =1 ; } +strbuf_release (&pattern);return res; fail_return: +strbuf_release (&pattern);free (res);return NULL ; }
t/t0003-attributes.sh +26 -0
@@ -13,10 +13,31 @@ attr_check () { test_line_count = 0 err } +attr_check_quote () { + + path="$1" + quoted_path="$2" + expect="$3" + + git check-attr test -- "$path" >actual && + echo "\"$quoted_path\": test: $expect" >expect && + test_cmp expect actual + +} + +test_expect_success 'open-quoted pathname' ' + echo "\"a test=a" >.gitattributes && + test_must_fail attr_check a a +' + + test_expect_success 'setup' ' mkdir -p a/b/d a/c b && ( echo "[attr]notest !test" + echo "\" d \" test=d" + echo " e test=e" + echo " e\" test=e" echo "f test=f" echo "a/i test=a/i" echo "onoff test -test"
@@ -69,6 +90,11 @@ test_expect_success 'command line checks' ' ' test_expect_success 'attribute test' ' + + attr_check " d " d && + attr_check e e && + attr_check_quote e\" e\\\" e && + attr_check f f && attr_check a/f f && attr_check a/c/f f &&
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 19/36 attr.c: add push_stack() helper
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
There are too many repetitious "I have this new attr_stack element; push it at the top of the stack" sequence. The new helper function push_stack() gives us a way to express what is going on at these places, and as a side effect, halves the number of times we mention the attr_stack global variable.
--- attr.c | 71 +++++++++++++++++++++++++++++++----------------------------------- 1 file changed, 33 insertions(+), 38 deletions(-)
attr.c +33 -38
@@ -521,6 +521,18 @@ static int git_attr_system(void) static GIT_PATH_FUNC(git_path_info_attributes, INFOATTRIBUTES_FILE) +static void push_stack(struct attr_stack **attr_stack_p, +struct attr_stack *elem,char *origin,size_t originlen) +{ +if (elem) { + elem->origin = origin; +if (origin) + elem->originlen = originlen; + elem->prev = *attr_stack_p; + *attr_stack_p = elem; + } +} +static void bootstrap_attr_stack (void ) {struct attr_stack *elem;
@@ -528,52 +540,35 @@ static void bootstrap_attr_stack(void)if (attr_stack)return ; - elem =read_attr_from_array (builtin_attr); - elem->origin =NULL ; - elem->prev = attr_stack; - attr_stack = elem; - -if (git_attr_system ()) { - elem =read_attr_from_file (git_etc_gitattributes (),1 ); -if (elem) { - elem->origin =NULL ; - elem->prev = attr_stack; - attr_stack = elem; - } - } +push_stack (&attr_stack,read_attr_from_array (builtin_attr),NULL ,0 ); + +if (git_attr_system ()) +push_stack (&attr_stack, +read_attr_from_file (git_etc_gitattributes (),1 ), +NULL ,0 );if (!git_attributes_file) git_attributes_file =xdg_config_home ("attributes" ); -if (git_attributes_file) { - elem =read_attr_from_file (git_attributes_file,1 ); -if (elem) { - elem->origin =NULL ; - elem->prev = attr_stack; - attr_stack = elem; - } - } +if (git_attributes_file) +push_stack (&attr_stack, +read_attr_from_file (git_attributes_file,1 ), +NULL ,0 );if (!is_bare_repository () || direction ==GIT_ATTR_INDEX ) { elem =read_attr (GITATTRIBUTES_FILE ,1 ); - elem->origin =xstrdup ("" ); - elem->originlen =0 ; - elem->prev = attr_stack; - attr_stack = elem; +push_stack (&attr_stack, elem,xstrdup ("" ),0 );debug_push (elem); } elem =read_attr_from_file (git_path_info_attributes (),1 );if (!elem) elem =xcalloc (1 ,sizeof (*elem)); - elem->origin =NULL ; - elem->prev = attr_stack; - attr_stack = elem; +push_stack (&attr_stack, elem,NULL ,0 ); }static void prepare_attr_stack (const char *path,int dirlen) {struct attr_stack *elem, *info; -int len;const char *cp;/*
@@ -633,20 +628,21 @@ static void prepare_attr_stack(const char *path, int dirlen)assert (attr_stack->origin);while (1 ) { - len =strlen (attr_stack->origin); +size_t len =strlen (attr_stack->origin); +char *origin; +if (dirlen <= len)break ; cp =memchr (path + len +1 ,'/' , dirlen - len -1 );if (!cp) cp = path + dirlen; -strbuf_add (&pathbuf, path, cp - path); -strbuf_addch (&pathbuf,'/' ); -strbuf_addstr (&pathbuf,GITATTRIBUTES_FILE ); +strbuf_addf (&pathbuf, +"%.*s/%s" , (int )(cp - path), path, +GITATTRIBUTES_FILE ); elem =read_attr (pathbuf.buf,0 );strbuf_setlen (&pathbuf, cp - path); - elem->origin =strbuf_detach (&pathbuf, &elem->originlen); - elem->prev = attr_stack; - attr_stack = elem; + origin =strbuf_detach (&pathbuf, &len); +push_stack (&attr_stack, elem, origin, len);debug_push (elem); }
@@ -656,8 +652,7 @@ static void prepare_attr_stack(const char *path, int dirlen)/* * Finally push the "info" one at the top of the stack. */ - info->prev = attr_stack; - attr_stack = info; +push_stack (&attr_stack, info,NULL ,0 ); } static int path_matches(const char *pathname, int pathlen,
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 20/36 attr.c: pass struct git_attr_check down the callchain
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
The callchain that starts from git_check_attrs() down to collect_some_attrs() used to take an array of git_attr_check_elem as their parameters. Pass the enclosing git_attr_check instance instead, so that they will have access to new fields we will add to the data structure.
--- attr.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-)
attr.c +24 -12
@@ -751,14 +751,25 @@ static int attr_check_is_dynamic(const struct git_attr_check *check)* check_all_attr. If num is non-zero, only attributes in check[] are * collected. Otherwise all attributes are collected. */ -static void collect_some_attrs (const char *path,int pathlen,int num, -struct git_attr_check_elem *check) +static void collect_some_attrs (const char *path,int pathlen, +struct git_attr_check *check) {struct attr_stack *stk;int i, rem, dirlen;const char *cp, *last_slash =NULL ;int basename_offset; +int num; +struct git_attr_check_elem *celem; + +if (!check) { +/* Yuck - ugly git_all_attrs() hack! */ + celem =NULL ; + num =0 ; + }else { + celem = check->check; + num = check->check_nr; + }for (cp = path; cp < path + pathlen; cp++) {if (*cp =='/' && cp[1 ])
@@ -778,9 +789,9 @@ static void collect_some_attrs(const char *path, int pathlen, int num,if (num && !cannot_trust_maybe_real) { rem =0 ;for (i =0 ; i < num; i++) { -if (!check[i].attr->maybe_real) { +if (!celem[i].attr->maybe_real) {struct git_attr_check_elem *c; - c = check_all_attr + check[i].attr->attr_nr; + c = check_all_attr + celem[i].attr->attr_nr; c->value =ATTR__UNSET ; rem++; }
@@ -794,18 +805,19 @@ static void collect_some_attrs(const char *path, int pathlen, int num, rem =fill (path, pathlen, basename_offset, stk, rem); } -static int git_check_attrs (const char *path,int pathlen,int num, -struct git_attr_check_elem *check) +static int git_check_attrs (const char *path,int pathlen, +struct git_attr_check *check) {int i; +struct git_attr_check_elem *celem = check->check; -collect_some_attrs (path, pathlen, num, check); +collect_some_attrs (path, pathlen, check); -for (i =0 ; i < num; i++) { -const char *value = check_all_attr[check[i].attr->attr_nr].value; +for (i =0 ; i < check->check_nr; i++) { +const char *value = check_all_attr[celem[i].attr->attr_nr].value;if (value ==ATTR__UNKNOWN ) value =ATTR__UNSET ; - check[i].value = value; + celem[i].value = value; }return 0 ;
@@ -816,7 +828,7 @@ void git_all_attrs(const char *path, struct git_attr_check *check)int i;git_attr_check_clear (check); -collect_some_attrs (path,strlen (path),0 ,NULL ); +collect_some_attrs (path,strlen (path),NULL );for (i =0 ; i < attr_nr; i++) {const char *name = check_all_attr[i].attr->name;
@@ -845,7 +857,7 @@ int git_check_attr_counted(const char *path, int pathlen, struct git_attr_check *check) { check->finalized =1 ; -return git_check_attrs (path, pathlen, check->check_nr, check->check); +return git_check_attrs (path, pathlen, check); }int git_check_attr (const char *path,struct git_attr_check *check)
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 21/36 attr.c: rename a local variable check
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
Throughout this series, we are trying to use "check" to name an instance of "git_attr_check" structure; let's rename a "check" that refers to an array whose elements are git_attr_check_elem to avoid confusion.
--- attr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
attr.c +2 -2
@@ -682,12 +682,12 @@ static int macroexpand_one(int attr_nr, int rem);static int fill_one (const char *what,struct match_attr *a,int rem) { -struct git_attr_check_elem *check = check_all_attr; +struct git_attr_check_elem *celem = check_all_attr;int i;for (i = a->num_attr -1 ;0 < rem &&0 <= i; i--) {struct git_attr *attr = a->state[i].attr; -const char **n = &(check[attr->attr_nr].value); +const char **n = &(celem[attr->attr_nr].value);const char *v = a->state[i].setto;if (*n ==ATTR__UNKNOWN ) {
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 22/36 attr.c: correct ugly hack for git_all_attrs()
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
The collect_some_attrs() function has an ugly hack since
06a604e6 (attr: avoid heavy work when we know the specified attr is not defined, 2014-12-28) added an optimization that relies on the fact that the caller knows what attributes it is interested in, so that we can leave once we know the final answer for all the attributes the caller asked.
git_all_attrs() that asks "what attributes are on this path?" however does not know what attributes it is interested in, other than the vague "we are interested in all of them", which is not a very useful thing to say. As a way to disable this optimization for this caller, the said commit added a code to skip it when the caller passes a NULL for the check structure.
However, it skipped the optimization not when check is NULL, but when the number of attributes being checked is 0, which is unnecessarily pessimistic.
--- attr.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)
attr.c +8 -16
@@ -748,8 +748,8 @@ static int attr_check_is_dynamic(const struct git_attr_check *check)/* * Collect attributes for path into the array pointed to by -* check_all_attr. If num is non-zero, only attributes in check[] are -* collected. Otherwise all attributes are collected. +* check_all_attr. If check is not NULL, only attributes in +* check[] are collected. Otherwise all attributes are collected. */ static void collect_some_attrs (const char *path,int pathlen,struct git_attr_check *check)
@@ -759,17 +759,6 @@ static void collect_some_attrs(const char *path, int pathlen,int i, rem, dirlen;const char *cp, *last_slash =NULL ;int basename_offset; -int num; -struct git_attr_check_elem *celem; - -if (!check) { -/* Yuck - ugly git_all_attrs() hack! */ - celem =NULL ; - num =0 ; - }else { - celem = check->check; - num = check->check_nr; - }for (cp = path; cp < path + pathlen; cp++) {if (*cp =='/' && cp[1 ])
@@ -786,9 +775,12 @@ static void collect_some_attrs(const char *path, int pathlen,prepare_attr_stack (path, dirlen);for (i =0 ; i < attr_nr; i++) check_all_attr[i].value =ATTR__UNKNOWN ; -if (num && !cannot_trust_maybe_real) { + +if (check && !cannot_trust_maybe_real) { +struct git_attr_check_elem *celem = check->check; + rem =0 ; -for (i =0 ; i < num; i++) { +for (i =0 ; i < check->check_nr; i++) {if (!celem[i].attr->maybe_real) {struct git_attr_check_elem *c; c = check_all_attr + celem[i].attr->attr_nr;
@@ -796,7 +788,7 @@ static void collect_some_attrs(const char *path, int pathlen, rem++; } } -if (rem == num) +if (rem == check->check_nr)return ; }
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 23/36 attr.c: introduce empty_attr_check_elems()
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
One codepath needs to just empty the git_attr_check_elem array in the git_attr_check structure, without releasing the entire resource. Introduce a helper to do so and rewrite git_attr_check_clear() using it.
--- attr.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
attr.c +9 -2
@@ -746,6 +746,14 @@ static int attr_check_is_dynamic(const struct git_attr_check *check)return (void *)(check->check) != (void *)(check +1 ); } +static void empty_attr_check_elems (struct git_attr_check *check) +{ +if (!attr_check_is_dynamic (check)) +die ("BUG: emptying a statically initialized git_attr_check" ); + check->check_nr =0 ; + check->finalized =0 ; +} +/* * Collect attributes for path into the array pointed to by * check_all_attr. If check is not NULL, only attributes in
@@ -912,12 +920,11 @@ struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *check,void git_attr_check_clear (struct git_attr_check *check) { +empty_attr_check_elems (check);if (!attr_check_is_dynamic (check))die ("BUG: clearing a statically initialized git_attr_check" );free (check->check); - check->check_nr =0 ; check->check_alloc =0 ; - check->finalized =0 ; }void git_attr_check_free (struct git_attr_check *check)
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 24/36 attr.c: always pass check[] to collect_some_attrs()
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
This function used to be called with check=NULL to signal it to collect all attributes in the global check_all_attr[] array.
Because the longer term plan is to allocate check_all_attr[] and attr_stack data structures per git_attr_check instance (i.e. "check" here) to make the attr subsystem thread-safe, it is unacceptable.
Pass "Are we grabbing all attributes defined in the system?" bit as a separate argument and pass it from the callers.
--- attr.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-)
attr.c +19 -18
@@ -756,11 +756,12 @@ static void empty_attr_check_elems(struct git_attr_check *check)/* * Collect attributes for path into the array pointed to by -* check_all_attr. If check is not NULL, only attributes in -* check[] are collected. Otherwise all attributes are collected. +* check_all_attr. If collect_all is zero, only attributes in +* check[] are collected. Otherwise, check[] is cleared and +* any and all attributes that are visible are collected in it. */ static void collect_some_attrs (const char *path,int pathlen, -struct git_attr_check *check) +struct git_attr_check *check,int collect_all) {struct attr_stack *stk;
@@ -781,10 +782,11 @@ static void collect_some_attrs(const char *path, int pathlen, }prepare_attr_stack (path, dirlen); +for (i =0 ; i < attr_nr; i++) check_all_attr[i].value =ATTR__UNKNOWN ; -if (check && !cannot_trust_maybe_real) { +if (!collect_all && !cannot_trust_maybe_real) {struct git_attr_check_elem *celem = check->check; rem =0 ;
@@ -803,6 +805,17 @@ static void collect_some_attrs(const char *path, int pathlen, rem = attr_nr;for (stk = attr_stack;0 < rem && stk; stk = stk->prev) rem =fill (path, pathlen, basename_offset, stk, rem); + +if (collect_all) { +empty_attr_check_elems (check); +for (i =0 ; i < attr_nr; i++) { +const struct git_attr *attr = check_all_attr[i].attr; +const char *value = check_all_attr[i].value; +if (value ==ATTR__UNSET || value ==ATTR__UNKNOWN ) +continue ; +git_attr_check_append (check, attr)->value = value; + } + } } static int git_check_attrs(const char *path, int pathlen,
@@ -811,7 +824,7 @@ static int git_check_attrs(const char *path, int pathlen,int i;struct git_attr_check_elem *celem = check->check; -collect_some_attrs (path, pathlen, check); +collect_some_attrs (path, pathlen, check,0 );for (i =0 ; i < check->check_nr; i++) {const char *value = check_all_attr[celem[i].attr->attr_nr].value;
@@ -825,19 +838,7 @@ static int git_check_attrs(const char *path, int pathlen,void git_all_attrs (const char *path,struct git_attr_check *check) { -int i; - -git_attr_check_clear (check); -collect_some_attrs (path,strlen (path),NULL ); - -for (i =0 ; i < attr_nr; i++) { -const char *name = check_all_attr[i].attr->name; -const char *value = check_all_attr[i].value; -if (value ==ATTR__UNSET || value ==ATTR__UNKNOWN ) -continue ; -git_attr_check_append (check,git_attr (name)); - check->check[check->check_nr -1 ].value = value; - } +collect_some_attrs (path,strlen (path), check,1 ); }void git_attr_set_direction (enum git_attr_direction new,struct index_state *istate)
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 25/36 attr.c: outline the future plans by heavily commenting
In reply to Stefan BellerFrom: Junio C Hamano <[email protected]>
--- attr.c | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-)
attr.c +39 -1
@@ -30,6 +30,11 @@ static const char git_attr__unknown[] = "(builtin)unknown";#define DEBUG_ATTR 0#endif +/* +* NEEDSWORK: the global dictionary of the interned attributes +* must stay a singleton even after we become thread-ready. +* Access to these must be surrounded with mutex when it happens. +*/ struct git_attr {struct git_attr *next;unsigned h;
@@ -39,10 +44,19 @@ struct git_attr {char name[FLEX_ARRAY ]; };static int attr_nr; +static struct git_attr *(git_attr_hash[HASHSIZE ]); + +/* +* NEEDSWORK: maybe-real, maybe-macro are not property of +* an attribute, as it depends on what .gitattributes are +* read. Once we introduce per git_attr_check attr_stack +* and check_all_attr, the optimization based on them will +* become unnecessary and can go away. So is this variable. +*/ static int cannot_trust_maybe_real; +/* NEEDSWORK: This will become per git_attr_check */ static struct git_attr_check_elem *check_all_attr; -static struct git_attr *(git_attr_hash[HASHSIZE ]);const char *git_attr_name (const struct git_attr *attr) {
@@ -117,6 +131,11 @@ struct git_attr *git_attr_counted(const char *name, size_t len) a->maybe_real =0 ; git_attr_hash[pos] = a; +/* +* NEEDSWORK: per git_attr_check check_all_attr +* will be initialized a lot more lazily, not +* like this, and not here. +*/ REALLOC_ARRAY (check_all_attr, attr_nr); check_all_attr[a->attr_nr].attr = a; check_all_attr[a->attr_nr].value =ATTR__UNKNOWN ;
@@ -329,6 +348,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,* .gitignore file and info/excludes file as a fallback. */ +/* NEEDSWORK: This will become per git_attr_check */ static struct attr_stack { struct attr_stack *prev; char *origin;
@@ -393,6 +413,24 @@ static struct attr_stack *read_attr_from_array(const char **list)return res; } +/* +* NEEDSWORK: these two are tricky. The callers assume there is a +* single, system-wide global state "where we read attributes from?" +* and when the state is flipped by calling git_attr_set_direction(), +* attr_stack is discarded so that subsequent attr_check will lazily +* read from the right place. And they do not know or care who called +* by them uses the attribute subsystem, hence have no knowledge of +* existing git_attr_check instances or future ones that will be +* created). +* +* Probably we need a thread_local that holds these two variables, +* and a list of git_attr_check instances (which need to be maintained +* by hooking into git_attr_check_alloc(), git_attr_check_initl(), and +* git_attr_check_clear(). Then git_attr_set_direction() updates the +* fields in that thread_local for these two variables, iterate over +* all the active git_attr_check instances and discard the attr_stack +* they hold. Yuck, but it sounds doable. +*/ static enum git_attr_direction direction;static struct index_state *use_index;
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 26/36 attr: make git_check_attr_counted static
In reply to Stefan BellerIt's not used outside the attr code, so let's keep it private.
--- attr.c | 4 ++-- attr.h | 1 - 2 files changed, 2 insertions(+), 3 deletions(-)
attr.c +2 -2
@@ -892,8 +892,8 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist use_index = istate; } -int git_check_attr_counted (const char *path,int pathlen, -struct git_attr_check *check) +static int git_check_attr_counted (const char *path,int pathlen, +struct git_attr_check *check) { check->finalized =1 ;return git_check_attrs (path, pathlen, check);
attr.h +0 -1
@@ -44,7 +44,6 @@ struct git_attr_check {extern struct git_attr_check *git_attr_check_initl (const char *, ...);extern int git_check_attr (const char *path,struct git_attr_check *); -extern int git_check_attr_counted (const char *,int ,struct git_attr_check *);extern struct git_attr_check *git_attr_check_alloc (void );extern struct git_attr_check_elem *git_attr_check_append (struct git_attr_check *,const struct git_attr *);
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 27/36 attr: convert to new threadsafe API
In reply to Stefan BellerThis 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(&check, "text", NULL);
/*
* Obtain a pointer to a correctly sized result
* statically allocated on the stack; this macro:
*/
GIT_ATTR_RESULT_INIT_FOR(myresult, 1);
/* Perform the check and act on it: */
git_check_attr(path, check, myresult);
act_on(myresult->value[0]);
/*
* 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.
*/--- Documentation/technical/api-gitattributes.txt | 91 +++++++++------- archive.c | 14 +-- attr.c | 143 ++++++++++++++++++-------- attr.h | 71 ++++++++----- builtin/check-attr.c | 35 ++++--- builtin/pack-objects.c | 16 +-- convert.c | 39 +++---- ll-merge.c | 25 +++-- userdiff.c | 16 +-- ws.c | 17 ++- 10 files changed, 279 insertions(+), 188 deletions(-)
Documentation/technical/api-gitattributes.txt +53 -38
@@ -16,15 +16,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
@@ -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,31 @@ value of the attribute for the path. Querying Specific Attributes ---------------------------- -* Prepare `struct git_attr_check` using git_attr_check_initl() +* 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() + 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 + from different threads at the same time; when check determines + the initialzisation 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. + +* Prepare a `struct git_attr_result` using `GIT_ATTR_RESULT_INIT_FOR()` + for the result for static allocations. When the result size is not known + at compile time, use `git_attr_result_alloc`. The call to initialize + the result is not thread safe, because different threads need their + own thread local result anyway. + +* Call `git_check_attr()` to check the attributes for the path, + the given `git_attr_result` will be filled with the result. + +* Inspect the returned `git_attr_result` structure to see how + each of the attribute in the array is defined for the path. Example -------
@@ -76,28 +90,23 @@ To see how attributes "crlf" and "ident" are set for different paths. we are checking two attributes): ------------ -static struct git_attr_check *check; -static void setup_check(void) -{ - if (check) - return; /* already done */ - check = git_attr_check_initl("crlf", "ident", NULL); -} + static struct git_attr_check *check; + git_attr_check_initl(check, "crlf", "ident", NULL); ------------ . Call `git_check_attr()` with the prepared `struct git_attr_check`: ------------ const char *path; + GIT_ATTR_RESULT_INIT_FOR(result, 2); - setup_check(); - git_check_attr(path, check); + git_check_attr(path, check, result); ------------ -. Act on `.value` member of the result, left in `check->check[]`: +. Act on `result->value[]`: ------------ - const char *value = check->check[0].value; + const char *value = result->value[0]; if (ATTR_TRUE(value)) { The attribute is Set, by listing only the name of the
@@ -123,12 +132,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); } ------------
@@ -138,17 +150,20 @@ Querying All Attributes To get the values of all attributes associated with a file: -* Prepare an empty `git_attr_check` structure by calling - `git_attr_check_alloc()`. +* 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. -* Call `git_all_attrs()`, which populates the `git_attr_check` - with the attributes attached to the path. +* Call `git_all_attrs()`. -* Iterate over the `git_attr_check.check[]` array to examine - the attribute names and values. The name of the attribute - described by a `git_attr_check.check[]` object can be retrieved via - `git_attr_name(check->check[i].attr)`. (Please note that no items +* Iterate over the `git_attr_check.attr[]` array to examine the + attribute names. The name of the attribute described by a + `git_attr_check.attr[]` object can be retrieved via + `git_attr_name(check->attr[i])`. (Please note that no items will be returned for unset attributes, so `ATTR_UNSET()` will return false for all returned `git_array_check` objects.) + The respective value for an attribute can be found in the same + index position in of `git_attr_result`. -* Free the `git_array_check` by calling `git_attr_check_free()`. +* Clear the variables by calling `git_attr_check_clear()` and + `git_attr_result_free()`.
archive.c +8 -6
@@ -107,12 +107,14 @@ static int write_archive_entry(const unsigned char *sha1, const char *base, void *context) {static struct strbuf path =STRBUF_INIT ; +static struct git_attr_check *check; +struct archiver_context *c = context;struct archiver_args *args = c->args;write_archive_entry_fn_t write_entry = c->write_entry; -static struct git_attr_check *check;const char *path_without_prefix;int err; +struct git_attr_result result[2 ]; args->convert =0 ;strbuf_reset (&path);
@@ -124,12 +126,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,strbuf_addch (&path,'/' ); path_without_prefix = path.buf + args->baselen; -if (!check) - check =git_attr_check_initl ("export-ignore" ,"export-subst" ,NULL ); -if (!git_check_attr (path_without_prefix, check)) { -if (ATTR_TRUE (check->check[0 ].value)) +git_attr_check_initl (&check,"export-ignore" ,"export-subst" ,NULL ); + +if (!git_check_attr (path_without_prefix, check, result)) { +if (ATTR_TRUE (result[0 ].value))return 0 ; - args->convert =ATTR_TRUE (check->check[1 ].value); + args->convert =ATTR_TRUE (result[1 ].value); }if (S_ISDIR (mode) ||S_ISGITLINK (mode)) {
attr.c +100 -43
260 lines not on this page · raw message
attr.h +43 -28
97 lines not on this page · raw message
builtin/check-attr.c +19 -16
@@ -24,13 +24,14 @@ static const struct option check_attr_options[] = { OPT_END() }; -static void output_attr (struct git_attr_check *check,const char *file) +static void output_attr (struct git_attr_check *check, +struct git_attr_result *result,const char *file) {int j;int cnt = check->check_nr;for (j =0 ; j < cnt; j++) { -const char *value = check->check[j].value; +const char *value = result[j].value;if (ATTR_TRUE (value)) value ="set" ;
@@ -44,11 +45,11 @@ static void output_attr(struct git_attr_check *check, const char *file)"%s%c" /* attrname */ "%s%c" /* attrvalue */ , file,0 , - git_attr_name(check->check[j].attr), 0, value, 0); + git_attr_name(check->attr[j]), 0, value, 0); }else {quote_c_style (file,NULL , stdout,0 );printf (": %s: %s\n" , -git_attr_name (check->check[j].attr), value); +git_attr_name (check->attr[j]), value); } } }
@@ -59,16 +60,20 @@ static void check_attr(const char *prefix, {char *full_path =prefix_path (prefix, prefix ?strlen (prefix) :0 , file); +struct git_attr_check local_check =GIT_ATTR_CHECK_INIT ; +struct git_attr_result *result =NULL ; +if (check !=NULL ) { -if (git_check_attr (full_path, check)) -die ("git_check_attr died" ); -output_attr (check, file); + result =git_attr_result_alloc (check); +git_check_attr (full_path, check, result); }else { - check =git_attr_check_alloc (); -git_all_attrs (full_path, check); -output_attr (check, file); -git_attr_check_free (check); +git_all_attrs (full_path, &local_check, &result); + check = &local_check; } +output_attr (check, result, file); +git_attr_check_clear (&local_check); + +git_attr_result_free (result);free (full_path); }
@@ -102,7 +107,7 @@ static NORETURN void error_with_usage(const char *msg)int cmd_check_attr (int argc,const char **argv,const char *prefix) { -struct git_attr_check *check; +struct git_attr_check *check =NULL ;int cnt, i, doubledash, filei;if (!is_bare_repository ())
@@ -162,10 +167,8 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)error_with_usage ("No file specified" ); } -if (all_attrs) { - check =NULL ; - }else { - check =git_attr_check_alloc (); +if (!all_attrs) { +git_attr_check_alloc (&check);for (i =0 ; i < cnt; i++) {struct git_attr *a =git_attr (argv[i]);if (!a)
builtin/pack-objects.c +9 -7
@@ -899,14 +899,16 @@ static void write_pack_file(void)static int no_try_delta (const char *path) {static struct git_attr_check *check; +int ret =0 ; +struct git_attr_result result[1 ]; -if (!check) - check =git_attr_check_initl ("delta" ,NULL ); -if (git_check_attr (path, check)) -return 0 ; -if (ATTR_FALSE (check->check[0 ].value)) -return 1 ; -return 0 ; +git_attr_check_initl (&check,"delta" ,NULL ); + +if (!git_check_attr (path, check, result)) { +if (ATTR_FALSE (result[0 ].value)) + ret =1 ; + } +return ret; }/*
convert.c +17 -22
@@ -718,10 +718,8 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,return 1 ; } -static enum crlf_action git_path_check_crlf (struct git_attr_check_elem *check) +static enum crlf_action git_path_check_crlf (const char *value) { -const char *value = check->value; -if (ATTR_TRUE (value))return CRLF_TEXT ;else if (ATTR_FALSE (value))
@@ -735,10 +733,8 @@ static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)return CRLF_UNDEFINED ; } -static enum eol git_path_check_eol (struct git_attr_check_elem *check) +static enum eol git_path_check_eol (const char *value) { -const char *value = check->value; -if (ATTR_UNSET (value)) ;else if (!strcmp (value,"lf" ))
@@ -748,9 +744,8 @@ static enum eol git_path_check_eol(struct git_attr_check_elem *check)return EOL_UNSET ; } -static struct convert_driver *git_path_check_convert (struct git_attr_check_elem *check) +static struct convert_driver *git_path_check_convert (const char *value) { -const char *value = check->value;struct convert_driver *drv;if (ATTR_TRUE (value) ||ATTR_FALSE (value) ||ATTR_UNSET (value))
@@ -761,10 +756,8 @@ static struct convert_driver *git_path_check_convert(struct git_attr_check_elemreturn NULL ; } -static int git_path_check_ident (struct git_attr_check_elem *check) +static int git_path_check_ident (const char *value) { -const char *value = check->value; -return !!ATTR_TRUE (value); }
@@ -778,25 +771,27 @@ struct conv_attrs {static void convert_attrs (struct conv_attrs *ca,const char *path) {static struct git_attr_check *check; +static int init_user_convert_tail; +struct git_attr_result result[5 ]; + +git_attr_check_initl (&check,"crlf" ,"ident" ,"filter" , +"eol" ,"text" ,NULL ); -if (!check) { - check =git_attr_check_initl ("crlf" ,"ident" , -"filter" ,"eol" ,"text" , -NULL ); +if (!init_user_convert_tail) { user_convert_tail = &user_convert;git_config (read_convert_config,NULL ); + init_user_convert_tail =1 ; } -if (!git_check_attr (path, check)) { -struct git_attr_check_elem *ccheck = check->check; - ca->crlf_action =git_path_check_crlf (ccheck +4 ); +if (!git_check_attr (path, check, result)) { + ca->crlf_action =git_path_check_crlf (result[4 ].value);if (ca->crlf_action ==CRLF_UNDEFINED ) - ca->crlf_action =git_path_check_crlf (ccheck +0 ); + ca->crlf_action =git_path_check_crlf (result[0 ].value); ca->attr_action = ca->crlf_action; - ca->ident =git_path_check_ident (ccheck +1 ); - ca->drv =git_path_check_convert (ccheck +2 ); + ca->ident =git_path_check_ident (result[1 ].value); + ca->drv =git_path_check_convert (result[2 ].value);if (ca->crlf_action !=CRLF_BINARY ) { -enum eol eol_attr =git_path_check_eol (ccheck +3 ); +enum eol eol_attr =git_path_check_eol (result[3 ].value);if (ca->crlf_action ==CRLF_AUTO && eol_attr ==EOL_LF ) ca->crlf_action =CRLF_AUTO_INPUT ;else if (ca->crlf_action ==CRLF_AUTO && eol_attr ==EOL_CRLF )
ll-merge.c +14 -11
@@ -353,12 +353,14 @@ int ll_merge(mmbuffer_t *result_buf, mmfile_t *theirs, const char *their_label, const struct ll_merge_options *opts) { - static struct git_attr_check *check; static const struct ll_merge_options default_opts;const char *ll_driver_name =NULL ;int marker_size =DEFAULT_CONFLICT_MARKER_SIZE ;const struct ll_merge_driver *driver; +static struct git_attr_check *check; +struct git_attr_result result[2 ]; +if (!opts) opts = &default_opts;
@@ -368,13 +370,12 @@ int ll_merge(mmbuffer_t *result_buf,normalize_file (theirs, path); } -if (!check) - check =git_attr_check_initl ("merge" ,"conflict-marker-size" ,NULL ); +git_attr_check_initl (&check,"merge" ,"conflict-marker-size" ,NULL ); -if (!git_check_attr (path, check)) { - ll_driver_name = check->check[0 ].value; -if (check->check[1 ].value) { - marker_size =atoi (check->check[1 ].value); +if (!git_check_attr (path, check, result)) { + ll_driver_name = result[0 ].value; +if (result[1 ].value) { + marker_size =atoi (result[1 ].value);if (marker_size <=0 ) marker_size =DEFAULT_CONFLICT_MARKER_SIZE ; }
@@ -395,13 +396,15 @@ int ll_merge_marker_size(const char *path) {static struct git_attr_check *check;int marker_size =DEFAULT_CONFLICT_MARKER_SIZE ; +struct git_attr_result result[1 ]; -if (!check) - check =git_attr_check_initl ("conflict-marker-size" ,NULL ); -if (!git_check_attr (path, check) && check->check[0 ].value) { - marker_size =atoi (check->check[0 ].value); +git_attr_check_initl (&check,"conflict-marker-size" ,NULL ); + +if (!git_check_attr (path, check, result) && !ATTR_UNSET (result[0 ].value)) { + marker_size =atoi (result[0 ].value);if (marker_size <=0 ) marker_size =DEFAULT_CONFLICT_MARKER_SIZE ; } +return marker_size; }
userdiff.c +9 -7
@@ -263,21 +263,23 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {struct userdiff_driver *userdiff_find_by_path (const char *path) {static struct git_attr_check *check; +struct git_attr_result result[1 ]; -if (!check) - check =git_attr_check_initl ("diff" ,NULL );if (!path)return NULL ; -if (git_check_attr (path, check)) + +git_attr_check_initl (&check,"diff" ,NULL ); + +if (git_check_attr (path, check, result))return NULL ; -if (ATTR_TRUE (check->check[0 ].value)) +if (ATTR_TRUE (result[0 ].value))return &driver_true; -if (ATTR_FALSE (check->check[0 ].value)) +if (ATTR_FALSE (result[0 ].value))return &driver_false; -if (ATTR_UNSET (check->check[0 ].value)) +if (ATTR_UNSET (result[0 ].value))return NULL ; -return userdiff_find_by_name (check->check[0 ].value); +return userdiff_find_by_name (result[0 ].value); }struct userdiff_driver *userdiff_get_textconv (struct userdiff_driver *driver)
ws.c +7 -10
@@ -74,15 +74,12 @@ unsigned parse_whitespace_rule(const char *string)unsigned whitespace_rule (const char *pathname) {static struct git_attr_check *attr_whitespace_rule; +struct git_attr_result result[1 ]; -if (!attr_whitespace_rule) - attr_whitespace_rule =git_attr_check_initl ("whitespace" ,NULL ); +git_attr_check_initl (&attr_whitespace_rule,"whitespace" ,NULL ); -if (!git_check_attr (pathname, attr_whitespace_rule)) { -const char *value; - - value = attr_whitespace_rule->check[0 ].value; -if (ATTR_TRUE (value)) { +if (!git_check_attr (pathname, attr_whitespace_rule, result)) { +if (ATTR_TRUE (result[0 ].value)) {/* true (whitespace) */ unsigned all_rule =ws_tab_width (whitespace_rule_cfg);int i;
@@ -91,15 +88,15 @@ unsigned whitespace_rule(const char *pathname) !whitespace_rule_names[i].exclude_default) all_rule |= whitespace_rule_names[i].rule_bits;return all_rule; - }else if (ATTR_FALSE (value)) { + }else if (ATTR_FALSE (result[0 ].value)) {/* false (-whitespace) */ return ws_tab_width (whitespace_rule_cfg); - }else if (ATTR_UNSET (value)) { + }else if (ATTR_UNSET (result[0 ].value)) {/* reset to default (!whitespace) */ return whitespace_rule_cfg; }else {/* string */ -return parse_whitespace_rule (value); +return parse_whitespace_rule (result[0 ].value); } }else {return whitespace_rule_cfg;
Signature · 1 line
2.10.1.508.g6572022
Re: [PATCH 27/36] attr: convert to new threadsafe API
Quoted 14 lines · 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:
Quoted 8 lines
* 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 thecheckvariable, i.e. the API has to look likegit_attr_check_initl(struct git_attr_check*, ...);
Make that a double-asterisk. The same problem appears in an updated example in technical/api-gitattributes.txt doc, but the example in the commit log message (below) is correct.
Quoted 16 lines
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(&check, "text", NULL); /* * Obtain a pointer to a correctly sized result * statically allocated on the stack; this macro: */ GIT_ATTR_RESULT_INIT_FOR(myresult, 1);
Are you sure about this? We've called attr_check_initl() already so if this is declaring myresult, it would be decl-after-stmt.
Quoted 8 lines
/* Perform the check and act on it: */
git_check_attr(path, check, myresult);
act_on(myresult->value[0]);
/*
* 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.
*/The latter half is questionable. If it is "static" it wouldn't be thread safe, no? I think the diff in this patch for archive.c shows that we only expect
struct git_attr_result result[2];
upfront without RESULT_INIT_FOR(), and the reason why there is no need to free the result[] is because it is on the stack. And each element in result[] may point at a string, but the string belongs to the attr subsystem and must not be freed.
Re: [PATCH 27/36] attr: convert to new threadsafe API
Quoted 4 lines · Junio C Hamano
Make that a double-asterisk. The same problem appears in an updated example in
technical/api-gitattributes.txtdoc, but the example in the commit log message (below) is correct.
The implementation is actually using a double pointer, see below, I forgot commit message and documentation
Quoted 4 lines
Quoted 1 line
GIT_ATTR_RESULT_INIT_FOR(myresult, 1);Are you sure about this? We've called
attr_check_initl()already so if this is declaring myresult, it would be decl-after-stmt.
I forgot to update the commit message and Documentation. GIT_ATTR_RESULT_INIT_FOR is gone in the header and in the implementation. I'll update that patch to be consistent throughout all of {Documentation, commit message, implementation}.
Quoted 5 lines
The latter half is questionable. If it is "static" it wouldn't be thread safe, no? I think the diff in this patch for
archive.cshows that we only expect
Quoted 7 lines
struct git_attr_result result[2];upfront without
RESULT_INIT_FOR(), and the reason why there is no need to free the result[] is because it is on the stack. And each element in result[] may point at a string, but the string belongs to the attr subsystem and must not be freed.
Same as above, it's bogus.
Thanks,
Stefan
Re: [PATCH 27/36] attr: convert to new threadsafe API
Quoted 10 lines · Stefan Beller
Quoted 4 lines · Junio C Hamano
Make that a double-asterisk. The same problem appears in an updated example in
technical/api-gitattributes.txtdoc, but the example in the commit log message (below) is correct.The implementation is actually using a double pointer, see below, I forgot commit message and documentation
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
GIT_ATTR_RESULT_INIT_FOR(myresult, 1);Are you sure about this? We've called
attr_check_initl()already so if this is declaring myresult, it would be decl-after-stmt.I forgot to update the commit message and Documentation.
GIT_ATTR_RESULT_INIT_FORis gone
I was asking whether this function/macro was not thread-safe and found out it didn't exist as well, and it's bed time so I'm stopping. Will continue my skimming on the next re-roll :)
Signature · 1 line
Duy
Re: [PATCH 27/36] attr: convert to new threadsafe API
In reply to Stefan BellerHi Stefan,
Review struct git_attr { · line 54
Quoted 9 lines · Stefan Beller
(quoted diff) +5
-0@@ -46,6 +47,19 @@ struct git_attr { static int attr_nr; static struct git_attr *(git_attr_hash[HASHSIZE]); +#ifndef NO_PTHREADS + +static pthread_mutex_t attr_mutex; +#define attr_lock() pthread_mutex_lock(&attr_mutex) +#define attr_unlock() pthread_mutex_unlock(&attr_mutex)
This mutex is never initialized. That may work on the system you tested, but it is incorrect, and it does segfault on Windows. A lot.
I need *at least* something like this to make it stop crashing all over the test suite:
-- snipsnap --
attr.c +10 -1
@@ -50,7 +50,16 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);#ifndef NO_PTHREADS static pthread_mutex_t attr_mutex; -#define attr_lock ()pthread_mutex_lock(&attr_mutex) +static inline void attr_lock (void ) +{ +static int initialized; + +if (!initialized) { +pthread_mutex_init (&attr_mutex,NULL ); + initialized =1 ; + } +pthread_mutex_lock (&attr_mutex); +}#define attr_unlock ()pthread_mutex_unlock(&attr_mutex)#else
Re: [PATCH 27/36] attr: convert to new threadsafe API
Quoted 9 lines · Johannes Schindelin
attr.c +0
-1@@ -50,7 +50,16 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);#ifndef NO_PTHREADS static pthread_mutex_t attr_mutex; -#define attr_lock ()pthread_mutex_lock(&attr_mutex)
attr.c static struct git_attr *(git_attr_hash[HASHSIZE]); · line 62
Quoted 10 lines
attr.c +10
-0+static inline void attr_lock (void ) +{ +static int initialized; + +if (!initialized) { +pthread_mutex_init (&attr_mutex,NULL ); + initialized =1 ; + } +pthread_mutex_lock (&attr_mutex); +}
This may initialize the mutex multiple times during the first lock (which may happen in parallel).
pthread provides static initializers. To quote the man page:
Variables of type pthread_mutex_t can also be initialized statically, using the constants PTHREAD_MUTEX_INITIALIZER (for fast mutexes), PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP (for recursive mutexes), and PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP (for error checking mutexes).
Regards
Simon
Signature · 3 lines
+ Privatsphäre ist notwendig + Ich verwende GnuPG http://gnupg.org + Öffentlicher Schlüssel: 0x92FEFDB7E44C32F9
Re: [PATCH 27/36] attr: convert to new threadsafe API
Quoted 16 lines · Simon Ruderich
attr.c static struct git_attr *(git_attr_hash[HASHSIZE]); · line 62
Quoted 12 lines
static
pthread_mutex_tattr_mutex;attr.c +10
-1-#define attr_lock ()pthread_mutex_lock(&attr_mutex) +static inline void attr_lock (void ) +{ +static int initialized; + +if (!initialized) { +pthread_mutex_init (&attr_mutex,NULL ); + initialized =1 ; + } +pthread_mutex_lock (&attr_mutex); +}This may initialize the mutex multiple times during the first lock (which may happen in parallel).
Quoted 8 lines
pthread provides static initializers. To quote the man page:
Variables of type
pthread_mutex_tcan also be initialized statically, using the constantsPTHREAD_MUTEX_INITIALIZER(for fast mutexes),PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP(for recursive mutexes), andPTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP(for error checking mutexes).
I seem to recall this does not work on Windows, where the pthread functions are thin wrappers over CRITICAL_SECTION. Other threaded code in git does an explicit setup step before entering threaded sections. E.g., see start_threads() in builtin/grep.c.
-Peff
Re: [PATCH 27/36] attr: convert to new threadsafe API
Quoted 27 lines · Jeff King
Quoted 16 lines · Simon Ruderich
attr.c static struct git_attr *(git_attr_hash[HASHSIZE]); · line 62
Quoted 12 lines
static
pthread_mutex_tattr_mutex;attr.c +10
-1-#define attr_lock ()pthread_mutex_lock(&attr_mutex) +static inline void attr_lock (void ) +{ +static int initialized; + +if (!initialized) { +pthread_mutex_init (&attr_mutex,NULL ); + initialized =1 ; + } +pthread_mutex_lock (&attr_mutex); +}This may initialize the mutex multiple times during the first lock (which may happen in parallel).
Quoted 8 lines
pthread provides static initializers. To quote the man page:
Variables of type
pthread_mutex_tcan also be initialized statically, using the constantsPTHREAD_MUTEX_INITIALIZER(for fast mutexes),PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP(for recursive mutexes), andPTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP(for error checking mutexes).
Quoted 5 lines
I seem to recall this does not work on Windows, where the pthread functions are thin wrappers over
CRITICAL_SECTION. Other threaded code in git does an explicit setup step before entering threaded sections. E.g., seestart_threads()inbuiltin/grep.c.
I wonder if we can have a similar thing as
http://stackoverflow.com/a/9490113 in compat/win32/pthread.{h.c} as it is very convenient to not have to explicitly initialize mutexes?
Re: [PATCH 27/36] attr: convert to new threadsafe API
Quoted 9 lines · Stefan Beller
Quoted 5 lines
I seem to recall this does not work on Windows, where the pthread functions are thin wrappers over
CRITICAL_SECTION. Other threaded code in git does an explicit setup step before entering threaded sections. E.g., seestart_threads()inbuiltin/grep.c.I wonder if we can have a similar thing as
http://stackoverflow.com/a/9490113 in compat/win32/pthread.{h.c} as it is very convenient to not have to explicitly initialize mutexes?
I agree it would be much more convenient and get rid of some repetitive boilerplate code. I'll leave it to Windows folks to decide if they are OK with that approach or not (I do not offhand know of any reason it would not work).
-Peff
Re: [PATCH 27/36] attr: convert to new threadsafe API
In reply to Stefan BellerAm 26.10.2016 um 21:51 schrieb Stefan Beller:
Quoted 2 lines
it is
very convenient to not have to explicitly initialize mutexes?
Not to initialize a mutex is still wrong for pthreads.
-- Hannes
Re: [PATCH 27/36] attr: convert to new threadsafe API
Quoted 5 lines · Johannes Sixt
Am 26.10.2016 um 21:51 schrieb Stefan Beller:
Quoted 2 lines
it is
very convenient to not have to explicitly initialize mutexes?Not to initialize a mutex is still wrong for pthreads.
I think Stefan was being loose with his wording. There would still be an initializer, but it would be a constant (and in the case of pthread emulation on Windows, would just be NULL).
-Peff
Re: [PATCH 27/36] attr: convert to new threadsafe API
Am 26.10.2016 um 22:26 schrieb Jeff King:
Quoted 11 lines · Johannes Sixt
Quoted 6 lines
Am 26.10.2016 um 21:51 schrieb Stefan Beller:
Quoted 2 lines
it is
very convenient to not have to explicitly initialize mutexes?Not to initialize a mutex is still wrong for pthreads.
I think Stefan was being loose with his wording. There would still be an initializer, but it would be a constant (and in the case of pthread emulation on Windows, would just be NULL).
And I was loose, too: Not to initialize a mutex with at least PTHREAD_MUTEX_INITILIZER (if not pthread_mutex_init) is still wrong.
-- Hannes
Re: [PATCH 27/36] attr: convert to new threadsafe API
Quoted 13 lines · Johannes Sixt
Am 26.10.2016 um 22:26 schrieb Jeff King:
Quoted 12 lines · Johannes Sixt
Quoted 8 lines
Am 26.10.2016 um 21:51 schrieb Stefan Beller:
Quoted 3 lines
it is
very convenient to not have to explicitly initialize mutexes?Not to initialize a mutex is still wrong for pthreads.
Quoted 8 lines
Quoted 3 lines
I think Stefan was being loose with his wording. There would still be an initializer, but it would be a constant (and in the case of pthread emulation on Windows, would just be NULL).
And I was loose, too: Not to initialize a mutex with at least
PTHREAD_MUTEX_INITILIZER(if notpthread_mutex_init) is still wrong.
My words were wrong, I meant statically initialized instead of the need to call a function to initialize a mutex. (For the attribute subsystem, where would that function go? We use attrs all over the place. My current thinking would be in git.c to initialize the Big Single Attr Lock. I feel like that is not very well
maintainable though).
Sorry for the confusion,
Stefan
PATCH v2 1/2 attr: convert to new threadsafe API
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(&check, "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.
*/---
In the reroll of this series, I plan to use the patch as-is below:
* Documentation/commit message matches actual implementation
* initialize the attr lock statically. It was uninitialized in the first version, which works for me on Linux, but not so in Windows; I split off the static initialized mutexes on Windows as a separate patch, see
https://public-inbox.org/git/CAGZ79kZryb-5jGif04BtK1V9tgFj-tnahqUk+1Lb7XeecU7cMQ@mail.gmail.com
which would be a requirement for this patch.
Documentation/technical/api-gitattributes.txt | 94 ++++++++++------- archive.c | 11 +- attr.c | 143 ++++++++++++++++++-------- attr.h | 71 ++++++++----- builtin/check-attr.c | 35 ++++--- builtin/pack-objects.c | 16 +-- convert.c | 40 +++---- ll-merge.c | 24 +++-- userdiff.c | 16 +-- ws.c | 8 +- 10 files changed, 280 insertions(+), 178 deletions(-)
Documentation/technical/api-gitattributes.txt +56 -38
@@ -16,15 +16,19 @@ 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 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 +36,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 +57,32 @@ value of the attribute for the path. Querying Specific Attributes ---------------------------- -* Prepare `struct git_attr_check` using git_attr_check_initl() +* 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() + 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 initialzisation 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. + +* Allocate an array of `struct git_attr_result` either statically on the + as a variable on the stack or dynamically via `git_attr_result_alloc` + when the result size is not known at compile time. The call to initialize + the result is not thread safe, because different threads need their + own thread local result anyway. + +* Call `git_check_attr()` to check the attributes for the path, + the given `git_attr_result` will be filled with the result. + +* Inspect each `git_attr_result` structure to see how + each of the attribute in the array is defined for the path. Example -------
@@ -76,28 +93,23 @@ To see how attributes "crlf" and "ident" are set for different paths. we are checking two attributes): ------------ -static struct git_attr_check *check; -static void setup_check(void) -{ - if (check) - return; /* already done */ - check = git_attr_check_initl("crlf", "ident", NULL); -} + static struct git_attr_check *check; + git_attr_check_initl(check, "crlf", "ident", NULL); ------------ . Call `git_check_attr()` with the prepared `struct git_attr_check`: ------------ const char *path; + struct git_attr_result result[2]; - setup_check(); - git_check_attr(path, check); + git_check_attr(path, check, result); ------------ -. Act on `.value` member of the result, left in `check->check[]`: +. Act on `result.value[]`: ------------ - const char *value = check->check[0].value; + const char *value = result.value[0]; if (ATTR_TRUE(value)) { The attribute is Set, by listing only the name of the
@@ -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); } ------------
@@ -138,17 +153,20 @@ Querying All Attributes To get the values of all attributes associated with a file: -* Prepare an empty `git_attr_check` structure by calling - `git_attr_check_alloc()`. +* 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. -* Call `git_all_attrs()`, which populates the `git_attr_check` - with the attributes attached to the path. +* Call `git_all_attrs()`. -* Iterate over the `git_attr_check.check[]` array to examine - the attribute names and values. The name of the attribute - described by a `git_attr_check.check[]` object can be retrieved via - `git_attr_name(check->check[i].attr)`. (Please note that no items +* Iterate over the `git_attr_check.attr[]` array to examine the + attribute names. The name of the attribute described by a + `git_attr_check.attr[]` object can be retrieved via + `git_attr_name(check->attr[i])`. (Please note that no items will be returned for unset attributes, so `ATTR_UNSET()` will return false for all returned `git_array_check` objects.) + The respective value for an attribute can be found in the same + index position in of `git_attr_result`. -* Free the `git_array_check` by calling `git_attr_check_free()`. +* Clear the variables by calling `git_attr_check_clear()` and + `git_attr_result_free()`.
archive.c +6 -5
@@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,struct archiver_args *args = c->args;write_archive_entry_fn_t write_entry = c->write_entry;static struct git_attr_check *check; +struct git_attr_result result[2 ];const char *path_without_prefix;int err;
@@ -124,12 +125,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,strbuf_addch (&path,'/' ); path_without_prefix = path.buf + args->baselen; -if (!check) - check =git_attr_check_initl ("export-ignore" ,"export-subst" ,NULL ); -if (!git_check_attr (path_without_prefix, check)) { -if (ATTR_TRUE (check->check[0 ].value)) +git_attr_check_initl (&check,"export-ignore" ,"export-subst" ,NULL ); + +if (!git_check_attr (path_without_prefix, check, result)) { +if (ATTR_TRUE (result[0 ].value))return 0 ; - args->convert =ATTR_TRUE (check->check[1 ].value); + args->convert =ATTR_TRUE (result[1 ].value); }if (S_ISDIR (mode) ||S_ISGITLINK (mode)) {
attr.c +100 -43
260 lines not on this page · raw message
attr.h +43 -28
97 lines not on this page · raw message
builtin/check-attr.c +19 -16
@@ -24,13 +24,14 @@ static const struct option check_attr_options[] = { OPT_END() }; -static void output_attr (struct git_attr_check *check,const char *file) +static void output_attr (struct git_attr_check *check, +struct git_attr_result *result,const char *file) {int j;int cnt = check->check_nr;for (j =0 ; j < cnt; j++) { -const char *value = check->check[j].value; +const char *value = result[j].value;if (ATTR_TRUE (value)) value ="set" ;
@@ -44,11 +45,11 @@ static void output_attr(struct git_attr_check *check, const char *file)"%s%c" /* attrname */ "%s%c" /* attrvalue */ , file,0 , - git_attr_name(check->check[j].attr), 0, value, 0); + git_attr_name(check->attr[j]), 0, value, 0); }else {quote_c_style (file,NULL , stdout,0 );printf (": %s: %s\n" , -git_attr_name (check->check[j].attr), value); +git_attr_name (check->attr[j]), value); } } }
@@ -59,16 +60,20 @@ static void check_attr(const char *prefix, {char *full_path =prefix_path (prefix, prefix ?strlen (prefix) :0 , file); +struct git_attr_check local_check =GIT_ATTR_CHECK_INIT ; +struct git_attr_result *result =NULL ; +if (check !=NULL ) { -if (git_check_attr (full_path, check)) -die ("git_check_attr died" ); -output_attr (check, file); + result =git_attr_result_alloc (check); +git_check_attr (full_path, check, result); }else { - check =git_attr_check_alloc (); -git_all_attrs (full_path, check); -output_attr (check, file); -git_attr_check_free (check); +git_all_attrs (full_path, &local_check, &result); + check = &local_check; } +output_attr (check, result, file); +git_attr_check_clear (&local_check); + +git_attr_result_free (result);free (full_path); }
@@ -102,7 +107,7 @@ static NORETURN void error_with_usage(const char *msg)int cmd_check_attr (int argc,const char **argv,const char *prefix) { -struct git_attr_check *check; +struct git_attr_check *check =NULL ;int cnt, i, doubledash, filei;if (!is_bare_repository ())
@@ -162,10 +167,8 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)error_with_usage ("No file specified" ); } -if (all_attrs) { - check =NULL ; - }else { - check =git_attr_check_alloc (); +if (!all_attrs) { +git_attr_check_alloc (&check);for (i =0 ; i < cnt; i++) {struct git_attr *a =git_attr (argv[i]);if (!a)
builtin/pack-objects.c +9 -7
@@ -899,14 +899,16 @@ static void write_pack_file(void)static int no_try_delta (const char *path) {static struct git_attr_check *check; +int ret =0 ; +struct git_attr_result result[1 ]; -if (!check) - check =git_attr_check_initl ("delta" ,NULL ); -if (git_check_attr (path, check)) -return 0 ; -if (ATTR_FALSE (check->check[0 ].value)) -return 1 ; -return 0 ; +git_attr_check_initl (&check,"delta" ,NULL ); + +if (!git_check_attr (path, check, result)) { +if (ATTR_FALSE (result[0 ].value)) + ret =1 ; + } +return ret; }/*
convert.c +21 -19
@@ -718,9 +718,9 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,return 1 ; } -static enum crlf_action git_path_check_crlf (struct git_attr_check_elem *check) +static enum crlf_action git_path_check_crlf (struct git_attr_result *result) { -const char *value = check->value; +const char *value = result->value;if (ATTR_TRUE (value))return CRLF_TEXT ;
@@ -735,9 +735,9 @@ static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)return CRLF_UNDEFINED ; } -static enum eol git_path_check_eol (struct git_attr_check_elem *check) +static enum eol git_path_check_eol (struct git_attr_result *result) { -const char *value = check->value; +const char *value = result->value;if (ATTR_UNSET (value)) ;
@@ -748,9 +748,9 @@ static enum eol git_path_check_eol(struct git_attr_check_elem *check)return EOL_UNSET ; } -static struct convert_driver *git_path_check_convert (struct git_attr_check_elem *check) +static struct convert_driver *git_path_check_convert (struct git_attr_result *result) { -const char *value = check->value; +const char *value = result->value;struct convert_driver *drv;if (ATTR_TRUE (value) ||ATTR_FALSE (value) ||ATTR_UNSET (value))
@@ -761,9 +761,9 @@ static struct convert_driver *git_path_check_convert(struct git_attr_check_elemreturn NULL ; } -static int git_path_check_ident (struct git_attr_check_elem *check) +static int git_path_check_ident (struct git_attr_result *result) { -const char *value = check->value; +const char *value = result->value;return !!ATTR_TRUE (value); }
@@ -778,25 +778,27 @@ struct conv_attrs {static void convert_attrs (struct conv_attrs *ca,const char *path) {static struct git_attr_check *check; +static int init_user_convert_tail; +struct git_attr_result result[5 ]; -if (!check) { - check =git_attr_check_initl ("crlf" ,"ident" , -"filter" ,"eol" ,"text" , -NULL ); +git_attr_check_initl (&check,"crlf" ,"ident" ,"filter" , +"eol" ,"text" ,NULL ); + +if (!init_user_convert_tail) { user_convert_tail = &user_convert;git_config (read_convert_config,NULL ); + init_user_convert_tail =1 ; } -if (!git_check_attr (path, check)) { -struct git_attr_check_elem *ccheck = check->check; - ca->crlf_action =git_path_check_crlf (ccheck +4 ); +if (!git_check_attr (path, check, result)) { + ca->crlf_action =git_path_check_crlf (&result[4 ]);if (ca->crlf_action ==CRLF_UNDEFINED ) - ca->crlf_action =git_path_check_crlf (ccheck +0 ); + ca->crlf_action =git_path_check_crlf (&result[0 ]); ca->attr_action = ca->crlf_action; - ca->ident =git_path_check_ident (ccheck +1 ); - ca->drv =git_path_check_convert (ccheck +2 ); + ca->ident =git_path_check_ident (&result[1 ]); + ca->drv =git_path_check_convert (&result[2 ]);if (ca->crlf_action !=CRLF_BINARY ) { -enum eol eol_attr =git_path_check_eol (ccheck +3 ); +enum eol eol_attr =git_path_check_eol (&result[3 ]);if (ca->crlf_action ==CRLF_AUTO && eol_attr ==EOL_LF ) ca->crlf_action =CRLF_AUTO_INPUT ;else if (ca->crlf_action ==CRLF_AUTO && eol_attr ==EOL_CRLF )
ll-merge.c +13 -11
@@ -353,12 +353,14 @@ int ll_merge(mmbuffer_t *result_buf, mmfile_t *theirs, const char *their_label, const struct ll_merge_options *opts) { - static struct git_attr_check *check; static const struct ll_merge_options default_opts;const char *ll_driver_name =NULL ;int marker_size =DEFAULT_CONFLICT_MARKER_SIZE ;const struct ll_merge_driver *driver; +static struct git_attr_check *check; +struct git_attr_result result[2 ]; +if (!opts) opts = &default_opts;
@@ -368,13 +370,12 @@ int ll_merge(mmbuffer_t *result_buf,normalize_file (theirs, path); } -if (!check) - check =git_attr_check_initl ("merge" ,"conflict-marker-size" ,NULL ); +git_attr_check_initl (&check,"merge" ,"conflict-marker-size" ,NULL ); -if (!git_check_attr (path, check)) { - ll_driver_name = check->check[0 ].value; -if (check->check[1 ].value) { - marker_size =atoi (check->check[1 ].value); +if (!git_check_attr (path, check, result)) { + ll_driver_name = result[0 ].value; +if (result[1 ].value) { + marker_size =atoi (result[1 ].value);if (marker_size <=0 ) marker_size =DEFAULT_CONFLICT_MARKER_SIZE ; }
@@ -395,11 +396,12 @@ int ll_merge_marker_size(const char *path) {static struct git_attr_check *check;int marker_size =DEFAULT_CONFLICT_MARKER_SIZE ; +struct git_attr_result result[1 ]; + +git_attr_check_initl (&check,"conflict-marker-size" ,NULL ); -if (!check) - check =git_attr_check_initl ("conflict-marker-size" ,NULL ); -if (!git_check_attr (path, check) && check->check[0 ].value) { - marker_size =atoi (check->check[0 ].value); +if (!git_check_attr (path, check, result) && !ATTR_UNSET (result[0 ].value)) { + marker_size =atoi (result[0 ].value);if (marker_size <=0 ) marker_size =DEFAULT_CONFLICT_MARKER_SIZE ; }
userdiff.c +9 -7
@@ -263,21 +263,23 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {struct userdiff_driver *userdiff_find_by_path (const char *path) {static struct git_attr_check *check; +struct git_attr_result result[1 ]; -if (!check) - check =git_attr_check_initl ("diff" ,NULL );if (!path)return NULL ; -if (git_check_attr (path, check)) + +git_attr_check_initl (&check,"diff" ,NULL ); + +if (git_check_attr (path, check, result))return NULL ; -if (ATTR_TRUE (check->check[0 ].value)) +if (ATTR_TRUE (result[0 ].value))return &driver_true; -if (ATTR_FALSE (check->check[0 ].value)) +if (ATTR_FALSE (result[0 ].value))return &driver_false; -if (ATTR_UNSET (check->check[0 ].value)) +if (ATTR_UNSET (result[0 ].value))return NULL ; -return userdiff_find_by_name (check->check[0 ].value); +return userdiff_find_by_name (result[0 ].value); }struct userdiff_driver *userdiff_get_textconv (struct userdiff_driver *driver)
ws.c +4 -4
@@ -74,14 +74,14 @@ unsigned parse_whitespace_rule(const char *string)unsigned whitespace_rule (const char *pathname) {static struct git_attr_check *attr_whitespace_rule; +struct git_attr_result result[1 ]; -if (!attr_whitespace_rule) - attr_whitespace_rule =git_attr_check_initl ("whitespace" ,NULL ); +git_attr_check_initl (&attr_whitespace_rule,"whitespace" ,NULL ); -if (!git_check_attr (pathname, attr_whitespace_rule)) { +if (!git_check_attr (pathname, attr_whitespace_rule, result)) {const char *value; - value = attr_whitespace_rule->check[0 ].value; + value = result[0 ].value;if (ATTR_TRUE (value)) {/* true (whitespace) */ unsigned all_rule =ws_tab_width (whitespace_rule_cfg);
Signature · 1 line
2.10.1.508.g6572022
Re: [PATCHv2 1/2] attr: convert to new threadsafe API
Quoted 12 lines · Stefan Beller
Documentation/technical/api-gitattributes.txt +1
-5@@ -53,19 +57,32 @@ value of the attribute for the path. Querying Specific Attributes ---------------------------- -* Prepare `struct git_attr_check` using git_attr_check_initl() +* 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.
Documentation/technical/api-gitattributes.txt value of the attribute for the path. · line 66
Quoted 10 lines
Documentation/technical/api-gitattributes.txt +4
-6- -* 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() + 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.
I think that my version that was discarded forbade appending once you started to use the check for querying, because the check was meant to be used as a key for an attr-stack and the check-specific attr-stack was planned to keep only the attrs the check is interested in (and appending is to change the set of attrs the check is interested in, invalidating the attr-stack at that point).
If you lost that restriction, that is good (I didn't check the implementation, though). Otherwise we'd need to say something here.
Documentation/technical/api-gitattributes.txt value of the attribute for the path. · line 70
Quoted 4 lines
Documentation/technical/api-gitattributes.txt +4
-0+ 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 initialzisation is still needed, the threads will use a
initialization?
Documentation/technical/api-gitattributes.txt value of the attribute for the path. · line 76
Quoted 6 lines
Documentation/technical/api-gitattributes.txt +6
-0+ single global mutex to perform the initialization just once, the + others will wait on the the thread to actually perform the + initialization. + +* Allocate an array of `struct git_attr_result` either statically on the + as a variable on the stack or dynamically via `git_attr_result_alloc`
Grammo? "either on the stack, or dynamically in the heap"?
Documentation/technical/api-gitattributes.txt value of the attribute for the path. · line 79
Quoted 3 lines
Documentation/technical/api-gitattributes.txt +3
-0+ when the result size is not known at compile time. The call to initialize + the result is not thread safe, because different threads need their + own thread local result anyway.
Having result defined statically is not thread safe for that reason. It is not clear what you mean by "The call to initialize the result"; having it on the stack or have one dynamically on the heap ought to be thread safe.
Quoted 11 lines
Documentation/technical/api-gitattributes.txt +5
-0+* Call `git_check_attr()` to check the attributes for the path, + the given `git_attr_result` will be filled with the result. + +* Inspect each `git_attr_result` structure to see how + each of the attribute in the array is defined for the path. Example -------@@ -76,28 +93,23 @@ To see how attributes "crlf" and "ident" are set for different paths. we are checking two attributes):
Documentation/technical/api-gitattributes.txt To see how attributes "crlf" and "ident" are set for different paths. · line 97
Quoted 10 lines
Documentation/technical/api-gitattributes.txt +2
-7------------ -static struct git_attr_check *check; -static void setup_check(void) -{ - if (check) - return; /* already done */ - check = git_attr_check_initl("crlf", "ident", NULL); -} + static struct git_attr_check *check; + git_attr_check_initl(check, "crlf", "ident", NULL);
I think you are still missing "&" here.
Quoted 23 lines
Documentation/technical/api-gitattributes.txt +4
-4------------ . Call `git_check_attr()` with the prepared `struct git_attr_check`: ------------ const char *path; + struct git_attr_result result[2]; - setup_check(); - git_check_attr(path, check); + git_check_attr(path, check, result); ------------ -. Act on `.value` member of the result, left in `check->check[]`: +. Act on `result.value[]`: ------------ - const char *value = check->check[0].value; + const char *value = result.value[0]; if (ATTR_TRUE(value)) { The attribute is Set, by listing only the name of the
Documentation/technical/api-gitattributes.txt the first step in the above would be different. · line 140
Quoted 6 lines
Documentation/technical/api-gitattributes.txt +2
-0@@ -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();
You may want to say that this is thread-unsafe.
Quoted 10 lines
Documentation/technical/api-gitattributes.txt +1
-0while (*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); } ------------
Documentation/technical/api-gitattributes.txt Querying All Attributes · line 159
Quoted 7 lines
Documentation/technical/api-gitattributes.txt +4
-0@@ -138,17 +153,20 @@ Querying All Attributes To get the values of all attributes associated with a file: +* 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. +* Call `git_all_attrs()`.
Hmph, the caller does not know what attribute it is interested in, and it is unclear "how" the former needs to be set up from this description. Should it prepare an empty one that can be appended?
For the same reason, the size of the result is also unknown at the callsite.
For these reasons, I thought git-all-attrs would need to use a different calling convention from the usual "query for these attributes -- get result for them" API. Even if the same attr_check and attr_result structures are used, they need to be initialized in very different way from the normal calls, and this part of the doc needs to explain how. Perhaps it is sufficient to say "just declare two pointer variables that point at these structures and a pair of newly allocated structures of these types will be stored there" or something.
It does not even have to use the usual "struct attr_check" "struct attr_result" pair (I am not suggesting to use different convention only to be different; I am merely pointing out that you have that lattitude, as the usage pattern for all-attrs is so different from the usual query).
Re: [PATCHv2 1/2] attr: convert to new threadsafe API
Quoted 32 lines · Junio C Hamano
Quoted 12 lines · Stefan Beller
(quoted diff) +1
-5@@ -53,19 +57,32 @@ value of the attribute for the path. Querying Specific Attributes ---------------------------- -* Prepare `struct git_attr_check` using git_attr_check_initl() +* 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.Review value of the attribute for the path. · line 66
Quoted 10 lines
(quoted diff) +4
-6- -* 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() + 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.I think that my version that was discarded forbade appending once you started to use the check for querying, because the check was meant to be used as a key for an attr-stack and the check-specific attr-stack was planned to keep only the attrs the check is interested in (and appending is to change the set of attrs the check is interested in, invalidating the attr-stack at that point).
Quoted 2 lines
If you lost that restriction, that is good (I didn't check the implementation, though). Otherwise we'd need to say something here.
That restriction still applies. Though I think mentioning it in the paragraph where we describe querying makes more sense.
Quoted 1 line
initialization?
done
Quoted 2 lines
Grammo? "either on the stack, or dynamically in the heap"?
done
Quoted 5 lines
Having result defined statically is not thread safe for that reason. It is not clear what you mean by "The call to initialize the result"; having it on the stack or have one dynamically on the heap ought to be thread safe.
done
Quoted 5 lines
Review value of the attribute for the path. · line 68
Quoted 3 lines
(quoted diff) +2
-1-} + static struct git_attr_check *check; + git_attr_check_initl(check, "crlf", "ident", NULL);I think you are still missing "&" here.
done
Quoted 5 lines
Review value of the attribute for the path. · line 71
Quoted 3 lines
(quoted diff) +2
-0+ if (check) + return; /* already done */ check = git_attr_check_alloc();You may want to say that this is thread-unsafe.
It is not; see the implementation:
void git_attr_check_append(struct git_attr_check *check,
const struct git_attr *attr)
{
int i;
if (check->finalized)
die("BUG: append after git_attr_check structure is finalized");
if (!attr_check_is_dynamic(check))
die("BUG: appending to a statically initialized git_attr_check");
attr_lock();
for (i = 0; i < check->check_nr; i++)
if (check->attr[i] == attr)
break;
if (i == check->check_nr) {
ALLOC_GROW(check->attr, check->check_nr + 1, check->check_alloc);
check->attr[check->check_nr++] = attr;
}
attr_unlock();
}Quoted 6 lines
Review value of the attribute for the path. · line 72
Quoted 1 line
(quoted diff) +1
-0+* Call `git_all_attrs()`.Hmph, the caller does not know what attribute it is interested in, and it is unclear "how" the former needs to be set up from this description. Should it prepare an empty one that can be appended?
Good point on clarifying this one. It is just needed to have NULL pointers around:
struct git_attr_check *check = NULL;
struct git_attr_result *result = NULL;
git_all_attrs(full_path, &local_check, &result);
// proceed as in the case aboveAll comments from above yield the following diff:
(quoted diff) +0 -0
b/Documentation/technical/api-gitattributes.txt
index 4d8ef93..d221736 100644
Documentation/technical/api-gitattributes.txt +6 -4
@@ -67,19 +67,21 @@ Querying Specific Attributes 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 initialzisation is still needed, the threads will use a + 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. -* Allocate an array of `struct git_attr_result` either statically on the - as a variable on the stack or dynamically via `git_attr_result_alloc` - when the result size is not known at compile time. The call to initialize +* Allocate an array of `struct git_attr_result` either on the stack + or via `git_attr_result_alloc` on the heap when the result size + is not known at compile time. The call to initialize the result is not thread safe, because different threads need their own thread local result anyway. * Call `git_check_attr()` to check the attributes for the path, the given `git_attr_result` will be filled with the result. + You must not change the `struct git_attr_check` after calling + `git_check_attr()`. * Inspect each `git_attr_result` structure to see how each of the attribute in the array is defined for the path.
@@ -103,7 +105,7 @@ To see how attributes "crlf" and "ident" are set
for different paths.
const char *path;
struct git_attr_result result[2];- git_check_attr(path, check, result);
+ git_check_attr(path, &check, result);
------------
. Act on result.value[]:
@@ -155,10 +157,20 @@ To get the values of all attributes associated with a file:
* 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. + `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); +------------
+
* Iterate over the git_attr_check.attr[] array to examine the attribute names. The name of the attribute described by a git_attr_check.attr[] object can be retrieved via
Re: [PATCHv2 1/2] attr: convert to new threadsafe API
Documentation/technical/api-gitattributes.txt Querying Specific Attributes · line 79
Quoted 5 lines · Stefan Beller
Documentation/technical/api-gitattributes.txt +3
-0+* Allocate an array of `struct git_attr_result` either on the stack + or via `git_attr_result_alloc` on the heap when the result size + is not known at compile time. The call to initialize the result is not thread safe, because different threads need their own thread local result anyway.
Do you want to keep the last sentence? "The call to initialize the result is not thread safe..."? Is that true?
Review
Quoted 7 lines
(quoted diff) +0
-0@@ -103,7 +105,7 @@ To see how attributes "crlf" and "ident" are setfor different paths.
const char *path; struct git_attr_result result[2];(quoted diff) +1
-1- git_check_attr(path, check, result); + git_check_attr(path, &check, result);
What's the point of this change? Isn't check typically a pointer already?
Re: [PATCHv2 1/2] attr: convert to new threadsafe API
Quoted 10 lines · Junio C Hamano
Documentation/technical/api-gitattributes.txt Querying Specific Attributes · line 79
Quoted 5 lines · Stefan Beller
Documentation/technical/api-gitattributes.txt +3
-0+* Allocate an array of `struct git_attr_result` either on the stack + or via `git_attr_result_alloc` on the heap when the result size + is not known at compile time. The call to initialize the result is not thread safe, because different threads need their own thread local result anyway.Do you want to keep the last sentence? "The call to initialize the result is not thread safe..."? Is that true?
I'll drop that sentence, as it overstates the situation.
To explain, you can either have:
struct git_attr_result result[2];
or
struct git_attr_result *result = git_attr_result_alloc(check);and both are running just fine in a thread. However you should not make that variable static. But maybe that is too much common sense and hence confusing.
Quoted 11 lines
Review
Quoted 8 lines
(quoted diff) +0
-0@@ -103,7 +105,7 @@ To see how attributes "crlf" and "ident" are setfor different paths.
const char *path; struct git_attr_result result[2];(quoted diff) +1
-1- git_check_attr(path, check, result); + git_check_attr(path, &check, result);What's the point of this change? Isn't check typically a pointer already?
This ought to go to
git_attr_check_initl(&check, "crlf", "ident", NULL);
instead.
Quoted 1 line
Re: [PATCHv2 1/2] attr: convert to new threadsafe API
Quoted 7 lines · Stefan Beller
To explain, you can either have:
struct git_attr_result result[2]; or struct git_attr_result *result = git_attr_result_alloc(check);and both are running just fine in a thread. However you should not make that variable static. But maybe that is too much common sense and hence confusing.
Yup, if you spelled it out that does make sense, but the description given in the patch was just misleading.
Thanks.
Re: [PATCHv2 1/2] attr: convert to new threadsafe API
In reply to Stefan Bellerattr.h extern const char git_attr__false[]; · line 63
Quoted 4 lines · Stefan Beller
extern struct
git_attr*git_attr(const char *);
...attr.h +2
-0+extern void git_attr_check_append (struct git_attr_check *, +const struct git_attr *);
Another thing. Do we still need to expose git_attr() to the outside callers? If we change git_attr_check_append() to take "const char *" as its second parameter, can we retire it from the public interface?
It being an "intern" function, by definition it is not thread-safe. Its use from prepare_attr_stack() inside git_check_attr() that takes the Big Attribute Subsystem Lock is safe, but the callers that do
struct git_attr_check *check = ...;
struct git_attr *text_attr = git_attr("text");
git_attr_check_append(check, text_attr);would risk racing to register the same entry to the "all the attributes in the known universe" table.
If the attribute API does not have to expose git_attr(const char *) to the external callers at all, that would be ideal. Otherwise, we would need to rename the current git_attr() that is 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.
Re: [PATCHv2 1/2] attr: convert to new threadsafe API
Quoted 28 lines · Junio C Hamano
attr.h extern const char git_attr__false[]; · line 63
Quoted 4 lines · Stefan Beller
extern struct
git_attr*git_attr(const char *);
...attr.h +2
-0+extern void git_attr_check_append (struct git_attr_check *, +const struct git_attr *);Another thing. Do we still need to expose
git_attr()to the outside callers? If we changegit_attr_check_append()to take "const char *" as its second parameter, can we retire it from the public interface?It being an "intern" function, by definition it is not thread-safe. Its use from
prepare_attr_stack()insidegit_check_attr()that takes the Big Attribute Subsystem Lock is safe, but the callers that dostruct git_attr_check *check = ...; struct git_attr *text_attr = git_attr("text"); git_attr_check_append(check, text_attr);would risk racing to register the same entry to the "all the attributes in the known universe" table.
If the attribute API does not have to expose
git_attr(const char *)to the external callers at all, that would be ideal. Otherwise, we would need to rename the currentgit_attr()that is used internally under the Big Lock to
Quoted 6 lines
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.
Yeah, I can make it work without exposing struct git_attr. However then the struct git_attr_check will contain more of internals exposed. (As the header file did not know the size of git_attr, the patch under discussion even must use a double pointer to a git_attr inside the git_attr_check as the git_attr size is unknown)
So I'll look into removing that struct git_attr completely.
Re: [PATCHv2 1/2] attr: convert to new threadsafe API
Quoted 1 line · Stefan Beller
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.
Re: [PATCHv2 1/2] attr: convert to new threadsafe API
Quoted 9 lines · Junio C Hamano
Quoted 1 line · Stefan Beller
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.
PATCH 1/1 attr: convert to new threadsafe API
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(&check, "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.
*/---
* use attr_start on Windows to dynamically initialize the Single Big Attr Mutex
* rewrote the documentation by Junios guiding comments
* interned struct git_attr, i.e. you hand "const char *" to the attr API, and internally it will make sense of it. When asking git_all_attrs, however you still need to know about git_attrs as you need to find out the name of the attribute via git_attr_name(struct git_attr *).
If there are no other huge design discussions, I'll reroll the whole series.
Thanks,
Stefan
Documentation/technical/api-gitattributes.txt | 104 +++++++++++------- archive.c | 11 +- attr.c | 150 ++++++++++++++++++-------- attr.h | 76 +++++++------ builtin/check-attr.c | 47 ++++---- builtin/pack-objects.c | 16 +-- compat/mingw.c | 3 + convert.c | 40 +++---- ll-merge.c | 24 +++-- userdiff.c | 16 +-- ws.c | 8 +- 11 files changed, 305 insertions(+), 190 deletions(-)
Documentation/technical/api-gitattributes.txt +66 -38
@@ -16,15 +16,19 @@ 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 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 +36,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 +57,32 @@ value of the attribute for the path. Querying Specific Attributes ---------------------------- -* Prepare `struct git_attr_check` using git_attr_check_initl() +* 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() + 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. + +* Allocate an array of `struct git_attr_result` either on the stack + or via `git_attr_result_alloc` on the heap when the result size + is not known at compile time. + +* Call `git_check_attr()` to check the attributes for the path, + the given `git_attr_result` will be filled with the result. + You must not change the `struct git_attr_check` after calling + `git_check_attr()`. + +* Inspect each `git_attr_result` structure to see how + each of the attribute in the array is defined for the path. Example -------
@@ -76,28 +93,23 @@ To see how attributes "crlf" and "ident" are set for different paths. we are checking two attributes): ------------ -static struct git_attr_check *check; -static void setup_check(void) -{ - if (check) - return; /* already done */ - check = git_attr_check_initl("crlf", "ident", NULL); -} + static struct git_attr_check *check; + git_attr_check_initl(&check, "crlf", "ident", NULL); ------------ . Call `git_check_attr()` with the prepared `struct git_attr_check`: ------------ const char *path; + struct git_attr_result result[2]; - setup_check(); - git_check_attr(path, check); + git_check_attr(path, check, result); ------------ -. Act on `.value` member of the result, left in `check->check[]`: +. Act on `result.value[]`: ------------ - const char *value = check->check[0].value; + const char *value = result.value[0]; if (ATTR_TRUE(value)) { The attribute is Set, by listing only the name of the
@@ -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); } ------------
@@ -138,17 +153,30 @@ Querying All Attributes To get the values of all attributes associated with a file: -* Prepare an empty `git_attr_check` structure by calling - `git_attr_check_alloc()`. +* 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()`. -* Call `git_all_attrs()`, which populates the `git_attr_check` - with the attributes attached to the path. +------------ + git_all_attrs(full_path, &check, &result); +------------ -* Iterate over the `git_attr_check.check[]` array to examine - the attribute names and values. The name of the attribute - described by a `git_attr_check.check[]` object can be retrieved via - `git_attr_name(check->check[i].attr)`. (Please note that no items +* Iterate over the `git_attr_check.attr[]` array to examine the + attribute names. The name of the attribute described by a + `git_attr_check.attr[]` object can be retrieved via + `git_attr_name(check->attr[i])`. (Please note that no items will be returned for unset attributes, so `ATTR_UNSET()` will return false for all returned `git_array_check` objects.) + The respective value for an attribute can be found in the same + index position in of `git_attr_result`. -* Free the `git_array_check` by calling `git_attr_check_free()`. +* Clear the variables by calling `git_attr_check_clear()` and + `git_attr_result_free()`.
archive.c +6 -5
@@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,struct archiver_args *args = c->args;write_archive_entry_fn_t write_entry = c->write_entry;static struct git_attr_check *check; +struct git_attr_result result[2 ];const char *path_without_prefix;int err;
@@ -124,12 +125,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,strbuf_addch (&path,'/' ); path_without_prefix = path.buf + args->baselen; -if (!check) - check =git_attr_check_initl ("export-ignore" ,"export-subst" ,NULL ); -if (!git_check_attr (path_without_prefix, check)) { -if (ATTR_TRUE (check->check[0 ].value)) +git_attr_check_initl (&check,"export-ignore" ,"export-subst" ,NULL ); + +if (!git_check_attr (path_without_prefix, check, result)) { +if (ATTR_TRUE (result[0 ].value))return 0 ; - args->convert =ATTR_TRUE (check->check[1 ].value); + args->convert =ATTR_TRUE (result[1 ].value); }if (S_ISDIR (mode) ||S_ISGITLINK (mode)) {
attr.c +107 -43
267 lines not on this page · raw message
attr.h +41 -35
102 lines not on this page · raw message
builtin/check-attr.c +26 -21
89 lines not on this page · raw message
builtin/pack-objects.c +9 -7
@@ -899,14 +899,16 @@ static void write_pack_file(void)static int no_try_delta (const char *path) {static struct git_attr_check *check; +int ret =0 ; +struct git_attr_result result[1 ]; -if (!check) - check =git_attr_check_initl ("delta" ,NULL ); -if (git_check_attr (path, check)) -return 0 ; -if (ATTR_FALSE (check->check[0 ].value)) -return 1 ; -return 0 ; +git_attr_check_initl (&check,"delta" ,NULL ); + +if (!git_check_attr (path, check, result)) { +if (ATTR_FALSE (result[0 ].value)) + ret =1 ; + } +return ret; }/*
compat/mingw.c +3 -0
@@ -2232,6 +2232,9 @@ void mingw_startup(void)/* initialize critical section for waitpid pinfo_t list */ InitializeCriticalSection (&pinfo_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);
convert.c +21 -19
@@ -722,9 +722,9 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,return 1 ; } -static enum crlf_action git_path_check_crlf (struct git_attr_check_elem *check) +static enum crlf_action git_path_check_crlf (struct git_attr_result *result) { -const char *value = check->value; +const char *value = result->value;if (ATTR_TRUE (value))return CRLF_TEXT ;
@@ -739,9 +739,9 @@ static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)return CRLF_UNDEFINED ; } -static enum eol git_path_check_eol (struct git_attr_check_elem *check) +static enum eol git_path_check_eol (struct git_attr_result *result) { -const char *value = check->value; +const char *value = result->value;if (ATTR_UNSET (value)) ;
@@ -752,9 +752,9 @@ static enum eol git_path_check_eol(struct git_attr_check_elem *check)return EOL_UNSET ; } -static struct convert_driver *git_path_check_convert (struct git_attr_check_elem *check) +static struct convert_driver *git_path_check_convert (struct git_attr_result *result) { -const char *value = check->value; +const char *value = result->value;struct convert_driver *drv;if (ATTR_TRUE (value) ||ATTR_FALSE (value) ||ATTR_UNSET (value))
@@ -765,9 +765,9 @@ static struct convert_driver *git_path_check_convert(struct git_attr_check_elemreturn NULL ; } -static int git_path_check_ident (struct git_attr_check_elem *check) +static int git_path_check_ident (struct git_attr_result *result) { -const char *value = check->value; +const char *value = result->value;return !!ATTR_TRUE (value); }
@@ -782,25 +782,27 @@ struct conv_attrs {static void convert_attrs (struct conv_attrs *ca,const char *path) {static struct git_attr_check *check; +static int init_user_convert_tail; +struct git_attr_result result[5 ]; -if (!check) { - check =git_attr_check_initl ("crlf" ,"ident" , -"filter" ,"eol" ,"text" , -NULL ); +git_attr_check_initl (&check,"crlf" ,"ident" ,"filter" , +"eol" ,"text" ,NULL ); + +if (!init_user_convert_tail) { user_convert_tail = &user_convert;git_config (read_convert_config,NULL ); + init_user_convert_tail =1 ; } -if (!git_check_attr (path, check)) { -struct git_attr_check_elem *ccheck = check->check; - ca->crlf_action =git_path_check_crlf (ccheck +4 ); +if (!git_check_attr (path, check, result)) { + ca->crlf_action =git_path_check_crlf (&result[4 ]);if (ca->crlf_action ==CRLF_UNDEFINED ) - ca->crlf_action =git_path_check_crlf (ccheck +0 ); + ca->crlf_action =git_path_check_crlf (&result[0 ]); ca->attr_action = ca->crlf_action; - ca->ident =git_path_check_ident (ccheck +1 ); - ca->drv =git_path_check_convert (ccheck +2 ); + ca->ident =git_path_check_ident (&result[1 ]); + ca->drv =git_path_check_convert (&result[2 ]);if (ca->crlf_action !=CRLF_BINARY ) { -enum eol eol_attr =git_path_check_eol (ccheck +3 ); +enum eol eol_attr =git_path_check_eol (&result[3 ]);if (ca->crlf_action ==CRLF_AUTO && eol_attr ==EOL_LF ) ca->crlf_action =CRLF_AUTO_INPUT ;else if (ca->crlf_action ==CRLF_AUTO && eol_attr ==EOL_CRLF )
ll-merge.c +13 -11
@@ -353,12 +353,14 @@ int ll_merge(mmbuffer_t *result_buf, mmfile_t *theirs, const char *their_label, const struct ll_merge_options *opts) { - static struct git_attr_check *check; static const struct ll_merge_options default_opts;const char *ll_driver_name =NULL ;int marker_size =DEFAULT_CONFLICT_MARKER_SIZE ;const struct ll_merge_driver *driver; +static struct git_attr_check *check; +struct git_attr_result result[2 ]; +if (!opts) opts = &default_opts;
@@ -368,13 +370,12 @@ int ll_merge(mmbuffer_t *result_buf,normalize_file (theirs, path); } -if (!check) - check =git_attr_check_initl ("merge" ,"conflict-marker-size" ,NULL ); +git_attr_check_initl (&check,"merge" ,"conflict-marker-size" ,NULL ); -if (!git_check_attr (path, check)) { - ll_driver_name = check->check[0 ].value; -if (check->check[1 ].value) { - marker_size =atoi (check->check[1 ].value); +if (!git_check_attr (path, check, result)) { + ll_driver_name = result[0 ].value; +if (result[1 ].value) { + marker_size =atoi (result[1 ].value);if (marker_size <=0 ) marker_size =DEFAULT_CONFLICT_MARKER_SIZE ; }
@@ -395,11 +396,12 @@ int ll_merge_marker_size(const char *path) {static struct git_attr_check *check;int marker_size =DEFAULT_CONFLICT_MARKER_SIZE ; +struct git_attr_result result[1 ]; + +git_attr_check_initl (&check,"conflict-marker-size" ,NULL ); -if (!check) - check =git_attr_check_initl ("conflict-marker-size" ,NULL ); -if (!git_check_attr (path, check) && check->check[0 ].value) { - marker_size =atoi (check->check[0 ].value); +if (!git_check_attr (path, check, result) && !ATTR_UNSET (result[0 ].value)) { + marker_size =atoi (result[0 ].value);if (marker_size <=0 ) marker_size =DEFAULT_CONFLICT_MARKER_SIZE ; }
userdiff.c +9 -7
@@ -263,21 +263,23 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {struct userdiff_driver *userdiff_find_by_path (const char *path) {static struct git_attr_check *check; +struct git_attr_result result[1 ]; -if (!check) - check =git_attr_check_initl ("diff" ,NULL );if (!path)return NULL ; -if (git_check_attr (path, check)) + +git_attr_check_initl (&check,"diff" ,NULL ); + +if (git_check_attr (path, check, result))return NULL ; -if (ATTR_TRUE (check->check[0 ].value)) +if (ATTR_TRUE (result[0 ].value))return &driver_true; -if (ATTR_FALSE (check->check[0 ].value)) +if (ATTR_FALSE (result[0 ].value))return &driver_false; -if (ATTR_UNSET (check->check[0 ].value)) +if (ATTR_UNSET (result[0 ].value))return NULL ; -return userdiff_find_by_name (check->check[0 ].value); +return userdiff_find_by_name (result[0 ].value); }struct userdiff_driver *userdiff_get_textconv (struct userdiff_driver *driver)
ws.c +4 -4
@@ -74,14 +74,14 @@ unsigned parse_whitespace_rule(const char *string)unsigned whitespace_rule (const char *pathname) {static struct git_attr_check *attr_whitespace_rule; +struct git_attr_result result[1 ]; -if (!attr_whitespace_rule) - attr_whitespace_rule =git_attr_check_initl ("whitespace" ,NULL ); +git_attr_check_initl (&attr_whitespace_rule,"whitespace" ,NULL ); -if (!git_check_attr (pathname, attr_whitespace_rule)) { +if (!git_check_attr (pathname, attr_whitespace_rule, result)) {const char *value; - value = attr_whitespace_rule->check[0 ].value; + value = result[0 ].value;if (ATTR_TRUE (value)) {/* true (whitespace) */ unsigned all_rule =ws_tab_width (whitespace_rule_cfg);
Signature · 1 line
2.10.1.530.g38c3896
Re: [PATCH] attr: convert to new threadsafe API
Hi Stefan,
Quoted 1 line · Stefan Beller
* use
attr_starton Windows to dynamically initialize the Single Big Attr Mutex
I would have preferred that call in common-main.c, but whatevs...
Thanks you for fixing the bug,
Dscho
Re: [PATCH] attr: convert to new threadsafe API
In reply to Stefan BellerQuoted 4 lines · Stefan Beller
Documentation/technical/api-gitattributes.txt +1
-1+* 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
Documentation/technical/api-gitattributes.txt value of the attribute for the path. · line 63
Quoted 10 lines
Documentation/technical/api-gitattributes.txt +1
-9- `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.
Documentation/technical/api-gitattributes.txt value of the attribute for the path. · line 73
Quoted 10 lines
Documentation/technical/api-gitattributes.txt +10
-0+ 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.
Documentation/technical/api-gitattributes.txt To see how attributes "crlf" and "ident" are set for different paths. · line 97
Quoted 2 lines
Documentation/technical/api-gitattributes.txt +2
-0+ static struct git_attr_check *check; + git_attr_check_initl(&check, "crlf", "ident", NULL);
OK.
Documentation/technical/api-gitattributes.txt To see how attributes "crlf" and "ident" are set for different paths. · line 106
Quoted 4 lines
Documentation/technical/api-gitattributes.txt +2
-0const 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.
Documentation/technical/api-gitattributes.txt To see how attributes "crlf" and "ident" are set for different paths. · line 112
Quoted 5 lines
+. Act on
result.value[]:------------
Documentation/technical/api-gitattributes.txt +1
-1- const char *value = check->check[0].value; + const char *value = result.value[0];
OK.
Documentation/technical/api-gitattributes.txt the first step in the above would be different. · line 146
Quoted 13 lines
Documentation/technical/api-gitattributes.txt +3
-0@@ -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.
Quoted 5 lines
Documentation/technical/api-gitattributes.txt +5
-0+* 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. +
Documentation/technical/api-gitattributes.txt Querying All Attributes · line 170
Quoted 10 lines
Documentation/technical/api-gitattributes.txt +9
-0+------------ + 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.
Re: [PATCH] attr: convert to new threadsafe API
Quoted 6 lines · Junio C Hamano
Documentation/technical/api-gitattributes.txt the first step in the above would be different. · line 143
Quoted 6 lines
Documentation/technical/api-gitattributes.txt +2
-0+ 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 17 lines
Documentation/technical/api-gitattributes.txt the first step in the above would be different. · line 146
Quoted 3 lines
Documentation/technical/api-gitattributes.txt +1
-0argv++; } + 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();
Quoted 11 lines
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.
Re: [PATCH] attr: convert to new threadsafe API
In reply to Stefan BellerAm 28.10.2016 um 00:15 schrieb Stefan Beller:
Quoted 1 line
* use
attr_starton Windows to dynamically initialize the Single Big Attr Mutex
I'm sorry, I didn't find time to test the patch on Windows. I won't be back at my Windows box before Wednesday.
-- Hannes
Re: [PATCH 27/36] attr: convert to new threadsafe API
In reply to Jeff KingQuoted 11 lines · Jeff King
Quoted 5 lines · Johannes Sixt
Am 26.10.2016 um 21:51 schrieb Stefan Beller:
Quoted 2 lines
it is
very convenient to not have to explicitly initialize mutexes?Not to initialize a mutex is still wrong for pthreads.
I think Stefan was being loose with his wording. There would still be an initializer, but it would be a constant (and in the case of pthread emulation on Windows, would just be NULL).
Exactly, so we would do
/* as per the man page of pthread_mutexes: */ pthread_mutex_t mymutex = PTHREAD_MUTEX_INITIALIZER;
int somefunction()
{
pthread_mutex_lock(&mymutex); /* threadsafely initialised on first use */
...
pthread_unlock(&mymutex);
}and for the Windows compat we'd do
#define PTHREAD_MUTEX_INITIALIZER NULL #define pthread_mutex_lock emulate_pthread_mutex_lock
int emulate_pthread_mutex_lock(volatile MUTEX_TYPE *mx)
{
if (*mx == NULL) /* static initializer? */
{ /* this stackoverflow magic to initialize threadsafely if not init'd */}
EnterCriticalSection(mx) /* as it currently is in compat/win32/pthread.h */
return 0;
}Quoted 2 lines
-Peff
Re: [PATCH 27/36] attr: convert to new threadsafe API
In reply to Stefan BellerQuoted 8 lines · 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);
Two cents. I read the .txt first and my first thought was "is _initl a typo, shouldn't it be jsut _init"? I know we have this 'l' variant at least in argv-array, but there we have many ways of adding arguments. And here it's just "initl", not "init" nor other "initX" variants, which looks odd. I wonder if the name git_attr_check_init() would do the job fine, since we don't have different init variants and the naming convention is not strong enough to tell me "it's multiple arguments ended with a NULL one" right away. If you're worried about people forgetting NULL at the end, how about passing an array of strings, terminated by NULL, instead?
Just thinking out loud. Maybe _initl _is_ a better name.
Signature · 1 line
Duy
PATCH v2 28/36 attr: keep attr stack for each check
In reply to Stefan BellerInstead of having a global attr stack, attach the stack to each check. This allows to use the attr in a multithreaded way.
--- attr.c | 101 +++++++++++++++++++++++++++++++++++++++----------------------- attr.h | 4 ++- hashmap.h | 2 ++ 3 files changed, 69 insertions(+), 38 deletions(-)
attr.c +64 -37
@@ -372,15 +372,17 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,* .gitignore file and info/excludes file as a fallback. */ -/* NEEDSWORK: This will become per git_attr_check */ -static struct attr_stack { +struct attr_stack {struct attr_stack *prev;char *origin;size_t originlen;unsigned num_matches;unsigned alloc;struct match_attr **attrs; -} *attr_stack; +}; + +struct hashmap all_attr_stacks; +int all_attr_stacks_init;static void free_attr_elem (struct attr_stack *e) {
@@ -561,11 +563,23 @@ static void debug_set(const char *what, const char *match, struct git_attr *attrstatic void drop_attr_stack (void ) { -while (attr_stack) { -struct attr_stack *elem = attr_stack; - attr_stack = elem->prev; -free_attr_elem (elem); +struct hashmap_iter iter; +struct git_attr_check *check; + +attr_lock (); +if (!all_attr_stacks_init) { +attr_unlock (); +return ; } +hashmap_iter_init (&all_attr_stacks, &iter); +while ((check =hashmap_iter_next (&iter))) { +while (check->attr_stack) { +struct attr_stack *elem = check->attr_stack; + check->attr_stack = elem->prev; +free_attr_elem (elem); + } + } +attr_unlock (); }static const char *git_etc_gitattributes (void )
@@ -595,40 +609,42 @@ static void push_stack(struct attr_stack **attr_stack_p, } } -static void bootstrap_attr_stack (void ) +static void bootstrap_attr_stack (struct git_attr_check *check) {struct attr_stack *elem; -if (attr_stack) +if (check->attr_stack)return ; -push_stack (&attr_stack,read_attr_from_array (builtin_attr),NULL ,0 ); +push_stack (&check->attr_stack, +read_attr_from_array (builtin_attr),NULL ,0 );if (git_attr_system ()) -push_stack (&attr_stack, +push_stack (&check->attr_stack,read_attr_from_file (git_etc_gitattributes (),1 ),NULL ,0 );if (!git_attributes_file) git_attributes_file =xdg_config_home ("attributes" );if (git_attributes_file) -push_stack (&attr_stack, +push_stack (&check->attr_stack,read_attr_from_file (git_attributes_file,1 ),NULL ,0 );if (!is_bare_repository () || direction ==GIT_ATTR_INDEX ) { elem =read_attr (GITATTRIBUTES_FILE ,1 ); -push_stack (&attr_stack, elem,xstrdup ("" ),0 ); +push_stack (&check->attr_stack, elem,xstrdup ("" ),0 );debug_push (elem); } elem =read_attr_from_file (git_path_info_attributes (),1 );if (!elem) elem =xcalloc (1 ,sizeof (*elem)); -push_stack (&attr_stack, elem,NULL ,0 ); +push_stack (&check->attr_stack, elem,NULL ,0 ); } -static void prepare_attr_stack (const char *path,int dirlen) +static void prepare_attr_stack (const char *path,int dirlen, +struct git_attr_check *check) {struct attr_stack *elem, *info;const char *cp;
@@ -648,13 +664,13 @@ static void prepare_attr_stack(const char *path, int dirlen)* .gitattributes in deeper directories to shallower ones, * and finally use the built-in set as the default. */ -bootstrap_attr_stack (); +bootstrap_attr_stack (check);/* * Pop the "info" one that is always at the top of the stack. */ - info = attr_stack; - attr_stack = info->prev; + info = check->attr_stack; + check->attr_stack = info->prev;/* * Pop the ones from directories that are not the prefix of
@@ -662,17 +678,17 @@ static void prepare_attr_stack(const char *path, int dirlen)* the root one (whose origin is an empty string "") or the builtin * one (whose origin is NULL) without popping it. */ -while (attr_stack->origin) { -int namelen =strlen (attr_stack->origin); +while (check->attr_stack->origin) { +int namelen =strlen (check->attr_stack->origin); - elem = attr_stack; + elem = check->attr_stack;if (namelen <= dirlen && !strncmp (elem->origin, path, namelen) && (!namelen || path[namelen] =='/' ))break ;debug_pop (elem); - attr_stack = elem->prev; + check->attr_stack = elem->prev;free_attr_elem (elem); }
@@ -688,9 +704,9 @@ static void prepare_attr_stack(const char *path, int dirlen)*/ struct strbuf pathbuf =STRBUF_INIT ; -assert (attr_stack->origin); +assert (check->attr_stack->origin);while (1 ) { -size_t len =strlen (attr_stack->origin); +size_t len =strlen (check->attr_stack->origin);char *origin;if (dirlen <= len)
@@ -704,7 +720,7 @@ static void prepare_attr_stack(const char *path, int dirlen) elem =read_attr (pathbuf.buf,0 );strbuf_setlen (&pathbuf, cp - path); origin =strbuf_detach (&pathbuf, &len); -push_stack (&attr_stack, elem, origin, len); +push_stack (&check->attr_stack, elem, origin, len);debug_push (elem); }
@@ -714,7 +730,13 @@ static void prepare_attr_stack(const char *path, int dirlen)/* * Finally push the "info" one at the top of the stack. */ -push_stack (&attr_stack, info,NULL ,0 ); +push_stack (&check->attr_stack, info,NULL ,0 ); +if (!all_attr_stacks_init) { +hashmap_init (&all_attr_stacks,NULL ,0 ); + all_attr_stacks_init =1 ; + } +if (!hashmap_get (&all_attr_stacks, check,NULL )) +hashmap_put (&all_attr_stacks, check); } static int path_matches(const char *pathname, int pathlen,
@@ -740,9 +762,10 @@ static int path_matches(const char *pathname, int pathlen, pattern, prefix, pat->patternlen, pat->flags); } -static int macroexpand_one (int attr_nr,int rem); +static int macroexpand_one (int attr_nr,int rem,struct git_attr_check *check); -static int fill_one (const char *what,struct match_attr *a,int rem) +static int fill_one (const char *what,struct match_attr *a,int rem, +struct git_attr_check *check) {struct git_attr_check_elem *celem = check_all_attr;int i;
@@ -758,14 +781,14 @@ static int fill_one(const char *what, struct match_attr *a, int rem) attr, v); *n = v; rem--; - rem =macroexpand_one (attr->attr_nr, rem); + rem =macroexpand_one (attr->attr_nr, rem, check); } }return rem; }static int fill (const char *path,int pathlen,int basename_offset, -struct attr_stack *stk,int rem) +struct attr_stack *stk,int rem,struct git_attr_check *check) {int i;const char *base = stk->origin ? stk->origin :"" ;
@@ -776,12 +799,12 @@ static int fill(const char *path, int pathlen, int basename_offset,continue ;if (path_matches (path, pathlen, basename_offset, &a->u.pat, base, stk->originlen)) - rem =fill_one ("fill" , a, rem); + rem =fill_one ("fill" , a, rem, check); }return rem; } -static int macroexpand_one (int nr,int rem) +static int macroexpand_one (int nr,int rem,struct git_attr_check *check) {struct attr_stack *stk;int i;
@@ -790,13 +813,13 @@ static int macroexpand_one(int nr, int rem) !check_all_attr[nr].attr->maybe_macro)return rem; -for (stk = attr_stack; stk; stk = stk->prev) { +for (stk = check->attr_stack; stk; stk = stk->prev) {for (i = stk->num_matches -1 ;0 <= i; i--) {struct match_attr *ma = stk->attrs[i];if (!ma->is_macro)continue ;if (ma->u.attr->attr_nr == nr) -return fill_one ("expand" , ma, rem); +return fill_one ("expand" , ma, rem, check); } }
@@ -845,7 +868,7 @@ static void collect_some_attrs(const char *path, int pathlen, dirlen =0 ; } -prepare_attr_stack (path, dirlen); +prepare_attr_stack (path, dirlen, check);for (i =0 ; i < attr_nr; i++) check_all_attr[i].value =ATTR__UNKNOWN ;
@@ -865,8 +888,8 @@ static void collect_some_attrs(const char *path, int pathlen, } rem = attr_nr; -for (stk = attr_stack;0 < rem && stk; stk = stk->prev) - rem =fill (path, pathlen, basename_offset, stk, rem); +for (stk = check->attr_stack;0 < rem && stk; stk = stk->prev) + rem =fill (path, pathlen, basename_offset, stk, rem, check);if (collect_all) {int check_nr =0 , check_alloc =0 ;
@@ -898,6 +921,8 @@ static int git_check_attrs(const char *path, int pathlen, {int i; +attr_lock (); +collect_some_attrs (path, pathlen, check, &result,0 );for (i =0 ; i < check->check_nr; i++) {
@@ -907,6 +932,8 @@ static int git_check_attrs(const char *path, int pathlen, result[i].value = value; } +attr_unlock (); +return 0 ; }
attr.h +3 -1
@@ -32,12 +32,14 @@ extern const char git_attr__false[];#define ATTR_UNSET (v) ((v) == NULL)struct git_attr_check { +struct hashmap_entry entry;int finalized;int check_nr;int check_alloc;const struct git_attr **attr; +struct attr_stack *attr_stack; }; -#define GIT_ATTR_CHECK_INIT {0, 0, 0, NULL} +#define GIT_ATTR_CHECK_INIT {HASHMAP_ENTRY_INIT, 0, 0, 0, NULL, NULL}struct git_attr_result {const char *value;
hashmap.h +2 -0
@@ -31,6 +31,8 @@ struct hashmap_entry {unsigned int hash; }; +#define HASHMAP_ENTRY_INIT {NULL, 0} +typedef int (*hashmap_cmp_fn )(const void *entry,const void *entry_or_key,const void *keydata);
Signature · 1 line
2.10.1.508.g6572022
Re: [PATCH 28/36] attr: keep attr stack for each check
Quoted 24 lines · Stefan Beller
Instead of having a global attr stack, attach the stack to each check. This allows to use the attr in a multithreaded way.
Signed-off-by Stefan Beller <[email protected]>--- attr.c | 101 +++++++++++++++++++++++++++++++++++++++----------------------- attr.h | 4 ++- hashmap.h | 2 ++ 3 files changed, 69 insertions(+), 38 deletions(-)attr.c +1
-2@@ -372,15 +372,17 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,* .gitignore file and info/excludes file as a fallback. */ -/* NEEDSWORK: This will become per git_attr_check */ -static struct attr_stack { +struct attr_stack {struct attr_stack *prev;
attr.c static struct match_attr *parse_attr_line(const char *line, const char *src, · line 385
Quoted 10 lines
attr.c +4
-1char *origin;size_t originlen;unsigned num_matches;unsigned alloc;struct match_attr **attrs; -} *attr_stack; +}; + +struct hashmap all_attr_stacks; +int all_attr_stacks_init;
Mark symbols 'all_attr_stacks' and 'all_attr_stacks_init' with the static keyword. (ie. these are file-local symbols).
ATB,
Ramsay Jones
Re: [PATCH 28/36] attr: keep attr stack for each check
Quoted 10 lines · Ramsay Jones
attr.c static struct match_attr *parse_attr_line(const char *line, const char *src, · line 385
Quoted 3 lines
attr.c +3
-0+ +struct hashmap all_attr_stacks; +int all_attr_stacks_init;Mark symbols '
all_attr_stacks' and 'all_attr_stacks_init' with the static keyword. (ie. these are file-local symbols).ATB,
Ramsay Jones
This fix will appear in a resend of the series.
Thanks,
Stefan
Re: [PATCH 28/36] attr: keep attr stack for each check
In reply to Stefan BellerQuoted 1 line · Stefan Beller
Instead of having a global attr stack, attach the stack to each check.
Two threads may be working on "git checkout", one "git_attr_check" in convert.c may be used by them to learn the EOL conversion for each path, and these threads are working in different parts of worktree in parallel. The set of .gitattributes files each of these threads wants to be cached at one time is tied to where in the directory hierarchy the thread is working in.
The view by API users would not have to change from the point on since 27/36 or so, but I think attr_stack needs to become per <check, thread> tuple when we are fully thread-ready for the above reason.
But we need to start somewhere to move away from the current "one single attr stack" to "there are multiple attr stacks", and this "two checks may and do use different attr stacks" is probably a reasonable first step. It may give a single-threaded API users immediate benefit if the "read and keep only the entries relevant to the query" optimization is done with this step alone, without making the cache per <check, thread> pair.
Quoted 1 line
This allows to use the attr in a multithreaded way.
With manipulation of attr stack protected with a single Big Attributes Lock, this should be safe. It may not perform very well when used by multiple threads, though ;-)
Re: [PATCH 28/36] attr: keep attr stack for each check
Quoted 11 lines · Junio C Hamano
Quoted 1 line · Stefan Beller
Instead of having a global attr stack, attach the stack to each check.
Two threads may be working on "git checkout", one "
git_attr_check" inconvert.cmay be used by them to learn the EOL conversion for each path, and these threads are working in different parts of worktree in parallel. The set of .gitattributes files each of these threads wants to be cached at one time is tied to where in the directory hierarchy the thread is working in.
Quoted 4 lines
The view by API users would not have to change from the point on since 27/36 or so, but I think
attr_stackneeds to become per <check, thread> tuple when we are fully thread-ready for the above reason.
I looked for a platform independent way to get a thread id as a natural number, i.e. I want to get 1,2,3,... such that I could have just added list/array of attr stacks to each check, which would be the <check, thread> tuple you envision.
However I think we do not really need it to be per check. If we had an easy portable way of getting such a thread id, I would have implemented a list of stacks per thread first. (Because each thread only looks at one check at a time.)
So this is not a baby step because I did not want to do it all at once, but because I did not find a suitable API to use.
Quoted 9 lines
But we need to start somewhere to move away from the current "one single attr stack" to "there are multiple attr stacks", and this "two checks may and do use different attr stacks" is probably a reasonable first step. It may give a single-threaded API users immediate benefit if the "read and keep only the entries relevant to the query" optimization is done with this step alone, without making the cache per <check, thread> pair.
Quoted 5 lines
Quoted 1 line
This allows to use the attr in a multithreaded way.
With manipulation of attr stack protected with a single Big Attributes Lock, this should be safe. It may not perform very well when used by multiple threads, though ;-)
I agree. So maybe it is not really a good fit for general consumption yet.
Re: [PATCH 28/36] attr: keep attr stack for each check
Quoted 9 lines · Stefan Beller
I looked for a platform independent way to get a thread id as a natural number, i.e. I want to get 1,2,3,... such that I could have just added list/array of attr stacks to each check, which would be the <check, thread> tuple you envision.
However I think we do not really need it to be per check. If we had an easy portable way of getting such a thread id, I would have implemented a list of stacks per thread first. (Because each thread only looks at one check at a time.)
It seems that by "list of stacks per thread", you mean "there is a list of stacks, each thread uses one and only element of that list", but I do not think it would be desirable.
"Each thread only looks at one check at a time" is false. For example, "write_archive_entry()" would use one check that is specific to "git archive" to learn about "export-ignore" and "export-subst" attributes, while letting convert_to_write_tree() called via sha1_file_to_archive() called via write_entry() method (i.e. write_tar_entry() or write_zip_entry()) to use a separate check that is specific to the convert.c API.
Quoted 5 lines
Quoted 3 lines
With manipulation of attr stack protected with a single Big Attributes Lock, this should be safe. It may not perform very well when used by multiple threads, though ;-)
I agree. So maybe it is not really a good fit for general consumption yet.
I would still think it is a good first step. It may already be thread-safe, but may not be thread-ready from performance point of view. IOW, this would not yet help an attempt to make the callers faster by making them multi-threaded.
PATCH v2 29/36 Documentation: fix a typo
In reply to Stefan Beller--- Documentation/gitattributes.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Documentation/gitattributes.txt +1 -1
@@ -88,7 +88,7 @@ is either not set or empty, $HOME/.config/git/attributes is used instead. Attributes for all users on a system should be placed in the `$(prefix)/etc/gitattributes` file. -Sometimes you would need to override an setting of an attribute +Sometimes you would need to override a setting of an attribute for a path to `Unspecified` state. This can be done by listing the name of the attribute prefixed with an exclamation point `!`.
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 30/36 pathspec: move long magic parsing out of prefix_pathspec
In reply to Stefan Bellerprefix_pathspec is quite a lengthy function and we plan on adding more. Split it up for better readability. As we want to add code into the inner loop of the long magic parsing, we also benefit from lower indentation.
--- pathspec.c | 84 +++++++++++++++++++++++++++++++++++--------------------------- 1 file changed, 47 insertions(+), 37 deletions(-)
pathspec.c +47 -37
@@ -88,6 +88,52 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,strbuf_addf (sb,",prefix:%d)" , prefixlen); } +static void eat_long_magic (struct pathspec_item *item,const char *elt, +unsigned *magic,int *pathspec_prefix, +const char **copyfrom_,const char **long_magic_end) +{ +int i; +const char *copyfrom = *copyfrom_; +/* longhand */ +const char *nextat; +for (copyfrom = elt +2 ; + *copyfrom && *copyfrom !=')' ; + copyfrom = nextat) { +size_t len =strcspn (copyfrom,",)" ); +if (copyfrom[len] ==',' ) + nextat = copyfrom + len +1 ; +else +/* handle ')' and '\0' */ + nextat = copyfrom + len; +if (!len) +continue ; +for (i =0 ; i <ARRAY_SIZE (pathspec_magic); i++) { +if (strlen (pathspec_magic[i].name) == len && + !strncmp (pathspec_magic[i].name, copyfrom, len)) { + *magic |= pathspec_magic[i].bit; +break ; + } +if (starts_with (copyfrom,"prefix:" )) { +char *endptr; + *pathspec_prefix =strtol (copyfrom +7 , + &endptr,10 ); +if (endptr - copyfrom != len) +die (_ ("invalid parameter for pathspec magic 'prefix'" )); +/* "i" would be wrong, but it does not matter */ +break ; + } + } +if (ARRAY_SIZE (pathspec_magic) <= i) +die (_ ("Invalid pathspec magic '%.*s' in '%s'" ), + (int ) len, copyfrom, elt); + } +if (*copyfrom !=')' ) +die (_ ("Missing ')' at the end of pathspec magic in '%s'" ), elt); + *long_magic_end = copyfrom; + copyfrom++; + *copyfrom_ = copyfrom; +} +/* * Take an element of a pathspec and check for magic signatures. * Append the result to the prefix. Return the magic bitmap.
@@ -150,43 +196,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, (flags & PATHSPEC_LITERAL_PATH)) { ;/* nothing to do */ } else if (elt[1] == '(') { -/* longhand */ -const char *nextat; -for (copyfrom = elt +2 ; - *copyfrom && *copyfrom !=')' ; - copyfrom = nextat) { -size_t len =strcspn (copyfrom,",)" ); -if (copyfrom[len] ==',' ) - nextat = copyfrom + len +1 ; -else -/* handle ')' and '\0' */ - nextat = copyfrom + len; -if (!len) -continue ; -for (i =0 ; i <ARRAY_SIZE (pathspec_magic); i++) { -if (strlen (pathspec_magic[i].name) == len && - !strncmp (pathspec_magic[i].name, copyfrom, len)) { - magic |= pathspec_magic[i].bit; -break ; - } -if (starts_with (copyfrom,"prefix:" )) { -char *endptr; - pathspec_prefix =strtol (copyfrom +7 , - &endptr,10 ); -if (endptr - copyfrom != len) -die (_ ("invalid parameter for pathspec magic 'prefix'" )); -/* "i" would be wrong, but it does not matter */ -break ; - } - } -if (ARRAY_SIZE (pathspec_magic) <= i) -die (_ ("Invalid pathspec magic '%.*s' in '%s'" ), - (int ) len, copyfrom, elt); - } -if (*copyfrom !=')' ) -die (_ ("Missing ')' at the end of pathspec magic in '%s'" ), elt); - long_magic_end = copyfrom; - copyfrom++; +eat_long_magic (item, elt, &magic, &pathspec_prefix, ©from, &long_magic_end); } else {/* shorthand */ for (copyfrom = elt + 1;
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 31/36 pathspec: move prefix check out of the inner loop
In reply to Stefan BellerThe prefix check is not related the check of pathspec magic; also there is no code that is relevant after we'd break the loop on a match for "prefix:". So move the check before the loop and shortcircuit the outer loop.
--- pathspec.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
pathspec.c +10 -9
@@ -107,21 +107,22 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt, nextat = copyfrom + len;if (!len)continue ; + +if (starts_with (copyfrom,"prefix:" )) { +char *endptr; + *pathspec_prefix =strtol (copyfrom +7 , + &endptr,10 ); +if (endptr - copyfrom != len) +die (_ ("invalid parameter for pathspec magic 'prefix'" )); +continue ; + } +for (i =0 ; i <ARRAY_SIZE (pathspec_magic); i++) {if (strlen (pathspec_magic[i].name) == len && !strncmp (pathspec_magic[i].name, copyfrom, len)) { *magic |= pathspec_magic[i].bit;break ; } -if (starts_with (copyfrom,"prefix:" )) { -char *endptr; - *pathspec_prefix =strtol (copyfrom +7 , - &endptr,10 ); -if (endptr - copyfrom != len) -die (_ ("invalid parameter for pathspec magic 'prefix'" )); -/* "i" would be wrong, but it does not matter */ -break ; - } }if (ARRAY_SIZE (pathspec_magic) <= i) die(_("Invalid pathspec magic '%.*s' in '%s'"),
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 32/36 pathspec: allow querying for attributes
In reply to Stefan BellerThe pathspec mechanism is extended via the new ":(attr:eol=input)pattern/to/match" syntax to filter paths so that it requires paths to not just match the given pattern but also have the specified attrs attached for them to be chosen.
--- Documentation/glossary-content.txt | 20 +++++ dir.c | 35 ++++++++ pathspec.c | 103 +++++++++++++++++++++- pathspec.h | 16 ++++ t/t6134-pathspec-with-labels.sh | 170 +++++++++++++++++++++++++++++++++++++ 5 files changed, 340 insertions(+), 4 deletions(-) create mode 100755 t/t6134-pathspec-with-labels.sh
Documentation/glossary-content.txt +20 -0
@@ -384,6 +384,26 @@ full pathname may have special meaning: + Glob magic is incompatible with literal magic. +attr;; +After `attr:` comes a space separated list of "attribute +requirements", all of which must be met in order for the +path to be considered a match; this is in addition to the +usual non-magic pathspec pattern matching. ++ +Each of the attribute requirements for the path takes one of +these forms: + +- "`ATTR`" requires that the attribute `ATTR` must be set. + +- "`-ATTR`" requires that the attribute `ATTR` must be unset. + +- "`ATTR=VALUE`" requires that the attribute `ATTR` must be + set to the string `VALUE`. + +- "`!ATTR`" requires that the attribute `ATTR` must be + unspecified. ++ + exclude;; After a path matches any non-exclude pathspec, it will be run through all exclude pathspec (magic signature: `!`). If it
dir.c +35 -0
@@ -9,6 +9,7 @@*/ #include "cache.h" #include "dir.h" +#include "attr.h" #include "refs.h" #include "wildmatch.h" #include "pathspec.h"
@@ -207,6 +208,37 @@ int within_depth(const char *name, int namelen,return 1 ; } +static int match_attrs (const char *name,int namelen, +const struct pathspec_item *item) +{ +int i; +struct git_attr_result *res =git_attr_result_alloc (item->attr_check); + +git_check_attr (name, item->attr_check, res); +for (i =0 ; i < item->attr_match_nr; i++) { +const char *value; +int matched; +enum attr_match_mode match_mode; + + value = res[i].value; + match_mode = item->attr_match[i].match_mode; + +if (ATTR_TRUE (value)) + matched = (match_mode ==MATCH_SET ); +else if (ATTR_FALSE (value)) + matched = (match_mode ==MATCH_UNSET ); +else if (ATTR_UNSET (value)) + matched = (match_mode ==MATCH_UNSPECIFIED ); +else + matched = (match_mode ==MATCH_VALUE && + !strcmp (item->attr_match[i].value, value)); +if (!matched) +return 0 ; + } + +return 1 ; +} +#define DO_MATCH_EXCLUDE 1#define DO_MATCH_DIRECTORY 2
@@ -262,6 +294,9 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix, strncmp(item->match, name - prefix, item->prefix))return 0 ; +if (item->attr_match_nr && !match_attrs (name, namelen, item)) +return 0 ; +/* If the match was just the prefix, we matched */ if (!*match)return MATCHED_RECURSIVELY ;
pathspec.c +99 -4
151 lines not on this page · raw message
pathspec.h +16 -0
@@ -32,6 +32,22 @@ struct pathspec {int len, prefix;int nowildcard_len;int flags; +int attr_match_nr; +int attr_match_alloc; +struct attr_match { +struct git_attr *attr; +char *value; +enum attr_match_mode { +MATCH_SET , +MATCH_UNSET , +MATCH_VALUE , +MATCH_UNSPECIFIED , +MATCH_NOT_UNSPECIFIED , +MATCH_SET_OR_VALUE , +INVALID_ATTR + } match_mode; + } *attr_match; +struct git_attr_check *attr_check; } *items; };
t/t6134-pathspec-with-labels.sh new file +170 -0
170 lines not on this page · raw message
Signature · 1 line
2.10.1.508.g6572022
Re: [PATCH 32/36] pathspec: allow querying for attributes
(sorry if this should have been answered if I went through the series patch by patch, I wanted to do a proper review but finally have to admit to myself I won't, so I just skim through a single giant diff instead)
Documentation/glossary-content.txt full pathname may have special meaning:
Quoted 4 lines · Stefan Beller
Documentation/glossary-content.txt +4
-0+attr;; +After `attr:` comes a space separated list of "attribute +requirements", all of which must be met in order for the +path to be considered a match;
What about (attr=abc def,attr=ghi lkj)? Does it mean (abc && def) || (ghi && lkj), or abc && def && ghi && lkj? Or is it forbidden to have multiple 'attr' attribute in the same pathspec?
Signature · 1 line
Duy
Re: [PATCH 32/36] pathspec: allow querying for attributes
Quoted 6 lines · Duy Nguyen
(sorry if this should have been answered if I went through the series patch by patch, I wanted to do a proper review but finally have to admit to myself I won't, so I just skim through a single giant diff instead)
On Sun, Oct 23, 2016 at 6:32 AM, Stefan Beller <[email protected]> wrote:
Quoted 8 lines
Documentation/glossary-content.txt full pathname may have special meaning:
Quoted 4 lines
Documentation/glossary-content.txt +4
-0+attr;; +After `attr:` comes a space separated list of "attribute +requirements", all of which must be met in order for the +path to be considered a match;What about (attr=abc def,attr=ghi lkj)? Does it mean (abc && def) || (ghi && lkj), or abc && def && ghi && lkj? Or is it forbidden to have multiple 'attr' attribute in the same pathspec?
Good point. I'll add a test for that.
Remembering the original discussion, multiple attrs are forbidden for now as it is unclear what you want to see as a user.
To model (abc && def) || (ghi && lkj), you would need to give multiple pathspec items as these are naturally ORed:
git ls-files :(attr:abc def) :(attr:ghi lkj) .
(compare "git ls-files Makefile README" which gives 2 files to you, that are named respectively.)
To get "abc && def && ghi && lkj" you go with
git ls-files :(attr:abc def ghi lkj) .
as then all things included into this one attr are ANDed. I hope the documentation is clear for one attr.
Thanks,
Stefan
Re: [PATCH 32/36] pathspec: allow querying for attributes
In reply to Stefan BellerQuoted 4 lines · Stefan Beller
The pathspec mechanism is extended via the new ":(attr:eol=input)pattern/to/match" syntax to filter paths so that it requires paths to not just match the given pattern but also have the specified attrs attached for them to be chosen.
I was looking at preload-index.c and its history again, because I wanted to ensure that Lars's process filter stuff introduces no unexpected interactions with getting multi-threaded, similar to the problem we had in the earlier incarnations of this step, which we worked around with <[email protected]> "pathspec: disable preload-index when attribute pathspec magic is in use".
I think that Lars's series is safe, because the only thing among what preload-index.c::preload_thread() does that can go outside the simple stat data and pattern matching with the pathname is the racy-git check in ie_match_stat(), and the object store access in that call was explicitly disabled by 7c4ea599b0 ("Fix index preloading for racy dirty case", 2008-11-17) long time ago.
By passing CE_MATCH_RACY_IS_DIRTY option to the call, this caller effectively says "A cache entry whose stat data matches may be actually dirty when the timestamp is racy, in which case we usually compare data to determine if it really is clean, but it is OK to err on the safe and lazy side by declaring it dirty and not marking it up-to-date while we are preloading. Do not bother to go to the object store".
The reason why I am bringing this up in this discussion thread on this patch is because I wonder if we would benefit by a similar "let's not do too involved things and be cheap by erring on the safe and lazy side" strategy in the call to ce_path_match() call made in this function to avoid making calls to the attr subsystem.
In other words, would it help the system by either simplifying the processing done or reducing the cycle spent in preload_thread() if we could tell ce_path_match() "A pathspec we are checking may require not just the pattern to match but also attributes given to the path to satisfy the criteria, but for the purpose of preloading, pretend that the attribute satisfies the match criteria" (or "pretend that it does not match"), thereby not having to make any call into the attribute subsystem at all from this codepath?
The strategy this round takes to make it unnecessary to punt preloading (i.e. dropping "pathspec: disable preload-index when attribute pathspec magic is in use" patch the old series had) is to make the attribute subsystem thread-safe. But that thread-safety in the initial round is based on a single Big Attribute Lock, so it may turn out that the end result performs better for this codepath if we did not to make any call into the attribute subsystem.
I am probably being two step ahead of ourselves by saying the above, which is just something to keep in mind as a possible solution if performance in this preload codepath becomes an issue when the pathspec has attributes match specified.
Re: [PATCH 32/36] pathspec: allow querying for attributes
Quoted 15 lines · Junio C Hamano
The reason why I am bringing this up in this discussion thread on this patch is because I wonder if we would benefit by a similar "let's not do too involved things and be cheap by erring on the safe and lazy side" strategy in the call to
ce_path_match()call made in this function to avoid making calls to the attr subsystem.In other words, would it help the system by either simplifying the processing done or reducing the cycle spent in
preload_thread()if we could tellce_path_match()"A pathspec we are checking may require not just the pattern to match but also attributes given to the path to satisfy the criteria, but for the purpose of preloading, pretend that the attribute satisfies the match criteria" (or "pretend that it does not match"), thereby not having to make any call into the attribute subsystem at all from this codepath?
Quoted 7 lines
The strategy this round takes to make it unnecessary to punt preloading (i.e. dropping "pathspec: disable preload-index when attribute pathspec magic is in use" patch the old series had) is to make the attribute subsystem thread-safe. But that thread-safety in the initial round is based on a single Big Attribute Lock, so it may turn out that the end result performs better for this codepath if we did not to make any call into the attribute subsystem.
It does sound good and I want to say "yes please do this", but is it making pathspec api a bit more complex (to express "assume all attr-related criteria match")? I guess we can have an api to simply filter out attr-related magic (basically set attr_match_nr back to zero) then pass a safe (but more relaxing) pathspec to the threaded code. That would not add big maintenance burden.
Quoted 4 lines
I am probably being two step ahead of ourselves by saying the above, which is just something to keep in mind as a possible solution if performance in this preload codepath becomes an issue when the pathspec has attributes match specified.
Signature · 1 line
Duy
Re: [PATCH 32/36] pathspec: allow querying for attributes
Quoted 24 lines · Duy Nguyen
Quoted 15 lines · Junio C Hamano
The reason why I am bringing this up in this discussion thread on this patch is because I wonder if we would benefit by a similar "let's not do too involved things and be cheap by erring on the safe and lazy side" strategy in the call to
ce_path_match()call made in this function to avoid making calls to the attr subsystem.In other words, would it help the system by either simplifying the processing done or reducing the cycle spent in
preload_thread()if we could tellce_path_match()"A pathspec we are checking may require not just the pattern to match but also attributes given to the path to satisfy the criteria, but for the purpose of preloading, pretend that the attribute satisfies the match criteria" (or "pretend that it does not match"), thereby not having to make any call into the attribute subsystem at all from this codepath?Quoted 7 lines
The strategy this round takes to make it unnecessary to punt preloading (i.e. dropping "pathspec: disable preload-index when attribute pathspec magic is in use" patch the old series had) is to make the attribute subsystem thread-safe. But that thread-safety in the initial round is based on a single Big Attribute Lock, so it may turn out that the end result performs better for this codepath if we did not to make any call into the attribute subsystem.
Quoted 7 lines
It does sound good and I want to say "yes please do this", but is it making pathspec api a bit more complex (to express "assume all attr-related criteria match")? I guess we can have an api to simply filter out attr-related magic (basically set
attr_match_nrback to zero) then pass a safe (but more relaxing) pathspec to the threaded code. That would not add big maintenance burden.
So with the current implementation, we already have the shortcut as:
if (item->attr_match_nr && !match_attrs(name, namelen, item))
i.e. if attr_match_nr is zero, we do not even look at the mutexes and such, so I am not sure what you intend to say in this email?
Re: [PATCH 32/36] pathspec: allow querying for attributes
Quoted 18 lines · Stefan Beller
Quoted 10 lines · Duy Nguyen
Quoted 8 lines · Junio C Hamano
...
The strategy this round takes to make it unnecessary to punt preloading (i.e. dropping "pathspec: disable preload-index when attribute pathspec magic is in use" patch the old series had) is to make the attribute subsystem thread-safe. But that thread-safety in the initial round is based on a single Big Attribute Lock, so it may turn out that the end result performs better for this codepath if we did not to make any call into the attribute subsystem.Quoted 6 lines
It does sound good and I want to say "yes please do this", but is it making pathspec api a bit more complex (to express "assume all attr-related criteria match")? I guess we can have an api to simply filter out attr-related magic (basically set
attr_match_nrback to zero) then pass a safe (but more relaxing) pathspec to the threaded code. That would not add big maintenance burden.
Quoted 6 lines
So with the current implementation, we already have the shortcut as:
if (item->attr_match_nr && !match_attrs(name, namelen, item))i.e. if
attr_match_nris zero, we do not even look at the mutexes and such, so I am not sure what you intend to say in this email?
I am not sure what relevance the "we call into attribute subsystem only when there is any need to check attributes" obvious short-cut has to what is being discussed.
The issue is specific to what preloading is about. It is merely an attempt to run cheap checks that could be easily multi-threaded with multiple threads early in the program that we _know_ we would need to eventually refresh the index before doing some interesting work. A full refresh_index() will be done eventually, and because it needs to trigger thread-unsafe part of the API, it needs to be done in the main thread. Doing the preload allows us to mark index entries that do not have to be scanned again in the upcoming refresh_index() call. It is OK for preload-index.c::preload_thread() to skip and not mark some index entries (iow, its sole purpose is to leave a note in each index entry "this is fresh, you do not need to look at it again", and it can choose to skip an entry, which essentially means "this I didn't check, so you, refresh_index(), need to check yourself").
preload_thread() for example skips index entries that needs to trigger "racy Git avoidance" logic that is heavyweight (it has to go to the filesystem and the object store), and it is a sensible thing to skip because they are rather rare.
The message by Duy you are responding to was his response to me who wondered if the attribute based pathspec match also falls into the same category. Just like racy Git code was deemed too heavyweight to be called from preloading codepath and CE_MATCH_RACY_IS_DIRTY bit was added as a way to ask ie_match_stat() API to avoid it (and hence we are skipping, the caller is also telling "if you suspect a racy, without checking for real, just answer 'I cannot say it is clean/fresh'"), if we invent a new flag and pass it through match_pathspec() down to match_pathspec_item() and have that if() statement you quoted also skip match_attrs() for a pathspec element with attribute based narrowing (as we are skipping, the flag may also have say "instead of checking the attributes, just pretend that the path did not satisfy the attribute narrowing"), would it benefit the overall performance?
To answer Duy's "would it make sense to force the caller to create a new pathspec from an existing one by filtering out pathspec elements with attr-based narrowing?" question, I do not think it does.
PATCH v2 33/36 pathspec: allow escaped query values
In reply to Stefan BellerIn our own .gitattributes file we have attributes such as:
*.[ch] whitespace=indent,trail,space
When querying for attributes we want to be able to ask for the exact value, i.e.
git ls-files :(attr:whitespace=indent,trail,space)
should work, but the commas are used in the attr magic to introduce the next attr, such that this query currently fails with
fatal: Invalid pathspec magic 'trail' in ':(attr:whitespace=indent,trail,space)'
This change allows escaping characters by a backslash, such that the query
git ls-files :(attr:whitespace=indent\,trail\,space)
will match all path that have the value "indent,trail,space" for the whitespace attribute. To accomplish this, we need to modify two places. First eat_long_magic needs to not stop early upon seeing a comma or closing paren that is escaped. As a second step we need to remove any escaping from the attr value.
--- pathspec.c | 53 +++++++++++++++++++++++++++++++++++++---- t/t6134-pathspec-with-labels.sh | 10 ++++++++ 2 files changed, 58 insertions(+), 5 deletions(-)
pathspec.c +48 -5
@@ -89,12 +89,56 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,strbuf_addf (sb,",prefix:%d)" , prefixlen); } +static size_t strcspn_escaped (const char *s,const char *stop) +{ +const char *i; + +for (i = s; *i; i++) { +/* skip the escaped character */ +if (i[0 ] =='\\' && i[1 ]) { + i++; +continue ; + } + +if (strchr (stop, *i)) +break ; + } +return i - s; +} + +static inline int invalid_value_char (const char ch) +{ +if (isalnum (ch) ||strchr (",-_" , ch)) +return 0 ; +return -1 ; +} + +static char *attr_value_unescape (const char *value) +{ +const char *src; +char *dst, *ret; + + ret =xmallocz (strlen (value)); +for (src = value, dst = ret; *src; src++, dst++) { +if (*src =='\\' ) { +if (!src[1 ]) +die (_ ("Escape character '\\' not allowed as " +"last character in attr value" )); + src++; + } +if (invalid_value_char (*src)) +die ("cannot use '%c' for value matching" , *src); + *dst = *src; + } + *dst ='\0' ; +return ret; +} +static void parse_pathspec_attr_match (struct pathspec_item *item,const char *value) {struct string_list_item *si;struct string_list list =STRING_LIST_INIT_DUP ; -if (!value || !strlen (value))die (_ ("attr spec must not be empty" ));
@@ -131,10 +175,9 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *vaif (attr[attr_len] !='=' ) am->match_mode =MATCH_SET ;else { +const char *v = &attr[attr_len +1 ]; am->match_mode =MATCH_VALUE ; - am->value =xstrdup (&attr[attr_len +1 ]); -if (strchr (am->value,'\\' )) -die (_ ("attr spec values must not contain backslashes" )); + am->value =attr_value_unescape (v); }break ; }
@@ -166,7 +209,7 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,for (copyfrom = elt +2 ; *copyfrom && *copyfrom !=')' ; copyfrom = nextat) { -size_t len =strcspn (copyfrom,",)" ); +size_t len =strcspn_escaped (copyfrom,",)" );if (copyfrom[len] ==',' ) nextat = copyfrom + len +1 ;else
t/t6134-pathspec-with-labels.sh +10 -0
@@ -167,4 +167,14 @@ test_expect_success 'abort on asking for wrong magic' ' test_must_fail git ls-files . ":(attr:!label=foo)" ' +test_expect_success 'check attribute list' ' + cat <<-EOF >>.gitattributes && + * whitespace=indent,trail,space + EOF + cat .gitattributes && + git ls-files ":(attr:whitespace=indent\,trail\,space)" >actual && + git ls-files >expect && + test_cmp expect actual +' + test_done
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 34/36 submodule update: add `--init-default-path` switch
In reply to Stefan BellerThe new switch --init-default-path initializes the submodules which are configured in submodule.defaultUpdatePath instead of those given as command line arguments before updating. In the first implementation this is made incompatible with further command line arguments as it is unclear what the user means by
git submodule update --init --init-default-path <paths>
This new switch allows to record more complex patterns as it saves retyping them whenever you invoke update.
--- Documentation/config.txt | 5 ++++ Documentation/git-submodule.txt | 17 +++++++++---- git-submodule.sh | 21 +++++++++++++--- t/t7400-submodule-basic.sh | 53 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 7 deletions(-)
Documentation/config.txt +5 -0
@@ -2886,6 +2886,11 @@ submodule.alternateErrorStrategy as computed via `submodule.alternateLocation`. Possible values are `ignore`, `info`, `die`. Default is `die`. +submodule.defaultUpdatePath:: + Specifies a set of submodules to initialize when calling + `git submodule --init-default-group` by using the pathspec + syntax. + tag.forceSignAnnotated:: A boolean to specify whether annotated tags created should be GPG signed. If `--annotate` is specified on the command line, it takes
Documentation/git-submodule.txt +13 -4
@@ -14,10 +14,10 @@ SYNOPSIS 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...] 'git submodule' [--quiet] init [--] [<path>...] 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] <path>...) -'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch] - [--[no-]recommend-shallow] [-f|--force] [--rebase|--merge] - [--reference <repository>] [--depth <depth>] [--recursive] - [--jobs <n>] [--] [<path>...] +'git submodule' [--quiet] update [--init[-default-path]] [--remote] [-N|--no-fetch] + [--[no-]recommend-shallow] + [-f|--force] [--rebase|--merge] [--reference <repository>] + [--depth <depth>] [--recursive] [--jobs <n>] [--] [<path>...] 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>] [commit] [--] [<path>...] 'git submodule' [--quiet] foreach [--recursive] <command>
@@ -194,6 +194,10 @@ If the submodule is not yet initialized, and you just want to use the setting as stored in .gitmodules, you can automatically initialize the submodule with the `--init` option. +You can configure a set of submodules using pathspec syntax in +submodule.defaultUpdatePath you can use `--init-default-path` to initialize +those before updating. + If `--recursive` is specified, this command will recurse into the registered submodules, and update any nested submodules within. --
@@ -361,6 +365,11 @@ the submodule itself. Initialize all submodules for which "git submodule init" has not been called so far before updating. +--init-default-path:: + This option is only valid for the update command. + Initialize all submodules configured in "`submodule.defaultUpdatePath`" + that have not been updated before. + --name:: This option is only valid for the add command. It sets the submodule's name to the given string instead of defaulting to its path. The name
git-submodule.sh +18 -3
@@ -9,7 +9,7 @@ USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <re or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...] or: $dashless [--quiet] init [--] [<path>...] or: $dashless [--quiet] deinit [-f|--force] (--all| [--] <path>...) - or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--] [<path>...] + or: $dashless [--quiet] update [--init[-default-path]] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--] [<path>...] or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...] or: $dashless [--quiet] foreach [--recursive] <command> or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
@@ -503,7 +503,12 @@ cmd_update() progress="--progress" ;; -i|--init) - init=1 + test -z $init || test $init = by_args || die "$(gettext "Only one of --init or --init-default-path may be used.")" + init=by_args + ;; + --init-default-path) + test -z $init || test $init = by_config || die "$(gettext "Only one of --init or --init-default-path may be used.")" + init=by_config ;; --remote) remote=1
@@ -572,7 +577,17 @@ cmd_update() if test -n "$init" then - cmd_init "--" "$@" || return + if test "$init" = "by_config" + then + if test $# -gt 0 + then + die "$(gettext "path arguments are incompatible with --init-default-path")" + fi + cmd_init "--" $(git config --get-all submodule.defaultUpdatePath) || return + else + cmd_init "--" "$@" || return + fi + fi {
t/t7400-submodule-basic.sh +53 -0
58 lines not on this page · raw message
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 35/36 clone: add --init-submodule=<pathspec> switch
In reply to Stefan BellerThe new switch passes the pathspec to git submodule update --init which is called after the actual clone is done.
Additionally this configures the submodule.defaultUpdatePath to be the given pathspec, such that any future invocation of git submodule update --init-default-paths will keep up with the pathspec.
--- Documentation/git-clone.txt | 23 +++++++++---- builtin/clone.c | 36 ++++++++++++++++++-- t/t7400-submodule-basic.sh | 81 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 9 deletions(-)
Documentation/git-clone.txt +16 -7
@@ -15,7 +15,8 @@ SYNOPSIS [--dissociate] [--separate-git-dir <git dir>] [--depth <depth>] [--[no-]single-branch] [--recursive | --recurse-submodules] [--[no-]shallow-submodules] - [--jobs <n>] [--] <repository> [<directory>] + [--init-submodule <pathspec>] [--jobs <n>] [--] + <repository> [<directory>] DESCRIPTION -----------
@@ -217,12 +218,20 @@ objects from the source repository into a pack in the cloned repository. --recursive:: --recurse-submodules:: - After the clone is created, initialize all submodules within, - using their default settings. This is equivalent to running - `git submodule update --init --recursive` immediately after - the clone is finished. This option is ignored if the cloned - repository does not have a worktree/checkout (i.e. if any of - `--no-checkout`/`-n`, `--bare`, or `--mirror` is given) + After the clone is created, initialize and clone all submodules + within, using their default settings. This is equivalent to + running `git submodule update --recursive --init ` + immediately after the clone is finished. This option is ignored + if the cloned repository does not have a worktree/checkout (i.e. + if any of `--no-checkout`/`-n`, `--bare`, or `--mirror` is given) + +--init-submodule:: + After the clone is created, initialize and clone specified + submodules within, using their default settings. It is possible + to give multiple specifications by giving this argument multiple + times. This is equivalent to configure `submodule.defaultUpdateGroup` + and then running `git submodule update --init-default-path` + immediately after the clone is finished. --[no-]shallow-submodules:: All submodules which are cloned will be shallow with a depth of 1.
builtin/clone.c +34 -2
@@ -56,6 +56,16 @@ static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;static struct string_list option_optional_reference =STRING_LIST_INIT_NODUP ;static int option_dissociate;static int max_jobs =-1 ; +static struct string_list init_submodules; + +static int init_submodules_cb (const struct option *opt,const char *arg,int unset) +{ +if (unset) +return -1 ; + +string_list_append ((struct string_list *)opt->value, arg); +return 0 ; +}static struct option builtin_clone_options[] = {OPT__VERBOSITY (&option_verbosity),
@@ -112,6 +122,9 @@ static struct option builtin_clone_options[] = { TRANSPORT_FAMILY_IPV4),OPT_SET_INT ('6' ,"ipv6" , &family,N_ ("use IPv6 addresses only" ),TRANSPORT_FAMILY_IPV6 ), +OPT_CALLBACK (0 ,"init-submodule" , &init_submodules,N_ ("<pathspec>" ), +N_ ("clone specific submodules. Pass multiple times for complex pathspecs" ), + init_submodules_cb), OPT_END() };
@@ -733,13 +746,21 @@ static int checkout(int submodule_progress) err |=run_hook_le (NULL ,"post-checkout" ,sha1_to_hex (null_sha1),sha1_to_hex (sha1),"1" ,NULL ); -if (!err && option_recursive) { +if (!err && (option_recursive || init_submodules.nr >0 )) {struct argv_array args =ARGV_ARRAY_INIT ; -argv_array_pushl (&args,"submodule" ,"update" ,"--init" ,"--recursive" ,NULL ); +argv_array_pushl (&args,"submodule" ,"update" ,NULL ); + +if (init_submodules.nr >0 ) +argv_array_pushf (&args,"--init-default-path" ); +else +argv_array_pushf (&args,"--init" );if (option_shallow_submodules ==1 )argv_array_push (&args,"--depth=1" ); +if (option_recursive) +argv_array_pushf (&args,"--recursive" ); +if (max_jobs !=-1 )argv_array_pushf (&args,"--jobs=%d" , max_jobs);
@@ -887,6 +908,17 @@ int cmd_clone(int argc, const char **argv, const char *prefix) option_no_checkout =1 ; } +if (init_submodules.nr >0 ) { +struct string_list_item *item; +struct strbuf sb =STRBUF_INIT ; +for_each_string_list_item (item, &init_submodules) { +strbuf_addf (&sb,"submodule.defaultUpdatePath=%s" , + item->string); +string_list_append (&option_config, +strbuf_detach (&sb,NULL )); + } + } +if (!option_origin) option_origin ="origin" ;
t/t7400-submodule-basic.sh +81 -0
85 lines not on this page · raw message
Signature · 1 line
2.10.1.508.g6572022
PATCH v2 36/36 completion: clone can initialize specific submodules
In reply to Stefan Beller--- contrib/completion/git-completion.bash | 1 + 1 file changed, 1 insertion(+)
contrib/completion/git-completion.bash +1 -0
@@ -1138,6 +1138,7 @@ _git_clone () --single-branch --branch --recurse-submodules + --init-submodule " return ;;
Signature · 1 line
2.10.1.508.g6572022