diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 35c3f03fd..bdaa195fd 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -44,6 +44,7 @@ use jj_lib::matchers::{EverythingMatcher, Matcher, PrefixMatcher}; use jj_lib::merged_tree::MergedTree; use jj_lib::op_heads_store::{self, OpHeadResolutionError, OpHeadsStore}; use jj_lib::op_store::{OpStore, OpStoreError, OperationId, WorkspaceId}; +use jj_lib::op_walk::{OpsetEvaluationError, OpsetResolutionError}; use jj_lib::operation::Operation; use jj_lib::repo::{ CheckOutCommitError, EditCommitError, MutableRepo, ReadonlyRepo, Repo, RepoLoader, @@ -213,6 +214,16 @@ impl From for CommandError { } } +impl From for CommandError { + fn from(err: OpsetEvaluationError) -> Self { + match err { + OpsetEvaluationError::OpsetResolution(err) => user_error(err.to_string()), + OpsetEvaluationError::OpHeadResolution(err) => err.into(), + OpsetEvaluationError::OpStore(err) => err.into(), + } + } +} + impl From for CommandError { fn from(err: SnapshotError) -> Self { match err { @@ -648,11 +659,12 @@ impl CommandHelper { }, ) } else { - resolve_op_for_load( + let operation = resolve_op_for_load( repo_loader.op_store(), repo_loader.op_heads_store(), &self.global_args.at_operation, - ) + )?; + Ok(operation) } } @@ -962,7 +974,7 @@ impl WorkspaceCommandHelper { git_ignores } - pub fn resolve_single_op(&self, op_str: &str) -> Result { + pub fn resolve_single_op(&self, op_str: &str) -> Result { // When resolving the "@" operation in a `ReadonlyRepo`, we resolve it to the // operation the repo was loaded at. resolve_single_op( @@ -2011,12 +2023,10 @@ pub fn resolve_op_for_load( op_store: &Arc, op_heads_store: &Arc, op_str: &str, -) -> Result { +) -> Result { let get_current_op = || { op_heads_store::resolve_op_heads(op_heads_store.as_ref(), op_store, |_| { - Err(user_error(format!( - r#"The "{op_str}" expression resolved to more than one operation"# - ))) + Err(OpsetResolutionError::MultipleOperations("@".to_owned()).into()) }) }; resolve_single_op(op_store, op_heads_store, get_current_op, op_str) @@ -2025,9 +2035,9 @@ pub fn resolve_op_for_load( fn resolve_single_op( op_store: &Arc, op_heads_store: &Arc, - get_current_op: impl FnOnce() -> Result, + get_current_op: impl FnOnce() -> Result, op_str: &str, -) -> Result { +) -> Result { let op_symbol = op_str.trim_end_matches('-'); let op_postfix = &op_str[op_symbol.len()..]; let mut operation = match op_symbol { @@ -2037,14 +2047,10 @@ fn resolve_single_op( for _ in op_postfix.chars() { let mut parent_ops = operation.parents(); let Some(op) = parent_ops.next().transpose()? else { - return Err(user_error(format!( - r#"The "{op_str}" expression resolved to no operations"# - ))); + return Err(OpsetResolutionError::EmptyOperations(op_str.to_owned()).into()); }; if parent_ops.next().is_some() { - return Err(user_error(format!( - r#"The "{op_str}" expression resolved to more than one operation"# - ))); + return Err(OpsetResolutionError::MultipleOperations(op_str.to_owned()).into()); } drop(parent_ops); operation = op; @@ -2091,11 +2097,9 @@ fn resolve_single_op_from_store( op_store: &Arc, op_heads_store: &Arc, op_str: &str, -) -> Result { +) -> Result { if op_str.is_empty() || !op_str.as_bytes().iter().all(|b| b.is_ascii_hexdigit()) { - return Err(user_error(format!( - "Operation ID \"{op_str}\" is not a valid hexadecimal prefix" - ))); + return Err(OpsetResolutionError::InvalidIdPrefix(op_str.to_owned()).into()); } if let Ok(binary_op_id) = hex::decode(op_str) { let op_id = OperationId::new(binary_op_id); @@ -2107,9 +2111,7 @@ fn resolve_single_op_from_store( // Fall through } Err(err) => { - return Err(CommandError::InternalError(format!( - "Failed to read operation: {err}" - ))); + return Err(OpsetEvaluationError::OpStore(err)); } } } @@ -2120,13 +2122,11 @@ fn resolve_single_op_from_store( } } if matches.is_empty() { - Err(user_error(format!("No operation ID matching \"{op_str}\""))) + Err(OpsetResolutionError::NoSuchOperation(op_str.to_owned()).into()) } else if matches.len() == 1 { Ok(matches.pop().unwrap()) } else { - Err(user_error(format!( - "Operation ID prefix \"{op_str}\" is ambiguous" - ))) + Err(OpsetResolutionError::AmbiguousIdPrefix(op_str.to_owned()).into()) } } diff --git a/cli/tests/test_operations.rs b/cli/tests/test_operations.rs index d9a63871f..84a1ea7a2 100644 --- a/cli/tests/test_operations.rs +++ b/cli/tests/test_operations.rs @@ -110,7 +110,7 @@ fn test_op_log() { ], ); insta::assert_snapshot!(test_env.jj_cmd_failure(&repo_path, &["log", "--at-op", "@-"]), @r###" - Error: The "@-" expression resolved to more than one operation + Error: The "@" expression resolved to more than one operation "###); test_env.jj_cmd_ok(&repo_path, &["st"]); insta::assert_snapshot!(test_env.jj_cmd_failure(&repo_path, &["log", "--at-op", "@-"]), @r###" diff --git a/lib/src/op_walk.rs b/lib/src/op_walk.rs index 1d10ece75..079165119 100644 --- a/lib/src/op_walk.rs +++ b/lib/src/op_walk.rs @@ -17,11 +17,50 @@ use std::cmp::Ordering; use itertools::Itertools as _; +use thiserror::Error; use crate::dag_walk; -use crate::op_store::OpStoreResult; +use crate::op_heads_store::OpHeadResolutionError; +use crate::op_store::{OpStoreError, OpStoreResult}; use crate::operation::Operation; +/// Error that may occur during evaluation of operation set expression. +#[derive(Debug, Error)] +pub enum OpsetEvaluationError { + /// Failed to resolve operation set expression. + #[error(transparent)] + OpsetResolution(#[from] OpsetResolutionError), + /// Failed to resolve the current operation heads. + #[error(transparent)] + OpHeadResolution(#[from] OpHeadResolutionError), + /// Failed to access operation object. + #[error(transparent)] + OpStore(#[from] OpStoreError), +} + +/// Error that may occur during parsing and resolution of operation set +/// expression. +#[derive(Debug, Error)] +pub enum OpsetResolutionError { + // TODO: Maybe empty/multiple operations should be allowed, and rejected by + // caller as needed. + /// Expression resolved to multiple operations. + #[error(r#"The "{0}" expression resolved to more than one operation"#)] + MultipleOperations(String), + /// Expression resolved to no operations. + #[error(r#"The "{0}" expression resolved to no operations"#)] + EmptyOperations(String), + /// Invalid symbol as an operation ID. + #[error(r#"Operation ID "{0}" is not a valid hexadecimal prefix"#)] + InvalidIdPrefix(String), + /// Operation ID not found. + #[error(r#"No operation ID matching "{0}""#)] + NoSuchOperation(String), + /// Operation ID prefix matches multiple operations. + #[error(r#"Operation ID prefix "{0}" is ambiguous"#)] + AmbiguousIdPrefix(String), +} + #[derive(Clone, Debug, Eq, Hash, PartialEq)] struct OperationByEndTime(Operation);