Stacked Diffs (and why you should know about them)

Yeah the piece that "stacking" also really helps on is code-review. So When you have the "contextual" diff in the second PR you can get different stake-holders to review that one, while maybe not needing them on PR 1 for example.

It also allows you to stay unblocked the entire time you're waiting on these dependent PRs.

For "how they do it on github": the way we do it at Graphite (spoiler I work there), is that we make the unit of change a PR instead of a commit. ie. every PR has one small commit, and these get stacked on top of each other. The tool itself abstracts some of the complexity out of rebasing and managing all of these stacked PRs (which in the article are referred to as stacked diffs).

Does that make sense?

The problem is that if you rebase branch1 based on feedback, your branch2 now has a dangling source commit. So you need rebase-fu to fix, and it’s per-stacked-branch. Even with rerere the merges can be annoying.

The tooling in Phab (and Graphite) makes stacks much easier to work with by auto-rebasing the whole stack as you push new changes to base commits.

The workflow you described is very hard to use in practice, for example:

1. If you edit A after reviewer comments, B will now have a dangling source. You'll need some rebase-fu to handle that, and the pain is significant if the chain is long. In stacked-diff workflow, it is handled automatically.

2. If you need to insert a new PR between A and B, or swap the order of some PRs in the chain, you'll need to manually do a lot of rebase-fu, and possibly manually updating the "target branch" on github. While in a stacked-diff workflow, all of these are handled automatically.

Per https://stacking.dev/, tools like sapling, spr, graphite.dev, ghstack all exist to handle the pain points you're mentioning. Agreed it's hard with vanilla git and github, but with extra tooling you can get it to work well.

Also, totally agree on the "smaller the better" - https://graphite.dev/blog/the-ideal-pr-is-50-lines-long

You have to make a new branch name for each diff. It's not too bad with vanilla Git.

I don't really understand why you'd do that unless one patch actually depends on another one though.

There seems to be a confusion. The stacked diff workflow mentioned here is just a way of sequencing a feature development by breaking it up into smaller submissions/PRs (for review and merge). The goal here is to reduce the time spent waiting for reviews and approval. This is similar to how processors achieve more throughput through pipelining (in terms of instruction scheduling). Here the 'stacking' they refer to is the stacking of feature branches.

I believe what you are referring to is the treatment of a change set as a stack of patches, using tools like quilt or stacked-git. The patch stack can be manipulated like pushing and popping patches off the stack, and merging changes in the working copy to the patch on top of the stack (called refreshing).

They are similar but it's different from Linux, which uses a better workflow than stacked diffs, imho.

In Linux kernel development, a PR is a sequence of commits that are reviewed both individually and in aggregate. The major difference is the resolution at which reviews are conducted, and the culture and tooling which facilitates that. It's a bit more zoomed out than typical company PR processes, so it gets more of a design review aspect (from zooming out), alongside focusing on correctness (from zooming in and from the potential reviewers watching different affected subsystems). The result is higher quality individual commits with higher quality commit descriptions.

Some individual commits in the sequence are approved easily and quickly, while others may prompt a design review or code changes, or be asked to be removed. Each of the commits must have a good commit message by itself, so that it can be understood and reviewed independently, and used standalone if appropriate. Each commit must not break the build; the kernel should be fine after each one is applied in turn. Each round of review, if there are multiple rounds, tends to look at a rebased-on-main new sequence.

The significant difference from stacked diffs is that stacked diffs don't generally involve reviewing the sequence as a whole as well as individual commits in it. Stacked diffs also tend to approve and merge some PRs while the rest of the sequence is still being written.

Linux kernel PRs tend to be accepted or not as a whole when ready (although this is not strict and individual changes from the PR are often cherry picked into main). The added context from considering them as a whole is important for many kinds of complex changes, and it adds an extra dimension of design review.

Linux kernel PR merges preserve the commit sequence in hisory; it's never a squash merge, and each commit must be high quality by itself. This is the opposite practice to how many places do GitHub PR merges that have multiple commits: Those places squash-merge the sequence down to a single commit on merge, losing all distinctions, and encouraging low quality commit messages (often a one-liner). Some see squashing as best practice, and some even enforce it with company policies and GitHub repo settings. Linux kernel devs would never approve of that.

For example, consider a feature which requires (1) some internal API changes motivated by the needs of the new feature, (2) some refectoring or cleanup of existing internals motived by the needs of the new feature (3) the new feature's user-facing new API #1 (such as a syscall), (4) the new feature's user-facing new API #2 (such as a file in /proc to show status).

Each of those (1) to (4) is also an individual, meaningful change which can be reviewed by itself for correctness, doesn't break the kernel and is smaller than the whole feature. They have to be in that order because any other order will break the kernel if the sequence is partially applied. They may reviewed by different people, especially if they touch different subsystems. With a "swarm" review culture like the Linux kernel where anyone interested might take a look, the smaller commits are more likely to be picked up for feedback by people who don't have time to look at the whole sequence.

So far this is like stacked diffs.

But when (1) to (4) are reviewed in aggregate, the motivation for (1) and (2) is made clear by the later commits which use them. Reading (3) and (4) informs the review of (1) and (2), and feedback will consider the interactions between those different changes. For example proposiing a different internal API change, because they can see how it's later used to solve a problem.

This affects code quality, design, direction, communication and culture. If you submit (1) and (2) as separate PRs in a typical company GitHub PR workflow ("one at a time" and "squash merge"), someone is more likely to wonder why you're pushing refactors or churn for no clear reason, or bloating the code with "nice to have" unnecessary internal features, unless the PR includes a message explaining that the motivation is further changes you intend to submit afterwards which needs those refactorings first. You might have to explain the motivation in some detail, which is a waste of time given there is code in a later PR showing it used. People don't always read the PR description anyway, and even if they do, months later other people who weren't involved may not see changes (3) and (4). Also in that culture, people aren't always sure if "intend to submit next" means things you'll do later if there's enough time, or how long in the future. Perhaps you submit all 4 PRs together for clarity, but if you keep doing that (and with larger numbers) people get review fatigue because of how well-known GUIs present them. Of course culture plays a big role. In some places you'll get in trouble if you appear to do refactorings "by themselves" that aren't attached to a feature or bug ticket, so you have to combine refactors and new features into one PR, which will be squashed. In others, they'll trust you're doing changes for a good reason even if it' s not clear why yet. But in those "accept by default" cultures designs can meander more because motivations aren't communicated as clearly. I've yet to see a place where someone sees 15 small linked PRs in GitHub and reads all of them before doing feedback on the first one, but I've seen that often with the Linux kernel.

In theory, GitHub's PRs made of multiple commits can be used the same way as the Linux kernel method. But there's a strong culture of squash-merging, poor individual commit messages, intermediate commits that break the build because it is known they will be squash-merged anyway and nobody cherry-picks, avoiding rebasing so there are often "trying X, undid X, trying X2" commits in the PR, and reviewing the PR diff as a whole instead of looking at the individual commits. So within GitHub's GUI, the Linux kernel method of using PRs is difficult to do, discouraged, and seems hard. It's just because of the tools though.