Add support for '#' in git branch names (as git does not disallow them) #4813
Conversation
pradyunsg
added
state: needs discussion
src/pip/_internal/vcs/__init__.py
Outdated
| @@ -227,6 +227,10 @@ def get_url_rev(self): | |||
| assert '+' in self.url, error_message % self.url | |||
| url = self.url.split('+', 1)[1] | |||
| scheme, netloc, path, query, frag = urllib_parse.urlsplit(url) | |||
| if '#' in frag: | |||
There was a problem hiding this comment.
It seems like this code may be more elegant if you pass allow_fragments=False to the call to urlsplit(), in part because you're effectively parsing the fragment yourself anyways.
|
Good point. I'll update the PR. |
|
I agree with @pradyunsg that discussion is needed here. Would another possible approach be to require that the "url" portion of the location be quoted, and then the "revision" portion (e.g. branch name) be unquoted when parsing it? This way, a "#" symbol could be represented using The above is just an idea for discussion purposes and might not work as I stated it (e.g. for backwards compatibility reasons if we already accept many unquoted characters, or for other reasons). |
pradyunsg
changed the title
Considering the following URL 'git+ssh://[email protected]/owner/reponame.git@issue#2035#egg=Reponame', previously we'd get an error similar to: ``` Collecting Reponame from git+ssh://[email protected]/owner/reponame.git@issue#2035#egg=Reponame Cloning ssh://[email protected]/owner/reponame.git (to issue) to /tmp/vampas/pip-build-ftjipp8c/Reponame Could not find a tag or branch 'issue', assuming commit. error: pathspec 'issue' did not match any file(s) known to git. ``` With this change, we check for an existing '#' in the frag part of the parsed URL and add back the remaining of the path in such case. We now get something like: ``` Collecting Reponame from git+ssh://[email protected]/owner/reponame.git@issue#2035#egg=Reponame Cloning ssh://[email protected]/owner/reponame.git (to revision issue#2035) to /tmp/vampas/pip-install-jjs7njl1/Reponame Installing collected packages: Reponame Running setup.py install for Reponame ... done Successfully installed Reponame-5.2 ```
|
PR Updated |
|
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
|
I think this should be kept open (or least kept open as an issue if not a PR). |
|
We're now building packages so this will no longer be an issue for us. |
|
@cjerdonek If OP doesn't really have the need for this anymore and since no one else has requested this, I'm happy to let this PR get closed. If someone else does want this functionality at some point in the future, I think they'll come around to this or given how simple this change is, submit an equivalent PR -- both of which are fine IMO. |
|
I can open a separate issue for branch names with special characters and
then close this PR.
…On Sun, Aug 26, 2018 at 8:25 AM Pradyun Gedam ***@***.***> wrote:
@cjerdonek <https://github.com/cjerdonek> If OP doesn't really have the
need for this anymore and since no one else has requested this, I'm happy
to let this PR get closed.
If someone else does want this functionality at some point in the future,
I think they'll come around to this or given how simple this change is,
submit an equivalent PR -- both of which are fine IMO.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4813 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAVt7hIkBXJb3kmjhiNeqyzCPPk7QzZPks5uUr3ggaJpZM4QGYJU>
.
|
|
Sure. Please do. |
|
I opened issue #5742 for this. |
|
Thanks. :) |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
lock
bot
added
the
auto-locked
Considering the following URL 'git+ssh://[email protected]/owner/reponame.git@issue#2035#egg=Reponame', previously we'd get an error similar to:
With this change, we check for an existing '#' in the frag part of the
parsed URL and add back the remaining of the path in such case.
We now get something like: