> 2) You can commit to main/master if it’s a reasonably small change not needing review
I earnestly do not believe there exists any change small enough to not need a review.
I earnestly do not believe there exists any change small enough to not need a review.
Prior massive change of mine went through pre-merge CI tests just fine, but the post-merge build process blew up because a very large list of possible arches was missing aarch64. For various reasons the pre-merge CI tests (which take minutes) are necessarily more limited than the post-merge (which take hours).
A massive list of labels gets one more label so there's no style cleanup concerns, and our CI is already red so its not getting any redder.
The prior real PR was reviewed and everyone missed the label that wasn't there.
Turns out the fairly massive CI test that I wrote caught the bug that everyone's human eyeballs entirely failed to catch (and I doubt anyone who reviewed it actually went through the code in the CI test in the same level of detail that I went through in order to create it).
I'll repeat that there's not a single code change I can think of, including comments and documentation, that can safely be merged without a review from a second set of eyes. This is exponentially true the more engineers work on the codebase. I can maybe understand why someone on a team of 3 or less doesn't see the benefit in having the overhead of mandatory code reviews.
A minimum of two sets of eyeballs on every change. CI cannot detect intentional backdoors being introduced.
I've seen plenty of subtle bugs get introduced by someone who has an overly simplistic view of some part of a system. And they expose a simple method to share their simplified view of some part to the world. "I believe you when you say that in all of your tests this array has a length of 1. This is a failure of your test cases. Don't add a getter method which returns arr[0]. Come with me and lets chat in front of a whiteboard."
The log4j bug might have been caught with more eyeballs. "Here's a small patch which adds JNDI support in log messages" -> "Whoa hold on - what are the implications of that? JNDI is complex". But of course, most opensource code can't afford to spend developer time on code review by multiple people.
If you’ve never found yourself in the position where you slipped up and accidentally included one of the following in a commit that you didn’t notice until after you pushed, I envy your attention to detail:
- a line of code commented out to bypass it when running locally
- a non passing test
- a disabled test
- a config change to point to local host instead of a real server
- a developer credential
- a change to a package lock file
- some build output that should have been gitignored
Yes, you should diligently check what you will push before you push; but since git won’t stop you pushing any of these things, and when you are making a ‘quick one line change’ is precisely when your guard is down because you don’t think you could possibly be accidentally about to ship one of these things, these things will get pushed to master if you allow pushes to master.
* A bigger text size might break the layout or the styleguide conventions
* Small style changes might break a11y
* Your branch might be messed up, causing you to push a bunch of unrelated changes.
Just a few reasons i can think of, I'm sure there are more.