There are lots of opinions on how to group changes in your git history. I think most people would agree that a commit should contain a group of changes which logically belong together. However, deciding what merits being in the same commit could be a matter of contention. I try to follow one simple rule — do not break git-bisect.
According to the git-bisect documentation:
This command uses a binary search algorithm to find which commit in your project’s history introduced a bug. You use it by first telling it a “bad” commit that is known to contain the bug, and a “good” commit that is known to be before the bug was introduced. Then git bisect picks a commit between those two endpoints and asks you whether the selected commit is “good” or “bad”. It continues narrowing down the range until it finds the exact commit that introduced the change.
If I’m honest, I rarely actually use bisect, but I like this mental model because it forces me to ask myself if the code will still work as advertised when it’s checked out at this specific commit. If this is something that could get flagged by bisect as broken code, then I want to re-think what I’ve included in this set of changes.
It mostly comes down to “if a change breaks something, then the fix should also belong in the change”.
Compass by Walt Stoneburner is licensed under CC BY 2.0.
Why This Matters#
Even if you never bisect, the knock-on effects of thinking about your commits in this way can be helpful both to you and others.
- Code reviewers can follow your code without having to keep a mental checklist of “This change breaks ‘A’ – I’ll watch the subsequent commits to see where ‘A’ gets fixed again”
- Future debugging becomes easier because you can see all relevant changes in the same set
- CI failures become harder to dismiss with “we were never expecting this commit to work 100%”
- Your commit history becomes a provably linear set of working changes rather than an aspirational attempt at correctness
The Common Anti-Pattern#
Let’s examine a common workflow for fixing a bug:
- Write a test which demonstrates the bug (
commit a) - Fix the bug (
commit b)
The beauty of this approach is that you can demonstrate that feature x was broken at commit a, but is clearly fixed by commit b.
The problem is that you have now introduced a point in your history where things don’t work. Anyone who lands at this point in future will need to clarify if you have truly introduced a bug and whether that bug has been fixed in a subsequent commit. This slows down code exploration, possibly making a tedious task even more unpleasant. So, while the intent of this approach is clearly one of good faith, the side effects can be problematic.
For me, I might produce the two commits as part of my workflow, but I would squash them together before shipping my code for review. If you do want to have the two unique commits, I’d suggest wrapping the failing test in a SKIP or TODO or FIXME block or whatever your testing framework supports. Remove the skip/todo/fixme in commit b as a demonstration that your bug has been fixed. That makes your intentions clear while not introducing new test failures into the commit history.
One reason it’s important to regulate this yourself is that your CI may not catch the commit with the broken test. If you wait until you have commits A and B ready before you ship to CI, then your CI will likely test only the last commit rather than both. You’ll be blissfully unaware that you’ve broken git-bisect. Unless your code reviewer runs the tests on all of your commits, they’ll also be none the wiser.
Making Review Easier, Not Harder#
Does this methodology make reviewing commits more difficult? I don’t think so. If all of the necessary changes are in the same commit, then there should be less cognitive load placed on the reviewer. However, this is not a rule, but rather an approach to consider. If you’re staring a monster commit in the face, do what you need to in order to untangle it. git-bisect may not be the hill you want to die on.
While this approach can certainly make for larger commits, hopefully you’re dealing with this via well commented code or well commented commit messages. If your code is self-documenting, then congratulations on being a mythical creature whose code requires no further explanation.
Even if you don’t buy into the whole git-bisect approach, this can still serve as a helpful way to think about your code. Keep the things together that are required to prevent new broken tests from being introduced.
There will be exceptions to this rule, of course. Maybe you are vendoring third-party dependencies. A massive update of node modules has broken one of your tests. Your changes are harder to spot in the thousands of lines of JSON changes. This might be one of those times where you have one commit consisting solely of npm updates and a following commit (or more) to fix breaking changes. I treat the git-bisect approach not as a rule, but as a guideline. Use it when it makes your code better, but not when it makes your code worse.
Related posts: