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

fix: do not use url.URL to support early node 6 and scp-style URLs #64

Closed
wants to merge 1 commit into from

Conversation

billneff79
Copy link

This gets rid of using url.URL, which fixes #60 and #61

This in theory could also be applied to the latest branch. While node v6 isn't supported in the latest branch, it may be a more elegant fix to #60 than leaving url.URL in place.

@billneff79
Copy link
Author

Note: this is an alternative fix to those proposed in PR #62 and PR #63 . If those are merged, this one probably shouldn't be, and vice-versa

legacy.auth = whatwg.username || ''
if (whatwg.password) legacy.auth += ':' + whatwg.password
// Replace the url decoded username:password with the url encoded username:password from the original url
legacy.auth = giturl.match(new RegExp('^[^/]+//([^@]+)@'))[1]
Copy link
Author

Choose a reason for hiding this comment

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

I originally had more checks here for URLs that did not have a protocol or slashes in it, and then I realized I couldn't test that because a URL without a protocol or slashes will never satisfy legacy.auth being truthy, so I removed them.

@isaacs
Copy link
Contributor

isaacs commented Feb 26, 2020

Ah, I'd already landed #62 when I saw this, sorry. I agree, this is a bit of a cleaner fix for legacy v6 node users, but since that node version is only barely supported anyway, I think it's fine either way. Closing this for now, if we get complaints about urlencoding in auth in urls in node v6 less than 6.12, we can revisit it :)

@isaacs isaacs closed this Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants