Updating PRs during review

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.

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)

	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.

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

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, 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 pop if you stashed some changes above). Then check the resulting code builds and passes its unit tests.
Run git add -p on the desired changes, then git commit --amend; polish the commit message for that commit.
Stash the rest.
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. 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.

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.

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 current branch, with only the latest changes included with git add -p. The original 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.

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/branch with fix-up commits can look like this:

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, you may proceed as follows:

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.
Check the proposed ordering, and add edit markers to the commits that have fix-up commits attached to them.
Save the rebase plan and let git run 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=something to automate the check during the rebase.)
- run git commit --amend and 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.

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?

The magic tool is git add -p (the -p argument). It makes it possible to add individual diffs within a file to the current git index, so that you can commit only part of the changes to a file. Pulling a change back from the index into the workspace is via git reset -p.

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

That said, reusing the common suggestions makes it easier to request help from a co-worker who shares that experience already.

In general, any approach is fine as long as it follows the PR organization philosophy described at the top, and is mindful of the reviewer’s time during review.