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

docs: Clarify commit message guidelines #14600

Merged
merged 1 commit into from Nov 1, 2018

Conversation

Projects
None yet
7 participants
@merland
Copy link
Contributor

commented Oct 29, 2018

In this small PR, the gist of this helpful and informative comment from @fanquake is added to the official contributing instructions, to help future first-time contributors get their commit messages right.

@fanquake fanquake added the Docs label Oct 29, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

ACK 73f358401cd452577a516ca565ff8f6742245808

@ch4ot1c

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

Helpful, thank you!

@jnewbery

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

"any @ mentions should be removed" isn't specific to the squashing commits section. There shouldn't be @ mentions in any commit messages at all.

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

@jnewbery Thanks. Do you know if this is mentioned anywhere in the bitcoin repo?

@jnewbery

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

I don't know. It should be in CONTRIBUTING.md if it's anywhere.

@merland merland force-pushed the merland:update-contrib branch Oct 29, 2018

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

@jnewbery I updated the commit after your suggestion, please have another look.

CONTRIBUTING.md Outdated
@@ -135,6 +137,9 @@ before it will be merged. The basic squashing workflow is shown below.
# Save and quit.
git push -f # (force push to GitHub)

The commit message should be updated so that it doesn't include all the
messages from the interim commits.

This comment has been minimized.

Copy link
@hebasto

hebasto Oct 29, 2018

Member

IMO, this requirement is not suitable for all cases.

This comment has been minimized.

Copy link
@merland

merland Oct 29, 2018

Author Contributor

Interesting! Would you (or someone else) care to elaborate?
Personally I don't really know, but it seems like something that would benefit from a set guideline.

This comment has been minimized.

Copy link
@hebasto

hebasto Oct 29, 2018

Member

e.g., 869193f
That commit was squashed from two ones. And its message does include all the messages from the squashed commits.

This comment has been minimized.

Copy link
@merland

merland Oct 29, 2018

Author Contributor

I think I see what you are saying; there can be cases where just listing the interim commits happens to form a coherent message.

New suggestion:
Please update the resulting commit message if needed, it should read as a coherent message. In most cases this means that you should not just list the interim commits.

This comment has been minimized.

Copy link
@hebasto

@merland merland changed the title docs: Add instructions about commit message when squashing docs: Clarify commit message guidelines Oct 29, 2018

@merland merland force-pushed the merland:update-contrib branch to 0e6de3a Oct 30, 2018

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

Commit updated after comments

@bitcoin bitcoin deleted a comment from ismail120572 Oct 30, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

ACK 0e6de3a

1 similar comment
@hebasto

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

ACK 0e6de3a

@fanquake

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

utACK 0e6de3a

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

utACK 0e6de3a

@laanwj laanwj merged commit 0e6de3a into bitcoin:master Nov 1, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 1, 2018

Merge #14600: docs: Clarify commit message guidelines
0e6de3a added details about commit messages (Martin Erlandsson)

Pull request description:

  In this small PR, the gist of [this helpful and informative comment from @fanquake](#14583 (comment)) is added to the official contributing instructions, to help future first-time contributors get their commit messages right.

Tree-SHA512: ca6e9b639e007944314690ef771470f346d6f0ad8aa398abf86e45258bc617334fdbd2ca10ff930d62669a5eca5b507a3ebae11f75971f80f8a44b0bb17ab054

@merland merland deleted the merland:update-contrib branch Nov 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.