mailmap: API and style cleanup

Author: mystor

include/git2/mailmap.h

@@ -85,8 +85,9 @@ GIT_EXTERN(void) git_mailmap_free(git_mailmap *mailmap);
  * @param mailmap the mailmap to perform the lookup in. (may be NULL)
  * @param name the name to resolve.
  * @param email the email to resolve.
+ * @return 0 on success, otherwise an error code.
  */
-GIT_EXTERN(void) git_mailmap_resolve(
+GIT_EXTERN(int) git_mailmap_resolve(
 	const char **name_out, const char **email_out,
 	const git_mailmap *mailmap, const char *name, const char *email);
 

src/mailmap.c

@@ -21,6 +21,18 @@ struct git_mailmap {
 	git_vector entries;
 };
 
+static void mailmap_entry_free(git_mailmap_entry *entry)
+{
+	if (!entry)
+		return;
+
+	git__free(entry->real_name);
+	git__free(entry->real_email);
+	git__free(entry->replace_name);
+	git__free(entry->replace_email);
+	git__free(entry);
+}
+
 /* Check if we're at the end of line, w/ comments */
 static bool is_eol(git_parse_ctx *ctx)
 {
@@ -43,7 +55,8 @@ static int advance_until(
 	return 0;
 }
 
-/* Parse a single entry from a mailmap file.
+/*
+ * Parse a single entry from a mailmap file.
  *
  * The output git_bufs will be non-owning, and should be copied before being
  * persisted.
@@ -61,16 +74,21 @@ static int parse_mailmap_entry(
 	git_buf_clear(replace_name);
 	git_buf_clear(replace_email);
 
-	/* Parse the real name */
 	git_parse_advance_ws(ctx);
+	if (is_eol(ctx))
+		return -1; /* blank line */
+
+	/* Parse the real name */
 	if (advance_until(&start, &len, ctx, '<') < 0)
 		return -1;
 
 	git_buf_attach_notowned(real_name, start, len);
 	git_buf_rtrim(real_name);
 
-	/* If this is the last email in the line, this is the email to replace,
-	 * otherwise, it's the real email. */
+	/*
+	 * If this is the last email in the line, this is the email to replace,
+	 * otherwise, it's the real email.
+	 */
 	if (advance_until(&start, &len, ctx, '>') < 0)
 		return -1;
 
@@ -131,38 +149,27 @@ int git_mailmap_from_buffer(git_mailmap **out, git_buf *buf)
 		if (error < 0) {
 			error = 0; /* Skip lines which don't contain a valid entry */
 			git_parse_advance_line(&ctx);
-			continue;
+			continue; /* TODO: warn */
 		}
 
-		GITERR_CHECK_ALLOC_ADD5(
-			&entry_size, sizeof(git_mailmap_entry) + 4 /* 4x'\0' */,
-			real_name.size, real_email.size,
-			replace_name.size, replace_email.size);
-		entry = git__calloc(1, entry_size);
+		entry = git__calloc(1, sizeof(git_mailmap_entry));
 		GITERR_CHECK_ALLOC(entry);
-
 		entry->version = GIT_MAILMAP_ENTRY_VERSION;
 
-		/* Copy strings into the buffer following entry */
-		entry_data = (char *)(entry + 1);
 		if (real_name.size > 0) {
-			memcpy(entry_data, real_name.ptr, real_name.size);
-			entry->real_name = entry_data;
-			entry_data += real_name.size + 1; /* advance past null from calloc */
+			entry->real_name = git__substrdup(real_name.ptr, real_name.size);
+			GITERR_CHECK_ALLOC(entry->real_name);
 		}
 		if (real_email.size > 0) {
-			memcpy(entry_data, real_email.ptr, real_email.size);
-			entry->real_email = entry_data;
-			entry_data += real_email.size + 1;
+			entry->real_email = git__substrdup(real_email.ptr, real_email.size);
+			GITERR_CHECK_ALLOC(entry->real_email);
 		}
 		if (replace_name.size > 0) {
-			memcpy(entry_data, replace_name.ptr, replace_name.size);
-			entry->replace_name = entry_data;
-			entry_data += replace_name.size + 1;
+			entry->replace_name = git__substrdup(replace_name.ptr, replace_name.size);
+			GITERR_CHECK_ALLOC(entry->replace_name);
 		}
-		/* replace_email is always non-null */
-		memcpy(entry_data, replace_email.ptr, replace_email.size);
-		entry->replace_email = entry_data;
+		entry->replace_email = git__substrdup(replace_email.ptr, replace_email.size);
+		GITERR_CHECK_ALLOC(entry->replace_email);
 
 		error = git_vector_insert(&mm->entries, entry);
 		if (error < 0)
@@ -175,7 +182,7 @@ int git_mailmap_from_buffer(git_mailmap **out, git_buf *buf)
 	mm = NULL;
 
 cleanup:
-	git__free(entry);
+	mailmap_entry_free(entry);
 	git_mailmap_free(mm);
 
 	/* We never allocate data in these buffers, but better safe than sorry */
@@ -191,11 +198,15 @@ void git_mailmap_free(git_mailmap *mailmap)
 	if (!mailmap)
 		return;
 
-	git_vector_free_deep(&mailmap->entries);
+	git_vector_foreach(&mailmap->entries, i, entry) {
+		mailmap_entry_free(entry);
+	}
+	git_vector_free(&mailmap->entries);
+
 	git__free(mailmap);
 }
 
-void git_mailmap_resolve(
+int git_mailmap_resolve(
 	const char **name_out, const char **email_out,
 	const git_mailmap *mailmap,
 	const char *name, const char *email)
@@ -207,7 +218,7 @@ void git_mailmap_resolve(
 	*email_out = email;
 
 	if (!mailmap)
-		return;
+		return 0;
 
 	entry = git_mailmap_entry_lookup(mailmap, name, email);
 	if (entry) {
@@ -216,6 +227,7 @@ void git_mailmap_resolve(
 		if (entry->real_email)
 			*email_out = entry->real_email;
 	}
+	return 0;
 }
 
 const git_mailmap_entry *git_mailmap_entry_lookup(
@@ -229,10 +241,12 @@ const git_mailmap_entry *git_mailmap_entry_lookup(
 		return NULL;
 
 	git_vector_foreach(&mailmap->entries, i, entry) {
-		if (!git__strcmp(email, entry->replace_email) &&
-		    (!entry->replace_name || !git__strcmp(name, entry->replace_name))) {
-			return entry;
-		}
+		if (git__strcmp(email, entry->replace_email))
+			continue;
+		if (entry->replace_name && git__strcmp(name, entry->replace_name))
+			continue;
+
+		return entry;
 	}
 
 	return NULL;

src/signature.c

@@ -111,11 +111,14 @@ int git_signature_with_mailmap(
 	git_signature *signature = NULL;
 	const char *name = NULL;
 	const char *email = NULL;
+	int error;
 
 	if (source == NULL)
 		return 0;
 
-	git_mailmap_resolve(&name, &email, mailmap, source->name, source->email);
+	error = git_mailmap_resolve(&name, &email, mailmap, source->name, source->email);
+	if (error < 0)
+		return error;
 
 	signature = git__calloc(1, sizeof(git_signature));
 	GITERR_CHECK_ALLOC(signature);

tests/clar_libgit2.c

@@ -171,8 +171,7 @@ static git_repository *_cl_repo = NULL;
 
 git_repository *cl_git_sandbox_init(const char *sandbox)
 {
-	/* Get the name of the sandbox folder which will be created
-	 */
+	/* Get the name of the sandbox folder which will be created */
 	const char *basename = cl_fixture_basename(sandbox);
 
 	/* Copy the whole sandbox folder from our fixtures to our test sandbox

tests/mailmap/basic.c

@@ -13,43 +13,59 @@ const char TEST_MAILMAP[] =
 	"<email@foo.com> <otheremail@foo.com>\n"
 	"<email@foo.com> Other Name <yetanotheremail@foo.com>\n";
 
+struct {
+	const char *real_name;
+	const char *real_email;
+	const char *replace_name;
+	const char *replace_email;
+} expected[] = {
+	{ "Foo bar", "foo@bar.com", NULL, "foo@baz.com" },
+	{ "Foo bar", "foo@bar.com", NULL, "foo@bal.com" },
+	{ NULL, "email@foo.com", NULL, "otheremail@foo.com" },
+	{ NULL, "email@foo.com", "Other Name", "yetanotheremail@foo.com" }
+};
+
 void test_mailmap_basic__initialize(void)
 {
 	git_buf buf = GIT_BUF_INIT;
-	git_buf_attach_notowned(&buf, TEST_MAILMAP, sizeof(TEST_MAILMAP) - 1);
+	git_buf_attach_notowned(&buf, TEST_MAILMAP, strlen(TEST_MAILMAP));
 
 	cl_git_pass(git_mailmap_from_buffer(&mailmap, &buf));
 }
 
 void test_mailmap_basic__cleanup(void)
 {
-	if (mailmap) {
-		git_mailmap_free(mailmap);
-		mailmap = NULL;
-	}
+	git_mailmap_free(mailmap);
+	mailmap = NULL;
 }
 
 void test_mailmap_basic__entry(void)
 {
 	const git_mailmap_entry *entry;
 
-	cl_assert(git_mailmap_entry_count(mailmap) == 4);
+	cl_assert_equal_sz(ARRAY_SIZE(expected), git_mailmap_entry_count(mailmap));
 
-	entry = git_mailmap_entry_byindex(mailmap, 0);
-	cl_assert(entry);
-	cl_assert(!entry->replace_name);
-	cl_assert(!git__strcmp(entry->replace_email, "foo@baz.com"));
+	for (size_t i = 0; i < ARRAY_SIZE(expected); ++i) {
+		entry = git_mailmap_entry_byindex(mailmap, i);
+		cl_assert(entry);
+		cl_assert_equal_s(entry->real_name, expected[i].real_name);
+		cl_assert_equal_s(entry->real_email, expected[i].real_email);
+		cl_assert_equal_s(entry->replace_name, expected[i].replace_name);
+		cl_assert_equal_s(entry->replace_email, expected[i].replace_email);
+	}
+}
 
-	entry = git_mailmap_entry_byindex(mailmap, 10000);
+void test_mailmap_basic__entry_large_index(void)
+{
+	const git_mailmap_entry *entry =
+		git_mailmap_entry_byindex(mailmap, 10000);
 	cl_assert(!entry);
 }
 
 void test_mailmap_basic__lookup_not_found(void)
 {
 	const git_mailmap_entry *entry = git_mailmap_entry_lookup(
-		mailmap,
-		"Whoever",
-		"doesnotexist@fo.com");
+		mailmap, "Whoever", "doesnotexist@fo.com");
 	cl_assert(!entry);
 }
 
@@ -58,31 +74,32 @@ void test_mailmap_basic__lookup(void)
 	const git_mailmap_entry *entry = git_mailmap_entry_lookup(
 		mailmap, "Typoed the name once", "foo@baz.com");
 	cl_assert(entry);
-	cl_assert(!git__strcmp(entry->real_name, "Foo bar"));
+	cl_assert_equal_s(entry->real_name, "Foo bar");
 }
 
 void test_mailmap_basic__empty_email_query(void)
 {
 	const char *name;
 	const char *email;
-	git_mailmap_resolve(
-		&name, &email, mailmap, "Author name", "otheremail@foo.com");
-	cl_assert(!git__strcmp(name, "Author name"));
-	cl_assert(!git__strcmp(email, "email@foo.com"));
+	cl_git_pass(git_mailmap_resolve(
+		&name, &email, mailmap, "Author name", "otheremail@foo.com"));
+	cl_assert_equal_s(name, "Author name");
+	cl_assert_equal_s(email, "email@foo.com");
 }
 
 void test_mailmap_basic__name_matching(void)
 {
 	const char *name;
 	const char *email;
-	git_mailmap_resolve(
-		&name, &email, mailmap, "Other Name", "yetanotheremail@foo.com");
-	cl_assert(!git__strcmp(name, "Other Name"));
-	cl_assert(!git__strcmp(email, "email@foo.com"));
+	cl_git_pass(git_mailmap_resolve(
+		&name, &email, mailmap, "Other Name", "yetanotheremail@foo.com"));
+
+	cl_assert_equal_s(name, "Other Name");
+	cl_assert_equal_s(email, "email@foo.com");
 
-	git_mailmap_resolve(
+	cl_git_pass(git_mailmap_resolve(
 		&name, &email, mailmap,
-		"Other Name That Doesn't Match", "yetanotheremail@foo.com");
-	cl_assert(!git__strcmp(name, "Other Name That Doesn't Match"));
-	cl_assert(!git__strcmp(email, "yetanotheremail@foo.com"));
+		"Other Name That Doesn't Match", "yetanotheremail@foo.com"));
+	cl_assert_equal_s(name, "Other Name That Doesn't Match");
+	cl_assert_equal_s(email, "yetanotheremail@foo.com");
 }

tests/mailmap/parsing.c

@@ -44,12 +44,9 @@ static void check_mailmap_resolve(
 
 	/* Check that the resolver behaves correctly */
 	for (idx = 0; idx < resolved_size; ++idx) {
-		git_mailmap_resolve(
-			&resolved_name,
-			&resolved_email,
-			mailmap,
-			resolved[idx].replace_name,
-			resolved[idx].replace_email);
+		cl_git_pass(git_mailmap_resolve(
+			&resolved_name, &resolved_email, mailmap,
+			resolved[idx].replace_name, resolved[idx].replace_email));
 		cl_assert_equal_s(resolved_name, resolved[idx].real_name);
 		cl_assert_equal_s(resolved_email, resolved[idx].real_email);
 	}
@@ -70,6 +67,27 @@ void test_mailmap_parsing__string(void)
 		g_mailmap, resolved_untracked, ARRAY_SIZE(resolved_untracked));
 }
 
+void test_mailmap_parsing__windows_string(void)
+{
+	git_buf unixbuf = GIT_BUF_INIT;
+	git_buf winbuf = GIT_BUF_INIT;
+
+	/* Parse with windows-style line endings */
+	git_buf_attach_notowned(&unixbuf, string_mailmap, strlen(string_mailmap));
+	git_buf_text_lf_to_crlf(&winbuf, &unixbuf);
+
+	cl_git_pass(git_mailmap_from_buffer(&g_mailmap, &winbuf));
+	git_buf_free(winbuf);
+
+	/* We should have parsed all of the entries */
+	check_mailmap_entries(g_mailmap, entries, ARRAY_SIZE(entries));
+
+	/* Check that resolving the entries works */
+	check_mailmap_resolve(g_mailmap, resolved, ARRAY_SIZE(resolved));
+	check_mailmap_resolve(
+		g_mailmap, resolved_untracked, ARRAY_SIZE(resolved_untracked));
+}
+
 void test_mailmap_parsing__fromrepo(void)
 {
 	g_repo = cl_git_sandbox_init("mailmap");