Comment by Okkef
4 hours 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.
> 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.
IMO, 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.
> That can be easily done with comments. If the PR are orthogonal, they could have been split.
Comments are ad-hoc and don't scale, relying on the author to interpret and adhere to the extent of the reviewers approval.
> 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.
you are free to look up, down, and around the stack; nobody is hiding the code from you. But in many cases this is just unnecessary.
The 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.
2 replies →
> 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.
It’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.
With 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
I 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.
You 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.
I 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.
it's basically a UI for a well-curated set of commits, and reviewing per commit.
I'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.
I don't get it, we already have CI for each commit, at least at our workplace, enforced.
> 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.
>Can't you run your CI locally?
If you mean automated tests and linters, sure.
Conceptually continuous integration is generally integrating 2+ different lineages of code together which is more common with multiple developers although I suppose it's becoming more relevant with agents creating a bunch of worktrees with different things.
In practice, CI has taken the same path as "DevOps Engineer" ie most people just mean "automated test server"
The opposite of CI is more-or-less merge windows or merge-fest like Linux where everyone mails in their changes and someone manually integrates.
You 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.
2 replies →
1. 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.
I think it is designed for AI Pull Requests. My impression was that it's for reviewing large generated changes.
[dead]