From 9c9b03b1f1fa84f4a723aac962e1126d27de4a4f Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:41 -0800 Subject: commit.c: use strchrnul() to scan for one line Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/commit.c b/commit.c index 2cf8515..0c4ee3d 100644 --- a/commit.c +++ b/commit.c @@ -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; -- cgit v0.10.2-6-g49f6 From 263434f20c20b193c1ef956eda43458b7f350eaa Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:42 -0800 Subject: attr.c: use strchrnul() to scan for one line Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 1fcf042..04d2433 100644 --- a/attr.c +++ b/attr.c @@ -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); -- cgit v0.10.2-6-g49f6 From 4894f4ffce3cc6c4664027de78c3bc281cda2979 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:43 -0800 Subject: attr.c: update a stale comment on "struct match_attr" 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. Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 04d2433..007f1a2 100644 --- a/attr.c +++ b/attr.c @@ -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 -- cgit v0.10.2-6-g49f6 From 5a8840194b0525590f930163d72e87a0ab50ac0a Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:44 -0800 Subject: attr.c: explain the lack of attr-name syntax check in parse_attr() Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 007f1a2..6b55a57 100644 --- a/attr.c +++ b/attr.c @@ -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++; -- cgit v0.10.2-6-g49f6 From 6f416cbaff72cd13b1ff44110121672530e69231 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:45 -0800 Subject: attr.c: complete a sentence in a comment Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 6b55a57..9bdf87a 100644 --- a/attr.c +++ b/attr.c @@ -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 { -- cgit v0.10.2-6-g49f6 From 034d35c9aa8e925425730957af71ce3d077ff335 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:46 -0800 Subject: attr.c: mark where #if DEBUG ends more clearly Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 9bdf87a..17297ff 100644 --- a/attr.c +++ b/attr.c @@ -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) { -- cgit v0.10.2-6-g49f6 From 4b0c6961678b3feddd8974e8ad6c49c62da9e5cd Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:47 -0800 Subject: attr.c: simplify macroexpand_one() 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. Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 17297ff..e42f931 100644 --- a/attr.c +++ b/attr.c @@ -705,24 +705,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; } -- cgit v0.10.2-6-g49f6 From ec4d77aa508ac36f1f65ca8f228d4cbac42d694c Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:48 -0800 Subject: attr.c: tighten constness around "git_attr" structure 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. Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index e42f931..f7cf7ae 100644 --- a/attr.c +++ b/attr.c @@ -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; } diff --git a/attr.h b/attr.h index 8b08d33..00d7a66 100644 --- a/attr.h +++ b/attr.h @@ -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 *); -- cgit v0.10.2-6-g49f6 From 62af896979403c6bcb441ca784121e720b161b30 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:49 -0800 Subject: attr.c: plug small leak in parse_attr_line() 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. Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index f7cf7ae..d180c78 100644 --- a/attr.c +++ b/attr.c @@ -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; } /* -- cgit v0.10.2-6-g49f6 From 860a74d9d909002b16c50b58050908756065125d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Fri, 27 Jan 2017 18:01:50 -0800 Subject: attr: support quoting pathname patterns in C style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt index e0b66c1..3173dee 100644 --- a/Documentation/gitattributes.txt +++ b/Documentation/gitattributes.txt @@ -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: diff --git a/attr.c b/attr.c index d180c78..e1c630f 100644 --- a/attr.c +++ b/attr.c @@ -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"; @@ -212,12 +213,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) { @@ -239,7 +249,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 */ @@ -282,9 +291,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; } diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index f0fbb42..f19ae4f 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -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 && -- cgit v0.10.2-6-g49f6 From 4c0ce0742b19c2a430b809224911b9212313265f Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:51 -0800 Subject: attr.c: add push_stack() helper 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. Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index e1c630f..8026d68 100644 --- a/attr.c +++ b/attr.c @@ -510,6 +510,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; @@ -517,37 +529,23 @@ 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); } @@ -558,15 +556,12 @@ static void bootstrap_attr_stack(void) 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; /* @@ -626,20 +621,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); } @@ -649,8 +645,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, -- cgit v0.10.2-6-g49f6 From faa4e8ceb5c37dd6ae8c89a02b5e2651276fe0f1 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Fri, 27 Jan 2017 18:01:52 -0800 Subject: Documentation: fix a typo Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt index 3173dee..a53d093 100644 --- a/Documentation/gitattributes.txt +++ b/Documentation/gitattributes.txt @@ -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 `!`. -- cgit v0.10.2-6-g49f6 From 7d42ec547cb8edea8cb1c527a646cfd21aa1c295 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:53 -0800 Subject: attr.c: outline the future plans by heavily commenting Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 8026d68..50e5ee3 100644 --- a/attr.c +++ b/attr.c @@ -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 *check_all_attr; -static struct git_attr *(git_attr_hash[HASHSIZE]); const char *git_attr_name(const struct git_attr *attr) { @@ -102,6 +116,11 @@ static struct git_attr *git_attr_internal(const char *name, int 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; @@ -318,6 +337,7 @@ fail_return: * .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; @@ -382,6 +402,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; -- cgit v0.10.2-6-g49f6 From 7bd18054d249d1f9aebad38d74ba6c53f8e6ecb3 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:54 -0800 Subject: attr: rename function and struct related to checking attributes 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 attr_check_item" and rename the function to "git_check_attrs()". Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/archive.c b/archive.c index 01751e5..b76bd46 100644 --- a/archive.c +++ b/archive.c @@ -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 attr_check_item *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 attr_check_item 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); diff --git a/attr.c b/attr.c index 50e5ee3..2f180d6 100644 --- a/attr.c +++ b/attr.c @@ -56,7 +56,7 @@ static struct git_attr *(git_attr_hash[HASHSIZE]); static int cannot_trust_maybe_real; /* NEEDSWORK: This will become per git_attr_check */ -static struct git_attr_check *check_all_attr; +static struct attr_check_item *check_all_attr; const char *git_attr_name(const struct git_attr *attr) { @@ -713,7 +713,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 attr_check_item *check = check_all_attr; int i; for (i = a->num_attr - 1; 0 < rem && 0 <= i; i--) { @@ -778,7 +778,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 attr_check_item *check) { struct attr_stack *stk; @@ -806,7 +806,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 attr_check_item *c; c = check_all_attr + check[i].attr->attr_nr; c->value = ATTR__UNSET; rem++; @@ -821,7 +821,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 attr_check_item *check) { int i; @@ -837,7 +837,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 attr_check_item **check) { int i, count, j; diff --git a/attr.h b/attr.h index 00d7a66..efc7bb3 100644 --- a/attr.h +++ b/attr.h @@ -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 attr_check_item { 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 attr_check_item *); /* * 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 attr_check_item **check); enum git_attr_direction { GIT_ATTR_CHECKIN, diff --git a/builtin/check-attr.c b/builtin/check-attr.c index 53a5a18..889264a 100644 --- a/builtin/check-attr.c +++ b/builtin/check-attr.c @@ -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 attr_check_item *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 attr_check_item *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 attr_check_item *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 attr_check_item *check; int cnt, i, doubledash, filei; if (!is_bare_repository()) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 8841f8b..8b8fbd8 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -894,7 +894,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 attr_check_item *check) { static struct git_attr *attr_delta; @@ -906,10 +906,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 attr_check_item 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; diff --git a/convert.c b/convert.c index 4e17e45..1b98292 100644 --- a/convert.c +++ b/convert.c @@ -1028,7 +1028,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 attr_check_item *check) { const char *value = check->value; @@ -1045,7 +1045,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 attr_check_item *check) { const char *value = check->value; @@ -1058,7 +1058,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 attr_check_item *check) { const char *value = check->value; struct convert_driver *drv; @@ -1071,7 +1071,7 @@ static struct convert_driver *git_path_check_convert(struct git_attr_check *chec return NULL; } -static int git_path_check_ident(struct git_attr_check *check) +static int git_path_check_ident(struct attr_check_item *check) { const char *value = check->value; @@ -1093,7 +1093,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 attr_check_item ccheck[NUM_CONV_ATTRS]; if (!ccheck[0].attr) { for (i = 0; i < NUM_CONV_ATTRS; i++) @@ -1102,7 +1102,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); diff --git a/ll-merge.c b/ll-merge.c index ad8be42..198f07a 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -336,13 +336,13 @@ static const struct ll_merge_driver *find_ll_merge_driver(const char *merge_attr return &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 attr_check_item 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 attr_check_item 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 attr_check_item 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; diff --git a/userdiff.c b/userdiff.c index 2125d6d..b0b4446 100644 --- a/userdiff.c +++ b/userdiff.c @@ -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 attr_check_item 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)) diff --git a/ws.c b/ws.c index ea4b2b1..fbd876e 100644 --- a/ws.c +++ b/ws.c @@ -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 attr_check_item *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 attr_check_item 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; -- cgit v0.10.2-6-g49f6 From 37293768d0482f41c5aa7d34cf91b03fdfee9d4e Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 30 Jan 2017 10:05:20 -0800 Subject: attr: (re)introduce git_check_attr() and struct attr_check A common pattern to check N attributes for many paths is to (1) prepare an array A of N attr_check_item 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 attr_check" that contains N, the number of attributes being sought, and A, the array that holds N attr_check_item 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. Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 2f180d6..e329851 100644 --- a/attr.c +++ b/attr.c @@ -370,6 +370,75 @@ static void free_attr_elem(struct attr_stack *e) free(e); } +struct attr_check *attr_check_alloc(void) +{ + return xcalloc(1, sizeof(struct attr_check)); +} + +struct attr_check *attr_check_initl(const char *one, ...) +{ + struct 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 = attr_check_alloc(); + check->nr = cnt; + check->alloc = cnt; + check->items = xcalloc(cnt, sizeof(struct attr_check_item)); + + check->items[0].attr = git_attr(one); + va_start(params, one); + for (cnt = 1; cnt < check->nr; cnt++) { + const struct git_attr *attr; + param = va_arg(params, const char *); + if (!param) + die("BUG: counted %d != ended at %d", + check->nr, cnt); + attr = git_attr(param); + if (!attr) + die("BUG: %s: not a valid attribute name", param); + check->items[cnt].attr = attr; + } + va_end(params); + return check; +} + +struct attr_check_item *attr_check_append(struct attr_check *check, + const struct git_attr *attr) +{ + struct attr_check_item *item; + + ALLOC_GROW(check->items, check->nr + 1, check->alloc); + item = &check->items[check->nr++]; + item->attr = attr; + return item; +} + +void attr_check_reset(struct attr_check *check) +{ + check->nr = 0; +} + +void attr_check_clear(struct attr_check *check) +{ + free(check->items); + check->items = NULL; + check->alloc = 0; + check->nr = 0; +} + +void attr_check_free(struct attr_check *check) +{ + attr_check_clear(check); + free(check); +} + static const char *builtin_attr[] = { "[attr]binary -diff -merge -text", NULL, @@ -865,6 +934,11 @@ int git_all_attrs(const char *path, int *num, struct attr_check_item **check) return 0; } +int git_check_attr(const char *path, struct attr_check *check) +{ + return git_check_attrs(path, check->nr, check->items); +} + void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate) { enum git_attr_direction old = direction; diff --git a/attr.h b/attr.h index efc7bb3..e611b13 100644 --- a/attr.h +++ b/attr.h @@ -29,6 +29,22 @@ struct attr_check_item { const char *value; }; +struct attr_check { + int nr; + int alloc; + struct attr_check_item *items; +}; + +extern struct attr_check *attr_check_alloc(void); +extern struct attr_check *attr_check_initl(const char *, ...); + +extern struct attr_check_item *attr_check_append(struct attr_check *check, + const struct git_attr *attr); + +extern void attr_check_reset(struct attr_check *check); +extern void attr_check_clear(struct attr_check *check); +extern void attr_check_free(struct attr_check *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 @@ -37,6 +53,7 @@ struct attr_check_item { extern const char *git_attr_name(const struct git_attr *); int git_check_attrs(const char *path, int, struct attr_check_item *); +extern int git_check_attr(const char *path, struct attr_check *check); /* * Retrieve all attributes that apply to the specified path. *num -- cgit v0.10.2-6-g49f6 From 7f8641112de8724a565a47b0f0b2a9b826b6baa9 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 30 Jan 2017 10:06:08 -0800 Subject: attr: convert git_all_attrs() to use "struct attr_check" This updates the other two ways the attribute check is done via an array of "struct attr_check_item" 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 attr_check_initl() to prepare the 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 attr_check_alloc() to allocate an empty attr_check, and then call attr_check_append() to add attribute names one by one. Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index e329851..4081824 100644 --- a/attr.c +++ b/attr.c @@ -906,32 +906,22 @@ int git_check_attrs(const char *path, int num, struct attr_check_item *check) return 0; } -int git_all_attrs(const char *path, int *num, struct attr_check_item **check) +void git_all_attrs(const char *path, struct attr_check *check) { - int i, count, j; + int i; - collect_some_attrs(path, 0, NULL); + attr_check_reset(check); + collect_some_attrs(path, check->nr, check->items); - /* 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; - } + struct attr_check_item *item; + if (value == ATTR__UNSET || value == ATTR__UNKNOWN) + continue; + item = attr_check_append(check, git_attr(name)); + item->value = value; } - - return 0; } int git_check_attr(const char *path, struct attr_check *check) diff --git a/attr.h b/attr.h index e611b13..9f27298 100644 --- a/attr.h +++ b/attr.h @@ -56,13 +56,10 @@ int git_check_attrs(const char *path, int, struct attr_check_item *); extern int git_check_attr(const char *path, struct attr_check *check); /* - * 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 attr_check_item **check); +extern void git_all_attrs(const char *path, struct attr_check *check); enum git_attr_direction { GIT_ATTR_CHECKIN, diff --git a/builtin/check-attr.c b/builtin/check-attr.c index 889264a..40cdff1 100644 --- a/builtin/check-attr.c +++ b/builtin/check-attr.c @@ -24,12 +24,13 @@ static const struct option check_attr_options[] = { OPT_END() }; -static void output_attr(int cnt, struct attr_check_item *check, - const char *file) +static void output_attr(struct attr_check *check, const char *file) { int j; + int cnt = check->nr; + for (j = 0; j < cnt; j++) { - const char *value = check[j].value; + const char *value = check->items[j].value; if (ATTR_TRUE(value)) value = "set"; @@ -42,36 +43,38 @@ static void output_attr(int cnt, struct attr_check_item *check, printf("%s%c" /* path */ "%s%c" /* attrname */ "%s%c" /* attrvalue */, - file, 0, git_attr_name(check[j].attr), 0, value, 0); + file, 0, + git_attr_name(check->items[j].attr), 0, value, 0); } else { quote_c_style(file, NULL, stdout, 0); - printf(": %s: %s\n", git_attr_name(check[j].attr), value); + printf(": %s: %s\n", + git_attr_name(check->items[j].attr), value); } - } } static void check_attr(const char *prefix, - int cnt, struct attr_check_item *check, + struct attr_check *check, + int collect_all, const char *file) { char *full_path = prefix_path(prefix, prefix ? strlen(prefix) : 0, file); - if (check != NULL) { - if (git_check_attrs(full_path, cnt, check)) - die("git_check_attrs died"); - output_attr(cnt, check, file); + + if (collect_all) { + git_all_attrs(full_path, check); } else { - if (git_all_attrs(full_path, &cnt, &check)) - die("git_all_attrs died"); - output_attr(cnt, check, file); - free(check); + if (git_check_attr(full_path, check)) + die("git_check_attr died"); } + output_attr(check, file); + free(full_path); } static void check_attr_stdin_paths(const char *prefix, - int cnt, struct attr_check_item *check) + struct attr_check *check, + int collect_all) { struct strbuf buf = STRBUF_INIT; struct strbuf unquoted = STRBUF_INIT; @@ -85,7 +88,7 @@ static void check_attr_stdin_paths(const char *prefix, die("line is badly quoted"); strbuf_swap(&buf, &unquoted); } - check_attr(prefix, cnt, check, buf.buf); + check_attr(prefix, check, collect_all, buf.buf); maybe_flush_or_die(stdout, "attribute to stdout"); } strbuf_release(&buf); @@ -100,7 +103,7 @@ static NORETURN void error_with_usage(const char *msg) int cmd_check_attr(int argc, const char **argv, const char *prefix) { - struct attr_check_item *check; + struct attr_check *check; int cnt, i, doubledash, filei; if (!is_bare_repository()) @@ -160,28 +163,25 @@ 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 = xcalloc(cnt, sizeof(*check)); + check = attr_check_alloc(); + if (!all_attrs) { for (i = 0; i < cnt; i++) { - const char *name; - struct git_attr *a; - name = argv[i]; - a = git_attr(name); + struct git_attr *a = git_attr(argv[i]); if (!a) return error("%s: not a valid attribute name", - name); - check[i].attr = a; + argv[i]); + attr_check_append(check, a); } } if (stdin_paths) - check_attr_stdin_paths(prefix, cnt, check); + check_attr_stdin_paths(prefix, check, all_attrs); else { for (i = filei; i < argc; i++) - check_attr(prefix, cnt, check, argv[i]); + check_attr(prefix, check, all_attrs, argv[i]); maybe_flush_or_die(stdout, "attribute to stdout"); } + + attr_check_free(check); return 0; } -- cgit v0.10.2-6-g49f6 From 2aef63d31c338a764099e925d35fe2a9c71348a8 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:57 -0800 Subject: attr: convert git_check_attrs() callers to use the new API 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 attr_check" struct with data that can be used in optimizing the query for the specific N attributes it contains. Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/archive.c b/archive.c index b76bd46..60b8891 100644 --- a/archive.c +++ b/archive.c @@ -87,19 +87,6 @@ void *sha1_file_to_archive(const struct archiver_args *args, return buffer; } -static void setup_archive_check(struct attr_check_item *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; @@ -120,10 +107,10 @@ static int write_archive_entry(const unsigned char *sha1, const char *base, void *context) { static struct strbuf path = STRBUF_INIT; + static struct attr_check *check; struct archiver_context *c = context; struct archiver_args *args = c->args; write_archive_entry_fn_t write_entry = c->write_entry; - struct attr_check_item check[2]; 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 = attr_check_initl("export-ignore", "export-subst", NULL); + if (!git_check_attr(path_without_prefix, check)) { + if (ATTR_TRUE(check->items[0].value)) return 0; - args->convert = ATTR_TRUE(check[1].value); + args->convert = ATTR_TRUE(check->items[1].value); } if (S_ISDIR(mode) || S_ISGITLINK(mode)) { diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 8b8fbd8..181e4a1 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -894,24 +894,15 @@ static void write_pack_file(void) written, nr_result); } -static void setup_delta_attr_check(struct attr_check_item *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 attr_check_item check[1]; + static struct attr_check *check; - setup_delta_attr_check(check); - if (git_check_attrs(path, ARRAY_SIZE(check), check)) + if (!check) + check = attr_check_initl("delta", NULL); + if (git_check_attr(path, check)) return 0; - if (ATTR_FALSE(check->value)) + if (ATTR_FALSE(check->items[0].value)) return 1; return 0; } diff --git a/convert.c b/convert.c index 1b98292..8d652bf 100644 --- a/convert.c +++ b/convert.c @@ -1085,24 +1085,19 @@ 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 attr_check_item ccheck[NUM_CONV_ATTRS]; + static struct 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 = 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 attr_check_item *ccheck = check->items; ca->crlf_action = git_path_check_crlf(ccheck + 4); if (ca->crlf_action == CRLF_UNDEFINED) ca->crlf_action = git_path_check_crlf(ccheck + 0); diff --git a/ll-merge.c b/ll-merge.c index 198f07a..ac0d4a5 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -336,15 +336,6 @@ static const struct ll_merge_driver *find_ll_merge_driver(const char *merge_attr return &ll_merge_drv[LL_TEXT_MERGE]; } -static int git_path_check_merge(const char *path, struct attr_check_item 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 attr_check_item check[2]; + static struct 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 = attr_check_initl("merge", "conflict-marker-size", NULL); + + if (!git_check_attr(path, check)) { + ll_driver_name = check->items[0].value; + if (check->items[1].value) { + marker_size = atoi(check->items[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 attr_check_item check; + static struct 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 = attr_check_initl("conflict-marker-size", NULL); + if (!git_check_attr(path, check) && check->items[0].value) { + marker_size = atoi(check->items[0].value); if (marker_size <= 0) marker_size = DEFAULT_CONFLICT_MARKER_SIZE; } diff --git a/userdiff.c b/userdiff.c index b0b4446..8b732e4 100644 --- a/userdiff.c +++ b/userdiff.c @@ -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 attr_check_item check; - - if (!attr) - attr = git_attr("diff"); - check.attr = attr; + static struct attr_check *check; + if (!check) + check = 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->items[0].value)) return &driver_true; - if (ATTR_FALSE(check.value)) + if (ATTR_FALSE(check->items[0].value)) return &driver_false; - if (ATTR_UNSET(check.value)) + if (ATTR_UNSET(check->items[0].value)) return NULL; - return userdiff_find_by_name(check.value); + return userdiff_find_by_name(check->items[0].value); } struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver) diff --git a/ws.c b/ws.c index fbd876e..a07caed 100644 --- a/ws.c +++ b/ws.c @@ -71,24 +71,17 @@ unsigned parse_whitespace_rule(const char *string) return rule; } -static void setup_whitespace_attr_check(struct attr_check_item *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 attr_check_item attr_whitespace_rule; + static struct attr_check *attr_whitespace_rule; + + if (!attr_whitespace_rule) + attr_whitespace_rule = 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->items[0].value; if (ATTR_TRUE(value)) { /* true (whitespace) */ unsigned all_rule = ws_tab_width(whitespace_rule_cfg); -- cgit v0.10.2-6-g49f6 From 1295c2152457c2267d605d353332ae4b3e5e5d5c Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:01:58 -0800 Subject: attr: retire git_check_attrs() API Since nobody uses the old API, make it file-scope static, and update the documentation to describe the new API. Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt index 2602668..e7cbb7c 100644 --- a/Documentation/technical/api-gitattributes.txt +++ b/Documentation/technical/api-gitattributes.txt @@ -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`:: +`struct attr_check_item`:: - This structure represents a set of attributes to check in a call - to `git_check_attr()` function, and receives the results. + This structure represents one attribute and its value. + +`struct attr_check`:: + + This structure represents a collection of `attr_check_item`. + It is passed to `git_check_attr()` function, specifying the + attributes to check, and receives their values. Attribute Values @@ -27,7 +32,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: +attr_check_item` records it. There are three macros to check these: `ATTR_TRUE()`:: @@ -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 attr_check` using attr_check_initl() + function, enumerating the names of attributes whose values you are + interested in, terminated with a NULL pointer. Alternatively, an + empty `struct attr_check` can be prepared by calling + `attr_check_alloc()` function and then attributes you want to + ask about can be added to it with `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 `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 attr_check` with two elements (because + we are checking two attributes): ------------ -static struct git_attr_check check[2]; +static struct 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 = 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 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->items[]`: ------------ - const char *value = check[0].value; + const char *value = check->items[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 attr_check *check; +static void setup_check(const char **argv) +{ + check = attr_check_alloc(); + while (*argv) { + struct git_attr *attr = git_attr(*argv); + 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 `attr_check` structure by calling + `attr_check_alloc()`. + +* Call `git_all_attrs()`, which populates the `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 `attr_check.items[]` array to examine + the attribute names and values. The name of the attribute + described by a `attr_check.items[]` object can be retrieved via + `git_attr_name(check->items[i].attr)`. (Please note that no items + will be returned for unset attributes, so `ATTR_UNSET()` will return + false for all returned `attr_check.items[]` objects.) -* Free the `git_array_check` array. +* Free the `attr_check` struct by calling `attr_check_free()`. diff --git a/attr.c b/attr.c index 4081824..c0e7893 100644 --- a/attr.c +++ b/attr.c @@ -890,7 +890,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 attr_check_item *check) +static int git_check_attrs(const char *path, int num, + struct attr_check_item *check) { int i; diff --git a/attr.h b/attr.h index 9f27298..b2cfd85 100644 --- a/attr.h +++ b/attr.h @@ -52,7 +52,6 @@ extern void attr_check_free(struct attr_check *check); */ extern const char *git_attr_name(const struct git_attr *); -int git_check_attrs(const char *path, int, struct attr_check_item *); extern int git_check_attr(const char *path, struct attr_check *check); /* -- cgit v0.10.2-6-g49f6 From 6bc2e3f709d04e416f3b5d1e23f2ac31f4cbc1d1 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Fri, 27 Jan 2017 18:01:59 -0800 Subject: attr: pass struct attr_check to collect_some_attrs The old callchain used to take an array of attr_check_item items. Instead pass the 'attr_check' container object to 'collect_some_attrs()' and access the fields in the data structure directly. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index c0e7893..81a3c74 100644 --- a/attr.c +++ b/attr.c @@ -846,9 +846,7 @@ static int macroexpand_one(int nr, int rem) * 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, - struct attr_check_item *check) - +static void collect_some_attrs(const char *path, struct attr_check *check) { struct attr_stack *stk; int i, pathlen, rem, dirlen; @@ -871,17 +869,18 @@ static void collect_some_attrs(const char *path, int num, 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->nr && !cannot_trust_maybe_real) { rem = 0; - for (i = 0; i < num; i++) { - if (!check[i].attr->maybe_real) { + for (i = 0; i < check->nr; i++) { + const struct git_attr *a = check->items[i].attr; + if (!a->maybe_real) { struct attr_check_item *c; - c = check_all_attr + check[i].attr->attr_nr; + c = check_all_attr + a->attr_nr; c->value = ATTR__UNSET; rem++; } } - if (rem == num) + if (rem == check->nr) return; } @@ -890,18 +889,17 @@ 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, - struct attr_check_item *check) +int git_check_attr(const char *path, struct attr_check *check) { int i; - collect_some_attrs(path, num, check); + collect_some_attrs(path, check); - for (i = 0; i < num; i++) { - const char *value = check_all_attr[check[i].attr->attr_nr].value; + for (i = 0; i < check->nr; i++) { + const char *value = check_all_attr[check->items[i].attr->attr_nr].value; if (value == ATTR__UNKNOWN) value = ATTR__UNSET; - check[i].value = value; + check->items[i].value = value; } return 0; @@ -912,7 +910,7 @@ void git_all_attrs(const char *path, struct attr_check *check) int i; attr_check_reset(check); - collect_some_attrs(path, check->nr, check->items); + collect_some_attrs(path, check); for (i = 0; i < attr_nr; i++) { const char *name = check_all_attr[i].attr->name; @@ -925,11 +923,6 @@ void git_all_attrs(const char *path, struct attr_check *check) } } -int git_check_attr(const char *path, struct attr_check *check) -{ - return git_check_attrs(path, check->nr, check->items); -} - void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate) { enum git_attr_direction old = direction; -- cgit v0.10.2-6-g49f6 From 428103c7f1a0cb8bb1432214efa60abc5bd5f198 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Jan 2017 18:02:00 -0800 Subject: attr: change validity check for attribute names to use positive logic Convert 'invalid_attr_name()' to 'attr_name_valid()' and use positive logic for the return value. In addition create a helper function that prints out an error message when an invalid attribute name is used. 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 Signed-off-by: Stefan Beller Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 81a3c74..9fe848f 100644 --- a/attr.c +++ b/attr.c @@ -74,23 +74,33 @@ 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; +} + +static void report_invalid_attr(const char *name, size_t len, + const char *src, int lineno) +{ + struct strbuf err = STRBUF_INIT; + strbuf_addf(&err, _("%.*s is not a valid attribute name"), + (int) len, name); + fprintf(stderr, "%s: %s:%d\n", err.buf, src, lineno); + strbuf_release(&err); } static struct git_attr *git_attr_internal(const char *name, int len) @@ -105,7 +115,7 @@ static struct git_attr *git_attr_internal(const char *name, int len) return a; } - if (invalid_attr_name(name, len)) + if (!attr_name_valid(name, len)) return NULL; FLEX_ALLOC_MEM(a, name, name, len); @@ -196,17 +206,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 == '!') { @@ -258,10 +266,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; } } -- cgit v0.10.2-6-g49f6 From 1a600b7555205f80b276659db4fd521658642505 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Fri, 27 Jan 2017 18:02:01 -0800 Subject: attr: use hashmap for attribute dictionary The current implementation of the attribute dictionary uses a custom hashtable. This modernizes the dictionary by converting it to the builtin 'hashmap' structure. Also, in order to enable a threaded API in the future add an accompanying mutex which must be acquired prior to accessing the dictionary of interned attributes. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 9fe848f..e008f30 100644 --- a/attr.c +++ b/attr.c @@ -14,6 +14,7 @@ #include "dir.h" #include "utf8.h" #include "quote.h" +#include "thread-utils.h" const char git_attr__true[] = "(builtin)true"; const char git_attr__false[] = "\0(builtin)false"; @@ -23,28 +24,17 @@ static const char git_attr__unknown[] = "(builtin)unknown"; #define ATTR__UNSET NULL #define ATTR__UNKNOWN git_attr__unknown -/* This is a randomly chosen prime. */ -#define HASHSIZE 257 - #ifndef DEBUG_ATTR #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; - int attr_nr; + int attr_nr; /* unique attribute number */ int maybe_macro; int maybe_real; - char name[FLEX_ARRAY]; + char name[FLEX_ARRAY]; /* attribute name */ }; static int attr_nr; -static struct git_attr *(git_attr_hash[HASHSIZE]); /* * NEEDSWORK: maybe-real, maybe-macro are not property of @@ -63,15 +53,94 @@ const char *git_attr_name(const struct git_attr *attr) return attr->name; } -static unsigned hash_name(const char *name, int namelen) +struct attr_hashmap { + struct hashmap map; +#ifndef NO_PTHREADS + pthread_mutex_t mutex; +#endif +}; + +static inline void hashmap_lock(struct attr_hashmap *map) +{ +#ifndef NO_PTHREADS + pthread_mutex_lock(&map->mutex); +#endif +} + +static inline void hashmap_unlock(struct attr_hashmap *map) { - unsigned val = 0, c; +#ifndef NO_PTHREADS + pthread_mutex_unlock(&map->mutex); +#endif +} - while (namelen--) { - c = *name++; - val = ((val << 7) | (val >> 22)) ^ c; - } - return val; +/* + * The global dictionary of all interned attributes. This + * is a singleton object which is shared between threads. + * Access to this dictionary must be surrounded with a mutex. + */ +static struct attr_hashmap g_attr_hashmap; + +/* The container for objects stored in "struct attr_hashmap" */ +struct attr_hash_entry { + struct hashmap_entry ent; /* must be the first member! */ + const char *key; /* the key; memory should be owned by value */ + size_t keylen; /* length of the key */ + void *value; /* the stored value */ +}; + +/* attr_hashmap comparison function */ +static int attr_hash_entry_cmp(const struct attr_hash_entry *a, + const struct attr_hash_entry *b, + void *unused) +{ + return (a->keylen != b->keylen) || strncmp(a->key, b->key, a->keylen); +} + +/* Initialize an 'attr_hashmap' object */ +static void attr_hashmap_init(struct attr_hashmap *map) +{ + hashmap_init(&map->map, (hashmap_cmp_fn) attr_hash_entry_cmp, 0); +} + +/* + * Retrieve the 'value' stored in a hashmap given the provided 'key'. + * If there is no matching entry, return NULL. + */ +static void *attr_hashmap_get(struct attr_hashmap *map, + const char *key, size_t keylen) +{ + struct attr_hash_entry k; + struct attr_hash_entry *e; + + if (!map->map.tablesize) + attr_hashmap_init(map); + + hashmap_entry_init(&k, memhash(key, keylen)); + k.key = key; + k.keylen = keylen; + e = hashmap_get(&map->map, &k, NULL); + + return e ? e->value : NULL; +} + +/* Add 'value' to a hashmap based on the provided 'key'. */ +static void attr_hashmap_add(struct attr_hashmap *map, + const char *key, size_t keylen, + void *value) +{ + struct attr_hash_entry *e; + + if (!map->map.tablesize) + attr_hashmap_init(map); + + e = xmalloc(sizeof(struct attr_hash_entry)); + hashmap_entry_init(e, memhash(key, keylen)); + e->key = key; + e->keylen = keylen; + e->value = value; + + hashmap_add(&map->map, e); } static int attr_name_valid(const char *name, size_t namelen) @@ -103,37 +172,44 @@ static void report_invalid_attr(const char *name, size_t len, strbuf_release(&err); } -static struct git_attr *git_attr_internal(const char *name, int len) +/* + * Given a 'name', lookup and return the corresponding attribute in the global + * dictionary. If no entry is found, create a new attribute and store it in + * the dictionary. + */ +static struct git_attr *git_attr_internal(const char *name, int namelen) { - unsigned hval = hash_name(name, len); - unsigned pos = hval % HASHSIZE; struct git_attr *a; - for (a = git_attr_hash[pos]; a; a = a->next) { - if (a->h == hval && - !memcmp(a->name, name, len) && !a->name[len]) - return a; - } - - if (!attr_name_valid(name, len)) + if (!attr_name_valid(name, namelen)) return NULL; - FLEX_ALLOC_MEM(a, name, name, len); - a->h = hval; - a->next = git_attr_hash[pos]; - a->attr_nr = attr_nr++; - a->maybe_macro = 0; - a->maybe_real = 0; - git_attr_hash[pos] = a; + hashmap_lock(&g_attr_hashmap); + + a = attr_hashmap_get(&g_attr_hashmap, name, namelen); + + if (!a) { + FLEX_ALLOC_MEM(a, name, name, namelen); + a->attr_nr = g_attr_hashmap.map.size; + a->maybe_real = 0; + a->maybe_macro = 0; + + attr_hashmap_add(&g_attr_hashmap, a->name, namelen, a); + assert(a->attr_nr == (g_attr_hashmap.map.size - 1)); + + /* + * 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; + assert(a->attr_nr == (attr_nr - 1)); + } + + hashmap_unlock(&g_attr_hashmap); - /* - * 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; return a; } @@ -941,3 +1017,10 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist drop_attr_stack(); use_index = istate; } + +void attr_start(void) +{ +#ifndef NO_PTHREADS + pthread_mutex_init(&g_attr_hashmap.mutex, NULL); +#endif +} diff --git a/attr.h b/attr.h index b2cfd85..898e1a8 100644 --- a/attr.h +++ b/attr.h @@ -67,4 +67,6 @@ enum git_attr_direction { }; void git_attr_set_direction(enum git_attr_direction, struct index_state *); +extern void attr_start(void); + #endif /* ATTR_H */ diff --git a/common-main.c b/common-main.c index c654f95..6a68900 100644 --- a/common-main.c +++ b/common-main.c @@ -1,5 +1,6 @@ #include "cache.h" #include "exec_cmd.h" +#include "attr.h" /* * Many parts of Git have subprograms communicate via pipe, expect the @@ -33,6 +34,8 @@ int main(int argc, const char **argv) git_setup_gettext(); + attr_start(); + git_extract_argv0_path(argv[0]); restore_sigpipe_to_default(); -- cgit v0.10.2-6-g49f6 From 685b2925757e98624e37abe09d49c205a7db5943 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Fri, 27 Jan 2017 18:02:02 -0800 Subject: attr: eliminate global check_all_attr array Currently there is a reliance on 'check_all_attr' which is a global array of 'attr_check_item' items which is used to store the value of each attribute during the collection process. This patch eliminates this global and instead creates an array per 'attr_check' instance which is then used in the attribute collection process. This brings the attribute system one step closer to being thread-safe. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index e008f30..2637804 100644 --- a/attr.c +++ b/attr.c @@ -34,7 +34,6 @@ struct git_attr { int maybe_real; char name[FLEX_ARRAY]; /* attribute name */ }; -static int attr_nr; /* * NEEDSWORK: maybe-real, maybe-macro are not property of @@ -45,9 +44,6 @@ static int attr_nr; */ static int cannot_trust_maybe_real; -/* NEEDSWORK: This will become per git_attr_check */ -static struct attr_check_item *check_all_attr; - const char *git_attr_name(const struct git_attr *attr) { return attr->name; @@ -143,6 +139,57 @@ static void attr_hashmap_add(struct attr_hashmap *map, hashmap_add(&map->map, e); } +struct all_attrs_item { + const struct git_attr *attr; + const char *value; +}; + +/* + * Reallocate and reinitialize the array of all attributes (which is used in + * the attribute collection process) in 'check' based on the global dictionary + * of attributes. + */ +static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check) +{ + int i; + + hashmap_lock(map); + + if (map->map.size < check->all_attrs_nr) + die("BUG: interned attributes shouldn't be deleted"); + + /* + * If the number of attributes in the global dictionary has increased + * (or this attr_check instance doesn't have an initialized all_attrs + * field), reallocate the provided attr_check instance's all_attrs + * field and fill each entry with its corresponding git_attr. + */ + if (map->map.size != check->all_attrs_nr) { + struct attr_hash_entry *e; + struct hashmap_iter iter; + hashmap_iter_init(&map->map, &iter); + + REALLOC_ARRAY(check->all_attrs, map->map.size); + check->all_attrs_nr = map->map.size; + + while ((e = hashmap_iter_next(&iter))) { + const struct git_attr *a = e->value; + check->all_attrs[a->attr_nr].attr = a; + } + } + + hashmap_unlock(map); + + /* + * Re-initialize every entry in check->all_attrs. + * This re-initialization can live outside of the locked region since + * the attribute dictionary is no longer being accessed. + */ + for (i = 0; i < check->all_attrs_nr; i++) { + check->all_attrs[i].value = ATTR__UNKNOWN; + } +} + static int attr_name_valid(const char *name, size_t namelen) { /* @@ -196,16 +243,6 @@ static struct git_attr *git_attr_internal(const char *name, int namelen) attr_hashmap_add(&g_attr_hashmap, a->name, namelen, a); assert(a->attr_nr == (g_attr_hashmap.map.size - 1)); - - /* - * 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; - assert(a->attr_nr == (attr_nr - 1)); } hashmap_unlock(&g_attr_hashmap); @@ -513,6 +550,10 @@ void attr_check_clear(struct attr_check *check) check->items = NULL; check->alloc = 0; check->nr = 0; + + free(check->all_attrs); + check->all_attrs = NULL; + check->all_attrs_nr = 0; } void attr_check_free(struct attr_check *check) @@ -860,16 +901,16 @@ 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(struct all_attrs_item *all_attrs, int nr, int rem); -static int fill_one(const char *what, struct match_attr *a, int rem) +static int fill_one(const char *what, struct all_attrs_item *all_attrs, + struct match_attr *a, int rem) { - struct attr_check_item *check = check_all_attr; int i; - for (i = a->num_attr - 1; 0 < rem && 0 <= i; i--) { + for (i = a->num_attr - 1; rem > 0 && i >= 0; i--) { struct git_attr *attr = a->state[i].attr; - const char **n = &(check[attr->attr_nr].value); + const char **n = &(all_attrs[attr->attr_nr].value); const char *v = a->state[i].setto; if (*n == ATTR__UNKNOWN) { @@ -878,14 +919,15 @@ 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(all_attrs, attr->attr_nr, rem); } } return rem; } static int fill(const char *path, int pathlen, int basename_offset, - struct attr_stack *stk, int rem) + struct attr_stack *stk, struct all_attrs_item *all_attrs, + int rem) { int i; const char *base = stk->origin ? stk->origin : ""; @@ -896,18 +938,18 @@ 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", all_attrs, a, rem); } return rem; } -static int macroexpand_one(int nr, int rem) +static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem) { struct attr_stack *stk; int i; - if (check_all_attr[nr].value != ATTR__TRUE || - !check_all_attr[nr].attr->maybe_macro) + if (all_attrs[nr].value != ATTR__TRUE || + !all_attrs[nr].attr->maybe_macro) return rem; for (stk = attr_stack; stk; stk = stk->prev) { @@ -916,7 +958,7 @@ static int macroexpand_one(int nr, int rem) if (!ma->is_macro) continue; if (ma->u.attr->attr_nr == nr) - return fill_one("expand", ma, rem); + return fill_one("expand", all_attrs, ma, rem); } } @@ -924,9 +966,9 @@ static int macroexpand_one(int nr, int rem) } /* - * 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. + * Collect attributes for path into the array pointed to by check->all_attrs. + * If check->check_nr is non-zero, only attributes in check[] are collected. + * Otherwise all attributes are collected. */ static void collect_some_attrs(const char *path, struct attr_check *check) { @@ -949,15 +991,15 @@ static void collect_some_attrs(const char *path, struct attr_check *check) } prepare_attr_stack(path, dirlen); - for (i = 0; i < attr_nr; i++) - check_all_attr[i].value = ATTR__UNKNOWN; + all_attrs_init(&g_attr_hashmap, check); + if (check->nr && !cannot_trust_maybe_real) { rem = 0; for (i = 0; i < check->nr; i++) { const struct git_attr *a = check->items[i].attr; if (!a->maybe_real) { - struct attr_check_item *c; - c = check_all_attr + a->attr_nr; + struct all_attrs_item *c; + c = check->all_attrs + a->attr_nr; c->value = ATTR__UNSET; rem++; } @@ -966,9 +1008,9 @@ static void collect_some_attrs(const char *path, struct attr_check *check) return; } - rem = attr_nr; + rem = check->all_attrs_nr; for (stk = attr_stack; 0 < rem && stk; stk = stk->prev) - rem = fill(path, pathlen, basename_offset, stk, rem); + rem = fill(path, pathlen, basename_offset, stk, check->all_attrs, rem); } int git_check_attr(const char *path, struct attr_check *check) @@ -978,7 +1020,8 @@ int git_check_attr(const char *path, struct attr_check *check) collect_some_attrs(path, check); for (i = 0; i < check->nr; i++) { - const char *value = check_all_attr[check->items[i].attr->attr_nr].value; + size_t n = check->items[i].attr->attr_nr; + const char *value = check->all_attrs[n].value; if (value == ATTR__UNKNOWN) value = ATTR__UNSET; check->items[i].value = value; @@ -994,9 +1037,9 @@ void git_all_attrs(const char *path, struct attr_check *check) attr_check_reset(check); collect_some_attrs(path, check); - for (i = 0; i < attr_nr; i++) { - const char *name = check_all_attr[i].attr->name; - const char *value = check_all_attr[i].value; + for (i = 0; i < check->all_attrs_nr; i++) { + const char *name = check->all_attrs[i].attr->name; + const char *value = check->all_attrs[i].value; struct attr_check_item *item; if (value == ATTR__UNSET || value == ATTR__UNKNOWN) continue; diff --git a/attr.h b/attr.h index 898e1a8..5aaf55c 100644 --- a/attr.h +++ b/attr.h @@ -4,6 +4,9 @@ /* An attribute is a pointer to this opaque structure */ struct git_attr; +/* opaque structure used internally for attribute collection */ +struct all_attrs_item; + /* * Given a string, return the gitattribute object that * corresponds to it. @@ -33,6 +36,8 @@ struct attr_check { int nr; int alloc; struct attr_check_item *items; + int all_attrs_nr; + struct all_attrs_item *all_attrs; }; extern struct attr_check *attr_check_alloc(void); -- cgit v0.10.2-6-g49f6 From 60a12722ac80bc55b43581a7719d25d1bb7a9882 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Fri, 27 Jan 2017 18:02:03 -0800 Subject: attr: remove maybe-real, maybe-macro from git_attr Whether or not a git attribute is real or a macro isn't a property of the attribute but rather it depends on the attribute stack (which .gitattribute files were read). This patch removes the 'maybe_real' and 'maybe_macro' fields in a git_attr and instead adds the 'macro' field to a attr_check_item. The 'macro' indicates (if non-NULL) that a particular attribute is a macro for the given attribute stack. It's populated, through a quick scan of the attribute stack, with the match_attr that corresponds to the macro's definition. This way the attribute stack only needs to be scanned a single time prior to attribute collection instead of each time a macro needs to be expanded. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 2637804..8f4402e 100644 --- a/attr.c +++ b/attr.c @@ -30,20 +30,9 @@ static const char git_attr__unknown[] = "(builtin)unknown"; struct git_attr { int attr_nr; /* unique attribute number */ - int maybe_macro; - int maybe_real; char name[FLEX_ARRAY]; /* attribute name */ }; -/* - * 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; - const char *git_attr_name(const struct git_attr *attr) { return attr->name; @@ -142,6 +131,12 @@ static void attr_hashmap_add(struct attr_hashmap *map, struct all_attrs_item { const struct git_attr *attr; const char *value; + /* + * If 'macro' is non-NULL, indicates that 'attr' is a macro based on + * the current attribute stack and contains a pointer to the match_attr + * definition of the macro + */ + const struct match_attr *macro; }; /* @@ -187,6 +182,7 @@ static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check) */ for (i = 0; i < check->all_attrs_nr; i++) { check->all_attrs[i].value = ATTR__UNKNOWN; + check->all_attrs[i].macro = NULL; } } @@ -238,8 +234,6 @@ static struct git_attr *git_attr_internal(const char *name, int namelen) if (!a) { FLEX_ALLOC_MEM(a, name, name, namelen); a->attr_nr = g_attr_hashmap.map.size; - a->maybe_real = 0; - a->maybe_macro = 0; attr_hashmap_add(&g_attr_hashmap, a->name, namelen, a); assert(a->attr_nr == (g_attr_hashmap.map.size - 1)); @@ -402,7 +396,6 @@ static struct match_attr *parse_attr_line(const char *line, const char *src, (is_macro ? 0 : namelen + 1)); if (is_macro) { res->u.attr = git_attr_internal(name, namelen); - res->u.attr->maybe_macro = 1; } else { char *p = (char *)&(res->state[num_attr]); memcpy(p, name, namelen); @@ -423,10 +416,6 @@ static struct match_attr *parse_attr_line(const char *line, const char *src, /* Second pass to fill the attr_states */ for (cp = states, i = 0; *cp; i++) { cp = parse_attr(src, lineno, cp, &(res->state[i])); - if (!is_macro) - res->state[i].attr->maybe_real = 1; - if (res->state[i].attr->maybe_macro) - cannot_trust_maybe_real = 1; } strbuf_release(&pattern); @@ -904,7 +893,7 @@ static int path_matches(const char *pathname, int pathlen, static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem); static int fill_one(const char *what, struct all_attrs_item *all_attrs, - struct match_attr *a, int rem) + const struct match_attr *a, int rem) { int i; @@ -945,24 +934,34 @@ static int fill(const char *path, int pathlen, int basename_offset, static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem) { - struct attr_stack *stk; - int i; + const struct all_attrs_item *item = &all_attrs[nr]; - if (all_attrs[nr].value != ATTR__TRUE || - !all_attrs[nr].attr->maybe_macro) + if (item->macro && item->value == ATTR__TRUE) + return fill_one("expand", all_attrs, item->macro, rem); + else return rem; +} - 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) - return fill_one("expand", all_attrs, ma, rem); +/* + * Marks the attributes which are macros based on the attribute stack. + * This prevents having to search through the attribute stack each time + * a macro needs to be expanded during the fill stage. + */ +static void determine_macros(struct all_attrs_item *all_attrs, + const struct attr_stack *stack) +{ + for (; stack; stack = stack->prev) { + int i; + for (i = stack->num_matches - 1; i >= 0; i--) { + const struct match_attr *ma = stack->attrs[i]; + if (ma->is_macro) { + int n = ma->u.attr->attr_nr; + if (!all_attrs[n].macro) { + all_attrs[n].macro = ma; + } + } } } - - return rem; } /* @@ -992,15 +991,15 @@ static void collect_some_attrs(const char *path, struct attr_check *check) prepare_attr_stack(path, dirlen); all_attrs_init(&g_attr_hashmap, check); + determine_macros(check->all_attrs, attr_stack); - if (check->nr && !cannot_trust_maybe_real) { + if (check->nr) { rem = 0; for (i = 0; i < check->nr; i++) { - const struct git_attr *a = check->items[i].attr; - if (!a->maybe_real) { - struct all_attrs_item *c; - c = check->all_attrs + a->attr_nr; - c->value = ATTR__UNSET; + int n = check->items[i].attr->attr_nr; + struct all_attrs_item *item = &check->all_attrs[n]; + if (item->macro) { + item->value = ATTR__UNSET; rem++; } } -- cgit v0.10.2-6-g49f6 From e810e0635767afbc9b304d5256fbdb26b59644fa Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Fri, 27 Jan 2017 18:02:04 -0800 Subject: attr: tighten const correctness with git_attr and match_attr Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 8f4402e..69643ae 100644 --- a/attr.c +++ b/attr.c @@ -220,7 +220,7 @@ static void report_invalid_attr(const char *name, size_t len, * dictionary. If no entry is found, create a new attribute and store it in * the dictionary. */ -static struct git_attr *git_attr_internal(const char *name, int namelen) +static const struct git_attr *git_attr_internal(const char *name, int namelen) { struct git_attr *a; @@ -244,14 +244,14 @@ static struct git_attr *git_attr_internal(const char *name, int namelen) return a; } -struct git_attr *git_attr(const char *name) +const struct git_attr *git_attr(const char *name) { return git_attr_internal(name, strlen(name)); } /* What does a matched pattern decide? */ struct attr_state { - struct git_attr *attr; + const struct git_attr *attr; const char *setto; }; @@ -278,7 +278,7 @@ struct pattern { struct match_attr { union { struct pattern pat; - struct git_attr *attr; + const struct git_attr *attr; } u; char is_macro; unsigned num_attr; @@ -898,7 +898,7 @@ static int fill_one(const char *what, struct all_attrs_item *all_attrs, int i; for (i = a->num_attr - 1; rem > 0 && i >= 0; i--) { - struct git_attr *attr = a->state[i].attr; + const struct git_attr *attr = a->state[i].attr; const char **n = &(all_attrs[attr->attr_nr].value); const char *v = a->state[i].setto; @@ -922,7 +922,7 @@ static int fill(const char *path, int pathlen, int basename_offset, const char *base = stk->origin ? stk->origin : ""; for (i = stk->num_matches - 1; 0 < rem && 0 <= i; i--) { - struct match_attr *a = stk->attrs[i]; + const struct match_attr *a = stk->attrs[i]; if (a->is_macro) continue; if (path_matches(path, pathlen, basename_offset, diff --git a/attr.h b/attr.h index 5aaf55c..abebbc1 100644 --- a/attr.h +++ b/attr.h @@ -11,7 +11,7 @@ struct all_attrs_item; * Given a string, return the gitattribute object that * corresponds to it. */ -struct git_attr *git_attr(const char *); +const struct git_attr *git_attr(const char *); /* Internal use */ extern const char git_attr__true[]; diff --git a/builtin/check-attr.c b/builtin/check-attr.c index 40cdff1..4d01ca0 100644 --- a/builtin/check-attr.c +++ b/builtin/check-attr.c @@ -166,7 +166,8 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix) check = attr_check_alloc(); if (!all_attrs) { for (i = 0; i < cnt; i++) { - struct git_attr *a = git_attr(argv[i]); + const struct git_attr *a = git_attr(argv[i]); + if (!a) return error("%s: not a valid attribute name", argv[i]); -- cgit v0.10.2-6-g49f6 From dc81cf377cd25193a9cf044767917e4f5553c285 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Fri, 27 Jan 2017 18:02:05 -0800 Subject: attr: store attribute stack in attr_check structure The last big hurdle towards a thread-safe API for the attribute system is the reliance on a global attribute stack that is modified during each call into the attribute system. This patch removes this global stack and instead a stack is stored locally in each attr_check instance. This opens up the opportunity for future optimizations to customize the attribute stack for the attributes that a particular attr_check struct is interested in. One caveat with pushing the attribute stack into the attr_check structure is that the attribute system now needs to keep track of all active attr_check instances. Due to the direction mechanism the stack needs to be dropped when the direction is switched. In order to ensure correctness when the direction is changed the attribute system needs to iterate through all active attr_check instances and drop each of their stacks. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 69643ae..bcee092 100644 --- a/attr.c +++ b/attr.c @@ -445,17 +445,16 @@ fail_return: * .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; +}; -static void free_attr_elem(struct attr_stack *e) +static void attr_stack_free(struct attr_stack *e) { int i; free(e->origin); @@ -478,9 +477,96 @@ static void free_attr_elem(struct attr_stack *e) free(e); } +static void drop_attr_stack(struct attr_stack **stack) +{ + while (*stack) { + struct attr_stack *elem = *stack; + *stack = elem->prev; + attr_stack_free(elem); + } +} + +/* List of all attr_check structs; access should be surrounded by mutex */ +static struct check_vector { + size_t nr; + size_t alloc; + struct attr_check **checks; +#ifndef NO_PTHREADS + pthread_mutex_t mutex; +#endif +} check_vector; + +static inline void vector_lock(void) +{ +#ifndef NO_PTHREADS + pthread_mutex_lock(&check_vector.mutex); +#endif +} + +static inline void vector_unlock(void) +{ +#ifndef NO_PTHREADS + pthread_mutex_unlock(&check_vector.mutex); +#endif +} + +static void check_vector_add(struct attr_check *c) +{ + vector_lock(); + + ALLOC_GROW(check_vector.checks, + check_vector.nr + 1, + check_vector.alloc); + check_vector.checks[check_vector.nr++] = c; + + vector_unlock(); +} + +static void check_vector_remove(struct attr_check *check) +{ + int i; + + vector_lock(); + + /* Find entry */ + for (i = 0; i < check_vector.nr; i++) + if (check_vector.checks[i] == check) + break; + + if (i >= check_vector.nr) + die("BUG: no entry found"); + + /* shift entries over */ + for (; i < check_vector.nr - 1; i++) + check_vector.checks[i] = check_vector.checks[i + 1]; + + check_vector.nr--; + + vector_unlock(); +} + +/* Iterate through all attr_check instances and drop their stacks */ +static void drop_all_attr_stacks(void) +{ + int i; + + vector_lock(); + + for (i = 0; i < check_vector.nr; i++) { + drop_attr_stack(&check_vector.checks[i]->stack); + } + + vector_unlock(); +} + struct attr_check *attr_check_alloc(void) { - return xcalloc(1, sizeof(struct attr_check)); + struct attr_check *c = xcalloc(1, sizeof(struct attr_check)); + + /* save pointer to the check struct */ + check_vector_add(c); + + return c; } struct attr_check *attr_check_initl(const char *one, ...) @@ -543,12 +629,19 @@ void attr_check_clear(struct attr_check *check) free(check->all_attrs); check->all_attrs = NULL; check->all_attrs_nr = 0; + + drop_attr_stack(&check->stack); } void attr_check_free(struct attr_check *check) { - attr_check_clear(check); - free(check); + if (check) { + /* Remove check from the check vector */ + check_vector_remove(check); + + attr_check_clear(check); + free(check); + } } static const char *builtin_attr[] = { @@ -705,15 +798,6 @@ static void debug_set(const char *what, const char *match, struct git_attr *attr #define debug_set(a,b,c,d) do { ; } while (0) #endif /* DEBUG_ATTR */ -static void drop_attr_stack(void) -{ - while (attr_stack) { - struct attr_stack *elem = attr_stack; - attr_stack = elem->prev; - free_attr_elem(elem); - } -} - static const char *git_etc_gitattributes(void) { static const char *system_wide; @@ -722,6 +806,14 @@ static const char *git_etc_gitattributes(void) return system_wide; } +static const char *get_home_gitattributes(void) +{ + if (!git_attributes_file) + git_attributes_file = xdg_config_home("attributes"); + + return git_attributes_file; +} + static int git_attr_system(void) { return !git_env_bool("GIT_ATTR_NOSYSTEM", 0); @@ -741,47 +833,50 @@ static void push_stack(struct attr_stack **attr_stack_p, } } -static void bootstrap_attr_stack(void) +static void bootstrap_attr_stack(struct attr_stack **stack) { - struct attr_stack *elem; + struct attr_stack *e; - if (attr_stack) + if (*stack) return; - 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); + /* builtin frame */ + e = read_attr_from_array(builtin_attr); + push_stack(stack, e, NULL, 0); - if (!git_attributes_file) - git_attributes_file = xdg_config_home("attributes"); - if (git_attributes_file) - push_stack(&attr_stack, - read_attr_from_file(git_attributes_file, 1), - NULL, 0); + /* system-wide frame */ + if (git_attr_system()) { + e = read_attr_from_file(git_etc_gitattributes(), 1); + push_stack(stack, e, NULL, 0); + } - if (!is_bare_repository() || direction == GIT_ATTR_INDEX) { - elem = read_attr(GITATTRIBUTES_FILE, 1); - push_stack(&attr_stack, elem, xstrdup(""), 0); - debug_push(elem); + /* home directory */ + if (get_home_gitattributes()) { + e = read_attr_from_file(get_home_gitattributes(), 1); + push_stack(stack, e, NULL, 0); } - if (startup_info->have_repository) - elem = read_attr_from_file(git_path_info_attributes(), 1); + /* root directory */ + if (!is_bare_repository() || direction == GIT_ATTR_INDEX) + e = read_attr(GITATTRIBUTES_FILE, 1); else - elem = NULL; + e = xcalloc(1, sizeof(struct attr_stack)); + push_stack(stack, e, xstrdup(""), 0); - if (!elem) - elem = xcalloc(1, sizeof(*elem)); - push_stack(&attr_stack, elem, NULL, 0); + /* info frame */ + if (startup_info->have_repository) + e = read_attr_from_file(git_path_info_attributes(), 1); + else + e = NULL; + if (!e) + e = xcalloc(1, sizeof(struct attr_stack)); + push_stack(stack, e, NULL, 0); } -static void prepare_attr_stack(const char *path, int dirlen) +static void prepare_attr_stack(const char *path, int dirlen, + struct attr_stack **stack) { - struct attr_stack *elem, *info; - const char *cp; + struct attr_stack *info; /* * At the bottom of the attribute stack is the built-in @@ -798,13 +893,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(stack); /* * Pop the "info" one that is always at the top of the stack. */ - info = attr_stack; - attr_stack = info->prev; + info = *stack; + *stack = info->prev; /* * Pop the ones from directories that are not the prefix of @@ -812,18 +907,19 @@ 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 ((*stack)->origin) { + int namelen = (*stack)->originlen; + struct attr_stack *elem; - elem = attr_stack; + elem = *stack; if (namelen <= dirlen && !strncmp(elem->origin, path, namelen) && (!namelen || path[namelen] == '/')) break; debug_pop(elem); - attr_stack = elem->prev; - free_attr_elem(elem); + *stack = elem->prev; + attr_stack_free(elem); } /* @@ -838,33 +934,43 @@ static void prepare_attr_stack(const char *path, int dirlen) */ struct strbuf pathbuf = STRBUF_INIT; - assert(attr_stack->origin); - while (1) { - size_t len = strlen(attr_stack->origin); + assert((*stack)->origin); + strbuf_addstr(&pathbuf, (*stack)->origin); + /* Build up to the directory 'path' is in */ + while (pathbuf.len < dirlen) { + size_t len = pathbuf.len; + struct attr_stack *next; char *origin; - if (dirlen <= len) - break; - cp = memchr(path + len + 1, '/', dirlen - len - 1); - if (!cp) - cp = path + dirlen; - strbuf_addf(&pathbuf, - "%.*s/%s", (int)(cp - path), path, - GITATTRIBUTES_FILE); - elem = read_attr(pathbuf.buf, 0); - strbuf_setlen(&pathbuf, cp - path); - origin = strbuf_detach(&pathbuf, &len); - push_stack(&attr_stack, elem, origin, len); - debug_push(elem); - } + /* Skip path-separator */ + if (len < dirlen && is_dir_sep(path[len])) + len++; + /* Find the end of the next component */ + while (len < dirlen && !is_dir_sep(path[len])) + len++; + + if (pathbuf.len > 0) + strbuf_addch(&pathbuf, '/'); + strbuf_add(&pathbuf, path + pathbuf.len, + (len - pathbuf.len)); + strbuf_addf(&pathbuf, "/%s", GITATTRIBUTES_FILE); + + next = read_attr(pathbuf.buf, 0); + /* reset the pathbuf to not include "/.gitattributes" */ + strbuf_setlen(&pathbuf, len); + + origin = xstrdup(pathbuf.buf); + push_stack(stack, next, origin, len); + + } strbuf_release(&pathbuf); } /* * Finally push the "info" one at the top of the stack. */ - push_stack(&attr_stack, info, NULL, 0); + push_stack(stack, info, NULL, 0); } static int path_matches(const char *pathname, int pathlen, @@ -915,20 +1021,23 @@ static int fill_one(const char *what, struct all_attrs_item *all_attrs, } static int fill(const char *path, int pathlen, int basename_offset, - struct attr_stack *stk, struct all_attrs_item *all_attrs, - int rem) + const struct attr_stack *stack, + struct all_attrs_item *all_attrs, int rem) { - int i; - const char *base = stk->origin ? stk->origin : ""; - - for (i = stk->num_matches - 1; 0 < rem && 0 <= i; i--) { - const struct match_attr *a = stk->attrs[i]; - if (a->is_macro) - continue; - if (path_matches(path, pathlen, basename_offset, - &a->u.pat, base, stk->originlen)) - rem = fill_one("fill", all_attrs, a, rem); + for (; rem > 0 && stack; stack = stack->prev) { + int i; + const char *base = stack->origin ? stack->origin : ""; + + for (i = stack->num_matches - 1; 0 < rem && 0 <= i; i--) { + const struct match_attr *a = stack->attrs[i]; + if (a->is_macro) + continue; + if (path_matches(path, pathlen, basename_offset, + &a->u.pat, base, stack->originlen)) + rem = fill_one("fill", all_attrs, a, rem); + } } + return rem; } @@ -971,7 +1080,6 @@ static void determine_macros(struct all_attrs_item *all_attrs, */ static void collect_some_attrs(const char *path, struct attr_check *check) { - struct attr_stack *stk; int i, pathlen, rem, dirlen; const char *cp, *last_slash = NULL; int basename_offset; @@ -989,9 +1097,9 @@ static void collect_some_attrs(const char *path, struct attr_check *check) dirlen = 0; } - prepare_attr_stack(path, dirlen); + prepare_attr_stack(path, dirlen, &check->stack); all_attrs_init(&g_attr_hashmap, check); - determine_macros(check->all_attrs, attr_stack); + determine_macros(check->all_attrs, check->stack); if (check->nr) { rem = 0; @@ -1008,8 +1116,7 @@ static void collect_some_attrs(const char *path, struct attr_check *check) } rem = check->all_attrs_nr; - for (stk = attr_stack; 0 < rem && stk; stk = stk->prev) - rem = fill(path, pathlen, basename_offset, stk, check->all_attrs, rem); + fill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem); } int git_check_attr(const char *path, struct attr_check *check) @@ -1056,7 +1163,7 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist direction = new; if (new != old) - drop_attr_stack(); + drop_all_attr_stacks(); use_index = istate; } @@ -1064,5 +1171,6 @@ void attr_start(void) { #ifndef NO_PTHREADS pthread_mutex_init(&g_attr_hashmap.mutex, NULL); + pthread_mutex_init(&check_vector.mutex, NULL); #endif } diff --git a/attr.h b/attr.h index abebbc1..6f4961f 100644 --- a/attr.h +++ b/attr.h @@ -4,8 +4,9 @@ /* An attribute is a pointer to this opaque structure */ struct git_attr; -/* opaque structure used internally for attribute collection */ +/* opaque structures used internally for attribute collection */ struct all_attrs_item; +struct attr_stack; /* * Given a string, return the gitattribute object that @@ -38,6 +39,7 @@ struct attr_check { struct attr_check_item *items; int all_attrs_nr; struct all_attrs_item *all_attrs; + struct attr_stack *stack; }; extern struct attr_check *attr_check_alloc(void); -- cgit v0.10.2-6-g49f6 From 0787dafdccfb09b26501293f72db74638c3834ad Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Fri, 27 Jan 2017 18:02:06 -0800 Subject: attr: push the bare repo check into read_attr() Push the bare repository check into the 'read_attr()' function. This avoids needing to have extra logic which creates an empty stack frame when inside a bare repo as a similar bit of logic already exists in the 'read_attr()' function. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index bcee092..62298ec 100644 --- a/attr.c +++ b/attr.c @@ -747,25 +747,28 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok) static struct attr_stack *read_attr(const char *path, int macro_ok) { - struct attr_stack *res; + struct attr_stack *res = NULL; - if (direction == GIT_ATTR_CHECKOUT) { + if (direction == GIT_ATTR_INDEX) { res = read_attr_from_index(path, macro_ok); - if (!res) - res = read_attr_from_file(path, macro_ok); - } - else if (direction == GIT_ATTR_CHECKIN) { - res = read_attr_from_file(path, macro_ok); - if (!res) - /* - * There is no checked out .gitattributes file there, but - * we might have it in the index. We allow operation in a - * sparsely checked out work tree, so read from it. - */ + } else if (!is_bare_repository()) { + if (direction == GIT_ATTR_CHECKOUT) { res = read_attr_from_index(path, macro_ok); + if (!res) + res = read_attr_from_file(path, macro_ok); + } else if (direction == GIT_ATTR_CHECKIN) { + res = read_attr_from_file(path, macro_ok); + if (!res) + /* + * There is no checked out .gitattributes file + * there, but we might have it in the index. + * We allow operation in a sparsely checked out + * work tree, so read from it. + */ + res = read_attr_from_index(path, macro_ok); + } } - else - res = read_attr_from_index(path, macro_ok); + if (!res) res = xcalloc(1, sizeof(*res)); return res; @@ -857,10 +860,7 @@ static void bootstrap_attr_stack(struct attr_stack **stack) } /* root directory */ - if (!is_bare_repository() || direction == GIT_ATTR_INDEX) - e = read_attr(GITATTRIBUTES_FILE, 1); - else - e = xcalloc(1, sizeof(struct attr_stack)); + e = read_attr(GITATTRIBUTES_FILE, 1); push_stack(stack, e, xstrdup(""), 0); /* info frame */ @@ -877,6 +877,7 @@ static void prepare_attr_stack(const char *path, int dirlen, struct attr_stack **stack) { struct attr_stack *info; + struct strbuf pathbuf = STRBUF_INIT; /* * At the bottom of the attribute stack is the built-in @@ -923,54 +924,47 @@ static void prepare_attr_stack(const char *path, int dirlen, } /* - * Read from parent directories and push them down + * bootstrap_attr_stack() should have added, and the + * above loop should have stopped before popping, the + * root element whose attr_stack->origin is set to an + * empty string. */ - if (!is_bare_repository() || direction == GIT_ATTR_INDEX) { - /* - * bootstrap_attr_stack() should have added, and the - * above loop should have stopped before popping, the - * root element whose attr_stack->origin is set to an - * empty string. - */ - struct strbuf pathbuf = STRBUF_INIT; - - assert((*stack)->origin); - strbuf_addstr(&pathbuf, (*stack)->origin); - /* Build up to the directory 'path' is in */ - while (pathbuf.len < dirlen) { - size_t len = pathbuf.len; - struct attr_stack *next; - char *origin; - - /* Skip path-separator */ - if (len < dirlen && is_dir_sep(path[len])) - len++; - /* Find the end of the next component */ - while (len < dirlen && !is_dir_sep(path[len])) - len++; - - if (pathbuf.len > 0) - strbuf_addch(&pathbuf, '/'); - strbuf_add(&pathbuf, path + pathbuf.len, - (len - pathbuf.len)); - strbuf_addf(&pathbuf, "/%s", GITATTRIBUTES_FILE); - - next = read_attr(pathbuf.buf, 0); - - /* reset the pathbuf to not include "/.gitattributes" */ - strbuf_setlen(&pathbuf, len); - - origin = xstrdup(pathbuf.buf); - push_stack(stack, next, origin, len); - - } - strbuf_release(&pathbuf); + assert((*stack)->origin); + + strbuf_addstr(&pathbuf, (*stack)->origin); + /* Build up to the directory 'path' is in */ + while (pathbuf.len < dirlen) { + size_t len = pathbuf.len; + struct attr_stack *next; + char *origin; + + /* Skip path-separator */ + if (len < dirlen && is_dir_sep(path[len])) + len++; + /* Find the end of the next component */ + while (len < dirlen && !is_dir_sep(path[len])) + len++; + + if (pathbuf.len > 0) + strbuf_addch(&pathbuf, '/'); + strbuf_add(&pathbuf, path + pathbuf.len, (len - pathbuf.len)); + strbuf_addf(&pathbuf, "/%s", GITATTRIBUTES_FILE); + + next = read_attr(pathbuf.buf, 0); + + /* reset the pathbuf to not include "/.gitattributes" */ + strbuf_setlen(&pathbuf, len); + + origin = xstrdup(pathbuf.buf); + push_stack(stack, next, origin, len); } /* * Finally push the "info" one at the top of the stack. */ push_stack(stack, info, NULL, 0); + + strbuf_release(&pathbuf); } static int path_matches(const char *pathname, int pathlen, -- cgit v0.10.2-6-g49f6 From f0dd042148233ad4681b29f35f3bc3ba3b962474 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Fri, 27 Jan 2017 18:02:07 -0800 Subject: attr: reformat git_attr_set_direction() function Move the 'git_attr_set_direction()' up to be closer to the variables that it modifies as well as a small formatting by renaming the variable 'new' to 'new_direction' so that it is more descriptive. Update the comment about how 'direction' is used to read the state of the world. It should be noted that callers of 'git_attr_set_direction()' should ensure that other threads are not making calls into the attribute system until after the call to 'git_attr_set_direction()' completes. This function essentially acts as reset button for the attribute system and should be handled with care. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/attr.c b/attr.c index 62298ec..5493bff 100644 --- a/attr.c +++ b/attr.c @@ -677,26 +677,30 @@ static struct attr_stack *read_attr_from_array(const char **list) } /* - * 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. + * Callers into the attribute system assume there is a single, system-wide + * global state where attributes are read from and when the state is flipped by + * calling git_attr_set_direction(), the stack frames that have been + * constructed need to be discarded so so that subsequent calls into the + * attribute system will lazily read from the right place. Since changing + * direction causes a global paradigm shift, it should not ever be called while + * another thread could potentially be calling into the attribute system. */ static enum git_attr_direction direction; static struct index_state *use_index; +void git_attr_set_direction(enum git_attr_direction new_direction, + struct index_state *istate) +{ + if (is_bare_repository() && new_direction != GIT_ATTR_INDEX) + die("BUG: non-INDEX attr direction in a bare repo"); + + if (new_direction != direction) + drop_all_attr_stacks(); + + direction = new_direction; + use_index = istate; +} + static struct attr_stack *read_attr_from_file(const char *path, int macro_ok) { FILE *fp = fopen(path, "r"); @@ -1148,19 +1152,6 @@ void git_all_attrs(const char *path, struct attr_check *check) } } -void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate) -{ - enum git_attr_direction old = direction; - - if (is_bare_repository() && new != GIT_ATTR_INDEX) - die("BUG: non-INDEX attr direction in a bare repo"); - - direction = new; - if (new != old) - drop_all_attr_stacks(); - use_index = istate; -} - void attr_start(void) { #ifndef NO_PTHREADS diff --git a/attr.h b/attr.h index 6f4961f..48ab3e1 100644 --- a/attr.h +++ b/attr.h @@ -72,7 +72,8 @@ enum git_attr_direction { GIT_ATTR_CHECKOUT, GIT_ATTR_INDEX }; -void git_attr_set_direction(enum git_attr_direction, struct index_state *); +void git_attr_set_direction(enum git_attr_direction new_direction, + struct index_state *istate); extern void attr_start(void); -- cgit v0.10.2-6-g49f6