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

Fix filepath never reaches root path #12

Merged
merged 1 commit into from Nov 24, 2014

Conversation

jednano
Copy link
Member

@jednano jednano commented Nov 24, 2014

Refer to #11

@xuhdev
Copy link
Member

xuhdev commented Nov 24, 2014

Overall it looks good, but I can see a potential issue -- what if someone names a file with backslash on a Linux system? It may not happen in human named files, but can happen in some temporary files generated by some programs.

};
function getFilepathRoot(filepath) {
var resolved = path.resolve(filepath);
var m = resolved.match(/[\\\/]/);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was talking about this line. See my comments.

@jednano
Copy link
Member Author

jednano commented Nov 24, 2014

@xuhdev I don't think it matters, because my regex is searching for the first slash, which wouldn't have anything to do with the file name itself. Oh, but *nix paths start with a slash, so they are like /z/ instead of just z/. Let me tweak this a bit. You get what I mean though?

@jednano
Copy link
Member Author

jednano commented Nov 24, 2014

BTW, we need to write tests for these use cases, @xuhdev.

@xuhdev
Copy link
Member

xuhdev commented Nov 24, 2014

editorconfig/editorconfig-core-test@09e6038

Added the case with backslash in the glob on non windows system.

@xuhdev
Copy link
Member

xuhdev commented Nov 24, 2014

You meant to add this case, right?

@jednano
Copy link
Member Author

jednano commented Nov 24, 2014

Yeah. I guess the other scenario can't be accounted for as a test. Where EC is installed on a different drive letter. Not sure how a test would be written for that.

@jednano
Copy link
Member Author

jednano commented Nov 24, 2014

@xuhdev I've updated the commit to include your latest test. I also changed the getFilepathRoot function. Please review.

@xuhdev
Copy link
Member

xuhdev commented Nov 24, 2014

I think everything's good now. For case with different drives, you can just run all tests different from your current drive. That is

Z:
ctest C:\path\to\source

jednano pushed a commit that referenced this pull request Nov 24, 2014
Fix filepath never reaches root path
@jednano jednano merged commit ceed42a into editorconfig:master Nov 24, 2014
@jednano jednano deleted the fix-path-resolve branch November 24, 2014 18:10
@treyhunner
Copy link
Member

I just published patch release 0.12.1 for this

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

Successfully merging this pull request may close these issues.

None yet

4 participants