Permalink
Switch branches/tags
Find file Copy path
Fetching contributors…
Cannot retrieve contributors at this time
329 lines (250 sloc) 12.8 KB

Commit Guidelines

A commit consists mainly of two things: the change it introduces and the commit message. These guidelines ought to demonstrate the value in splitting up changes into a sequence of individual commits, and the importance of writing good commit messages to go along with them.

Commit Message

A good commit message should answer three questions:

  1. Why is it necessary?
    It may fix a bug, add a feature, improve performance, reliability, stability, or just be a change for the sake of correctness.

  2. How does it address the issue?
    For short obvious patches this part can be omitted, but it should be a high level description of what the approach was.

  3. What effects does the patch have?
    In addition to the obvious ones, this may include benchmarks, side effects, etc.

A definitive commit message:
Capitalized, short (preferably 50 chars or less, 70 max) summary

More detailed explanatory text, if necessary.  Wrap it to about 72
characters or so.  In some contexts, the first line is treated as the
subject of an email and the rest of the text as the body.  The blank
line separating the summary from the body is critical (unless you omit
the body entirely); tools like rebase can get confused if you run the
two together.

Write your commit message in the imperative: "Fix bug" and not
"Fixed bug" or "Fixes bug". This convention matches up with commit
messages generated by commands like git merge and git revert.

Further paragraphs come after blank lines.

- Bullet points are okay, too

- Typically a hyphen or asterisk is used for the bullet, followed by a
  single space, with blank lines in between, but conventions vary here

If you use an issue tracker, put references to them at the bottom,
like this:

Fixes #5678, fixes #459 (see details at
https://help.github.com/articles/closing-issues-via-commit-messages/).

Format

Each commit message consists of a subject and a body. The subject is required and must adhere to a more strict set of rules, while the body is optional and liberally written, using normal punctuation and capital letters where appropriate.

<subject>
<BLANK_LINE>
<body>

Note that a single blank line should exist between the subject and the body.

Subject

The subject contains succinct description of the change and should follow these rules:

  • use the imperative, present tense: "Fix bug" not "Fixed bug" or "Fixes bug"
  • try to keep it under 50 characters, but make sure it's no longer than 70 chars
  • start with a capital letter, not considering the optional prefix (see below)
  • don't end it with a period - it's a title and titles don't end with a period
  • can start with an optional prefix, eg. "security: "
  • summarize the change itself, not the bug or effects/benefits (that goes in the body)
  • describe it for everyone, not just for you

A properly formed git commit subject should always be able to complete the following sentence:

  • If applied, this commit will <your commit subject here>

Eg: If applied, this commit will Fix registration for OS X users

Subject examples
Bad Good
Clean the users controller. Clean the users controller
Removed deprecated methods Remove deprecated methods
More fixes for broken stuff Add more fixes for broken stuff
Changing behavior of X Change behavior of X
added more info in the installation guide Add more info in the installation guide

Body

The body should include the motivation for the change and any other information that can be useful for someone interested in that commit. Additional rules:

  • as in the subject, use the imperative, present tense "Add feature" not "Added feature" or "Adds feature"
  • use normal punctuation and capital letters where appropriate
  • lines should not exceed 72 characters, except for compiler error messages or other "non-prose" explanation
  • can include multiple paragraphs
  • references should be added only in the last paragraph
Body examples

Bad:

Fix missing import in compute/utils.py

Fixes #1014829

Closes #24

Problem: this does not mention what imports where missing and why they were needed. Also the references are not in the last paragraph of the body. A better message is:

Good:

Add missing import of 'exception' in compute/utils.py

nova/compute/utils.py makes a reference to exception.NotFound,
however exception has not been imported.

Fixes #1014829
Closes #24

Bad:

Present correct ec2id format for volumes and snaps

Fixes #1013765
* Add template argument to ec2utils.id_to_ec2_id() calls

Change-Id: I5e574f8e60d091ef8862ad814e2c8ab993daa366

Problem: this does not mention what the current (broken) format is, nor what the new fixed format is. Also the reference is not in the last paragraph. A better message is:

Good:

Present correct ec2id format for volumes and snaps

During the volume uuid migration, done by changeset XXXXXXX,
ec2 id formats for volumes and snapshots was dropped and is
now using the default instance format (i-xxxxx). These need
to be changed back to vol-xxx and snap-xxxx.

Adds a template argument to ec2utils.id_to_ec2_id() calls

Fixes #1013765

Information in Commit Messages

