Skip to content
This repository was archived by the owner on Aug 15, 2024. It is now read-only.

Handle UNC paths in windows (fixes #23) - #28

Open
gajus wants to merge 8 commits into
masterfrom
feature/handle-unc-paths-in-windows
Open

Handle UNC paths in windows (fixes #23)#28
gajus wants to merge 8 commits into
masterfrom
feature/handle-unc-paths-in-windows

Conversation

@gajus

@gajusgajus commented Sep 7, 2016

Copy link
Copy Markdown
Contributor

No description provided.

@gajus

gajus commented Sep 7, 2016

Copy link
Copy Markdown
ContributorAuthor

@sokra Please review this, as it affects the logic outside of just pathToArray (normalisation).

Comment threadlib/MemoryFileSystem.js Outdated
function pathToArray(path) {
path = normalize(path);

var UNC = /^\\\\/.test(path);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lower case unc. all upper case only for constants.

@sokra

sokra commented Sep 7, 2016

Copy link
Copy Markdown
Member

needs test cases for normalization

i. e. fs.normalize("\\\\a\\b/../c\\d\\..").should.be.eql("\\\\a\\c");

@codecov-io

codecov-io commented Sep 7, 2016

Copy link
Copy Markdown

Codecov Report

Merging #28 into master will increase coverage by 0.47%.
The diff coverage is 100%.

@@ Coverage Diff @@## master #28 +/- ##
==========================================
+ Coverage 98.78% 99.25% +0.47% 
==========================================
Files 3 3 Lines 246 268 +22 Branches 46 49 +3 ==========================================
+ Hits 243 266 +23 + Misses 3 2 -1
Impacted FilesCoverage Δ
lib/normalize.js100% <100%> (ø)
lib/join.js100% <100%> (ø)
lib/MemoryFileSystem.js99.1% <100%> (+0.58%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a12b5c...3ccfa79. Read the comment docs.

@gajus

gajus commented Sep 7, 2016

Copy link
Copy Markdown
ContributorAuthor

@sokra are you certain regarding addition/support of this test case?

fs.normalize("\\\\a\\b/../c\\d\\..").should.be.eql("\\\\a\\c");

It would make the code quite ugly. That is, I had a few dabs at it, and reverted. The fact that in *nix a file/ directory name can contain a backslash (such as in the case of this test), makes it particularly ugly.

Besides, none of the existing test cases would support an equivalent mix of *nix and windows DS, e.g. these will fail:

"/a/b/c/..\\.."// AssertionError: expected '/a/b/c/..\\..' to equal '/a'"C:\\a\\b\\\c\\../.."// AssertionError: expected 'C:\\a\\b\\c\\../..' to equal 'C:\\a'

am I overlooking something?

@sokra

sokra commented Sep 7, 2016

Copy link
Copy Markdown
Member

oh yeah, I was wrong. the normalization does only care for . and ... The path joining corrects the slashes. Here the absoluteWinRegexp need to include UNC paths.

@gajus

gajus commented Sep 7, 2016

Copy link
Copy Markdown
ContributorAuthor

Here the absoluteWinRegexp need to include UNC paths.

Whats the need for the addition?

What test case am I missing?

4070531

As it is, all scenarios are covered, as far as my testing goes.

@gajus

gajus commented Sep 8, 2016

Copy link
Copy Markdown
ContributorAuthor

@sokra I had a second look into this. I don't see what you want to achieve by adding the UNC path support to absoluteWinRegexp. UNC paths are already handled as expected, except in cases when there is a mix of nix and windows DS, which does not work with the current test suite anyway.

I am happy to add the support for mixing windows/nix DS, though as I have mentioned earlier, it will require some refactoring and it is not going to work in all cases, since nix paths can contain backwards slashes in file/directory names.

@sokra

sokra commented Sep 9, 2016

Copy link
Copy Markdown
Member

mixed windows/nix paths will occur. i. e. when resolving ./relative/path in \\srv\share\src.
But windows style path separators will not appear in linux paths.

So \\srv\share\src/relative/path is possible, but /root/src\relative\path is not possible (it's possible but \ is not a path separator in this case). Because of this lucky circumstance we do not have a problem with backwards slashes in linux paths.

Comment threadlib/normalize.js Outdated

if (unc) {
path = '\\' + path;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

instead of this block, could we just do:

vardoubleSlashWinRegExp=/(?!^)\\+/g;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@gajus

Copy link
Copy Markdown
ContributorAuthor

oh yeah, I was wrong. the normalization does only care for . and ... The path joining corrects the slashes. Here the absoluteWinRegexp need to include UNC paths.

3ccfa79

@gajus

Copy link
Copy Markdown
ContributorAuthor

So \srv\share\src/relative/path is possible, but /root/src\relative\path is not possible (it's possible but \ is not a path separator in this case). Because of this lucky circumstance we do not have a problem with backwards slashes in linux paths.

I have fixed the refactoring issue and fixed the join function for use with UNC paths. However, I am leaving the normalisation of a mix of path to issue open. I don't see how it can be solved without major refactoring of normalise.js.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@gajus@sokra@codecov-io