Add normalizePath and tests #6587

FedericoCeratto · 2017-10-24T19:32:29Z

Related to #6486

yglukhov · 2017-10-25T08:44:56Z

Thats cool, but maybe its better to move normalizePath to ospaths.nim? Also please have a look at https://github.com/yglukhov/nimx/blob/master/nimx/pathutils.nim#L33. That is an inplace version to avoid allocations. Immutable version might be called normalizedPath and reuse the inplace version. Also my impl may optionally respect native path separators which proved to be quite useful.

jxy · 2017-12-21T23:48:42Z

Any news on this? I would like these to be in the stdlib somewhere.

kaushalmodi · 2018-05-31T12:49:45Z

Just recently, I wished such a function existed that converted relative paths like ../foo/ to absolute paths.

Can this please be added?

dom96 · 2018-05-31T23:32:08Z

This is great, but it sucks that os in general doesn't offer in-place versions of procs. Providing it for this is a bit awkward.

FedericoCeratto · 2018-06-01T10:01:30Z

Rebased. Feel free to merge it as it is or modify the PR.

dom96 · 2018-06-01T10:38:59Z

Thanks for rebasing, but tests are failing.

dom96 · 2018-07-03T12:39:36Z

The tos test is failing so your new tests or implementation must be wrong.

timotheecour · 2018-07-05T19:45:54Z

just sent out #8213 add os.absolutePath; fixes #8174
it allows passing in a 2nd (optional) argument (root), and works correctly with cases where path is already absolute, ignoring the root argument in that case

Araq · 2018-07-06T07:54:48Z

Is absolutePath the same as result = if isAbsolute(x): x else: getCurrentDir() / x ?

dom96

As a general note. These functions should all be moved to ospaths.

dom96 · 2018-07-06T11:03:54Z

lib/pure/os.nim

+  ## raises OSError in case of an error. Follows symlinks.
+  ##
+  ## To create absolute paths from any path (existing or not) see
+  ## `<#absolutePath>`_.


I think expandPath would make more sense as a name here. absolutePath makes me wonder how it differs from normalizePath.

dom96 · 2018-07-06T11:11:35Z

lib/pure/os.nim

+  ##
+  ## Warning: URL-encoded and Unicode attempts at directory traversal are not detected.
+  ## Triple dot is not handled.
+  let is_abs = path[0] == separator


not a big deal, but convention in stdlib is camelCase.

Also, on Windows this is wrong, no? You should use the isAbsolute procedure.

dom96 · 2018-07-06T11:20:33Z

tests/stdlib/tos.nim

+  doAssert normalizedPath("\\a\\b\\..\\..\\..\\foo", '\\') == "\\foo"
+
+when defined(Linux) or defined(osx):


That's cheating. You can't add something to os and only test it on Linux/Mac.

dom96 · 2018-07-06T11:26:28Z

lib/pure/os.nim

+  else:
+    path = "."
+
+proc normalizedPath*(path: string, separator=DirSep): string {.rtl, extern: "nos$1", tags: [].} =


Can you remember where in the stdlib currently this convention is in use? It's about time this is documented but I can't find any other module that uses it.

dom96 · 2018-07-06T11:27:46Z

tests/stdlib/tos.nim

+      doAssert normalizedPath("/../..") == "/"
+      doAssert normalizedPath("/../../") == "/"
+      doAssert normalizedPath("/../../../") == "/"


I wonder if it would make more sense to raise an error for these.

That's the point of normalization. If you are looking for a way to detect and prevent directory traversal I think that should go into joinPath

I agree with @dom96 , throwing seems much saner behavior.
Anecdotically, D's version also doesn't throw but that behavior isn't documented (just filed https://issues.dlang.org/show_bug.cgi?id=19069 to see if good arguments come up)

EDIT likewise with python:
os.path.normpath('/../..')
Out[6]: '/'

even though i don't see that being documented...

what's a use case where silently accepting is preferable to throwing?

dom96 · 2018-07-06T11:33:58Z

I do also prefer the absolutePath implementation that @timotheecour made in #8213. (But I do still believe it should be name expandPath)

FedericoCeratto · 2018-07-06T12:38:14Z

@Araq absolutePath is meant to do that (see implementation) and it's also normalizing the output by trimming away a trailing separator.

FedericoCeratto · 2018-07-06T12:40:23Z

@dom96 "expansion" usually refers to expanding "~" into the home directory and so on.
absolutePath is meant to go from relative to absolute path, hence the name.

kaushalmodi · 2018-07-06T13:18:15Z

@dom96 I agree on the "absolutePath" naming. I don't see any confusion when one sees this name. I infact ended up on this issue thread when I couldn't find anything named "absolutePath" in the stdlib.

FedericoCeratto · 2018-07-06T13:22:53Z

See expandFilename() discussion in #6486 . Unlike the proposed absolutePath, expandFilename() is acting on existing files and directories only.

dom96 · 2018-07-06T19:10:35Z

Okay, absolutePath it is. Can anyone point me to where this normalize vs. normalized convention is used? I can't find it in the stdlib and I want to document it.

timotheecour · 2018-07-07T10:26:23Z

lib/pure/os.nim

@@ -338,6 +338,47 @@ proc expandFilename*(filename: string): string {.rtl, extern: "nos$1",
      result = $r
      c_free(cast[pointer](r))

+proc normalizePath*(path: var string) {.rtl, extern: "nos$1", tags: [].} =


this should be in ospaths, no? nothing in normalizePath accesses file system

oh yeah, I forgot about this. But @FedericoCeratto has waited long enough with this PR so we can fix this in a separate PR.

of course; I much prefer faster PR turnaround than waiting forever for PR's to be so-called "perfect" (this slow turnaround really hurts D ecosystem IMO, Nim seems better in that regard)

=> kept track of that in #8177 meta-issue so we don't forget

jxy mentioned this pull request Oct 24, 2017

checkModuleName uses non-canonicalized path name #6547

Closed

FedericoCeratto force-pushed the normalizePath branch from 635017b to b3165fb Compare October 24, 2017 21:16

FedericoCeratto force-pushed the normalizePath branch 2 times, most recently from 8b2c961 to 14492f3 Compare November 19, 2017 15:01

FedericoCeratto force-pushed the normalizePath branch 2 times, most recently from 316702c to 390ab7d Compare November 23, 2017 13:48

FedericoCeratto force-pushed the normalizePath branch 2 times, most recently from 78383a6 to f3e0a2a Compare April 15, 2018 18:53

kaushalmodi approved these changes May 31, 2018

View reviewed changes

FedericoCeratto force-pushed the normalizePath branch from f3e0a2a to 6aa7a6a Compare June 1, 2018 09:52

FedericoCeratto force-pushed the normalizePath branch from 6aa7a6a to 0523159 Compare July 1, 2018 22:51

demotomohiro mentioned this pull request Jul 2, 2018

[ospaths, os] miscellaneous issues #8177

Open

26 tasks

FedericoCeratto force-pushed the normalizePath branch 5 times, most recently from e55e30f to b237bd4 Compare July 5, 2018 11:13

FedericoCeratto mentioned this pull request Jul 5, 2018

missing ospaths.absolutePath function to get absolute path from a path #8174

Closed

dom96 requested changes Jul 6, 2018

View reviewed changes

FedericoCeratto force-pushed the normalizePath branch 2 times, most recently from 24b9eec to d7cefc8 Compare July 6, 2018 18:48

FedericoCeratto changed the title ~~Add normalizePath, absolutePath and tests~~ Add normalizePath and tests Jul 6, 2018

Add normalizePath and tests

d65429d

FedericoCeratto force-pushed the normalizePath branch from d7cefc8 to d65429d Compare July 6, 2018 19:22

dom96 merged commit 53ce58f into nim-lang:devel Jul 7, 2018

timotheecour reviewed Jul 7, 2018

View reviewed changes

FedericoCeratto deleted the normalizePath branch July 7, 2018 11:18

timotheecour added the OS (stdlib) Related to OS modules in standard library label Oct 25, 2018

timotheecour mentioned this pull request Oct 25, 2018

Add relativePath proc and test #8166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add normalizePath and tests #6587

Add normalizePath and tests #6587

FedericoCeratto commented Oct 24, 2017

yglukhov commented Oct 25, 2017

jxy commented Dec 21, 2017

kaushalmodi commented May 31, 2018

dom96 commented May 31, 2018

FedericoCeratto commented Jun 1, 2018

dom96 commented Jun 1, 2018

dom96 commented Jul 3, 2018

timotheecour commented Jul 5, 2018

Araq commented Jul 6, 2018

dom96 left a comment

dom96 Jul 6, 2018

dom96 Jul 6, 2018

dom96 Jul 6, 2018

dom96 Jul 6, 2018

dom96 Jul 6, 2018

dom96 Jul 6, 2018

FedericoCeratto Jul 6, 2018

timotheecour Jul 7, 2018 •

edited

Loading

dom96 commented Jul 6, 2018 •

edited

Loading

FedericoCeratto commented Jul 6, 2018

FedericoCeratto commented Jul 6, 2018

kaushalmodi commented Jul 6, 2018

FedericoCeratto commented Jul 6, 2018

dom96 commented Jul 6, 2018

timotheecour Jul 7, 2018

dom96 Jul 7, 2018

timotheecour Jul 7, 2018

timotheecour Jul 7, 2018

		doAssert normalizedPath("\\a\\b\\..\\..\\..\\foo", '\\') == "\\foo"

		when defined(Linux) or defined(osx):

Add normalizePath and tests #6587

Add normalizePath and tests #6587

Conversation

FedericoCeratto commented Oct 24, 2017

yglukhov commented Oct 25, 2017

jxy commented Dec 21, 2017

kaushalmodi commented May 31, 2018

dom96 commented May 31, 2018

FedericoCeratto commented Jun 1, 2018

dom96 commented Jun 1, 2018

dom96 commented Jul 3, 2018

timotheecour commented Jul 5, 2018

Araq commented Jul 6, 2018

dom96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

dom96 commented Jul 6, 2018 • edited Loading

FedericoCeratto commented Jul 6, 2018

FedericoCeratto commented Jul 6, 2018

kaushalmodi commented Jul 6, 2018

FedericoCeratto commented Jul 6, 2018

dom96 commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour Jul 7, 2018 •

edited

Loading

dom96 commented Jul 6, 2018 •

edited

Loading