mirror of
				https://github.com/zulip/zulip.git
				synced 2025-11-04 05:53:43 +00:00 
			
		
		
		
	
		
			
				
	
	
		
			344 lines
		
	
	
		
			16 KiB
		
	
	
	
		
			Markdown
		
	
	
	
	
	
			
		
		
	
	
			344 lines
		
	
	
		
			16 KiB
		
	
	
	
		
			Markdown
		
	
	
	
	
	
# Commit discipline
 | 
						|
 | 
						|
We follow the Git project's own commit discipline practice of "Each
 | 
						|
commit is a minimal coherent idea". This discipline takes a bit of work,
 | 
						|
but it makes it much easier for code reviewers to spot bugs, and
 | 
						|
makes the commit history a much more useful resource for developers
 | 
						|
trying to understand why the code works the way it does, which also
 | 
						|
helps a lot in preventing bugs.
 | 
						|
 | 
						|
Use `git rebase -i` as much as you need to shape your commit structure. See the
 | 
						|
[Git guide](../git/overview.md) for useful resources on mastering Git.
 | 
						|
 | 
						|
## Each commit must be coherent
 | 
						|
 | 
						|
- It should pass tests (so test updates needed by a change should be
 | 
						|
  in the same commit as the original change, not a separate "fix the
 | 
						|
  tests that were broken by the last commit" commit).
 | 
						|
- It should not make Zulip worse. For example, it is fine to add backend
 | 
						|
  capabilities without adding a frontend to access them. It's not fine to add a
 | 
						|
  frontend component with no backend to make it work.
 | 
						|
- It should be safe to deploy individually, or explain in detail in
 | 
						|
  the commit message as to why it isn't (maybe with a [manual] tag).
 | 
						|
  So implementing a new API endpoint in one commit and then adding the
 | 
						|
  security checks in a future commit should be avoided -- the security
 | 
						|
  checks should be there from the beginning.
 | 
						|
- Error handling should generally be included along with the code that
 | 
						|
  might trigger the error.
 | 
						|
- TODO comments should be in the commit that introduces the issue or
 | 
						|
  the functionality with further work required.
 | 
						|
 | 
						|
## Commits should generally be minimal
 | 
						|
 | 
						|
Whenever possible, find chunks of complexity that you can separate from the
 | 
						|
rest of the project.
 | 
						|
 | 
						|
- If you need to refactor code, add tests for existing functionality,
 | 
						|
  rename variables or functions, or make other changes that do not
 | 
						|
  change the functionality of the product, make those changes into a
 | 
						|
  series of preparatory commits that can be merged independently of
 | 
						|
  building the feature itself.
 | 
						|
- Moving code from one file to another should be done in a separate
 | 
						|
  commits from functional changes or even refactoring within a file.
 | 
						|
- 2 different refactorings should be done in different commits.
 | 
						|
- 2 different features should be done in different commits.
 | 
						|
- If you find yourself writing a commit message that reads like a list
 | 
						|
  of somewhat dissimilar things that you did, you probably should have
 | 
						|
  just done multiple commits.
 | 
						|
 | 
						|
### When not to be overly minimal
 | 
						|
 | 
						|
- For completely new features, you don't necessarily need to split out
 | 
						|
  new commits for each little subfeature of the new feature. E.g., if
 | 
						|
  you're writing a new tool from scratch, it's fine to have the
 | 
						|
  initial tool have plenty of options/features without doing separate
 | 
						|
  commits for each one. That said, reviewing a 2000-line giant blob of
 | 
						|
  new code isn't fun, so please be thoughtful about submitting things
 | 
						|
  in reviewable units.
 | 
						|
- Don't bother to split backend commits from frontend commits, even
 | 
						|
  though the backend can often be coherent on its own.
 | 
						|
 | 
						|
## Write a clean commit history
 | 
						|
 | 
						|
- Overly fine commits are easy to squash later, but not vice versa.
 | 
						|
  So err toward small commits, and the code reviewer can advise on
 | 
						|
  squashing.
 | 
						|
- If a commit you write doesn't pass tests, you should usually fix
 | 
						|
  that by amending the commit to fix the bug, not writing a new "fix
 | 
						|
  tests" commit on top of it.
 | 
						|
 | 
						|
Zulip expects you to structure the commits in your pull requests to form
 | 
						|
a clean history before we will merge them. It's best to write your
 | 
						|
commits following these guidelines in the first place, but if you don't,
 | 
						|
you can always fix your history using `git rebase -i` (more on that
 | 
						|
[here](../git/fixing-commits.md)).
 | 
						|
 | 
						|
Never mix multiple changes together in a single commit, but it's great
 | 
						|
to include several related changes, each in their own commit, in a
 | 
						|
single pull request. If you notice an issue that is only somewhat
 | 
						|
related to what you were working on, but you feel that it's too minor
 | 
						|
to create a dedicated pull request, feel free to append it as an
 | 
						|
additional commit in the pull request for your main project (that
 | 
						|
commit should have a clear explanation of the bug in its commit
 | 
						|
message). This way, the bug gets fixed, but this independent change
 | 
						|
is highlighted for reviewers. Or just create a dedicated pull request
 | 
						|
for it. Whatever you do, don't squash unrelated changes together in a
 | 
						|
single commit; the reviewer will ask you to split the changes out into
 | 
						|
their own commits.
 | 
						|
 | 
						|
It can take some practice to get used to writing your commits with a
 | 
						|
clean history so that you don't spend much time doing interactive
 | 
						|
rebases. For example, often you'll start adding a feature, and discover
 | 
						|
you need to do a refactoring partway through writing the feature. When that
 | 
						|
happens, we recommend you stash your partial feature, do the refactoring,
 | 
						|
commit it, and then unstash and finish implementing your feature.
 | 
						|
 | 
						|
For additional guidance on how to structure your commits (and why it matters!),
 | 
						|
check out GitHub's excellent [blog post](https://github.blog/2022-06-30-write-better-commits-build-better-projects).
 | 
						|
 | 
						|
## Commit messages
 | 
						|
 | 
						|
Commit messages have two parts:
 | 
						|
 | 
						|
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.
 | 
						|
 | 
						|
In Zulip, commit summaries have a two-part structure:
 | 
						|
 | 
						|
1. A one or two word description of the part of the codebase changed
 | 
						|
   by the commit.
 | 
						|
2. A short sentence summarizing your changes.
 | 
						|
 | 
						|
Here is an
 | 
						|
[example](https://github.com/zulip/zulip/commit/084dd216f017c32e15c1b13469bcbc928cd0bce9)
 | 
						|
of a good commit message:
 | 
						|
 | 
						|
> 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.
 | 
						|
 | 
						|
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 commit message mistakes.
 | 
						|
 | 
						|
[commit-hook]: ../git/zulip-tools.md#set-up-git-repo-script
 | 
						|
 | 
						|
### Commit summary, part 1
 | 
						|
 | 
						|
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 codebase
 | 
						|
  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 codebase, but hasn't
 | 
						|
  been involved with the effort you're working on.
 | 
						|
- Use no more than 72 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.`
 | 
						|
- `channel_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".
 | 
						|
 | 
						|
#### Mentioning other contributors
 | 
						|
 | 
						|
You can
 | 
						|
[credit](https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors)
 | 
						|
co-authors on a commit by adding a `Co-authored-by:` line after a blank line at
 | 
						|
the end of your commit message:
 | 
						|
 | 
						|
    Co-authored-by: Greg Price <greg@zulip.com>
 | 
						|
 | 
						|
You should always give credit where credit is due. See our [guide on continuing
 | 
						|
unfinished work](../contributing/continuing-unfinished-work.md) for step-by-step
 | 
						|
guidance on continuing work someone else has started.
 | 
						|
 | 
						|
You can also add other notes, such as `Reported-by:`, `Debugged-by:`, or
 | 
						|
`Suggested-by:`, but we don't typically do so.
 | 
						|
 | 
						|
**Never @-mention a contributor in a commit message**, as GitHub will turn this into
 | 
						|
a notification for the person every time a version of the commit is rebased and
 | 
						|
pushed somewhere. If you want to send someone a notification about a change,
 | 
						|
@-mention them in the PR thread.
 | 
						|
 | 
						|
#### 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)
 |