working_copy: don't crash when updating and tracked file exits on disk

Before this patch, when updating to a commit that has a file that's
currently an ignored file on disk, jj would crash. After this patch,
we instead leave the conflicting files or directories on disk. We
print a helpful message about how to inspect the differences between
the intended working copy and the actual working copy, and how to
discard the unintended changes.

Closes #976.

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

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,14 +1760,33 @@ 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,
             "Added {} files, modified {} files, removed {} files",
             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(())
 }
 

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

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
+    "###);
+}

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<bool, CheckoutError> {
     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<GitIgnoreFile>,
     pub fsmonitor_kind: Option<FsmonitorKind>,
@@ -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<CheckoutStats, CheckoutError> {
         let mut apply_diff = |path: RepoPath,
                               before: Merge<Option<TreeValue>>,
                               after: Merge<Option<TreeValue>>|
-         -> Result<(), CheckoutError> {
+         -> Result<bool, CheckoutError> {
             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)
     }

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");