Yeah sometimes you just have to write a megapatch. I’d be very sad if I had to work on a project that didn’t allow that.
Unless it's the name of an HTTP header?
I know that Phillip Hallam-Baker probably first used that spelling in a document
In general, the large, sprawling diffs we're talking about here, the sort that actually justify commits of this size, are almost always going to also be the sort that justify more closer scrutiny at review time.
Good tests, a great fuzzer, and a highly motivated security research community - that’s how you find the great security bugs!
It's also immensely difficult to write tests to find errors in code you're writing - most patches I see with "extensive tests" test all the correct code paths, and _maybe_ a few error cases. It's a very easy trap to get yourself into.
The purpose of review is not to catch obvious issues - though it hopefully will - but the subtle ones.
It makes no difference for finding actual bugs.
Reviews are great for spreading knowledge about the change and making sure the style is dialed in. Reviews aren’t great for ensuring test coverage or finding security bugs.
You are right that reviews aren't necessarily there to catch bugs. Reviews are there to tell you to write code that's simple and understandable. If your coworkers can't understand what's going on, how could you expect anyone else to do so in a year or so?
int size = 0;
char *buffer;
if (a) {
buffer = new char[1];
size = 1;
} else if (b) {
buffer = new char[2];
size = 2;
}
...
if (b) {
buffer[1]++;
}
...
Code review can help you prod you to find ways to simplify your code, so that you have fewer paths to consider.
Also it's often unit tests only, with no hint of any integration tests. I'd settle for a tests that check integration of at least two units, but alas.
Then the mega patch PR is " one last look ", and not thousands of changes storming the gates of your sanity.
To prevent bugs you need tests, fuzzing, static analysis, static & strong type checking, etc.
how do you effectively manage them in the review process
For a big impactful change discuss the work in progress beforehand (specifically: architecture stuff, database changes, impact this will have on other code) with them as much as possible before final review time. There's no other sane way to do it.
Make sure you're all in agreement on the approach you're taking or, at least, make sure they're aware of it. Also a good way to (hopefully) make sure you're not going to conflict with somebody else's work!
Dumping a big commit full of surprises on reviewers is AT BEST poor practice and is at worst kind of shady and a sign you don't want to collab, don't respect your coworkers and/or or are hiding something.
We missed an OS update cycle and had to consume all the changes in one giant patch. But upstream had migrated from Python 2 to 3 in-between and rewrote half their build scripts to accommodate. Each of those changes needed to be reviewed manually because we had modified things to support reproducible builds and the resulting merge conflict was monstrous. I contributed maybe 2300 of those before my sanity failed and called for help.
Now we have a whole team to do that job more regularly.