Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

A Better Model for Stacked Pull Requests

timothyandrew.dev

Post reply on HN
Print view View original

31–40 of 64 posts

Re: A Better Model for Stacked Pull Requests

#31

Post by aledalgrande »

andrewstuart2 wrote: post #30
aledalgrande wrote: post #20

I don't like this workflow, I see it as needlessly complicated. If you really can't cherry pick your big PR into simple branches to merge directly into master (which I rarely have seen over the years), create a longer lived branch and make small PRs to that. Then merge longer lived branch into master. You should never be going off and work on huge changes without keeping in sync with the base branch. Stacked PRs make…

Depends on the team you're on. "Small PRs" to some people could mean a few lines. But if those few lines are required for the next thing I do, on the same day, and I don't want to mess around with refactors or merge conflicts if somebody merges out of order, and I'm not going to get enough eyes on the PR quickly enough, this is exactly the strategy I've used before, minus the tools. Just stack up some very very small…

In that case I just use git stash. Stay on the branch you need and keep working, stash when needing to make changes to the open PR.

Re: A Better Model for Stacked Pull Requests

#32

Post by smcameron »

jedimastert wrote: post #21
aledalgrande wrote: post #20

I don't like this workflow, I see it as needlessly complicated. If you really can't cherry pick your big PR into simple branches to merge directly into master (which I rarely have seen over the years), create a longer lived branch and make small PRs to that. Then merge longer lived branch into master. You should never be going off and work on huge changes without keeping in sync with the base branch. Stacked PRs make…

I agree. I will regularly have a patch that lives for more than a month and I am rarely more than a couple days out of date. If the area you're working on is rarely touched, it isn't an issue. If you're constantly having merge conflicts, you might need to sync up with the team a bit more.

The other use case that a stack of patches workflow really fits is for maintainers of open source linux drivers (what I used to do). In that case, you're maintaining the driver in the upstream kernel, but you also need to get your code into Redhat's various kernels, SuSE's, Ubuntu's, and whoever else is the flavor of the day. All those guys have several kernels they support and they're all a little bit different. Porting over a pile of small patches is really the only sane way to go for that job, and for that, these kind of tools are a godsend.

Re: A Better Model for Stacked Pull Requests

#33

Post by jsw »

bob1029 wrote: post #3

I've considered building systems that would speculatively run builds using forward projections of all pending pull requests, as well as run multiple parallel scenarios on merge order + rebase such that merge conflicts are minimized. Maybe sprinkle in some monte carlo if the numbers get a little intense. In practice, I feel it is much easier to assign work and organize your codebase so that logically-different busines…

Mergify just introduced a “speculative merge queues[1]” feature that does something very similar.

[1] https://blog.mergify.io/announcing-speculative-merge-queues

Re: A Better Model for Stacked Pull Requests

#34

Post by house9-2 »

aledalgrande wrote: post #20

I don't like this workflow, I see it as needlessly complicated. If you really can't cherry pick your big PR into simple branches to merge directly into master (which I rarely have seen over the years), create a longer lived branch and make small PRs to that. Then merge longer lived branch into master. You should never be going off and work on huge changes without keeping in sync with the base branch. Stacked PRs make…

> create a longer lived branch and make small PRs to that.

I follow this pattern a lot, we call it an epic branch and usually name it something like `epic/foobar`

Re: A Better Model for Stacked Pull Requests

#35

Post by ilammy »

HALtheWise wrote: post #11

The article claims that GitHub doesn't support this workflow, but it actually does a pretty good job natively. In particular, if PR2 targets PR1, merging PR1 into master will automatically change the base branch of PR2 to be master, and PR2 will then be able to merge cleanly. I suspect this might not work as cleanly if you rebase or squash as the merge method, since you're destroying the historical information that g…

> I suspect this might not work as cleanly if you rebase or squash as the merge method

It does not. After PR1 is merged, PR2 gets retargeted onto the target branch of PR1—but all the commits it was based on now move to the PR2, sometimes causing conflicts. GitHub won't automagically rebase your PR2, that makes sense.

Re: A Better Model for Stacked Pull Requests

#36

Post by kqr »

house9-2 wrote: post #34
aledalgrande wrote: post #20

I don't like this workflow, I see it as needlessly complicated. If you really can't cherry pick your big PR into simple branches to merge directly into master (which I rarely have seen over the years), create a longer lived branch and make small PRs to that. Then merge longer lived branch into master. You should never be going off and work on huge changes without keeping in sync with the base branch. Stacked PRs make…

> create a longer lived branch and make small PRs to that. I follow this pattern a lot, we call it an epic branch and usually name it something like `epic/foobar`

Yeah, that works as long as only one person does this, and they regularly merge into and out of master. Otherwise you still get all the communication and feedback problems long lived branches have.

Re: A Better Model for Stacked Pull Requests

#37

Post by tta »

ezyang wrote: post #12

While working on PyTorch, I also wrote an equivalent tool (funnily named nearly the same thing) for doing stack diffs ( https://github.com/ezyang/ghstack/ ), which most of our team uses for more complicated PRs. The UX for working on commits is a bit different than this tool though; instead of pushing branches individually, you just run "ghstack" on a stack of commits and it will create a PR per commit in the chain (…

This sounds pretty great, I wish I'd seen this a few months ago! One quick question: does this support adding commits to open PRs, or are you effectively locked in to one-commit-per-PR?

Re: A Better Model for Stacked Pull Requests

#38

Post by tta »

barbazoo wrote: post #24

> Changes that build on the original either go in the same PR, or have to wait until the PR is merged so a second PR can be created. For feature branches feature-2 that's based on feature-1, why not create a PR from feature-2 onto feature-1 to get the review started? Once feature-1 is merged, simply rebase feature-2 and update the PR. Pipeline that shit, baby!

This is exactly right! The quoted section assumes stacked PRs aren't an option, as a way to introduce the problems that the stacking workflow addresses.

Re: A Better Model for Stacked Pull Requests

#39

Post by tta »

jasonpeacock wrote: post #2

I'm still reading the article, but it looks like there's a typo where it refers to: Merge in the stack from top to bottom - in this case we're going to merge Feature 3's PR, Feature 2's PR, and Feature 1's PR, in that order, using GitHub's UI. But the "top to bottom" would be merging F1, then F2, then F3 (and you'd have to rebase & re-upstream each as you go).

This is good feedback, thank you! I intend "top" to mean "the PR furthest from `master`" and "bottom" to mean "the PR closest to `master`". I'll add a section clarifying this nomenclature.

Re: A Better Model for Stacked Pull Requests

#40

Post by tta »

aledalgrande wrote: post #20

I don't like this workflow, I see it as needlessly complicated. If you really can't cherry pick your big PR into simple branches to merge directly into master (which I rarely have seen over the years), create a longer lived branch and make small PRs to that. Then merge longer lived branch into master. You should never be going off and work on huge changes without keeping in sync with the base branch. Stacked PRs make…

I have a couple of comments here.

> You should never be going off and work on huge changes without keeping in sync with the base branch.

I agree, but the stacked PR approach doesn't preclude this. If you periodically `autorebase` the stack against `origin/master` right after a `git fetch origin`, your stack stays in sync with `master`.

> ate a longer lived branch and make small PRs to that. Then merge longer lived branch into master

Large features don't necessarily imply long-running feature branches, though. I personally prefer feature flags (or some other mechanism for conditional execution) on the base branch for features that are "open" for more than a week or so, say.

"Large" is also subjective. You could have changes that are large in terms of lines of code added, the number of services that are being touched, or the number of different+unrelated concerns involved.

As an example, this week I worked on a semi-greenfield Kafka service. Stacking PRs helped me keep track of the review process a lot more easily than "one big PR" would allow, and the stack was merged in 4-ish days. I structured the stack like this:

  - Add a `proto` file covering messages that this Kafka consumer reads off the incoming topic
  - Pull in a gradle library for protobuf code generation
  - Add in a simple Kafka consumer and producer
  - Add a `Processor` that works on incoming messages
  - Hook everything up; the sevice consumes off Kafka, "processes" the data, and produces back to Kafka
These PRs aren't similar in terms of size or complexity, but the various concerns that comprise this service are nicely split up.

> Stacked PRs make all this more difficult.

I'm in agreement. Stacked PRs involve more overhead, but there are times when this is a reasonable tradeoff.

Post reply on HN