Live data from Hacker News

GitHub Stacked PRs

github.github.com

301–310 of 548 posts

Re: GitHub Stacked PRs

#301
post #255

I really don't get the point of stacked PRs. Just using git, you'd send a set of patches, which can be reviewed, tested and applied individually. The PR workflow makes a patch series an undivisible set of changes, which must be reviewed, tested and applied in unison. And stacked PRs tries to work around this issue, but the issue is how PRs are implemented in the first place. What you really want is the ability to rev…

It’s useful for large PRs in large repos with many contributors. It reduces the burden for reviewers.

Still not sure this is the right solution. My problem is if one of your first stages gets rejected in review or requires significant changes, it invalidates so much work that comes after it. I've always when possible preferred to get small stuff merged in to production as it happens rather than build an entire feature and put it up for review.

Re: GitHub Stacked PRs

#302

Earlier quoted context omitted.

What is kind of funny here is that you're right locally. At the same time, the larger tech companies (Meta and Google, specifically) ended up building off of hg and not git because (at the time, especially) git cannot scale up to their use cases. So while the git CLI was super fast, and the hg CLI was slow, "performance" means more than just CLI speed. I was never a fan of hg either, but now I can use jj, and get som…

>At the same time, the larger tech companies (Meta and Google, specifically) ended up building off of hg and not git because (at the time, especially) git cannot scale up to their use cases. Fun story: I don't really know what Microsoft's server-side infra looked like when they migrated the OS repo to git (which, contrary to the name, contains more than just stuff related to the Windows OS), but after a few years the…

didn't msft write an ~entire new file system specifically to scale git to the windows code base?

I have fuzzy memories on reading about it.

Re: GitHub Stacked PRs

#303
post #168

Maybe this is just a skill issue, but even with several attempts I just can't figure out why I would use stacked diffs/PRs. Though maybe that's because of the way I work? I notice a lot of examples just vaguely mention "oh, you can have others review your previous changes while you continue working", but this one doesnt make sense to me. Often times, the first set of commits doesn't even make it to the end result. I'…

We use this feature extensively at $dayjob. Imagine you have some task you are working on, and you wish to share your progress with people in bite sized chunks that they can review one at a time, but you also don’t want to wait for their reviews before you continue working on your task. Using a stacked set of PRs you can continue producing new work, which depends on the work you’ve already completed, without waiting…

I've always done this by just creating new PRs that are based on the branch of the old one. Maybe this is a UI improvement?

Re: GitHub Stacked PRs

#304
This just reeks to me of bad practice. Why use this as opposed to breaking your change into smaller PRs and merging them individually behind a feature flag or similar? With this, you have a marginally better UX for reviewing through the Github website, but the underlying issues are the same. The change being introduced is not sufficiently testable by itself, or it's (somehow) too tightly coupled to other parts of the UI/codebase that it can't be split. You still need to test for integration issues at every point of the stack, and some architecture issues or points of code reuse can't be seen from stacked changes like this.

Not for me, but I'm glad it fits other people's workflows. I just hope it doesn't encourage people to try make poorly reasoned changes!

Re: GitHub Stacked PRs

#307

Earlier quoted context omitted.

What is kind of funny here is that you're right locally. At the same time, the larger tech companies (Meta and Google, specifically) ended up building off of hg and not git because (at the time, especially) git cannot scale up to their use cases. So while the git CLI was super fast, and the hg CLI was slow, "performance" means more than just CLI speed. I was never a fan of hg either, but now I can use jj, and get som…

>At the same time, the larger tech companies (Meta and Google, specifically) ended up building off of hg and not git because (at the time, especially) git cannot scale up to their use cases. Fun story: I don't really know what Microsoft's server-side infra looked like when they migrated the OS repo to git (which, contrary to the name, contains more than just stuff related to the Windows OS), but after a few years the…

I have heard that the Google monorepo is called google3 but I don't know why. Maybe those things are common...

Re: GitHub Stacked PRs

#308

> a chain of small, focused pull requests that build on each other — each one independently reviewable. I have never understood what this even means. Either changes are orthogonal (and can be merged independently), or they’re not. If they are, they can each be their own PR. If they’re not, why do you want to review them independently? If you reject change A and approve change B, nothing can merge, because B needs A t…

>If you reject change A and approve change B, nothing can merge The feature is also half done in this case. The author can fix up the concerns the reviewer had in A and then both can be merged at the same time.

Couldn’t they do that in one PR? Seriously, couldn’t you just say “hey Alice, could you review the A parts of this PR” and “hey Bob, could you review the B parts”, then only merge once both of them approve? Even GitHub, for all its faults, supports code owners files, such that this can even be policy.

Re: GitHub Stacked PRs

#309
I've been doing stacked PRs for ~2 years now. Thus, I don't quite see the need for this CLI. Git has had some additions in the last few years that make this work natively – specifically the --update-refs flag[1] or the rebase.updateRefs config. Combined with `git commit --fixup`, rebase.autoStash, and rebase.autoSquash rebasing stacks becomes a breeze (as long as you work off from the tip of your stack). Add in git-absorb[2] and the heavy-lifting is taken care of.

My biggest gripe with GitHub when working with stacks – and something that's not clarified in these docs – is whether fast-forward merges are possible. Its "Merge with rebase" button always rewrites the commit. They do mention that the stack needs to be rebased in order to merge it. My workaround has been `git merge --ff-only top-branch-of-stack` to merge the entire stack locally into main (or anything in between actually) and then push. GitHub neatly recognizes that each PR in the stack is now in main and marks them all as merged. If there are subsequent PRs that weren't merged it updates the base branch.

Having said that, it's great to see GitHub getting a proper UI for this. It's also great that it understands the intent that branch B that goes on top of branch A is a stack and thus CI runs against. I just hope that it's not mandatory to use their CLI in order to create stacks. They do cover this briefly in the FAQ[3], but it might be necessary to use `gh stack init --adopt branch-a branch-b branch-c`. On the other hand, if that removes the need to manually create the N PRs for my stack, that's nice.

[1]: https://git-scm.com/docs/git-rebase#Documentation/git-rebase...

[2]: https://github.com/tummychow/git-absorb

[3]: https://github.github.com/gh-stack/faq/#will-this-work-with-...

Re: GitHub Stacked PRs

#310

Earlier quoted context omitted.

we have been stacking on tangled.org for a while now, you can see a few examples of stacks we have made here: https://tangled.org/tangled.org/core/pulls?state=merged&q=st... for example, this stack adds a search bar: https://tangled.org/tangled.org/core/pulls/1287 - the first PR in the stack creates a search index. - the second one adds a search API handler. - the last few do the UI. these are all related. you are ri…

> - the first PR in the stack creates a search index. > - the second one adds a search API handler. > - the last few do the UI. So you're saying you're going to merge (and continuously integrate, perhaps to production) a dangling, unused search index, consuming resources with no code using it, just to make your review process easier? It's very depressing that review UX is so abysmal that you have to merge features be…

This is not just about the UI, it's about the mental model and management of the changes.

Just covering the review process:

Yes, you can structure your PR into 3 commits to be reviewed separately. I occasionally structure my PRs like this - it does help in some cases. But if those separate parts are large, you really want more structure around it than just a commit.

For example, let's say you have parts A, B and C, with B depending on A, and C depending on B.

1. I may want to open a PR for A while still working on B. Someone may review A soon, in which case I can merge immediately. Or perhaps it will only be reviewed after I finished C, in which case I'll use a stacked PR. 2. The PR(s) may need follow up changes after initial review. By using stacked PRs instead of just separate commits, I can add more commits to the individual PRs. That makes it clear what parts those commits are relevant to, and makes it easy to re-review the individual parts with updated changes. Separate commits don't give you that.

Stacked PRs is not a workflow I'd use often, but there are cases where it's a valuable tool.

Then apart from the review process, there are lots of advantages to keeping changes small. Typically, the larger a change, the longer it lives in a separate branch. That gives more time for merge conflicts to build up. That gives more time for underlying assumptions to change. That makes it more difficult to keep a mental map of all the changes that will be merged.

There are also advantages to deploying small changes at a time, that I won't go into here. But the parent's process of potentially merging and deploying the search index first makes a lot of sense. The extra overhead of managing the index while it's "unused" for a couple of days is not going to hurt you. It allows early testing of the index maintenance in production, seeing the performance overhead and other effects. If there's an issue, it's easy to revert without affecting users.

The overall point is that as features become large, the entire lifecycle becomes easier to manage if you can split it into smaller parts. Sometimes the smaller parts may be user-visible, sometimes not. For features developed in a day or two, there's no need to split it further. But if it will span multiple weeks, in a project with many other developers working on, then splitting into smaller changes helps a lot.

Stacked PRs is not some magical solution here, but it is one tool that helps manage this.

Post reply on HN