* [PATCH 16/27] attr: convert git_all_attrs() to use "struct attr_check"
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, pclouds, sbeller, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 38 ++++++++++++---------------------
attr.h | 9 +++-----
builtin/check-attr.c | 60 ++++++++++++++++++++++++++--------------------------
3 files changed, 47 insertions(+), 60 deletions(-)
diff --git a/attr.c b/attr.c
index be9e398e9..d2eaa0410 100644
--- a/attr.c
+++ b/attr.c
@@ -837,42 +837,32 @@ 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->check_nr, check->check);
- /* 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;
}
-struct attr_check *attr_check_alloc(void)
+int git_check_attr(const char *path, struct attr_check *check)
{
- return xcalloc(1, sizeof(struct attr_check));
+ return git_check_attrs(path, check->check_nr, check->check);
}
-int git_check_attr(const char *path, struct attr_check *check)
+struct attr_check *attr_check_alloc(void)
{
- return git_check_attrs(path, check->check_nr, check->check);
+ return xcalloc(1, sizeof(struct attr_check));
}
struct attr_check *attr_check_initl(const char *one, ...)
diff --git a/attr.h b/attr.h
index 459347f4b..971bb9a38 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);
+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 889264a5b..3d4704be5 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->check_nr;
+
for (j = 0; j < cnt; j++) {
- const char *value = check[j].value;
+ const char *value = check->check[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->check[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->check[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;
}
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 23/27] attr: remove maybe-real, maybe-macro from git_attr
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, gitster, pclouds, sbeller
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
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 <bmwill@google.com>
---
attr.c | 69 ++++++++++++++++++++++++++++++------------------------------------
attr.h | 6 ++++++
2 files changed, 37 insertions(+), 38 deletions(-)
diff --git a/attr.c b/attr.c
index 38b0d4347..633a12cc3 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;
@@ -182,6 +171,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;
}
}
@@ -233,8 +223,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));
@@ -397,7 +385,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);
@@ -418,10 +405,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);
@@ -826,7 +809,7 @@ static int path_matches(const char *pathname, int pathlen,
static int macroexpand_one(struct attr_check_item *all_attrs, int nr, int rem);
static int fill_one(const char *what, struct attr_check_item *all_attrs,
- struct match_attr *a, int rem)
+ const struct match_attr *a, int rem)
{
int i;
@@ -867,24 +850,34 @@ static int fill(const char *path, int pathlen, int basename_offset,
static int macroexpand_one(struct attr_check_item *all_attrs, int nr, int rem)
{
- struct attr_stack *stk;
- int i;
+ const struct attr_check_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 attr_check_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;
}
/*
@@ -914,15 +907,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->check_nr && !cannot_trust_maybe_real) {
+ if (check->check_nr) {
rem = 0;
for (i = 0; i < check->check_nr; i++) {
- const struct git_attr *a = check->check[i].attr;
- if (!a->maybe_real) {
- struct attr_check_item *c;
- c = check->all_attrs + a->attr_nr;
- c->value = ATTR__UNSET;
+ int n = check->check[i].attr->attr_nr;
+ struct attr_check_item *item = &check->all_attrs[n];
+ if (item->macro) {
+ item->value = ATTR__UNSET;
rem++;
}
}
diff --git a/attr.h b/attr.h
index 44b21d82c..f40524875 100644
--- a/attr.h
+++ b/attr.h
@@ -27,6 +27,12 @@ extern const char git_attr__false[];
struct attr_check_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;
};
struct attr_check {
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 24/27] attr: tighten const correctness with git_attr and match_attr
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, gitster, pclouds, sbeller
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 14 +++++++-------
attr.h | 2 +-
builtin/check-attr.c | 3 ++-
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/attr.c b/attr.c
index 633a12cc3..90f576044 100644
--- a/attr.c
+++ b/attr.c
@@ -209,7 +209,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;
@@ -233,14 +233,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;
};
@@ -267,7 +267,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;
@@ -814,7 +814,7 @@ static int fill_one(const char *what, struct attr_check_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;
@@ -838,7 +838,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,
@@ -988,7 +988,7 @@ struct attr_check *attr_check_initl(const char *one, ...)
check->check[0].attr = git_attr(one);
va_start(params, one);
for (cnt = 1; cnt < check->check_nr; cnt++) {
- struct git_attr *attr;
+ const struct git_attr *attr;
param = va_arg(params, const char *);
if (!param)
die("BUG: counted %d != ended at %d",
diff --git a/attr.h b/attr.h
index f40524875..9b4dc07d8 100644
--- a/attr.h
+++ b/attr.h
@@ -8,7 +8,7 @@ struct git_attr;
* 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 3d4704be5..cc6caf7ac 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]);
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 21/27] attr: use hashmap for attribute dictionary
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, gitster, pclouds, sbeller
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
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 <bmwill@google.com>
---
attr.c | 171 ++++++++++++++++++++++++++++++++++++++++++----------------
attr.h | 2 +
common-main.c | 3 ++
3 files changed, 131 insertions(+), 45 deletions(-)
diff --git a/attr.c b/attr.c
index 5399e1cb3..8cf2ea901 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)
{
- unsigned val = 0, c;
+#ifndef NO_PTHREADS
+ pthread_mutex_lock(&map->mutex);
+#endif
+}
- while (namelen--) {
- c = *name++;
- val = ((val << 7) | (val >> 22)) ^ c;
- }
- return val;
+static inline void hashmap_unlock(struct attr_hashmap *map)
+{
+#ifndef NO_PTHREADS
+ pthread_mutex_unlock(&map->mutex);
+#endif
+}
+
+/*
+ * 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 */
+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,8 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
drop_attr_stack();
use_index = istate;
}
+
+void attr_start(void)
+{
+ pthread_mutex_init(&g_attr_hashmap.mutex, NULL);
+}
diff --git a/attr.h b/attr.h
index 3db9893ef..8505bca79 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 c654f9555..6a689007e 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();
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 25/27] attr: store attribute stacks in hashmap
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, gitster, pclouds, sbeller
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
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 retrieved or
constructed locally. Since each of these stacks is only used as a
read-only structure once constructed, they can be stored in a hashmap
and shared between threads. The key into the hashmap of attribute
stacks is, in the general case, the directory that corresponds to the
attribute stack frame. For the core stack frames (builtin, system,
home, and info) a key of ".git/<name>-attr" is used to prevent potential
collisions since a directory or file named ".git" is disallowed.
One caveat with storing and sharing the stack frames like this is that
the info stack needs to be treated separately from the rest of the
attribute stack. This is because each stack frame holds a pointer to
the stack that comes before it and if it was placed on top of the rest
of the attribute stack then this pointer would be different for each
attribute stack and wouldn't be able to be shared between threads. In
order to allow for sharing the info stack frame it needs to be its own
isolated frame and can simply be processed first to have the same affect
of being at the top of the stack.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 375 +++++++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 235 insertions(+), 140 deletions(-)
diff --git a/attr.c b/attr.c
index 90f576044..78562592b 100644
--- a/attr.c
+++ b/attr.c
@@ -434,17 +434,19 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
* .gitignore file and info/excludes file as a fallback.
*/
-/* NEEDSWORK: This will become per git_attr_check */
-static struct attr_stack {
- struct attr_stack *prev;
+struct attr_stack {
+ const struct attr_stack *prev;
char *origin;
size_t originlen;
unsigned num_matches;
unsigned alloc;
struct match_attr **attrs;
-} *attr_stack;
+};
+
+/* Dictionary of stack frames; access should be surrounded by mutex */
+static struct attr_hashmap g_stack_hashmap;
-static void free_attr_elem(struct attr_stack *e)
+static void attr_stack_free(struct attr_stack *e)
{
int i;
free(e->origin);
@@ -467,6 +469,25 @@ static void free_attr_elem(struct attr_stack *e)
free(e);
}
+static void drop_attr_stack(void)
+{
+ struct hashmap_iter iter;
+ struct attr_hash_entry *e;
+
+ hashmap_lock(&g_stack_hashmap);
+
+ hashmap_iter_init(&g_stack_hashmap.map, &iter);
+ while ((e = hashmap_iter_next(&iter))) {
+ struct attr_stack *stack = e->value;
+ attr_stack_free(stack);
+ free(e);
+ }
+
+ hashmap_free(&g_stack_hashmap.map, 0);
+
+ hashmap_unlock(&g_stack_hashmap);
+}
+
static const char *builtin_attr[] = {
"[attr]binary -diff -merge -text",
NULL,
@@ -621,15 +642,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;
@@ -638,6 +650,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);
@@ -645,142 +665,208 @@ 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)
+/*
+ * This funciton should only be called from 'get_attr_stack()' or
+ * 'get_info_stack()', which already needs to acquire the lock to the stack
+ * hashmap, so there is no need to also acquire the lock in this function.
+ */
+static void push_stack(const struct attr_stack **attr_stack_p,
+ struct attr_stack *elem,
+ const char *origin, size_t originlen)
{
if (elem) {
- elem->origin = origin;
- if (origin)
- elem->originlen = originlen;
+ elem->origin = xmemdupz(origin, originlen);
+ elem->originlen = originlen;
elem->prev = *attr_stack_p;
*attr_stack_p = elem;
+ attr_hashmap_add(&g_stack_hashmap, elem->origin,
+ elem->originlen, elem);
}
}
-static void bootstrap_attr_stack(void)
+/*
+ * Return the path base that can be used in the pattern matching operation. In
+ * order to enable storing the core and info stack frames in the stack hashmap
+ * an origin string other than NULL needed to be used. Since git disallows
+ * tracking a ".git" file or directory the core and info stack frames have an
+ * origin string of ".git/<frame>" and must be converted to the empty string
+ * when being used to pattern match.
+ */
+static const char *attr_stack_get_base(const struct attr_stack *stack,
+ size_t *baselen)
{
- struct attr_stack *elem;
+ const char *base;
- if (attr_stack)
- return;
+ if (starts_with(stack->origin, ".git/")) {
+ base = "";
+ *baselen = 0;
+ } else {
+ base = stack->origin;
+ *baselen = stack->originlen;
+ }
- push_stack(&attr_stack, read_attr_from_array(builtin_attr), NULL, 0);
+ return base;
+}
- if (git_attr_system())
- push_stack(&attr_stack,
- read_attr_from_file(git_etc_gitattributes(), 1),
- NULL, 0);
+/*
+ * At the bottom of the attribute stack is the built-in
+ * set of attribute definitions, followed by the contents
+ * of $(prefix)/etc/gitattributes and a file specified by
+ * core.attributesfile. Then, contents from
+ * .gitattribute files from directories closer to the
+ * root to the ones in deeper directories are pushed
+ * to the stack. Finally, at the very top of the stack
+ * we always keep the contents of $GIT_DIR/info/attributes.
+ *
+ * When checking, we use entries from near the top of the
+ * stack, preferring $GIT_DIR/info/attributes, then
+ * .gitattributes in deeper directories to shallower ones,
+ * and finally use the built-in set as the default.
+ *
+ * The info stack needs to be treated separately from the rest of the attribute
+ * stack. This is because each stack frame holds a pointer to the stack that
+ * comes before it and if it was placed on top of the rest of the attribute
+ * stack then this pointer would be different for each attribute stack and
+ * wouldn't be able to be shared between threads. If the info stack is to be
+ * shared then it needs to be its own isolated frame and can simply be
+ * processed first to have the same affect of being at the top of the stack.
+ */
+static const struct attr_stack *get_info_stack(void)
+{
+ const struct attr_stack *info;
+ const char *key = ".git/info-attr";
+ size_t keylen = strlen(key);
- 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);
-
- if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
- elem = read_attr(GITATTRIBUTES_FILE, 1);
- push_stack(&attr_stack, elem, xstrdup(""), 0);
- debug_push(elem);
+ hashmap_lock(&g_stack_hashmap);
+
+ info = attr_hashmap_get(&g_stack_hashmap, key, keylen);
+
+ if (!info) {
+ struct attr_stack *e = NULL;
+
+ if (startup_info->have_repository)
+ e = read_attr_from_file(git_path_info_attributes(), 1);
+
+ if (!e)
+ e = xcalloc(1, sizeof(struct attr_stack));
+ e->origin = xstrdup(key);
+ e->originlen = keylen;
+
+ attr_hashmap_add(&g_stack_hashmap, e->origin, e->originlen, e);
+ info = e;
}
- if (startup_info->have_repository)
- elem = read_attr_from_file(git_path_info_attributes(), 1);
- else
- elem = NULL;
+ hashmap_unlock(&g_stack_hashmap);
- if (!elem)
- elem = xcalloc(1, sizeof(*elem));
- push_stack(&attr_stack, elem, NULL, 0);
+ return info;
}
-static void prepare_attr_stack(const char *path, int dirlen)
+/*
+ * This funciton should only be called from 'get_attr_stack()', which already
+ * needs to acquire the lock to the stack hashmap, so there is no need to also
+ * acquire the lock in this function.
+ */
+static const struct attr_stack *core_attr_stack(void)
{
- struct attr_stack *elem, *info;
- const char *cp;
+ const struct attr_stack *core;
- /*
- * At the bottom of the attribute stack is the built-in
- * set of attribute definitions, followed by the contents
- * of $(prefix)/etc/gitattributes and a file specified by
- * core.attributesfile. Then, contents from
- * .gitattribute files from directories closer to the
- * root to the ones in deeper directories are pushed
- * to the stack. Finally, at the very top of the stack
- * we always keep the contents of $GIT_DIR/info/attributes.
- *
- * When checking, we use entries from near the top of the
- * stack, preferring $GIT_DIR/info/attributes, then
- * .gitattributes in deeper directories to shallower ones,
- * and finally use the built-in set as the default.
- */
- bootstrap_attr_stack();
+ core = attr_hashmap_get(&g_stack_hashmap, "", 0);
- /*
- * Pop the "info" one that is always at the top of the stack.
- */
- info = attr_stack;
- attr_stack = info->prev;
+ if (!core) {
+ struct attr_stack *e;
+ const char *key;
- /*
- * Pop the ones from directories that are not the prefix of
- * the path we are checking. Break out of the loop when we see
- * 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);
-
- elem = attr_stack;
- if (namelen <= dirlen &&
- !strncmp(elem->origin, path, namelen) &&
- (!namelen || path[namelen] == '/'))
- break;
-
- debug_pop(elem);
- attr_stack = elem->prev;
- free_attr_elem(elem);
- }
+ /* builtin frame */
+ e = read_attr_from_array(builtin_attr);
+ key = ".git/builtin-attr";
+ push_stack(&core, e, key, strlen(key));
- /*
- * Read from parent directories and push them down
- */
- 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(attr_stack->origin);
- while (1) {
- 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_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);
+ /* system-wide frame */
+ if (git_attr_system()) {
+ e = read_attr_from_file(git_etc_gitattributes(), 1);
+ key = ".git/system-attr";
+ push_stack(&core, e, key, strlen(key));
}
- strbuf_release(&pathbuf);
+ /* home directory */
+ if (get_home_gitattributes()) {
+ e = read_attr_from_file(get_home_gitattributes(), 1);
+ key = ".git/home-attr";
+ push_stack(&core, e, key, strlen(key));
+ }
+
+ /* root directory */
+ if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
+ e = read_attr(GITATTRIBUTES_FILE, 1);
+ } else {
+ e = xcalloc(1, sizeof(struct attr_stack));
+ }
+ key = "";
+ push_stack(&core, e, key, strlen(key));
}
- /*
- * Finally push the "info" one at the top of the stack.
- */
- push_stack(&attr_stack, info, NULL, 0);
+ assert(core);
+ return core;
+}
+
+static const struct attr_stack *get_attr_stack(const char *path, int dirlen)
+{
+ const struct attr_stack *stack = NULL;
+ struct strbuf key = STRBUF_INIT;
+
+ strbuf_addstr(&key, path);
+
+ hashmap_lock(&g_stack_hashmap);
+
+ /* Search for the deepest, pre-constructed stack frame */
+ while (key.len && !stack) {
+ size_t len = key.len;
+
+ /* Find start of the last component */
+ while (len > 0 && !is_dir_sep(key.buf[len - 1]))
+ len--;
+ /* Skip path-separator */
+ if (len > 0 && is_dir_sep(key.buf[len - 1]))
+ len--;
+ strbuf_setlen(&key, len);
+
+ stack = attr_hashmap_get(&g_stack_hashmap, key.buf, key.len);
+ }
+
+ /* At least start with the core stack */
+ if (!stack) {
+ stack = core_attr_stack();
+ }
+
+ /* Build up to the directory 'path' is in */
+ while (key.len < dirlen) {
+ size_t len = key.len;
+ struct attr_stack *next;
+
+ /* 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 (key.len > 0)
+ strbuf_addch(&key, '/');
+ strbuf_add(&key, path + key.len, (len - key.len));
+ strbuf_addf(&key, "/%s", GITATTRIBUTES_FILE);
+
+ next = read_attr(key.buf, 0);
+
+ /* reset the keybuffer to not include "/.gitattributes" */
+ strbuf_setlen(&key, len);
+
+ push_stack(&stack, next, key.buf, key.len);
+ }
+
+ hashmap_unlock(&g_stack_hashmap);
+
+ strbuf_release(&key);
+ return stack;
}
static int path_matches(const char *pathname, int pathlen,
@@ -831,20 +917,24 @@ static int fill_one(const char *what, struct attr_check_item *all_attrs,
}
static int fill(const char *path, int pathlen, int basename_offset,
- struct attr_stack *stk, struct attr_check_item *all_attrs,
+ const struct attr_stack *stack, struct attr_check_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;
+ size_t baselen;
+ const char *base = attr_stack_get_base(stack, &baselen);
+
+ for (i = stack->num_matches - 1; rem > 0 && i >= 0; 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, baselen))
+ rem = fill_one("fill", all_attrs, a, rem);
+ }
}
+
return rem;
}
@@ -887,10 +977,11 @@ static void determine_macros(struct attr_check_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;
+ const struct attr_stack *stack;
+ const struct attr_stack *info;
for (cp = path; *cp; cp++) {
if (*cp == '/' && cp[1])
@@ -905,9 +996,12 @@ static void collect_some_attrs(const char *path, struct attr_check *check)
dirlen = 0;
}
- prepare_attr_stack(path, dirlen);
+ info = get_info_stack();
+ stack = get_attr_stack(path, dirlen);
+
all_attrs_init(&g_attr_hashmap, check);
- determine_macros(check->all_attrs, attr_stack);
+ determine_macros(check->all_attrs, info);
+ determine_macros(check->all_attrs, stack);
if (check->check_nr) {
rem = 0;
@@ -924,8 +1018,8 @@ 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);
+ rem = fill(path, pathlen, basename_offset, info, check->all_attrs, rem);
+ fill(path, pathlen, basename_offset, stack, check->all_attrs, rem);
}
int git_check_attr(const char *path, struct attr_check *check)
@@ -1052,4 +1146,5 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
void attr_start(void)
{
pthread_mutex_init(&g_attr_hashmap.mutex, NULL);
+ pthread_mutex_init(&g_stack_hashmap.mutex, NULL);
}
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 22/27] attr: eliminate global check_all_attr array
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, gitster, pclouds, sbeller
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
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 <bmwill@google.com>
---
attr.c | 114 +++++++++++++++++++++++++++++++++++++++++++----------------------
attr.h | 2 ++
2 files changed, 78 insertions(+), 38 deletions(-)
diff --git a/attr.c b/attr.c
index 8cf2ea901..38b0d4347 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,52 @@ static void attr_hashmap_add(struct attr_hashmap *map,
hashmap_add(&map->map, e);
}
+/*
+ * 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 +238,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);
@@ -791,16 +823,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 attr_check_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 attr_check_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) {
@@ -809,14 +841,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 attr_check_item *all_attrs,
+ int rem)
{
int i;
const char *base = stk->origin ? stk->origin : "";
@@ -827,18 +860,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 attr_check_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) {
@@ -847,7 +880,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);
}
}
@@ -855,9 +888,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)
{
@@ -880,15 +913,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->check_nr && !cannot_trust_maybe_real) {
rem = 0;
for (i = 0; i < check->check_nr; i++) {
const struct git_attr *a = check->check[i].attr;
if (!a->maybe_real) {
struct attr_check_item *c;
- c = check_all_attr + a->attr_nr;
+ c = check->all_attrs + a->attr_nr;
c->value = ATTR__UNSET;
rem++;
}
@@ -897,9 +930,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)
@@ -909,7 +942,8 @@ int git_check_attr(const char *path, struct attr_check *check)
collect_some_attrs(path, check);
for (i = 0; i < check->check_nr; i++) {
- const char *value = check_all_attr[check->check[i].attr->attr_nr].value;
+ size_t index = check->check[i].attr->attr_nr;
+ const char *value = check->all_attrs[index].value;
if (value == ATTR__UNKNOWN)
value = ATTR__UNSET;
check->check[i].value = value;
@@ -925,9 +959,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;
@@ -997,6 +1031,10 @@ void attr_check_clear(struct attr_check *check)
check->check = NULL;
check->check_alloc = 0;
check->check_nr = 0;
+
+ free(check->all_attrs);
+ check->all_attrs = NULL;
+ check->all_attrs_nr = 0;
}
void attr_check_free(struct attr_check *check)
diff --git a/attr.h b/attr.h
index 8505bca79..44b21d82c 100644
--- a/attr.h
+++ b/attr.h
@@ -33,6 +33,8 @@ struct attr_check {
int check_nr;
int check_alloc;
struct attr_check_item *check;
+ int all_attrs_nr;
+ struct attr_check_item *all_attrs;
};
extern struct attr_check *attr_check_alloc(void);
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 27/27] attr: reformat git_attr_set_direction() function
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, gitster, pclouds, sbeller
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
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 <bmwill@google.com>
---
attr.c | 49 ++++++++++++++++++++-----------------------------
attr.h | 3 ++-
2 files changed, 22 insertions(+), 30 deletions(-)
diff --git a/attr.c b/attr.c
index cbb07d25d..f5cc68b67 100644
--- a/attr.c
+++ b/attr.c
@@ -521,26 +521,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_attr_stack();
+
+ 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");
@@ -1130,19 +1134,6 @@ void attr_check_free(struct attr_check *check)
free(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_attr_stack();
- use_index = istate;
-}
-
void attr_start(void)
{
pthread_mutex_init(&g_attr_hashmap.mutex, NULL);
diff --git a/attr.h b/attr.h
index 9b4dc07d8..b8be37c91 100644
--- a/attr.h
+++ b/attr.h
@@ -73,7 +73,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);
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 26/27] attr: push the bare repo check into read_attr()
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, gitster, pclouds, sbeller
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
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 <bmwill@google.com>
---
attr.c | 40 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/attr.c b/attr.c
index 78562592b..cbb07d25d 100644
--- a/attr.c
+++ b/attr.c
@@ -591,25 +591,29 @@ 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;
@@ -796,11 +800,7 @@ static const struct attr_stack *core_attr_stack(void)
}
/* 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);
key = "";
push_stack(&core, e, key, strlen(key));
}
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 13/27] attr.c: outline the future plans by heavily commenting
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, pclouds, sbeller, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 40 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 8026d68bd..50e5ee393 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 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
* .gitignore file and info/excludes file as a fallback.
*/
+/* NEEDSWORK: This will become per git_attr_check */
static struct attr_stack {
struct attr_stack *prev;
char *origin;
@@ -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;
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 12/27] Documentation: fix a typo
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Stefan Beller, gitster, pclouds, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Stefan Beller <sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/gitattributes.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 3173dee7e..a53d093ca 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 `!`.
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 01/27] commit.c: use strchrnul() to scan for one line
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, pclouds, sbeller, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
commit.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/commit.c b/commit.c
index 2cf85158b..0c4ee3de4 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;
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 02/27] attr.c: use strchrnul() to scan for one line
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, pclouds, sbeller, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/attr.c b/attr.c
index 1fcf042b8..04d24334e 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);
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 11/27] attr.c: add push_stack() helper
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, pclouds, sbeller, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 71 +++++++++++++++++++++++++++++++-----------------------------------
1 file changed, 33 insertions(+), 38 deletions(-)
diff --git a/attr.c b/attr.c
index e1c630f79..8026d68bd 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,
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 08/27] attr.c: tighten constness around "git_attr" structure
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, pclouds, sbeller, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 2 +-
attr.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index e42f931b3..f7cf7ae30 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 8b08d33af..00d7a662c 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 *);
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 00/27] Revamp the attribute system; another round
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, gitster, pclouds, sbeller
This series has been bounced around a bit (from Junio to Stefan) and finally
landed in my lap. The end result of Stefan's attempt at the series still had a
couple of things that needed more tweaking. It also has a few patches on top
which added functionality to pathspecs to be able to query into the attribute
system, which I've dropped from this series due to this series' length.
As a reminder the intent of this series is to revamp the attribute system so
that it can be thread-safe as well as a couple of other quality of life
changes. This entailed removing dependencies on writing global data structures
during the attribute collection process. Major changes are as follows:
* The global array used to collect attributes needed to be made local and as a
result was pushed out to the attr_check structure the caller prepares before
querying the attribute system.
* As it turns out the attribute stack ends up being used as a read-only
structure during the collection process and as such parts of the attribute
stack can be shared between different threads calling into the system. To
enable this sharing the attribute stack frames are stored in a hashmap and
can be read out (or created and stored in the hashmap) based on the
directory name of the path being queried. This is possible because if a
particular stack frame is included in the overall stack for a particular
query, all of the frames underneath it will be the same for all queries that
use this frame (only exception is the info frame which is handled special
case, see the patch for details).
I took many of the first patches of this series as is from the series Stefan
prepared as as such may only need a cursory glace. I did modify and change
some of the later patches authored by Junio to address a couple of naming
changes and to redistribute some code between patches so those patches would
need a closer look.
Thanks again to all the work Junio and Stefan put into this before I got a hold
of it.
Any comments are appreciated!
Thanks,
Brandon Williams
Brandon Williams (8):
attr: pass struct attr_check to collect_some_attrs
attr: use hashmap for attribute dictionary
attr: eliminate global check_all_attr array
attr: remove maybe-real, maybe-macro from git_attr
attr: tighten const correctness with git_attr and match_attr
attr: store attribute stacks in hashmap
attr: push the bare repo check into read_attr()
attr: reformat git_attr_set_direction() function
Junio C Hamano (17):
commit.c: use strchrnul() to scan for one line
attr.c: use strchrnul() to scan for one line
attr.c: update a stale comment on "struct match_attr"
attr.c: explain the lack of attr-name syntax check in parse_attr()
attr.c: complete a sentence in a comment
attr.c: mark where #if DEBUG ends more clearly
attr.c: simplify macroexpand_one()
attr.c: tighten constness around "git_attr" structure
attr.c: plug small leak in parse_attr_line()
attr.c: add push_stack() helper
attr.c: outline the future plans by heavily commenting
attr: rename function and struct related to checking attributes
attr: (re)introduce git_check_attr() and struct attr_check
attr: convert git_all_attrs() to use "struct attr_check"
attr: convert git_check_attrs() callers to use the new API
attr: retire git_check_attrs() API
attr: change validity check for attribute names to use positive logic
Nguyễn Thái Ngọc Duy (1):
attr: support quoting pathname patterns in C style
Stefan Beller (1):
Documentation: fix a typo
Documentation/gitattributes.txt | 10 +-
Documentation/technical/api-gitattributes.txt | 86 ++-
archive.c | 24 +-
attr.c | 932 +++++++++++++++++---------
attr.h | 50 +-
builtin/check-attr.c | 66 +-
builtin/pack-objects.c | 19 +-
commit.c | 3 +-
common-main.c | 3 +
convert.c | 25 +-
ll-merge.c | 33 +-
t/t0003-attributes.sh | 26 +
userdiff.c | 19 +-
ws.c | 19 +-
14 files changed, 834 insertions(+), 481 deletions(-)
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply
* [PATCH 14/27] attr: rename function and struct related to checking attributes
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, pclouds, sbeller, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
archive.c | 6 +++---
attr.c | 12 ++++++------
attr.h | 8 ++++----
builtin/check-attr.c | 19 ++++++++++---------
builtin/pack-objects.c | 6 +++---
convert.c | 12 ++++++------
ll-merge.c | 10 +++++-----
userdiff.c | 4 ++--
ws.c | 6 +++---
9 files changed, 42 insertions(+), 41 deletions(-)
diff --git a/archive.c b/archive.c
index 01751e574..b76bd4691 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 50e5ee393..2f180d609 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 00d7a662c..efc7bb3b3 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 53a5a18c1..889264a5b 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 8841f8b36..8b8fbd814 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 4e17e45ed..1b9829279 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 ad8be42f9..198f07aca 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 2125d6da2..b0b44467a 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 ea4b2b1df..fbd876e84 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;
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 06/27] attr.c: mark where #if DEBUG ends more clearly
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, pclouds, sbeller, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 9bdf87a6f..17297fffe 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)
{
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 07/27] attr.c: simplify macroexpand_one()
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, pclouds, sbeller, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/attr.c b/attr.c
index 17297fffe..e42f931b3 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;
}
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 10/27] attr: support quoting pathname patterns in C style
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy, gitster, sbeller,
Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
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 <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/gitattributes.txt | 8 +++++---
attr.c | 15 +++++++++++++--
t/t0003-attributes.sh | 26 ++++++++++++++++++++++++++
3 files changed, 44 insertions(+), 5 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e0b66c122..3173dee7e 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 d180c7833..e1c630f79 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 f0fbb4255..f19ae4f8c 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 &&
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 09/27] attr.c: plug small leak in parse_attr_line()
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, pclouds, sbeller, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/attr.c b/attr.c
index f7cf7ae30..d180c7833 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;
}
/*
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 04/27] attr.c: explain the lack of attr-name syntax check in parse_attr()
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, pclouds, sbeller, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/attr.c b/attr.c
index 007f1a299..6b55a57ef 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++;
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 05/27] attr.c: complete a sentence in a comment
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, pclouds, sbeller, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 6b55a57ef..9bdf87a6f 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 {
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 03/27] attr.c: update a stale comment on "struct match_attr"
From: Brandon Williams @ 2017-01-12 23:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, pclouds, sbeller, Brandon Williams
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index 04d24334e..007f1a299 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
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* Re: Bug report: Documentation error in git-bisect man description
From: Manuel Ullmann @ 2017-01-13 0:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqq8tqfj15z.fsf@gitster.mtv.corp.google.com>
I see. Thanks for the clarification. The pairing not being pairs of
opposites was indeed, what confused me. So that description was not
meant in the sense, that you use these pairs when working with bisect, but
instead they are ordered according to the argument possibilities.
Sorry for spreading confusion. I think the second paragraph should be
sufficient for documentation.
Best regards
Manuel
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Manuel Ullmann <ullman.alias@posteo.de> writes:
>>
>> Hmmm, I tend to agree, modulo a minor fix.
>>
>> If the description were in a context inside a paragraph like this:
>>
>> When you want to tell 'git bisect' that a <rev> belongs to
>> the newer half of the history, you say
>>
>> git bisect (bad|new) [<rev>]
>>
>> On the other hand, when you want to tell 'git bisect' that a
>> <rev> belongs to the older half of the history, you can say
>>
>> git bisect (good|old) [<rev>]
>>
>> then the pairing we see in the current text makes quite a lot of
>> sense.
>
> Actually, the above is _exactly_ what was intended. I misread the
> current documentation when I made the comment, and I think that the
> current one _IS_ correct. The latter half of the above is not about
> a single rev. You can paint multiple commits with the "older half"
> color, i.e.
>
> On the other hand, when you want to tell 'git bisect' that
> one or more <rev>s belong to the older half of the history,
> you can say
>
> git bisect (good|old) [<rev>...]
>
> In contrast, you can mark only one <rev> as newer (or "already
> bad"). So pairing (bad|good) and (new|old) like you suggested
> breaks the correctness of the command line description.
>
> If (bad|new) and (good|old) bothers you because they may mislead the
> readers to think bad is an opposite of new (and good is an opposite
> of old), the only solution I can think of to that problem is to
> expand these two lines into four and list them like this:
>
> git bisect bad [<rev>]
> git bisect good [<rev>...]
> git bisect new [<rev>]
> git bisect old [<rev>...]
^ permalink raw reply
* Re: [PATCH 1/5] doc: add documentation for OPT_STRING_LIST
From: Jacob Keller @ 2017-01-13 0:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <alpine.DEB.2.20.1701121044120.3469@virtualbox>
On Thu, Jan 12, 2017 at 1:47 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Jake,
>
> On Wed, 11 Jan 2017, Jacob Keller wrote:
>
>> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
>> index 27bd701c0d68..15e876e4c804 100644
>> --- a/Documentation/technical/api-parse-options.txt
>> +++ b/Documentation/technical/api-parse-options.txt
>> @@ -168,6 +168,11 @@ There are some macros to easily define options:
>> Introduce an option with string argument.
>> The string argument is put into `str_var`.
>>
>> +`OPT_STRING_LIST(short, long, &list, arg_str, description)`::
>> + Introduce an option with a string argument. Repeated invocations
>> + accumulate into a list of strings. Reset and clear the list with
>> + `--no-option`.
>
> One suggestions: as the list parameter is not type-safe (apart from
> checking that it can be cast to a `void *`), it would be good to mention
> in the documentation that `list` must be of type `struct string_list`.
>
Ok.
> I was about to suggest that `--no-option` may be misleading, as the
> command-line option is not really called `--option` in almost all cases,
> but I see that the rest of that document uses that convention to refer to
> the negated option already...
Also, I am merely documenting what already existed, since the original
commit failed to add documentation. I don't know if we could make a
better implementation, but it certainly would be a behavior change for
the current users.
Thanks,
Jake
>
> Ciao,
> Dscho
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox