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.

Sign up for GitHub

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

Jump to bottom

fix: support windows path with drive or unc volume #64

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

fix: support windows path with drive or unc volume #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .npmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package-lock=false
33 changes: 30 additions & 3 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
'use strict';

var isGlob = require('is-glob');
var pathPosixDirname = require('path').posix.dirname;
var path = require('path');
var pathPosixDirname = path.posix.dirname;
var isWin32 = require('os').platform() === 'win32';

var slash = '/';
Expand All @@ -16,6 +17,12 @@ var escaped = /\\([!*?|[\](){}])/g;
module.exports = function globParent(str, opts) {
var options = Object.assign({ flipBackslashes: true }, opts);

var winDriveOrUncVolume = '';
if (isWin32) {
winDriveOrUncVolume = getWinDriveOrUncVolume(str);
str = str.slice(winDriveOrUncVolume.length);
}

// flip windows path separators
if (options.flipBackslashes && isWin32 && str.indexOf(slash) < 0) {
str = str.replace(backslash, slash);
Expand All @@ -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

// remove path parts that are globby
do {
str = pathPosixDirname(str);
} while (isGlobby(str));

// remove escape chars and return result
return str.replace(escaped, '$1');
str = str.replace(escaped, '$1');

// replace continuous slashes to single slash
str = str.replace(/\/+/g, '/');

if (isWin32 && winDriveOrUncVolume) {
str = winDriveOrUncVolume + str;
}

return str;
};

function isEnclosure(str) {
Expand Down Expand Up @@ -73,3 +89,14 @@ function isGlobby(str) {
}
return isGlob(str);
}

function getWinDriveOrUncVolume(fp) {
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 '';
Comment on lines +94 to +101
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.

}
Empty file removed test/.gitkeep
View file
Open in desktop
Empty file.
71 changes: 71 additions & 0 deletions test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ describe('glob-parent', function () {
expect(gp('.*')).toEqual('.');
expect(gp('/.*')).toEqual('/');
expect(gp('/.*/')).toEqual('/');
expect(gp('//')).toEqual('/');
expect(gp('//*')).toEqual('/');
expect(gp('.//')).toEqual('./');
expect(gp('.//*')).toEqual('./');
Comment on lines +13 to +16
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

expect(gp('a/.*/b')).toEqual('a');
expect(gp('a*/.*/b')).toEqual('.');
expect(gp('*/a/b/c')).toEqual('.');
Expand Down Expand Up @@ -258,4 +262,71 @@ if (isWin32) {
done();
});
});

describe('windows path with drive or UNC volume', function() {
it('should return parent dirname from absolute path with drive letter', function(done) {
expect(gp('C:/')).toEqual('C:/');
expect(gp('C:/.')).toEqual('C:/');
expect(gp('C:/*')).toEqual('C:/');
expect(gp('C:/./*')).toEqual('C:/.');
phated marked this conversation as resolved.
Show resolved Hide resolved
expect(gp('C://')).toEqual('C:/');
expect(gp('C://*')).toEqual('C:/');
expect(gp('C:/path/*.js')).toEqual('C:/path');

expect(gp('C:\\')).toEqual('C:/');
expect(gp('C:\\.')).toEqual('C:/');
expect(gp('C:\\*')).toEqual('C:/');
expect(gp('C:\\.\\*')).toEqual('C:/.');
phated marked this conversation as resolved.
Show resolved Hide resolved
expect(gp('C:\\\\')).toEqual('C:/');
expect(gp('C:\\\\*')).toEqual('C:/');
expect(gp('C:\\path\\*.js')).toEqual('C:/path');

done();
});

it('should return parent dirname from relative path with drive letter', function(done) {
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:./');
Comment on lines +288 to +293
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?

expect(gp('C:path/*.js')).toEqual('C:path');

expect(gp('C:.\\*')).toEqual('C:.');
expect(gp('C:.\\\\')).toEqual('C:./');
expect(gp('C:.\\\\*')).toEqual('C:./');
expect(gp('C:path\\*.js')).toEqual('C:path');

done();
});

it('should return parent dirname from UNC path', function(done) {
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$/.');
phated marked this conversation as resolved.
Show resolved Hide resolved
expect(gp('\\\\System07\\C$//')).toEqual('\\\\System07\\C$/');
expect(gp('\\\\System07\\C$//*')).toEqual('\\\\System07\\C$/');
expect(gp('\\\\System07\\C$/path/*.js')).toEqual('\\\\System07\\C$/path');

Comment on lines +305 to +312
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.

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

expect(gp('\\\\System07\\C$/path/*.js')).toEqual('\\\\System07\\C$/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');

done();
});
});
}