From 5b78fe75b12f0eab80907f1e353c2f50e817e14e Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 5 Jul 2023 22:42:08 +0900 Subject: [PATCH] git_backend: propagate load() error to caller #1794 --- examples/custom-backend/main.rs | 9 ++++----- lib/src/git_backend.rs | 29 ++++++++++++++++++++++++----- lib/src/repo.rs | 11 +++++++---- src/cli_util.rs | 10 +++++----- tests/test_global_opts.rs | 13 ++++++++----- 5 files changed, 48 insertions(+), 24 deletions(-) diff --git a/examples/custom-backend/main.rs b/examples/custom-backend/main.rs index 3ed73b565..d394d3f23 100644 --- a/examples/custom-backend/main.rs +++ b/examples/custom-backend/main.rs @@ -39,7 +39,7 @@ fn create_store_factories() -> StoreFactories { // must match `Backend::name()`. store_factories.add_backend( "jit", - Box::new(|store_path| Box::new(JitBackend::load(store_path))), + Box::new(|store_path| Ok(Box::new(JitBackend::load(store_path)?))), ); store_factories } @@ -80,10 +80,9 @@ impl JitBackend { Ok(JitBackend { inner }) } - fn load(store_path: &Path) -> Self { - JitBackend { - inner: GitBackend::load(store_path), - } + fn load(store_path: &Path) -> BackendResult { + let inner = GitBackend::load(store_path)?; + Ok(JitBackend { inner }) } } diff --git a/lib/src/git_backend.rs b/lib/src/git_backend.rs index d6a77a626..d192ec692 100644 --- a/lib/src/git_backend.rs +++ b/lib/src/git_backend.rs @@ -56,6 +56,20 @@ impl From for BackendError { } } +#[derive(Debug, Error)] +pub enum GitBackendLoadError { + #[error("Failed to open git repository: {0}")] + OpenRepository(#[source] git2::Error), + #[error(transparent)] + Path(#[from] PathError), +} + +impl From for BackendError { + fn from(err: GitBackendLoadError) -> Self { + BackendError::Other(err.to_string()) + } +} + pub struct GitBackend { repo: Mutex, root_commit_id: CommitId, @@ -106,12 +120,17 @@ impl GitBackend { Ok(GitBackend::new(repo, extra_metadata_store)) } - pub fn load(store_path: &Path) -> Self { - let git_repo_path_str = fs::read_to_string(store_path.join("git_target")).unwrap(); - let git_repo_path = store_path.join(git_repo_path_str).canonicalize().unwrap(); - let repo = git2::Repository::open(git_repo_path).unwrap(); + pub fn load(store_path: &Path) -> Result { + let git_repo_path = { + let target_path = store_path.join("git_target"); + let git_repo_path_str = fs::read_to_string(&target_path).context(&target_path)?; + let git_repo_path = store_path.join(git_repo_path_str); + git_repo_path.canonicalize().context(&git_repo_path)? + }; + let repo = + git2::Repository::open(git_repo_path).map_err(GitBackendLoadError::OpenRepository)?; let extra_metadata_store = TableStore::load(store_path.join("extra"), HASH_LENGTH); - GitBackend::new(repo, extra_metadata_store) + Ok(GitBackend::new(repo, extra_metadata_store)) } pub fn git_repo(&self) -> MutexGuard<'_, git2::Repository> { diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 1bba05e3e..1496fb4a6 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -332,7 +332,7 @@ impl Repo for ReadonlyRepo { } } -type BackendFactory = Box Box>; +type BackendFactory = Box BackendResult>>; type OpStoreFactory = Box Box>; type OpHeadsStoreFactory = Box Box>; type IndexStoreFactory = Box Box>; @@ -353,11 +353,11 @@ impl Default for StoreFactories { // Backends factories.add_backend( "local", - Box::new(|store_path| Box::new(LocalBackend::load(store_path))), + Box::new(|store_path| Ok(Box::new(LocalBackend::load(store_path)))), ); factories.add_backend( "git", - Box::new(|store_path| Box::new(GitBackend::load(store_path))), + Box::new(|store_path| Ok(Box::new(GitBackend::load(store_path)?))), ); // OpStores @@ -400,6 +400,9 @@ pub enum StoreLoadError { store: &'static str, source: io::Error, }, + // TODO: might be better to introduce BackendLoadError type + #[error(transparent)] + Backend(#[from] BackendError), } impl StoreFactories { @@ -448,7 +451,7 @@ impl StoreFactories { store_type: backend_type.to_string(), } })?; - Ok(backend_factory(store_path)) + Ok(backend_factory(store_path)?) } pub fn add_op_store(&mut self, name: &str, factory: OpStoreFactory) { diff --git a/src/cli_util.rs b/src/cli_util.rs index 435ccb058..e882465cc 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -1399,11 +1399,11 @@ jj init --git-repo=.", "This version of the jj binary doesn't support this type of repo: {err}" )) } - WorkspaceLoadError::StoreLoadError(err @ StoreLoadError::ReadError { .. }) => { - CommandError::InternalError(format!( - "The repository appears broken or inaccessible: {err}" - )) - } + WorkspaceLoadError::StoreLoadError( + err @ (StoreLoadError::ReadError { .. } | StoreLoadError::Backend(_)), + ) => CommandError::InternalError(format!( + "The repository appears broken or inaccessible: {err}" + )), } } diff --git a/tests/test_global_opts.rs b/tests/test_global_opts.rs index fe48f6800..5c4d997c8 100644 --- a/tests/test_global_opts.rs +++ b/tests/test_global_opts.rs @@ -215,12 +215,15 @@ fn test_broken_repo_structure() { let test_env = TestEnvironment::default(); test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); let repo_path = test_env.env_root().join("repo"); + let store_path = repo_path.join(".jj").join("repo").join("store"); + let store_type_path = store_path.join("type"); - let store_type_path = repo_path - .join(".jj") - .join("repo") - .join("store") - .join("type"); + // Test the error message when the git repository can't be located. + std::fs::remove_file(store_path.join("git_target")).unwrap(); + let stderr = test_env.jj_cmd_internal_error(&repo_path, &["log"]); + insta::assert_snapshot!(stderr, @r###" + Internal error: The repository appears broken or inaccessible: Error: Cannot access $TEST_ENV/repo/.jj/repo/store/git_target + "###); // Test the error message when the commit backend is of unknown type. std::fs::write(&store_type_path, "unknown").unwrap();