...
When using the "one commit per logical change" principle, each commit must individually have a standalone explanatory commit message, as per the page Git Commit Messages.
One aspect we’re strongly aiming for is that each standalone commit can be built into a CockroachDB executable, and pass unit tests. Achieving this strengthens the confidence we have that the commits are, in fact, pertaining to just one logical change. This also simplifies the work of analyzing the git history for regressions using the git bisect command. There are exceptions, of course, that can be negotiated on a per-PR basis with the reviewer or your team’s TL.
...
Changes may require moving blocks of code (or tests) to new packages or files, and then making edits to the moved code. We recommend splitting this type of refactoring into two commits. The first commit relocates the code without logical changes. The second makes the required logical changes to the code in its new location. This makes it easier for reviewers to scrutinize the moved code and logical changes separately. If both are contained in a single commit the logical changes will be “hidden” from the reviewer in a large block of newly added lines.
Aesthetic changes
Sometimes an author feels compelled to update spelling, grammar, or the overall structure of the code for legibility or other subjective purposes. When doing so, the author should be mindful that moving/editing code for aesthetic reasons also has a downside: it pollutes the output of git blame, which connects code to its “main” author.
It's nice for a blame to be as clean as possible. It's nice for the history of a stanza to have fewer entries. Being mindful of blame is just common hygiene. There is a bar for changing a line of code; we don't do it gratuitously, and, generally speaking, simply fixing an obvious typo (in a comment) does not meet that bar.
Besides the effect of blame churn on humans, it also has an effect on machines. For example, GitHub might start suggesting this person as a reviewer for future patches.
Authors should thus carefully weigh the legibility gains of aesthetic changes with the maintainability cost of impairing history navigation.
Updating the PR during reviews
...
The approaches proposed below reflects our practice from past experience. We have observed that approaching iterating through PR changes this way during review tends to produce PRs of higher quality, makes the iteration easier for the programmer, and tends to let the review cycles complete faster.
PR containing just 1 logical change commit | PR containing a small number of logical changes (e.g. less than 3) | PR containing a larger number of logical changes (e.g. more than 3) | |
|---|---|---|---|
Answer one round of reviewer's comments. | Suggestions:
The latter is slightly easier but requires an extra step at the end (see below). | Suggestions:
The latter is slightly easier but requires an extra step at the end (see below). | Preferably Otherwise, the reviewer tool (e.g reviewable) quickly becomes confused with the amount of combined changes. |
What to do if the review has more iterations (e.g. more than 5) | Same as above. | Prefer Consider breaking down the PR into sub-PRs. | Break down the PR into sub-PRs. Merge the “prefix” PRs that the reviewer has approved already. |
At the end of the review, when the PR has been approved. | Squash all fixup commits together, so that only 1 commit per logical change remains. Polish the commit message that results from multiple fixups. Ensure it describes the change accurately and add relevant release notes. | Squash fixup commits to their respective logical change. Polish each commit message that results from multiple fixups. Ensure it describes the change accurately and add relevant release notes. Ensure each commit builds into a cockroach executable and passes its unit tests. | Squash fixup commits to their respective logical change.. Polish each commit message that results from multiple fixups. Ensure it describes the change accurately and add relevant release notes. Ensure each commit builds into a cockroach executable and passes its unit tests. |
Additional notes | This is the most common case, also the case we expect from first-time external contributors. | New Cockroach Labs team members should learn how to operate PRs with a small number of logical commits. This is a required skill. We try to avoid letting external contributors submit multi-commit PRs. When they do, we encourage them to break down the PR into sub-PRs upfront. | PRs with numerous commits are generally frowned upon in the Cockroach Labs team. They introduce significant risk in the engineering process; either reviews are unable to conclude, or can let more mistakes slip through. The skill required to manipulate numerous commits through review rounds simultaneously is more advanced and we don't commonly expect it from new co-workers. Likewise, the skill required to review numerous commits in one PR is also advanced. It is also more expensive: it costs most energy and time to review the PR. |
Amending commits
Note: This section is intended to teach how to use git to achieve the goals outlined above, based on what the team has observed works well from past experience.
...
After multiple round of review, a PR/branch with fix-up commits can look like this:
|
Then, at the end of the review, when the reviewer has approved the changes, you may proceed as follows:
...