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

Fixed issue with slash separated paths on windows #117

Closed
wants to merge 2 commits into from
Closed

Fixed issue with slash separated paths on windows #117

wants to merge 2 commits into from

Conversation

felicienfrancois
Copy link

No description provided.

@felicienfrancois
Copy link
Author

Tested with the following paths:

path\to\some\dir=> path\to\some
path\to\some\dir\ => path\to\some\dir
path/to/some/dir=> path\to\some
path/to/some/dir/ => path\to\some\dir
dir => .
dir\ => dir
dir/ => dir

@jprichardson
Copy link
Owner

I'd like to accept this, but I can't until I get all of the other Windows issues currently fixed as reported by Appveyor.

@jprichardson
Copy link
Owner

OK, all Appveyor Windows tests are passing now. It's not quite clear to me what problem this is solving?

@felicienfrancois
Copy link
Author

Before the fix, the copy method were not working correctly when the dest path was slash separated instead of anti-slash separated.
Additionally, using the copy method with a relative dir without any separator were not working

Before the processing of dest dir was:
"path\to\some\dir" => "path\to\some" OK
"path\to\some\dir\" => "path\to\some\dir" OK
"path/to/some/dir" => "" FAIL
"path/to/some/dir/" => "" FAIL
"dir" => "" FAIL
"dir\" => "dir" OK
"dir/" => "" FAIL


Now it is:
`"path\to\some\dir"` => `"path\to\some"`        OK
`"path\to\some\dir\"` => `"path\to\some\dir"`  OK
`"path/to/some/dir"` => `"path\to\some"`        OK
`"path/to/some/dir/"` => "path\to\some\dir"     OK
`"dir"` => "."     OK
`"dir\"` => "dir"     OK
`"dir/"` => "dir"     OK

This was due to the use of split(path.sep) (IE \ on windows) on a possibly not normalized path.
The || "." addition fix the bug for relative dir without separator

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 26, 2016

@jprichardson ping?

@jprichardson
Copy link
Owner

@jprichardson ping?

I'm not sure here. Maybe we need to normalize the paths now? This issue is pretty old.

@RyanZim
Copy link
Collaborator

RyanZim commented Mar 7, 2017

Closing this PR since resolving the merge conflicts would be more work than re-implementing. If anyone has a similar issue, please open an issue in the issue tracker with a reduced test case; we can go from there.

@RyanZim RyanZim closed this Mar 7, 2017
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.

3 participants