...
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.
...
To edit an earlier commit, you may proceed as follows:
Stash your changes (
git stash).Run
git rebase -i origin/master(or the branch base if you’re forking something else than master).Mark the commit(s) you want to amend with "edit" in the first column
Save the edit plan. Git will present you the commits to edit one by one.
For each commit to amend, make the changes you wish (or
git stash popif you stashed some changes above). Then check the resulting code builds and passes its unit tests.Run
git add -pon the desired changes, thengit commit --amend; polish the commit message for that commit.Stash the rest.
Once you finish editing each commit, run
git rebase --continueto go to the next commit in the list you've selected at step 2. Iterate steps 5-8 until completion.
After you are done editing all commits, the branch has been rewritten and you can push it to the PR again.
...
Then, at the end of the review, when the reviewer has approved the changes, you may proceed as follows:
run the following command:
git rebase -i --autosquash ...(give the branch base SHA as argument). The argument--autosquashrecognizes all thefixup!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.Check the proposed ordering, and add
editmarkers to the commits that have fix-up commits attached to them.Save the rebase plan and let
gitrun it.For each commit with fix-up commits, check:
that the source code builds and passes its unit tests (You can use
git rebase -i origin/master --exec make test PKG=something TESTS=somethingto automate the check during the rebase.)run
git commit --amendand polish the commit message so it is explanatory, accurate and contains the relevant release notes
Once all the fixups have been squashed, you can push the resulting changes to the PR for final approval and for merging.
...