diff --git a/crates/maestro-daemon/src/projects.rs b/crates/maestro-daemon/src/projects.rs index fb31467..68b49c4 100644 --- a/crates/maestro-daemon/src/projects.rs +++ b/crates/maestro-daemon/src/projects.rs @@ -29,9 +29,14 @@ pub const DEFAULT_HOME_IGNORE: &[&str] = &["node_modules", "Downloads", "Library", "go", "snap", "opt"]; /// How long a discovery snapshot is served as-is before the next request -/// triggers a rescan. Short: a scan is a bounded filesystem walk plus one -/// `git worktree list` per hit. -pub const CACHE_TTL: Duration = Duration::from_secs(5); +/// triggers a rescan. +/// +/// A scan walks the roots and runs one `git worktree list` per hit, holding +/// the scanner lock throughout, and clients re-request whenever a draft pane +/// re-mounts — which `multiview.render()` does on unrelated events. Keep this +/// comfortably longer than [`REFRESH_INTERVAL`] so the background timer is +/// what refreshes, and a request only rescans when that timer has failed to. +pub const CACHE_TTL: Duration = Duration::from_secs(180); /// Background refresh cadence. Repositories appear rarely; this keeps the /// served list eventually fresh without anyone asking. pub const REFRESH_INTERVAL: Duration = Duration::from_secs(60); @@ -154,9 +159,15 @@ fn list_worktrees(repo: &Path) -> Option> { (!paths.is_empty()).then_some(paths) } -/// Mtime of the checkout's HEAD file. For a linked worktree `.git` is a file -/// holding a `gitdir:` pointer; resolve it so worktrees report their own HEAD -/// rather than nothing. +/// Mtime of the checkout's reflog, falling back to `HEAD` itself. +/// +/// `HEAD` is only rewritten on checkout — committing updates +/// `refs/heads/` and appends to `logs/HEAD`, leaving `HEAD` alone. A +/// repo worked on daily from one long-lived branch would otherwise report its +/// last *branch switch* as its activity and get dimmed as stale (#54 review). +/// +/// For a linked worktree `.git` is a file holding a `gitdir:` pointer; +/// resolve it so worktrees report their own reflog rather than nothing. fn head_mtime(repo: &Path) -> Option> { let dot_git = repo.join(".git"); let head = if dot_git.is_dir() { @@ -173,6 +184,9 @@ fn head_mtime(repo: &Path) -> Option> { }; p.join("HEAD") }; + // Prefer the reflog next to whichever HEAD we resolved. + let reflog = head.with_file_name("logs").join("HEAD"); + let head = if reflog.exists() { reflog } else { head }; let mtime = std::fs::metadata(head).ok()?.modified().ok()?; Some(DateTime::::from(mtime)) } diff --git a/crates/maestro-daemon/src/worktree.rs b/crates/maestro-daemon/src/worktree.rs index c723066..164021c 100644 --- a/crates/maestro-daemon/src/worktree.rs +++ b/crates/maestro-daemon/src/worktree.rs @@ -39,14 +39,32 @@ impl WorktreeManager { } /// Where a worktree for `branch` will be created: the override root, or - /// `/worktrees/` by default. The path is + /// `
/worktrees/` by default. The path is /// predictable (no UUID suffix), so an existing directory at the target /// is an error rather than a silently different location. + /// + /// `base_repo` is whatever the client picked, which the project list makes + /// easy to point at a *linked worktree* — placing the new tree inside it + /// would nest one worktree in another, and git hides a registered + /// worktree's contents from the outer `git status`, so the outer tree + /// would later look clean and its removal would delete the inner one's + /// uncommitted work (#54 review). Always place against the main checkout. + /// + /// The override root gets the repo's basename too: without it two repos + /// spawning the same branch name target one directory and the second + /// fails with `PathExists`. fn target_path(&self, base_repo: &Path, branch: &str) -> PathBuf { let slug = sanitize_branch_for_path(branch); + let main = main_checkout(base_repo); match &self.root_override { - Some(root) => root.join(slug), - None => base_repo.join("worktrees").join(slug), + Some(root) => { + let repo = main + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + .unwrap_or_else(|| "repo".into()); + root.join(repo).join(slug) + } + None => main.join("worktrees").join(slug), } } @@ -108,6 +126,12 @@ impl WorktreeManager { if !path.exists() { return prune(base_repo); } + if let Some(inner) = nested_worktree(path) { + return Err(WorktreeError::Guarded { + path: path.to_path_buf(), + reason: format!("holds a nested worktree at {}", inner.display()), + }); + } if let Some(reason) = unsafe_to_remove(path)? { return Err(WorktreeError::Guarded { path: path.to_path_buf(), @@ -124,6 +148,45 @@ impl WorktreeManager { } } +/// The repository's main checkout. `--git-common-dir` points at the shared +/// `.git` for every linked worktree, so its parent is the main tree. Falls +/// back to the input when git can't answer. +fn main_checkout(path: &Path) -> PathBuf { + let out = Command::new("git") + .arg("-C") + .arg(path) + .args(["rev-parse", "--path-format=absolute", "--git-common-dir"]) + .output(); + let Ok(out) = out else { + return path.to_path_buf(); + }; + if !out.status.success() { + return path.to_path_buf(); + } + let common = PathBuf::from(String::from_utf8_lossy(&out.stdout).trim().to_string()); + common + .parent() + .map(Path::to_path_buf) + .unwrap_or_else(|| path.to_path_buf()) +} + +/// `Some(inner)` when a registered worktree lives under `path`. Git omits +/// those directories from `git status`, so the dirty-work guard cannot see +/// them and removal would delete them (#54 review). +fn nested_worktree(path: &Path) -> Option { + let out = Command::new("git") + .arg("-C") + .arg(path) + .args(["worktree", "list", "--porcelain"]) + .output() + .ok()?; + let text = String::from_utf8_lossy(&out.stdout); + text.lines() + .filter_map(|l| l.strip_prefix("worktree ")) + .map(PathBuf::from) + .find(|p| p != path && p.starts_with(path)) +} + fn sanitize_branch_for_path(branch: &str) -> String { let mut out = String::with_capacity(branch.len()); for c in branch.chars() { diff --git a/crates/maestro-daemon/tests/worktree.rs b/crates/maestro-daemon/tests/worktree.rs index 722875a..50f63dd 100644 --- a/crates/maestro-daemon/tests/worktree.rs +++ b/crates/maestro-daemon/tests/worktree.rs @@ -70,8 +70,11 @@ fn worktree_root_override_redirects_placement() { let wt_root = TempDir::new().unwrap(); let mgr = WorktreeManager::new(Some(wt_root.path().to_path_buf())); + // The repo's basename sits between the root and the branch slug, so two + // repos spawning the same branch name don't target one directory. + let repo_name = repo.file_name().unwrap(); let path = mgr.add_new(&repo, "feat/example").expect("add worktree"); - assert_eq!(path, wt_root.path().join("feat-example")); + assert_eq!(path, wt_root.path().join(repo_name).join("feat-example")); assert!(path.exists()); } @@ -288,3 +291,75 @@ fn add_fails_when_branch_already_exists() { let msg = format!("{err}"); assert!(msg.contains("git exited"), "{msg}"); } + +/// The project picker lists linked worktrees as pickable rows, so `base_repo` +/// is easily a worktree rather than the main checkout. Placing inside it would +/// nest one worktree in another — and git hides a registered worktree's +/// contents from the outer `git status`, so the outer tree reads clean and +/// removing it deletes the inner one's uncommitted work (#54 review). +#[test] +fn placement_resolves_a_linked_worktree_to_the_main_checkout() { + let repo_dir = TempDir::new().unwrap(); + let repo = repo_dir.path().to_path_buf(); + init_repo(&repo); + + let mgr = WorktreeManager::new(None); + let outer = mgr.add_new(&repo, "feat/outer").expect("add outer"); + + // Spawn again with the *worktree* as the base, the way the picker would. + let inner = mgr.add_new(&outer, "feat/inner").expect("add inner"); + assert!( + !inner.starts_with(&outer), + "must not nest: {inner:?} inside {outer:?}" + ); + assert_eq!( + inner.parent().and_then(|p| p.parent()), + Some(repo.canonicalize().unwrap().as_path()), + "placed against the main checkout" + ); +} + +/// Belt and braces for a nesting that predates this daemon: refuse to remove +/// a worktree that has another registered underneath it. +#[test] +fn remove_refuses_a_worktree_holding_a_nested_one() { + let repo_dir = TempDir::new().unwrap(); + let repo = repo_dir.path().to_path_buf(); + init_repo(&repo); + + let mgr = WorktreeManager::new(None); + let outer = mgr.add_new(&repo, "feat/outer").expect("add outer"); + // Register one by hand inside `outer`, bypassing our placement. + let nested = outer.join("worktrees").join("hand-rolled"); + run_git( + &repo, + &[ + "worktree", + "add", + "-b", + "feat/hand", + nested.to_str().unwrap(), + ], + ); + std::fs::write(nested.join("notes.txt"), b"unsaved\n").unwrap(); + + mgr.remove(&repo, &outer) + .expect_err("a worktree holding a nested one must be refused"); + assert!(nested.join("notes.txt").exists(), "inner work survives"); +} + +/// Two repos spawning the same branch name under one override root must not +/// collide (#54 review). +#[test] +fn override_root_keeps_repos_apart() { + let root_dir = TempDir::new().unwrap(); + let a_dir = TempDir::new().unwrap(); + let b_dir = TempDir::new().unwrap(); + init_repo(a_dir.path()); + init_repo(b_dir.path()); + + let mgr = WorktreeManager::new(Some(root_dir.path().to_path_buf())); + let a = mgr.add_new(a_dir.path(), "feat/shared").expect("repo a"); + let b = mgr.add_new(b_dir.path(), "feat/shared").expect("repo b"); + assert_ne!(a, b, "same branch name in two repos must not collide"); +} diff --git a/web/src/multiview.ts b/web/src/multiview.ts index f31e607..0a727c0 100644 --- a/web/src/multiview.ts +++ b/web/src/multiview.ts @@ -154,13 +154,14 @@ export function createMultiView(opts: MultiViewOptions): MultiView { onSuccess: (session) => { // The pane can be closed while the POST is in flight. Mounting the // session onto a leaf no longer in the tree would strand it in no - // pane at all, so only claim the leaf while it is still there. - const t = activeTab(layout); - const attached = leaves(t.root).includes(leaf); - if (attached) { + // pane at all, so only claim the leaf while it is still there — + // searching every tab, since the operator may have switched tabs + // while the request was in flight. + const host = layout.tabs.find((t) => leaves(t.root).includes(leaf)); + if (host) { leaf.sessionId = session.id; leaf.draft = null; - t.focusedLeafId = leaf.id; + host.focusedLeafId = leaf.id; } render(); opts.onOpenSessionsChange?.();