Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect .mailmap #2485

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

acuteenvy
Copy link

This Pull Request fixes/closes #2406.

It changes the following:

  • Make gitui respect .mailmap, just like the Git CLI
  • Add a unit test for it

I followed the checklist:

  • I added unittests
  • I ran make check without errors
  • I tested the overall application
  • I added an appropriate item to the changelog

@Joshix-1
Copy link
Contributor

Joshix-1 commented Jan 16, 2025

This is mostly pretty good.

But it fails if there is a commit with an empty email: env GIT_AUTHOR_EMAIL= git commit -m test

I would fallback to the original name if getting the name with the mailmap fails.

diff --git a/asyncgit/src/sync/commit_details.rs b/asyncgit/src/sync/commit_details.rs
index 85b7250..2203c7f 100644
--- a/asyncgit/src/sync/commit_details.rs
+++ b/asyncgit/src/sync/commit_details.rs
@@ -87,6 +87,24 @@ impl CommitDetails {
 	}
 }
 
+/// Get the author of a commit
+pub fn get_author_of_commit<'a>(
+	commit: &'a git2::Commit<'a>,
+	mailmap: &git2::Mailmap,
+) -> git2::Signature<'a> {
+	match commit.author_with_mailmap(mailmap) {
+		Ok(author) => author,
+		Err(err) => {
+			log::error!(
+				"Couldn't get author with mailmap for {} {:?}: {err}",
+				commit.id(),
+				commit.message(),
+			);
+			commit.author()
+		}
+	}
+}
+
 ///
 pub fn get_commit_details(
 	repo_path: &RepoPath,
@@ -99,8 +117,9 @@ pub fn get_commit_details(
 
 	let commit = repo.find_commit(id.into())?;
 
-	let author =
-		CommitSignature::from(&commit.author_with_mailmap(&mailmap)?);
+	let author = CommitSignature::from(&get_author_of_commit(
+		&commit, &mailmap,
+	));
 	let committer = CommitSignature::from(
 		&commit.committer_with_mailmap(&mailmap)?,
 	);
diff --git a/asyncgit/src/sync/commit_filter.rs b/asyncgit/src/sync/commit_filter.rs
index 10b02fc..b1f2f26 100644
--- a/asyncgit/src/sync/commit_filter.rs
+++ b/asyncgit/src/sync/commit_filter.rs
@@ -1,4 +1,7 @@
-use super::{commit_files::get_commit_diff, CommitId};
+use super::{
+	commit_details::get_author_of_commit,
+	commit_files::get_commit_diff, CommitId,
+};
 use crate::error::Result;
 use bitflags::bitflags;
 use fuzzy_matcher::FuzzyMatcher;
@@ -200,14 +203,18 @@ pub fn filter_commit_by_search(
 				.fields
 				.contains(SearchFields::AUTHORS)
 				.then(|| -> Result<bool> {
-					let name_match = commit
-						.author_with_mailmap(&mailmap)?
-						.name()
-						.is_some_and(|name| filter.match_text(name));
-					let mail_match = commit
-						.author_with_mailmap(&mailmap)?
-						.email()
-						.is_some_and(|name| filter.match_text(name));
+					let name_match =
+						get_author_of_commit(&commit, &mailmap)
+							.name()
+							.is_some_and(|name| {
+								filter.match_text(name)
+							});
+					let mail_match =
+						get_author_of_commit(&commit, &mailmap)
+							.email()
+							.is_some_and(|name| {
+								filter.match_text(name)
+							});
 
 					Ok(name_match || mail_match)
 				})
diff --git a/asyncgit/src/sync/commits_info.rs b/asyncgit/src/sync/commits_info.rs
index 9ee891a..4d1f490 100644
--- a/asyncgit/src/sync/commits_info.rs
+++ b/asyncgit/src/sync/commits_info.rs
@@ -1,7 +1,10 @@
 use std::fmt::Display;
 
 use super::RepoPath;
-use crate::{error::Result, sync::repository::repo};
+use crate::{
+	error::Result,
+	sync::{commit_details::get_author_of_commit, repository::repo},
+};
 use git2::{Commit, Error, Oid};
 use scopetime::scope_time;
 use unicode_truncate::UnicodeTruncateStr;
@@ -102,8 +105,9 @@ pub fn get_commits_info(
 	let res = commits
 		.map(|c: Commit| {
 			let message = get_message(&c, Some(message_length_limit));
-			let author =
-				c.author_with_mailmap(&mailmap)?.name().map_or_else(
+			let author = get_author_of_commit(&c, &mailmap)
+				.name()
+				.map_or_else(
 					|| String::from("<unknown>"),
 					String::from,
 				);
@@ -130,7 +134,7 @@ pub fn get_commit_info(
 	let mailmap = repo.mailmap()?;
 
 	let commit = repo.find_commit((*commit_id).into())?;
-	let author = commit.author_with_mailmap(&mailmap)?;
+	let author = get_author_of_commit(&commit, &mailmap);
 
 	Ok(CommitInfo {
 		message: commit.message().unwrap_or("").into(),

@acuteenvy
Copy link
Author

Thanks, it also failed when the committer email was empty, now I made it so that it also falls back to the original.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Respect .mailmap
2 participants