Node: Using core.whitespace fix in git

[gibfahn]

We recommend using git config --global --add core.whitespace fix in doc/onboarding.md, but I can't find any mention of that setting in the git docs. There is a core.whitespace, but it doesn't seem to have a fix option. However, apply.whitespace does, see the apply docs.

I think what we need is git config --global --add apply.whitespace fix. Note that this only fixes whitespace when you apply a patch. Rebase has a whitespace option as well, but it's apparently incompatible with --interactive, so I'm not sure if we should use it.

We might also consider suggesting git config --global diff.wsErrorHighlight all, which makes git diff and git show highlight whitespace errors (see docs).

cc/ @Fishrock123 (from git blame)

[Fishrock123]

I think you can also set these per-repo. Is it possible that .gitattributes or similar could store this?

I do think it is worthwhile to still suggest people enable the whitespace fixer globally. Not sure about altering git diff, etc.

[gibfahn]

@Fishrock123 what I'm saying is I don't think core.whitespace fix actually does anything, I think it's just a wrong option. I think the option should be apply.whitespace fix.

I'm pretty sure this stuff has to go in .git/config if it's local, so I don't think we could store it.

[silverwind]

Let's drop that recommendation, we already recommend --whitespace=fix when landing:

https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#technical-howto

[silverwind]

Regarding diff.wsErrorHighlight: I'm not sure we should make any more recommendations. There's a lot one could recommend for git, and most of these options are personal preference. For example, I auto-trim trailing whitespace in the editor, so it will never be visible in a diff.

[matkoniecz]

I can confirm that option core.whitespace fix is not doing for an unpatched git. Given that it is not documented in https://git-scm.com/docs/git-config it is not surprising.

[matkoniecz]

PR #12445 removes this misleading instruction

[mscdex]

I think I'd rather just correct the option name rather than remove it. The less explicit parameters needed the better IMHO.

[matkoniecz]

I think the option should be apply.whitespace fix.

According to docs it is used only on applying patch. Is it useful/intended result of this instruction?

[addaleax]

12445 landed, I think this can be closed now. Feel free to re-open if I’m wrong.