* [PATCH v3 16/27] attr: convert git_all_attrs() to use "struct attr_check"
From: Brandon Williams @ 2017-01-28 2:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170128020207.179015-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 | 168 ++++++++++++++++++++++++---------------------------
attr.h | 9 +--
builtin/check-attr.c | 60 +++++++++---------
3 files changed, 112 insertions(+), 125 deletions(-)
diff --git a/attr.c b/attr.c
index de8bf35a3..40818246f 100644
--- a/attr.c
+++ b/attr.c
@@ -132,75 +132,6 @@ struct git_attr *git_attr(const char *name)
return git_attr_internal(name, strlen(name));
}
-struct attr_check *attr_check_alloc(void)
-{
- return xcalloc(1, sizeof(struct attr_check));
-}
-
-struct attr_check *attr_check_initl(const char *one, ...)
-{
- struct attr_check *check;
- int cnt;
- va_list params;
- const char *param;
-
- va_start(params, one);
- for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
- ;
- va_end(params);
-
- check = attr_check_alloc();
- check->nr = cnt;
- check->alloc = cnt;
- check->items = xcalloc(cnt, sizeof(struct attr_check_item));
-
- check->items[0].attr = git_attr(one);
- va_start(params, one);
- for (cnt = 1; cnt < check->nr; cnt++) {
- const struct git_attr *attr;
- param = va_arg(params, const char *);
- if (!param)
- die("BUG: counted %d != ended at %d",
- check->nr, cnt);
- attr = git_attr(param);
- if (!attr)
- die("BUG: %s: not a valid attribute name", param);
- check->items[cnt].attr = attr;
- }
- va_end(params);
- return check;
-}
-
-struct attr_check_item *attr_check_append(struct attr_check *check,
- const struct git_attr *attr)
-{
- struct attr_check_item *item;
-
- ALLOC_GROW(check->items, check->nr + 1, check->alloc);
- item = &check->items[check->nr++];
- item->attr = attr;
- return item;
-}
-
-void attr_check_reset(struct attr_check *check)
-{
- check->nr = 0;
-}
-
-void attr_check_clear(struct attr_check *check)
-{
- free(check->items);
- check->items = NULL;
- check->alloc = 0;
- check->nr = 0;
-}
-
-void attr_check_free(struct attr_check *check)
-{
- attr_check_clear(check);
- free(check);
-}
-
/* What does a matched pattern decide? */
struct attr_state {
struct git_attr *attr;
@@ -439,6 +370,75 @@ static void free_attr_elem(struct attr_stack *e)
free(e);
}
+struct attr_check *attr_check_alloc(void)
+{
+ return xcalloc(1, sizeof(struct attr_check));
+}
+
+struct attr_check *attr_check_initl(const char *one, ...)
+{
+ struct attr_check *check;
+ int cnt;
+ va_list params;
+ const char *param;
+
+ va_start(params, one);
+ for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
+ ;
+ va_end(params);
+
+ check = attr_check_alloc();
+ check->nr = cnt;
+ check->alloc = cnt;
+ check->items = xcalloc(cnt, sizeof(struct attr_check_item));
+
+ check->items[0].attr = git_attr(one);
+ va_start(params, one);
+ for (cnt = 1; cnt < check->nr; cnt++) {
+ const struct git_attr *attr;
+ param = va_arg(params, const char *);
+ if (!param)
+ die("BUG: counted %d != ended at %d",
+ check->nr, cnt);
+ attr = git_attr(param);
+ if (!attr)
+ die("BUG: %s: not a valid attribute name", param);
+ check->items[cnt].attr = attr;
+ }
+ va_end(params);
+ return check;
+}
+
+struct attr_check_item *attr_check_append(struct attr_check *check,
+ const struct git_attr *attr)
+{
+ struct attr_check_item *item;
+
+ ALLOC_GROW(check->items, check->nr + 1, check->alloc);
+ item = &check->items[check->nr++];
+ item->attr = attr;
+ return item;
+}
+
+void attr_check_reset(struct attr_check *check)
+{
+ check->nr = 0;
+}
+
+void attr_check_clear(struct attr_check *check)
+{
+ free(check->items);
+ check->items = NULL;
+ check->alloc = 0;
+ check->nr = 0;
+}
+
+void attr_check_free(struct attr_check *check)
+{
+ attr_check_clear(check);
+ free(check);
+}
+
static const char *builtin_attr[] = {
"[attr]binary -diff -merge -text",
NULL,
@@ -906,32 +906,22 @@ int git_check_attrs(const char *path, int num, struct attr_check_item *check)
return 0;
}
-int git_all_attrs(const char *path, int *num, struct attr_check_item **check)
+void git_all_attrs(const char *path, struct attr_check *check)
{
- int i, count, j;
+ int i;
- collect_some_attrs(path, 0, NULL);
+ attr_check_reset(check);
+ collect_some_attrs(path, check->nr, check->items);
- /* Count the number of attributes that are set. */
- count = 0;
- for (i = 0; i < attr_nr; i++) {
- const char *value = check_all_attr[i].value;
- if (value != ATTR__UNSET && value != ATTR__UNKNOWN)
- ++count;
- }
- *num = count;
- ALLOC_ARRAY(*check, count);
- j = 0;
for (i = 0; i < attr_nr; i++) {
+ const char *name = check_all_attr[i].attr->name;
const char *value = check_all_attr[i].value;
- if (value != ATTR__UNSET && value != ATTR__UNKNOWN) {
- (*check)[j].attr = check_all_attr[i].attr;
- (*check)[j].value = value;
- ++j;
- }
+ struct attr_check_item *item;
+ if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
+ continue;
+ item = attr_check_append(check, git_attr(name));
+ item->value = value;
}
-
- return 0;
}
int git_check_attr(const char *path, struct attr_check *check)
diff --git a/attr.h b/attr.h
index e611b139a..9f2729842 100644
--- a/attr.h
+++ b/attr.h
@@ -56,13 +56,10 @@ int git_check_attrs(const char *path, int, struct attr_check_item *);
extern int git_check_attr(const char *path, struct attr_check *check);
/*
- * Retrieve all attributes that apply to the specified path. *num
- * will be set to the number of attributes on the path; **check will
- * be set to point at a newly-allocated array of git_attr_check
- * objects describing the attributes and their values. *check must be
- * free()ed by the caller.
+ * Retrieve all attributes that apply to the specified path.
+ * check holds the attributes and their values.
*/
-int git_all_attrs(const char *path, int *num, struct attr_check_item **check);
+extern void git_all_attrs(const char *path, struct attr_check *check);
enum git_attr_direction {
GIT_ATTR_CHECKIN,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 889264a5b..40cdff13e 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -24,12 +24,13 @@ static const struct option check_attr_options[] = {
OPT_END()
};
-static void output_attr(int cnt, struct attr_check_item *check,
- const char *file)
+static void output_attr(struct attr_check *check, const char *file)
{
int j;
+ int cnt = check->nr;
+
for (j = 0; j < cnt; j++) {
- const char *value = check[j].value;
+ const char *value = check->items[j].value;
if (ATTR_TRUE(value))
value = "set";
@@ -42,36 +43,38 @@ static void output_attr(int cnt, struct attr_check_item *check,
printf("%s%c" /* path */
"%s%c" /* attrname */
"%s%c" /* attrvalue */,
- file, 0, git_attr_name(check[j].attr), 0, value, 0);
+ file, 0,
+ git_attr_name(check->items[j].attr), 0, value, 0);
} else {
quote_c_style(file, NULL, stdout, 0);
- printf(": %s: %s\n", git_attr_name(check[j].attr), value);
+ printf(": %s: %s\n",
+ git_attr_name(check->items[j].attr), value);
}
-
}
}
static void check_attr(const char *prefix,
- int cnt, struct attr_check_item *check,
+ struct attr_check *check,
+ int collect_all,
const char *file)
{
char *full_path =
prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
- if (check != NULL) {
- if (git_check_attrs(full_path, cnt, check))
- die("git_check_attrs died");
- output_attr(cnt, check, file);
+
+ if (collect_all) {
+ git_all_attrs(full_path, check);
} else {
- if (git_all_attrs(full_path, &cnt, &check))
- die("git_all_attrs died");
- output_attr(cnt, check, file);
- free(check);
+ if (git_check_attr(full_path, check))
+ die("git_check_attr died");
}
+ output_attr(check, file);
+
free(full_path);
}
static void check_attr_stdin_paths(const char *prefix,
- int cnt, struct attr_check_item *check)
+ struct attr_check *check,
+ int collect_all)
{
struct strbuf buf = STRBUF_INIT;
struct strbuf unquoted = STRBUF_INIT;
@@ -85,7 +88,7 @@ static void check_attr_stdin_paths(const char *prefix,
die("line is badly quoted");
strbuf_swap(&buf, &unquoted);
}
- check_attr(prefix, cnt, check, buf.buf);
+ check_attr(prefix, check, collect_all, buf.buf);
maybe_flush_or_die(stdout, "attribute to stdout");
}
strbuf_release(&buf);
@@ -100,7 +103,7 @@ static NORETURN void error_with_usage(const char *msg)
int cmd_check_attr(int argc, const char **argv, const char *prefix)
{
- struct attr_check_item *check;
+ struct attr_check *check;
int cnt, i, doubledash, filei;
if (!is_bare_repository())
@@ -160,28 +163,25 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
error_with_usage("No file specified");
}
- if (all_attrs) {
- check = NULL;
- } else {
- check = xcalloc(cnt, sizeof(*check));
+ check = attr_check_alloc();
+ if (!all_attrs) {
for (i = 0; i < cnt; i++) {
- const char *name;
- struct git_attr *a;
- name = argv[i];
- a = git_attr(name);
+ struct git_attr *a = git_attr(argv[i]);
if (!a)
return error("%s: not a valid attribute name",
- name);
- check[i].attr = a;
+ argv[i]);
+ attr_check_append(check, a);
}
}
if (stdin_paths)
- check_attr_stdin_paths(prefix, cnt, check);
+ check_attr_stdin_paths(prefix, check, all_attrs);
else {
for (i = filei; i < argc; i++)
- check_attr(prefix, cnt, check, argv[i]);
+ check_attr(prefix, check, all_attrs, argv[i]);
maybe_flush_or_die(stdout, "attribute to stdout");
}
+
+ attr_check_free(check);
return 0;
}
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 14/27] attr: rename function and struct related to checking attributes
From: Brandon Williams @ 2017-01-28 2:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170128020207.179015-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.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 18/27] attr: retire git_check_attrs() API
From: Brandon Williams @ 2017-01-28 2:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170128020207.179015-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Since nobody uses the old API, make it file-scope static, and update
the documentation to describe the new API.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/technical/api-gitattributes.txt | 86 +++++++++++++++++----------
attr.c | 3 +-
attr.h | 1 -
3 files changed, 58 insertions(+), 32 deletions(-)
diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
index 260266867..e7cbb7c13 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -16,10 +16,15 @@ Data Structure
of no interest to the calling programs. The name of the
attribute can be retrieved by calling `git_attr_name()`.
-`struct git_attr_check`::
+`struct attr_check_item`::
- This structure represents a set of attributes to check in a call
- to `git_check_attr()` function, and receives the results.
+ This structure represents one attribute and its value.
+
+`struct attr_check`::
+
+ This structure represents a collection of `attr_check_item`.
+ It is passed to `git_check_attr()` function, specifying the
+ attributes to check, and receives their values.
Attribute Values
@@ -27,7 +32,7 @@ Attribute Values
An attribute for a path can be in one of four states: Set, Unset,
Unspecified or set to a string, and `.value` member of `struct
-git_attr_check` records it. There are three macros to check these:
+attr_check_item` records it. There are three macros to check these:
`ATTR_TRUE()`::
@@ -48,49 +53,51 @@ value of the attribute for the path.
Querying Specific Attributes
----------------------------
-* Prepare an array of `struct git_attr_check` to define the list of
- attributes you would want to check. To populate this array, you would
- need to define necessary attributes by calling `git_attr()` function.
+* Prepare `struct attr_check` using attr_check_initl()
+ function, enumerating the names of attributes whose values you are
+ interested in, terminated with a NULL pointer. Alternatively, an
+ empty `struct attr_check` can be prepared by calling
+ `attr_check_alloc()` function and then attributes you want to
+ ask about can be added to it with `attr_check_append()`
+ function.
* Call `git_check_attr()` to check the attributes for the path.
-* Inspect `git_attr_check` structure to see how each of the attribute in
- the array is defined for the path.
+* Inspect `attr_check` structure to see how each of the
+ attribute in the array is defined for the path.
Example
-------
-To see how attributes "crlf" and "indent" are set for different paths.
+To see how attributes "crlf" and "ident" are set for different paths.
-. Prepare an array of `struct git_attr_check` with two elements (because
- we are checking two attributes). Initialize their `attr` member with
- pointers to `struct git_attr` obtained by calling `git_attr()`:
+. Prepare a `struct attr_check` with two elements (because
+ we are checking two attributes):
------------
-static struct git_attr_check check[2];
+static struct attr_check *check;
static void setup_check(void)
{
- if (check[0].attr)
+ if (check)
return; /* already done */
- check[0].attr = git_attr("crlf");
- check[1].attr = git_attr("ident");
+ check = attr_check_initl("crlf", "ident", NULL);
}
------------
-. Call `git_check_attr()` with the prepared array of `struct git_attr_check`:
+. Call `git_check_attr()` with the prepared `struct attr_check`:
------------
const char *path;
setup_check();
- git_check_attr(path, ARRAY_SIZE(check), check);
+ git_check_attr(path, check);
------------
-. Act on `.value` member of the result, left in `check[]`:
+. Act on `.value` member of the result, left in `check->items[]`:
------------
- const char *value = check[0].value;
+ const char *value = check->items[0].value;
if (ATTR_TRUE(value)) {
The attribute is Set, by listing only the name of the
@@ -109,20 +116,39 @@ static void setup_check(void)
}
------------
+To see how attributes in argv[] are set for different paths, only
+the first step in the above would be different.
+
+------------
+static struct attr_check *check;
+static void setup_check(const char **argv)
+{
+ check = attr_check_alloc();
+ while (*argv) {
+ struct git_attr *attr = git_attr(*argv);
+ attr_check_append(check, attr);
+ argv++;
+ }
+}
+------------
+
Querying All Attributes
-----------------------
To get the values of all attributes associated with a file:
-* Call `git_all_attrs()`, which returns an array of `git_attr_check`
- structures.
+* Prepare an empty `attr_check` structure by calling
+ `attr_check_alloc()`.
+
+* Call `git_all_attrs()`, which populates the `attr_check`
+ with the attributes attached to the path.
-* Iterate over the `git_attr_check` array to examine the attribute
- names and values. The name of the attribute described by a
- `git_attr_check` object can be retrieved via
- `git_attr_name(check[i].attr)`. (Please note that no items will be
- returned for unset attributes, so `ATTR_UNSET()` will return false
- for all returned `git_array_check` objects.)
+* Iterate over the `attr_check.items[]` array to examine
+ the attribute names and values. The name of the attribute
+ described by a `attr_check.items[]` object can be retrieved via
+ `git_attr_name(check->items[i].attr)`. (Please note that no items
+ will be returned for unset attributes, so `ATTR_UNSET()` will return
+ false for all returned `attr_check.items[]` objects.)
-* Free the `git_array_check` array.
+* Free the `attr_check` struct by calling `attr_check_free()`.
diff --git a/attr.c b/attr.c
index 40818246f..c0e7893b5 100644
--- a/attr.c
+++ b/attr.c
@@ -890,7 +890,8 @@ static void collect_some_attrs(const char *path, int num,
rem = fill(path, pathlen, basename_offset, stk, rem);
}
-int git_check_attrs(const char *path, int num, struct attr_check_item *check)
+static int git_check_attrs(const char *path, int num,
+ struct attr_check_item *check)
{
int i;
diff --git a/attr.h b/attr.h
index 9f2729842..b2cfd8550 100644
--- a/attr.h
+++ b/attr.h
@@ -52,7 +52,6 @@ extern void attr_check_free(struct attr_check *check);
*/
extern const char *git_attr_name(const struct git_attr *);
-int git_check_attrs(const char *path, int, struct attr_check_item *);
extern int git_check_attr(const char *path, struct attr_check *check);
/*
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 19/27] attr: pass struct attr_check to collect_some_attrs
From: Brandon Williams @ 2017-01-28 2:01 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, gitster, pclouds
In-Reply-To: <20170128020207.179015-1-bmwill@google.com>
The old callchain used to take an array of attr_check_item items.
Instead pass the 'attr_check' container object to 'collect_some_attrs()'
and access the fields in the data structure directly.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 33 +++++++++++++--------------------
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/attr.c b/attr.c
index c0e7893b5..81a3c74d8 100644
--- a/attr.c
+++ b/attr.c
@@ -846,9 +846,7 @@ static int macroexpand_one(int nr, int rem)
* check_all_attr. If num is non-zero, only attributes in check[] are
* collected. Otherwise all attributes are collected.
*/
-static void collect_some_attrs(const char *path, int num,
- struct attr_check_item *check)
-
+static void collect_some_attrs(const char *path, struct attr_check *check)
{
struct attr_stack *stk;
int i, pathlen, rem, dirlen;
@@ -871,17 +869,18 @@ static void collect_some_attrs(const char *path, int num,
prepare_attr_stack(path, dirlen);
for (i = 0; i < attr_nr; i++)
check_all_attr[i].value = ATTR__UNKNOWN;
- if (num && !cannot_trust_maybe_real) {
+ if (check->nr && !cannot_trust_maybe_real) {
rem = 0;
- for (i = 0; i < num; i++) {
- if (!check[i].attr->maybe_real) {
+ for (i = 0; i < check->nr; i++) {
+ const struct git_attr *a = check->items[i].attr;
+ if (!a->maybe_real) {
struct attr_check_item *c;
- c = check_all_attr + check[i].attr->attr_nr;
+ c = check_all_attr + a->attr_nr;
c->value = ATTR__UNSET;
rem++;
}
}
- if (rem == num)
+ if (rem == check->nr)
return;
}
@@ -890,18 +889,17 @@ static void collect_some_attrs(const char *path, int num,
rem = fill(path, pathlen, basename_offset, stk, rem);
}
-static int git_check_attrs(const char *path, int num,
- struct attr_check_item *check)
+int git_check_attr(const char *path, struct attr_check *check)
{
int i;
- collect_some_attrs(path, num, check);
+ collect_some_attrs(path, check);
- for (i = 0; i < num; i++) {
- const char *value = check_all_attr[check[i].attr->attr_nr].value;
+ for (i = 0; i < check->nr; i++) {
+ const char *value = check_all_attr[check->items[i].attr->attr_nr].value;
if (value == ATTR__UNKNOWN)
value = ATTR__UNSET;
- check[i].value = value;
+ check->items[i].value = value;
}
return 0;
@@ -912,7 +910,7 @@ void git_all_attrs(const char *path, struct attr_check *check)
int i;
attr_check_reset(check);
- collect_some_attrs(path, check->nr, check->items);
+ collect_some_attrs(path, check);
for (i = 0; i < attr_nr; i++) {
const char *name = check_all_attr[i].attr->name;
@@ -925,11 +923,6 @@ void git_all_attrs(const char *path, struct attr_check *check)
}
}
-int git_check_attr(const char *path, struct attr_check *check)
-{
- return git_check_attrs(path, check->nr, check->items);
-}
-
void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
{
enum git_attr_direction old = direction;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 17/27] attr: convert git_check_attrs() callers to use the new API
From: Brandon Williams @ 2017-01-28 2:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170128020207.179015-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
The remaining callers are all simple "I have N attributes I am
interested in. I'll ask about them with various paths one by one".
After this step, no caller to git_check_attrs() remains. After
removing it, we can extend "struct attr_check" struct with data
that can be used in optimizing the query for the specific N
attributes it contains.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
archive.c | 24 ++++++------------------
builtin/pack-objects.c | 19 +++++--------------
convert.c | 17 ++++++-----------
ll-merge.c | 33 ++++++++++++++-------------------
userdiff.c | 19 ++++++++-----------
ws.c | 19 ++++++-------------
6 files changed, 45 insertions(+), 86 deletions(-)
diff --git a/archive.c b/archive.c
index b76bd4691..60b889198 100644
--- a/archive.c
+++ b/archive.c
@@ -87,19 +87,6 @@ void *sha1_file_to_archive(const struct archiver_args *args,
return buffer;
}
-static void setup_archive_check(struct attr_check_item *check)
-{
- static struct git_attr *attr_export_ignore;
- static struct git_attr *attr_export_subst;
-
- if (!attr_export_ignore) {
- attr_export_ignore = git_attr("export-ignore");
- attr_export_subst = git_attr("export-subst");
- }
- check[0].attr = attr_export_ignore;
- check[1].attr = attr_export_subst;
-}
-
struct directory {
struct directory *up;
struct object_id oid;
@@ -120,10 +107,10 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
void *context)
{
static struct strbuf path = STRBUF_INIT;
+ static struct attr_check *check;
struct archiver_context *c = context;
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
- struct attr_check_item check[2];
const char *path_without_prefix;
int err;
@@ -137,11 +124,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
strbuf_addch(&path, '/');
path_without_prefix = path.buf + args->baselen;
- setup_archive_check(check);
- if (!git_check_attrs(path_without_prefix, ARRAY_SIZE(check), check)) {
- if (ATTR_TRUE(check[0].value))
+ if (!check)
+ check = attr_check_initl("export-ignore", "export-subst", NULL);
+ if (!git_check_attr(path_without_prefix, check)) {
+ if (ATTR_TRUE(check->items[0].value))
return 0;
- args->convert = ATTR_TRUE(check[1].value);
+ args->convert = ATTR_TRUE(check->items[1].value);
}
if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8b8fbd814..181e4a198 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -894,24 +894,15 @@ static void write_pack_file(void)
written, nr_result);
}
-static void setup_delta_attr_check(struct attr_check_item *check)
-{
- static struct git_attr *attr_delta;
-
- if (!attr_delta)
- attr_delta = git_attr("delta");
-
- check[0].attr = attr_delta;
-}
-
static int no_try_delta(const char *path)
{
- struct attr_check_item check[1];
+ static struct attr_check *check;
- setup_delta_attr_check(check);
- if (git_check_attrs(path, ARRAY_SIZE(check), check))
+ if (!check)
+ check = attr_check_initl("delta", NULL);
+ if (git_check_attr(path, check))
return 0;
- if (ATTR_FALSE(check->value))
+ if (ATTR_FALSE(check->items[0].value))
return 1;
return 0;
}
diff --git a/convert.c b/convert.c
index 1b9829279..8d652bf27 100644
--- a/convert.c
+++ b/convert.c
@@ -1085,24 +1085,19 @@ struct conv_attrs {
int ident;
};
-static const char *conv_attr_name[] = {
- "crlf", "ident", "filter", "eol", "text",
-};
-#define NUM_CONV_ATTRS ARRAY_SIZE(conv_attr_name)
-
static void convert_attrs(struct conv_attrs *ca, const char *path)
{
- int i;
- static struct attr_check_item ccheck[NUM_CONV_ATTRS];
+ static struct attr_check *check;
- if (!ccheck[0].attr) {
- for (i = 0; i < NUM_CONV_ATTRS; i++)
- ccheck[i].attr = git_attr(conv_attr_name[i]);
+ if (!check) {
+ check = attr_check_initl("crlf", "ident", "filter",
+ "eol", "text", NULL);
user_convert_tail = &user_convert;
git_config(read_convert_config, NULL);
}
- if (!git_check_attrs(path, NUM_CONV_ATTRS, ccheck)) {
+ if (!git_check_attr(path, check)) {
+ struct attr_check_item *ccheck = check->items;
ca->crlf_action = git_path_check_crlf(ccheck + 4);
if (ca->crlf_action == CRLF_UNDEFINED)
ca->crlf_action = git_path_check_crlf(ccheck + 0);
diff --git a/ll-merge.c b/ll-merge.c
index 198f07aca..ac0d4a5d7 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -336,15 +336,6 @@ static const struct ll_merge_driver *find_ll_merge_driver(const char *merge_attr
return &ll_merge_drv[LL_TEXT_MERGE];
}
-static int git_path_check_merge(const char *path, struct attr_check_item check[2])
-{
- if (!check[0].attr) {
- check[0].attr = git_attr("merge");
- check[1].attr = git_attr("conflict-marker-size");
- }
- return git_check_attrs(path, 2, check);
-}
-
static void normalize_file(mmfile_t *mm, const char *path)
{
struct strbuf strbuf = STRBUF_INIT;
@@ -362,7 +353,7 @@ int ll_merge(mmbuffer_t *result_buf,
mmfile_t *theirs, const char *their_label,
const struct ll_merge_options *opts)
{
- static struct attr_check_item check[2];
+ static struct attr_check *check;
static const struct ll_merge_options default_opts;
const char *ll_driver_name = NULL;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
@@ -376,10 +367,14 @@ int ll_merge(mmbuffer_t *result_buf,
normalize_file(ours, path);
normalize_file(theirs, path);
}
- if (!git_path_check_merge(path, check)) {
- ll_driver_name = check[0].value;
- if (check[1].value) {
- marker_size = atoi(check[1].value);
+
+ if (!check)
+ check = attr_check_initl("merge", "conflict-marker-size", NULL);
+
+ if (!git_check_attr(path, check)) {
+ ll_driver_name = check->items[0].value;
+ if (check->items[1].value) {
+ marker_size = atoi(check->items[1].value);
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
}
@@ -398,13 +393,13 @@ int ll_merge(mmbuffer_t *result_buf,
int ll_merge_marker_size(const char *path)
{
- static struct attr_check_item check;
+ static struct attr_check *check;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
- if (!check.attr)
- check.attr = git_attr("conflict-marker-size");
- if (!git_check_attrs(path, 1, &check) && check.value) {
- marker_size = atoi(check.value);
+ if (!check)
+ check = attr_check_initl("conflict-marker-size", NULL);
+ if (!git_check_attr(path, check) && check->items[0].value) {
+ marker_size = atoi(check->items[0].value);
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
}
diff --git a/userdiff.c b/userdiff.c
index b0b44467a..8b732e40b 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -262,25 +262,22 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {
struct userdiff_driver *userdiff_find_by_path(const char *path)
{
- static struct git_attr *attr;
- struct attr_check_item check;
-
- if (!attr)
- attr = git_attr("diff");
- check.attr = attr;
+ static struct attr_check *check;
+ if (!check)
+ check = attr_check_initl("diff", NULL);
if (!path)
return NULL;
- if (git_check_attrs(path, 1, &check))
+ if (git_check_attr(path, check))
return NULL;
- if (ATTR_TRUE(check.value))
+ if (ATTR_TRUE(check->items[0].value))
return &driver_true;
- if (ATTR_FALSE(check.value))
+ if (ATTR_FALSE(check->items[0].value))
return &driver_false;
- if (ATTR_UNSET(check.value))
+ if (ATTR_UNSET(check->items[0].value))
return NULL;
- return userdiff_find_by_name(check.value);
+ return userdiff_find_by_name(check->items[0].value);
}
struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver)
diff --git a/ws.c b/ws.c
index fbd876e84..a07caedd5 100644
--- a/ws.c
+++ b/ws.c
@@ -71,24 +71,17 @@ unsigned parse_whitespace_rule(const char *string)
return rule;
}
-static void setup_whitespace_attr_check(struct attr_check_item *check)
-{
- static struct git_attr *attr_whitespace;
-
- if (!attr_whitespace)
- attr_whitespace = git_attr("whitespace");
- check[0].attr = attr_whitespace;
-}
-
unsigned whitespace_rule(const char *pathname)
{
- struct attr_check_item attr_whitespace_rule;
+ static struct attr_check *attr_whitespace_rule;
+
+ if (!attr_whitespace_rule)
+ attr_whitespace_rule = attr_check_initl("whitespace", NULL);
- setup_whitespace_attr_check(&attr_whitespace_rule);
- if (!git_check_attrs(pathname, 1, &attr_whitespace_rule)) {
+ if (!git_check_attr(pathname, attr_whitespace_rule)) {
const char *value;
- value = attr_whitespace_rule.value;
+ value = attr_whitespace_rule->items[0].value;
if (ATTR_TRUE(value)) {
/* true (whitespace) */
unsigned all_rule = ws_tab_width(whitespace_rule_cfg);
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 22/27] attr: eliminate global check_all_attr array
From: Brandon Williams @ 2017-01-28 2:02 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, gitster, pclouds
In-Reply-To: <20170128020207.179015-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 | 121 ++++++++++++++++++++++++++++++++++++++++++++---------------------
attr.h | 5 +++
2 files changed, 87 insertions(+), 39 deletions(-)
diff --git a/attr.c b/attr.c
index e008f3026..2637804b1 100644
--- a/attr.c
+++ b/attr.c
@@ -34,7 +34,6 @@ struct git_attr {
int maybe_real;
char name[FLEX_ARRAY]; /* attribute name */
};
-static int attr_nr;
/*
* NEEDSWORK: maybe-real, maybe-macro are not property of
@@ -45,9 +44,6 @@ static int attr_nr;
*/
static int cannot_trust_maybe_real;
-/* NEEDSWORK: This will become per git_attr_check */
-static struct attr_check_item *check_all_attr;
-
const char *git_attr_name(const struct git_attr *attr)
{
return attr->name;
@@ -143,6 +139,57 @@ static void attr_hashmap_add(struct attr_hashmap *map,
hashmap_add(&map->map, e);
}
+struct all_attrs_item {
+ const struct git_attr *attr;
+ const char *value;
+};
+
+/*
+ * Reallocate and reinitialize the array of all attributes (which is used in
+ * the attribute collection process) in 'check' based on the global dictionary
+ * of attributes.
+ */
+static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)
+{
+ int i;
+
+ hashmap_lock(map);
+
+ if (map->map.size < check->all_attrs_nr)
+ die("BUG: interned attributes shouldn't be deleted");
+
+ /*
+ * If the number of attributes in the global dictionary has increased
+ * (or this attr_check instance doesn't have an initialized all_attrs
+ * field), reallocate the provided attr_check instance's all_attrs
+ * field and fill each entry with its corresponding git_attr.
+ */
+ if (map->map.size != check->all_attrs_nr) {
+ struct attr_hash_entry *e;
+ struct hashmap_iter iter;
+ hashmap_iter_init(&map->map, &iter);
+
+ REALLOC_ARRAY(check->all_attrs, map->map.size);
+ check->all_attrs_nr = map->map.size;
+
+ while ((e = hashmap_iter_next(&iter))) {
+ const struct git_attr *a = e->value;
+ check->all_attrs[a->attr_nr].attr = a;
+ }
+ }
+
+ hashmap_unlock(map);
+
+ /*
+ * Re-initialize every entry in check->all_attrs.
+ * This re-initialization can live outside of the locked region since
+ * the attribute dictionary is no longer being accessed.
+ */
+ for (i = 0; i < check->all_attrs_nr; i++) {
+ check->all_attrs[i].value = ATTR__UNKNOWN;
+ }
+}
+
static int attr_name_valid(const char *name, size_t namelen)
{
/*
@@ -196,16 +243,6 @@ static struct git_attr *git_attr_internal(const char *name, int namelen)
attr_hashmap_add(&g_attr_hashmap, a->name, namelen, a);
assert(a->attr_nr == (g_attr_hashmap.map.size - 1));
-
- /*
- * NEEDSWORK: per git_attr_check check_all_attr
- * will be initialized a lot more lazily, not
- * like this, and not here.
- */
- REALLOC_ARRAY(check_all_attr, ++attr_nr);
- check_all_attr[a->attr_nr].attr = a;
- check_all_attr[a->attr_nr].value = ATTR__UNKNOWN;
- assert(a->attr_nr == (attr_nr - 1));
}
hashmap_unlock(&g_attr_hashmap);
@@ -513,6 +550,10 @@ void attr_check_clear(struct attr_check *check)
check->items = NULL;
check->alloc = 0;
check->nr = 0;
+
+ free(check->all_attrs);
+ check->all_attrs = NULL;
+ check->all_attrs_nr = 0;
}
void attr_check_free(struct attr_check *check)
@@ -860,16 +901,16 @@ static int path_matches(const char *pathname, int pathlen,
pattern, prefix, pat->patternlen, pat->flags);
}
-static int macroexpand_one(int attr_nr, int rem);
+static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem);
-static int fill_one(const char *what, struct match_attr *a, int rem)
+static int fill_one(const char *what, struct all_attrs_item *all_attrs,
+ struct match_attr *a, int rem)
{
- struct attr_check_item *check = check_all_attr;
int i;
- for (i = a->num_attr - 1; 0 < rem && 0 <= i; i--) {
+ for (i = a->num_attr - 1; rem > 0 && i >= 0; i--) {
struct git_attr *attr = a->state[i].attr;
- const char **n = &(check[attr->attr_nr].value);
+ const char **n = &(all_attrs[attr->attr_nr].value);
const char *v = a->state[i].setto;
if (*n == ATTR__UNKNOWN) {
@@ -878,14 +919,15 @@ static int fill_one(const char *what, struct match_attr *a, int rem)
attr, v);
*n = v;
rem--;
- rem = macroexpand_one(attr->attr_nr, rem);
+ rem = macroexpand_one(all_attrs, attr->attr_nr, rem);
}
}
return rem;
}
static int fill(const char *path, int pathlen, int basename_offset,
- struct attr_stack *stk, int rem)
+ struct attr_stack *stk, struct all_attrs_item *all_attrs,
+ int rem)
{
int i;
const char *base = stk->origin ? stk->origin : "";
@@ -896,18 +938,18 @@ static int fill(const char *path, int pathlen, int basename_offset,
continue;
if (path_matches(path, pathlen, basename_offset,
&a->u.pat, base, stk->originlen))
- rem = fill_one("fill", a, rem);
+ rem = fill_one("fill", all_attrs, a, rem);
}
return rem;
}
-static int macroexpand_one(int nr, int rem)
+static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem)
{
struct attr_stack *stk;
int i;
- if (check_all_attr[nr].value != ATTR__TRUE ||
- !check_all_attr[nr].attr->maybe_macro)
+ if (all_attrs[nr].value != ATTR__TRUE ||
+ !all_attrs[nr].attr->maybe_macro)
return rem;
for (stk = attr_stack; stk; stk = stk->prev) {
@@ -916,7 +958,7 @@ static int macroexpand_one(int nr, int rem)
if (!ma->is_macro)
continue;
if (ma->u.attr->attr_nr == nr)
- return fill_one("expand", ma, rem);
+ return fill_one("expand", all_attrs, ma, rem);
}
}
@@ -924,9 +966,9 @@ static int macroexpand_one(int nr, int rem)
}
/*
- * Collect attributes for path into the array pointed to by
- * check_all_attr. If num is non-zero, only attributes in check[] are
- * collected. Otherwise all attributes are collected.
+ * Collect attributes for path into the array pointed to by check->all_attrs.
+ * If check->check_nr is non-zero, only attributes in check[] are collected.
+ * Otherwise all attributes are collected.
*/
static void collect_some_attrs(const char *path, struct attr_check *check)
{
@@ -949,15 +991,15 @@ static void collect_some_attrs(const char *path, struct attr_check *check)
}
prepare_attr_stack(path, dirlen);
- for (i = 0; i < attr_nr; i++)
- check_all_attr[i].value = ATTR__UNKNOWN;
+ all_attrs_init(&g_attr_hashmap, check);
+
if (check->nr && !cannot_trust_maybe_real) {
rem = 0;
for (i = 0; i < check->nr; i++) {
const struct git_attr *a = check->items[i].attr;
if (!a->maybe_real) {
- struct attr_check_item *c;
- c = check_all_attr + a->attr_nr;
+ struct all_attrs_item *c;
+ c = check->all_attrs + a->attr_nr;
c->value = ATTR__UNSET;
rem++;
}
@@ -966,9 +1008,9 @@ static void collect_some_attrs(const char *path, struct attr_check *check)
return;
}
- rem = attr_nr;
+ rem = check->all_attrs_nr;
for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
- rem = fill(path, pathlen, basename_offset, stk, rem);
+ rem = fill(path, pathlen, basename_offset, stk, check->all_attrs, rem);
}
int git_check_attr(const char *path, struct attr_check *check)
@@ -978,7 +1020,8 @@ int git_check_attr(const char *path, struct attr_check *check)
collect_some_attrs(path, check);
for (i = 0; i < check->nr; i++) {
- const char *value = check_all_attr[check->items[i].attr->attr_nr].value;
+ size_t n = check->items[i].attr->attr_nr;
+ const char *value = check->all_attrs[n].value;
if (value == ATTR__UNKNOWN)
value = ATTR__UNSET;
check->items[i].value = value;
@@ -994,9 +1037,9 @@ void git_all_attrs(const char *path, struct attr_check *check)
attr_check_reset(check);
collect_some_attrs(path, check);
- for (i = 0; i < attr_nr; i++) {
- const char *name = check_all_attr[i].attr->name;
- const char *value = check_all_attr[i].value;
+ for (i = 0; i < check->all_attrs_nr; i++) {
+ const char *name = check->all_attrs[i].attr->name;
+ const char *value = check->all_attrs[i].value;
struct attr_check_item *item;
if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
continue;
diff --git a/attr.h b/attr.h
index 898e1a8c9..5aaf55c3e 100644
--- a/attr.h
+++ b/attr.h
@@ -4,6 +4,9 @@
/* An attribute is a pointer to this opaque structure */
struct git_attr;
+/* opaque structure used internally for attribute collection */
+struct all_attrs_item;
+
/*
* Given a string, return the gitattribute object that
* corresponds to it.
@@ -33,6 +36,8 @@ struct attr_check {
int nr;
int alloc;
struct attr_check_item *items;
+ int all_attrs_nr;
+ struct all_attrs_item *all_attrs;
};
extern struct attr_check *attr_check_alloc(void);
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 21/27] attr: use hashmap for attribute dictionary
From: Brandon Williams @ 2017-01-28 2:02 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, gitster, pclouds
In-Reply-To: <20170128020207.179015-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 | 173 +++++++++++++++++++++++++++++++++++++++++++---------------
attr.h | 2 +
common-main.c | 3 +
3 files changed, 133 insertions(+), 45 deletions(-)
diff --git a/attr.c b/attr.c
index 9fe848f59..e008f3026 100644
--- a/attr.c
+++ b/attr.c
@@ -14,6 +14,7 @@
#include "dir.h"
#include "utf8.h"
#include "quote.h"
+#include "thread-utils.h"
const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
@@ -23,28 +24,17 @@ static const char git_attr__unknown[] = "(builtin)unknown";
#define ATTR__UNSET NULL
#define ATTR__UNKNOWN git_attr__unknown
-/* This is a randomly chosen prime. */
-#define HASHSIZE 257
-
#ifndef DEBUG_ATTR
#define DEBUG_ATTR 0
#endif
-/*
- * NEEDSWORK: the global dictionary of the interned attributes
- * must stay a singleton even after we become thread-ready.
- * Access to these must be surrounded with mutex when it happens.
- */
struct git_attr {
- struct git_attr *next;
- unsigned h;
- int attr_nr;
+ int attr_nr; /* unique attribute number */
int maybe_macro;
int maybe_real;
- char name[FLEX_ARRAY];
+ char name[FLEX_ARRAY]; /* attribute name */
};
static int attr_nr;
-static struct git_attr *(git_attr_hash[HASHSIZE]);
/*
* NEEDSWORK: maybe-real, maybe-macro are not property of
@@ -63,15 +53,94 @@ const char *git_attr_name(const struct git_attr *attr)
return attr->name;
}
-static unsigned hash_name(const char *name, int namelen)
+struct attr_hashmap {
+ struct hashmap map;
+#ifndef NO_PTHREADS
+ pthread_mutex_t mutex;
+#endif
+};
+
+static inline void hashmap_lock(struct attr_hashmap *map)
+{
+#ifndef NO_PTHREADS
+ pthread_mutex_lock(&map->mutex);
+#endif
+}
+
+static inline void hashmap_unlock(struct attr_hashmap *map)
{
- unsigned val = 0, c;
+#ifndef NO_PTHREADS
+ pthread_mutex_unlock(&map->mutex);
+#endif
+}
- while (namelen--) {
- c = *name++;
- val = ((val << 7) | (val >> 22)) ^ c;
- }
- return val;
+/*
+ * The global dictionary of all interned attributes. This
+ * is a singleton object which is shared between threads.
+ * Access to this dictionary must be surrounded with a mutex.
+ */
+static struct attr_hashmap g_attr_hashmap;
+
+/* The container for objects stored in "struct attr_hashmap" */
+struct attr_hash_entry {
+ struct hashmap_entry ent; /* must be the first member! */
+ const char *key; /* the key; memory should be owned by value */
+ size_t keylen; /* length of the key */
+ void *value; /* the stored value */
+};
+
+/* attr_hashmap comparison function */
+static int attr_hash_entry_cmp(const struct attr_hash_entry *a,
+ const struct attr_hash_entry *b,
+ void *unused)
+{
+ return (a->keylen != b->keylen) || strncmp(a->key, b->key, a->keylen);
+}
+
+/* Initialize an 'attr_hashmap' object */
+static void attr_hashmap_init(struct attr_hashmap *map)
+{
+ hashmap_init(&map->map, (hashmap_cmp_fn) attr_hash_entry_cmp, 0);
+}
+
+/*
+ * Retrieve the 'value' stored in a hashmap given the provided 'key'.
+ * If there is no matching entry, return NULL.
+ */
+static void *attr_hashmap_get(struct attr_hashmap *map,
+ const char *key, size_t keylen)
+{
+ struct attr_hash_entry k;
+ struct attr_hash_entry *e;
+
+ if (!map->map.tablesize)
+ attr_hashmap_init(map);
+
+ hashmap_entry_init(&k, memhash(key, keylen));
+ k.key = key;
+ k.keylen = keylen;
+ e = hashmap_get(&map->map, &k, NULL);
+
+ return e ? e->value : NULL;
+}
+
+/* Add 'value' to a hashmap based on the provided 'key'. */
+static void attr_hashmap_add(struct attr_hashmap *map,
+ const char *key, size_t keylen,
+ void *value)
+{
+ struct attr_hash_entry *e;
+
+ if (!map->map.tablesize)
+ attr_hashmap_init(map);
+
+ e = xmalloc(sizeof(struct attr_hash_entry));
+ hashmap_entry_init(e, memhash(key, keylen));
+ e->key = key;
+ e->keylen = keylen;
+ e->value = value;
+
+ hashmap_add(&map->map, e);
}
static int attr_name_valid(const char *name, size_t namelen)
@@ -103,37 +172,44 @@ static void report_invalid_attr(const char *name, size_t len,
strbuf_release(&err);
}
-static struct git_attr *git_attr_internal(const char *name, int len)
+/*
+ * Given a 'name', lookup and return the corresponding attribute in the global
+ * dictionary. If no entry is found, create a new attribute and store it in
+ * the dictionary.
+ */
+static struct git_attr *git_attr_internal(const char *name, int namelen)
{
- unsigned hval = hash_name(name, len);
- unsigned pos = hval % HASHSIZE;
struct git_attr *a;
- for (a = git_attr_hash[pos]; a; a = a->next) {
- if (a->h == hval &&
- !memcmp(a->name, name, len) && !a->name[len])
- return a;
- }
-
- if (!attr_name_valid(name, len))
+ if (!attr_name_valid(name, namelen))
return NULL;
- FLEX_ALLOC_MEM(a, name, name, len);
- a->h = hval;
- a->next = git_attr_hash[pos];
- a->attr_nr = attr_nr++;
- a->maybe_macro = 0;
- a->maybe_real = 0;
- git_attr_hash[pos] = a;
+ hashmap_lock(&g_attr_hashmap);
+
+ a = attr_hashmap_get(&g_attr_hashmap, name, namelen);
+
+ if (!a) {
+ FLEX_ALLOC_MEM(a, name, name, namelen);
+ a->attr_nr = g_attr_hashmap.map.size;
+ a->maybe_real = 0;
+ a->maybe_macro = 0;
+
+ attr_hashmap_add(&g_attr_hashmap, a->name, namelen, a);
+ assert(a->attr_nr == (g_attr_hashmap.map.size - 1));
+
+ /*
+ * NEEDSWORK: per git_attr_check check_all_attr
+ * will be initialized a lot more lazily, not
+ * like this, and not here.
+ */
+ REALLOC_ARRAY(check_all_attr, ++attr_nr);
+ check_all_attr[a->attr_nr].attr = a;
+ check_all_attr[a->attr_nr].value = ATTR__UNKNOWN;
+ assert(a->attr_nr == (attr_nr - 1));
+ }
+
+ hashmap_unlock(&g_attr_hashmap);
- /*
- * NEEDSWORK: per git_attr_check check_all_attr
- * will be initialized a lot more lazily, not
- * like this, and not here.
- */
- REALLOC_ARRAY(check_all_attr, attr_nr);
- check_all_attr[a->attr_nr].attr = a;
- check_all_attr[a->attr_nr].value = ATTR__UNKNOWN;
return a;
}
@@ -941,3 +1017,10 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
drop_attr_stack();
use_index = istate;
}
+
+void attr_start(void)
+{
+#ifndef NO_PTHREADS
+ pthread_mutex_init(&g_attr_hashmap.mutex, NULL);
+#endif
+}
diff --git a/attr.h b/attr.h
index b2cfd8550..898e1a8c9 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.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 26/27] attr: push the bare repo check into read_attr()
From: Brandon Williams @ 2017-01-28 2:02 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, gitster, pclouds
In-Reply-To: <20170128020207.179015-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 | 114 +++++++++++++++++++++++++++++++----------------------------------
1 file changed, 54 insertions(+), 60 deletions(-)
diff --git a/attr.c b/attr.c
index bcee0921d..62298ec2f 100644
--- a/attr.c
+++ b/attr.c
@@ -747,25 +747,28 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
static struct attr_stack *read_attr(const char *path, int macro_ok)
{
- struct attr_stack *res;
+ struct attr_stack *res = NULL;
- if (direction == GIT_ATTR_CHECKOUT) {
+ if (direction == GIT_ATTR_INDEX) {
res = read_attr_from_index(path, macro_ok);
- if (!res)
- res = read_attr_from_file(path, macro_ok);
- }
- else if (direction == GIT_ATTR_CHECKIN) {
- res = read_attr_from_file(path, macro_ok);
- if (!res)
- /*
- * There is no checked out .gitattributes file there, but
- * we might have it in the index. We allow operation in a
- * sparsely checked out work tree, so read from it.
- */
+ } else if (!is_bare_repository()) {
+ if (direction == GIT_ATTR_CHECKOUT) {
res = read_attr_from_index(path, macro_ok);
+ if (!res)
+ res = read_attr_from_file(path, macro_ok);
+ } else if (direction == GIT_ATTR_CHECKIN) {
+ res = read_attr_from_file(path, macro_ok);
+ if (!res)
+ /*
+ * There is no checked out .gitattributes file
+ * there, but we might have it in the index.
+ * We allow operation in a sparsely checked out
+ * work tree, so read from it.
+ */
+ res = read_attr_from_index(path, macro_ok);
+ }
}
- else
- res = read_attr_from_index(path, macro_ok);
+
if (!res)
res = xcalloc(1, sizeof(*res));
return res;
@@ -857,10 +860,7 @@ static void bootstrap_attr_stack(struct attr_stack **stack)
}
/* root directory */
- if (!is_bare_repository() || direction == GIT_ATTR_INDEX)
- e = read_attr(GITATTRIBUTES_FILE, 1);
- else
- e = xcalloc(1, sizeof(struct attr_stack));
+ e = read_attr(GITATTRIBUTES_FILE, 1);
push_stack(stack, e, xstrdup(""), 0);
/* info frame */
@@ -877,6 +877,7 @@ static void prepare_attr_stack(const char *path, int dirlen,
struct attr_stack **stack)
{
struct attr_stack *info;
+ struct strbuf pathbuf = STRBUF_INIT;
/*
* At the bottom of the attribute stack is the built-in
@@ -923,54 +924,47 @@ static void prepare_attr_stack(const char *path, int dirlen,
}
/*
- * Read from parent directories and push them down
+ * bootstrap_attr_stack() should have added, and the
+ * above loop should have stopped before popping, the
+ * root element whose attr_stack->origin is set to an
+ * empty string.
*/
- if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
- /*
- * bootstrap_attr_stack() should have added, and the
- * above loop should have stopped before popping, the
- * root element whose attr_stack->origin is set to an
- * empty string.
- */
- struct strbuf pathbuf = STRBUF_INIT;
-
- assert((*stack)->origin);
- strbuf_addstr(&pathbuf, (*stack)->origin);
- /* Build up to the directory 'path' is in */
- while (pathbuf.len < dirlen) {
- size_t len = pathbuf.len;
- struct attr_stack *next;
- char *origin;
-
- /* Skip path-separator */
- if (len < dirlen && is_dir_sep(path[len]))
- len++;
- /* Find the end of the next component */
- while (len < dirlen && !is_dir_sep(path[len]))
- len++;
-
- if (pathbuf.len > 0)
- strbuf_addch(&pathbuf, '/');
- strbuf_add(&pathbuf, path + pathbuf.len,
- (len - pathbuf.len));
- strbuf_addf(&pathbuf, "/%s", GITATTRIBUTES_FILE);
-
- next = read_attr(pathbuf.buf, 0);
-
- /* reset the pathbuf to not include "/.gitattributes" */
- strbuf_setlen(&pathbuf, len);
-
- origin = xstrdup(pathbuf.buf);
- push_stack(stack, next, origin, len);
-
- }
- strbuf_release(&pathbuf);
+ assert((*stack)->origin);
+
+ strbuf_addstr(&pathbuf, (*stack)->origin);
+ /* Build up to the directory 'path' is in */
+ while (pathbuf.len < dirlen) {
+ size_t len = pathbuf.len;
+ struct attr_stack *next;
+ char *origin;
+
+ /* Skip path-separator */
+ if (len < dirlen && is_dir_sep(path[len]))
+ len++;
+ /* Find the end of the next component */
+ while (len < dirlen && !is_dir_sep(path[len]))
+ len++;
+
+ if (pathbuf.len > 0)
+ strbuf_addch(&pathbuf, '/');
+ strbuf_add(&pathbuf, path + pathbuf.len, (len - pathbuf.len));
+ strbuf_addf(&pathbuf, "/%s", GITATTRIBUTES_FILE);
+
+ next = read_attr(pathbuf.buf, 0);
+
+ /* reset the pathbuf to not include "/.gitattributes" */
+ strbuf_setlen(&pathbuf, len);
+
+ origin = xstrdup(pathbuf.buf);
+ push_stack(stack, next, origin, len);
}
/*
* Finally push the "info" one at the top of the stack.
*/
push_stack(stack, info, NULL, 0);
+
+ strbuf_release(&pathbuf);
}
static int path_matches(const char *pathname, int pathlen,
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 25/27] attr: store attribute stack in attr_check structure
From: Brandon Williams @ 2017-01-28 2:02 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, gitster, pclouds
In-Reply-To: <20170128020207.179015-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 stored
locally in each attr_check instance. This opens up the opportunity for
future optimizations to customize the attribute stack for the attributes
that a particular attr_check struct is interested in.
One caveat with pushing the attribute stack into the attr_check
structure is that the attribute system now needs to keep track of all
active attr_check instances. Due to the direction mechanism the stack
needs to be dropped when the direction is switched. In order to ensure
correctness when the direction is changed the attribute system needs to
iterate through all active attr_check instances and drop each of their
stacks.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 284 +++++++++++++++++++++++++++++++++++++++++++++--------------------
attr.h | 4 +-
2 files changed, 199 insertions(+), 89 deletions(-)
diff --git a/attr.c b/attr.c
index 69643ae77..bcee0921d 100644
--- a/attr.c
+++ b/attr.c
@@ -445,17 +445,16 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
* .gitignore file and info/excludes file as a fallback.
*/
-/* NEEDSWORK: This will become per git_attr_check */
-static struct attr_stack {
+struct attr_stack {
struct attr_stack *prev;
char *origin;
size_t originlen;
unsigned num_matches;
unsigned alloc;
struct match_attr **attrs;
-} *attr_stack;
+};
-static void free_attr_elem(struct attr_stack *e)
+static void attr_stack_free(struct attr_stack *e)
{
int i;
free(e->origin);
@@ -478,9 +477,96 @@ static void free_attr_elem(struct attr_stack *e)
free(e);
}
+static void drop_attr_stack(struct attr_stack **stack)
+{
+ while (*stack) {
+ struct attr_stack *elem = *stack;
+ *stack = elem->prev;
+ attr_stack_free(elem);
+ }
+}
+
+/* List of all attr_check structs; access should be surrounded by mutex */
+static struct check_vector {
+ size_t nr;
+ size_t alloc;
+ struct attr_check **checks;
+#ifndef NO_PTHREADS
+ pthread_mutex_t mutex;
+#endif
+} check_vector;
+
+static inline void vector_lock(void)
+{
+#ifndef NO_PTHREADS
+ pthread_mutex_lock(&check_vector.mutex);
+#endif
+}
+
+static inline void vector_unlock(void)
+{
+#ifndef NO_PTHREADS
+ pthread_mutex_unlock(&check_vector.mutex);
+#endif
+}
+
+static void check_vector_add(struct attr_check *c)
+{
+ vector_lock();
+
+ ALLOC_GROW(check_vector.checks,
+ check_vector.nr + 1,
+ check_vector.alloc);
+ check_vector.checks[check_vector.nr++] = c;
+
+ vector_unlock();
+}
+
+static void check_vector_remove(struct attr_check *check)
+{
+ int i;
+
+ vector_lock();
+
+ /* Find entry */
+ for (i = 0; i < check_vector.nr; i++)
+ if (check_vector.checks[i] == check)
+ break;
+
+ if (i >= check_vector.nr)
+ die("BUG: no entry found");
+
+ /* shift entries over */
+ for (; i < check_vector.nr - 1; i++)
+ check_vector.checks[i] = check_vector.checks[i + 1];
+
+ check_vector.nr--;
+
+ vector_unlock();
+}
+
+/* Iterate through all attr_check instances and drop their stacks */
+static void drop_all_attr_stacks(void)
+{
+ int i;
+
+ vector_lock();
+
+ for (i = 0; i < check_vector.nr; i++) {
+ drop_attr_stack(&check_vector.checks[i]->stack);
+ }
+
+ vector_unlock();
+}
+
struct attr_check *attr_check_alloc(void)
{
- return xcalloc(1, sizeof(struct attr_check));
+ struct attr_check *c = xcalloc(1, sizeof(struct attr_check));
+
+ /* save pointer to the check struct */
+ check_vector_add(c);
+
+ return c;
}
struct attr_check *attr_check_initl(const char *one, ...)
@@ -543,12 +629,19 @@ void attr_check_clear(struct attr_check *check)
free(check->all_attrs);
check->all_attrs = NULL;
check->all_attrs_nr = 0;
+
+ drop_attr_stack(&check->stack);
}
void attr_check_free(struct attr_check *check)
{
- attr_check_clear(check);
- free(check);
+ if (check) {
+ /* Remove check from the check vector */
+ check_vector_remove(check);
+
+ attr_check_clear(check);
+ free(check);
+ }
}
static const char *builtin_attr[] = {
@@ -705,15 +798,6 @@ static void debug_set(const char *what, const char *match, struct git_attr *attr
#define debug_set(a,b,c,d) do { ; } while (0)
#endif /* DEBUG_ATTR */
-static void drop_attr_stack(void)
-{
- while (attr_stack) {
- struct attr_stack *elem = attr_stack;
- attr_stack = elem->prev;
- free_attr_elem(elem);
- }
-}
-
static const char *git_etc_gitattributes(void)
{
static const char *system_wide;
@@ -722,6 +806,14 @@ static const char *git_etc_gitattributes(void)
return system_wide;
}
+static const char *get_home_gitattributes(void)
+{
+ if (!git_attributes_file)
+ git_attributes_file = xdg_config_home("attributes");
+
+ return git_attributes_file;
+}
+
static int git_attr_system(void)
{
return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
@@ -741,47 +833,50 @@ static void push_stack(struct attr_stack **attr_stack_p,
}
}
-static void bootstrap_attr_stack(void)
+static void bootstrap_attr_stack(struct attr_stack **stack)
{
- struct attr_stack *elem;
+ struct attr_stack *e;
- if (attr_stack)
+ if (*stack)
return;
- push_stack(&attr_stack, read_attr_from_array(builtin_attr), NULL, 0);
-
- if (git_attr_system())
- push_stack(&attr_stack,
- read_attr_from_file(git_etc_gitattributes(), 1),
- NULL, 0);
+ /* builtin frame */
+ e = read_attr_from_array(builtin_attr);
+ push_stack(stack, e, NULL, 0);
- if (!git_attributes_file)
- git_attributes_file = xdg_config_home("attributes");
- if (git_attributes_file)
- push_stack(&attr_stack,
- read_attr_from_file(git_attributes_file, 1),
- NULL, 0);
+ /* system-wide frame */
+ if (git_attr_system()) {
+ e = read_attr_from_file(git_etc_gitattributes(), 1);
+ push_stack(stack, e, NULL, 0);
+ }
- if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
- elem = read_attr(GITATTRIBUTES_FILE, 1);
- push_stack(&attr_stack, elem, xstrdup(""), 0);
- debug_push(elem);
+ /* home directory */
+ if (get_home_gitattributes()) {
+ e = read_attr_from_file(get_home_gitattributes(), 1);
+ push_stack(stack, e, NULL, 0);
}
- if (startup_info->have_repository)
- elem = read_attr_from_file(git_path_info_attributes(), 1);
+ /* root directory */
+ if (!is_bare_repository() || direction == GIT_ATTR_INDEX)
+ e = read_attr(GITATTRIBUTES_FILE, 1);
else
- elem = NULL;
+ e = xcalloc(1, sizeof(struct attr_stack));
+ push_stack(stack, e, xstrdup(""), 0);
- if (!elem)
- elem = xcalloc(1, sizeof(*elem));
- push_stack(&attr_stack, elem, NULL, 0);
+ /* info frame */
+ if (startup_info->have_repository)
+ e = read_attr_from_file(git_path_info_attributes(), 1);
+ else
+ e = NULL;
+ if (!e)
+ e = xcalloc(1, sizeof(struct attr_stack));
+ push_stack(stack, e, NULL, 0);
}
-static void prepare_attr_stack(const char *path, int dirlen)
+static void prepare_attr_stack(const char *path, int dirlen,
+ struct attr_stack **stack)
{
- struct attr_stack *elem, *info;
- const char *cp;
+ struct attr_stack *info;
/*
* At the bottom of the attribute stack is the built-in
@@ -798,13 +893,13 @@ static void prepare_attr_stack(const char *path, int dirlen)
* .gitattributes in deeper directories to shallower ones,
* and finally use the built-in set as the default.
*/
- bootstrap_attr_stack();
+ bootstrap_attr_stack(stack);
/*
* Pop the "info" one that is always at the top of the stack.
*/
- info = attr_stack;
- attr_stack = info->prev;
+ info = *stack;
+ *stack = info->prev;
/*
* Pop the ones from directories that are not the prefix of
@@ -812,18 +907,19 @@ static void prepare_attr_stack(const char *path, int dirlen)
* the root one (whose origin is an empty string "") or the builtin
* one (whose origin is NULL) without popping it.
*/
- while (attr_stack->origin) {
- int namelen = strlen(attr_stack->origin);
+ while ((*stack)->origin) {
+ int namelen = (*stack)->originlen;
+ struct attr_stack *elem;
- elem = attr_stack;
+ elem = *stack;
if (namelen <= dirlen &&
!strncmp(elem->origin, path, namelen) &&
(!namelen || path[namelen] == '/'))
break;
debug_pop(elem);
- attr_stack = elem->prev;
- free_attr_elem(elem);
+ *stack = elem->prev;
+ attr_stack_free(elem);
}
/*
@@ -838,33 +934,43 @@ static void prepare_attr_stack(const char *path, int dirlen)
*/
struct strbuf pathbuf = STRBUF_INIT;
- assert(attr_stack->origin);
- while (1) {
- size_t len = strlen(attr_stack->origin);
+ assert((*stack)->origin);
+ strbuf_addstr(&pathbuf, (*stack)->origin);
+ /* Build up to the directory 'path' is in */
+ while (pathbuf.len < dirlen) {
+ size_t len = pathbuf.len;
+ struct attr_stack *next;
char *origin;
- if (dirlen <= len)
- break;
- cp = memchr(path + len + 1, '/', dirlen - len - 1);
- if (!cp)
- cp = path + dirlen;
- strbuf_addf(&pathbuf,
- "%.*s/%s", (int)(cp - path), path,
- GITATTRIBUTES_FILE);
- elem = read_attr(pathbuf.buf, 0);
- strbuf_setlen(&pathbuf, cp - path);
- origin = strbuf_detach(&pathbuf, &len);
- push_stack(&attr_stack, elem, origin, len);
- debug_push(elem);
- }
+ /* Skip path-separator */
+ if (len < dirlen && is_dir_sep(path[len]))
+ len++;
+ /* Find the end of the next component */
+ while (len < dirlen && !is_dir_sep(path[len]))
+ len++;
+
+ if (pathbuf.len > 0)
+ strbuf_addch(&pathbuf, '/');
+ strbuf_add(&pathbuf, path + pathbuf.len,
+ (len - pathbuf.len));
+ strbuf_addf(&pathbuf, "/%s", GITATTRIBUTES_FILE);
+
+ next = read_attr(pathbuf.buf, 0);
+ /* reset the pathbuf to not include "/.gitattributes" */
+ strbuf_setlen(&pathbuf, len);
+
+ origin = xstrdup(pathbuf.buf);
+ push_stack(stack, next, origin, len);
+
+ }
strbuf_release(&pathbuf);
}
/*
* Finally push the "info" one at the top of the stack.
*/
- push_stack(&attr_stack, info, NULL, 0);
+ push_stack(stack, info, NULL, 0);
}
static int path_matches(const char *pathname, int pathlen,
@@ -915,20 +1021,23 @@ static int fill_one(const char *what, struct all_attrs_item *all_attrs,
}
static int fill(const char *path, int pathlen, int basename_offset,
- struct attr_stack *stk, struct all_attrs_item *all_attrs,
- int rem)
+ const struct attr_stack *stack,
+ struct all_attrs_item *all_attrs, int rem)
{
- int i;
- const char *base = stk->origin ? stk->origin : "";
-
- for (i = stk->num_matches - 1; 0 < rem && 0 <= i; i--) {
- const struct match_attr *a = stk->attrs[i];
- if (a->is_macro)
- continue;
- if (path_matches(path, pathlen, basename_offset,
- &a->u.pat, base, stk->originlen))
- rem = fill_one("fill", all_attrs, a, rem);
+ for (; rem > 0 && stack; stack = stack->prev) {
+ int i;
+ const char *base = stack->origin ? stack->origin : "";
+
+ for (i = stack->num_matches - 1; 0 < rem && 0 <= i; i--) {
+ const struct match_attr *a = stack->attrs[i];
+ if (a->is_macro)
+ continue;
+ if (path_matches(path, pathlen, basename_offset,
+ &a->u.pat, base, stack->originlen))
+ rem = fill_one("fill", all_attrs, a, rem);
+ }
}
+
return rem;
}
@@ -971,7 +1080,6 @@ static void determine_macros(struct all_attrs_item *all_attrs,
*/
static void collect_some_attrs(const char *path, struct attr_check *check)
{
- struct attr_stack *stk;
int i, pathlen, rem, dirlen;
const char *cp, *last_slash = NULL;
int basename_offset;
@@ -989,9 +1097,9 @@ static void collect_some_attrs(const char *path, struct attr_check *check)
dirlen = 0;
}
- prepare_attr_stack(path, dirlen);
+ prepare_attr_stack(path, dirlen, &check->stack);
all_attrs_init(&g_attr_hashmap, check);
- determine_macros(check->all_attrs, attr_stack);
+ determine_macros(check->all_attrs, check->stack);
if (check->nr) {
rem = 0;
@@ -1008,8 +1116,7 @@ static void collect_some_attrs(const char *path, struct attr_check *check)
}
rem = check->all_attrs_nr;
- for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
- rem = fill(path, pathlen, basename_offset, stk, check->all_attrs, rem);
+ fill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);
}
int git_check_attr(const char *path, struct attr_check *check)
@@ -1056,7 +1163,7 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
direction = new;
if (new != old)
- drop_attr_stack();
+ drop_all_attr_stacks();
use_index = istate;
}
@@ -1064,5 +1171,6 @@ void attr_start(void)
{
#ifndef NO_PTHREADS
pthread_mutex_init(&g_attr_hashmap.mutex, NULL);
+ pthread_mutex_init(&check_vector.mutex, NULL);
#endif
}
diff --git a/attr.h b/attr.h
index abebbc19c..6f4961fdb 100644
--- a/attr.h
+++ b/attr.h
@@ -4,8 +4,9 @@
/* An attribute is a pointer to this opaque structure */
struct git_attr;
-/* opaque structure used internally for attribute collection */
+/* opaque structures used internally for attribute collection */
struct all_attrs_item;
+struct attr_stack;
/*
* Given a string, return the gitattribute object that
@@ -38,6 +39,7 @@ struct attr_check {
struct attr_check_item *items;
int all_attrs_nr;
struct all_attrs_item *all_attrs;
+ struct attr_stack *stack;
};
extern struct attr_check *attr_check_alloc(void);
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 20/27] attr: change validity check for attribute names to use positive logic
From: Brandon Williams @ 2017-01-28 2:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170128020207.179015-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Convert 'invalid_attr_name()' to 'attr_name_valid()' and use positive
logic for the return value. In addition create a helper function that
prints out an error message when an invalid attribute name is used.
We could later update the message to exactly spell out what the
rules for a good attribute name are, etc.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/attr.c b/attr.c
index 81a3c74d8..9fe848f59 100644
--- a/attr.c
+++ b/attr.c
@@ -74,23 +74,33 @@ static unsigned hash_name(const char *name, int namelen)
return val;
}
-static int invalid_attr_name(const char *name, int namelen)
+static int attr_name_valid(const char *name, size_t namelen)
{
/*
* Attribute name cannot begin with '-' and must consist of
* characters from [-A-Za-z0-9_.].
*/
if (namelen <= 0 || *name == '-')
- return -1;
+ return 0;
while (namelen--) {
char ch = *name++;
if (! (ch == '-' || ch == '.' || ch == '_' ||
('0' <= ch && ch <= '9') ||
('a' <= ch && ch <= 'z') ||
('A' <= ch && ch <= 'Z')) )
- return -1;
+ return 0;
}
- return 0;
+ return 1;
+}
+
+static void report_invalid_attr(const char *name, size_t len,
+ const char *src, int lineno)
+{
+ struct strbuf err = STRBUF_INIT;
+ strbuf_addf(&err, _("%.*s is not a valid attribute name"),
+ (int) len, name);
+ fprintf(stderr, "%s: %s:%d\n", err.buf, src, lineno);
+ strbuf_release(&err);
}
static struct git_attr *git_attr_internal(const char *name, int len)
@@ -105,7 +115,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)
return a;
}
- if (invalid_attr_name(name, len))
+ if (!attr_name_valid(name, len))
return NULL;
FLEX_ALLOC_MEM(a, name, name, len);
@@ -196,17 +206,15 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
cp++;
len--;
}
- if (invalid_attr_name(cp, len)) {
- fprintf(stderr,
- "%.*s is not a valid attribute name: %s:%d\n",
- len, cp, src, lineno);
+ if (!attr_name_valid(cp, len)) {
+ report_invalid_attr(cp, len, src, lineno);
return NULL;
}
} else {
/*
* As this function is always called twice, once with
* e == NULL in the first pass and then e != NULL in
- * the second pass, no need for invalid_attr_name()
+ * the second pass, no need for attr_name_valid()
* check here.
*/
if (*cp == '-' || *cp == '!') {
@@ -258,10 +266,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
name += strlen(ATTRIBUTE_MACRO_PREFIX);
name += strspn(name, blank);
namelen = strcspn(name, blank);
- if (invalid_attr_name(name, namelen)) {
- fprintf(stderr,
- "%.*s is not a valid attribute name: %s:%d\n",
- namelen, name, src, lineno);
+ if (!attr_name_valid(name, namelen)) {
+ report_invalid_attr(name, namelen, src, lineno);
goto fail_return;
}
}
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 24/27] attr: tighten const correctness with git_attr and match_attr
From: Brandon Williams @ 2017-01-28 2:02 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, gitster, pclouds
In-Reply-To: <20170128020207.179015-1-bmwill@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 12 ++++++------
attr.h | 2 +-
builtin/check-attr.c | 3 ++-
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/attr.c b/attr.c
index 8f4402ef3..69643ae77 100644
--- a/attr.c
+++ b/attr.c
@@ -220,7 +220,7 @@ static void report_invalid_attr(const char *name, size_t len,
* dictionary. If no entry is found, create a new attribute and store it in
* the dictionary.
*/
-static struct git_attr *git_attr_internal(const char *name, int namelen)
+static const struct git_attr *git_attr_internal(const char *name, int namelen)
{
struct git_attr *a;
@@ -244,14 +244,14 @@ static struct git_attr *git_attr_internal(const char *name, int namelen)
return a;
}
-struct git_attr *git_attr(const char *name)
+const struct git_attr *git_attr(const char *name)
{
return git_attr_internal(name, strlen(name));
}
/* What does a matched pattern decide? */
struct attr_state {
- struct git_attr *attr;
+ const struct git_attr *attr;
const char *setto;
};
@@ -278,7 +278,7 @@ struct pattern {
struct match_attr {
union {
struct pattern pat;
- struct git_attr *attr;
+ const struct git_attr *attr;
} u;
char is_macro;
unsigned num_attr;
@@ -898,7 +898,7 @@ static int fill_one(const char *what, struct all_attrs_item *all_attrs,
int i;
for (i = a->num_attr - 1; rem > 0 && i >= 0; i--) {
- struct git_attr *attr = a->state[i].attr;
+ const struct git_attr *attr = a->state[i].attr;
const char **n = &(all_attrs[attr->attr_nr].value);
const char *v = a->state[i].setto;
@@ -922,7 +922,7 @@ static int fill(const char *path, int pathlen, int basename_offset,
const char *base = stk->origin ? stk->origin : "";
for (i = stk->num_matches - 1; 0 < rem && 0 <= i; i--) {
- struct match_attr *a = stk->attrs[i];
+ const struct match_attr *a = stk->attrs[i];
if (a->is_macro)
continue;
if (path_matches(path, pathlen, basename_offset,
diff --git a/attr.h b/attr.h
index 5aaf55c3e..abebbc19c 100644
--- a/attr.h
+++ b/attr.h
@@ -11,7 +11,7 @@ struct all_attrs_item;
* Given a string, return the gitattribute object that
* corresponds to it.
*/
-struct git_attr *git_attr(const char *);
+const struct git_attr *git_attr(const char *);
/* Internal use */
extern const char git_attr__true[];
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 40cdff13e..4d01ca0c8 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.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 27/27] attr: reformat git_attr_set_direction() function
From: Brandon Williams @ 2017-01-28 2:02 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, gitster, pclouds
In-Reply-To: <20170128020207.179015-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 62298ec2f..5493bff22 100644
--- a/attr.c
+++ b/attr.c
@@ -677,26 +677,30 @@ static struct attr_stack *read_attr_from_array(const char **list)
}
/*
- * NEEDSWORK: these two are tricky. The callers assume there is a
- * single, system-wide global state "where we read attributes from?"
- * and when the state is flipped by calling git_attr_set_direction(),
- * attr_stack is discarded so that subsequent attr_check will lazily
- * read from the right place. And they do not know or care who called
- * by them uses the attribute subsystem, hence have no knowledge of
- * existing git_attr_check instances or future ones that will be
- * created).
- *
- * Probably we need a thread_local that holds these two variables,
- * and a list of git_attr_check instances (which need to be maintained
- * by hooking into git_attr_check_alloc(), git_attr_check_initl(), and
- * git_attr_check_clear(). Then git_attr_set_direction() updates the
- * fields in that thread_local for these two variables, iterate over
- * all the active git_attr_check instances and discard the attr_stack
- * they hold. Yuck, but it sounds doable.
+ * Callers into the attribute system assume there is a single, system-wide
+ * global state where attributes are read from and when the state is flipped by
+ * calling git_attr_set_direction(), the stack frames that have been
+ * constructed need to be discarded so so that subsequent calls into the
+ * attribute system will lazily read from the right place. Since changing
+ * direction causes a global paradigm shift, it should not ever be called while
+ * another thread could potentially be calling into the attribute system.
*/
static enum git_attr_direction direction;
static struct index_state *use_index;
+void git_attr_set_direction(enum git_attr_direction new_direction,
+ struct index_state *istate)
+{
+ if (is_bare_repository() && new_direction != GIT_ATTR_INDEX)
+ die("BUG: non-INDEX attr direction in a bare repo");
+
+ if (new_direction != direction)
+ drop_all_attr_stacks();
+
+ direction = new_direction;
+ use_index = istate;
+}
+
static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
{
FILE *fp = fopen(path, "r");
@@ -1148,19 +1152,6 @@ void git_all_attrs(const char *path, struct attr_check *check)
}
}
-void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
-{
- enum git_attr_direction old = direction;
-
- if (is_bare_repository() && new != GIT_ATTR_INDEX)
- die("BUG: non-INDEX attr direction in a bare repo");
-
- direction = new;
- if (new != old)
- drop_all_attr_stacks();
- use_index = istate;
-}
-
void attr_start(void)
{
#ifndef NO_PTHREADS
diff --git a/attr.h b/attr.h
index 6f4961fdb..48ab3e1c2 100644
--- a/attr.h
+++ b/attr.h
@@ -72,7 +72,8 @@ enum git_attr_direction {
GIT_ATTR_CHECKOUT,
GIT_ATTR_INDEX
};
-void git_attr_set_direction(enum git_attr_direction, struct index_state *);
+void git_attr_set_direction(enum git_attr_direction new_direction,
+ struct index_state *istate);
extern void attr_start(void);
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 23/27] attr: remove maybe-real, maybe-macro from git_attr
From: Brandon Williams @ 2017-01-28 2:02 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, gitster, pclouds
In-Reply-To: <20170128020207.179015-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 | 75 +++++++++++++++++++++++++++++++++---------------------------------
1 file changed, 37 insertions(+), 38 deletions(-)
diff --git a/attr.c b/attr.c
index 2637804b1..8f4402ef3 100644
--- a/attr.c
+++ b/attr.c
@@ -30,20 +30,9 @@ static const char git_attr__unknown[] = "(builtin)unknown";
struct git_attr {
int attr_nr; /* unique attribute number */
- int maybe_macro;
- int maybe_real;
char name[FLEX_ARRAY]; /* attribute name */
};
-/*
- * NEEDSWORK: maybe-real, maybe-macro are not property of
- * an attribute, as it depends on what .gitattributes are
- * read. Once we introduce per git_attr_check attr_stack
- * and check_all_attr, the optimization based on them will
- * become unnecessary and can go away. So is this variable.
- */
-static int cannot_trust_maybe_real;
-
const char *git_attr_name(const struct git_attr *attr)
{
return attr->name;
@@ -142,6 +131,12 @@ static void attr_hashmap_add(struct attr_hashmap *map,
struct all_attrs_item {
const struct git_attr *attr;
const char *value;
+ /*
+ * If 'macro' is non-NULL, indicates that 'attr' is a macro based on
+ * the current attribute stack and contains a pointer to the match_attr
+ * definition of the macro
+ */
+ const struct match_attr *macro;
};
/*
@@ -187,6 +182,7 @@ static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)
*/
for (i = 0; i < check->all_attrs_nr; i++) {
check->all_attrs[i].value = ATTR__UNKNOWN;
+ check->all_attrs[i].macro = NULL;
}
}
@@ -238,8 +234,6 @@ static struct git_attr *git_attr_internal(const char *name, int namelen)
if (!a) {
FLEX_ALLOC_MEM(a, name, name, namelen);
a->attr_nr = g_attr_hashmap.map.size;
- a->maybe_real = 0;
- a->maybe_macro = 0;
attr_hashmap_add(&g_attr_hashmap, a->name, namelen, a);
assert(a->attr_nr == (g_attr_hashmap.map.size - 1));
@@ -402,7 +396,6 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
(is_macro ? 0 : namelen + 1));
if (is_macro) {
res->u.attr = git_attr_internal(name, namelen);
- res->u.attr->maybe_macro = 1;
} else {
char *p = (char *)&(res->state[num_attr]);
memcpy(p, name, namelen);
@@ -423,10 +416,6 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
/* Second pass to fill the attr_states */
for (cp = states, i = 0; *cp; i++) {
cp = parse_attr(src, lineno, cp, &(res->state[i]));
- if (!is_macro)
- res->state[i].attr->maybe_real = 1;
- if (res->state[i].attr->maybe_macro)
- cannot_trust_maybe_real = 1;
}
strbuf_release(&pattern);
@@ -904,7 +893,7 @@ static int path_matches(const char *pathname, int pathlen,
static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem);
static int fill_one(const char *what, struct all_attrs_item *all_attrs,
- struct match_attr *a, int rem)
+ const struct match_attr *a, int rem)
{
int i;
@@ -945,24 +934,34 @@ static int fill(const char *path, int pathlen, int basename_offset,
static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem)
{
- struct attr_stack *stk;
- int i;
+ const struct all_attrs_item *item = &all_attrs[nr];
- if (all_attrs[nr].value != ATTR__TRUE ||
- !all_attrs[nr].attr->maybe_macro)
+ if (item->macro && item->value == ATTR__TRUE)
+ return fill_one("expand", all_attrs, item->macro, rem);
+ else
return rem;
+}
- for (stk = attr_stack; stk; stk = stk->prev) {
- for (i = stk->num_matches - 1; 0 <= i; i--) {
- struct match_attr *ma = stk->attrs[i];
- if (!ma->is_macro)
- continue;
- if (ma->u.attr->attr_nr == nr)
- return fill_one("expand", all_attrs, ma, rem);
+/*
+ * Marks the attributes which are macros based on the attribute stack.
+ * This prevents having to search through the attribute stack each time
+ * a macro needs to be expanded during the fill stage.
+ */
+static void determine_macros(struct all_attrs_item *all_attrs,
+ const struct attr_stack *stack)
+{
+ for (; stack; stack = stack->prev) {
+ int i;
+ for (i = stack->num_matches - 1; i >= 0; i--) {
+ const struct match_attr *ma = stack->attrs[i];
+ if (ma->is_macro) {
+ int n = ma->u.attr->attr_nr;
+ if (!all_attrs[n].macro) {
+ all_attrs[n].macro = ma;
+ }
+ }
}
}
-
- return rem;
}
/*
@@ -992,15 +991,15 @@ static void collect_some_attrs(const char *path, struct attr_check *check)
prepare_attr_stack(path, dirlen);
all_attrs_init(&g_attr_hashmap, check);
+ determine_macros(check->all_attrs, attr_stack);
- if (check->nr && !cannot_trust_maybe_real) {
+ if (check->nr) {
rem = 0;
for (i = 0; i < check->nr; i++) {
- const struct git_attr *a = check->items[i].attr;
- if (!a->maybe_real) {
- struct all_attrs_item *c;
- c = check->all_attrs + a->attr_nr;
- c->value = ATTR__UNSET;
+ int n = check->items[i].attr->attr_nr;
+ struct all_attrs_item *item = &check->all_attrs[n];
+ if (item->macro) {
+ item->value = ATTR__UNSET;
rem++;
}
}
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* Re: difflame
From: Jeff King @ 2017-01-28 3:53 UTC (permalink / raw)
To: Edmundo Carmona Antoranz; +Cc: Git List
In-Reply-To: <CAOc6etaCk=OEyarMNhorM94MBnYRscCkJBM-K08snv1ecmOaPQ@mail.gmail.com>
On Tue, Jan 17, 2017 at 11:24:02PM -0600, Edmundo Carmona Antoranz wrote:
> For a very long time I had wanted to get the output of diff to include
> blame information as well (to see when something was added/removed).
This is something I've wanted, too. The trickiest part, though, is
blaming deletions, because git-blame only tracks the origin of content,
not the origin of a change.
For example, try this case:
git init
for i in $(seq 1 10); do
echo $i >>file
git add file
git commit -m "add $i"
done
sed 4d <file >tmp && mv tmp file
git commit -am "drop 4"
Running "difflame HEAD~5 HEAD" produces this output:
diff --git a/file b/file
index b414108..051c298 100644
--- a/file
+++ b/file
@@ -1,6 +1,9 @@
^2b028ff (Jeff King 2017-01-27 22:44:10 -0500 1) 1
ed056366 (Jeff King 2017-01-27 22:44:10 -0500 2) 2
771030d8 (Jeff King 2017-01-27 22:44:10 -0500 3) 3
-89c09c82 (Jeff King 2017-01-27 22:44:10 -0500 4) 4
b619039c (Jeff King 2017-01-27 22:44:10 -0500 4) 5
6a7aa0e5 (Jeff King 2017-01-27 22:44:10 -0500 5) 6
+39bc9dc4 (Jeff King 2017-01-27 22:44:10 -0500 6) 7
+f253cc8f (Jeff King 2017-01-27 22:44:10 -0500 7) 8
+85c10f46 (Jeff King 2017-01-27 22:44:10 -0500 8) 9
+89c09c82 (Jeff King 2017-01-27 22:44:10 -0500 9) 10
The last 4 lines are right; they correspond to the addition commits. But
the line taking away 4 is wrong. You can see even without looking at its
patch, because it is blamed to the same commit that added "10", which
is wrong.
Sorry I don't have a solution. I think it's an open problem with
git-blame, though you could probably script something around "git blame
--reverse". See the commit message of 85af7929e (git-blame --reverse,
2008-04-02) for some discussion.
-Peff
^ permalink raw reply
* Re: [PATCH v5 1/3] color.c: fix color_parse_mem() with value_len == 0
From: Jeff King @ 2017-01-28 4:07 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <20170119163841.zhtpz6rxdieaywuy@sigill.intra.peff.net>
On Thu, Jan 19, 2017 at 11:38:41AM -0500, Jeff King wrote:
> On Thu, Jan 19, 2017 at 06:41:21PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
> > In this code we want to match the word "reset". If len is zero,
> > strncasecmp() will return zero and we incorrectly assume it's "reset" as
> > a result.
>
> This is probably a good idea. This _is_ user-visible, so it's possible
> somebody was using empty config as a synonym for "reset". But since it
> was never documented, I feel like relying on that is somewhat crazy.
Hrm. This seems to break the add--interactive script if you do not have
color.diff.plain set:
$ GIT_TRACE=1 git add -p
...
22:58:12.568990 [pid=11401] git.c:387 trace: built-in: git 'config' '--get-color' 'color.diff.plain' ''
fatal: unable to parse default color value
config --get-color color.diff.plain : command returned error: 128
As you can see, the default value the empty string, which is now an
error.
The default in the C code for that value is GIT_COLOR_NORMAL, which
really is the empty string. So I think the old code was buggy to choose
"reset", but the new one is worse because it fails entirely. :)
We probably want something like this instead:
diff --git a/color.c b/color.c
index 190b2da96..dee61557e 100644
--- a/color.c
+++ b/color.c
@@ -212,8 +212,10 @@ int color_parse_mem(const char *value, int value_len, char *dst)
len--;
}
- if (!len)
- return -1;
+ if (!len) {
+ dst[0] = '\0';
+ return 0;
+ }
if (!strncasecmp(ptr, "reset", len)) {
xsnprintf(dst, end - dst, GIT_COLOR_RESET);
The breakage is in 'next' (it looks like it went out a few days ago; I'm
surprised I didn't notice until now).
-Peff
^ permalink raw reply related
* Re: show all merge conflicts
From: G. Sylvie Davies @ 2017-01-28 5:42 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Spiegel, git
In-Reply-To: <20170127175151.srhhczliqgvbzcre@sigill.intra.peff.net>
On Fri, Jan 27, 2017 at 9:51 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Jan 27, 2017 at 11:56:08AM -0500, Michael Spiegel wrote:
>
>> I'm trying to determine whether a merge required a conflict to resolve
>> after the merge has occurred. The git book has some advice
>> (https://git-scm.com/book/en/v2/Git-Tools-Advanced-Merging) to use
>> `git show` on the merge commit or use `git log --cc -p -1`. These
>> strategies work when the merge conflict was resolved with a change
>> that is different from either parent. When the conflict is resolved
>> with a change that is the same as one of the parents, then these
>> commands are indistinguishable from a merge that did not conflict. Is
>> it possible to distinguish between a conflict-free merge and a merge
>> conflict that is resolved by with the changes from one the parents?
>
> No. You'd have to replay the merge to know if it would have had
> conflicts.
>
Aside from the usual "git log -cc", I think this should work (replace
HEAD with whichever commit you are analyzing):
git diff --name-only HEAD^2...HEAD^1 > m1
git diff --name-only HEAD^1...HEAD^2 > b1
git diff --name-only HEAD^1..HEAD > m2
git diff --name-only HEAD^2..HEAD > b2
If files listed between m1 and b2 differ, then the merge is dirty.
Similarly for m2 and b1.
More information here:
http://stackoverflow.com/questions/27683077/how-do-you-detect-an-evil-merge-in-git/41356308#41356308
- Sylvie
> There was a patch series a few years ago that added a new diff-mode to
> do exactly that, and show the diff against what was resolved. It had a
> few issues (I don't remember exactly what) and never got merged.
>
> Certainly one complication is that you don't know exactly _how_ the
> merge was done in the first place (e.g., which merge strategy, which
> custom merge drivers were in effect, etc). But in general, replaying
> with a standard merge-recursive would get you most of what you want to
> know.
>
> I've done this manually sometimes when digging into erroneous merges
> (e.g., somebody accidentally runs "git reset -- <paths>" in the middle
> of a merge and throws away some changes.
>
> You should be able to do:
>
> git checkout $merge^1
> git merge $merge^2
> git diff -R $merge
>
> to see what the original resolution did.
>
> -Peff
^ permalink raw reply
* Re: show all merge conflicts
From: Philip Oakley @ 2017-01-28 13:43 UTC (permalink / raw)
To: G. Sylvie Davies, Jeff King; +Cc: Michael Spiegel, git
In-Reply-To: <CAAj3zPzO4+9t9_L2OXFmkihw-HwFvzybb7GXs4tTeFRyZHOaNQ@mail.gmail.com>
From: "G. Sylvie Davies" <sylvie@bit-booster.com>
> On Fri, Jan 27, 2017 at 9:51 AM, Jeff King <peff@peff.net> wrote:
>> On Fri, Jan 27, 2017 at 11:56:08AM -0500, Michael Spiegel wrote:
>>
>>> I'm trying to determine whether a merge required a conflict to resolve
>>> after the merge has occurred. The git book has some advice
>>> (https://git-scm.com/book/en/v2/Git-Tools-Advanced-Merging) to use
>>> `git show` on the merge commit or use `git log --cc -p -1`. These
>>> strategies work when the merge conflict was resolved with a change
>>> that is different from either parent. When the conflict is resolved
>>> with a change that is the same as one of the parents, then these
>>> commands are indistinguishable from a merge that did not conflict. Is
>>> it possible to distinguish between a conflict-free merge and a merge
>>> conflict that is resolved by with the changes from one the parents?
>>
>> No. You'd have to replay the merge to know if it would have had
>> conflicts.
>>
>
>
> Aside from the usual "git log -cc", I think this should work (replace
> HEAD with whichever commit you are analyzing):
>
> git diff --name-only HEAD^2...HEAD^1 > m1
> git diff --name-only HEAD^1...HEAD^2 > b1
> git diff --name-only HEAD^1..HEAD > m2
> git diff --name-only HEAD^2..HEAD > b2
>
> If files listed between m1 and b2 differ, then the merge is dirty.
> Similarly for m2 and b1.
>
> More information here:
>
> http://stackoverflow.com/questions/27683077/how-do-you-detect-an-evil-merge-in-git/41356308#41356308
>
>
> - Sylvie
This feels as though there ought to be some sort of --left-right option to
get an indication of which side various changes came from
>
>> There was a patch series a few years ago that added a new diff-mode to
>> do exactly that, and show the diff against what was resolved. It had a
>> few issues (I don't remember exactly what) and never got merged.
>>
>> Certainly one complication is that you don't know exactly _how_ the
>> merge was done in the first place (e.g., which merge strategy, which
>> custom merge drivers were in effect, etc). But in general, replaying
>> with a standard merge-recursive would get you most of what you want to
>> know.
>>
>> I've done this manually sometimes when digging into erroneous merges
>> (e.g., somebody accidentally runs "git reset -- <paths>" in the middle
>> of a merge and throws away some changes.
>>
>> You should be able to do:
>>
>> git checkout $merge^1
>> git merge $merge^2
>> git diff -R $merge
>>
>> to see what the original resolution did.
>>
>> -Peff
>
^ permalink raw reply
* Re: show all merge conflicts
From: Jeff King @ 2017-01-28 14:28 UTC (permalink / raw)
To: G. Sylvie Davies; +Cc: Michael Spiegel, git
In-Reply-To: <CAAj3zPzO4+9t9_L2OXFmkihw-HwFvzybb7GXs4tTeFRyZHOaNQ@mail.gmail.com>
On Fri, Jan 27, 2017 at 09:42:41PM -0800, G. Sylvie Davies wrote:
> Aside from the usual "git log -cc", I think this should work (replace
> HEAD with whichever commit you are analyzing):
>
> git diff --name-only HEAD^2...HEAD^1 > m1
> git diff --name-only HEAD^1...HEAD^2 > b1
> git diff --name-only HEAD^1..HEAD > m2
> git diff --name-only HEAD^2..HEAD > b2
>
> If files listed between m1 and b2 differ, then the merge is dirty.
> Similarly for m2 and b1.
>
> More information here:
>
> http://stackoverflow.com/questions/27683077/how-do-you-detect-an-evil-merge-in-git/41356308#41356308
I don't think that can reliably find evil merges, since it looks at the
file level. If you had one hunk resolved for "theirs" and one hunk for
"ours" in a given file, then the file will be listed in each diff,
whether it has evil hunks or not.
I don't think this is just about evil merges, though. For instance,
try:
seq 1 10 >file
git add file
git commit -m base
sed s/4/master/ <file >tmp && mv tmp file
git commit -am master
git checkout -b other HEAD^
sed s/4/other/ <file >tmp && mv tmp file
git commit -am other
git merge master
git checkout --ours file
git commit -am merged
merge=$(git rev-parse HEAD)
The question is: were there conflicts in $merge, and how were they
resolved?
That isn't an evil merge, but there's still something interesting to
show that "git log --cc" won't display.
Replaying the merge like:
git checkout $merge^1
git merge $merge^2
git diff -R $merge
shows you the patch to go from the conflict state to the final one.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] Documentation/stash: remove mention of git reset --hard
From: Thomas Gummerer @ 2017-01-28 19:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, git, Stephan Beyer, Marc Strapetz, Johannes Schindelin
In-Reply-To: <xmqqfuk6alba.fsf@gitster.mtv.corp.google.com>
On 01/25, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Sat, Jan 21, 2017 at 08:08:02PM +0000, Thomas Gummerer wrote:
> >
> >> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> >> index 2e9cef06e6..0ad5335a3e 100644
> >> --- a/Documentation/git-stash.txt
> >> +++ b/Documentation/git-stash.txt
> >> @@ -47,8 +47,9 @@ OPTIONS
> >>
> >> save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
> >>
> >> - Save your local modifications to a new 'stash', and run `git reset
> >> - --hard` to revert them. The <message> part is optional and gives
> >> + Save your local modifications to a new 'stash', and revert the
> >> + the changes in the working tree to match the index.
> >> + The <message> part is optional and gives
> >
> > Hrm. "git reset --hard" doesn't just make the working tree match the
> > index. It also resets the index to HEAD. So either the original or your
> > new description is wrong.
> >
> > I think it's the latter. We really do reset the index unless
> > --keep-index is specified.
>
> Correct. So the patch is a net loss. Perhaps not requiring readers
> to know "reset --hard" might be an improvement (I personally do not
> think so), but this loses crucial information from the description.
>
> Save your local modifications (both in the working tree and
> to the index) to a new 'stash', and resets the index to HEAD
> and makes the working tree match the index (i.e. runs "git
> reset --hard").
>
> That's one-and-a-half lines longer than the original, though.
Thanks all who chimed in here. My new description is definitely not
right. The reason I wanted to change it is part because it's an
implementation detail, and part because it's going to be not quite
right when the filename argument is introduced.
How about the following:
Save your local modifications to a new 'stash' and roll them back
both in the working tree and in the index.
As an added bonus this also matches what git stash save -p does.
^ permalink raw reply
* [PATCH] t0001: don't let a default ACL interfere with the umask test
From: Matt McCutchen @ 2017-01-28 20:25 UTC (permalink / raw)
To: git
The "init creates a new deep directory (umask vs. shared)" test expects
the permissions of newly created files to be based on the umask, which
fails if a default ACL is inherited from the working tree for git. So
attempt to remove a default ACL if there is one. Same idea as
8ed0a740dd42bd0724aebed6e3b07c4ea2a2d5e8. (I guess I'm the only one who
ever runs the test suite with a default ACL set.)
Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---
t/t0001-init.sh | 3 +++
1 file changed, 3 insertions(+)
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index b8fc588..e424de5 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -258,6 +258,9 @@ test_expect_success POSIXPERM 'init creates a new deep directory (umask vs. shar
(
# Leading directories should honor umask while
# the repository itself should follow "shared"
+ mkdir newdir &&
+ # Remove a default ACL if possible.
+ (setfacl -k newdir 2>/dev/null || true) &&
umask 002 &&
git init --bare --shared=0660 newdir/a/b/c &&
test_path_is_dir newdir/a/b/c/refs &&
--
2.9.3
^ permalink raw reply related
* [PATCH] merge-recursive: make "CONFLICT (rename/delete)" message show both paths
From: Matt McCutchen @ 2017-01-28 20:37 UTC (permalink / raw)
To: git
The current message printed by "git merge-recursive" for a rename/delete
conflict is like this:
CONFLICT (rename/delete): new-path deleted in HEAD and renamed in
other-branch. Version other-branch of new-path left in tree.
To be more helpful, the message should show both paths of the rename and
state that the deletion occurred at the old path, not the new path. So
change the message to the following format:
CONFLICT (rename/delete): old-path deleted in HEAD and renamed to
new-path in other-branch. Version other-branch of new-path left in tree.
Since this doubles the number of cases in handle_change_delete (modify vs.
rename), refactor the code to halve the number of cases again by merging the
cases where o->branch1 has the change and o->branch2 has the delete with the
cases that are the other way around.
Also add a simple test of the new conflict message.
Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---
This came up at:
https://github.com/cristibalan/braid/issues/41#issuecomment-275826716
Please check that my refactoring is indeed correct! All the existing tests pass
for me, but the existing test coverage of these conflict messages looks poor.
merge-recursive.c | 117 ++++++++++++++++++++++-------------------
t/t6045-merge-rename-delete.sh | 23 ++++++++
2 files changed, 86 insertions(+), 54 deletions(-)
create mode 100755 t/t6045-merge-rename-delete.sh
diff --git a/merge-recursive.c b/merge-recursive.c
index d327209..e8fce10 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1061,16 +1061,20 @@ static int merge_file_one(struct merge_options *o,
}
static int handle_change_delete(struct merge_options *o,
- const char *path,
+ const char *path, const char *old_path,
const struct object_id *o_oid, int o_mode,
- const struct object_id *a_oid, int a_mode,
- const struct object_id *b_oid, int b_mode,
+ const struct object_id *changed_oid,
+ int changed_mode,
+ const char *change_branch,
+ const char *delete_branch,
const char *change, const char *change_past)
{
- char *renamed = NULL;
+ char *alt_path = NULL;
+ const char *update_path = path;
int ret = 0;
+
if (dir_in_way(path, !o->call_depth, 0)) {
- renamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);
+ update_path = alt_path = unique_path(o, path, change_branch);
}
if (o->call_depth) {
@@ -1081,43 +1085,43 @@ static int handle_change_delete(struct merge_options *o,
*/
ret = remove_file_from_cache(path);
if (!ret)
- ret = update_file(o, 0, o_oid, o_mode,
- renamed ? renamed : path);
- } else if (!a_oid) {
- if (!renamed) {
- output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
- "and %s in %s. Version %s of %s left in tree."),
- change, path, o->branch1, change_past,
- o->branch2, o->branch2, path);
- ret = update_file(o, 0, b_oid, b_mode, path);
- } else {
- output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
- "and %s in %s. Version %s of %s left in tree at %s."),
- change, path, o->branch1, change_past,
- o->branch2, o->branch2, path, renamed);
- ret = update_file(o, 0, b_oid, b_mode, renamed);
- }
+ ret = update_file(o, 0, o_oid, o_mode, update_path);
} else {
- if (!renamed) {
- output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
- "and %s in %s. Version %s of %s left in tree."),
- change, path, o->branch2, change_past,
- o->branch1, o->branch1, path);
+ if (!alt_path) {
+ if (!old_path) {
+ output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
+ "and %s in %s. Version %s of %s left in tree."),
+ change, path, delete_branch, change_past,
+ change_branch, change_branch, path);
+ } else {
+ output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
+ "and %s to %s in %s. Version %s of %s left in tree."),
+ change, old_path, delete_branch, change_past, path,
+ change_branch, change_branch, path);
+ }
} else {
- output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
- "and %s in %s. Version %s of %s left in tree at %s."),
- change, path, o->branch2, change_past,
- o->branch1, o->branch1, path, renamed);
- ret = update_file(o, 0, a_oid, a_mode, renamed);
+ if (!old_path) {
+ output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
+ "and %s in %s. Version %s of %s left in tree at %s."),
+ change, path, delete_branch, change_past,
+ change_branch, change_branch, path, alt_path);
+ } else {
+ output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
+ "and %s to %s in %s. Version %s of %s left in tree at %s."),
+ change, old_path, delete_branch, change_past, path,
+ change_branch, change_branch, path, alt_path);
+ }
}
/*
- * No need to call update_file() on path when !renamed, since
- * that would needlessly touch path. We could call
- * update_file_flags() with update_cache=0 and update_wd=0,
- * but that's a no-op.
+ * No need to call update_file() on path when change_branch ==
+ * o->branch1 && !alt_path, since that would needlessly touch
+ * path. We could call update_file_flags() with update_cache=0
+ * and update_wd=0, but that's a no-op.
*/
+ if (change_branch != o->branch1 || alt_path)
+ ret = update_file(o, 0, changed_oid, changed_mode, update_path);
}
- free(renamed);
+ free(alt_path);
return ret;
}
@@ -1125,28 +1129,17 @@ static int handle_change_delete(struct merge_options *o,
static int conflict_rename_delete(struct merge_options *o,
struct diff_filepair *pair,
const char *rename_branch,
- const char *other_branch)
+ const char *delete_branch)
{
const struct diff_filespec *orig = pair->one;
const struct diff_filespec *dest = pair->two;
- const struct object_id *a_oid = NULL;
- const struct object_id *b_oid = NULL;
- int a_mode = 0;
- int b_mode = 0;
-
- if (rename_branch == o->branch1) {
- a_oid = &dest->oid;
- a_mode = dest->mode;
- } else {
- b_oid = &dest->oid;
- b_mode = dest->mode;
- }
if (handle_change_delete(o,
o->call_depth ? orig->path : dest->path,
+ o->call_depth ? NULL : orig->path,
&orig->oid, orig->mode,
- a_oid, a_mode,
- b_oid, b_mode,
+ &dest->oid, dest->mode,
+ rename_branch, delete_branch,
_("rename"), _("renamed")))
return -1;
@@ -1665,11 +1658,27 @@ static int handle_modify_delete(struct merge_options *o,
struct object_id *a_oid, int a_mode,
struct object_id *b_oid, int b_mode)
{
+ const char *modify_branch, *delete_branch;
+ struct object_id *changed_oid;
+ int changed_mode;
+
+ if (a_oid) {
+ modify_branch = o->branch1;
+ delete_branch = o->branch2;
+ changed_oid = a_oid;
+ changed_mode = a_mode;
+ } else {
+ modify_branch = o->branch2;
+ delete_branch = o->branch1;
+ changed_oid = b_oid;
+ changed_mode = b_mode;
+ }
+
return handle_change_delete(o,
- path,
+ path, NULL,
o_oid, o_mode,
- a_oid, a_mode,
- b_oid, b_mode,
+ changed_oid, changed_mode,
+ modify_branch, delete_branch,
_("modify"), _("modified"));
}
diff --git a/t/t6045-merge-rename-delete.sh b/t/t6045-merge-rename-delete.sh
new file mode 100755
index 0000000..8f33244
--- /dev/null
+++ b/t/t6045-merge-rename-delete.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+test_description='Merge-recursive rename/delete conflict message'
+. ./test-lib.sh
+
+test_expect_success 'rename/delete' '
+echo foo >A &&
+git add A &&
+git commit -m "initial" &&
+
+git checkout -b rename &&
+git mv A B &&
+git commit -m "rename" &&
+
+git checkout master &&
+git rm A &&
+git commit -m "delete" &&
+
+test_must_fail git merge --strategy=recursive rename >output &&
+test_i18ngrep "CONFLICT (rename/delete): A deleted in HEAD and renamed to B in rename. Version rename of B left in tree." output
+'
+
+test_done
--
2.9.3
^ permalink raw reply related
* [PATCH 0/5] introduce SWAP macro
From: René Scharfe @ 2017-01-28 21:13 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Johannes Schindelin
Exchanging the value of two variables requires declaring a temporary
variable and repeating their names. The swap macro in apply.c
simplifies this for its callers without changing the compiled binary.
Polish this macro and export it, then use it throughout the code to
reduce repetition and hide the declaration of temporary variables.
add SWAP macro
apply: use SWAP macro
use SWAP macro
diff: use SWAP macro
graph: use SWAP macro
apply.c | 23 +++++++----------------
builtin/diff-tree.c | 4 +---
builtin/diff.c | 9 +++------
contrib/coccinelle/swap.cocci | 28 ++++++++++++++++++++++++++++
diff-no-index.c | 6 ++----
diff.c | 12 ++++--------
git-compat-util.h | 10 ++++++++++
graph.c | 11 ++---------
line-range.c | 3 +--
merge-recursive.c | 5 +----
object.c | 4 +---
pack-revindex.c | 5 +----
prio-queue.c | 4 +---
strbuf.h | 4 +---
14 files changed, 63 insertions(+), 65 deletions(-)
create mode 100644 contrib/coccinelle/swap.cocci
--
2.11.0
^ permalink raw reply
* [PATCH 1/5] add SWAP macro
From: René Scharfe @ 2017-01-28 21:38 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <8ef4c833-45bd-6831-0683-6d01f30aa518@web.de>
Add a macro for exchanging the values of variables. It allows users
to avoid repetition and takes care of the temporary variable for them.
It also makes sure that the storage sizes of its two parameters are the
same. Its memcpy(1) calls are optimized away by current compilers.
Also add a conservative semantic patch for transforming only swaps of
variables of the same type.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
contrib/coccinelle/swap.cocci | 28 ++++++++++++++++++++++++++++
git-compat-util.h | 10 ++++++++++
2 files changed, 38 insertions(+)
create mode 100644 contrib/coccinelle/swap.cocci
diff --git a/contrib/coccinelle/swap.cocci b/contrib/coccinelle/swap.cocci
new file mode 100644
index 0000000000..a0934d1fda
--- /dev/null
+++ b/contrib/coccinelle/swap.cocci
@@ -0,0 +1,28 @@
+@ swap_with_declaration @
+type T;
+identifier tmp;
+T a, b;
+@@
+- T tmp = a;
++ T tmp;
++ tmp = a;
+ a = b;
+ b = tmp;
+
+@ swap @
+type T;
+T tmp, a, b;
+@@
+- tmp = a;
+- a = b;
+- b = tmp;
++ SWAP(a, b);
+
+@ extends swap @
+identifier unused;
+@@
+ {
+ ...
+- T unused;
+ ... when != unused
+ }
diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092b..66cd466eea 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char *suffix)
return strip_suffix(str, suffix, &len);
}
+#define SWAP(a, b) do { \
+ void *_swap_a_ptr = &(a); \
+ void *_swap_b_ptr = &(b); \
+ unsigned char _swap_buffer[sizeof(a)]; \
+ memcpy(_swap_buffer, _swap_a_ptr, sizeof(a)); \
+ memcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) + \
+ BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b))); \
+ memcpy(_swap_b_ptr, _swap_buffer, sizeof(a)); \
+} while (0)
+
#if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
#ifndef PROT_READ
--
2.11.0
^ permalink raw reply related
* [PATCH 2/5] apply: use SWAP macro
From: René Scharfe @ 2017-01-28 21:40 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <8ef4c833-45bd-6831-0683-6d01f30aa518@web.de>
Use the exported macro SWAP instead of the file-scoped macro swap and
remove the latter's definition.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
apply.c | 23 +++++++----------------
1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/apply.c b/apply.c
index 2ed808d429..0e2caeab9c 100644
--- a/apply.c
+++ b/apply.c
@@ -2187,29 +2187,20 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si
return offset + hdrsize + patchsize;
}
-#define swap(a,b) myswap((a),(b),sizeof(a))
-
-#define myswap(a, b, size) do { \
- unsigned char mytmp[size]; \
- memcpy(mytmp, &a, size); \
- memcpy(&a, &b, size); \
- memcpy(&b, mytmp, size); \
-} while (0)
-
static void reverse_patches(struct patch *p)
{
for (; p; p = p->next) {
struct fragment *frag = p->fragments;
- swap(p->new_name, p->old_name);
- swap(p->new_mode, p->old_mode);
- swap(p->is_new, p->is_delete);
- swap(p->lines_added, p->lines_deleted);
- swap(p->old_sha1_prefix, p->new_sha1_prefix);
+ SWAP(p->new_name, p->old_name);
+ SWAP(p->new_mode, p->old_mode);
+ SWAP(p->is_new, p->is_delete);
+ SWAP(p->lines_added, p->lines_deleted);
+ SWAP(p->old_sha1_prefix, p->new_sha1_prefix);
for (; frag; frag = frag->next) {
- swap(frag->newpos, frag->oldpos);
- swap(frag->newlines, frag->oldlines);
+ SWAP(frag->newpos, frag->oldpos);
+ SWAP(frag->newlines, frag->oldlines);
}
}
}
--
2.11.0
^ permalink raw reply related
* [PATCH 3/5] use SWAP macro
From: René Scharfe @ 2017-01-28 21:40 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <8ef4c833-45bd-6831-0683-6d01f30aa518@web.de>
Apply the semantic patch swap.cocci to convert hand-rolled swaps to use
the macro SWAP. The resulting code is shorter and easier to read, the
object code is effectively unchanged.
The patch for object.c had to be hand-edited in order to preserve the
comment before the change; Coccinelle tried to eat it for some reason.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
builtin/diff-tree.c | 4 +---
builtin/diff.c | 9 +++------
diff-no-index.c | 3 +--
diff.c | 8 +++-----
graph.c | 5 +----
line-range.c | 3 +--
merge-recursive.c | 5 +----
object.c | 4 +---
pack-revindex.c | 5 +----
prio-queue.c | 4 +---
strbuf.h | 4 +---
11 files changed, 15 insertions(+), 39 deletions(-)
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 806dd7a885..8ce00480cd 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -147,9 +147,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
tree1 = opt->pending.objects[0].item;
tree2 = opt->pending.objects[1].item;
if (tree2->flags & UNINTERESTING) {
- struct object *tmp = tree2;
- tree2 = tree1;
- tree1 = tmp;
+ SWAP(tree2, tree1);
}
diff_tree_sha1(tree1->oid.hash,
tree2->oid.hash,
diff --git a/builtin/diff.c b/builtin/diff.c
index 7f91f6d226..3d64b85337 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -45,12 +45,9 @@ static void stuff_change(struct diff_options *opt,
return;
if (DIFF_OPT_TST(opt, REVERSE_DIFF)) {
- unsigned tmp;
- const unsigned char *tmp_u;
- const char *tmp_c;
- tmp = old_mode; old_mode = new_mode; new_mode = tmp;
- tmp_u = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_u;
- tmp_c = old_name; old_name = new_name; new_name = tmp_c;
+ SWAP(old_mode, new_mode);
+ SWAP(old_sha1, new_sha1);
+ SWAP(old_name, new_name);
}
if (opt->prefix &&
diff --git a/diff-no-index.c b/diff-no-index.c
index f420786039..1ae09894d7 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -186,9 +186,8 @@ static int queue_diff(struct diff_options *o,
if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
unsigned tmp;
- const char *tmp_c;
tmp = mode1; mode1 = mode2; mode2 = tmp;
- tmp_c = name1; name1 = name2; name2 = tmp_c;
+ SWAP(name1, name2);
}
d1 = noindex_filespec(name1, mode1);
diff --git a/diff.c b/diff.c
index f08cd8e033..9de1ba264f 100644
--- a/diff.c
+++ b/diff.c
@@ -5118,13 +5118,11 @@ void diff_change(struct diff_options *options,
if (DIFF_OPT_TST(options, REVERSE_DIFF)) {
unsigned tmp;
- const unsigned char *tmp_c;
- tmp = old_mode; old_mode = new_mode; new_mode = tmp;
- tmp_c = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_c;
+ SWAP(old_mode, new_mode);
+ SWAP(old_sha1, new_sha1);
tmp = old_sha1_valid; old_sha1_valid = new_sha1_valid;
new_sha1_valid = tmp;
- tmp = old_dirty_submodule; old_dirty_submodule = new_dirty_submodule;
- new_dirty_submodule = tmp;
+ SWAP(old_dirty_submodule, new_dirty_submodule);
}
if (options->prefix &&
diff --git a/graph.c b/graph.c
index d4e8519c90..4c722303d2 100644
--- a/graph.c
+++ b/graph.c
@@ -997,7 +997,6 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf
static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)
{
int i;
- int *tmp_mapping;
short used_horizontal = 0;
int horizontal_edge = -1;
int horizontal_edge_target = -1;
@@ -1132,9 +1131,7 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
/*
* Swap mapping and new_mapping
*/
- tmp_mapping = graph->mapping;
- graph->mapping = graph->new_mapping;
- graph->new_mapping = tmp_mapping;
+ SWAP(graph->mapping, graph->new_mapping);
/*
* If graph->mapping indicates that all of the branch lines
diff --git a/line-range.c b/line-range.c
index de4e32f942..323399d16c 100644
--- a/line-range.c
+++ b/line-range.c
@@ -269,8 +269,7 @@ int parse_range_arg(const char *arg, nth_line_fn_t nth_line_cb,
return -1;
if (*begin && *end && *end < *begin) {
- long tmp;
- tmp = *end; *end = *begin; *begin = tmp;
+ SWAP(*end, *begin);
}
return 0;
diff --git a/merge-recursive.c b/merge-recursive.c
index d327209443..b7ff1ada3c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1390,14 +1390,11 @@ static int process_renames(struct merge_options *o,
branch1 = o->branch1;
branch2 = o->branch2;
} else {
- struct rename *tmp;
renames1 = b_renames;
renames2Dst = &a_by_dst;
branch1 = o->branch2;
branch2 = o->branch1;
- tmp = ren2;
- ren2 = ren1;
- ren1 = tmp;
+ SWAP(ren2, ren1);
}
if (ren1->processed)
diff --git a/object.c b/object.c
index 67d9a9e221..e680d881a4 100644
--- a/object.c
+++ b/object.c
@@ -104,9 +104,7 @@ struct object *lookup_object(const unsigned char *sha1)
* that we do not need to walk the hash table the next
* time we look for it.
*/
- struct object *tmp = obj_hash[i];
- obj_hash[i] = obj_hash[first];
- obj_hash[first] = tmp;
+ SWAP(obj_hash[i], obj_hash[first]);
}
return obj;
}
diff --git a/pack-revindex.c b/pack-revindex.c
index 6bc7c94033..1b7ebd8d7e 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -59,7 +59,6 @@ static void sort_revindex(struct revindex_entry *entries, unsigned n, off_t max)
* be a no-op, as everybody lands in the same zero-th bucket.
*/
for (bits = 0; max >> bits; bits += DIGIT_SIZE) {
- struct revindex_entry *swap;
unsigned i;
memset(pos, 0, BUCKETS * sizeof(*pos));
@@ -97,9 +96,7 @@ static void sort_revindex(struct revindex_entry *entries, unsigned n, off_t max)
* Now "to" contains the most sorted list, so we swap "from" and
* "to" for the next iteration.
*/
- swap = from;
- from = to;
- to = swap;
+ SWAP(from, to);
}
/*
diff --git a/prio-queue.c b/prio-queue.c
index e4365b00d6..17252d231b 100644
--- a/prio-queue.c
+++ b/prio-queue.c
@@ -12,9 +12,7 @@ static inline int compare(struct prio_queue *queue, int i, int j)
static inline void swap(struct prio_queue *queue, int i, int j)
{
- struct prio_queue_entry tmp = queue->array[i];
- queue->array[i] = queue->array[j];
- queue->array[j] = tmp;
+ SWAP(queue->array[i], queue->array[j]);
}
void prio_queue_reverse(struct prio_queue *queue)
diff --git a/strbuf.h b/strbuf.h
index 2262b12683..cf1b5409e7 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -109,9 +109,7 @@ extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);
*/
static inline void strbuf_swap(struct strbuf *a, struct strbuf *b)
{
- struct strbuf tmp = *a;
- *a = *b;
- *b = tmp;
+ SWAP(*a, *b);
}
--
2.11.0
^ permalink raw reply related
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