It is safe to say that smaller commits is a perennial topic at Mozilla.
On the reviewer side, I definitely prefer smaller commits.
I find it’s easier — and faster — to review them. Often not in the neighborhood of microcommits, but at least 300-400 line patches.
For the thing I work on we use GitHub, and PRs seem to work best on small things. When tasked with a large task, it seems that many engineers will break it up into suitably small PRs and things work well.
However sometimes tasks are more complicated than you’d imagine.
Bug 1437646 seemed simple — let’s just log JSON to standard out. But logging json from apache is hard (for a variety of reasons outside the scope of this post).
At first I was able to split this into two tasks:
1. add a log server that listens on localhost:5880 and serializes all lines sent to it to stdout.
2. (finally) implement Log::Log4perl
However Bugzilla is not simple; it does not yield easily to such changes.
We need a place to keep the log4perl config file(s), and the wrapper I wrote to start apache in docker isn’t smart enough to wait until our socket-logging-server is running. So what was planned as roughly two tasks resulted in two relatively large PRs.
So the problem is — how can I make this easier to review, using the tools available? In the case my own discomfort is an an acceptable trade-off if it means my code can be reviewed more easily.
Below is what I came up with — and it’s immediately useful though far from perfect.
- Work out a series of commits (microcommits or at least smaller ones)
- for each commit, create a branch. This means for 4 commits you have 4 branches. For the examples below we’ll use feature/1, feature/2, feature/3, and feature/4. Imagine that “feature” is a more descriptive word.
- For feature/1, make a PR that would merge it into feature/0-reviewed
- Make a branch that is identical to feature/1, called feature/1-reviewed
- For feature/2, make a PR that merge it into feature/1-reviewed
- Continue this until the end. Every feature/N will merge into feature/N-reviewed, and feature/N-reviewed will be equal to feature/(N -1).
As a result of doing this, there will be 4 PRs that each only show the differences of the commits in question. After they’re all reviewed, it will still be required to cherry-pick each of the reviewed commits into a final branch and make a PR out of that, but getting that (rather larger) PR approved
can be a less involved process than code review — perhaps even the responsibility of a bot?
I’ve got some shell scripts to accomplish this, so I’m going to try it a bit and see what the results are.