4 ms·

I dislike them reinforcing the component approach to delivering work through their examples, like the top screenshot showing "database schema changes", "api cha

by necovek 3mo ago

I dislike them reinforcing the component approach to delivering work through their examples, like the top screenshot showing "database schema changes", "api changes" and "frontend implementation" as separate branches in a stack. So really, one does consider full stack a single feature, but unless they are reviewed in one go — which defeats the purpose of stacked branches and pull requests — you can end up landing one and a later review in branches higher in the stack needing changes in the lower branches even if they were already reviewed. When you instead focus on full use-case per branch, but scope them down, it is much less likely you will need to change branches lower in the stack after they are reviewed. Another obvious use-case is to do a pre-emptive refactor, though I actually prefer doing a post-refactor after the new use-case has been merged in — it's much easier to know the target best approach when you've got your use-cases right in front of you (or you may hit a similar problem as above). FWIW, I remember fondly using bzr-pipeline plugin to bzr VCS ~15 years ago to do exactly this.

nirvdrum 3mo ago
Thanks for mentioning this. It seemed odd to me, too, so I spent some time trying to work it out. As a reviewer, I'm not sure how I'm supposed to assess database or API changes without knowing how they're intended to be used. And deploying them independently seems odd, too, especially if you need to roll it all back. I think in my ideal world there would be a clean history and I could review a PR commit-by-commit. But, you can't just approve a single commit, so there's a tooling problem there. And most CI runs on an entire push rather than individual commits. And increasingly I see devs using git as an offsite backup for whatever change they just made, rather than breaking commits up into logical chunks. In that workflow, squashed merges make the most sense. It's probably flawed, but the mental model I came up with is each stacked PR collapses into what would have been an individual commit in a clean PR, with the advantage of being able to be reviewed separately from the other changes and forced to clear CI. And then the whole stack becomes what would have been a clean PR in the old model. That, I can kinda see the benefit of. But, merging only part of the stack into trunk is the mental hurdle I can't clear; it'd be like merging only some commits from a PR. It kinda reminds me of when projects used CVS. I've really only seen stacked PRs used on projects where history is little more than an audit log. I'm keen to see how this gets employed by open source projects. I think there's a disconnect and it's likely I'm not going to really get it until I see it.
- ollysb 3mo ago
  This is my confusion, we already have commits to bundle changes, why not simply allow commits to be reviewed independently within a pr?
  - rendaw 3mo ago
    Doesn't this have the same issue? If you need database changes that also need query changes or api changes, then you need to modify multiple commits.
  - flexagoon 3mo ago
    This is similar to how things like Gerrit do code review, and it's pretty nice
  - eddythompson80 3mo ago
    Ok, then how do you approve/take the first 2 commits and not the last one? Or how do you insert a commit in between 2 commits or address feedback on a given commit? The person making the change is now going to have to run multiple confusing interactive rebases and git shenanigans, the rewrite the history of the PR branch on every feedback, then you have to re-review all the commits again because they are all different. It’s possible of course to push all that complexity on the tooling. Have GitHub and git provide tooling for doing all that within the context of a single branch/PR. But why is that better? Multiple branches are easier to manage in git, and as long as they don’t conflict on the merge. Obviously if a feedback on PR#1 causes a conflict in PR#2 which causes a conflict in PR#3 it’s still tedious, but it’s a lot more doable than managing interactive rebases on every feedback comment.
    - Kinrany 3mo ago
      Jujutsu makes all this easy
      - Degorath 3mo ago
        Jujutsu makes the author-side of it all fairly nice, indeed. But the reviewer-side has been so horribly abandoned by github that it has been frustrating me for a long while now.
radicality 3mo ago
At Facebook (i left >4 years ago) we used Phabricator where shipping stacked changes like this was table stakes. I still miss it. Some few random points, not really organized, i'm sure i'm forgetting bunch more benefits: * I often used it as a way to make reviews significantly easier for the reviewer. If I split something more complex into smaller units where the small change seems obviously correct, it's much more likely i'll get a faster accept than with a very large change. * Critically, once a unit of the stack has been approved, the approval will still stay with rebases of the whole stack. * You can keep building up a stacs and once you think you have enough for whatever you are doing, ship the whole stack. * Suppose you have a 'hole' in the reviews and you're missing a review for a middle part of the stack. You can still merge the bottom part up to where all are reviewed. * Let's assume you're the reviewer: smaller changes is much easier to review. Also, each part of the stack builds incrementally on the previous one. With your example, suppose you're reviewing 'api changes' part of the stack, but it has failing tests because something is broken with the lower 'db schema changes' change - but otherwise, the 'api changes' diff looks fine - you can then accept it and this part of the stack is already reviewed. The author can now whenever fix their bottom-most change (db schema change) to have tests passing, and get someone to review that (perhaps someone completely different more familiar with db), and then rebase the 'api changes' diff, and _it will not_ require review because you haven't changed anything, just updated the diff underneath it. In this way you parallelized your approvals. Right now at work, using github, I have to serialize my PRs and can't effectively get things reviewed until the previous PR is approved.
- necovek 3mo ago
  As I mentioned, this is similar to a workflow we used at Canonical ~15 years ago with bzr-pipeline plugin for bzr. Except that you tried not to divide it by technical aspect (DB changes vs API vs frontend) bit instead into smaller, semantic full features (we also used a 800/400 line diff limit which helped enforce it).
