4 ms·

This seems like a fancy name for an ages old practice. Isn't that what Linux was doing even before git was developed?

goku12 3y ago
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).
- funcDropShadow 3y ago
  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.
jlokier 3y ago
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.

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).

funcDropShadow 3y ago
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.

jlokier 3y ago

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.