Are pull requests bad because they originate from open-source development?
blog.ploeh.dk
blog.ploeh.dk
This guy (not the post author, but Dave Farley, the guy who wrote the above excerpt) has never heard of "trust but verify".
PR is about verifying, having another set of eyes for anythine the original dev might have missed, giving an opinion before something on a branch goes into permanent git history, and so on.
This is completely orthogonal with whether you trust your team members or not. Besides "blind trust" is not the only form of trust.
These days Pull Requests are used to enable the same workflow.
Generally, the lesson I took away from that place is that you can blow $10 million in seed money and end up with worthless code if you don't pay attention to the basics. They were in the right market, at the right time, with the right product, and 3-5 years ahead of the competition, but you can't sell non-functioning software.
My favourite example (of many) was that they used a source control system that could cache Java ".class" files to enable incremental compilation at all times, even without IDE support. This was literally a check-box that someone un-ticked years before I got there. I re-enabled it for myself as a local setting override and compile times went from 30 minutes to 5 seconds. That inner-loop speed-up was just insane, I could fix bugs in minutes that would take the rest of the team days.
Speaking of code review, the code that I saw getting "rubber-stamped" was so bad it was unbelievable. One guy spent months working on multi-threaded animation code that I replaced with a single line of code. Which actually worked, didn't stutter, and didn't leak threads, or use threading at all for that matter.
The essentials matter! Proper code review, well set up tooling, hygienic build pipelines, etc...
Without enforcement, you can make the argument that speed limits don’t save lives because everyone drives as fast as they can anyway.
Same thing for code review. If it’s not policed by management, then it does nothing.
I saw one place that would send alert emails if a commit was approved too fast, assuming that someone must have had it insta-approved by a friend as a favour.
People have to be open to constructive feedback and want to grow, however.
This is interesting as a counterpoint to trusting, but it’s funny, I’ve never thought about PRs in either of those terms, not within a team or company anyway. The Pull Request can also be about the code writer asking for review, asking the question ‘is this generally the right way to go?’ The funny thing about projecting ‘trust’ and ‘verification’ onto the process is that PRs simply do not prevent all or even most accidents, automated builds and tests are better at that. PRs can help steer things in the right direction, and they’re good as one of many tools for mentoring people about code they’re unfamiliar with. But PRs can also be used as the opposite of verification: if the reviewer is responsible for verification then PRs can be a CYA where the reviewer is implicated in a mistake along with the writer.
I know that "my" team members look at PRs as more of a safety net for oopsies than evidence of lack of trust between team members. We allow self approval of PRs so that if/when someone is the only developer working they can still push code if needed.
When I started in the project we did not use PRs and we all agree that they have made the code better and that they have unified our coding style (the fact that Visual Studio/ReSharper auto-formats changed code also helps with that aspect).
I did have a 6 month contract with a fairly tight team back in 2019, and it was definitely overall a positive experience. Mix of experience levels, and they were growing perhaps a bit too fast, but there was enough balance and communication. Most people were on site, which did, I think, help out some. I know "remote only 100%" is everyone's favorite 'go to' mode (mine too) but there's some extra discipline and rigor the whole org needs to have to function smoothly to get the best out of a mostly or fully remote workforce.
I think he has, and actually knows where it comes from and what it means.
"Trust but verify" is a Russian proverb that was used to describe the hoped-for and treaty-mediated relationship between the Soviet Union and the United States during the Reagan-Gorbachev era.
So arch-enemies of a Cold War that are pointing world-ending nuclear arsenals at each other and slowly coming to realise that maybe it would be better to limit those by negotiated treaty.
If that's a good description of the relationships in your team, then I think David Farley wasn't just right, but actually being very diplomatic ;-)
- You are allowed to expense things while traveling, but someone will look at what you put on the company card afterwards.
- They don't wait on your background check before moving ahead with interviews.
- Someone really should look at your code, at some point, before it is released.
1. Continuous deployment means your mainline is sacred and always in a releasable state.
2. Unit tests and automated verifications only tell part of the story
3. Churn should be avoided (e.g. fixing code style issues); if fixes are made before it enters the mainline, it is avoided.
4. Code reviews should be asynchronous, because pulling someone out of their flow to switch to doing a code review is rude and unproductive.
One of the big Taylorist mistakes of our industry IMO. It is OK to have churn, and it's certainly better to have a little too much than too few.
If you want to make those:
* make it in separate bunch of commits * make sure team uses tooling that will not allow new style problems to happen, like mandating certain formatter with certain settings before each commit. Most editors have hooks to do it automatically, and if they don't, Git have them too
It also makes git blame less useful althought it now have options to ignore moved code or whitespace changes so it isn't as much of a deal now.
However, plenty of useful changes also qualify as churn, for instance:
* renaming stuff
* extracting a function to split a block that may have been fine for the author, but that you found hard to read. And yes, sometimes, reviews don't catch all those.
* Add a comment to provide context to an algorithm after you talked with business
All of these things (and others) are pretty valuable, and the cumulative effect of reducing friction against these changes add up to much better codebases, with less places where no one wants to work because of abandonment. This kind of light touches should take a couple minutes while reading your codebase, not half-a-day because you need to fill a PR and wait for someone to be available.
Well if that is true why not just stop there?
If you have to get all expenses approved before they are charged to the card, then it adds much more overhead and planning which decreases efficiency. The amount of fraud that happens by letting someone charge things and verifying it later ends up being a smaller expense than the amount of preplanning.
Similar with interviews. The amount of time wasted by interviews of someone who doesn't pass a background check is a small cost than the cost of doing interviews of everyone before doing a background check (not just the cost of the background check, but having a longer interview process leading to missing out on candidates and more candidates getting part the way through the process and dropping out because another place gave them an offer).
Imagine a father trying to "trust but verify" the paternity of their child. If there has to be verification, it will be taken as a lack of trust.
You are on your way to a conference. You are at the airport and want to buy a coffee and a donut. You make a request... You've been waiting for almost an hour now. Nobody takes a look at and you still need multiple approvals, so you ping Barbara in charge of approvals to get the ball rolling, she says she will take a look after a meeting she is currently attending because she wants to be careful with what she approves. Oh, Tom, her colleague asks you why don't you eat salad and orange juice, it's cheaper and faster. You have a quick call with Tom to explain why you want a coffee and donut. Awesome, you convinced Tom to approve in just under 20 minutes. Barbara is back, but she doesn't respond... She is MIA, she went for breakfast... Your plane is leaving, so you skip breakfast. You arrive, by the time you land Barbara approved your breakfast, but now you are in a new country and circumstances changed, so you update your request. The earlier approvals are now invalid. You go through the whole charade again...
Pull requests assume you cannot trust your colleagues to make reasonable choices and you also don't trust your automated tests to catch anything insane (buying a 100k car for breakfast in the above example), so you have to gatekeep and slow down your team to make sure (?) that the code they commit is good.
You cannot trust any human to make reasonable choices 100% of the time.
If people are making good choices and writing good code then the PR process should be super smooth and seamless, right?
> You make a request... You've been waiting for almost an hour now.
1. Don't design your process so that waiting an hour to merge a pull request is going to be a problem as bad as waiting an hour to eat. Why are you waiting? Move on to the next thing. If it depends on the pending pull request, branch the pending pull request, and work on the new branch--if you have to integrate feedback you can rebase it into the new branch. Occasionally this will cause problems, but they're usually rare.
> Nobody takes a look at and you still need multiple approvals,
2. Choose a number of approvals that is appropriate for your team. If you're writing software where the consequences of a mistake are very high that might be a high number, but in most cases in my career, 1 has been a perfectly reasonable number of approvals. In some cases, 0 is the appropriate number of approvals.
> so you ping Barbara in charge of approvals to get the ball rolling, she says she will take a look after a meeting she is currently attending because she wants to be careful with what she approves.
3. Don't make one team member in charge of approvals--that's almost guaranteed to make that person a bottleneck. Anyone on the team with the knowledge to evaluate the code effectively should be able to approve it. I've been on one team where one guy was in charge of approvals and wasn't a bottleneck, which was because he did nothing but approvals. That worked great for everyone except him--his life was miserable until we scrapped that idea.
> Oh, Tom, her colleague asks you why don't you eat salad and orange juice, it's cheaper and faster. You have a quick call with Tom to explain why you want a coffee and donut.
4. If this sort of exchange is about personal preferences like what you eat or some stylistic nitpicking, then you have to have a team discussion about making sure that we prioritize code review feedback that's actually important and not bikeshedding. But in a lot of cases, Tom is asking because he legitimately doesn't know, and explaining your reasoning to Tom is part of your responsibility to help your teammate keep their understanding of the codebase up-to-date. This isn't wasted time, it's one of the benefits of code review.
> You arrive, by the time you land Barbara approved your breakfast, but now you are in a new country and circumstances changed, so you update your request. The earlier approvals are now invalid.
5. If different pull requests depend on each other or are touching the same areas of the code, ideally do them serially rather than working on them at the same time. If they really need to be worked on at the same time, then you need to communicate earlier in the process to be sure you aren't stepping on each other's toes, and it would make sense for the two people working on the same areas of code to review each other's pull requests. Not doing pull requests wouldn't fix this--this is just a challenge of multiple people changing the same codebase.
> Pull requests assume you cannot trust your colleagues to make reasonable choices
Sometimes reasonable choices are costly mistakes.
Even the best coders make mistakes, and unlike a mistake on what food to eat for breakfast, code mistakes can be very costly.
> and you also don't trust your automated tests to catch anything insane
Automated tests don't catch when your architecture is bad, or when there was a mistake in the specification, or when you've introduced a security vulnerability. Sometimes the mistake caught in code review is in the automated test.
> you have to gatekeep and slow down your team to make sure (?) that the code they commit is good.
Good code can always be better. I, for one, want people looking at my code to make it better. Obviously there are some tradeoffs here, and code review shouldn't be a massive bottleneck in your process. Done well, code reviews speed up your team: the earlier you catch mistakes the quicker they are to fix.
That's a good description of most teams, open or closed, private or public. Hierarchies are universal, for good reason. I'm not entirely sure what hippie commune David Farley programs in where Intern #15 can just merrily hit the big red nuke button and send everything into production
In open source projects pull requests are great because you don't trust people you don't know, in private companies you don't trust people because you know them. Hell I don't trust myself enough and I'm glad processes like this exist
Really? "Arch-enemies whose stated goal is to destroy each other and who are pointing world-ending nuclear arsenals at each other" is a good description of most teams?
My condolences.
A successful tech company in particular can go from 5 to 500 people in just two years. How do you run on trust when 95% of people don't know each other?
But if one is ideologically committed to seeing peer rerview as adversarial ... then that's the kind of viewpoint what one is committed to, I guess.
"This guy" sounds like someone that never worked on project size bigger than one. It is utterly idiotic take, on every level.
Anecdotal, but I feel like I see more teams where that "Merge" button is available to most people, but in general doesn't get pressed until the code has at least one review, whereas before it was often only accessible to a select few, or a review was required before it activated.
There's a great value in "every change was peer reviewed by someone", of course. There's also a value in "not every change is significant and sometimes you can trust developers to know that".
You can possibly install the fear of accountability if a bug happens in code that the reviewer was the developer until they want that learning opportunity of a code review.
In my experience, most of the "expert-level" developers almost always would love any feedback anyone wanted to offer because their imposter syndrome tells them it is awful and not to sign off on it, but are also more likely to get blocked if their PRs don't move fast enough.
I love PR’s as a concept and tool. I also often use PR’s to merge changes on my personal projects even when I’m the only developer.
If you have to verify, is there really trust?
We need to get to the point that a lack of trust isn't some inherently bad thing. I don't trust myself to never make mistakes. A PR is a way to get others to add a layer of verification to help catch bugs. I don't even trust the PR to catch all bugs, but it is a cost effective for the amount of bugs it does catch. Why is this lack of trust a bad thing? Humans are imperfect, we should build systems that require verification because we cannot trust ourselves to never make a mistake.
If you can verify, trust is not required.
As opposed to "trust blindly"
The opportunity to verify immediately is rarely available.
The way I'd put this is: writing code and reviewing code are - not two different skillsets maybe, but - two different headspaces. Code review is a kind of "technical QA", and indeed I am pretty sure that you see the same basic practice in factory work (where we get the concept of "QA" from).
I've seen teams that try to mimic OSS development and only merge absolutely perfect PRs, dragging out long-living branches and integration. At some point the perfectness can't keep up with the plan, huge PRs are suddenly reviewed hastily, and the end result is worse and slower.
Bottomline: PRs can be used slightly differently in companies to improve speed and reduce overhead.
Of course you're going to miss stuff and have a long review cycle if you have 20+ files with meaningful changes in a PR.
Requesting for example a big architectural change in a pull request is not a good idea IMO. Ideally it should have happened in the design phase, but overspecifying everything is not the solution. Now, in a company, a request for a bigger change can simply be mentioned in the PR, and the tasks can then be created and assigned as follow ups.
This is usually not possible in OSS; the author of the PR might agree to the necessary changes but later not do them for whatever reason, and there might be no other volunteers to "clean up". Therefore it does makes sense to gatekeep and drag out some PRs in OSS.
Also if the team is mature enough, the PR can be reviewed after the merge, so the necessary changes can be made with the current code already integrated and already running in production. The downside for this is that the team needs to have some discipline to review the already merged PRs and the developers needs to be disciplined to organize your time to make the fixes after the PR merged.
> "I think this is really bad way to organise a development team.
> "If you can't trust your team mates to make changes carefully, then your version control system is not going to fix that for you."
Man, I don't trust myself to be perfect. I trust myself to be careful but no amount of care makes me infallible. I make mistakes. And if we can catch my mistakes early by getting a second set of eyes on them, they're less painful for everyone to fix. When someone spends their valuable time catching my mistakes, they're doing me a favor.
Distrust might be part of the origin of pull requests, but that doesn't have to be the reason why your team uses pull requests. It can be about catching each others mistakes to learn, create a better product, and get better rewards. The tool is yours to use how you see fit--don't let ego prevent you from making it useful.
I don't trust myself either; trusting your own or others code is hubris. The two-person rule - or more - is applied EVERYWHERE.
> If you take away the appeal to trust, though, there isn't much left of the argument. What remains is essentially: Pull requests were invented to solve a particular problem in open-source development. Internal software development is not open source. Pull requests are bad for internal software development.
The specific point he tries to reword exists to explain that not everyone needs as much trust than open-source, and, therefore, that not having PR isn't a fatal flaw outside of open-source. The people who use it do so within a discourse along with other points explaining what they gain from forgoing PRs (usually, better continuous integration).
I don't buy the whole discourse about not using PRs, but I'm also not sold on the idea of dismantling a coherent discourse into separate talking points and demanding that they should stand separately. I'm also not a fan of rewording someone else's point and slipping a big non-sequitur in that rewording.
The reasoning behind this is exactly that we trust individuals on the team to merge. and because reviewing in the traditional sense seems to be a formalisation of distrust: Something you do when you don't trust your peers to implement and put in production properly tested code.
I trust my peers, but we all make mistakes and we do code reviews because we understand that working together can catch issues sooner. We aren't overly picky or mean during code reviews, but we make sure there aren't any glaring issues, and if there are, we talk about them and work together to fix them.
If you can trust the team with one and not the other then something is wrong. There shouldn't be any "this way is better than this other way" argument. Your team should be capable of doing both. If it isn't then neither will work very well. You're going to have problems, conflicts, bugs, etc no matter what you do.
Believing that the team should stop PRs because they're too slow, or they should stop trunk-based dev because pairing takes too much effort, are both red flags. They're both saying that the team has a problem to address.
> First, it includes an appeal to trust, which is a line of reasoning with which I don't agree. You can't trust your colleagues, just like you can't trust yourself. A code review serves more purposes than keeping malicious actors out of the code base. It also helps catch mistakes, security issues, or misunderstandings. It can also improve shared understanding of common goals and standards. Yes, this is also possible with other means, such as pair or ensemble programming, but from that, it doesn't follow that code reviews can't do that. They can. I've lived that dream.
I think that the other counter-argument discussed in the article can't confute Farley because, contrary to the author's stated intentions, it is in fact a straw man. The author focuses on the benefits of "asynchronous" workflows over "synchronous" ones: i.e., in his words, reviewing pull requests is easier (for an introvert like himself, at least) than dealing with the social stress involved in pair/ensemble programming.
PR are not only about placing a bouncer at the club door to, figuratively speaking, check entering members for excessive drunkenness, they are about placing a bouncer at the door to give access to a wider group.
This is quite obvious in open source, where the PR model has been a revolution in terms of lowering barriers of entry. You can join the party without asking for the keys. But similar barriers exist in companies. When they don't exits on the technical level (they usually do, sometimes to the point where collaborating teams wouldn't even know what version control the other side is using), they exist on the social level and a model like PR/MR can help nudge both towards the "internal open source" mindset.
If doing a pr takes longer than 5 minutes in general then consider if you are relying too much on pr's.
That’s how we treat PRs anyway. Occasionally I or a colleague indeed accepts their own PR (or use direct pushing) if it’s really minor (think a doc string) and need to get something out. So PRs are not just for envs without trust.
If you’re interpreting this through the lens of a single main/master/trunk and multiple branches, then like both Farley and the author, you’re trying to interpret a foreign culture (distributed version control) through a parochial lens (single trunk-multiple branch).
The DVCS world is one of many sources of truth. For example, in the kernel community, Red Hat, Google, Debian, Android, etc. all have their own branches. Linus has a particularly popular branch. None of them are the main branch.
And as an UI workflows which 'formalises' the discussion that needs to happen around a merge anyway they are quite nice.
(e.g. without PRs you still need to talk about "hey, I'm going to do this thing" => "hey this thing is ready" => "ok, let's merge this thing")
If no one is to review the code, then all code is either written by pairing/mobbing, or else it is the product of one persons thinking, blindspots, biases and all. It's great if you work in a team that pairs in everything, does trunk based development etc; that doesn't make it suitable for every team.
All merges to main is deployed across environments so pull requests help us get faster feedback if something fails.
I think pull requests get a bad rep because it is thought as a review process. We see them as short lived branches. They actually improve code quality and unwanted commits even without manual review/approval.
The way it was done was to have QA check your code, usually as part of a release.
I think PRs became more popular in non-OSS because of CI/CD and agile methodologies.
But what else could be a nice short term for "collection of changes with technically enforced reviews and tests"?
This actual change could be anything. You could just as well provide them as a patch they could apply by hand. The only formal aspects of the process, e.g. reviews and tests, are organizational. As an organization the acceptance of a pull request doesn't need a bunch of reviews and tests associated.
Originally, "pull request" was exactly what name implied, someone asked you to pull from their repository. The original Git collaboration model was that, you pulling changes from other repos and integrating them in your own, vs handing merge request.
Github co-opted the term to mean exact opposite, accepting essentially push from different repo.
Uh... Pull requests are not a version control workflow - it's a human workflow. It's also a way for the team to actually LOOK at the code that is going in and to be aware of what changes are incoming.
I mean, that's just one point, there are so many ways in which this view is fatally flawed, I am not even going to... ugh.
I know this is not trolling, but this is so scandalous, it comes across that way. This is beyond a "hot take".
In the corporate world is more like Seinfield's Soup Nazi skit.
We are all working towards AI accepting or rejecting code changes. Humans have egos and their idiosyncratic bias/baggage get in the way of adopting or rejecting code changes in an objective manner. Githook 50/72 rule anyone?
Am I missing something, or is the irony lost on this one.
This topic reminds of something Joel Spolsky told me about Stack Exchange, in the context of social networks. Giving users the ability quote text when replying is something they really wanted to avoid because it causes arguments to devolve into semantics — far more so than if people had to write out responses in full. A tool like quote-replying shapes (and deforms) our ability to have productive debate.
The unit of change in software is a patch and yet most of us use completely different tools for writing patches, reading our own patches, and reading other peoples (IDE/editor, git diff/show, and GitHub/Lab respectively.)
I like the change of context that web based patch review gives me. I often find bugs or untidiness when looking at my own changes in a browser. When working with others, I’ve found the best code review to be about collaboration and sharing of ideas and responsibility, picking out unseen errors, adding overlooked refinements, giving advice, and improving the team’s bus factor.
If we’re supposed to be playing co-op instead of death-match, shouldn’t we all be in the same level?