4 ms·

> a chain of small, focused pull requests that build on each other — each one independently reviewable. I have never understood what this even means. Either c

> a chain of small, focused pull requests that build on each other — each one independently reviewable. I have never understood what this even means. Either changes are orthogonal (and can be merged independently), or they’re not. If they are, they can each be their own PR. If they’re not, why do you want to review them independently? If you reject change A and approve change B, nothing can merge, because B needs A to proceed. If you approve change A and reject change B, then the feature is only half done. Is it just about people wanting to separate logical chunks of a change so they can avoid get distracted by other changes? Because that seems like something you can already do by just breaking a PR into commits and letting people look at one of those at a time. I’ve tried my best to give stacked-diff proponents the benefit of the doubt but none of it actually makes sense to me.

steveklabnik 6mo ago
The canonical example here is a feature for a website that requires both backend and frontend work. The frontend depends on the backend, but the backend does not depend on the frontend. This means that the first commit is "independent" in the sense that it can land without the second, but the second is not, hence, a stack. The root of the stack can always be landed independently of what is on top of it, while the rest of the stack is dependent. > If they’re not, why do you want to review them independently? For this example, you may want review from both a backend engineer and a frontend engineer. That said, see this too though: > that seems like something you can already do by just breaking a PR into commits and letting people look at one of those at a time. If you do this in a PR, both get assigned to review the whole thing. Each person sees the code that they don't care about, because they're grouped together. Notifications go to all parties instead of the parties who care about each section. Both reviews can proceed independently in a stack, whereas they happen concurrently in a PR. > If you approve change A and reject change B, then the feature is only half done. It depends on what you mean by "the feature." Seen as one huge feature, then yes, it's true that it's not finished until both land. But seen as two separate but related features, it's fine to land the independent change before the dependent one: one feature is finished, but the other is not.
- Phelinofist 6mo ago
  If the layers of a stack have a disjoint set of reviewers things are viewed in separation which might lead to issues if there is no one reviewing the full picture.
  - steveklabnik 6mo ago
    That is why your forge will show that these two things are related to each other, and you may have the same person assigned to review both. It can show you this particular change in the context of the rest of them. But not every reviewer will always want to see all of the full context at all times.
- ninkendo 6mo ago
  > If you do this in a PR, both get assigned to review the whole thing. Each person sees the code that they don't care about, because they're grouped together. There are two separate issues you’re bringing up: - Both groups being “assigned” the PR: fixable with code owners files. It’s more elegant than assigning diffs to people: groups of people have ownership over segments of the codebase and are responsible for approving changes to it. Solves the problem way better IMO. - Both groups “seeing” all the changes: I already said GitHub lets you view single commits during PR review. That is already a solved problem. And I didn’t even bring up the fact that you can just open a second PR for the frontend change that has the backend commit as the parent. Yes, the second PR is a superset of the first, but we’ve already established that (1) the second change isn’t orthogonal to the first one and can’t be merged independently anyway, and (2) reviewers can select only the commits that are in the frontend range. Generally you just mark the second PR as draft until the first one merges (or do what Gitlab does and mark it as “depends on” the first, which prevents it from merging until the first one is done.) The first PR being merged will instantly make the second PR’s diff collapse to just the unique changes once you rebase/merge in the latest main, too. All of this is to explain how we can already do pretty much all of this. But in reality, it’s silly to have people review change B if change A hasn’t landed yet. A reviewer from A may completely throw the whole thing out and tell you to start over, or everything could otherwise go back to the drawing board. Making reviewers look at change B before this is done, is a potential for a huge waste of time. But then you may think reviewers from change B may opt to make the whole plan go back to the drawing board too, so what makes A so special? And the answer is it’s both a bad approach: just make the whole thing in one PR, and discuss it holistically. Code owners files are for assigning ownership, and breaking things into separate commits is to help people look at a subset of the changes. (Or just, like, have them click on the folder in the source tree they care about. This is not a problem that needs a whole new code review paradigm.)
charcircuit 6mo ago
>If you reject change A and approve change B, nothing can merge The feature is also half done in this case. The author can fix up the concerns the reviewer had in A and then both can be merged at the same time.
- ninkendo 6mo ago
  Couldn’t they do that in one PR? Seriously, couldn’t you just say “hey Alice, could you review the A parts of this PR” and “hey Bob, could you review the B parts”, then only merge once both of them approve? Even GitHub, for all its faults, supports code owners files, such that this can even be policy.
  - charcircuit 6mo ago
    Technically yes, but the work for A and B may not be done at the same time so you may want to get a head start on getting A reviewed while B is still being worked on. As a counter example. Why use multiple PRs when you can always just merge them into a single one. It's possible to make huge PRs with a bunch of different changes all included, but then the GitHub tools with managing stuff don't really work that well and you have to just do everything as comments instead of being about to actually accept a single accepted change for example.
