Improved gitattributes macro implementation

This updates to implementation of gitattribute macros to be much more
similar to core git (albeit not 100%) and to handle expansion of
macros within macros, etc.  It also cleans up the refcounting usage
with macros to be much cleaner.

Also, this adds a new vector function `git_vector_insert_sorted()`
which allows you to maintain a sorted list as you go.  In order to
write that function, this changes the function `git__bsearch()` to
take a somewhat different set of parameters, although the core
functionality is still the same.

src/attr_file.c

@@ -12,17 +12,9 @@ static void git_attr_rule__clear(git_attr_rule *rule);
 
 int git_attr_cache__insert_macro(git_repository *repo, git_attr_rule *macro)
 {
-	unsigned int i;
-	git_attr_assignment *assign;
-
 	if (macro->assigns.length == 0)
 		return git__throw(GIT_EMISSINGOBJDATA, "git attribute macro with no values");
 
-	git_vector_foreach(&macro->assigns, i, assign) {
-		GIT_REFCOUNT_OWN(assign, macro);
-		GIT_REFCOUNT_INC(assign);
-	}
-
 	return git_hashtable_insert(
 		repo->attrcache.macros, macro->match.pattern, macro);
 }
@@ -358,7 +350,7 @@ static int sort_by_hash_and_name(const void *a_raw, const void *b_raw)
 		return strcmp(b->name, a->name);
 }
 
-static void free_assign(git_attr_assignment *assign)
+static void git_attr_assignment__free(git_attr_assignment *assign)
 {
 	git__free(assign->name);
 	assign->name = NULL;
@@ -371,6 +363,16 @@ static void free_assign(git_attr_assignment *assign)
 	git__free(assign);
 }
 
