Re: [PATCH/RFC 4/4] attr: avoid heavy work when we know the specified attr is not defined

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

+static void collect_selected_attrs(const char *path, int num,
+				   struct git_attr_check *check)
+{
+	struct attr_stack *stk;
+	int i, pathlen, rem, dirlen;
+	int basename_offset;
+
+	pathlen = split_path(path, &dirlen, &basename_offset);
+	prepare_attr_stack(path, dirlen);
+	if (cannot_trust_maybe_real) {
+		for (i = 0; i < git_attr_nr; i++)
+			check_all_attr[i].value = ATTR__UNKNOWN;

Judging from the fact that

 (1) the only caller calls this function in this fashion based on the
     setting of "cannot-trust" bit,

 (2) this and the other function the only caller calls share the
     same code in their beginning part, and

 (3) the body of the if() statement here duplicates the code from
     collect_all_attrs(),

I smell that a much better split is possible.

Why isn't this all inside a single function collect_all_attrs()?
That single function may no longer be collect_ALL_attrs, so renaming
it to collect_attrs() is fine, but then that function may have this
if () to initialize all of them to ATTR__UNKNOWN or do the else part
we see below, and when organized that way we do not need to have
duplicated code (or split_path() helper function), no?

+	} else {
+		rem = num;
+		for (i = 0; i < num; i++) {
+			struct git_attr_check *c;
+			c = check_all_attr + check[i].attr->attr_nr;
+			if (check[i].attr->maybe_real)
+				c->value = ATTR__UNKNOWN;
+			else {
+				c->value = ATTR__UNSET;
+				rem--;
+			}
+		}
+		if (!rem)
+			return;
+	}
+	rem = git_attr_nr;
+	for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
+		rem = fill(path, pathlen, basename_offset, stk, rem);
+}
+
 int git_check_attr(const char *path, int num, struct git_attr_check *check)
 {
 	int i;
 
-	collect_all_attrs(path);
+	if (cannot_trust_maybe_real)
+		collect_all_attrs(path);
+	else
+		collect_selected_attrs(path, num, check);
 
 	for (i = 0; i < num; i++) {
 		const char *value = check_all_attr[check[i].attr->attr_nr].value;

On Tue, Dec 09, 2014 at 04:18:57PM -0800, Junio C Hamano wrote:

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

+static void collect_selected_attrs(const char *path, int num,
+				   struct git_attr_check *check)
+{
+	struct attr_stack *stk;
+	int i, pathlen, rem, dirlen;
+	int basename_offset;
+
+	pathlen = split_path(path, &dirlen, &basename_offset);
+	prepare_attr_stack(path, dirlen);
+	if (cannot_trust_maybe_real) {
+		for (i = 0; i < git_attr_nr; i++)
+			check_all_attr[i].value = ATTR__UNKNOWN;

Judging from the fact that

 (1) the only caller calls this function in this fashion based on the
     setting of "cannot-trust" bit,

 (2) this and the other function the only caller calls share the
     same code in their beginning part, and

 (3) the body of the if() statement here duplicates the code from
     collect_all_attrs(),

I smell that a much better split is possible.

Why isn't this all inside a single function collect_all_attrs()?
That single function may no longer be collect_ALL_attrs, so renaming
it to collect_attrs() is fine, but then that function may have this
if () to initialize all of them to ATTR__UNKNOWN or do the else part
we see below, and when organized that way we do not need to have
duplicated code (or split_path() helper function), no?

Something like this? Definitely looks better.

-- 8< --

diff --git a/attr.c b/attr.c
index b80e52b..0f828e3 100644
--- a/attr.c
+++ b/attr.c

@@ -33,9 +33,11 @@ struct git_attr {
 	unsigned h;
 	int attr_nr;
 	int maybe_macro;
+	int maybe_real;
 	char name[FLEX_ARRAY];
 };
 static int attr_nr;
+static int cannot_trust_maybe_real;
 
 static struct git_attr_check *check_all_attr;
 static struct git_attr *(git_attr_hash[HASHSIZE]);

@@ -97,6 +99,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)
 	a->next = git_attr_hash[pos];
 	a->attr_nr = attr_nr++;
 	a->maybe_macro = 0;
+	a->maybe_real = 0;
 	git_attr_hash[pos] = a;
 
 	REALLOC_ARRAY(check_all_attr, attr_nr);

@@ -269,6 +272,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 	/* Second pass to fill the attr_states */
 	for (cp = states, i = 0; *cp; i++) {
 		cp = parse_attr(src, lineno, cp, &(res->state[i]));
+		if (!is_macro)
+			res->state[i].attr->maybe_real = 1;
+		if (res->state[i].attr->maybe_macro)
+			cannot_trust_maybe_real = 1;
 	}
 
 	return res;

@@ -713,7 +720,9 @@ static int macroexpand_one(int nr, int rem)
  * Collect all attributes for path into the array pointed to by
  * check_all_attr.
  */
-static void collect_all_attrs(const char *path)
+static void collect_some_attrs(const char *path, int num,
+			       struct git_attr_check *check)
+
 {
 	struct attr_stack *stk;
 	int i, pathlen, rem, dirlen;

@@ -736,6 +745,19 @@ static void collect_all_attrs(const char *path)
 	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) {
+		rem = 0;
+		for (i = 0; i < num; i++) {
+			if (!check[i].attr->maybe_real) {
+				struct git_attr_check *c;
+				c = check_all_attr + check[i].attr->attr_nr;
+				c->value = ATTR__UNSET;
+				rem++;
+			}
+		}
+		if (rem == num)
+			return;
+	}
 
 	rem = attr_nr;
 	for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)

@@ -746,7 +768,7 @@ int git_check_attr(const char *path, int num, struct git_attr_check *check)
 {
 	int i;
 
-	collect_all_attrs(path);
+	collect_some_attrs(path, num, check);
 
 	for (i = 0; i < num; i++) {
 		const char *value = check_all_attr[check[i].attr->attr_nr].value;

@@ -762,7 +784,7 @@ int git_all_attrs(const char *path, int *num, struct git_attr_check **check)
 {
 	int i, count, j;
 
-	collect_all_attrs(path);
+	collect_some_attrs(path, 0, NULL);
 
 	/* Count the number of attributes that are set. */
 	count = 0;
-- 8< --