diff --git a/src/worktree.rs b/src/worktree.rs index 7bdfaf3..432a7b4 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -4,13 +4,13 @@ //! Per-commit git worktrees so each parallel review sees the working tree //! pinned to its commit's SHA, not the latest state of the main checkout. //! -//! Layout: `/.boro/worktrees///` — a `git worktree -//! add --detach` checkout of ``, where `` is `review` / `build` -//! / `test`. Per-command subdirs let you run e.g. `boro build` and -//! `boro test` against the same commit at the same time without their -//! worktree paths colliding. The main repo is left untouched, so the user -//! can keep editing / building / switching branches there while reviews are -//! in flight. +//! Layout: `/.boro/worktrees////` — a +//! `git worktree add --detach` checkout of ``, where `` is +//! `review` / `build` / `test`. Per-command and per-run subdirs let you run +//! multiple commands, or even multiple instances of the same command, against +//! the same commit at the same time without their worktree paths colliding. The +//! main repo is left untouched, so the user can keep editing / building / +//! switching branches there while reviews are in flight. //! //! Cleanup: a [`Worktree`] is an RAII handle whose `Drop` runs //! `git worktree remove --force`. Tokio aborts (e.g. on Ctrl-C) drop the @@ -19,14 +19,171 @@ //! crashes — scoped to the current command's subdir so it doesn't disturb //! a concurrent boro run that's using a different command. +use std::fs::{self, File, OpenOptions, TryLockError}; +use std::io::{ErrorKind, Write}; use std::path::{Path, PathBuf}; use std::process::Command; +use std::sync::{Arc, Mutex, OnceLock, Weak}; +use std::time::{SystemTime, UNIX_EPOCH}; use anyhow::{Context, Result}; use crate::verbose::VerboseDest; const WORKTREES_SUBDIR: &str = ".boro/worktrees"; +const RUN_LOCK_FILE: &str = ".boro-run.lock"; + +fn command_dir_path(main_repo: &Path, command_label: &str) -> PathBuf { + main_repo.join(WORKTREES_SUBDIR).join(command_label) +} + +fn process_run_id() -> &'static str { + static RUN_ID: OnceLock = OnceLock::new(); + RUN_ID.get_or_init(|| { + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0); + format!("{}-{now}", std::process::id()) + }) +} + +fn run_dir_path(main_repo: &Path, command_label: &str) -> PathBuf { + command_dir_path(main_repo, command_label).join(process_run_id()) +} + +fn worktree_path(main_repo: &Path, command_label: &str, sha: &str) -> PathBuf { + run_dir_path(main_repo, command_label).join(sha) +} + +fn ensure_no_symlink_components(root: &Path, path: &Path) -> Result<()> { + let rel = path + .strip_prefix(root) + .with_context(|| format!("{} not under {}", path.display(), root.display()))?; + let mut current = root.to_path_buf(); + for component in rel.components() { + current.push(component); + match fs::symlink_metadata(¤t) { + Ok(meta) if meta.file_type().is_symlink() => { + anyhow::bail!("refusing symlinked path {}", current.display()); + } + Ok(_) => {} + Err(e) if e.kind() == ErrorKind::NotFound => return Ok(()), + Err(e) => { + return Err(e).with_context(|| format!("symlink_metadata {}", current.display())); + } + } + } + Ok(()) +} + +fn validate_cleanup_dir(main_repo: &Path, expected_parent: &Path, path: &Path) -> Result<()> { + if path.parent() != Some(expected_parent) { + anyhow::bail!( + "refusing path outside expected directory tree: {}", + path.display() + ); + } + ensure_no_symlink_components(main_repo, path)?; + let meta = fs::symlink_metadata(path) + .with_context(|| format!("symlink_metadata {}", path.display()))?; + let file_type = meta.file_type(); + if file_type.is_symlink() { + anyhow::bail!("refusing symlinked directory {}", path.display()); + } + if !file_type.is_dir() { + anyhow::bail!("expected directory at {}", path.display()); + } + Ok(()) +} + +fn remove_empty_dir_if_possible(path: &Path) { + let _ = fs::remove_dir(path); +} + +struct RunLease { + run_dir: PathBuf, + lock_path: PathBuf, + file: File, +} + +impl RunLease { + fn create(run_dir: &Path) -> Result { + fs::create_dir_all(run_dir).with_context(|| format!("create {}", run_dir.display()))?; + let lock_path = run_dir.join(RUN_LOCK_FILE); + let mut file = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .open(&lock_path) + .with_context(|| format!("open {}", lock_path.display()))?; + file.try_lock() + .with_context(|| format!("lock {}", lock_path.display()))?; + file.set_len(0) + .with_context(|| format!("truncate {}", lock_path.display()))?; + writeln!(file, "pid={}", std::process::id()) + .with_context(|| format!("write {}", lock_path.display()))?; + writeln!(file, "run_id={}", process_run_id()) + .with_context(|| format!("write {}", lock_path.display()))?; + let _ = file.sync_all(); + Ok(Self { + run_dir: run_dir.to_path_buf(), + lock_path, + file, + }) + } +} + +impl Drop for RunLease { + fn drop(&mut self) { + if run_dir_has_entries_other_than_lock(&self.run_dir) { + return; + } + let _ = self.file.unlock(); + let _ = fs::remove_file(&self.lock_path); + let _ = fs::remove_dir(&self.run_dir); + if let Some(command_dir) = self.run_dir.parent() { + remove_empty_dir_if_possible(command_dir); + } + } +} + +fn run_dir_has_entries_other_than_lock(run_dir: &Path) -> bool { + let entries = match fs::read_dir(run_dir) { + Ok(entries) => entries, + Err(_) => return true, + }; + for entry in entries.flatten() { + if entry.file_name() != RUN_LOCK_FILE { + return true; + } + } + false +} + +fn run_leases() -> &'static Mutex>> { + static RUN_LEASES: OnceLock>>> = OnceLock::new(); + RUN_LEASES.get_or_init(|| Mutex::new(Vec::new())) +} + +fn run_lease(main_repo: &Path, command_label: &str) -> Result> { + let command_dir = command_dir_path(main_repo, command_label); + ensure_no_symlink_components(main_repo, &command_dir)?; + let run_dir = run_dir_path(main_repo, command_label); + ensure_no_symlink_components(main_repo, &run_dir)?; + let mut leases = run_leases() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + leases.retain(|lease| lease.strong_count() > 0); + for lease in leases.iter().filter_map(Weak::upgrade) { + if lease.run_dir == run_dir { + return Ok(lease); + } + } + let lease = Arc::new(RunLease::create(&run_dir)?); + leases.push(Arc::downgrade(&lease)); + Ok(lease) +} /// RAII handle for a `git worktree add --detach ` checkout. /// @@ -35,6 +192,7 @@ const WORKTREES_SUBDIR: &str = ".boro/worktrees"; pub struct Worktree { main_repo: PathBuf, path: PathBuf, + _lease: Option>, } /// A posted patch series applied in a disposable worktree as input to `boro apply`. @@ -136,6 +294,7 @@ impl ImportedSeries { let worktree = Worktree { main_repo: main_repo.to_path_buf(), path, + _lease: None, }; dest.line(format!( "message-id: applying posted series in {}", @@ -196,9 +355,9 @@ impl ImportedSeries { } impl Worktree { - /// Create `/.boro/worktrees///` checked out at `sha`. + /// Create `/.boro/worktrees////` checked out at `sha`. /// `command_label` is the boro subcommand name (`"review"` / `"build"` / `"test"`); per-command - /// subdirs let concurrent runs of different commands coexist on the same SHA. + /// and per-run subdirs let concurrent runs coexist on the same SHA. /// /// The main repo's working tree is unchanged. Concurrent calls from /// different worker tasks are safe: git serializes worktree-add via the @@ -209,9 +368,13 @@ impl Worktree { sha: &str, dest: &VerboseDest, ) -> Result { - let parent = main_repo.join(WORKTREES_SUBDIR).join(command_label); - std::fs::create_dir_all(&parent).with_context(|| format!("create {}", parent.display()))?; - let path = parent.join(sha); + let lease = run_lease(main_repo, command_label)?; + let path = worktree_path(main_repo, command_label, sha); + let parent = path + .parent() + .ok_or_else(|| anyhow::anyhow!("invalid worktree path {}", path.display()))?; + ensure_no_symlink_components(main_repo, parent)?; + fs::create_dir_all(parent).with_context(|| format!("create {}", parent.display()))?; let out = Command::new("git") .current_dir(main_repo) @@ -235,6 +398,7 @@ impl Worktree { Ok(Self { main_repo: main_repo.to_path_buf(), path, + _lease: Some(lease), }) } @@ -272,82 +436,242 @@ impl Drop for Worktree { } } -/// Best-effort cleanup of `.boro/worktrees//*` left over from prior runs. +/// Best-effort cleanup of stale Git worktree registry records and dead boro run dirs. /// /// 1. `git worktree prune --expire=now` drops registry records whose dirs are /// already gone (e.g. user `rm -rf`'d them). This is repo-wide and safe — it /// only touches entries whose on-disk path is missing, so a concurrent boro /// run with a still-existing worktree is not affected. -/// 2. For each `/.boro/worktrees///` dir found -/// on disk: try `git worktree remove --force`; if that doesn't clear it, -/// fall back to `fs::remove_dir_all`. Failures are logged and ignored — a -/// concurrent boro run of *the same* command may legitimately own one of -/// these dirs. Other commands' subdirs are left alone so e.g. a startup -/// sweep for `boro build` won't disturb a running `boro test`. +/// 2. Each run dir has a lock file held by its owning process. If the lock is +/// held, the run is live and left alone. If the lock can be acquired, the +/// owner is gone, so its registered worktrees and files can be removed. +/// Dirs without a lock file are left alone because ownership is unknown. /// 3. Append `.boro/` to `/.git/info/exclude` if not already /// present, so the worktree dirs don't show up as untracked in /// `git status` of the main checkout. pub fn sweep_stale(main_repo: &Path, command_label: &str, dest: &VerboseDest) -> Result<()> { ensure_gitignore(main_repo, dest); + let command_dir = command_dir_path(main_repo, command_label); + if let Err(e) = ensure_no_symlink_components(main_repo, &command_dir) { + dest.line(format!( + "worktree: startup sweep skipped {} ({e:#})", + command_dir.display() + )); + return Ok(()); + } + let _ = Command::new("git") .current_dir(main_repo) .args(["worktree", "prune", "--expire=now"]) .output(); - let parent = main_repo.join(WORKTREES_SUBDIR).join(command_label); - let entries = match std::fs::read_dir(&parent) { + let entries = match fs::read_dir(&command_dir) { Ok(it) => it, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(e) if e.kind() == ErrorKind::NotFound => return Ok(()), Err(e) => { - return Err(e).with_context(|| format!("read_dir {}", parent.display())); + return Err(e).with_context(|| format!("read_dir {}", command_dir.display())); } }; let mut removed = 0u32; let mut kept = 0u32; for entry in entries.flatten() { - let path = entry.path(); - if !path.is_dir() { + let run_dir = entry.path(); + let file_type = match entry.file_type() { + Ok(file_type) => file_type, + Err(e) => { + dest.line(format!( + "worktree: sweep skipped {} ({e})", + run_dir.display() + )); + kept += 1; + continue; + } + }; + if file_type.is_symlink() { + dest.line(format!( + "worktree: sweep skipped {} (refusing symlinked run dir)", + run_dir.display() + )); + kept += 1; continue; } - let out = Command::new("git") - .current_dir(main_repo) - .arg("worktree") - .arg("remove") - .arg("--force") - .arg(&path) - .output(); - let removed_via_git = matches!(out, Ok(ref o) if o.status.success()); - if !removed_via_git && path.exists() { - // Not a registered worktree — wipe the directory. If this races a - // concurrent boro that just registered it, `remove_dir_all` will - // either succeed (their files vanish) or fail; we tolerate either. - match std::fs::remove_dir_all(&path) { - Ok(()) => {} - Err(e) => { - dest.line(format!("worktree: sweep skipped {} ({e})", path.display(),)); - kept += 1; - continue; - } - } + if !file_type.is_dir() { + continue; + } + match sweep_run_dir(main_repo, &command_dir, &run_dir, dest) { + SweepOutcome::Removed => removed += 1, + SweepOutcome::Kept => kept += 1, } - removed += 1; } + let _ = Command::new("git") + .current_dir(main_repo) + .args(["worktree", "prune", "--expire=now"]) + .output(); + if removed > 0 || kept > 0 { dest.line(format!( - "worktree: startup sweep removed {removed} stale dir(s), skipped {kept}", + "worktree: startup sweep removed {removed} stale run(s), skipped {kept}", )); } Ok(()) } +enum SweepOutcome { + Removed, + Kept, +} + +enum RunLock { + Stale(File), + Live, + Unknown, +} + +fn sweep_run_dir( + main_repo: &Path, + command_dir: &Path, + run_dir: &Path, + dest: &VerboseDest, +) -> SweepOutcome { + if let Err(e) = validate_cleanup_dir(main_repo, command_dir, run_dir) { + dest.line(format!( + "worktree: sweep skipped {} ({e:#})", + run_dir.display() + )); + return SweepOutcome::Kept; + } + + let lock = match try_lock_run(run_dir) { + Ok(lock) => lock, + Err(e) => { + dest.line(format!( + "worktree: sweep skipped {} ({e:#})", + run_dir.display() + )); + return SweepOutcome::Kept; + } + }; + let RunLock::Stale(file) = lock else { + return SweepOutcome::Kept; + }; + + let entries = match fs::read_dir(run_dir) { + Ok(entries) => entries, + Err(e) => { + dest.line(format!( + "worktree: sweep skipped {} ({e})", + run_dir.display() + )); + return SweepOutcome::Kept; + } + }; + let mut all_removed = true; + for entry in entries.flatten() { + let path = entry.path(); + if entry.file_name() == RUN_LOCK_FILE { + continue; + } + let file_type = match entry.file_type() { + Ok(file_type) => file_type, + Err(e) => { + dest.line(format!("worktree: sweep skipped {} ({e})", path.display())); + all_removed = false; + continue; + } + }; + if file_type.is_symlink() { + dest.line(format!( + "worktree: sweep skipped {} (refusing symlinked cleanup path)", + path.display() + )); + all_removed = false; + continue; + } + if file_type.is_dir() { + if !remove_stale_dir(main_repo, run_dir, &path, dest) { + all_removed = false; + } + } else if let Err(e) = fs::remove_file(&path) { + dest.line(format!("worktree: sweep skipped {} ({e})", path.display())); + all_removed = false; + } + } + + if !all_removed || run_dir_has_entries_other_than_lock(run_dir) { + return SweepOutcome::Kept; + } + + drop(file); + let lock_path = run_dir.join(RUN_LOCK_FILE); + let _ = fs::remove_file(lock_path); + match fs::remove_dir(run_dir) { + Ok(()) => { + remove_empty_dir_if_possible(command_dir); + SweepOutcome::Removed + } + Err(e) if e.kind() == ErrorKind::NotFound => { + remove_empty_dir_if_possible(command_dir); + SweepOutcome::Removed + } + Err(e) => { + dest.line(format!( + "worktree: sweep skipped {} ({e})", + run_dir.display() + )); + SweepOutcome::Kept + } + } +} + +fn try_lock_run(run_dir: &Path) -> Result { + let lock_path = run_dir.join(RUN_LOCK_FILE); + let file = match OpenOptions::new().read(true).write(true).open(&lock_path) { + Ok(file) => file, + Err(e) if e.kind() == ErrorKind::NotFound => return Ok(RunLock::Unknown), + Err(e) => return Err(e).with_context(|| format!("open {}", lock_path.display())), + }; + match file.try_lock() { + Ok(()) => Ok(RunLock::Stale(file)), + Err(TryLockError::WouldBlock) => Ok(RunLock::Live), + Err(TryLockError::Error(e)) => { + Err(e).with_context(|| format!("lock {}", lock_path.display())) + } + } +} + +fn remove_stale_dir(main_repo: &Path, run_dir: &Path, path: &Path, dest: &VerboseDest) -> bool { + if let Err(e) = validate_cleanup_dir(main_repo, run_dir, path) { + dest.line(format!( + "worktree: sweep skipped {} ({e:#})", + path.display() + )); + return false; + } + + let out = Command::new("git") + .current_dir(main_repo) + .arg("worktree") + .arg("remove") + .arg("--force") + .arg(path) + .output(); + let removed_via_git = matches!(out, Ok(ref o) if o.status.success()); + if !removed_via_git && path.exists() { + if let Err(e) = fs::remove_dir_all(path) { + dest.line(format!("worktree: sweep skipped {} ({e})", path.display())); + } + } + !path.exists() +} + fn ensure_gitignore(main_repo: &Path, dest: &VerboseDest) { let exclude_path = main_repo.join(".git/info/exclude"); let line = ".boro/"; - let existing = std::fs::read_to_string(&exclude_path).unwrap_or_default(); + let existing = fs::read_to_string(&exclude_path).unwrap_or_default(); if existing.lines().any(|l| l.trim() == line) { return; } @@ -357,7 +681,7 @@ fn ensure_gitignore(main_repo: &Path, dest: &VerboseDest) { } new_contents.push_str(line); new_contents.push('\n'); - if let Err(e) = std::fs::write(&exclude_path, new_contents) { + if let Err(e) = fs::write(&exclude_path, new_contents) { dest.line(format!( "worktree: could not append `.boro/` to {}: {e}", exclude_path.display(), @@ -370,13 +694,15 @@ fn ensure_gitignore(main_repo: &Path, dest: &VerboseDest) { } } -#[cfg(all(test, unix))] +#[cfg(test)] mod tests { use super::*; - use std::fs; - use std::os::unix::fs::PermissionsExt; + use tempfile::TempDir; + + #[cfg(unix)] + use std::os::unix::fs::{symlink, PermissionsExt}; - fn git(repo: &Path, args: &[&str]) -> String { + fn run_git_test(repo: &Path, args: &[&str]) -> String { let out = Command::new("git") .current_dir(repo) .args(args) @@ -384,32 +710,82 @@ mod tests { .unwrap(); assert!( out.status.success(), - "git {args:?}: {}", + "git {} failed: {}", + args.join(" "), String::from_utf8_lossy(&out.stderr) ); String::from_utf8_lossy(&out.stdout).trim().to_string() } - fn init_repo() -> tempfile::TempDir { - let tmp = tempfile::tempdir().unwrap(); - git(tmp.path(), &["init", "-q"]); - git(tmp.path(), &["config", "user.name", "Boro Test"]); - git(tmp.path(), &["config", "user.email", "boro@example.com"]); + fn init_repo() -> TempDir { + let tmp = TempDir::new().unwrap(); + run_git_test(tmp.path(), &["init", "-q"]); + run_git_test(tmp.path(), &["config", "user.name", "Boro Test"]); + run_git_test(tmp.path(), &["config", "user.email", "boro@example.com"]); fs::write(tmp.path().join("README"), "base\n").unwrap(); - git(tmp.path(), &["add", "README"]); - git(tmp.path(), &["commit", "-q", "-m", "base"]); - let base = git(tmp.path(), &["rev-parse", "HEAD"]); + run_git_test(tmp.path(), &["add", "README"]); + run_git_test(tmp.path(), &["commit", "-q", "-m", "base"]); + let base = run_git_test(tmp.path(), &["rev-parse", "HEAD"]); fs::write(tmp.path().join("upstream.txt"), "upstream\n").unwrap(); - git(tmp.path(), &["add", "upstream.txt"]); - git(tmp.path(), &["commit", "-q", "-m", "series base"]); + run_git_test(tmp.path(), &["add", "upstream.txt"]); + run_git_test(tmp.path(), &["commit", "-q", "-m", "series base"]); fs::write(tmp.path().join("README"), "posted\n").unwrap(); - git(tmp.path(), &["add", "README"]); - git(tmp.path(), &["commit", "-q", "-m", "imported"]); - git(tmp.path(), &["branch", "series-tip"]); - git(tmp.path(), &["reset", "--hard", &base]); + run_git_test(tmp.path(), &["add", "README"]); + run_git_test(tmp.path(), &["commit", "-q", "-m", "imported"]); + run_git_test(tmp.path(), &["branch", "series-tip"]); + run_git_test(tmp.path(), &["reset", "--hard", &base]); tmp } + fn repo_with_commit() -> (TempDir, String) { + let d = init_repo(); + fs::write(d.path().join("f.c"), "int x = 1;\n").unwrap(); + run_git_test(d.path(), &["add", "f.c"]); + run_git_test(d.path(), &["commit", "-q", "-m", "extra"]); + let sha = run_git_test(d.path(), &["rev-parse", "HEAD"]); + (d, sha) + } + + fn write_unlocked_run_lock(run_dir: &Path) { + fs::create_dir_all(run_dir).unwrap(); + fs::write( + run_dir.join(RUN_LOCK_FILE), + format!("pid={}\nrun_id=test\n", std::process::id()), + ) + .unwrap(); + } + + fn lock_run_dir(run_dir: &Path) -> File { + fs::create_dir_all(run_dir).unwrap(); + let file = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .open(run_dir.join(RUN_LOCK_FILE)) + .unwrap(); + file.try_lock().unwrap(); + file + } + + fn git_worktree_add(repo: &Path, path: &Path, sha: &str) { + let out = Command::new("git") + .current_dir(repo) + .arg("worktree") + .arg("add") + .arg("--detach") + .arg(path) + .arg(sha) + .output() + .unwrap(); + assert!( + out.status.success(), + "git worktree add {} {sha} failed: {}", + path.display(), + String::from_utf8_lossy(&out.stderr) + ); + } + + #[cfg(unix)] fn fake_b4(dir: &Path) -> PathBuf { let path = dir.join("b4"); fs::write( @@ -431,6 +807,7 @@ mod tests { path } + #[cfg(unix)] fn failing_b4(dir: &Path) -> PathBuf { let path = dir.join("b4-fail"); fs::write( @@ -446,6 +823,7 @@ mod tests { path } + #[cfg(unix)] fn blocking_b4(dir: &Path) -> PathBuf { let path = dir.join("b4-block"); fs::write(&path, "#!/bin/sh\nexec sleep 30\n").unwrap(); @@ -455,27 +833,178 @@ mod tests { path } + #[cfg(unix)] fn worktree_list(repo: &Path) -> String { - git(repo, &["worktree", "list", "--porcelain"]) + run_git_test(repo, &["worktree", "list", "--porcelain"]) } + #[cfg(unix)] fn interrupt_signal() -> tokio::signal::unix::Signal { tokio::signal::unix::signal(tokio::signal::unix::SignalKind::interrupt()).unwrap() } + #[test] + fn worktree_path_uses_process_run_namespace() { + let d = TempDir::new().unwrap(); + let path = worktree_path(d.path(), "review", "abc123"); + assert_eq!( + path, + d.path() + .join(WORKTREES_SUBDIR) + .join("review") + .join(process_run_id()) + .join("abc123") + ); + } + + #[test] + fn create_uses_process_run_namespace() { + let (d, sha) = repo_with_commit(); + let worktree = + Worktree::create(d.path(), "review", &sha, &VerboseDest::new(false)).unwrap(); + let path = worktree.path().to_path_buf(); + let run_dir = path.parent().unwrap().to_path_buf(); + let command_dir = run_dir.parent().unwrap().to_path_buf(); + + assert!(path.starts_with( + d.path() + .join(WORKTREES_SUBDIR) + .join("review") + .join(process_run_id()) + )); + assert!(path.join("f.c").is_file(), "missing {}", path.display()); + + drop(worktree); + assert!(!path.exists(), "drop left {}", path.display()); + assert!(!run_dir.exists(), "drop left {}", run_dir.display()); + assert!(!command_dir.exists(), "drop left {}", command_dir.display()); + } + + #[test] + fn sweep_stale_removes_unlocked_registered_run_dir() { + let (d, sha) = repo_with_commit(); + let command_dir = d.path().join(WORKTREES_SUBDIR).join("review"); + let run_dir = command_dir.join("stale-run"); + write_unlocked_run_lock(&run_dir); + let worktree = run_dir.join(&sha); + git_worktree_add(d.path(), &worktree, &sha); + let listed = run_git_test(d.path(), &["worktree", "list", "--porcelain"]); + assert!(listed.contains(&worktree.to_string_lossy().to_string())); + + sweep_stale(d.path(), "review", &VerboseDest::new(false)).unwrap(); + + assert!(!worktree.exists(), "sweep left {}", worktree.display()); + assert!(!run_dir.exists(), "sweep left {}", run_dir.display()); + assert!( + !command_dir.exists(), + "sweep left {}", + command_dir.display() + ); + let listed = run_git_test(d.path(), &["worktree", "list", "--porcelain"]); + assert!(!listed.contains(&worktree.to_string_lossy().to_string())); + } + + #[test] + fn sweep_stale_keeps_live_locked_run_dirs() { + let d = init_repo(); + let run_dir = d + .path() + .join(WORKTREES_SUBDIR) + .join("review") + .join("live-run"); + let live_file = run_dir.join("deadbeef").join("live.txt"); + fs::create_dir_all(live_file.parent().unwrap()).unwrap(); + fs::write(&live_file, "still in use\n").unwrap(); + let _lock = lock_run_dir(&run_dir); + + sweep_stale(d.path(), "review", &VerboseDest::new(false)).unwrap(); + + assert!(live_file.exists(), "sweep removed {}", live_file.display()); + } + + #[test] + fn sweep_stale_keeps_existing_command_worktree_dirs() { + let d = init_repo(); + let live_file = d + .path() + .join(WORKTREES_SUBDIR) + .join("review") + .join("other-run") + .join("deadbeef") + .join("live.txt"); + fs::create_dir_all(live_file.parent().unwrap()).unwrap(); + fs::write(&live_file, "still in use\n").unwrap(); + + sweep_stale(d.path(), "review", &VerboseDest::new(false)).unwrap(); + + assert!(live_file.exists(), "sweep removed {}", live_file.display()); + } + + #[cfg(unix)] + #[test] + fn sweep_stale_skips_symlinked_run_dirs() { + let d = init_repo(); + let command_dir = d.path().join(WORKTREES_SUBDIR).join("review"); + fs::create_dir_all(&command_dir).unwrap(); + let target = d.path().join("outside-run"); + fs::create_dir_all(&target).unwrap(); + let target_file = target.join("keep.txt"); + fs::write(&target_file, "keep me\n").unwrap(); + let run_dir = command_dir.join("link-run"); + symlink(&target, &run_dir).unwrap(); + + sweep_stale(d.path(), "review", &VerboseDest::new(false)).unwrap(); + + assert!(run_dir.exists(), "sweep removed {}", run_dir.display()); + assert!( + target_file.exists(), + "sweep removed {}", + target_file.display() + ); + } + + #[cfg(unix)] + #[test] + fn sweep_stale_skips_symlinked_worktree_dirs() { + let d = init_repo(); + let run_dir = d + .path() + .join(WORKTREES_SUBDIR) + .join("review") + .join("stale-run"); + write_unlocked_run_lock(&run_dir); + let target = d.path().join("outside-worktree"); + fs::create_dir_all(&target).unwrap(); + let target_file = target.join("keep.txt"); + fs::write(&target_file, "keep me\n").unwrap(); + let worktree = run_dir.join("deadbeef"); + symlink(&target, &worktree).unwrap(); + + sweep_stale(d.path(), "review", &VerboseDest::new(false)).unwrap(); + + assert!(run_dir.exists(), "sweep removed {}", run_dir.display()); + assert!(worktree.exists(), "sweep removed {}", worktree.display()); + assert!( + target_file.exists(), + "sweep removed {}", + target_file.display() + ); + } + + #[cfg(unix)] #[tokio::test] async fn message_id_import_isolated_from_source_checkout() { let repo = init_repo(); let tools = tempfile::tempdir().unwrap(); let b4 = fake_b4(tools.path()); fs::write(repo.path().join("README"), "target\n").unwrap(); - git(repo.path(), &["add", "README"]); - git(repo.path(), &["commit", "-q", "-m", "target"]); - let base = git(repo.path(), &["rev-parse", "HEAD"]); - let series_base = git(repo.path(), &["rev-parse", "series-tip^"]); + run_git_test(repo.path(), &["add", "README"]); + run_git_test(repo.path(), &["commit", "-q", "-m", "target"]); + let base = run_git_test(repo.path(), &["rev-parse", "HEAD"]); + let series_base = run_git_test(repo.path(), &["rev-parse", "series-tip^"]); fs::write(repo.path().join("README"), "locally modified\n").unwrap(); fs::write(repo.path().join("untracked.txt"), "keep me\n").unwrap(); - let status_before = git(repo.path(), &["status", "--short"]); + let status_before = run_git_test(repo.path(), &["status", "--short"]); let worktrees_before = worktree_list(repo.path()); let mut interrupt = interrupt_signal(); @@ -495,10 +1024,16 @@ mod tests { } assert!(range.starts_with(&format!("{series_base}.."))); - assert_eq!(git(repo.path(), &["rev-list", "--count", &range]), "1"); - assert_eq!(git(repo.path(), &["rev-parse", "HEAD"]), base); + assert_eq!( + run_git_test(repo.path(), &["rev-list", "--count", &range]), + "1" + ); + assert_eq!(run_git_test(repo.path(), &["rev-parse", "HEAD"]), base); assert_eq!(worktree_list(repo.path()), worktrees_before); - assert_eq!(git(repo.path(), &["status", "--short"]), status_before); + assert_eq!( + run_git_test(repo.path(), &["status", "--short"]), + status_before + ); assert_eq!( fs::read_to_string(repo.path().join("README")).unwrap(), "locally modified\n" @@ -509,12 +1044,13 @@ mod tests { ); } + #[cfg(unix)] #[tokio::test] async fn failed_message_id_import_cleans_up_temporary_worktree() { let repo = init_repo(); let tools = tempfile::tempdir().unwrap(); let b4 = failing_b4(tools.path()); - let base = git(repo.path(), &["rev-parse", "HEAD"]); + let base = run_git_test(repo.path(), &["rev-parse", "HEAD"]); let worktrees_before = worktree_list(repo.path()); let mut interrupt = interrupt_signal(); @@ -532,17 +1068,18 @@ mod tests { }; assert!(format!("{error:#}").contains("test download failed")); - assert_eq!(git(repo.path(), &["rev-parse", "HEAD"]), base); + assert_eq!(run_git_test(repo.path(), &["rev-parse", "HEAD"]), base); assert_eq!(worktree_list(repo.path()), worktrees_before); - assert!(git(repo.path(), &["status", "--short"]).is_empty()); + assert!(run_git_test(repo.path(), &["status", "--short"]).is_empty()); } + #[cfg(unix)] #[tokio::test] async fn cancelled_message_id_import_cleans_up_temporary_worktree() { let repo = init_repo(); let tools = tempfile::tempdir().unwrap(); let b4 = blocking_b4(tools.path()); - let base = git(repo.path(), &["rev-parse", "HEAD"]); + let base = run_git_test(repo.path(), &["rev-parse", "HEAD"]); let worktrees_before = worktree_list(repo.path()); let mut interrupt = interrupt_signal(); @@ -559,11 +1096,18 @@ mod tests { .await; assert!(result.is_err(), "blocking import was not cancelled"); - assert_eq!(git(repo.path(), &["rev-parse", "HEAD"]), base); + for _ in 0..20 { + if worktree_list(repo.path()) == worktrees_before { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + assert_eq!(run_git_test(repo.path(), &["rev-parse", "HEAD"]), base); assert_eq!(worktree_list(repo.path()), worktrees_before); - assert!(git(repo.path(), &["status", "--short"]).is_empty()); + assert!(run_git_test(repo.path(), &["status", "--short"]).is_empty()); } + #[cfg(unix)] #[tokio::test] async fn invalid_message_id_creates_no_worktree() { let repo = init_repo();