Include commit message format in contributing guidelines #5943
Conversation
dinatale2
force-pushed
the
contrib-changes
branch
from
664c47f to
4aeb1e3
Compare
|
Some OpenZFS commits have more than 50 characters in first line, what shall we do in this case? (in context with openzfs/zfs-buildbot#68 ) |
|
@gmelikov I'm planning to do different checks if the commit message is an OpenZFS port. @behlendorf had similar concerns. It may be worth mentioning that OpenZFS port commit message are different and provide a sample commit message for one. |
dinatale2
force-pushed
the
contrib-changes
branch
6 times, most recently
from
7fd7881 to
47ffbc2
Compare
|
@gmelikov @behlendorf I refreshed this to have a section on OpenZFS port commit messages. Would both of you mind looking over it? |
.github/CONTRIBUTING.md
Outdated
| approach was chosen or a brief description of the bug you are resolving. | ||
| Each line of the body must be 70 characters or less. | ||
| * The last line must be a `Signed-off-by:` line with the developer's | ||
| name followed by their Github ID or email. |
There was a problem hiding this comment.
An email address is preferable to their Github ID.
|
|
||
| #### OpenZFS Patch Ports | ||
| If you are porting an OpenZFS patch, the commit message must meet | ||
| the following guidelines: |
There was a problem hiding this comment.
We need to make sure this agrees with what's already described on the wiki, https://github.com/zfsonlinux/zfs/wiki/OpenZFS-Patches. We should also update the wiki to describe cherry-picking patches from an OpenZFS remote since it's a simpler method.
There was a problem hiding this comment.
Just one minor question - do we need to duplicate this information here? Maybe just make a link to wiki page?
There was a problem hiding this comment.
It depends which we think is appropriate. Maybe it's time to make the contributing guidelines the authoritative document for these types of things? Then the wiki can refer to it when appropriate?
Thoughts on that?
There was a problem hiding this comment.
I agree, we should have this in one place and the guidelines make the most sense to me.
There was a problem hiding this comment.
@behlendorf @dinatale2 I wrote the instruction for cherry-pick, please check it https://github.com/zfsonlinux/zfs/wiki/OpenZFS-Patches#porting-a-patch
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| @@ -29,4 +29,5 @@ | |||
| - [ ] I have read the **CONTRIBUTING** document. | |||
| - [ ] I have added tests to cover my changes. | |||
| - [ ] All new and existing tests passed. | |||
| - [ ] All commit messages are properly formatted. | |||
There was a problem hiding this comment.
including adding your signed-off-by.
|
|
||
| #### OpenZFS Patch Ports | ||
| If you are porting an OpenZFS patch, the commit message must meet | ||
| the following guidelines: |
There was a problem hiding this comment.
Just one minor question - do we need to duplicate this information here? Maybe just make a link to wiki page?
dinatale2
force-pushed
the
contrib-changes
branch
3 times, most recently
from
327a245 to
dca5277
Compare
|
@behlendorf @gmelikov Refreshed again. I'm providing the commit check script in the zfs tree and have a make target for it. You can now run |
dinatale2
force-pushed
the
contrib-changes
branch
3 times, most recently
from
d39ae2c to
59605c4
Compare
There was a problem hiding this comment.
As justification for this let's reference one of the many posts explaining why enforcing this git style is a good idea. For example, https://chris.beams.io/posts/git-commit/ which also includes a short list of basic rules.
The seven rules of a great git commit message
Separate subject from body with a blank line
Limit the subject line to 50 characters
Capitalize the subject line
Do not end the subject line with a period
Use the imperative mood in the subject line
Wrap the body at 72 characters
Use the body to explain what and why vs. how
Makefile.am
Outdated
| commitcheck: | ||
| @if git rev-parse --git-dir > /dev/null 2>&1; then \ | ||
| scripts/commitcheck.sh; \ | ||
| fi |
There was a problem hiding this comment.
nit: should be tabs not spaces
scripts/commitcheck.sh
Outdated
| function test_url() | ||
| { | ||
| url="$1" | ||
| if ! curl --output /dev/null --max-time 60 --silent --head --fail "$url" ; then |
There was a problem hiding this comment.
nit: extra space after !
scripts/commitcheck.sh
Outdated
| # check commit message for a normal commit | ||
| function new_change_commit() | ||
| { | ||
| RC=0 |
There was a problem hiding this comment.
I think replacing RC with error would be clearer, s/RC/error/g
scripts/commitcheck.sh
Outdated
| RC=1 | ||
| fi | ||
|
|
||
| # ensure that no lines in the body of the commit are over 70 characters |
There was a problem hiding this comment.
So actually 72 characters is the recommended limit.
dinatale2
force-pushed
the
contrib-changes
branch
from
59605c4 to
529675a
Compare
|
@behlendorf Updated. Did you want me to include a copy of the rules in the contributing guidelines? I only provided a link to the page as further reading. |
Add the need to have a commit message with a specific format to the contributing guidelines. Provide a script to help enforce commit message style. Signed-off-by: Giuseppe Di Natale <[email protected]>
dinatale2
force-pushed
the
contrib-changes
branch
from
529675a to
80ba70c
Compare
Add the need to have a commit message with a specific
format to the contributing guidelines.
Signed-off-by: Giuseppe Di Natale [email protected]