Versions Compared

Key

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

...

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:

  • git commit --amend

  • git commit --fixup

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

Suggestions:

  • 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.

What to do if the review has more iterations (e.g. more than 5)

Same as above.

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.

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.

...

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

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.

...

Once all the fixups have been squashed, you can push the resulting changes to the PR for final approval and for merging.

How to split amends/fix-ups across multiple commits

Say, you have a multi-commit PR, and you have started editing multiple files, making changes that really belong to separate commits inside your PR. How to make this work easily?

...

(To completely cancel part of a change in afile, i.e. revert the code on disk to what was committed earlier, use git checkout -p)

Alternate commands to amend and fix-up PRs

The specific git commands suggested above are sometimes automated by code editors, IDEs, or command-line aliases. The instructions above are informative suggestions more than prescription. We welcome additional approaches.

...