From bf4a8528a6c864a13cc47e88a4704531669e37d7 Mon Sep 17 00:00:00 2001 From: Jake Bromberg Date: Mon, 22 Jun 2026 10:23:28 -0700 Subject: [PATCH] Migrate remote-tidy onto the CleanPipeline seam Rewrite git-remote-tidy's hand-rolled clean loop to delegate to the shared git_tidy_core::clean::run_clean seam (issue #118 step 3). The public run_clean signature and CleanResult return type are unchanged, so no callers or tests required edits. Remotes are grouped per repo, so the seam is invoked once per RepoGroup and the per-group CleanResults are accumulated into the combined succeeded/failed/skipped buckets; this keeps each group's repo_path and name stable for that call's act closure while leaving the loop oblivious to grouping. The decide predicate is the existing should_clean classification filter unchanged: with --all it admits any non-Active remote, otherwise only Unreachable. The prune-vs-remove split is preserved verbatim inside act: orphaned remotes have their tracking refs pruned via prune_remote_refs (emitting "pruned N refs for ...", carrying the per-remote count in RemovedRemote.refs_pruned), while configured/unreachable remotes are removed via remote_remove (emitting "removed ..."); dry-run short-circuits both with the original "would prune refs for"/"would remove" wording and refs_pruned 0. Origin protection stays an act-level guard rather than a decide filter because it prints a warning before skipping: removing the origin remote without --force writes "warning: skipping origin remote in ... (use --force to remove)" and returns Outcome::Skipped. Success maps to Outcome::Cleaned(RemovedRemote), action failures to Outcome::Failed(FailedItem) so the loop continues. Refs #118 --- crates/git-remote-tidy/src/clean.rs | 166 ++++++++++++++++------------ 1 file changed, 93 insertions(+), 73 deletions(-) diff --git a/crates/git-remote-tidy/src/clean.rs b/crates/git-remote-tidy/src/clean.rs index 7dc653d..0f9f00e 100644 --- a/crates/git-remote-tidy/src/clean.rs +++ b/crates/git-remote-tidy/src/clean.rs @@ -1,6 +1,7 @@ use std::io::Write; use std::path::PathBuf; +use git_tidy_core::clean::{Decision, Outcome, run_clean as core_run_clean}; use git_tidy_core::error::Error; use git_tidy_core::git::GitOps; use git_tidy_core::types::{CleanResult, FailedItem}; @@ -28,6 +29,16 @@ pub struct RemovedRemote { } /// Run the clean operation on a scan result. +/// +/// Remotes are grouped per repo, so the shared [`core_run_clean`] seam is invoked +/// once per group (each group's `name`/`repo_path` are stable for that call's +/// `act` closure) and the per-group [`CleanResult`]s are accumulated. `decide` is +/// the pure [`should_clean`] classification filter; `act` owns the two distinct +/// actions — orphaned remotes get their tracking refs pruned via +/// [`GitOps::prune_remote_refs`], configured/unreachable remotes get the remote +/// removed via [`GitOps::remote_remove`] — plus the origin-protection guard +/// (removing `origin` requires `--force`, otherwise it prints a warning and +/// skips) and the dry-run branching. pub fn run_clean( git: &dyn GitOps, scan_result: &RemoteScanResult, @@ -39,84 +50,93 @@ pub fn run_clean( let mut skipped = 0; for group in &scan_result.repos { - for remote in &group.items { - if !should_clean(&remote.classification, options) { - skipped += 1; - continue; - } - - // Origin safety check - if remote.is_origin && !options.force { - writeln!( - out, - "warning: skipping origin remote in {} (use --force to remove)", - group.name, - )?; - skipped += 1; - continue; - } - - if options.dry_run { - let action = if remote.classification == RemoteClassification::Orphaned { - "would prune refs for" + let group_result = core_run_clean( + &group.items, + |remote| { + if should_clean(&remote.classification, options) { + Decision::Clean } else { - "would remove" - }; - writeln!(out, "{action} {} in {}", remote.name, group.name)?; - succeeded.push(RemovedRemote { - repo: group.repo_path.clone(), - name: remote.name.clone(), - refs_pruned: 0, - }); - continue; - } - - // Orphaned remotes: prune refs (no config to remove) - if remote.classification == RemoteClassification::Orphaned { - match git.prune_remote_refs(&group.repo_path, &remote.name) { - Ok(count) => { - writeln!( - out, - "pruned {count} refs for {} in {}", - remote.name, group.name, - )?; - succeeded.push(RemovedRemote { - repo: group.repo_path.clone(), - name: remote.name.clone(), - refs_pruned: count, - }); - } - Err(e) => { - writeln!(out, "error: could not prune refs for {}: {e}", remote.name,)?; - failed.push(FailedItem { - repo: group.repo_path.clone(), - name: remote.name.clone(), - reason: e.to_string(), - }); - } + Decision::Skip + } + }, + |remote, out| { + // Origin safety check: removing origin requires --force. Prints a + // warning then skips, so it is an `act`-level guard, not `decide`. + if remote.is_origin && !options.force { + writeln!( + out, + "warning: skipping origin remote in {} (use --force to remove)", + group.name, + )?; + return Ok(Outcome::Skipped); + } + + if options.dry_run { + let action = if remote.classification == RemoteClassification::Orphaned { + "would prune refs for" + } else { + "would remove" + }; + writeln!(out, "{action} {} in {}", remote.name, group.name)?; + return Ok(Outcome::Cleaned(RemovedRemote { + repo: group.repo_path.clone(), + name: remote.name.clone(), + refs_pruned: 0, + })); } - } else { - // Configured remotes: git remote remove - match git.remote_remove(&group.repo_path, &remote.name) { - Ok(()) => { - writeln!(out, "removed {} in {}", remote.name, group.name)?; - succeeded.push(RemovedRemote { - repo: group.repo_path.clone(), - name: remote.name.clone(), - refs_pruned: 0, - }); + + // Orphaned remotes: prune refs (no config to remove) + if remote.classification == RemoteClassification::Orphaned { + match git.prune_remote_refs(&group.repo_path, &remote.name) { + Ok(count) => { + writeln!( + out, + "pruned {count} refs for {} in {}", + remote.name, group.name, + )?; + Ok(Outcome::Cleaned(RemovedRemote { + repo: group.repo_path.clone(), + name: remote.name.clone(), + refs_pruned: count, + })) + } + Err(e) => { + writeln!(out, "error: could not prune refs for {}: {e}", remote.name,)?; + Ok(Outcome::Failed(FailedItem { + repo: group.repo_path.clone(), + name: remote.name.clone(), + reason: e.to_string(), + })) + } } - Err(e) => { - writeln!(out, "error: could not remove {}: {e}", remote.name)?; - failed.push(FailedItem { - repo: group.repo_path.clone(), - name: remote.name.clone(), - reason: e.to_string(), - }); + } else { + // Configured remotes: git remote remove + match git.remote_remove(&group.repo_path, &remote.name) { + Ok(()) => { + writeln!(out, "removed {} in {}", remote.name, group.name)?; + Ok(Outcome::Cleaned(RemovedRemote { + repo: group.repo_path.clone(), + name: remote.name.clone(), + refs_pruned: 0, + })) + } + Err(e) => { + writeln!(out, "error: could not remove {}: {e}", remote.name)?; + Ok(Outcome::Failed(FailedItem { + repo: group.repo_path.clone(), + name: remote.name.clone(), + reason: e.to_string(), + })) + } } } - } - } + }, + out, + )?; + + succeeded.extend(group_result.succeeded); + failed.extend(group_result.failed); + skipped += group_result.skipped; } Ok(CleanResult {