Migrate branch-tidy onto core run_pipeline scan seam#197
Merged
Conversation
Route branch-tidy through the shared `run_pipeline` seam added in PR B, replacing its hand-rolled `BranchRepoGroup`/`BranchScanResult`/orchestration with the generic `RepoGroup<T>`/`ScanResult<T>` and a thin classifier closure. `BranchInfo` now declares `Classified` and `IntoJsonItem` and relies on the core blanket `FlatJsonItems` impl, so the per-tool `--json` flat array (incl. `landed_ratio`/`landed_total`/`unmatched_commits`), the 8-field porcelain rows, and the `LANDED_SUMMARY` human line stay byte-identical.
Unlike the offline tools migrated in PR C, branch-tidy is `fetch: true`: it owned its own `parallel_fetch` call before classification. That fetch is handed to the seam via `ScanOptions { fetch: true }` — `run_pipeline` fetches every repo first and orders fetch warnings ahead of classify warnings, exactly as the old `parallel_fetch(...) + warnings.extend(scan_warnings)` did. The `--include-remote` remote-only discovery, the per-repo `LandedCache`, the entity filter, and the inner per-branch `par_iter` all stay inside the closure as domain logic.
Empty groups are already dropped today (the closure returned `None` when nothing classified), so routing through `run_pipeline` — which also drops empty groups — is a no-op here, not the intentional behavior change the offline tools carried. A regression-guard test (`run_scan_repos_skips_repo_when_entity_filter_excludes_all_branches`) pins the preserved behavior.
The closure has two verbose sites (local branches and remote-only), so `repo_display_name(repo_path)` is inlined at each `if verbose` block rather than bound once, keeping it off the quiet-run path now that `run_pipeline` computes the group name itself.
Part of #117
jakebromberg
force-pushed
the
117d-scan-pipeline
branch
from
6ba251d to
0b83f29
Compare
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.
Migrates
git-branch-tidyonto the sharedrun_pipelinescan seam (core PR B), the first non-trivial tool: it isfetch: true, owns its fetch today, runs nested parallelism, discovers remote-only branches under--include-remote, and carries a per-repoLandedCache. The seam absorbs the orchestration; the domain logic stays in the closure.What changed
types.rs: deleteBranchRepoGroup;BranchScanResultbecomesScanResult<BranchInfo>(the core generic).BranchInfonow implementsClassifiedandIntoJsonItem, replacing the hand-rolledFlatJsonItems for BranchScanResult(the orphan-rule-safe shape).JsonBranchand itsFrom<&BranchInfo>are untouched, so the public JSON is identical.scan.rs:run_scan_reposhands its fetch to the seam viaScanOptions { fetch: true }and delegates torun_pipeline. The closure returnsVec<BranchInfo>(empty on skip); the manualparallel_fetchsetup, counts/total_scannedfold, and group assembly are gone.--include-remotediscovery,LandedCache, the entity filter, and the inner per-branchpar_iterstay inside the closure.output.rs/clean.rs/tests/integration_scan.rs:.branches→.itemsfield rename; test fixtures use the genericRepoGroup.Behavior preservation
run_pipelinefetches every repo before classifying and orders fetch warnings ahead of classify warnings — identical to the oldparallel_fetch(...)+warnings.extend(scan_warnings). Covered by core seam tests from PR B.--jsonflat array (incl.landed_ratio/landed_total/unmatched_commits), 8-field porcelain rows, and theLANDED_SUMMARYhuman summary are unchanged; the existingoutput.rsstring-assertion tests pass unchanged save the.branches→.itemsfixture rename.Nonewhen nothing classified), so routing throughrun_pipelineis a no-op for that case — not the intentional behavior change the offline tools carried. A regression-guard test (run_scan_repos_skips_repo_when_entity_filter_excludes_all_branches) pins the preserved behavior.sort_by_key(classification.priority()); the seam does not reorder.inprocess.rscalls the same 7-argrun_scan_reposand reads.counts; the type alias keepsrepos/total_scanned/counts/warnings, so audit JSON keys are identical.Verification
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D clippy::all,cargo test --workspace, andcargo +1.93.0 check --workspace(MSRV) all pass locally.Net −20 lines (deleting the manual fetch setup + counts fold + group assembly, minus the added regression test).
Part of #117