esafak 6mo ago
Feature B depends on feature A, but you don't need B to understand A. Why wouldn't you create separate PRs?? It is faster to review and deploy.
- fmbb 6mo ago
  Of course you would create separate PRs. Why would you waste time faffing about building B on top of a fantasy version of A? Your time is probably better spent reviewing your colleague’s feature X so they can look at your A.
mh2266 6mo ago
you're upgrading the repository from language version 1 to 2, version 2 adds new compiler errors that rejects some old code, or the library has removed some old deprecated API the repository was still using in some places—the key here being that it can't be something that needs to be completely atomic. you have hundreds or thousands of files to fix. that is unreviewable as a single commit, but as a per-file, per-library, per-oncall, etc. commit it is not that bad.
- ninkendo 6mo ago
  > you have hundreds or thousands of files to fix. that is unreviewable as a single commit, but as a per-file, per-library, per-oncall, etc. commit it is not that bad Why is it intrinsically unreviewable as a single commit? Why can't the discussion/review system allow scoping discussions to a single folder of the change, or a single library, or a particular code-owner's "slice" of the repo, etc? The answer to this question is always unsatisfactory to me. It always ends up being "because GitHub's UI makes it hard to <foo>" and it's just taken as an immutable law of the universe that we're stuck with that UI's limitations. If a change is huge, find some basis by which to discuss it in smaller chunks. That basis doesn't have to be the PR itself (such that you have to make smaller PR's to make discussion manageable.) It can be a subdirectory of the diff. A wildcard-match over the source files. Whatever the case needs to be, the idea is still that the discussion UX shouldn't make reviewing large changes painful. Why do we tolerate the fact that GitHub doesn't let you say "approved for changes in `frontend/*`" or "approved for the changes I'm a code-owner of", and have the PR check system mark the PR as approved once all slices have been approved? Why do we tolerate that a thousand-file change is "unreviewable"? Instead we have to change our unit of integration, allowing partially-complete work to be merged, just because the review UX sucks.
  - dontlikeyoueith 6mo ago
    Why do you insist on a different but functionally equivalent solution to the problem? It's weird. > Why do we tolerate the fact that GitHub doesn't let you say "approved for changes in `frontend/*` That's literally what stacked PRs are adding.
nerdypepper 6mo ago
we have been stacking on tangled.org for a while now, you can see a few examples of stacks we have made here: https://tangled.org/tangled.org/core/pulls?state=merged&q=state%3Aopen&state=merged https://tangled.org/tangled.org/core/pulls?state=merged&q=st... for example, this stack adds a search bar: https://tangled.org/tangled.org/core/pulls/1287 https://tangled.org/tangled.org/core/pulls/1287 - the first PR in the stack creates a search index. - the second one adds a search API handler. - the last few do the UI. these are all related. you are right that you can do this by breaking a change into commits, and effectively that is what i do with jujutsu. when i submit my commits to the UI, they form a PR stack. the commits are individually reviewable and updatable in this stacking model. gh's model is inherently different in that they want you to create a new branch for every new change, which can be quite a nuisance. have written more about the model here: https://blog.tangled.org/stacking/ https://blog.tangled.org/stacking/
- ninkendo 6mo ago
  > - the first PR in the stack creates a search index. > - the second one adds a search API handler. > - the last few do the UI. So you're saying you're going to merge (and continuously integrate, perhaps to production) a dangling, unused search index, consuming resources with no code using it, just to make your review process easier? It's very depressing that review UX is so abysmal that you have to merge features before they're done just to un-fuck it. Why can't the change still be a big branch that is either all merged or not... and people can review it in chunks? Why do we require that the unit of integration equals the unit of review? The perverse logic always goes something like this: "This PR is too big, break it up into several" Why? "It's easier to review small, focused changes" Why can't we do that in one PR? "Because... well, you see GitHub's UI makes it really hard to ..." And that ends up being the root-cause answer. I should be able to make a 10,000 line change in a single commit if I want, and reviewers should be able to view subsets of it however they want: A thread of discussion for the diffs within the `backend` folder. A thread of discussion for the diffs within the `frontend` folder, etc etc. Or at the very least I should be able to make a single branch with multiple commits based on topic (and under no obligation for any of them to even compile, let alone be merge-able) and it should feel natural to review each commit independently. None of this should require me to contort the change into allowing integration partially-completed work, just to allow the review UX to be manageable.

steveklabnik 6mo ago

The canonical example here is a feature for a website that requires both backend and frontend work. The frontend depends on the backend, but the backend does not depend on the frontend. This means that the first commit is "independent" in the sense that it can land without the second, but the second is not, hence, a stack. The root of the stack can always be landed independently of what is on top of it, while the rest of the stack is dependent. > If they’re not, why do you want to review them independently? For this example, you may want review from both a backend engineer and a frontend engineer. That said, see this too though: > that seems like something you can already do by just breaking a PR into commits and letting people look at one of those at a time. If you do this in a PR, both get assigned to review the whole thing. Each person sees the code that they don't care about, because they're grouped together. Notifications go to all parties instead of the parties who care about each section. Both reviews can proceed independently in a stack, whereas they happen concurrently in a PR. > If you approve change A and reject change B, then the feature is only half done. It depends on what you mean by "the feature." Seen as one huge feature, then yes, it's true that it's not finished until both land. But seen as two separate but related features, it's fine to land the independent change before the dependent one: one feature is finished, but the other is not.

Phelinofist 6mo ago
If the layers of a stack have a disjoint set of reviewers things are viewed in separation which might lead to issues if there is no one reviewing the full picture.
- steveklabnik 6mo ago
  That is why your forge will show that these two things are related to each other, and you may have the same person assigned to review both. It can show you this particular change in the context of the rest of them. But not every reviewer will always want to see all of the full context at all times.
ninkendo 6mo ago
> If you do this in a PR, both get assigned to review the whole thing. Each person sees the code that they don't care about, because they're grouped together. There are two separate issues you’re bringing up: - Both groups being “assigned” the PR: fixable with code owners files. It’s more elegant than assigning diffs to people: groups of people have ownership over segments of the codebase and are responsible for approving changes to it. Solves the problem way better IMO. - Both groups “seeing” all the changes: I already said GitHub lets you view single commits during PR review. That is already a solved problem. And I didn’t even bring up the fact that you can just open a second PR for the frontend change that has the backend commit as the parent. Yes, the second PR is a superset of the first, but we’ve already established that (1) the second change isn’t orthogonal to the first one and can’t be merged independently anyway, and (2) reviewers can select only the commits that are in the frontend range. Generally you just mark the second PR as draft until the first one merges (or do what Gitlab does and mark it as “depends on” the first, which prevents it from merging until the first one is done.) The first PR being merged will instantly make the second PR’s diff collapse to just the unique changes once you rebase/merge in the latest main, too. All of this is to explain how we can already do pretty much all of this. But in reality, it’s silly to have people review change B if change A hasn’t landed yet. A reviewer from A may completely throw the whole thing out and tell you to start over, or everything could otherwise go back to the drawing board. Making reviewers look at change B before this is done, is a potential for a huge waste of time. But then you may think reviewers from change B may opt to make the whole plan go back to the drawing board too, so what makes A so special? And the answer is it’s both a bad approach: just make the whole thing in one PR, and discuss it holistically. Code owners files are for assigning ownership, and breaking things into separate commits is to help people look at a subset of the changes. (Or just, like, have them click on the folder in the source tree they care about. This is not a problem that needs a whole new code review paradigm.)

charcircuit 6mo ago

>If you reject change A and approve change B, nothing can merge The feature is also half done in this case. The author can fix up the concerns the reviewer had in A and then both can be merged at the same time.

ninkendo 6mo ago
Couldn’t they do that in one PR? Seriously, couldn’t you just say “hey Alice, could you review the A parts of this PR” and “hey Bob, could you review the B parts”, then only merge once both of them approve? Even GitHub, for all its faults, supports code owners files, such that this can even be policy.
- charcircuit 6mo ago
  Technically yes, but the work for A and B may not be done at the same time so you may want to get a head start on getting A reviewed while B is still being worked on. As a counter example. Why use multiple PRs when you can always just merge them into a single one. It's possible to make huge PRs with a bunch of different changes all included, but then the GitHub tools with managing stuff don't really work that well and you have to just do everything as comments instead of being about to actually accept a single accepted change for example.

esafak 6mo ago

Feature B depends on feature A, but you don't need B to understand A. Why wouldn't you create separate PRs?? It is faster to review and deploy.

fmbb 6mo ago
Of course you would create separate PRs. Why would you waste time faffing about building B on top of a fantasy version of A? Your time is probably better spent reviewing your colleague’s feature X so they can look at your A.

mh2266 6mo ago

you're upgrading the repository from language version 1 to 2, version 2 adds new compiler errors that rejects some old code, or the library has removed some old deprecated API the repository was still using in some places—the key here being that it can't be something that needs to be completely atomic. you have hundreds or thousands of files to fix. that is unreviewable as a single commit, but as a per-file, per-library, per-oncall, etc. commit it is not that bad.

ninkendo 6mo ago
> you have hundreds or thousands of files to fix. that is unreviewable as a single commit, but as a per-file, per-library, per-oncall, etc. commit it is not that bad Why is it intrinsically unreviewable as a single commit? Why can't the discussion/review system allow scoping discussions to a single folder of the change, or a single library, or a particular code-owner's "slice" of the repo, etc? The answer to this question is always unsatisfactory to me. It always ends up being "because GitHub's UI makes it hard to <foo>" and it's just taken as an immutable law of the universe that we're stuck with that UI's limitations. If a change is huge, find some basis by which to discuss it in smaller chunks. That basis doesn't have to be the PR itself (such that you have to make smaller PR's to make discussion manageable.) It can be a subdirectory of the diff. A wildcard-match over the source files. Whatever the case needs to be, the idea is still that the discussion UX shouldn't make reviewing large changes painful. Why do we tolerate the fact that GitHub doesn't let you say "approved for changes in `frontend/*`" or "approved for the changes I'm a code-owner of", and have the PR check system mark the PR as approved once all slices have been approved? Why do we tolerate that a thousand-file change is "unreviewable"? Instead we have to change our unit of integration, allowing partially-complete work to be merged, just because the review UX sucks.
- dontlikeyoueith 6mo ago
  Why do you insist on a different but functionally equivalent solution to the problem? It's weird. > Why do we tolerate the fact that GitHub doesn't let you say "approved for changes in `frontend/*` That's literally what stacked PRs are adding.

nerdypepper 6mo ago

we have been stacking on tangled.org for a while now, you can see a few examples of stacks we have made here: https://tangled.org/tangled.org/core/pulls?state=merged&q=state%3Aopen&state=merged https://tangled.org/tangled.org/core/pulls?state=merged&q=st... for example, this stack adds a search bar: https://tangled.org/tangled.org/core/pulls/1287 https://tangled.org/tangled.org/core/pulls/1287 - the first PR in the stack creates a search index. - the second one adds a search API handler. - the last few do the UI. these are all related. you are right that you can do this by breaking a change into commits, and effectively that is what i do with jujutsu. when i submit my commits to the UI, they form a PR stack. the commits are individually reviewable and updatable in this stacking model. gh's model is inherently different in that they want you to create a new branch for every new change, which can be quite a nuisance. have written more about the model here: https://blog.tangled.org/stacking/ https://blog.tangled.org/stacking/

ninkendo 6mo ago
> - the first PR in the stack creates a search index. > - the second one adds a search API handler. > - the last few do the UI. So you're saying you're going to merge (and continuously integrate, perhaps to production) a dangling, unused search index, consuming resources with no code using it, just to make your review process easier? It's very depressing that review UX is so abysmal that you have to merge features before they're done just to un-fuck it. Why can't the change still be a big branch that is either all merged or not... and people can review it in chunks? Why do we require that the unit of integration equals the unit of review? The perverse logic always goes something like this: "This PR is too big, break it up into several" Why? "It's easier to review small, focused changes" Why can't we do that in one PR? "Because... well, you see GitHub's UI makes it really hard to ..." And that ends up being the root-cause answer. I should be able to make a 10,000 line change in a single commit if I want, and reviewers should be able to view subsets of it however they want: A thread of discussion for the diffs within the `backend` folder. A thread of discussion for the diffs within the `frontend` folder, etc etc. Or at the very least I should be able to make a single branch with multiple commits based on topic (and under no obligation for any of them to even compile, let alone be merge-able) and it should feel natural to review each commit independently. None of this should require me to contort the change into allowing integration partially-completed work, just to allow the review UX to be manageable.