diff --git a/src/update.rs b/src/update.rs index 442d886..6557fc6 100644 --- a/src/update.rs +++ b/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>, + work: &std::collections::HashMap, +) -> 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() { diff --git a/tests/update_test.rs b/tests/update_test.rs index 16611f8..0ae85be 100644 --- a/tests/update_test.rs +++ b/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); +}