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

joinPath("foo", "/bar") should return "/bar" #112

Closed
timotheecour opened this issue Jul 10, 2018 · 10 comments
Closed

joinPath("foo", "/bar") should return "/bar" #112

timotheecour opened this issue Jul 10, 2018 · 10 comments

Comments

@timotheecour
Copy link
Member

joinPath("foo", "/bar") should return "/bar"
It's almost always the right thing to do. Eg:

proc maybeJoin (p1, p2: string): string =
  if p2.isAbsolute: p2 else: p1 / p2

Most languages I use do that, eg:
D, python, C#

rdmd --eval 'std.path.buildPath("a", "/b").writeln'
/b
  • python:
os.path.join('a', '/b')
Out[3]: '/b'

For C++: as mentioned here (rust-lang/rust#16507 (comment)) The C++ committee has recently been dealing with this problem.
http://isocpp.org/files/papers/n4211.html issue 64

  • NOTE: the current buggy behavior is not documented (no example like joinPath("foo", "/bar") is provided) so that makes fixing it less problematic.

  • The PR that fixes it should make sure overload joinPath(parts: varargs[string]) is also fixed (with behavior: any absolute path found kills all the previous arguments) ; likewise with / operator

option B: if for some reason the breaking change is not acceptable, I suggest this: intoduce an optional third arg enum:

JoinBehavior = enum
  throwIfAbs # throw if 2nd arg is abs
  joinAsPath # behavior I suggest
  joinAsString # lagacy behavior
  
joinPath(string, string, joinBehavior = throwIfAbs) = string

joinPath("foo", "bar") # foo/bar
joinPath("foo", "/bar") # will throw
joinPath("foo", "/bar", joinAsPath) # /bar
joinPath("foo", "/bar", joinAsString) # foo/bar

option C: another option is to go ahead with the proposed semantic change but when a flag --transition=8268 is given, throw when 2nd arg is abs

But I prefer to keep it simple and correct and follow the semantics of D, python, C#

@timotheecour
Copy link
Member Author

/cc @andreaferretti @dom96 would a PR to fix that be accepted?

@dom96
Copy link
Contributor

dom96 commented Jul 15, 2018

Sure, I guess.

@Araq
Copy link
Member

Araq commented Jul 17, 2018

I think it's a terrible idea, but if everybody else does it this way... shrug

Copy all files to /backup:

# assume 'walkFiles' happens to return absolute paths:
for x in walkFiles("*.nim"):
  # yay, '/' is smart and makes this either a nop or a failure
  copyFile(x, "/backup" / x)

@timotheecour
Copy link
Member Author

just use relativePath for that:

copyFile(x, "/backup" / x)
=>
copyFile(x, "/backup" / x.relativePath("/"))

where relativePath is defined here nim-lang/Nim#8166 (comment) (same as python's, D's versions)

@Araq
Copy link
Member

Araq commented Jul 18, 2018

You know... I think we need type Path = distinct string; type Filename = distinct string; type FileExt = distinct string and a new path handling module that uses Nim's type system.

@dom96
Copy link
Contributor

dom96 commented Oct 27, 2018

You know... I think we need type Path = distinct string; type Filename = distinct string; type FileExt = distinct string and a new path handling module that uses Nim's type system.

Too late for v1 ;P

@narimiran narimiran transferred this issue from nim-lang/Nim Jan 13, 2019
@ringabout ringabout mentioned this issue Jan 21, 2022
33 tasks
@github-actions
Copy link

This RFC is stale because it has been open for 1095 days with no activity. Contribute a fix or comment on the issue, or it will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jun 13, 2023
@ZoomRmc
Copy link

ZoomRmc commented Jun 13, 2023

Precedents are convincing enough, but I'd argue that the current beheviour seems logical: "Interpret /bar in context of foo being a parent directory". Might be useful in a hypothetical chroot scenario.

Current implementation of relativePath is broken, btw.

@github-actions github-actions bot removed the Stale label Jun 14, 2023
@Zectbumo
Copy link

Please take into consideration the security risk when changing the library:

"htdocs" / "/etc/passwd"

current version:
file not found

after a surprise update that includes this proposal:
returns passwd contents

Btw,

NOTE: the current buggy behavior is not documented (no example like joinPath("foo", "/bar") is provided) so that makes fixing it less problematic.

This has since been documented.
https://nim-lang.org/docs/os.html#joinPath%2Cstring%2Cstring
assert joinPath("usr/", "/lib") == "usr/lib"

@Araq
Copy link
Member

Araq commented Jul 21, 2023

Rejected, it's a terrible idea and an invitation for security breaches.

@Araq Araq closed this as completed Jul 21, 2023
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

No branches or pull requests

5 participants