diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index d1dcbf874f..30c27c2bf3 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -15,6 +15,7 @@ use std::cell::{RefCell, RefMut}; use std::collections::{BTreeMap, HashSet}; use std::convert::TryInto; +use std::ffi::OsString; use std::fs; use std::fs::{File, OpenOptions}; use std::io::Read; @@ -189,6 +190,18 @@ pub struct CheckoutStats { pub removed_files: u32, } +#[derive(Debug, Error)] +pub enum SnapshotError { + #[error("Failed to open file {path}: {err:?}")] + FileOpenError { path: PathBuf, err: std::io::Error }, + #[error("Working copy path {} is not valid UTF-8", path.to_string_lossy())] + InvalidUtf8Path { path: OsString }, + #[error("Symlink {path} target is not valid UTF-8")] + InvalidUtf8SymlinkTarget { path: PathBuf, target: PathBuf }, + #[error("Internal backend error: {0:?}")] + InternalBackendError(BackendError), +} + #[derive(Debug, Error, PartialEq, Eq)] pub enum CheckoutError { // The current checkout was deleted, maybe by an overly aggressive GC that happened while @@ -313,20 +326,46 @@ impl TreeState { .unwrap(); } - fn write_file_to_store(&self, path: &RepoPath, disk_path: &Path) -> FileId { - let file = File::open(disk_path).unwrap(); - self.store.write_file(path, &mut Box::new(file)).unwrap() + fn write_file_to_store( + &self, + path: &RepoPath, + disk_path: &Path, + ) -> Result { + let file = File::open(disk_path).map_err(|err| SnapshotError::FileOpenError { + path: disk_path.to_path_buf(), + err, + })?; + self.store + .write_file(path, &mut Box::new(file)) + .map_err(SnapshotError::InternalBackendError) } - fn write_symlink_to_store(&self, path: &RepoPath, disk_path: &Path) -> SymlinkId { - let target = disk_path.read_link().unwrap(); - let str_target = target.to_str().unwrap(); - self.store.write_symlink(path, str_target).unwrap() + fn write_symlink_to_store( + &self, + path: &RepoPath, + disk_path: &Path, + ) -> Result { + let target = disk_path + .read_link() + .map_err(|err| SnapshotError::FileOpenError { + path: disk_path.to_path_buf(), + err, + })?; + let str_target = + target + .to_str() + .ok_or_else(|| SnapshotError::InvalidUtf8SymlinkTarget { + path: disk_path.to_path_buf(), + target: target.clone(), + })?; + self.store + .write_symlink(path, str_target) + .map_err(SnapshotError::InternalBackendError) } /// Look for changes to the working copy. If there are any changes, create /// a new tree from it and return it, and also update the dirstate on disk. - pub fn snapshot(&mut self, base_ignores: Arc) -> TreeId { + pub fn snapshot(&mut self, base_ignores: Arc) -> Result { let sparse_matcher = self.sparse_matcher(); let mut work = vec![( RepoPath::root(), @@ -345,7 +384,11 @@ impl TreeState { let entry = maybe_entry.unwrap(); let file_type = entry.file_type().unwrap(); let file_name = entry.file_name(); - let name = file_name.to_str().unwrap(); + let name = file_name + .to_str() + .ok_or_else(|| SnapshotError::InvalidUtf8Path { + path: file_name.clone(), + })?; if name == ".jj" || name == ".git" { continue; } @@ -367,7 +410,7 @@ impl TreeState { entry.path(), git_ignore.as_ref(), &mut tree_builder, - ); + )?; } } } @@ -378,7 +421,7 @@ impl TreeState { tree_builder.remove(file.clone()); } self.tree_id = tree_builder.write_tree(); - self.tree_id.clone() + Ok(self.tree_id.clone()) } fn has_files_under(&self, dir: &RepoPath) -> bool { @@ -406,14 +449,14 @@ impl TreeState { disk_path: PathBuf, git_ignore: &GitIgnoreFile, tree_builder: &mut TreeBuilder, - ) { + ) -> Result<(), SnapshotError> { let maybe_current_file_state = self.file_states.get_mut(&repo_path); if maybe_current_file_state.is_none() && git_ignore.matches_file(&repo_path.to_internal_file_string()) { // If it wasn't already tracked and it matches the ignored paths, then // ignore it. - return; + return Ok(()); } #[cfg_attr(unix, allow(unused_mut))] let mut new_file_state = file_state(&disk_path).unwrap(); @@ -422,7 +465,7 @@ impl TreeState { // untracked let file_type = new_file_state.file_type.clone(); self.file_states.insert(repo_path.clone(), new_file_state); - let file_value = self.write_path_to_store(&repo_path, &disk_path, file_type); + let file_value = self.write_path_to_store(&repo_path, &disk_path, file_type)?; tree_builder.set(repo_path, file_value); } Some(current_file_state) => { @@ -477,7 +520,7 @@ impl TreeState { }; *current_file_state = new_file_state; tree_builder.set(repo_path, TreeValue::Conflict(new_conflict_id)); - return; + return Ok(()); } } } @@ -485,11 +528,12 @@ impl TreeState { if !clean { let file_type = new_file_state.file_type.clone(); *current_file_state = new_file_state; - let file_value = self.write_path_to_store(&repo_path, &disk_path, file_type); + let file_value = self.write_path_to_store(&repo_path, &disk_path, file_type)?; tree_builder.set(repo_path, file_value); } } }; + Ok(()) } fn write_path_to_store( @@ -497,15 +541,15 @@ impl TreeState { repo_path: &RepoPath, disk_path: &Path, file_type: FileType, - ) -> TreeValue { + ) -> Result { match file_type { FileType::Normal { executable } => { - let id = self.write_file_to_store(repo_path, disk_path); - TreeValue::Normal { id, executable } + let id = self.write_file_to_store(repo_path, disk_path)?; + Ok(TreeValue::Normal { id, executable }) } FileType::Symlink => { - let id = self.write_symlink_to_store(repo_path, disk_path); - TreeValue::Symlink(id) + let id = self.write_symlink_to_store(repo_path, disk_path)?; + Ok(TreeValue::Symlink(id)) } FileType::Conflict { .. } => panic!("conflicts should be handled by the caller"), } @@ -965,7 +1009,7 @@ impl LockedWorkingCopy<'_> { // The base_ignores are passed in here rather than being set on the TreeState // because the TreeState may be long-lived if the library is used in a // long-lived process. - pub fn snapshot(&mut self, base_ignores: Arc) -> TreeId { + pub fn snapshot(&mut self, base_ignores: Arc) -> Result { self.wc .tree_state() .as_mut() diff --git a/lib/tests/test_working_copy.rs b/lib/tests/test_working_copy.rs index cc51000538..783a623a18 100644 --- a/lib/tests/test_working_copy.rs +++ b/lib/tests/test_working_copy.rs @@ -41,7 +41,7 @@ fn test_root(use_git: bool) { let wc = test_workspace.workspace.working_copy_mut(); assert_eq!(wc.sparse_patterns(), vec![RepoPath::root()]); let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); locked_wc.discard(); let checkout_id = repo.view().get_checkout(&WorkspaceId::default()).unwrap(); let checkout_commit = repo.store().get_commit(checkout_id).unwrap(); @@ -205,7 +205,7 @@ fn test_checkout_file_transitions(use_git: bool) { // Check that the working copy is clean. let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); locked_wc.discard(); assert_eq!(new_tree_id, right_tree_id); @@ -307,7 +307,7 @@ fn test_reset() { assert!(ignored_path.to_fs_path(&workspace_root).is_file()); assert!(!wc.file_states().contains_key(&ignored_path)); let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); assert_eq!(new_tree_id, *tree_without_file.id()); locked_wc.discard(); @@ -320,7 +320,7 @@ fn test_reset() { assert!(ignored_path.to_fs_path(&workspace_root).is_file()); assert!(!wc.file_states().contains_key(&ignored_path)); let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); assert_eq!(new_tree_id, *tree_without_file.id()); locked_wc.discard(); @@ -332,7 +332,7 @@ fn test_reset() { assert!(ignored_path.to_fs_path(&workspace_root).is_file()); assert!(wc.file_states().contains_key(&ignored_path)); let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); assert_eq!(new_tree_id, *tree_with_file.id()); locked_wc.discard(); } @@ -408,7 +408,7 @@ fn test_commit_racy_timestamps(use_git: bool) { .unwrap(); } let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); locked_wc.discard(); assert_ne!(new_tree_id, previous_tree_id); previous_tree_id = new_tree_id; @@ -442,7 +442,7 @@ fn test_gitignores(use_git: bool) { let wc = test_workspace.workspace.working_copy_mut(); let mut locked_wc = wc.start_mutation(); - let new_tree_id1 = locked_wc.snapshot(GitIgnoreFile::empty()); + let new_tree_id1 = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); locked_wc.finish(repo.op_id().clone()); let tree1 = repo .store() @@ -472,7 +472,7 @@ fn test_gitignores(use_git: bool) { testutils::write_working_copy_file(&workspace_root, &subdir_ignored_path, "2"); let mut locked_wc = wc.start_mutation(); - let new_tree_id2 = locked_wc.snapshot(GitIgnoreFile::empty()); + let new_tree_id2 = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); locked_wc.discard(); let tree2 = repo .store() @@ -532,7 +532,7 @@ fn test_gitignores_checkout_overwrites_ignored(use_git: bool) { // Check that the file is in the tree created by committing the working copy let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); locked_wc.discard(); let new_tree = repo .store() @@ -578,7 +578,7 @@ fn test_gitignores_ignored_directory_already_tracked(use_git: bool) { // Check that the file is still in the tree created by committing the working // copy (that it didn't get removed because the directory is ignored) let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); locked_wc.discard(); let new_tree = repo .store() @@ -609,7 +609,7 @@ fn test_dotgit_ignored(use_git: bool) { "contents", ); let mut locked_wc = test_workspace.workspace.working_copy_mut().start_mutation(); - let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); assert_eq!(new_tree_id, *repo.store().empty_tree_id()); locked_wc.discard(); std::fs::remove_dir_all(&dotgit_path).unwrap(); @@ -621,7 +621,7 @@ fn test_dotgit_ignored(use_git: bool) { "contents", ); let mut locked_wc = test_workspace.workspace.working_copy_mut().start_mutation(); - let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); assert_eq!(new_tree_id, *repo.store().empty_tree_id()); locked_wc.discard(); } diff --git a/lib/tests/test_working_copy_concurrent.rs b/lib/tests/test_working_copy_concurrent.rs index 5c3105e35e..1ed5d65e60 100644 --- a/lib/tests/test_working_copy_concurrent.rs +++ b/lib/tests/test_working_copy_concurrent.rs @@ -129,7 +129,7 @@ fn test_checkout_parallel(use_git: bool) { // write_tree() should take the same lock as check_out(), write_tree() // should never produce a different tree. let mut locked_wc = workspace.working_copy_mut().start_mutation(); - let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); locked_wc.discard(); assert!(tree_ids.contains(&new_tree_id)); }); diff --git a/lib/tests/test_working_copy_sparse.rs b/lib/tests/test_working_copy_sparse.rs index a7d20e2ad8..039f074a8d 100644 --- a/lib/tests/test_working_copy_sparse.rs +++ b/lib/tests/test_working_copy_sparse.rs @@ -167,7 +167,7 @@ fn test_sparse_commit() { // Create a tree from the working copy. Only dir1/file1 should be updated in the // tree. let mut locked_wc = wc.start_mutation(); - let modified_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let modified_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); locked_wc.finish(repo.op_id().clone()); let modified_tree = repo .store() @@ -191,7 +191,7 @@ fn test_sparse_commit() { // Create a tree from the working copy. Only dir1/file1 and dir2/file1 should be // updated in the tree. let mut locked_wc = wc.start_mutation(); - let modified_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let modified_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); locked_wc.finish(repo.op_id().clone()); let modified_tree = repo .store() @@ -232,7 +232,7 @@ fn test_sparse_commit_gitignore() { // Create a tree from the working copy. Only dir1/file2 should be updated in the // tree because dir1/file1 is ignored. let mut locked_wc = wc.start_mutation(); - let modified_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()); + let modified_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); locked_wc.finish(repo.op_id().clone()); let modified_tree = repo .store() diff --git a/src/commands.rs b/src/commands.rs index bdd871dd80..3db95ea313 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -56,7 +56,9 @@ use jujutsu_lib::settings::UserSettings; use jujutsu_lib::store::Store; use jujutsu_lib::transaction::Transaction; use jujutsu_lib::tree::{merge_trees, Tree, TreeDiffIterator}; -use jujutsu_lib::working_copy::{CheckoutStats, LockedWorkingCopy, ResetError, WorkingCopy}; +use jujutsu_lib::working_copy::{ + CheckoutStats, LockedWorkingCopy, ResetError, SnapshotError, WorkingCopy, +}; use jujutsu_lib::workspace::{Workspace, WorkspaceInitError, WorkspaceLoadError}; use jujutsu_lib::{conflicts, dag_walk, diff, files, git, revset, tree}; use maplit::{hashmap, hashset}; @@ -111,6 +113,12 @@ impl From for CommandError { } } +impl From for CommandError { + fn from(err: SnapshotError) -> Self { + CommandError::InternalError(format!("Failed to snapshot the working copy: {:?}", err)) + } +} + impl From for CommandError { fn from(_: ResetError) -> Self { CommandError::InternalError("Failed to reset the working copy".to_string()) @@ -640,7 +648,7 @@ impl WorkspaceCommandHelper { ))); } } - let new_tree_id = locked_wc.snapshot(base_ignores); + let new_tree_id = locked_wc.snapshot(base_ignores)?; if new_tree_id != *checkout_commit.tree_id() { let mut tx = self.repo.start_transaction("commit working copy"); let mut_repo = tx.mut_repo(); @@ -2080,7 +2088,7 @@ fn cmd_untrack( locked_working_copy.reset(&new_tree)?; // Commit the working copy again so we can inform the user if paths couldn't be // untracked because they're not ignored. - let wc_tree_id = locked_working_copy.snapshot(base_ignores); + let wc_tree_id = locked_working_copy.snapshot(base_ignores)?; if wc_tree_id != new_tree_id { let wc_tree = store.get_tree(&RepoPath::root(), &wc_tree_id)?; let added_back = wc_tree.entries_matching(matcher.as_ref()).collect_vec(); diff --git a/src/diff_edit.rs b/src/diff_edit.rs index d3f1c73b4c..2d5b5f469f 100644 --- a/src/diff_edit.rs +++ b/src/diff_edit.rs @@ -26,7 +26,7 @@ use jujutsu_lib::repo_path::RepoPath; use jujutsu_lib::settings::UserSettings; use jujutsu_lib::store::Store; use jujutsu_lib::tree::Tree; -use jujutsu_lib::working_copy::{CheckoutError, TreeState}; +use jujutsu_lib::working_copy::{CheckoutError, SnapshotError, TreeState}; use tempfile::tempdir; use thiserror::Error; @@ -48,6 +48,8 @@ pub enum DiffEditError { }, #[error("I/O error: {0:?}")] IoError(#[source] std::io::Error), + #[error("Failed to snapshot changes: {0:?}")] + SnapshotError(SnapshotError), } impl From for DiffEditError { @@ -56,6 +58,12 @@ impl From for DiffEditError { } } +impl From for DiffEditError { + fn from(err: SnapshotError) -> Self { + DiffEditError::SnapshotError(err) + } +} + fn check_out( store: Arc, wc_dir: PathBuf, @@ -159,5 +167,5 @@ pub fn edit_diff( std::fs::remove_file(instructions_path).ok(); } - Ok(right_tree_state.snapshot(base_ignores)) + Ok(right_tree_state.snapshot(base_ignores)?) }