Comment by steveklabnik
1 day ago
Sorry, I am talking about stacked diffs in general, not this specific implementation on GitHub. That "Patch 2/5" is five stacked diffs, on top of each other. Forges that are stacked-diff native do that same kernel flow, just on the web instead of over email. You can also see this corroborated over here: https://news.ycombinator.com/item?id=47758251
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.
> Sorry, I am talking about stacked diffs in general, not this specific implementation on GitHub
My point is that the LKML and what GitHub do is so different that the definition of “stacked diffs in general” can only describe a tiny aspect of each, if you want to call both of their approaches by the same name. From where I sit, the only common element between them is “they offer a way to keep discussion separated.”
If that’s all people are actually complaining about, there are a thousand better ways to “keep discussion separated” that don’t require me to pretend that it’s ok that only a subset of my branch is ok to merge.
In git, a branch is the thing you either merge or don’t. You merge multiple commits at once, or you don’t. It’s a great model. Breaking up the branch into smaller pieces, and giving people the impression it’s ok to merge the first commit but not the rest, just to unfuck the discussion UX, is putting the cart before the horse. I make a branch strictly because I want it to either all merge or none of it merge. It’s the only sensible approach in my book. If a discussion system is so bad that this is unworkable, it means the discussion system is bad, it doesn’t mean the conceptual model of a merge is bad.
> My point is that the LKML and what GitHub do is so different that the definition of “stacked diffs in general” can only describe a tiny aspect of each
That's fine, what I mean is, when we started this convo, I thought you were asking about the general concept of stacked diffs, not the specifics of what GitHub is releasing here. That's my mistake for misunderstanding, sorry about that.
This is also (assumedly, anyway) why they're calling this "stacked PRs" and not "stacked diffs," because what they're doing is slightly different than Gerrit, Phabricator, Critique, etc.
Thanks for indulging me so far, by the way, I really appreciate this discussion, it's very stimulating.
After thinking about the whole thing I think I can summarize my opinion a lot better now:
Stacked diffs are a category error. Units of discussion, and units of integration, should not be conflated.
A branch is my unit of intended integration: merge all of it or none of it. The fact that reviewers need smaller slices to discuss does not imply those slices should become independently landable history objects. That’s a UX concern for the review tool, not something I should have to encode into Git history.
The ideal system would let me seed discussion however I want (by commit, by path, by subsystem, by semantic region of the diff, etc) without forcing me to pretend those are separate merge units.
Github nails the "merge unit" (CI runs against the whole branch, the branch either merges or doesn't, etc), but absolutely fumbles in the discussion part. I hate that I'd have to change the merge unit just to fix their discussion UX.