Big Pull Requests
Vložit
- čas přidán 1. 06. 2024
- DISCLAIMER: Graphite sponsors the channel, but they did not pay me to talk about this!
Wanted to talk about this awhile and been waiting for the chance to post it. Big pull requests are terrifying and I'm thankful we have better tools to deal with them.
SOURCES
graphite.dev/blog/how-large-p...
stacking.dev/
Check out my Twitch, Twitter, Discord more at t3.gg
S/O Ph4se0n3 for the awesome edit 🙏 - Věda a technologie
I'd argue that having 15 teams that need to review one team's code also doesn't work
Exactly. If you have to have more than 2 of anyone to review your code, you have a bigger problem.
As everything in programming, there are no absolutes. I think big PRs work in cases where they make sense. If somebody split up a complicated refactor into 5 PRs just to "keep them small", it might hinder my ability to piece all of those things together and see if they make sense like that. It also might happen that I reviewed PR 1, then somebody else did PR 2 without seeing first one so they don't even have previous context.
It's just not that black and white. But I would actually like to see some real life example of this stacking workflow. Reading about it doesn't mean anything. I want to see it shown by someone actually experienced with it.
I unknowingly used such a workflow multiple times, because I didn't want to wait for my previous PR to be merged. The only issue is that a lot of PR tools can't easily show only the stacked changes by default. I usually put a link to the commit diff in the description of the PR to work around this. Some tools I used did allow specifying dependent PRs, but didn't allow for a chain of dependent PRs.
I know a guy who has a 15k-line PR on his resume, made to Apache Airflow. The only thing he did was unified the buildsystem in one of their monorepos (I think, can't remember too well)
So technically big PRs "can" work, but they generallly must be made really well and not that exhausting to go through by the rest of maintainers.
loc is just one of the measures, and even is the least important dimension to measure if a PR is too big or not too big. However, because it is so easy to compare, visualize and communicate, ppl keep using it from time to time. even comparing with different systems, languages, domains, industries.
The thing is, when someone does such routine refactoring, you still need to verify that they didn't sneak in something bad, either deliberately or accidentally, and that's hard. See the recent xz events.
@@vytah Mentioning XZ is like mentioning that the sky is blue...
Everybody has heard of it by now.
@@shapelessed Thx. this speaks my mind.
Did he made any autoconf changes within these lines?
Instead of micro managing PRs do we just need source control innovation at this point?
Git is fairly simple (until it isn't) but I sometimes wonder how it could be better, specifically at scale. Monorepos are also heck.
Agreed, I just don't know what that looks like in practice. Maybe something like "git but with semantic diff like graphtage"
Here is how we do it
we have big FEAT branch and small feat branches
when we start on any project, we create a big FEAT, then every dev work on small feat and merges to big FEAT
small feat always gets review, and big feat is handed to QA to test (with each small merged to big FEAT)
We have some dangerjs rules on small feat regarding the number of files and the number of line that can modified/added
We do something very similar. Works well 👍
in this case, big feat would pretty much be a nightly branch, right?
Yeah, this is actually what's nice about mercurial (which is used at meta.) You can have several "PR"s in the same PR, and each part is reviewed.
@@comfysage Yeah, my team has a large `Develop` branch, that features get merged into, and sometimes that is recursive.
@@comfysage not necessarily, as there can be more than 1 big FEAT at any given time
8:50 Does the PR for feature B include the changes for A in the diff?
Usually I create a PR from B -> A so the diff excludes A, but allow reviewers to begin looking at B.
Then when A is merged, repoint the dependent PR for B back to main.
I didn't know others had begun to formalize this concept as "stacking".
I agree, big PRs produce lower quality code, and are risky.
Thanks for sharing! :)
GitButler really helps make trunk based workflow look a lot more like the stacking workflow. Its so nice to be able to be working with multiple branches applied to your working directory at the same time.
My theory, the reason merges happen faster with many more files is that the vast majority of big changes are either actually a bunch of small nonsense changes like formatting or it’s a conglomeration of code that has already been tested many times.
I like the Graphite tooling. But I wish
- merging was faster (merging a bigger stack takes ages)
- I could have multiple trunks (monorepo) or could just opt-in to track a few branches without any connection to trunk
"Graphite does sponsor the channel, they are not specifically sponsoring this video"
"So hyped Graphite was down to sponsor this video!"
🤔
My bad, description was ripped from another video I have coming up with graphite! They did sponsor the other one and I wanted to get this one up first :)
@@t3dotgg Maybe you need a stacking workflow for your video descriptions as well lol
@@t3dotggyou’re very erratic so it’s not surprising you missed that
@@t3dotgg All good, great video - keep up the interesting content :D
interestingly, ive been using this technique somewhat without ever realizing it. when i had a large PR where I was refactoring a heck ton of code, I split it into smaller PRs, each branch of the subsequent PR was dependent on the previous PR's branch, and every PR was bite sized.
the only pain was if I had to change something in the lower PR, all upper PRs had to fix merge conflicts.
for me, I setup just 2 branches master main and dev branch. Since we are group consist only on 3. Setting up a smaller branch is easy to handle for PRs
Thanks for showing this. You explain stuff very simple and precise. But I would like some more advanced examples. This seems easy if I work alone on a feature. How do I collaborate with my team? Can multiple people add stacks and modify the same stacks? We are always multiple developers on the same task.
One of the biggest problems that hasn't really been explored in this article is that big PR's can be required by the Agile process of the company. The last client I worked for would tie large features to one ticket at a time and then tie tickets to branches. So a branch couldn't be approved unless it met all the acceptance criteria of the ticket that was written by the client. So this approach is fundamentally at odds with this project management style which I've found to be the source of all of these issues.
it could be a case for subtasks where you would just make 1 branch = 1 subtask
You probably can just make subbranches with PRs that merge into that branch.
like others said, sounds like only the nearest branch must be approved. you can create and approve subbranches however you like
Here’s the system I helped implement at work on a team with about 10+ devs.
From master
- QA testing branch
- dev branch (this is where most PRs are made, it’s like a playground
- small feature branches that are tested in dev, and once tested in dev by dev team, we then go to merge this branch into the QA branch.
- Production (is merged into from the approved QA branch).
Love this type of content! Thank you Theo
Stacking seems like it requires a lot more mental overhead, and code organisation by the PR creator.
I often make changes and commit often, meaning the code changes I made are among many different commits.
Thoughts?
I'd love to know in which context a PR requires 5+ teams to review code. That sounds absolutely insane to me
Then again, I've only worked in companies with dev teams no larger than 25-30
You're scared of work! That's whacha' scared of!
But in all seriousness, If I saw a PR as big as on the thumbnail, I would really consider pointing the person who made it to a good therapist.
I think it depends on what the PR is.
I've worked a lot with legacy code, and sometimes something as simple as just converting all files to use tabs instead of spaces of mixed results in a massive list of changes, but with zero functional changes.
Similarly when we moved to using openapi for API documentation we needed to restructure our existing docs for the methods, but it's just annotation changes that are very easy to review simply by looking at the compiled openapi.json file rather than individual lines modified.
And the most recent time I saw something like this was with a legacy application were we committed the minified JS codebase, even a small change resulted in the dist.min.js file having huge changes because the minifier would choose new variable/function names and/or wrap the lines in different places. As long as the file looked to be updated to match the latest changes no one ever looked manually.
If someone modified thousands of lines of code by hand with functional changes attached then yeah it should be rejected.
#ainiddinEnglish
I once did +100k -50k PR. Can you share some therapist contact?
I do stacking sometimes too, but it really sucks when you miss something and are hit with a change request that affects something that might already be approved. For bigger and more complex PRs we might do a sync review and it’ll cut down a lot of reviewing time trying to build context and understand the code. However, I’m at a much smaller company that doesn’t have that many teams working on the same code base.
Is this an issue with people not knowing how to use git? I've always used rebase and this has always been easy.
Rebase from CLI is scary because some details get loss in the shuffle. It is easy to misunderstand what theirs and yours means and picking changes is much easier using a GUI. Granted, git has an option to use GUI when merging or diffing which fixes a lot of this.
I rebase but only with a GUI to prevent mistakes. Git is extremely dumb and that can be awesome and it can be painful. Rebase is a destructive action. Use wisely.
Yikes, just learn git. It's not that hard, and you can recover lost commits easily using git reflog. @@JacobSantosDev
Squashing can be just as bad as rebasing.
I definitely have encountered this kind of problem (minus 7+ teams needing to approve; that is just insane). But I feel like the real solution is something like trunk based development and real CI. By real CI I mean making sure no feature branch gets too out of sync with the main branch. If you need longer lived branches, you should branch smaller pieces of it off of it and merge back in very frequently. That way you can be more confident things are not getting too out of sync and minimizing conflicts. There is nothing worse than working on a long feature branch with others than to encounter conflicts you don't even know how to resolve properly because of the distributed nature and the synchronization problem. Have a culture of real CI in development, use testing and merge as often as you can. That is the solution in my opinion.
I worked on a branch like that. We were still releasing new versions from master, and that was a rewrite of one of the core system, so it wasn't possible to just "add it in smaller chunks" (first, there was no way to just slice it, and second, it would destabilise the releases - there is a reason we made a whole major release out of it). Instead, we'd merge master to that branch every now and then to resolve conflicts bedore they get too big (and not allowing anyone else work on related core code, which wasn't too much of a limitation).
I've been doing stacking manually simply because waiting long in between complex new system development that touches on existing code will inevitably reduce it's quality of i can't work on it uninterrupted maintaining the domain in my head while doing it.
I'll definitely take a look at the tooling to see if they could help me out.
I've being doing a less refined version of this without realising it. It just made sense to me.
I get you. I love small PRs because it gives me time to really give good feedback and fully evaluate and test the PR.
The problem I have is that any process has to work within the team. Is small PRs a good thing? Definitely, objectively so. Will every team employ small PRs? No. I haven't worked at most companies but I will make the prediction that most companies don't Agile as well as they think. They don't even Scrum or Kanban as well as they think.
That is fine. A process is only as useful as it helps the team continue their flow unimpeded. If smaller PRs are preventing and blocking people in the team than it doesn't matter how good they are. Large PRs will continue to be a thing.
Managers and Leads also forget that they can't have contradictions in the processes. If the rule from the project managers is "One PR per card" or worse, "one PR per Story" then it doesn't matter what policy you have for submitting PRs, you are only going to do one PR per story.
There is a saying, i forget the exact quote, but it is along the lines of, "Make it easier to do the right thing without thinking of it". A policy should not dictate what you should do but provide the boundaries for doing the best thing possible. Something something good UX design.
TLDR: you want it to be so easy and quick to do stacking PRs that devs naturally flow into the process while still allowing for larger PRs when possible.
Part of that might be breaking stories and cards down to where you have the least amount of changes necessary for the story, task, card, etc so that the PR is naturally as small as possible.
I would say that the reason PRs are large are that the story/task is large and needs to be refined to limit its scope.
I worked for a team where it was one PR per card and they also had a policy that tasks could not be cards. Cards had to be stories of at least the equivalent of a day's work (might have been half a day).
This is why small microservice lambda functions are great, easy to update and easy to review.
Turns out I was unknowingly already using a best-practice :P
A friendly CTO recommended trunk-based development to solve PR hell. Whats your take on that?
Dave Farley from Continuous Delivery recommend that but I’m not quite sure how it would work in practice
@@edumorangobolcombr Usually you'd make heavy use of feature flags (or something similar) to ensure the trunk can always be built and released. It gets a little hairy with DB migrations (especially if you're dropping columns or tables) but if you have a good migration strategy it should be relatively easy to deal with.
Of course you will almost certainly run into the problem of managing your feature flags. So it's important to stay on top of them and remove them as soon as a feature is fully completed as to not have your codebase littered with them.
Other than that, if you make sure to keep branches short lived (like, they can't live for longer than a day or two) trunk based development works out pretty great, at least in my experience.
@@kiramaticc This is the one topic I've seen people both enthusiasts and detractors eat each other alive, at least from my experience. But I'd like to see the topic covered
So instead of getting a 1500 line diff I get a hundred MRs that are each 15 line diffs. Yeah, that isn't the improvement you imply it is.
If you're the only one reviewing, probably not. I think the idea is for when the review/approval work can be distributed.
really interested to see you make a video using stacking + graphite + feature flags
Theo, does Prime go with you skating too?
That level of fragmentation is nuts to me, but yet it happens so often. My suggestion is just to have a BDPP, a modification of BDFL to apply "per project" instead of "for life". Then that single person is the one to consult for all changes and no need to wait on a team.
Or automate the test suite in CI/CD so you can immediately fix the tests after replacing tabs in 50 files instead of being forced to roll back your commit last minute because the tests failed.
What I don't understand is why do we need to create a new feature branch b off of feature branch a? Why not just create the first PR and then continue to work on that branch, make the frontend changes and then create another PR?
does this not mean that PR A is small but PR B is big ? (containing code from both A and B) or did i miss something ?
But isn't git branch already a stack? This seems like branching & rebasing with extra steps.
I feel like I'm getting all of these advantages except where they work with "PR's in a stack" I work with commits in a branch. I can do it because I got good at interactive rebase. (With mostly vanilla CLI, yeah, deal with it.)
I start with a feature by creating branch off main. But as I do refactoring and various fixes, before the feature is ready (or doomed) I usually end up building the branch up to 3 to 5 commits. E.g. 2 of them cleanups, 1 doc fix, 1 refactoring, 1 the actual feature. Remember, this is all in one branch -- `dev` off `main`. As I'm testing and improving, because I can always rebase, (meaning squashing, splitting, reordering, editing commits) I work on the individual commits just as they were separate PR's.
At any point, I can move any commit to other branch, so I can do most of my initial work like this without really thinking how do I split it to branches / PR's. I only make that decision as I'm getting ready for request -- and honestly if I'm working with someone who knows I can rebase/split as they need, they don't really need to insist on splitting things too much. As long as I make the individual commits easy to review/audit and reversible (which should be any SWE's priority anyway) it does not really matter per se if I have N commits in one PR. (It matters if I mix them wrong, eg. have multiple larger things with varying scope or priority.)
I know, I said part of why I can do it is that I learned rebase, and lots of people are scared of that, but I promise you, while it's lot of pain to learn (hint: having another window with eg. `watch git log` so that you can see what you are doing in real time helps a lot) it will give you ton of power.
10:20 how can you change `feature a` without invalidating all the stamps you got on `feature b`?
And before you say, "because my codebase is architectured well" then OK, great job, but if you can say that, then you could still have `feature a` and `feature b` in the same branch, right?
It's still just a merkle tree; PR's are just fancy-ish pages that allow referring to nodes and whatnot, but you know you can have a workflow where you do reviews on individual commits, right? (I don't know about github but with gitlab you can comment on commits, regardless of branching/PR's.)
Now if the reason is that they are "too unrelated" then again, why have them in "same stack"?
I’ve long felt uneasy about large reviews, but we are a small team doing greenfield development. New features require touching db, orm, services, api, and the front end. Any one part is not useful.
Stacking is interesting, but could still slow things down. We have called them checkpoints.
Idk I feel like this is 100% about separation of concerns. He said it’s not but what else could it be separate huge pieces of functionality into separate PRs
I: "is it working?", you: "Yes", I: "merge it" hahah
Dude, today I had a PR named "Refactor CSS" which was +26K lines and -52K lines. I mean it is good that we are with 25K lines less but honestly when I was requested to review this I was just wtf??????
just hit accept, what could possibly go wrong
2 minutes ago is sublime
I think I’ve read this article before, but it didn’t stick. Still kinda unclear on how stacking works I guess. Would be nice to see someone actually using it in a workflow
Locally you just create a new branch from the current branch.
Main -> branch1 -> branch2 -> branch3
Each of you branches should be it's own PR.
Branch1 -> PR1
Branch2 -> PR2
Branch3 -> PR3
The PRs should be dependent on the previous PR and the first should merge into main.
Main
@@asdf8948 When you open the PR, what do you base off of?
If you base PR2 off main, the reviewer sees the diff of Branch 2 and Branch 1. They're left unclear on what they're reviewing
If you base off Branch 1, the reviewer does the review under the assumption that the code in Branch 1 won't change, which it very well might.
Neither of these scenarios seem good to me
@@asdf8948wait, does this mean that PR2 merges into branch1 not main?
Just have many huge PRs and merge them directly to main which will be on prod on friday. yolo.
With stacking no going to beach for you.
A year for a pull request?! 🤯
I was with you until 8:00.
Then I was ready to throw money down and pray to the gods this becomes normal.
Luckily i work in closed source software and don't really need to worry about people trying to sneak malicious code past me because if they did we'd know their names and adresses and they'd be in huge legal trouble lol
In open source though this is a very real threat and a major problem
How do I know you didn't sneak something in?
@@cherubin7th Security audits which happen every month, benchmarks that run on every release and the fact everyone else in the company uses the stuff as does the public and if anything were ever found it would quickly lead back to my name and i'd be quickly fired, blacklisted from other companies and facing a lawsuit on top.
It's not really worth it, you know? Too much risk. In OSS because i'd be anonymous i wouldn't really need to care, just make sure i can't be traced by IP and i'm safe.
Oh yess the hair!
Using Graphite for a while now
While helpful, its more annoying at times
Constant restacking is such a pain
A change to one pr and 5 other stacks on top need to be changed
Having to split prs arbitrarily - super annoying to deal with
Also a pain point is Y shaped dependent prs
Say a pr depends on two unrelated prs to be merged
You can't stack on either
Looking into gitbulter currently to see if it helps somehow
Why not use Gerrit?
Anything not GitHub-centered ?
I love graphite!
Notion catching strays out of nowhere 😂💀
Turns out i've been doing stacking for a few years now not knowing its a "thing" in the industry
Smaller PRs are good for mental health too
i like big pr and i cannot lie
sounds like rebasing.
Forget traditional PRs! Train and hire great engineers, align on values, invest in test automation, merge low stakes changes to main without code reviews, and review code later when you want to learn.
want more views from Poles? change 2,105 to 2,137 on the thumbnail
What does this mean
@@ononaokisama It's Polish meme number.
"In Poland, 2137 refers to the time (9:37 PM) when Pope John Paul II passed away, which is a significant moment in Polish history and culture. This event holds emotional and cultural importance, leading to memes and jokes referencing it"
btw yesterday was 19th anniversary of his death
@@ononaokisama the other person gave you an explanation, basically it's Pope John Paul 2 death time. I've seen a map showing "funniest numbers in countries" and all countries around have 69, and then Poland has 2137. It's probably because the youth is getting less religious but still fed propaganda about the Pope by older generations.
You can just use cherry picks to avoid rebasing
Isn't cherry-pick the more tedious option? Since you do it commit by commit.
But that argument in the video )oe ratger the article) "rebasing manually is too tedious" is weird, you still need to rebase or merge (and handle conflicts) for every PR you have.
@@tymondabrowski12 rebasing is weird for stacked branches, but works great for everything else.
You can make a bash alias to remove your last commit, reset to a new origin and cherry-pick it back.
@@spetz911 I'm sorry if I misunderstand something but wouldn't that be a direct equivalent of "git rebase master"? Assuming the master already has those previous changes that you build on top of (which it should, since it's supposed to depend on them anyway).
@@tymondabrowski12 if you made small fixups to your previous commits, then rebase would likely fail. Check the “Pro Git” book: “3.6 Git Branching - Rebasing” section “The Perils of Rebasing”.
Maybe I didn’t fully understood a question, but I hope this info is helpful.
"© 2023 Ping Labs. All rights reserved." - from uploadthing webpage (why 2023??)
Why on earth would you "catch a bunch of things" in a 10-15 line PR unless you are working with a bunch of Jr's or a really complicated codebase and the submitter is new to it?
1k lines is a "too big" PR? 500 lines is too much to review properly?!
A PR should be properly sized. What's the proper size? IT DEPENDS.
I will say this though.. PRs, like any process that involves handoff, come with a ton of hidden overhead and that needs to be considered strongly. Personally, as a very senior engineer and previous maintainer of a substantial OSS project, I don't have the time to review 10 intermediate PRs before we have even arrived at a complete draft solution. God thinking about the context switching costs alone make me real. I want to see the big picture so I can review it holistically.
Again, it depends. But for most of my teams a PR should be delivering some value on its own, or be broken off for some strategic reason other than "500 lines too much".
No please don't do stacking changes, just call the guy and approve your PR right away. Practice CI and extreme programming, and you will know what speed is. Also remember to write tests.
What if the reviewer is in different timezone or working on higher priorities. It's unrealistic to expect immediate review or even same day for smaller teams.
"Approve your PR right away" - in what kind of organisation or company is it realistic? Are they going to stop everything they're doing to get your code, build it, see if it really does what it's supposed to etc.? And you need to wait through the whole process only because you can't branch out from the previous work? Why not continue working and just do a (local) rebase when it's accepted?
@@trappedcat3615 I agree that if your team works in different time zones it won't be possible, but that's a managements thing to fix
@@tymondabrowski12 Don't iterate on assumption that you code is good for prod and ofcourse your teammats can be bussy at the moment but ultimately whole team should have only one goal for current "Sprint". Remember that PR's not only are for checking code quality, but also for shearing knowledge in your team, detecting future bugs and validating next steps.
@@TheJubeiam True. It depends on many factors. Sadly these decisions can be based on economic paths before development.
Real problem, dumb solution.
Real solution : Trunk based development, do peer/ensemble programming instead of PRs, trust your engineers, create a COP (community of practice).
Tech solutionism is a plague, you don't (always) need tools to fix problems. Be more pragmatic
I'd add to this: evaluate your design if you need to touch so many files each time you want to introduce even a smallest, simplest feature.
mai blog hi padh leta hu, video dekhne ka koi point nahi hai
This is why PR and feature branches are the most effective way to slow a project down. We use CI and one branch and a common design review once week.
"Stacking" isn't some hip new thing.. Devs that use excuses that they can't do work because something needs to be merged are just being lazy. Which is fine.
A bit abstract for someone that does not understand a pull request
Why would you watch an in-depth video about a topic you don't understand and then complain about it?
first
Great. Now what.
#ainiddinEnglish
comment
immediately rejected. break it up into smaller parts. sorry
This is why my projects typically are a single 30 MB file.
THEOretical concerne hahah hihi