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

21–30 of 64 posts

Re: A Better Model for Stacked Pull Requests

#21

Post by jedimastert »

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.

Re: A Better Model for Stacked Pull Requests

#22

Post by bertr4nd »

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 (…

ghstack is amazing. When I started working on PyTorch in GitHub I desperately missed the stacked diff workflow of Phabricator, and ghstack basically made me whole again :-).

Re: A Better Model for Stacked Pull Requests

#23

Post by the_duke »

MereInterest wrote: post #14
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've never really understood the hate for a merge-based workflow for that exact reason. Having a true history is more important than having a clean history. (And "git log --first-parent" gives you a clean history, too.) Rebasing for a pull request means there is no way to tell that my local branch has been merged in, because the commit I made has disappeared into the aether.

I prefer a situational policy:

* Branch can be reduced to a single, coherent commit: squash + rebase

* Branch can be reduced to a single commit plus one or two unrelated fixup commits (like formatting): rebase

* Multiple commits: merge

But that requires value judgement and can lead to discussions, so it's not ideal for many teams.

Of course then there are also a lot of companies that use Jira as their primary source of history, where most commits are just "Fix ABC-123". For those it really doesn't matter much what you do to the Git history anywway.

Re: A Better Model for Stacked Pull Requests

#24

Post by barbazoo »

> 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!

Re: A Better Model for Stacked Pull Requests

#25

Post by calderwoodra »

There is an even simpler approach that doesn't involve cherry-pick or rebase.

Instead of doing this:

```

checkout f2

git rebase f1

push -fu origin f2

checkout f3

git rebase f2

push -fu origin f3

```

Do this instead:

```

git checkout f2

git merge f1

git push

git checkout f3

git merge f2

git push

```

You would follow this same approach if someone merged a PR ahead of yours into master:

```

git checkout master

git pull

git checkout f1

git merge master

git push

```

I stack PRs everyday because of the points mentioned in the article, but haven't found a tool which will automatically rollup merges yet (haven't looked either).

(edit: formatting)

Re: A Better Model for Stacked Pull Requests

#27

Post by smcameron »

cyphactor wrote: post #17

I highly suggest reading my article on How we should be using Git. It covers a Git Patch Stack workflow, where it originated from and the tooling we built around it. It has important ties to how the Linux Kernel and Git dev teams work as well as breaks down the benefits in relation to CI as a methodology. https://upte.ch/blog/how-we-should-be-using-git/

How does git-ps compare to stgit?

I've been using stgit for a very long time, and before that, I used quilt, and before that, Andrew Morton's patch scripts. If I am not mistaken, Andrew Morton's patch scripts were the inspiration for quilt.

* https://stacked-git.github.io/ * https://linux.die.net/man/1/quilt * https://lkml.org/lkml/2002/10/20/149

Re: A Better Model for Stacked Pull Requests

#28

Post by carterschonwald »

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 (…

I was wondering how your tool related to this article and low and behold I find you remarking on this very topic! :)

Re: A Better Model for Stacked Pull Requests

#29

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.

Horses for courses. These tools originated with linux kernel maintainers. I remember sending a patch to improve Andrew Morton's patch scripts, and he told me it was "slow". I was using a dumb algorithm for something, and so I asked him how many patches he had in his stack, and I don't remember the precise number, but I remember that it was in the thousands! My algorithm worked fine with 10 or 100 patches, but thousands? Hadn't occurred to me someone would be dealing with such a pile.

I haven't had a pile that big myself, but I have had a pile that was in the low 100s.

Re: A Better Model for Stacked Pull Requests

#30

Post by andrewstuart2 »

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 MRs in the order they're depending and you can easily have people understand the changes across them without wasting time solving merge conflicts you already knew you'd otherwise have to deal with.

Post reply on HN