From 70d3c64b1eabf061eef81f4632ad2790a1aa7922 Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Tue, 25 Jul 2023 09:40:12 -0500 Subject: [PATCH] operation: propagate `OpStoreError` This is a rote propagation of the error value to hopefully improve the panic in https://github.com/martinvonz/jj/issues/1907. --- lib/src/default_index_store.rs | 16 ++++++++++++---- lib/src/op_heads_store.rs | 8 +++++--- lib/src/operation.rs | 9 +++++---- lib/src/repo.rs | 30 +++++++++++++++++++----------- lib/src/transaction.rs | 9 +++++---- lib/tests/test_load_repo.rs | 2 +- src/cli_util.rs | 28 +++++++++++++++++++++------- src/commands/debug.rs | 4 ++-- src/commands/operation.rs | 6 +++--- 9 files changed, 73 insertions(+), 39 deletions(-) diff --git a/lib/src/default_index_store.rs b/lib/src/default_index_store.rs index c434ca20d..c6ecc326a 100644 --- a/lib/src/default_index_store.rs +++ b/lib/src/default_index_store.rs @@ -40,12 +40,20 @@ use crate::file_util::persist_content_addressed_temp_file; use crate::index::{ HexPrefix, Index, IndexStore, IndexWriteError, MutableIndex, PrefixResolution, ReadonlyIndex, }; -use crate::op_store::OperationId; +use crate::op_store::{OpStoreError, OperationId}; use crate::operation::Operation; use crate::revset::{ResolvedExpression, Revset, RevsetEvaluationError}; use crate::store::Store; use crate::{backend, dag_walk, default_revset_engine}; +#[derive(Debug, Error)] +pub enum DefaultIndexStoreError { + #[error(transparent)] + Io(#[from] io::Error), + #[error(transparent)] + OpStore(#[from] OpStoreError), +} + #[derive(Debug)] pub struct DefaultIndexStore { dir: PathBuf, @@ -99,8 +107,8 @@ impl DefaultIndexStore { &self, store: &Arc, operation: &Operation, - ) -> io::Result> { - let view = operation.view(); + ) -> Result, DefaultIndexStoreError> { + let view = operation.view()?; let operations_dir = self.dir.join("operations"); let commit_id_length = store.commit_id_length(); let change_id_length = store.change_id_length(); @@ -116,7 +124,7 @@ impl DefaultIndexStore { parent_op_id = Some(op.id().clone()) } } else { - for head in op.view().heads() { + for head in op.view()?.heads() { new_heads.insert(head.clone()); } } diff --git a/lib/src/op_heads_store.rs b/lib/src/op_heads_store.rs index 46f10b65b..9aef7ce87 100644 --- a/lib/src/op_heads_store.rs +++ b/lib/src/op_heads_store.rs @@ -22,15 +22,17 @@ use itertools::Itertools; use thiserror::Error; use crate::dag_walk; -use crate::op_store::{OpStore, OperationId}; +use crate::op_store::{OpStore, OpStoreError, OperationId}; use crate::operation::Operation; -#[derive(Debug, Error, PartialEq, Eq)] +#[derive(Debug, Error)] pub enum OpHeadResolutionError { #[error("Operation log has no heads")] NoHeads, + #[error(transparent)] + OpStore(#[from] OpStoreError), #[error("Op resolution error: {0}")] - Err(#[from] E), + Err(#[source] E), } pub trait OpHeadsStoreLock<'a> { diff --git a/lib/src/operation.rs b/lib/src/operation.rs index 5b844f3ca..c9c3cb1c5 100644 --- a/lib/src/operation.rs +++ b/lib/src/operation.rs @@ -21,7 +21,7 @@ use std::hash::{Hash, Hasher}; use std::sync::Arc; use crate::backend::CommitId; -use crate::op_store::{OpStore, OperationId, ViewId}; +use crate::op_store::{OpStore, OpStoreResult, OperationId, ViewId}; use crate::{dag_walk, op_store}; #[derive(Clone)] @@ -93,9 +93,10 @@ impl Operation { parents } - pub fn view(&self) -> View { - let data = self.op_store.read_view(&self.data.view_id).unwrap(); - View::new(self.op_store.clone(), self.data.view_id.clone(), data) + pub fn view(&self) -> OpStoreResult { + let data = self.op_store.read_view(&self.data.view_id)?; + let view = View::new(self.op_store.clone(), self.data.view_id.clone(), data); + Ok(view) } pub fn store_operation(&self) -> &op_store::Operation { diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 649d2ad72..5ddf8967e 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -41,7 +41,7 @@ use crate::git_backend::GitBackend; use crate::index::{HexPrefix, Index, IndexStore, MutableIndex, PrefixResolution, ReadonlyIndex}; use crate::local_backend::LocalBackend; use crate::op_heads_store::{self, OpHeadResolutionError, OpHeadsStore}; -use crate::op_store::{BranchTarget, OpStore, OperationId, RefTarget, WorkspaceId}; +use crate::op_store::{BranchTarget, OpStore, OpStoreError, OperationId, RefTarget, WorkspaceId}; use crate::operation::Operation; use crate::refs::merge_ref_targets; use crate::revset::{self, ChangeIdIndex, Revset, RevsetExpression}; @@ -298,11 +298,11 @@ impl ReadonlyRepo { pub fn reload_at_head( &self, user_settings: &UserSettings, - ) -> Result, OpHeadResolutionError> { + ) -> Result, OpHeadResolutionError> { self.loader().load_at_head(user_settings) } - pub fn reload_at(&self, operation: &Operation) -> Arc { + pub fn reload_at(&self, operation: &Operation) -> Result, RepoLoaderError> { self.loader().load_at(operation) } } @@ -558,6 +558,14 @@ fn read_store_type_compat( .map_err(|source| StoreLoadError::ReadError { store, source }) } +#[derive(Debug, Error)] +pub enum RepoLoaderError { + #[error(transparent)] + TreeMerge(#[from] TreeMergeError), + #[error(transparent)] + OpStore(#[from] OpStoreError), +} + #[derive(Clone)] pub struct RepoLoader { repo_path: PathBuf, @@ -617,19 +625,19 @@ impl RepoLoader { pub fn load_at_head( &self, user_settings: &UserSettings, - ) -> Result, OpHeadResolutionError> { + ) -> Result, OpHeadResolutionError> { let op = op_heads_store::resolve_op_heads( self.op_heads_store.as_ref(), &self.op_store, |op_heads| self._resolve_op_heads(op_heads, user_settings), )?; - let view = View::new(op.view().take_store_view()); + let view = View::new(op.view()?.take_store_view()); Ok(self._finish_load(op, view)) } - pub fn load_at(&self, op: &Operation) -> Arc { - let view = View::new(op.view().take_store_view()); - self._finish_load(op.clone(), view) + pub fn load_at(&self, op: &Operation) -> Result, RepoLoaderError> { + let view = View::new(op.view()?.take_store_view()); + Ok(self._finish_load(op.clone(), view)) } pub fn create_from( @@ -658,11 +666,11 @@ impl RepoLoader { &self, op_heads: Vec, user_settings: &UserSettings, - ) -> Result { - let base_repo = self.load_at(&op_heads[0]); + ) -> Result { + let base_repo = self.load_at(&op_heads[0])?; let mut tx = base_repo.start_transaction(user_settings, "resolve concurrent operations"); for other_op_head in op_heads.into_iter().skip(1) { - tx.merge_operation(other_op_head); + tx.merge_operation(other_op_head)?; tx.mut_repo().rebase_descendants(user_settings)?; } let merged_repo = tx.write().leave_unpublished(); diff --git a/lib/src/transaction.rs b/lib/src/transaction.rs index 1888f326f..a10359fa0 100644 --- a/lib/src/transaction.rs +++ b/lib/src/transaction.rs @@ -22,7 +22,7 @@ use crate::index::ReadonlyIndex; use crate::op_store; use crate::op_store::OperationMetadata; use crate::operation::Operation; -use crate::repo::{MutableRepo, ReadonlyRepo, Repo, RepoLoader}; +use crate::repo::{MutableRepo, ReadonlyRepo, Repo, RepoLoader, RepoLoaderError}; use crate::settings::UserSettings; use crate::view::View; @@ -70,7 +70,7 @@ impl Transaction { &mut self.mut_repo } - pub fn merge_operation(&mut self, other_op: Operation) { + pub fn merge_operation(&mut self, other_op: Operation) -> Result<(), RepoLoaderError> { let ancestor_op = closest_common_node( self.parent_ops.clone(), vec![other_op.clone()], @@ -79,11 +79,12 @@ impl Transaction { ) .unwrap(); let repo_loader = self.base_repo().loader(); - let base_repo = repo_loader.load_at(&ancestor_op); - let other_repo = repo_loader.load_at(&other_op); + let base_repo = repo_loader.load_at(&ancestor_op)?; + let other_repo = repo_loader.load_at(&other_op)?; self.parent_ops.push(other_op); let merged_repo = self.mut_repo(); merged_repo.merge(&base_repo, &other_repo); + Ok(()) } /// Writes the transaction to the operation store and publishes it. diff --git a/lib/tests/test_load_repo.rs b/lib/tests/test_load_repo.rs index 773bf5a02..c9acc7852 100644 --- a/lib/tests/test_load_repo.rs +++ b/lib/tests/test_load_repo.rs @@ -40,6 +40,6 @@ fn test_load_at_operation(use_git: bool) { // If we load the repo at the previous operation, we should see the commit since // it has not been removed yet let loader = RepoLoader::init(&settings, repo.repo_path(), &StoreFactories::default()).unwrap(); - let old_repo = loader.load_at(repo.operation()); + let old_repo = loader.load_at(repo.operation()).unwrap(); assert!(old_repo.view().heads().contains(commit.id())); } diff --git a/src/cli_util.rs b/src/cli_util.rs index f9e221441..8cc5a7d75 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -43,7 +43,7 @@ use jj_lib::op_store::{OpStore, OpStoreError, OperationId, RefTarget, WorkspaceI use jj_lib::operation::Operation; use jj_lib::repo::{ CheckOutCommitError, EditCommitError, MutableRepo, ReadonlyRepo, Repo, RepoLoader, - RewriteRootCommit, StoreFactories, StoreLoadError, + RepoLoaderError, RewriteRootCommit, StoreFactories, StoreLoadError, }; use jj_lib::repo_path::{FsPathParseError, RepoPath}; use jj_lib::revset::{ @@ -196,6 +196,7 @@ impl From> for CommandError { OpHeadResolutionError::NoHeads => CommandError::InternalError( "Corrupt repository: there are no operations".to_string(), ), + OpHeadResolutionError::OpStore(err) => err.into(), OpHeadResolutionError::Err(e) => e, } } @@ -213,6 +214,18 @@ impl From for CommandError { } } +impl From for CommandError { + fn from(err: OpStoreError) -> Self { + CommandError::InternalError(format!("Failed to load an operation: {err}")) + } +} + +impl From for CommandError { + fn from(err: RepoLoaderError) -> Self { + CommandError::InternalError(format!("Failed to load the repo: {err}")) + } +} + impl From for CommandError { fn from(_: ResetError) -> Self { CommandError::InternalError("Failed to reset the working copy".to_string()) @@ -514,7 +527,7 @@ impl CommandHelper { ) -> Result { let workspace = self.load_workspace()?; let op_head = self.resolve_operation(ui, workspace.repo_loader())?; - let repo = workspace.repo_loader().load_at(&op_head); + let repo = workspace.repo_loader().load_at(&op_head)?; let mut workspace_command = self.for_loaded_repo(ui, workspace, repo)?; if snapshot { workspace_command.snapshot(ui)?; @@ -554,7 +567,7 @@ impl CommandHelper { ui, "Concurrent modification detected, resolving automatically.", )?; - let base_repo = repo_loader.load_at(&op_heads[0]); + let base_repo = repo_loader.load_at(&op_heads[0])?; // TODO: It may be helpful to print each operation we're merging here let mut tx = start_repo_transaction( &base_repo, @@ -563,7 +576,7 @@ impl CommandHelper { "resolve concurrent operations", ); for other_op_head in op_heads.into_iter().skip(1) { - tx.merge_operation(other_op_head); + tx.merge_operation(other_op_head)?; let num_rebased = tx.mut_repo().rebase_descendants(&self.settings)?; if num_rebased > 0 { writeln!( @@ -606,7 +619,7 @@ impl CommandHelper { .read_operation(op_id) .map_err(|e| CommandError::InternalError(format!("Failed to read operation: {e}")))?; let operation = Operation::new(op_store.clone(), op_id.clone(), op_data); - let repo = workspace.repo_loader().load_at(&operation); + let repo = workspace.repo_loader().load_at(&operation)?; self.for_loaded_repo(ui, workspace, repo) } } @@ -1107,7 +1120,7 @@ impl WorkspaceCommandHelper { let (repo, wc_commit) = match check_stale_working_copy(&locked_wc, &wc_commit, &repo) { Ok(None) => (repo, wc_commit), Ok(Some(wc_operation)) => { - let repo = repo.reload_at(&wc_operation); + let repo = repo.reload_at(&wc_operation)?; let wc_commit = if let Some(wc_commit) = get_wc_commit(&repo)? { wc_commit } else { @@ -1614,7 +1627,8 @@ fn resolve_op_for_load( ))) }) }; - let operation = resolve_single_op(op_store, op_heads_store, get_current_op, op_str)?; + let operation = resolve_single_op(op_store, op_heads_store, get_current_op, op_str) + .map_err(OpHeadResolutionError::Err)?; Ok(operation) } diff --git a/src/commands/debug.rs b/src/commands/debug.rs index 7c3732540..f5b6a7e41 100644 --- a/src/commands/debug.rs +++ b/src/commands/debug.rs @@ -148,7 +148,7 @@ pub fn cmd_debug( repo.index_store().as_any().downcast_ref(); if let Some(default_index_store) = default_index_store { default_index_store.reinit(); - let repo = repo.reload_at(repo.operation()); + let repo = repo.reload_at(repo.operation())?; let index_impl: &ReadonlyIndexWrapper = repo .readonly_index() .as_any() @@ -177,7 +177,7 @@ pub fn cmd_debug( writeln!(ui, "{:#?}", op.store_operation())?; } if operation_args.display != DebugOperationDisplay::Operation { - writeln!(ui, "{:#?}", op.view().store_view())?; + writeln!(ui, "{:#?}", op.view()?.store_view())?; } } DebugCommands::Watchman(watchman_subcommand) => { diff --git a/src/commands/operation.rs b/src/commands/operation.rs index 015d098bf..4396aade0 100644 --- a/src/commands/operation.rs +++ b/src/commands/operation.rs @@ -260,8 +260,8 @@ pub fn cmd_op_undo( let mut tx = workspace_command.start_transaction(&format!("undo operation {}", bad_op.id().hex())); let repo_loader = tx.base_repo().loader(); - let bad_repo = repo_loader.load_at(&bad_op); - let parent_repo = repo_loader.load_at(&parent_ops[0]); + let bad_repo = repo_loader.load_at(&bad_op)?; + let parent_repo = repo_loader.load_at(&parent_ops[0])?; tx.mut_repo().merge(&bad_repo, &parent_repo); let new_view = view_with_desired_portions_restored( tx.repo().view().store_view(), @@ -285,7 +285,7 @@ fn cmd_op_restore( let mut tx = workspace_command .start_transaction(&format!("restore to operation {}", target_op.id().hex())); let new_view = view_with_desired_portions_restored( - target_op.view().store_view(), + target_op.view()?.store_view(), tx.base_repo().view().store_view(), &process_what_arg(&args.what, repo_is_colocated), );