threads / patch / 42563

patch, 5 partsA bit more "attributes"

Subject: [PATCH 0/5] A bit more "attributes"

## tl;dr

13 messages between Jun 8, 2016 and Jun 16, 2016. Diffs are folded; open one to read it.

replies: 12people: 2as markdown or json

Junio C Hamano· Jun 8, 2016, 22:58 UTC · lore

The whole jc/attr topic is now depended on Stefan's "pathspec label" topic, both of which are in 'next' and cannot be rewound until the next cycle. These five patches are to continue my recent "attributes" work while the tree is in frozen state.

A few central data structures, namely "attr_stack" that keeps track of the attributes read from the .gitattributes files in the directory hierarchy of interest and "check_all_attr[]" that keeps track of the state of the attributes for the path being checked, are implemented as file-scope global singletons that belong to the attribute subsystem ("attr.c"). The envisioned endgame is to make them into per "struct git_attr_check" instance, so that the entire system can have multiple "directory hierarchy of interest" and "path being checked", i.e. allow multiple threads to keep their own "git_attr_check" and to use the attribute system simultaneously.

To further that plan, the first two patches pass the check[] structure further down the callchain, and the last two patches remove a hack that prevented check[] from being passed in one corner case and always pass it down the callchain.

Junio C Hamano (5):
  attr.c: add push_stack() helper
  attr.c: pass struct git_attr_check down the callchain
  fixup! d5ad6c13
  attr.c: correct ugly hack for git_all_attrs()
  attr.c: always pass check[] to collect_some_attrs()
 attr.c | 143 +++++++++++++++++++++++++++++++++--------------------------------
 attr.h |   2 +-
 2 files changed, 74 insertions(+), 71 deletions(-)
-- 
2.9.0-rc2-262-g9161bbf
Junio C Hamano· Jun 8, 2016, 22:58 UTC · re: Junio C Hamano · lore

[PATCH 1/5] attr.c: add push_stack() helper

There are too many repetitious "I have this new attr_stack element; push it at the top of the stack" sequence. The new helper function push_stack() gives us a way to express what is going on at these places, and as a side effect, halves the number of times we mention the attr_stack global variable.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 attr.c | 71 +++++++++++++++++++++++++++++++-----------------------------------
 1 file changed, 33 insertions(+), 38 deletions(-)
Show changes to attr.c +33 −38
diff --git a/attr.c b/attr.c
index fc63712..26228ce 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.9.0-rc2-262-g9161bbf
Stefan Beller· Jun 8, 2016, 23:43 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/5] attr.c: add push_stack() helper

On Wed, Jun 8, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> +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;

Why do we need to be conditional on origin for setting the originlen, in all occurrences below, we pass in a reasonable number (0), so I would leave out the condition here.

We make use of the `originlen` in `fill` only, so maybe we can even get rid of the length and when we need it compute it with strlen? (I am unsure on this tradeoff)

Show 8 quoted lines
>
>         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);

I wonder why we need to pass an empty-but-not-null string here? In `fill` we use

  const char *base = stk->origin ? stk->origin : "";
so there it would be the same. In prepare_stack we have
       /*
        * Pop the ones from directories that are not the prefix of
        * the path we are checking. Break out of the loop when we see
        * the root one (whose origin is an empty string "") or the builtin
        * one (whose origin is NULL) without popping it.
        */
        while (attr_stack->origin) {

which seems to answer my question that we make a difference between empty and null `origin`. However I wonder if that could be made more clear? (By a well named bit flag in the attr_stack?)

Show 5 quoted lines
> -                       strbuf_add(&pathbuf, path, cp - path);
> -                       strbuf_addch(&pathbuf, '/');
> -                       strbuf_addstr(&pathbuf, GITATTRIBUTES_FILE);
> +                       strbuf_addf(&pathbuf,
> +                                   "%.*s/%s", (int)(cp - path), path,

This is neat way of handling strings that are not null terminated! I have the suspicion I could have used this before.

As this is meant as a refactoring patch, which doesn't want to change semantics, it looks good to me, the questions are rather meant for followups.

Thanks, Stefan

Junio C Hamano· Jun 9, 2016, 00:15 UTC · re: Stefan Beller · lore

Re: [PATCH 1/5] attr.c: add push_stack() helper

Stefan Beller <sbeller@google.com> writes:

All of your questions is about !origin vs !*origin. This applies to the code in 'master', not specific to jc/attr topic's improvements. It is how the prepare_attr_stack() tells where the .gitattributes for the top-level directory is when popping the elements for other directory hierarchy we have left (origin && !*origin is the root level). !origin denotes entries that did not come from in-tree files (i.e. $GIT_DIR/info/attributes, built-in ones, etc.).

Junio C Hamano· Jun 8, 2016, 22:58 UTC · re: Junio C Hamano · lore

[PATCH 2/5] attr.c: pass struct git_attr_check down the callchain

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>
---
 attr.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)
Show changes to attr.c +24 −12
diff --git a/attr.c b/attr.c
index 26228ce..4e2172a 100644
--- a/attr.c
+++ b/attr.c
@@ -746,14 +746,25 @@ static int macroexpand_one(int nr, int rem)
  * check_all_attr. If num is non-zero, only attributes in check[] are
  * collected. Otherwise all attributes are collected.
  */
-static void collect_some_attrs(const char *path, int 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])
@@ -773,9 +784,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++;
 			}
