Repository navigation
Fetch tags with a forced refspec - #476
Conversation
`--tags` is `refs/tags/*:refs/tags/*` unforced, and `git fetch --prune` prunes per refspec. One word, two bugs. A tag the remote moved was rejected with `would clobber existing tag`, which fails the whole fetch and keeps failing, since nothing here ever resolves it. `RepoManager::fetch_repo` reports that as `Refused`, so a repository's freshness fetches stayed broken until a human deleted the local tag by hand. The heads pushed alongside it did not arrive either. And a tag the remote retracted was never pruned, leaving `refs/tags` monotone in every tag the remote ever advertised and pinning every object those tags reach for the life of the cache. Pruning a tag is a deletion, so the reading that licenses it is written down where the refspec is: a tag in the bare is a copy of an upstream ref and holds no work that exists nowhere else, which is the argument that already licenses pruning heads. Nothing in the bare is a place work is authored, and devlaunch creates no tags. The older consequence of pruning heads, a dangling `HEAD` symref when the default branch goes upstream, is recorded there too rather than handled.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThe PR makes broad repository fetches use explicit forced refspecs for both heads and tags, allowing moved tags to update, deleted tags to prune, and same-fetch branch updates to succeed instead of being blocked by a rejected tag. Tests and documentation establish the intended command line, safety assumptions, and unchanged out-of-scope behavior. Sequence diagram for forced tag and branch fetchingsequenceDiagram
participant RepoManager
participant Git
participant Remote
participant BareCache
RepoManager->>Git: fetch_all(bare, limit)
Git->>Remote: fetch origin +refs/heads/*:refs/heads/* +refs/tags/*:refs/tags/* --prune
Remote-->>Git: branch updates, tag updates, deleted-tag pruning
Git->>BareCache: update refs/heads and refs/tags
Git-->>RepoManager: successful fetch
Flow diagram for moved and deleted tag handlingflowchart LR
A[Remote changes tags] --> B[Forced tag refspec]
B --> C{Remote tag state}
C -->|Moved| D[Update refs/tags]
C -->|Deleted| E[Prune refs/tags]
B --> F[Branch refspec also applies]
F --> G[Same-fetch branch updates succeed]
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.
Fresh-context review of 13072989041e0a1a7aa3dd100317c253ae2a803a against merge-base e55239a. Five files, one production line. Because that line turns tag fetching into something that deletes refs automatically and unflagged, the principle-1 surface was re-measured from scratch rather than read off the breadcrumbs. Everything below was run, not reasoned about; git 2.51.1 in this repo's container.
Standards
Clippy (--locked --all-targets -D warnings) clean, fmt --check clean, cargo test --workspace green (1291 core tests), pytest test/ 412 passed. All 15 CI checks green, MERGEABLE / CLEAN.
S1 (minor) — flows/repo_manager.rs:3551: "licensed by principle 1" is an unresolvable reference. grep -rn "principle 1" over every *.rs and *.md in the tree returns this line and nothing else, and the sentence is missing its connective ("principle 1 reading a bare's tag as..."). It points at an out-of-tree wayfinder principles list, so a reader cannot tell whether it names a doc section, an ADR, or a typo. The same argument is stated in full at clients/git.rs:557-560; point there.
Not a smell, checked rather than assumed — the four-place argv pin. clients/git.rs:575, clients/git/tests.rs:483, flows/lifecycle.rs:5493, flows/repo_manager.rs:2460. clone --bare is already pinned at eight-plus sites across the same four layers, so this is the established layered-pin convention, each site guarding a different layer's fake runner. The diff added no new site.
Not a violation — the doc comment's em dashes. test/test_docs_prose.py globs README.md plus docs/*.md; Rust source is out of scope and the surrounding house style uses them freely.
Test conventions — both new tests reuse a_cache / a_fixture_remote / real_git / a_manager / run_git / refs_of / ignoring / commit_on exactly as the neighbouring real_git_fetches_a_commit_pushed_after_the_clone_was_made does, keep the real_git_ prefix, and duplicate no helper.
Spec
Ticket #464. All four asks met.
- "replace
--tagswith an explicit+refs/tags/*:refs/tags/*" —clients/git.rs:576. Done. - "A test per effect" — all three present, and verified genuinely red: with only the argv reverted to
--tagsin a scratch copy,real_git_prunes_a_tag_the_remote_deletedfails (refs/tags/retractedsurvives) andreal_git_updates_a_tag_the_remote_moved...fails withRefused { ... would clobber existing tag }. Argv pinned at three layers. - "If that reading is accepted, say so" — stated in code, not only on the ticket:
clients/git.rs:557-560. - "record it as a known consequence with the symptom named" —
clients/git.rs:563-568.
P1 (minor, accuracy) — clients/git.rs:566-567: "rev-parse HEAD is what fails" is measured false. On a bare whose upstream deleted its default branch, plain git rev-parse HEAD exits 0 and prints the literal string HEAD; only rev-parse --verify HEAD exits 128. No shipped code runs rev-parse HEAD on a bare at all (git.rs:465 runs rev-parse --abbrev-ref HEAD, on the clone, and that also exits 0 printing HEAD). The symptom the sentence names is right, and the real observable is git clone <bare> warning remote HEAD refers to nonexistent ref, unable to checkout. Concrete failure: a future reader reproducing the edge runs the command the note names, sees exit 0, and writes the whole note off as stale.
Refuted, and recorded because it is re-derivable and wrong. The claim at git.rs:551-552 and repo_manager.rs:3512-3514 that a rejected tag takes the head in the same push down with it is correct, though git's own stdout argues otherwise. Under --tags, git prints ddbc007..57db69e main -> main before ! [rejected] release, which reads as the head having arrived. It has not: for-each-ref on the bare shows refs/heads/main still at the pre-fetch sha. On git 2.51.1 the ref updates are one transaction and the rejected tag rolls the head back, repeatably on every retry. So repo_manager.rs:3541-3543's second assertion is meaningful, not decorative. Worth knowing that this atomicity is a property of the pinned toolchain rather than of git in general.
The principle-1 press
(a) "the bare and nothing else, and devlaunch authors no tag" — verified exhaustively, holds.
fetch_all has exactly one production caller: RepoManager::fetch_repo at repo_manager.rs:1386, whose bare is self.bare_dir(owner, repo) (:1371, :1085-1087, :93-95) and can be nothing else. The other two references are in clients/git/tests.rs. The bare is never a user's clone: workspace clones are siblings created by clone_from_cache (workspace_clone.rs:875) and repointed at the forge by step 2 (:885-897), which hard-errors if it fails, and .bare is deliberately not bind-mounted into the devcontainer. On tag authorship, I enumerated every Invocation::new(PROGRAM) in clients/git.rs — clone, clone --bare, fetch, symbolic-ref, branch, ls-remote, for-each-ref, show-ref, push -u, remote set-url/get-url, checkout, ls-files, lfs fetch/pull, log, rev-parse — and git has exactly one entry point (PROGRAM, git.rs:60). There is no tag verb, no push --tags, no --follow-tags, and grep -rn "git tag" over the whole tree returns nothing outside the new fixtures, which tag the remote work tree. Only four things write into the bare: clone_bare, fetch_all, fetch_ref (heads only), lfs_fetch_into_cache. No counterexample.
One residual, named because it is real and because it is not this PR's: if set_remote_url fails after the clone, the workspace keeps origin pointing at the bare, and a later launch takes the is_new_workspace = false path which never retries step 2. A hand git push origin <tag> there would author a tag in the bare. That hazard exists identically today for branches under the pre-existing +refs/heads/* + --prune, so this diff extends a standing policy rather than opening a class.
(b) what happens to the objects the old tag reached — verified, nothing needs them.
Measured end to end: bare's rel→OLD, upstream force-moves it, fetch under the new refspec, then git gc --prune=now. OLD is gone from the bare (cat-file fatal); an existing workspace clone still holds it (cat-file → commit), fsck clean, and its own refs/tags/rel still pins OLD — a local clone-from-path hardlinks its objects, so the clone is independent of the bare's gc. No shipped code reads reachability from the bare (grep rev-list over shipped source: nothing; #446's rev-list --not --all on the bare is design, not code), so no safety verdict moves. devlaunch never launches at a tag: fetch_ref is +refs/heads/<b>:refs/heads/<b> and the start point is origin/<branch> in a clone whose origin is the forge. LFS is fetched into the bare per branch ref (lfs_fetch_into_cache), so no payload there is pinned by a tag alone. And by (a), OLD was a copy of what upstream once advertised.
(c) the #471 interaction — re-run, not trusted. Narrowing confirmed; "cannot widen" is over-stated in the safe direction.
Real bare→clone topology, probe = log --oneline --all --not --remotes in the clone.
| case | old --tags |
new forced refspec |
|---|---|---|
| tag whose branch upstream deleted (docs/cleanup.md's half) | phantom relcommit, exit 0 |
phantom relcommit, exit 0 — identical |
| tag itself retracted upstream | tag survives, phantom commit in every later clone | pruned, probe silent — genuine narrowing |
| tag force-moved onto a commit upstream then orphans | fetch exit 1, tag frozen at X, probe silent | t [tag update], probe reports sidework |
Row 2 is the author's claim and it holds. Row 3 is a construction where the new refspec produces a false positive the old one did not — but the only state it improves on is one where the repository's freshness fetch is permanently broken, and the false positive it lands in is exactly the class docs/cleanup.md:470-474 already names. Direction is toward keeping a clone, which is principle 1's preferred failure. So: not blocking, and no reversal of #471 — the diff touches five Rust files, none of them docs/cleanup.md, and Git::unpushed_commits and its doc comment (git.rs:480-517) are untouched. The resolution record on #464/#444 should read "narrows in every reachable non-wedged state" rather than "cannot widen", because #475 and #455 will cite it.
(d) the workspace_clone guard with one fewer discriminator — verified, loses nothing.
nothing_broad_is_ever_fetched_in_the_foreground (workspace_clone.rs:2558-2599) is anchored by assert_eq!(fetches, [["git","fetch","origin","+refs/heads/feature/x:refs/heads/feature/x"]]) at :2586. Exact equality over the whole fetch list already forbids --tags, --prune, any wildcard, and any extra fetch; the residual assert! is wholly implied by it, so the deleted clause was never an independent discriminator. The surviving contains('*') clause does cover the new broad argv, since +refs/tags/*:refs/tags/* contains *.
The deliberate exclusion is honest. Measured both refspecs against a bare whose upstream deleted its default branch: identical outcomes — refs/heads/main pruned, symbolic-ref HEAD still answering refs/heads/main at exit 0, git clone <bare> warning remote HEAD refers to nonexistent ref, unable to checkout. Both halves are produced by +refs/heads/* and --prune, neither of which this diff touches. Not caused here, not worsened here.
Verdict
Approve. (Posted as a comment: GitHub refuses --approve on a same-account PR, so this written verdict is the gate.)
Nothing blocks. The deletion this PR introduces is licensed: exhaustive enumeration finds no place devlaunch authors a tag, one production caller pointing at the bare cache alone, and no shipped consumer of the objects an overwritten tag reached. The two findings are both two-sentence comment fixes and neither touches behavior:
- S1
flows/repo_manager.rs:3551— "principle 1" resolves to nothing in-tree. - P1
clients/git.rs:566-567—rev-parse HEADdoes not in fact fail;rev-parse --verify HEADdoes.
Plus one record-only correction: (c)'s "cannot widen" should be "narrows in every reachable non-wedged state", so #475 and #455 inherit an accurate premise rather than a slightly strong one.
The HEAD note named `rev-parse HEAD` as what fails. Measured, it exits 0 printing the literal `HEAD`; it takes `--verify` to fail, which is why nothing here reports one, and what an eye actually meets is git clone warning about a nonexistent remote HEAD. The prune test's licence cited "principle 1", the only such reference in the tree and one that resolves to nothing inside it. State the argument instead, as `fetch_all` already does.
1 participant
Closes #464.
Git::fetch_allasked for+refs/heads/*:refs/heads/* --tags --prune.--tagsisrefs/tags/*:refs/tags/*unforced, andgit fetch --pruneprunes per refspec, so that one word cost two things at once. It now asks for both refspecs spelled out and both forced.Red first
Two tests at the repository-manager fetch, against a real remote on a local path, which is the level the wedge is reported at rather than merely caused at. Both failed before the change, with git's own words:
Read the first failure's
reasonclosely:main -> mainis in it. A rejected tag fails the whole fetch, so the head pushed in the same breath did not arrive either. The test asserts both, because the collateral damage is the part a reader would not guess.I re-verified #453's measurement rather than trusting it, on git 2.51.1: the production refspec exits 1 on a force-moved tag and exits 1 again on every repeat, which is what makes
FetchRepoError::Refusedpermanent; the forced refspec printst [tag update], exits 0, and prunes the retracted tag in the same run.The third test is the argv pin, updated in three places (the git client, the manager's faked sweep, and lifecycle's
BROAD_FETCH) and now carrying its reason in a comment, so the refspec cannot regress to--tagsquietly.The principle-1 call, made explicitly
Pruning a tag deletes a ref, so the reading that licenses it is written down beside the refspec rather than left implied: a tag in the bare is a copy of an upstream ref and holds no work that exists nowhere else, which is exactly the argument that already licenses pruning heads. Two things make that safe rather than merely plausible here, and both are checked rather than assumed:
fetch_allruns against the bare cache and nothing else, never a user's clone; and devlaunch creates no tag anywhere, so the bare'srefs/tagshas no local author. Automatic, not a flagged path.The #471 interaction, measured
Forcing and pruning the tag refspec narrows #471's deliberate false positive and never widens it.
The clone probe asks
log --oneline --all --not --remotes, and--allspansrefs/tagswhile--remotesspans branches only. Built the case and ran it both ways: a tag on a branch the remote later deleted, in a bare that had fetched before the deletion, then a clone cut from that bare.refs/tags/v1, and the probe in the clone answers142964b b. A phantom unpushed commit, permanently, in that clone and in every later clone of the repository.- [deleted] (none) -> v1, and the probe answers nothing.So the strongest half of the class goes: a tag upstream has retracted no longer reaches a clone at all. What survives is the half
docs/cleanup.mdalready describes and only that half, a tag the remote still carries which no remote branch reaches, so the page's wording is accurate before and after. Widening is not reachable from here: forcing only ever moves a local tag onto the position upstream advertises, and pruning only ever removes one upstream no longer advertises, so no commit can become clone-only that was not clone-only already.#471's trade is untouched on purpose. It was decided, and it is decided on a class this change does not reach; reversing it is #475's, not this PR's.
Also in scope, and recorded rather than handled
#453 found that
--prunetakesrefs/heads/mainwith it when the remote's own default branch disappears, leaving the bare'sHEADsymref dangling. That is a consequence of the heads refspec and predates this change, so it is written intofetch_all's doc comment with the symptom named:symbolic-ref HEADstill answersrefs/heads/mainand default-branch detection still returns a name, so the failure surfaces later, as a launch aimed at a branch the cache no longer has. Handling it means deciding what a bare should pointHEADat when upstream has no default branch, which is a decision, not a fix, and does not belong in a one-word refspec change.Not in scope
No new flag, so
README.mdis untouched.docs/cleanup.mdneeds no edit: its claim about tags reading as unpushed was already the narrower, post-fix-accurate one. The other half of #453's report,pack-refson the detached sweep, is #465's.🤖 Generated with Claude Code
Summary by Sourcery
Force tag refspecs during repository cache fetches to keep cached tags synchronized with the remote.
Bug Fixes:
Enhancements:
Tests: