Extract CleanPipeline skeleton into git-tidy-core + migrate repo-tidy#192
Merged
Conversation
…repo-tidy
Each tool's clean.rs reimplemented the same loop: iterate items, filter by classification, dry-run-or-act, aggregate succeeded/failed/skipped. This adds `git_tidy_core::clean::run_clean<I, S>(items, decide, act, out)` that owns the loop and the aggregation exactly once, with `Decision::{Clean,Skip}` for the pure filter and `Outcome::{Cleaned(S),Skipped,Failed(FailedItem)}` for the per-item action result. The pipeline is generic over the item type because only branch and worktree classify on the shared `Classification`; the other five tools use their own enums.
`act` owns everything tool-specific: dry-run-vs-real branching and the exact wording, any IO guard (returning `Outcome::Skipped` after printing its own warning), the delete/prune, and construction of the success record. It returns `Err` only for genuinely fatal errors (an `out` write failure, a hard removal failure), which short-circuit the loop; a per-item delete failure is `Ok(Outcome::Failed(..))` and the loop continues. Aggregate extras the pipeline doesn't model stay tool-side via a captured `&mut`.
`git-repo-tidy` is migrated as the proof adapter: its `run_clean` body now delegates to the shared pipeline while keeping its public signature, `CleanOptions`, `RepoCleanResult`, `DeletedRepo`, `should_clean`, and entire test module unchanged. Behavior is byte-identical (same wording, records, counting, dirty_blocked, ordering). The other six tools are migrated one at a time in a later step.
Refs #118
jakebromberg
force-pushed
the
clean-pipeline-extract
branch
from
48d30cd to
971b4a4
Compare
…-outcome paths Code-review follow-ups on the CleanPipeline seam (no behavior change): Extend run_clean's doc comment to record how the other six tools map onto the flat, per-item loop in step 3 — grouped scans flatten to (group, item) pairs with group context carried in the item type, order-sensitive deletes (stash) pre-sort at the call site, and aggregate state the result doesn't model (repo-tidy's dirty_blocked, lfs-tidy's recommendations) stays tool-side. This was implied but undocumented, so the next migration would have hit it cold. Add two unit tests: one drives all three outcomes (Cleaned/Failed/Skipped) through a single run to guard against cross-bucket contamination, and one exercises the documented fatal path where a writeln! failure inside act surfaces as Err and short-circuits the loop (the existing error test hand-returns Err rather than failing a write). Refs #118
Second code-review pass on the CleanPipeline seam (no behavior change): The mapping doc added in the previous commit described grouped tools flattening every group into one run_clean call and sorting the flattened items — which would interleave per-repo output and, for stash, needs a (group, descending-index) sort to stay correct. Rewrite it around the accurate pattern: grouped tools call run_clean once per group, which keeps each repo's output contiguous and lets per-group emission (lfs recommendations) and aggregate extras (repo-tidy's dirty_blocked) live around the call. The seam owns iterate/filter/act/aggregate, not per-group output. Derive Debug on the public Decision and Outcome types to match the crate convention (CleanResult, FailedItem, RepoGroup, ScanResult all derive Debug) and to let callers assert on an act closure's return directly. Refs #118
… handling Third code-review pass on the CleanPipeline seam (doc only, no behavior change): The mapping doc implied all six remaining tools fit the seam cleanly, but tag-tidy fans out: deleting one remote-only tag iterates its remotes (git-tag-tidy/src/clean.rs), pushing a FailedItem per failed remote while still recording the tag as removed — so one item yields both a failed and a succeeded record, which a one-of-three Outcome cannot express. Make the seam's one-outcome-per-item assumption explicit and show that fan-out sub-failures use the same tool-side accumulation as dirty_blocked: act returns the item's primary Outcome and pushes secondary FailedItems into a captured &mut, merged after the run. This was an untested path (no test covers a failing remote delete on the append path), so the mismatch would otherwise have surfaced silently mid-migration. Refs #118
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Issue #118 step 2: extract the generic clean-loop skeleton into
git-tidy-corebehind unit tests, and migrate one tool (git-repo-tidy) to prove the seam.Adds
git_tidy_core::clean::run_clean<I, S>(items, decide, act, out) -> Result<CleanResult<S>, Error>, which owns the shared clean loop — iterate → pre-filter → act → aggregatesucceeded/failed/skipped— exactly once. It is generic over the item type because only branch and worktree classify on the sharedClassification; the other five tools use their own enums.Each tool supplies:
decidepredicate (the pure, silent classification filter —Decision::Clean/Skip), andactclosure owning everything tool-specific: dry-run-vs-real branching and the exact "would …" wording, any IO guard (returningOutcome::Skippedafter printing its own warning), the actual delete/prune, and construction of the success record.actreturnsErronly for genuinely fatal errors (anoutwrite failure, a hard removal failure), which short-circuit the loop; a per-item delete failure isOk(Outcome::Failed(..))and the loop continues. Aggregate extras the pipeline doesn't model (e.g. repo-tidy'sdirty_blocked) stay tool-side via a captured&mut.Scope
crates/git-tidy-core/src/clean.rswithrun_clean,Decision,Outcome, and 7 unit tests covering the contract (decide-skip doesn't callact,Cleanedordering,Failedaggregation + loop continuation,Skippedcounting,Errshort-circuit,outwrites, empty input).git-repo-tidymigrated as the proof adapter —run_cleandelegates to the shared pipeline while keeping its public signature,CleanOptions,RepoCleanResult,DeletedRepo,should_clean, and entire test module unchanged. Behavior is byte-identical (same wording, records, counting,dirty_blocked, ordering); its existing unit + integration tests are the regression gate.CLAUDE.mddocuments the seam.The other six tools (branch, worktree, stash, remote, tag, lfs) are deliberately not migrated here — that's step 3 (one tool at a time). The design was validated against all six (dirty/origin/release guards via
actSkipped; stash descending-order via tool pre-sort; lfs per-group recommendations tool-side) but no behavior changes for them in this PR.Verification
Local CI replicated green:
cargo fmt --check,cargo clippy --workspace --all-targets -- -D clippy::all,cargo test --workspace, and MSRVcargo checkon Rust 1.93.0. No dependency changes, socargo auditis unaffected.Refs #118