fix: keep the git subdirectory when resolving a committish - #513
superdejooo wants to merge 2 commits into
Conversation
`addGitSha` rebuilt the spec from the repository url plus the resolved
sha, dropping the `::path:<dir>` selector that npm-package-arg had parsed
out of the committish.
Resolving a named ref therefore rewrote
<repo>#main::path:packages/plugin
into
<repo>#<sha>
so the resolved spec recorded for the install pointed at the repository
root. Installing it either failed with `Could not read package.json`, or,
when the root happened to have a manifest, silently installed the wrong
package. Specs pinned to a full sha were unaffected, because those skip
this rewrite, which is why the selector only broke for branches, tags and
semver ranges.
Re-attach the selector to the resolved spec. hosted-git-info keeps it as
part of the committish, so the hosted branch round trips as well.
There was a problem hiding this comment.
🟢 Approval recommended
The functional change is well-scoped and backed by targeted unit + E2E tests; remaining feedback is minor naming/comment clarity.
Pull request overview
This PR fixes git dependency resolution so that repository subdirectory selectors (::path:<dir>) are preserved when a named ref (branch/tag/semver range) is resolved to a SHA, preventing later reinstalls from incorrectly targeting the repository root.
Changes:
- Update
addGitShato re-attach::path:<dir>when rebuilding a resolved git spec. - Add unit tests ensuring subdirectory selectors survive committish → SHA rewriting and round-trip through
npm-package-arg. - Add an end-to-end
GitFetchertest verifying resolved specs keepgitSubdirand still fetch the intended subpackage.
File summaries
| File | Description |
|---|---|
| lib/util/add-git-sha.js | Preserve gitSubdir by appending ::path:<dir> when rewriting specs to a resolved SHA. |
| test/util/add-git-sha.js | Add coverage for subdirectory selectors across multiple URL/spec forms and round-trip parsing. |
| test/git.js | Add E2E regression test to ensure GitFetcher.resolve() retains gitSubdir and manifests come from the subdirectory. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // hosted specs rebuild the url from hosted-git-info instead of the raw spec | ||
| const hosted = `git://127.0.0.1:${gitPort}/subdir-ref-repo#test-branch::path:packages/subpkg` | ||
| const resolvedHosted = await new GitFetcher(hosted, opts).resolve() | ||
| t.match(resolvedHosted, /::path:packages\/subpkg$/, 'hosted resolved spec keeps the subdirectory') |
There was a problem hiding this comment.
127.0.0.1 is hosted here: the fixture registers it as localhosthttps with git: among its protocols, while localhost is not.
Existing line 625 already relies on that — it expects git+${remoteHosted}#${REPO_HEAD} for the same URL shape, and that git+ prefix is only added by repoUrl() in the hosted branch.
The `::path:` suffix strips a leading slash only when npm-package-arg added one. Every other case goes through npa, which always adds it, so that fallback stayed unreachable and left add-git-sha.js at 92.3% branch coverage, under the 100% threshold this repo enforces. Exercise it with a hand built spec, the same way the surrounding tests reach the non-hosted branch directly.
What / Why
addGitSharebuilds a git spec from the repository url plus the resolved sha. It builds that url withnoCommittish: trueand then appends only#<sha>, so the::path:<dir>selector thatnpm-package-argparsed out of the committish is dropped.Resolving a named ref rewrites
into
The resolved spec is what gets recorded and reinstalled from later, so it ends up pointing at the repository root instead of the selected subdirectory. Installing it either fails with
Could not read package.jsonwhen the root has no manifest, or silently installs the wrong package when it does.Specs pinned to a full sha are unaffected, because
GitFetchersetsresolvedfromrawSpecand never callsaddGitSha. That is why the selector only breaks for branches, tags and semver ranges.Reproduction against
main:Fix
Re-attach the selector to the resolved spec in both branches.
npm-package-argstoresgitSubdirwith a leading slash, which the spec syntax does not use, so it is stripped before re-serializing.The hosted branch round trips too:
hosted-git-infotreats<sha>::path:<dir>as the committish and preserves it throughshortcut()/https()/sshurl(), so#setResolvedWithSha'snpa(withSha).hosted→repoUrl(...)step keeps the selector.Tests
test/util/add-git-sha.jscovers the unknown-host, shorthand, https-with-auth, https-without-auth and ssh forms, plus a semver range paired with a subdirectory and a nested subdirectory path. It also asserts the result round trips back throughnpawith itsgitSubdirintact, and that specs without a subdirectory are unchanged.test/git.jsadds an end-to-end case against the local git daemon: a repository whose root manifest isroot-packageand whosepackages/subpkgmanifest issub-package, fetched by branch name. It asserts the resolved spec keeps the selector and that fetching that resolved spec still returnssub-packagerather than the root manifest — the behaviour that actually breaks today. The hosted url path is asserted as well.All new assertions fail on
mainand pass with the fix.npm run lintpasses.