fix #231 allow square brackets in path #264

loveky · 2018-05-23T15:15:41Z

fixes #231 fixes #220

jsf-clabot · 2018-05-23T15:15:43Z

All committers have signed the CLA.

alexander-akait

Need tests

loveky · 2018-05-24T02:35:01Z

I have a problem when setting up development environment on Windows. Because the file name/dir name under tests/helpers containes ? * which is not allow on Windows. git clone/git reset just fails. same with #220

Currently what I have done:

clone the reproduce repo in Won't copy files when there are square brackets [ ] in the path #231; npm install; modify copy-webpack-plugin under node_modules, verify it solves the issue.
on mac clone copy-webpack-plugin repo. apply the same change, run test to make sure it didn't broken on mac system.

any suggestion?

alexander-akait · 2018-05-24T10:32:26Z

@loveky can you fix it in this PR? Just disable this test if you in window

loveky · 2018-05-25T05:26:47Z

got it. will work on that

alexander-akait · 2018-05-25T10:58:36Z

src/utils/escape.js

@@ -8,7 +8,8 @@ export default function escape(context, from) {
        // Handles special characters in paths
        const absoluteContext = path.resolve(context)
            .replace(/\\/, '/')
-            .replace(/[\*|\?|\!|\(|\)|\[|\]|\{|\}]/g, (substring) => `\\${substring}`);
+            .replace(/[\*|\?|\!|\(|\)|\{|\}]/g, (substring) => `\\${substring}`)
+            .replace(/\[/g, '[[]');


Looks strange. Can you describe why past solution doesn't work?

loveky · 2018-05-27T13:21:30Z

@evilebottnawi

the reason why I modified escape.js is that minimatch will replace all \ in path to /。This leads to a wrong directory hierarchy since it translate \[special directory\] to /[special directory/]. So I use another way to escape special characters by putting every character into a [] which makes the character lost its special meaning.

In the updated commits, I also move the creation of special helper directory to a script. The scrip will create directories/files based the system developers working on. This should also resolve #220

alexander-akait · 2018-05-28T11:23:00Z

scripts/createSpecialDirectory.js

+    const dir = path.dirname(file);
+    mkdirp.sync(path.join(baseDir, dir));
+    fs.writeFileSync(path.join(baseDir, file), specialFiles[originFile]);
+});


Add \n at end file

@loveky just one small fix

which file should I add \n? createSpecialDirectory.js itself? or the files created by it? 🤣

@loveky in this file 😄

filipesilva · 2018-05-31T14:51:38Z

tests/index.js

@@ -15,6 +15,8 @@ import cacache from 'cacache';
 import isGzip from 'is-gzip';
 import zlib from 'zlib';

+import removeIllegalCharacterForWindows from '../scripts/removeIllegalCharacterForWindows';


This should probably be in src/utils instead of scripts/ otherwise it won't get published.

@loveky yep, move this to tests/utils

filipesilva · 2018-05-31T14:52:46Z

Tried a build from this PR on my windows machine with a directory using parenthesis and the assets were copied.

filipesilva · 2018-05-31T14:54:56Z

The lack of new tests might lead to regressions on special character support though.

loveky · 2018-05-31T15:02:51Z

Hi @filipesilva , the square brackets is already in the tests. Should I add more tests? any suggestion?

filipesilva · 2018-05-31T15:15:04Z

I did not realize that, sorry for the confusion!

alexander-akait · 2018-05-31T15:15:47Z

@filipesilva can we merge this, for me LGTM, but i don't have windows near

alexander-akait · 2018-05-31T15:16:12Z

tests/index.js

@@ -15,6 +15,8 @@ import cacache from 'cacache';
 import isGzip from 'is-gzip';
 import zlib from 'zlib';

+import removeIllegalCharacterForWindows from '../scripts/removeIllegalCharacterForWindows';


@loveky yep, move this to tests/utils

clydin · 2018-05-31T14:51:36Z

tests/index.js

