logoalt Hacker News

Okkeftoday at 6:14 PM12 repliesview on HN

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.


Replies

dastbetoday at 6:28 PM

> 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.

show 1 reply
dualvariabletoday at 7:24 PM

> 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.

teeraytoday at 7:01 PM

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.

show 2 replies
Cedricgctoday at 6:45 PM

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

aeturnumtoday at 8:57 PM

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.

paxystoday at 8:09 PM

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.

show 1 reply
EduardoBautistatoday at 6:50 PM

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.

madeofpalktoday at 6:33 PM

it's basically a UI for a well-curated set of commits, and reviewing per commit.

mmlbtoday at 6:51 PM

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.

show 2 replies
IshKebabtoday at 8:35 PM

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.

hoppptoday at 6:46 PM

I think it is designed for AI Pull Requests. My impression was that it's for reviewing large generated changes.

catlover76today at 7:47 PM

[dead]