Move loading of Git config into config crate

Author: MitMaro

src/application.rs

@@ -9,9 +9,9 @@ pub(crate) use crate::application::app_data::AppData;
 use crate::{
 	Args,
 	Exit,
-	config::Config,
+	config::{Config, ConfigLoader},
 	display::Display,
-	git::Repository,
+	git::{Repository, open_repository_from_env},
 	help::build_help,
 	input::{Event, EventHandler, EventReaderFn, KeyBindings, StandardEvent},
 	module::{self, ExitStatus, ModuleHandler, State},
@@ -40,7 +40,8 @@ where ModuleProvider: module::ModuleProvider + Send + 'static
 	{
 		let filepath = Self::filepath_from_args(args)?;
 		let repository = Self::open_repository()?;
-		let config = Arc::new(Self::load_config(&repository)?);
+		let config_loader = ConfigLoader::from(repository);
+		let config = Self::load_config(&config_loader)?;
 		let todo_file = Arc::new(Mutex::new(Self::load_todo_file(filepath.as_str(), &config)?));
 
 		let display = Display::new(tui, &config.theme);
@@ -72,8 +73,9 @@ where ModuleProvider: module::ModuleProvider + Send + 'static
 		let search_state = search_threads.state();
 		threads.push(Box::new(search_threads));
 
+		let keybindings = KeyBindings::new(&config.key_bindings);
 		let app_data = AppData::new(
-			Arc::clone(&config),
+			config,
 			State::WindowSizeError,
 			Arc::clone(&todo_file),
 			view_state.clone(),
@@ -82,8 +84,8 @@ where ModuleProvider: module::ModuleProvider + Send + 'static
 		);
 
 		let module_handler = ModuleHandler::new(
-			EventHandler::new(KeyBindings::new(&config.key_bindings)),
-			ModuleProvider::new(repository, &app_data),
+			EventHandler::new(keybindings),
+			ModuleProvider::new(Repository::from(config_loader.eject_repository()), &app_data),
 		);
 		let process = Process::new(&app_data, initial_display_size, module_handler, thread_statuses.clone());
 		let process_threads = process::Thread::new(process.clone());
@@ -135,17 +137,17 @@ where ModuleProvider: module::ModuleProvider + Send + 'static
 		})
 	}
 
