Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using core.whitespace fix in git #11412

Closed
gibfahn opened this issue Feb 16, 2017 · 9 comments
Closed

Using core.whitespace fix in git #11412

gibfahn opened this issue Feb 16, 2017 · 9 comments
Labels
meta Issues and PRs related to the general management of the project.

Comments

@gibfahn
Copy link
Member

gibfahn commented Feb 16, 2017

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)

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label Feb 16, 2017
@Fishrock123
Copy link
Contributor

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
Copy link
Member Author

gibfahn commented Feb 16, 2017

@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
Copy link
Contributor

silverwind commented Feb 17, 2017

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

PR #12445 removes this misleading instruction

@mscdex
Copy link
Contributor

mscdex commented Apr 16, 2017

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

@matkoniecz
Copy link
Contributor

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?

jasnell pushed a commit that referenced this issue Apr 18, 2017
Use apply.whitespace=fix rather than core.whitespace=fix

Ref: #11412
PR-URL: #12445
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
evanlucas pushed a commit that referenced this issue Apr 25, 2017
Use apply.whitespace=fix rather than core.whitespace=fix

Ref: #11412
PR-URL: #12445
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
Copy link
Member

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

evanlucas pushed a commit that referenced this issue May 1, 2017
Use apply.whitespace=fix rather than core.whitespace=fix

Ref: #11412
PR-URL: #12445
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
evanlucas pushed a commit that referenced this issue May 2, 2017
Use apply.whitespace=fix rather than core.whitespace=fix

Ref: #11412
PR-URL: #12445
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gibfahn pushed a commit that referenced this issue May 16, 2017
Use apply.whitespace=fix rather than core.whitespace=fix

Ref: #11412
PR-URL: #12445
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue May 18, 2017
Use apply.whitespace=fix rather than core.whitespace=fix

Ref: #11412
PR-URL: #12445
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Use apply.whitespace=fix rather than core.whitespace=fix

Ref: nodejs/node#11412
PR-URL: nodejs/node#12445
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests

6 participants