The information describing the change in a commit is as important as the change itself. When writing commit messages there are some important things to remember:

  • Do not assume the reviewer understands what the original problem was.
    When reading bug reports, after a number of back & forth comments, it is often as clear as mud, what the root cause problem is. The commit message should have a clear statement as to what the original problem is. The bug is merely interesting historical background on /how/ the problem was identified. It should be possible to review a proposed patch for correctness without needing to read the bug ticket.

  • Do not assume the reviewer has access to external web services/site.
    In 6 months time when someone is on a train/plane/coach/beach/pub troubleshooting a problem & browsing Git history, there is no guarantee they will have access to the online bug tracker, or online blueprint documents. The great step forward with distributed SCM is that you no longer need to be "online" to have access to all information about the code repository. The commit message should be totally self-contained, to maintain that benefit.

  • Do not assume the code is self-evident/self-documenting.
    What is self-evident to one person, might be clear as mud to another person. Always document what the original problem was and how it is being fixed, for any change except the most obvious typos, or whitespace only commits.

  • Describe why a change is being made.
    A common mistake is to just document how the code has been written, without describing /why/ the developer chose to do it that way. By all means describe the overall code structure, particularly for large changes, but more importantly describe the intent/motivation behind the changes.

  • Read the commit message to see if it hints at improved code structure.
    Often when describing a large commit message, it becomes obvious that a commit should have in fact been split into 2 or more parts. Don't be afraid to go back and rebase the change to split it up into separate commits.

  • Ensure sufficient information to decide whether to review.
    The commit message must contain sufficient information to alert the potential reviewers to the fact that this is a patch they need to look at.

  • The first commit line is the most important.
    In Git commits, the first line of the commit message has special significance. It is used as email subject line, git annotate messages, gitk viewer annotations, merge commit messages and many more places where space is at a premium. As well as summarizing the change itself, it should take care to detail what part of the code is affected.

  • Describe any limitations of the current code.
    If the code being changed still has future scope for improvements, or any known limitations then mention these in the commit message. This demonstrates to the reviewer that the broader picture has been considered and what tradeoffs have been done in terms of short term goals vs. long term wishes.

  • Do not include patch set-specific comments.
    In other words, if you rebase your change please don't add "Patch set 2: rebased" to your commit message. That isn't going to be relevant once your change has merged. This also applies to comments such as "Add unit tests", "Fix localization problems", or any other such patch set to patch set changes that don't affect the overall intent of your commit.

The main rule to follow is:

The commit message must contain all the information required to fully understand & review the patch for correctness. Less is not more. More is more.

Remember: The commit message is mainly for other people, so they should be able to understand it now and six months later.

Logical Changes

The #1 rule for creating good commits is to ensure there is only one "logical change" per commit. There are many reasons why this is an important rule:

  • The smaller the amount of code being changed, the quicker & easier it is to review & identify potential flaws.
  • If a change is found to be flawed later, it may be necessary to revert the broken commit. This is much easier to do if there are not other unrelated code changes entangled with the original commit.
  • When troubleshooting problems using git's bisect capability, small well defined changes will aid in isolating exactly where the code problem was introduced.
  • When browsing history using git annotate/blame, small well defined changes also aid in isolating exactly where & why a piece of code came from.

Things to avoid when creating commits

Below are some common examples of bad things to avoid:

  • Mixing whitespace changes with functional code changes.
    The whitespace changes will obscure the important functional changes, making it harder for a reviewer to correctly determine whether the change is correct.
    Solution: Create 2 commits, one with the whitespace changes, one with the functional changes. Typically the whitespace change would be done first, but that need not be a hard rule.

  • Mixing two unrelated functional changes.
    Again the reviewer will find it harder to identify flaws if two unrelated changes are mixed together. If it becomes necessary to later revert a broken commit, the two unrelated changes will need to be untangled, with further risk of bug creation.

  • Sending large new features in a single giant commit.
    It may well be the case that the code for a new feature is only useful when all of it is present. This does not, however, imply that the entire feature should be provided in a single commit. New features often entail refactoring existing code. It is highly desirable that any refactoring is done in commits which are separate from those implementing the new feature. This helps reviewers and test suites validate that the refactoring has no unintentional functional changes. Even the newly written code can often be split up into multiple pieces that can be independently reviewed. For example, changes which add new internal APIs/classes, can be in self-contained commits. This allows other developers to cherry-pick small parts of the work, if the entire new feature is not immediately ready for merge.

The basic rule to follow is:

If a code change can be split into a sequence of patches/commits, then it should be split. Less is not more. More is more.

You can read more about the Atomic Commit Convention.

References