PRs suck. Stop trying to fix them.

I read through the article Your GitHub pull request workflow is slowing everyone down (Graphite.Dev) with great interest because I, too, am not thrilled about how PRs work. While I agree with the problems Graphite see with PRs, I think they miss other problems—and I don’t like their solution very much.

PRs are, apparently, HUGE

“The single most important bottleneck is PR size − large PRs can make code reviews frustrating and ineffective. The average PR on GitHub has 900+ lines of code changes. For speed and quality, PRs should be maintained under 200 lines—with 50 lines being ideal. To put this in perspective, where giant 500+ line PRs take around 9 days to get merged on average, tiny PRs under 100 lines can make it from creation to landing within hours.”

Holy shit! The average is 900 lines? That’s already using the system completely incorrectly. That’s so wild. It absolutely confirms my theory that PRs are a terrible way of committing code. I already thought they were terrible just because of the limited UI and lack of introspection of what the code you’re reviewing actually does.

PRs don’t encourage starting and running the change to verify that it actually works as advertised. You’re not using any of the tools that you use to develop code to review it. How silly is that? If you load changes into an IDE, you can see how many warnings there are, see if the layout shifts when you format the document, etc. Why would you want to review in a completely different environment? As Robin Williams once eloquently put it, It’s like masturbating with an oven mitt. (YouTube).

Not only that, but people probably aren’t looking at individual commits, so they’re just reviewing 900+ lines at once. The fewer people there are looking at individual commits, the fewer people there will be who make good, individual commits. This is a shame because it would counteract the awfulness of reviewing code in the PR web-UI, at least a little bit.

PR web UIs are not good for reviews

There are far better and more efficient ways of reviewing code than with PR web UIs. Reviewing through a PR web UI should be a fallback that you only use when nothing else is possible.

If you’re in the same time zone and working on the same schedule as the rest of your team, there is absolutely no reason why you should you be using the PR web UI instead of real-time reviews of local commits.

What the current PR machinery does is fool remote, async teams into thinking that they’re reviewing code efficiently. A face-to-face, real-time review will be much more efficient and yield much higher-quality code.

I honestly can’t believe the high pain threshold that some developers have.

If the developer hasn’t pushed yet, then:

1. Review with the person who wrote it.

If the developer has pushed and is not available for real-time review, then:

1. Pull.
2. Open the branch in SmartGit.
3. Examine the commits.
4. Launch the solution/project.
5. Run the tests locally.

6. Apply your own commits instead of review notes wherever possible.

   Yes, you can do this! Why not? You’re both on the same team. It’s a shared code base, not someone’s personal zen garden. Instead of explaining what you would want changed, just make your suggestion in the form of a commit. It’s often more efficient than writing prose.

7. Add review notes in SmartGit (synced with GitHub, Azure, GitLab, etc.) or use the PR Web UI to add review notes.

You can thank me later.

Errors slip in

“Problems can easily get hidden between the diffs, and reviewers often make assumptions instead of testing to avoid feeling overwhelmed. One particularly interesting finding is that as the size of a PR increases (by number of files changed), the amount of time reviewers spend on each file decreases significantly (for PRs with 8 or more files changed).”

Obviously! But it’s good to measure—this was my intuition. PRs don’t encourage local testing or verification in an environment similar to that which the original developer used.

Dumbing down Git

“By default, every PR is restricted to only 1 commit of <200 lines, keeping changes tightly scoped. This forces developers to consciously limit work to related changes—the registration endpoint PR can’t sneak in unrelated styling tweaks.”

Yikes! I don’t like the sound of that. So you make multiple PRs rather than one PR with multiple smaller commits? Why don’t you just review commits rather than one giant blob? Do you really need to corral each commit into its own branch and PR to force yourselves to actually make useful commits?

Yeeess? 🧐

“Stacking centers around breaking down big feature work into chains of smaller pull requests. Each PR is typically limited to 1 commit focused on an isolated change. This restriction guides developers to consciously make only a single change, squashing and rebasing along the way, instead of cluttering the PR with random unnecessary commits like “typo fixes”.”

This is yet another technique invented to accommodate teams that don’t trust each other, or that contain people who, if they can’t be trained to do better—or don’t understand what better is—probably shouldn’t be programming yet. Instead of teaching team members how to use their tools, they impose an arbitrary rule. What a kindergarten.

Integrate all the time!?

“Unlike Git workflows, where it is easy to neglect staying updated, Graphite centers your workflow around continually integrating with the current mainline state.”

Yikes! I don’t love the sound of that, either. Doesn’t that force you to spend more time on integration that you might have spent working? I understand you don’t want to have long-lived branches, but now you’re just shooting to the other extreme, forcing integration on every pull.

It’s not bad as long as the integrations are automatic, but might not be appropriate for developers who aren’t great at resolving merge conflicts. Even if they know how to deal with them well, might they not waste time resolving conflicts integrating a version of their code that wasn’t at all ready to be integrated?

I understand that this feature follows from the logic of “if you integrate more often, then integration is easier,” but, again, you’re taking agency out of developers’ hands, implicitly not trusting your team members. I don’t like it.

If you have several stacked commits, I wonder how much shuffling there is in the working tree (causing unwanted IDE reloads) during the integration cascade. Are they somehow integrating without touching the working tree? I don’t know that that’s possible.

Go ahead and work on the main branch if you want—I do it all the time—but this should be more of a choice than it sounds like it is.

I remember this…

“This command will add your changes and create a new branch in one motion. You can then continue iterating by creating and stacking additional branches:”

Ah, I see now. They’ve reinvented Mercurial’s patch queues. Everything old is new again.

A really bright and good friend of mine added an extension to Mercurial’s mq decades ago that sounds like it works the same. I remember discussing the technique with him as he was developing it.

Conclusion

I’m a bit worried about two things:

1. The one-commit-per-branch thing
2. The auto-integration-cascade

“By cleaning up your PR commit history, you ensure a clear and concise main branch history that makes it easy to see exactly what’s changed over time.”

By enforcing one commit per branch, you dumb everything down.

It does seem that, instead of acknowledging that PR supremacy is stupid, Graphite doubles down, strips branches of most of their functionality by equating them to commits, and uses multiple PRs to force people to review by commit. It seems like a waste.

But, hey, maybe I need to actually try it. I might be missing something.

Still, instead of adding another tool, I think you should use git better.

• Set up your local tools—or in-cloud IDEs, whatever—to support building the kind of code you want. See How to replace “warnings as errors” in your process for more information.
• Encourage your team members to learn how to use those tools.
• Have a review culture centered on real-time reviews where quick fixes and changes can be made before you’ve even pushed anything. This cultivates a culture of respect for commits.
• Use PRs only to “stamp” a set of changes and merge them to the trunk.