Pack the bare caches' refs on the sweep - #478
Conversation
Every ref a fetch updates is written as a loose file costing a whole filesystem block, and nothing in devlaunch ever collapsed them: pack-refs --auto is a no-op on git's files backend and no gc runs on a bare. The packing goes where the loose refs are made. fetch_all has exactly one production caller, so "after the fetch" is the detached background sweep by construction, in a repo-lock scope it already holds, and the pass that skips the fetch spawns nothing. Not justified by disk: the cost is one block per ref, so it tracks how many branches a remote leaves open rather than repository size, and a whole cache is 30 MB to 60 MB. What carries it is placement plus a second payment in probe latency. Measured on git 2.51.1 over a bare of 551 refs with 301 loose, the rev-list probe fell from 5.3 ms to 2.8 ms, the loose files held 1204 KiB of blocks against a 30 KiB packed-refs, and the pack took 23 ms. A pack that refuses is not a fetch that failed. It is a CacheNotice naming the repository and git's own words, and the freshness stamp still lands: the fetch happened, and withholding the stamp would re-fetch the whole repository every interval forever over a representation change that did not come off.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideAfter each successful all-refs freshness fetch, the detached sweep now runs a bounded Sequence diagram for packing refs after a freshness fetchsequenceDiagram
participant Sweep as Detached freshness sweep
participant Manager as RepositoryManager
participant Git as Git client
participant Bare as Bare cache
participant Record as Cache record
Sweep->>Manager: fetch_repo(owner, repo)
Manager->>Git: fetch_all()
Git->>Bare: Write updated refs as loose files
Git-->>Manager: Fetch succeeded
Manager->>Git: pack_refs(bare)
Git->>Bare: git pack-refs --all
Bare-->>Git: Packed refs
Git-->>Manager: Pack result
Manager->>Record: Update last_fetched
Manager-->>Sweep: FetchedUpdates
Flow diagram for a refused ref packflowchart TD
A[Successful fetch_all] --> B["pack_refs(bare)"]
B --> C{Pack succeeded?}
C -->|Yes| D[Update last_fetched]
C -->|No| E[Emit RefsNotPacked notice]
E --> D
D --> F[Fetch still counts as complete]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Reviewed at ec9619aa9be0231fdeae2d045a73cd5e3beddd07 in a fresh context, against #470 and the decision on #465. Two axes, run independently and not merged. In-container: cargo fmt --check clean, cargo clippy --locked --all-targets -- -D warnings clean, cargo test --workspace all suites green, pytest test/ 412 passed.
The code is right. Both blocking findings are single sentences of prose that state a prospective benefit and an unreachable reporting channel in the present tense. No defect in the pack, the placement, the notice arm, or the tests.
Standards
Nothing blocks.
Checked and found not to be smells: reason: String on RefsNotPacked (repo_manager.rs:250-255) matches every other arm that carries a git refusal verbatim (LfsCacheNotFilled, TrackedFilesNotListed, LfsFilesNotListed), where NotRefreshed is the fetch vocabulary and three of its four arms are meaningless for a pack, so reusing it would model unreachable states. The { owner, repo } clump is pre-existing in nine arms. cache_notice (dl/src/render.rs:890) is the only exhaustive match over CacheNotice in the tree and has no wildcard, so the arm required exactly the compiler-forced set and no site was missed. swap_remove(0) is correct: calls() returns an owned clone and swap_remove(0) returns index 0, the fetch.
Non-blocking, ranked:
repo_manager.rs:2516and:2734useswap_remove(0)where the file's own idiom is indexing.repo_manager.rs:2574, added by this same PR 58 lines later, andworkspace_clone.rs:2096both writecalls()[n]. A reader has to recall thatswap_removereturns the removed element rather than the one swapped into its place before they can see the assertion is about the fetch. Cost is one confused reader per visit.- The measured figures now live in four unguarded copies.
1204 KiB / 30 KiB / 23 msatclients/git.rs:596-598anddocs/cleanup.md:374-377;5.3 ms / 2.8 msatrepo_manager.rs:1419-1420anddocs/cleanup.md:374-375; thepack-refs --autono-op claim atgit.rs:594,docs/cleanup.md:355andrepo_manager.rs:3756. Re-measuring on a later git is four edits and nothing fails if one is missed. In keeping with the repo's stated position that quoted numbers are not guarded, just at a higher copy count than elsewhere. repo_manager.rs:1430has notiming::spanwhere its siblingfetch_allat:1397does.DEVLAUNCH_TIMINGon a sweep shows the fetch and not the 23 ms pack beside it, so a future regression in pack cost is invisible to the tool this repo measures with.
Spec
#470 items 1, 2 and 4: met. clients/git.rs:621 spawns exactly ["pack-refs", "--all"], cwd the bare, .with_timeout(ABOUT_ONE_REPO) where git.rs:70 defines that as 30s; no --auto anywhere. The call at repo_manager.rs:1428 sits after the fetched.refusal() early return (ending :1410) and before update_repository (:1440), as its own CacheNotice arm and not a let _ =. docs/cleanup.md:349-351 carries the contract row; U+2014/U+2013 across docs/ and README.md hit only rust-port-scope.md, the documented exclusion.
#470 item 3: met. Sourcery's assessment is refuted on the merits, by construction. Sourcery marked item 3 unaddressed because "the real Git test does not explicitly capture and compare the complete ref set before and after packing, nor does it assert that the packed-refs count increases". Right on the letter, wrong on the entailment: the fixture is closed, so the hardcoded assert_eq!(refs_of(&bare), [...]) at repo_manager.rs:3799 is the before-set. I built the counterexample it alleges rather than arguing it. In a scratch worktree I replaced pack_refs with a call that packs correctly and silently drops one ref:
.with_args(["-c", "git update-ref -d refs/heads/feature/test && git pack-refs --all"])
panicked at repo_manager.rs:3799: packing changes the representation and must lose no ref
left: ["refs/heads/main", "refs/tags/v1"]
right: ["refs/heads/feature/test", "refs/heads/main", "refs/tags/v1"]
A pack that packs nothing is caught twice over, at :3781 (loose set non-empty) and at :3792, and that second assert is stronger than "the count rises": it proves the main line was rewritten to the post-fetch sha, not merely that a line was added. No counterexample survives. Non-blocking nit: refs_of is name-only, so feature/test's objectname is unpinned, but that is the one ref with no stale packed line, so the failure mode cannot arise there.
BLOCKING S1 — docs/cleanup.md:372-375 and repo_manager.rs:1417-1420 state a prospective benefit in the present tense.
docs/cleanup.md:372— "the guards that decide whether a clone is safe to remove ask the bare a reachability question that walks every ref; with the refs packed that walk reads one file instead of thousands."
repo_manager.rs:1417— "packing pays a second time in probe latency, since the reachability question asked of the bare walks every ref and today reads one file per ref to do it".
commits_beyond_every_ref does not exist in the tree. The shipped probe is git log --oneline --all --not --remotes (clients/git.rs:514), run via about(clone, ...) against the clone, called only from domain/workspace_state.rs:469. No shipped code asks the bare a reachability question, so packing the bare buys the guards zero today, not less. #446's rev-list --not --all on the bare is decided and unbuilt.
Concrete failure: docs/cleanup.md is #444's contract page, the artefact the map's destination is defined as. A reader weighing whether to keep, tune or remove this pack reads a measured 5.3 ms to 2.8 ms saving that no shipped code collects. The PR body concedes both are prospective under "Out of scope"; the page and the call site do not, and a PR body is not where a reader meets this. This map corrected exactly this class of false present-tense doc claim before merge on #464. Fix is tense plus a pointer to #446. Note the doc comment on Git::pack_refs (git.rs:589-620) is clean, makes no probe claim, and is the right shape.
BLOCKING S2 — docs/cleanup.md:380 promises a warning on a channel that is /dev/null in the only path that can produce it.
"so a refusal is a line on stderr naming the repository and git's own words, the record's freshness stamp still lands, and the next sweep tries again."
Traced: RefsNotPacked reaches SweepReport.notices, say() at dl/src/commands.rs:1191, eprintln!. But sweep_repo_fetches has exactly one production caller (dl/src/commands.rs:334, inside render_update_cache), reachable only as dl --update-cache, which is spawned only at lifecycle.rs:352 via runner.detach(...), and ProcessRunner::detach sets command.stderr(Stdio::null()) at devlaunch-runner/src/lib.rs:637. ProcessRunner is the only production Runner. So the line is written into a null descriptor on every production path, and --update-cache is hide = true.
Concrete failure: someone whose bare keeps loose refs is told by the contract page to look for a named warning on stderr. There is never one. The render.rs:3086 test comment compounds it, calling a line that did not name the repository "unactionable", which asserts an actionability that no user can reach.
In fairness, and this is why the fix is prose and not code: the invisibility is pre-existing and systemic. FetchingUpdates and FetchedUpdates are equally nulled, because fetch_repo is sweep-only. This PR did not introduce it. It is the first place that promises a user-visible warning for the sweep, and that promise is not kept. Fixing the sweep's reporting channel is a different ticket; correcting the sentence is this one.
#444 principles: honoured, and principle 2 holds on measurement rather than on argument. I tested whether a permanently refusing pack accumulates. It does not, it plateaus. Eight consecutive sweeps, each moving all 50 heads, no pack ever run:
sweep | refs | loose files | bytes in loose refs (blocks)
1 | 51 | 50 | 204800
...
8 | 51 | 50 | 204800
Flat. Loose refs are one file per ref, rewritten in place by the next fetch, never appended to. So a persistently failing pack degrades to exactly the pre-PR steady state, bounded by the remote's ref count, which is the state #465 already classified as "principle 2 barely bites". Warn-forever is the accepted trade, not a new straggler. Principle 1 is untouched: packing removes no ref, and the refused path keeps both the stamp and the fresh cache.
What I verified by running it, not by reading it
Concurrent readers during pack-refs --all (the container/lock question): safe, 636 iterations, zero anomalies. git 2.51.1, a 401-ref bare, writer making all 300 heads loose then pack-refs --all, 60 rounds, against a reader looping git clone from the bare plus fsck, for-each-ref and rev-parse --verify. Every clone exit 0, every clone got exactly 401 refs, every clone fsck-clean, zero missing refs, zero wrong shas across ~2000 resolutions. (Two earlier runs reported anomalies that were my harness miscounting HEAD and origin/HEAD, not git.) The architecture makes the window narrower than it looks anyway: clone_from_cache hardlinks and then set_remote_url repoints the clone at the forge, so a container's clone has its own refs and never reads .bare/refs at all, and the one devlaunch reader that does read them runs on the launch path under the same per-repo lock the sweep takes via run_if_lock_free.
The deletion finding: re-run, confirmed, and extended to tags. Case A, 100 packed heads plus 100 packed tags retracted upstream, fetched with devlaunch's real refspec +refs/heads/*:refs/heads/* +refs/tags/*:refs/tags/* --prune: 401 refs to 201, packed-refs 22487 B to 11403 B, zero loose files created, survivors intact. Case B, the resurrection case, built for a head and a tag: a loose file at the new sha over a stale packed line at the old sha, then retracted upstream. Run twice, once on a bare left unpacked and once on the same bare after pack-refs --all:
=== bare-nopack === === bare-packed ===
refs/heads/shadowhead -> GONE refs/heads/shadowhead -> GONE
refs/tags/shadowtag -> GONE refs/tags/shadowtag -> GONE
loose files left: 0 loose files left: 0
packed-refs mentions shadow*: 0 packed-refs mentions shadow*: 0
Nothing resurrected at the old sha in either arm, survivors intact, outcomes identical. Also confirmed the pack itself resolves the shadow correctly: after pack-refs --all the packed line carried the new sha, not the stale one. The author's claim holds, including for the tags #464 added an hour before this branch.
--all is pinned by tests, not only by prose: three of them, proved by mutation. Removing --all in a scratch worktree fails clients/git/tests.rs:516 (left: ["git", "pack-refs"]), repo_manager.rs:2559 (the flow argv sequence), and behaviourally repo_manager.rs:3781, which leaves refs/heads/main loose. That third one is the 301-untouched-heads observation reproduced in miniature by the suite itself.
The branch was left on wayfinder/devlaunch-470 at ec9619a, working tree clean; the mutation worktree was removed.
Verdict
Request changes (posted as a comment: GitHub refuses --request-changes on a same-account PR, so this written verdict is the gate).
Two blocking findings, both in the Spec axis, both single sentences, both accuracy rather than design:
- S1 —
docs/cleanup.md:372-375andrust/devlaunch-core/src/flows/repo_manager.rs:1417-1420state #446's bare-side reachability probe as present tense. It is unbuilt; the shipped probe runs on the clone. Fix the tense and point at #446. - S2 —
docs/cleanup.md:380promises a stderr line for a refused pack. The sweep's only production path is a detached child withstderr(Stdio::null()), so no user ever sees it. Say what actually happens, or say the channel is not reachable today.
Standards passes with no blocking findings. The pack, its placement, the CacheNotice arm keeping last_fetched, and the test set are all correct, and the two claims the ticket flagged for hard scrutiny both survived independent re-measurement.
…reads The probe saving is real and nothing collects it: the bare-side reachability guard is decided and unbuilt, and the probe that ships asks the clone. Placement is what carries this call today, so say that in the contract page and at the call site rather than only in the PR. The refusal notice is not a line on stderr. The sweep runs detached with its output discarded, so every notice it raises goes to a null descriptor. Name that, and name what a failing pack costs while it goes unread: loose refs are rewritten in place, so the count stays flat at one sweep's worth rather than growing.
1 participant
Closes #470. Implements the decision on #465.
One
git pack-refs --allon the detached freshness sweep, after a fetch that succeeded.The red test
real_git_the_sweep_packs_the_loose_refs_its_own_fetch_just_wrote, against a real bare cut from a real fixture remote. Clone (which arrives packed), movemainon and push a new tag, runfetch_repo, assert nothing is loose:The two files are the fetch's own output, which is the whole point of the placement. The test's second half is what stops it being vacuous:
packed-refsmust hold the post-fetch sha formainand a line forv1, so a run that packed nothing would leave both refs as files with a clone-timepacked-refsbeside them.Re-measured, because #464 moved the population
#464 made the tags refspec forced, so the sweep now prunes tags as well as heads and #453's 5286 loose refs describe a bare that no longer exists. Measured fresh on git 2.51.1, and the more useful correction is about where loose refs come from at all:
git clone --barearrives packed. 601 refs, zero loose files, a 33787 Bpacked-refs. The loose refs are made by fetching, one file per ref the fetch updates, and they shadow apacked-refsthat keeps the pre-fetch shas.pack-refs --alltook it back to zero loose with no ref lost.rev-list --count HEAD --not --allon the bare, 30 runs each: 5.3 ms to 5.5 ms loose, 2.7 ms to 2.9 ms packed over 551 refs with 301 of them loose. The pack itself: 23 ms.for-each-refover name plus objectname).The disk headline stays where #465 left it: one block per ref, tracking how many branches a remote leaves open rather than repository size, so a whole cache is 30 MB to 60 MB. That is not what carries the change.
--allis not a nicety. Against a bare holding 301 loose heads and 101 loose tags, a barepack-refstook the tags to zero and left all 301 heads untouched.Packed versus loose deletion
Checked because packing and pruning meet on the same refs, and it is a no-difference finding rather than a hazard:
--pruneremoved all 100, rewrotepacked-refs(33787 B to 28255 B), created no loose files, and the survivors were untouched.So packing changes nothing about what a later prune may delete or keep. The only difference is cost, and it falls on the prune: removing a packed ref rewrites the whole file where removing a loose one unlinks one path.
A pack that refuses
The fetch is the point of this function and the packing is the optional half, so a refusal is a
CacheNotice::RefsNotPackedcarrying the repository and git's own words, not aFetchRepoError, andlast_fetchedstill lands. Principle 1 decides it and it is not close: the fetch happened, the cache is fresh, nothing is at risk. Withholding the stamp would be the actual harm, since every later sweep would re-fetch the whole repository forever on account of a representation change that did not come off. Its own arm rather thanlet _ =, which is the disciplineRecordUpdate::Absentalready gets three lines below. Pinned bya_pack_that_refused_is_reported_and_the_fetch_still_counts_as_done.Argv pins
pack-refs --all, cwd the bare, bounded at 30 s, is pinned at the client seam; the flow seam pins that the pack follows the fetch and precedes the stamp. Three existing pins moved with the second spawn: twoonly_call()reads became the fetch call, and the lazy-fetch count went 1 to 2, with the skipped pass still spawning nothing.Out of scope
Not on
--prune, per #465:PrunePlanis classify-and-remove-directories with no arm for "mutate something we keep". No--autoand no loose-ref threshold. Therefs/headsandrefs/tagsdirectories git leaves empty behind a pack are left alone. And the "second payment" in probe latency is measured here but prospective in the tree: the shipped unpushed probe islog --all --not --remoteson the clone (#471); #446'srev-list --not --allagainst the bare is decided and not yet built.🤖 Generated with Claude Code
Summary by Sourcery
Pack loose refs in bare caches after successful sweep fetches while preserving fetch success semantics and reporting packing refusals.
New Features:
Bug Fixes:
Enhancements:
pack-refs --alland integrate it between fetching and freshness bookkeeping.Documentation:
Tests: