From 0b7029aad95c11e420c57e10e6ed3402b0a7b7a9 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Sat, 22 Aug 2026 10:18:41 -0500 Subject: [PATCH] fix(prune): protect the remote default branch, not just current and target `gr prune --execute` deleted the local `main` on any repo whose manifest target is not `main`. The guard skipped exactly two branches -- the one checked out and `repo.target_branch()` -- so once the integration target moved to `dev`, `main` was neither and fell out of protection. Nothing was edited to cause this: `main` had been protected only by coincidence, because target and default used to be the same value. Scope of the defect, measured rather than assumed: LOCAL ONLY. The `--remote` path is `git fetch --prune`, which prunes stale remote-tracking refs and does not delete remote branches, so the worst outcome was a local branch recreated from the remote. The fix protects a SET of three: current, target, and the remote's default branch read from `refs/remotes//HEAD`. The default is the only one of the three that asserts "permanent" rather than "currently interesting", which is the property a cleanup rule needs. Resolution is local -- a cleanup verb should not acquire a network failure mode. When the default cannot be resolved, the protected set GROWS rather than shrinks: `main` and `dev` are protected by name and the run says so. A resolution failure that silently dropped a branch would reproduce this exact defect inside its own fix, and would do it with nothing going red. Two witnesses, each killed by exactly one mutation: - target=dev with `main` present: `main` and `dev` survive while a genuinely merged branch is still deleted. That last assertion is a positive control; without it the test would pass against a guard that protected everything. - `origin/HEAD` deleted from a real clone, so resolution genuinely fails rather than being stubbed: both branches survive AND the output states the default could not be determined. A protected-more that is silent still reads as "the default resolved fine" to the next reader. The pre-existing `test_prune_skips_current_and_default` is left in place but does not cover any of this: its fixture has current == target == `main`, so `main` is protected twice over and the test passes with EITHER clause of the old guard removed. It kills no mutant while carrying the name of the guarantee it fails to check. Co-Authored-By: Claude --- src/cli/commands/prune.rs | 35 +++++++++-- src/git/mod.rs | 18 ++++++ tests/test_prune.rs | 123 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 172 insertions(+), 4 deletions(-) diff --git a/src/cli/commands/prune.rs b/src/cli/commands/prune.rs index 773d0e5c..4d11834e 100644 --- a/src/cli/commands/prune.rs +++ b/src/cli/commands/prune.rs @@ -1,14 +1,15 @@ //! Prune command implementation //! -//! Deletes local branches that have been merged into the default branch. +//! Deletes local branches that have been merged into the manifest target. //! Optionally prunes remote tracking refs. use crate::cli::output::Output; use crate::core::manifest::Manifest; use crate::core::repo::{filter_repos, RepoInfo}; use crate::git::branch::{delete_local_branch, is_branch_merged, list_local_branches}; -use crate::git::{get_current_branch, open_repo, path_exists}; +use crate::git::{get_current_branch, get_default_branch, open_repo, path_exists}; use crate::util::log_cmd; +use std::collections::BTreeSet; use std::path::Path; use std::process::Command; @@ -63,11 +64,37 @@ pub fn run_prune( } }; + // Branches that must never be pruned. Three members, not two: the branch + // checked out, the manifest target, and the remote's DEFAULT branch. The + // default is the only one that asserts "permanent" rather than "currently + // interesting" -- and until the target moved off main it was protected + // only by coincidence, because target and default were the same value. + let mut protected: BTreeSet = BTreeSet::new(); + protected.insert(current_branch.clone()); + protected.insert(repo.target_branch().to_string()); + match get_default_branch(&git_repo, &repo.sync_remote) { + Some(default_branch) => { + protected.insert(default_branch); + } + None => { + // Unknown default: protect MORE, never less. A resolution failure + // that silently shrinks this set reproduces the exact defect the + // set exists to prevent, one level up, inside its own fix -- and + // it would fail with nothing going red. Say so out loud, because + // a protected-more that is silent still reads as "resolved fine". + protected.insert("main".to_string()); + protected.insert("dev".to_string()); + Output::warning(&format!( + "{}: could not determine the default branch (no {}/HEAD); protecting 'main' and 'dev' by name", + repo.name, repo.sync_remote + )); + } + } + let mut merged_branches: Vec = Vec::new(); for branch in &branches { - // Skip current branch and default branch - if branch == ¤t_branch || branch == repo.target_branch() { + if protected.contains(branch) { continue; } diff --git a/src/git/mod.rs b/src/git/mod.rs index cd7ebcca..4c49e849 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -198,6 +198,24 @@ pub fn get_current_branch(repo: &Repository) -> Result { } } +/// Resolve the remote's default branch from `refs/remotes//HEAD`. +/// +/// Local only: a cleanup verb must not acquire a network failure mode, so this +/// reads the ref that `clone` writes rather than asking the remote. Returns +/// `None` when the ref is absent, or present but not symbolic — both are normal +/// states for a clone that was never given one, and both mean "unknown" rather +/// than "no default exists". Callers must treat `None` as a reason to protect +/// more, never as a reason to protect less. +pub fn get_default_branch(repo: &Repository, remote: &str) -> Option { + let reference = repo + .find_reference(&format!("refs/remotes/{}/HEAD", remote)) + .ok()?; + let target = reference.symbolic_target()?; + target + .strip_prefix(&format!("refs/remotes/{}/", remote)) + .map(|name| name.to_string()) +} + #[cfg(test)] mod tests { use super::*; diff --git a/tests/test_prune.rs b/tests/test_prune.rs index 53001d09..a84733a9 100644 --- a/tests/test_prune.rs +++ b/tests/test_prune.rs @@ -2,9 +2,41 @@ mod common; +use assert_cmd::Command as AssertCommand; +use predicates::prelude::*; + use common::fixtures::WorkspaceBuilder; use common::git_helpers; +/// Put a fixture into the shape production actually runs in: manifest target is +/// `dev`, `dev` is checked out, and `main` exists as the release branch. Before +/// 2026-08-01 target and default were both `main`, which protected `main` by +/// coincidence; every prune test still encodes that retired arrangement. +fn dev_target_workspace(repo: &str) -> common::fixtures::WorkspaceFixture { + let ws = WorkspaceBuilder::new().add_repo(repo).build(); + let manifest_path = ws + .workspace_root + .join(".gitgrip") + .join("spaces") + .join("main") + .join("gripspace.yml"); + let yaml = std::fs::read_to_string(&manifest_path).unwrap(); + assert!( + yaml.contains("default_branch: main"), + "fixture no longer declares a target this helper knows how to move: {yaml}" + ); + std::fs::write( + &manifest_path, + yaml.replace("default_branch: main", "default_branch: dev"), + ) + .unwrap(); + + let repo_path = ws.repo_path(repo); + git_helpers::create_branch(&repo_path, "dev"); + assert!(git_helpers::branch_exists(&repo_path, "main")); + ws +} + #[test] fn test_prune_dry_run_lists_merged_branches() { let ws = WorkspaceBuilder::new().add_repo("alpha").build(); @@ -131,3 +163,94 @@ fn test_prune_no_merged_branches() { // Unmerged branch should still exist assert!(git_helpers::branch_exists(&repo_path, "feat/unmerged")); } + +#[test] +fn test_prune_protects_main_when_target_is_dev() { + // The production shape: target `dev`, standing on `dev`, `main` present. + // `main` is neither current nor target here, which is exactly the state the + // old two-slot guard left unprotected. + let ws = dev_target_workspace("alpha"); + let repo_path = ws.repo_path("alpha"); + + // A genuinely merged branch, so this test also proves prune still WORKS. + // Without it, a fix that protected everything would pass just as happily. + git_helpers::create_branch(&repo_path, "feat/spent"); + git_helpers::commit_file(&repo_path, "spent.txt", "x", "spent work"); + git_helpers::checkout(&repo_path, "dev"); + std::process::Command::new("git") + .args(["merge", "feat/spent", "--no-ff", "-m", "merge spent"]) + .current_dir(&repo_path) + .output() + .unwrap(); + + AssertCommand::cargo_bin("gr") + .unwrap() + .current_dir(&ws.workspace_root) + .args(["prune", "--execute", "--repo", "alpha"]) + .assert() + .success(); + + assert!( + git_helpers::branch_exists(&repo_path, "main"), + "the release branch must survive a prune run from dev" + ); + assert!(git_helpers::branch_exists(&repo_path, "dev")); + assert!( + !git_helpers::branch_exists(&repo_path, "feat/spent"), + "positive control: prune must still delete a merged branch, or this test \ + would pass against a guard that simply protects everything" + ); +} + +#[test] +fn test_prune_protects_more_and_says_so_when_default_is_unresolvable() { + // The default is made GENUINELY unresolvable -- origin/HEAD is deleted from a + // real clone -- rather than stubbed. A stub would assert the code path against + // a fixture instead of against the condition, and the real failure (a clone + // that was never given an origin/HEAD) would go unexercised. + let ws = dev_target_workspace("alpha"); + let repo_path = ws.repo_path("alpha"); + + let before = std::process::Command::new("git") + .args(["symbolic-ref", "refs/remotes/origin/HEAD"]) + .current_dir(&repo_path) + .output() + .unwrap(); + assert!( + before.status.success(), + "control: the clone must HAVE an origin/HEAD before we remove it, or this \ + test proves nothing about removing it" + ); + + std::process::Command::new("git") + .args(["symbolic-ref", "--delete", "refs/remotes/origin/HEAD"]) + .current_dir(&repo_path) + .output() + .unwrap(); + let after = std::process::Command::new("git") + .args(["symbolic-ref", "refs/remotes/origin/HEAD"]) + .current_dir(&repo_path) + .output() + .unwrap(); + assert!( + !after.status.success(), + "the removal must actually make resolution fail" + ); + + AssertCommand::cargo_bin("gr") + .unwrap() + .current_dir(&ws.workspace_root) + .args(["prune", "--execute", "--repo", "alpha"]) + .assert() + .success() + // Protecting more must not be silent. A silent protected-more reads as + // "the default resolved fine" to the next person who runs this. + // NOTE: Output::warning writes to stdout, not stderr -- asserted against + // the stream the binary actually uses, verified by running it. + .stdout(predicate::str::contains( + "could not determine the default branch", + )); + + assert!(git_helpers::branch_exists(&repo_path, "main")); + assert!(git_helpers::branch_exists(&repo_path, "dev")); +}