fix(purl): encode gitmodules PURLs and drop pnpm's unreachable fallback - #1353
Merged
Conversation
`.gitmodules` built `pkg:github/{ns}/{name}` and the gitlab equivalent with
`format!`, so a repository path needing percent-encoding produced a PURL that did
not survive a round trip. Case is unaffected: the central normalizer already
lowercases namespace and name for github, gitlab and bitbucket at the output
boundary, so this is purely the encoding half.
pnpm's `create_purl` fell back to a hand-formatted string when `npm_purl`
returned `None`. That branch cannot be reached — the type is a literal, and the
crate's `with_namespace` rejects only namespace-prohibited types while
`with_version` has no error path at all — and it would have emitted an unencoded
PURL if it somehow were. Return the `Option` instead of inventing a value.
Leaves two sites that are deliberate and documented rather than hand-rolled:
CocoaPods appends a subspec literally so a conventional `+` in a subspec name is
not escaped, and Go splices its namespace to preserve path-part case, which
`normalize_purl` then lowercases host-only per the spec direction.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Maxim Stykow <maxim.stykow@gmail.com>
Greptile SummaryThe PR replaces hand-formatted PURLs with shared encoded constructors and removes pnpm’s unreachable unencoded fallback.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/parsers/gitmodules.rs | Replaces direct GitHub and GitLab PURL formatting with the shared namespaced constructor so components are encoded consistently. |
| src/parsers/pnpm_lock.rs | Propagates optional npm PURLs through dependency records and removes the hand-formatted fallback; the revised contract documentation resolves the prior review concern. |
| src/parsers/pnpm_lock_test.rs | Updates scoped and unscoped PURL tests to unwrap the now-explicit optional return value. |
Reviews (2): Last reviewed commit: "docs(pnpm): state create_purl's contract..." | Re-trigger Greptile
Greptile review: the doc comment narrated what the function used to do and why the old branch was unreachable, which is commit-message material and goes stale as soon as the helper changes. State the return contract only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Maxim Stykow <maxim.stykow@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.gitmodulesbuiltpkg:github/{ns}/{name}and the gitlab equivalent withformat!, so a repository path needing percent-encoding produced a PURL that did not survive a round trip.create_purlfell back to a hand-formatted string whennpm_purlreturnedNone. That branch cannot be reached — the type is a literal,with_namespacerejects only namespace-prohibited types, andwith_versionhas no error path at all — and it would have emitted an unencoded PURL if it somehow were. It now returns theOptionrather than inventing a value.This completes the sweep: no production site builds a PURL with
format!("pkg:…")any more.Scope and exclusions
Case is unaffected for gitmodules, which is what made this safe to do mechanically:
normalize_purlalready lowercases namespace and name forgithub,gitlabandbitbucketat the output boundary, so only the encoding half was missing.Two sites are deliberately left as they are, because both are documented decisions rather than hand-rolled strings — each already builds through the crate and then does one targeted, explained edit:
+in a subspec name (NSData+zlib) is not escaped to%2B.normalize_purlthen lowercases host-only, per the acknowledged spec direction (purl-spec#308).How to verify
The whole-sweep check is the useful one:
rg 'format!\("pkg:' src/ --glob '!*test*'should return nothing.Intentional differences from Python
Expected-output fixture changes
Optionthatcreate_purlalways returned in practice.