It's not clear to me how this approach would work with common tools like GitHub and gitlab. If we look at the visualisation[0] of the proposed approach, each diff goes onto the original PR branch and gets reviewed and merged independently. But neither GitHub not gitlab has this capability. A commit on a PR/MR branch becomes a part of the changeset and it will all be merged together.
Gitlab allows you to review individual versions of a merge request (GitHub probably does too, I haven't used it in years). So the review stages here would work, although this workflow falls apart if the author ever rebases or merges master.
Unless I'm missing something, you would need to create pull requests into PR branches and then change their targets as each one gets merged, or target them all at master but notify reviewers to please ignore the dependent changes and only review this stack.
Ultimately this feels like a hack to work around bad processes and bad CI/CD. I encourage developers to work in the smallest pull requests possible and focus on getting each work item merged, rather than hopping around from work item to work item. Very small PRs can be reviewed quickly. This workflow depends on a good CI pipeline, feature flagging and excellent communication in the team. I'm absolutely in favour of breaking features into multiple PR's. The smaller the better - smaller than you think.
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.
We have frameworks and project generators that could be repos you fork. The main advantage being more help with upgrading to versions with breaking changes (generators are terrible at this, and I end up faking this myself to see the diffs).
When I was at AWS, we used the same technique and I highly recommend it. No coupling to mainline, reviews made in parallel. For us one thing was super important: the commit cannot break the build, because it messes up with automated bisection, which was (is?) quite critical.
The way this is explained is like it hasn't occurred to them that 'create a branch' doesn't have to mean '...off master' - I continue working with a new branch that depends on the old thing that's in a PR still simply by basing it on top - but if this comes from Google and Meta and is popular, obviously that's not the case.
My guess is that it's more about somehow seeing a more 'contextual' (not semantic exactly) diff in the second PR? So rather than including all of PR 1's changes that it's based on, just showing the actual diffs in the commits in isolation, or as if rebased (only PR 2's intended patch set, not the master fork point through tip of PR 1) on master?
Though I haven't read further than I can as a non-subscriber, perhaps it would be clearer when it explains how they're doing it on GitHub (the above would require some rebasing or target branch and further PR automation).
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).
Yeah kind of. It's just a bit confusing to read about presented as an entirely different/novel thing when it's pretty much my mental model of git and how I use it anyway.
If PR2 is done before PR1 is merged I tend to put it up as a draft with the target branch set to that of PR1, and say it depends on PR1. Then the diff shown is only PR1..PR2 anyway.
AIUI what I don't get doing that is the ability to potentially merge PR2 before PR1? But then it depended on it for some reason anyway, so..? (And I gain the ability to still have multiple commits in a PR / reviewed 'thing'.)
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.
Ah ok, fair enough. I can certainly appreciate that would be helpful, because yeah the 'annoying rebase-fu' is how I do that.
TFA seems to get hung up on reviewing individual commits instead of a set of them ('a diff is basically a commit' - err well yes) when actually the benefit per my guesswork and your description is more about the automation, and there's no inherent reason (as far as we've discussed anyway) that it couldn't be doing that with a set of multiple commits anyway.
> So you need rebase-fu to fix, and it’s per-stacked-branch.
Note that `git rebase --update-refs` lets you just rebase the last branch and the entire stack gets updated. You just need to know which branches to forcepush.
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).
Yes, I am referring to exchanging sets of patches with or without tools like quilt. But I don't see a confusion. These stacked diffs seem to be a recreation of the advantages of patchsets on top of the limitting PR model of GitHub.
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 ch...
Not entirely sure why a new term and why merge those local 'diffs' to your local 'main' at all?
Open PR A-->main, while it's getting reviewed (or maybe it's in WIP state for some reason), create a branch B off A, and eventually open PR B-->A. Now, you never actually merge B-->A, it's only there so that the PR can be reviewed and diff can be seen. Can be repeated as needed.
In fact, IIRC, in the example above, if you merge PR A, GitHub will switch PR B's target branch to whatever A was merged to (in this case, main).
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.
> 1. … You'll need some rebase-fu to handle that, and the pain is significant if the chain is long.
`git rebase --update-refs` on the last branch will handle updating of all the intermediate branches in the stack, you just need to force-push them all at the end.
It does help with 2. as well, but you will indeed need to manually update the target branch of a single PR on GitHub.
18 comments
[ 0.24 ms ] story [ 41.8 ms ] threadGitlab allows you to review individual versions of a merge request (GitHub probably does too, I haven't used it in years). So the review stages here would work, although this workflow falls apart if the author ever rebases or merges master.
Unless I'm missing something, you would need to create pull requests into PR branches and then change their targets as each one gets merged, or target them all at master but notify reviewers to please ignore the dependent changes and only review this stack.
Ultimately this feels like a hack to work around bad processes and bad CI/CD. I encourage developers to work in the smallest pull requests possible and focus on getting each work item merged, rather than hopping around from work item to work item. Very small PRs can be reviewed quickly. This workflow depends on a good CI pipeline, feature flagging and excellent communication in the team. I'm absolutely in favour of breaking features into multiple PR's. The smaller the better - smaller than you think.
0 https://substackcdn.com/image/fetch/w_728,c_limit,f_webp,q_a...
Also, totally agree on the "smaller the better" - https://graphite.dev/blog/the-ideal-pr-is-50-lines-long
We have frameworks and project generators that could be repos you fork. The main advantage being more help with upgrading to versions with breaking changes (generators are terrible at this, and I end up faking this myself to see the diffs).
I don't really understand why you'd do that unless one patch actually depends on another one though.
My guess is that it's more about somehow seeing a more 'contextual' (not semantic exactly) diff in the second PR? So rather than including all of PR 1's changes that it's based on, just showing the actual diffs in the commits in isolation, or as if rebased (only PR 2's intended patch set, not the master fork point through tip of PR 1) on master?
Though I haven't read further than I can as a non-subscriber, perhaps it would be clearer when it explains how they're doing it on GitHub (the above would require some rebasing or target branch and further PR automation).
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?
If PR2 is done before PR1 is merged I tend to put it up as a draft with the target branch set to that of PR1, and say it depends on PR1. Then the diff shown is only PR1..PR2 anyway.
AIUI what I don't get doing that is the ability to potentially merge PR2 before PR1? But then it depended on it for some reason anyway, so..? (And I gain the ability to still have multiple commits in a PR / reviewed 'thing'.)
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.
TFA seems to get hung up on reviewing individual commits instead of a set of them ('a diff is basically a commit' - err well yes) when actually the benefit per my guesswork and your description is more about the automation, and there's no inherent reason (as far as we've discussed anyway) that it couldn't be doing that with a set of multiple commits anyway.
Note that `git rebase --update-refs` lets you just rebase the last branch and the entire stack gets updated. You just need to know which branches to forcepush.
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).
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 ch...
Open PR A-->main, while it's getting reviewed (or maybe it's in WIP state for some reason), create a branch B off A, and eventually open PR B-->A. Now, you never actually merge B-->A, it's only there so that the PR can be reviewed and diff can be seen. Can be repeated as needed.
In fact, IIRC, in the example above, if you merge PR A, GitHub will switch PR B's target branch to whatever A was merged to (in this case, main).
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.
> 1. … You'll need some rebase-fu to handle that, and the pain is significant if the chain is long.
`git rebase --update-refs` on the last branch will handle updating of all the intermediate branches in the stack, you just need to force-push them all at the end.
It does help with 2. as well, but you will indeed need to manually update the target branch of a single PR on GitHub.