Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle git+ssh with semver #6239

Merged
merged 2 commits into from Mar 20, 2023
Merged

handle git+ssh with semver #6239

merged 2 commits into from Mar 20, 2023

Conversation

andreineculau
Copy link
Contributor

@andreineculau andreineculau commented Mar 18, 2023

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.

@welcome
Copy link

welcome bot commented Mar 18, 2023

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

@andreineculau
Copy link
Contributor Author

andreineculau commented Mar 18, 2023

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
Copy link
Member

zkochan commented Mar 18, 2023

Could you add some tests?

@andreineculau
Copy link
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
Copy link
Member

zkochan commented Mar 20, 2023

ok, also add a changeset for your changes.

pnpm -w changeset

@zkochan zkochan merged commit 2879637 into pnpm:main Mar 20, 2023
7 checks passed
@welcome
Copy link

welcome bot commented Mar 20, 2023

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.

None yet

2 participants