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

61–70 of 308 posts

Re: Stacked PRs are now live on GitHub

#61

Post by leo60228 »

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…

Is support for cross-fork stacked PRs coming in the near future? I was surprised that didn't come before the feature entered public preview, as it seems rather important for the feature to be useful on public repositories.

Re: Stacked PRs are now live on GitHub

#64

Post by skydhash »

a_t48 wrote: post #49
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…

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.

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

Re: Stacked PRs are now live on GitHub

#66

Post by pyth0 »

malcolmgreaves wrote: post #41
Insimwytim wrote: post #33

Earlier quoted context omitted.

I do that too. I do introduce changes in separate PRs. They could be related to the same problem and referenced accordingly. and the changes depend on each other so I can't open multiple independent PRs Why? Is it a technical restriction? Tightly coupled architecture is not the best solution anyway.

A refactor and then a feature implemented on that refactor is tightly coupled by definition. It would not make sense to implement the feature and refactor independently, because you’d be wasting time implementing a feature that you would have to refactor again.

This is literally the point of stacked PRs. You implement the refactor in branch A, then branch B is created on top of that so you can implement your feature. Then you make separate PRs, one for merging branch A into main, and another for merging branch B into A. The GitHub feature just provides a nicer UX when interacting with such PRs by clearly showing dependency and allowing you to merge a stack together.

Re: Stacked PRs are now live on GitHub

#67

Post by catlover76 »

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…

[dead]

Re: Stacked PRs are now live on GitHub

#68

Post by dastbe »

skydhash wrote: post #40
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…

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…

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

Re: Stacked PRs are now live on GitHub

#69

Post by a_t48 »

skydhash wrote: post #64
a_t48 wrote: post #49

Earlier quoted context omitted.

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.

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

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

Re: Stacked PRs are now live on GitHub

#70

Post by masklinn »

sameenkarim wrote: post #50
matharmin wrote: post #13

I've been using the preview for a bit, and I'm quite surprised to see them expanding the preview with so many unfixed issue. For example, merging an entire stack is completely broken in many cases: https://github.com/github/gh-stack/discussions/212 You can merge one by one, but if you're using squash and merge, you need a re-approval for each PR in the stack if you require reviews. This makes you lose out on arguably…

We're rolling out a series of bug fixes for the issues with squash merging. There's an internal system we have called CPRMC (Create Pull Request Merge Commit) that is used to evaluate whether a PR is "ready" to merge. This covers everything from mergeability (checking for merge conflicts) to rule evaluations (ensuring that approvals match the potential commit that will be created by merge) and more. This becomes part…

> There's an internal system we have called CPRMC (Create Pull Request Merge Commit) that is used to evaluate whether a PR is "ready" to merge. This covers everything from mergeability (checking for merge conflicts) to rule evaluations (ensuring that approvals match the potential commit that will be created by merge) and more.

By the way could there be a way to disable that when doing integrations externally? It seems to be quite costly (which makes sense), and the pull/ refs kinda bloat the reflist.

I’m sure that external integration is not exactly beloved internally but there’s really just a small handful of big annoyances which would make it so much nicer and more comfortable.

Post reply on HN