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.

Sign up for GitHub

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

Jump to bottom

cleanPath: use path.IsAbs after converting ToSlash #355

Merged
merged 2 commits into from
Jul 16, 2020
Merged

cleanPath: use path.IsAbs after converting ToSlash #355

merged 2 commits into from
Jul 16, 2020

Conversation

drakkan
Copy link
Collaborator

@drakkan drakkan commented Jun 6, 2020

we need a POSIX path, filepath.IsAbs could give unexpected results on Windows.

I don't have any specific test case for this anyway I think that filepath.IsAbs could consider an absolute path things like C:/path on Windows

@drakkan
cleanPath: use path.IsAbs after converting ToSlash
166a37e 
we need a POSIX path filepath.IsAbs can give unexpected results on Windows
@puellanivis
Copy link
Collaborator

Seems likely, but it’s not immediately clear that this is the more correct behavior. I will take a look sometime (probably next week) and make sure it’s solid before merging.

@drakkan
Copy link
Collaborator Author

drakkan commented Jun 29, 2020

Hi, sorry for the delay, I just found some time to test on Windows and I added a test case.

As expected without the patch it fails on Windows:

 request-server_test.go:426:
             Error Trace:    request-server_test.go:426
             Error:          Not equal:
                             expected: "/C:/a"
                             actual  : "C:/a"

                             Diff:
                             --- Expected
                             +++ Actual
                             @@ -1 +1 @@
                             -/C:/a
                             +C:/a
             Test:           TestCleanPath

@drakkan drakkan mentioned this pull request Jul 16, 2020
Merged
@puellanivis
Copy link
Collaborator

Looks legit. The function even says, “a clean POSIX path”.

@puellanivis puellanivis merged commit 97b9df6 into pkg:master Jul 16, 2020
@eikenb eikenb added the bug label Jul 19, 2020
@eikenb eikenb added this to the v1.12.0 milestone Jul 19, 2020
@drakkan drakkan deleted the path branch November 26, 2020 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
v1.12.0
Development

Successfully merging this pull request may close these issues.
3 participants
@drakkan @puellanivis @eikenb