Versions Compared

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.

...

During a review, some attention is given to the coding style.

  1. CockroachDB uses a couple extra formatting rules in addition to those used by gofmt. We provide the command crlfmt to enforce those automaticaly: Use:
    crlfmt -tab 2
    We advise to integrate this in your text editor.
  2. We value good naming for variables, functions for new concepts, etc. If your change extends an area where there is already an established naming pattern, you can reuse the existing pattern. If you are introducing new concepts, be creative! But during the review we might kindly request you to rename some of the entities.

...

  • A larger project may be decomposed in sub-parts / steps. We recommend one commit per logical change in the PR, up to and including when the PR is merged. See below for details.
  • During code review, to answer reviewer comments. We accept seeing fix-up commits during the review, but we request they are merged together (“squashed”) before the PR is merged. See below for more nuance.

Decomposing a PR into logical changes

When a project requires changes to multiple layers in CockroachDB, or needs to evolve concepts used in the source code, reviewing the combination of changes becomes more difficult.

To make the life of the reviewer easier, and to accelerate the review, it is then useful to split the change into multiple sub-changes, each covering just one topic.

We call this “one commit per logical change”.

For example, a change to add a new SQL statement could have the following two commits:

  1. one commit to add support for the SQL syntax, with parsing unit tests. In this commit, the planning code is changed to report an "unimplemented error" when the new statement is planned. A planning unit test is added that verifies that the unimplemented error is reported
  2. another commit to plan the statement, with planning unit tests. In this commit, the execution code is changed to report an "unimplemented error" when the new statement is executed, and the planning unit test is updated to check the result of logical planning.
  3. a final commit to execute the statement, with execution unit tests. In this commit, the execution unit test is updated to check the results of execution.

All these 3 commits can be combined in a single PR.  However, we do not encourage PRs with numerous commits. After 3 logical changes in a single PR, consider splitting the PR into sub-PRs to be reviewed separately.

When using the "one commit per logical change" principle, each commit must individually have a standalone explanatory commit message, as per the section above.

Additionally, we request that each standalone commit can be built into a CockroachDB executable, and pass unit tests. This simplifies the work of analyzing the git history for regressions using the git bisect command.

Updating the PR during reviews

During a review, the reviewer requests additional change to a PR.

How to modify the PR to address reviewer comments? There are two main ways to do this: git commit --amend and git commit --fixup. Both are detailed below.

Summary:

...

Either

  • git commit --amend
  • git commit --fixup

Both are equally acceptable.

The latter is slightly easier but requires an extra step at the end (see below).

...

Either:

  • git rebase -i then edit separate commits at the point of the reviewer's comment.
    In Emacs, use magit-commit-instant-fixup.
  • git commit --fixup

The latter is slightly easier but requires an extra step at the end (see below).

...

Preferably git commit --fixup.

Otherwise, the reviewer tool (e.g reviewable) quickly becomes confused with the amount of combined changes.

...

Prefer git commit --fixup.

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.

...

Squash all fixup commits together, so that only 1 commit per logical change remains.

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 all fixup commits together, so that only 1 commit per logical change remains.

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.

...

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 peer engineers.

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

The command git commit --amend "edits" an existing commit in-place with new changes.

For a single-commit PR, this is the preferred and most simple way to edit the PR during reviews.

When there are multiple commits in the PR already, git commit --amend only edits the latest commit in the sequence.

To edit an earlier commit, proceed as follows:

  1. run git rebase -i and give the branch base SHA as argument.
  2. Mark the commit(s) you want to amend with "edit" in the first column
  3. Save the edit plan. Git will present you the commits to edit one by one.
  4. For each commit to amend, make the changes you wish. Then check the resulting code builds and passes its unit tests.
  5. Run git add on the desired files, then git commit --amend; polish the commit message for that commit.
  6. Once you finish editing each commit, run git rebase --continue to go to the next commit in the list you've selected at step 2.

After you are done editing all commits, the branch has been rewritten and you can push it to the PR again.

Note that for multi-commit PRs, amending commits in this way is more error-prone. Consider fixup commits (next section) if unsure.

Fix-up commits

The command git commit --fixup   takes the SHA of another commit on the command line, which you intend to amend/modify. Then it creates an additional commit at the end of the existing PR, with only the latest change included with git add. The target commit you want to amend/modify is not yet modified at that point.

You can add multiple fix-up commits to a PR, one per commit that needs to be changed based on reviewer comments.

Then you can push the fix-up commits so the reviewer can look at them. Through the review, you can add more fix-up commits iteratively in this way.

This solution is simpler because it does not require rewriting the entire PR branch every review round, and simplifies the display of changes inside the review tool.

After multiple round of review, a PR with fix-up commits can look like this:

Code Block
ba30a29 my first change
deaf012 my second change
55301bf fixup! my first change
900b0c1 fixup! my second change
9bbbd35 fixup! my second change
4c56fb3 fixup! my first change

Then, at the end of the review, when the reviewer has approved the changes, proceed as follows:

  1. run the following command: git rebase -i --autosquash ... (give the branch base SHA as argument). The argument --autosquash recognizes all the fixup! markers and re-organizes the entire history so that the fixup commits are in the right order and "squashed" into the commit they are amending.
  2. Check the proposed ordering, and add edit markers to the commits that have fix-up commits attached to them.
  3. Save the rebase plan and let git run it.
  4. For each commit with fix-up commits, check:
    • that the source code builds and passes its unit tests
    • run git commit --amend and polish the commit message so it is explanatory, accurate and contains the relevant release notes

Once all the commits have been squashed, you can push the resulting changes to the PR for final approval and for mergingSee Decomposing changes and updating PRs during reviews for details.