diff --git a/crates/maestro-cli/src/cli.rs b/crates/maestro-cli/src/cli.rs index 3f497e6..877d344 100644 --- a/crates/maestro-cli/src/cli.rs +++ b/crates/maestro-cli/src/cli.rs @@ -67,6 +67,11 @@ pub struct SpawnArgs { /// Branch name for the worktree (required with --worktree-from). #[arg(long)] pub branch: Option, + /// Check out an existing branch instead of creating one. Without it the + /// daemon runs `worktree add -b`, which fails if the branch is already + /// there. + #[arg(long)] + pub existing_branch: bool, /// Remove the worktree when the session is killed. Kept for wire /// compatibility (#56): the daemon now always cleans up on kill, guarded /// against unsaved work. @@ -170,6 +175,31 @@ mod tests { } } + /// Review fix: without a flag the CLI always sent `create_branch: true`, + /// so the daemon's existing-branch path was unreachable and spawning onto + /// a branch that already existed just failed. + #[test] + fn parses_spawn_onto_an_existing_branch() { + let cli = Cli::try_parse_from([ + "maestro", + "spawn", + "--cwd", + "/tmp", + "--command", + "bash", + "--worktree-from", + "/repo", + "--branch", + "feat/existing", + "--existing-branch", + ]) + .unwrap(); + let Command::Spawn(a) = cli.command else { + panic!("expected spawn") + }; + assert!(a.existing_branch); + } + #[test] fn spawn_worktree_requires_branch() { let err = Cli::try_parse_from([ diff --git a/crates/maestro-cli/src/sessions.rs b/crates/maestro-cli/src/sessions.rs index e3ca37e..1a087dc 100644 --- a/crates/maestro-cli/src/sessions.rs +++ b/crates/maestro-cli/src/sessions.rs @@ -44,7 +44,7 @@ pub async fn run_spawn(client: &DaemonClient, args: SpawnArgs) -> Result None, }; diff --git a/crates/maestro-daemon/src/worktree.rs b/crates/maestro-daemon/src/worktree.rs index 0e8ad5e..c723066 100644 --- a/crates/maestro-daemon/src/worktree.rs +++ b/crates/maestro-daemon/src/worktree.rs @@ -69,6 +69,9 @@ impl WorktreeManager { create_branch: bool, ) -> Result { let path = self.target_path(base_repo, branch); + // Clear registrations whose directory is gone before testing the + // path, so a previously-leaked entry doesn't block this branch. + prune(base_repo)?; if path.symlink_metadata().is_ok() { return Err(WorktreeError::PathExists { path }); } @@ -97,6 +100,14 @@ impl WorktreeManager { /// which makes this guard the only thing standing between a stray kill /// and destroyed work. pub fn remove(&self, base_repo: &Path, path: &Path) -> Result<(), WorktreeError> { + // Gone from disk already (operator `rm -rf`, or a create that failed + // half-way). Probing it with git would fail and be misread as unsaved + // work, stranding the registration — and since paths are + // deterministic, `worktree add` would then refuse that branch forever + // with "missing but already registered" (#56 review). + if !path.exists() { + return prune(base_repo); + } if let Some(reason) = unsafe_to_remove(path)? { return Err(WorktreeError::Guarded { path: path.to_path_buf(), @@ -128,55 +139,35 @@ fn sanitize_branch_for_path(branch: &str) -> String { out } -/// `Some(reason)` when deleting `path` would destroy work: uncommitted -/// changes, or commits reachable from no ref other than the worktree's own -/// branch (`git worktree remove` keeps the branch, but nothing checks it out -/// anymore — treat its commits as at risk anyway, per #56). +/// `Some(reason)` when deleting `path` would destroy work. +/// +/// Only an unclean working tree qualifies. `git worktree remove` deletes the +/// checkout but keeps `refs/heads/`, so committed work — pushed or +/// not — stays reachable and is never at risk. Guarding on "commits on no +/// other ref" instead would refuse every ordinary unpushed feature branch, +/// and because the path is deterministic that branch could then never be +/// spawned again (#56 review). fn unsafe_to_remove(path: &Path) -> Result, WorktreeError> { - let status = run_git(path, ["status", "--porcelain"])?; + // `--ignored=no` keeps build output out of it; untracked sources count, + // since removal would delete them. + let status = run_git(path, ["status", "--porcelain", "--ignored=no"])?; if !status.stdout.is_empty() { return Ok(Some("has uncommitted changes".into())); } - - let head = String::from_utf8_lossy(&run_git(path, ["rev-parse", "HEAD"])?.stdout) - .trim() - .to_string(); - let branch = - String::from_utf8_lossy(&run_git(path, ["rev-parse", "--abbrev-ref", "HEAD"])?.stdout) - .trim() - .to_string(); - - // Every ref except this worktree's own branch counts as a safe home for - // its commits. Detached HEAD excludes everything. - let refs = - String::from_utf8_lossy(&run_git(path, ["for-each-ref", "--format=%(refname)"])?.stdout) - .lines() - .filter(|r| match branch.as_str() { - "HEAD" => true, - b => *r != format!("refs/heads/{b}"), - }) - .map(str::to_string) - .collect::>(); - - let mut cmd = Command::new("git"); - cmd.arg("-C").arg(path).args(["rev-list", "--count", &head]); - if !refs.is_empty() { - cmd.arg("--not").args(&refs); - } - let out = cmd.output()?; - check(out.clone())?; - let count: usize = String::from_utf8_lossy(&out.stdout) - .trim() - .parse() - .unwrap_or(0); - if count > 0 { - return Ok(Some(format!( - "has {count} commit(s) not present on any other ref" - ))); - } Ok(None) } +/// `git -C worktree prune` — drops registrations whose directory +/// no longer exists. +fn prune(base_repo: &Path) -> Result<(), WorktreeError> { + let out = Command::new("git") + .arg("-C") + .arg(base_repo) + .args(["worktree", "prune"]) + .output()?; + check(out) +} + fn run_git( dir: &Path, args: impl IntoIterator, diff --git a/crates/maestro-daemon/tests/worktree.rs b/crates/maestro-daemon/tests/worktree.rs index fdb6945..722875a 100644 --- a/crates/maestro-daemon/tests/worktree.rs +++ b/crates/maestro-daemon/tests/worktree.rs @@ -206,18 +206,50 @@ fn remove_refuses_worktree_with_uncommitted_changes() { .expect_err("dirty worktree must be refused"); assert!(path.exists(), "guarded worktree must stay on disk"); - // Committing the change doesn't help while it is the only copy… + // Committing it makes removal safe: `worktree remove` keeps + // `refs/heads/feat/dirty`, so the work is still reachable. Refusing here + // instead would make every unpushed branch a one-shot, since the path is + // deterministic and the leftover tree blocks the next spawn (#56 review). run_git(&path, &["add", "."]); - run_git(&path, &["commit", "-q", "-m", "unsaved"]); + run_git(&path, &["commit", "-q", "-m", "saved"]); + + mgr.remove(&repo, &path).expect("committed work can go"); + assert!(!path.exists()); + assert!( + worktree_branches(&repo).contains(&"main".to_string()), + "the branch survives removal" + ); + let branches = String::from_utf8( + Command::new("git") + .arg("-C") + .arg(&repo) + .args(["for-each-ref", "--format=%(refname:short)", "refs/heads"]) + .output() + .unwrap() + .stdout, + ) + .unwrap(); + assert!(branches.contains("feat/dirty"), "{branches}"); +} + +/// A worktree whose directory vanished must be pruned, not misread as holding +/// unsaved work — otherwise the registration is stranded and, because paths +/// are deterministic, that branch can never be spawned again (#56 review). +#[test] +fn remove_prunes_a_worktree_whose_directory_vanished() { + let repo_dir = TempDir::new().unwrap(); + let repo = repo_dir.path().to_path_buf(); + init_repo(&repo); + + let mgr = WorktreeManager::new(None); + let path = mgr.add_new(&repo, "feat/gone").expect("add"); + std::fs::remove_dir_all(&path).unwrap(); mgr.remove(&repo, &path) - .expect_err("unique commit must be refused"); - assert!(path.exists()); - - // …but once the commit is reachable from main, removal is safe. - run_git(&repo, &["merge", "-q", "--no-edit", "feat/dirty"]); - mgr.remove(&repo, &path).expect("merged worktree can go"); - assert!(!path.exists()); + .expect("a vanished worktree prunes"); + // The registration is gone too, so the same branch can be spawned again. + mgr.add_existing(&repo, "feat/gone") + .expect("branch is spawnable again"); } fn run_git(dir: &std::path::Path, args: &[&str]) { diff --git a/crates/maestro-tui/src/app.rs b/crates/maestro-tui/src/app.rs index aae1d75..98eac6c 100644 --- a/crates/maestro-tui/src/app.rs +++ b/crates/maestro-tui/src/app.rs @@ -464,7 +464,16 @@ fn encode_key(key: &InputKey) -> Option> { InputKey::Home => Some(b"\x1b[H".to_vec()), InputKey::End => Some(b"\x1b[F".to_vec()), InputKey::Delete => Some(b"\x1b[3~".to_vec()), - InputKey::Paste(text) => Some(text.as_bytes().to_vec()), + InputKey::Paste(text) => { + // Wrap in the DECSET 2004 markers. Raw bytes look like typed keys + // to an app that enabled bracketed paste, so a multi-line paste + // would execute line by line instead of arriving as one block. + let mut out = Vec::with_capacity(text.len() + 12); + out.extend_from_slice(b"\x1b[200~"); + out.extend_from_slice(text.as_bytes()); + out.extend_from_slice(b"\x1b[201~"); + Some(out) + } InputKey::Mouse { .. } => None, } } @@ -697,12 +706,14 @@ mod tests { /// Regression review fix: `Paste(_)` used to be silently dropped, so a /// paste into an attached session vanished once bracketed paste was - /// enabled — the payload must be forwarded verbatim. + /// enabled. It also has to keep its DECSET 2004 markers — without them a + /// remote shell reads the payload as typed keys and runs a multi-line + /// paste line by line. #[test] - fn a_bracketed_paste_is_forwarded_verbatim_to_the_session() { + fn a_bracketed_paste_reaches_the_session_with_its_markers() { assert_eq!( - encode_key(&InputKey::Paste("cd /tmp/foo".into())), - Some(b"cd /tmp/foo".to_vec()) + encode_key(&InputKey::Paste("cd /tmp/foo\nls".into())), + Some(b"\x1b[200~cd /tmp/foo\nls\x1b[201~".to_vec()) ); } diff --git a/web/src/dirAutocomplete.ts b/web/src/dirAutocomplete.ts index 0eb5efe..3ec9114 100644 --- a/web/src/dirAutocomplete.ts +++ b/web/src/dirAutocomplete.ts @@ -109,6 +109,14 @@ export function attachDirAutocomplete(input: HTMLInputElement): DirAutocompleteH } function applyFilter(entries: string[], prefix: string): void { + // A fetch can outlive the dismissal that was meant to close this popup + // (Tab-commit re-arms a refresh, then blur dismisses 120ms later). Showing + // it now would anchor a dropdown under an unfocused input that nothing + // closes until the next interaction. + if (dismissed || document.activeElement !== input) { + close(); + return; + } const lower = prefix.toLowerCase(); filtered = entries.filter((e) => e.toLowerCase().startsWith(lower)); highlight = filtered.length > 0 ? 0 : -1; diff --git a/web/src/spawnForm.test.ts b/web/src/spawnForm.test.ts index 4b7dd1b..4559526 100644 --- a/web/src/spawnForm.test.ts +++ b/web/src/spawnForm.test.ts @@ -8,7 +8,6 @@ const base: SpawnFormValues = { name: "amber-fox", worktreeEnabled: false, worktreeBranch: "", - worktreeCleanup: false, }; describe("buildSpawnRequest", () => { @@ -24,9 +23,9 @@ describe("buildSpawnRequest", () => { expect(res).toEqual({ req: { cwd: base.cwd, command: base.command, metadata: {} } }); }); - it("attaches a trimmed branch and cleanup flag when worktree is enabled", () => { + it("attaches a trimmed branch when worktree is enabled, and always cleans up", () => { const res = buildSpawnRequest( - { ...base, worktreeEnabled: true, worktreeBranch: " feat/foo ", worktreeCleanup: true }, + { ...base, worktreeEnabled: true, worktreeBranch: " feat/foo " }, "/home/user/project", ); expect(res).toEqual({ diff --git a/web/src/spawnForm.ts b/web/src/spawnForm.ts index 7b78eb4..d28d295 100644 --- a/web/src/spawnForm.ts +++ b/web/src/spawnForm.ts @@ -14,7 +14,6 @@ export interface SpawnFormSeed { name?: string; worktreeEnabled?: boolean; worktreeBranch?: string; - worktreeCleanup?: boolean; cwdRawVisible?: boolean; } @@ -26,7 +25,6 @@ export interface SpawnFormValues { name: string; worktreeEnabled: boolean; worktreeBranch: string; - worktreeCleanup: boolean; /** Whether the cwd lives in the raw text input (vs a picked recent row). */ cwdRawVisible?: boolean; } @@ -55,7 +53,9 @@ export function buildSpawnRequest( req.worktree = { base_repo: baseRepo, branch, - cleanup_on_kill: values.worktreeCleanup, + // The daemon always cleans up, guarded against unsaved work (#56); + // the wire field stays for older daemons. + cleanup_on_kill: true, }; } @@ -130,10 +130,6 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { Branch - @@ -155,9 +151,6 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { const wtBody = form.querySelector(".wt-body") as HTMLElement; const wtRepo = form.querySelector(".wt-repo") as HTMLElement; const branchInput = form.querySelector("input[name=worktree_branch]") as HTMLInputElement; - const cleanupInput = form.querySelector( - "input[name=worktree_cleanup_on_kill]", - ) as HTMLInputElement; const nameInput = form.querySelector("input[name=name]") as HTMLInputElement; // ---- form state ------------------------------------------------------ @@ -172,14 +165,22 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { // settles — so the fetch can't race past and clobber the seeded directory. let seedCwd: string | null = prefill.cwd ?? null; + // `emit` stays silent until the initial load settles and goes silent again + // once the form is destroyed. Construction runs with `cwd === ""` and the + // seed is only restored two fetches later, so emitting in between would + // blank the leaf's draft; and `multiview.render()` rebuilds forms while the + // old one's fetches are still in flight, so a destroyed form emitting would + // clobber the live one's leaf (#57 review). + let ready = false; + let destroyed = false; const emit = () => { + if (!ready || destroyed) return; opts.onChange?.({ command, cwd, name: nameInput.value, worktreeEnabled: wtEnabled.checked, worktreeBranch: branchInput.value, - worktreeCleanup: cleanupInput.checked, cwdRawVisible: !cwdRawWrap.hidden, }); }; @@ -296,6 +297,9 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { cwdRaw.focus(); cwdRaw.select(); } + // Both the visibility and the directory are draft state: without this a + // render before the operator types snaps back to the recents picker. + emit(); } cwdRaw.addEventListener("input", () => { @@ -357,10 +361,9 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { } else { cwdRawWrap.hidden = true; } - // The seeded cwd is in place — a restored worktree can probe now. + // Everything seeded: the form may publish from here on. + ready = true; if (wtEnabled.checked) void probeGitRoot(); - // Publish it: construction ran `emit()` with an empty cwd, so without this - // the leaf's draft keeps `cwd: ""` and the next render reseeds it blank. emit(); } @@ -423,7 +426,10 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { wtRepo.textContent = `repo: ${root} (inferred)`; } else { inferredBaseRepo = null; + // Publish the untick too, or the next render seeds the box checked with + // no inferred repo and submitting fails. wtEnabled.checked = false; + emit(); wtBody.hidden = true; wtHint.hidden = false; wtHint.textContent = "Not inside a git repository — pick a dir in a repo to use a worktree."; @@ -440,14 +446,12 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { wtHint.hidden = true; } }); - cleanupInput.addEventListener("change", emit); // A seeded worktree restores its checkbox/body up front; its gitroot probe // waits for loadRecents to apply the seeded cwd. - if (prefill.worktreeEnabled || prefill.worktreeBranch || prefill.worktreeCleanup) { + if (prefill.worktreeEnabled || prefill.worktreeBranch) { wtEnabled.checked = Boolean(prefill.worktreeEnabled); if (prefill.worktreeBranch) branchInput.value = prefill.worktreeBranch; - cleanupInput.checked = Boolean(prefill.worktreeCleanup); wtBody.hidden = !wtEnabled.checked; wtHint.hidden = true; emit(); @@ -471,8 +475,7 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { name: nameInput.value, worktreeEnabled: wtEnabled.checked, worktreeBranch: branchInput.value, - worktreeCleanup: cleanupInput.checked, - }, + }, inferredBaseRepo, ); if ("error" in built) { @@ -494,6 +497,7 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { return { el: form, destroy() { + destroyed = true; autocomplete.destroy(); form.remove(); },