-
Notifications
You must be signed in to change notification settings - Fork 2k
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
do not encode / in ref names #1433
Conversation
fixes google#1432 Signed-off-by: Hanno Hecker <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @vetinari!
Before we merge this, can you please add unit test(s) that demonstrate the before/after effects of this change?
Also, please add a comment to the new refURLEscape
function (even though it isn't exported) with text that shows what kind of URLs this function fixes.
this is not visible in the original tests, because the muxter uses url.Path and not url.RawPath to match the route.
func refURLEscape(ref string) string { | ||
parts := strings.Split(ref, "/") | ||
for i, s := range parts { | ||
parts[i] = url.QueryEscape(s) | ||
parts[i] = url.PathEscape(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... with this change from url.QueryEscape
to url.PathEscape
, I'm now concerned that we are breaking what was fixed in #1099.
Can you please add a unit test that demonstrates the URL found in #1099?
Unfortunately, I did not request that unit tests be added in #1099 to demonstrate the before/after changes. :-(
But I'm trying to fix that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as given in the original google#1099
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @vetinari!
LGTM.
Awaiting second LGTM before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👌
Thank you, @wesleimp! |
fixes #1432
Signed-off-by: Hanno Hecker [email protected]