diff --git a/CHANGELOG.md b/CHANGELOG.md index abfb157d35..f4bb22efd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed bugs +* Updating the working copy to a commit where a file that's currently ignored + in the working copy no longer leads to a crash + ([#976](https://github.com/martinvonz/jj/issues/976)). + ## [0.10.0] - 2023-10-04 ### Breaking changes diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 309410342d..9541cd587d 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1405,7 +1405,7 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin } } if let Some(stats) = stats { - print_checkout_stats(ui, stats)?; + print_checkout_stats(ui, stats, new_commit)?; } Ok(()) } @@ -1760,7 +1760,11 @@ pub fn check_stale_working_copy( } } -pub fn print_checkout_stats(ui: &mut Ui, stats: CheckoutStats) -> Result<(), std::io::Error> { +pub fn print_checkout_stats( + ui: &mut Ui, + stats: CheckoutStats, + new_commit: &Commit, +) -> Result<(), std::io::Error> { if stats.added_files > 0 || stats.updated_files > 0 || stats.removed_files > 0 { writeln!( ui, @@ -1768,6 +1772,21 @@ pub fn print_checkout_stats(ui: &mut Ui, stats: CheckoutStats) -> Result<(), std stats.added_files, stats.updated_files, stats.removed_files )?; } + if stats.skipped_files != 0 { + writeln!( + ui.warning(), + "{} of those updates were skipped because there were conflicting changes in the \ + working copy.", + stats.skipped_files + )?; + writeln!( + ui.hint(), + "Hint: Inspect the changes compared to the intended target with `jj diff --from {}`. +Discard the conflicting changes with `jj restore --from {}`.", + short_commit_hash(new_commit.id()), + short_commit_hash(new_commit.id()) + )?; + } Ok(()) } diff --git a/cli/src/commands/mod.rs b/cli/src/commands/mod.rs index be86eb4656..64efba6c66 100644 --- a/cli/src/commands/mod.rs +++ b/cli/src/commands/mod.rs @@ -3873,7 +3873,7 @@ fn cmd_workspace_update_stale( workspace_command.write_commit_summary(fmt, &desired_wc_commit) })?; ui.write("\n")?; - print_checkout_stats(ui, stats)?; + print_checkout_stats(ui, stats, &desired_wc_commit)?; } } Ok(()) @@ -3926,7 +3926,7 @@ fn cmd_sparse_set( workspace_command.workspace_root().clone(), ) }); - let (mut locked_wc, _wc_commit) = workspace_command.start_working_copy_mutation()?; + let (mut locked_wc, wc_commit) = workspace_command.start_working_copy_mutation()?; let mut new_patterns = HashSet::new(); if args.reset { new_patterns.insert(RepoPath::root()); @@ -3957,7 +3957,7 @@ fn cmd_sparse_set( })?; let operation_id = locked_wc.old_operation_id().clone(); locked_wc.finish(operation_id)?; - print_checkout_stats(ui, stats)?; + print_checkout_stats(ui, stats, &wc_commit)?; Ok(()) } diff --git a/cli/tests/test_gitignores.rs b/cli/tests/test_gitignores.rs index 5042abb50b..b7dc79f6fd 100644 --- a/cli/tests/test_gitignores.rs +++ b/cli/tests/test_gitignores.rs @@ -65,3 +65,50 @@ fn test_gitignores() { A file3 "###); } + +#[test] +fn test_gitignores_ignored_file_in_target_commit() { + let test_env = TestEnvironment::default(); + let workspace_root = test_env.env_root().join("repo"); + git2::Repository::init(&workspace_root).unwrap(); + test_env.jj_cmd_success(&workspace_root, &["init", "--git-repo", "."]); + + // Create a commit with file "ignored" in it + std::fs::write(workspace_root.join("ignored"), "committed contents\n").unwrap(); + test_env.jj_cmd_success(&workspace_root, &["branch", "create", "with-file"]); + let target_commit_id = test_env.jj_cmd_success( + &workspace_root, + &["log", "--no-graph", "-T=commit_id", "-r=@"], + ); + + // Create another commit where we ignore that path + test_env.jj_cmd_success(&workspace_root, &["new", "root()"]); + std::fs::write(workspace_root.join("ignored"), "contents in working copy\n").unwrap(); + std::fs::write(workspace_root.join(".gitignore"), ".gitignore\nignored\n").unwrap(); + + // Update to the commit with the "ignored" file + let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["edit", "with-file"]); + insta::assert_snapshot!(stdout, @r###" + Working copy now at: qpvuntsm 4a703628 with-file | (no description set) + Parent commit : zzzzzzzz 00000000 (empty) (no description set) + Added 1 files, modified 0 files, removed 0 files + "###); + insta::assert_snapshot!(stderr, @r###" + 1 of those updates were skipped because there were conflicting changes in the working copy. + Hint: Inspect the changes compared to the intended target with `jj diff --from 4a703628bcb2`. + Discard the conflicting changes with `jj restore --from 4a703628bcb2`. + "###); + let stdout = test_env.jj_cmd_success( + &workspace_root, + &["diff", "--git", "--from", &target_commit_id], + ); + insta::assert_snapshot!(stdout, @r###" + diff --git a/ignored b/ignored + index 8a69467466...4d9be5127b 100644 + --- a/ignored + +++ b/ignored + @@ -1,1 +1,1 @@ + -committed contents + +contents in working copy + "###); +} diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 415d8910f5..bbbab4eb2a 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -82,6 +82,20 @@ pub struct FileState { } impl FileState { + /// Indicates that a file exists in the tree but that it needs to be + /// re-stat'ed on the next snapshot. + fn placeholder() -> Self { + #[cfg(unix)] + let executable = false; + #[cfg(windows)] + let executable = (); + FileState { + file_type: FileType::Normal { executable }, + mtime: MillisSinceEpoch(0), + size: 0, + } + } + fn for_file(executable: bool, size: u64, metadata: &Metadata) -> Self { #[cfg(windows)] let executable = { @@ -207,7 +221,10 @@ fn sparse_patterns_from_proto(proto: &crate::protos::working_copy::TreeState) -> /// Note that this does not prevent TOCTOU bugs caused by concurrent checkouts. /// Another process may remove the directory created by this function and put a /// symlink there. -fn create_parent_dirs(working_copy_path: &Path, repo_path: &RepoPath) -> Result<(), CheckoutError> { +fn create_parent_dirs( + working_copy_path: &Path, + repo_path: &RepoPath, +) -> Result { let (_, dir_components) = repo_path .components() .split_last() @@ -223,6 +240,9 @@ fn create_parent_dirs(working_copy_path: &Path, repo_path: &RepoPath) -> Result< .map(|m| m.is_dir()) .unwrap_or(false) => {} Err(err) => { + if dir_path.is_file() { + return Ok(true); + } return Err(CheckoutError::IoError { message: format!( "Failed to create parent directories for {}", @@ -233,7 +253,7 @@ fn create_parent_dirs(working_copy_path: &Path, repo_path: &RepoPath) -> Result< } } } - Ok(()) + Ok(false) } fn mtime_from_metadata(metadata: &Metadata) -> MillisSinceEpoch { @@ -284,6 +304,7 @@ pub struct CheckoutStats { pub updated_files: u32, pub added_files: u32, pub removed_files: u32, + pub skipped_files: u32, } #[derive(Debug, Error)] @@ -345,15 +366,6 @@ impl CheckoutError { } } -fn suppress_file_exists_error(orig_err: CheckoutError) -> Result<(), CheckoutError> { - match orig_err { - CheckoutError::IoError { err, .. } if err.kind() == std::io::ErrorKind::AlreadyExists => { - Ok(()) - } - _ => Err(orig_err), - } -} - pub struct SnapshotOptions<'a> { pub base_ignores: Arc, pub fsmonitor_kind: Option, @@ -1180,7 +1192,7 @@ impl TreeState { }, other => CheckoutError::InternalBackendError(other), })?; - let stats = self.update(&old_tree, new_tree, self.sparse_matcher().as_ref(), Err)?; + let stats = self.update(&old_tree, new_tree, self.sparse_matcher().as_ref())?; self.tree_id = new_tree.id(); Ok(stats) } @@ -1200,22 +1212,19 @@ impl TreeState { let added_matcher = DifferenceMatcher::new(&new_matcher, &old_matcher); let removed_matcher = DifferenceMatcher::new(&old_matcher, &new_matcher); let empty_tree = MergedTree::resolved(Tree::null(self.store.clone(), RepoPath::root())); - let added_stats = self.update( - &empty_tree, - &tree, - &added_matcher, - suppress_file_exists_error, // Keep un-ignored file and mark it as modified - )?; - let removed_stats = self.update(&tree, &empty_tree, &removed_matcher, Err)?; + let added_stats = self.update(&empty_tree, &tree, &added_matcher)?; + let removed_stats = self.update(&tree, &empty_tree, &removed_matcher)?; self.sparse_patterns = sparse_patterns; assert_eq!(added_stats.updated_files, 0); assert_eq!(added_stats.removed_files, 0); assert_eq!(removed_stats.updated_files, 0); assert_eq!(removed_stats.added_files, 0); + assert_eq!(removed_stats.skipped_files, 0); Ok(CheckoutStats { updated_files: 0, added_files: added_stats.added_files, removed_files: removed_stats.removed_files, + skipped_files: added_stats.skipped_files, }) } @@ -1224,19 +1233,26 @@ impl TreeState { old_tree: &MergedTree, new_tree: &MergedTree, matcher: &dyn Matcher, - mut handle_error: impl FnMut(CheckoutError) -> Result<(), CheckoutError>, ) -> Result { let mut apply_diff = |path: RepoPath, before: Merge>, after: Merge>| - -> Result<(), CheckoutError> { + -> Result { let disk_path = path.to_fs_path(&self.working_copy_path); if before.is_present() { fs::remove_file(&disk_path).ok(); } + if before.is_absent() && disk_path.exists() { + self.file_states.insert(path, FileState::placeholder()); + return Ok(true); + } if after.is_present() { - create_parent_dirs(&self.working_copy_path, &path)?; + let skip = create_parent_dirs(&self.working_copy_path, &path)?; + if skip { + self.file_states.insert(path, FileState::placeholder()); + return Ok(true); + } } // TODO: Check that the file has not changed before overwriting/removing it. match after.into_resolved() { @@ -1274,13 +1290,16 @@ impl TreeState { self.file_states.insert(path, file_state); } } - Ok(()) + Ok(false) }; + // TODO: maybe it's better not include the skipped counts in the "intended" + // counts let mut stats = CheckoutStats { updated_files: 0, added_files: 0, removed_files: 0, + skipped_files: 0, }; for (path, before, after) in old_tree.diff(new_tree, matcher) { if after.is_absent() { @@ -1290,7 +1309,10 @@ impl TreeState { } else { stats.updated_files += 1; } - apply_diff(path, before, after).or_else(&mut handle_error)?; + let skipped = apply_diff(path, before, after)?; + if skipped { + stats.skipped_files += 1; + } } Ok(stats) } diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index 8c5a1f4175..9d36fa8616 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -28,7 +28,7 @@ use itertools::Itertools; use jj_lib::backend::{MergedTreeId, ObjectId, TreeId, TreeValue}; use jj_lib::fsmonitor::FsmonitorKind; use jj_lib::local_working_copy::{ - LocalWorkingCopy, LockedLocalWorkingCopy, SnapshotError, SnapshotOptions, + CheckoutStats, LocalWorkingCopy, LockedLocalWorkingCopy, SnapshotError, SnapshotOptions, }; use jj_lib::merge::Merge; use jj_lib::merged_tree::{MergedTree, MergedTreeBuilder}; @@ -325,6 +325,75 @@ fn test_tree_builder_file_directory_transition() { assert!(!child_path.to_fs_path(&workspace_root).exists()); } +#[test] +fn test_conflicting_changes_on_disk() { + let settings = testutils::user_settings(); + let test_workspace = TestWorkspace::init(&settings); + let repo = &test_workspace.repo; + let mut workspace = test_workspace.workspace; + let workspace_root = workspace.workspace_root().clone(); + + // file on disk conflicts with file in target commit + let file_file_path = RepoPath::from_internal_string("file-file"); + // file on disk conflicts with directory in target commit + let file_dir_path = RepoPath::from_internal_string("file-dir"); + // directory on disk conflicts with file in target commit + let dir_file_path = RepoPath::from_internal_string("dir-file"); + let tree = create_tree( + repo, + &[ + (&file_file_path, "committed contents"), + ( + &file_dir_path.join(&RepoPathComponent::from("file")), + "committed contents", + ), + (&dir_file_path, "committed contents"), + ], + ); + + std::fs::write( + file_file_path.to_fs_path(&workspace_root), + "contents on disk", + ) + .unwrap(); + std::fs::write( + file_dir_path.to_fs_path(&workspace_root), + "contents on disk", + ) + .unwrap(); + std::fs::create_dir(dir_file_path.to_fs_path(&workspace_root)).unwrap(); + std::fs::write( + dir_file_path.to_fs_path(&workspace_root).join("file"), + "contents on disk", + ) + .unwrap(); + + let wc = workspace.working_copy_mut(); + let stats = wc.check_out(repo.op_id().clone(), None, &tree).unwrap(); + assert_eq!( + stats, + CheckoutStats { + updated_files: 0, + added_files: 3, + removed_files: 0, + skipped_files: 3, + } + ); + + assert_eq!( + std::fs::read_to_string(file_file_path.to_fs_path(&workspace_root)).ok(), + Some("contents on disk".to_string()) + ); + assert_eq!( + std::fs::read_to_string(file_dir_path.to_fs_path(&workspace_root)).ok(), + Some("contents on disk".to_string()) + ); + assert_eq!( + std::fs::read_to_string(dir_file_path.to_fs_path(&workspace_root).join("file")).ok(), + Some("contents on disk".to_string()) + ); +} + #[test] fn test_reset() { let settings = testutils::user_settings(); @@ -637,7 +706,7 @@ fn test_gitignores_checkout_never_overwrites_ignored() { // "contents". The exiting contents ("garbage") shouldn't be replaced in the // working copy. let wc = test_workspace.workspace.working_copy_mut(); - assert!(wc.check_out(repo.op_id().clone(), None, &tree).is_err()); + assert!(wc.check_out(repo.op_id().clone(), None, &tree).is_ok()); // Check that the old contents are in the working copy let path = workspace_root.join("modified"); diff --git a/lib/tests/test_local_working_copy_sparse.rs b/lib/tests/test_local_working_copy_sparse.rs index a90a3e4827..22a36d140f 100644 --- a/lib/tests/test_local_working_copy_sparse.rs +++ b/lib/tests/test_local_working_copy_sparse.rs @@ -62,7 +62,8 @@ fn test_sparse_checkout() { CheckoutStats { updated_files: 0, added_files: 0, - removed_files: 3 + removed_files: 3, + skipped_files: 0, } ); assert_eq!(locked_wc.sparse_patterns().unwrap(), sparse_patterns); @@ -106,7 +107,8 @@ fn test_sparse_checkout() { CheckoutStats { updated_files: 0, added_files: 2, - removed_files: 2 + removed_files: 2, + skipped_files: 0, } ); assert_eq!(locked_wc.sparse_patterns().unwrap(), sparse_patterns);