-
Notifications
You must be signed in to change notification settings - Fork 1k
Improve handling of paths containing repeated separators (fix #1946). #1948
Conversation
Error message on unexpected output string parameters were reversed. Previous error message showed: "expected GOPATH to be <result>, got <test data>."
…Prefix Previously strings were being cleaned with strings.TrimSuffix() to remove trailing separators. However, filepath.Clean() additionally removes directory navigation symbols (e.g. './', '../') and repeated separators. TestHasFilepathPrefix has additional test cases added to ensure trailing, and repeated separators are handled correctly. Fixes golang#1946
Removes artifacts such as trailing and repeated separators, directory navigation symbols ('./' '../'). Also add a test case to ensure that repeated separators are handled correctly (relevant for golang#1946).
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
1 similar comment
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
CLAs look good, thanks! |
1 similar comment
CLAs look good, thanks! |
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.
Looks like it should be fine!
Changes to this logic has historically given us some nasty bites, but it really seems like a normalizing function like filepath.Clean()
should be safe.
So, as long as the tests pass, best thing to do is just give it a try.
What does this do / why do we need it?
Issue: #1946
Host system GOPATH may contain repeated separators or other artifacts which can cause an error
when not needed. E.g:
GOPATH:
Error:
The function
filepath.Clean()
is used in order to simplify filepaths and remove the artifacts.What should your reviewer look out for in this PR?
In order to fix the issue only the changes to
internal/fs/fs.go
are required. Howevercontext.go
is alsoupdated to further clean the GOPATH as it gets returned from
TestDetectGOPATH
.Test cases were added in
context_test.go
andinternal/fs/fs_test.go
, which currently only cover repeatedslashes, since this was what the bugfix is targeting.
There is also one small fix to the test case
TestDetectGOPATH
where the two string values were reversed in the error statement.Do you need help or clarification on anything?
I'm not sure if the test case changes should live in separate commits to the actual code fix.
Which issue(s) does this PR fix?
fixes #1649