-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
doc: fix recommendation to use useless config #12445
Conversation
Why useless? |
See: #11412 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be git config --global --add apply.whitespace fix
, as that fixes whitespace when you apply a patch, which is where a collaborator would need it.
Also a Refs: https://github.com/nodejs/node/issues/11412
in the commit message would be good.
Please could you format your commit message according to the Contributing Guidelines? Maybe something like this?
If you don't have time it can be rewritten by whoever lands this, but if you are going to help out more on the project (as I hope you do), then it saves work if you produce better commit messages. |
I reformatted commit message. |
It makes sense, given "Please never use GitHub's green "Merge Pull Request" button" in https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#landing-pull-requests . |
@gibfahn It is ready for rereview. |
Minor nit: there's a typo in the commit message (below the first line). |
Old config instruction instructed people to enable option that does not exist and is silently ignored by git Refs: #11412
Thanks, should be fixed now. |
Landed in def78e8 with a modified commit log message. |
@jasnell commit did not link? |
That's odd! let's try again with the full hash def78e8 |
@jasnell yeah I've noticed it not link properly quite a few times, possibly a GitHub issue. I generally just go to https://github.com/nodejs/node and click on the |
Maybe we are starting to be out of seven-character hashes and there was an ambiguity. FWIW, I copy the full commit hash via UPD: nope, no ambiguity, that's the only commit to start with |
Use apply.whitespace=fix rather than core.whitespace=fix Ref: nodejs/node#11412 PR-URL: nodejs/node#12445 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Checklist
Affected core subsystem(s)
doc