Commit e3a2a04ceff1d3657629fd6a7245d9a9fc53f24b

Russell Belfer 2014-04-18T14:29:58

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.

diff --git a/src/attr.c b/src/attr.c
index 6228743..6b9a3d6 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 65bbf78..8a8d86a 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 71dc7a5..9aab7ed 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(