Tweak esignore by refack · Pull Request #16372 · nodejs/node · GitHub

Fixes issue with unicode paths not being ignored by eslint
b485bc4 solves what #16010 tried to acomplish
The second commit solves it at the root

Refs: #16010
Refs: #16278

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

tools,test

/cc @Trott @chrisbudy @nodejs/testing

First commit seems fine. Not clear on how/why the second commit. Can you elaborate a bit?

The original bug is caused by

node/tools/eslint/node_modules/ignore/ignore.js

Line 416 in 8e268c7

return (/^\\\\\?\\/.test(str) || /[^\x00-\x80]+/.test(str) ? str : str.replace(/\\/g, '/')

in ignore. This RegExp fails when there are non ASCII characters in the path. so it fails to "posix"ise the path
ignore slices the path by / then test east slice for a match exclusion with the exclusion rules
eslint has a default ignore rule to exclude any path starting with . so if the temp directories we create start with dot we don't need an extra rule, and it's not slice dependant.

@refack Thanks for the clear explanation. Does the bug only manifest on Windows? I've not seen it anywhere else... EDIT: Never mind, I see that the buggy code is indeed Windows-specific.

LGTM if CI is green. I dislike using .tmp instead of tmp but that's an aesthetic decision, and working around bugs is more important than accommodating my aesthetic preferences.

The bug manifests only on Windows because the step that is sensitive to unicode (or UTF-8) is the conversion from \ to /.

I guess it makes sense to leave test/tmp* in .gitignore because those directories will still be hanging around after this commit lands in someone's clone.

Maybe it makes sense to add test/tmp* (and test/.tmp* because why not?) to the clean target in Makefile and vcbuild.bat so that there's a chance of removal of test/tmp* from .gitignore eventually? Could be a separate PR, especially if we think a fix might be coming to ignore.js and people might actively dislike the change to .tmp for whatever reason.

Maybe it makes sense to add test/tmp* (and test/.tmp* because why not?) to the clean target in Makefile and vcbuild.bat so that there's a chance of removal of test/tmp* from .gitignore eventually? Could be a separate PR, especially if we think a fix might be coming to ignore.js and people might actively dislike the change to .tmp for whatever reason.

I'm a big believer in git clean -Xfd (Capital X to remove ignored files), but for those who are used to make targets will do...

I guess it makes sense to leave test/tmp* in .gitignore because those directories will still be hanging around after this commit lands in someone's clone.

P.S. a .gitignore cleanup/refactor in the works...

RE the third commit (25ff3343d326c4295f580db32e7d0c7b5a1a8127) I'm doing it semi blind, so would appreciate some extra manual validation.
Decided to un-document the vcbuild.bat command since it seems vestigial (does very little actual cleaning), and only relevant for release build scenarios.

I think the indentation in the current Makefile makes more sense because this is a continuation of the previous line and not a new command.

Could you add a comment? Maybe something like:

// Eslint ignores directories beginning with '.' by default.

I'm not sure we need this, if we never had it before, then why not just stick with the new one? (partly a question for @Trott )

If we are adding it (and it's already legacy) then could you add a comment saying # Remove this at some point

So this is only used in a few places right?

node/vcbuild.bat

Lines 171 to 177 in 7a171fd

    
           if "%target%"=="Clean" echo deleting %~dp0deps\icu 
        
           if "%target%"=="Clean" rmdir /S /Q %~dp0deps\icu 
        
           :no-depsicu 
        
           call :getnodeversion || exit /b 1 
        
           if "%target%"=="Clean" rmdir /Q /S "%~dp0%config%\node-v%FULLVERSION%-win-%target_arch%" > nul 2> nul

Agreed that it doesn't look like a user-facing thing.

Might have been once... Now it seems more like clean-release-artifacts. So, confusing.

So how exactly does this make the ignoring more correct?

It's an ignore thing, it builds the exclusion regex based on this line (requiring a terminal /), then the directory itself is matched as test/addons/12_XFD (with no trailing /) so it passes and all it's children are enumerated (only they fail).
So the line pre PR is equivalent to test/addons/??_*/* and test/addons/??_* make it exclude the directories and not even enumerate their content.

Git doesn't ignore it by default, so maybe remove git and

Makes eslint exclude directories without enumerating their content PR-URL: nodejs#16372 Refs: nodejs#16010 Refs: nodejs#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

`.tmp` prefix allows easier exclusion PR-URL: nodejs#16372 Refs: nodejs#16010 Refs: nodejs#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: nodejs#16372 Refs: nodejs#16010 Refs: nodejs#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Makes eslint exclude directories without enumerating their content PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

`.tmp` prefix allows easier exclusion PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Makes eslint exclude directories without enumerating their content PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

`.tmp` prefix allows easier exclusion PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Makes eslint exclude directories without enumerating their content PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

`.tmp` prefix allows easier exclusion PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Makes eslint exclude directories without enumerating their content PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

`.tmp` prefix allows easier exclusion PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Makes eslint exclude directories without enumerating their content PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

`.tmp` prefix allows easier exclusion PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

This should likely be part of a batch update to eslint

Makes eslint exclude directories without enumerating their content PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

`.tmp` prefix allows easier exclusion PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 21, 2017

refack added test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform. labels Oct 21, 2017

Trott approved these changes Oct 22, 2017

View reviewed changes

joyeecheung approved these changes Oct 22, 2017

View reviewed changes

lpinca approved these changes Oct 22, 2017

View reviewed changes

gibfahn approved these changes Oct 22, 2017

View reviewed changes

Trott reviewed Oct 22, 2017

View reviewed changes

gibfahn reviewed Oct 22, 2017

View reviewed changes

jasnell approved these changes Oct 23, 2017

View reviewed changes

refack added 3 commits October 26, 2017 18:38

refack force-pushed the tweak-esignore branch from d86197f to 9ab6481 Compare October 26, 2017 22:39

refack merged commit 9ab6481 into nodejs:master Oct 26, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

Trott mentioned this pull request Nov 14, 2017

tools: make lint-js should not lint test/tmp* #17029

Closed

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

refack deleted the tweak-esignore branch April 26, 2018 21:15

	if "%target%"=="Clean" echo deleting %~dp0deps\icu
	if "%target%"=="Clean" rmdir /S /Q %~dp0deps\icu
	:no-depsicu

	call :getnodeversion \|\| exit /b 1

	if "%target%"=="Clean" rmdir /Q /S "%~dp0%config%\node-v%FULLVERSION%-win-%target_arch%" > nul 2> nul

Tweak esignore by refack · Pull Request #16372 · nodejs/node · GitHub

Uh oh!

Uh oh!

Tweak esignore - #16372

Tweak esignore#16372refack merged 3 commits intonodejs:masternodejs/node:masterfrom refack:tweak-esignorerefack/node:tweak-esignoreCopy head branch name to clipboard

Conversation

refack commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Oct 21, 2017

Uh oh!

Trott commented Oct 22, 2017

Uh oh!

refack commented Oct 22, 2017

Uh oh!

Trott commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Oct 22, 2017

Uh oh!

Trott commented Oct 22, 2017

Uh oh!

refack commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Oct 22, 2017

Uh oh!

refack commented Oct 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Tweak esignore#16372
refack merged 3 commits into
nodejs:masterfrom
refack:tweak-esignore

refack commented Oct 21, 2017 •
edited
Loading

Trott commented Oct 22, 2017 •
edited
Loading

refack commented Oct 22, 2017 •
edited
Loading