From a9263bf1bc12532bad1c1199946255b259f42315 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Fri, 21 Aug 2026 06:35:59 -0500 Subject: [PATCH] fix(checkout): count repo failures and disjoin them into the exit code gr checkout reported every per-repo failure to the terminal and incremented no counter, so a run in which every repo failed printed "Switched 0/N repos to " and exited 0. The printed ratio is something a caller has to read and interpret; the exit code is the only failure signal a script sees, and it reported the batch as done. Adopt cli/repo_iter::for_each_repo -- which is what gives this command an error count at all -- and make a nonzero exit disjoin the per-repo failures. Two changes fall out of the adoption: - for_each_repo's cloned-check called path_exists on the repo directory while its own docstring promised to skip repos that "aren't cloned". RepoInfo::exists tests for .git, which is what cloned means; the two diverge on a directory that exists and is not a clone. Aligning the code with the docstring preserves checkout's existing behavior exactly rather than reclassifying a skip as an error. - for_each_repo_path had no callers and gains none here, so it is removed rather than left relying on pub visibility to keep its own dead-code warning quiet. The plan step prescribing that suppression is struck in the same change. An unused private item warns and an unused pub item does not, so following the step as written left the module with zero callers and nothing anywhere going red for as long as it existed. A warning is a detector; silencing one to reach a clean build is not a fix. Tests pin both sides of one property. The discriminator is an ABSENT .git versus a PRESENT BUT UNOPENABLE one: absent is a skip and still exits 0, present but unopenable is an error and exits nonzero. Without both sides, a witness asserting only the failure case could pass while the skip path had silently become an error too. These are two separate tests with their own fixtures, and those fixtures differ in more than the discriminating property -- one corrupts a single repo and the other both, one creates a branch first, and they target different branches. The claim is about which property discriminates, not about the fixtures being otherwise identical. --- docs/IMPLEMENTATION_PLAN.md | 2 +- docs/PLAN-p2-maintainability.md | 6 +- src/cli/commands/checkout.rs | 103 ++++++++++++++------------------ src/cli/repo_iter.rs | 55 +++-------------- tests/test_checkout.rs | 64 ++++++++++++++++++++ 5 files changed, 122 insertions(+), 108 deletions(-) diff --git a/docs/IMPLEMENTATION_PLAN.md b/docs/IMPLEMENTATION_PLAN.md index 16bd1da3..69c54bfb 100644 --- a/docs/IMPLEMENTATION_PLAN.md +++ b/docs/IMPLEMENTATION_PLAN.md @@ -59,7 +59,7 @@ Pass `--hostname` to `gh` CLI in `src/platform/github.rs` `enable_auto_merge()` ### Phase 3: Repo iteration helper - [x] New `src/cli/repo_iter.rs`: `RepoVisitResult`, `RepoOpSummary`, `for_each_repo()`, `for_each_repo_path()` -- [ ] Wire into commands — Deferred: most commands accumulate custom state that doesn't fit the simple Success/Skipped/Error enum +- [ ] Wire into commands — Partially done: `checkout` adopted it (2026-08-21). The remaining commands are blocked on `?` propagation out of the loop, three disagreeing skip taxonomies, and per-arm payload types — not on the enum alone ### Phase 4-6: Migrate all commands to WorkspaceContext - [x] 28/30 commands in main.rs use `load_workspace_context()` (Init, Completions, Bench don't need workspace) diff --git a/docs/PLAN-p2-maintainability.md b/docs/PLAN-p2-maintainability.md index 9af36a1a..64ca2555 100644 --- a/docs/PLAN-p2-maintainability.md +++ b/docs/PLAN-p2-maintainability.md @@ -24,7 +24,9 @@ No conflicts expected — P0/P1 didn't touch `main.rs` or `cli/mod.rs`. ``` cargo build && cargo test && cargo clippy && cargo fmt --check ``` -Watch for clippy warnings on unused `repo_iter` imports. If flagged, the `pub` visibility from `cli/mod.rs` → `lib.rs` should suppress it. +~~Watch for clippy warnings on unused `repo_iter` imports. If flagged, the `pub` visibility from `cli/mod.rs` → `lib.rs` should suppress it.~~ + +**Struck.** This prescribed silencing the one instrument that would have reported the problem. `pub` does not resolve a dead-code warning, it disables the analysis — an unused private item warns, an unused `pub` item does not — so following this step left `repo_iter` with zero callers and nothing anywhere going red for as long as it existed. A warning is a detector; suppressing it to reach a clean build is not a fix, and writing the suppression down as a step made it the default for whoever came next. If an item has no consumer, give it one or delete it. ### 3. Commit the refactor Stage all 4 files and commit: @@ -59,7 +61,7 @@ gr pr create -t "refactor: P2 maintainability — WorkspaceContext and load_grip | Command signature migration to `&WorkspaceContext` | Would touch every command file + every test; current ctx field extraction in dispatch works fine | | Compact dispatch function (Phase 7) | 612-line match for 30 commands is standard; only one dispatch site | | sync.rs / release.rs decomposition (Phase 8) | Already have helpers (`sync_single_repo`, `execute_post_sync_hooks`, etc.) | -| Wiring `for_each_repo()` into commands | Most commands accumulate custom state that doesn't fit the simple Success/Skipped/Error enum | +| Wiring `for_each_repo()` into commands | Largely accurate, and now measured rather than assumed. Of the 9 command files that hand-roll these counters, `sync`/`pull` do not iterate repos at all, `pr/merge` iterates PRs and awaits, and `push`/`commit`/`forall` propagate `?` out of the loop — which the closure's return type cannot express. `checkout` was the one clean fit and has adopted it. | ## Files touched diff --git a/src/cli/commands/checkout.rs b/src/cli/commands/checkout.rs index 87888ebc..b3e90dd0 100644 --- a/src/cli/commands/checkout.rs +++ b/src/cli/commands/checkout.rs @@ -1,15 +1,13 @@ //! Checkout command implementation use crate::cli::output::Output; +use crate::cli::repo_iter::{for_each_repo, RepoVisitResult}; use crate::core::manifest::Manifest; use crate::core::repo::{ filter_repos, get_manifest_repo_info, validate_repo_filters_known, RepoInfo, }; use crate::core::workspace_checkout; -use crate::git::{ - branch::{branch_exists, checkout_branch, create_and_checkout_branch}, - open_repo, -}; +use crate::git::branch::{branch_exists, checkout_branch, create_and_checkout_branch}; use std::path::Path; /// Run the checkout command @@ -51,71 +49,62 @@ pub fn run_checkout( )); println!(); - let mut success_count = 0; - let mut _skip_count = 0; - - for repo in &repos { - if !repo.exists() { - Output::warning(&format!("{}: not cloned", repo.name)); - _skip_count += 1; - continue; - } - - match open_repo(&repo.absolute_path) { - Ok(git_repo) => { - let exists = branch_exists(&git_repo, branch_name); - - if create { - // -b flag: create if doesn't exist, checkout if it does - if exists { - match checkout_branch(&git_repo, branch_name) { - Ok(()) => { - Output::success(&format!( - "{}: checked out (already exists)", - repo.name - )); - success_count += 1; - } - Err(e) => Output::error(&format!("{}: {}", repo.name, e)), - } - } else { - match create_and_checkout_branch(&git_repo, branch_name) { - Ok(()) => { - Output::success(&format!("{}: created and checked out", repo.name)); - success_count += 1; - } - Err(e) => Output::error(&format!("{}: {}", repo.name, e)), - } - } - } else { - // Normal checkout: skip if branch doesn't exist - if !exists { - Output::info(&format!("{}: branch doesn't exist, skipping", repo.name)); - _skip_count += 1; - continue; - } - - match checkout_branch(&git_repo, branch_name) { - Ok(()) => { - Output::success(&repo.name); - success_count += 1; - } - Err(e) => Output::error(&format!("{}: {}", repo.name, e)), + // Iterating through for_each_repo rather than by hand is what gives this + // command an error count at all. The previous loop reported every failure + // to the terminal and incremented nothing, so the summary below counted + // successes against a total and stayed silent about the difference. + let summary = for_each_repo(&repos, false, |repo, git_repo| { + let exists = branch_exists(git_repo, branch_name); + + if create { + // -b flag: create if doesn't exist, checkout if it does + if exists { + match checkout_branch(git_repo, branch_name) { + Ok(()) => RepoVisitResult::Success(format!( + "{}: checked out (already exists)", + repo.name + )), + Err(e) => RepoVisitResult::Error(format!("{}: {}", repo.name, e)), + } + } else { + match create_and_checkout_branch(git_repo, branch_name) { + Ok(()) => { + RepoVisitResult::Success(format!("{}: created and checked out", repo.name)) } + Err(e) => RepoVisitResult::Error(format!("{}: {}", repo.name, e)), } } - Err(e) => Output::error(&format!("{}: {}", repo.name, e)), + } else if !exists { + // Normal checkout: a missing branch is a skip, not a failure. + RepoVisitResult::Skipped(format!("{}: branch doesn't exist, skipping", repo.name)) + } else { + match checkout_branch(git_repo, branch_name) { + Ok(()) => RepoVisitResult::Success(repo.name.clone()), + Err(e) => RepoVisitResult::Error(format!("{}: {}", repo.name, e)), + } } - } + }); println!(); println!( "Switched {}/{} repos to {}", - success_count, + summary.success_count, repos.len(), Output::branch_name(branch_name) ); + // The count above is a ratio a caller has to read and interpret. The exit + // code is the only failure signal a script sees, so it has to disjoin the + // per-repo failures rather than report the batch as done. + if summary.error_count > 0 { + anyhow::bail!( + "{} of {} repos failed to switch to {}", + summary.error_count, + repos.len(), + branch_name + ); + } + Ok(()) } diff --git a/src/cli/repo_iter.rs b/src/cli/repo_iter.rs index 24f38511..f99f1c54 100644 --- a/src/cli/repo_iter.rs +++ b/src/cli/repo_iter.rs @@ -5,7 +5,7 @@ use crate::cli::output::Output; use crate::core::repo::RepoInfo; -use crate::git::{open_repo, path_exists}; +use crate::git::open_repo; use git2::Repository; /// Result of visiting a single repo @@ -45,7 +45,12 @@ where }; for repo in repos { - if !path_exists(&repo.absolute_path) { + // `RepoInfo::exists` tests for `.git`, which is what "cloned" means. + // A bare `path_exists` on the directory answers a different question: + // a checkout whose `.git` is gone still has its files, so it would + // pass that check and then fail to open, turning a not-cloned skip + // into an error. This is what the docstring above has always claimed. + if !repo.exists() { if !quiet { Output::warning(&format!("{}: not cloned", repo.name)); } @@ -81,49 +86,3 @@ where summary } - -/// Iterate over repos by path (without opening git2::Repository). -/// -/// Useful for operations that shell out to `git` directly rather than -/// using libgit2 (e.g., cherry-pick, gc). -pub fn for_each_repo_path(repos: &[RepoInfo], quiet: bool, mut op: F) -> RepoOpSummary -where - F: FnMut(&RepoInfo) -> RepoVisitResult, -{ - let mut summary = RepoOpSummary { - success_count: 0, - skip_count: 0, - error_count: 0, - }; - - for repo in repos { - if !path_exists(&repo.absolute_path) { - if !quiet { - Output::warning(&format!("{}: not cloned", repo.name)); - } - summary.skip_count += 1; - continue; - } - - match op(repo) { - RepoVisitResult::Success(msg) => { - if !quiet { - Output::success(&msg); - } - summary.success_count += 1; - } - RepoVisitResult::Skipped(msg) => { - if !quiet { - Output::info(&msg); - } - summary.skip_count += 1; - } - RepoVisitResult::Error(msg) => { - Output::error(&msg); - summary.error_count += 1; - } - } - } - - summary -} diff --git a/tests/test_checkout.rs b/tests/test_checkout.rs index 847a0ce3..f75cc276 100644 --- a/tests/test_checkout.rs +++ b/tests/test_checkout.rs @@ -635,3 +635,67 @@ fn test_absolute_repo_path_in_metadata_cannot_escape_the_checkout() { "a rejected reconstruction should explain why, not fail silently" ); } + +// ── Every repo fails to open ──────────────────────────────────── +// Witness for the error arms, which previously reported each failure to the +// terminal and incremented no counter, so the command printed "Switched 0/N" +// and exited 0. Each .git is replaced by an empty directory: the repo still +// counts as cloned under either cloned-check, so this exercises the error arm +// rather than the not-cloned skip arm. + +#[test] +fn test_checkout_all_repos_failing_is_not_success() { + let ws = WorkspaceBuilder::new() + .add_repo("frontend") + .add_repo("backend") + .build(); + + let manifest = ws.load_manifest(); + + for name in ["frontend", "backend"] { + let git_dir = ws.repo_path(name).join(".git"); + std::fs::remove_dir_all(&git_dir).unwrap(); + std::fs::create_dir(&git_dir).unwrap(); + assert!( + git_dir.exists(), + "{name}: .git must still exist for this witness" + ); + } + + let result = gitgrip::cli::commands::checkout::run_checkout( + &ws.workspace_root, + &manifest, + "main", + false, + None, + None, + ); + + assert!( + result.is_err(), + "every repo failed to open; checkout must not report success" + ); +} + +// The companion that keeps the fix from over-correcting: a branch that is +// simply absent is a skip, not a failure, and must still exit zero. +#[test] +fn test_checkout_absent_branch_is_still_success() { + let ws = WorkspaceBuilder::new().add_repo("frontend").build(); + let manifest = ws.load_manifest(); + + let result = gitgrip::cli::commands::checkout::run_checkout( + &ws.workspace_root, + &manifest, + "no-such-branch", + false, + None, + None, + ); + + assert!( + result.is_ok(), + "an absent branch is a skip, not a failure: {:?}", + result.err() + ); +}