- alepar 3mo ago
  > it's much more likely i'll get a faster accept than with a very large change. I do agree the above is true. And maybe it's just me, but it feels like I'm kind of cheating if I stack PRs to get a faster approval. Because when one reviews some dangling code without the context of how it's going to be wired in, they can't really tell what the blast radius will be and consequently what bar they should hold the code against (e.g. performance, security, etc). In these circumstances, usually the most one can do is raise some stylistic nits (which to be fair PR process often degrades into anyway) and move on.

by necovek 3mo ago

nirvdrum 3mo ago

Thanks for mentioning this. It seemed odd to me, too, so I spent some time trying to work it out. As a reviewer, I'm not sure how I'm supposed to assess database or API changes without knowing how they're intended to be used. And deploying them independently seems odd, too, especially if you need to roll it all back. I think in my ideal world there would be a clean history and I could review a PR commit-by-commit. But, you can't just approve a single commit, so there's a tooling problem there. And most CI runs on an entire push rather than individual commits. And increasingly I see devs using git as an offsite backup for whatever change they just made, rather than breaking commits up into logical chunks. In that workflow, squashed merges make the most sense. It's probably flawed, but the mental model I came up with is each stacked PR collapses into what would have been an individual commit in a clean PR, with the advantage of being able to be reviewed separately from the other changes and forced to clear CI. And then the whole stack becomes what would have been a clean PR in the old model. That, I can kinda see the benefit of. But, merging only part of the stack into trunk is the mental hurdle I can't clear; it'd be like merging only some commits from a PR. It kinda reminds me of when projects used CVS. I've really only seen stacked PRs used on projects where history is little more than an audit log. I'm keen to see how this gets employed by open source projects. I think there's a disconnect and it's likely I'm not going to really get it until I see it.

ollysb 3mo ago
This is my confusion, we already have commits to bundle changes, why not simply allow commits to be reviewed independently within a pr?
- rendaw 3mo ago
  Doesn't this have the same issue? If you need database changes that also need query changes or api changes, then you need to modify multiple commits.
- flexagoon 3mo ago
  This is similar to how things like Gerrit do code review, and it's pretty nice
- eddythompson80 3mo ago
  Ok, then how do you approve/take the first 2 commits and not the last one? Or how do you insert a commit in between 2 commits or address feedback on a given commit? The person making the change is now going to have to run multiple confusing interactive rebases and git shenanigans, the rewrite the history of the PR branch on every feedback, then you have to re-review all the commits again because they are all different. It’s possible of course to push all that complexity on the tooling. Have GitHub and git provide tooling for doing all that within the context of a single branch/PR. But why is that better? Multiple branches are easier to manage in git, and as long as they don’t conflict on the merge. Obviously if a feedback on PR#1 causes a conflict in PR#2 which causes a conflict in PR#3 it’s still tedious, but it’s a lot more doable than managing interactive rebases on every feedback comment.
  - Kinrany 3mo ago
    Jujutsu makes all this easy
    - Degorath 3mo ago
      Jujutsu makes the author-side of it all fairly nice, indeed. But the reviewer-side has been so horribly abandoned by github that it has been frustrating me for a long while now.

radicality 3mo ago

At Facebook (i left >4 years ago) we used Phabricator where shipping stacked changes like this was table stakes. I still miss it. Some few random points, not really organized, i'm sure i'm forgetting bunch more benefits: * I often used it as a way to make reviews significantly easier for the reviewer. If I split something more complex into smaller units where the small change seems obviously correct, it's much more likely i'll get a faster accept than with a very large change. * Critically, once a unit of the stack has been approved, the approval will still stay with rebases of the whole stack. * You can keep building up a stacs and once you think you have enough for whatever you are doing, ship the whole stack. * Suppose you have a 'hole' in the reviews and you're missing a review for a middle part of the stack. You can still merge the bottom part up to where all are reviewed. * Let's assume you're the reviewer: smaller changes is much easier to review. Also, each part of the stack builds incrementally on the previous one. With your example, suppose you're reviewing 'api changes' part of the stack, but it has failing tests because something is broken with the lower 'db schema changes' change - but otherwise, the 'api changes' diff looks fine - you can then accept it and this part of the stack is already reviewed. The author can now whenever fix their bottom-most change (db schema change) to have tests passing, and get someone to review that (perhaps someone completely different more familiar with db), and then rebase the 'api changes' diff, and _it will not_ require review because you haven't changed anything, just updated the diff underneath it. In this way you parallelized your approvals. Right now at work, using github, I have to serialize my PRs and can't effectively get things reviewed until the previous PR is approved.

necovek 3mo ago
As I mentioned, this is similar to a workflow we used at Canonical ~15 years ago with bzr-pipeline plugin for bzr. Except that you tried not to divide it by technical aspect (DB changes vs API vs frontend) bit instead into smaller, semantic full features (we also used a 800/400 line diff limit which helped enforce it).
alepar 3mo ago
> it's much more likely i'll get a faster accept than with a very large change. I do agree the above is true. And maybe it's just me, but it feels like I'm kind of cheating if I stack PRs to get a faster approval. Because when one reviews some dangling code without the context of how it's going to be wired in, they can't really tell what the blast radius will be and consequently what bar they should hold the code against (e.g. performance, security, etc). In these circumstances, usually the most one can do is raise some stylistic nits (which to be fair PR process often degrades into anyway) and move on.