memory-fs icon indicating copy to clipboard operation
memory-fs copied to clipboard

Handle UNC paths in windows (fixes #23)

Open gajus opened this issue 9 years ago • 10 comments

gajus avatar Sep 07 '16 18:09 gajus

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

gajus avatar Sep 07 '16 18:09 gajus

needs test cases for normalization

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

sokra avatar Sep 07 '16 18:09 sokra

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 Files Coverage Δ
lib/normalize.js 100% <100%> (ø) :white_check_mark:
lib/join.js 100% <100%> (ø) :white_check_mark:
lib/MemoryFileSystem.js 99.1% <100%> (+0.58%) :white_check_mark:

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.

codecov-io avatar Sep 07 '16 19:09 codecov-io

@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?

gajus avatar Sep 07 '16 20:09 gajus

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.

sokra avatar Sep 07 '16 20:09 sokra

Here the absoluteWinRegexp need to include UNC paths.

Whats the need for the addition?

What test case am I missing?

https://github.com/webpack/memory-fs/pull/28/commits/407053104022c5cf90b640efe04bdd21073d8349

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

gajus avatar Sep 07 '16 20:09 gajus

@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.

gajus avatar Sep 08 '16 11:09 gajus

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.

sokra avatar Sep 09 '16 14:09 sokra

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 avatar Sep 17 '16 19:09 gajus

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.

gajus avatar Sep 17 '16 19:09 gajus