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

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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(-)

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

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

[Stefan Beller]

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


>
>         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?)

> -                       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

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

[Junio C Hamano]

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.).

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

[Junio C Hamano]

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;
 
-	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

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

[Stefan Beller]

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

[PATCH 3/5] fixup! d5ad6c13

[Junio C Hamano]

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(-)

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

Re: [PATCH 3/5] fixup! d5ad6c13

[Stefan Beller]

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.

?


> ---
>  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

Re: [PATCH 3/5] fixup! d5ad6c13

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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

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

[Junio C Hamano]

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(-)

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

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

[Junio C Hamano]

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)
 
 {
 	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

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

[Stefan Beller]

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

>
>  {
>         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?

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>>  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).