Small diffs are wasting everyone’s time: the proper diff size is the size of the change where context is available to understand it.
I really want to agree with this, it feels right.
But: no data.
Questions: Do we know for a fact that larger diffs introduce more bugs than smaller diffs? Do we know for a fact that there’s such a thing as “review fatigue”, as opposed to just regular can’t-focus-for-too-long-at-a-time fagitue? If so, why not include links to articles about those things?
I instinctively agree with this, but I can’t guarantee I’m right (even though I would love to. Being right is great.)
I mostly agree with this idea, especially coming from a previous company where I would regularly receive PRs in the 2-5K LoC range, and was expected to review it within minutes, because startups.
However, I find the author’s interchangeable use of the word “diff” to be a bit misleading in some cases. For example:
Imagine you’ve got a large diff with four logical changes in it. If any one of those changes breaks something you now have to rollback the entire diff.
This has never really been my experience, unless by “diff”, the author means “commit”. Because in the least, even if you have a massive set of changes in a “diff”, you can almost certainly break it down by commit, and at least sometimes fix bugs by rolling back at that level of granularity.
[Comment removed by author]
I’d love a “compare and contrast” with squash merges + branch cleanups. It does make the history lighter and cleaner, but I don’t think I want that.
Just hold a meeting and explain what’s happening with your reviewers? Then assign different people to do a “fine comb pass”?
I think this is only relevant in a context where this type of effort is required to get an offline review, notably open source where people aren’t being paid to be responsible. If I work with you and you refuse to attend a review meeting for a large improvement, we’ll have a problem.
That probably shouldn’t be the norm though, right? If every diff requires an in-person, synchronous meeting between the writer and the reviewers, isn’t it going to take forever to ship each diff? Isn’t it better, when possible, to just make your diffs easy to review so they don’t require a full fledge meeting?
In my experience, it’s pretty rare for a large improvement to be so tightly coupled as to not be able to be broken into a couple smaller improvements.
There’s of course a tradeoff , since more reviews takes more time. But, inversely, single-issue reviews take much less time than multi-issue ones. In a multi-issue review, you not only have to consider each issue, but the interactions between each issue.
I’m a major fan of tiny reviews though, so a bit biased.
There’s no way I’m spotting a potential Null Pointer Exception when all my brain wants to do is make the pain stop.
And there we have the nub.
We are writing in programming languages that require human review to stop elementary stupid things from happening, and these languages lack the static analysis tools for finding stupid, and we have insufficient tests.
The answer is clear.
Stop doing that.
On the flip side, I’m very happily using mercurial evolution and I sort each of my changesets into “this does one small thing, and takes the code from compiles and works to something better and compiles and works”.
A tool that is indispensible for me when working with long chains of patches in git is stgit. It replaces git rebase -i for me when I want to make amends to a patch further up the chain. It is typically used for distributing patches via mailing lists, but treating branches as stacks you can push onto and pop from fits well with my mental model of git.
git rebase -i