Comment by matharmin
13 hours ago
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.
> But if those separate parts are large, you really want more structure around it than just a commit.
Why? I reject the notion that large commits should be intrinsically hard to review.
GitHub already has the concept of "code owners", which are people who have ownership/review responsibility over slices of the codebase, based on globs/pattern matching. But they don't implement the other half of that, which is that a reviewer should be able to see a projection of a given PR, which matches the part of the repo they're the owner of.
There. That solves the entire problem of "this is too big, I can't look at all of it" (because your code ownership says this is the chunk of codebase you say you care about), and if that still isn't sufficient, there's a zillion UI features GitHub could add that they simply don't. Why can't I "focus" on a subset of the changes during review, in a way that helps me ignore unrelated discussions/changes? That is, even if I'm not code owner of the `frontend/` folder, why isn't there a UI affordance that says "let me focus on changes inside `frontend/` and ignore discussions/etc for the rest"?
> By using stacked PRs instead of just separate commits, I can add more commits to the individual PRs
Or you could just add commits to the PR, and if GitHub got the damned UI right, it would be natural to see the "slice" you care about, for all the new commits. Having to rearrange commits into separate PR's and slice-and-dice followup changes to file them into the right PR unit, is (to me) a workaround for how shitty GitHub's review UX is. It really shouldn't be this way.
> Then apart from the review process, there are lots of advantages to keeping changes small [...]
I agree with you on most of these points, but the decision to land smaller changes earlier should be made based on things like "let's get early feedback behind a feature flag" or "let's see how this chunk behaves in production so we can validate assumptions", or "let's merge what we have now so to cut back on conflicts", etc. That's all fine. But I'm vehemently opposed to being required to slice up my changes this way, just to work around a terrible review UI.
Personally, I review code in my development environment GitHub's UI is nonsensically terrible to read code. I could go on for hours about this[0], but when looking in my IDE I can drill into a subfolder and look at the diffs there. I can click and follow symbols. I can look at the individual diff history for any wildcarded subset of the repo, and see how the change was broken into commits. If I'm typing up some feedback to say "try doing it this way instead", I can actually try it myself first to make sure I'm not suggesting that someone do something that doesn't even compile.
And GH's discussion UX is by far the worst part of all of it. If you have a thread of discussions around a line of code, then wake up the next morning and want to see what new comments have been added? Good luck. Your best bet is to check your email inbox, because the comments are actually shown to you there. Using GitHub's "inbox" feature? All that is is a link to PR's you have to look at, with no hints at "why" (it could be a CI run finished for all you know.) Good luck figuring out "why" a PR is on your list. Did someone @-mention you? Who knows. So, find the blue dot next to the PR, click it, and then figure out for yourself what changed since the last time you looked. No, you can't just scroll and find it because GitHub hides half the discussions by default. So you have to go and expand all the collapsed sections to hopefully find that conversation you were having yesterday. But oh, you can only find it in the diff tab. So you click that, but the relevant file is collapsed by default ("Large diffs are not rendered blah blah"), so then click that. Then you may find that discussion.
Contrast this to a mailing list. The discussions are... discussion threads. You pick up where you left off. People's comments are right there in your inbox, newest one on top (or whatever your preference is.) You get notified when there's a new message, and when you tap the notification, it's the actual message, not some link to the PR that makes you click 6 more things to maybe find the message that just happened.
[0] like how the first thing you have to do when opening up the changes tab is ctrl+f search for "Large diffs are not rendered by default" to find the actually-important diffs that are not shown to you because GitHub's backend can't scale to rendering large diffs without friction. Countless times I've been burned by approving a PR because I don't see it making a change to some functionality, only to find out it actually did make said change, but GitHub just decided not to show me it. Seriously, the "large diffs" are the most important ones, and those are the ones you don't see without extra clicks. The mind boggles.)