feat(maestro): shared unsaved-work probe for worktrees (#79)
One function, unsaved_work(), returns the structured breakdown — uncommitted tracked changes, untracked non-ignored files, unpushed commits — so the kill guard and the natural-exit classifier (#82) answer from the same place. Ignored files never count: every Rust checkout has a target/, and counting those would leave no session killable. WorktreeManager::remove drops its own dirty-tree guard; git itself still refuses a dirty tree, and the real guard moves onto the kill.
This commit is contained in:
@@ -10,7 +10,9 @@ pub enum WorktreeError {
|
||||
Spawn(#[from] std::io::Error),
|
||||
#[error("git exited with code {code}: {stderr}")]
|
||||
NonZero { code: i32, stderr: String },
|
||||
/// The worktree holds unsaved work; removal was refused (#56).
|
||||
/// A registered worktree lives under the tree about to be removed; git
|
||||
/// hides its contents from `git status`, so removal would delete its
|
||||
/// uncommitted work unseen (#54 review).
|
||||
#[error("left {} on disk: {}", .path.display(), .reason)]
|
||||
Guarded { path: PathBuf, reason: String },
|
||||
}
|
||||
@@ -126,12 +128,13 @@ impl WorktreeManager {
|
||||
Ok(path)
|
||||
}
|
||||
|
||||
/// `git -C <base_repo> worktree remove <path>` — but only when the
|
||||
/// worktree holds nothing unique (#56). A worktree with uncommitted
|
||||
/// changes, or with commits reachable from no other ref, is refused and
|
||||
/// left on disk so the operator can rescue it; cleanup is unconditional,
|
||||
/// which makes this guard the only thing standing between a stray kill
|
||||
/// and destroyed work.
|
||||
/// `git -C <base_repo> worktree remove <path>`.
|
||||
///
|
||||
/// Unconditional (#79): the guard moved off the worktree and onto the
|
||||
/// kill, so by the time removal runs the caller has already chosen to
|
||||
/// destroy the tree — the kill only gets here after
|
||||
/// [`unsaved_work`] came back empty. Still refused for a tree holding a
|
||||
/// nested registered worktree, which `git status` cannot see.
|
||||
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
|
||||
@@ -147,12 +150,6 @@ impl WorktreeManager {
|
||||
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(),
|
||||
reason,
|
||||
});
|
||||
}
|
||||
let out = Command::new("git")
|
||||
.arg("-C")
|
||||
.arg(base_repo)
|
||||
@@ -251,22 +248,49 @@ fn branch_relpath(branch: &str) -> Option<PathBuf> {
|
||||
Some(candidate.to_path_buf())
|
||||
}
|
||||
|
||||
/// `Some(reason)` when deleting `path` would destroy work.
|
||||
/// The unsaved-work probe (#79): what would be lost if `path` were deleted.
|
||||
///
|
||||
/// Only an unclean working tree qualifies. `git worktree remove` deletes the
|
||||
/// checkout but keeps `refs/heads/<branch>`, 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<Option<String>, WorktreeError> {
|
||||
// `--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()));
|
||||
/// One function, three answers, shared by every caller that must make the
|
||||
/// same decision — the kill guard refuses on any non-empty result and the
|
||||
/// natural-exit classifier (#82) retains on it — so the two paths cannot
|
||||
/// drift. A worktree whose directory is already gone holds nothing to save.
|
||||
pub fn unsaved_work(path: &Path) -> Result<maestro_protocol::UnsavedWork, WorktreeError> {
|
||||
if std::fs::symlink_metadata(path).is_err() {
|
||||
return Ok(maestro_protocol::UnsavedWork::default());
|
||||
}
|
||||
Ok(None)
|
||||
// `--ignored=no` keeps build output out of it; ignored files never block
|
||||
// (#79), or no Rust checkout would ever be killable.
|
||||
let out = run_git(path, &["status", "--porcelain", "--ignored=no"])?;
|
||||
let status = String::from_utf8_lossy(&out.stdout);
|
||||
let mut dirty = 0u32;
|
||||
let mut untracked = 0u32;
|
||||
for line in status.lines() {
|
||||
if line.starts_with("??") {
|
||||
untracked += 1;
|
||||
} else {
|
||||
dirty += 1;
|
||||
}
|
||||
}
|
||||
|
||||
// Detached HEAD has no branch; the tip itself still works as a starting
|
||||
// point for the reachability query below.
|
||||
let head = run_git(path, &["rev-parse", "--abbrev-ref", "HEAD"])?;
|
||||
let branch = String::from_utf8_lossy(&head.stdout).trim().to_string();
|
||||
let tip: &str = if branch == "HEAD" { "HEAD" } else { &branch };
|
||||
let count = run_git(path, &["rev-list", "--count", tip, "--not", "--remotes"])?;
|
||||
let unpushed: u32 = String::from_utf8_lossy(&count.stdout)
|
||||
.trim()
|
||||
.parse()
|
||||
.map_err(|e| WorktreeError::NonZero {
|
||||
code: 0,
|
||||
stderr: format!("unparseable rev-list count: {e}"),
|
||||
})?;
|
||||
|
||||
Ok(maestro_protocol::UnsavedWork {
|
||||
dirty_tracked_files: dirty,
|
||||
untracked_files: untracked,
|
||||
unpushed_commits: unpushed,
|
||||
})
|
||||
}
|
||||
|
||||
/// `git -C <base_repo> worktree prune` — drops registrations whose directory
|
||||
@@ -280,10 +304,7 @@ fn prune(base_repo: &Path) -> Result<(), WorktreeError> {
|
||||
check(out)
|
||||
}
|
||||
|
||||
fn run_git(
|
||||
dir: &Path,
|
||||
args: impl IntoIterator<Item = &'static str>,
|
||||
) -> Result<Output, WorktreeError> {
|
||||
fn run_git(dir: &Path, args: &[&str]) -> Result<Output, WorktreeError> {
|
||||
let mut cmd = Command::new("git");
|
||||
cmd.arg("-C").arg(dir).args(args);
|
||||
let out = cmd.output()?;
|
||||
|
||||
@@ -93,6 +93,38 @@ fn run(cmd: &mut Command) {
|
||||
);
|
||||
}
|
||||
|
||||
/// An empty bare repository at `dir`, usable as a push target (#79).
|
||||
pub fn init_bare_repo(dir: &Path) {
|
||||
run(Command::new("git")
|
||||
.arg("init")
|
||||
.arg("-q")
|
||||
.arg("--bare")
|
||||
.arg(dir.to_str().expect("non-utf8 bare repo path")));
|
||||
}
|
||||
|
||||
/// Push every branch of `repo` to `origin`, creating remote-tracking refs.
|
||||
///
|
||||
/// The unsaved-work probe (#79) counts commits reachable from no
|
||||
/// remote-tracking ref as unpushed, so tests that want a killable worktree
|
||||
/// must give its branch one. Safe to call again: re-adding an existing
|
||||
/// remote is ignored.
|
||||
pub fn push_all_to_origin(repo: &Path, origin: &Path) {
|
||||
let _ = Command::new("git")
|
||||
.arg("-C")
|
||||
.arg(repo)
|
||||
.args([
|
||||
"remote",
|
||||
"add",
|
||||
"origin",
|
||||
origin.to_str().expect("non-utf8 origin path"),
|
||||
])
|
||||
.status();
|
||||
run(Command::new("git")
|
||||
.arg("-C")
|
||||
.arg(repo)
|
||||
.args(["push", "-q", "--all", "origin"]));
|
||||
}
|
||||
|
||||
/// Async polling sibling of [`wait_for`] for tokio tests.
|
||||
pub async fn wait_for_async<F, Fut>(timeout: Duration, mut cond: F)
|
||||
where
|
||||
|
||||
@@ -6,8 +6,8 @@ mod common;
|
||||
use std::path::PathBuf;
|
||||
use std::process::Command;
|
||||
|
||||
use common::init_repo;
|
||||
use maestro_daemon::worktree::WorktreeManager;
|
||||
use common::{init_bare_repo, init_repo, push_all_to_origin};
|
||||
use maestro_daemon::worktree::{unsaved_work, WorktreeManager};
|
||||
use tempfile::TempDir;
|
||||
|
||||
fn worktree_branches(repo: &PathBuf) -> Vec<String> {
|
||||
@@ -134,6 +134,123 @@ fn add_twice_generates_distinct_paths() {
|
||||
assert!(p2.exists());
|
||||
}
|
||||
|
||||
/// The unsaved-work probe (#79) counts uncommitted tracked changes.
|
||||
#[test]
|
||||
fn the_probe_counts_uncommitted_tracked_changes() {
|
||||
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/dirty").expect("add");
|
||||
std::fs::write(path.join("README"), b"unsaved edits\n").unwrap();
|
||||
|
||||
let w = unsaved_work(&path).expect("probe");
|
||||
assert_eq!(w.dirty_tracked_files, 1);
|
||||
assert_eq!(w.untracked_files, 0);
|
||||
}
|
||||
|
||||
/// Untracked sources count; a staged new file is also unsaved tracked work.
|
||||
#[test]
|
||||
fn the_probe_counts_untracked_files() {
|
||||
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/untracked").expect("add");
|
||||
std::fs::write(path.join("notes.txt"), b"wip\n").unwrap();
|
||||
std::fs::write(path.join("more.txt"), b"also wip\n").unwrap();
|
||||
|
||||
let w = unsaved_work(&path).expect("probe");
|
||||
assert_eq!(w.untracked_files, 2);
|
||||
assert_eq!(w.dirty_tracked_files, 0);
|
||||
|
||||
run_git(&path, &["add", "notes.txt"]);
|
||||
let w = unsaved_work(&path).expect("probe");
|
||||
assert_eq!(w.dirty_tracked_files, 1, "staged counts as uncommitted");
|
||||
assert_eq!(w.untracked_files, 1);
|
||||
}
|
||||
|
||||
/// Ignored files NEVER block: every Rust checkout has a `target/`, and
|
||||
/// counting ignored output would leave no session killable (#79).
|
||||
#[test]
|
||||
fn the_probe_ignores_ignored_files() {
|
||||
let repo_dir = TempDir::new().unwrap();
|
||||
let repo = repo_dir.path().to_path_buf();
|
||||
init_repo(&repo);
|
||||
|
||||
// .gitignore must be committed, or it would itself read as untracked.
|
||||
std::fs::write(repo.join(".gitignore"), b"/target\n").unwrap();
|
||||
run_git(&repo, &["add", ".gitignore"]);
|
||||
run_git(&repo, &["commit", "-q", "-m", "ignore target"]);
|
||||
|
||||
let mgr = WorktreeManager::new(None);
|
||||
let path = mgr.add_new(&repo, "feat/builds").expect("add");
|
||||
std::fs::create_dir_all(path.join("target")).unwrap();
|
||||
std::fs::write(path.join("target").join("app.o"), b"binary\n").unwrap();
|
||||
|
||||
let w = unsaved_work(&path).expect("probe");
|
||||
assert_eq!(w.dirty_tracked_files, 0);
|
||||
assert_eq!(w.untracked_files, 0);
|
||||
|
||||
// With the branch pushed too, an ignored-only tree reads fully clean.
|
||||
let origin_dir = TempDir::new().unwrap();
|
||||
init_bare_repo(origin_dir.path());
|
||||
push_all_to_origin(&repo, origin_dir.path());
|
||||
let w = unsaved_work(&path).expect("probe after push");
|
||||
assert!(w.is_empty(), "ignored-only worktree must read clean: {w:?}");
|
||||
}
|
||||
|
||||
/// Commits reachable from no remote-tracking ref block; pushing the branch
|
||||
/// clears them. With no remote at all every commit is unreachable from one,
|
||||
/// which is exactly why feature branches almost always refuse (#79).
|
||||
#[test]
|
||||
fn the_probe_counts_unpushed_commits_until_the_branch_is_pushed() {
|
||||
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/branch").expect("add");
|
||||
|
||||
let w = unsaved_work(&path).expect("probe");
|
||||
assert!(
|
||||
w.unpushed_commits >= 1,
|
||||
"a fresh branch has no remote-tracking ref: {w:?}"
|
||||
);
|
||||
|
||||
let origin_dir = TempDir::new().unwrap();
|
||||
init_bare_repo(origin_dir.path());
|
||||
push_all_to_origin(&repo, origin_dir.path());
|
||||
|
||||
let w = unsaved_work(&path).expect("probe after push");
|
||||
assert_eq!(w.unpushed_commits, 0, "{w:?}");
|
||||
assert!(w.is_empty(), "pushed and clean must be empty: {w:?}");
|
||||
}
|
||||
|
||||
/// The full breakdown names each blocker with its count.
|
||||
#[test]
|
||||
fn the_probe_reports_every_blocker_at_once() {
|
||||
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/mixed").expect("add");
|
||||
std::fs::write(path.join("README"), b"edited\n").unwrap();
|
||||
std::fs::write(path.join("scratch.txt"), b"untracked\n").unwrap();
|
||||
|
||||
let w = unsaved_work(&path).expect("probe");
|
||||
assert_eq!(w.dirty_tracked_files, 1);
|
||||
assert_eq!(w.untracked_files, 1);
|
||||
assert!(w.unpushed_commits >= 1);
|
||||
let summary = w.to_string();
|
||||
assert!(summary.contains("1 uncommitted file"), "{summary}");
|
||||
assert!(summary.contains("1 untracked file"), "{summary}");
|
||||
assert!(summary.contains("unpushed commit"), "{summary}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn remove_prunes_worktree() {
|
||||
let repo_dir = TempDir::new().unwrap();
|
||||
@@ -211,8 +328,14 @@ fn add_existing_checks_out_branch_without_creating_it() {
|
||||
);
|
||||
}
|
||||
|
||||
/// Removal is guarded upstream now (#79): the kill refuses unsaved work
|
||||
/// before anything is torn down, so by the time `remove` runs the tree is
|
||||
/// clean. Git still refuses a dirty tree on its own — this asserts that last
|
||||
/// line of defense — and committed-but-unpushed work does not block removal,
|
||||
/// because `worktree remove` keeps `refs/heads/<branch>` and the commits stay
|
||||
/// reachable.
|
||||
#[test]
|
||||
fn remove_refuses_worktree_with_uncommitted_changes() {
|
||||
fn remove_still_relies_on_git_refusing_a_dirty_tree() {
|
||||
let repo_dir = TempDir::new().unwrap();
|
||||
let repo = repo_dir.path().to_path_buf();
|
||||
init_repo(&repo);
|
||||
@@ -225,10 +348,6 @@ fn remove_refuses_worktree_with_uncommitted_changes() {
|
||||
.expect_err("dirty worktree must be refused");
|
||||
assert!(path.exists(), "guarded worktree must stay on disk");
|
||||
|
||||
// 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", "saved"]);
|
||||
|
||||
@@ -410,7 +529,8 @@ fn a_guarded_leftover_is_never_adopted() {
|
||||
let first = mgr.add_new(&repo, "feat/resume").expect("add");
|
||||
std::fs::write(first.join("notes.txt"), b"unsaved\n").unwrap();
|
||||
|
||||
// The guard refuses, leaving it on disk.
|
||||
// Removal is refused — git itself refuses a dirty tree — leaving it on
|
||||
// disk; the daemon-side guard lives on the kill (#79).
|
||||
mgr.remove(&repo, &first)
|
||||
.expect_err("dirty tree is guarded");
|
||||
|
||||
|
||||
@@ -143,6 +143,62 @@ pub struct SessionWorktree {
|
||||
pub base_repo: PathBuf,
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Unsaved-work breakdown (#79)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/// What a worktree holds that would be lost if its directory were destroyed.
|
||||
///
|
||||
/// The kill guard refuses `DELETE /api/sessions/:id` with this breakdown, and
|
||||
/// natural exits (#82) classify with the same daemon-side probe, so the two
|
||||
/// paths cannot drift. Ignored files never count: every Rust checkout has a
|
||||
/// `target/`, and counting those would leave no session killable.
|
||||
#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)]
|
||||
pub struct UnsavedWork {
|
||||
/// Tracked files with uncommitted modifications or staged changes.
|
||||
#[serde(default)]
|
||||
pub dirty_tracked_files: u32,
|
||||
/// Untracked files that are not ignored.
|
||||
#[serde(default)]
|
||||
pub untracked_files: u32,
|
||||
/// Commits on the branch reachable from no remote-tracking ref.
|
||||
#[serde(default)]
|
||||
pub unpushed_commits: u32,
|
||||
}
|
||||
|
||||
impl UnsavedWork {
|
||||
pub fn is_empty(&self) -> bool {
|
||||
self.dirty_tracked_files == 0 && self.untracked_files == 0 && self.unpushed_commits == 0
|
||||
}
|
||||
}
|
||||
|
||||
impl std::fmt::Display for UnsavedWork {
|
||||
/// "3 uncommitted files, 2 untracked files, 2 unpushed commits" — zero
|
||||
/// parts are omitted; all-zero renders "no unsaved work".
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
let parts = [
|
||||
(self.dirty_tracked_files, "uncommitted file"),
|
||||
(self.untracked_files, "untracked file"),
|
||||
(self.unpushed_commits, "unpushed commit"),
|
||||
];
|
||||
let said: Vec<String> = parts
|
||||
.into_iter()
|
||||
.filter(|(n, _)| *n > 0)
|
||||
.map(|(n, unit)| {
|
||||
if n == 1 {
|
||||
format!("1 {unit}")
|
||||
} else {
|
||||
format!("{n} {unit}s")
|
||||
}
|
||||
})
|
||||
.collect();
|
||||
if said.is_empty() {
|
||||
return write!(f, "no unsaved work");
|
||||
}
|
||||
write!(f, "{}", said.join(", "))
|
||||
}
|
||||
}
|
||||
|
||||
/// HTTP `POST /api/sessions` body.
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
pub struct CreateSessionRequest {
|
||||
@@ -248,6 +304,10 @@ pub struct BranchInfo {
|
||||
pub struct ErrorInfo {
|
||||
pub code: String,
|
||||
pub message: String,
|
||||
/// Set only by the kill guard (#79): the unsaved work that blocked
|
||||
/// `DELETE /api/sessions/:id`, so a client can render counts directly.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub unsaved_work: Option<UnsavedWork>,
|
||||
}
|
||||
|
||||
/// The envelope every `/api/*` failure is wrapped in: `{"error": {…}}`.
|
||||
@@ -268,6 +328,7 @@ impl ApiError {
|
||||
error: ErrorInfo {
|
||||
code: code.into(),
|
||||
message: message.into(),
|
||||
unsaved_work: None,
|
||||
},
|
||||
}
|
||||
}
|
||||
@@ -578,6 +639,7 @@ mod tests {
|
||||
ClientControl::Error(ErrorInfo {
|
||||
code: "bad_frame".into(),
|
||||
message: "decode failed".into(),
|
||||
unsaved_work: None,
|
||||
}),
|
||||
resize,
|
||||
] {
|
||||
|
||||
Reference in New Issue
Block a user