Skip to content

handle git+ssh with semver#6239

Merged
zkochan merged 2 commits into
pnpm:mainfrom
andreineculau-forks:main
Mar 20, 2023
Merged

handle git+ssh with semver#6239
zkochan merged 2 commits into
pnpm:mainfrom
andreineculau-forks:main

Conversation

@andreineculau

@andreineculau andreineculau commented Mar 18, 2023

Copy link
Copy Markdown
Contributor

5 years later I'm still pestering you with git+ssh+semver @zkochan :)

Currently pnpm fails to install git+ssh://git@example.com:org/repo.git#semver:~1.2.3 (where example.com is an internal git server, not github.com) because it will end up looking for refspec /~1.2.3 which doesn't match any git tag. We end up there both because of escaping the colon in semver, but also because we assume git+ssh URL cannot accept semver range spec.

Trying to install https://example.com/org/repo.git#semver:~1.2.3 works as expected.

Looking through the code, we had quite a few patches to handle SCP-like urls, because we wanted to use new URL, and not the deprecated url.parse.

Yarn has this patch https://github.com/yarnpkg/yarn/blob/master/src/util/git.js#L103
but npm https://github.com/npm/git/blob/f60bb15da404679160c4bb6ad1c31a66d5f6ae72/lib/clone.js#L20 was mentioning the shim in https://github.com/npm/hosted-git-info/blob/f03bfbd3022c8f6283a991ff879ed97704ac35fa/lib/parse-url.js#L42

The shim didn't work out, so a similar take as yarn's landed in this PR.

@andreineculau
andreineculau requested a review from zkochan as a code owner March 18, 2023 19:36
@welcome

welcome Bot commented Mar 18, 2023

Copy link
Copy Markdown

💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

Comment thread resolving/git-resolver/src/parsePref.ts Outdated
@andreineculau

andreineculau commented Mar 18, 2023

Copy link
Copy Markdown
Contributor Author

offtopic: it is great to see pnpm can install git dependencies without a package.json 🎉 🚀 pnpm is a generic package manager 😱 Is that true? If so, I L O V E I T 😍 🤩

LATER EDIT: not quite apparently
pnpm package name defaults to repo.git#semver:~1.2.3 and version to 0.0.0, instead of repo and 1.2.4 (hypothetically if v1.2.4 tag exists and matches the semver range)

@zkochan

zkochan commented Mar 18, 2023

Copy link
Copy Markdown
Member

Could you add some tests?

@andreineculau

Copy link
Copy Markdown
Contributor Author

I added two tests (SCP-like and regular URL) for semver range. Both are failing on the current version highlighting the two bugs:

  • test with SCP-like URL - fails because it doesn't look for ranges
  • test with regular URL - fails because it tries to match /~x.y.z with semver tags

@zkochan

zkochan commented Mar 20, 2023

Copy link
Copy Markdown
Member

ok, also add a changeset for your changes.

pnpm -w changeset

@zkochan
zkochan merged commit 2879637 into pnpm:main Mar 20, 2023
@welcome

welcome Bot commented Mar 20, 2023

Copy link
Copy Markdown

Congrats on merging your first pull request! 🎉🎉🎉

zkochan pushed a commit that referenced this pull request Mar 20, 2023
@zkochan zkochan added this to the v7.30 milestone Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants