Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Stacked PRs are now live on GitHub

github.blog

Post reply on HN
Print view View original

241–250 of 308 posts

Re: Stacked PRs are now live on GitHub

#241

Post by [deleted] »

eddythompson80 wrote: post #230
hypendev wrote: post #217

Earlier quoted context omitted.

But 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?

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

Re: Stacked PRs are now live on GitHub

#242

Post by Game_Ender »

sunshowers wrote: post #149
sameenkarim wrote: post #44

Hey from the GitHub Stacked PRs team! Excited to release this more broadly so anyone can start stacking: https://gh.io/stacks Would love to hear any feedback, especially with the UI and CLI. We've got a lot more updates to the PR experience in store! Also happy to answer questions about the design decisions we made. There's a bunch happening behind the scenes, and it's one of the largest launches in GitHub history co…

I apologize for being somewhat direct but what prior art did you engage with? Why are you making people create a branch for each change in a stack? Why do developers have to create new commits when iterating on the PR? Where is the proper support for interdiffs? What about change IDs? The fundamental issue with GitHub -- really, its original sin -- is that the review model is wrong. It encourages a new commit + merge…

About 4.5 years ago they had an internal prototype for changing PRs to be diff based instead of commit based. I even saw some screenshots of it! I assume the change was too drastic, or had too many corner cases so it was dropped. There is a lot of complexity in there system around PR status checks that I could see possibly having issues.

The optimistic person in me thinks now that have a major feature which rebases and amends all the time, they will dust that off and get it shipped. Like you said, is shows a fundamental miss-understanding of the problem.

Re: Stacked PRs are now live on GitHub

#243

Post by beaker52 »

IshKebab wrote: post #239
beaker52 wrote: post #220

Looking at the way GitHub are selling this “feature”, I feel like some of the engineers who are going to be excited about this feature for “reviewability” reasons are, in particular, those who’ve forgotten that they should be splitting changes into multiple logical commits inside a PR. And instead of that they’re now going to use multiple, single commit branches and stack them because stacked PRs are a “new” “feature…

Splitting changes into multiple commits is a much worse experience if you actually have self-contained dependent changes. 1. The whole review interface isn't set up for reviewing individual commits. 2. You can't merge changes progressively. 3. CI doesn't run on each commit. 4. If you have linear history (good idea IMO) you'll lose your nice commit history when you merge it. This is much better.

You can read each individual commit and review the PR as a whole. This is the way many people design and review PRs.

You don’t need to merge those changes progressively. If you do, you go through exactly the same process of creating a separate branch and PR. The only difference is that GH has now added some UI and automation for rebasing and merging the PRs. In the past we would have explained the chaining in the PR and rebased manually.

You don’t need CI to run on each commit.

You only lose your commit history if you squash merge, many people don’t, and you don’t have to either.

The arguments come from angle that doesn’t appear to be aware that stacked PRs were a thing before GH made these UX improvements.

Re: Stacked PRs are now live on GitHub

#244

Post by skydhash »

tdeck wrote: post #236
skydhash wrote: post #40

Earlier quoted context omitted.

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, the…

This articulates a source of unease I was having trouble pinpointing. Encouraging piling up changes like this puts pressure on the reviewer to approve things as-is because "i've already built Y and Z on top of X so changing X now is a pain". In my opinion there's already too much of a bias toward stamping PRs, and organizations that maintain high standards do it partly by resisting that bias in key parts of the codeb…

I'm strongly on the camp that one PR review shouldn't take more than 10 minutes (vibe number) to review, including reading a linked design spec or speaking with the author for more clarification. Even if I wanted to pull down the branch to test locally, that shouldn't add more than 2 minutes.

Anything that would take a bit longer is either a failure of communication (no design specs, no PR description), a failure of tooling (setting up the project, running tests,...) or a lack of policies (no big changes in a PR).

Re: Stacked PRs are now live on GitHub

#245

Post by defmacr0 »

nirvdrum wrote: post #183
necovek wrote: post #173

I dislike them reinforcing the component approach to delivering work through their examples, like the top screenshot showing "database schema changes", "api changes" and "frontend implementation" as separate branches in a stack. So really, one does consider full stack a single feature, but unless they are reviewed in one go — which defeats the purpose of stacked branches and pull requests — you can end up landing one…

Thanks for mentioning this. It seemed odd to me, too, so I spent some time trying to work it out. As a reviewer, I'm not sure how I'm supposed to assess database or API changes without knowing how they're intended to be used. And deploying them independently seems odd, too, especially if you need to roll it all back. I think in my ideal world there would be a clean history and I could review a PR commit-by-commit. Bu…

Yeah, clean, neatly seperated and logically independent PRs are very nice for reviewers, but usually it requires one to complete the whole feature and then go back and think about the best way to seperate it into a series of smaller changes again. It works for projects like linux where there is tons of motivated manpower such that requiring authors put in a day of additional effort to make a change as presentable as possible is acceptable.

Re: Stacked PRs are now live on GitHub

#246

Post by m11a »

I think it's telling how long it took GitHub to release a v1 of this feature. Folks have wanted this for a long time. Graphite came along and did it years ago (and I'm sure they pondered whether GitHub would do this).

And the v1 is also a bit... basic, and buggy. And I'm surprised there's not clear documentation for agents (given using GitHub stacked PRs CLI won't be in models' training data yet).

It does feel like GitHub hasn't been great at shipping new features for a few years now. Nonetheless, I'm glad to see this rolling out. Once polished, it's going to be exciting to use.

Re: Stacked PRs are now live on GitHub

#249

Post by m11a »

Okkef wrote: post #17

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 w…

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

Re: Stacked PRs are now live on GitHub

#250

Post by m11a »

hypendev wrote: post #217
dastbe wrote: post #21

Earlier quoted context omitted.

> 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 gr…

But 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?

In 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.
Post reply on HN