update: fix UPDATE-04, UPDATE-07

src/update.rs

@@ -196,7 +196,9 @@ pub fn run(
         anyhow::bail!(hint);
     }
 
-    // Materialize: delete files not in target, then write target files.
+    // Materialize: validate untracked collisions before any delete, then delete
+    // files not in target, then write target files. This ordering prevents a
+    // bail from leaving a half-mutated tree (deletions applied, writes undone).
     // Atomicity: deletes propagate errors (except NotFound); writes use
     // temp-file + rename so a crash never leaves a truncated file.
     // --force is required to delete untracked files not in target
@@ -207,6 +209,9 @@ pub fn run(
     // Preserve untracked markers (not in the current head) so empty dirs like
     // `logs/` or `tmp/` survive across updates. Tracked markers follow the
     // target like any other file — if the target deletes them, delete them.
+    if !force {
+        validate_untracked_writes(&root, &snapshot.files, &work)?;
+    }
     for path in work.keys() {
         if !force && is_keep_marker(path) && !head_filtered.contains_key(path) {
             continue;
@@ -270,16 +275,7 @@ pub fn run(
 
     for (path, contents) in &snapshot.files {
         let full = root.join(path);
-        if !force && full.exists() && !work.contains_key(path) {
-            if let Ok(existing_bytes) = std::fs::read(&full) {
-                if existing_bytes != *contents {
-                    anyhow::bail!(
-                        "untracked working file '{path}' would be overwritten by update; commit, move, or pass --force"
-                    );
-                }
-            }
-        }
-        // Preserve exec bits from tree.
+        // Untracked collisions are validated before deletes (and skipped with --force).
         let executable = target_files.get(path).map(|(_, e)| *e).unwrap_or(false);
         write_file_atomic(&full, &root, contents, executable)?;
     }
@@ -430,6 +426,24 @@ fn materialize_merge(
         }
     }
 
+    // Validate untracked collisions before any delete so a bail leaves the tree
+    // untouched. Check any path we will write that is not in the working copy.
+    for (path, contents) in &snapshot.files {
+        if keep_work.contains(path) {
+            continue;
+        }
+        let is_filtered_path = target_filtered.contains_key(path)
+            || head_filtered.contains_key(path)
+            || work.contains_key(path);
+        if is_filtered_path && !take_target.contains(path) {
+            continue;
+        }
+        // Collision is only possible when the working copy does not track this path.
+        if !work.contains_key(path) {
+            check_untracked_overwrite(&root.join(path), path, contents)?;
+        }
+    }
+
     // 1) Delete: only delete work files that are take-target and target has no entry
     //    (i.e., target deleted it and work didn't change it).
     //    Keep-work files are never deleted. Preserve untracked .ootkeep/.oothave
@@ -538,16 +552,7 @@ fn materialize_merge(
         }
 
         let full = root.join(path);
-        if !is_filtered_path && full.exists() && !work.contains_key(path) {
-            if let Ok(existing_bytes) = std::fs::read(&full) {
-                if existing_bytes != *contents {
-                    anyhow::bail!(
-                        "untracked working file '{path}' would be overwritten by update; commit, move, or pass --force"
-                    );
-                }
-            }
-        }
-
+        // Untracked collisions are validated before deletes.
         let executable = target_files.get(path).map(|(_, e)| *e).unwrap_or(false);
         write_file_atomic(&full, root, contents, executable)?;
     }
@@ -588,6 +593,40 @@ fn is_keep_marker(path: &str) -> bool {
         .is_some_and(|name| name == ".ootkeep" || name == ".oothave")
 }
 
+fn check_untracked_overwrite(full: &Path, path: &str, contents: &[u8]) -> Result<()> {
+    if !full.exists() {
+        return Ok(());
+    }
+    let meta = std::fs::symlink_metadata(full)
+        .with_context(|| format!("failed to inspect existing path '{path}' before update"))?;
+    if meta.file_type().is_dir() {
+        anyhow::bail!(
+            "cannot overwrite existing directory '{path}' with a file during update; remove it or pass --force"
+        );
+    }
+    let existing_bytes = std::fs::read(full)
+        .with_context(|| format!("failed to read existing path '{path}' to check for overwrite"))?;
+    if existing_bytes != contents {
+        anyhow::bail!(
+            "untracked working file '{path}' would be overwritten by update; commit, move, or pass --force"
+        );
+    }
+    Ok(())
+}
+
+fn validate_untracked_writes(
+    root: &Path,
+    snapshot_files: &std::collections::HashMap<String, Vec<u8>>,
+    work: &std::collections::HashMap<String, (String, bool)>,
+) -> Result<()> {
+    for (path, contents) in snapshot_files {
+        if !work.contains_key(path) {
+            check_untracked_overwrite(&root.join(path), path, contents)?;
+        }
+    }
+    Ok(())
+}
+
 fn write_file_atomic(full: &Path, root: &Path, contents: &[u8], executable: bool) -> Result<()> {
     if let Ok(meta) = std::fs::symlink_metadata(full) {
         if meta.file_type().is_symlink() {

tests/update_test.rs

@@ -1018,6 +1018,67 @@ fn test_update_multi_branch_clean_ff_advances_head_update01_02() {
     let _ = std::fs::remove_dir_all(&tmp);
 }
 
+#[test]
+fn test_update_untracked_collision_does_not_delete_before_bail() {
+    // Regression for UPDATE-04 (straight path): validate untracked collisions
+    // before deleting, so a bail leaves the tree untouched.
+    let tmp = unique_tmp("collision-no-delete");
+    let _ = std::fs::remove_dir_all(&tmp);
+    let proj = tmp.join("proj");
+    std::fs::create_dir_all(&proj).unwrap();
+    assert!(oot(&["init"], &proj).0);
+
+    // Target change (change 1): has a file that will collide and omits victim.
+    std::fs::write(proj.join("a.txt"), "v1\n").unwrap();
+    std::fs::write(proj.join("blocker"), "target\n").unwrap();
+    let (ok, msg) = oot(&["record", "-m", "target"], &proj);
+    assert!(ok, "{msg}");
+    let (ok, log) = oot(&["log"], &proj);
+    assert!(ok, "{log}");
+    let target_id = log
+        .lines()
+        .next()
+        .unwrap()
+        .split_whitespace()
+        .next()
+        .unwrap()
+        .to_string();
+
+    // Current head (change 2): removes blocker, adds victim (clean work).
+    std::fs::write(proj.join("a.txt"), "v2\n").unwrap();
+    let _ = std::fs::remove_file(proj.join("blocker"));
+    std::fs::write(proj.join("victim.txt"), "doomed\n").unwrap();
+    let (ok, msg) = oot(&["record", "-m", "head"], &proj);
+    assert!(ok, "{msg}");
+
+    // Create an untracked blocker that conflicts with the target. Use an empty
+    // directory on all platforms: collect_worktree does not record empty dirs,
+    // so work stays clean (straight path) and the target's file collides.
+    std::fs::create_dir_all(proj.join("blocker")).unwrap();
+
+    let (ok, out) = oot(&["update", "--change", &target_id[..7]], &proj);
+    assert!(!ok, "untracked collision should fail: {out}");
+    assert!(
+        out.contains("cannot overwrite existing directory")
+            || out.contains("would be overwritten")
+            || out.contains("blocker"),
+        "{out}"
+    );
+    // victim.txt must survive: no deletes before the collision bail.
+    assert!(
+        proj.join("victim.txt").exists(),
+        "victim.txt deleted despite collision bail: {out}"
+    );
+    assert_eq!(
+        std::fs::read_to_string(proj.join("victim.txt")).unwrap(),
+        "doomed\n"
+    );
+    // a.txt must not have been rewritten to the target.
+    assert_eq!(std::fs::read_to_string(proj.join("a.txt")).unwrap(), "v2\n");
+
+    let _ = std::fs::remove_dir_all(&tmp);
+}
+
 #[test]
 fn test_update_already_up_to_date_advances_head_update02() {
     let tmp = unique_tmp("uptodate-head");
@@ -1129,3 +1190,127 @@ fn test_update_fails_when_current_head_change_is_corrupt() {
 
     let _ = std::fs::remove_dir_all(&tmp);
 }
+
+#[cfg(unix)]
+#[test]
+fn test_update_untracked_symlink_collision_does_not_delete_before_bail() {
+    // Regression for UPDATE-04 with a symlink collision: collect_worktree skips
+    // symlinks, so the symlink is not in work and collides with the target file.
+    let tmp = unique_tmp("collision-symlink");
+    let _ = std::fs::remove_dir_all(&tmp);
+    let proj = tmp.join("proj");
+    std::fs::create_dir_all(&proj).unwrap();
+    assert!(oot(&["init"], &proj).0);
+
+    std::fs::write(proj.join("a.txt"), "v1\n").unwrap();
+    std::fs::write(proj.join("blocker"), "target\n").unwrap();
+    let (ok, msg) = oot(&["record", "-m", "target"], &proj);
+    assert!(ok, "{msg}");
+    let (ok, log) = oot(&["log"], &proj);
+    assert!(ok, "{log}");
+    let target_id = log
+        .lines()
+        .next()
+        .unwrap()
+        .split_whitespace()
+        .next()
+        .unwrap()
+        .to_string();
+
+    std::fs::write(proj.join("a.txt"), "v2\n").unwrap();
+    let _ = std::fs::remove_file(proj.join("blocker"));
+    std::fs::write(proj.join("victim.txt"), "doomed\n").unwrap();
+    let (ok, msg) = oot(&["record", "-m", "head"], &proj);
+    assert!(ok, "{msg}");
+
+    let elsewhere = tmp.join("elsewhere");
+    std::fs::write(&elsewhere, "different\n").unwrap();
+    std::os::unix::fs::symlink(&elsewhere, proj.join("blocker")).unwrap();
+
+    let (ok, out) = oot(&["update", "--change", &target_id[..7]], &proj);
+    assert!(!ok, "untracked symlink collision should fail: {out}");
+    assert!(out.contains("would be overwritten"), "{out}");
+    assert!(
+        proj.join("victim.txt").exists(),
+        "victim.txt deleted despite symlink collision bail: {out}"
+    );
+    assert_eq!(
+        std::fs::read_to_string(proj.join("victim.txt")).unwrap(),
+        "doomed\n"
+    );
+
+    let _ = std::fs::remove_dir_all(&tmp);
+}
+
+#[test]
+fn test_update_merge_untracked_collision_does_not_delete_before_bail() {
+    // Regression for UPDATE-04/UPDATE-07 (merge path): validate untracked
+    // collisions before deleting so a dirty merge bail leaves the tree intact.
+    let tmp = unique_tmp("collision-merge");
+    let _ = std::fs::remove_dir_all(&tmp);
+    let proj = tmp.join("proj");
+    std::fs::create_dir_all(&proj).unwrap();
+    assert!(oot(&["init"], &proj).0);
+
+    // Base/current head state.
+    std::fs::write(proj.join("shared.txt"), "base\n").unwrap();
+    std::fs::write(proj.join("victim.txt"), "doomed\n").unwrap();
+    let (ok, msg) = oot(&["record", "-m", "base"], &proj);
+    assert!(ok, "{msg}");
+
+    // Target change: deletes victim and adds blocker (regular file).
+    std::fs::write(proj.join("shared.txt"), "target\n").unwrap();
+    let _ = std::fs::remove_file(proj.join("victim.txt"));
+    std::fs::write(proj.join("blocker"), "target\n").unwrap();
+    let (ok, msg) = oot(&["record", "-m", "target"], &proj);
+    assert!(ok, "{msg}");
+    let (ok, log) = oot(&["log"], &proj);
+    assert!(ok, "{log}");
+    let target_id = log
+        .lines()
+        .next()
+        .unwrap()
+        .split_whitespace()
+        .next()
+        .unwrap()
+        .to_string();
+
+    // Restore base state (clean), then make work dirty so update takes the
+    // merge path while an untracked blocker directory collides with target.
+    let (ok, msg) = oot(&["update", "--change", &target_id[..7], "--force"], &proj);
+    assert!(ok, "{msg}");
+    // Go back to base by recording/using log - easier: reconstruct base state.
+    // (we know base had shared.txt=base, victim.txt=doomed, no blocker)
+    std::fs::write(proj.join("shared.txt"), "base\n").unwrap();
+    std::fs::write(proj.join("victim.txt"), "doomed\n").unwrap();
+    let _ = std::fs::remove_file(proj.join("blocker"));
+    let (ok, msg) = oot(&["record", "-m", "base2"], &proj);
+    assert!(ok, "{msg}");
+
+    // Dirty work (kept by merge) + untracked colliding directory.
+    std::fs::write(proj.join("local.txt"), "local\n").unwrap();
+    std::fs::create_dir_all(proj.join("blocker")).unwrap();
+
+    let (ok, out) = oot(&["update", "--change", &target_id[..7]], &proj);
+    assert!(!ok, "merge with untracked collision should fail: {out}");
+    assert!(
+        out.contains("cannot overwrite existing directory")
+            || out.contains("would be overwritten")
+            || out.contains("blocker"),
+        "{out}"
+    );
+    assert!(
+        proj.join("victim.txt").exists(),
+        "victim.txt deleted despite merge collision bail: {out}"
+    );
+    assert_eq!(
+        std::fs::read_to_string(proj.join("victim.txt")).unwrap(),
+        "doomed\n"
+    );
+    assert_eq!(
+        std::fs::read_to_string(proj.join("local.txt")).unwrap(),
+        "local\n"
+    );
+
+    let _ = std::fs::remove_dir_all(&tmp);
+}