improve error message when missing ops head

When reading the operation head directory fails, the thread panics and
does not return a useful error message to the user. Create
OpHeadStoreError to prevent thread panic and print a more useful error
message

cli/src/command_error.rs

@@ -22,7 +22,7 @@ use jj_lib::backend::BackendError;
 use jj_lib::fileset::{FilePatternParseError, FilesetParseError, FilesetParseErrorKind};
 use jj_lib::git::{GitConfigParseError, GitExportError, GitImportError, GitRemoteManagementError};
 use jj_lib::gitignore::GitIgnoreError;
-use jj_lib::op_heads_store::OpHeadResolutionError;
+use jj_lib::op_heads_store::{OpHeadResolutionError, OpHeadStoreError};
 use jj_lib::op_store::OpStoreError;
 use jj_lib::op_walk::OpsetEvaluationError;
 use jj_lib::repo::{CheckOutCommitError, EditCommitError, RepoLoaderError, RewriteRootCommit};
@@ -288,10 +288,17 @@ impl From<OpsetEvaluationError> for CommandError {
             OpsetEvaluationError::OpsetResolution(err) => user_error(err),
             OpsetEvaluationError::OpHeadResolution(err) => err.into(),
             OpsetEvaluationError::OpStore(err) => err.into(),
+            OpsetEvaluationError::OpHeadsStore(err) => err.into(),
         }
     }
 }
 
