From 7b95849be42406b2e23bcdc4592d71d8724361ba Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Fri, 18 Jan 2019 16:34:58 -0500
Subject: attr: do not mark queried macros as unset

Since 60a12722ac (attr: remove maybe-real, maybe-macro from git_attr,
2017-01-27), we will always mark an attribute macro (e.g., "binary")
that is specifically queried for as "unspecified", even though listing
_all_ attributes would display it at set. E.g.:

  $ echo "* binary" >.gitattributes

  $ git check-attr -a file
  file: binary: set
  file: diff: unset
  file: merge: unset
  file: text: unset

  $ git check-attr binary file
  file: binary: unspecified

The problem stems from an incorrect conversion of the optimization from
06a604e670 (attr: avoid heavy work when we know the specified attr is
not defined, 2014-12-28). There we tried in collect_some_attrs() to
avoid even looking at the attr_stack when the user has asked for "foo"
and we know that "foo" did not ever appear in any .gitattributes file.

It used a flag "maybe_real" in each attribute struct, where "real" meant
that the attribute appeared in an actual file (we have to make this
distinction because we also create an attribute struct for any names
that are being queried). But as explained in that commit message, the
meaning of "real" was tangled with some special cases around macros.

When 60a12722ac later refactored the macro code, it dropped maybe_real
entirely. This missed the fact that "maybe_real" could be unset for two
reasons: because of a macro, or because it was never found during
parsing. This had two results:

  - the optimization in collect_some_attrs() ceased doing anything
    meaningful, since it no longer kept track of "was it found during
    parsing"

  - worse, it actually kicked in when the caller _did_ ask about a macro
    by name, causing us to mark it as unspecified

It should be possible to salvage this optimization, but let's start with
just removing the remnants. It hasn't been doing anything (except
creating bugs) since 60a12722ac, and nobody seems to have noticed the
performance regression. It's more important to fix the correctness
problem clearly first.

I've added two tests here. The second one actually shows off the bug.
The test of "check-attr -a" is not strictly necessary, but we currently
do not test attribute macros much, and the builtin "binary" not at all.
So this increases our general test coverage, as well as making sure we
didn't mess up this related case.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/attr.c b/attr.c
index 60d2847..2937552 100644
--- a/attr.c
+++ b/attr.c
@@ -1104,7 +1104,7 @@ static void collect_some_attrs(const struct index_state *istate,
 			       const char *path,
 			       struct attr_check *check)
 {
-	int i, pathlen, rem, dirlen;
+	int pathlen, rem, dirlen;
 	const char *cp, *last_slash = NULL;
 	int basename_offset;
 
@@ -1125,20 +1125,6 @@ static void collect_some_attrs(const struct index_state *istate,
 	all_attrs_init(&g_attr_hashmap, check);
 	determine_macros(check->all_attrs, check->stack);
 
-	if (check->nr) {
-		rem = 0;
-		for (i = 0; i < check->nr; i++) {
-			int n = check->items[i].attr->attr_nr;
-			struct all_attrs_item *item = &check->all_attrs[n];
-			if (item->macro) {
-				item->value = ATTR__UNSET;
-				rem++;
-			}
-		}
-		if (rem == check->nr)
-			return;
-	}
-
 	rem = check->all_attrs_nr;
 	fill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);
 }
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 22499bc..71e63d8 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -322,4 +322,24 @@ test_expect_success 'bare repository: test info/attributes' '
 	)
 '
 
+test_expect_success 'binary macro expanded by -a' '
+	echo "file binary" >.gitattributes &&
+	cat >expect <<-\EOF &&
+	file: binary: set
+	file: diff: unset
+	file: merge: unset
+	file: text: unset
+	EOF
+	git check-attr -a file >actual &&
+	test_cmp expect actual
+'
+
+
+test_expect_success 'query binary macro directly' '
+	echo "file binary" >.gitattributes &&
+	echo file: binary: set >expect &&
+	git check-attr binary file >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
cgit v0.10.2-6-g49f6