Attention is currently required from: Felix Singer, Himanshu Sahdev, Julius Werner, Paul Menzel.
Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75495?usp=email )
Change subject: Docs: Document coreboot's git commit message rules. ......................................................................
Patch Set 1:
(17 comments)
File Documentation/contributing/git_commit_messages.md:
https://review.coreboot.org/c/coreboot/+/75495/comment/0b7adb69_5a56799b : PS1, Line 8: everthing
Thanks, missed that.
Done
https://review.coreboot.org/c/coreboot/+/75495/comment/937bcd34_3b30d75d : PS1, Line 9:
Let us list two: […]
Done
https://review.coreboot.org/c/coreboot/+/75495/comment/95d0ba56_20503724 : PS1, Line 15: - If reflowing prose to 75 characters can reduce the length of the : commit message by 2 or more lines, please reflow it.
I would be for saying, it should also tried to be reached on each line: […]
I understand how you feel about this, but in my opinion, it's not worth worrying about to save a single line. That's why we came up with the 2 lines or more standard.
My opinion is that if it doesn't save any lines, just elongates the first and shortens the next, it's not worth the time to even request it.
Even saving a single line in the commit message is really nitpicky and requesting a change for that feels like being disrespectful of the time and effort it takes to make the change. The commit messages don't need to be *perfect*.
This respect for the author/committer of the patch is really what I'm trying to protect with this compromise.
https://review.coreboot.org/c/coreboot/+/75495/comment/4723e683_591e326f : PS1, Line 17: if
in
Done
https://review.coreboot.org/c/coreboot/+/75495/comment/8d1a9579_d1646014 : PS1, Line 20: subject
I believe the correct git term is *title* or *summary*.
Both links you gave above for how to write a good git commit call it the subject.
https://review.coreboot.org/c/coreboot/+/75495/comment/33931458_6695c800 : PS1, Line 22: Fix whitespace
This is an examples for imperative mood. […]
Done
https://review.coreboot.org/c/coreboot/+/75495/comment/e89be032_fce4f105 : PS1, Line 24: the acronyms list
Make it a link?
Done
https://review.coreboot.org/c/coreboot/+/75495/comment/3535a881_7a6be3a6 : PS1, Line 28: - Start the subject line with the path or area of the change.
Maybe: […]
Done
https://review.coreboot.org/c/coreboot/+/75495/comment/7e6577f0_9a8ddc93 : PS1, Line 29: src/
Mark it up with `?
Done
https://review.coreboot.org/c/coreboot/+/75495/comment/06158002_c92678a2 : PS1, Line 29: - Don't include src/
Can we say "may omit leading path components before vendor/subsystem" or "may truncate down to two d […]
Updated with
``` Because this prefix takes space used by the rest of the title, it should be kept short while still uniquely describing the area. ```
I think that says what you're asking for.
https://review.coreboot.org/c/coreboot/+/75495/comment/4a93c156_70dce1b2 : PS1, Line 40: CB:XXXXX or a 10 character hash
I’d prefer only the git hash part as git does not operate with CB:XXXXX.
I understand what you're saying, but the CB:XXXXX is very convenient because jenkins turns it into a URL. This is officially supported, so I don't want to say it shouldn't be used here.
https://review.coreboot.org/c/coreboot/+/75495/comment/8d489965_d29cc1a9 : PS1, Line 45: - Information about how a patch was tested is useful, but not required.
I’d use: […]
Done
https://review.coreboot.org/c/coreboot/+/75495/comment/95ec1787_eeb3da91 : PS1, Line 46: - All but the most trivial of patches should generally have a body.
I’d leave this out.
Why? My opinion is that it saves people from asking why a patch was done. Maybe the reason for a patch is obvious to the author, but just a title like "replace XXX with YYY" isn't sufficient in my opinion.
Also if you highly recommend a TEST= line, that should be in there as well.
https://review.coreboot.org/c/coreboot/+/75495/comment/7c94b0af_ee97749e : PS1, Line 50: signed-off-by
Signed-off-by
Done
https://review.coreboot.org/c/coreboot/+/75495/comment/0c0a3cad_b960221b : PS1, Line 51: commit
Sounds good. Will update. […]
Done
https://review.coreboot.org/c/coreboot/+/75495/comment/f4cefcf2_daf78b31 : PS1, Line 52: footers from previous commits
Sorry, I do not understand, what footers you mean.
Updated:
``` - When adding a patch that has already gone through another git or gerrit, the footers from those previous commits may be added, but keep the list reasonable. ```
https://review.coreboot.org/c/coreboot/+/75495/comment/5dc7f898_f3697da9 : PS1, Line 58: Found-by: Coverity CID 1469611
I’d add the type of error in brackets at the end too. […]
Done
-
Martin L Roth (Code Review)