3 ms·
What's the benefit of this type of stacked PRs over a well-curated set of commits, and reviewing per commit? I think the bigger problem is that big AI PR's nee
by Okkef 2mo ago
What's the benefit of this type of stacked PRs over a well-curated set of commits, and reviewing per commit?
I think the bigger problem is that big AI PR's need a different way of reviewing. For example, the order in which the diff's are shown can make a big difference in how easy the commits are to read (e.g., function definition change first, then all call sites, then the tests).
Or maybe we should go to a system where diffs & comments are intertwined, a bit like how "Literate Programming" intertwines code and prose.
Literate diffs / literate pull requests... I haven't found anything like that yet.
- dastbe 2mo ago> What's the benefit of this type of stacked PRs over a well-curated set of commits, and reviewing per commit? For the people who work with stacked diffs (in phab/otherwise) this is exactly what they'd consider reviewing a well-curated set of commits one-by-one. One distinction is that cognitively a unit of review (a PR, a diff) remains a single bound change. Comments are focused on that change and the PR does not grow with size of the feature Another distinction is the ability to focus each part of the stack to a particular audience. One change may require review from an external team, another may be just your team mate, a third might be the consuming team. By focusing the stack to the different reviewers you can avoid ambiguity about "what a person is signing off on" in the stack. aside: one thing that would be great for github reviews is the adoption of change ids such that comments persist across reviews with a rebase workflow.
- skydhash 2mo agoIMO, in a team settings, improving the review policies and speed has a much better benefit. A PR is supposed to be a proposal for some change, adding more proposals on top of something that is not reviewed is a bit icky. > . By focusing the stack to the different reviewers you can avoid ambiguity about "what a person is signing off on" in the stack. That can be easily done with comments. If the PR are orthogonal, they could have been split. And if they're not, I would really like to know how the part that I'm reviewing interacts with the rest of the changes.
- a_t48 2mo agoThe PRs may be orthogonal but still be dependent. Feature X depends on improvement Y which also needs bugfix Z. You might go and implement X in a branch, tweaking the codebase as you go, but split the branch apart for review. You put X/Y/Z up, but X contains Y and Z, which means you can't request reviews for X without Y and Z merging, or else have a bunch of extra code that gets in the way.
- skydhash 2mo agoLet's say that Z has an error (some assumption that does not hold), and needed to be reverted. How does that impact X's viability? I wouldn't trust any reviews of X after that. I strongly believe that PR should be compared to the main branch, and not rely on unmerged code. Unless you merge everything together in one go. And in the latter case, everything should be reviewed together.
- a_t48 2mo agoI think the answer is - it depends! This is why we make good money. I don’t think there’s a hard and fast rule here to apply.
- makeitdouble 2mo agoIf the tooling allows rollbacking X, Y and Z as a set, it could make sense ? I'm also on the "compare to the main branch" camp in general, but will sometimes end up with a set of X,Y,Z branches that have different purposes but all depend on X. More often than not, the base dependency is a set of constants or additional class/methods that could be released with no impact (no reference in live code) but still need a somewhat lenghty review process. Reverts would be happening on the higher level PRs, which hopefully are independent.
- Degorath 2mo agoMy experience is that in companies that rely on this flow, these assumptions are generally hashed out during design doc review/discussions. I have never had it happen to me that I need to discard the rest of the stack for an assumption that did not hold. From another angle, if I fear that this might be the case, I hold off on sending parts of the stack to review until I am sure of the assumption holding OR I instead have a "workspace ball of mud" from which I can `split` out well-formed commits. I found this to be an incredibly productive workflow.
- hypendev 2mo agoBut couldnt you just create a PR2 that targets your PR1 branch? What does stack contribute that multiple PRs targeting each other dont? i.e. like this where each one targets prior PR1 db_migrations PR2 backend_base PR3 adds_frontend_scoobydoodle PR4 adds_some_landing_page_thingy PR5 probably_have_to_fix_something Or is stacks just a feature built to make this type of PR stacking easier to review?
- eddythompson80 2mo ago> Or is stacks just a feature built to make this type of PR stacking easier to review? Pretty much, yes. It’s just a UI to make it clear that those 5 PRs are liked (stacked) and gives some tooling around reviewing, merging and managing them, reduce redundant CI runs if desired, etc
- deleted 2mo ago[deleted]
- m11a 2mo agoIn essence. But if you have a chain PR1 > PR2 > PR3, and PR1 gets merged, all the others (ime) seem to not cleanly rebase on main. They end up with conflicts that require manual fixing. I've not really figured out why, tbh. It'd also be nice to see a coherent link in the UI between PR1, PR2, PR3.
- madeofpalk 2mo agoit's basically a UI for a well-curated set of commits, and reviewing per commit.
- EduardoBautista 2mo agoI feel it's more of a limitation of the GitHub UI. It is much easier to group together reviews and comments by PR than it is by commits.
- ghthor 2mo agoYep, this exactly
- Cedricgc 2mo agoWith stacking you can continue working on a longer change while creating reviewable diffs. Merged diffs can be rebased on your HEAD. For teams that support stacked diffs you don't usually even need to manage branches and work directly off trunk and rebase changes as they come in
- hoppp 2mo agoI think it is designed for AI Pull Requests. My impression was that it's for reviewing large generated changes.
- mmlb 2mo agoI've had that same "what's the point" thought every time I've read about stacked PRs, but recently had an (obvious) epiphany. The benefit is you get CI for each commit! I've always hated fixup/typo/fix tests commits and toyed with having a CI check that enforced ci passing on each commit but this drops that need.
- skydhash 2mo ago> I've always hated fixup/typo/fix tests commits and toyed with having a CI check that enforced ci passing on each commit but this drops that need. Can't you run your CI locally? I know it's not feasible for some codebase, but at least the linting, formatting, unit tests, some integration tests should be able to be done locally.
- steveklabnik 2mo agoYou can't have mac/windows/linux/whatever all locally simultanously. Not every project requires this, but for those that do, it's impossible. Also, it is much harder to enforce "everyone must run each commit through the CI equivalent properly" than it is when it's on your forge.
- skydhash 2mo ago> You can't have mac/windows/linux/whatever all locally simultanously. Why can't you? That's what VMs are for. And even then, most cross-platform codebases have an abstraction layer that rarely changes. So even testing on one platform can raise your confidence very high. > Also, it is much harder to enforce "everyone must run each commit through the CI equivalent properly" Again why? I wouldn't care about the dev's local branch. But what is send to the main repo can be easily scripted to run the CI on every commit. You just send the result back with each commit that fails. They can replicate the same workflow on their local workspace as a pre-push process.
- steveklabnik 2mo agoYou can't run MacOS in a VM on non-Mac hardware without violating Apple's EULA. > But what is send to the main repo can be easily scripted to run the CI on every commit. Sure, this could work. I didn't say it was impossible, just more difficult. You have to build all of this support on top of the system that already does it for you: have your forge run CI on every commit.
- teeray 2mo agoIt’s the “well-curated” part. Many folks treat commits like video game save points, and find providing any kind of message burdensome (e.g. “fix bug”, “do work”). This alone is fine, but then they can’t be bothered to go back and clean it up with `git rebase -i`, so you end up with a mountain of trash in your git log if you don’t turn on mandatory squash and merge. For these folks, the PR becomes the commit. Stacked PRs is revolutionary because it’s as though these developers can finally have multiple commits which comprise a change.
- eddythompson80 2mo ago> (e.g. “fix bug”, “do work”) or "skjdnfks" and "fsdfs" commits.
- chillfox 2mo agoThere is real value in having all the messy commit history, especially now with AI, but you do need some kind of description on them that indicates what they are. One use I have got from it is asking an agent to go through the git history and categories the mistakes/bugs, then turn the common ones into CI checks or AGENTS.md rules. A well curated history just hides a lot of valuable info.
- ghthor 2mo agoThis is it, but also, because there was no way of asking a reviewer to review a single commit and then merge only that commit out of 2+ commits without a lot of ceremony, work streams had to be completely orthogonal to parallelize anything . Now I can fully develop a sequence of 20 changes (each a single commit) and start getting reviews and merging while still progressing towards the goal. I’ve been doing this for 2years with graphite and more before that manually or in other systems/cultures that actually can review a commit/patch and merge that thing. Honestly all GitHub needed to do was make reviewing and merging commit(s) from a PR possible and that would also been neat.
- dualvariable 2mo ago> What's the benefit of this type of stacked PRs over a well-curated set of commits, and reviewing per commit? What I can see is that you can easily append commits to e.g. the first PR in a stack, which would insert them into the middle of sequence of commits. This will require rebasing and fixing the subsequent PRs in a stack the same way you'd need to rebase and fix the subsequent commits in a mega-PR. But it makes the right thing easy (keeping all the commits to the foundation of the change together) rather than makin the wrong thing easy (appending fixup commits across the entire change in a random order so that the actual foundational change is lost). Keeping all the foundational commits together also keeps all the discussion over the foundational change together. You could argue that you'd want to only do the foundational PR and stop, but doing the whole stack of PRs gives the reviewers more information about where you're going, and allows work to continue asynchronously.
- catlover76 2mo ago[dead]
- paxys 2mo agoYou can’t merge one commit at a time in a PR. In a stack, if the first 4 parts of a feature are good to go and there’s a problem with the 5th, the whole thing doesn’t need to be blocked.
- ahepp 2mo agoI wonder if they could have added the ability to take only certain commits from a PR though? It kinda seems like they're duplicating the "unit of change" arbitrarily, rather than just fixing the way a PR works. I think the reason I find that a bit icky is it seems like it's diverging GitHub from the underlying git tool, which I trust a lot more.
- mcintyre1994 2mo agoThe issue you’d have then is the CI might not be green after the 4th commit but it is after the 5th. A single PR will hide that, so you’d need a way to run the CI on the subset of commits you want to extract. It’s also much easier to isolate those blocking comments on the 5th commit if it’s in its own PR and not mixed with all the review comments on the other ones. I don’t really see it as diverging much from the underlying git tool TBH - it’s still just git branches pointing at each other.
- paxys 2mo agoIt isn't arbitrary. A branch is the unit of change. You can stack branches on top of each other and merge them one at a time. You can also keep adding to the end of the stack as needed.
- ahepp 2mo agoI understand that GitHub kinda ate the world on this, but using a branch PR as the basic unit of change seems like an arbitrary choice to me. Torvalds invented Git for work on the Linux kernel, and the basic unit of change there is commits over email. It's not uncommon for them to take some commits but reject others. Which isn't meant to say "pull requests are obviously wrong and commits over email are obviously correct", but rather to point out that the idea of a take-it-or-leave-it branch isn't some kind of natural unit of contribution https://lkml.iu.edu/hypermail/linux/kernel/1402.2/00508.html https://lkml.iu.edu/hypermail/linux/kernel/1402.2/00508.html https://lkml.indiana.edu/2011.2/05416.html https://lkml.indiana.edu/2011.2/05416.html https://lkml.iu.edu/2409.0/00960.html https://lkml.iu.edu/2409.0/00960.html https://lkml.iu.edu/hypermail/linux/kernel/2510.3/01369.html https://lkml.iu.edu/hypermail/linux/kernel/2510.3/01369.html
- IshKebab 2mo ago1. Github's UI doesn't really support reviewing individual commits in a big PR. 2. It also can't merge subsets of commits from a single PR. E.g. if you have two commits, A and B, where B depends on A... sure you can make a PR containing A and B, but if A gets approved and B doesn't, then you can't merge A. 3. The thing you want to do with a set of commits and reviewing each commit IS stacked PRs. This is nothing to do with AI.
- aeturnum 2mo agoI would say that it makes it easier to design a PR and then go back and update that PR as things evolve. For instance, you might decide you need one table so your first PR in the stack adds that table. But then you finish up with 2 or 3 more PRs and you get reviewed and you and your colleagues decide to add a 2nd table. Now you get to go back to that first PR, add the 2nd table and then you get to cleanly review that 2nd table. Then each PR in the stack also gets a clean small addition to handle the table, that can be reviewed in its own context with smaller diffs. So the idea is you can much more cleanly isolate changes for large features.
- ahepp 2mo agoI don't have experience with the stacked PR workflow and often find myself asking the same questions you are. As far as I can tell, the biggest benefit of stacked PRs over just making a coherent series of commits, is that it might make it easier to start work on your second PR before you merge the first one? With human-in-the-loop coding, that sounds like it could lead to a lot of wasted work if the first PR gets substantial feedback. But with agentic coding, I can imagine how it might be desirable to keep the agent chugging while the first PR is under review. Interested in learning more about it and generally agree that AI is stressing the current review paradigms a lot of us are accustomed to.
- deleted 2mo ago[deleted]
- 332451b 2mo agoFor me it's hard to imagine not starting on the second PR after submitting the first one, regardless of AI or stacked PRs. For focused work I find it important to avoid context switching, stay in the zone and keep everything in my head. Wasted work in comparison feels less significant to me. If review is fast I'd be switching back and forth between tasks throughout the day. If review is slow I might end up implementing a feature over the course of two weeks rather than two days. With reviewers in different time zones, limited time or doing their own focused work, it adds a lot of latency going back and forth for every PR rather than iterating on a stack of PRs. Personally I find the biggest benefit is that it lets both the author and reviewers work at the pace that works for them, with reduced context switching and latency.
- ghthor 2mo agoIf you make good commits and each of them is a valuable unit of work; having tooling that can manage rebasing them and creating a reviewable PR for each is super valuable for maintaining momentum and making reviews easier.
- lnrd 2mo ago> What's the benefit of this type of stacked PRs over a well-curated set of commits, and reviewing per commit? Even if the reviewer does the review commit-by-commit, all the comments and discussions will be on the same PR leading to multiple ongoing conversations about different topics that would be split if the PR are stacked. Also, all the new commits addressing the comments with spoil this commit-by-commit design, as the previous commits will be outdated and the new changes will be on top of those. I think it's beneficial for new changes to be a separate commit and not rewriting history, to not force the reviewers to re-read everything but just the latest changes.
- brabel 2mo agoI’ve been using stacked prs for many years, AI did not change anything in that area. The advantage is that you can control what lands in the trunk independently of what a particular review contains, and that you can commit freely without having to later go back and do archeology on past commits to make them look reviewable. The unit of commit and the unit of review is not the same and you shouldn’t try to make them so, they have entirely different purposes.
- m11a 2mo agoGitHub's PR workflow doesn't nicely support being able to review individual commits, realise which comments are associated with which commits, etc. Or shipping individual commits to main, while working on some others (unless you allow cherry-picking and direct push to main). Or amending a certain commit with respect to feedback and seeing a diff from the previous patch of that commit to the next. If GitHub's unit of change were a diff, and not a branch, then that would work pretty well.
- othmanosx 2mo agohave you tried pyor.review? it is actually just github but with better UI, and you get a commits rail that makes it easy to switch between commits on the fly