Skip to content

Basic URL canonicalization. Fixes #84 #88

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 29, 2014
Merged

Basic URL canonicalization. Fixes #84 #88

merged 4 commits into from
Jun 29, 2014

Conversation

@brson
Copy link
Contributor

@brson brson commented Jun 28, 2014

...#84.

No tests because there's no preexisting network tests.

@brson
Account for trailing / in URLs when determining on-disk location. Fixes
541a060 
rust-lang#84.

No tests because there's no preexisting network tests.
@brson
Copy link
Contributor Author

brson commented Jun 28, 2014

This may not actually fix #84. There is another rev-parse error that occurs when packages don't agree on the capitalization of source URLs, so e.g. when some people use PistonDevelopers and others use `pistondevelopers'.

@brson
Copy link
Contributor Author

brson commented Jun 28, 2014

Think I'll throw in a few unit tests at least.

@brson
Copy link
Contributor Author

brson commented Jun 28, 2014

I've also stripped trailing .git extensions. This is ready.

@brson brson changed the title Account for trailing / in URLs when determining on-disk location. Fixes ... Basic URL canonicalization. Fixes #84 Jun 28, 2014
alexcrichton
alexcrichton reviewed Jun 29, 2014
View reviewed changes
src/cargo/sources/git/source.rs Outdated
Copy link
Member

@alexcrichton alexcrichton Jun 29, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case we change the hashing algorithm in the future, could this test just test equality of all the urls when hashed? (don't actually look at the specific hash).

Could you also add a test for case insensitivity? The one above just calculates one hash but it's not clear from the test that the hash was generated from a lowercase url.

Copy link
Contributor Author

@brson brson Jun 29, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will, but this is following the existing pattern.

@alexcrichton
Copy link
Member

alexcrichton commented Jun 29, 2014

r=me with a minor comment

brson added a commit that referenced this pull request Jun 29, 2014
@brson
Merge pull request #88 from brson/fix
bc31d2a 
Basic URL canonicalization. Fixes #84
@brson brson merged commit bc31d2a into rust-lang:master Jun 29, 2014
@brson brson mentioned this pull request Jun 29, 2014
Merged
@alexcrichton alexcrichton mentioned this pull request Jun 30, 2014
Closed
alexcrichton pushed a commit to alexcrichton/cargo that referenced this pull request Sep 2, 2014
@brson
Merge pull request rust-lang#88 from brson/fix
3254717 
Basic URL canonicalization. Fixes rust-lang#84
ehuss pushed a commit to ehuss/cargo that referenced this pull request Nov 19, 2023
@alexcrichton
Warn users when fixes break code
59b3617 
The rustfix tool detects it, so let's inform the user what just happened!

Closes rust-lang#88
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@brson @alexcrichton