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

Warn if using hash in git URL, Fixes #8241 #8297

Merged
merged 4 commits into from
Jun 1, 2020

Conversation

mjarkk
Copy link
Contributor

@mjarkk mjarkk commented May 30, 2020

This fixes an issue where if the user wants to set the git rev but doesn't know how and as results tries to set the ref in the url hash as also shown when downloading the dependency.
Now cargo returns a warning notifying the user about the correct way to set the ref.

Fixes #8241

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2020
@alexcrichton
Copy link
Member

Thanks! Could a test be added for this as well?

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor

ehuss commented Jun 1, 2020

Oh, and another thought, it might be clearer to specifically mention the URL fragment, and also say where the rev should go. Maybe something like this:

format!("URL fragment `#{}` in git URL is ignored for dependency ({}). \
    If you were trying to specify a specific git revision, \
    use `rev = \"{}\"` in the dependency declaration.", fragment, name_in_toml, fragment);

mjarkk and others added 3 commits June 1, 2020 19:34
@mjarkk mjarkk requested a review from ehuss June 1, 2020 17:50
@mjarkk
Copy link
Contributor Author

mjarkk commented Jun 1, 2020

@alexcrichton I've added a test

@ehuss
Copy link
Contributor

ehuss commented Jun 1, 2020

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 1, 2020

📌 Commit 91f6617 has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 1, 2020
@bors
Copy link
Contributor

bors commented Jun 1, 2020

⌛ Testing commit 91f6617 with merge 40ebd52...

@bors
Copy link
Contributor

bors commented Jun 1, 2020

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 40ebd52 to master...

@bors bors merged commit 40ebd52 into rust-lang:master Jun 1, 2020
@mjarkk mjarkk deleted the warn-when-using-hash-in-git-url branch June 2, 2020 09:46
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2020
Update cargo

9 commits in 9fcb8c1d20c17f51054f7aa4e08ff28d381fe096..40ebd52206e25c7a576ee42c137cc06a745a167a
2020-05-25 16:25:36 +0000 to 2020-06-01 22:35:00 +0000
- Warn if using hash in git URL, Fixes rust-lang/cargo#8241 (rust-lang/cargo#8297)
- reset lockfile information between resolutions (rust-lang/cargo#8274)
- Disable strip_works test on macos. (rust-lang/cargo#8301)
- Fix typo in impl Display for Strip (rust-lang/cargo#8299)
- Add support for rustdoc root URL mappings. (rust-lang/cargo#8287)
- Fix tests with enoent error message on non-english systems. (rust-lang/cargo#8295)
- Fix fingerprinting for lld on Windows with dylib. (rust-lang/cargo#8290)
- Fix a typo (rust-lang/cargo#8289)
- Fix several issues with close_output test. (rust-lang/cargo#8286)
@rofrol
Copy link

rofrol commented Aug 29, 2020

Added this to Cargo.toml:

typed-html = { git = "https://github.com/bodil/typed-html", rev = "4c13ecca" }
typed-html-macros = { git = "https://github.com/bodil/typed-html", rev = "4c13ecca" }

Now I got urls like ?rev=4c13ecca#4c13ecca:

   Compiling typed-html-macros v0.2.2 (https://github.com/bodil/typed-html?rev=4c13ecca#4c13ecca)
   Compiling typed-html v0.2.2 (https://github.com/bodil/typed-html?rev=4c13ecca#4c13ecca)

@ehuss ehuss added this to the 1.46.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If rev is added to git url cargo gives no warnings and has unexpected behavior
6 participants