+impl From<OpHeadStoreError> for CommandError {
+    fn from(err: OpHeadStoreError) -> Self {
+        internal_error_with_message("Failed to load the set of operation heads", err)
+    }
+}
+
 impl From<SnapshotError> for CommandError {
     fn from(err: SnapshotError) -> Self {
         match err {

lib/src/op_heads_store.rs

@@ -23,6 +23,7 @@ use itertools::Itertools;
 use thiserror::Error;
 
 use crate::dag_walk;
+use crate::file_util::PathError;
 use crate::op_store::{OpStore, OpStoreError, OperationId};
 use crate::operation::Operation;
 
@@ -32,6 +33,18 @@ pub enum OpHeadResolutionError {
     NoHeads,
 }
 
+#[derive(Debug, Error)]
+#[error(transparent)]
+pub struct OpHeadStoreError(pub Box<dyn std::error::Error + Send + Sync>);
+
+pub type OpHeadStoreResult<T> = Result<T, OpHeadStoreError>;
+
+impl From<PathError> for OpHeadStoreError {
+    fn from(err: PathError) -> Self {
+        OpHeadStoreError(err.into())
+    }
+}
+
 pub trait OpHeadsStoreLock {}
 
 /// Manages the set of current heads of the operation log.
@@ -45,7 +58,7 @@ pub trait OpHeadsStore: Send + Sync + Debug {
     /// The old op heads must not contain the new one.
     fn update_op_heads(&self, old_ids: &[OperationId], new_id: &OperationId);
 
-    fn get_op_heads(&self) -> Vec<OperationId>;
+    fn get_op_heads(&self) -> OpHeadStoreResult<Vec<OperationId>>;
 
     /// Optionally takes a lock on the op heads store. The purpose of the lock
     /// is to prevent concurrent processes from resolving the same divergent
@@ -64,9 +77,9 @@ pub fn resolve_op_heads<E>(
     resolver: impl FnOnce(Vec<Operation>) -> Result<Operation, E>,
 ) -> Result<Operation, E>
 where
-    E: From<OpHeadResolutionError> + From<OpStoreError>,
+    E: From<OpHeadResolutionError> + From<OpStoreError> + From<OpHeadStoreError>,
 {
-    let mut op_heads = op_heads_store.get_op_heads();
+    let mut op_heads = op_heads_store.get_op_heads()?;
 
     // TODO: De-duplicate this 'simple-resolution' code.
     if op_heads.is_empty() {
@@ -88,7 +101,7 @@ where
     // only to prevent other concurrent processes from doing the same work (and
     // producing another set of divergent heads).
     let _lock = op_heads_store.lock();
-    let op_head_ids = op_heads_store.get_op_heads();
+    let op_head_ids = op_heads_store.get_op_heads()?;
 
     if op_head_ids.is_empty() {
         return Err(OpHeadResolutionError::NoHeads.into());

lib/src/op_store.rs

@@ -28,6 +28,7 @@ use crate::backend::{CommitId, MillisSinceEpoch, Timestamp};
 use crate::content_hash::ContentHash;
 use crate::merge::Merge;
 use crate::object_id::{id_type, HexPrefix, ObjectId, PrefixResolution};
+use crate::op_heads_store::OpHeadStoreError;
 
 #[derive(ContentHash, PartialEq, Eq, PartialOrd, Ord, Clone, Hash)]
 pub struct WorkspaceId(String);
@@ -417,6 +418,12 @@ pub enum OpStoreError {
 
 pub type OpStoreResult<T> = Result<T, OpStoreError>;
 
+impl From<OpHeadStoreError> for OpStoreError {
+    fn from(err: OpHeadStoreError) -> Self {
+        OpStoreError::Other(err.into())
+    }
+}
+
 pub trait OpStore: Send + Sync + Debug {
     fn as_any(&self) -> &dyn Any;
 

lib/src/op_walk.rs

@@ -14,6 +14,8 @@
 
 //! Utility for operation id resolution and traversal.
 
+#![allow(missing_docs)]
+
 use std::cmp::Ordering;
 use std::collections::{HashMap, HashSet};
 use std::slice;
@@ -23,7 +25,7 @@ use itertools::Itertools as _;
 use thiserror::Error;
 
 use crate::object_id::{HexPrefix, PrefixResolution};
-use crate::op_heads_store::{OpHeadResolutionError, OpHeadsStore};
+use crate::op_heads_store::{OpHeadResolutionError, OpHeadStoreError, OpHeadsStore};
 use crate::op_store::{OpStore, OpStoreError, OpStoreResult, OperationId};
 use crate::operation::Operation;
 use crate::repo::{ReadonlyRepo, Repo as _, RepoLoader};
@@ -41,8 +43,13 @@ pub enum OpsetEvaluationError {
     /// Failed to access operation object.
     #[error(transparent)]
     OpStore(#[from] OpStoreError),
+    /// Failed to access operation head.
+    #[error(transparent)]
+    OpHeadsStore(#[from] OpHeadStoreError),
 }
 
+pub type OpsetEvaluationResult<T> = Result<T, OpsetEvaluationError>;
+
 /// Error that may occur during parsing and resolution of operation set
 /// expression.
 #[derive(Debug, Error)]
@@ -70,7 +77,7 @@ pub enum OpsetResolutionError {
 pub fn resolve_op_for_load(
     repo_loader: &RepoLoader,
     op_str: &str,
-) -> Result<Operation, OpsetEvaluationError> {
+) -> OpsetEvaluationResult<Operation> {
     let op_store = repo_loader.op_store();
     let op_heads_store = repo_loader.op_heads_store().as_ref();
     let get_current_op = || {
@@ -85,10 +92,7 @@ pub fn resolve_op_for_load(
 /// Resolves operation set expression against the loaded repo.
 ///
 /// The "@" symbol will be resolved to the operation the repo was loaded at.
-pub fn resolve_op_with_repo(
-    repo: &ReadonlyRepo,
-    op_str: &str,
-) -> Result<Operation, OpsetEvaluationError> {
+pub fn resolve_op_with_repo(repo: &ReadonlyRepo, op_str: &str) -> OpsetEvaluationResult<Operation> {
     resolve_op_at(repo.op_store(), repo.operation(), op_str)
 }
 
@@ -97,7 +101,7 @@ pub fn resolve_op_at(
     op_store: &Arc<dyn OpStore>,
     head_op: &Operation,
     op_str: &str,
-) -> Result<Operation, OpsetEvaluationError> {
+) -> OpsetEvaluationResult<Operation> {
     let get_current_op = || Ok(head_op.clone());
     let get_head_ops = || Ok(vec![head_op.clone()]);
     resolve_single_op(op_store, get_current_op, get_head_ops, op_str)
@@ -107,10 +111,10 @@ pub fn resolve_op_at(
 /// callbacks.
 fn resolve_single_op(
     op_store: &Arc<dyn OpStore>,
-    get_current_op: impl FnOnce() -> Result<Operation, OpsetEvaluationError>,
-    get_head_ops: impl FnOnce() -> OpStoreResult<Vec<Operation>>,
+    get_current_op: impl FnOnce() -> OpsetEvaluationResult<Operation>,
+    get_head_ops: impl FnOnce() -> OpsetEvaluationResult<Vec<Operation>>,
     op_str: &str,
-) -> Result<Operation, OpsetEvaluationError> {
+) -> OpsetEvaluationResult<Operation> {
     let op_symbol = op_str.trim_end_matches(['-', '+']);
     let op_postfix = &op_str[op_symbol.len()..];
     let head_ops = op_postfix.contains('+').then(get_head_ops).transpose()?;
@@ -136,7 +140,7 @@ fn resolve_single_op(
 fn resolve_single_op_from_store(
     op_store: &Arc<dyn OpStore>,
     op_str: &str,
-) -> Result<Operation, OpsetEvaluationError> {
+) -> OpsetEvaluationResult<Operation> {
     if op_str.is_empty() {
         return Err(OpsetResolutionError::InvalidIdPrefix(op_str.to_owned()).into());
     }
@@ -161,11 +165,11 @@ fn resolve_single_op_from_store(
 pub fn get_current_head_ops(
     op_store: &Arc<dyn OpStore>,
     op_heads_store: &dyn OpHeadsStore,
-) -> OpStoreResult<Vec<Operation>> {
+) -> OpsetEvaluationResult<Vec<Operation>> {
     op_heads_store
-        .get_op_heads()
+        .get_op_heads()?
         .into_iter()
-        .map(|id| -> OpStoreResult<Operation> {
+        .map(|id| -> OpsetEvaluationResult<Operation> {
             let data = op_store.read_operation(&id)?;
             Ok(Operation::new(op_store.clone(), id, data))
         })

lib/src/repo.rs

@@ -40,7 +40,7 @@ use crate::file_util::{IoResultExt as _, PathError};
 use crate::index::{ChangeIdIndex, Index, IndexStore, MutableIndex, ReadonlyIndex};
 use crate::local_backend::LocalBackend;
 use crate::object_id::{HexPrefix, ObjectId, PrefixResolution};
-use crate::op_heads_store::{self, OpHeadResolutionError, OpHeadsStore};
+use crate::op_heads_store::{self, OpHeadResolutionError, OpHeadStoreError, OpHeadsStore};
 use crate::op_store::{
     OpStore, OpStoreError, OperationId, RefTarget, RemoteRef, RemoteRefState, WorkspaceId,
 };
@@ -591,6 +591,8 @@ pub enum RepoLoaderError {
     #[error(transparent)]
     OpHeadResolution(#[from] OpHeadResolutionError),
     #[error(transparent)]
+    OpHeadStore(#[from] OpHeadStoreError),
+    #[error(transparent)]
     OpStore(#[from] OpStoreError),
 }
 

lib/src/simple_op_heads_store.rs

@@ -19,9 +19,10 @@ use std::fmt::{Debug, Formatter};
 use std::fs;
 use std::path::{Path, PathBuf};
 
+use crate::file_util::PathError;
 use crate::lock::FileLock;
 use crate::object_id::ObjectId;
-use crate::op_heads_store::{OpHeadsStore, OpHeadsStoreLock};
+use crate::op_heads_store::{OpHeadStoreError, OpHeadStoreResult, OpHeadsStore, OpHeadsStoreLock};
 use crate::op_store::OperationId;
 
 pub struct SimpleOpHeadsStore {
@@ -88,16 +89,20 @@ impl OpHeadsStore for SimpleOpHeadsStore {
         }
     }
 
-    fn get_op_heads(&self) -> Vec<OperationId> {
+    fn get_op_heads(&self) -> OpHeadStoreResult<Vec<OperationId>> {
         let mut op_heads = vec![];
-        for op_head_entry in std::fs::read_dir(&self.dir).unwrap() {
-            let op_head_file_name = op_head_entry.unwrap().file_name();
+        for op_head_entry in std::fs::read_dir(&self.dir)
+            .map_err(|err| io_to_op_head_store_error(err, self.dir.clone()))?
+        {
+            let op_head_file_name = op_head_entry
+                .map_err(|err| io_to_op_head_store_error(err, self.dir.clone()))?
+                .file_name();
             let op_head_file_name = op_head_file_name.to_str().unwrap();
             if let Ok(op_head) = hex::decode(op_head_file_name) {
                 op_heads.push(OperationId::new(op_head));
             }
         }
-        op_heads
+        Ok(op_heads)
     }
 
     fn lock(&self) -> Box<dyn OpHeadsStoreLock + '_> {
@@ -106,3 +111,8 @@ impl OpHeadsStore for SimpleOpHeadsStore {
         })
     }
 }
+
+fn io_to_op_head_store_error(error: std::io::Error, path: PathBuf) -> OpHeadStoreError {
+    let err = PathError { path, error };
+    err.into()
+}