+static int merge_assignments(void **old_raw, void *new_raw)
+{
+	git_attr_assignment **old = (git_attr_assignment **)old_raw;
+	git_attr_assignment *new = (git_attr_assignment *)new_raw;
+
+	GIT_REFCOUNT_DEC(*old, git_attr_assignment__free);
+	*old = new;
+	return GIT_EEXISTS;
+}
+
 int git_attr_assignment__parse(
 	git_repository *repo,
 	git_vector *assigns,
@@ -382,6 +384,8 @@ int git_attr_assignment__parse(
 
 	assert(assigns && !assigns->length);
 
+	assigns->_cmp = sort_by_hash_and_name;
+
 	while (*scan && *scan != '\n' && error == GIT_SUCCESS) {
 		const char *name_start, *value_start;
 
@@ -395,6 +399,7 @@ int git_attr_assignment__parse(
 				error = GIT_ENOMEM;
 				break;
 			}
+			GIT_REFCOUNT_INC(assign);
 		}
 
 		assign->name_hash = 5381;
@@ -449,8 +454,8 @@ int git_attr_assignment__parse(
 			}
 		}
 
-		/* expand macros (if given a repo) */
-		if (repo != NULL) {
+		/* expand macros (if given a repo with a macro cache) */
+		if (repo != NULL && assign->value == GIT_ATTR_TRUE) {
 			git_attr_rule *macro =
 				git_hashtable_lookup(repo->attrcache.macros, assign->name);
 
@@ -458,30 +463,25 @@ int git_attr_assignment__parse(
 				unsigned int i;
 				git_attr_assignment *massign;
 
-				/* issue warning: if assign->value != GIT_ATTR_TRUE */
-
-				git__free(assign->name);
-				assign->name = NULL;
-				if (assign->is_allocated) {
-					git__free((void *)assign->value);
-					assign->value = NULL;
-				}
-
 				git_vector_foreach(&macro->assigns, i, massign) {
-					error = git_vector_insert(assigns, massign);
-					if (error != GIT_SUCCESS)
-						break;
-					GIT_REFCOUNT_INC(&massign->rc);
-				}
+					GIT_REFCOUNT_INC(massign);
 
-				/* continue to next assignment */
-				continue;
+					error = git_vector_insert_sorted(
+						assigns, massign, &merge_assignments);
+
+					if (error == GIT_EEXISTS)
+						error = GIT_SUCCESS;
+					else if (error != GIT_SUCCESS)
+						break;
+				}
 			}
 		}
 
 		/* insert allocated assign into vector */
-		error = git_vector_insert(assigns, assign);
-		if (error < GIT_SUCCESS)
+		error = git_vector_insert_sorted(assigns, assign, &merge_assignments);
+		if (error == GIT_EEXISTS)
+			error = GIT_SUCCESS;
+		else if (error < GIT_SUCCESS)
 			break;
 
 		/* clear assign since it is now "owned" by the vector */
@@ -490,13 +490,9 @@ int git_attr_assignment__parse(
 
 	if (!assigns->length)
 		error = git__throw(GIT_ENOTFOUND, "No attribute assignments found for rule");
-	else {
-		assigns->_cmp = sort_by_hash_and_name;
-		git_vector_sort(assigns);
-	}
 
 	if (assign != NULL)
-		free_assign(assign);
+		git_attr_assignment__free(assign);
 
 	while (*scan && *scan != '\n') scan++;
 	if (*scan == '\n') scan++;
@@ -518,11 +514,8 @@ static void git_attr_rule__clear(git_attr_rule *rule)
 	rule->match.pattern = NULL;
 	rule->match.length = 0;
 
-	git_vector_foreach(&rule->assigns, i, assign) {
-		if (GIT_REFCOUNT_OWNER(assign) == rule)
-			GIT_REFCOUNT_OWN(assign, NULL);
-		GIT_REFCOUNT_DEC(assign, free_assign);
-	}
+	git_vector_foreach(&rule->assigns, i, assign)
+		GIT_REFCOUNT_DEC(assign, git_attr_assignment__free);
 
 	git_vector_free(&rule->assigns);
 }

src/util.c

@@ -348,22 +348,30 @@ uint32_t git__hash(const void *key, int len, uint32_t seed)
  * Copyright (c) 1990 Regents of the University of California.
  * All rights reserved.
  */
-void **git__bsearch(const void *key, void **base, size_t nmemb, int (*compar)(const void *, const void *))
+int git__bsearch(
+	void **array,
+	size_t array_len,
+	const void *key,
+	int (*compare)(const void *, const void *),
+	size_t *position)
 {
 	int lim, cmp;
-		void **p;
+	void **part, **base = array;
 
-		for (lim = nmemb; lim != 0; lim >>= 1) {
-				p = base + (lim >> 1);
-				cmp = (*compar)(key, *p);
-				if (cmp > 0) { /* key > p: move right */
-						base = p + 1;
+	for (lim = array_len; lim != 0; lim >>= 1) {
+		part = base + (lim >> 1);
+		cmp = (*compare)(key, *part);
+		if (cmp == 0) {
+			*position = (part - array);
+			return GIT_SUCCESS;
+		} else if (cmp > 0) { /* key > p; take right partition */
+			base = part + 1;
 			lim--;
-				} else if (cmp == 0) {
-						return (void **)p;
-				} /* else move left */
+		} /* else take left partition */
 	}
-		return NULL;
+
+	*position = (base - array);
+	return GIT_ENOTFOUND;
 }
 
 /**

src/util.h

@@ -105,8 +105,13 @@ extern void git__strtolower(char *str);
 extern int git__fnmatch(const char *pattern, const char *name, int flags);
 
 extern void git__tsort(void **dst, size_t size, int (*cmp)(const void *, const void *));
-extern void **git__bsearch(const void *key, void **base, size_t nmemb,
-	int (*compar)(const void *, const void *));
+
+extern int git__bsearch(
+	void **array,
+	size_t array_len,
+	const void *key,
+	int (*compare)(const void *, const void *),
+	size_t *position);
 
 extern int git__strcmp_cb(const void *a, const void *b);
 

src/vector.c

@@ -74,6 +74,45 @@ int git_vector_insert(git_vector *v, void *element)
 	return GIT_SUCCESS;
 }
 
+int git_vector_insert_sorted(git_vector *v, void *element, int (*on_dup)(void **old, void *new))
+{
+	int error = GIT_SUCCESS;
+	size_t pos;
+
+	assert(v && v->_cmp);
+
+	if (!v->sorted)
+		git_vector_sort(v);
+
+	if (v->length >= v->_alloc_size) {
+		if (resize_vector(v) < 0)
+			return GIT_ENOMEM;
+	}
+
+	error = git__bsearch(v->contents, v->length, element, v->_cmp, &pos);
+
+	/* If we found the element and have a duplicate handler callback,
+	 * invoke it.  If it returns an error, then cancel insert, otherwise
+	 * proceed with normal insert.
+	 */
+	if (error == GIT_SUCCESS && on_dup != NULL) {
+		error = on_dup(&v->contents[pos], element);
+		if (error != GIT_SUCCESS)
+			return error;
+	}
+
+	/* shift elements to the right */
+	if (pos < v->length) {
+		memmove(v->contents + pos + 1, v->contents + pos,
+		        (v->length - pos) * sizeof(void *));
+	}
+
+	v->contents[pos] = element;
+	v->length++;
+
+	return GIT_SUCCESS;
+}
+
 void git_vector_sort(git_vector *v)
 {
 	assert(v);
@@ -87,7 +126,7 @@ void git_vector_sort(git_vector *v)
 
 int git_vector_bsearch2(git_vector *v, git_vector_cmp key_lookup, const void *key)
 {
-	void **find;
+	size_t pos;
 
 	assert(v && key && key_lookup);
 
@@ -97,9 +136,9 @@ int git_vector_bsearch2(git_vector *v, git_vector_cmp key_lookup, const void *ke
 
 	git_vector_sort(v);
 
-	find = git__bsearch(key, v->contents, v->length, key_lookup);
-	if (find != NULL)
-		return (int)(find - v->contents);
+	if (git__bsearch(v->contents, v->length, key, key_lookup,
+			&pos) == GIT_SUCCESS)
+		return (int)pos;
 
 	return git__throw(GIT_ENOTFOUND, "Can't find element");
 }

src/vector.h

@@ -45,6 +45,9 @@ GIT_INLINE(void *) git_vector_get(git_vector *v, unsigned int position)
 	for ((iter) = (v)->length; (iter) > 0 && ((elem) = (v)->contents[(iter)-1], 1); (iter)-- )
 
 int git_vector_insert(git_vector *v, void *element);
+int git_vector_insert_sorted(git_vector *v, void *element,
+	int (*on_dup)(void **old, void *new));
 int git_vector_remove(git_vector *v, unsigned int idx);
 void git_vector_uniq(git_vector *v);
+
 #endif

tests-clay/attr/repo.c

@@ -40,8 +40,11 @@ void test_attr_repo__get_one(void)
 		{ "root_test2", "repoattr", GIT_ATTR_TRUE },
 		{ "root_test2", "rootattr", GIT_ATTR_FALSE },
 		{ "root_test2", "missingattr", NULL },
+		{ "root_test2", "multiattr", GIT_ATTR_FALSE },
 		{ "root_test3", "repoattr", GIT_ATTR_TRUE },
 		{ "root_test3", "rootattr", NULL },
+		{ "root_test3", "multiattr", "3" },
+		{ "root_test3", "multi2", NULL },
 		{ "subdir/subdir_test1", "repoattr", GIT_ATTR_TRUE },
 		{ "subdir/subdir_test1", "rootattr", GIT_ATTR_TRUE },
 		{ "subdir/subdir_test1", "missingattr", NULL },
@@ -166,21 +169,68 @@ void test_attr_repo__macros(void)
 {
 	const char *names[5] = { "rootattr", "binary", "diff", "crlf", "frotz" };
 	const char *names2[5] = { "mymacro", "positive", "negative", "rootattr", "another" };
+	const char *names3[3] = { "macro2", "multi2", "multi3" };
 	const char *values[5];
 
 	cl_git_pass(git_attr_get_many(g_repo, "binfile", 5, names, values));
 
 	cl_assert(values[0] == GIT_ATTR_TRUE);
-	cl_assert(values[1] == NULL);
+	cl_assert(values[1] == GIT_ATTR_TRUE);
 	cl_assert(values[2] == GIT_ATTR_FALSE);
 	cl_assert(values[3] == GIT_ATTR_FALSE);
 	cl_assert(values[4] == NULL);
 
 	cl_git_pass(git_attr_get_many(g_repo, "macro_test", 5, names2, values));
 
-	cl_assert(values[0] == NULL);
+	cl_assert(values[0] == GIT_ATTR_TRUE);
 	cl_assert(values[1] == GIT_ATTR_TRUE);
 	cl_assert(values[2] == GIT_ATTR_FALSE);
 	cl_assert(values[3] == NULL);
 	cl_assert_strequal("77", values[4]);
+
+	cl_git_pass(git_attr_get_many(g_repo, "macro_test", 3, names3, values));
+
+	cl_assert(values[0] == GIT_ATTR_TRUE);
+	cl_assert(values[1] == GIT_ATTR_FALSE);
+	cl_assert_strequal("answer", values[2]);
+}
+
+void test_attr_repo__bad_macros(void)
+{
+	const char *names[6] = { "rootattr", "positive", "negative",
+		"firstmacro", "secondmacro", "thirdmacro" };
+	const char *values[6];
+
+	cl_git_pass(git_attr_get_many(g_repo, "macro_bad", 6, names, values));
+
+	/* these three just confirm that the "mymacro" rule ran */
+	cl_assert(values[0] == NULL);
+	cl_assert(values[1] == GIT_ATTR_TRUE);
+	cl_assert(values[2] == GIT_ATTR_FALSE);
+
+	/* file contains:
+	 *     # let's try some malicious macro defs
+	 *     [attr]firstmacro -thirdmacro -secondmacro
+	 *     [attr]secondmacro firstmacro -firstmacro
+	 *     [attr]thirdmacro secondmacro=hahaha -firstmacro
+	 *     macro_bad firstmacro secondmacro thirdmacro
+	 *
+	 * firstmacro assignment list ends up with:
+	 *     -thirdmacro -secondmacro
+	 * secondmacro assignment list expands "firstmacro" and ends up with:
+	 *     -thirdmacro -secondmacro -firstmacro
+	 * thirdmacro assignment don't expand so list ends up with:
+	 *     secondmacro="hahaha"
+	 *
+	 * macro_bad assignment list ends up with:
+	 *     -thirdmacro -secondmacro firstmacro &&
+	 *     -thirdmacro -secondmacro -firstmacro secondmacro &&
+	 *     secondmacro="hahaha" thirdmacro
+	 *
+	 * so summary results should be:
+	 *     -firstmacro secondmacro="hahaha" thirdmacro
+	 */
+	cl_assert(values[3] == GIT_ATTR_FALSE);
+	cl_assert_strequal("hahaha", values[4]);
+	cl_assert(values[5] == GIT_ATTR_TRUE);
 }

tests-clay/clay.h

@@ -68,6 +68,7 @@ extern void test_attr_lookup__check_attr_examples(void);
 extern void test_attr_lookup__from_buffer(void);
 extern void test_attr_lookup__match_variants(void);
 extern void test_attr_lookup__simple(void);
+extern void test_attr_repo__bad_macros(void);
 extern void test_attr_repo__cleanup(void);
 extern void test_attr_repo__foreach(void);
 extern void test_attr_repo__get_many(void);
@@ -141,6 +142,9 @@ extern void test_core_strtol__int64(void);
 extern void test_core_vector__0(void);
 extern void test_core_vector__1(void);
 extern void test_core_vector__2(void);
+extern void test_core_vector__3(void);
+extern void test_core_vector__4(void);
+extern void test_core_vector__5(void);
 extern void test_index_rename__single_file(void);
 extern void test_network_remotes__cleanup(void);
 extern void test_network_remotes__fnmatch(void);

tests-clay/clay_main.c

@@ -122,6 +122,7 @@ static const struct clay_func _clay_cb_attr_lookup[] = {
 	{"simple", &test_attr_lookup__simple}
 };
 static const struct clay_func _clay_cb_attr_repo[] = {
+    {"bad_macros", &test_attr_repo__bad_macros},
 	{"foreach", &test_attr_repo__foreach},
 	{"get_many", &test_attr_repo__get_many},
 	{"get_one", &test_attr_repo__get_one},
@@ -214,7 +215,10 @@ static const struct clay_func _clay_cb_core_strtol[] = {
 static const struct clay_func _clay_cb_core_vector[] = {
     {"0", &test_core_vector__0},
 	{"1", &test_core_vector__1},
-	{"2", &test_core_vector__2}
+	{"2", &test_core_vector__2},
+	{"3", &test_core_vector__3},
+	{"4", &test_core_vector__4},
+	{"5", &test_core_vector__5}
 };
 static const struct clay_func _clay_cb_index_rename[] = {
     {"single_file", &test_index_rename__single_file}
@@ -338,7 +342,7 @@ static const struct clay_suite _clay_suites[] = {
         "attr::repo",
         {"initialize", &test_attr_repo__initialize},
         {"cleanup", &test_attr_repo__cleanup},
-        _clay_cb_attr_repo, 5
+        _clay_cb_attr_repo, 6
     },
 	{
         "buf::basic",
@@ -428,7 +432,7 @@ static const struct clay_suite _clay_suites[] = {
         "core::vector",
         {NULL, NULL},
         {NULL, NULL},
-        _clay_cb_core_vector, 3
+        _clay_cb_core_vector, 6
     },
 	{
         "index::rename",
@@ -559,7 +563,7 @@ static const struct clay_suite _clay_suites[] = {
 };
 
 static size_t _clay_suite_count = 39;
-static size_t _clay_callback_count = 134;
+static size_t _clay_callback_count = 138;
 
 /* Core test functions */
 static void

tests-clay/core/vector.c

@@ -64,3 +64,128 @@ void test_core_vector__2(void)
 }
 
 
+static int compare_them(const void *a, const void *b)
+{
+	return (int)((long)a - (long)b);
+}
+
+/* insert_sorted */
+void test_core_vector__3(void)
+{
+	git_vector x;
+	long i;
+	git_vector_init(&x, 1, &compare_them);
+
+	for (i = 0; i < 10; i += 2) {
+		git_vector_insert_sorted(&x, (void*)(i + 1), NULL);
+	}
+
+	for (i = 9; i > 0; i -= 2) {
+		git_vector_insert_sorted(&x, (void*)(i + 1), NULL);
+	}
+
+	cl_assert(x.length == 10);
+	for (i = 0; i < 10; ++i) {
+		cl_assert(git_vector_get(&x, i) == (void*)(i + 1));
+	}
+
+	git_vector_free(&x);
+}
+
+/* insert_sorted with duplicates */
+void test_core_vector__4(void)
+{
+	git_vector x;
+	long i;
+	git_vector_init(&x, 1, &compare_them);
+
+	for (i = 0; i < 10; i += 2) {
+		git_vector_insert_sorted(&x, (void*)(i + 1), NULL);
+	}
+
+	for (i = 9; i > 0; i -= 2) {
+		git_vector_insert_sorted(&x, (void*)(i + 1), NULL);
+	}
+
+	for (i = 0; i < 10; i += 2) {
+		git_vector_insert_sorted(&x, (void*)(i + 1), NULL);
+	}
+
+	for (i = 9; i > 0; i -= 2) {
+		git_vector_insert_sorted(&x, (void*)(i + 1), NULL);
+	}
+
+	cl_assert(x.length == 20);
+	for (i = 0; i < 20; ++i) {
+		cl_assert(git_vector_get(&x, i) == (void*)(i / 2 + 1));
+	}
+
+	git_vector_free(&x);
+}
+
+typedef struct {
+	int content;
+	int count;
+} my_struct;
+
+static int _struct_count = 0;
+
+static int compare_structs(const void *a, const void *b)
+{
+	return ((const my_struct *)a)->content -
+		((const my_struct *)b)->content;
+}
+
+static int merge_structs(void **old_raw, void *new)
+{
+	my_struct *old = *(my_struct **)old_raw;
+	cl_assert(((my_struct *)old)->content == ((my_struct *)new)->content);
+	((my_struct *)old)->count += 1;
+	git__free(new);
+	_struct_count--;
+	return GIT_EEXISTS;
+}
+
+static my_struct *alloc_struct(int value)
+{
+	my_struct *st = git__malloc(sizeof(my_struct));
+	st->content = value;
+	st->count = 0;
+	_struct_count++;
+	return st;
+}
+
+/* insert_sorted with duplicates and special handling */
+void test_core_vector__5(void)
+{
+	git_vector x;
+	int i;
+
+	git_vector_init(&x, 1, &compare_structs);
+
+	for (i = 0; i < 10; i += 2)
+		git_vector_insert_sorted(&x, alloc_struct(i), &merge_structs);
+
+	for (i = 9; i > 0; i -= 2)
+		git_vector_insert_sorted(&x, alloc_struct(i), &merge_structs);
+
+	cl_assert(x.length == 10);
+	cl_assert(_struct_count == 10);
+
+	for (i = 0; i < 10; i += 2)
+		git_vector_insert_sorted(&x, alloc_struct(i), &merge_structs);
+
+	for (i = 9; i > 0; i -= 2)
+		git_vector_insert_sorted(&x, alloc_struct(i), &merge_structs);
+
+	cl_assert(x.length == 10);
+	cl_assert(_struct_count == 10);
+
+	for (i = 0; i < 10; ++i) {
+		cl_assert(((my_struct *)git_vector_get(&x, i))->content == i);
+		git__free(git_vector_get(&x, i));
+		_struct_count--;
+	}
+
+	git_vector_free(&x);
+}