Your GitHub pull request workflow is slowing everyone down
graphite.dev
graphite.dev
Put in-progress code paths behind development flags and merge frequently to main.
That one sentence removes the need for extra tooling and obviates the need for more buzzwordy workflow names like "stacking" or "trunk based" or whatever the trendy thing is at the moment.
We use the "merge when pipeline succeeds" button on GitLab, but if two people have clicked that button it's a race. The winner gets their PR merged, the loser gets their pipeline invalidated because `main` has changed; they have to rebase onto main and try again.
OTOH, if instead of rebasing onto main when running the final pipeline before merge you rebase onto what will be main once its pipeline succeeds your own pipeline will still be valid.
In other words, by using stacking we can do final merge CI pipelines in parallel rather than series.
Unfortunately, we haven't found good tooling for this flow that works with GitLab. git town does stacks and works with GitLab, but it has a few limitations that make it a poor fit for what I call "merge trains".
It's not common enough to fix if the alternative is slowing down merges. But you can have your cake and eat it too -- merge trains allow rapid merging and ensure that the combination gets tested before merging.
Unfortunately, it is very hard to build good code review tooling on top of GitHub due to the severe impedance mismatch between the two models as well as GitHub API limitations and rate limits. That mismatch cannot be fully resolved (branches vs. patches) and results in constant friction, and you end up trusting a third party with full control over your repositories and approval workflows.
Graphite is the best attempt I've seen so far, but it still doesn't come anywhere close to Gerrit[2], which simply uses plain Git commits. Every commit becomes a review, and stacking is accomplished by simply pushing multiple commits. No custom tooling required - just `git push`.
It has very, very good code review UX and allows meaningful and in-depth back-and-forth during a code review, without losing context or having to re-review the entire diff. Once you're past the initial learning curve, it is blissful.
Gerrit is open source and it's what Google uses for many of their public projects such as Chromium and Android, and it is quite easy to self-host. It is entirely built on JGit and even stores code reviews as Git commits alongside the repositories.
If you want to give it a try - there's a well-maintained public instance, Gerrithub[3], operated by Gerritforge. Many open source projects use it.
[1]: https://github.com/ejoffe/spr
[2]: https://www.gerritcodereview.com/
[3]: https://gerrithub.io
Graphite really has nice support for that sort of stuff and opens PRs directly against master. I don’t really like that it needs an external service beyond just github and git, but oh well. The only thing I had problems with is that merging from github is very confusing and I don’t want to be forced to adopt their webinterface. I‘d prefer to merge/land my stacks from the CLI.
- it stores review information as "git notes", so you can see in the repository itself with `git show` who reviewed which commit and what score it was given, even though it doesn't alter the commit hashes by editing the commit message
- you can have arbitrarilty many different types of review, for example a QA test score that's separate from a code review score also separate from unit test scores. Much better than the all-builds-green other options offer
Gerrit is the best code review tool I've ever used and I don't get why people copy Github instead of Gerrit.
My go-to example of this is the "Attention Set," which came directly from Google's Critique. In short, it just tells you "Who should look at this change next," i.e. if you publish a change, it's the reviewers turn. They leave comments you need to fix, so now it's your turn. And so on and so forth.
That's a feature that, in retrospect, seems instantly obvious and "duh", but it's something that actually is not obvious to the designer unless you have spent a lot of time in the general problem space.
Gerrit is a really great, productive tool.
There has been some discussion before on the Git development list about adding Change Ids directly to Git, which would solve a tremendous amount of these problems. You'd never need to have commit-msg hooks and forges could natively support Change IDs easily. I don't think this idea was ever shot down. As usual it's mostly a matter of "someone's gotta do it" and go hack on the big Git codebase. (I don't have a link right now, sorry.)
Gerrit is probably one of the two best code review tool/development flow I've ever used; though, I liked many of Phabricator's design and UX decisions a lot more, to be frank.
(Disclosure: I work on Jujutsu. Yadda yadda.)
Are there plans for Gerrit interop with Jujutsu?
Personally, I've never been comfortable signing off changes that don't correctly and fully implement the requirements.
If I see a function has something that might be a security bug, depending on how it's called from elsewhere in the codebase, how am I supposed to recognise it as a bug or otherwise, when the code to call it hasn't even been written yet?
Though, a PR that introduces dead code (function that isn't called anywhere) wouldn't get approved by me :)
Reviews either happen before it's merged or they never happen.
You would just leave a comment on the change, and ask for the author to explain themselves? Or propose they fix it? I mean, that's the entire reason you're doing the code review?
If the feature is huge, the PR can be huge.
If reviewing huge PR's is a burden, I will make better tooling for it.
What I don't want is a big feature to be split into 10 PR's, where the first 9 provide no user benefit and are just refactors or adding dead code ready for the last PR.
This is why Linux kernel workflow contains n patches per feature. To understand what's done in a clear manner.
Lastly, adding feature flags to mask incomplete feature code paths is very risky, and is a bug by itself I think.
To be frank, PRs are almost never universally controversial. It's the details that make them controversial, so this outcome is likely more often than not.
It's lower risk to make small changes under feature flags. The final step of deployment can then be turning on the feature, and turning off is trivial. If something breaks along the way, undoing the commit is easy because it'll be small.
Small changes frequently applied are the path to low risk continuous deployment.
Systems like Gerrit and Graphite already have that, because they show you an incremental diff between versions of the change. If you have a change with 500 lines, you only typically read that whole thing once. Incremental modifications result in incremental diffs, so if it takes 50 lines of code to address review comments, the reviewer only needs to read that 50 line change. This is loosely the same as git range-diff. I talked about this here: https://news.ycombinator.com/item?id=38371748
> What I don't want is a big feature to be split into 10 PR's, where the first 9 provide no user benefit and are just refactors or adding dead code ready for the last PR.
These tools do not use "PRs." They do not use branches. They use changes. You are not proposing that a branch is merged. You're proposing a single, atomic change (one that can pass integration and the testsuite), to the codebase. You review individual changes. You may review a sequence of related changes, but you do not review "branches", and the tool largely ignores branches. This is a subtle but very important distinction.
Tools like Gerrit or Graphite will show you the "relation chain" which is basically just the child/parent relationship between these changes. So, if you have 3 commits, those become 3 changes to review in the UI, and at any point, while looking at any of them, you can see the other 2 changes for context, quickly. You can do something similar on GitHub. Go to a multi-commit PR on github. Click any of the commits. You can now hit "Next" (shortcut 'n') or "Prev" (shortcut 'p') in order to flip between commits, like the pages of a book. This is a review style that some projects adopt.
In reality, stacking commits "well" requires a bit of practice. It's a "garbage in, garbage out" process, so if you're just throwing 50 changes up there a day it isn't going to work well. There is a bit of an art and also a science to make concise, easily readable and easily reviewable changes.
In practice I'd say the "ideal stack" is anywhere from 2-7 commits; line counts I don't think are important. (Sometimes a single atomic change really does need to be 500 lines.) 7 isn't a magical number. It's just that having too large of a stack is often easy to lose track of. But that's roughly the same problem as a branch on Github that has 80 commits or whatever.
Graphite is quite hostile to collaborating on stacked branches. If one dev restacks and a collaborator is working in a dependent branch, they’re hosed. If a collaborator pushes a change to a dependent branch and they're not using Graphite, hosed. I think one of the main problems is that Graphite abstracts out potentially dangerous git commands into a fancy CLI such that the dev doesn’t really know what’s happening under the hood. It makes things even worse if you’re trying to collaborate on stacked PRs and not all devs use Graphite. Just forget about it.
We could have just been using this tool “wrong” but we had to stop using it within our team because it became so disruptive. At the time it seemed like you should only use Graphite if:
1. You are the only person working on a stack. You do not expect collaborators to be pushing changes to any of the branches within the stack, and
2. Everyone on the team is using it too.
Maybe things have changed in the intervening time, but I’m always dubious of tools like this.
It’s just git. It’s a wrapper CLI around git with a nice GitHub PR integration and a dashboard. It’s already easy to stack branches in git.
I used the stacking workflow years ago when I was working at Facebook. As I recall no one rebases their work on top of someone else’s unmerged stack. I haven’t used Graphite though so I can’t be 100% sure how they intend it to be used or how they advertise the stacking workflow.
You mentioned that you started stacking commits before trying out Graphite. What made you begin stacking in the first place? Did someone on your team use stacking before? I recall the one of the Graphite founders saying that they got a lot of their early customers because of ex-Facebook or ex-Google engineers who missed their stacking workflow.
But even I don't insist on any particular line count metrics. Sometimes doing just the one thing does mean a 1k line PR or more. That might be the MVP of a feature or refactor that's in a state that can be reviewed. Splitting it up into functions which can't yet be understood in their broader context makes them harder to review, not easier.
I do try to split out any preliminary refactor which makes the big diff smaller and more focused, but sometimes that just shaves off 10 lines. It's good hygiene but it doesn't change the reviewer's experience much, and the commit description now has to pull the weight of explaining why this refactor came first.
Stacking is definitely a huge pain in GitHub and after years of attempts I have mostly given up. I very occasionally include multiple commits in one PR, so reviewers can switch between focusing on one change and seeing it in broader context at their choosing. `git rebase --interactive` means amending the whole branch is still just one command with no third party tooling to buy or learn.
I think it's a very reasonable compromise with the tools available today. The biggest limiation is they can still only approve the PR overall no matter how many commits it has, but when you do this you scope the PR appropriately for that. That's the Linux kernel model of splitting commits but still soliciting review for the lot of them.
As as senior engineer, either I want to review every single change that's being made and I should be doing pair programming anyway, and PR approval is almost purely symbolic.
Or, I trust my colleague enough that I'll spend 5 minutes looking at their justification, explanation, testing strategy, rollout plan and decide to trust their code.
Stacked PRs and insisting on small PRs seems to want to fill some gap in the middle between these two, which isn't necessarily the correct solution.
Now I'm in an environment with several projects per person instead of several people per project. Now not only do we have a bus factor where nobody is familiar with anything but their own projects, even if they do review something they can't do much to anticipate issues or add context. Reviews have higher friction and less value.
For my own projects, I have instead invested in really rich testing, far beyond any other project here. It not only lets me move forward without regressions, it means if someone else ever has to take over without me, the tests will teach them broader context and catch regressions. All crafted in a way that test expectations are still easy to update when changes are intentional.
But I'm still the only one doing it this way. Most projects have 0 tests, or worse, lots of clunky tests of which 0 are useful. At best I'm hoping I can inspire others to follow even if it's only for their own sake at first. I'm not holding my breath for systemic change.
If the answer is "unlikely", or you're not sure, then do it for its own sake (you enjoy it) or do it for yourself (maybe it'll look good on your CV, or it's good practice) - but don't expect recognition for it.
I'm talking about global control systems in the critical path of user-facing traffic. But it's not as bad as it sounds; when I've seen these done by larger teams, they indulged scope creep and had no excuse not to. We get to say no a lot, and that's often better than having the resources to build more stuff that then compounds in complexity and has to be maintained forever.
The way I've done things on specific projects is recognized a fair amount, but not yet replicated enough. I get trusted with the most mission-critical projects because I've proven I can do them with minimal risk. I've tried teaching the rest of the team how, but they still fall into bad habits again under pressure, and create a death spiral where their shortcuts taken under pressure create more pressure indefinitely.
I make my deadlines because I invest in testability first and I'd always rather refactor earlier than later so tech debt doesn't compound. I don't mean anything dogmatic like TDD, I mean whatever gets me the most ROI on time and effort for a given kind of project. When time is the most constrained resource, it would be really silly to waste it on impedance mismatch between problems and practices.
Recently I wanted to add a hook to a certain part of our pipeline, which involved a lot of plumbing work (bad smell, but anyway). It was already about 200 lines of code and then some tests to ensure the hook was called.
I raised that with a null implementation of the listener.
I then raised a separate PR adding the actual listener I wanted, with busisness logic and tests for that logic.
Am I understanding that some reviewers would object to the first PR because they can't understand the broader context?
I think that's silly.
I assume the way you wrote it is that you already had the real version ready so you knew the API was sufficient even when you only sent the null version for review. Your reviewer probably trusted you to do that, or at least assumed it was in your interests to do that. But if that wasn't assumed or asserted, the reviewer could rightly ask how you know the first version is the right design based only on the null version.
I've definitely seen people come up with an architecture, API, or even network protocol based on what they hope will work without testing it on any real implementation yet. Just one detail from the real implementation can undermine the whole idea, scrapping any dev and review work done up to that point. Your change didn't have that problem, but if we're recommending principles for others then it's only fair to say this can be a problem.
I don't ask for a review for any design I haven't already fleshed out to a working solution, and even then, what works for one real consumer may not generalize well enough to other real consumers. But I consider one the bare minimum, and I make sure reviewers know what that one real implementation is as context for review, even if it does happen to be in a separate change or even separate repo.
At an extreme, it's like how you don't post an RFC that imagines a neat protocol that could exist, you post an RFC describing a protocol that has already been implemented and can now be generalized for broader interoperability. An internal API surface is the other extreme because it is much easier to evolve later, and a library API surface is somewhere in between, but all of them benefit from demonstrating real-world implementations of the abstract interface.
Yes. But not only that, just wiring in the null implementation required several choices. I wanted to know that reviewers would focus on that important detail and it not get drowned out by a review of business logic.
Larger PRs often have large parts of the code rubber stamped through sheer wariness of the reviewers. Like it or not, that happens. And that way design choices are approved that should not have. I see it all the time.
You can argue that is the fault of the review system, but if it just constantly happens, how do you fix it? Keep banging at people to review properly? They're people, they're goin to take a path of least resistance, that just happens. If it were up to me I'd put in a CI failure step for PRs over 500 lines. You could override that step if people deemed it reasonable (eg. large search and replace PRs), but it would have to be used judiciously.
I just generally find people who raise large PRs don't respect the reviewer enough to make the effort. So I often leave those PRs sitting, but I know someone is just going to come along and rubber stamp them.
Being used to trunk-based development, my approach was simply to make small commits. And if you insist on PRs, make small PRs. Although not totally cost free, they are easy enough to create that the overhead is trivial even with very small PR sizes.
I asked them how this improved on just making small PRs, particularly with the need for extra tooling an complexity, and all I got was blank stares.
Reading the blog entry, I don't see anything that goes beyond "just make small PRs. And now here's some tooling that's totally unnecessary if you make small PRs, but you'll have to organize your big PRs as a set of smaller PRs anyway"
What am I missing?
Now that I'm using Github full time, I miss the stack. I found stacks allowed me to easily commit for a few problems at once without bogging down a reviewer.
It sounds like you're already a fan of PRs being small, and that probably means _doing one thing_.
What about a spelling mistake in a nearby but unrelated comment? That probably should be a separate PR. In the stack-world, I was happy to make a second commit, create a second review artifact, and be OK with it shipping eventually when the other features of my commit ship (especially if they're close enough to cause a merge conflict!)
Today, I have to: * Stash/commit -m temp my current work * Checkout a new branch, make the fix, push it up, make a PR, etc. * Check back out my old work, potentially rebase on top of it if it's going to cause a merge conflict * get back to work on the real issue * Create a PR for the real issue... and hopefully not have it reviewed until the doc fix is already merged, because it's going to show up as an unrelated change here anyway!
There's also merge-into-other-branches that might help with this workflow, but I haven't found anything graceful yet.
In practice, I avoid the documentation fix to avoid the change in flow, when I'd have been happy to just have it as a different commit in the stack.
--
that covers more generically why I like stacks, I'm not sure graphite would solve my problems after reading the OP though.
Say I introduce some new abstraction and then convert two components to use it. I would want to create 3 small PRs, one to introduce the abstraction, and then 2 that convert one component each. That's especially useful if the changes to the components need approval from their respective owners. I want to be able to do all this in one go without being blocked by waiting for reviews.
The problem is that GitHub doesn't support this workflow, as it always uses the same branch for the base and the target. I would want to be able to create the first PR against the trunk branch, and then have the subsequent PRs show only the diff against that PR, but merge into trunk (of course you shouldn't be able to merge them until the first PR is merged). Currently, you have to either put everything in a single PR (creating huge, hard to review PRs), or wait until the first PR is merged before you can open the subsequent PRs.
Craming your day with as much "work" as possible isn't faster.
AKA feedback
If reviews are instantaneous, how does that inhibit you from continuing to work?
You can set the target branch on a PR. So pr A targets main, B and C target A. After you merge A, B and C even get automatically retargeted to main (or whatever you merge A into). This handles the "block B and C until A is merged" requirement as well.
Now, you can accidentally merge B and C early by accident into A and mess with the diff, and you need to have your CI properly configured to only check the diff between them and their target (not just main) but it works fine for most use cases in my experience. The other unfortunate thing is the potential for merge conflicts in the stack, of course, if you change A after branching the others off.
The other problem is that without extra tooling it doesn't give a good user experience. It's too easy for B and C to accidentally be merged into A, unless you mark them as WIP, but that requires manually unmarking them when A is ready. There's also no UI to easily navigate across the stack of PRs.
I'd be curious to see what those repositories where, and how differently things look based on, say, number of contributors. I found this really interesting:
> According to Graphite’s data, only 24% of massive 1000+ line PRs receive any review comments.
But would love to know what these PRs actually looked like in detail. I have a bunch of stuff on github where I'm essentially the sole maintainer and user, if anyone analysed those projects, there'd see lots of bad practices, like uncommented PRs, commits to main etc etc.
I don't think anyone would be too concerned about those things in my tiny repo though, but I'd be pretty terrified if I saw that kind of stuff happening on a big project with a huge userbase.
If that's 2000 lines as in the case of changing an API signature across the whole application, so be it.
Most of the time you can go much smaller than that though.
Also, i believe you've never had to go through year old large commits to figure out why a change was made? :)
Frequently. Folks joke that I'm not a software engineer but a software archaeologist. As you say the line count doesn't make any difference. If the PR is doing one thing that is good. If it has a good description describing the why and linking additional tickets/documents that's even better.
Just to illustrate. Imagine thet you have a bug. You trace it back either via bisect or git blame to a particular commit. Which one would you prefer:
- a 5 line commit with title “Bugfixes”. 3 of those lines is changing various parameters sligtly and 2 makes some code change which seemingly “should do the same”. The associated PR has no description, and no discussion. Just an approving comment saying “LGTM”. Both the author and the approver has since left the company.
Vs
- a 5000 line commit with the title “Add timeout parameter to frobnicator API”, and when you check the PR in the description they explain why they think the timeout has to be configurable and link to a bug report ticket, and a design document containing lively debate about the merits of alternative solutions. There is also a comment about the testing they have done to validate the change and plots from the trial run. (Let’s also say for fairness that everyone involved in the change has since departed in this case too.)
Which one do you prefer? I for sure would prefer the second one. Yes it is much much longer, but it does “one thing”, and has ample information about the context it was created in and about the thinking which was going on in the heads at the time.
Also there’s a bit of process fetish there
Agree overall that most are too big but 900 is way more than I typically experience.
Stacked commits need frequent rewriting of the stack. You push the first commit or two for review then you continue onwards. But you need to address comments on the earlier commits so you continually need to rebase things. This is tedious in git without extra tooling.
The review tooling needs updating to match. Ideally you have 2 dimensional navigation of the stack: up and down the stack, and left and right across revisions.
As a reviewer you want to be able to see the delta between revisions of the same (rewritten, force-pushed) commit to see that comments have been addressed. This needs a UI more like Gerrit and less like GitHub. It also requires some cooperation like Gerrit change IDs so rewritten commits have persistent identity beyond their content hash.
[0]: https://git-scm.com/docs/git-rebase#Documentation/git-rebase...
[1]: https://adamj.eu/tech/2022/10/15/how-to-rebase-stacked-git-b...
However, it starts to erode when a change in one of the initial layers has effect on others, such as having to make a change in the service layer that affects the way you pass a parameter in the upper layers. Then you find yourself just doing a rebase nightmare to keep the whole set of PRs in sync.
I wonder how many companies or teams would consider paying 30 bucks a month per user just to solve this problem.
You are asking for feedback and then effing off somewhere else and then before they can even give you feedback you are asking for more different feedback. What if the feedback on the first commit was "this is trash and needs to be redone"? Then you have to redo both commits.
This is the same reason why waterfall doesn't work. This flies right in the face of Lean. TOC says you are NOT being more efficient doing this.
Do one thing at a time get feedback and then move on. Don't stack a bunch of changes in a queue so you can remove the slack out of the process. THE SLACK IS SUPPOSED TO BE THERE!
On the rest, I am not disagreeing at all, it just might be worth looking at the reasons, but I hope statistics will sort out what I described above.
A small sensible PR will get people debating intricacies. A big unwieldy one will scare reviewers into doing a quick sign-off
* Add new field to the database
* Add new feature to API
* Add new feature to front end
* Add new feature end to end test
Each would make sense on it's own from a code review point of view, but the full feature doesn't exist until they are all included. You don't want to merge them one by one because QA would have to test every step doesn't break something else, but wouldn't be able to test the feature itself until later. Splitting it up means code reviewers can consider each part without the clutter of the others, and git blame may be more helpful in the future (although maybe not in this over simplistic case).
1. Using Git Flow. This inevitably leads to many branches for different teams/deploys/reasons and cherry picking among them and something always goes wrong. Because management opted into depending on a fragile branching system.
2. Many repos with some kind of inter-repo dependencies (usually testing or ops). Some of that can be cleared up with packaging. Some maybe not so much. This is another kind of complexity that management chooses, but management fails to account for that complexity when scheduling.
The "oh, no, your PRs are too big!" problem happens with every dev. Probably more than once. But it doesn't seem like a serious ongoing issue. As long as someone says "oh, no, your PRs are too big!" It's recognized as a problem and dealt with. The other two I've mentioned are not even recognized as problems.
I agree that big PRs slow down everyone, but managing many small PRs still slows down the developer herself.
> For speed and quality, PRs should be maintained under 200 lines—with 50 lines being ideal.
Is maybe a reasonable goal for a commit. The PR should then have as many of those commits (which probably doesn't strictly sum their LoC count) as necessary for the feature/fix.
Edit: In fact - I thought this was familiar, I just couldn't find a past submission from the same URL, because it was a newsletter puff-piece - I've said similarly before here: https://news.ycombinator.com/item?id=37917559
Also related, even more recently, is GitButler: https://news.ycombinator.com/item?id=39357068 which despite my scepticism there I think is a better solution for this 'problem' (i.e. an easier way to handle rebasing dependent branches) than for some reason having to up-end code review as a side-effect; I don't really understand the link at all if you look at Graphite's home page and 'learn more about stacking', nice to make that easier but what's it got to do with how we review the changes on those branches?
also juggling branches doesn't scale beyond single repo, use worktrees instead
The main argument seems to be "shorter commits are less likely to have bugs and are merged faster, so if we split a 900-lines commit into 9 100-lines commits, then our codebase will be better". But the issue with shorter commits is that they also tend to be less critical - splitting a large commit into smaller chunks is generally a good idea, but the complexity still has to go somewhere. Yes, structuring your commit in shorter chunks is helpful because it forces you to better think about what you're doing but that's a correlation, not causation.
I can be convinced that this silver bullet actually helps, but for that I'd need a before-and-after comparison of using the tool.
You need to make sure all your commits compile and work without breaking stuff.
It requires it's own skillset and strategy to do this.
Plus when seen in isolation bad code might be easier to push through like cooking a frog.
I just had to do a code refactoring that resulted in 1600 files being deleted. Stacking 40 PRs would have gotten me actually stabbed by my team, and I would have deserved it.
Your commits should be atomic, the smallest number of changes to achieve x, where x is a single task that can easily summarized by the commit message. Your PR should contain the smallest number of commits required to achieve y, which is either a minimum viable product to achieve feature z (if z is small enough to be done in one step/PR, good for you), or a step in the path to achieve z. Start your review early (using a WIP/draft PR) if you have the misfortune of having to do a large refactoring so your reviewer(s) can get in on the ground level and review in chunks. If you’re following the rule of “atomic commit” above, it’s really not a big deal for a reviewer to look at changes between each commit which can speed up reviews of big refactorings.