mirror of https://github.com/zulip/zulip.git
docs: Rewrite guidelines for writing commit messages.
The focus is on making the instructions more specific and organized, so that they are easier to follow. Co-authored-by: Tim Abbott <tabbott@zulip.com>
This commit is contained in:
Alya Abbott
GitHub
parent
3a27b12a7d
commit
13680651c5
|
|
@ -86,87 +86,225 @@ check out GitHub's excellent [blog post](https://github.blog/2022-06-30-write-be
|
|||
|
||||
## Commit messages
|
||||
|
||||
First, check out
|
||||
[these](https://github.com/zulip/zulip/commit/4869e1b0b2bc6d56fcf44b7d0e36ca20f45d0521)
|
||||
[examples](https://github.com/zulip/zulip/commit/cd5b38f5d8bdcc1771ad794f37262a61843c56c0)
|
||||
of commits with good commit messages.
|
||||
Commit messages have two parts:
|
||||
|
||||
The first line of the commit message is the **summary**. The summary:
|
||||
1. A **summary**, which is a brief one-line overview of the changes.
|
||||
2. A **description**, which provides further details on the changes,
|
||||
the motivation behind them, and why they improve the project.
|
||||
|
||||
- is written in the imperative (e.g., "Fix ...", "Add ...")
|
||||
- is kept short (max 76 characters, ideally less), while concisely
|
||||
explaining what the commit does
|
||||
- is clear about what part of the code is affected -- often by prefixing
|
||||
with the name of the subsystem and a colon, like "zjsunit: ..." or "docs: ..."
|
||||
- is a complete sentence.
|
||||
In Zulip, commit summaries have a two-part structure:
|
||||
|
||||
### Good summaries:
|
||||
1. A one or two word description of the part of the code base changed
|
||||
by the commit.
|
||||
2. A short sentence summarizing your changes.
|
||||
|
||||
Below is an example of a good commit summary line. It starts with the
|
||||
prefix "provision:", using lowercase "**p**". Next, "Improve performance of
|
||||
install npm." starts with a capital "**I**", uses imperative tense,
|
||||
and ends with a period.
|
||||
Here is an
|
||||
[example](https://github.com/zulip/zulip/commit/084dd216f017c32e15c1b13469bcbc928cd0bce9)
|
||||
of a good commit message:
|
||||
|
||||
> _provision: Improve performance of installing npm._
|
||||
> tests: Remove ignored realm_str parameter from message send test.
|
||||
>
|
||||
> In commit
|
||||
> [8181ec4](https://github.com/zulip/zulip/commit/8181ec4b56abf598223112e7bc65ce20f3a6236b),
|
||||
> we removed the `realm_str` as a parameter for `send_message_backend`. This
|
||||
> removes a missed test that included this as a parameter for that
|
||||
> endpoint/function.
|
||||
|
||||
Here are some more positive examples:
|
||||
|
||||
> channel: Discard all HTTP responses while reloading.
|
||||
|
||||
> integrations: Add GitLab integration.
|
||||
|
||||
> typeahead: Convert to ES6 module.
|
||||
|
||||
> tests: Compile Handlebars templates with source maps.
|
||||
|
||||
> blueslip: Add feature to time common operations.
|
||||
|
||||
> gather_subscriptions: Fix exception handling bad input.
|
||||
|
||||
Compare "_gather_subscriptions: Fix exception handling bad input._" with:
|
||||
|
||||
- "_gather_subscriptions was broken_", which doesn't explain how
|
||||
it was broken (and isn't in the imperative)
|
||||
- "_Fix exception when given bad input_", in which it's impossible to
|
||||
tell from the summary what part of the codebase was changed
|
||||
- "_gather_subscriptions: Fixing exception when given bad input._",
|
||||
not in the imperative
|
||||
- "_gather_subscriptions: Fixed exception when given bad input._",
|
||||
not in the imperative
|
||||
|
||||
The summary is followed by a blank line, and then the body of the
|
||||
commit message.
|
||||
The commit message is a key piece of how you communicate with reviewers and
|
||||
future contributors, and is no less important than the code you write. This
|
||||
section provides detailed guidance on how to write an excellent commit message.
|
||||
|
||||
**Tip:** You can set up [Zulip's Git pre-commit hook][commit-hook] to
|
||||
automatically catch common mistakes in the commit message itself.
|
||||
automatically catch common commit message mistakes.
|
||||
|
||||
[commit-hook]: ../git/zulip-tools.md#set-up-git-repo-script
|
||||
|
||||
### Message body:
|
||||
### Commit summary, part 1
|
||||
|
||||
- The body is written in prose, with full paragraphs; each paragraph should
|
||||
be separated from the next by a single blank line.
|
||||
- The body explains:
|
||||
- why and how the change was made
|
||||
- any manual testing you did in addition to running the automated tests
|
||||
- any aspects of the commit that you think are questionable and
|
||||
you'd like special attention applied to.
|
||||
- If the commit makes performance improvements, you should generally
|
||||
include some rough benchmarks showing that it actually improves the
|
||||
performance.
|
||||
- When you fix a GitHub issue, [mark that you've fixed the issue in
|
||||
your commit
|
||||
message](https://help.github.com/en/articles/closing-issues-via-commit-messages)
|
||||
so that the issue is automatically closed when your code is merged.
|
||||
Zulip's preferred style for this is to have the final paragraph of
|
||||
the commit message read e.g. "Fixes: \#123.".
|
||||
- Avoid `Partially fixes #1234`; GitHub's regular expressions ignore
|
||||
the "partially" and close the issue. `Fixes part of #1234` is a good alternative.
|
||||
- Any paragraph content in the commit message should be line-wrapped
|
||||
to about 68 characters per line, but no more than 70, so that your
|
||||
commit message will be reasonably readable in `git log` in a normal
|
||||
terminal. You may find it helpful to:
|
||||
- configure Git to use your preferred editor, with the EDITOR
|
||||
environment variable or `git config --global core.editor`, and
|
||||
- configure the editor to automatically wrap text to 70 or fewer
|
||||
columns per line (all text editors support this).
|
||||
The first part of the commit summary should only be 1-2 **lower-case**
|
||||
words, followed by a `:`, describing what the part of the product the
|
||||
commit changes. These prefixes are essential for maintainers to
|
||||
efficiently skim commits when doing release management or
|
||||
investigating regressions.
|
||||
|
||||
Common examples include: settings, message feed, compose, left
|
||||
sidebar, right sidebar, recent (for **Recent conversations**), search,
|
||||
markdown, tooltips, popovers, drafts, integrations, email, docs, help,
|
||||
and api docs.
|
||||
|
||||
When it's possible to do so concisely, it's helpful to be a little more
|
||||
specific, e.g., emoji, spoilers, polls. However, a simple `settings:` is better
|
||||
than a lengthy description of a specific setting.
|
||||
|
||||
If your commit doesn't cleanly map to a part of the product, you might
|
||||
use something like "css" for CSS-only changes, or the name of the file
|
||||
or technical subsystem principally being modified (not the full path,
|
||||
so `realm_icon`, not `zerver/lib/realm_icon.py`).
|
||||
|
||||
There is no need to be creative here! If one of the examples above
|
||||
fits your commit, use it. Consistency makes it easier for others to
|
||||
scan commit messages to find what they need.
|
||||
|
||||
Additional tips:
|
||||
|
||||
- Use lowercase (e.g., "settings", not "Settings").
|
||||
- If it's hard to find a 1-2 word description of the part of the code base
|
||||
affected by your commit, consider again whether you have structured your
|
||||
commits well.
|
||||
- Never use a generic term like "bug", "fix", or "refactor".
|
||||
|
||||
### Commit summary, part 2
|
||||
|
||||
This is a **complete sentence** that briefly summarizes your changes. There are
|
||||
a few rules to keep in mind:
|
||||
|
||||
- Start the sentence with an
|
||||
[imperative](https://en.wikipedia.org/wiki/Imperative_mood) verb, e.g.
|
||||
"fix", "add", "change", "rename", etc.
|
||||
- Use proper capitalization and punctuation.
|
||||
- Avoid abbreviations and acronyms.
|
||||
- Be concise, and don't include unnecessary details. For example, "Change X and
|
||||
update tests/docs," would be better written as just, "Change X," since (as
|
||||
discussed above) _every_ commit is expected to update tests and documentation
|
||||
as needed.
|
||||
- Make it readable to someone who is familiar with Zulip's code base, but hasn't
|
||||
been involved with the effort you're working on.
|
||||
- Use no more than 76 characters for the entire commit summary (parts 1 and 2).
|
||||
|
||||
### Examples of good commit summaries
|
||||
|
||||
- `provision: Improve performance of installing npm.`
|
||||
- `channel: Discard all HTTP responses while reloading.`
|
||||
- `integrations: Add GitLab integration.`
|
||||
- `typeahead: Rename compare_by_popularity() for clarity.`
|
||||
- `typeahead: Convert to ES6 module.`
|
||||
- `tests: Compile Handlebars templates with source maps.`
|
||||
- `blueslip: Add feature to time common operations.`
|
||||
- `gather_subscriptions: Fix exception handling bad input.`
|
||||
- `stream_settings: Fix save/discard widget on narrow screens.`
|
||||
|
||||
#### Detailed example
|
||||
|
||||
- **Good summary**: "gather_subscriptions: Fix exception handling bad input."
|
||||
- **Not so good alternatives**:
|
||||
- "gather_subscriptions was broken": This doesn't explain how it was broken, and
|
||||
doesn't follow the format guidelines for commit summaries.
|
||||
- "Fix exception when given bad input": It's impossible to tell what part of the
|
||||
codebase was changed.
|
||||
- Not using the imperative:
|
||||
- "gather_subscriptions: Fixing exception when given bad input."
|
||||
- "gather_subscriptions: Fixed exception when given bad input."
|
||||
|
||||
### Commit description
|
||||
|
||||
The body of the commit message should explain why and how the change
|
||||
was made. Like a good code comment, it should provide context and
|
||||
motivation that will help both a reviewer now, and a developer looking
|
||||
at your changes a year later, understand the motivation behind your
|
||||
decisions.
|
||||
|
||||
Many decisions may be documented in multiple places (for example, both
|
||||
in a commit message and a code comment). The general rules of thumb are:
|
||||
|
||||
- Use the commit message for information that's relevant for someone
|
||||
trying to understand the change this commit is making, or the difference
|
||||
between the old version of the code and the new version. In particular,
|
||||
this includes information about why the new version of the code is better than,
|
||||
or not worse than, the old version.
|
||||
- Use code comments, or the code itself, for information that's relevant
|
||||
for someone trying to read and understand the new version of the code
|
||||
in the future, without comparing it to the old version.
|
||||
- If the information is helpful for reviewing your work (for example,
|
||||
an alternative approach that you rejected or are considering,
|
||||
something you noticed that seemed weird, or an error you aren't sure
|
||||
you resolved correctly), include it in the PR description /
|
||||
discussion.
|
||||
|
||||
As an example, if you have a question that you expect to be resolved
|
||||
during the review process, put it in a PR comment attached to a
|
||||
relevant part of the changes. When the question is resolved, remember
|
||||
to update code comments and/or the commit description to document the
|
||||
reasoning behind the decisions.
|
||||
|
||||
There are some cases when the best approach is improving the code or commit
|
||||
structure, not writing up details in a comment or a commit message. For example:
|
||||
|
||||
- If the information is the description of a calculation or function,
|
||||
consider the abstractions you're using. Often, a better name for a
|
||||
variable or function is a better path to readable code than writing
|
||||
a prose explanation.
|
||||
- If the information describes an additional change that you made while working
|
||||
on the commit, consider whether it is separable from the rest of the changes.
|
||||
If it is, it should probably be moved to its own commit, with its own commit
|
||||
message explaining it. Reviewing and integrating a series of several
|
||||
well-written commits is far easier than reviewing those same changes in a
|
||||
single commit.
|
||||
|
||||
When you fix a GitHub issue, [mark that you have fixed the issue in
|
||||
your commit
|
||||
message](https://help.github.com/en/articles/closing-issues-via-commit-messages)
|
||||
so that the issue is automatically closed when your code is merged,
|
||||
and the commit has a permanent reference to the issue(s) that it
|
||||
resolves. Zulip's preferred style for this is to have the final
|
||||
paragraph of the commit message read, e.g., `Fixes #123.`.
|
||||
|
||||
**Note:** Avoid using a phrase like `Partially fixes #1234.`, as
|
||||
GitHub's regular expressions ignore the "partially" and close the
|
||||
issue. `Fixes part of #1234.` is a good alternative.
|
||||
|
||||
#### The purpose of the commit description
|
||||
|
||||
The commit summary and description should, taken together, explain to another
|
||||
Zulip developer (who may not be deeply familiar with the specific
|
||||
files/subsystems you're changing) why this commit improves the project. This
|
||||
means explaining both what it accomplishes, and why it won't break things one
|
||||
might worry about it breaking.
|
||||
|
||||
- Include any important investigation/reasoning that another developer
|
||||
would need to understand in order to verify the correctness of your
|
||||
change. For example, if you're removing a parameter from a function,
|
||||
the commit message might say, "It's safe to remove this parameter
|
||||
because it was always False," or, "This behavior needs to be removed
|
||||
because ...". A reviewer will likely check that indeed it was always
|
||||
`False` as part of checking your work -- what you're doing is
|
||||
providing them a chain of reasoning that they can verify.
|
||||
- Provide background context. A good pattern in a commit message
|
||||
description is, "Previously, when X happened, this caused Y to
|
||||
happen, which resulted in ...", followed by a description of the
|
||||
negative outcome.
|
||||
- Don't include details that are obvious from looking at the diff for
|
||||
the commit, such as lists of the names of the files or functions
|
||||
that were changed, or the fact that you updated the tests.
|
||||
- Avoid unnecessary personal narrative about the process through which
|
||||
you developed this commit or pull request, like "First I tried X" or
|
||||
"I changed Y".
|
||||
|
||||
#### Formatting guidelines
|
||||
|
||||
There are a few specific formatting guidelines to keep in mind:
|
||||
|
||||
- The commit description should be separated from the commit summary
|
||||
by a blank line. Most tools, including GitHub, will misrender commit
|
||||
messages that don't do this.
|
||||
- Use full sentences and paragraphs, with proper punctuation and
|
||||
capitalization. Paragraphs should be separated with a single blank
|
||||
line.
|
||||
- Be sure to check your description for typos, spelling, and grammar
|
||||
mistakes; commit messages are important technical writing and
|
||||
English mistakes will distract reviewers from your ideas.
|
||||
- Your commit message should be line-wrapped to about 68 characters
|
||||
per line, but no more than 70, so that your commit message will be
|
||||
easy to read in `git log` in a normal terminal. (It's OK for links
|
||||
to be longer -- ignore `gitlint` when it complains about them.)
|
||||
|
||||
**Tip:** You may find it helpful to configure Git to use your preferred editor
|
||||
using the `EDITOR` environment variable or `git config --global core.editor`,
|
||||
and configure the editor to automatically wrap text to 70 or fewer columns per
|
||||
line (all text editors support this).
|
||||
|
||||
### Examples of good commit messages
|
||||
|
||||
- [A backend testing
|
||||
commit](https://github.com/zulip/zulip/commit/4869e1b0b2bc6d56fcf44b7d0e36ca20f45d0521)
|
||||
- [A development environment provisioning
|
||||
commit](https://github.com/zulip/zulip/commit/cd5b38f5d8bdcc1771ad794f37262a61843c56c0)
|
||||
|
|
|
|||
Loading…
Reference in New Issue