Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Correctly normalize UNC paths #6041

Merged
merged 3 commits into from
Nov 19, 2013
Merged

Correctly normalize UNC paths #6041

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
95408e2
Add support for normalizing UNC paths in the FileSystem, controlled b…
Nov 19, 2013
44396aa
Rename the duplicated-slash regular expression and add JSDoc
Nov 19, 2013
a49cc5f
Assume the FileSystem is initialized within `_normalizePath`; initial…
Nov 19, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions src/filesystem/FileSystem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,21 +388,27 @@ define(function (require, exports, module) {
return (fullPath[0] === "/" || fullPath[1] === ":");
};

// Matches continguous groups of forward slashes
var _duplicatedSlashRE = /\/{2,}/g;
Copy link
Member

Choose a reason for hiding this comment

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

We might want to name this all-upercase and add @const tag in docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


/**
* Returns a canonical version of the path: no duplicated "/"es, no ".."s,
* and directories guaranteed to end in a trailing "/"
* @param {!string} path Absolute path, using "/" as path separator
* @param {boolean=} isDirectory
* @return {!string}
*/
function _normalizePath(path, isDirectory) {
FileSystem.prototype._normalizePath = function (path, isDirectory) {

if (!FileSystem.isAbsolutePath(path)) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess isAbsolutePath() still works on the UNC path by misinterpreting it as a Unix-style path. But at some point I still think we should generalize the current normalizeUNCPaths flag into a handleWindowsPaths flag that both enabled the UNC path handling and disables the ":" check in isAbsolutePath()...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, we could do that. Does it really buy us anything right now though?

throw new Error("Paths must be absolute: '" + path + "'"); // expect only absolute paths
}


var normalizeUNCPaths = this._impl && this._impl.normalizeUNCPaths,
Copy link
Member

Choose a reason for hiding this comment

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

Is it a bug if we get called while this._impl is undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is at least one group of instances of this that didn't seem crazy to me: In JSUtils-tests.js, there are calls to FileSystem.getFileForPath within the module scope. I decided not to call this a bug, but I guess I could be convinced otherwise.

isUNCPath = normalizeUNCPaths && path.search(_duplicatedSlashRE) === 0;

// Remove duplicated "/"es
path = path.replace(/\/{2,}/g, "/");
path = path.replace(_duplicatedSlashRE, "/");

// Remove ".." segments
if (path.indexOf("..") !== -1) {
Expand All @@ -427,8 +433,13 @@ define(function (require, exports, module) {
}
}

if (isUNCPath) {
// Restore the leading double slash that was removed previously
path = "/" + path;
}

return path;
}
};

/**
* Return a File object for the specified path.
Expand All @@ -438,7 +449,7 @@ define(function (require, exports, module) {
* @return {File} The File object. This file may not yet exist on disk.
*/
FileSystem.prototype.getFileForPath = function (path) {
path = _normalizePath(path, false);
path = this._normalizePath(path, false);
var file = this._index.getEntry(path);

if (!file) {
Expand All @@ -457,7 +468,7 @@ define(function (require, exports, module) {
* @return {Directory} The Directory object. This directory may not yet exist on disk.
*/
FileSystem.prototype.getDirectoryForPath = function (path) {
path = _normalizePath(path, true);
path = this._normalizePath(path, true);
var directory = this._index.getEntry(path);

if (!directory) {
Expand Down Expand Up @@ -579,7 +590,7 @@ define(function (require, exports, module) {
return;
}

path = _normalizePath(path, false);
path = this._normalizePath(path, false);
var entry = this._index.getEntry(path);

if (entry) {
Expand Down
3 changes: 3 additions & 0 deletions src/filesystem/impls/appshell/AppshellFileSystem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,4 +419,7 @@ define(function (require, exports, module) {
exports.watchPath = watchPath;
exports.unwatchPath = unwatchPath;
exports.unwatchAll = unwatchAll;

// Only perform UNC path normalization on Windows
exports.normalizeUNCPaths = appshell.platform === "win";
});
52 changes: 52 additions & 0 deletions test/spec/FileSystem-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,58 @@ define(function (require, exports, module) {
});

it("should eliminate duplicated (contiguous) slashes", function () {
MockFileSystemImpl.normalizeUNCPaths = false;
testPrefixes(["", "c:"], function () {
expectNormDir("/", "/");
expectNormDir("//", "/");
expectNormDir("///", "/");
expectNormDir("//foo", "/foo/");
expectNormDir("/foo//", "/foo/");
expectNormDir("//foo//", "/foo/");
expectNormDir("///foo///", "/foo/");
expectNormDir("/foo//bar", "/foo/bar/");
expectNormDir("/foo///bar", "/foo/bar/");

expectNormFile("/foo", "/foo");
expectNormFile("//foo", "/foo");
expectNormFile("///foo", "/foo");
expectNormFile("/foo//bar", "/foo/bar");
expectNormFile("/foo///bar", "/foo/bar");
expectNormFile("//foo///bar", "/foo/bar");
expectNormFile("///foo///bar", "/foo/bar");
expectNormFile("///foo//bar", "/foo/bar");
expectNormFile("///foo/bar", "/foo/bar");
});
});

it("should normalize continguous-slash prefixes for UNC paths", function () {
// UNC paths should have leading slashes reduced to a single leading pair
MockFileSystemImpl.normalizeUNCPaths = true;
testPrefixes([""], function () {
expectNormDir("/", "/");
expectNormDir("//", "//");
expectNormDir("///", "//");
expectNormDir("//foo", "//foo/");
expectNormDir("/foo//", "/foo/");
expectNormDir("//foo//", "//foo/");
expectNormDir("///foo///", "//foo/");
expectNormDir("/foo//bar", "/foo/bar/");
expectNormDir("/foo///bar", "/foo/bar/");

expectNormFile("/foo", "/foo");
expectNormFile("//foo", "//foo");
expectNormFile("///foo", "//foo");
expectNormFile("/foo//bar", "/foo/bar");
expectNormFile("/foo///bar", "/foo/bar");
expectNormFile("//foo///bar", "//foo/bar");
expectNormFile("///foo///bar", "//foo/bar");
expectNormFile("///foo//bar", "//foo/bar");
expectNormFile("///foo/bar", "//foo/bar");
});

// UNC paths do not begin with a letter, so normalization is unchanged
testPrefixes(["c:"], function () {
expectNormDir("/", "/");
expectNormDir("//", "/");
expectNormDir("///", "/");
expectNormDir("//foo", "/foo/");
Expand All @@ -151,6 +202,7 @@ define(function (require, exports, module) {
expectNormDir("/foo//bar", "/foo/bar/");
expectNormDir("/foo///bar", "/foo/bar/");

expectNormFile("/foo", "/foo");
expectNormFile("//foo", "/foo");
expectNormFile("///foo", "/foo");
expectNormFile("/foo//bar", "/foo/bar");
Expand Down
7 changes: 7 additions & 0 deletions test/spec/MockFileSystemImpl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ define(function (require, exports, module) {
}
};

// Indicates whether, by default, the FS should perform UNC Path normalization
var _normalizeUNCPathsDefault = false;

// "Live" data for this instance of the file system. Use reset() to
// initialize with _initialData
var _data;
Expand Down Expand Up @@ -339,11 +342,15 @@ define(function (require, exports, module) {
exports.unwatchPath = unwatchPath;
exports.unwatchAll = unwatchAll;

exports.normalizeUNCPaths = _normalizeUNCPathsDefault;

// Test methods
exports.reset = function () {
_data = {};
$.extend(_data, _initialData);
_hooks = {};

exports.normalizeUNCPaths = _normalizeUNCPathsDefault;
};

/**
Expand Down