Here are few advices on how to create better commits and pull requests (collection of related commits). Good commits (with good commit messages) make review process smooth and enable better code understanding in the future.
- Each commit
- is be obviously correct
- is covered by necessary
- unit tests
- regression tests
- property tests
- smoke tests
- integration tests
- implements one logical change
- builds without compilation warnings
- runs correctly (so git bisect works)
- should be justifiable on its own merits
- may depends on another commit
- must fix a real bug / add real feature that bothers people (not a “This could be a problem…” or “this could be useful…” type thing).
- Code is compliant with coding style
- (Exception is when moving code from one file to another - in this case you should not modify the moved code at all in the same commit which moves it.)
git diff --checkto spot whitespace errors.
- Avoid pure coding style fixes. Fix piece of code while working on it for other reasons.
- Code is itself readable, with comments explaining the more subtle aspects.
- Probably every review comment which does not result in a code change should result in an additional code comment instead.
- Commit message is well writen (see below).
Commit message should describe what, why and how are you changing. The commit musts implement the specification from commit message.
The format of the commit message is:
subsystem: Summary (<70 chars, no period) (empty line) Body with explanation. Use plain English, present tense and imperative mode everywere. Keep text width under 80 columns. Use ascii characters only (if possible).
The summary is a one-line description of what the patch does. This message should be enough for a reader who sees it with no other context to figure out the scope of the patch; it is the line that will show up in the “short form” changelogs. This message is usually formatted with the relevant subsystem name first, followed by the purpose of the patch.
The body of commit message should describe the commit fully:
Why the thing need patching.
Describe your problem. There must be an underlying problem that motivated you to do this work. Convince the reviewer that there is a problem worth fixing, reviewing and maintaining. (A surprising number of developers fail to provide information about why the thing need patching.)
Describe impact of the problem and of the fix.
Quantify optimizations and trade-offs.
The overall design approach in the commit.
Once the problem is established, describe what you are actually doing about it in technical detail. It’s important to describe the change in plain English for the reviewer to verify that the code is behaving as you intend it to.
Implementation details if any.
Testing results if any.
- Related (fixed, reverted, …) commits with
hash ("summary of commit").
- External sources (URLs), and always summarize external source in text.
- External systems (Jira issue ID, …)
- Involved persons, mainly the reviewer with
Reviewed-by: name <mail>.
This document is based primary on Linux Documentation.