diff --git a/cli/src/command_error.rs b/cli/src/command_error.rs index f5a34447f..4e0de94c1 100644 --- a/cli/src/command_error.rs +++ b/cli/src/command_error.rs @@ -411,6 +411,8 @@ impl From for CommandError { impl From for CommandError { fn from(err: FsPathParseError) -> Self { + // TODO: implement pattern prefix like "root:" or "--cwd" option, + // and suggest it if the user input looks like repo-relative path #3216. user_error(err) } } diff --git a/cli/tests/test_global_opts.rs b/cli/tests/test_global_opts.rs index 2cdea5574..ff2278950 100644 --- a/cli/tests/test_global_opts.rs +++ b/cli/tests/test_global_opts.rs @@ -225,6 +225,25 @@ fn test_no_workspace_directory() { "###); } +#[test] +fn test_bad_path() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["cat", "../out"]); + insta::assert_snapshot!(stderr.replace('\\', "/"), @r###" + Error: Path "../out" is not in the repo "." + Caused by: Invalid component ".." in repo-relative path "../out" + "###); + + let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["cat", "-Rrepo", "out"]); + insta::assert_snapshot!(stderr.replace('\\', "/"), @r###" + Error: Path "out" is not in the repo "repo" + Caused by: Invalid component ".." in repo-relative path "../out" + "###); +} + #[test] fn test_broken_repo_structure() { let test_env = TestEnvironment::default(); diff --git a/cli/tests/test_revset_output.rs b/cli/tests/test_revset_output.rs index 7d6bd19eb..de836cc3d 100644 --- a/cli/tests/test_revset_output.rs +++ b/cli/tests/test_revset_output.rs @@ -141,13 +141,13 @@ fn test_bad_function_call() { "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", r#"file(a, "../out")"#]); - insta::assert_snapshot!(stderr, @r###" + insta::assert_snapshot!(stderr.replace('\\', "/"), @r###" Error: Failed to parse revset: --> 1:9 | 1 | file(a, "../out") | ^------^ | - = Invalid file pattern: Path "../out" is not in the repo + = Invalid file pattern: Path "../out" is not in the repo ".": Invalid component ".." in repo-relative path "../out" "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "branches(bad:pattern)"]); diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index e5ab6c87e..03c39c3d2 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -1043,7 +1043,7 @@ impl TreeState { let repo_paths = trace_span!("processing fsmonitor paths").in_scope(|| { changed_files .into_iter() - .filter_map(RepoPathBuf::from_relative_path) + .filter_map(|path| RepoPathBuf::from_relative_path(path).ok()) .collect_vec() }); diff --git a/lib/src/repo_path.rs b/lib/src/repo_path.rs index d1006d737..122f99204 100644 --- a/lib/src/repo_path.rs +++ b/lib/src/repo_path.rs @@ -204,14 +204,18 @@ impl RepoPathBuf { /// Converts repo-relative `Path` to `RepoPathBuf`. /// /// The input path should not contain `.` or `..`. - pub fn from_relative_path(relative_path: impl AsRef) -> Option { + pub fn from_relative_path( + relative_path: impl AsRef, + ) -> Result { let relative_path = relative_path.as_ref(); let mut components = relative_path .components() .map(|c| match c { - Component::Normal(name) => Some(name.to_str().unwrap()), - // TODO: better to return Err instead of None? - _ => None, + Component::Normal(name) => Ok(name.to_str().unwrap()), + _ => Err(RelativePathParseError::InvalidComponent { + component: c.as_os_str().to_string_lossy().into(), + path: relative_path.into(), + }), }) .fuse(); let mut value = String::with_capacity(relative_path.as_os_str().len()); @@ -222,7 +226,7 @@ impl RepoPathBuf { value.push('/'); value.push_str(name?); } - Some(RepoPathBuf { value }) + Ok(RepoPathBuf { value }) } /// Parses an `input` path into a `RepoPathBuf` relative to `base`. @@ -241,8 +245,11 @@ impl RepoPathBuf { if repo_relative_path == Path::new(".") { return Ok(Self::root()); } - Self::from_relative_path(repo_relative_path) - .ok_or_else(|| FsPathParseError::InputNotInRepo(input.to_owned())) + Self::from_relative_path(repo_relative_path).map_err(|source| FsPathParseError { + base: file_util::relative_path(cwd, base).into(), + input: input.into(), + source, + }) } /// Consumes this and returns the underlying string representation. @@ -411,9 +418,20 @@ impl PartialOrd for RepoPathBuf { } #[derive(Clone, Debug, Eq, Error, PartialEq)] -pub enum FsPathParseError { - #[error(r#"Path "{}" is not in the repo"#, .0.display())] - InputNotInRepo(PathBuf), +pub enum RelativePathParseError { + #[error(r#"Invalid component "{component}" in repo-relative path "{path}""#)] + InvalidComponent { + component: Box, + path: Box, + }, +} + +#[derive(Clone, Debug, Eq, Error, PartialEq)] +#[error(r#"Path "{input}" is not in the repo "{base}""#)] +pub struct FsPathParseError { + base: Box, + input: Box, + source: RelativePathParseError, } fn is_valid_repo_path_component_str(value: &str) -> bool { @@ -428,6 +446,7 @@ fn is_valid_repo_path_str(value: &str) -> bool { mod tests { use std::panic; + use assert_matches::assert_matches; use itertools::Itertools as _; use super::*; @@ -667,9 +686,12 @@ mod tests { RepoPathBuf::parse_fs_path(&cwd_path, wc_path, "dir/file").as_deref(), Ok(repo_path("dir/file")) ); - assert_eq!( + assert_matches!( RepoPathBuf::parse_fs_path(&cwd_path, wc_path, ".."), - Err(FsPathParseError::InputNotInRepo("..".into())) + Err(FsPathParseError { + source: RelativePathParseError::InvalidComponent { .. }, + .. + }) ); assert_eq!( RepoPathBuf::parse_fs_path(&cwd_path, &cwd_path, "../repo").as_deref(), @@ -717,9 +739,12 @@ mod tests { RepoPathBuf::parse_fs_path(&cwd_path, &wc_path, "..").as_deref(), Ok(RepoPath::root()) ); - assert_eq!( + assert_matches!( RepoPathBuf::parse_fs_path(&cwd_path, &wc_path, "../.."), - Err(FsPathParseError::InputNotInRepo("../..".into())) + Err(FsPathParseError { + source: RelativePathParseError::InvalidComponent { .. }, + .. + }) ); assert_eq!( RepoPathBuf::parse_fs_path(&cwd_path, &wc_path, "../other-dir/file").as_deref(), @@ -733,13 +758,19 @@ mod tests { let cwd_path = temp_dir.path().join("cwd"); let wc_path = cwd_path.join("repo"); - assert_eq!( + assert_matches!( RepoPathBuf::parse_fs_path(&cwd_path, &wc_path, ""), - Err(FsPathParseError::InputNotInRepo("".into())) + Err(FsPathParseError { + source: RelativePathParseError::InvalidComponent { .. }, + .. + }) ); - assert_eq!( + assert_matches!( RepoPathBuf::parse_fs_path(&cwd_path, &wc_path, "not-repo"), - Err(FsPathParseError::InputNotInRepo("not-repo".into())) + Err(FsPathParseError { + source: RelativePathParseError::InvalidComponent { .. }, + .. + }) ); assert_eq!( RepoPathBuf::parse_fs_path(&cwd_path, &wc_path, "repo").as_deref(),