Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Reproduced with a local dumb-HTTP git server. On the released build, PR: #36689 |
|
Updated 7:12 PM PT - Aug 12th, 2026
⏳ @robobun, your commit 7949481 is still building in |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/install/PackageManager/PackageManagerEnqueue.rs:1182-1195— The newshould_updatepredicate in theTag::Gitarm gates only onis_root_dependency(id), dropping the|| dependency.version.tag == Tag::Catalogdisjunct that the npm sibling it's modeled on carries (lines ~2002-2011, comment: "catalogs are root-scoped"). A catalog entry that resolves to agit+...spec reaches this arm withdependency.version.tag == Catalog(see the catalog resolution at ~728-743), so when it's consumed by a workspace member rather than the root,is_root_dependencyis false andbun updatestill short-circuits to the stale lockfile SHA — the same bug this PR fixes, left unfixed for the catalog-via-member sibling. One-line fix: mirror the|| Tag::Catalogdisjunct from the npm path.Extended reasoning...
What the bug is
The PR introduces a
should_updatepredicate in theTag::Gitarm ofenqueue_dependency_with_main_and_success_fn(PackageManagerEnqueue.rs:1182-1195) so thatbun updateskips theget_package_idshort-circuit and re-resolves the committish against the remote. This predicate is copied from the pre-existing npm path at lines ~2002-2011, but drops one disjunct. The npm path reads:this.to_update // Update direct deps of the current workspace; catalogs are root-scoped. && (dependency.version.tag == dependency::version::Tag::Catalog || unsafe { &*(*this_ptr).lockfile }.is_root_dependency(unsafe { &mut *this_ptr }, dependency_id)) && (this.update_requests.is_empty() || ...)
whereas the new git path reads:
this.to_update && dep.resolved.is_empty() && { unsafe { &*(*this_ptr).lockfile }.is_root_dependency(unsafe { &mut *this_ptr }, id) && (this.update_requests.is_empty() || ...) }
The
|| Tag::Catalogclause is missing.Why the git arm is reachable with a Catalog tag
At lines ~728-743, when
dependency.version.tag == Tag::Catalog, the catalog entry is looked up and its resolved version is assigned to the localversionbinding (break 'version v). The subsequentmatch version.tagat line ~753 then dispatches on the catalog entry's tag — so a catalog entry whose value isgit+https://...lands in theTag::Gitarm at line 1171. Crucially,dependencyitself is untouched:dependency.version.tagis stillCatalog. This is exactly why the npm sibling checksdependency.version.tag == Tag::Catalogexplicitly, with the comment "catalogs are root-scoped".Why
is_root_dependencyalone is insufficientis_root_dependency(lockfile.rs:854-864) checks whether the dependency id is in the current root package's dependency list (viamanager.workspace_name_hash/root_package_id). Catalogs are declared in the workspace root'spackage.jsonbut consumed by workspace members: when memberpackages/foodeclares"lib": "catalog:", the resulting dependency id belongs topackages/foo's dependency list, not the root's. Sois_root_dependency(id)returnsfalsefor that dep whenbun updateis run from the workspace root.Step-by-step proof
- Workspace root
package.jsondefines"workspaces": { "catalog": { "lib": "git+http://.../lib.git" } }and a memberpackages/foo. packages/foo/package.jsondeclares"lib": "catalog:". The root does not depend onlibdirectly.bun installresolveslibto commit A and pins it in the lockfile.- Upstream pushes commit B.
- User runs
bun updatefrom the workspace root.this.to_updateis true. - Enqueue reaches
packages/foo'slibdependency:dependency.version.tag == Catalog, so lines 728-743 assign the catalog's git version toversion;match version.tagentersTag::Gitat 1171. dep.resolved.is_empty()is true (the spec has no#sha).is_root_dependency(id)checks the root package's dep list —lib's id is inpackages/foo's list, not the root's — returnsfalse.should_updateis thereforefalse;get_package_idmatches the lockfile-loaded package via committish (emptyresolvedfallback inRepository::eql) and short-circuits to commit A's stale SHA. The clone/fetch path never runs.
Result:
bun updatefrom the root does not move a catalog-defined git dependency consumed only by a workspace member — the exact bug class #13769 reports and this PR fixes for direct root deps.Impact and severity
This is an incomplete fix, not a regression: before this PR the git arm had no
should_updategate at all, so catalog git deps didn't update either. Merging leaves that narrow scenario (catalog + git spec + consumed only by a non-root workspace member) exactly where it was, while the primary reported case is fixed. REVIEW.md ("Fix the whole class in the same PR ... parallel switch arms") asks that sibling arms share the same guard shape, and the fix is a one-token addition mirroring the sibling the author copied from — so it's worth flagging, but not worth blocking merge.Fix
Mirror the npm sibling:
let should_update = this.to_update && dep.resolved.is_empty() && { let this_ptr: *mut PackageManager = this; (dependency.version.tag == dependency::version::Tag::Catalog || unsafe { &*(*this_ptr).lockfile }.is_root_dependency(unsafe { &mut *this_ptr }, id)) && (this.update_requests.is_empty() || this.updating_packages.contains( dependency.name.slice(this.lockfile.buffers.string_bytes.as_slice()), )) };
- Workspace root
|
Good catch on the catalog case. Added the On the related issues: the refspec half of this change covers #11548 and #18947 (stale refs in the bare cache). #8590 is the |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/install/PackageManager/PackageManagerEnqueue.rs:1175-1186— TheTag::Catalogdisjunct added here in cfa0ae8 makesshould_updatepermanently true for acatalog:dep whose catalog entry isgit+..., so the post-checkout re-enqueue never reachesget_package_idandresolutions[id]is left atinvalid_package_id. The PR description's termination invariant ("After the checkout writesresolvedback into the dependency... the short-circuit applies again") does not hold for Catalog: runTasks.rs:1509-1518 writes intodependencies[id].version.value.git, but pass 3 re-readsversionfromlockfile.catalogs(line 728-742), which was never updated — sodep.resolved.is_empty()stays true across all three passes. Fix the write-back to branch ondependencies[id].version.tagand, forCatalog, writeresolvedinto the catalog store (or setresolutions[id]directly there) so pass 3 can terminate.Extended reasoning...
What the bug is
For a workspace member consuming a
git+...catalog entry via"lib": "catalog:", runningbun updatefrom the root now leaves that dependency's resolution slot atinvalid_package_idwith an orphan callback intask_queue. Before this PR the same dep resolved (stalely) to the lockfile pin; after this PR it does not resolve at all.Code path
The git resolve loop takes three passes through
enqueue_dependencyfor the sameid: (1) enqueue clone, (2) after clone →find_commit→ enqueue checkout, (3) after checkout → must terminate by bindingresolutions[id].Termination relies on the write-back at runTasks.rs:1509-1518:
let repo = unsafe { &mut *manager.lockfile.buffers.dependencies[id].version.value.git }; repo.resolved = pkg.resolution.git().resolved;
For a plain
Tag::Gitdep, pass 3 clonesdependencies[id].versionat line 749, sodep.resolvedis now the SHA →dep.resolved.is_empty()at line 1177 is false →should_update = false→get_package_idmatches the freshly-appended package →success_fnsetsresolutions[id]. ✅For a
Tag::Catalogdep this write-back is invisible on two counts:dependencies[id].version.tag == Catalog, so the active union arm is.catalog(aSemverString), not.git; the write lands in bytes that are never read back as.catalog.- More importantly, on re-enqueue line 728-742 re-derives
versionfromthis.lockfile.catalogs.get(...)— the catalog store, notdependencies[id]. Nothing wroteresolvedinto the catalog store, soversion.git().resolvedis still empty.
Why the guard never flips
On pass 3 with a Catalog→git dep and bare
bun update:this.to_update == truedep.resolved.is_empty() == true(catalog store was never touched)is_direct_update_target(id, dependency): thedep.version.tag == Catalogdisjunct at line 2760 short-circuits totrue;update_requests.is_empty()istrue→ returns true
So
should_update == trueagain,get_package_idis skipped, control falls through to thegit_repositorieshit →find_commitreturns the same SHA → samecheckout_id.needs_ctx = (resolutions[id] == invalid_package_id) == true→ ctx is re-pushed totask_queue[checkout_id](which wasmem::take'd at runTasks.rs:1492).has_created_network_task(checkout_id)finds the existing dedupe entry → returnstrue→return Ok(()).resolutions[id]is never assigned;success_fnnever fires; an orphan callback sits intask_queue[checkout_id]with no task to drain it.process_extracted_tarball_packageappends the package but does not setresolutions[dep_id]— that is delegated to the re-enqueue'ssuccess_fn, which is unreachable here.Step-by-step proof
- Root
package.json:{"workspaces": {"packages": ["pkg/*"], "catalog": {"lib": "git+http://…/lib.git"}}};pkg/app/package.json:{"dependencies": {"lib": "catalog:"}}. bun install→ lockfile pins commit A.bun updatefrom root → for thelibdep inpkg/app,dependency.version.tag == Catalog,to_update == true,update_requestsempty.- Pass 1: line 728 resolves
versionfrom catalog store (Tag::Git,resolvedempty).should_update = true→ skipget_package_id→git_repositoriesempty → push ctx totask_queue[clone_id], enqueue clone. - Pass 2 (after clone): same
version,should_update = true→ skipget_package_id→ repo present →find_commit→ push ctx totask_queue[checkout_id], enqueue checkout. - GitCheckout completes: package appended; runTasks.rs:1509-1518 writes
resolvedintodependencies[id].version.value.git(wrong arm);process_dependency_list_itemre-enqueues. - Pass 3: line 728 fires again,
versionre-read fromlockfile.catalogs—resolvedstill empty.should_update = true→ skipget_package_id→ repo present → samecheckout_id→ ctx re-pushed →has_created_network_tasktrue →return Ok(()).resolutions[id]never assigned.
Why this is a regression from this PR
Before this PR the
Tag::Gitarm ranget_package_idunconditionally. On pass 1 it matched the stale lockfile entry viaRepository::eql's committish fallback and bound the slot — the "silently stale" bug this PR fixes. The Catalog disjunct inis_direct_update_target(added in cfa0ae8 specifically to handle this case per the earlier review note) now sends catalog→git down the clone/checkout path but removes its only termination mechanism. The scenario goes from "resolves to stale commit" to "resolution slot left invalid + orphan task-queue entry".Fix
The write-back at runTasks.rs:1509-1518 already assumes the active union arm is
.git, which is false for Catalog deps. Branch ondependencies[id].version.tagthere: forCatalog, writeresolved/package_nameintolockfile.catalogs.get_mut(...).version.value.gitinstead (or in addition), so line 728-742's read on pass 3 sees a non-emptyresolvedandshould_updateflips to false. Alternatively, setresolutions[id] = package_iddirectly in the GitCheckout handler before re-enqueue and gate theneeds_ctxpush on it — but the write-back fix keeps the termination story symmetric with plainTag::Git.
|
On the catalog termination concern: the |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/regression/issue/13769.test.ts:178-201— This test never reachesRepository::download(): afterbun add, node_modules already matches the lockfile pin, sobun installshort-circuits (installer.verify()→ true,needs_install= false) and no GitClone task is scheduled — both assertions are reading state written bybun add. To make it a real guard,rm -rf node_modulesand the@G@<sha>checkout-cache entry betweenpublishCommitandbun installso the warm-cache fetch runs (movingrefs/heads/mainto B locally) and the checkout must still land on the lockfile's SHA A. Nit — tests 1 and 2 already fail-before/pass-after on the refspec fix; this is missing coverage, not incorrect behaviour.Extended reasoning...
What the test claims vs. what it does
Test 4's stated purpose (per the PR description and the inline comment "
installmust honour the lockfile pin, not float to HEAD") is to prove that the new explicit-refspec fetch does not causebun installto float off the lockfile's pinned SHA when the remote has moved. But the code path this PR changes —Repository::download()in repository.rs:722-745 — is never reached by this test.The trace
bun add git+http://…/repo.gitpopulatesnode_modules/issue13769libat COMMIT_A, writesbun.lockpinning SHA_A, writes.bun-tag= SHA_A into the installed dir, and warms both the bare-clone cache and the per-SHA@G@<sha>checkout cache.publishCommit(..., "COMMIT_B")mutates only the served bare repo. Nothing underapp/orcache/changes.bun installloads the lockfile;package.jsonis unchanged sincebun add, soDiff::generatemaps every dep andsummary.add/updatestay 0 —enqueue_dependency_with_mainis not called for the git dep, and noGitClonetask is scheduled from the resolve phase.- In the link phase,
PackageInstaller.rs:1591computesneeds_install = !installer.verify(...).verify()for git deps (PackageInstall.rs:833-836→verify_git_resolution) readsnode_modules/issue13769lib/.bun-tagand compares it to the lockfile's resolved SHA — both are SHA_A → returnstrue. Soneeds_install = false, theif needs_installblock at line 1598 (which contains the onlyenqueue_git_for_checkoutcall) is skipped, andsummary.skippedis incremented.
Net:
Repository::download()— the only function this PR changes — never runs.bun installdoes zero work.Why the assertions are vacuous
Both
assertInstalled(app, "COMMIT_A")andlockAfter === lockBeforeare asserting on bytes thatbun addwrote andbun installnever touched. If the refspec change floated the pin to HEAD, panicked, or was reverted entirely, test 4 would still pass — the changed code is unreachable from it. This is exactly what REVIEW.md's "Prove the test fails for the RIGHT reason" section warns against: "check that … fast paths can't satisfy the test without running your code" and "confirm deleting each load-bearing clause of your fix breaks at least one test — a test that passes both ways is worse than no test".How to make it a real regression guard
Force the changed code path to run and then choose between the fetched HEAD and the lockfile pin:
await publishCommit(env, upstream, serve, "COMMIT_B"); // Force re-checkout from the (now-refreshed) bare cache using the lockfile SHA. await Bun.$`rm -rf ${join(app, "node_modules")}`; for await (const d of new Bun.Glob("@G@*").scan({ cwd: cache, onlyFiles: false })) { await Bun.$`rm -rf ${join(cache, d)}`; } const lockBefore = await Bun.file(join(app, "bun.lock")).text(); const { stderr, exitCode } = await runBun(app, cache, env, "install", "--no-progress");
With node_modules gone and the per-SHA checkout cache cleared (but the bare clone left in place),
needs_installandpackage_missing_from_cacheare both true,enqueue_git_for_checkoutschedules aGitClone(fresh process ⇒git_repositoriesempty),download()takes the cache-hit branch and runs the new refspec fetch — movingrefs/heads/mainin the local bare clone to B — and the subsequent checkout must still land on SHA_A because the lockfile'sresolved(notfind_commit) drives it. Now the assertions can actually fail if the fix regresses.Severity
Nit. The refspec fix itself is correct and adequately covered by tests 1 and 2, which do exercise
download()and fail-before/pass-after. Test 4 as written just doesn't provide the additional "install still honours the pin" guard it claims to; merging as-is causes no concrete failure.
|
Strengthened the install-pin guard in cb62ca8: it now drops |
There was a problem hiding this comment.
LGTM — the runtime change is now just three extra args to the warm-cache git fetch (origin + the two refspecs), which is the minimal correct fix for a bare clone with no configured fetch refspec.
What was reviewed:
- Verified
clone --baresetsremote.origin.urlbut nofetch =refspec, sooriginresolves and the pre-PR default-remote behavior is unchanged; only the refspecs are new. - Confirmed the fifth test's tag-only push (serve's
refs/heads/mainstays at commit A) means tag-auto-follow on the heads refspec cannot seev2, so deleting the tags refspec breaks it — both load-bearing clauses are now covered. - Tests are hermetic (local dumb-HTTP
Bun.serve, per-testtempDir/cache,GIT_CONFIG_NOSYSTEM), usetest.concurrent, and release resources viausing/await using. - All four earlier review threads (enqueue guard, catalog disjunct,
url→origin, tag coverage) are addressed in the final diff.
Extended reasoning...
Overview
After several review iterations the PR reduced to a single-site runtime change in src/install/repository.rs: the warm-cache git fetch --quiet now passes origin +refs/heads/*:refs/heads/* +refs/tags/*:refs/tags/*. The earlier PackageManagerEnqueue.rs guard was dropped entirely once tracing showed get_package_id already misses on the update path (git deps' realname() is empty pre-checkout), so the stale SHA came solely from find_commit reading un-refreshed refs in the bare cache. I locally confirmed git clone --bare writes remote.origin.url but no remote.origin.fetch, so a refspec-less fetch only touches FETCH_HEAD — the mechanism in the PR description is accurate. Using origin rather than the passed url keeps the pre-PR remote-selection behavior (the argless fetch already defaulted to origin), so no new https-then-ssh retry noise on SSH-only hosts.
The new test/regression/issue/13769.test.ts adds five test.concurrent cases over a local dumb-HTTP git server: no-committish bun update, named bun update <pkg> on #branch, no-op update leaves lockfile byte-identical, bun install honours the lockfile pin after dropping node_modules and the @G@<sha> checkout cache, and a tag-only push case for #11548/#18947 where serve's branch heads stay at commit A so only the explicit tags refspec can surface v2.
Security risks
None. The added git args are static byte literals; no user input flows into argv here. is_safe_resolved_tag and the --end-of-options guard in find_commit are unchanged. The + force prefix is appropriate for a cache mirror and does not touch any user working tree.
Level of scrutiny
Low-to-moderate. The package manager is production-critical, but the change is three constant argv entries on an existing git invocation whose remote-selection semantics are unchanged from before the PR. The failure mode of getting this wrong is "fetch fails and we log an error" (same error path as today), not silent corruption.
Other factors
The bug-hunting system found nothing this run. All four of my prior inline threads are resolved in the final diff — in particular the tag-coverage gap was closed with a test the author verified fails when the tags refspec is deleted. Tests follow harness conventions: tempDir, bunEnv spread, port: 0, using/await using, stderr asserted before exit code, no external network, isolated GIT_CONFIG_GLOBAL/GIT_CONFIG_NOSYSTEM/BUN_INSTALL_CACHE_DIR per test. The PR body's evidence block shows fail-before/pass-after on both ASAN-debug and release.
There was a problem hiding this comment.
LGTM — the final diff is the minimal fix (three static argv entries on the warm-cache git fetch) and all four rounds of feedback have been addressed.
What was reviewed:
origin+ explicit heads/tags refspecs —clone --barealways writesremote.origin.url, so the remote exists; static literals, no user input reaches argv.- Fifth test's tag-only push leaves served
refs/heads/mainat A, so tag auto-follow can't satisfy it — the+refs/tags/*line is now load-bearing. - Confirmed this is the only
git fetchsite insrc/install/; no sibling to update. - Tests are hermetic (
Bun.serve({port:0})dumb-HTTP git,tempDir, per-test cache dir), pipes drained concurrently, resources released viausing.
Extended reasoning...
Overview
The PR now touches two files: a 14-line change to src/install/repository.rs that adds origin plus +refs/heads/*:refs/heads/* and +refs/tags/*:refs/tags/* to the existing warm-cache git fetch --quiet, and a 234-line regression test at test/regression/issue/13769.test.ts with five concurrent cases. Earlier revisions also touched PackageManagerEnqueue.rs with a should_update guard; that was dropped in b4486ea after tracing showed get_package_id already misses on the update path (git deps' realname() is empty pre-checkout), leaving only the refspec fix.
Security risks
None. The three new argv entries are compile-time byte literals; no user-controlled data reaches them. The pre-existing url argument is no longer passed to the warm-cache fetch (it now uses the fixed string origin), which is strictly less surface than before. find_commit already guards the committish with --end-of-options, and checkout already gates resolved on is_safe_resolved_tag — neither path is touched here.
Level of scrutiny
Medium. src/install/ is a load-bearing subsystem, but the actual code change is a well-understood git-CLI argv adjustment with no new control flow, no memory or lifetime changes, and no platform-gated code. The root cause (clone --bare writes remote.origin.url but no remote.origin.fetch, so a bare git fetch only updates FETCH_HEAD) is documented git behaviour, and the fix is the canonical remedy. I grepped src/install/ for other b"fetch" sites — this is the only one.
Other factors
This PR has been through four review rounds from me, each addressed:
updating_packagesnever holds git deps → the enqueue guard was dropped entirely once tracing showed it was a no-op.- Passing
urlto the warm-cache fetch would re-try thetry_httpsrewrite on SSH-only hosts and dump git stderr on every update → switched toorigin(75bab7e). - The install-pin guard test wasn't forcing the fetch path → strengthened to drop
node_modulesand the@G@<sha>checkout cache while keeping the bare clone (cb62ca8). - No test covered the
+refs/tags/*refspec → added a fifth case (de70167) with a tag-only push so heads auto-follow can't satisfy it; author verified delete-the-tags-line turns it red.
The tests follow harness conventions: tempDir, bunEnv/bunExe, port: 0, test.concurrent, await using for the server, Promise.all on stdout/stderr/exited, per-test isolated BUN_INSTALL_CACHE_DIR, and GIT_CONFIG_NOSYSTEM/GIT_CONFIG_GLOBAL pointing at an empty file so CI git config can't interfere. The gate evidence shows fail-before on the debug build. No CODEOWNERS entry covers src/install/. The bug-hunting system found nothing this run.
alii
left a comment
There was a problem hiding this comment.
Right fix, but as written the refresh can start failing on caches that work today.
- needs --prune, otherwise a branch or tag renamed into a nested name upstream makes every later warm-cache install of that repo fail until bun pm cache rm
- the fetch hardcodes origin but the clone does not pin the remote name, so clone.defaultRemoteName in ~/.gitconfig breaks it
- these bugs never worked, so the tests belong in test/cli/install/bun-install-git-deps.test.ts, not test/regression
This is the same change as #35566. Whichever of the two lands, the other should be closed, and the same three points apply to that one.
… remote
`bun update` would report "no changes" for a `git+...` dependency after
new commits were pushed upstream, leaving the lockfile pinned to the
original SHA.
Two independent problems combined here:
- `enqueue_dependency` looks up the dependency's spec in
`Lockfile::get_package_id` before scheduling a clone. The spec's
`resolved` is empty, so `Repository::eql` falls back to comparing the
committish and matches the package loaded from the lockfile. The clone
was never scheduled and the stale pin was reused.
- The cached bare clone is refreshed with `git fetch --quiet`, but
`git clone --bare` configures `remote.origin.url` without a fetch
refspec, so that fetch only updates `FETCH_HEAD` and `find_commit`
still reads the old refs.
For an update target whose `resolved` is not yet known, skip the
`get_package_id` short-circuit so the clone/fetch/find-commit path runs
and the committish is resolved against the remote. After the checkout
writes `resolved` back into the dependency, the short-circuit applies
again and binds the slot to the freshly appended package.
Pass explicit `+refs/heads/*` and `+refs/tags/*` refspecs (against the
dependency URL) to the cache refresh so `find_commit` sees the current
remote state, including caches populated by older builds. The
fetch-error log is now guarded on `attempt > 1` like the clone branch,
so the https-then-ssh fallback stays quiet when only the second
transport works.
The regression test serves a bare repo over git's dumb HTTP protocol
via `Bun.serve({port: 0})` and covers `bun update`, `bun update <name>`,
an unchanged HEAD (lockfile unchanged), and `bun install` (lockfile pin
preserved).
Fixes #13769
Replaces the raw-pointer split borrow with a disjoint field borrow in `PackageManager::is_direct_update_target`, and drops the explanatory paragraph at the call site.
Tracing showed `get_package_id` was already a miss on the update path: `realname()` for a git dependency returns `version.git().package_name`, which is empty until the checkout has run, so `name_hash` is 0 and the lockfile lookup cannot match the stale entry. The clone was always scheduled; the stale SHA came from `find_commit` reading the un-refreshed bare cache. All four regression tests pass with the repository.rs change alone.
`remote.origin.url` records the transport that the initial clone succeeded with, so fetching from `origin` avoids re-trying the `try_https` rewrite on the warm-cache path for SSH-only hosts. The error log is restored to its original unconditional form since the fetch no longer depends on which rewrite the caller passed.
Remove node_modules and the @g@<sha> checkout cache before `bun install` so the install has to run `Repository::download()` (refreshing the bare cache to commit B) and then check out the lockfile's pinned SHA.
The new tag is published without moving any branch head, so git's tag auto-follow on the heads refspec cannot see it and only the explicit `+refs/tags/*:refs/tags/*` makes `find_commit` resolve `#v2`. Fails with "no commit matching" on main and if the tags refspec is removed.
Review follow-ups for the bare-clone refresh: - `git fetch --prune`: a branch or tag renamed upstream into a nested name (`release` -> `release/1.0`) collides with the stale loose ref in the bare clone and, without pruning, fails every later refresh of that repository until `bun pm cache rm`. Lockfile-pinned installs take this path too whenever the pinned checkout is missing from the cache. - `git clone --bare -o origin`: the refresh fetches from `origin` by name, so the clone must not let `clone.defaultRemoteName` in the user's gitconfig pick something else. - The tests move from test/regression to test/cli/install/bun-install-git-deps.test.ts, reusing its helpers. None of these scenarios ever worked, so they are not regressions. The six cases: `bun update` on a HEAD-tracking dep (with `clone.defaultRemoteName` set in the global gitconfig), `bun update <name>` on a branch dep, an unchanged upstream leaving the lockfile byte-identical, a pinned `bun install` after upstream moved, an upstream branch renamed into a nested name, and a tag-only push of a new tag. Removing any one of `--prune`, `-o origin`, the heads refspec or the tags refspec fails at least one of them.
e3c60fa to
9014c04
Compare
alii
left a comment
There was a problem hiding this comment.
Checked the new head against the three points from the last round: the refresh now prunes, the clone pins the remote name, and the cases moved into bun-install-git-deps.test.ts and cover both. Ran the new fetch and clone command lines against local bare repos (nested branch and tag renames, moved tag, clone.defaultRemoteName, pinned checkout after prune, dumb http) and they behave as the PR describes. One non-blocking note inline; looks ready to merge from this side. Neither touched file has moved on main since the merge base.
…sh test Folds the cases from #36689 into git-cache-refresh.test.ts: bun update moves a committish-less dependency to the new HEAD and a #branch dependency to the branch tip, bun install keeps the lockfile pin after the cache has been refreshed past it, and bun update against an unchanged remote leaves the lockfile unchanged.
|
Consolidated into #35566, which makes the same |
Problem
bun updatereports "no changes" for agit+...dependency after new commits are pushed upstream, andbun pm cache rmis the only way to pick them up (Bun installs the first commit of a library that's not on NPM.JS #13769). Switching a dependency to a tag created after the repo was first cached fails withno commit matching "<tag>" found for "<pkg>" (but repository exists)(bun requires clearing cache when updating package that is using a git tag #11548, bun requires clearing cache when updating package that is using a git tag #18947).git clone --barewritesremote.origin.urlbut no fetch refspec, so the refresh inRepository::download(src/install/repository.rs, warm-cache branch) ran a baregit fetch --quietthat only updatesFETCH_HEAD.find_committhen resolved branches, tags andHEADagainst the refs as they were when the repo was first cached.Fix
git fetch --quiet --prune origin +refs/heads/*:refs/heads/* +refs/tags/*:refs/tags/*.find_commitreads;+follows force-pushes and moved tags.--pruneremoves refs deleted or renamed upstream. Without it a rename into a nested name (releasetorelease/1.0) collides with the stale loose ref and every later refresh of that repository fails, including lockfile-pinned installs whose checkout is missing from the cache.originby name (not the URL the caller passed) keeps using whichever transport the original clone succeeded with; the clone now passes-o originsoclone.defaultRemoteNamein the user's gitconfig cannot change the name.bun installwith a lockfile still checks out the pinned commit: the refresh only moves refs in the bare clone, and the pinned SHA is what gets checked out.test/cli/install/bun-install-git-deps.test.ts(these scenarios never worked, so they are not regressions):bun updateon a HEAD-tracking dep moves to the new upstream HEAD (withclone.defaultRemoteNameset in the global gitconfig)bun update <name>on a#branchdep moves to the new branch tipbun updatewith an unchanged upstream leavesbun.lockbyte-identicalbun installafter upstream moved (node_modules and the@G@<sha>checkout removed, bare clone kept) still installs the locked commit#<tag>bun updatecases and the tag case fail; the other three are guards and pass. Removing any one of--prune,-o origin, the heads refspec or the tags refspec from the fix fails at least one case.Background
<cache>/<hash>.git) and each resolved commit as a separate checkout directory (<cache>/<name>@G@<sha>). Resolving a dependency means refreshing the bare clone, runninggit log -1 <committish>in it to get a SHA, then checking that SHA out.+refs/heads/*:refs/heads/*) tellsgit fetchwhich remote refs to copy to which local refs.git clonenormally writes one into the repo config;git clone --baredoes not, sogit fetchwith no arguments in such a repo only records what it saw inFETCH_HEADand updates nothinggit logcan see.git fetch --prunedeletes local refs that no longer exist on the remote, in the namespaces covered by the refspecs. A ref namedreleaseand a ref namedrelease/1.0cannot coexist (one is a file, the other needs a directory of the same name), so a rename upstream needs the stale ref pruned before the new one can be created.Bun.serve({ port: 0 }), so no network is involved.Follow-up
Two warm-cache edge cases are left for a separate PR; both recover with
bun pm cache rm, and cold clones and lockfile-pinned installs are unaffected by either:HEADsymref is still the one written at clone time. If upstream renames its default branch,--pruneremoves the old head and a dependency with no committish fails withno commit matching ""(it previously resolved the stale tip silently). Fix: repointHEADafter the fetch (git ls-remote --symref origin HEAD, thengit symbolic-ref HEAD ...).clone.defaultRemoteNameset have no remote calledorigin, so the new refresh fails there (the old one worked because git falls back to the only configured remote).-o origincovers every clone made from now on; repairing old caches means discovering the remote name on failure, which belongs with theHEADrepoint above.Fixes #13769
Fixes #11548
Fixes #18947
Same underlying fix as #35566; whichever lands first, the other can be closed.
Earlier revisions
The first version of this PR also skipped the
get_package_idshort-circuit inenqueue_dependencyfor update targets. Tracing showed that lookup already misses for git dependencies (theirrealname()is empty until a checkout has run), so the stale result came entirely from the un-refreshed bare clone, and that change was dropped. The tests originally lived intest/regression/issue/13769.test.ts; review pointed out these paths never worked, so they moved to the existing git-deps test file.