attr_file: ignore macros defined in subdirectories

pks-t · pks-t · commit f83469059b73 · 2019-07-12T09:26:22.000+02:00
Right now, we are unconditionally applying all macros found in a
gitatttributes file. But quoting gitattributes(5):

    Custom macro attributes can be defined only in top-level
    gitattributes files ($GIT_DIR/info/attributes, the .gitattributes
    file at the top level of the working tree, or the global or
    system-wide gitattributes files), not in .gitattributes files in
    working tree subdirectories. The built-in macro attribute "binary"
    is equivalent to:

So gitattribute files in subdirectories of the working tree may
explicitly _not_ contain macro definitions, but we do not currently
enforce this limitation.

This patch introduces a new parameter to the gitattributes parser that
tells whether macros are allowed in the current file or not. If set to
`false`, we will still parse macros, but silently ignore them instead of
adding them to the list of defined macros. Update all callers to
correctly determine whether the to-be-parsed file may contain macros or
not. Most importantly, when walking up the directory hierarchy, we will
only set it to `true` once it reaches the root directory of the repo
itself.

Add a test that verifies that we are indeed not applying macros from
subdirectories. Previous to these changes, the test would've failed.
diff --git a/src/attr.c b/src/attr.c
@@ -252,15 +252,16 @@ static int preload_attr_file(
 	git_attr_session *attr_session,
 	git_attr_file_source source,
 	const char *base,
-	const char *file)
+	const char *file,
+	bool allow_macros)
 {
 	int error;
 	git_attr_file *preload = NULL;
 
 	if (!file)
 		return 0;
-	if (!(error = git_attr_cache__get(
-			&preload, repo, attr_session, source, base, file, git_attr_file__parse_buffer)))
+	if (!(error = git_attr_cache__get(&preload, repo, attr_session, source, base, file,
+					  git_attr_file__parse_buffer, allow_macros)))
 		git_attr_file__free(preload);
 
 	return error;
@@ -324,31 +325,31 @@ static int attr_setup(git_repository *repo, git_attr_session *attr_session)
 
 	if ((error = system_attr_file(&path, attr_session)) < 0 ||
 	    (error = preload_attr_file(repo, attr_session, GIT_ATTR_FILE__FROM_FILE,
-				       NULL, path.ptr)) < 0) {
+				       NULL, path.ptr, true)) < 0) {
 		if (error != GIT_ENOTFOUND)
 			goto out;
 	}
 
 	if ((error = preload_attr_file(repo, attr_session, GIT_ATTR_FILE__FROM_FILE,
-				       NULL, git_repository_attr_cache(repo)->cfg_attr_file)) < 0)
+				       NULL, git_repository_attr_cache(repo)->cfg_attr_file, true)) < 0)
 		goto out;
 
 	git_buf_clear(&path); /* git_repository_item_path expects an empty buffer, because it uses git_buf_set */
 	if ((error = git_repository_item_path(&path, repo, GIT_REPOSITORY_ITEM_INFO)) < 0 ||
 	    (error = preload_attr_file(repo, attr_session, GIT_ATTR_FILE__FROM_FILE,
-				       path.ptr, GIT_ATTR_FILE_INREPO)) < 0) {
+				       path.ptr, GIT_ATTR_FILE_INREPO, true)) < 0) {
 		if (error != GIT_ENOTFOUND)
 			goto out;
 	}
 
 	if ((workdir = git_repository_workdir(repo)) != NULL &&
 	    (error = preload_attr_file(repo, attr_session, GIT_ATTR_FILE__FROM_FILE,
-				       workdir, GIT_ATTR_FILE)) < 0)
+				       workdir, GIT_ATTR_FILE, true)) < 0)
 			goto out;
 
 	if ((error = git_repository_index__weakptr(&idx, repo)) < 0 ||
 	    (error = preload_attr_file(repo, attr_session, GIT_ATTR_FILE__FROM_INDEX,
-				       NULL, GIT_ATTR_FILE)) < 0)
+				       NULL, GIT_ATTR_FILE, true)) < 0)
 			goto out;
 
 	if (attr_session)
@@ -436,13 +437,14 @@ static int push_attr_file(
 	git_vector *list,
 	git_attr_file_source source,
 	const char *base,
-	const char *filename)
+	const char *filename,
+	bool allow_macros)
 {
 	int error = 0;
 	git_attr_file *file = NULL;
 
 	error = git_attr_cache__get(&file, repo, attr_session,
-		source, base, filename, git_attr_file__parse_buffer);
+		source, base, filename, git_attr_file__parse_buffer, allow_macros);
 
 	if (error < 0)
 		return error;
@@ -457,16 +459,18 @@ static int push_attr_file(
 
 static int push_one_attr(void *ref, const char *path)
 {
-	int error = 0, n_src, i;
 	attr_walk_up_info *info = (attr_walk_up_info *)ref;
 	git_attr_file_source src[2];
+	int error = 0, n_src, i;
+	bool allow_macros;
 
 	n_src = attr_decide_sources(
 		info->flags, info->workdir != NULL, info->index != NULL, src);
+	allow_macros = info->workdir ? !strcmp(info->workdir, path) : false;
 
 	for (i = 0; !error && i < n_src; ++i)
-		error = push_attr_file(info->repo, info->attr_session,
-			info->files, src[i], path, GIT_ATTR_FILE);
+		error = push_attr_file(info->repo, info->attr_session, info->files,
+				       src[i], path, GIT_ATTR_FILE, allow_macros);
 
 	return error;
 }
@@ -515,7 +519,7 @@ static int collect_attr_files(
 
 	if ((error = git_repository_item_path(&attrfile, repo, GIT_REPOSITORY_ITEM_INFO)) < 0 ||
 	    (error = push_attr_file(repo, attr_session, files, GIT_ATTR_FILE__FROM_FILE,
-				    attrfile.ptr, GIT_ATTR_FILE_INREPO)) < 0) {
+				    attrfile.ptr, GIT_ATTR_FILE_INREPO, true)) < 0) {
 		if (error != GIT_ENOTFOUND)
 			goto cleanup;
 	}
@@ -537,9 +541,8 @@ static int collect_attr_files(
 		goto cleanup;
 
 	if (git_repository_attr_cache(repo)->cfg_attr_file != NULL) {
-		error = push_attr_file(
-			repo, attr_session, files, GIT_ATTR_FILE__FROM_FILE,
-			NULL, git_repository_attr_cache(repo)->cfg_attr_file);
+		error = push_attr_file(repo, attr_session, files, GIT_ATTR_FILE__FROM_FILE,
+				       NULL, git_repository_attr_cache(repo)->cfg_attr_file, true);
 		if (error < 0)
 			goto cleanup;
 	}
@@ -548,9 +551,8 @@ static int collect_attr_files(
 		error = system_attr_file(&dir, attr_session);
 
 		if (!error)
-			error = push_attr_file(
-				repo, attr_session, files, GIT_ATTR_FILE__FROM_FILE,
-				NULL, dir.ptr);
+			error = push_attr_file(repo, attr_session, files, GIT_ATTR_FILE__FROM_FILE,
+					       NULL, dir.ptr, true);
 		else if (error == GIT_ENOTFOUND)
 			error = 0;
 	}
diff --git a/src/attr_file.c b/src/attr_file.c
@@ -105,7 +105,8 @@ int git_attr_file__load(
 	git_attr_session *attr_session,
 	git_attr_file_entry *entry,
 	git_attr_file_source source,
-	git_attr_file_parser parser)
+	git_attr_file_parser parser,
+	bool allow_macros)
 {
 	int error = 0;
 	git_blob *blob = NULL;
@@ -177,7 +178,7 @@ int git_attr_file__load(
 	if (attr_session)
 		file->session_key = attr_session->key;
 
-	if (parser && (error = parser(repo, file, content_str)) < 0) {
+	if (parser && (error = parser(repo, file, content_str, allow_macros)) < 0) {
 		git_attr_file__free(file);
 		goto cleanup;
 	}
@@ -249,7 +250,7 @@ static bool parse_optimized_patterns(
 	const char *pattern);
 
 int git_attr_file__parse_buffer(
-	git_repository *repo, git_attr_file *attrs, const char *data)
+	git_repository *repo, git_attr_file *attrs, const char *data, bool allow_macros)
 {
 	const char *scan = data, *context = NULL;
 	git_attr_rule *rule = NULL;
@@ -287,6 +288,8 @@ int git_attr_file__parse_buffer(
 
 		if (rule->match.flags & GIT_ATTR_FNMATCH_MACRO) {
 			/* TODO: warning if macro found in file below repo root */
+			if (!allow_macros)
+				continue;
 			if ((error = git_attr_cache__insert_macro(repo, rule)) < 0)
 				goto out;
 		} else if ((error = git_vector_insert(&attrs->rules, rule)) < 0)
@@ -355,7 +358,7 @@ int git_attr_file__load_standalone(git_attr_file **out, const char *path)
 	 */
 
 	if ((error = git_attr_file__new(&file, NULL, GIT_ATTR_FILE__FROM_FILE)) < 0 ||
-	    (error = git_attr_file__parse_buffer(NULL, file, content.ptr)) < 0 ||
+	    (error = git_attr_file__parse_buffer(NULL, file, content.ptr, true)) < 0 ||
 	    (error = git_attr_cache__alloc_file_entry(&file->entry, NULL, path, &file->pool)) < 0)
 		goto out;
 
diff --git a/src/attr_file.h b/src/attr_file.h
@@ -131,7 +131,8 @@ extern int git_attr_get_many_with_session(
 typedef int (*git_attr_file_parser)(
 	git_repository *repo,
 	git_attr_file *file,
-	const char *data);
+	const char *data,
+	bool allow_macros);
 
 /*
  * git_attr_file API
@@ -150,7 +151,8 @@ int git_attr_file__load(
 	git_attr_session *attr_session,
 	git_attr_file_entry *ce,
 	git_attr_file_source source,
-	git_attr_file_parser parser);
+	git_attr_file_parser parser,
+	bool allow_macros);
 
 int git_attr_file__load_standalone(
 	git_attr_file **out, const char *path);
@@ -159,7 +161,7 @@ int git_attr_file__out_of_date(
 	git_repository *repo, git_attr_session *session, git_attr_file *file);
 
 int git_attr_file__parse_buffer(
-	git_repository *repo, git_attr_file *attrs, const char *data);
+	git_repository *repo, git_attr_file *attrs, const char *data, bool allow_macros);
 
 int git_attr_file__clear_rules(
 	git_attr_file *file, bool need_lock);
diff --git a/src/attrcache.c b/src/attrcache.c
@@ -208,7 +208,8 @@ int git_attr_cache__get(
 	git_attr_file_source source,
 	const char *base,
 	const char *filename,
-	git_attr_file_parser parser)
+	git_attr_file_parser parser,
+	bool allow_macros)
 {
 	int error = 0;
 	git_attr_cache *cache = git_repository_attr_cache(repo);
@@ -221,7 +222,7 @@ int git_attr_cache__get(
 
 	/* load file if we don't have one or if existing one is out of date */
 	if (!file || (error = git_attr_file__out_of_date(repo, attr_session, file)) > 0)
-		error = git_attr_file__load(&updated, repo, attr_session, entry, source, parser);
+		error = git_attr_file__load(&updated, repo, attr_session, entry, source, parser, allow_macros);
 
 	/* if we loaded the file, insert into and/or update cache */
 	if (updated) {
diff --git a/src/attrcache.h b/src/attrcache.h
@@ -34,7 +34,8 @@ extern int git_attr_cache__get(
 	git_attr_file_source source,
 	const char *base,
 	const char *filename,
-	git_attr_file_parser parser);
+	git_attr_file_parser parser,
+	bool allow_macros);
 
 extern bool git_attr_cache__is_cached(
 	git_repository *repo,
diff --git a/src/ignore.c b/src/ignore.c
@@ -163,13 +163,15 @@ static int does_negate_rule(int *out, git_vector *rules, git_attr_fnmatch *match
 }
 
 static int parse_ignore_file(
-	git_repository *repo, git_attr_file *attrs, const char *data)
+	git_repository *repo, git_attr_file *attrs, const char *data, bool allow_macros)
 {
 	int error = 0;
 	int ignore_case = false;
 	const char *scan = data, *context = NULL;
 	git_attr_fnmatch *match = NULL;
 
+	GIT_UNUSED(allow_macros);
+
 	if (git_repository__cvar(&ignore_case, repo, GIT_CVAR_IGNORECASE) < 0)
 		git_error_clear();
 
@@ -244,9 +246,8 @@ static int push_ignore_file(
 	int error = 0;
 	git_attr_file *file = NULL;
 
-	error = git_attr_cache__get(
-		&file, ignores->repo, NULL, GIT_ATTR_FILE__FROM_FILE,
-		base, filename, parse_ignore_file);
+	error = git_attr_cache__get(&file, ignores->repo, NULL, GIT_ATTR_FILE__FROM_FILE,
+				    base, filename, parse_ignore_file, false);
 	if (error < 0)
 		return error;
 
@@ -272,12 +273,12 @@ static int get_internal_ignores(git_attr_file **out, git_repository *repo)
 	if ((error = git_attr_cache__init(repo)) < 0)
 		return error;
 
-	error = git_attr_cache__get(
-		out, repo, NULL, GIT_ATTR_FILE__IN_MEMORY, NULL, GIT_IGNORE_INTERNAL, NULL);
+	error = git_attr_cache__get(out, repo, NULL, GIT_ATTR_FILE__IN_MEMORY, NULL,
+				    GIT_IGNORE_INTERNAL, NULL, false);
 
 	/* if internal rules list is empty, insert default rules */
 	if (!error && !(*out)->rules.length)
-		error = parse_ignore_file(repo, *out, GIT_IGNORE_DEFAULT_RULES);
+		error = parse_ignore_file(repo, *out, GIT_IGNORE_DEFAULT_RULES, false);
 
 	return error;
 }
@@ -487,7 +488,7 @@ int git_ignore_add_rule(git_repository *repo, const char *rules)
 	if ((error = get_internal_ignores(&ign_internal, repo)) < 0)
 		return error;
 
-	error = parse_ignore_file(repo, ign_internal, rules);
+	error = parse_ignore_file(repo, ign_internal, rules, false);
 	git_attr_file__free(ign_internal);
 
 	return error;
@@ -503,7 +504,7 @@ int git_ignore_clear_internal_rules(git_repository *repo)
 
 	if (!(error = git_attr_file__clear_rules(ign_internal, true)))
 		error = parse_ignore_file(
-			repo, ign_internal, GIT_IGNORE_DEFAULT_RULES);
+				repo, ign_internal, GIT_IGNORE_DEFAULT_RULES, false);
 
 	git_attr_file__free(ign_internal);
 	return error;
diff --git a/tests/attr/lookup.c b/tests/attr/lookup.c
@@ -252,7 +252,7 @@ void test_attr_lookup__from_buffer(void)
 
 	cl_git_pass(git_attr_file__new(&file, NULL, 0));
 
-	cl_git_pass(git_attr_file__parse_buffer(NULL, file, "a* foo\nabc bar\n* baz"));
+	cl_git_pass(git_attr_file__parse_buffer(NULL, file, "a* foo\nabc bar\n* baz", true));
 
 	cl_assert(file->rules.length == 3);
 
diff --git a/tests/attr/macro.c b/tests/attr/macro.c
@@ -125,6 +125,22 @@ void test_attr_macro__changing_macro_in_root_wd_updates_attributes(void)
 	cl_assert_equal_s(value, "second");
 }
 
+void test_attr_macro__macros_in_subdir_do_not_apply(void)
+{
+	const char *value;
+
+	g_repo = cl_git_sandbox_init("empty_standard_repo");
+
+	cl_git_pass(p_mkdir("empty_standard_repo/dir", 0777));
+	cl_git_rewritefile("empty_standard_repo/dir/.gitattributes",
+			   "[attr]customattr key=value\n"
+			   "file customattr\n");
+
+	/* This should _not_ pass, as macros in subdirectories shall be ignored */
+	cl_git_pass(git_attr_get(&value, g_repo, 0, "dir/file", "key"));
+	cl_assert_equal_p(value, NULL);
+}
+
 void test_attr_macro__adding_macro_succeeds(void)
 {
 	const char *value;