@@ -984,7 +993,7 @@ describe('apply function', () => {
                    'nested/nestedfile.txt'
                ],
                patterns: [{
-                    from: '[special?directory]'
+                    from: (path.sep === '/' ? '[special?directory]' : '[specialdirectory]')


The path.sep conditionals are fairly non-obvious as to their purpose. Separate windows only tests would probably make more sense and simplify maintenance moving forward.

@evilebottnawi A Windows CI instance would probably be very useful for this plugin since windows pathing is vastly different than linux/MacOS/etc.

@clydin we already use appveyor for many plugins, need merge webpack-default here, will be done in near future

clydin · 2018-05-31T15:05:00Z

tests/index.js

@@ -1390,7 +1399,7 @@ describe('apply function', () => {
                        'noextension'
                    ],
                    options: {
-                        ignore: ['directory/**/*', '\\[special\\?directory\\]/**/*']
+                        ignore: ['directory/**/*', (path.sep === '/' ? '\\[special\\?directory\\]/**/*' : '[[]specialdirectory]/**/*')]


The escape sequences should be consistent and there should really be a test for both escaping techniques (where appropriate) as they are both accepted.

clydin · 2018-05-31T15:06:54Z

scripts/removeIllegalCharacterForWindows.js

+const path = require('path');
+
+module.exports = function (string) {
+    return path.sep === '/' ? string : string.replace(/[*?"<>|]/g, '');


If those are illegal characters on windows only then a check for windows should be used instead of path.sep which does not guarantee windows. (process.platform === 'win32'?)

clydin · 2018-05-31T15:07:56Z

scripts/createSpecialDirectory.js

+const specialFiles = {
+    '[special?directory]/nested/nestedfile.txt': '',
+    '[special?directory]/(special-*file).txt': 'special',
+    '[special?directory]/directoryfile.txt': 'new'


a test with ! in the file/path should be added to ensure that [!] works as intended.

@loveky yep, add test

clydin · 2018-05-31T15:11:53Z

src/utils/escape.js

@@ -8,7 +8,7 @@ export default function escape(context, from) {
        // Handles special characters in paths
        const absoluteContext = path.resolve(context)
            .replace(/\\/, '/')
-            .replace(/[\*|\?|\!|\(|\)|\[|\]|\{|\}]/g, (substring) => `\\${substring}`);
+            .replace(/[\*|\?|\!|\(|\)|\[|\{|\}]/g, (substring) => `[${substring}]`);


The closing brace should probably stay for consistency (i.e., makes for a simple rule of escape all special characters).

clydin · 2018-05-31T15:14:55Z

src/utils/escape.js

@@ -8,7 +8,7 @@ export default function escape(context, from) {
        // Handles special characters in paths
        const absoluteContext = path.resolve(context)
            .replace(/\\/, '/')


Shouldn't this replacement be removed if the character class escaping method is being used?

Also, on MacOS, this r\!4 is a valid directory name.

filipesilva · 2018-05-31T15:26:36Z

I can say that in my windows machine the test suite in npm test passed. Would you like me to check something else on my machine?

alexander-akait · 2018-05-31T15:30:12Z

@filipesilva not, just this PR

loveky · 2018-06-01T01:08:11Z

@evilebottnawi @clydin @filipesilva I have updated code and added some new tests.

alexander-akait · 2018-06-01T21:15:23Z

wait @filipesilva approve

filipesilva · 2018-06-01T23:05:50Z

Oh sorry, didn't mean to keep you waiting!

alexander-akait

Thanks to everyone.

fix webpack-contrib#231 allow brackets in path

179e339

alexander-akait requested changes May 23, 2018

View reviewed changes

only need take care of [

9b842b0

alexander-akait requested changes May 25, 2018

View reviewed changes

loveky added 3 commits May 26, 2018 21:40

use character sets to escape special character to avoid issue on Windows

8c4405c

[fix 220] create special directory/file through scripts

a065c7f

rm helpers directory on every test

d9e9d32

alexander-akait approved these changes May 28, 2018

View reviewed changes

add new line to the end of scripts

0ffc4f7

alexander-akait requested review from michael-ciniawsky and joshwiens May 28, 2018 12:20

alexander-akait added type: Bug severity: 3 (broken) priority: 4 (important) semver: Patch labels May 28, 2018

filipesilva mentioned this pull request May 31, 2018

Files declared in .angular-cli "assets" are not copied angular/angular-cli#9230

Closed

filipesilva reviewed May 31, 2018

View reviewed changes

alexander-akait requested changes May 31, 2018

View reviewed changes

clydin suggested changes May 31, 2018

View reviewed changes

update code, add tests

e19b4ee

clydin approved these changes Jun 1, 2018

View reviewed changes

alexander-akait approved these changes Jun 1, 2018

View reviewed changes

filipesilva approved these changes Jun 1, 2018

View reviewed changes

alexander-akait approved these changes Jun 1, 2018

View reviewed changes

alexander-akait merged commit 3ef5b6c into webpack-contrib:master Jun 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #231 allow square brackets in path #264

fix #231 allow square brackets in path #264

loveky commented May 23, 2018 •

edited by alexander-akait

jsf-clabot commented May 23, 2018 •

edited

alexander-akait left a comment

loveky commented May 24, 2018 •

edited

alexander-akait commented May 24, 2018 •

edited

loveky commented May 25, 2018

alexander-akait May 25, 2018

loveky commented May 27, 2018 •

edited

alexander-akait May 28, 2018

alexander-akait May 28, 2018

loveky May 28, 2018

alexander-akait May 28, 2018

loveky May 28, 2018

filipesilva May 31, 2018

alexander-akait May 31, 2018

filipesilva commented May 31, 2018

filipesilva commented May 31, 2018

loveky commented May 31, 2018

filipesilva commented May 31, 2018

alexander-akait commented May 31, 2018

alexander-akait May 31, 2018

clydin May 31, 2018

alexander-akait May 31, 2018

clydin May 31, 2018 •

edited

clydin May 31, 2018

clydin May 31, 2018

alexander-akait May 31, 2018

clydin May 31, 2018

clydin May 31, 2018

filipesilva commented May 31, 2018

alexander-akait commented May 31, 2018

loveky commented Jun 1, 2018

alexander-akait commented Jun 1, 2018

filipesilva commented Jun 1, 2018

alexander-akait left a comment

fix #231 allow square brackets in path #264

fix #231 allow square brackets in path #264

Conversation

loveky commented May 23, 2018 • edited by alexander-akait

jsf-clabot commented May 23, 2018 • edited

alexander-akait left a comment

Choose a reason for hiding this comment

loveky commented May 24, 2018 • edited

alexander-akait commented May 24, 2018 • edited

loveky commented May 25, 2018

Choose a reason for hiding this comment

loveky commented May 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipesilva commented May 31, 2018

filipesilva commented May 31, 2018

loveky commented May 31, 2018

filipesilva commented May 31, 2018

alexander-akait commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clydin May 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipesilva commented May 31, 2018

alexander-akait commented May 31, 2018

loveky commented Jun 1, 2018

alexander-akait commented Jun 1, 2018

filipesilva commented Jun 1, 2018

alexander-akait left a comment

Choose a reason for hiding this comment

loveky commented May 23, 2018 •

edited by alexander-akait

jsf-clabot commented May 23, 2018 •

edited

loveky commented May 24, 2018 •

edited

alexander-akait commented May 24, 2018 •

edited

loveky commented May 27, 2018 •

edited

clydin May 31, 2018 •

edited