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

Incorrect handling of ../.. #11

Closed
zmeyc opened this issue May 3, 2019 · 6 comments
Closed

Incorrect handling of ../.. #11

zmeyc opened this issue May 3, 2019 · 6 comments
Assignees
Labels
available on master Fix is done on master branch, issue closed on next release bug Something isn't working high Severity: high
Milestone

Comments

@zmeyc
Copy link

zmeyc commented May 3, 2019

Describe the bug

With ../.. only 1 dir is unrolled.

To Reproduce

    const char *path = "ab/cd/ef/../../qw";
    ghc::filesystem::path srcPath = ghc::filesystem::u8path(path);
    std::string destPath = srcPath.lexically_normal().u8string();

Expected behavior
Should produce ab/qw, produces ab/cd/qw.

Attached is a test project.

filesystem_test.zip

UPD:

More test cases:

"\\/\\///\\/" produces //////, expected /.

"a/b/..\\//..///\\/../c\\\\/" produces a/b/c///, expected ../c/.

"a/b/../../../c" produces a/c, expected ../c.

"..a/b/..\\//..///\\/../c\\\\/" produces ..a/b/c///, expected ../c/.

@gulrak gulrak self-assigned this May 3, 2019
@gulrak gulrak added bug Something isn't working high Severity: high labels May 3, 2019
@gulrak gulrak added this to the v1.1.2 milestone May 3, 2019
@gulrak
Copy link
Owner

gulrak commented May 3, 2019

The testcase "ab/cd/ef/../../qw" is indeed wrong, and indeed sequences of ".." are handled wrong. Sadly I didn't have one of those in my tests.

Concerning the additional cases what is the operating system, this is run? The bash.sh looked like Linux but the backslashes look like Windows?

@gulrak gulrak added the question Further information is requested label May 3, 2019
@zmeyc
Copy link
Author

zmeyc commented May 3, 2019

Sorry, I forgot that I had an extra step replacing \ with / in additional test cases.
The code was compiled in VS 2017 in C++17 mode:

std::cout << std::filesystem::u8path("\\/\\///\\/").lexically_normal().u8string() << std::endl;
std::cout << std::filesystem::u8path("ab/cd/ef/../../qw").lexically_normal().u8string() << std::endl;
std::cout << std::filesystem::u8path("a/b/..\\//..///\\/../c\\\\/").lexically_normal().u8string() << std::endl;
std::cout << std::filesystem::u8path("a/b/../../../c").lexically_normal().u8string() << std::endl;
std::cout << std::filesystem::u8path("..a/b/..\\//..///\\/../c\\\\/").lexically_normal().u8string() << std::endl;

Output:

\
ab\qw
..\c\
..\c
..\c\

gulrak added a commit that referenced this issue May 3, 2019
@gulrak gulrak added available on master Fix is done on master branch, issue closed on next release and removed question Further information is requested labels May 3, 2019
@gulrak
Copy link
Owner

gulrak commented May 3, 2019

FIxed on master, will be part of the upcomming bugfix release v1.1.2

@gulrak gulrak closed this as completed May 3, 2019
@zmeyc
Copy link
Author

zmeyc commented May 3, 2019

Awesome, thank you!

#ifdef GHC_OS_WINDOWS
     CHECK(fs::path("\\/\\///\\/").lexically_normal() == "/");
     CHECK(fs::path("a/b/..\\//..///\\/../c\\\\/").lexically_normal() == "../c/");
     CHECK(fs::path("..a/b/..\\//..///\\/../c\\\\/").lexically_normal() == "../c/");
 #endif

These should probably have backslashes on Windows if compatibility with std::filesystem is needed? I.e. "\" and "..\\c\\"

@gulrak
Copy link
Owner

gulrak commented May 3, 2019

You are very wellcome!

Actually no need to use backslashes on the right hand side, as lexically_normal() returns an fs::path and the operator==(path, path) that is triggered will lead to implicit conversion of the right hand side string to a fs::path, so the actual comparison is on path level, working with ghc::filesystem and with MSVCs C++17 implementation of std::filesystem, and I ran the tests against both locally before closing the issue.

@gulrak
Copy link
Owner

gulrak commented May 5, 2019

Released with v1.1.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
available on master Fix is done on master branch, issue closed on next release bug Something isn't working high Severity: high
Projects
None yet
Development

No branches or pull requests

2 participants