-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix(git): respect scp-like URL for nested submodules #12359
Conversation
r? @ehuss (rustbot has picked a reviewer for you, use r? to override) |
@@ -3673,14 +3673,14 @@ fn different_user_relative_submodules() { | |||
"\ | |||
[UPDATING] git repository `{}` | |||
[UPDATING] git submodule `{}` | |||
[UPDATING] git submodule `{}` | |||
[UPDATING] git submodule `{}/../dep2` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regression of URL display is one downside of it. However, since we drop Url::parse
here so now we can have relative submodule inside a scp-like submodule. For example, this was not possible before this PR:
direct git dependency
submodule url `[email protected]/user/repo`
submodule url `../`
`parent_remote_url` used to be `&str` before rust-lang#12244. However, we changed the type to `Url` and it started failing to parse scp-like URLs since they are not compliant with WHATWG URL spec. In this commit, we change it back to `&str` and construct the URL manually. This should be safe since Cargo already checks if it is a relative URL for that if branch.
Looks good to me. Yea, there isn't a particularly good way to test scp-style URLs since they don't support port numbers. My only idea would be to put cargo itself inside a docker container, and orchestrate the test around using port 22. But that sounds sufficiently annoying that I wouldn't want to bother with it yet. A beta backport sounds good to me, too. @bors r+ |
☀️ Test successful - checks-actions |
Update cargo 11 commits in 694a579566a9a1482b20aff8a68f0e4edd99bd28..1b15556767f4b78a64e868eedf4073c423f02b93 2023-07-11 22:28:29 +0000 to 2023-07-18 14:44:47 +0000 - Fix "cargo doc --open" crash on WSL2 (rust-lang/cargo#12373) - fix(git): respect scp-like URL for nested submodules (rust-lang/cargo#12359) - Upgrade to indexmap v2 (rust-lang/cargo#12368) - refactor: Clean up package metadata (rust-lang/cargo#12352) - Correct unspecifiead to unspecified (rust-lang/cargo#12363) - Replace invalid `panic_unwind` std feature with `panic-unwind` (rust-lang/cargo#12364) - Bump to 0.74.0; update changelog (rust-lang/cargo#12361) - Bump version of crates-io due to unintentional semver-breaking change (rust-lang/cargo#12357) - chore: Automatically update dependencies monthly (rust-lang/cargo#12341) - docs: Use heading attributes to control the fragment. (rust-lang/cargo#12339) - Rustfmt with latest nightly. (rust-lang/cargo#12351) r? ghost
…nglo [beta-1.72] Update cargo 2 commits in 45782b6b8afd1da042d45c2daeec9c0744f72cc7..dd6536b8ed28f73c0e82089c72ef39a03bc634be 2023-07-05 16:54:51 +0000 to 2023-07-18 14:02:13 +0000 - [beta-1.72] backport rust-lang/cargo#12359 (rust-lang/cargo#12371) - [beta-1.72] Bump version of crates-io due to unintentional semver-breaking change (rust-lang/cargo#12356) r? ghost
What does this PR try to resolve?
parent_remote_url
used to be&str
before #12244. However, we changedthe type to
Url
and it started failing to parse scp-like URLs sincethey are not compliant with WHATWG URL spec.
In this commit, we change it back to
&str
and construct the URLmanually. This should be safe since Cargo already checks if it is a
relative URL for that if branch.
How should we test and review this PR?
This is current a draft because I had a hard time adding a test around it.
[email protected]:rust-lang/cargo.git
) doesn't accept custom port. In ourcontainer_test
we need to specify a custom port number.To test it, I ended up manually creating a repo with a submodule with scp-like URL.
Additional information
I feel like this worth a beta backport (we missed the nightly window).
I also feel like it is pretty safe to merge without adding a test if we cannot figure out how to.
Fixes #12295