Skip to content

Agree on commit reverting strategy #12979

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.

Sign up for GitHub

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

Jump to bottom
Closed
gibfahn opened this issue May 11, 2017 · 7 comments
Closed

Agree on commit reverting strategy #12979

gibfahn opened this issue May 11, 2017 · 7 comments
Labels
meta Issues and PRs related to the general management of the project.

Comments

@gibfahn
Copy link
Member

gibfahn commented May 11, 2017 •
edited
Loading

The COLLABORATOR_GUIDE doesn't seem to cover how to revert commits, and specifically what the commit message should be. I think we should try to agree what the standard process is.

For single commits with git revert HASH:

  1. Leave the commit message as is.
  2. Modify (not least to pass node-validate-commit), e.g. fs: Revert throw on invalid callbacks #12976

For multiple commits with git revert FROM...TO:

  1. Leave as individual commits
  2. Format commit messages, e.g. Revert commits that cause failing tests on Windows CI #4679
  3. Squash the commits
@gibfahn gibfahn mentioned this issue May 11, 2017
Merged
@gibfahn
Copy link
Member Author

gibfahn commented May 11, 2017

FWIW my preference is 1. and 2.

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label May 11, 2017
@evanlucas
Copy link
Contributor

I was under the impression we always used just git revert <sha> and then amended the commit message with the reason for reverting

@jasnell
Copy link
Member

jasnell commented May 12, 2017

I don't believe I've ever seen us do a multiple commit revert. My preference is definitely git revert HASH on a single commit at a time with an amended commit message.

@gibfahn
Copy link
Member Author

gibfahn commented May 12, 2017

Refs #4679 (comment) from @rvagg :

FYI, we've consistently used the format: Revert "original commit msg" as reversion messages, so even the subsystem prefix goes into the quotes. The tooling we have recognises this format too.

@gibfahn
Copy link
Member Author

gibfahn commented May 12, 2017

Okay, I think we have consensus, so I'll close this as decided. We use git revert, and leave the commits as they are.

If anyone disagrees then comment/reopen.

@gibfahn gibfahn closed this as completed May 12, 2017
@joyeecheung
Copy link
Member

Ah..wait, but this practice is still not documented, no?

@gibfahn gibfahn reopened this May 12, 2017
@gibfahn
Copy link
Member Author

gibfahn commented May 12, 2017

@joyeecheung Good point

@gibfahn gibfahn mentioned this issue May 13, 2017
Merged
2 tasks
gibfahn added a commit to gibfahn/node that referenced this issue May 16, 2017
@gibfahn
doc: document method for reverting commits
92f3b30 
PR-URL: nodejs#13015
Fixes: nodejs#12979
Refs: nodejs#4679 (comment)
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this issue May 19, 2017
@gibfahn
doc: document method for reverting commits
3e5131b 
PR-URL: nodejs#13015
Fixes: nodejs#12979
Refs: nodejs#4679 (comment)
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Jun 22, 2017
@gibfahn @MylesBorins
doc: document method for reverting commits
bf80ba4 
PR-URL: #13015
Fixes: #12979
Refs: #4679 (comment)
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 11, 2017
@gibfahn @MylesBorins
doc: document method for reverting commits
2d5e2e9 
PR-URL: #13015
Fixes: #12979
Refs: #4679 (comment)
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Joyee Cheung <[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
Assignees
No one assigned
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@mscdex @jasnell @evanlucas @joyeecheung @gibfahn