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

GitHub Stacked PRs

github.github.com

Post reply on HN
Print view View original

221–230 of 548 posts

Re: GitHub Stacked PRs

#221

Post by steveklabnik »

ninkendo wrote: post #214
steveklabnik wrote: post #209

Earlier quoted context omitted.

> fixable with code owners files. Code owners automatically assigns reviewers. You still end up in the state where many groups are assigned to the same PR, rather than having independent reviews. > I already said GitHub lets you view single commits during PR review. Yes, you can look at them, but your review is still in the context of the full PR. > And I didn’t even bring up the fact that you can just open a second…

> You still end up in the state where many groups are assigned to the same PR > Yes, you can look at them, but your review is still in the context of the full PR. Why is this a bad thing? I don’t get it. This has literally never been a problem once in my career. Is the issue that people can’t possibly scroll past another discussion? Or… I seriously am racking my brain trying to imagine why it’s a bad thing to have mo…

It's totally possible that you aren't the target audience for this sort of feature. It tends to be more useful in very large team and/or monorepo contexts.

This isn't about siloing discussions: it's about focus. You can always see the full stack if you want to go look at the other parts, the key is that you don't have to.

The goal is to get thoroughly reviewed changes. 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. Keeping commits small and tightly reviewed leads to better outcomes in the end. Massive PRs lead to rubber stamps of +1.

I agree that that scenario sounds like a nightmare. But I don't think that a PR is the right place to solve that problem: it sounds like something that should have been sorted before any of the code was written in the first place.

Re: GitHub Stacked PRs

#222

Post by patrickthebold »

akersten wrote: post #79

Does 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'm not sure I follow your workflow exactly. If PR B is merged, then I'd expect PR A to already be merged (I'd normally branch off of A to make B.)

That said, after the squash merge of A and git fetch origin, you want something like git rebase --update-refs --onto origin/main A C (or whatever the tip of the chain of branches is)

The --update-refs will make sure pr B is in the right spot. Of course, you need to (force) push the updated branches. AFAICT the gh command line tool makes this a bit smoother.

Re: GitHub Stacked PRs

#223

Post by ninkendo »

steveklabnik wrote: post #221
ninkendo wrote: post #214

Earlier quoted context omitted.

> You still end up in the state where many groups are assigned to the same PR > Yes, you can look at them, but your review is still in the context of the full PR. Why is this a bad thing? I don’t get it. This has literally never been a problem once in my career. Is the issue that people can’t possibly scroll past another discussion? Or… I seriously am racking my brain trying to imagine why it’s a bad thing to have mo…

It's totally possible that you aren't the target audience for this sort of feature. It tends to be more useful in very large team and/or monorepo contexts. This isn't about siloing discussions: it's about focus. You can always see the full stack if you want to go look at the other parts, the key is that you don't have to. The goal is to get thoroughly reviewed changes. It's much easier to review five 100 line changes…

> 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 mergeable independently), proponents always seem to advocate that stacked diffs can somehow change this fact. “Oh if only we had stacked diffs we could break this into smaller changes”, ignoring the fact that no, they’d still be ordered and dependent on one another.

Stacked diffs seem like a UI convenience for reviewers… that’s fine I guess. GitHub is basically what you get when you ask the question “how can we make code review as tedious and unhelpful as possible”, and literally anything would be better than what we have (seriously I could fill a book with how bad GitHub is. I don’t think I could design a worse experience if I tried.) So, maybe I should just be happy they’re trying anything.

Re: GitHub Stacked PRs

#224

Post by p-e-w »

sunshowers wrote: post #109
js2 wrote: post #86

Earlier quoted context omitted.

> stacked diff workflow pioneered by Phabricator Ahem, pioneered by gerrit. But actually, I'm almost certain even that wasn't original art. I think gerrit just brought it to git. https://www.gerritcodereview.com/about.html

To my knowledge, stacked diffs were first done in the Linux kernel as stacks of patches sent over email. From there they spread to Google and Facebook. (Source: I worked on Facebook's source control team from 2012-2018 and did a lot of work to enable stacked diffs there.)

At some point, a derivative idea becomes so different from the original one that it’s a novel idea in essence. Just like SMS is ultimately a derivative of cuneiform tablets, and yet it isn’t in any meaningful sense.

Re: GitHub Stacked PRs

#225

Post by normie3000 »

stephbook wrote: post #62
topaztee wrote: post #13

how is this different than viewing a PR one commit at a time?

Commits are immutable and you never know which feedback goes stale when you add another commit. I'm not a huge fan, since stacked PRs mean the underlying issues don't get addressed (reviews clearly taking too long, too much content in there), but it seems they want something that works for their customers, right now, as they work in real life.

> Commits are immutable

I guess this is why you're getting downvoted. Commits can be edited.

Re: GitHub Stacked PRs

#226

Post by dathanb82 »

nine_k wrote: post #95
tcoff91 wrote: post #38

Earlier quoted context omitted.

Most editors have some kind of way to review github PRs in your editor. VSCode has a great one. I use octo.nvim since I use neovim.

Can these tools e.g. do per-commit review? I mean, it's not the UI what's the problem (though it's not ideal), it's the whole idea of commenting the entire PR at once, partly ignoring the fact that the code in it changes with more commits pushed. Phabricator and even Gerrit are significantly nicer.

Unless you have a “every commit must build” rule, why would you review commits independently? The entire PR is the change set - what’s problematic about reviewing it as such?

Re: GitHub Stacked PRs

#227

Post by altano »

fmbb wrote: post #170

> Large pull requests are hard to review, slow to merge, and prone to conflicts. Reviewers lose context, feedback quality drops, and the whole team slows down. OK, yeah, I’m with you. > Stacked PRs solve this by breaking big changes into a chain of small, focused pull requests that build on each other — each one independently reviewable. I don’t get this part. It seems like you are just wasting your own time building…

This _is_ a solution to slow reviews. Smaller reviews are faster to get in. And many small reviews take less time to review than one large review.

Plus there's no review that's instant. Being able to continue working is always better.

Re: GitHub Stacked PRs

#228

Post by muti »

MrJohz wrote: post #180
mikeocool wrote: post #138

Earlier quoted context omitted.

Constantly rewriting git history with squashes, rebases, manual changes, and force pushes has always seemed like leaving a loaded gun pointed at your foot to me. Especially since you get all of the same advantages with plain old stream on consciousness commits and merges using: git merge --no-ff git log --first-parent git bisect --first-parent

I find rebases are only a footgun because the standard git cli is so bad at representing them - things like --force being easier to write than --force-with-lease, there being no way to easily absorb quick fixes into existing commits, interdiffs not really being possible without guesswork, rebases halting the entire workflow if they don't succeed, etc. I've switched over pretty much entirely to Jujutsu (or JJ), which…

+1 on this, I also switched to jj when working with any git repo.

What's funny is how much better I understand git now, and despite using jj full time, I have been explaining concepts like rebasing, squashing, and stacked PRs to colleagues who exclusively use git tooling

Re: GitHub Stacked PRs

#229

Post by altano »

The `gh stack` CLI sounds essential for people using git, but I hope it doesn't become required, as people using things like jj/sl should be able to work with stacks. `gs submit`/`gs push` being the interface is fine, but `gs init` and `gs add` should be optional.

Re: GitHub Stacked PRs

#230

Post by steveklabnik »

ninkendo wrote: post #223
steveklabnik wrote: post #221

Earlier quoted context omitted.

It's totally possible that you aren't the target audience for this sort of feature. It tends to be more useful in very large team and/or monorepo contexts. This isn't about siloing discussions: it's about focus. You can always see the full stack if you want to go look at the other parts, the key is that you don't have to. The goal is to get thoroughly reviewed changes. It's much easier to review five 100 line changes…

> 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, everyone is also entitled to their preferences :)

Post reply on HN