Large pull requests slow down development
graphite.dev
graphite.dev
The only way developers are able to make any kind of velocity in their work is to babysit their PRs morning, evening, and night. "Time for bed. Let me crack open my laptop and check to see if my PR made it past the merge checks. Oops, $RANDOM_CHECK failed because the server that runs it was borked. Let me kick that off again and hope it works overnight so first thing in the morning before I leave for the office I can check again to see if there's some other random failure I can fix and then re-kick off the merge checks so they can run while I'm commuting into the office."
In spite of having it broken into 12 neat commits, a reviewer demanded that I break one of my PRs into multiple PRs. I don't work outside of standard business hours, so I've been iterating on it for the past 4 weeks now in between meetings, design reviews, interviews, etc. Meanwhile a team of about 4 people have been blocked waiting for the full patchset to fully go in.
If instead it were one big PR with multiple commits as I originally had it, I can have gotten it merged in a few days.
Ah… about that
So, you either run the expensive tests asynchronously and all that entails or you allow CI to take forever.
I agree that larger PRs can increase the latency of getting feedback and getting quality feedback. I also have another view.
Most code reviews struggle not from the reviewing part and shockingly not the authoring part, but the laying groundwork part. The laying the groundwork part is before any hands to the keyboard to write actual implementation. This part focuses on explaining what is trying to be solved and for what problem. It is bringing everyone who would like to participate in the conversation up to speed on terminology, background, understanding of past decisions and the different proposals looked at, ruled out, etc.
I've seen too many PRs ask for reviews where the reviewer had to read the description half a dozen times and spend an additional hour understanding other systems that interact with the diff to be able to even attempt to discuss if the implementation solution is even the "correct" implementation that should be tackled for the problem at hand.
IMO, there are four key parts to better quality PRs, laying the groundwork, authoring the technical implementation, reviewing and feedback.
Seems to me like it just can't work. Show a programmer a 5 line program, as the adage goes, and get 5 critiques. Show her a 500 line program, and get a thumbs up; "looks good!"
Really curious to know what keeps professionals from resorting to that timeless adage. Besides sheer chutzpah, how do folks muster the gumption to tell their peers what should have been tackled? Or, do I have my hat on backwards?
We are professionals and it's about time we started acting like it.
I had an experience very similar to your, "just add a couple of columns". That experience which I'll explain was the final straw to getting me to start drafting the book.
An individual at an enterprise company made a PR to change the code from handling a single item to a list of items. Of course this showed promising performance because it reduced a bunch of the overhead operations that used to be O(N) down to some sub N amount.
It took me about 3 hours to fully understand the surrounding systems. At that point I did not review the implementation, BUT instead the problem. A system was falling behind and timing out. This implementation did help fix the issue by batching API calls. While I as the reviewer did the "laying groundwork" portion I realized that a lot of the API calls themselves did not need to be triggered in the first place.
Looking purely at the implementation for review was a LGTM (Looks Good To Me) kind of review because I could not weigh out the solutions to the problem without the information on the systems to understand where the problem could be.
You can make large, easy to review PRs but if the commit history isn't useful then you have to view it all in one big chunk.
If instead you take a note out of the patchset workflow and make each commit it's own fairly discrete and digestible change after you wrap up development on the feature, then reviewers can just start on commit 1/N and review each one as if it was its own mini PR.
A 10k LOC PR is going to be a pain to review if it's treated as one big commit but if it's broken into a bunch of <100 to 500 LOC commits, each with a descriptive title, a useful message, and functioning code/tests, then even if reviewing takes a long time, you can review and test each discrete chunk fairly quickly without too much cognitive load. And then as you keep making revisions, the changes per commit revision get smaller and may only take a few minutes to review, even if it is a 1k+ LOC revision.
We expect clean, readable code so we develop engineers who can write that code.
If we expect clean, readable commit histories (like any project on the lore. https://lore.kernel.org/), we can develop engineers who produce those commit histories.
There's an adoption cost but IMHO it's worth it.
I’ve tried to do this but the defaults on GH means even if I stack 5 commits each doing atomic things nobody reviews it as anything other than a single patch. And if I split them into distinct PRs the last one looks like the giant PR. And we’re in now I’m shepherding 5 PRs.
Until we have tools or at the very least processes that encourage that behavior nobody will do it.
And don’t get me started on commit message display.
(Obviously I make exceptions for things like large scale code generation or automated refractors).
But for new code? I'm okay with more LOC than normal but I think 10k lines is my hard limit (including tests and documentation). And I expect no more than 500 lines in any given commit/patch. And realistically I am expecting that patchset/PR to be around 50-100 discrete commits/patches in length. Also it's likely that once things start stabilising that patchset can be broken up and merged in chunk by chunk.
And with prior reviews (i.e. merging an incubation branch into mainline) I expect there to be effectively no changes other than merge resolutions to aggregate PRs/patchsets into the incubation branch that is getting pulled into mainline.
You should absolutely strive to keep smaller PRs, but I've frequently seen this become "you should only have small PRs."
To make it work, you have to think differently and code differently. You have to think in advance: "does what I want to do require changing a lot?" If so, think about how else you can solve your problem. Or when implementing something for the first time, think about how difficult it would be for someone to change it.
You end up building things with high cohesion, low coupling, object factories, etc. It makes for very different code that is more maintainable, flexible, easier to change. You end up not needing a big PR, or the changes are to a single high-cohesion component so it's much easier to review than changing 10 different components.
With that said, using methods like stacking and feature flagging, I've been finding even new feature development to be possible while keeping my PRs to roughly 5 files changed or less.
Not sure I understand the "artificial" part here. There's nothing "artificial" about breaking up your larger changes into smaller PRs. It's just good practice.
Helps reviewers who are reviewing the code, and helps the author be more focused with their changes.
Even in net new feature development it's a good idea to break up your large changes to something more manageable.
Sorry if I'm not understanding, what do you believe the downside to be?
Often you need the full context when evaluating a new feature end to end.
Or you spend 2 days splitting up a PR into smaller PRs so that a person can review it in 30 minutes instead of 2 hours.
I can't say I've ever seen benefit from it both as a reviewer or as a developer, but it could be an effect of different companies and different teams.
I'd agree keeping the unified context is preferable but it's probably easier to do that by having developers rebase their changes into discrete commits that can be reviewed one by one.
Let's give a hypothetical situation where you split up a PR by the backend and frontend components, but you have some extra fields and endpoints that are not used in your final product. They accidentally get left in because you miss them.
I believe the chance that it would be caught in review is significantly higher in a unified PR review instead of artificially splitting front and back into separate PRs.
And if you need to have the 2 PRs open side by side, then why split it up in the first place?
I agree however I think PR's should be split up. Just not into separate PRs. The solution is to actually start reviewing PRs by commit (you can do this in the PR web interface). Everything is split up and can be reviewed separately but you can still see the final diff and while each discrete unit is reviewable, the greater feature is also still one discrete reviewable item.
Organizing by clean commits is definitely important.
I just grabbed a random patchset off of the git mailing list and linked it below [2] to demonstrate this.
You'll notice that on top of the overall patchset (equivalent to a PR) having a detailed description (in the coverletter, i.e. PATCH 00/xx), each commit has a descriptive name, message detailing what the changes within do, and signoffs by everyone who contributed to it.
Then as each reviewer comes in, they review each individual patch (i.e. commit) separately, replying to the message for that patch. And any high level concerns can be in reply to the coverletter (addressing the entire patchset).
As the contributor responds to comments and makes the requested changes, those changes get made per patch/commit via rebasing rather than just added on top. And when they are finished making the revisions, a new v2 patchset is released in reply to the cover letter of the first patchset, now also containing a diff per commit/patch against the previous revision.
Then the cycle repeats until everyone is happy. At that point the maintainer will merge the changes into their incubation branch (the git devs call theirs `next`) and after some time has passed, they merge it into master/main and it becomes established history.
The worst part of the whole workflow is that it uses email but other than that it is a far preferable reviewing experience to using github's stock pull request workflow.
2. https://lore.kernel.org/git/20231010123847.2777056-1-christi...
How would you know what to critically evaluate in such case?
If you are talking about refactoring prior to merging into the tree? then no it's not a waste of time. That's the intended workflow. You make your changes to the commits or add new commits in between using rebase. This is how all the development for linux is done.
If you are talking about after they are merged into the mainline? That's also not a waste of time. You don't have to go back and change those commits because they are now finalised and your iterative refactoring can be done via small one off patches gradually merged into main, a series of large patches merged into main, or a set of patch series all merged into an incubation branch that is kept up with main and eventually merged back into main once the refactor has made sufficient progress to take over.
1. I open a branch.
2. I start coding.
3. During coding I do various commits.
4. I realise I need to refactor/rethink something, I do more commits, overriding the code/ideas of previous commits.
5. Then I finally put it up for code review. But in such case there's no point in going through all commits, since a lot of that gets changed anyhow.
6. Usually I would squash everything into a single commit and merge after doing that. Because a lot of my commits would've been pointless anyway.
What is the suggestion exactly?
1. I open a branch. i.e. `ID-XXXX-branch_name-working`.
2. I start coding.
3. I make a ton of quick tiny commits. (I generally label these commits "NO-MERGE: ").
4. I make a bunch of changes.
5. I finally get everything done.
6. I now create a new branch `ID-XXXX-branch_name` from `ID-XXXX-branch_name-working`.
7. I rebase that branch to get my code cleanly formatted by commit with each discrete feature or change getting its own commit.
8. My code goes up for review.
9. I get changes requested.
10. I make a new branch `ID-XXXX-branch_name-v2-working` from `ID-XXXX-branch_name`.
11. I make the requested changes as a bunch of new small "NO-MERGE: " commits.
12. I am now ready for re-review.
13. I now create a new branch `ID-XXXX-branch_name-v2` from `ID-XXXX-branch_name-v2-working`.
14. I rebase those NO-MERGE changes into my "presentable" commits, adding or removing well documented commits as necessary.
15. I now send out my v2 revision to the mailing list or I change the HEAD of my PR from `ID-XXXX-branch_name` to `ID-XXXX-branch_name-v2`. If I'm using a PR workflow, I link a diff between the two revision branches (you can do this in github using `https://github.com/org/repo/compare`). That isn't necessary with patchsets since I can easily do a range diff there. I suppose I could do a cover letter with range diff and paste it to github but it somehow doesn't seem as nice.
16. Rinse repeat steps 8-15 as necessary for each new revision.
17. "LGTM"
18. Merge into `main`/`master` (like actual merge, not rebase or squash merge) and close PR.
19. Clean up branches. Either save them somewhere for prosperity if you have trust issues like me or just delete them.
This looks like a lot but I was trying to be as detailed about that workflow as I can be. Realistically it's not so bad and you can get a hold of it very quickly.
Also with regards to rebasing changes into well documented, discrete commits, if when you are doing your development you make your commits small and self contained, with `git rebase -i` you can actually just reorder the list of commits to chunk together the related commits and 99% of the time it'll rebase with little to no merge conflicts. Then you can just squash those chunks down into your presentable commits. This also applies to your v2 changes and on. You can just move those commits in the rebase TODO to put them after the commit you want to squash/fixup them into and if they are small clean changes, they should rebase without conflict. Things only get nasty and break when your commits are spanning multiple unrelated files and you try to break those up.
I'd estimate rebasing new changes into an already documented set of commits probably takes me 1-2 minutes on average so I consider it well worth the extra 30 minutes spent over the course of the week.
In my experience, this kind of thing happens much more frequently with large PRs.
If a PR is large enough, things will slip through the cracks. If there are too many change requests, even if they're sensible and make sense, things will slip through the cracks. If there's the need for multiple checks by the reviewer, even more things will slip through the cracks.
Also, breaking up PRs in frontend vs backend is not the best idea. Build the feature iteratively if possible.
EDIT: Since the grandparent is talking about reviewing by commit: if those are more reviewable, just use those as the way to separate PRs instead, maybe?
If a PR introduces a new function that will be used in the next commit, I would much rather see that next commit using git, than hunt for it in the PR queue.
But as a code writer myself, for example, if I am building a new feature, firstly it's really hard for me to know what the whole thing would look like without going through it all and it's probably very iterative process as I'm doing it, so I usually wouldn't be able to split it up or it would very suboptimal to split it up before I've finished everything.
Then I would try to split it up as I've finished to appease reviewers, but again, it requires whole lot of creativity to do. Should I try to split up shared component first? Because I surely can't split up whatever is using those shared components. If I do then, people won't see whatever is implementing those shared components so they won't have understanding on why those shared components provide certain functionality etc.
Overall it complicates a lot it seems because if I was to do it during my iterative process then I would write a PR, later refactor bunch of it anyway, and I would do it in the order that feels best for me, but wouldn't necessarily be easy to understand for anyone not within it.
Of course, past a certain size reviewing a large change becomes tiring (~500 LOCs IME), so in these cases I ask authors to create separate PRs where each one is focused on a single thing (refactor, fix, feature, etc.). This way review duty could be split across the team as well.
And then there are cases where large PRs are unavoidable, so we deal with it. But atomic commits are a must in all cases.
Also, please don't automatically squash-merge PRs! If the history is messy, clean it up, but leave atomic commits behind, and do a merge commit, or rebase, if applicable. I've found this to be a controversial topic, for some reason.
When I make changes to a codebase that very few people are experts in and is generally perceived to be difficult to understand, I make "smaller" pull requests so that the change log is followable and changes easier revert.
As with many things in software development: be pragmatic, think of others, and do the best you can.
Not trying to be snarky, genuinely curious, is this a situation you find yourself in very often? Are you describing something while working at a company or in an open-source project?
Consider adding a feature. I might structure this as three commits:
1. Refactor in a way that preserves existing behavior but prepares for the new feature
2. Implement the feature, creating a user-visible change
3. Remove old code rendered unnecessary with the new feature
Each of these commits is independently reviewable, passes all lints and tests, and supports bisecting. Small, separate commits like this is good.
But there's no rationale for three small, separate PRs, because they are not independent. Either all the commits should land, or none of them. If we later revert, we would revert them all. And review is made easier by seeing how the commits relate to each other.
I suspect that structuring PRs as multiple independent commits requires a certain level of git history-grooming which is difficult to learn. If git's interactive rebase were more first-class, we would have fewer, larger, and better-structured PRs.
It doesn't?
As for the rest of your comment, this is why we have feature flags. What you're describing should be a single PR. I don't care how many commits it is.
Spared you 5 minutes
I suspect they still slow down development in the long run though due to more bugs and less theory building among the team
In dysfunctional orgs with enormous code review time per PR, with lots of nitpicking, and a CI/CD pipeline that is constantly broken and takes a LONG time to get things into a staging environment.
Fortunately modern tooling formats code automatically so there's less bikeshedding.
(Assuming the code is in a language with a good type system and without reflection, I'm confident in automatic refactors and don't need to spend time on them in code review)
I prefer incremental and used to roll my eyes at them, but it's just a different way of thinking and people/teams are diverse.
It doesn't really matter. You can start with a big change initially as a 'big thinker'. You just have to break it down afterwards.
I often have a bit of feature creep when working on a change, and add all kinds of incidental fixes I find along the way.
But afterwards, I use git's tooling to 'peel off' all those extra changes and stick them into their own PRs. That makes my main PR slim again. If the main thrust is still to big after that diet, I think about breaking it down into incremental changes that make sense for reviewers.
That process of breaking down is all about telling an understandable story to reviewers (both reviewers right now, and the poor folks who have to look at my commits in a few months to figure out when and where we introduced a bug, or why certain decisions were made. Those poor folks often include my future self.)
> I prefer incremental and used to roll my eyes at them, but it's just a different way of thinking and people/teams are diverse.
Some people produce incremental PRs naturally, and that's great for them. But for the 'big thinkers' it's a learnable skill to break their PRs into manageable chunks after the fact. No need to change their natural style.
There's a clear parallel here to the idea of a "genius" who isn't able to communicate their ideas effectively. Can they really be considered that smart if no one understands them?
But that's extremely difficult I think and requires some out of the box extreme creative thinking on how to split it up after the fact. And I would also think it doesn't help at all.
I usually like to imagine how I build the new feature in head. Then I vomit out whole bunch of code, but I feel like splitting it up is extremely difficult and it will just lose context of why I did something as I did.
E.g. if I split it in a way where I only show shared components, it won't be understandable why I made them shared in certain way because you won't see the actual other logic that uses them, so reviewers are kind of left to trust that these will be used in later PRs.
And I think the way you make things reusable is likely the most important part because there's a fine balance there, you don't want to make everything too reusable that is not intended to be reusable, but at the same time you want to make it as reusable as possible considering the future, and it will take a lot of thinking to understand future implications. A reviewer won't have understanding of the reasoning without going through the process themselves on how something might be reused.
There's a lot of disagreement as well on how DRY you exactly have to be. Some people follow the rule of write it 3 times then refactor to be reusable, some want to do it immediately, etc. And having shared components first no one knows what is going on.
And then if they were to criticise my shared logic, and they want me to change anything, which could even be a change they request because of misunderstanding I will have to change all the other logic down the other PRs as well.
Thanks for the compliments, but I don't think I'm such a genius, and I manage this regularly. Splitting gets easier with practice.
It's the same as writing any technical document or a scientific paper: you have some idea, and then you decide how to split up the presentation so that your readers can make sense of it.
> E.g. if I split it in a way where I only show shared components, it won't be understandable why I made them shared in certain way because you won't see the actual other logic that uses them, so reviewers are kind of left to trust that these will be used in later PRs.
You can explain that with PR descriptions. Or you can have multiple clean-up commits in a single PR.
> A reviewer won't have understanding of the reasoning without going through the process themselves on how something might be reused.
You are allowed to tell them. That's what the PR description and code comments are for.
(It's better to put as much as is reasonable into code comments, because they are easier to see for future folks trying to understand the code. And, of course, even better is to make the code self-explanatory, where feasible. It's a hierarchy of descriptions.)
> And then if they were to criticise my shared logic, and they want me to change anything, which could even be a change they request because of misunderstanding I will have to change all the other logic down the other PRs as well.
That's always a problem, if you make a big change and only give it to review afterwards. No matter how you present that to the reviewer.
Yes, it is easier to write code that incorporates review feedback, if you can get review feedback early.