-	fn open_repository() -> Result<Repository, Exit> {
-		Repository::open_from_env().map_err(|err| {
+	fn open_repository() -> Result<git2::Repository, Exit> {
+		open_repository_from_env().map_err(|err| {
 			Exit::new(
 				ExitStatus::StateError,
 				format!("Unable to load Git repository: {err}").as_str(),
 			)
 		})
 	}
 
-	fn load_config(repo: &Repository) -> Result<Config, Exit> {
-		Config::try_from(repo).map_err(|err| Exit::new(ExitStatus::ConfigError, format!("{err:#}").as_str()))
+	fn load_config(config_loader: &ConfigLoader) -> Result<Config, Exit> {
+		Config::try_from(config_loader).map_err(|err| Exit::new(ExitStatus::ConfigError, format!("{err:#}").as_str()))
 	}
 
 	fn todo_file_options(config: &Config) -> TodoFileOptions {

src/application/app_data.rs

@@ -16,15 +16,15 @@ pub(crate) struct AppData {
 
 impl AppData {
 	pub(crate) fn new(
-		config: Arc<Config>,
+		config: Config,
 		active_module: module::State,
 		todo_file: Arc<Mutex<TodoFile>>,
 		view_state: view::State,
 		input_state: input::State,
 		search_state: search::State,
 	) -> Self {
 		Self {
-			config,
+			config: Arc::new(config),
 			active_module: Arc::new(Mutex::new(active_module)),
 			todo_file,
 			view_state,

src/config.rs

@@ -8,6 +8,7 @@
 //! these utilities are not tested, and often are optimized for developer experience than
 //! performance should only be used in test code.
 mod color;
+mod config_loader;
 mod diff_ignore_whitespace_setting;
 mod diff_show_whitespace_setting;
 mod errors;
@@ -19,18 +20,16 @@ mod utils;
 use self::utils::{get_bool, get_diff_ignore_whitespace, get_diff_show_whitespace, get_string, get_unsigned_integer};
 pub(crate) use self::{
 	color::Color,
+	config_loader::ConfigLoader,
 	diff_ignore_whitespace_setting::DiffIgnoreWhitespaceSetting,
 	diff_show_whitespace_setting::DiffShowWhitespaceSetting,
 	git_config::GitConfig,
 	key_bindings::KeyBindings,
 	theme::Theme,
 };
-use crate::{
-	config::{
-		errors::{ConfigError, ConfigErrorCause, InvalidColorError},
-		utils::get_optional_string,
-	},
-	git::Repository,
+use crate::config::{
+	errors::{ConfigError, ConfigErrorCause, InvalidColorError},
+	utils::get_optional_string,
 };
 
 const DEFAULT_SPACE_SYMBOL: &str = "\u{b7}"; // ·
@@ -95,16 +94,16 @@ impl Config {
 	}
 }
 
-impl TryFrom<&Repository> for Config {
+impl TryFrom<&ConfigLoader> for Config {
 	type Error = ConfigError;
 
 	/// Creates a new Config instance loading the Git Config using [`crate::git::Repository`].
 	///
 	/// # Errors
 	///
 	/// Will return an `Err` if there is a problem loading the configuration.
-	fn try_from(repo: &Repository) -> Result<Self, Self::Error> {
-		let config = repo
+	fn try_from(config_loader: &ConfigLoader) -> Result<Self, Self::Error> {
+		let config = config_loader
 			.load_config()
 			.map_err(|e| ConfigError::new_read_error("", ConfigErrorCause::GitError(e)))?;
 		Self::new_with_config(Some(&config))
@@ -130,9 +129,10 @@ mod tests {
 	use crate::test_helpers::{invalid_utf, with_git_config, with_temp_bare_repository};
 
 	#[test]
-	fn try_from_repository() {
+	fn try_from_config_loader() {
 		with_temp_bare_repository(|repository| {
-			assert_ok!(Config::try_from(&repository));
+			let loader = ConfigLoader::from(repository);
+			assert_ok!(Config::try_from(&loader));
 		});
 	}
 

src/config/config_loader.rs

@@ -0,0 +1,69 @@
+use std::fmt::{Debug, Formatter};
+
+use git2::Repository;
+
+use crate::git::{Config, GitError};
+
+pub(crate) struct ConfigLoader {
+	repository: Repository,
+}
+
+impl ConfigLoader {
+	/// Load the git configuration for the repository.
+	///
+	/// # Errors
+	/// Will result in an error if the configuration is invalid.
+	pub(crate) fn load_config(&self) -> Result<Config, GitError> {
+		self.repository.config().map_err(|e| GitError::ConfigLoad { cause: e })
+	}
+
+	pub(crate) fn eject_repository(self) -> Repository {
+		self.repository
+	}
+}
+
+impl From<Repository> for ConfigLoader {
+	fn from(repository: Repository) -> Self {
+		Self { repository }
+	}
+}
+
+impl Debug for ConfigLoader {
+	fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), std::fmt::Error> {
+		f.debug_struct("ConfigLoader")
+			.field("[path]", &self.repository.path())
+			.finish()
+	}
+}
+
+// Paths in Windows make these tests difficult, so disable
+#[cfg(all(unix, test))]
+mod unix_tests {
+	use claims::assert_ok;
+
+	use crate::{
+		config::ConfigLoader,
+		test_helpers::{with_temp_bare_repository, with_temp_repository},
+	};
+
+	#[test]
+	fn load_config() {
+		with_temp_bare_repository(|repository| {
+			let config = ConfigLoader::from(repository);
+			assert_ok!(config.load_config());
+		});
+	}
+
+	#[test]
+	fn fmt() {
+		with_temp_repository(|repository| {
+			let path = repository.path().canonicalize().unwrap();
+			let loader = ConfigLoader::from(repository);
+			let formatted = format!("{loader:?}");
+			assert_eq!(
+				formatted,
+				format!("ConfigLoader {{ [path]: \"{}/\" }}", path.to_str().unwrap())
+			);
+		});
+	}
+}

src/git.rs

@@ -45,3 +45,56 @@ pub(crate) use self::{
 	status::Status,
 	user::User,
 };
+
+/// Find and open an existing repository, respecting git environment variables. This will check
+/// for and use `$GIT_DIR`, and if unset will search for a repository starting in the current
+/// directory, walking to the root.
+///
+/// # Errors
+/// Will result in an error if the repository cannot be opened.
+pub(crate) fn open_repository_from_env() -> Result<git2::Repository, GitError> {
+	git2::Repository::open_from_env().map_err(|e| {
+		GitError::RepositoryLoad {
+			kind: RepositoryLoadKind::Environment,
+			cause: e,
+		}
+	})
+}
+
+// Paths in Windows make these tests difficult, so disable
+#[cfg(all(unix, test))]
+mod tests {
+	use claims::assert_ok;
+	use git2::ErrorClass;
+
+	use super::*;
+	use crate::test_helpers::with_git_directory;
+
+	#[test]
+	fn open_repository_from_env_success() {
+		with_git_directory("fixtures/simple", |_| {
+			assert_ok!(open_repository_from_env());
+		});
+	}
+
+	#[test]
+	fn open_repository_from_env_error() {
+		with_git_directory("fixtures/does-not-exist", |path| {
+			match open_repository_from_env() {
+				Ok(_) => {
+					panic!("open_repository_from_env should return error")
+				},
+				Err(err) => {
+					assert_eq!(err, GitError::RepositoryLoad {
+						kind: RepositoryLoadKind::Environment,
+						cause: git2::Error::new(
+							ErrorCode::NotFound,
+							ErrorClass::Os,
+							format!("failed to resolve path '{path}': No such file or directory")
+						),
+					});
+				},
+			}
+		});
+	}
+}

src/git/commit.rs

@@ -177,27 +177,26 @@ mod tests {
 	#[test]
 	fn new_authored_date_same_committed_date() {
 		with_temp_repository(|repository| {
-			create_commit(
-				&repository,
-				Some(CreateCommitOptions::new().author_time(JAN_2021_EPOCH)),
-			);
-			let commit = repository.find_commit("refs/heads/main").unwrap();
+			let repo = crate::git::Repository::from(repository);
+			create_commit(&repo, Some(CreateCommitOptions::new().author_time(JAN_2021_EPOCH)));
+			let commit = repo.find_commit("refs/heads/main").unwrap();
 			assert_none!(commit.authored_date());
 		});
 	}
 
 	#[test]
 	fn new_authored_date_different_than_committed() {
 		with_temp_repository(|repository| {
+			let repo = crate::git::Repository::from(repository);
 			create_commit(
-				&repository,
+				&repo,
 				Some(
 					CreateCommitOptions::new()
 						.commit_time(JAN_2021_EPOCH)
 						.author_time(JAN_2021_EPOCH + 1),
 				),
 			);
-			let commit = repository.find_commit("refs/heads/main").unwrap();
+			let commit = repo.find_commit("refs/heads/main").unwrap();
 			assert_some_eq!(
 				commit.authored_date(),
 				&DateTime::parse_from_rfc3339("2021-01-01T00:00:01Z").unwrap()
@@ -208,8 +207,9 @@ mod tests {
 	#[test]
 	fn new_committer_different_than_author() {
 		with_temp_repository(|repository| {
-			create_commit(&repository, Some(CreateCommitOptions::new().committer("Committer")));
-			let commit = repository.find_commit("refs/heads/main").unwrap();
+			let repo = crate::git::Repository::from(repository);
+			create_commit(&repo, Some(CreateCommitOptions::new().committer("Committer")));
+			let commit = repo.find_commit("refs/heads/main").unwrap();
 			assert_some_eq!(
 				commit.committer(),
 				&User::new(Some("Committer"), Some("[email protected]"))
@@ -220,15 +220,16 @@ mod tests {
 	#[test]
 	fn new_committer_same_as_author() {
 		with_temp_repository(|repository| {
-			let commit = repository.find_commit("refs/heads/main").unwrap();
+			let repo = crate::git::Repository::from(repository);
+			let commit = repo.find_commit("refs/heads/main").unwrap();
 			assert_none!(commit.committer());
 		});
 	}
 
 	#[test]
 	fn try_from_success() {
 		with_temp_repository(|repository| {
-			let reference = repository.repository().find_reference("refs/heads/main").unwrap();
+			let reference = repository.find_reference("refs/heads/main").unwrap();
 			let commit = Commit::try_from(&reference).unwrap();
 
 			assert_eq!(commit.reference.unwrap().shortname(), "main");
@@ -238,11 +239,10 @@ mod tests {
 	#[test]
 	fn try_from_error() {
 		with_temp_repository(|repository| {
-			let repo = repository.repository();
-			let blob = repo.blob(b"foo").unwrap();
-			_ = repo.reference("refs/blob", blob, false, "blob").unwrap();
+			let blob = repository.blob(b"foo").unwrap();
+			_ = repository.reference("refs/blob", blob, false, "blob").unwrap();
 
-			let reference = repo.find_reference("refs/blob").unwrap();
+			let reference = repository.find_reference("refs/blob").unwrap();
 			assert_err_eq!(Commit::try_from(&reference), GitError::CommitLoad {
 				cause: git2::Error::new(
 					git2::ErrorCode::InvalidSpec,