ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

working_copy: propagate errors when snapshotting

Browse source 
Closes #258
This commit is contained in:
Martin von Zweigbergk 2022-04-30 22:36:46 -07:00 committed by Martin von Zweigbergk
parent 6649dbabae
commit 7268e5608e
6 changed files with 103 additions and 43 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

88
lib/src/working_copy.rs
View file

@ -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<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(),
@ -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<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"),
}
@ -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()

24
lib/tests/test_working_copy.rs
View file

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

2
lib/tests/test_working_copy_concurrent.rs
View file

@ -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));
});

6
lib/tests/test_working_copy_sparse.rs
View file

@ -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()

14
src/commands.rs
View file

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

12
src/diff_edit.rs
View file

@ -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<CheckoutError> for DiffEditError {
@ -56,6 +58,12 @@ impl From<CheckoutError> for DiffEditError {
}
}
impl From<SnapshotError> for DiffEditError {
fn from(err: SnapshotError) -> Self {
DiffEditError::SnapshotError(err)
}
}
fn check_out(
store: Arc<Store>,
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)?)
}