fix: consistency and correctness bugs across scanning, status and output - #54
Merged
Conversation
…-merge state Three related problems around `--fetch` / `--ff`: - A repository without a remote made `fetch_origin` return an error, which `RepoInfo::new` propagated. The repository was then counted as *failed* and dropped from the output entirely, so `--fetch` over a directory containing a local-only repository silently hid it. The same happened with `--ff` for a branch without an upstream. Both are now logged as warnings instead. - `merge_ff` ran *after* ahead/behind, commit count and status were gathered, so a fast-forwarded repository was displayed with its pre-merge numbers: still "behind 1" while simultaneously marked as fast-forwarded. Fetch and merge now happen before any state is read. - `fetch_origin` ran git in `repo.path().parent()`. For a worktree the git directory is `<main>/.git/worktrees/<name>`, whose parent is not a working directory. It now uses `workdir()` and only falls back to the git directory's parent for bare repositories.
Repositories are collected in parallel via rayon, so the order of the result vectors depended on thread scheduling. Only the table sorted afterwards, which left `--json` and the "failed to process" warnings emitting a different order on every run over the same directory - unpleasant to read and impossible to diff. Sort both vectors in `find_repositories` instead, so every consumer sees the same order, and drop the now redundant sort from the printer. `repositories_table` no longer mutates its input and takes a shared slice.
`--non-clean` was implemented inside the table printer, so it only ever affected the table. `--json --non-clean` happily emitted every clean repository, which makes the flag useless for the exact scripting use case JSON exists for. The filter now lives in `Args::filter_repos` and is applied once in `main`, so the table and the JSON output cannot disagree about what the user asked to see. It borrows the scan result unless a filter is actually active. Two side effects of moving it out of the printer: - `repositories_table` now receives an already filtered slice, so filtering everything away correctly reports "No repositories found." instead of printing a table header with no rows underneath it. - `--summary` keeps describing the whole scan rather than the filtered subset.
The name shown for a repository is derived from its remote URL by stripping a
`.git` suffix and taking the last `/`-separated segment. That mishandles three
shapes git accepts:
- `git@host:repo.git` (SCP-like, no slash in the path) contains no `/` after the
host, so the whole `git@host:repo` string became the repository name.
- A trailing slash (`https://host/user/repo/`) left the last segment empty, so
the repository was displayed without a name at all.
- `trim_end_matches(".git")` strips *every* trailing occurrence, so a repository
genuinely named `repo.git` was displayed as `repo`.
Parsing now lives in `repo_name_from_url`, splits on `/`, `:` and `\`, strips at
most one `.git` suffix, and returns `None` when nothing usable is left - which
makes the caller fall back to the directory name, as it already did for
repositories without a remote.
…itory itself `repo_path` is the repository's path relative to the scanned directory. Running git-statuses from inside a checkout - the no-argument default - makes those two the same path, so the relative path is empty and the code fell back to the full absolute path. The Directory column then read `/home/user/code/my-repo` for a repository that would have been listed as plain `my-repo` had the parent directory been scanned instead. Fall back to the directory name, which is what the column shows in every other case. The absolute location remains available via `--path`.
Whether a repository is dirty and how many changed files it has were decided by two separately maintained lists of git status bits. Both listed new / modified / deleted / conflicted and neither listed TYPECHANGE, so replacing a tracked file with a symlink - which `git status` reports as a modification - left the repository reported as Clean with zero changes. Both checks now share a single `CHANGED` bitmask, which additionally covers TYPECHANGE and RENAMED, so they cannot drift apart again. `get_changed_count` also skips ignored entries explicitly, as the dirty check already did.
The depth guard read `depth != -1 && depth >= 0`, in which the first half is dead - anything that is `>= 0` is not `-1`. What it actually did was send *any* negative value down the unlimited branch, so `--depth -5` silently scanned the entire tree while the help text only ever promised that for `-1`. Rather than reject the other negative values, document what they already do: any negative depth means "no limit". Depth 0, which would produce a max depth of 0 and find nothing, is clamped to 1 as before, now via `max(1)` instead of an if/else that duplicated the constant. `i32::MIN` is covered by a test: it never reaches the `as usize` cast.
…tching Worktree metadata was kept out of the results by testing whether the path contained the literal substring `/.git/worktrees/`. That is a Unix path, so on Windows - where the separator is `\` - the check never fired. Prune the whole `.git` directory from the walk instead, using `filter_entry` on the file name rather than a rendered path. That is separator independent, and it also stops an unlimited scan from walking and stat-ing every object, ref and hook inside every repository it finds, none of which can ever be a result.
Coverage was 89.95%, which is *below* the 90% threshold the tarpaulin job already enforces - the gate was failing on main. The bulk of the gap was `src/main.rs` at 0/19 lines: everything lived in `main()`, which no test can call. Split the body out into `run(args, out)`, which takes the completion output as a writer so tests can drive every branch (table, JSON, legend, summary, non-clean, failed repositories, completions) without spawning a process. `run` turned out not to be fallible at all - repositories that cannot be read are collected into the failed list rather than aborting the scan - so it no longer returns a `Result` it never uses. New tests also cover behaviour that had none: - every in-progress git operation (merge, revert, cherry-pick, bisect, the three rebase flavours, `git am`) and the sequencer variants of revert/cherry-pick - Clean vs Unpushed against a real remote, via a clone - bare repositories, which have no working directory - `--subdir`. The existing test for it passed without ever exercising the flag: its repository sat at a depth where the ordinary scan already found it. The new one puts the checkout below the scanned level, where only `--subdir` can find it. Coverage is now 96.55%, with printer.rs, status.rs, repoinfo.rs and util.rs fully covered. The gate moves to 95% to hold the line. What remains uncovered is `main()` itself and a handful of defensive branches that need a repository git cannot produce.
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.
Fixes a set of bugs found while auditing the repo. Each is a separate commit with a regression test that fails without the fix.
Repositories silently disappearing or showing stale numbers
--fetchon a repository without a remote madefetch_originreturn an error, which propagated and got the repository filed as failed — so it vanished from the output entirely. The same happened with--ffon a branch without an upstream.Separately,
merge_ffran after ahead/behind, commit count and status were gathered, so a fast-forwarded repository was displayed with its pre-merge numbers: still "behind 1" while simultaneously marked as fast-forwarded. Fetch and merge now happen before any state is read, and both are best-effort with a warning.fetch_originalso ran git inrepo.path().parent(), which for a worktree is<main>/.git/worktrees, not a working directory.A typechanged file was reported as Clean
Whether a repository is dirty and how many files changed were two separately maintained lists of git status bits, and neither included
TYPECHANGE. Replacing a tracked file with a symlink — whichgit statusreports asT— left the repository reported as Clean with zero changes. Both checks now share a singleCHANGEDbitmask.--jsonignored--non-cleanThe filter lived inside the table printer, so
--json --non-cleanemitted every clean repository — useless for the exact scripting case JSON exists for. It now lives inArgs::filter_reposand is applied once, so the formats cannot disagree. Filtering everything away now correctly reports "No repositories found." instead of printing a table header with no rows.Non-deterministic output order
Repositories are collected in parallel via rayon and only the table sorted afterwards, so
--jsonand the failure warnings emitted a different order on every run over the same directory.Remote URL name parsing
git@host:repo.git(no slash in the path) became the wholegit@host:repostring; a trailing slash produced an empty name; andtrim_end_matches(".git")strips every trailing occurrence, so a repository genuinely namedrepo.gitwas displayed asrepo.Directory column for the scan root
Running git-statuses from inside a checkout — the no-argument default — left the relative path empty and fell back to the absolute path, so the column read
/home/user/code/my-repofor a repository that would be listed as plainmy-repohad the parent been scanned.Depth guard and Windows path matching
The depth guard read
depth != -1 && depth >= 0, in which the first half is dead. Any negative value already meant "unlimited", which is now what the help text says. Worktree metadata was excluded by matching the literal substring/.git/worktrees/, which cannot match on Windows; the.gitdirectory is now pruned from the walk by file name.Coverage
Coverage was 89.95%, below the 90% threshold the tarpaulin job already enforces — the gate was failing on main. Most of the gap was
src/main.rsat 0/19 lines, since everything lived inmain(), which no test can call. It is now split into a testablerun(args, out).Coverage is now 96.55% (
printer.rs,status.rs,repoinfo.rs,util.rsat 100%) and the gate moves to 95%.Two things in the diff worth flagging for review:
test_integration_repository_fast_forwardassertedcommits == 1, behind == 1on a repo it had just fast-forwarded — it had pinned the stale-stats bug in place.test_integration_subdir_functionalitynever exercised--subdir. Its repository sat at a depth where the ordinary scan already found it. It is left in place and a real one added alongside.