From 99b9f56153340fbd1fe1accfcb7c6df08bda67e0 Mon Sep 17 00:00:00 2001 From: Kriday Dave Date: Sun, 8 Nov 2026 13:16:09 +0530 Subject: [PATCH 1/3] update: validate untracked collisions before deletes Check for untracked overwrite collisions before mutating the working tree so a bail does not leave deletions applied and writes undone. Report directory and read errors clearly instead of skipping the check and failing later during rename. Fixes UPDATE-04, UPDATE-07. --- src/update.rs | 81 ++++++++++++++----- tests/update_test.rs | 185 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 245 insertions(+), 21 deletions(-) diff --git a/src/update.rs b/src/update.rs index 9cdff02..a399ded 100644 --- a/src/update.rs +++ b/src/update.rs @@ -199,7 +199,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 @@ -210,6 +212,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; @@ -273,16 +278,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)?; } @@ -434,6 +430,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 @@ -542,16 +556,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)?; } @@ -592,6 +597,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 3d9625c..ab97f93 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"); @@ -1054,3 +1115,127 @@ fn test_update_already_up_to_date_advances_head_update02() { 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); +} From 765d6f3fd45c8e45d94a556602f102373c76b3b2 Mon Sep 17 00:00:00 2001 From: Kriday Dave Date: Mon, 9 Nov 2026 00:04:22 +0530 Subject: [PATCH 2/3] test(jj): add missing #[ignore] for tests using removed jj_available TST2-03 removed fn jj_available() but merge of X-09 added tests that still called it. Match the #[ignore = "requires jj"] pattern. --- tests/jj_adapter_test.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/jj_adapter_test.rs b/tests/jj_adapter_test.rs index 8625ad0..af06ea6 100644 --- a/tests/jj_adapter_test.rs +++ b/tests/jj_adapter_test.rs @@ -205,6 +205,7 @@ fn test_jj_adjudicate_3way_clean_unilateral_change() { /// the base tip. When head already is the ancestor but the base tip has /// moved on, diffing the tip against head reports files touched even though /// ancestor..head changed nothing. +#[ignore = "requires jj"] #[test] #[ignore = "requires jj"] fn test_jj_docket_diff_uses_ancestor() { @@ -251,6 +252,7 @@ fn test_jj_docket_diff_uses_ancestor() { assert_eq!(docket.intent, "no files changed"); } +#[ignore = "requires jj"] #[test] #[ignore = "requires jj"] fn test_jj_adjudicate_3way_respects_explicit_authors_override() { @@ -317,6 +319,7 @@ fn test_jj_conflicted_commit_detected_not_parsed() { ); } +#[ignore = "requires jj"] #[test] #[ignore = "requires jj"] fn test_jj_authors_bogus_revset_errors() { From 2a4ccd86b2ccba5b690fd16d9cf617e3604c50f3 Mon Sep 17 00:00:00 2001 From: Oot Bot Date: Mon, 9 Nov 2026 00:33:59 +0530 Subject: [PATCH 3/3] test(jj): fix mangled ignore attributes from rebase merge --- tests/jj_adapter_test.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/jj_adapter_test.rs b/tests/jj_adapter_test.rs index af06ea6..8625ad0 100644 --- a/tests/jj_adapter_test.rs +++ b/tests/jj_adapter_test.rs @@ -205,7 +205,6 @@ fn test_jj_adjudicate_3way_clean_unilateral_change() { /// the base tip. When head already is the ancestor but the base tip has /// moved on, diffing the tip against head reports files touched even though /// ancestor..head changed nothing. -#[ignore = "requires jj"] #[test] #[ignore = "requires jj"] fn test_jj_docket_diff_uses_ancestor() { @@ -252,7 +251,6 @@ fn test_jj_docket_diff_uses_ancestor() { assert_eq!(docket.intent, "no files changed"); } -#[ignore = "requires jj"] #[test] #[ignore = "requires jj"] fn test_jj_adjudicate_3way_respects_explicit_authors_override() { @@ -319,7 +317,6 @@ fn test_jj_conflicted_commit_detected_not_parsed() { ); } -#[ignore = "requires jj"] #[test] #[ignore = "requires jj"] fn test_jj_authors_bogus_revset_errors() {