I Dislike Pull Requests (2020)
11 points by AccidentallyInvited
11 points by AccidentallyInvited
One more thing that sucks about PRs:
They inherently give you tunnel vision. Staring at a diff is just telling you what changed, and the immediately surrounding context. It's hard to verify that, all places that should've been touched actually have been touched. Or that this new helper function is a duplicate of another that lives elsewhere. Or that this inline construction can be replaced by a call to an existing helper function.
Having said all that, I still think PRs are the worst except for all the alternatives.
AFAICT, the author also doesn't mention what the suggested alternatives even are.
Many years ago now, I had a student whose project was to infer tree transforms from diffs. Given the before and after versions, he'd construct the ASTs (with libclang or Python's built-in parser) and then try to match a tree-transform pattern. Rather than reviewing a huge text diff, you could review a simple prose description that said 'foo() was renamed to bar() and all callers updated'. It could also spot inlining and outlining and a few other common refactoring operations. The really interesting thing was that it did some fuzzy matching and could also tell you had almost done a refactoring. Things like 'foo() was renamed to bar() and all callers updated except the one in baz.c'. He did some analysis of the tool by replaying commit histories in a bunch of open-source repos and found several examples of this kind of summary, followed by another commit that fixed the last case. It's a shame this kind of tool isn't part of standard flows. People are now approximating it with LLMs, but they introduce other sorts of error.
Semantic diff could also really help with resolving merge conflicts. That's another area where LLMs can often outperform humans, but obviously they're (literally) overpowered for the task.
My question to you and your student is this: why infer?
I agree that at certain moments a machine that guesses what you want can be pleasant, but even though you're not using LLMs to do it you're still talking about a system that has to guess what the intentions of its user are.
You don't want a system that guesses, you want a system that knows. That's why I'm working on the same problem, but instead of trying to create heuristics to recreate user intent, my idea is just to give the user an environment in which they have the means to directly express intent like foo() was renamed to bar().
Because most users edit text.
If you’re in an environment like Smalltalk, it’s easy to build refactoring tools into the environment and the revision-control system. But when you’re using text-based languages and text-based revision control, we wanted to see what was possible to retrofit.
We already have tools that do these refactorings in the forwards direction. Things like clang-rename will do renaming across a codebase, for example, and integrate easily into editors. But most people don’t use them.
But if you define these refactorings as AST transforms (as most tools do) then you have a fairly simple graph-similarity problem to try to spot them. And then you’re completely agnostic to what produced them, whether it’s another tool, human editing, or part of a larger cleanup.
Most people don't use the tools that transform ASTs because there are 500 different ones. The tools for transforming C ASTs aren't the same tools as the tools due transforming Python ASTs and Java ASTs. JavaScript had at least 5 different tools and AST formats within its ecosystem.
So yeah the problem isn't that environments don't exist, it's that there are too many and no standards.
If there were one DOM for code like there is one DOM for HTML documents (offered by every browser) then it would be easy for people (and also LLMs) to learn to use it.
Also I would note that working with a DOM doesn't mean that you give up editing text. I'm editing text stored inside a DOM right now to compose this message...
Given the before and after versions, he'd construct the ASTs... and then try to match a tree-transform pattern. Rather than reviewing a huge text diff, you could review a simple prose description that said 'foo() was renamed to bar() and all callers updated'...
Is the tool's source code available somewhere?
I don’t think so. It was very much student code (undergrad final-year project) but it showed the ideas work. It probably wouldn’t take someone who knows what they’re doing more than a couple of weeks to reimplement. Tree-similarity algorithms are very old and well documented.
If you feel "slowed down" by pull requests and see them as a "hindrance" I immediately wonder about your ability to actually consider overal context and impact. It is a generalization, but many times when I hear people argue this point it comes from people who don't really play that well with other people. Who are often called out on going for patterns and styles not matching the code they are working on.
There are other reasons to have pull requests in place, but slowing down people like these is also one.
I think there's more subtlety to this. Sometimes you're in the room with someone else. You have a small fix. Both you and the other person look at it and are like "... yeah that seems right".
Obviously the PR can get easily checked off, and it does! It's just some more friction.
In our company we've had things set up to where the most senior people can merge in without getting reviews. We still ask for reviews on most things. But for small cleanups etc? Or for changes that would get a rubber stamp anyways? We're not bothering as much.
The theory here: we still ask for feedback from our peers. We do it when it "makes sense". That line is tricky to navigate.
Sure, at the same time I have seen "small fixes" like that cause a lot of extra damage because they weren't as small or easy in the end. I can relate to the feeling but I honestly rather have a speedbump where at least all basic things are sure to be ticked off.
Also because, in my experience, there rarely is an actual urgency that warrants the time and or effort saved by being able to push directly.
If the urgency is to have a fix be deployed on a testable environment then there is also room for improvement there. Here each pull request automatically triggers a build of changed code and complete deployment to a merge request namespace (unchanged code is pulled directly from previously build images making it quite speedy) giving you an environment as complete as the one that you'll get when pushing to main.
Finally, as someone in a senior position, seniority often is overrated. Specifically in areas like this I appreciate a fresher insight and double eye principle. To me it is a good thing that all basic things are checked automatically before it lands in main and potentially becomes everyone's issue.
Its just my experience here, but I rather trust a process here with actually automated gates rather than people. At the detriment of those few times where an actual easy fix takes a bit longer.
Maybe I have poor luck with the organisations and talent I have worked with. But considering the variety of organizations I have worked in with developers of widely different backgrounds I am fairly confident in standing behind my words here.
Also because, in my experience, there rarely is an actual urgency that warrants the time and or effort saved by being able to push directly.
So for "urgent"/important fixes things we would go through the more formal process. What I'm talking about is actually more used for things that matter way less, and where the (supposed!) cost of a mistake is much lower.
For example, a small tweak to some visualization might break, and it's not good that it breaks, but reverts are just there and available to be done. But anything actually writing data is much more risky.
I agree on seniority, and we're still trying to figure out the line here. We have been using this as a pressure release valve for to lower the amount of "tedious" reviews that aren't giving us much value (for example someone has been working on moving our type checking over to a new stack... it's not impossible for something to break there but pretty unlikely).
(in some sense I think this is working for us because we have low hanging fruit of lots of small incidental things to fix up + add tests for due to ... reasons)
I think the author wants design reviews but didn't say so.
I had the same thought, but up-front design review has a cost of its own. Some pitfalls only show up once you're deep in the implementation, and a few of them call for a structural change. A design review leaves the team with an agreed design, and pivoting away from it means un-agreeing with people, which is a psychological barrier the implementer wouldn't otherwise have.
Daily standups soften this a bit. At one point we also pushed everyone to open a draft PR from the first commit. It mostly didn't work: others rarely looked at the drafts in time. And people who keep a pile of uncommitted changes, then stage and split them into tidy commits at the end, ended up opening the “draft” when the work was nearly done anyway.
Has anyone found something that works here?
They did say that they were ok with reviews earlier in the process (at whiteboard stage?) but I don't know if that's considered a design review. Seems they also didn't like written down reviews.
Perhaps, like you, I was disappointed that an alternative solution wasn't talked about. I don't know what an alternative solution would look like either.
Some interesting ideas here but some of these criticisms I think can be resolved without direct commits to main let’s say.
Why do I dislike them?
The workflow for unknown and untrusted contributors to public projects, has been largely adopted unquestionably for private projects with known and trusted contributors.
Users are now the testers. Formal testing has disappeared. Careful incremental releases are too often replaced with continuous deployment. Programmers are untrusted and have no code ownership, resulting in them being viewed as generic replaceable resources.
Formal testing has disappeared.
Still being done in many places. Maybe not in the old waterfall sense of testing, but functional testing done by hand on a deployed instance based on the branch. Merge requests are also the one place where automated tests can run and detect faults before you have to fix main.
Also, to be frank, in a large enough organization I also don't outright trust people to commit to main responsibly. I have seen too many developers cut corners, overlook context and much more being caught by even half baked merge request processes.
When I kicked off in software engineering, there were no pull requests. Even source control wasn't an automatic thing. You had to communicate in other ways (and yes, you were mostly in person so you quite literally just talked about it).
The Pull Request orthodoxy came in with Github (in my universe anyway) - which was geared around Open Source and at-a-distance development over the Internet. Not to say it wasn't useful and didn't make a lot of sense for many cases.
Applying a PR x-reviewer mandatory policy probably is a decent enough blunt instrument, but it's not perfect. And probably AI has blown it to bits as a proposition.
All that said - I'd keep them. CODEOWNERS and a bit of liberalization around merge rules goes a long way.
PR are mostly a github thing.
It doesn’t really address the post here but I really prefer patches.
Patches are their own file. When done using git-send-mail, you can forward a patch, you can apply it to whatever version of a code you want, you can directly write your feedback in the code itself to be send by email. You can even slightly modify a patch yourself to test something.
Doesn’t address most mentioned issue but I believe patches improves over PR and I can’t thing of a "less worse" solution.
Also stacked diffs encourage smaller diffs than PRs, that can be reviewed independently even if they have logical dependencies.
I prefer Phabricator to the GitHub PR interface, but it still sucks for anything but trivial changes. I want to do all review in my editor, where I can evaluate changes in context, using the full power of my editor. Something like this:
https://tigerbeetle.com/blog/2025-08-04-code-review-can-be-better/
I can't believe that a startup hasn't run with this idea.
Have you tried something like Gerrit? It has native integration with jujutsu, and seems to support stacking commits quite a lot better from what I can see. I've been meaning to try it for myself but haven't gotten to it yet.
Though using it with git looks a bit cumbersome
I know Gerrit exists and supports stacked diffs, but sadly I don't get to choose dev infra at $WORK (where it looks like we're inexorably moving to GitHub PRs in the near future).