Tidy Commit History
part of Coding Style
Please try to leave the Git history tidy and readable. This helps with git bisect, as well as basic human readability.
GOOD: tidy commit history, with both single-commit and multi-commit PRs:
BAD: tangled commit history, due to merge commits:
Some PRs wind up with many tiny commits as feedback issues are addressed. Please consider rewriting or squashing the commits together so the result tells a better story. It's fine to wind up with multiple commits in a PR, and sometimes this is far more readable than if they were all squashed into a single commit (i.e. one commit moves some files around or performs non-behavior-changing refactoring, then the second commit makes the real behavioral changes). Having lots of tiny "fixed typo" commits is noisy and distracting for the later reader. My (warner) practice is to address feedback on the branch in-place with new (small) commits, then once approval is received, squash the small commits into the others until the sequence is easy to read, force-push the branch and allow CI to check my work, then land. The code tree on my squashed branch should be exactly the same as the one approved by reviewers.
In a straight-line sequence of commits, each one should move the tree from one functional coherent state to the next (all tests should pass on every commit). When merges occur, the left-most parent of each merge should move the tree from one functional coherent state to the next. It's ok for the changes on the right-hand side to fail tests, e.g. if one commit introduces a new test, and the second commit fixes the bug it revealed.
If a PR is just a single commit, please rebase it to current trunk before landing, to produce a linear commit history. The GitHub squash or rebase Merge PR buttons will do this as as side-effect. If the branch is up to date with trunk, the merge commit button will produce a new merge commit (just like git merge --no-ff), which is slightly messy, but not the end of the world. If the branch is not up to date, the "merge commit" will produce a tangled history as it merges the newer trunk commits into the out-of-date branch.
If a PR involves multiple commits, it is even more important to rebase first, because the GitHub squash Merge PR button will change the shape of your branch (losing the distinct commits you carefully crafted), and the rebase mode will start the new commits in the right place but won't create a merge commit at the end, making it look like each commit was an independent change, not part of a larger PR. The merge commit mode will produce a particularly tangled branch.
The GitHub PR page offers an Update branch button when your branch is not up to date. Pushing this creates a merge commit with the current trunk as the left-hand parent, and your branch as the right-hand. This is fine to let CI test the updated code, but the result can get tangled if landed as-is, especially if the Update branch button is used multiple times. When the PR only contains a single commit, landing it with squash or rebase will take care of everything. But if it contains multiple commits, the result will be ugly.
If your PR contains multiple commits and is not up-to-date with trunk, please run git rebase master locally, which should collapse the history into just the commits you care about, removing all the spurious merges. Then force-push the result to your branch and let CI execute. Once CI is happy and your multi-commit branch is up-to-date, use the GitHub merge commit mode to land the PR.
Stacking pull requests is a good technique for building a sequence of separately-reviewable pull requests that build upon each other, minimizing merge conflicts among your own changes. Tools for restacking branches exist that make it easier to update multiple pull requests as you apply review feedback to prior changes.
category: Coding Style