Enable line ending normalization in git, not in the linter

[joaocgreis]

Checklist

• tests and code linting passes
• the commit message follows commit guidelines

Affected core subsystem(s)

meta, tools

Description of change

After #6685 landed, linting is broken on Windows because line endings are CRLF and the linter expects only LF (ref #6912). The linter rule is necessary because files committed to the repository should be strictly LF.

In Windows, it's recommended to use git with core.autocrlf=true, but this was not enforced and the Unix counterpart (core.autocrlf=input) wasn't even recommended. The first commit of this PR adds * text=auto to .gitattributes (as described in https://git-scm.com/docs/gitattributes ), forcing end-of-line normalization for files detected to be text on all systems. Unix users will check out LF files, Windows users will check out CRLF files, and the files stored in the repository will be LF. Git detection of text vs binary files should not be a problem, as it is used by autocrlf and currently done in CI.

Exceptions are added for some text files currently committed as CRLF (only from dependencies), and should be removed when those files are updated. The exception for text fixtures is left as it were, since those were also ignored by ESLint.

With this change, the linter rule is no longer necessary and can be removed, fixing vcbuild jslint on Windows.

Note: Windows users not using autocrlf will have to reset the files in the working dir as explained in https://help.github.com/articles/dealing-with-line-endings/#refreshing-a-repository-after-changing-line-endings . Unix users and Windows users using autocrlf should not be affected.

Fixes: #6912

cc @nodejs/testing @nodejs/platform-windows

[bnoordhuis]

Wildcard text=auto is rather dangerous, IMO. It will happily mangle binary files if git perceives them to be text files and can be brutal to debug. I'd use a whitelisting approach, like:

*.js text=auto
*.md text=auto  # just an example, don't know if this should apply to *.md 

[joaocgreis]

I used text=auto for its simplicity. We are testing with it on Windows now, so we know it's most likely working fine. But I agree it can be a pain to debug in extreme cases, I'll try to make something along the lines you suggested.

[orangemocha]

Rather than removing this rule, can you change its value to the default value? The net result would be the same as removing it, but having the explicit value might prevent people in the future from readding the "wrong" rule (at least they might be prompted to look at the commit history for that line).

[Trott]

unix is the default value.

[Trott]

The options with this rule are unix, windows, or disabling/removing the rule. If we want to leave the line there and have the rule explicitly disabled, change 2 to 0. We don't do that for any other rules, though, if that matters.

[joaocgreis]

Updated.

@bnoordhuis the last commit has the best balance of extensions I could come up with (out of close to 400 file extensions and unique file names), but I don't think it should go in. For two reasons:

• This is a huge break in Windows development. New clones will be ok, but old ones will show 2900 modified files after updating to this commit, so we risk getting a flood of issues like bat files shows as modified when clone repo in linux environment #330. The way around this is described in https://help.github.com/articles/dealing-with-line-endings/#refreshing-a-repository-after-changing-line-endings, which is a nuisance for someone who is not expecting it.
• By defaulting to binary, text files with an extension that is not listed will not be checked by git. This means that we can commit files with any line endings if the file has an extension that I missed or if it is a new one.

Letting git handle this with text=auto has been working well in Windows for a long time. Furthermore, a checkout of master and a checkout of this PR without the last commit are binary equal (except for the files changed here). So, I believe that this PR without the last commit is the best trade-off.

[orangemocha]

+1 for undoing the last commit, and using text=auto.

[joaocgreis]

@bnoordhuis would a change in tools/jslint.js to check for CRLF when process.platform === 'win32' be better in your opinion?

[bnoordhuis]

That seems like a reasonable solution.

[a-metta]

Hi any updates on this? Currently master is still broken for someone working on windows. I've tried these changes locally and they seem to be fixing running vcbuild test on windows. Let me know if there is anything that I can help with as well.

[Trott]

@n0f3 This might be not much of an answer, perhaps, but if all your line endings are CRLF and that is the only lint error you are getting, and you don't mind converting to all LF endings, you can use this to have eslint fix the issue for you:

node tools\eslint\bin\eslint.js --fix --cache --rulesdir=tools\eslint-rules benchmark lib test tools

This is basically what vcbuild.bat lint runs, but with the --fix option added.

As far as this PR, I'd defer to @joaocgreis, @bnoordhuis, and @orangemocha on the status. Although it might not be a terrible idea to ask someone at Microsoft what they think the best solution is. /cc @joshgav

[joaocgreis]

I have another fix for this in progress, with changes in tools/jslint.js as mentioned above. Just have to test it well and I'll open a PR.

[a-metta]

@Trott thanks, that actually fixes the tests and i can run vcbuild test 👍
@joaocgreis that's awesome! let me know if you want any additional testing done and i can absolutely help out 👍

[Trott]

@joaocgreis Note that there has been a change such that the jslint task in Makefile and vcbuild.bat no longer uses jslint.js so that it can take advantage of ESLint's caching of results. On CI, we still use jslint.js. Since we don't have this problem on CI, you can probably ignore jslint.js.

You can change vcbuild.bat to override the rule on the command line. http://eslint.org/docs/user-guide/command-line-interface#rule I think you can disable the rule with --rule='linebreak-style: 0'.

[joaocgreis]

Replaced by #8785