Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

working_copy: propagate errors when snapshotting #280

Merged
merged 1 commit into from May 2, 2022
Merged

working_copy: propagate errors when snapshotting #280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
88 changes: 66 additions & 22 deletions lib/src/working_copy.rs
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<FileId, SnapshotError> {
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<SymlinkId, SnapshotError> {
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<GitIgnoreFile>) -> TreeId {
pub fn snapshot(&mut self, base_ignores: Arc<GitIgnoreFile>) -> Result<TreeId, SnapshotError> {
let sparse_matcher = self.sparse_matcher();
let mut work = vec![(
RepoPath::root(),
Expand All @@ -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;
}
Expand All @@ -367,7 +410,7 @@ impl TreeState {
entry.path(),
git_ignore.as_ref(),
&mut tree_builder,
);
)?;
}
}
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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();
Expand All @@ -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) => {
Expand Down Expand Up @@ -477,35 +520,36 @@ impl TreeState {
};
*current_file_state = new_file_state;
tree_builder.set(repo_path, TreeValue::Conflict(new_conflict_id));
return;
return Ok(());
}
}
}
}
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(
&self,
repo_path: &RepoPath,
disk_path: &Path,
file_type: FileType,
) -> TreeValue {
) -> Result<TreeValue, SnapshotError> {
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"),
}
Expand Down Expand Up @@ -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<GitIgnoreFile>) -> TreeId {
pub fn snapshot(&mut self, base_ignores: Arc<GitIgnoreFile>) -> Result<TreeId, SnapshotError> {
self.wc
.tree_state()
.as_mut()
Expand Down
24 changes: 12 additions & 12 deletions lib/tests/test_working_copy.rs
View file
Open in desktop
Expand Up @@ -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();
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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();

Expand All @@ -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();

Expand All @@ -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();
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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();
Expand All @@ -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();
}
2 changes: 1 addition & 1 deletion lib/tests/test_working_copy_concurrent.rs
View file
Open in desktop
Expand Up @@ -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));
});
Expand Down
6 changes: 3 additions & 3 deletions lib/tests/test_working_copy_sparse.rs
View file
Open in desktop
Expand Up @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
14 changes: 11 additions & 3 deletions src/commands.rs
View file
Open in desktop
Expand Up @@ -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};
Expand Down Expand Up @@ -111,6 +113,12 @@ impl From<OpHeadResolutionError> for CommandError {
}
}

impl From<SnapshotError> for CommandError {
fn from(err: SnapshotError) -> Self {
CommandError::InternalError(format!("Failed to snapshot the working copy: {:?}", err))
}
}

impl From<ResetError> for CommandError {
fn from(_: ResetError) -> Self {
CommandError::InternalError("Failed to reset the working copy".to_string())
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down