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

Please give the trailing slash back to os.tmpdir() #1669

Closed
ghost opened this issue May 10, 2015 · 13 comments
Closed

Please give the trailing slash back to os.tmpdir() #1669

ghost opened this issue May 10, 2015 · 13 comments
Labels
os Issues and PRs related to the os subsystem.

Comments

@ghost
Copy link

ghost commented May 10, 2015

Please revert the commit b57cc51

Since we can get any string (like "/oh/my/xxx/////"), from the environment, why bother to remove the trailing slash? For consistency between __dirname and os.tmpdir() #715 #747 ??

@mscdex
Copy link
Contributor

mscdex commented May 10, 2015

Why not use path.join() or similar path methods?

$ iojs
> path.join('/oh/my/xxx/////', 'bar')
'/oh/my/xxx/bar'

@mscdex mscdex added the os Issues and PRs related to the os subsystem. label May 10, 2015
@ghost
Copy link
Author

ghost commented May 10, 2015

@mscdex Sorry, maybe I should say it more clearly.

I'm asking why you think it is a necessary breaking change. Just removing one trailing slash won't complete the “consistency” between __dirname and os.tmpdir(), because the user environment may give us some random strings, like "///oh///my///xxx//..//".

And here is another question: Will making os.tmpdir() “consistent” with __dirname encourage people to write unsafe code? If the string "/" or "\\" comes from the user environment (though hardly possible), gets trimmed into "", what directory does it represent (and on what platform)? Should the api doc have a note for this?

EDIT: Added "\\" to the last paragraph.

@rlidwka
Copy link
Contributor

rlidwka commented May 10, 2015

If the string "/" comes from the user environment (though hardly possible), gets trimmed into "", what directory does it represent (and on what platform)?

That's true actually:

$ TEMP=/ ~/io.js/iojs  -e 'console.log(JSON.stringify(require("os").tmpdir()))'
""

/ should not be removed if it's the only character there.

dirname works correctly in that case:

$ cat /test.js
console.log(__dirname)
$ ~/io.js/iojs /test.js
/

@ghost
Copy link
Author

ghost commented May 10, 2015

@rlidwka I've updated my previous comment. Please think about backslash \ too.

@Fishrock123
Copy link
Contributor

cc @tellnes

@brendanashworth
Copy link
Contributor

Perhaps semver-major changes aren't looked at hard enough... maybe we should introduce a hard minimum amount of reviewers for a major change, say 3.

@tellnes
Copy link
Contributor

tellnes commented May 10, 2015

We could change it so we remove all trailing slashes and keep one if there is no other characters there?

@benjamingr
Copy link
Member

@tellnes Yeah, that sounds like a solid workaround.

@jakwings would that satisfy you? I realize this is a breaking change and that you'd have to normalize it through the library, but isn't that a one-time thing and the new behavior makes more sense?

@ghost
Copy link
Author

ghost commented May 11, 2015

@beanieboi No. I don't care the trailing slash and will not use such a boring API any more.

@ghost
Copy link
Author

ghost commented May 11, 2015

I must point out that this is not only a severe problem for the root path "/" or a directory named "\\" at the root dir, but also for every path ended with backslash \ on Linux. So if you insist on removing the trailing slash, you must get the proper path separator first.

@tellnes
Copy link
Contributor

tellnes commented May 11, 2015

So remove trailing path.sep then, but keep one if there is no other characters?

@ghost
Copy link
Author

ghost commented May 11, 2015

@tellnes I have no more opinions on how to deal with those separators. At least, use the correct separator for different platforms. This is a stable API, I hope you all take it more seriously.

cjihrig added a commit that referenced this issue May 13, 2015
os.tmpdir() began stripping trailing slashes in
b57cc51. This causes problems if
the temp directory is simply '/'. It also stripped trailing
slashes without first determining which slash type is used by
the current operating system. This commit only strips trailing
slashes if another character precedes the slash. On Windows, it
checks for ':', as not to strip slashes from something like 'C:\'.
It also only strips slashes that are appropriate for the user's
operating system.

Fixes: #1669
PR-URL: #1673
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Christian Tellnes <christian@tellnes.no>
@cjihrig
Copy link
Contributor

cjihrig commented May 13, 2015

As of 7693705, trailing slashes will not be stripped from / or C:\. Additionally, only slashes relevant to the current operating system will be stripped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os Issues and PRs related to the os subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants