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…
GitHub Stacked PRs
241–250 of 548 posts
Re: GitHub Stacked PRs
#242Maybe 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'…
> The time of other devs is quite valuable and I can't imagine wasting it by having them review something that doesn't even make it in.
this is now what stacked diffs are for. stacked diffs doesn't mean putting up code that isn't ready. for example you are updating some library that needs an API migration, or compiler version that adds additional stricter errors. you need to touch hundreds of files around the repository to do this. rather than putting up one big diff (or PR) you stack up hundreds of them that are trivial to review on their own, they land immediately (mitigating the risk of merge conflicts as you keep going) then one final one that completes the migration.
Re: GitHub Stacked PRs
#243> 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…
you have hundreds or thousands of files to fix. that is unreviewable as a single commit, but as a per-file, per-library, per-oncall, etc. commit it is not that bad.
Re: GitHub Stacked PRs
#244Earlier quoted context omitted.
It's basically trying to bring the stacked diff workflow pioneered by Phabricator to GitHub. The idea is that it allows you to better handle working on top of stuff that's not merged yet, and makes it easier for reviewers to review pieces of a larger stack of work independently. It's really useful in larger corporate environments. I've used stacked PRs when doing things like upgrading react-native in a monorepo. It r…
What if main/master moves in between reviews?
1: https://git-scm.com/docs/git-rebase#Documentation/git-rebase...
Re: GitHub Stacked PRs
#245Does it fix the current UX issue with Squash & Merge? Right now I manually do "stacked PRs" like this: main If PR B merges first, PR A can merge to main no problems. If PR A merges to main first, fixing PR B is a nightmare. The GitHub UI automatically changes the "target" branch of the PR to main, but instantly conflicts spawn from nowhere. Try to rebase it and you're going to be manually looking at every non-conflic…
I agree that this is annoying and unintuitive. But I don’t see the simplest solution here, so: All you need to do is pull main, then do an interactive rebase with the next branch in your stack with ‘git rebase -i main’, then drop all the commits that are from the branch you just merged.
Re: GitHub Stacked PRs
#246Earlier quoted context omitted.
Whatever your opinion on one tool or another might be - it does seem weird that the "market" has been captured by what you are saying is a lesser product. IOW, what do you know that nobody else does?
Welcome to VHS and Betamax. the superior product does not always win the market.
Re: GitHub Stacked PRs
#247Re: GitHub Stacked PRs
#248Earlier quoted context omitted.
> It's much easier to review five 100 line changes than one 500 line one, and it's easier to review five 500 line changes than it is a 2500 line one. This is true if the changes are orthogonal and are truly independent. One should always favor small independent changes if one can. But when changes are all actually part of the same unit, and aren’t separable (apart from maybe the first of N of them which may be mergea…
In stacked diffs systems, the idea is that the base of the stack (once reviewed) can always be merged independently, so you're totally right that like, if you just purely think you can split things up when they shouldn't be split up, that would be bad. This is the model that the kernel uses, as well as tons of other projects (any Gerrit user, for example), and so it has gotten real-world use and at scale. That said,…
Nah.
The kernel uses a mailing list, and a “review” means a mailing list thread. With some nice CLI tools to integrate with git when you want to actually apply the patch (or start a review thread.)
In that world, “[PATCH 2/5]” (or whatever) in the subject title, and a different CC list for each patch, is a nice way to be able to ensure different subsets of the patch series have different discussions. That’s great.
But if you’re going to compare this to a GitHub UI, you have to choose the basis for comparison, because the two are so utterly different. Choosing one aspect (can we make sure discussions are kept separate), and saying “therefore the kernel uses stacked diffs” is a huge misrepresentation of how different GitHub’s approach is.
Because the kernel approach is the platonic ideal of a code review: it’s a simple threaded discussion between stakeholders, centered around a topic (the patch, which is inlined right in the email.) I would wager close zero kernel maintainer actually look at the diffs exclusively via their email client. They probably just check out the changes locally and look at them, and the purpose of the mailing list is to facilitate focused discussion on parts of the change (which is all we really want, in the end.)
GitHub has so thoroughly shit the bed on actually developing a good model of “threaded discussion about a change”, that you have to change the way you think about git’s model to fix how awful GitHub is at allowing review discussion to stay focused. You shouldn’t need to think about stacked diffs and multiple PR’s. You should use git branches as intended, multiple commits representing changes, and a merge meaning “this branch makes it or not.” That GitHub’s UI for discussing subsets of a change is so abysmal, does not mean the model is wrong. It means their discussion system is so abysmal that a mailing list TUI can run circles around it. Fixing this is GitHub’s problem, and doesn’t require any changes to how PR’s should be split up.
If you have a 2500-line PR with 5 500-line commits, GitHub should not require you to split things up further in any way, just to unfuck their discussion system.
Random idea I spent 10 seconds thinking about: let me start a “here’s a thread discussing the UI changes” and add folks to it, and “here’s a thread discussing the backend changes”, and add folks to that. I can then say “let’s not merge this until both threads are green”. You still see the whole change in the UI. (You can click directories to drill into the changes, that solves the “but the diff is too big” issue.) Discussion on a chunk of the diff is scoped to a discussion thread, which you select when sending the message. Thus, all discussion on any part of the diff is still scoped to a “discussion thread” of arbitrary subsets of stakeholders.
None of this needs me to change how I split up my git branches, an entire logical change is still either “merged” or “not-merged” (seriously who cares about the Pyrrhic victory of merging only change 1/N), and if we want to limit scopes of discussion to subsets of a change, we can just… do that.
Re: GitHub Stacked PRs
#249Earlier quoted context omitted.
My only complaint off the bat is the reliance on the GH CLI, which I don't use either. But maybe by the time it's GA they'll have added UI support.
Stacked PRs can be created via the UI, API, or CLI. You can also run a combination of these. For ex, use another tool like jj to develop locally, push up the branches, and use the gh CLI to batch create a stack of n PRs, without touching local state.
Re: GitHub Stacked PRs
#250Earlier quoted context omitted.
In stacked diffs systems, the idea is that the base of the stack (once reviewed) can always be merged independently, so you're totally right that like, if you just purely think you can split things up when they shouldn't be split up, that would be bad. This is the model that the kernel uses, as well as tons of other projects (any Gerrit user, for example), and so it has gotten real-world use and at scale. That said,…
> This is the model that the kernel uses Nah. The kernel uses a mailing list, and a “review” means a mailing list thread. With some nice CLI tools to integrate with git when you want to actually apply the patch (or start a review thread.) In that world, “[PATCH 2/5]” (or whatever) in the subject title, and a different CC list for each patch, is a nice way to be able to ensure different subsets of the patch series hav…
All of the advantages, like "it’s a simple threaded discussion between stakeholders, centered around a topic", is exactly why people like stacked diffs over PRs.
GitHub is doing "stacked PRs", which is like stacked diffs but more like PRs in the sense that they're stacked branches rather than stacked diffs. I agree that this seems less ideal, but they also are putting it into an existing project, rather than rebuilding everything around it. There's pros and cons to both approaches, but I agree that I'd prefer a native system built for this, personally. I'm still glad they're going to be popularizing the general concept.