Repository navigation
The bare cache becomes the repo's LFS store; workspaces hardlink from it - #209
Conversation
git-lfs objects are not git objects, so the bare cache that makes a workspace's history free carried none of the large files: every workspace of an LFS repo downloaded the whole payload from the forge and kept a private copy of it in .git/lfs/objects, on top of the worktree copy. Fetch the launched ref's LFS objects once into <bare>/lfs with the bare as cwd and the fetchrecent knobs zeroed, then materialize each workspace with `git lfs pull "file://<bare>"`. Measured (git-lfs 3.7.1, ext4): that pull hardlinks -- same (st_dev, st_ino) as the cache's object, zero extra disk -- and completes with the remote deleted from disk, so the second workspace touches the network for its large files not at all. Nothing host-specific is persisted into the clone: no lfs.storage, no added remote. The clone is bind-mounted into the devcontainer and .bare is not, so a host path in its config would break every in-container `git checkout` of an LFS repo while working on the host. The origin pull remains as a second phase, entered when a pointer survives the cache phase -- decided by pointer content, not by the cache commands' exit codes -- so the RuntimeError-and-retry contract is unchanged and neither cache step can fail a launch. Closes #163
Reviewer's GuideMakes the bare cache the authoritative git-lfs object store for each repo and updates workspace cloning to hardlink LFS objects from the bare cache, with new unit/integration tests, docs, and test infrastructure to enforce correct behavior and host-agnostic configuration. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #209 +/- ##
==========================================
+ Coverage 92.28% 92.38% +0.09%
==========================================
Files 24 24
Lines 3448 3467 +19
==========================================
+ Hits 3182 3203 +21
+ Misses 266 264 -2
🚀 New features to boost your workflow:
|
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two-axis review of #209 against ticket #163 at merge-base ba14410. Preflight: all checks green (ci ×5, e2e, prek, gate, codecov patch+project, GitGuardian), non-empty diff (11 files, +1120/−472), MERGEABLE.
Standards
Nothing blocking. The change matches this repo's house style (constraint-carrying docstrings, static helpers on WorkspaceCloneManager, timing.span per subprocess, module-level pytestmark, fixtures re-exported through test/conftest.py's import + __all__), and the lockfile claim verifies.
- Lockfile claim verified mechanically (observation). Parsed both
pixi.lockrevisions: across all five environments (default,py310–py313, linux-64) the only membership change is+git-lfs-3.7.1-hd3da7ce_3.conda; zero removals, zero version/build changes, no environment added or dropped. The 872-line churn is the v6→v7 format rewrite alone. One residual: v6 recordedoptions: pypi-prerelease-mode: if-necessary-or-explicitin each of the 5 envs and v7 records nooptionsblock anywhere. Package set is identical so resolution is unaffected, but it is the one line of the rewrite that isn't pure reshaping. Also nothing pins a pixi version (setup-pixi@v0.9.6, nopixi-version), so contributors on a pre-v7 pixi now can't read the lock. _materialize_lfsshape is defensible but asymmetric (nit). It is now gate → cache-fill → cache-pull → gate → inline origin pull. Two of three steps are extracted helpers withlogger.warning-and-continue semantics; the third is inlined with raise semantics. Extracting_pull_lfs_from_originwould make the body five lines that read as the phase diagram the 30-line docstring describes. Not a god function — it's an orchestrator with one inlined leaf.- Rationale prose is duplicated ~5× (nit, Shotgun Surgery in comments). The "
file://not-c lfs.storage, becauseGIT_CONFIG_PARAMETERSreaches the remote-side child" argument and the "measured, git-lfs 3.7.1, ext4, hardlinks" claim each appear in_pull_lfs_from_cache,_materialize_lfs, thetest_lfs_object_sharing.pymodule docstring, its class docstring, README and CHANGELOG. When the constraint changes, five places drift. - Stale prose the rider left behind (nit).
devlaunch/worktree/workspace_clone.py:590still says the integration test asserts "samest_ino" — the rider changed exactly that test to(st_dev, st_ino). - Fixture name now means two things (nit). The folded
clone_manager(real cache, real git) shares its name with local fixtures intest/test_workspace_clone.py:128(mock-based) andtest/test_repo_lock_cycles.py:234(tmp_path). Shadowing resolves it, but the fold's own argument — one name, one wiring — points atreal_clone_manager, matching the siblingreal_managers. - Sibling test files re-derive the identity check (nit).
identity()intest_lfs_object_sharing.py:131;test_clone_object_sharing.py:91inlines the same tuple pair, in a file the new module names as its sibling.
Spec
Verified locally: git-lfs 3.7.1 present; all 4 new integration tests pass; full suite 1624 passed (matches PR claim); mutation _fill_cache_lfs_store/_pull_lfs_from_cache removed → exactly the 2 sharing tests go red, the 2 host-specific guards stay green.
Nothing blocks.
Spec conformance (all satisfied):
- "Run
git -c lfs.fetchrecentrefsdays=0 -c lfs.fetchrecentcommitsdays=0 lfs fetch origin <ref>with the bare as cwd" —workspace_clone.py:339-361, argv andcwd=bare_pathexactly as written, ref isworkspace.ref. - "Then
git lfs pull "file://<bare_path>"in the workspace" —:376-380,cwd=ws_path. - "Keep today's
git lfs pull originonly as a second phase, entered when_has_lfs_pointersstill holds" —:422-435: the gate is the content predicate re-run, not exit codes. Cache-phase failures are caught and only warned. Confirmed empirically: the remote-deleted integration test would raiseRuntimeErrorif a pointer survived, and it passes. - "Keep the existing
RuntimeError-and-retry contract and the deliberate non-gating onis_new_workspace" — originexceptclause byte-identical;_materialize_lfscall at:708is at method-body indent, outside theis_new_workspaceblock. - "no
lfs.storagein.git/config, no extra remote" — both integration guards pass; URL checked, not just name. - "Add
git-lfsto the pixi test-feature dependencies" —pyproject.toml:36-38; all five environments includetest, so it is genuinely present. - "Touch nothing in
repo_manager.py's fetch flow" — untouched. - Amendment: "Any test this ticket adds against the union behaviour must use real git." No new unit test asserts union behaviour; the union path is exercised only by real git in the new integration file. Compliant. The amendment's "re-examine the predicate under this ticket's own design" is satisfied only implicitly (the remote-deleted test would fail if it didn't hold) and is never stated as such in the PR body — non-blocking.
Non-blocking findings:
- Stale comment at
workspace_clone.py:625-628: "the clone source is the local bare cache, which has no LFS objects". After this PR the bare does hold LFS objects for previously-fetched refs, so the stated rationale forGIT_LFS_SKIP_SMUDGE=1is now false. Behaviour is still correct; the comment contradicts the feature it sits next to. The same wording was correctly deleted from_materialize_lfs's docstring and missed here. - Every new workspace of an already-full cache still pays a network round trip for the bare-side fetch. Spec-mandated ("1. Run … 2. Then …"), so not a defect — just noting the saving is the payload, not the trip.
Declared deviations:
- pixi.lock v6→v7 — sound. Verified mechanically by both axes independently: the sorted set of
conda:/pypi:URLs differs by exactly one line,git-lfs-3.7.1-hd3da7ce_3.conda. - Two of four integration tests green-before — sound. The ticket itself asks for them; they are regression guards for a constraint the ticket calls binding, and M7 is caught by nothing else. The reviewer's own red/green run reproduces exactly the split the PR declares.
- #208 riders folded in — sound. Test-only, at the same seams, no production behaviour; the
clone_managermove is shadowed correctly by the two local fixtures that still define their own.
Verdict
Approve — no blocking findings on either axis; the Spec axis independently reproduced the mutation split and the union-path behavior with real git. Non-blocking riders for the next toucher of these seams: fix the now-false comment at workspace_clone.py:625-628 (bare "has no LFS objects") and the stale "same st_ino" prose at :590; consider real_clone_manager as the shared fixture's name; consider extracting _pull_lfs_from_origin. GitHub refuses --approve from the PR author's account, so this is posted as a comment; this written verdict governs. Merging with a merge commit per repo rules.
Closes #163. Decided on #154; the direct extension of #208, which pinned that
git clone <bare> <ws>hardlinks the pack files. This does the same for thefiles the pack does not contain.
What was wrong
git-lfs objects are not git objects. The bare cache makes a workspace's history
free, and carried none of the large files — so every workspace of an LFS repo
downloaded the entire payload from the forge and kept a private copy of it in
.git/lfs/objects, on top of the worktree copy. The cache made the cheap partfree and left the expensive part fully duplicated, per workspace, forever.
What it does now
_materialize_lfsgained the bare and the ref, and runs in two phases:git -c lfs.fetchrecentrefsdays=0 -c lfs.fetchrecentcommitsdays=0 lfs fetch origin <ref>with the bare as cwd — that cwd, and nothing else, is what puts the objects
in
<bare>/lfs, which is why nolfs.storageis needed. The two knobs boundit to the launched ref; at their defaults git-lfs also walks recent refs and
recent commits, so launching one branch of a busy repo would drag several
branches' payloads down with it.
git lfs pull "file://<bare>"in the workspace.git lfs pull origin— unchanged.Measured, before writing any of it
git 2.5x, git-lfs 3.7.1, ext4, against a local fixture remote built the way the
suite builds one:
lfs/is empty, so the bare-side fetch is genuinely what fills itgit lfs pull "file://<bare>"shares the object(st_dev, st_ino),st_nlink5 — a hardlink, zero extra diskoriginat a URL that does not resolvelfs.storagein.git/configlfs.repositoryformatversion, which is not host-specificgit remoteis justorigin, still the forgeFailure arms measured too, because they decide the shape: an empty cache store
makes the
file://pull exit 2 (remote missing object), and a dead origin makesboth the bare-side fetch and
git lfs pull originexit 2. So neither cache stepcan be fatal — each degrades to the origin pull, and the
RuntimeError-and-retrycontract stays the only thing that raises.
The constraint the design is shaped around
Nothing host-specific may be persisted into the clone.
dlhands the clonedirectory to
devpod up, which bind-mounts it;.bareis a sibling and is notmounted. An
lfs.storageentry or an added remote naming a host path would workperfectly on the host and break every in-container
git checkoutof an LFSrepo — a failure that appears nowhere near its cause. So the cache is named as a
bare
file://argument, which is gone when the command is. (-c lfs.storageis doubly out: #163 records it as measured to break against local-path remotes
outright, since
GIT_CONFIG_PARAMETERSis inherited by the remote-side git-lfschild.)
That property is worth its own two tests because it is otherwise invisible:
mutation M7 below — add a
cacheremote and pull from it — leaves every disk andnetwork assertion passing and only that guard goes red.
Tests
New integration file
test/integration/test_lfs_object_sharing.py, real git andreal git-lfs, offline, over a local fixture remote with a real LFS payload. Its
fixture scopes
GIT_CONFIG_GLOBALand runsgit lfs install --skip-repo, becausegit lfs pullfetches objects but silently skips the checkout with nofilter.lfsconfig — on a machine where nobody rangit lfs install, theassertions would fail for a reason unrelated to the cache.
is
rmtree'd between the two launches, which is harder than an unreachable URL:nothing can be silently re-fetched, so what the second workspace holds provably
came from the cache. Asserts real content and
(st_dev, st_ino)identity —either alone is passable by accident.
materializing from origin and leaving the cache empty would still give that
workspace real content and leave every later one downloading again.
lfs.storage, and exactly one remote, still the forge (URL checked,not just the name).
Unit, at the
subprocess.runargv seam intest/test_workspace_clone.py:the ref is named, both knobs present.
a cache pull issued after the origin pull costs the download it exists to avoid
and every disk assertion still holds.
stated as a command that does not run. Decided by surviving pointers rather than
exit codes, since git-lfs exits zero having fetched only some objects.
at all still says so — the two halves of the unchanged contract.
cmd[:2] == ["git","lfs"]check is blind to the newgit -c … -c … lfs fetchform, so it now looks for the word anywhere in the argv.
Mutation checks — every one caught
fetchrecentknobs droppedorigininstead of the cachelfs.storagepersisted into the clonelfs.storageguard (+ both sharing tests)cacheremote added to the cloneThe whole cache phase removed is the red this started from: the two sharing tests
failed, the two host-specific guards passed.
Also in here
git-lfs >=3.4,<4added to the pixitestfeature. It was absent, so thesetests would have skipped — the failure mode that looks like a pass.
the
clone_managerfixture (duplicated attest_clone_object_sharing.py≡test_repo_manager_real.py, and about to become a third copy) is folded intotest/fixtures/git_fixtures.py; andtest_clone_object_sharing.pynow compares(st_dev, st_ino)rather thanst_inoalone, thedisk_usage.pyidiom.How much disk a workspace costs, and thehost-preprow of the timingtable) and CHANGELOG under
## [Unreleased].Declared deviation
pixi.lockmoved from format v6 to v7 — 440/432 lines. That is not adependency change, it is what the pinned
pixi(0.76.1) writes; it had alreadybeen warning
run pixi lock to upgrade to v7on every task in this repo. Verifiedmechanically: the sorted set of every
conda:/pypi:package URL in the lock isidentical before and after apart from the single added
git-lfs-3.7.1-hd3da7ce_3.condaline.setup-pixi@v0.9.6installs current pixi,which reads v7.
CI:
pixi run ciexits 0 — pylint 10.00/10, 1624 passed.🤖 Generated with Claude Code
Summary by Sourcery
Make the bare cache the shared git‑LFS object store for each repo so workspaces hardlink LFS objects from it instead of downloading and storing private copies, while preserving the existing origin‑based fallback behaviour.
New Features:
Enhancements:
Build:
Tests:
Chores: