diff --git a/docs/IMPLEMENTATION_PLAN.md b/docs/IMPLEMENTATION_PLAN.md index 16bd1da..69c54bf 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 9af36a1..64ca255 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 87888eb..b3e90dd 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 24f3851..f99f1c5 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 847a0ce..f75cc27 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() + ); +}