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

Protect `@` as safe character when cleaning URLs #6440

Merged
merged 1 commit into from Apr 25, 2019

Conversation

Projects
None yet
2 participants
@nicolasbock
Copy link
Contributor

commented Apr 24, 2019

Remote URLs that contain a link to a git repository and a tag
reference will have the @ character converted into %40. This is
incorrect.

Fixes: #6437

Signed-off-by: Nicolas Bock nicolasbock@gmail.com

@nicolasbock nicolasbock force-pushed the nicolasbock:index_with_git branch 2 times, most recently from ccdd1a3 to 2759ffb Apr 24, 2019

@nicolasbock

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

I am not sure about the added test. When I locally run tox without the safe argument, the test still passes. The @ character doesn't seem to be quoted as the OP found in #6437 .

@cjerdonek

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

It might be because you're missing a path portion. Try something more like-- "git+ssh://example.com/path.git@1.0#egg=my-package-1.0.201810251140"

@nicolasbock nicolasbock force-pushed the nicolasbock:index_with_git branch from 2759ffb to f3c33d5 Apr 25, 2019

@nicolasbock

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

I think you are right @cjerdonek . Please have a look at the updated change set.

@cjerdonek
Copy link
Member

left a comment

Thanks! Some comments.

Show resolved Hide resolved news/6440.bugfix Outdated
Show resolved Hide resolved src/pip/_internal/index.py
Show resolved Hide resolved tests/unit/test_index.py Outdated
Show resolved Hide resolved tests/unit/test_index.py Outdated
@nicolasbock

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

Thanks @cjerdonek for the helpful comments! I pushed an updated version of this change set.

Protect `@` as safe character when cleaning URLs
Remote URLs that contain a link to a git repository and a tag
reference will have the `@` character converted into `%40`. This is
incorrect.

Fixes: #6437

Signed-off-by: Nicolas Bock <nicolasbock@gmail.com>

@nicolasbock nicolasbock force-pushed the nicolasbock:index_with_git branch from addba30 to 265b4e7 Apr 25, 2019

@cjerdonek
Copy link
Member

left a comment

Looks great. Thanks for your quick attention to this, @nicolasbock.

@cjerdonek cjerdonek merged commit 60d3f11 into pypa:master Apr 25, 2019

29 checks passed

Linux Build #20190425.9 succeeded
Details
Linux (Package) Package succeeded
Details
Linux (Test Primary Python27) Test Primary Python27 succeeded
Details
Linux (Test Primary Python36) Test Primary Python36 succeeded
Details
Linux (Test Secondary Python34) Test Secondary Python34 succeeded
Details
Linux (Test Secondary Python35) Test Secondary Python35 succeeded
Details
Linux (Test Secondary Python37) Test Secondary Python37 succeeded
Details
Windows Build #20190425.9 succeeded
Details
Windows (Package) Package succeeded
Details
Windows (Test Primary Python27-x64) Test Primary Python27-x64 succeeded
Details
Windows (Test Primary Python36-x64) Test Primary Python36-x64 succeeded
Details
Windows (Test Secondary Python27-x86) Test Secondary Python27-x86 succeeded
Details
Windows (Test Secondary Python34-x64) Test Secondary Python34-x64 succeeded
Details
Windows (Test Secondary Python34-x86) Test Secondary Python34-x86 succeeded
Details
Windows (Test Secondary Python35-x64) Test Secondary Python35-x64 succeeded
Details
Windows (Test Secondary Python35-x86) Test Secondary Python35-x86 succeeded
Details
Windows (Test Secondary Python36-x86) Test Secondary Python36-x86 succeeded
Details
Windows (Test Secondary Python37-x64) Test Secondary Python37-x64 succeeded
Details
Windows (Test Secondary Python37-x86) Test Secondary Python37-x86 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
macOS Build #20190425.9 succeeded
Details
macOS (Package) Package succeeded
Details
macOS (Test Primary Python27) Test Primary Python27 succeeded
Details
macOS (Test Primary Python36) Test Primary Python36 succeeded
Details
macOS (Test Secondary Python34) Test Secondary Python34 succeeded
Details
macOS (Test Secondary Python35) Test Secondary Python35 succeeded
Details
macOS (Test Secondary Python37) Test Secondary Python37 succeeded
Details
news-file/pr News files updated and/or change is trivial.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.