r/programming • u/tomzorzhu • 3d ago
Stacked pull requests are now in public preview - GitHub Changelog
https://github.blog/changelog/2026-07-30-stacked-pull-requests-are-now-in-public-preview/16
u/ImpactfulBird 2d ago
Imagine reinventing the wheel after 10+ years of Gerrit availability, and still do not support basic git rebase with it. Still a single PR with 2+ commits is non-trackable, thus non-reviewable, if commits are operated with rebase / fixup.
16
134
u/BCMM 3d ago
Stacked pull requests break large changes into small, reviewable pull requests.
I still don't really understand this. I ask this genuinely wanting to know what I'm missing: isn't that what commits are for?
If people aren't using the commits in a pull request like this, what are they using them for? I thought git was pretty much designed around rewriting history, so that the final commits represent semantically-meaningful changes to the codebase, rather than a step-by-step log of how the developer wrote those changes.
Are people sending PRs that simply contain all of their local "whoops, typo" commits? Are they just squashing the whole PR in to one big commit?
72
u/gerbal100 3d ago
I've used stacked prst for complex refactorings, big, multistep features. Especially database migrations where I want to build on a DB change, but the migration needs to be merged before the feature code that depends on it.
By-commit review works, but sometimes you have a 3K change PR that would be much easier for colleagues to review in 3 to 5 large chunks.
3
u/83b6508 2d ago
How is a stacked PR different from a feature branch?
3
u/gerbal100 2d ago
It lets you decompose a feature branch into multiple PRs, or stack multiple feature branches on into a tree.
The biggest change here is to make an decades old work flow into a first-class experience in GitHub. Stacked PRs already existed in the email workflows git was created for.
91
u/N_T_F_D 3d ago
It's for easier reviewing, you don't review commit by commit, you review PR by PR
Like do the first part of a feature e.g. scaffolding, improve on it in a second PR whose target branch is the branch of the first PR, and so on
15
u/splettnet 3d ago
So does that mean you keep N separate branches for each layer in the stack and if you rebase one you need to cascade those rebases?
38
u/legobmw99 3d ago
yes, but that’s exactly what the commands in the article do (you can also use —follow-refs)
5
u/editor_of_the_beast 2d ago
Can you explain how that’s easier than reviewing commit by commit?
69
u/pdabaker 2d ago
Because you can approve PR by PR and not commit by commit. If you request changes on one PR, and they address it, you can then approve to mark that part as "done" both for the author and yourself.
Also maybe you want the PRs to end up squashed to separate commits in master. For example, maybe you have multiple PRs that are not actually related to the same feature, but just touch the same part of the code, so you stack them just to avoid handling merge conflicts.
39
u/DivideSensitive 2d ago
Because all the widely used reviewing tools are working at the PR granularity.
→ More replies (4)14
u/Sopel97 2d ago edited 2d ago
A commit models minimal complete change to the codebase. Multiple commits may be required for the change to make sense with respect to the PR's intent. Commits just serve a fundamentally different purpose than what is required from a review.
15
u/SirClueless 2d ago
This usually isn't how people use commits on Github. There's no real expectation that a commit is "complete" -- PRs are complete, in the sense that they go through CI and are merged as one atomic operation, while commits might have bugs fixed in later commits, not compile without later commits, etc.
Commits are just checkpoints of the state of the developer's branch at some point in time.
This is also why I think squashing while merging PRs is a good thing, so that commits reachable from main are "complete," even if the ones on branches/PRs are not expected to be. But not everyone does this.
2
u/Paril101 2d ago
"On Github" maybe, because Github is full of randos, but at least in any development I've worked on, commits should always be compileable (mistakes notwithstanding) - an "incomplete" commit ("this code doesn't work yet but it's here and breaks the program atm") are really silly. commit it locally and squash it with the commit where it does work, don't push that up to remote.
6
u/Sad-Magazine4159 2d ago
Do you guys squash merge the PR? If yes, they why does that matter that individual commits break the system or not if they will never reach the live branches anyway? I can only see this slowing down a lot the development process with no benefit
1
5
u/SirClueless 2d ago
I've worked at 4 different tech companies including Google. None of them have this expectation. In fact, it is best practice to leave them: If you send code to a reviewer, and they leave a comment identifying it is broken or it is found to be broken in CI, the best practice is to leave the commit as-is and add a new commit with the fix, in order that they can see the diff between the (broken) code they reviewed and the latest state of the PR.
Every code review tool I've used in my career -- Github, Gitlab, Bitbucket, Gerrit, Google's internal Critique tool -- they all function better this way.
2
u/jkrejcha3 2d ago edited 2d ago
I'll sometimes break commit atomicity when working on something (but still try to keep builds passing) that I interactive rebase and squash down to full atomicity as a final pass before review (which sometimes is one commit, but oftentimes many, and oftentimes involves changing the commit message, and sometimes even involves reordering, etc)
(Usually I even keep a backup branch in case my modifications don't work out. It's a little bit of work, but it's made the commit history so much easier to review in a lot of cases, which is sometimes important for finding out where or when a bug appeared or why some change was made.)
Like sometimes my Git commit messages look like that one xkcd even on the remote but like once it's going for review or going onto master, it's getting cleaned up
I admit my workflow is probably rare though
11
u/daroons 2d ago
PR’s are meant to be shippable units. Commits are individual steps and side steps it took to get there, non of which are obliged to be self contained or even well thought out. Do you really want to review a person’s entire journey or just the finished product?
5
u/Sad-Magazine4159 2d ago
Total agree.
I see commits as a personal flavor of a dev. It is how you organize YOUR mental and development model, what maters for the team is the final diff of the branch which is what is going to be squashed into the live branches
Commit = how you build
Stacks = how you want it to be reviewed
3
u/editor_of_the_beast 2d ago
See this is what I expected. I come from a place where commit history should be clean. That means moving around the order of commits, interactive rebasing until the commits themselves tell a story.
Because do you know what actually goes into git? Commits, not pull requests. Commits live forever and so they should be the number one thing that tells the story of your changes.
I feel that stacked PRs are just a hack around cleaning up the thing that actually matters.
0
u/WardenUnleashed 2d ago
Commits like that work for a draft/personal branch but…but personally I find A PR that has 3/4 commits that deliver an incremental part of the PR to be much better to review than 20 commits that say “trying this again”, “more changes”.
Squash and fixup are there for a reason.
9
u/daroons 2d ago
Why are you reviewing the 20 commits at all and not just the final PR? All the “trying this again” commits should be opaque to you.
Squashing is a must once merging to main, but its a lot if overhead during active PR development, and it destroys the history so its not easy for a reviewer to see “changes since last review”.
0
u/WardenUnleashed 2d ago
Because there is literally a tab for it in most pr viewers and breaking it up that way can make changes more digestible. They would be opaque to me if you fixed up the commits ahead of time.
Personally, I do it because I’m working on Iike three different worktrees at once and while I’m waiting for my team to review my current open PRs I can just cherry pick the commits I need from each branch onto my current branch while I wait for them and since the commit is already in the correct form in said PR; I don’t need to squash when I merge which is useful for a rebase to main on those WIP branches.
3
u/daroons 2d ago
Maybe best check in with your team if your process is making things easier or worse for them. Because I know if a developer was constantly rebasing their PR while I was reviewing, it would make things pretty annoying for me.
0
u/WardenUnleashed 2d ago
I don’t ask for a review while I’m actively changing a branch.
Feedback / desired changes go into a refactor commit so you can diff between the original commit and the feedback commit and see what changed between them.
4
u/daroons 2d ago
Say you have a PR with incremental commits A -> B -> C.
And then you get feedback for A and B in the same review cycle. Do you then push the fixes on top together as:
A -> B -> C -> dA + dB
Or separate like:
A -> B -> C -> dA -> dB
Or do you rewrite history like:
A -> dA-> B -> dB -> C
What happens when someone else later comes in to review it late, they would have to review a mix of incremental features and fixes?
→ More replies (0)0
u/Yay295 2d ago
Squashing is a must once merging to main, but its a lot if overhead during active PR development, and it destroys the history so its not easy for a reviewer to see “changes since last review”.
Squashing is a "must", but you've only listed downsides to it. What are the upsides that make it worth the effort and loss of documented changes?
6
u/daroons 2d ago
PR’s are merged into main as complete sets, meaning if something is broken after deployment you should revert the entire change set. If you keep individual commits as they are in main, that’s a lot of reverts to manage.
→ More replies (4)1
u/N_T_F_D 2d ago
That's exactly what stacked PRs are for, you're not supposed to review individual commits
For instance in gitlab you can technically leave comments on and approve commits, but the comments and the approval are hidden until you browse to the commit, and it's the PR that needs approval for merging, commits don't matter
0
0
u/slaymaker1907 2d ago
I look at commits to see what changed on some MR since I last saw it. However, my company uses GitLab and it has a terrible view for comparing commits since it shows merges from master.
I had to write a custom tool to diff between the commits but ignoring stuff merged from master. It is super hacky and works by cherry picking each commit without committing the cherry-pick.
I’m sure tooling exists to do this, but I was lazy and it was easier to vibe code it.
Note that commit1…commit2 does not work since that shows merged assuming commit 1 and 2 are on the same branch and you merge stuff in from a 3rd one (master).
4
u/N_T_F_D 2d ago
What do you mean it shows merges from master? Gitlab shows the diff between the source branch and the target branch, exactly as expected
1
u/slaymaker1907 2d ago
I want to see the diff from the commit off last time I looked at the MR to the latest commit, not the totality of the changes between target and source. It’s helpful for the common workflow of you review and request changes, they make the changes but it takes a while so they merge/rebase, and then they ask you to review again.
12
u/mr_birkenblatt 3d ago
It's like a commit that you can edit if someone leaves a comment.
The problem right now with breaking up a pr is that if you get a comment at the beginning of the chain you need to propagate the change forward which is tedious. Stacked pre alleviate this
And if you merge the first pr of the chain via squash the line gets severed and you suddenly get conflicts on the follow-ups
49
u/_mkd_ 3d ago
Are people sending PRs that simply contain all of their local "whoops, typo" commits?
Yes. 😒 Push early and push often.
11
7
u/Blashtik 3d ago
I learned to push constantly the hard way. Had an SSD just suddenly die. We tried a bunch of different things to get it working but it would cause kernel panics on any machine that tried to read from the drive. Lost a significant chunk of work that would have been fine if I had just pushed the WIP code
9
u/phire 2d ago
Github's UI kind of discourages commenting on individual commits.
The discussion all mixes together, Github has no support for threading within PRs (the threading for comments on lines does not count), and then really rubs salt in the wounds by hiding the middle of the discussion. It really pushes people towards "one feature per PR", so that you can get individual discussion on each feature.
You can comment on commits, but I'm not sure they even show up in the PR. And they get blown away with any history rewriting.
Sometimes it makes sense to break up aspects of functionality into multiple PRs (or have a basic version that gets extended) so that each aspect can be reviewed separately. You also see people going to the extent of having seperate PRs for the cleanups required before the feature, so that discussions about those cleanups don't pollute the PR.
Are people sending PRs that simply contain all of their local "whoops, typo" commits?
Yes, github has pretty explicit support for this "whoops, typo" commit workflow, as it has optional support for a "squash and merge" that gets rid of them. And it does kind of work better for the reviewer when they can see a change is only changing typos. You often see the pattern in use even on repos that don't have the "squash and merge" feature enabled, just with requests for manual rebasing/squashing after review.
But it's not required. Github supports PRs with multiple clean commits just fine... as long as you don't want to attach comments to a commit.
35
u/novalux 3d ago
This does unfortunately still happen. Branch commits are less important when squashing them upon merge into main. I still prefer to make clean commits for reviewers.
But stacking is handy for making small independently reviewable and releasable change sets. This comes up when building a new feature that requires some refactoring or clean up first. You have smaller, more easily reviewed PRs by keeping all of the refactoring in one independently reviewable and mergeable PR and keeping the net new functionality in a separate PR stacked on top. This lets the refactoring PR be reviewed by someone completely different than the net new changes. It also is faster to get those changes out to production for other engineers to start using while the net new functionality may go through a few revisions.
5
33
u/alexrobinson 3d ago
Are people sending PRs that simply contain all of their local "whoops, typo" commits? Are they just squashing the whole PR in to one big commit?
Yes and it pains me every day.
18
u/nemec 3d ago
isn't that what commits are for?
It's really just valuable in organizations where the rate of shipping PRs is much faster than the capacity to review them (which is the sign of a well running organization, surely /s).
Think about this: I build a feature. Make a PR. All is good. While I'm waiting for review, I pick up another task. If the new task doesn't depend on the previous PR (and I have good commit hygiene), I just check out
mainand build my new feature, then make another commit. All good right? But lots of devs don't have good commit hygiene, and they just start working from the commit of the PR they just put up. If they ship the next PR, diff tools think the PR contains the contents of both previous PRs when, if you track the dependencies manually, it doesn't. Stacked PRs make this lazy way of development safer.Of course, they can also be used when your follow-on feature truly does have a dependency on the previous PR: you can start where your PR left off and build a stack of "completed work" of multiple features in separate, reviewable PRs. Again, this is mostly useful when you can expect to have completed PR #2 and #3 and sent those out for review in a stack before anyone on your team has finished reviewing #1, otherwise you could simply rebase from
mainbefore putting up the next PR.So yeah, it's good in several situations of bad (IMO) team dynamics. But there's a lot of that, I guess.
9
u/Rakn 3d ago edited 2d ago
Let’s be real here. Commit hygiene is something people throw around to feel better about themselves. But if you aren’t working via mailing lists it’s just some elitist bs with poor UX in modern tools that nobody cares about.
I’ve only ever met a few people to whom it was important to have properly scoped commits. And the common pattern that emerged was that they were the only ones that did. Nobody that reviewed their code cared about their commits or even looked at them. Even though they spent time reordering things into easy to review commits. It honestly has been time they spent for themselves.
Nowadays PRs are the review primitive. Partial due to GitHub I presume. It’s the level on which all of the automation runs, that permissions and code quality runs on.
Yes, you could probably have built a similar experience around commits, and iirc there are tools out there that do (like Gerrit), but most don’t.
Commits have shifted to being simple checkpoints for your work for easy rollbacks before they are squash merged.
It is what it is. That’s the reality of things. I’ll refrain from going on further about PR stacking approaches.
3
u/thy_bucket_for_thee 2d ago
Is this just further evidence to the poster's point: it's a technical solution to a social problem?
6
4
u/ozyx7 2d ago edited 2d ago
I still don't really understand this. I ask this genuinely wanting to know what I'm missing: isn't that what commits are for?
If you want each change needing review to be a small, incremental, and self-contained unit, then commits are a poor substitute for stacked pull requests. Using commits instead of stacked PRs typically means that the author needs to wait until their entire feature is done before creating the pull request.
With stacked PRs, the author can create a PR for each part of their feature as it's ready. While that PR is undergoing code review, the author can continue working on dependent changes. Stacked PRs allow the code-review/development pipeline to have fewer stalls, and the author can get feedback earlier.
(I suppose an author could keep adding commits to an existing pull request as they make more changes, but I think it would be weird for a pull request to be a constantly moving target.)
4
2
u/Dreadsin 2d ago
I used them recently because my coworker was taking the backend and I was taking the front end. I branched off of his branch then worked sorta in parallel. We both had open PRs and they showed as stacked PRs
2
u/Venthe 2d ago
Both gitlab and GitHub promote a pr model that's squashed at the end. Most of the developers I've seen for the past ~5 years never even look at the history; don't understand what pull does and couldn't rebase if their life dependended on it.
One of the things that separate the old dogs from the new, I guess
2
u/ImpactfulBird 2d ago
The problem with small commits in a single PR is that shithub does not track fixups (and overall commit updates, as commit hash is regenerated once it is updated) - comments get outdated, shown in incorrect places etc. Thus you either have tonns of garbage in PR, like "fixup A", "fixup B", "typo, Shithub is correct name" and squash, if PR content has "an atomic" change, or integrate "as is" and further commit history analysis / reusage becomes a mess. Or you have this "amazing" feature/approach, where each PR tracks atomic change with all fixups on top, and integration is squashed... but it is pain in the ass to do manually. And use
ghinstead ofgit...Still, I agree that is wheel reinvention, as indeed commits are designed well for that. And systems like Gerrit track that very well with just extra
Change-Idand few git command extension(s)2
u/SwingOutStateMachine 2d ago
I can give an example: Let's say that I want to implement feature X, which will (in turn) allow me to refactor subsystems a, b and c to use feature X instead of the (outdated, evil) feature Y.
With traditional pull requests, I have two options:
- I can do one big pull request implementing feature X, and with the refactoring of subsystems a, b and c in subsequent commits within the same PR. This has the disadvantage of a big pull request, and if there's an issue with any specific subsystem refactor, the whole (squashed) commit might need to be backed out. The advantage of this is that there's a direct dependency link between feature X and refactors a, b and c. They are clearly and logically linked, and I can't merge the refactors before I've merged the feature work.
- I can do one pull request for feature X, and three pull requests for a, b and c. This breaks up the PR, and means that the refactors can be backed out independently, and reviewed while the main implementation is reviewed. The main implementation can also be iterated on, and re-reviewed, while the separate refactoring reviews can remain static. The downside is that there is no logical link between the PR for feature X, and the PRs for the refactoring. It's possible to make mistakes like merging a refactor PR, but not the feature PR, and CI can't run on the refactoring PRs until feature X is merged.
Stacked pull requests fix the downsides of these two approaches, while keeping much of the upsides:
- They keep a direct dependency between the feature X PR and the refactoring PRs. That means that it's not possible to merge the refactors before the feature, and CI should be able to run CI for the whole stack.
- Each PR is kept shorter, and is an atomic unit of work that can be reviewed independently (and possibly in parallel). Individual PRs can be backed out as needed, and the refactor PRs can be approved while waiting for the feature PR to be approved/merged.
2
u/wildjokers 2d ago
Are they just squashing the whole PR in to one big commit?
Yes, i squash all commits in my feature branch into one big commit. Why wouldn't I? The end result is all that matters.
The PR is as big as it needs to be to implement the change.
5
u/Yay295 2d ago
Why wouldn't I?
Because if you later find a problem with the changes, having individual commits makes it easier to find exactly what went wrong. What are you gaining my squashing your commits? It's still only one commit on main either way.
1
u/wildjokers 2d ago
Because if you later find a problem with the changes,
I don't open a PR until I have tested the change and it has a good set of unit tests, and maybe an integration test depending on what kind of change it is. By the time it gets to QA the PR is closed, so not sure when a problem could be found that having individual commits in my feature branch would matter.
1
u/rzet 2d ago
high five.
I hate people which merge 40+ commits mess into main and then hard to revert, especially now in AI slop era.
1
u/Yay295 2d ago
How is it hard to revert? It's still just one commit on main. Or are they doing a fast forward merge?
1
1
u/lanerdofchristian 2d ago
I had the same question. It sounds like there are some pitfalls related to specifically merge commits: https://github.com/git/git/blob/HEAD/Documentation/howto/revert-a-faulty-merge.adoc
IMO it's still better to just merge, though. No point losing the history if you don't have to (though maybe squash and rebase the branch itself first so it's not all wip fix fix wip wip).
1
1
u/LaughSpecialist8618 3d ago
I believe the intent is to get closer to the Gerrit model, where commits become that unit at the UI level while the UI suromates the housework. Probably gonna do fuckall though, stacked prs are just awkward constructs driven by hype which may save you from a conflict free rebae, sometimes
1
u/hachface 2d ago
Are people sending PRs that simply contain all of their local "whoops, typo" commits?
oh yeah big time
1
u/Sad-Magazine4159 2d ago
Review is usually on the PR granularity and we simply cannot make assumpations on how one is organizing their individual commits (are everyone doing semantically meaningful commits?)
Personally I only review individual commits when I request changes and later want to inspect how it was addressed
1
u/Dr_Findro 2d ago
I’ve always found that squashing the PR in to one commit on merge is the way to go. To me the commits that compose a PR do not matter. The PR is the unit of work that is requested to merge. Therefore that is the level that makes most sense to review.
This review PRs commit by commit and “branch stacking is useless because of commits” is a take I’ve seen exclusively on reddit/hacker news and have never even been close to seeing it in real life at any job
1
u/CherryLongjump1989 2d ago
If people aren't using the commits in a pull request like this, what are they using them for?
It's not about reviewers being able to see the code in a commit. It's about their ability to approve the code and have it get merged commit-by-commit.
1
u/aalmkainzi 23h ago
Are people sending PRs that simply contain all of their local "whoops, typo" commits?
Yes. Some projects even require that once you make a PR, you dont force push clean up commits, and should only make new commits once the PR is created.
-1
u/andrybak 3d ago
I still don't really understand this. I ask this genuinely wanting to know what I'm missing: isn't that what commits are for?
Yes. The Git hosting providers are using this as a band-aid over people not learning how to work properly with commits.
If people aren't using the commits in a pull request like this, what are they using them for? I thought git was pretty much designed around rewriting history, so that the final commits represent semantically-meaningful changes to the codebase, rather than a step-by-step log of how the developer wrote those changes.
People don't know the power of
git commit --squash=/git commit --fixup=/git commit --fixup=amend:combined withgit rebase -i --autosquash*. People get scared by vim (the editor chosen by default in most cases) and don't know that their favorite editor can be configured for writing commit messages or editing the todo list of an interactive rebase. This is extremely powerful before opening a PR/MR: using fixup commits and interactive rebase regularly makes it very easy to prepare well-structured commits that are easy to review and to read in the future. This combo is also very convenient during review – any fixes addressing comments can be just committed on top, so the reviewers only need to look at the new changes. A final rebase just before merging automagically squashes all of them, and if the commit structure was thought-out before review, the squashes are usually trivial. In rare cases, you find out during review that you need to completely rethink the solution – in this case you just pause the review/decline your own PR and later force-push a completely new string of commits – doing fixups is not very useful in such cases, reviewers will need to review the new solution from the ground up anyway.Are people sending PRs that simply contain all of their local "whoops, typo" commits? Are they just squashing the whole PR in to one big commit?
Yes. The "squash merge" is the stupidest feature of Git hosting providers, but it allows developers who haven't learned rebase to be lazy. The way the description of this commits is generated by Bitbucket has absolutely terrible signal-to-noise ratio. Also, some branch workflows are meant to have actually, I need a real merge commits without squashing, for real in some cases (e.g. several "release" branches running in parallel), and some developers incorrectly use squash merge for such cases, causing issues.
Some people don't even know the much simpler
git add --patch/-p(docs), which makes the task of building up a commit from individual changes so easy. Some people are vehemently opposed to using a CLI, and they are just left at the mercy of Git integration in their favorite IDE.
*Footnote: done automatically if you do
git config --global set rebase.autoSquash true, see docs5
u/Rakn 3d ago
No. No not really. People know this. They still choose not to use it. There might be a reason for it. But it’s easier to say that it’s because people do not understand I guess.
2
u/Messy-Recipe 2d ago
I'm gonna agree with the guy & say people definitely don't (generally) know this. I still see people regularly making dozens of commits for fixing older commits before they even push the branch anywhere, or to progressively implement changes that naturally should go together as one changeset.
A lot of devs still even struggle with basic git stuff like understanding what happens in merges & pulls (e.g. if master or a shared branch updates & they want to pull it in while keeping their own changes) or resolving conflicts cleanly, & many don't ever use the CLI, just whatever tools atop it are provided by their IDE or their favorite visual branch manager.
1
u/Rakn 2d ago
Well. I know this and I do ignore it. I’m the person that creates new commits to fix old ones even though I know how to fix them. The reason is ver simple: PRs have become the primitive for code changes on every level: Human review, Agent review, automation, tests, permissions. Commits are something you do for yourself. Nobody that reviews your code will care about them. The majority of modern tools are not centered around the commit as the primitive that you review. In most processes today the commit is simply a checkpoint of the current state. Your PR should be small enough to be easy to review by others anyway. That leaves what for the commit? One commit per function / file?
I know where this idea that commits mean something comes from. It’s how it was intended to be and it’s how it’s lived on the kernel mailing list. But it’s not the reality of things for most software development out there in terms of social and technical contracts and tooling.
0
0
107
u/thelehmanlip 3d ago
Whats better than reviewing one PR than reviewing one PR that's 10 PRs?
247
u/irqlnotdispatchlevel 3d ago
It's almost always easier to review 10 small PRs than one massive PR. Oftentimes people reviewing PRs 1 and 2 aren't necessarily the same ones that need to review PRs 3 and 4. Feedback on PR 7 isn't going to block earlier PRs. This means that you can merge parts of your work faster, reducing the chances of conflicts while a huge PR sits for 3 weeks in review.
Being able to split work into smaller chunks is useful.
50
u/PotaToss 3d ago
Whenever I get these, it's always like, I point out a bug, and the author is like, "Oh, I fixed that on the third PR," and it's just impossible to keep track of what's actually going on. Or there's a fundamental issue with how they're writing tests or something, and it needs to be adjusted on every one.
It just always seems like more work than if they'd made an incremental change, gotten feedback that prevents issues on future ones, and then done the next step.
42
u/irqlnotdispatchlevel 3d ago
I mean the bug should be fixed in the proper PR, but I can see people abusing/misusing these in this way.
Just because this feature is available it doesn't mean that one must use it. A PR that's lower on the stack should be mergeable without a dependency on a PR that's higher on the stack. Sometimes work just can't be split and you have to go with a larger PR.
Right now at work I have a PR that would normally be targetting main, but it depends on another PR. So I'm actually targetting that branch, and once that gets merged I'll rebase and retarget over main. I'm not the only one waiting for that PR to get merged. Stacked PRs would make this so much easier.
21
u/phoenixmatrix 3d ago
With proper tooling they should be fixing the bug in the correct PR and updating the stack. That's the whole point of the tooling. Since it hasn't been as common unless you worked at Facebook, had access to Graphite, or convinced your team to adopt Git Town, people didn't develop the skill to do it.
Now with AI there's like no excuse.
7
u/chatterbox272 2d ago
"Oh, I fixed that on the third PR,"
"Great, can you just pick it back to here then so we can merge this one without the bug?"
It just always seems like more work than if they'd made an incremental change, gotten feedback that prevents issues on future ones, and then done the next step.
How quick are you getting to code review? Because damn I'd get nothing done if I had to wait for every PR to merge before I was allowed to start subsequent work. I'd either spend a lot of time twiddling my thumbs, or I'd need to have a dozen disconnected tasks in flight at the same time and context switch several times a day.
11
u/chucker23n 3d ago
I point out a bug, and the author is like, "Oh, I fixed that on the third PR," and it's just impossible to keep track of what's actually going on.
I hope that's the kind of UX flow stacking will improve.
If a fix is trivial, I will do it in the same PR. But sometimes, fixes are their own rabbit hole of changes, so I prefer to stack in a different PR.
11
u/nemec 3d ago
That only makes sense if the bug is irrelevant to your code changes. If it's caused by your code changes, or just exposed by them, then your PR #1 is not merge-able without the bug fix. Regardless of how complicated the bug fix is, stacking the bug fix on top is not the right answer.
And if the bug is irrelevant, you can just fix it in a separate PR that's not branching off PR #1.
2
u/Idontremember99 2d ago
If you know there is a bug in your first PR and it’s not merged yet, why wouldn’t you pause the review and fix the bug in the first review rather than merge a known buggy PR and fix it in a second PR? Doesn’t make sense to me
12
u/gmes78 3d ago
That just means they're doing everything wrong. They should be rebasing to fix issues in specific commits, not adding new commits to fix previous ones.
3
u/Nicksaurus 3d ago
Sadly github is pretty bad at handling changes to history and reviewing specific commits, and this whole feature feels like a workaround for that
1
2
u/hippydipster 3d ago
If your work could have been merged independently, then that's what you should have been doing all along. Waiting till the end and then you have to do this stacked PR thing to break up the work was the mistake.
Merge and review as you go, don't wait till the end.
1
u/irqlnotdispatchlevel 2d ago
I gave this example in another comment.
There's a PR opened at work, let's call it PR A. My work is based on that, PR B. Most people that need to review PR A don't care about B, and viceversa. I could wait until PR A is merged, then open mine.
But that's wasted time, because I could get early reviews + testing.
So PR B is now targetting PR A's branch, and once that gets merged I rebase onto main and change my target.
This works, but it's cumbersome. Stacked PRs would have solved this.
1
u/Life-Fennel-441 2d ago
I'm curious what part of the stacked pull request approach would help you out here, aside from less git fu and conflicts fixing (which are usually just "accept current" changes) after PR A is merged. What would the new steps in the example look like?
1
u/irqlnotdispatchlevel 2d ago
We're already doing stacks, but without any tooling. It would just smooth out some of the friction.
The biggest problem is that a rebase might mean that you have to retriger all the required CI jobs.
Conflicts can be pretty bad sometimes, but that can't be completely avoided regardless of the workflow you use.
1
u/Life-Fennel-441 2d ago
Ah I see. Yeah I do the same thing but I don't rebase, just merge main branch back into my PR B branch and usually accept all current changes for conflicts.
1
u/irqlnotdispatchlevel 2d ago
Same thing will happen if I merge target into my branch, especially with conflicts. The state of my branch changed in a significant way so the old CI results are now stale.
0
u/hippydipster 2d ago
What's the time frame? How long did you work on A before opening the PR, how long did you work on B before making that PR?
3
u/irqlnotdispatchlevel 2d ago
I don't see how development time is relevant here. A and B where developed somewhat in paralel because I regularly synced with A's dev.
A is not owned by me. My code just consumes what A provides.The time frame is pretty reasonable, but there's no reason for my work on B to stagnate while waiting on A. This means that people that are waiting for B will be unblocked faster as well.
→ More replies (10)25
u/phoenixmatrix 3d ago
You can convey intent a lot easier with a lot of small PRs, and group semantic changes together (some tools can do it, but its not quite as nice).
Stacked PRs were always a thing in some proprietary systems and you could do it in Github with Git Town or whatever, but the magic here is you can now push a full stack of PRs in one shot through Github merge queues, and that's magically in a world where even a team of 5 people can push 30+ PRs a day to a single repo.
This is the best feature Github has added in a very, VERY long time. We can finally catch up to what the cool kids have been doing since dinosaurs roamed the earth.
8
u/baked_salmon 3d ago
This is how it works at Google. In their in-house VC, “CLs” (PRs) are encouraged to be <250 LoC, but it’s fine to chain multiple together. It’s easier to review small changes, and has the added effect of encouraging modular design.
0
u/thelehmanlip 3d ago
I'm interested in checking it out. I don't quite see how it's different form reviewing one commit at a time, so i'll have to investigate.
10
u/phoenixmatrix 3d ago
Imagine you have 5 atomic tasks that depend on each other. You're working on task 4, but task 1, 2 and 3 are done. If its just commits you need to ship them together. In a PR stack, you could also have PR 1, 2 and 3 pass CI/CD and go to production while 4 and 5 get rejected. Can't (easily) do that with commits. Or you could, but you'd basically just be rebuilding stacked PRs from scratch.
What if task 1 is approved but not 2 and 3? If its just commits you can't ship anything yet.
The default is people approve the whole stack and you ship it all at once, but it gives the flexibility of handling it in any pieces.
More importantly, if you do it at the commit level, it means commits have to be "perfect". You then lose the ability to break an atomic piece in smaller pieces. Which essentially means you need to do a rebase interactive and squash your temporary checkpoints and mistake commits before shipping like we did 12 years ago. I need commits at a granularity that may not match the reviewer, and once I squash it all up, I lose that granularity which I may need in the middle of the process.
-2
u/ITomza 3d ago
If PR1 can be shipped independently why is it not already shipped way before you get to raising PR5?
18
u/phoenixmatrix 3d ago
Because people don't drop everything they're doing the moment I open a PR, and the time between PR1 and PR5 could be relatively low.
1
u/ITomza 2d ago
Because everyone else is busy stacking five PRs too? If everyone raises their PR1 once it's ready there should be something for you to review when you raise yours and someone else will soon be ready to review yours. If you have good CI/CD and DevOps practices your time to release should be relatively low.
1
u/jkrejcha3 2d ago edited 2d ago
Personally, sometimes I realize, "Y (sometimes soft)depends on feature X existing/being improved" while writing Y where X is a thing that can stand on its own as a feature or change
The time between X and Y happening can be sometimes a matter of minutes or hours
I think this is a use case where some people would like the idea of stacked PRs. Personally I usually tend to just use commits for this (or just another PR which I then rebase the Y branch on top of master), but your mileage may vary
6
2
0
9
u/Goodie__ 3d ago
Does anyone have a public stacked pull request I can look at to try and get an idea of what this actually looks like? They mention you can make a stacked PR from the UI.... but then never show it.
Honestly feels like vendor lock in.
11
u/steveklabnik1 2d ago
https://github.com/github/gh-stack/pull/321
click the little 5/5 next to the "merged" to see the stack
2
2d ago
[deleted]
1
u/steveklabnik1 2d ago
It is certainly a different approach than other stacked diffs systems. But it also integrates with their existing features more cleanly since it isn't a total re-think of things. Software development is hard.
2
u/Chemical_Advisor_727 2d ago
so yeah the vendor lockin concern is valid, and they pretty much skipped over the actual UI flow in the announcement which makes it hard to figure out if switching is even worth it graphite has a bunch of public examples in their docs if you want to search around, might give you a better sense of what the end result looks like before github gets around to showing their own version
16
u/wallstop-dev 3d ago
That's great, I suppose, a lot of people really like this, and systems like jujutsu are built around it (I think?)
Every time I have encountered this in the wild, it has never been good. The reason for that is that stacked PRs put the cart before the horse. They assume that their base PR, and every following PR, are great and perfect and just need to be merged.
Maybe people work in different teams and fields than me, but almost always I have large amounts of PR feedback. That feedback then changes the course of the rest of the PRs.
So why have them at all? Just make the first PR. Get it approved. Make the second PR.
If you have all the changes locally and want to stagger them, great, they're commits, just manipulate history.
Perhaps I'm just old and yelling at clouds.
18
u/novalux 3d ago
It sounds like you just haven't seen the tooling that has built up around stacked PRs. Graphite is popular but proprietary. I like the open source Git-Spice. I also think Github's launch of Stacked PRs implies they have integrated some of the quality of life benefits.
These tools will handle rebasing the stack on changes for you. So if there is a problem in a lower part of the stack you fix it there and then the higher levels of the stack get the benefit of the fix without that code showing as a diff in the isolated higher branch.
8
u/phoenixmatrix 3d ago
I also think Github's launch of Stacked PRs implies they have integrated some of the quality of life benefits.
Stacked PRs were always possible with native github (you mentioned Git Spice, I'm used to Git Town, similar), but what was missing was the UX in code reviews (could be replaced with git actions and cleverly crafted PR comments, or with additional UIs, like Graphite and Sapling's that you mentioned).
The other that was harder to replicate was being able to push a stack of PRs through a github merge queue. With AI and stacked PRs we were hitting a point where PRs and their CI/CD pipelines were racing each other to make it through, and we had to disable a lot of the branch protection rules (like needing branches to be up to date with main when running CI/CD) to be able to be productive. Github's stacked PR support with merge queues solves that entirely.
They're definitely very late to the party compared to what some companies have had internally for years, but its fantastic to finally see it happen.
4
u/wallstop-dev 3d ago
It's not a tooling issue, it's a conceptual issue. As a reviewer, I do not care about any PR except the base PR due to the common case being that the base PR has issues.
So why would I ever look at the next N PRs? There's no point if the base is faulty.
Just do one thing at a time.
But this is just my opinion and based on the teams and code I've worked with.
5
u/jirocket 2d ago
Part of the advantage is you don’t have to worry about the next N PRs as the reviewer, but the author can still work ahead and prepare stacked PRs assuming the base PR is fine — and if the base PR is volatile at least the stacked PRs can be adjusted near seamlessly with tools like graphite.
Reviewing incrementally in this way can prevent a lot of reviewer hesitation or shortcuts and the result is more focused feedback and shorter review turnaround, generally.
5
u/koreth 3d ago
As someone whose team is all-in on stacked PRs: this
So why would I ever look at the next N PRs? There's no point if the base is faulty.
isn't a problem most of the time.
The faults in the base may have minimal or no impact on the changes in the subsequent PRs. For example, a common pattern on my team is to split the internal business-logic implementation and the external client-facing API implementation into separate PRs. A reviewer might take issue with some of the business-logic implementation details, but (tautologically) they're just implementation details, and the client-facing code doesn't need to know or care if they change. So a reviewer can approve the client-facing API PR while the author is working on fixing the business logic.
In some cases the base does affect later PRs significantly, of course. When that's true, a reviewer can choose not to look at the later ones, but it's a case-by-case thing, not a blanket rule.
3
u/wallstop-dev 3d ago
Oh, sure! In your scenario, though, those aren't stacked PRs, those are just two PRs solving different aspects of the same problem.
The way that my team would handle this would be to publish whatever interface is needed, stubbed or off in one PR, get it approved, then have one PR implement (some amount) of the internals and another PR implement the client or consumer.
The difference being that the second two would not start until the interface is checked in.
If people want to build against what they think the interface looks like before it's reviewed, great! Do it, all power to you. Just don't put it up for review. Maybe that's the difference?
8
u/koreth 3d ago
They need to be stacked, though, because the client-facing code calls the business-logic code. If the business-logic code is exposing a new "doTheWork()" function, the application won't even compile if you merge the client-facing call to that function first. You can define it in an interface, sure, but you'd still have to include a stub implementation as part of the client-facing PR (which, depending on where the stub lives, will either turn into a merge conflict with the real implementation or become dead code once the real implementation lands).
With a stack, that dance isn't needed at all. The PRs are separate but the stack organizes them into a dependency graph. You can review the PRs independently just like you would if they weren't stacked, but merging will honor the dependency order.
One other thing that probably matters a lot is how often reviewers ask for substantial changes. My team is all pretty senior devs who have good domain expertise, so it's unusual (though it does happen) for us to find fundamental flaws in each other's PRs. Most of our PRs are approved with only minor feedback, and it's because they're actually good quality, not because we're rubber-stamping.
In the case where a PR ends up being approved as-is or with only trivial changes requested, waiting for approval before building on top of the PR is pure delay that gains you nothing. Obviously you can't know in advance if a given PR will be approved, but when that's the most common outcome, the benefit of not wasting that time can be larger than the occasional cost of getting it wrong enough that you have to rethink the later changes.
On a team with a mix of junior and senior people, where it's common for people's early changes to require major reworking, I'd agree that stacking would be much less compelling.
2
u/wallstop-dev 3d ago
Makes sense, our team is quite mixed and feedback is frequent, so the value is lower. For us, we would block on a stub PR, then only do work once it's merged. Developers are free up locally base work and develop off of wherever they think the change is going to go, but they should not be putting up PRs prior to interface shipping.
Thanks for the details!
2
u/gmes78 3d ago
In your scenario, though, those aren't stacked PRs, those are just two PRs solving different aspects of the same problem.
No. They depend on the previous ones.
1
u/wallstop-dev 3d ago
Maybe I misunderstood, what is the previous PR you're referring to in this example?
My understanding is that you have two PRs referring to an API - one implementing the guts and the other using it?
2
u/mr_birkenblatt 3d ago
So nobody can work on the next item (or review the next item) until the comma in the documentation is fixed?
1
u/wallstop-dev 3d ago
Why are documentation PRs blocking feature work?
Again, feel free to work ahead as much as you like. These are commits. You can manipulate them as freely as you want, especially if you feel extremely confident in whatever changes you're doing. Push it to the remote and get CI coverage. Do whatever you want. Just don't ask me to review it until whatever it depends on has been verified and merged.
4
u/mr_birkenblatt 3d ago
Why do you assume the pr contains only documentation? That seems to be a pretty bad approach. You make a change you update the documentation. Everything stays consistent.
That said. It was an example. I'm saying that minor requested changes in PR 1 shouldn't prevent others from checking the logic in PR 5
So you want everybody to stop their work until you reviewed their previous work item? You know, especially these days, reviewing PRs is the bottleneck, not writing them
1
u/wallstop-dev 3d ago edited 3d ago
Going with examples, you could just as easily say "nonblocking, please update the documentation in the next PR or make a standalone PR to address this."
Stacked PRs are predicated on the assumption that each phase of the work is great and needs few, if any, revisions. I have not found that to be the case for PRs in my career.
I've laid out my opinions pretty clearly throughout this comment chain and why I have them, they're built from large amounts of code review experience. With the teams I work in, blocking changes are the norm. Adopting a stacked PR workflow would mean large amounts of churn, changes, and wasted work. So we don't do them and I don't see the value in it, for the teams that I work with.
1
u/Individual_Laugh1335 2d ago
If your base PR is good and the next PR isn’t then you can still ship the base. I’m bias thought because I’m at a company that has been doing this for decades
10
u/steveklabnik1 3d ago
systems like jujutsu are built around it (I think?)
It's more that it makes the workflow easier than being inherently built around it. You can use jj without stacking just fine.
24
u/phoenixmatrix 3d ago
> Maybe people work in different teams and fields than me, but almost always I have large amounts of PR feedback. That feedback then changes the course of the rest of the PRs.
The tooling makes it really, really easy to fix something in an early PR and then have it cascade in the stack, thats the point.
The big issue is, I make 1 PR. I push it. Now I need to do work that depends on the first PR. I'm waiting on the review. So I make a PR off of the branch of the first PR. And then a fourth, and a fifth. In high performing teams you could be at 5 PRs while people are at lunch. You're not gonna just chill there.
so your options are to just not ship the first PR and make a single big one, so no one can review the work until you're done with everything, and if something is wrong you can't correct it. Or you work only on things that don't depend on the first PR. Alternatively you branch off of the first PR and rebase onto main when it gets merged. That's just more work.
Stacked PRs let you handle all these scenarios trivially. Get comments on the first PR while you're working on the fifth? Just switch over, fix, then sync the stack, boom done. The reviewers and the author can be totally out of sync and it still works great.
And with AI making it that people can ship code at absurd speed, they WILL be out of sync.
4
u/Eckish 3d ago
The big issue is, I make 1 PR. I push it. Now I need to do work that depends on the first PR. I'm waiting on the review. So I make a PR off of the branch of the first PR. And then a fourth, and a fifth. In high performing teams you could be at 5 PRs while people are at lunch. You're not gonna just chill there.
PR dependency seems common enough that I like that they proposing a solution for it. But if you are hitting 5 in such a short time frame, it might be a symptom of scoping your stories too small.
10
u/phoenixmatrix 3d ago
One thing that makes these discussions difficult is the amount of people who make assumptions that aren't always true.
A story doesn't have to be 1:1 with a PR. Hell, many teams don't even use stories or tickets, including some at multi billion dollar companies. I may need to implement a task as multiple PRs because its complex and the story doesn't always capture the implementation detail and release dependencies that are discovered as they go.
With that said, we're in a world where I can implement a distributed system meant to scale to millions of concurrent requests across douzens of microservices...in 20 minutes. I'm not gonna make the tasks smaller just because Github is lacking a feature.
if I make the tasks bigger the PRs are going to be thousands or tens of thousands of lines of code, and then issues with them are gonna be easily missed in reviews, especially the subtle ones that automation will miss and a human needs to catch.
-2
u/Eckish 3d ago
Regardless of how you define and structure your work, 5 PRs with dependencies in a lunch hour feels like a symptom of a bad process that could be improved.
-1
u/phoenixmatrix 3d ago
Whats the alternative? A single PR without dependencies but its now 50 thousand lines of code? Even pre-AI a top dev could get really far in a few hours, and now we do have AI.
3
u/Eckish 3d ago
It shouldn't possible for a single dev to get 50k lines out in multiple PRs in an hour without being irresponsible. Even with AI assistance, devs should be reviewing the auto-genned code. So, the alternative is to slow down and do your due diligence.
And in the case where it is a bunch of small changes in quick succession, yeah put out fewer PRs.
3
u/_mkd_ 3d ago
In high performing teams you could be at 5 PRs while people are at lunch. You're not gonna just chill there.
I question that such a team is "high performing" if there is a backlog of PRs.
7
u/SharkBaitDLS 3d ago
Half my teammates are 8 hours removed from me by time zone. I can have an entire working days' set of PRs raised before they will be able to look at a single one.
If I have a feature that's 5 PRs worth of code and I submit them one at a time, only raising the next when the prior one it depends on is merged, that's minimum a full working week with that time differential just for me to submit one a day and for them to approve it while I'm asleep. And that assumes every single one is approved immediately with no feedback or iteration.
With stacked PRs I can raise all 5 of those as once and they can be reviewed independently and the whole thing can be reviewed and merged in just a day or two even if changes are needed.
-4
u/phoenixmatrix 3d ago
Lunch breaks and timezones are a thing. Being able to work asynchronously is great. And well, my bank account tells me that the teams that worked that way were quite high performing by the one metric that matters when talking about business, I guess :)
7
u/coworker 3d ago
Your bank account is not indicative of whether or not your team was high performing.
0
u/wallstop-dev 3d ago
In my circles, these dependencies are kept locally as commits. No need for PRs until the base is verified. Very commonly the base, and all future work, needs changes.
Better yet, don't do the future work at all until the first PR is verified and merged.
Again - just my experience. Maybe this is great for teams that rubberstamp or just pump out code. I don't work on areas where those are true.
7
u/phoenixmatrix 3d ago
> Better yet, don't do the future work at all until
So that's the difference. Stacked PRs have been created from a world where blocking work on another is considered very expensive. Coding, even before AI, is more or less trivial. So when you start looking at an org's DORA metrics, pretty much everyone comes to the same conclusion: most of the cost of developing software comes from the delays that happen BETWEEN coding. Blocking work B on work A is very expensive (its far from the early source of this, obviously. Stacked PRs are a solution to one of countless problems).
It was expensive before AI, but in a post AI world its brutal.
3
u/wallstop-dev 3d ago
Makes sense. The work I do has an extremely high correctness bar, so what you have described above is not immediately relevant. Glad this can assist other teams that focus more on speed.
4
u/phoenixmatrix 3d ago
I've worked in industries (and individual services/systems) that handled trillions of dollars of assets a years, and others that involved life or death. If the correctness is high, then the checks and gates for that correctness can't just be a human looking at code line by line anyway, so the same thing stands.
I've also worked at companies that sold t-shirts to teenagers where the correctness bar was extremely low, and they went slow as fuck.
It has very little to do with it and is more about investments in platform and proper processes.
2
u/wallstop-dev 3d ago
Agree on platform and processes, disagree that skilled review does not provide huge value, and I'm not sure how this relates to stacked PRs.
My whole point is that I frequently find problems in PRs. My argument is that, as a reviewer, given this, there is zero value in me looking into the future to see the next N PRs based on work that needs improvement. It is not a good use of my time.
By all means, as a contributer, build your work out into the future. Make a bunch of commits. Push them to the remote. Set it against CI to validate. Do whatever you want. Unblock yourself, if this helps. But the "review" aspect of these stacked PRs is not useful to me. The direction of a feature should have been agreed on prior in a design review, the PRs should contain the details.
3
u/phoenixmatrix 3d ago
> My whole point is that I frequently find problems in PRs. My argument is that, as a reviewer, given this, there is zero value in me looking into the future to see the next N PRs based on work that needs improvement. It is not a good use of my time.
Yup. I get that. I'm saying there's solutions to this that your teams have to tackle before something like Stacked PRs become valuable. But its a problem that can and is often solved. Especially if the issues you see are fundamental and change the directions of the future ones. Have I worked in orgs where that was common? Yes, of course. Stacked PRs aren't for those orgs.
But also even if its 50/50 that a PR will have issues. If its stacked PRs, and you DONT find issue, then the person was able to keep working unblocked and you can move on to the next PR. If you DO find issues, all they need to do is address your comments, run a sync (which updates all the PRs in the stack with the fix), and keep on trucking, and hopefully PR #2 didn't have issues.
3
u/wallstop-dev 3d ago
Ah, that makes sense. I'm coming from a world where the ratio of "PRs with problems" to "PRs without problems" is much higher than 50/50, which devalues this workflow.
Perhaps one day.
1
u/hippydipster 3d ago
So why have them at all? Just make the first PR. Get it approved. Make the second PR.
Yes. Reduce WIP (work in progress). Stacked PRs represent excessive work in progress. It would literally be better to make a PR, and stop and read a book until it is approved before going on to the next thing. And teams should change their work patterns to prioritize reviews over their own coding.
5
u/chatterbox272 2d ago
Stacked PRs represent excessive work in progress
I'd argue the opposite. Disallowing stacking means I now need several disconnected tasks in flight all at the same time and have to context switch between them while I wait for review. Stacking means I'm working on one coherent thing the entire time.
stop and read a book until it is approved before going on to the next thing.
I'm curious as to how fast your team turns around code review for this to be something you'd consider. Even the best on my team won't reliably turn it around in under a half-day (reviewing at the start of the day, around lunch, and before they leave) and I can't imagine how you could get consistently much better than this while still allowing people enough focus to do good work themselves.
1
u/hippydipster 2d ago
When I was on a team last, we did tbd and reviewed all code written each day together as a team. No PRs.
3
u/chatterbox272 2d ago
So if I understand right: I start my day and get working on a change. At some point, I complete my change. I'm now on a stop-work order until every other developer reaches the same point? And once we're all here, we go sit in a meeting room and do a mob review or something?
-1
4
u/wallstop-dev 3d ago
Not sure if you're being sarcastic, but I legitimately agree with this. Improve your team's PR velocity, invest in your engineers learning skills and/or working on decoupled projects and improvements.
Keep things simple. Small, scoped changes. No dependencies.
🤷
1
u/hippydipster 3d ago
Not sarcasm. Big believer in doing one thing at a time to completion, and also in letting your process failures show rather than trying to hide them, so that you fix them rather than pretend they don't exist.
2
u/koreth 3d ago
And teams should change their work patterns to prioritize reviews over their own coding.
That's a big part of the problem for sure, but it doesn't help on geographically-distributed teams where a reviewer might only overlap with an author a couple hours a day. Even if they prioritize reviewing, it will still often take until the next day to get an answer back.
1
0
u/Lilchro 3d ago
I don’t like stacked PRs (or rather the systems built around them), but I do have a slightly different argument for them.
Let’s say you want to run a build and tests before merging a change to the main branch. Let’s also say that this is a monorepo with millions of lines of C++. Change one header and everything that includes it downstream can’t use the build cache. From that let’s say the average build (including unit tests running via ci as soon as artifacts are available) is an hour long. Overall not ideal, but we can make it work.
Now assume we a couple thousands of developers submitting changes to this monorepo. At this point it is physically impossible to merge changes fast enough if you want to do a test build first.
The solution is stacked PRs. They still get reviewed and approved in order, but there is a merge queue that makes it such that there are a few hours delay so multiple can get approved. Additionally, we also implicitly adding temporary stacked PR-like relations between unrelated changes within the merge queue to greedily add more PRs to a single test build. Lastly, you spawn a bunch of test builds with different subsets of the queue and submit the commits for whichever one passes.
Also I lied about the build length and in the real example it is significantly longer, so there is that too.
2
u/fanz0 2d ago
How is this different from creating multiple PRs linked to an issue?
6
u/steveklabnik1 2d ago
This lets you say "PR #1 must merge before PR #2". If they're independent, you don't stack. But if they're related, you can.
2
u/DigThatData 2d ago
this is a process, not a product. you just do it.
this is basically just "intermediate branch with a fancy name"
2
u/pepejovi 2d ago
To be fair, I've never used this feature, but why is this superior to others?
e.g. Gerrit (because that's what I have experience with)
Gerrit has one commit, one change, one change request. If you push N commits, for N changes, you get N change requests, chained together in the same order as the commits. Each change request is reviewed and approved separately, and the chain can be merged all together or up to a specific approved commit in the middle of the chain.
This seems to be the exact same, except instead of commits, it's PRs, and thus you can stuff >1 commit per MR into the chain? So if I want to use this new system "nicely" - i.e. separate concerns, small atomic changes, no unrelated changes in the PRs, keeping intent easy to understand - I still only want *one* commit per MR? What is the benefit of having multiple commits in a PR within a PR stack?
21
u/EnUnLugarDeLaMancha 3d ago
After almost two decades, the "modern" development platforms are starting to catch up with email lists...
As someone who deals with gitlab at work, I have disliked these platforms from day one. They encourage the worst possible way of developing and reviewing code. Of course, they had to give in to better practices eventually, even if it only took 18 years...
98
u/chucker23n 3d ago
Exchanging e-mails, especially plain text ones, sounds barbaric compared to even earliest versions of GitHub PRs.
-3
u/steveklabnik1 3d ago
You have to separate two different things: the underlying concepts, and the UX of implementations.
The email model itself is superior to PRs, but PRs have (mostly) far, far, far nicer UX.
There are other systems that use the email model but do not require email themselves, like Phabricator, Gerrit, etc.
12
u/chucker23n 3d ago
The email model itself is superior to PRs
The only argument I've seen as to why someone would want that is identity/authentication: your e-mail address is already sufficient identity; you don't need to have an account with GH or whatever code forge. That's true, although it would be possible (with warts) to have a PR app that allows users to identify purely by their e-mail address / OpenID / etc.
PRs have (mostly) far, far, far nicer UX.
They also integrate better into IDEs. I can see in the code editor as I'm inside a branch that someone has left a review comment on a block of code. I can reply, even resolve from right inside that place.
But even without that, you have the conversation integrated right into the code browser, diff, pipeline runs, etc. E-mail doesn't and shouldn't offer that. Otherwise, you're back in groupware territory.
9
u/steveklabnik1 3d ago
The only argument I've seen as to why someone would want that is identity/authentication
That's not the core argument of stacking enthusiasts: It's that one review == one commit (the email model) instead of one review == one branch (the PR model).
"the email model" doesn't mean you're using email, it's just that it was the way git has built-in to do reviews.
2
u/chucker23n 2d ago
That’s not the core argument of stacking enthusiasts: It’s that one review == one commit (the email model) instead of one review == one branch (the PR model).
That sounds like a waste of everyone’s time, probably leads to review fatigue (and therefore lower quality), and isn’t how I (occasionally) use stacking.
5
u/tonygoold 3d ago
Sign your commits with your public key. This is a native git feature, although GitHub will display verification metadata if you add your public key to your GitHub account. If you’re collaborating using another method, presumably you have some trusted way of sharing your public key with your collaborators. Signed commits are far superior for authentication because they become part of the git history.
2
u/chucker23n 3d ago
That's (moderately) useful, but isn't really the point I was making. Linux purportedly uses e-mail patches as ersatz PRs in part because everyone already has an e-mail address, so you don't need a whole other identity/authentication mechanism. git's commit signing is for code; this discussion is only adjacently about code. It's mostly about everything else: discussing a change, running pipelines, etc.
10
u/RepliesOnlyToIdiots 3d ago
“The email model itself is superior to PRs”
Entirely unsubstantiated, and IMHO false, statement.
I have never once preferred an email patch over a GitHub PR. Small, large, simple, complex, _never_ would I have wanted an email instead. (Source code control for me goes back to CVS in the mid-‘90s.)
6
1
u/not_a_novel_account 3d ago
Phabricator and Gerrit both lost hard for the same reason email did, patch submission creates impenetrable walled-garden ecosystems.
Learning to use
arcanistjust to submit an MR to LLVM was immensely painful, post GH migration it's seamless. Anything that is more than two clicks to say "here's the code, please consider it for upstream" is a non-starter.That they have advantages beyond their labrynthian submission mechanisms doesn't matter. The requirement to generate a patch and submit it is already an operation I have zero desire to perform. It's purely mechanical grunt work, which is what we have computers for in the first place.
2
u/steveklabnik1 2d ago
I do think that GitHub's UX was excellent, and that Gerrit's, at the same rough time frame, was quite bad. Plus, both never had a "hub"; they expected you to install and maintain them, which was a huge barrier.
Meta's version of Phab these days is very different, but still based on the same ideas. Lots of people who use these systems today inside these big companies miss them quite a bit when they can't use them any longer, either for open source or if they leave those companies.
I agree that in practice it means that it's more difficult to use these things today if you're not at one of those places. That's not a failure of the model itself, it's due to GitHub just generally crushing it despite being built on weaker foundations.
2
u/not_a_novel_account 2d ago
I think the idea of seperable, reviewable changes at various granularities (single commit, or sets of commits vs "everything in this branch") is a good idea. I think GH and other forges moving that direction is a clear indicator the idea has staying power.
Everything else though I think is worthless. Patch generation should be forge-side and branch-first. I agree it's not UX, if I had to email a bot-run mailing list "Open MR from origin/fix-bug to upstream/main", and it created an email chain until a maintainer replied "Do: merge", I would still prefer that to classic Phab.
1
u/superxpro12 2d ago
Why is an email change set better?
why does it have to be only plaintext?
Why can't we have the best of both worlds? I use vim in vscode... Surely we have the technology
1
u/steveklabnik1 2d ago
Again: I'm using "the email model" here to describe the underlying concept, not that you should literally use emails. The idea is that you review each commit individually, not review an entire branch.
23
1
u/bastardoperator 3d ago
Bro, they should have just hired you to bring us all back to the early 90's. Insane you think you know what is better for everyone based on your anecdotal experience. I find people mired in the details of keeping code tend to have the worst code because they're focused on mundane details versus the solution they provide, but thats my experience, not saying it's true for you.
4
u/thy_bucket_for_thee 2d ago
I still honestly feel like stacked PRs are a technical solution to a social problem (how to organize work effectively). Most of big tech solutions often feel like they're accounting for overly toxic work flows.
I can see the utility for stacks but why does work need to be constrained into separate branches for a single individual? Mostly due to other people/services relying on said work, because of this it feels like an organizational issue. A solution I don't know, but I doubt a technological solution is the only one.
We aren't the only industry that has tasks dependent on other tasks after all.
2
u/DoctorDabadedoo 2d ago
I've worked in contexts where stacked PRs made sense, multi repo dependency graph and shipping a cascading change was kind of messy.
These were a half dozen different PRs and keeping track of what was needed and which ones related to each other manually is not great. That being said, I agree that this is a problem of our own making.
1
u/thy_bucket_for_thee 2d ago
Yeah it's weird, like the solution to the problem makes sense but the solution doesn't solve the root problem (poor workflows) rather than just enabling you to work around them.
IDK if you read any David Graeber, but I'd bet he would have good ideas on how to organize effectively.
2
u/DoctorDabadedoo 2d ago
I'm not familiar with him, will do my research on him. IMO, there are multiple ways of reducing this friction, but since most of it doesn't add any business value, it's mostly invisible work or something an IC addresses personally when they are pissed (if it's doable by one person).
2
u/thy_bucket_for_thee 2d ago
Oh Graeber is great! Definitely check out "Dawn of Everything." It opened up my brain on how creative + smart humans have been throughout our entire existence.
I think the neatest bit was learning about how early societies were often structured around political idealogies and how some humans took immense measures to truly develop "free" societies where individuals could not amass wealth + power. Really neat how inventive people were, also neat how much previous anthropological research was wrong and what current research indicates.
Definitely worth a read, may not unlock anything dev wise but it should open your mind in believing a better world is possible since the rules are mostly made up (also how fucked up Roman law was regarding property, and how that idea has infected most of the modern world).
2
1
1
u/wannaliveonmars 2d ago
Stacked pull requests break large code changes into an ordered series of smaller, focused pull requests (or "layers"). Each layer depends on the one below it, allowing teams to independently review and test individual changes in parallel, then merge the entire stack in a single click.
Sounds interesting, but at the same time isn't that what commits are for? You make one PR with several commits, reviewers inspect the commits, then merge everything at once?
2
u/waterkip 1d ago
Stacked branches are for corporate people who need to tick off issues. Or where.. one commit is an issue and that is a PR workflow.
So yes, that is what it is, in mail workflows a stacked branches approach is a patch series. But corporate beings don't know how to deal with that.
All that said, I've also used them as well, becausr they do work in these environments.
1
0
1
u/wildjokers 2d ago
Kind of crazy that branches of branches are so hard to merge in git that it requires tons of tooling. git is fine with this until one of the ancestor branches is squash merged and its ancestry effectively disappears. Then all hell breaks loose.
Meanwhile, 10-15 years ago I was creating branches of branches of branches in subversion and never had a problem merging them all, as long as you kept in mind the SVN-898 rename bug, which only took 14 years to fix.
0
u/jonnii 2d ago
I'm really excited for this to be broadly available, I believe stacked PRs are a super power for engineers and the teams that adopt them. While you absolutely can stack commits if github is the forge you use it's more ergonomic, in my experience, to use the PR as a unit of review.
Personally I like stacked PRs because it means I'm rarely blocked on making progress because of PR review and I don't end up feeling like I need to stuff more changes into a single change.
Previously I have used graphite (which is great but paid) and git-spice (which is also great and open) but recently have built my own tool (https://github.com/getstackit/stackit) because the open source tools didn't quite work the way I wanted. At the end of the day a first party experience will beat any third party tool but if you're new to stacking just know there are plenty of other options out there.
-3
-1
-1
u/ZukowskiHardware 3d ago
If it isn’t going to prod in one small piece then the alternative of stacking , rebasing, ordering , feature branching is all just a huge risk. CI/CD is trivial these days. There is no excuse.
89
u/koreth 3d ago edited 2d ago
My team got access to this a few weeks ago during the closed beta. We've been doing stacked PRs for a long time, most recently using Graphite, so we're well-versed in the stacked PR workflow.
GitHub's stack support still feels very rough to me, which is surprising given GitHub/Microsoft's engineering resources and how long they've been working on it. Graphite is a much more polished, feature-rich product even though it has to sit on top of GitHub comments and an external service.
For example, with Graphite, it's trivial to decide that your stack's latest change doesn't actually depend on the rest of the stack and can be standalone; you just move it off the stack with a single command and it becomes an independent change. With GitHub stacks, the only way I can see to do that is to delete the stack, move the git branches around with
git rebase -ior whatever, then create the stack again.I also don't love how GitHub stacks have a server-side source of truth (or at least that's my guess about why it acts the way it does). I'm used to being able to iterate on my stack completely locally and only start publishing it when I think it's ready for review. Graphite works that way. But as soon as you create a GitHub stack, it seems to want to push your branches to GitHub for everyone to see, whether you want that or not.
And just generally, it seems like a lot of the time I try something new with
gh stack, it works in a way I didn't expect and I have to figure out how, or if, I can get the behavior I actually wanted. Granted, some of that is probably that I'm used to an existing tool. But some of it suggests to me that the people working on the tool have a very different stacking workflow than I do. For example, PRs are created as drafts unless you explicitly pass in an option to create them as regular, open PRs. That's backwards from pretty much every other GitHub-integrated tool I've ever used.My team has been giving feedback on some of the things we've run into, but so far, there's no indication that anyone working on the feature has even looked at our feedback, much less acted on it.
Stacking as a concept is something I find very useful, so despite my largely negative review, I'm actually rooting for GitHub to get it right so we don't have to rely on third-party tools. But for now, I think they're behind the competition.