Branches and PRs: institutionalised distrust?
jimkinsey.com
jimkinsey.com
It's incredibly freeing to be able to say "This branch is treated as a sandbox. The code is in flux, history is mutable." and "This (master) branch is stable. Every single commit in it is good, passes all known tests and linting, should be safe to rollback to, should be amenable to `git bisect`".
When developing on a team, even if I can push directly to master, I usually shouldn't and won't. I want a second pair of eyes on that code.
Does this "process-heavy" workflow mean I distrust myself? Sure. I expect that I am human, make mistakes and oversights. I also expect that my coworkers are human, and I'm not going to trust their solo efforts any more than my own.
If branches and PRs are institutionalized distrust, well, remember that a healthy amount of distrust is good for you.
Personally I have stopped seeing branches as part of a distrust system and PR's being some sort of gate-keeping/control mechanism. And more and more see them as a healthy self caution.
I no longer see making a PR as something for others to judge my work and disapprove of it, but more as an opening for collaboration and discussion. More of a friendly "can we look at this together so we can make it even better", as opposed to an authoritarian "I've done the best I can, I hope it meets all requirements and I don't get complaints". Of course this depends on how people view it at the other end of the line as well, and some people never change. But overall having this look at things made my life as a (open source) developer a lot less stressful.
It's not about distrust - it's about making systems reliable. People make mistakes. Having things branched out from master means that those mistakes aren't as damaging.
Using other branches does not require you to institute onerous restrictions. I don't understand what this author is arguing against - it reminds me of a comment on another recent submission:
> In his book, The Checklist Manifesto, Gawande reports that at the end of a big checklist pilot project for the World Health Organization (IIRC), participating surgeons were surveyed.
> • One of the survey questions was, in essence: Would the use of checklists improve your surgical practice? Most of the surgeons responded no. This was consistent with the resistance that Gawande encountered: I don't need no stinking checklist; I'm a good surgeon!
> • But the more-telling survey question was this: Would you want another surgeon to use a checklist if operating on you or one of your family members? Something like 93% of the surgeons surveyed responded ... yes.
First, branching/PRs are not automatically process-heavy. If it is for your company, there's probably something else wrong.
Second, this process—if done right—dramatically INCREASES code quality and DECREASES defects. This can be backed up with data. Code reviews, if they're not rubber stamps, are extremely valuable.
Third, the trunk-based model described in the article often leads to big-bang release branches. Master should be gold. This facilitates rapid and small releases, which directly impacts the business. This can also be backed up by data (e.g. see the Accelerate book).
Fourth, this model (again if done right), plays an instrumental role in compliance. The article doesn't even mention compliance, which is a HUGE thing bogging down a lot of companies trying to adopt DevOps practices right now. It has nothing to do with discipline or trust.
I have worked at places where one person should never have the ability to push unreviewed code into master because the company was huge and already successful and it'd be a security risk.
I have also worked with coworkers who fetishized the process part of stuff and wanted to create all sorts of rules with high overhead to solve entirely hypothetical problems from their imagination.
The hard part is differentiating the two.
I worked with such a workflow before and it worked like a charm. You only ask for a third-party approval when you know the feature you worked on is big and might break something. It is all about trust.
The pull request interface is very good to show you all the code you worked on and review everything before committing to master.
I find that trunk based works extremely well combined with pairing and/or ci/CD, it's the fastest way to deliver value if you have the checks in place (mostly good tests and review through pairing).
* Catching errors more quickly.
* Increasing the bus factor my having each bit of code read by at least two developers.
* Getting everybody on the same page, both by disseminating information, and by holding each other to a consistent coding style.
Sure, you can do coordination offline as an asynchronous thing. But making it a blocking part of the commit process just gives you so much higher confidence that it's going to actually happen.
Though really, you're even better off if, as much as possible, you do the coordination and communication before the code review. If you have agreement on basic approach before then, you can reduce the amount of last-minute stuff to minor style changes, fixing errors, and cleanup.
Code review, as a business process, tends to push things this direction. If you don't do reviews, you might skip coordination and communication. If you do reviews, you know it's happening at some point, and it only takes a few bad experiences of having to redo all your work to clue in that maybe those discussions (which you are now definitely having) should be started earlier.
> To me the latter issue is much bigger. If there isn't trust and discipline within a team, then that team is unlikely to be highly effective and may even slide into dysfunctionality.
I trust coworkers' discipline 95% of the time, but everyone makes mistakes. Automated testing and code reviews catch bugs, and allow codebases to scale from tiny teams to large teams.
(emphasis mine) If you're making "well-crafted" commits, you're staging them somewhere. If it's not on a branch, then it's not sharable - but it's still HAPPENING.
I mainly use it to test if features meet their functional description before merging, but also reviewing the code at the same time does help to spot subtle misunderstandings in the requirements.
I also disagree that it would hold back development of inexperienced members of the team. Having the structure with PRs allows them to work on things that they otherwise may not have been trusted with yet to work on at all.
> building a process into a team which signifies a lack of trust is unlikely to ever improve matters
This is one of those "I can't help fix problems because you're just hopeless if you have problems, but I can make perfect better" articles that come up time to time about why you should use X (that nobody does in practice, or not for very long).
This is a really great insight, and I'm glad I read the article twice, because on the whole, I think it's a bit hysterical.
Expanding brain: give up distributed version control entirely.
Galaxy brain: write directly to the deployment server.
Right now, I'm on a team of 3. 2 embedded devs and 1 web dev. The web dev keeps asking me to PR their web framework changes, but I'm not a web guy. The other embedded dev has ADHD and can only rubberstamp my reviews. Basically I'm just wasting time complying with the branch/PR/merge/tag cycle.
1) Don't look at the PR diff as a whole, look at individual commits, and judge each one, then read the PR diff.
2) If the commits are too big to keep in your head, don't make individual sense, etc., ask your coworker to rewrite their history until you can do 1)
2b) If the individual commits make sense, but the PR still is too big to comprehend, this PR should probably be split into multiple smaller PRs which can be reviewed, approved, and released individually (or at least in sequence).
3) If you lack still lack enough context to make a good review, ask your coworker to walk you through the commit, and just explain out loud why they're making the changes they're making. Sometimes, a "rubber-duck code review" is enough. Even if it's not, any programmer can read and understand a single line of code at a time, and you can likely tell if that line of code is complex or elegant.
Plus, even if you don't know the full technical context of a proposed change, people are fairly good at picking up whether another person has an explanation that makes sense, or if they seem to be making it up as they go.
4) Spend the time and do the reviews, and become more familiar with that part of the code. It sounds like right now, that part of the code has a bus factor of 1.
5) Track time spent doing code reviews, and track bugs caught during code review. Code review has notable benefits for your team/company, so justify and sell that. You have fewer bugs & a better bus factor. When estimating your next sprint (or other unit of work), know that you'll spend X hours per week doing code review, and integrate that.