* [PATCH 19/28] attr.c: add push_stack() helper
From: Stefan Beller @ 2016-10-11 0:21 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161011002115.23312-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
There are too many repetitious "I have this new attr_stack element;
push it at the top of the stack" sequence. The new helper function
push_stack() gives us a way to express what is going on at these
places, and as a side effect, halves the number of times we mention
the attr_stack global variable.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 71 +++++++++++++++++++++++++++++++-----------------------------------
1 file changed, 33 insertions(+), 38 deletions(-)
diff --git a/attr.c b/attr.c
index 6a14c9a..f9ebdd1 100644
--- a/attr.c
+++ b/attr.c
@@ -521,6 +521,18 @@ static int git_attr_system(void)
static GIT_PATH_FUNC(git_path_info_attributes, INFOATTRIBUTES_FILE)
+static void push_stack(struct attr_stack **attr_stack_p,
+ struct attr_stack *elem, char *origin, size_t originlen)
+{
+ if (elem) {
+ elem->origin = origin;
+ if (origin)
+ elem->originlen = originlen;
+ elem->prev = *attr_stack_p;
+ *attr_stack_p = elem;
+ }
+}
+
static void bootstrap_attr_stack(void)
{
struct attr_stack *elem;
@@ -528,52 +540,35 @@ static void bootstrap_attr_stack(void)
if (attr_stack)
return;
- elem = read_attr_from_array(builtin_attr);
- elem->origin = NULL;
- elem->prev = attr_stack;
- attr_stack = elem;
-
- if (git_attr_system()) {
- elem = read_attr_from_file(git_etc_gitattributes(), 1);
- if (elem) {
- elem->origin = NULL;
- elem->prev = attr_stack;
- attr_stack = elem;
- }
- }
+ push_stack(&attr_stack, read_attr_from_array(builtin_attr), NULL, 0);
+
+ if (git_attr_system())
+ push_stack(&attr_stack,
+ read_attr_from_file(git_etc_gitattributes(), 1),
+ NULL, 0);
if (!git_attributes_file)
git_attributes_file = xdg_config_home("attributes");
- if (git_attributes_file) {
- elem = read_attr_from_file(git_attributes_file, 1);
- if (elem) {
- elem->origin = NULL;
- elem->prev = attr_stack;
- attr_stack = elem;
- }
- }
+ if (git_attributes_file)
+ push_stack(&attr_stack,
+ read_attr_from_file(git_attributes_file, 1),
+ NULL, 0);
if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
elem = read_attr(GITATTRIBUTES_FILE, 1);
- elem->origin = xstrdup("");
- elem->originlen = 0;
- elem->prev = attr_stack;
- attr_stack = elem;
+ push_stack(&attr_stack, elem, xstrdup(""), 0);
debug_push(elem);
}
elem = read_attr_from_file(git_path_info_attributes(), 1);
if (!elem)
elem = xcalloc(1, sizeof(*elem));
- elem->origin = NULL;
- elem->prev = attr_stack;
- attr_stack = elem;
+ push_stack(&attr_stack, elem, NULL, 0);
}
static void prepare_attr_stack(const char *path, int dirlen)
{
struct attr_stack *elem, *info;
- int len;
const char *cp;
/*
@@ -633,20 +628,21 @@ static void prepare_attr_stack(const char *path, int dirlen)
assert(attr_stack->origin);
while (1) {
- len = strlen(attr_stack->origin);
+ size_t len = strlen(attr_stack->origin);
+ char *origin;
+
if (dirlen <= len)
break;
cp = memchr(path + len + 1, '/', dirlen - len - 1);
if (!cp)
cp = path + dirlen;
- strbuf_add(&pathbuf, path, cp - path);
- strbuf_addch(&pathbuf, '/');
- strbuf_addstr(&pathbuf, GITATTRIBUTES_FILE);
+ strbuf_addf(&pathbuf,
+ "%.*s/%s", (int)(cp - path), path,
+ GITATTRIBUTES_FILE);
elem = read_attr(pathbuf.buf, 0);
strbuf_setlen(&pathbuf, cp - path);
- elem->origin = strbuf_detach(&pathbuf, &elem->originlen);
- elem->prev = attr_stack;
- attr_stack = elem;
+ origin = strbuf_detach(&pathbuf, &len);
+ push_stack(&attr_stack, elem, origin, len);
debug_push(elem);
}
@@ -656,8 +652,7 @@ static void prepare_attr_stack(const char *path, int dirlen)
/*
* Finally push the "info" one at the top of the stack.
*/
- info->prev = attr_stack;
- attr_stack = info;
+ push_stack(&attr_stack, info, NULL, 0);
}
static int path_matches(const char *pathname, int pathlen,
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* [PATCH 18/28] attr: support quoting pathname patterns in C style
From: Stefan Beller @ 2016-10-11 0:21 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161011002115.23312-1-sbeller@google.com>
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Full pattern must be quoted. So 'pat"t"ern attr' will give exactly
'pat"t"ern', not 'pattern'. Also clarify that leading whitespaces are
not part of the pattern and document comment syntax.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Documentation/gitattributes.txt | 8 +++++---
attr.c | 15 +++++++++++++--
t/t0003-attributes.sh | 26 ++++++++++++++++++++++++++
3 files changed, 44 insertions(+), 5 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 7aff940..8a061af 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -21,9 +21,11 @@ Each line in `gitattributes` file is of form:
pattern attr1 attr2 ...
That is, a pattern followed by an attributes list,
-separated by whitespaces. When the pattern matches the
-path in question, the attributes listed on the line are given to
-the path.
+separated by whitespaces. Leading and trailing whitespaces are
+ignored. Lines that begin with '#' are ignored. Patterns
+that begin with a double quote are quoted in C style.
+When the pattern matches the path in question, the attributes
+listed on the line are given to the path.
Each attribute can be in one of these states for a given path:
diff --git a/attr.c b/attr.c
index 6e3bd25..6a14c9a 100644
--- a/attr.c
+++ b/attr.c
@@ -13,6 +13,7 @@
#include "attr.h"
#include "dir.h"
#include "utf8.h"
+#include "quote.h"
const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
@@ -225,12 +226,21 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
const char *cp, *name, *states;
struct match_attr *res = NULL;
int is_macro;
+ struct strbuf pattern = STRBUF_INIT;
cp = line + strspn(line, blank);
if (!*cp || *cp == '#')
return NULL;
name = cp;
- namelen = strcspn(name, blank);
+
+ if (*cp == '"' && !unquote_c_style(&pattern, name, &states)) {
+ name = pattern.buf;
+ namelen = pattern.len;
+ } else {
+ namelen = strcspn(name, blank);
+ states = name + namelen;
+ }
+
if (strlen(ATTRIBUTE_MACRO_PREFIX) < namelen &&
starts_with(name, ATTRIBUTE_MACRO_PREFIX)) {
if (!macro_ok) {
@@ -250,7 +260,6 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
else
is_macro = 0;
- states = name + namelen;
states += strspn(states, blank);
/* First pass to count the attr_states */
@@ -293,9 +302,11 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
cannot_trust_maybe_real = 1;
}
+ strbuf_release(&pattern);
return res;
fail_return:
+ strbuf_release(&pattern);
free(res);
return NULL;
}
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index f0fbb42..f19ae4f 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -13,10 +13,31 @@ attr_check () {
test_line_count = 0 err
}
+attr_check_quote () {
+
+ path="$1"
+ quoted_path="$2"
+ expect="$3"
+
+ git check-attr test -- "$path" >actual &&
+ echo "\"$quoted_path\": test: $expect" >expect &&
+ test_cmp expect actual
+
+}
+
+test_expect_success 'open-quoted pathname' '
+ echo "\"a test=a" >.gitattributes &&
+ test_must_fail attr_check a a
+'
+
+
test_expect_success 'setup' '
mkdir -p a/b/d a/c b &&
(
echo "[attr]notest !test"
+ echo "\" d \" test=d"
+ echo " e test=e"
+ echo " e\" test=e"
echo "f test=f"
echo "a/i test=a/i"
echo "onoff test -test"
@@ -69,6 +90,11 @@ test_expect_success 'command line checks' '
'
test_expect_success 'attribute test' '
+
+ attr_check " d " d &&
+ attr_check e e &&
+ attr_check_quote e\" e\\\" e &&
+
attr_check f f &&
attr_check a/f f &&
attr_check a/c/f f &&
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* [PATCH 21/28] attr.c: rename a local variable check
From: Stefan Beller @ 2016-10-11 0:21 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161011002115.23312-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
Throughout this series, we are trying to use "check" to name an
instance of "git_attr_check" structure; let's rename a "check" that
refers to an array whose elements are git_attr_check_elem to avoid
confusion.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/attr.c b/attr.c
index c1d5222..abf23d8 100644
--- a/attr.c
+++ b/attr.c
@@ -682,12 +682,12 @@ static int macroexpand_one(int attr_nr, int rem);
static int fill_one(const char *what, struct match_attr *a, int rem)
{
- struct git_attr_check_elem *check = check_all_attr;
+ struct git_attr_check_elem *celem = check_all_attr;
int i;
for (i = a->num_attr - 1; 0 < rem && 0 <= i; i--) {
struct git_attr *attr = a->state[i].attr;
- const char **n = &(check[attr->attr_nr].value);
+ const char **n = &(celem[attr->attr_nr].value);
const char *v = a->state[i].setto;
if (*n == ATTR__UNKNOWN) {
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* [PATCH 25/28] attr.c: outline the future plans by heavily commenting
From: Stefan Beller @ 2016-10-11 0:21 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161011002115.23312-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 40 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index a08783a..e34b4c4 100644
--- a/attr.c
+++ b/attr.c
@@ -30,6 +30,11 @@ static const char git_attr__unknown[] = "(builtin)unknown";
#define DEBUG_ATTR 0
#endif
+/*
+ * NEEDSWORK: the global dictionary of the interned attributes
+ * must stay a singleton even after we become thread-ready.
+ * Access to these must be surrounded with mutex when it happens.
+ */
struct git_attr {
struct git_attr *next;
unsigned h;
@@ -39,10 +44,19 @@ struct git_attr {
char name[FLEX_ARRAY];
};
static int attr_nr;
+static struct git_attr *(git_attr_hash[HASHSIZE]);
+
+/*
+ * NEEDSWORK: maybe-real, maybe-macro are not property of
+ * an attribute, as it depends on what .gitattributes are
+ * read. Once we introduce per git_attr_check attr_stack
+ * and check_all_attr, the optimization based on them will
+ * become unnecessary and can go away. So is this variable.
+ */
static int cannot_trust_maybe_real;
+/* NEEDSWORK: This will become per git_attr_check */
static struct git_attr_check_elem *check_all_attr;
-static struct git_attr *(git_attr_hash[HASHSIZE]);
const char *git_attr_name(const struct git_attr *attr)
{
@@ -117,6 +131,11 @@ struct git_attr *git_attr_counted(const char *name, size_t len)
a->maybe_real = 0;
git_attr_hash[pos] = a;
+ /*
+ * NEEDSWORK: per git_attr_check check_all_attr
+ * will be initialized a lot more lazily, not
+ * like this, and not here.
+ */
REALLOC_ARRAY(check_all_attr, attr_nr);
check_all_attr[a->attr_nr].attr = a;
check_all_attr[a->attr_nr].value = ATTR__UNKNOWN;
@@ -329,6 +348,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
* .gitignore file and info/excludes file as a fallback.
*/
+/* NEEDSWORK: This will become per git_attr_check */
static struct attr_stack {
struct attr_stack *prev;
char *origin;
@@ -393,6 +413,24 @@ static struct attr_stack *read_attr_from_array(const char **list)
return res;
}
+/*
+ * NEEDSWORK: these two are tricky. The callers assume there is a
+ * single, system-wide global state "where we read attributes from?"
+ * and when the state is flipped by calling git_attr_set_direction(),
+ * attr_stack is discarded so that subsequent attr_check will lazily
+ * read from the right place. And they do not know or care who called
+ * by them uses the attribute subsystem, hence have no knowledge of
+ * existing git_attr_check instances or future ones that will be
+ * created).
+ *
+ * Probably we need a thread_local that holds these two variables,
+ * and a list of git_attr_check instances (which need to be maintained
+ * by hooking into git_attr_check_alloc(), git_attr_check_initl(), and
+ * git_attr_check_clear(). Then git_attr_set_direction() updates the
+ * fields in that thread_local for these two variables, iterate over
+ * all the active git_attr_check instances and discard the attr_stack
+ * they hold. Yuck, but it sounds doable.
+ */
static enum git_attr_direction direction;
static struct index_state *use_index;
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* [PATCH 20/28] attr.c: pass struct git_attr_check down the callchain
From: Stefan Beller @ 2016-10-11 0:21 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161011002115.23312-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
The callchain that starts from git_check_attrs() down to
collect_some_attrs() used to take an array of git_attr_check_elem
as their parameters. Pass the enclosing git_attr_check instance
instead, so that they will have access to new fields we will add to
the data structure.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/attr.c b/attr.c
index f9ebdd1..c1d5222 100644
--- a/attr.c
+++ b/attr.c
@@ -751,14 +751,25 @@ static int attr_check_is_dynamic(const struct git_attr_check *check)
* check_all_attr. If num is non-zero, only attributes in check[] are
* collected. Otherwise all attributes are collected.
*/
-static void collect_some_attrs(const char *path, int pathlen, int num,
- struct git_attr_check_elem *check)
+static void collect_some_attrs(const char *path, int pathlen,
+ struct git_attr_check *check)
{
struct attr_stack *stk;
int i, rem, dirlen;
const char *cp, *last_slash = NULL;
int basename_offset;
+ int num;
+ struct git_attr_check_elem *celem;
+
+ if (!check) {
+ /* Yuck - ugly git_all_attrs() hack! */
+ celem = NULL;
+ num = 0;
+ } else {
+ celem = check->check;
+ num = check->check_nr;
+ }
for (cp = path; cp < path + pathlen; cp++) {
if (*cp == '/' && cp[1])
@@ -778,9 +789,9 @@ static void collect_some_attrs(const char *path, int pathlen, int num,
if (num && !cannot_trust_maybe_real) {
rem = 0;
for (i = 0; i < num; i++) {
- if (!check[i].attr->maybe_real) {
+ if (!celem[i].attr->maybe_real) {
struct git_attr_check_elem *c;
- c = check_all_attr + check[i].attr->attr_nr;
+ c = check_all_attr + celem[i].attr->attr_nr;
c->value = ATTR__UNSET;
rem++;
}
@@ -794,18 +805,19 @@ static void collect_some_attrs(const char *path, int pathlen, int num,
rem = fill(path, pathlen, basename_offset, stk, rem);
}
-static int git_check_attrs(const char *path, int pathlen, int num,
- struct git_attr_check_elem *check)
+static int git_check_attrs(const char *path, int pathlen,
+ struct git_attr_check *check)
{
int i;
+ struct git_attr_check_elem *celem = check->check;
- collect_some_attrs(path, pathlen, num, check);
+ collect_some_attrs(path, pathlen, check);
- for (i = 0; i < num; i++) {
- const char *value = check_all_attr[check[i].attr->attr_nr].value;
+ for (i = 0; i < check->check_nr; i++) {
+ const char *value = check_all_attr[celem[i].attr->attr_nr].value;
if (value == ATTR__UNKNOWN)
value = ATTR__UNSET;
- check[i].value = value;
+ celem[i].value = value;
}
return 0;
@@ -816,7 +828,7 @@ void git_all_attrs(const char *path, struct git_attr_check *check)
int i;
git_attr_check_clear(check);
- collect_some_attrs(path, strlen(path), 0, NULL);
+ collect_some_attrs(path, strlen(path), NULL);
for (i = 0; i < attr_nr; i++) {
const char *name = check_all_attr[i].attr->name;
@@ -845,7 +857,7 @@ int git_check_attr_counted(const char *path, int pathlen,
struct git_attr_check *check)
{
check->finalized = 1;
- return git_check_attrs(path, pathlen, check->check_nr, check->check);
+ return git_check_attrs(path, pathlen, check);
}
int git_check_attr(const char *path, struct git_attr_check *check)
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* [PATCH 26/28] attr: make git_attr_counted static
From: Stefan Beller @ 2016-10-11 0:21 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161011002115.23312-1-sbeller@google.com>
It's not used outside the attr code, so let's keep it private.
Change-Id: I0d15e0f2ea944b31d68b9cf1a4edecac0ca2170d
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 2 +-
attr.h | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/attr.c b/attr.c
index e34b4c4..ea6d5c1 100644
--- a/attr.c
+++ b/attr.c
@@ -108,7 +108,7 @@ static void report_invalid_attr(const char *name, size_t len,
strbuf_release(&err);
}
-struct git_attr *git_attr_counted(const char *name, size_t len)
+static struct git_attr *git_attr_counted(const char *name, size_t len)
{
unsigned hval = hash_name(name, len);
unsigned pos = hval % HASHSIZE;
diff --git a/attr.h b/attr.h
index 40abc16..f85a95f 100644
--- a/attr.h
+++ b/attr.h
@@ -10,9 +10,6 @@ struct git_attr;
*/
extern struct git_attr *git_attr(const char *);
-/* The same, but with counted string */
-extern struct git_attr *git_attr_counted(const char *, size_t);
-
extern int attr_name_valid(const char *name, size_t namelen);
extern void invalid_attr_name_message(struct strbuf *, const char *, int);
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* [PATCH 27/28] attr: make git_check_attr_counted static
From: Stefan Beller @ 2016-10-11 0:21 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161011002115.23312-1-sbeller@google.com>
It's not used outside the attr code, so let's keep it private.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 4 ++--
attr.h | 1 -
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index ea6d5c1..fea0f10 100644
--- a/attr.c
+++ b/attr.c
@@ -892,8 +892,8 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
use_index = istate;
}
-int git_check_attr_counted(const char *path, int pathlen,
- struct git_attr_check *check)
+static int git_check_attr_counted(const char *path, int pathlen,
+ struct git_attr_check *check)
{
check->finalized = 1;
return git_check_attrs(path, pathlen, check);
diff --git a/attr.h b/attr.h
index f85a95f..163fcd6 100644
--- a/attr.h
+++ b/attr.h
@@ -41,7 +41,6 @@ struct git_attr_check {
extern struct git_attr_check *git_attr_check_initl(const char *, ...);
extern int git_check_attr(const char *path, struct git_attr_check *);
-extern int git_check_attr_counted(const char *, int, struct git_attr_check *);
extern struct git_attr_check *git_attr_check_alloc(void);
extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *, const struct git_attr *);
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* [PATCH 23/28] attr.c: introduce empty_attr_check_elems()
From: Stefan Beller @ 2016-10-11 0:21 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161011002115.23312-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
One codepath needs to just empty the git_attr_check_elem array in
the git_attr_check structure, without releasing the entire resource.
Introduce a helper to do so and rewrite git_attr_check_clear() using
it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/attr.c b/attr.c
index 8d8df20..cb3b702 100644
--- a/attr.c
+++ b/attr.c
@@ -746,6 +746,14 @@ static int attr_check_is_dynamic(const struct git_attr_check *check)
return (void *)(check->check) != (void *)(check + 1);
}
+static void empty_attr_check_elems(struct git_attr_check *check)
+{
+ if (!attr_check_is_dynamic(check))
+ die("BUG: emptying a statically initialized git_attr_check");
+ check->check_nr = 0;
+ check->finalized = 0;
+}
+
/*
* Collect attributes for path into the array pointed to by
* check_all_attr. If check is not NULL, only attributes in
@@ -912,12 +920,11 @@ struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *check,
void git_attr_check_clear(struct git_attr_check *check)
{
+ empty_attr_check_elems(check);
if (!attr_check_is_dynamic(check))
die("BUG: clearing a statically initialized git_attr_check");
free(check->check);
- check->check_nr = 0;
check->check_alloc = 0;
- check->finalized = 0;
}
void git_attr_check_free(struct git_attr_check *check)
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* [PATCH 28/28] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-11 0:21 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161011002115.23312-1-sbeller@google.com>
This revamps the API of the attr subsystem to be thread safe.
Before we had the question and its results in one struct type.
The typical usage of the API was
static struct git_attr_check;
if (!check)
check = git_attr_check_initl("text", NULL);
git_check_attr(path, check);
act_on(check->value[0]);
This has a couple of issues when it comes to thread safety:
* the initialization is racy in this implementation. To make it
thread safe, we need to acquire a mutex, such that only one
thread is executing the code in git_attr_check_initl.
As we do not want to introduce a mutex at each call site,
this is best done in the attr code. However to do so, we need
to have access to the `check` variable, i.e. the API has to
look like
git_attr_check_initl(struct git_attr_check*, ...);
Then one of the threads calling git_attr_check_initl will
acquire the mutex and init the `check`, while all other threads
will wait on the mutex just to realize they're late to the
party and they'll return with no work done.
* While the check for attributes to be questioned only need to
be initalized once as that part will be read only after its
initialisation, the answer may be different for each path.
Because of that we need to decouple the check and the answer,
such that each thread can obtain an answer for the path it
is currently processing.
This commit only changes the API, i.e. this doesn't implement
actual thread safety. However a later commit that introduces
thread safety needs to touch code in attr.c only and doesn't
have to add changes all over the place.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Documentation/technical/api-gitattributes.txt | 80 +++++++++++++-------
archive.c | 11 ++-
attr.c | 101 +++++++++++++++-----------
attr.h | 67 ++++++++++-------
builtin/check-attr.c | 27 ++++---
builtin/pack-objects.c | 9 ++-
convert.c | 36 ++++-----
ll-merge.c | 21 ++++--
userdiff.c | 15 ++--
ws.c | 13 ++--
10 files changed, 230 insertions(+), 150 deletions(-)
diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
index 92fc32a..2059aab 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -8,6 +8,18 @@ attributes to set of paths.
Data Structure
--------------
+extern struct git_attr *git_attr(const char *);
+
+/*
+ * Return the name of the attribute represented by the argument. The
+ * return value is a pointer to a null-delimited string that is part
+ * of the internal data structure; it should not be modified or freed.
+ */
+extern const char *git_attr_name(const struct git_attr *);
+
+extern int attr_name_valid(const char *name, size_t namelen);
+extern void invalid_attr_name_message(struct strbuf *, const char *, int);
+
`struct git_attr`::
An attribute is an opaque object that is identified by its name.
@@ -16,15 +28,17 @@ Data Structure
of no interest to the calling programs. The name of the
attribute can be retrieved by calling `git_attr_name()`.
-`struct git_attr_check_elem`::
-
- This structure represents one attribute and its value.
-
`struct git_attr_check`::
- This structure represents a collection of `git_attr_check_elem`.
+ This structure represents a collection of `struct git_attrs`.
It is passed to `git_check_attr()` function, specifying the
- attributes to check, and receives their values.
+ attributes to check, and receives their values into a corresponding
+ `struct git_attr_result`.
+
+`struct git_attr_result`::
+
+ This structure represents a collection of results to its
+ corresponding `struct git_attr_check`, that has the same order.
Attribute Values
@@ -56,16 +70,22 @@ Querying Specific Attributes
* Prepare `struct git_attr_check` using git_attr_check_initl()
function, enumerating the names of attributes whose values you are
interested in, terminated with a NULL pointer. Alternatively, an
- empty `struct git_attr_check` can be prepared by calling
- `git_attr_check_alloc()` function and then attributes you want to
- ask about can be added to it with `git_attr_check_append()`
- function.
+ empty `struct git_attr_check` as alloced by git_attr_check_alloc()
+ can be prepared by calling `git_attr_check_alloc()` function and
+ then attributes you want to ask about can be added to it with
+ `git_attr_check_append()` function.
+ git_attr_check_initl is thread safe, i.e. you can call it
+ from different threads at the same time; internally however only one
+ call at a time is processed. If the calls from different threads have
+ the same arguments, the returned `git_attr_check` may be the same.
-* Call `git_check_attr()` to check the attributes for the path.
+* Call `git_check_attr()` to check the attributes for the path,
+ the returned `git_attr_result` contains the result.
-* Inspect `git_attr_check` structure to see how each of the
- attribute in the array is defined for the path.
+* Inspect the returned `git_attr_result` structure to see how
+ each of the attribute in the array is defined for the path.
+* Do not free the result as the memory is owned by the attr subsystem.
Example
-------
@@ -89,15 +109,20 @@ static void setup_check(void)
------------
const char *path;
+ struct git_attr_result *result;
setup_check();
- git_check_attr(path, check);
+ result = git_check_attr(path, check);
------------
-. Act on `.value` member of the result, left in `check->check[]`:
+The `result` must not be free'd as it is owned by the attr subsystem.
+It is reused by the same thread, so a subsequent call to git_check_attr
+in the same thread will overwrite the result.
+
+. Act on `result->value[]`:
------------
- const char *value = check->check[0].value;
+ const char *value = result->value[0];
if (ATTR_TRUE(value)) {
The attribute is Set, by listing only the name of the
@@ -123,6 +148,8 @@ the first step in the above would be different.
static struct git_attr_check *check;
static void setup_check(const char **argv)
{
+ if (check)
+ return; /* already done */
check = git_attr_check_alloc();
while (*argv) {
struct git_attr *attr = git_attr(*argv);
@@ -138,17 +165,20 @@ Querying All Attributes
To get the values of all attributes associated with a file:
-* Prepare an empty `git_attr_check` structure by calling
- `git_attr_check_alloc()`.
+* Setup a local variables on the stack for both the question
+ `struct git_attr_check` as well as the result `struct git_attr_result`.
+ Zero them out via their respective _INIT macro.
-* Call `git_all_attrs()`, which populates the `git_attr_check`
- with the attributes attached to the path.
+* Call `git_all_attrs()`
-* Iterate over the `git_attr_check.check[]` array to examine
- the attribute names and values. The name of the attribute
- described by a `git_attr_check.check[]` object can be retrieved via
- `git_attr_name(check->check[i].attr)`. (Please note that no items
+* Iterate over the `git_attr_check.attr[]` array to examine the
+ attribute names. The name of the attribute described by a
+ `git_attr_check.attr[]` object can be retrieved via
+ `git_attr_name(check->attr[i])`. (Please note that no items
will be returned for unset attributes, so `ATTR_UNSET()` will return
false for all returned `git_array_check` objects.)
+ The respective value for an attribute can be found in the same
+ index position in of `git_attr_result`.
-* Free the `git_array_check` by calling `git_attr_check_free()`.
+* Clear the local variables by calling `git_attr_check_clear()` and
+ `git_attr_result_clear()`.
diff --git a/archive.c b/archive.c
index 11e3951..795678a 100644
--- a/archive.c
+++ b/archive.c
@@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
static struct git_attr_check *check;
+ struct git_attr_result *result;
const char *path_without_prefix;
int err;
@@ -125,11 +126,13 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
path_without_prefix = path.buf + args->baselen;
if (!check)
- check = git_attr_check_initl("export-ignore", "export-subst", NULL);
- if (!git_check_attr(path_without_prefix, check)) {
- if (ATTR_TRUE(check->check[0].value))
+ git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
+
+ result = git_check_attr(path_without_prefix, check);
+ if (result) {
+ if (ATTR_TRUE(result->value[0]))
return 0;
- args->convert = ATTR_TRUE(check->check[1].value);
+ args->convert = ATTR_TRUE(result->value[1]);
}
if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
diff --git a/attr.c b/attr.c
index fea0f10..12205b7 100644
--- a/attr.c
+++ b/attr.c
@@ -55,6 +55,16 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);
*/
static int cannot_trust_maybe_real;
+/*
+ * 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_elem {
+ const struct git_attr *attr;
+ const char *value;
+};
+
/* NEEDSWORK: This will become per git_attr_check */
static struct git_attr_check_elem *check_all_attr;
@@ -781,7 +791,7 @@ static int macroexpand_one(int nr, int rem)
static int attr_check_is_dynamic(const struct git_attr_check *check)
{
- return (void *)(check->check) != (void *)(check + 1);
+ return (void *)(check->attr) != (void *)(check + 1);
}
static void empty_attr_check_elems(struct git_attr_check *check)
@@ -799,7 +809,8 @@ static void empty_attr_check_elems(struct git_attr_check *check)
* any and all attributes that are visible are collected in it.
*/
static void collect_some_attrs(const char *path, int pathlen,
- struct git_attr_check *check, int collect_all)
+ struct git_attr_check *check,
+ struct git_attr_result *result, int collect_all)
{
struct attr_stack *stk;
@@ -825,13 +836,11 @@ static void collect_some_attrs(const char *path, int pathlen,
check_all_attr[i].value = ATTR__UNKNOWN;
if (!collect_all && !cannot_trust_maybe_real) {
- struct git_attr_check_elem *celem = check->check;
-
rem = 0;
for (i = 0; i < check->check_nr; i++) {
- if (!celem[i].attr->maybe_real) {
+ if (!check->attr[i]->maybe_real) {
struct git_attr_check_elem *c;
- c = check_all_attr + celem[i].attr->attr_nr;
+ c = check_all_attr + check->attr[i]->attr_nr;
c->value = ATTR__UNSET;
rem++;
}
@@ -845,38 +854,45 @@ static void collect_some_attrs(const char *path, int pathlen,
rem = fill(path, pathlen, basename_offset, stk, rem);
if (collect_all) {
- empty_attr_check_elems(check);
for (i = 0; i < attr_nr; i++) {
const struct git_attr *attr = check_all_attr[i].attr;
const char *value = check_all_attr[i].value;
if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
continue;
- git_attr_check_append(check, attr)->value = value;
+
+ git_attr_check_append(check, attr);
+
+ ALLOC_GROW(result->value,
+ result->check_nr + 1,
+ result->check_alloc);
+ result->value[result->check_nr++] = value;
}
}
}
static int git_check_attrs(const char *path, int pathlen,
- struct git_attr_check *check)
+ struct git_attr_check *check,
+ struct git_attr_result *result)
{
int i;
- struct git_attr_check_elem *celem = check->check;
- collect_some_attrs(path, pathlen, check, 0);
+ collect_some_attrs(path, pathlen, check, result, 0);
for (i = 0; i < check->check_nr; i++) {
- const char *value = check_all_attr[celem[i].attr->attr_nr].value;
+ const char *value = check_all_attr[check->attr[i]->attr_nr].value;
if (value == ATTR__UNKNOWN)
value = ATTR__UNSET;
- celem[i].value = value;
+ result->value[i] = value;
}
return 0;
}
-void git_all_attrs(const char *path, struct git_attr_check *check)
+void git_all_attrs(const char *path,
+ struct git_attr_check *check,
+ struct git_attr_result *result)
{
- collect_some_attrs(path, strlen(path), check, 1);
+ collect_some_attrs(path, strlen(path), check, result, 1);
}
void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
@@ -892,36 +908,41 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
use_index = istate;
}
-static int git_check_attr_counted(const char *path, int pathlen,
- struct git_attr_check *check)
+struct git_attr_result *git_check_attr(const char *path,
+ struct git_attr_check *check)
{
- check->finalized = 1;
- return git_check_attrs(path, pathlen, check);
-}
+ static struct git_attr_result *result;
+ if (!result)
+ result = xcalloc(1, sizeof(*result));
-int git_check_attr(const char *path, struct git_attr_check *check)
-{
- return git_check_attr_counted(path, strlen(path), check);
+ ALLOC_GROW(result->value, check->check_nr, result->check_alloc);
+ result->check_nr = check->check_nr;
+
+ check->finalized = 1;
+ git_check_attrs(path, strlen(path), check, result);
+ return result;
}
-struct git_attr_check *git_attr_check_initl(const char *one, ...)
+extern void git_attr_check_initl(struct git_attr_check **check_,
+ const char *one, ...)
{
- struct git_attr_check *check;
int cnt;
va_list params;
const char *param;
+ struct git_attr_check *check;
va_start(params, one);
for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
;
va_end(params);
+
check = xcalloc(1,
- sizeof(*check) + cnt * sizeof(*(check->check)));
+ sizeof(*check) + cnt * sizeof(*(check->attr)));
check->check_nr = cnt;
check->finalized = 1;
- check->check = (struct git_attr_check_elem *)(check + 1);
+ check->attr = (const struct git_attr **)(check + 1);
- check->check[0].attr = git_attr(one);
+ check->attr[0] = git_attr(one);
va_start(params, one);
for (cnt = 1; cnt < check->check_nr; cnt++) {
struct git_attr *attr;
@@ -932,10 +953,10 @@ struct git_attr_check *git_attr_check_initl(const char *one, ...)
attr = git_attr(param);
if (!attr)
die("BUG: %s: not a valid attribute name", param);
- check->check[cnt].attr = attr;
+ check->attr[cnt] = attr;
}
va_end(params);
- return check;
+ *check_ = check;
}
struct git_attr_check *git_attr_check_alloc(void)
@@ -943,18 +964,15 @@ struct git_attr_check *git_attr_check_alloc(void)
return xcalloc(1, sizeof(struct git_attr_check));
}
-struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *check,
- const struct git_attr *attr)
+void git_attr_check_append(struct git_attr_check *check,
+ const struct git_attr *attr)
{
- struct git_attr_check_elem *elem;
if (check->finalized)
die("BUG: append after git_attr_check structure is finalized");
if (!attr_check_is_dynamic(check))
die("BUG: appending to a statically initialized git_attr_check");
- ALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);
- elem = &check->check[check->check_nr++];
- elem->attr = attr;
- return elem;
+ ALLOC_GROW(check->attr, check->check_nr + 1, check->check_alloc);
+ check->attr[check->check_nr++] = attr;
}
void git_attr_check_clear(struct git_attr_check *check)
@@ -962,12 +980,13 @@ void git_attr_check_clear(struct git_attr_check *check)
empty_attr_check_elems(check);
if (!attr_check_is_dynamic(check))
die("BUG: clearing a statically initialized git_attr_check");
- free(check->check);
+ free(check->attr);
check->check_alloc = 0;
}
-void git_attr_check_free(struct git_attr_check *check)
+void git_attr_result_clear(struct git_attr_result *result)
{
- git_attr_check_clear(check);
- free(check);
+ free(result->value);
+ result->check_nr = 0;
+ result->check_alloc = 0;
}
diff --git a/attr.h b/attr.h
index 163fcd6..30bed1e 100644
--- a/attr.h
+++ b/attr.h
@@ -10,6 +10,13 @@ struct git_attr;
*/
extern struct git_attr *git_attr(const char *);
+/*
+ * Return the name of the attribute represented by the argument. The
+ * return value is a pointer to a null-delimited string that is part
+ * of the internal data structure; it should not be modified or freed.
+ */
+extern const char *git_attr_name(const struct git_attr *);
+
extern int attr_name_valid(const char *name, size_t namelen);
extern void invalid_attr_name_message(struct strbuf *, const char *, int);
@@ -22,44 +29,52 @@ extern const char git_attr__false[];
#define ATTR_FALSE(v) ((v) == git_attr__false)
#define ATTR_UNSET(v) ((v) == NULL)
-/*
- * 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_elem {
- const struct git_attr *attr;
- const char *value;
-};
-
struct git_attr_check {
int finalized;
int check_nr;
int check_alloc;
- struct git_attr_check_elem *check;
+ const struct git_attr **attr;
};
+#define GIT_ATTR_CHECK_INIT {0, 0, 0, NULL}
-extern struct git_attr_check *git_attr_check_initl(const char *, ...);
-extern int git_check_attr(const char *path, struct git_attr_check *);
+struct git_attr_result {
+ int check_nr;
+ int check_alloc;
+ const char **value;
+};
+#define GIT_ATTR_RESULT_INIT {0, 0, NULL}
+/*
+ * Initialize the `git_attr_check` via one of the following three functions:
+ *
+ * git_attr_check_alloc allocates an empty check, add more attributes via
+ * git_attr_check_append.
+ * git_all_attrs allocates a check and fills in all attributes that
+ * are set for the given path.
+ * git_attr_check_initl takes a pointer to where the check will be initialized,
+ * followed by all attributes that are to be checked.
+ * This makes it potentially thread safe as it could
+ * internally have a mutex for that memory location.
+ * Currently it is not thread safe!
+ */
extern struct git_attr_check *git_attr_check_alloc(void);
-extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *, const struct git_attr *);
+extern void git_attr_check_append(struct git_attr_check *, const struct git_attr *);
+extern void git_all_attrs(const char *path, struct git_attr_check *, struct git_attr_result *);
+extern void git_attr_check_initl(struct git_attr_check **, const char *, ...);
-extern void git_attr_check_clear(struct git_attr_check *);
-extern void git_attr_check_free(struct git_attr_check *);
+/* Query a path for its attributes */
+extern struct git_attr_result *git_check_attr(const char *path,
+ struct git_attr_check *);
-/*
- * Return the name of the attribute represented by the argument. The
- * return value is a pointer to a null-delimited string that is part
- * of the internal data structure; it should not be modified or freed.
+/**
+ * Free or clear the result struct. The underlying strings are not free'd
+ * as they are globally known.
*/
-extern const char *git_attr_name(const struct git_attr *);
+extern void git_attr_result_free(struct git_attr_result *);
+extern void git_attr_result_clear(struct git_attr_result *);
+
+extern void git_attr_check_clear(struct git_attr_check *);
-/*
- * Retrieve all attributes that apply to the specified path.
- * check holds the attributes and their values.
- */
-void git_all_attrs(const char *path, struct git_attr_check *check);
enum git_attr_direction {
GIT_ATTR_CHECKIN,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index ec61476..609b192 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -24,13 +24,17 @@ static const struct option check_attr_options[] = {
OPT_END()
};
-static void output_attr(struct git_attr_check *check, const char *file)
+static void output_attr(struct git_attr_check *check,
+ struct git_attr_result *result, const char *file)
{
int j;
int cnt = check->check_nr;
+ if (check->check_nr != result->check_nr)
+ die("BUG: confused check and result internally");
+
for (j = 0; j < cnt; j++) {
- const char *value = check->check[j].value;
+ const char *value = result->value[j];
if (ATTR_TRUE(value))
value = "set";
@@ -44,11 +48,11 @@ static void output_attr(struct git_attr_check *check, const char *file)
"%s%c" /* attrname */
"%s%c" /* attrvalue */,
file, 0,
- git_attr_name(check->check[j].attr), 0, value, 0);
+ git_attr_name(check->attr[j]), 0, value, 0);
} else {
quote_c_style(file, NULL, stdout, 0);
printf(": %s: %s\n",
- git_attr_name(check->check[j].attr), value);
+ git_attr_name(check->attr[j]), value);
}
}
}
@@ -60,14 +64,17 @@ static void check_attr(const char *prefix,
char *full_path =
prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
if (check != NULL) {
- if (git_check_attr(full_path, check))
+ struct git_attr_result *result = git_check_attr(full_path, check);
+ if (!result)
die("git_check_attr died");
- output_attr(check, file);
+ output_attr(check, result, file);
} else {
- check = git_attr_check_alloc();
- git_all_attrs(full_path, check);
- output_attr(check, file);
- git_attr_check_free(check);
+ struct git_attr_check check = GIT_ATTR_CHECK_INIT;
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
+ git_all_attrs(full_path, &check, &result);
+ output_attr(&check, &result, file);
+ git_attr_result_clear(&result);
+ git_attr_check_clear(&check);
}
free(full_path);
}
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index df4e6b6..fa8d7a9 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -898,12 +898,15 @@ static void write_pack_file(void)
static int no_try_delta(const char *path)
{
static struct git_attr_check *check;
+ struct git_attr_result *result;
if (!check)
- check = git_attr_check_initl("delta", NULL);
- if (git_check_attr(path, check))
+ git_attr_check_initl(&check, "delta", NULL);
+
+ result = git_check_attr(path, check);
+ if (!result)
return 0;
- if (ATTR_FALSE(check->check[0].value))
+ if (ATTR_FALSE(result->value[0]))
return 1;
return 0;
}
diff --git a/convert.c b/convert.c
index bb2435a..8f1c405 100644
--- a/convert.c
+++ b/convert.c
@@ -718,10 +718,8 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
return 1;
}
-static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)
+static enum crlf_action git_path_check_crlf(const char *value)
{
- const char *value = check->value;
-
if (ATTR_TRUE(value))
return CRLF_TEXT;
else if (ATTR_FALSE(value))
@@ -735,10 +733,8 @@ static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)
return CRLF_UNDEFINED;
}
-static enum eol git_path_check_eol(struct git_attr_check_elem *check)
+static enum eol git_path_check_eol(const char *value)
{
- const char *value = check->value;
-
if (ATTR_UNSET(value))
;
else if (!strcmp(value, "lf"))
@@ -748,9 +744,8 @@ static enum eol git_path_check_eol(struct git_attr_check_elem *check)
return EOL_UNSET;
}
-static struct convert_driver *git_path_check_convert(struct git_attr_check_elem *check)
+static struct convert_driver *git_path_check_convert(const char *value)
{
- const char *value = check->value;
struct convert_driver *drv;
if (ATTR_TRUE(value) || ATTR_FALSE(value) || ATTR_UNSET(value))
@@ -761,10 +756,8 @@ static struct convert_driver *git_path_check_convert(struct git_attr_check_elem
return NULL;
}
-static int git_path_check_ident(struct git_attr_check_elem *check)
+static int git_path_check_ident(const char *value)
{
- const char *value = check->value;
-
return !!ATTR_TRUE(value);
}
@@ -778,25 +771,26 @@ struct conv_attrs {
static void convert_attrs(struct conv_attrs *ca, const char *path)
{
static struct git_attr_check *check;
+ struct git_attr_result *result;
if (!check) {
- check = git_attr_check_initl("crlf", "ident",
- "filter", "eol", "text",
- NULL);
+ git_attr_check_initl(&check, "crlf", "ident", "filter",
+ "eol", "text", NULL);
user_convert_tail = &user_convert;
git_config(read_convert_config, NULL);
}
- if (!git_check_attr(path, check)) {
- struct git_attr_check_elem *ccheck = check->check;
- ca->crlf_action = git_path_check_crlf(ccheck + 4);
+ result = git_check_attr(path, check);
+
+ if (result) {
+ ca->crlf_action = git_path_check_crlf(result->value[4]);
if (ca->crlf_action == CRLF_UNDEFINED)
- ca->crlf_action = git_path_check_crlf(ccheck + 0);
+ ca->crlf_action = git_path_check_crlf(result->value[0]);
ca->attr_action = ca->crlf_action;
- ca->ident = git_path_check_ident(ccheck + 1);
- ca->drv = git_path_check_convert(ccheck + 2);
+ ca->ident = git_path_check_ident(result->value[1]);
+ ca->drv = git_path_check_convert(result->value[2]);
if (ca->crlf_action != CRLF_BINARY) {
- enum eol eol_attr = git_path_check_eol(ccheck + 3);
+ enum eol eol_attr = git_path_check_eol(result->value[3]);
if (ca->crlf_action == CRLF_AUTO && eol_attr == EOL_LF)
ca->crlf_action = CRLF_AUTO_INPUT;
else if (ca->crlf_action == CRLF_AUTO && eol_attr == EOL_CRLF)
diff --git a/ll-merge.c b/ll-merge.c
index bc6479c..385c26a 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -355,6 +355,7 @@ int ll_merge(mmbuffer_t *result_buf,
{
static struct git_attr_check *check;
static const struct ll_merge_options default_opts;
+ struct git_attr_result *result;
const char *ll_driver_name = NULL;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
const struct ll_merge_driver *driver;
@@ -369,12 +370,13 @@ int ll_merge(mmbuffer_t *result_buf,
}
if (!check)
- check = git_attr_check_initl("merge", "conflict-marker-size", NULL);
+ git_attr_check_initl(&check, "merge", "conflict-marker-size", NULL);
- if (!git_check_attr(path, check)) {
- ll_driver_name = check->check[0].value;
- if (check->check[1].value) {
- marker_size = atoi(check->check[1].value);
+ result = git_check_attr(path, check);
+ if (result) {
+ ll_driver_name = result->value[0];
+ if (result->value[1]) {
+ marker_size = atoi(result->value[1]);
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
}
@@ -394,12 +396,15 @@ int ll_merge(mmbuffer_t *result_buf,
int ll_merge_marker_size(const char *path)
{
static struct git_attr_check *check;
+ struct git_attr_result *result;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
if (!check)
- check = git_attr_check_initl("conflict-marker-size", NULL);
- if (!git_check_attr(path, check) && check->check[0].value) {
- marker_size = atoi(check->check[0].value);
+ git_attr_check_initl(&check, "conflict-marker-size", NULL);
+
+ result = git_check_attr(path, check);
+ if (result && result->value[0]) {
+ marker_size = atoi(result->value[0]);
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
}
diff --git a/userdiff.c b/userdiff.c
index 46dfd32..afc142a 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -263,21 +263,24 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {
struct userdiff_driver *userdiff_find_by_path(const char *path)
{
static struct git_attr_check *check;
+ struct git_attr_result *result;
if (!check)
- check = git_attr_check_initl("diff", NULL);
+ git_attr_check_initl(&check, "diff", NULL);
if (!path)
return NULL;
- if (git_check_attr(path, check))
+
+ result = git_check_attr(path, check);
+ if (!result)
return NULL;
- if (ATTR_TRUE(check->check[0].value))
+ if (ATTR_TRUE(result->value[0]))
return &driver_true;
- if (ATTR_FALSE(check->check[0].value))
+ if (ATTR_FALSE(result->value[0]))
return &driver_false;
- if (ATTR_UNSET(check->check[0].value))
+ if (ATTR_UNSET(result->value[0]))
return NULL;
- return userdiff_find_by_name(check->check[0].value);
+ return userdiff_find_by_name(result->value[0]);
}
struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver)
diff --git a/ws.c b/ws.c
index bb3270c..a6ffbae 100644
--- a/ws.c
+++ b/ws.c
@@ -73,15 +73,16 @@ unsigned parse_whitespace_rule(const char *string)
unsigned whitespace_rule(const char *pathname)
{
- static struct git_attr_check *attr_whitespace_rule;
+ static struct git_attr_check *check;
+ struct git_attr_result *result;
- if (!attr_whitespace_rule)
- attr_whitespace_rule = git_attr_check_initl("whitespace", NULL);
+ if (!check)
+ git_attr_check_initl(&check, "whitespace", NULL);
- if (!git_check_attr(pathname, attr_whitespace_rule)) {
- const char *value;
+ result = git_check_attr(pathname, check);
- value = attr_whitespace_rule->check[0].value;
+ if (result) {
+ const char *value = result->value[0];
if (ATTR_TRUE(value)) {
/* true (whitespace) */
unsigned all_rule = ws_tab_width(whitespace_rule_cfg);
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* [PATCH 24/28] attr.c: always pass check[] to collect_some_attrs()
From: Stefan Beller @ 2016-10-11 0:21 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161011002115.23312-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
This function used to be called with check=NULL to signal it to
collect all attributes in the global check_all_attr[] array.
Because the longer term plan is to allocate check_all_attr[] and
attr_stack data structures per git_attr_check instance (i.e. "check"
here) to make the attr subsystem thread-safe, it is unacceptable.
Pass "Are we grabbing all attributes defined in the system?" bit as
a separate argument and pass it from the callers.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/attr.c b/attr.c
index cb3b702..a08783a 100644
--- a/attr.c
+++ b/attr.c
@@ -756,11 +756,12 @@ static void empty_attr_check_elems(struct git_attr_check *check)
/*
* Collect attributes for path into the array pointed to by
- * check_all_attr. If check is not NULL, only attributes in
- * check[] are collected. Otherwise all attributes are collected.
+ * check_all_attr. If collect_all is zero, only attributes in
+ * check[] are collected. Otherwise, check[] is cleared and
+ * any and all attributes that are visible are collected in it.
*/
static void collect_some_attrs(const char *path, int pathlen,
- struct git_attr_check *check)
+ struct git_attr_check *check, int collect_all)
{
struct attr_stack *stk;
@@ -781,10 +782,11 @@ static void collect_some_attrs(const char *path, int pathlen,
}
prepare_attr_stack(path, dirlen);
+
for (i = 0; i < attr_nr; i++)
check_all_attr[i].value = ATTR__UNKNOWN;
- if (check && !cannot_trust_maybe_real) {
+ if (!collect_all && !cannot_trust_maybe_real) {
struct git_attr_check_elem *celem = check->check;
rem = 0;
@@ -803,6 +805,17 @@ static void collect_some_attrs(const char *path, int pathlen,
rem = attr_nr;
for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
rem = fill(path, pathlen, basename_offset, stk, rem);
+
+ if (collect_all) {
+ empty_attr_check_elems(check);
+ for (i = 0; i < attr_nr; i++) {
+ const struct git_attr *attr = check_all_attr[i].attr;
+ const char *value = check_all_attr[i].value;
+ if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
+ continue;
+ git_attr_check_append(check, attr)->value = value;
+ }
+ }
}
static int git_check_attrs(const char *path, int pathlen,
@@ -811,7 +824,7 @@ static int git_check_attrs(const char *path, int pathlen,
int i;
struct git_attr_check_elem *celem = check->check;
- collect_some_attrs(path, pathlen, check);
+ collect_some_attrs(path, pathlen, check, 0);
for (i = 0; i < check->check_nr; i++) {
const char *value = check_all_attr[celem[i].attr->attr_nr].value;
@@ -825,19 +838,7 @@ static int git_check_attrs(const char *path, int pathlen,
void git_all_attrs(const char *path, struct git_attr_check *check)
{
- int i;
-
- git_attr_check_clear(check);
- collect_some_attrs(path, strlen(path), NULL);
-
- for (i = 0; i < attr_nr; i++) {
- const char *name = check_all_attr[i].attr->name;
- const char *value = check_all_attr[i].value;
- if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
- continue;
- git_attr_check_append(check, git_attr(name));
- check->check[check->check_nr - 1].value = value;
- }
+ collect_some_attrs(path, strlen(path), check, 1);
}
void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* [PATCH 22/28] attr.c: correct ugly hack for git_all_attrs()
From: Stefan Beller @ 2016-10-11 0:21 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161011002115.23312-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
The collect_some_attrs() function has an ugly hack since
06a604e6 (attr: avoid heavy work when we know the specified attr is
not defined, 2014-12-28) added an optimization that relies on the
fact that the caller knows what attributes it is interested in, so
that we can leave once we know the final answer for all the
attributes the caller asked.
git_all_attrs() that asks "what attributes are on this path?"
however does not know what attributes it is interested in, other
than the vague "we are interested in all of them", which is not a
very useful thing to say. As a way to disable this optimization
for this caller, the said commit added a code to skip it when
the caller passes a NULL for the check structure.
However, it skipped the optimization not when check is NULL, but
when the number of attributes being checked is 0, which is
unnecessarily pessimistic.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/attr.c b/attr.c
index abf23d8..8d8df20 100644
--- a/attr.c
+++ b/attr.c
@@ -748,8 +748,8 @@ static int attr_check_is_dynamic(const struct git_attr_check *check)
/*
* Collect attributes for path into the array pointed to by
- * check_all_attr. If num is non-zero, only attributes in check[] are
- * collected. Otherwise all attributes are collected.
+ * check_all_attr. If check is not NULL, only attributes in
+ * check[] are collected. Otherwise all attributes are collected.
*/
static void collect_some_attrs(const char *path, int pathlen,
struct git_attr_check *check)
@@ -759,17 +759,6 @@ static void collect_some_attrs(const char *path, int pathlen,
int i, rem, dirlen;
const char *cp, *last_slash = NULL;
int basename_offset;
- int num;
- struct git_attr_check_elem *celem;
-
- if (!check) {
- /* Yuck - ugly git_all_attrs() hack! */
- celem = NULL;
- num = 0;
- } else {
- celem = check->check;
- num = check->check_nr;
- }
for (cp = path; cp < path + pathlen; cp++) {
if (*cp == '/' && cp[1])
@@ -786,9 +775,12 @@ static void collect_some_attrs(const char *path, int pathlen,
prepare_attr_stack(path, dirlen);
for (i = 0; i < attr_nr; i++)
check_all_attr[i].value = ATTR__UNKNOWN;
- if (num && !cannot_trust_maybe_real) {
+
+ if (check && !cannot_trust_maybe_real) {
+ struct git_attr_check_elem *celem = check->check;
+
rem = 0;
- for (i = 0; i < num; i++) {
+ for (i = 0; i < check->check_nr; i++) {
if (!celem[i].attr->maybe_real) {
struct git_attr_check_elem *c;
c = check_all_attr + celem[i].attr->attr_nr;
@@ -796,7 +788,7 @@ static void collect_some_attrs(const char *path, int pathlen,
rem++;
}
}
- if (rem == num)
+ if (rem == check->check_nr)
return;
}
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* [PATCH 11/28] attr: (re)introduce git_check_attr() and struct git_attr_check
From: Stefan Beller @ 2016-10-11 0:20 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161011002115.23312-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
A common pattern to check N attributes for many paths is to
(1) prepare an array A of N git_attr_check_elem items;
(2) call git_attr() to intern the N attribute names and fill A;
(3) repeatedly call git_check_attrs() for path with N and A;
A look-up for these N attributes for a single path P scans the
entire attr_stack, starting from the .git/info/attributes file and
then .gitattributes file in the directory the path P is in, going
upwards to find .gitattributes file found in parent directories.
An earlier commit 06a604e6 (attr: avoid heavy work when we know the
specified attr is not defined, 2014-12-28) tried to optimize out
this scanning for one trivial special case: when the attribute being
sought is known not to exist, we do not have to scan for it. While
this may be a cheap and effective heuristic, it would not work well
when N is (much) more than 1.
What we would want is a more customized way to skip irrelevant
entries in the attribute stack, and the definition of irrelevance
is tied to the set of attributes passed to git_check_attrs() call,
i.e. the set of attributes being sought. The data necessary for
this optimization needs to live alongside the set of attributes, but
a simple array of git_attr_check_elem simply does not have any place
for that.
Introduce "struct git_attr_check" that contains N, the number of
attributes being sought, and A, the array that holds N
git_attr_check_elem items, and a function git_check_attr() that
takes a path P and this structure as its parameters. This structure
can later be extended to hold extra data necessary for optimization.
Also, to make it easier to write the first two steps in common
cases, introduce git_attr_check_initl() helper function, which takes
a NULL-terminated list of attribute names and initialize this
structure.
As an illustration of this new API, convert archive.c that asks for
export-subst and export-ignore attributes for each paths.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
archive.c | 24 ++++++------------------
attr.c | 34 ++++++++++++++++++++++++++++++++++
attr.h | 9 +++++++++
3 files changed, 49 insertions(+), 18 deletions(-)
diff --git a/archive.c b/archive.c
index 2dc8d6c..11e3951 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 git_attr_check_elem *check)
-{
- static struct git_attr *attr_export_ignore;
- static struct git_attr *attr_export_subst;
-
- if (!attr_export_ignore) {
- attr_export_ignore = git_attr("export-ignore");
- attr_export_subst = git_attr("export-subst");
- }
- check[0].attr = attr_export_ignore;
- check[1].attr = attr_export_subst;
-}
-
struct directory {
struct directory *up;
struct object_id oid;
@@ -123,7 +110,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
struct archiver_context *c = context;
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
- struct git_attr_check_elem check[2];
+ static struct git_attr_check *check;
const char *path_without_prefix;
int err;
@@ -137,11 +124,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
strbuf_addch(&path, '/');
path_without_prefix = path.buf + args->baselen;
- setup_archive_check(check);
- if (!git_check_attrs(path_without_prefix, ARRAY_SIZE(check), check)) {
- if (ATTR_TRUE(check[0].value))
+ if (!check)
+ check = git_attr_check_initl("export-ignore", "export-subst", NULL);
+ if (!git_check_attr(path_without_prefix, check)) {
+ if (ATTR_TRUE(check->check[0].value))
return 0;
- args->convert = ATTR_TRUE(check[1].value);
+ args->convert = ATTR_TRUE(check->check[1].value);
}
if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
diff --git a/attr.c b/attr.c
index c99e23a..861e1a2 100644
--- a/attr.c
+++ b/attr.c
@@ -829,3 +829,37 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
drop_attr_stack();
use_index = istate;
}
+
+int git_check_attr(const char *path, struct git_attr_check *check)
+{
+ return git_check_attrs(path, check->check_nr, check->check);
+}
+
+struct git_attr_check *git_attr_check_initl(const char *one, ...)
+{
+ struct git_attr_check *check;
+ int cnt;
+ va_list params;
+ const char *param;
+
+ va_start(params, one);
+ for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
+ ;
+ va_end(params);
+ check = xcalloc(1,
+ sizeof(*check) + cnt * sizeof(*(check->check)));
+ check->check_nr = cnt;
+ check->check = (struct git_attr_check_elem *)(check + 1);
+
+ check->check[0].attr = git_attr(one);
+ va_start(params, one);
+ for (cnt = 1; cnt < check->check_nr; cnt++) {
+ param = va_arg(params, const char *);
+ if (!param)
+ die("BUG: counted %d != ended at %d",
+ check->check_nr, cnt);
+ check->check[cnt].attr = git_attr(param);
+ }
+ va_end(params);
+ return check;
+}
diff --git a/attr.h b/attr.h
index dd3c4a3..3fd8690 100644
--- a/attr.h
+++ b/attr.h
@@ -29,6 +29,15 @@ struct git_attr_check_elem {
const char *value;
};
+struct git_attr_check {
+ int check_nr;
+ int check_alloc;
+ struct git_attr_check_elem *check;
+};
+
+extern struct git_attr_check *git_attr_check_initl(const char *, ...);
+extern int git_check_attr(const char *path, struct git_attr_check *);
+
/*
* Return the name of the attribute represented by the argument. The
* return value is a pointer to a null-delimited string that is part
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* [PATCH 12/28] attr: convert git_all_attrs() to use "struct git_attr_check"
From: Stefan Beller @ 2016-10-11 0:20 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161011002115.23312-1-sbeller@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 git_attr_check_elem" elements. These two niches
appear only in "git check-attr".
* The caller does not know offhand what attributes it wants to ask
about and cannot use git_attr_check_initl() to prepare the
git_attr_check structure.
* The caller may not know what attributes it wants to ask at all,
and instead wants to learn everything that the given path has.
Such a caller can call git_attr_check_alloc() to allocate an empty
git_attr_check, and then call git_attr_check_append() to add
attribute names one by one. A new attribute can be appended until
git_attr_check structure is "finalized", which happens when it is
used to ask for attributes for any path by calling git_check_attr()
or git_all_attrs(). A git_attr_check structure that is initialized
by git_attr_check_initl() is already finalized when it is returned.
I am not at all happy with the way git_all_attrs() API turned out to
be, but it is only to support one niche caller ("check-attr --all"),
so I'll stop here for now.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 75 ++++++++++++++++++++++++++++++++++++++--------------
attr.h | 16 ++++++-----
builtin/check-attr.c | 51 ++++++++++++++++++-----------------
3 files changed, 90 insertions(+), 52 deletions(-)
diff --git a/attr.c b/attr.c
index 861e1a2..76f0d6b 100644
--- a/attr.c
+++ b/attr.c
@@ -724,6 +724,11 @@ static int macroexpand_one(int nr, int rem)
return rem;
}
+static int attr_check_is_dynamic(const struct git_attr_check *check)
+{
+ return (void *)(check->check) != (void *)(check + 1);
+}
+
/*
* Collect attributes for path into the array pointed to by
* check_all_attr. If num is non-zero, only attributes in check[] are
@@ -789,32 +794,21 @@ int git_check_attrs(const char *path, int num, struct git_attr_check_elem *check
return 0;
}
-int git_all_attrs(const char *path, int *num, struct git_attr_check_elem **check)
+void git_all_attrs(const char *path, struct git_attr_check *check)
{
- int i, count, j;
+ int i;
+ git_attr_check_clear(check);
collect_some_attrs(path, 0, NULL);
- /* Count the number of attributes that are set. */
- count = 0;
- for (i = 0; i < attr_nr; i++) {
- const char *value = check_all_attr[i].value;
- if (value != ATTR__UNSET && value != ATTR__UNKNOWN)
- ++count;
- }
- *num = count;
- ALLOC_ARRAY(*check, count);
- j = 0;
for (i = 0; i < attr_nr; i++) {
+ const char *name = check_all_attr[i].attr->name;
const char *value = check_all_attr[i].value;
- if (value != ATTR__UNSET && value != ATTR__UNKNOWN) {
- (*check)[j].attr = check_all_attr[i].attr;
- (*check)[j].value = value;
- ++j;
- }
+ if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
+ continue;
+ git_attr_check_append(check, git_attr(name));
+ check->check[check->check_nr - 1].value = value;
}
-
- return 0;
}
void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
@@ -832,6 +826,7 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
int git_check_attr(const char *path, struct git_attr_check *check)
{
+ check->finalized = 1;
return git_check_attrs(path, check->check_nr, check->check);
}
@@ -849,17 +844,57 @@ struct git_attr_check *git_attr_check_initl(const char *one, ...)
check = xcalloc(1,
sizeof(*check) + cnt * sizeof(*(check->check)));
check->check_nr = cnt;
+ check->finalized = 1;
check->check = (struct git_attr_check_elem *)(check + 1);
check->check[0].attr = git_attr(one);
va_start(params, one);
for (cnt = 1; cnt < check->check_nr; cnt++) {
+ struct git_attr *attr;
param = va_arg(params, const char *);
if (!param)
die("BUG: counted %d != ended at %d",
check->check_nr, cnt);
- check->check[cnt].attr = git_attr(param);
+ attr = git_attr(param);
+ if (!attr)
+ die("BUG: %s: not a valid attribute name", param);
+ check->check[cnt].attr = attr;
}
va_end(params);
return check;
}
+
+struct git_attr_check *git_attr_check_alloc(void)
+{
+ return xcalloc(1, sizeof(struct git_attr_check));
+}
+
+struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *check,
+ const struct git_attr *attr)
+{
+ struct git_attr_check_elem *elem;
+ if (check->finalized)
+ die("BUG: append after git_attr_check structure is finalized");
+ if (!attr_check_is_dynamic(check))
+ die("BUG: appending to a statically initialized git_attr_check");
+ ALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);
+ elem = &check->check[check->check_nr++];
+ elem->attr = attr;
+ return elem;
+}
+
+void git_attr_check_clear(struct git_attr_check *check)
+{
+ if (!attr_check_is_dynamic(check))
+ die("BUG: clearing a statically initialized git_attr_check");
+ free(check->check);
+ check->check_nr = 0;
+ check->check_alloc = 0;
+ check->finalized = 0;
+}
+
+void git_attr_check_free(struct git_attr_check *check)
+{
+ git_attr_check_clear(check);
+ free(check);
+}
diff --git a/attr.h b/attr.h
index 3fd8690..0d94077 100644
--- a/attr.h
+++ b/attr.h
@@ -30,6 +30,7 @@ struct git_attr_check_elem {
};
struct git_attr_check {
+ int finalized;
int check_nr;
int check_alloc;
struct git_attr_check_elem *check;
@@ -38,6 +39,12 @@ struct git_attr_check {
extern struct git_attr_check *git_attr_check_initl(const char *, ...);
extern int git_check_attr(const char *path, struct git_attr_check *);
+extern struct git_attr_check *git_attr_check_alloc(void);
+extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *, const struct git_attr *);
+
+extern void git_attr_check_clear(struct git_attr_check *);
+extern void git_attr_check_free(struct git_attr_check *);
+
/*
* Return the name of the attribute represented by the argument. The
* return value is a pointer to a null-delimited string that is part
@@ -48,13 +55,10 @@ extern const char *git_attr_name(const struct git_attr *);
int git_check_attrs(const char *path, int, struct git_attr_check_elem *);
/*
- * Retrieve all attributes that apply to the specified path. *num
- * will be set to the number of attributes on the path; **check will
- * be set to point at a newly-allocated array of git_attr_check
- * objects describing the attributes and their values. *check must be
- * free()ed by the caller.
+ * Retrieve all attributes that apply to the specified path.
+ * check holds the attributes and their values.
*/
-int git_all_attrs(const char *path, int *num, struct git_attr_check_elem **check);
+void git_all_attrs(const char *path, struct git_attr_check *check);
enum git_attr_direction {
GIT_ATTR_CHECKIN,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 97e3837..ec61476 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 git_attr_check_elem *check,
- const char *file)
+static void output_attr(struct git_attr_check *check, const char *file)
{
int j;
+ int cnt = check->check_nr;
+
for (j = 0; j < cnt; j++) {
- const char *value = check[j].value;
+ const char *value = check->check[j].value;
if (ATTR_TRUE(value))
value = "set";
@@ -42,36 +43,37 @@ static void output_attr(int cnt, struct git_attr_check_elem *check,
printf("%s%c" /* path */
"%s%c" /* attrname */
"%s%c" /* attrvalue */,
- file, 0, git_attr_name(check[j].attr), 0, value, 0);
+ file, 0,
+ git_attr_name(check->check[j].attr), 0, value, 0);
} else {
quote_c_style(file, NULL, stdout, 0);
- printf(": %s: %s\n", git_attr_name(check[j].attr), value);
+ printf(": %s: %s\n",
+ git_attr_name(check->check[j].attr), value);
}
-
}
}
static void check_attr(const char *prefix,
- int cnt, struct git_attr_check_elem *check,
+ struct git_attr_check *check,
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 (git_check_attr(full_path, check))
+ die("git_check_attr died");
+ output_attr(check, file);
} else {
- if (git_all_attrs(full_path, &cnt, &check))
- die("git_all_attrs died");
- output_attr(cnt, check, file);
- free(check);
+ check = git_attr_check_alloc();
+ git_all_attrs(full_path, check);
+ output_attr(check, file);
+ git_attr_check_free(check);
}
free(full_path);
}
static void check_attr_stdin_paths(const char *prefix,
- int cnt, struct git_attr_check_elem *check)
+ struct git_attr_check *check)
{
struct strbuf buf = STRBUF_INIT;
struct strbuf unquoted = STRBUF_INIT;
@@ -85,7 +87,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, buf.buf);
maybe_flush_or_die(stdout, "attribute to stdout");
}
strbuf_release(&buf);
@@ -100,7 +102,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_elem *check;
+ struct git_attr_check *check;
int cnt, i, doubledash, filei;
if (!is_bare_repository())
@@ -163,24 +165,21 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
if (all_attrs) {
check = NULL;
} else {
- check = xcalloc(cnt, sizeof(*check));
+ check = git_attr_check_alloc();
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]);
+ git_attr_check_append(check, a);
}
}
if (stdin_paths)
- check_attr_stdin_paths(prefix, cnt, check);
+ check_attr_stdin_paths(prefix, check);
else {
for (i = filei; i < argc; i++)
- check_attr(prefix, cnt, check, argv[i]);
+ check_attr(prefix, check, argv[i]);
maybe_flush_or_die(stdout, "attribute to stdout");
}
return 0;
--
2.10.1.382.ga23ca1b.dirty
^ permalink raw reply related
* Invisible file remove during merge
From: Octavio Alvarez @ 2016-10-11 1:04 UTC (permalink / raw)
To: git
Hi.
If I do a "git rm file" during conflict resolution, the removed file
(mysterious-file.txt) does not show up on "git log --stat" nor "git show
--patch" for the merge commit.
"git log --all --stat --follow -- mysterious-file.txt" only shows the
commit where the file was added.
I also tried --full-history
I prepared a sample repository that exhibits a behavior I was not expecting:
https://github.com/alvarezp/invisible-remove-git-demo
Am I missing some option?
Thanks.
^ permalink raw reply
* Re: Invisible file remove during merge
From: Jeff King @ 2016-10-11 2:42 UTC (permalink / raw)
To: Octavio Alvarez; +Cc: git
In-Reply-To: <d7e42701-363b-70a4-8eb0-da4715cd9ba5@alvarezp.ods.org>
On Mon, Oct 10, 2016 at 06:04:26PM -0700, Octavio Alvarez wrote:
> If I do a "git rm file" during conflict resolution, the removed file
> (mysterious-file.txt) does not show up on "git log --stat" nor "git show
> --patch" for the merge commit.
"git log" does not do merge diffs by default; you need to pass one of
"--cc", "-c", or "-m".
"git show" will do "--cc" by default. Any file that is the same in the
merge result and _any_ of the branches is considered uninteresting here,
and that is what happened in your example. One parent had the file, the
other did not, and you deleted it to match the second parent.
See the "Combined Diff" section of "git help log" for more details.
When debugging merge resolutions, I often find it useful to use "-m",
which diffs separately against _each_ parent (though it is often tedious
to pick through the uninteresting bits, which is why "-c" and "--cc"
were invented in the first place).
> "git log --all --stat --follow -- mysterious-file.txt" only shows the
> commit where the file was added.
Try that with "-m", which will show the merge in question (and another,
more boring merge, which hopefully shows why "-m" is not the default).
-Peff
^ permalink raw reply
* [PATCH v2] pretty: fix document link for color specification
From: René Scharfe @ 2016-10-11 3:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Tom Hale, git, Duy Nguyen
In-Reply-To: <20161010205521.vxpwjrlelig3ilpe@sigill.intra.peff.net>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Removed confusing and unnecessary reference to the "colors" paragraph.
Documentation/pretty-formats.txt | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index a942d57..89e3bc6 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -166,7 +166,8 @@ endif::git-rev-list[]
- '%Cgreen': switch color to green
- '%Cblue': switch color to blue
- '%Creset': reset color
-- '%C(...)': color specification, as described in color.branch.* config option;
+- '%C(...)': color specification, as described under Values in the
+ "CONFIGURATION FILE" section of linkgit:git-config[1];
adding `auto,` at the beginning will emit color only when colors are
enabled for log output (by `color.diff`, `color.ui`, or `--color`, and
respecting the `auto` settings of the former if we are going to a
--
2.10.1
^ permalink raw reply related
* Re: [PATCH v2] pretty: fix document link for color specification
From: Jeff King @ 2016-10-11 4:45 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Tom Hale, git, Duy Nguyen
In-Reply-To: <e0d64654-1562-2e71-8d42-3fc549a98d12@web.de>
On Tue, Oct 11, 2016 at 05:41:14AM +0200, René Scharfe wrote:
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> Removed confusing and unnecessary reference to the "colors" paragraph.
This looks good to me.
-Peff
^ permalink raw reply
* Re: [PATCH v4 4/4] mergetool: honor -O<orderfile>
From: David Aguilar @ 2016-10-11 5:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Johannes Sixt, Luis Gutierrez
In-Reply-To: <xmqqa8ecf48c.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 10, 2016 at 11:28:35AM -0700, Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> writes:
>
> > Teach mergetool to pass "-O<orderfile>" down to `git diff` when
> > specified on the command-line.
> >
> > Helped-by: Johannes Sixt <j6t@kdbg.org>
> > Signed-off-by: David Aguilar <davvid@gmail.com>
> > ---
> > Since v3:
> >
> > I missed one last piped invocation of "git mergetool" in the tests,
> > which has been fixed.
>
> I only see 4/4 in v4; am I correct to assume that 1-3/4 of v4 are
> the same as their counterparts in v3?
Yes, 1-3 are unchanged since v3.
Thanks for checking,
--
David
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Matthieu Moy @ 2016-10-11 7:39 UTC (permalink / raw)
To: Jeff King
Cc: Larry Finger, Mathieu Lienard--Mayor, Jorge Juan Garcia Garcia,
Remi Lespinet, git
In-Reply-To: <20161010215711.oqnoiz7qfmxm27cr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> [+cc authors of b1c8a11, which regressed this case; I'll quote liberally
> to give context]
>
> On Mon, Oct 10, 2016 at 05:48:56PM -0400, Jeff King wrote:
>
>> I can't reproduce the problem with this simple setup:
>>
>> git init
>> echo content >file && git add file
>> git commit -F- <<-\EOF
>> the subject
>>
>> the body
>>
>> Cc: Stable <stable@vger.kernel.org> [4.8+]
Is this RFC2822 compliant (https://tools.ietf.org/html/rfc2822)? Not an
expert of the norm, but my understanding is that you're allowed to use
either "Name <addr@domain.com>" (name-addr) or addr@domain.com
(addr-spec), and that comments are allowed within parenthesis like
"Stable <stable@vger.kernel.org> (4.8+)".
What is this [4.8+] supposed to mean?
The guilty function is parse_mailboxes in perl/Git.pm. It should be
rather easy to modify it but I need to understand the spec before I can
try to implement anything.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* RE: git merge deletes my changes
From: Eduard Egorov @ 2016-10-11 7:11 UTC (permalink / raw)
To: 'Jeff King', 'paul@mad-scientist.net'
Cc: 'git@vger.kernel.org'
In-Reply-To: <20161010152626.frc3ypflwnhzidea@sigill.intra.peff.net>
Hello Jeff, Paul
Thanks a lot for sharing your time!
As you correctly stated, I used "--squash" for preventing my project history being flooded with the ceph_ansible's commits and I didn't really understand that this is critical for having relationship history with other repository.
Also I realized that made another big mistake - I should use another branch (that is directly branched from 'ceph-ansible' one) for storing my ceph-ansible's code updates. And, periodically, this branch will be merged into my main repo with '-s subtree --squash' without worrying about merge conflicts since this folder would never be changed in main repo's branches.
Again - thank you very much for your help!
P.S. Please feel free to copy-paste this thread as an answer on SO (http://stackoverflow.com/questions/39954265/git-merge-s-subtree-works-incorrectly ) - I will accept it with great pleasure ;-)
With best regards
Eduard Egorov
-----Original Message-----
From: Paul Smith [mailto:paul@mad-scientist.net]
Sent: Monday, October 10, 2016 8:52 PM
To: Eduard Egorov <Eduard.Egorov@icl-services.com>; 'git@vger.kernel.org' <git@vger.kernel.org>
Subject: Re: git merge deletes my changes
On Mon, 2016-10-10 at 10:19 +0000, Eduard Egorov wrote:
> # ~/gitbuild/git-2.10.1/git merge -s subtree --squash ceph_ansible
>
> Can somebody confirm this please? Doesn't "merge -s subtree" really
> merges branches?
I think possibly you're not fully understanding what the --squash flag does... that's what's causing your issue here, not the "-s" option.
A squash merge takes the commits that would be merged from the origin branch and squashes them into a single patch and applies them to the current branch as a new commit... but this new commit is not a merge commit (that is, when you look at it with "git show" etc. the commit will have only one parent, not two--or more--parents like a normal merge commit).
Basically, it's syntactic sugar for a diff plus patch operation plus some Git goodness wrapped around it to make it easier to use.
But ultimately once you're done, Git has no idea that this new commit has any relationship whatsoever to the origin branch. So the next time you merge, Git doesn't know that there was a previous merge and it will try to merge everything from scratch rather than starting at the previous common merge point.
So either you'll have to use a normal, non-squash merge, or else you'll have to tell Git by hand what the previous common merge point was (as Jeff King's excellent email suggests). Or else, you'll have to live with this behavior.
-----Original Message-----
From: Jeff King [mailto:peff@peff.net]
Sent: Monday, October 10, 2016 6:26 PM
To: Eduard Egorov <Eduard.Egorov@icl-services.com>
Cc: 'git@vger.kernel.org' <git@vger.kernel.org>
Subject: Re: git merge deletes my changes
On Mon, Oct 10, 2016 at 09:39:13AM +0000, Eduard Egorov wrote:
> A week ago, I've reset a state of 'ceph-ansible' folder in %current%
> branch with code from corresponding branch (that tracks an upstream
> from github):
>
> # git read-tree --prefix=ceph-ansible/ -u ceph_ansible
This pulls in the subtree files, but there's no actual relationship with the commit history in ceph_ansible.
So later...
> Then I've committed several changes, including:
>
> 1. Renamed file and commited:
> # git mv site.yml.sample site.yml
>
> 2. Made some changes and committed
>
> 3. Pulled updates from original branch by:
> # git merge -s subtree --squash ceph_ansible
>
> It said:
> Auto-merging ceph-ansible/site.yml.sample
> blablabla
> Squash commit -- not updating HEAD
> Automatic merge went well; stopped before committing as requested
When you merge from ceph_ansible, there is no shared history, and git uses the empty tree as a common ancestor. It looks like the other side added site.yml.sample, for instance, because that is a change from the empty tree.
> A post on SO:
> http://stackoverflow.com/questions/39954265/git-merge-deletes-my-chang
> es
As you noted on SO, modern git disallows merges of unrelated history by default, because it's usually a mistake to do so.
If you are doing repeated merges into the subtree, you need to somehow tell git how the histories are related. The obvious answer is to do a "git merge -s ours ceph_ansible" after your initial read-tree, so that git knows you've pulled in the changes up to that point. But I'd guess from your use of "--squash" that you don't want to carry the ceph_ansible history in your project.
So you need to record the original upstream commit somewhere (probably in the commit message when you commit the read-tree result), and then ask git to use that as the merge-base during subsequent merges (which will require using plumbing codes, as git-merge wants to compute the merge base itself). I believe the git-subtree command (in contrib/subtree of git.git) handles this use case, but I haven't used it myself.
-Peff
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Lars Schneider @ 2016-10-11 8:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, peff
In-Reply-To: <xmqqint0dlis.fsf@gitster.mtv.corp.google.com>
> On 10 Oct 2016, at 21:58, Junio C Hamano <gitster@pobox.com> wrote:
>
> larsxschneider@gmail.com writes:
>
> [...]
>> +# Count unique lines except clean invocations in two files and compare
>> +# them. Clean invocations are not counted because their number can vary.
>> +# c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
>> +test_cmp_count_except_clean () {
>> + for FILE in $@
>> + do
>> + sort $FILE | uniq -c | sed "s/^[ ]*//" |
>> + sed "s/^\([0-9]\) IN: clean/x IN: clean/" >$FILE.tmp
>> + cat $FILE.tmp >$FILE
>> + done &&
>> + test_cmp $@
>> +}
>
> Why do you even _care_ about the number of invocations? While I
> told you why "clean" could be called multiple times under racy Git
> as an example, that was not meant to be an exhaustive example. I
> wouldn't be surprised if we needed to run smudge twice, for example,
> in some weirdly racy cases in the future.
>
> Can we just have the correctness (i.e. "we expect that the working
> tree file gets this as the result of checking it out, and we made
> sure that is the case") test without getting into such an
> implementation detail?
My goal is to check that clean/smudge is invoked at least once. I could
just run `uniq` to achieve that but then all other filter commands could
happen multiple times and the test would not detect that.
I also prefer to check the filter commands to ensure the filter is
working as expected (e.g. no multiple start ups etc) in addition to
checking the working tree.
Would the patch below work for you? If yes, then please squash it into
"convert: add filter.<driver>.process option".
Thank you,
Lars
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 9f892c0..714f706 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -31,38 +31,33 @@ filter_git () {
rm -f git-stderr.log
}
-# Count unique lines in two files and compare them.
-test_cmp_count () {
- for FILE in $@
- do
- sort $FILE | uniq -c | sed "s/^[ ]*//" >$FILE.tmp
- cat $FILE.tmp >$FILE
- done &&
- test_cmp $@
-}
-
-# Count unique lines except clean invocations in two files and compare
-# them. Clean invocations are not counted because their number can vary.
+# Compare two files and ensure that `clean` and `smudge` respectively are
+# called at least once if specified in the `expect` file. The actual
+# invocation count is not relevant because their number can vary.
# c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
-test_cmp_count_except_clean () {
- for FILE in $@
+test_cmp_count () {
+ expect=$1 actual=$2
+ for FILE in "$expect" "$actual"
do
- sort $FILE | uniq -c | sed "s/^[ ]*//" |
- sed "s/^\([0-9]\) IN: clean/x IN: clean/" >$FILE.tmp
- cat $FILE.tmp >$FILE
+ sort "$FILE" | uniq -c | sed "s/^[ ]*//" |
+ sed "s/^\([0-9]\) IN: clean/x IN: clean/" |
+ sed "s/^\([0-9]\) IN: smudge/x IN: smudge/" >"$FILE.tmp" &&
+ cat "$FILE.tmp" >"$FILE"
done &&
- test_cmp $@
+ test_cmp "$expect" "$actual"
}
-# Compare two files but exclude clean invocations because they can vary.
+# Compare two files but exclude all `clean` invocations because Git can
+# call `clean` zero or more times.
# c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
test_cmp_exclude_clean () {
- for FILE in $@
+ expect=$1 actual=$2
+ for FILE in "$expect" "$actual"
do
- grep -v "IN: clean" $FILE >$FILE.tmp
- cat $FILE.tmp >$FILE
+ grep -v "IN: clean" "$FILE" >"$FILE.tmp" &&
+ cat "$FILE.tmp" >"$FILE"
done &&
- test_cmp $@
+ test_cmp "$expect" "$actual"
}
# Check that the contents of two files are equal and that their rot13 version
@@ -395,7 +390,7 @@ test_expect_success PERL 'required process filter should filter data' '
IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
STOP
EOF
- test_cmp_count_except_clean expected.log rot13-filter.log &&
+ test_cmp_count expected.log rot13-filter.log &&
rm -f test2.r "testsubdir/test3 '\''sq'\'',\$x.r" &&
^ permalink raw reply related
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Johannes Schindelin @ 2016-10-11 9:44 UTC (permalink / raw)
To: Jeff King; +Cc: Duy Nguyen, Johannes Sixt, Junio C Hamano, Git Mailing List
In-Reply-To: <20161009205854.byq2wqgemtmwudfb@sigill.intra.peff.net>
Hi,
On Sun, 9 Oct 2016, Jeff King wrote:
> On Sun, Oct 09, 2016 at 06:32:38PM +0700, Duy Nguyen wrote:
>
> > > If you mean ambiguity between the old "alias.X" and the new "alias.X.*",
> > > then yes, I think that's an unavoidable part of the transition. IMHO,
> > > the new should take precedence over the old, and people will gradually
> > > move from one to the other.
> >
> > Do we really need to treat this differently than
> >
> > [alias]
> > d2u = !dos2unix
> > d2u = C:/cygwin/bin/dos3unix.exe
> >
> > ?
> >
> > Another similar case is one d2u (could be either old syntax or new) is
> > defined in ~/.gitconfig and the other d2u in $GIT_DIR/config. In
> > either case, the "latest" d2u definition wins.
>
> Yeah, that's reasonable, too. So:
>
> [alias]
> d2u = "!dos2unix"
>
> acts exactly as if:
>
> [alias "d2u"]
> command = dos2unix
> type = shell
>
> was specified at that point, which is easy to understand.
It is easy to understand, and even easier to get wrong or out of sync. I
really liked the ease of *one* `git config` call to add new aliases. Not
sure that I like the need for more such calls just to add *one* alias (one
config call for "shell", one for "don't cd up", etc).
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 05/25] sequencer: eventually release memory allocated for the option values
From: Johannes Schindelin @ 2016-10-11 10:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqqzimbc0gw.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 10 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/builtin/revert.c b/builtin/revert.c
> > index 7365559..fce9c75 100644
> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -174,6 +174,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
> >
> > if (argc > 1)
> > usage_with_options(usage_str, options);
> > +
> > + /* These option values will be free()d */
> > + if (opts->gpg_sign)
> > + opts->gpg_sign = xstrdup(opts->gpg_sign);
> > + if (opts->strategy)
> > + opts->strategy = xstrdup(opts->strategy);
> > }
>
> This certainly is good, but I wonder if a new variant of OPT_STRING
> and OPTION_STRING that does the strdup for you, something along the
> lines of ...
>
> diff --git a/parse-options.c b/parse-options.c
> index 312a85dbde..6aab6b0b05 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -138,6 +138,21 @@ static int get_value(struct parse_opt_ctx_t *p,
> return get_arg(p, opt, flags, (const char **)opt->value);
> return 0;
>
> + case OPTION_STRDUP:
> + err = 0;
> + free(opt->value);
Probably
free(*(char **)opt->value);
instead.
> + if (unset)
> + *(const char **)opt->value = NULL;
> + else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
> + *(const char **)opt->value = xstrdup(opt->defval);
> + else {
> + const char *v;
> + err = get_arg(p, opt, flags, &v);
> + if (!err)
> + *(const char **)opt->value = xstrdup(v);
> + }
> + return err;
> +
> case OPTION_FILENAME:
> err = 0;
> if (unset)
>
> ... may make it even more pleasant to use?
With s/even// I would agree.
I will keep this patch in mind and will try to come back to it, once the
rebase--helper patches are well on target for `master`.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Torsten Bögershausen @ 2016-10-11 10:09 UTC (permalink / raw)
To: Lars Schneider; +Cc: Junio C Hamano, git, Jakub Narębski, peff
In-Reply-To: <02E73D87-B036-40CA-AF54-F93415A028BC@gmail.com>
On Tue, Oct 11, 2016 at 10:11:22AM +0200, Lars Schneider wrote:
>
> > On 10 Oct 2016, at 21:58, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > larsxschneider@gmail.com writes:
> >
> > [...]
> >> +# Count unique lines except clean invocations in two files and compare
> >> +# them. Clean invocations are not counted because their number can vary.
> >> +# c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
> >> +test_cmp_count_except_clean () {
> >> + for FILE in $@
> >> + do
> >> + sort $FILE | uniq -c | sed "s/^[ ]*//" |
> >> + sed "s/^\([0-9]\) IN: clean/x IN: clean/" >$FILE.tmp
> >> + cat $FILE.tmp >$FILE
> >> + done &&
> >> + test_cmp $@
> >> +}
> >
> > Why do you even _care_ about the number of invocations? While I
> > told you why "clean" could be called multiple times under racy Git
> > as an example, that was not meant to be an exhaustive example. I
> > wouldn't be surprised if we needed to run smudge twice, for example,
> > in some weirdly racy cases in the future.
> >
> > Can we just have the correctness (i.e. "we expect that the working
> > tree file gets this as the result of checking it out, and we made
> > sure that is the case") test without getting into such an
> > implementation detail?
>
> My goal is to check that clean/smudge is invoked at least once. I could
> just run `uniq` to achieve that but then all other filter commands could
> happen multiple times and the test would not detect that.
>
> I also prefer to check the filter commands to ensure the filter is
> working as expected (e.g. no multiple start ups etc) in addition to
> checking the working tree.
>
> Would the patch below work for you? If yes, then please squash it into
> "convert: add filter.<driver>.process option".
>
> Thank you,
> Lars
>
>
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 9f892c0..714f706 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -31,38 +31,33 @@ filter_git () {
> rm -f git-stderr.log
> }
>
> -# Count unique lines in two files and compare them.
> -test_cmp_count () {
> - for FILE in $@
> - do
> - sort $FILE | uniq -c | sed "s/^[ ]*//" >$FILE.tmp
> - cat $FILE.tmp >$FILE
> - done &&
> - test_cmp $@
> -}
> -
> -# Count unique lines except clean invocations in two files and compare
> -# them. Clean invocations are not counted because their number can vary.
> +# Compare two files and ensure that `clean` and `smudge` respectively are
> +# called at least once if specified in the `expect` file. The actual
> +# invocation count is not relevant because their number can vary.
> # c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
> -test_cmp_count_except_clean () {
> - for FILE in $@
> +test_cmp_count () {
> + expect=$1 actual=$2
That could be
expect="$1"
actual="$2"
> + for FILE in "$expect" "$actual"
> do
> + sort "$FILE" | uniq -c | sed "s/^[ ]*//" |
> + sed "s/^\([0-9]\) IN: clean/x IN: clean/" |
> + sed "s/^\([0-9]\) IN: smudge/x IN: smudge/" >"$FILE.tmp" &&
> + cat "$FILE.tmp" >"$FILE"
How about
cp "$FILE.tmp" "$FILE"
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Johannes Schindelin @ 2016-10-11 10:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqqlgxvdf90.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 10 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > if (parent && parse_commit(parent) < 0)
> > - /* TRANSLATORS: The first %s will be "revert" or
> > - "cherry-pick", the second %s a SHA1 */
> > + /*
> > + * TRANSLATORS: The first %s will be a "todo" command like
> > + * "revert" or "pick", the second %s a SHA1.
> > + */
>
> You may want to double check this with i18n folks; IIRC the tool
> that extracts TRANSLATORS: comment was somewhat particular about
> where that magic "TRANSLATORS:" token resides on a comment line and
> that is why we have this multi-line comment formatted in an unusual
> way.
>
> Ahh, no you do not have to bug i18n folks. 47fbfded53 ("i18n: only
> extract comments marked with "TRANSLATORS:"", 2014-04-17) is an
> example of such an adjustment.
Urgh. Thanks for pointing this out to me, though. Will be fixed in the
next iteration.
> > + while ((commit = get_revision(opts->revs))) {
> > + struct todo_item *item = append_new_todo(todo_list);
> > + const char *commit_buffer = get_commit_buffer(commit, NULL);
> > + const char *subject;
> > + int subject_len;
> > +
> > + item->command = command;
> > + item->commit = commit;
> > + item->offset_in_buf = todo_list->buf.len;
> > + subject_len = find_commit_subject(commit_buffer, &subject);
> > + strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
> > + find_unique_abbrev(commit->object.oid.hash,
> > + DEFAULT_ABBREV),
> > + subject_len, subject);
>
> I am personally fine with this line; two things come to mind:
>
> - This would work just fine as-is with Linus's change to turn
> DEFAULT_ABBREV to -1.
>
> - It appears that it is more fashionable to use
> strbuf_add_unique_abbrev() these days.
Right, I actually looked at this place when I tried to decide where I
could use that function. Somehow I thought I'd not break up the flow here.
But since you asked so nicely, I'll squash this in (I personally find it
uglier, and longer, but it does use strbuf_add_unique_abbrev() now):
-- snipsnap --
@@ -906,11 +904,13 @@ static int walk_revs_populate_todo(struct todo_list
*todo_list,
item->command = command;
item->commit = commit;
item->offset_in_buf = todo_list->buf.len;
+ strbuf_addstr(&todo_list->buf, command);
+ strbuf_addch(&todo_list->buf, ' ');
+ strbuf_add_unique_abbrev(&todo_list->buf,
+ commit->object.oid.hash,
+ DEFAULT_ABBREV);
subject_len = find_commit_subject(commit_buffer, &subject);
- strbuf_addf(&todo_list->buf, "%s %s %.*s\n",
command_string,
- find_unique_abbrev(commit->object.oid.hash,
- DEFAULT_ABBREV),
- subject_len, subject);
+ strbuf_add(&todo_list->buf, subject, subject_len);
unuse_commit_buffer(commit, commit_buffer);
}
return 0;
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox