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

regression: normalizePath("foo/..") now incorrectly returns `""`, should be `"."` like before + in almost all other languages #10017

Closed
timotheecour opened this issue Dec 16, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@timotheecour
Copy link
Contributor

commented Dec 16, 2018

/cc @Araq
my code broke due to a recent regression introduced here: ce9815b

import os
block:
  var s = "foo/.."
  normalizePath(s)
  echo "{", s, "}" # WAS {.}; now: {}

other languages:

conflating . with empty string is a bad idea; empty string (as a path) represents un-initialized path; this is different from . which represents current dir

Pretty much all other languages I use don't do that mistake:

  • D: buildNormalizedPath("foo/..") prints .
  • python: os.path.normpath('foo/..') prints .
  • C++: boost::filesystem::path("foo/..").lexically_normal().string() => .
  • node JS: path.normalize('foo/..') prints .
  • go: fmt.Println(filepath.Clean("foo/..")) prints .

note

there was already a test case in place:
doAssert normalizedPath(".") == "."
but it got overwritten by ce9815b
doAssert normalizedPath(".") == ""

[EDIT]
see also @krux02 's comment from #10017 (comment):

you forgot the most important example, the shell. No matter if it is Linux or Window, everywhere the current directory is ".". "" is not a valid path on my shell (fish).

timotheecour added a commit to timotheecour/Nim that referenced this issue Dec 16, 2018

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2018

=> PR #10018

@andreaferretti

This comment has been minimized.

Copy link
Collaborator

commented Dec 17, 2018

Same for me: my tests for Rosencrantz are broken since now "." / "LICENSE" equals "/LICENSE" instead of "./LICENSE" (nothing major, just the tests are broken)

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

@andreaferretti I'm confused about your msg:

"." / "LICENSE" equals "/LICENSE" instead of "./LICENSE"

that doesn't seem true, and seems unrelated to this issue; "." / "LICENSE" already correctly returns "LICENSE" ; please show a minimized test example if that's not the case

import os
doAssert "." / "LICENSE" == "LICENSE"

git rev-parse HEAD
0409f23

@andreaferretti

This comment has been minimized.

Copy link
Collaborator

commented Dec 17, 2018

You are right, it was a previous mistake on my part, where I was doing (incorrectly) "." / "/LICENSE", which used to return (incorrectly) "./LICENSE". When I saw this issue, I thought it was the same - I am instead going to fix this in Rosencrantz tests

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

wow, you're bringing up a good point, looks like "." / "/LICENSE" now returns /LICENSE but used to return ./LICENSE ; which is related to https://github.com/nim-lang/Nim/issues/8268 except it's now buggy because it's inconsistent with joinPath("usr/", "/lib") that still returns usr/lib : just filed another regression, which affects joinPath: #10025

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

adding high priority because regressions like this can cause a lot of harm (eg wrong file being deleted etc)

@krux02

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

@timotheecour you forgot the most important example, the shell. No matter if it is Linux or Window, everywhere the current directory is ".". "" is not a valid path on my shell (fish).

@timotheecour timotheecour changed the title regression: normalizePath("foo/..") now incorrectly returns "", should be "" like before + in almost all other languages regression: normalizePath("foo/..") now incorrectly returns `""`, should be `"."` like before + in almost all other languages Dec 17, 2018

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

added your comment on top post. So could someone please merge my PR to fix it? #10018

timotheecour added a commit to timotheecour/Nim that referenced this issue Dec 18, 2018

timotheecour added a commit to timotheecour/Nim that referenced this issue Dec 18, 2018

@Araq Araq closed this in #10018 Dec 18, 2018

Araq added a commit that referenced this issue Dec 18, 2018

[os] fix #10017 regression, fix #10025 regression (#10018)
* [os] fix #10017 regression
* [os] fix #10025 regression
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.