@@ -789,18 +800,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;
@@ -811,7 +823,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;
@@ -840,7 +852,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.9.0-rc2-262-g9161bbf
Stefan Beller· Jun 8, 2016, 23:54 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/5] attr.c: pass struct git_attr_check down the callchain

On Wed, Jun 8, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 66 quoted lines
> 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>
> ---
>  attr.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index 26228ce..4e2172a 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -746,14 +746,25 @@ static int macroexpand_one(int nr, int rem)
>   * check_all_attr. If num is non-zero, only attributes in check[] are
>   * collected. Otherwise all attributes are collected.
>   */
> -static void collect_some_attrs(const char *path, int 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])
> @@ -773,9 +784,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++;
>                         }
> @@ -789,18 +800,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;

We don't need this outside the for loop; maybe we want to move this in there including indexing?

Or rather dropping the arrays and use pointers in there if we're concerned about performance? (That would be a slightly larger refactoring, roughly):

    struct git_attr_check_elem *cur = check->check;
    struct git_attr_check_elem *end = cur + check->check_nr;
    while (cur < end) {
        value = check_all_attr[cur->attr->attr_nr].value;
        cur->value = (value == ATTR__UNKNOWN) ? ATTR__UNSET : value;
        cur++;
    }
Looks correct as it is, though.

Thanks, Stefan

Junio C Hamano· Jun 8, 2016, 22:58 UTC · re: Junio C Hamano · lore

[PATCH 3/5] fixup! d5ad6c13

As the topic is in 'next' already, I'll leave this floating near the
tip for now, until we can rewind the topic after the next release.
---
 attr.c | 9 ++++++---
 attr.h | 2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)
Show changes to 2 files +7 −4

attr.c, attr.h

diff --git a/attr.c b/attr.c
index 4e2172a..0e61950 100644
--- a/attr.c
+++ b/attr.c
@@ -899,13 +899,16 @@ struct git_attr_check *git_attr_check_alloc(void)
 	return xcalloc(1, sizeof(struct git_attr_check));
 }
 
-void git_attr_check_append(struct git_attr_check *check,
-			   const struct git_attr *attr)
+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");
 	ALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);
-	check->check[check->check_nr++].attr = attr;
+	elem = &check->check[check->check_nr++];
+	elem->attr = attr;
+	return elem;
 }
 
 void git_attr_check_clear(struct git_attr_check *check)
diff --git a/attr.h b/attr.h
index fc72030..40abc16 100644
--- a/attr.h
+++ b/attr.h
@@ -47,7 +47,7 @@ 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 void git_attr_check_append(struct git_attr_check *, const struct git_attr *);
+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 *);
-- 
2.9.0-rc2-262-g9161bbf
Stefan Beller· Jun 9, 2016, 00:00 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/5] fixup! d5ad6c13

On Wed, Jun 8, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> As the topic is in 'next' already, I'll leave this floating near the
> tip for now, until we can rewind the topic after the next release.
I don't quite understand the motivation behind this commit.
    We return the last element to allow succeeding operations
    access to the bottom of the stack without needing to walk it?
    This makes the follow up operations faster, because we expect the stack
    to be larger than 5 elements.
?
Show 49 quoted lines
> ---
>  attr.c | 9 ++++++---
>  attr.h | 2 +-
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index 4e2172a..0e61950 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -899,13 +899,16 @@ struct git_attr_check *git_attr_check_alloc(void)
>         return xcalloc(1, sizeof(struct git_attr_check));
>  }
>
> -void git_attr_check_append(struct git_attr_check *check,
> -                          const struct git_attr *attr)
> +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");
>         ALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);
> -       check->check[check->check_nr++].attr = attr;
> +       elem = &check->check[check->check_nr++];
> +       elem->attr = attr;
> +       return elem;
>  }
>
>  void git_attr_check_clear(struct git_attr_check *check)
> diff --git a/attr.h b/attr.h
> index fc72030..40abc16 100644
> --- a/attr.h
> +++ b/attr.h
> @@ -47,7 +47,7 @@ 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 void git_attr_check_append(struct git_attr_check *, const struct git_attr *);
> +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 *);
> --
> 2.9.0-rc2-262-g9161bbf
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Junio C Hamano· Jun 9, 2016, 00:10 UTC · re: Stefan Beller · lore

Re: [PATCH 3/5] fixup! d5ad6c13

Stefan Beller <sbeller@google.com> writes:
Show 12 quoted lines
> On Wed, Jun 8, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> As the topic is in 'next' already, I'll leave this floating near the
>> tip for now, until we can rewind the topic after the next release.
>
> I don't quite understand the motivation behind this commit.
>
>     We return the last element to allow succeeding operations
>     access to the bottom of the stack without needing to walk it?
>     This makes the follow up operations faster, because we expect the stack
>     to be larger than 5 elements.
>
> ?

Think how you call _append() that returns void and then set the value to the last appended item. YOu'd need to do something unwieldy like

	git_attr_check_append(check, attr);
	check->check[check->check_nr-1].value = value
Junio C Hamano· Jun 8, 2016, 22:58 UTC · re: Junio C Hamano · lore

[PATCH 4/5] attr.c: correct ugly hack for git_all_attrs()

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>
---
 attr.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)
Show changes to attr.c +8 −16
diff --git a/attr.c b/attr.c
index 0e61950..cdf064e 100644
--- a/attr.c
+++ b/attr.c
@@ -743,8 +743,8 @@ static int macroexpand_one(int nr, int rem)
 
 /*
  * Collect attributes for path into the array pointed to by
- * check_all_attr. If num is non-zero, only attributes in check[] are
- * collected. Otherwise all attributes are collected.
+ * 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)
@@ -754,17 +754,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])
@@ -781,9 +770,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;
@@ -791,7 +783,7 @@ static void collect_some_attrs(const char *path, int pathlen,
 				rem++;
 			}
 		}
-		if (rem == num)
+		if (rem == check->check_nr)
 			return;
 	}
 
-- 
2.9.0-rc2-262-g9161bbf
Junio C Hamano· Jun 8, 2016, 22:58 UTC · re: Junio C Hamano · lore

[PATCH 5/5] attr.c: always pass check[] to collect_some_attrs()

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>
---
 attr.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)
Show changes to attr.c +19 −18
diff --git a/attr.c b/attr.c
index cdf064e..95d2f77 100644
--- a/attr.c
+++ b/attr.c
@@ -743,11 +743,12 @@ static int macroexpand_one(int nr, int rem)
 
 /*
  * 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;
@@ -768,10 +769,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;
@@ -790,6 +792,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) {
+		git_attr_check_clear(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,
@@ -798,7 +811,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;
@@ -812,19 +825,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.9.0-rc2-262-g9161bbf
Stefan Beller· Jun 9, 2016, 00:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 5/5] attr.c: always pass check[] to collect_some_attrs()

On Wed, Jun 8, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 32 quoted lines
> 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>
> ---
>  attr.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index cdf064e..95d2f77 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -743,11 +743,12 @@ static int macroexpand_one(int nr, int rem)
>
>  /*
>   * 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)

I think it may be better to have a collect_all_attrs instead of a flag here, as more than half the executed code differs (the parts conditioned on collect_all are rather large in the function)

Show 8 quoted lines
>
>  {
>         struct attr_stack *stk;
> @@ -768,10 +769,11 @@ static void collect_some_attrs(const char *path, int pathlen,
>         }
>
>         prepare_attr_stack(path, dirlen);
> +
stray empty line?
Junio C Hamano· Jun 9, 2016, 18:15 UTC · re: Stefan Beller · lore

Re: [PATCH 5/5] attr.c: always pass check[] to collect_some_attrs()

Stefan Beller <sbeller@google.com> writes:
Show 7 quoted lines
>>  static void collect_some_attrs(const char *path, int pathlen,
>> -                              struct git_attr_check *check)
>> +                              struct git_attr_check *check, int collect_all)
>
> I think it may be better to have a collect_all_attrs instead of a flag here,
> as more than half the executed code differs (the parts conditioned on
> collect_all are rather large in the function)

It is understandable if it appears to anybody who does not know the future that way ;-)

The plan is to remove that "if (!collect_all)" optimization block. Once the attr_stack and check_all_attr becomes per git_attr_check instance, there is no reason to keep record of all attribute definitions read from the .gitattribute files. The entries that are known not to matter (i.e. not listed in git_attr_check) can be discarded when they are read before it hits attr_stack.

The "optimization" in that block is to optimize one special case: an attribute may be defined in the git_attr_hash[] dictionary, and the ones that appeared in various .gitattribute files are marked with a bit (an attribute may be in that dictionary only because the caller who asks about the attribute filled it in check->check[] array, and may not appear in the .gitattribute files prepare_attr_stack() read). If the check->check[] wants to know about an attribute that does not appear in any of the .gitattribute files, we can answer "that attribute is not set to anything" without actually executing any matches with path patterns.

The "an attr_stack that is per git_attr_check only has to contain what matters and can discard irrelevant records" is a natural extension of the current optimization. Instead of the current "when 0 rules refer to attribute, do not do any of the 47 path matches defined in the attribute system, but even if one rule refers to it, do all 47 path matches to check", we would do "among 47 path patterns, only attempt to match patterns that affect the attributes that we are checking (which could be 0 rules).

← back to recent threads