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: support windows path with drive or unc volume #64

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sttk
Copy link
Contributor

@sttk sttk commented Jul 1, 2023

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

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Some questions for you @sttk

test/index.test.js Show resolved Hide resolved
test/index.test.js Show resolved Hide resolved
Comment on lines +288 to +293
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:./');
Copy link
Member

Choose a reason for hiding this comment

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

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?

test/index.test.js Show resolved Hide resolved
Comment on lines +313 to +318
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$/');
Copy link
Member

Choose a reason for hiding this comment

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

Flipping the backslashes on a glob containing a UNC path shouldn't flip the root slashes?

Copy link
Member

Choose a reason for hiding this comment

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

I need to understand flipBackslashes again because this seems weird/wrong still.

@phated
Copy link
Member

phated commented Jul 15, 2023

@sttk should your latest commit be a separate PR since it might be considered breaking?

@sttk
Copy link
Contributor Author

sttk commented Jul 17, 2023

@phated I've removed the commit about the last dot and created new PR #66 for it.

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Some other thoughts on this.

@@ -28,14 +35,23 @@ module.exports = function globParent(str, opts) {

// preserves full path in case of trailing path separator
str += 'a';

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +13 to +16
expect(gp('//')).toEqual('/');
expect(gp('//*')).toEqual('/');
expect(gp('.//')).toEqual('./');
expect(gp('.//*')).toEqual('./');
Copy link
Member

Choose a reason for hiding this comment

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

Reviewing this PR again, I think collapsing duplicate slashes is a separate feature (that is breaking) and we need to submit a separate PR

Comment on lines +94 to +101
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 '';
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using path.win32 APIs, can we instead iterate the string and process the root?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Comment on lines +305 to +312
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');

Copy link
Member

Choose a reason for hiding this comment

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

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.

Comment on lines +313 to +318
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$/');
Copy link
Member

Choose a reason for hiding this comment

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

I need to understand flipBackslashes again because this seems weird/wrong still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Incorrect identification of the static part of the pattern for the disk root on Windows
2 participants