Stacked PRs – Pros and Cons?

[rupertdenton]

I've been doing a bit of reading about "stacked pull requests". The two major benefits that people seem to like are: it reduces the size of PRs which is a very good thing, it allows devs to keep working while things are in review (rather than waiting for it to be reviewed before continuing.

Have seen a bit of conversation on HN about stacked PRs, curious about people's experience with it? Pros and cons?

[rektide]

For any long work, I love having a draft PR going, and then making PR's into that. Its basically like stacked PRs but I don't need each PR to be shippable independently.

The advantage is so nice. You can make a bunch of finer-grained PRs that have really good descriptions for what happened, what's changing. And it builds a big PR that has the top-down view. It creates a very understandable hierarchy of understanding. And it shows work over time.

Edit: apologies... maybe this is also Stacked PRs? I think of Stacked PRs as a series of PRs into main. But I see posts like this which show PRs into PRs, which is what I'm describing above. https://benjamincongdon.me/blog/2022/07/17/In-Praise-of-Stac...

[rupertdenton]

Nice that sounds like a really neat workflow. I haven't used stacked PRs before, I feel like everywhere I've worked has generally been: work on a feature, PR/MR the branch and try to keep it small. Any idea what it isn't more common?

[matthew16550]

Tangentially related, sometimes I find Stacked Git helpful when figuring out complex features or refactoring. Until it's nearly finished I'm not sure what would be worthwhile submitting as a PR but once it's ready then several smaller PRs are much easier to understand.

It's local stacked PRs and you can jump between them to edit as the ideas evolve.

https://stacked-git.github.io/

But if the nearby code is evolving quickly from other people this can be a bad approach because of merge hell when the work is finally submitted.

[rupertdenton]

Yeah merge hell is what sorta keeps me shy of doing the stacked approach. Is this something you've encountered?

[travisb]

I love stacked PRs (though I prefer to call them "cascaded MRs" or similar because I find it easier to explain how they work to people that way).

You've covered the major pros at a high level, but I think the details matter. For example "smaller PR" is a rather high-level pro which is true. But it's not just that PRs are smaller, but that they can be ordered to build functionality as a story and justify each change. This is much like how the Linux Kernel wants patch series and not just a single big patch. I find this helps make reviewing easier and faster. It can also help clarify some people's thinking.

Similarly, "keep working" is a high level truth, but it has important organizational consequences. For example, if there is no rush to merge things it means the entire team doesn't need to be interrupted several times a day to merge something. It also mean it's OK to open a deep discussion on something in the MR. I find both these make the team as a whole more productive because they can take the time to think. For example, I push my team to only do reviews once a day so they don't need to context switch and it gives discussions time for well thought out replies instead of rushed single-sentence responses.

There are some cons which are by no means insurmountable, but aren't free to solve.

The biggest one is teaching everybody on the team how to use cascaded MRs in a technical sense. The details depend on how you implement them, but I prefer cascaded branches with full merge commits. Tooling can help, but if they don't understand the underlying mechanics there will undoubtedly be problems where people can't get the review to look like what they want.

Secondary to this is teaching what a good patch series looks like and how to use the VCS to create it. Each PR really needs to pass CI on its own, which requires ordering code changes in a particular way which is opposite the order they were mostly likely implemented. For example, while working on something you'll probably notice a bug or refactor which needs to be done, but you only notice it mid-way through the work. So you want to move the refactor/fix to before the main work in the series. There are different ways to do this with different trade-offs and developers need to understand how and when to use them.

Finally, the biggest problem I've seen with stacked PRs is difficulty bringing in changes from master or whatever the parent is. With a single PR this is an easy merge/rebase. With a long series of these it's a repetitive series of merges (my preference) or rebases. Doing this manually is annoying and writing automation for it requires formalizing branch (or whatever) parent-child relationships. This isn't terribly difficult, but ideally the command line and IDE and repository viewer and code review software all agree on it, which is a bunch of tedious work.

[rupertdenton]

This is a really interesting set of insights. I definitely see the theoretical benefits of stacking but as you indicate it feels like it requires a pretty good grip of git to manage. Do you have tooling to support with this in your team? To your last point, so let's say you've got 5 stacked PRs chained off of each other, you then need to merge master into each of them individually?

[travisb]

On a previous team we did create a wrapper command around git which did the parent-child branch tracking and added some convenience commands. On my current team we don't and it's OK because conflicting changes are rare so merging in from master is infrequently necessary and our code review tool can't make effective use of the extra stack information anyways.

Suppose you have the stacked PRs in the following sequence: A, B, C, D, E, F

If you implement stacked PRs using branches in git, then they will be a series of branches. For example, A will be a child branch of master, and B a child of A. To bring new changes in from master all the way down to F requires at least three commands and a build per stack member:

   git fetch
   git checkout A
   git merge master
   build_and_run_tests
   git checkout B
   git merge A
   build_and_run_tests
   git checkout C
   git merge B
   build_and_run_tests
   ...

Failure to do this for every PR in the stack tends to create confusing diffs in the code review system. For example, if you only merged the changes from master as far as C, then the code review system will probably try to show the diff between D and C:head which will contain the removal of a bunch of new master code. The code review really should be showing the diff between D and C:d_merge_base, but some code review tools don't support that and others need help, like the wrapper tool I mentioned previously.

[rupertdenton]

That's really interesting. So just so I understand the wrapper command automated those steps you've outlined? What were the convenience commands?

[travisb]

The wrapper was named "gut". I don't recall all the details so the names below are approximate. It wrapped the git command and added project-specific options (like --first-parent) to existing git commands, introduced new ones (like "gut check" which ran the build and tests), or hid native git command which were discouraged (like rebase).

"gut sync" merged in all the changes from the branch's parent into a single child branch, fetching if appropriate. We never got around to automating sequential syncing since the CI setup was weak in this project.

"gut create" created a new cascaded branch with the necessary tracking information from an arbitrary parent branch.

"gut review" posted the cascaded branch to the code review system so the diff would turn out correct.

"gut pdiff" would show the diff of the branch relative to the merge-base of the parent.

"gut reparent" would change the parent of a branch.

"gut insert" would insert a new cascaded branch between two existing branches -- useful for inserting refactors earlier in the series.

"gut merge" which merged the branch into the parent only once various checks, like review approval and passing tests and being at the head of the stack, were satisfied.