Preload attribute files that may contain macros

There was a latent bug where files that use macro definitions
could be parsed before the macro definitions were loaded.  Because
of attribute file caching, preloading files that are going to be
used doesn't add a significant amount of overhead, so let's always
preload any files that could contain macros before we assemble the
actual vector of files to scan for attributes.

3 files changed, 136 insertions, 45 deletions

diff --git a/src/attr.c b/src/attr.c
index 622874348..6b9a3d614 100644
--- a/src/attr.c
+++ b/src/attr.c
@@ -217,6 +217,74 @@ cleanup:
 	return error;
 }
 
+static int preload_attr_file(
+	git_repository *repo,
+	git_attr_file_source source,
+	const char *base,
+	const char *file)
+{
+	int error;
+	git_attr_file *preload = NULL;
+
+	if (!file)
+		return 0;
+	if (!(error = git_attr_cache__get(
+			&preload, repo, source, base, file, git_attr_file__parse_buffer)))
+		git_attr_file__free(preload);
+
+	return error;
+}
+
+static int attr_setup(git_repository *repo)
+{
+	int error = 0;
+	const char *workdir = git_repository_workdir(repo);
+	git_index *idx = NULL;
+	git_buf sys = GIT_BUF_INIT;
+
+	if ((error = git_attr_cache__init(repo)) < 0)
+		return error;
+
+	/* preload attribute files that could contain macros so the
+	 * definitions will be available for later file parsing
+	 */
+
+	if (!(error = git_sysdir_find_system_file(&sys, GIT_ATTR_FILE_SYSTEM))) {
+		error = preload_attr_file(
+			repo, GIT_ATTR_FILE__FROM_FILE, NULL, sys.ptr);
+		git_buf_free(&sys);
+	}
+	if (error < 0) {
+		if (error == GIT_ENOTFOUND) {
+			giterr_clear();
+			error = 0;
+		} else
+			return error;
+	}
+
+	if ((error = preload_attr_file(
+			repo, GIT_ATTR_FILE__FROM_FILE,
+			NULL, git_repository_attr_cache(repo)->cfg_attr_file)) < 0)
+		return error;
+
+	if ((error = preload_attr_file(
+			repo, GIT_ATTR_FILE__FROM_FILE,
+			git_repository_path(repo), GIT_ATTR_FILE_INREPO)) < 0)
+		return error;
+
+	if (workdir != NULL &&
+		(error = preload_attr_file(
+			repo, GIT_ATTR_FILE__FROM_FILE, workdir, GIT_ATTR_FILE)) < 0)
+		return error;
+
+	if ((error = git_repository_index__weakptr(&idx, repo)) < 0 ||
+		(error = preload_attr_file(
+			repo, GIT_ATTR_FILE__FROM_INDEX, NULL, GIT_ATTR_FILE)) < 0)
+		return error;
+
+	return error;
+}
+
 int git_attr_add_macro(
 	git_repository *repo,
 	const char *name,
@@ -226,8 +294,8 @@ int git_attr_add_macro(
 	git_attr_rule *macro = NULL;
 	git_pool *pool;
 
-	if (git_attr_cache__init(repo) < 0)
-		return -1;
+	if ((error = attr_setup(repo)) < 0)
+		return error;
 
 	macro = git__calloc(1, sizeof(git_attr_rule));
 	GITERR_CHECK_ALLOC(macro);
@@ -348,7 +416,7 @@ static int collect_attr_files(
 	const char *workdir = git_repository_workdir(repo);
 	attr_walk_up_info info = { NULL };
 
-	if ((error = git_attr_cache__init(repo)) < 0)
+	if ((error = attr_setup(repo)) < 0)
 		return error;
 
 	/* Resolve path in a non-bare repo */
diff --git a/src/attr_file.c b/src/attr_file.c
index 65bbf78e8..8a8d86a2d 100644
--- a/src/attr_file.c
+++ b/src/attr_file.c
@@ -248,9 +248,7 @@ int git_attr_file__parse_buffer(
 				repo, &attrs->pool, &rule->assigns, &scan)))
 		{
 			if (rule->match.flags & GIT_ATTR_FNMATCH_MACRO)
-				/* should generate error/warning if this is coming from any
-				 * file other than .gitattributes at repo root.
-				 */
+				/* TODO: warning if macro found in file below repo root */
 				error = git_attr_cache__insert_macro(repo, rule);
 			else
 				error = git_vector_insert(&attrs->rules, rule);
diff --git a/tests/attr/repo.c b/tests/attr/repo.c
index 71dc7a5b5..9aab7ed96 100644
--- a/tests/attr/repo.c
+++ b/tests/attr/repo.c
@@ -23,49 +23,74 @@ void test_attr_repo__cleanup(void)
 	g_repo = NULL;
 }
 
+static struct attr_expected get_one_test_cases[] = {
+	{ "root_test1", "repoattr", EXPECT_TRUE, NULL },
+	{ "root_test1", "rootattr", EXPECT_TRUE, NULL },
+	{ "root_test1", "missingattr", EXPECT_UNDEFINED, NULL },
+	{ "root_test1", "subattr", EXPECT_UNDEFINED, NULL },
+	{ "root_test1", "negattr", EXPECT_UNDEFINED, NULL },
+	{ "root_test2", "repoattr", EXPECT_TRUE, NULL },
+	{ "root_test2", "rootattr", EXPECT_FALSE, NULL },
+	{ "root_test2", "missingattr", EXPECT_UNDEFINED, NULL },
+	{ "root_test2", "multiattr", EXPECT_FALSE, NULL },
+	{ "root_test3", "repoattr", EXPECT_TRUE, NULL },
+	{ "root_test3", "rootattr", EXPECT_UNDEFINED, NULL },
+	{ "root_test3", "multiattr", EXPECT_STRING, "3" },
+	{ "root_test3", "multi2", EXPECT_UNDEFINED, NULL },
+	{ "sub/subdir_test1", "repoattr", EXPECT_TRUE, NULL },
+	{ "sub/subdir_test1", "rootattr", EXPECT_TRUE, NULL },
+	{ "sub/subdir_test1", "missingattr", EXPECT_UNDEFINED, NULL },
+	{ "sub/subdir_test1", "subattr", EXPECT_STRING, "yes" },
+	{ "sub/subdir_test1", "negattr", EXPECT_FALSE, NULL },
+	{ "sub/subdir_test1", "another", EXPECT_UNDEFINED, NULL },
+	{ "sub/subdir_test2.txt", "repoattr", EXPECT_TRUE, NULL },
+	{ "sub/subdir_test2.txt", "rootattr", EXPECT_TRUE, NULL },
+	{ "sub/subdir_test2.txt", "missingattr", EXPECT_UNDEFINED, NULL },
+	{ "sub/subdir_test2.txt", "subattr", EXPECT_STRING, "yes" },
+	{ "sub/subdir_test2.txt", "negattr", EXPECT_FALSE, NULL },
+	{ "sub/subdir_test2.txt", "another", EXPECT_STRING, "zero" },
+	{ "sub/subdir_test2.txt", "reposub", EXPECT_TRUE, NULL },
+	{ "sub/sub/subdir.txt", "another", EXPECT_STRING, "one" },
+	{ "sub/sub/subdir.txt", "reposubsub", EXPECT_TRUE, NULL },
+	{ "sub/sub/subdir.txt", "reposub", EXPECT_UNDEFINED, NULL },
+	{ "does-not-exist", "foo", EXPECT_STRING, "yes" },
+	{ "sub/deep/file", "deepdeep", EXPECT_TRUE, NULL },
+	{ "sub/sub/d/no", "test", EXPECT_STRING, "a/b/d/*" },
+	{ "sub/sub/d/yes", "test", EXPECT_UNDEFINED, NULL },
+};
+
 void test_attr_repo__get_one(void)
 {
-	struct attr_expected test_cases[] = {
-		{ "root_test1", "repoattr", EXPECT_TRUE, NULL },
-		{ "root_test1", "rootattr", EXPECT_TRUE, NULL },
-		{ "root_test1", "missingattr", EXPECT_UNDEFINED, NULL },
-		{ "root_test1", "subattr", EXPECT_UNDEFINED, NULL },
-		{ "root_test1", "negattr", EXPECT_UNDEFINED, NULL },
-		{ "root_test2", "repoattr", EXPECT_TRUE, NULL },
-		{ "root_test2", "rootattr", EXPECT_FALSE, NULL },
-		{ "root_test2", "missingattr", EXPECT_UNDEFINED, NULL },
-		{ "root_test2", "multiattr", EXPECT_FALSE, NULL },
-		{ "root_test3", "repoattr", EXPECT_TRUE, NULL },
-		{ "root_test3", "rootattr", EXPECT_UNDEFINED, NULL },
-		{ "root_test3", "multiattr", EXPECT_STRING, "3" },
-		{ "root_test3", "multi2", EXPECT_UNDEFINED, NULL },
-		{ "sub/subdir_test1", "repoattr", EXPECT_TRUE, NULL },
-		{ "sub/subdir_test1", "rootattr", EXPECT_TRUE, NULL },
-		{ "sub/subdir_test1", "missingattr", EXPECT_UNDEFINED, NULL },
-		{ "sub/subdir_test1", "subattr", EXPECT_STRING, "yes" },
-		{ "sub/subdir_test1", "negattr", EXPECT_FALSE, NULL },
-		{ "sub/subdir_test1", "another", EXPECT_UNDEFINED, NULL },
-		{ "sub/subdir_test2.txt", "repoattr", EXPECT_TRUE, NULL },
-		{ "sub/subdir_test2.txt", "rootattr", EXPECT_TRUE, NULL },
-		{ "sub/subdir_test2.txt", "missingattr", EXPECT_UNDEFINED, NULL },
-		{ "sub/subdir_test2.txt", "subattr", EXPECT_STRING, "yes" },
-		{ "sub/subdir_test2.txt", "negattr", EXPECT_FALSE, NULL },
-		{ "sub/subdir_test2.txt", "another", EXPECT_STRING, "zero" },
-		{ "sub/subdir_test2.txt", "reposub", EXPECT_TRUE, NULL },
-		{ "sub/sub/subdir.txt", "another", EXPECT_STRING, "one" },
-		{ "sub/sub/subdir.txt", "reposubsub", EXPECT_TRUE, NULL },
-		{ "sub/sub/subdir.txt", "reposub", EXPECT_UNDEFINED, NULL },
-		{ "does-not-exist", "foo", EXPECT_STRING, "yes" },
-		{ "sub/deep/file", "deepdeep", EXPECT_TRUE, NULL },
-		{ "sub/sub/d/no", "test", EXPECT_STRING, "a/b/d/*" },
-		{ "sub/sub/d/yes", "test", EXPECT_UNDEFINED, NULL },
-		{ NULL, NULL, 0, NULL }
-	}, *scan;
-
-	for (scan = test_cases; scan->path != NULL; scan++) {
+	int i;
+
+	for (i = 0; i < (int)ARRAY_SIZE(get_one_test_cases); ++i) {
+		struct attr_expected *scan = &get_one_test_cases[i];
+		const char *value;
+
+		cl_git_pass(git_attr_get(&value, g_repo, 0, scan->path, scan->attr));
+		attr_check_expected(
+			scan->expected, scan->expected_str, scan->attr, value);
+	}
+
+	cl_assert(git_attr_cache__is_cached(
+		g_repo, GIT_ATTR_FILE__FROM_FILE, ".git/info/attributes"));
+	cl_assert(git_attr_cache__is_cached(
+		g_repo, GIT_ATTR_FILE__FROM_FILE, ".gitattributes"));
+	cl_assert(git_attr_cache__is_cached(
+		g_repo, GIT_ATTR_FILE__FROM_FILE, "sub/.gitattributes"));
+}
+
+void test_attr_repo__get_one_start_deep(void)
+{
+	int i;
+
+	for (i = (int)ARRAY_SIZE(get_one_test_cases) - 1; i >= 0; --i) {
+		struct attr_expected *scan = &get_one_test_cases[i];
 		const char *value;
+
 		cl_git_pass(git_attr_get(&value, g_repo, 0, scan->path, scan->attr));
-		attr_check_expected(scan->expected, scan->expected_str, scan->attr, value);
+		attr_check_expected(
+			scan->expected, scan->expected_str, scan->attr, value);
 	}
 
 	cl_assert(git_attr_cache__is_cached(