Your GitHub pull request workflow is slowing everyone down(graphite.dev)
graphite.dev
Your GitHub pull request workflow is slowing everyone down
https://graphite.dev/blog/your-github-pr-workflow-is-slow
30 comments
What do you do if CI is slow and you're merging small commits frequently? For example, what if CI takes 15 minutes and you're trying to merge more than 4 PR's per hour? Our integration tests spin up and reboot Windows machines, it would be tough to get that test under 15 minutes.
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".
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".
We have dozens to hundreds of PRs merged per hour using this flow. Yes the pattern of requiring unchanged main causes a backup. Don't do that.
And you never break main doing that? MR A could be fine, MR B could be fine, but A+B can be broken. It's rare, but it does happen.
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.
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.
Graphite is neat. If you want something lighter-weight, spr[1] is also worth taking a look at as an entirely client-side "solution" to PR stacking.
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
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
You mentioned a lot of Gerrit features but you didn't mention two:
- 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.
- 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.
It also has a significant amount of detail in subtle ways, some that can't really be appreciated until you try it.
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.
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.
Shoutout to ezyang‘s ghstack as well!
ghstack also adopts the one-PR-per-commit paradigm, which means you have to get comfortable with a lot of amending and interactive rebases and moving fixup commits around your stack. Amendments are nicely mapped to new commits in the PR though.
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.
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.
https://stacking.dev/ lists 5 tools to help with stacking, including spr and Graphite. AFAICT Git Town is the only one that works with GitLab.
The key part of the data model mismatch, one that could be solved, is that Git does not natively give you a way to refer to a commit by a "logical ID", one that remains stable under rebases and content rewrites, i.e. a unique identifier that allows you to identify a change anywhere in the commit graph. The commit hash is more like a "physical ID", because it is derived purely from the contents of the change. Version control systems like Jujutsu natively support the concept of a logical ID, which is called a "Change Id", and is exactly the same kind of ID used by Gerrit in its `Change-Id: ` footer.
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.)
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.)
Phabricator is what we used before we migrated to Gerrit. The review UI was very nice, the issue tracker is still best-in-class, but Arcanist was a menace. Much prefer a simple "git push" over that client-side PHP contraption :-)
Are there plans for Gerrit interop with Jujutsu?
Are there plans for Gerrit interop with Jujutsu?
> For speed and quality, PRs should be maintained under 200 lines—with 50 lines being ideal.
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?
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?
We should probably be doing periodic full codebase reviews anyway, regardless of how PRs are being made. It's only our tools that encourage us to purely look at the diff of a PR rather than check out the branch and think more big picture. You could also encourage/force people to write design documents or such before starting work on the actual PRs.
Though, a PR that introduces dead code (function that isn't called anywhere) wouldn't get approved by me :)
Though, a PR that introduces dead code (function that isn't called anywhere) wouldn't get approved by me :)
That never happens. There are like 100 things that need to happen that never happen that should happen before doing a whole code base review.
Reviews either happen before it's merged or they never happen.
Reviews either happen before it's merged or they never happen.
> how am I supposed to recognise it as a bug or otherwise, when the code to call it hasn't even been written yet?
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?
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?
I disagree. 1 PR should map approximately to 1 benefit (ie. a new feature or bugfix).
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.
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.
Also, A PR can contain tons of commits, and if the commits are well cut, it's very easy to understand implementation of a new thing step by step.
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.
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.
Wouldn’t it be beneficial to get the 9 refactors merged even though the 10th commit needs iteration? It would make it much easier for colleagues that can then branch off of the tested and merged refactors without running into merge conflicts later.
No. If the feature requires 10 commits and only 9 are universally agreed on then under this model you would merge 9 commits. Those 9 commits, despite doing nothing, would have agency and real estate in the code. There's nothing that says that 10th commit will even get merged if it's so controversial.
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.
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.
Big changes are risky. The core of making continuous deployment work safely is ensuring changes are small. If you have CD after a huge PR lands you are in risky territory, the chance you need to unwind is higher, but the size of the PR means it's more likely to have entanglements.
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.
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.
> If reviewing huge PR's is a burden, I will make better tooling for it.
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.
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.
If the feature is huge, you should be having incremental reviews throughout the process. The PRs should be made to the feature branch.
If your “better tooling” is to split this PR into multiple smaller chunks, then you’re just reinventing the original workflow
My experience, and my team’s experience, with Graphite has been the complete opposite. Graphite slowed us down considerably, which is a shame because it seems like Graphite was purpose built for a development workflow like ours. We continually stack changes to make code review less burdensome. We had been doing it in just git for some time and decided to try out Graphite.
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.
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’m think that you’re not supposed to have more than 1 person working on the same stack.
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.
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.
I'm one of the most obsessive team members when it comes to commit / PR size and hygiene. I want each PR to change just one thing, for the commit title to uniquely identify it, and the rest of the commit description to explain everything a reviewer or future maintainer would want to know.
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.
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.
I'm starting to lean towards, this is a people problem and not a tools problem.
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.
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.
I've worked in environments where we had several people per large project and reviewers could genuinely point out important context from parts of the project they were more familiar with. It was an awesome environment for learning not just the project but experienced engineering in general.
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.
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.
I think in situations like that, it's useful to take a step back and ask how much business value you're adding. If you really are the sole person working on a project, how likely is it that this one project having a bug or crash at some point, will super negatively impact the business?
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.
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 horrified to say that individual bugs we make can cost our large employer millions, yet leadership still overloads people with multiple projects on tight deadlines and loose engineering standards.
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.
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.
Testing is for sure the way to go to keep maintenance burden low.
I wish what you just said was more widely accepted insight :)
I think it is. I think it is much more prevalent for people to raise 1000 line changes putting refactors and functional changes and fix-ups in the same PR.
> Splitting it up into functions which can't yet be understood in their broader context makes them harder to review, not easier.
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.
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.
Your null implementation did help put it into context to some extent. Even then, in general, the API surface sufficient for a null implementation may not be enough for some real implementations. What if at least one real implementation ended up needing another parameter, and making that parameter available required a cascade of changes through several other functions and types? If that's what happened, the second change would have to carry that weight, and the changes combined required more review overall than one change which had the final API from the start.
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.
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.
>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.
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.
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.
I had some colleagues at a former company who absolutely swore by this.
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?
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?
I used a stacking review system at two other roles (gerrit and phabriactor). At the time I found it annoying and missed the pretty Github UI and the ability to bunch commits together.
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.
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.
I have used ghstack for exactly the problem you describe, but I like how graphite managed trees of stacks, which is better than ghstack‘s linear stacks
Small PRs are great, but GitHub's workflow sucks if you have dependencies between PRs.
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.
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.
GitHub does support that workflow, if I'm understanding you right.
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.
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 big issue on GitHub is that it doesn't work across forks: I can't create a PR for B that targets A, which both live in my fork, in the parent repository.
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.
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.
So it's a workaround for "even small PRs take too long to review"?
No, it's a solution for reviews being an asynchronous process. Even if I get a review within an hour or two, I should be able to continue further work in that time.
Isn't it often better to get a single thing all the way through? I know it feels like you are still working on the same feature, but if you want feedback you shouldn't move onto something else while waiting for it. That completely scrambles your flow.
Craming your day with as much "work" as possible isn't faster.
Craming your day with as much "work" as possible isn't faster.
I don't necessarily create the first PR because I want feedback. The design could've been discussed beforehand, and the PR is just to get a second set of eyes on the details, run the CI checks and to fulfill the review requirements. Nothing that comes out of that would have implications for the follow-on PRs, so why shouldn't I continue working on them?
> is just to get a second set of eyes on the details, run the CI checks and to fulfill the review requirements
AKA feedback
AKA feedback
How is "being an asynchronous process" different from "taking too long", in your opinion?
If reviews are instantaneous, how does that inhibit you from continuing to work?
If reviews are instantaneous, how does that inhibit you from continuing to work?
Small PRs and more than that, small work chunks are a really good way to get a team going and to be able to be more agile. Estimation is also easier because instead of even T-shirt sizes all you do is "is it small enough?" / "can we finish this task in under 1-2 days?" and if you do that to everything you can get pretty fast (and still offer predictability to the business by measuring throughput).
The advice here is essentially just the time old "keep PRs small and incremental so they can be properly reviewed", but its nice to see some of the stats/data around the size of pull requests in the wild.
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.
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.
Even in multi-user or organizational repositories, I wonder how many >1k SLOC RPs involve tons generated code or (poorly) vendored external dependencies. Typically it's pointless to try to review that in detail, either it applies cleanly or it doesn't.
That looks a lot like the workflow the Linux kernel uses (with MRs broken up into individual logical patches), just re-invented on top of a proprietary tool to charge $30 for it?
You're free to use the workflow used by the Linux kernel, but a lot of people prefer Graphite because it is easier to use and integrates with GitHub PRs better. If you find value in Graphite (as many devs do), then you can pay $30 and support its development.
This is not only about pull requests. 900 line commits will bite you in the ass later no matter if you use PRs or not.
To be honest I can't even decide if you consider 900 lines too small or too big.
Counting lines is useless. I try to commit the smallest "logical unit" (which is only fuzzily defined in my head) possible.
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? :)
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? :)
> 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.
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.
But then you'd have to agree that as a rule of thumb 900 lines is too big.
I can only quote myself: “the line count doesn't make any difference”
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.
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.
You are mixing code length with documentation length.
Also there’s a bit of process fetish there
Also there’s a bit of process fetish there
I wonder how much impact changes to package-lock.json, yarn.lock, Gemfile.lock and friends have on these PR sizes.
Agree overall that most are too big but 900 is way more than I typically experience.
Agree overall that most are too big but 900 is way more than I typically experience.
Rather than stacking PRs, wouldn’t it be faster and safer to incrementally check in work to the mainline branch and keep it behind a feature flag?
Looks like a partial attempt at reimplementing Google's Piper/Critique workflow. Which works well, but not on GitHub because GitHub is not like Critique or even Gerrit.
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.
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.
If you're interested in some of these concepts (i.e. smaller, more incremental PRs and stacked branches) but don't want to jump whole-hog into using a new product or tool, I highly recommend taking a look at `git rebase --update-refs`[0] to grease the wheels so to speak of implementing a similar approach with stock tooling. There's a great write-up on the feature and how it can help you here[1].
[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...
[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...
In my experience, stacking a big feature in multiple small pull requests plays well in order to start reviewing and delivering work that can be merged mainline while building more parts of the feature (say, sending for review a service object or an API client while you are working on the controller for that service), and it is easy to do with any Git client.
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.
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.
Stacking changes sounds insane. If you did this while talking to someone face to face they'd look at you like you were crazy.
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!
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!
OP seems to be ignoring sensibilities of maintainers and how seamless getting a PR in is per se, without looking at the changes at all. It may not be the norm, but I've seen occurrences of 3 smaller PRs taking longer to be reviewed and merged (for bogus reasons, not because they touch hot code) than one big chunk (best example would be a maintainer who is arguing about literally everything that is not their own code). Sometimes the process is the actual problem. Maybe it's even worse in companies than in open source projects...
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.
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.
Big PRs are often faster, due to the bike shedding problem.
A small sensible PR will get people debating intricacies. A big unwieldy one will scare reviewers into doing a quick sign-off
A small sensible PR will get people debating intricacies. A big unwieldy one will scare reviewers into doing a quick sign-off
1 commit per PR sounds like terrible advice, unless it's a change of just a couple of lines. Don't squash your PRs to make logs look shorter, learn to use the `--first-parent` option to show only the merges if you want less detail.
Can you elaborate? Our team has a squash-and-merge policy that I never questioned before. I guess people just don’t write clean commits and consider a PR the smallest unit that makes sense.
Over-simplistically, I might have commits like:
* 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).
* 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).
Commits should represent a single change that can be added or reverted independently. Everything should be squashed to that point, no more no less.
Honestly, the things I've seen slow down deploys and engineering schedules are:
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.
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 can just get my code past reviewers better when broken up in many small PRs, but I absolutely need to be able to keep working downstream of such changes without waiting for reviews.
The result is that I‘m much more comfortable with a tool that helps me manage a stack of PRs.
I used ezyang‘s ghstack for a few years, but the polish of graphite is worth it for me.
I agree that big PRs slow down everyone, but managing many small PRs still slows down the developer herself.
I agree that big PRs slow down everyone, but managing many small PRs still slows down the developer herself.
I think the killer feature of graphite is that it doesn’t just support a stacked PR workflow, but let’s you maintain a tree of stacks ahead of master.
I found that to be the killer feature and rebasing a tree of branches on top of a pull of master while keeping its branchpoints intact is extremely well done in graphite!
What really slows git down is asking PR writers to rebase constantly while they are trying to fix typos in their PR.
automatic rebasing is nice but how does graphite/ et al. solve the issue of creating a rebase conflict when working in feedback in the middle of the stack? And big prs have usually the property that you cannot just incrementally merge parts to main without breaking main. An api producer might change at one place and the accompanied change for an api consumer changes in another. Splitting up prs so the parts can be merged is significant work and part of the reason why big prs happen in the first place. I dont see how this works without a "virtual" main branch target that only gets merged to real main when the whole merge is done but then you have to reconcile differences for things that happened on main in between.
Anything that rebases cleanly is rebased and anything else is marked for needing a manual resolution.
IMO, number of commits is irrelevant since you can always squash them. LoC is the relevant metric.
This just seems to conflate PRs & commits, or not care about the latter at all; the perspective of someone who files a PR with 100 commits like 'fix typo', 'try again', 'the test broke', etc.
> 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?
> 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?
we tried to break up features into smaller PRs at one $dayjob, and the problem with small increments is that you have to plan out to some degree of detail upfront, which can't be always fast, and isn't always reasonable. otherwise, you can miss the bigger picture reviewing intermediate "steps", especially if it isn't the same person who reviews the entire feature.
also juggling branches doesn't scale beyond single repo, use worktrees instead
also juggling branches doesn't scale beyond single repo, use worktrees instead
My impression as a grumpy person is that this article mixes the cause and the consequence.
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.
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.
Yeah this is also like the mythical man month. 0 chance you can split a 900 line code Change into only 9 100 line commits.
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.
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.
advertisement pretending to be helpful.
Should you prefer small, incremental changes? Yes. Do you need be pathologically averse to PRs that hit some arbitrary limit? Absolutely fucking not.
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.
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.
[deleted]
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.