fix: support windows path with drive or unc volume #64
Conversation
| expect(gp('C:')).toEqual('C:.'); | ||
| expect(gp('C:.')).toEqual('C:.'); | ||
| expect(gp('C:*')).toEqual('C:.'); | ||
| expect(gp('C:./*')).toEqual('C:.'); | ||
| expect(gp('C:.//')).toEqual('C:./'); | ||
| expect(gp('C:.//*')).toEqual('C:./'); |
There was a problem hiding this comment.
Why does this add . here? Shouldn't it add / because C: is the root?
Or is this because : should be an allowed character in a relative path?
| expect(gp('\\\\System07\\C$/', { flipBackslashes: false })).toEqual('\\\\System07\\C$/'); | ||
| expect(gp('\\\\System07\\C$/.', { flipBackslashes: false })).toEqual('\\\\System07\\C$/'); | ||
| expect(gp('\\\\System07\\C$/*', { flipBackslashes: false })).toEqual('\\\\System07\\C$/'); | ||
| expect(gp('\\\\System07\\C$/./*', { flipBackslashes: false })).toEqual('\\\\System07\\C$/.'); | ||
| expect(gp('\\\\System07\\C$//', { flipBackslashes: false })).toEqual('\\\\System07\\C$/'); | ||
| expect(gp('\\\\System07\\C$//*', { flipBackslashes: false })).toEqual('\\\\System07\\C$/'); |
There was a problem hiding this comment.
Flipping the backslashes on a glob containing a UNC path shouldn't flip the root slashes?
There was a problem hiding this comment.
I need to understand flipBackslashes again because this seems weird/wrong still.
|
@sttk should your latest commit be a separate PR since it might be considered breaking? |
| @@ -28,14 +35,23 @@ module.exports = function globParent(str, opts) { | |||
|
|
|||
| // preserves full path in case of trailing path separator | |||
| str += 'a'; | |||
|
|
|||
| expect(gp('//')).toEqual('/'); | ||
| expect(gp('//*')).toEqual('/'); | ||
| expect(gp('.//')).toEqual('./'); | ||
| expect(gp('.//*')).toEqual('./'); |
There was a problem hiding this comment.
Reviewing this PR again, I think collapsing duplicate slashes is a separate feature (that is breaking) and we need to submit a separate PR
| if (/^([a-zA-Z]:|\\\\)/.test(fp)) { | ||
| var root = path.win32.parse(fp).root; | ||
| if (path.win32.isAbsolute(fp)) { | ||
| root = root.slice(0, -1); // Strip last path separator | ||
| } | ||
| return root; | ||
| } | ||
| return ''; |
There was a problem hiding this comment.
Instead of using path.win32 APIs, can we instead iterate the string and process the root?
There was a problem hiding this comment.
Alternatively, we could use an implementation like seen at https://github.com/ocsigen/js_of_ocaml/blob/master/runtime/fs.js#L59-L70 to avoid a full-parse on the file/glob path.
| expect(gp('\\\\System07\\C$/')).toEqual('\\\\System07\\C$/'); | ||
| expect(gp('\\\\System07\\C$/.')).toEqual('\\\\System07\\C$/'); | ||
| expect(gp('\\\\System07\\C$/*')).toEqual('\\\\System07\\C$/'); | ||
| expect(gp('\\\\System07\\C$/./*')).toEqual('\\\\System07\\C$/.'); | ||
| expect(gp('\\\\System07\\C$//')).toEqual('\\\\System07\\C$/'); | ||
| expect(gp('\\\\System07\\C$//*')).toEqual('\\\\System07\\C$/'); | ||
| expect(gp('\\\\System07\\C$/path/*.js')).toEqual('\\\\System07\\C$/path'); | ||
|
|
There was a problem hiding this comment.
Does nodejs on Windows understand /System07/C$/ as a UNC path? If so, we should be flipping those backslashes since we say that no \ character is allowed in globs.
| expect(gp('\\\\System07\\C$/', { flipBackslashes: false })).toEqual('\\\\System07\\C$/'); | ||
| expect(gp('\\\\System07\\C$/.', { flipBackslashes: false })).toEqual('\\\\System07\\C$/'); | ||
| expect(gp('\\\\System07\\C$/*', { flipBackslashes: false })).toEqual('\\\\System07\\C$/'); | ||
| expect(gp('\\\\System07\\C$/./*', { flipBackslashes: false })).toEqual('\\\\System07\\C$/.'); | ||
| expect(gp('\\\\System07\\C$//', { flipBackslashes: false })).toEqual('\\\\System07\\C$/'); | ||
| expect(gp('\\\\System07\\C$//*', { flipBackslashes: false })).toEqual('\\\\System07\\C$/'); |
There was a problem hiding this comment.
I need to understand flipBackslashes again because this seems weird/wrong still.
This PR is to fix the issue #63 by separating a drive or a UNC volume from a path string before extracting and combines it after extracting.
And this PR adds replacement of continuous slashes to single slash in a path string.
Closes #63