Are Pull Requests Holding Back Your Team?
Pull requests are great for open source. But they can hinder the team performance
Intro
The rise of Git, GitHub, and Pull Requests (PR) has resulted in some big changes to the practices and workflows within the software industry. In particular, they’ve revolutionised the world of open source, providing a robust mechanism that allows and encourages strangers to contribute to projects.
The branch and pull request workflow has also been adopted by swathes of commercial software teams, and it seems to have become the de-facto standard of development workflows. But as a user of this workflow for a few years myself, I’m starting to question the following:
- Has the commercial software industry unconsciously acquired feature branching simply as a means of adopting pull requests?
- If so, are we forgoing the benefits of “real” Continuous Integration (CI) in order to accommodate pull requests?
- Are pull requests even a worthwhile endeavour for commercial software teams?
Why We Wanted Pull Requests
Pull requests have an obvious appeal. They provide a really convenient view of new code side by side with the current code. Surely there is no better mechanism for reviewing code?* It also allows inline comments to be added, prompting discussions to take place — that can only be a good thing, right?*
Modern Git-based systems (such as Bitbucket) also provide the ability to set up merge checks against pull requests, requiring a successful build before it can be merged. You can also configure your build server to pull the mainline branch and locally merge it with the pull request branch. Building and executing tests against the result of merge gives you early visibility on whether the two versions are compatible.
These features are desirable and feel in tune with Lean Thinking. By validating changes before they are even merged, we’re building quality in early, aren’t we?*
*Loaded questions
I Got Sucked In
For the reasons above, and if I’m being really honest, possibly because GitHub and PRs were the trendy “in thing,” I’ve been an advocate for introducing PRs. Their sheer popularity led me to assume that they must be beneficial because the industry as a whole was adopting them. I may have even judged other organisations/teams as being “amateur” if they weren’t using them.
But after spending a few years using this approach, and acquiring newfound aspirations of achieving Continuous Delivery, I’ve started to realise that they come with serious drawbacks and maybe that they’re not actually beneficial in commercial teams.
If You’re Using Pull Requests, You Probably Aren’t Doing Continuous Integration
The software industry suffers badly from Cargo Cults. Organisations and individuals are often looking so hard for those Silver Bullets that they convince themselves they will magically solve all their problems. But alas, time often isn’t taken to really understand the rationale nor applicability of said bullet. The ultimate example of this is probably “Agile™”; if you’re doing stand-ups and sprints you’re Agile™, right? Microservices is a more recent example, and I think Continuous Integration may be another. The term “CICD” for instance is bandied about frequently but I feel the “CI” part, in particular, has lost its meaning.
Continuous Integration means something very specific. It originates from Kent Beck and is one of the 12 practices of his Extreme Programming methodology. Martin Fowler has documented the practice here.
I think the most fundamental rule is:
Everyone commits to the mainline every day.
This rule is so fundamental to the practice you can reconcile it with the name:
Continuous — Every day (the when)
Integration — The mainline branch (the where)
So in a team that uses pull requests, unless everyone in your team is merging at least one pull request every day, can you really claim to be doing Continuous Integration?
In my experience, despite the intention of using short-lived branches, teams using the pull request workflow unconsciously fall into the feature branch strategy where developers work on tickets in isolation, for at least a few days at a time, before they get reviewed and merged/integrated. But strictly speaking, that isn’t Continuous Integration. Steve Smith gave a great talk on this subject: The Death of Continuous Integration.
No Dogma Here, BUT….
I should point out that I am absolutely against following rules in a dogmatic fashion. Teams should experiment and adopt a workflow that works for them. Not adopting Continuous Integration in its “original” or “pure” form may be a justified and conscious choice for a given team. Plus, you can still get some of the benefits of CI despite using pull requests.
But I do think that teams using pull requests are unlikely to be abiding by this fundamental rule, and therefore compromising what makes CI so powerful; the sociotechnical system it produces. If practised as originally intended, CI helps to optimise the behaviour of team members to favour performance of the team as a whole, as opposed to optimising for the individual.
Feature Branches vs. Trunk-Based Development
You’re probably thinking that my argument so far is not really about pull requests but rather branching strategies, i.e., feature branching vs trunk-based development. That is true; it is the shift towards feature branching that compromises CI rather than pull requests themselves, but…
Do Pull Requests Encourage Long-Lived Branches?
(long-lived = more than a day)
Reflecting on my personal experience of using pull requests the past few years, I’d say they have been a contributing factor to working in a separate branch for more than a day — often more than three days. I think the workflow has encouraged me to get everything done in one hit: a nice, big, fat squashed atomic commit that encompasses the entirety of a sometimes sizable piece of functionality.
My personal practices are obviously not reflective of our industry as a whole, but I think that the use of PRs naturally pushes developers to work in isolation for days. So I have a hunch that teams that use PRs will correlate with teams using long-lived branches.
Long-Lived Branches = Lower Team Performance
Whilst I have no evidence for this correlation, by deduction of this hypothesis, teams raising PRs are unlikely to be categorised as a high-performing team. This assertion of linking teams using longer running branches with lower performance isn’t my opinion; this is proven by the research and data produced in the State Of DevOps reports (SODR) and presented in the fantastic book Accelerate:
“Our research also found that developing off trunk/master rather than long-lived feature branches was correlated with higher delivery performance.”
— Accelerate, Forsgren et al., 2018
It would be interesting if they surveyed teams in the next report to see if there is a direct correlation between PRs and team performance.
By the way, the categorisation of what makes a “high performing” team isn’t just a perception/opinion. The authors of Accelerate clearly define the characteristics of what makes a high performing team:
- Deployment Frequency — On demand (multiple times per day)
- Lead Time for Changes — Less than one hour
- Mean Time to Recovery — Less than one hour
- Change Failure Rates — 0–15%
In addition to my hunch that PRs lead to longer branches, I think there are other aspects to PRs that will hinder both the performance and culture of a team.
Pull Requests Inadvertently Create an Environment of Distrust
Pull requests were explicitly designed to create a workflow to handle distrust. They are a gate-keeping mechanism for onboarding code from strangers. So it seems strange to introduce such a mechanism within what should be a close-knit team. Do we really need a process that treats our team members the same way we’d treat strangers?
The most efficient and successful (not to mention happiest) teams I’ve worked in have been the teams with a high degree of trust and a culture of psychological safety, and we didn’t use PRs.
I’ve come to realise that PRs can create a barrier to achieving a culture of trust and psychological safety, which the SODR research concludes is a contributor to performance:
“A culture of psychological safety contributes to SDO performance, organizational performance, and productivity, showing that growing and fostering a healthy culture reaps benefits for organizations and individuals.”
— State of DevOps Report 2019
One of my loaded questions above suggested that the ability to write inline comments was obviously a good thing, and whilst I have had positive discussions using this medium, I’ve seen it work negatively. Text comments can easily be taken the wrong way — judgemental rather than constructive, a public shaming rather than an open team discussion. I’ve witnessed PR comments being fuel for tension and mistrust that I believe wouldn’t have been the case if communicated via a different medium, such as face to face or in a private IM. Obviously, this depends on your team, but if there are soured relationships within it, I don’t think the PR workflow is likely to help build bridges.
This tweet really resonated with me when talking about the issue of trust and PRs:
“In an environment where high trust is possible, adopting processes that require trust will help create and retain it.” — Giles Edwards-Alexander
Continuous Integration (in its original form) requires a degree of trust. Even if a team starts without trust, this process could help create it. By contrast, this is unlikely with PRs — it’s a process to mitigate a lack of trust and is therefore unlikely to encourage or develop it.
An interesting observation is that we as software teams want empowerment, autonomy, and trust to be bestowed upon us from management, but from the outside looking in, it could be deduced that we don’t appear to trust each other!
Pull Requests Aren’t Actually That Good for Code Reviews
Another loaded question above asked whether there was any better way to do a code review other than PRs. I think there is.
Pull requests are very good for quickly eyeballing code and spotting obvious mistakes, but I’m not convinced they can be relied upon for consistent in-depth scrutiny. I think it’s likely that reviewers regularly only engage in System 1 thinking rather than the necessary System 2. And I confess: I am guilty of this. It typically happens when I’m busy trying to get something done before the end of the sprint and someone wants me to review their PR.
In this situation, you need to fight against your instincts of prioritising your own work over the team’s collective need — which is for you to stop what you’re doing and diligently scrutinise your colleagues' work. Of course, the level of diligence/scrutiny in PR reviews will vary on the level of discipline in your team, but everyone is susceptible to human nature. If you’re under pressure you’re not going to be as diligent.
When reviewers aren’t exercising diligence, PR approval becomes somewhat of a pointless tick-box exercise. Comparisons can be drawn with a bad Change Approval Board (CAB) process, something which Jez Humble labels as “Change Theatre.”
Avoidance of Accountability
I wonder whether PRs encourage developers to feel a false sense of security, allowing us to subconsciously think we can avoid a level of accountability by shifting it onto the reviewer.
For instance, is it possible that the PR workflow encourages a thought process such as: “Well if there are some issues with this code, the reviewer will pick them up.” or, perhaps worse still, “…and if they don’t pick them up, at least they’ll share the blame because they approved the changes.”
I realise this sounds cynical, and I’m not suggesting developers necessarily have these thoughts literally or consciously, but could the nature of PRs play negatively on human nature in this way? I wonder if I myself have manifested this type of behaviour when under pressure to deliver something — probably.
This (perhaps unconscious) mindset has obvious problems. Firstly, there is no safety net if reviewers are only engaging System 1 thinking and approving as a tick-box exercise. Second, and perhaps more pertinent in terms of team culture, this attitude puts a burden on reviewers to take responsibility for the other person’s changes.
Whilst software development is a team sport and the team as a whole should take responsibility for any failures, burdening a single person to review and share accountability for the code disincentives them from picking up the PR to review. Not only that, but it requires a developer who is working on their own stuff to stop what they’re doing and context switch to review the PR — another disincentive.
Because of these disincentives, I’ve found it common practice for PR authors to have to chase down team members for approval, paste PR links into the team chat, and periodically beg for someone to take a look.
Lean Thinking?
It’s reasonable to think that PRs are synonymous with Lean Thinking; we’re eliminating waste and building in quality from the start because we’re checking the code before merging it in. It feels like we’re doing something earlier than we ordinarily would do. But I think this is a false premise. If we were really building quality in early, we’d be reviewing the code as it was being written — not waiting for developers to come out of their period of isolation and then check it.
The PR workflow generates wasted time by the potential of multiple rounds of comments and additional commits (if you’re lucky enough to have a diligent reviewer). Each comment/commit added to the PR fires off an asynchronous message between author and reviewer, and a period of time follows waiting for one of them to reply. During this wait, either of them is likely to go back to working on something else. This means that when a comment/commit response is added to the PR, the same disincentive that prevents the PR from being picked up in the first place is at play: they are required to switch context to go back to the PR. I’ve found that because of this, open PRs can be knocking about for quite a while.
The asynchronous workflow makes complete sense in the open source world as people are often maintaining projects in their spare time. This workflow affords them the ability to evaluate incoming changes at leisure. But in commercial teams, we’re looking for maximum efficiency (not to be confused with resource utilisation). We don’t want to have PRs sitting around stacking up in a WIP queue. Indeed WIP queues are the antithesis of Lean Thinking; WIP is considered waste (ergo, open PRs are waste).
Agile is all about fast feedback cycles — ideally synchronous. Pull requests by their asynchronous nature don’t provide that, so is it too bold to claim they are anti-agile?
Don’t Hold up the Project!
Another negative experience I’ve encountered with PRs is their relationship to projects with outsourced development teams. This is where an outsourced team has raised a (usually huge) PR, and the in-house developers are tasked with reviewing the changes. This feels like a sensible use of PRs as it’s more akin to the open source paradigm — onboarding code from an external source. But the problem arises when due to the aforementioned disincentives, developers who already have pressures to deliver their own team’s work are reluctant to take on the review.
At this point, management gets involved and chases developers to approve the open PR. We now enter the realm of micromanaging.
Things can get even worse if the reviewer starts questioning the work done. The project deadline (for which the outsourced team is on a fixed fee to deliver) is looming, and the reviewer is now seen as the person holding up the project. The outsourced team claims the developer is being overly critical and management applies further pressure on the developer to approve it. This results in resentment all-round and ultimately an unproductive environment. Quality and good business outcomes become a secondary priority to the outsourced team hitting their deadline. To paraphrase from Robert Martin’s The Clean Coder: A Code of Conduct for Professional Programmers: what would a surgeon say to management if they sought to cut corners in surgical procedures in order to save time?
So if Pull Requests Are Bad, What Is Good?
Firstly, I’m not saying that PRs are flat-out bad and can’t work well for your team; maybe your team functions better with them than without. What I am saying is that rather than accepting PRs as best practice just because everyone else uses them, I think it’s worth considering how much value the PR workflow is actually providing and weigh that value up against the costs — some of which I’ve described in this post.
With that caveat out of the way, I am of the opinion that the introduction of pull requests has probably had an overall negative effect on commercial software teams, and limits the potential of good teams to become high performers (in the SODR sense).
I’m not the only one who thinks this. Here’s an example of someone with a similar mindset:
So what’s the alternative? I think that the practice of Continuous Integration (in its intended form) combined with Pair/Mob Programming is likely to yield the best results. The synchronous nature of CI keeps the flow of changes optimised without producing static WIP queues, and the quality of code is naturally higher because the process of continuously reviewing code as it’s being written makes it difficult to not be engaged in System 2 thinking.
Side note: Maybe Pair/Mob programming should be renamed to Continuous Review? CRID, anyone? No?
Admittedly, I don’t have huge experience with Pair Programming and, as of this writing, the current COVID-19 situation makes it all the harder. But even without Pair/Mob Programming — having the entire team pull and run your changes ASAP is probably a more effective form of code review when you consider PRs in terms of ROI. How much value are PRs really returning?
I’ve found it’s usually relatively small things that get pointed out on PRs, things like typos/naming, etc. — in this case, I think it may be more efficient to simply hope developers will notice them after pulling the changes and just fix them on the spot. You might have raised an eyebrow with the word “hope.” Maybe you’re thinking: Surely we should be doing something more robust than “hoping”? But whilst PRs definitely help to put obvious mistakes front and centre, we are still just hoping things get noticed by the reviewer. It’s not guaranteed.
Let’s assume they don’t get noticed. I’m not convinced the costs of PRs are worth it just to have a better chance of getting them noticed. If changes are genuinely problematic then surely some form of test would be failing anyway, and if they aren’t, you have bigger problems to address.
In the case where a PR review uncovers something more significant — perhaps where a developer has misunderstood and produced a large set of misguided changes — it’s tempting to think it’s a victory for the PR process. But rather than viewing this as successful mitigation of bad changes, I consider the PR workflow complicit in wasting the developer’s time.
It has allowed (if not encouraged) the developer to go into isolation mode and write something that is incorrect. If the developer had been following CI and pushed smaller commits daily, it’s likely that their misunderstanding would have been picked up by the rest of the team early on, affording the team the ability to correct course without wasting much time.
Conclusion
I’m now of the opinion that pull requests in a commercial team are generally suboptimal.
Given the negative effects I’ve described in this post, together with the evidence from SODR suggesting long-lived branches and lack of trust affects team performance, my aspiration is to try and convince my team to ditch PRs and go back to working from the same branch with small commits.
I suspect that my suggestion of not using PRs will be met with some raised eyebrows, particularly as PRs are mandated from the “higher-ups” where I currently work. But nonetheless, I no longer think PRs are a worthwhile endeavour.
