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

Why path.basename behaves differently by platform? #6587

Closed
sttk opened this issue May 5, 2016 · 5 comments · Fixed by #6590
Closed

Why path.basename behaves differently by platform? #6587

sttk opened this issue May 5, 2016 · 5 comments · Fixed by #6590
Labels
path Issues and PRs related to the path subsystem.

Comments

@sttk
Copy link

sttk commented May 5, 2016

  • Version: 0.10, 0.11, 4, 5, 6
  • Platform: darwin, linux, win32
  • Subsystem: path

  1. When path is non-string

    [darwin] path.basename(null);  // => TypeError
    [linux]  path.basename(null);  // => 'null'
    [win32]  path.basename(null);  // => 'null'
    
    [darwin] path.basename(123);  // => TypeError
    [linux]  path.basename(123);  // => '123'
    [win32]  path.basename(123);  // => '123'
  2. When ext matches path's tail and contains a path separator.

    [darwin] path.basename('aaa/bbb', '/bbb');   // => ''
    [linux]  path.basename('aaa/bbb', '/bbb');   // => ''
    [win32]  path.basename('aaa/bbb', '/bbb');   // => 'bbb'
    [win32]  path.basename('aaa\\bbb', '\\bbb'); // => 'bbb'
    
    [darwin] path.basename('aaa/bbb', 'a/bbb');   // => ''
    [linux]  path.basename('aaa/bbb', 'a/bbb');   // => ''
    [win32]  path.basename('aaa/bbb', 'a/bbb');   // => 'bbb'
    [win32]  path.basename('aaa\\bbb', 'a\\bbb'); // => 'bbb'

    FYI, basename command behaves as follows (I checked on darwin and linux):

    $ basename aaa/bbb bbb
    bbb
    $ basename aaa/bbb /bbb
    bbb
    $ basename aaa/bbb a/bbb
    bbb
    
@addaleax addaleax added the path Issues and PRs related to the path subsystem. label May 5, 2016
@addaleax
Copy link
Member

addaleax commented May 5, 2016

I think 1. can be considered fixed in v6.0.0 in the sense that those always throw a TypeError now. 2. is certainly a bit more interesting, although I can’t really tell what the expected behaviour in such a case might be.

@jasnell
Copy link
Member

jasnell commented May 5, 2016

/cc @mscdex

@sttk
Copy link
Author

sttk commented May 5, 2016

@addaleax

I think 1. can be considered fixed in v6.0.0 in the sense that those always throw a TypeError now.

I've checked on appveyor (windows) and travis (linux) that 1. was fixed in v6.

@sttk
Copy link
Author

sttk commented May 5, 2016

I'm sorry.
2. was also fixed in v5 that node returns '' on all platform.
This problem is not by platform but by version.

@sttk sttk closed this as completed May 5, 2016
@mscdex
Copy link
Contributor

mscdex commented May 5, 2016

No, the second case is definitely an unintended regression/bug. I am working on a fix.

@mscdex mscdex reopened this May 5, 2016
mscdex added a commit to mscdex/io.js that referenced this issue May 18, 2016
This commit fixes a regression in basename() for the case when a
supplied extension name completely matches the resulting basename.

Fixes: nodejs#6587
PR-URL: nodejs#6590
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Fishrock123 pushed a commit that referenced this issue May 23, 2016
This commit fixes a regression in basename() for the case when a
supplied extension name completely matches the resulting basename.

Fixes: #6587
PR-URL: #6590
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
rvagg pushed a commit that referenced this issue Jun 2, 2016
This commit fixes a regression in basename() for the case when a
supplied extension name completely matches the resulting basename.

Fixes: #6587
PR-URL: #6590
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants