-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Confusing error message when using GitHub's SSH URL #13549
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
Comments
I remember us having a PR that did some special logic for github, improving error messages I think. Unfortunately, searching for "github" on github doesn't return a lot of quality results. |
I tried searching as well before opening the issue, but nothing came up as duplicate/highly-related. |
I'm a little confused, as I'm uncertain why this would be specific to GitHub? The (If we bother to add a parser for it, we should probably just support the shorthand à la #1851.) |
Oh, btw, we added ssh shorthand support for submodules in #7238. |
It's not specific to GitHub, but I'm assuming that's the reason most people run into this issue. |
I'm also not an expert in SSH formats 😅 |
I'd like to see some improvements on it, specifically the first one Though I don't really have time thinking hard. |
From #1851 (comment):
|
### What does this PR try to resolve? It's very common for users to attempt to use the pseudo-URLs that GitHub or other providers provide in the form `[email protected]:rust-lang/rust.git` as a source in Cargo.toml, which are the default format accepted by OpenSSH. Unfortunately, these are not actually URLs, and unsurprisingly, the `url` crate doesn't accept them. However, our error message is unhelpful and looks like this: invalid url `[email protected]:rust-lang/rust.git`: relative URL without a base This is technically true, but we can do better. The user actually wants to write a real SSH URL, so if the non-URL starts with `git@`, let's rewrite it into a real URL for them to help them and include that in the error message. `git@` is the prefix used by all major forges, as well as the default configuration for do-it-yourself implementations like Gitolite. While other options are possible, they are much less common, and this is an extremely easy and cheap heuristic that does not necessitate complicated parsing, but which we can change in the future should that be necessary. This also avoids the problem where users try to turn the pseudo-URL into a real URL by just prepending `ssh://`, which causes an error message about the invalid port number due to the colon which they have not changed. Since they can just copy and paste the proposed answer, they'll be less likely to attempt this invalid approach. Fixes #13549 ### How should we test and review this PR? 1. `cargo init pkg1` 2. `cd pkg1` 3. Edit `Cargo.toml` to add the dependency line `bytes = { git = "[email protected]:tokio-rs/bytes.git", tag = "v1.10.0" }`. 4. Run `cargo build` 5. Notice that the error suggests a URL to try. 6. Replace the Git URL with the suggested URL. 7. Run `cargo build`. 8. Notice that the package compiles cleanly. ### Additional information N/A
Problem
It is fairly common for a Rust user to assume that the URL you get from GitHub to clone a repository (e.g.
[email protected]:rust-lang/cargo.git
for Cargo) should be a valid value for thegit
field in a dependency or when doingcargo install --git <URL>
.It isn't, and that's OK.
But we also don't nudge the user in the right direction.
The first error they get is:
If they figure out they need to add
ssh://
in front, the error becomes:because the org name is interpreted as a port number (which isn't obvious from the error message, since it doesn't say what is being picked up as port number).
Steps
No response
Possible Solution(s)
I'd love to see better diagnostics from
cargo
on this.If the format of the URL matches what GitHub provides, we should be able to suggest the correct value (i.e. from
[email protected]:{org_name}/{repo_name}.git
tossh://[email protected]/{org_name}/{repo_name}.git
).Notes
No response
Version
No response
The text was updated successfully, but these errors were encountered: