diff --git a/src/worktree.rs b/src/worktree.rs index 9649e282b..66253c669 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -29,6 +29,8 @@ mod preservation_tests; #[cfg(test)] mod review_recovery_tests; #[cfg(test)] +mod skill_tests; +#[cfg(test)] mod synchronization_tests; #[derive(Clone, Copy)] diff --git a/src/worktree/skill.rs b/src/worktree/skill.rs index 2cb26ff52..168055221 100644 --- a/src/worktree/skill.rs +++ b/src/worktree/skill.rs @@ -1,5 +1,6 @@ //! Standalone skill checkouts start at the Launch directory's committed HEAD. +use std::os::unix::fs::MetadataExt; use std::path::Path; use anyhow::Result; @@ -44,13 +45,9 @@ impl SkillWorktree { let recovered = retained.is_some(); let checkout = match retained { Some(checkout) => checkout, - None => { - let path = tempfile::Builder::new() - .prefix("thirdshift-review-") - .tempdir()? - .keep(); - acquisition::add_review(launch, branch, &path, commit)? - } + None => acquire_in_temp("thirdshift-review-", |path| { + acquisition::add_review(launch, branch, path, commit) + })?, }; Ok(Self { launch: Git::new(root), @@ -62,16 +59,13 @@ impl SkillWorktree { fn checkout(launch: &Git, commit: &str) -> Result { let root = launch.run(&["rev-parse", "--show-toplevel"])?; - // Cleanup belongs to the captured checkout, never TempDir's recursive Drop. - let path = tempfile::Builder::new() - .prefix("thirdshift-skill-") - .tempdir()? - .keep(); - progress::step(format_args!( - "creating skill worktree {} at {commit}", - path.display() - )); - let checkout = acquisition::add(launch, None, &path, commit)?; + let checkout = acquire_in_temp("thirdshift-skill-", |path| { + progress::step(format_args!( + "creating skill worktree {} at {commit}", + path.display() + )); + acquisition::add(launch, None, path, commit) + })?; Ok(Self { launch: Git::new(root), checkout, @@ -130,6 +124,26 @@ impl SkillWorktree { } } +/// Cleanup belongs to the captured checkout, never TempDir's recursive Drop: +/// a failed acquisition can retain work there. Failure removes only the +/// allocated directory itself, still empty, and keeps acquisition's error. +fn acquire_in_temp( + prefix: &str, + acquire: impl FnOnce(&Path) -> Result, +) -> Result { + let directory = tempfile::Builder::new().prefix(prefix).tempdir()?; + let allocated = std::fs::symlink_metadata(directory.path())?; + let path = directory.keep(); + acquire(&path).inspect_err(|_| { + // remove_dir refuses a directory with contents, such as a retained checkout. + if std::fs::symlink_metadata(&path).is_ok_and(|current| { + current.is_dir() && current.dev() == allocated.dev() && current.ino() == allocated.ino() + }) { + let _ = std::fs::remove_dir(&path); + } + }) +} + impl Drop for SkillWorktree { fn drop(&mut self) { self.checkout diff --git a/src/worktree/skill_tests.rs b/src/worktree/skill_tests.rs new file mode 100644 index 000000000..db2989f4a --- /dev/null +++ b/src/worktree/skill_tests.rs @@ -0,0 +1,194 @@ +//! Skill and Review checkout allocation, each run in a child with its own temp root. + +use std::fs; +use std::path::Path; + +use super::*; + +const CHILD: &str = "THIRDSHIFT_SKILL_ALLOCATION_TEST"; +const MISSING_COMMIT: &str = "1234567890123456789012345678901234567890"; + +/// Run the calling test again in a child whose temp root holds only its +/// fixtures, so allocation leftovers can be counted. +fn ran_in_isolated_child(configure: impl FnOnce(&Path, &mut std::process::Command)) -> bool { + if std::env::var_os(CHILD).is_some() { + return false; + } + let root = tempfile::tempdir().unwrap(); + let name = std::thread::current().name().unwrap().to_string(); + // The launch fixture's empty clone warns, so judge only the child's status. + let mut command = std::process::Command::new(std::env::current_exe().unwrap()); + command + .args(["--exact", &name, "--nocapture"]) + .env(CHILD, "1") + .env("TMPDIR", root.path()); + configure(root.path(), &mut command); + let output = command.output().unwrap(); + assert!( + output.status.success(), + "{}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + true +} + +fn allocations() -> Vec { + fs::read_dir(std::env::temp_dir()) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .filter(|name| { + name.starts_with("thirdshift-review-") || name.starts_with("thirdshift-skill-") + }) + .collect() +} + +#[test] +fn a_failed_skill_checkout_leaves_no_allocation_behind() { + if ran_in_isolated_child(|_, _| {}) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + + assert!(SkillWorktree::at(&launch, MISSING_COMMIT).is_err()); + + assert_eq!(allocations(), Vec::::new()); +} + +#[test] +fn a_failed_review_checkout_leaves_no_allocation_behind() { + if ran_in_isolated_child(|_, _| {}) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + + assert!(SkillWorktree::on_pull_request(&launch, "feature", MISSING_COMMIT).is_err()); + + assert_eq!(allocations(), Vec::::new()); + assert_eq!(local_head(&launch, "feature").unwrap(), None); +} + +#[test] +fn a_branch_occupied_after_review_preflight_keeps_the_occupier_and_no_allocation() { + if ran_in_isolated_child(|root, command| { + // Occupy the branch once, between preflight and Git's add. + let real = String::from_utf8( + std::process::Command::new("sh") + .args(["-c", "command -v git"]) + .output() + .unwrap() + .stdout, + ) + .unwrap(); + let bin = root.join("bin"); + fs::create_dir(&bin).unwrap(); + crate::test_support::write_executable( + &bin.join("git"), + &format!( + "#!/bin/sh\nif [ \"$1 $2 $3\" = 'worktree add -b' ] && [ ! -e '{root}/occupier' ]; then\n '{real}' worktree add -q -b \"$4\" '{root}/occupier' \"$6\" || exit 97\nfi\nexec '{real}' \"$@\"\n", + root = root.display(), + real = real.trim(), + ), + ); + let path = std::env::join_paths( + std::iter::once(bin).chain(std::env::split_paths(&std::env::var_os("PATH").unwrap())), + ) + .unwrap(); + command.env("PATH", path); + }) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + let start = launch.run(&["rev-parse", "HEAD"]).unwrap(); + + let Err(error) = SkillWorktree::on_pull_request(&launch, "feature", &start) else { + panic!("a branch occupied after preflight was acquired"); + }; + + assert!(format!("{error:#}").contains("feature"), "{error:#}"); + assert_eq!(allocations(), Vec::::new()); + let occupier = std::env::temp_dir().join("occupier"); + assert_eq!( + Git::new(&occupier).run(&["rev-parse", "HEAD"]).unwrap(), + start + ); + assert_eq!(local_head(&launch, "feature").unwrap(), Some(start)); +} + +#[test] +fn a_branch_occupied_before_review_is_refused_before_allocation() { + if ran_in_isolated_child(|_, _| {}) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + let start = launch.run(&["rev-parse", "HEAD"]).unwrap(); + let occupier = std::env::temp_dir().join("occupier"); + launch + .run(&[ + "worktree", + "add", + "-q", + "-b", + "feature", + occupier.to_str().unwrap(), + ]) + .unwrap(); + + let Err(error) = SkillWorktree::on_pull_request(&launch, "feature", &start) else { + panic!("an occupied branch was acquired"); + }; + + assert!( + format!("{error:#}").contains("Occupied checkout"), + "{error:#}" + ); + assert_eq!(allocations(), Vec::::new()); + assert_eq!(local_head(&launch, "feature").unwrap(), Some(start)); +} + +#[test] +fn hook_work_from_a_failed_skill_or_review_checkout_survives() { + if ran_in_isolated_child(|_, _| {}) { + return; + } + let (temp, launch) = super::tests::launch_directory(); + let start = launch.run(&["rev-parse", "HEAD"]).unwrap(); + crate::test_support::write_executable( + &temp.path().join("hooks/post-checkout"), + "#!/bin/sh\necho hook work > hook.txt\nexit 1\n", + ); + + assert!(SkillWorktree::at(&launch, &start).is_err()); + assert!(SkillWorktree::on_pull_request(&launch, "feature", &start).is_err()); + + let mut retained = allocations(); + retained.sort_by_key(|name| name.starts_with("thirdshift-review-")); + assert!( + retained.len() == 2 + && retained[0].starts_with("thirdshift-skill-") + && retained[1].starts_with("thirdshift-review-"), + "{retained:?}" + ); + for name in retained { + let path = std::env::temp_dir().join(name); + assert_eq!( + fs::read_to_string(path.join("hook.txt")).unwrap(), + "hook work\n" + ); + } + assert_eq!(local_head(&launch, "feature").unwrap(), Some(start)); +} + +#[test] +fn successful_skill_and_review_checkouts_are_cleaned_up() { + if ran_in_isolated_child(|_, _| {}) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + let start = launch.run(&["rev-parse", "HEAD"]).unwrap(); + + drop(SkillWorktree::at(&launch, &start).unwrap()); + drop(SkillWorktree::on_pull_request(&launch, "feature", &start).unwrap()); + + assert_eq!(allocations(), Vec::::new()); +}