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

In Praise of Stacked PRs

benjamincongdon.me

Post reply on HN
Print view View original

111–120 of 230 posts

Re: In Praise of Stacked PRs

#111

Post by vlovich123 »

wpietri wrote: post #75
is0tope wrote: post #8

I've usually kept a rule that you should avoid stacking, and if you must only one level deep. The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Stacking in my personal experience usually leads to merge conflict hell as changes and PR suggestions get merged underneath you.

For sure. One of the things I learned from the Lean folks was to look for inventory; it's one of the 7 Wastes. [1] In physical manufacturing, it's pretty obvious, because it's physical stuff sitting around on the journey to becoming actually useful. With software it can be harder to notice because you don't have to make room for it. But in essence it's the same deal; it's anything we have paid to create that isn't ye…

I have stacked diffs sometimes when the rework is large. I want to make sure that I know the full story sounds the change I’m making because I’m forced to think about that upfront. What refactoring was needed? Was it actually needed? What new path do I carve out in the code or how do features interplay? Broken tests with good coverage tell me if I made a foundational mistake. Even if I decide to throw away the work because it was a dead end (rare) the team will have learned something by me explaining what didn’t work out. More often, I’ll need to go back and clean up. But I save significant reviewer time by doing that before putting up random prs one at a time that are not well understood. With the exception of very simple work, stacked reviews generally save significant time. You get reviews of non objectionable prs. Coworkers can see a bigger picture if that’s helpful to understand the context of the change that’s still coming into shape. It actually reduces merge conflicts because, for example, you can enable a refactor that everyone agrees needs to be done and land that. Then your conflict space is smaller.

Small prs don’t need it of course but complex features benefit from shaking out things earlier. Commit more than 100 lines are really hard to review (lots of anecdotal and empirical research). If you’re not reviewing small commit by small commit, the reviews are easily missing things. A single PR that’s 800 lines adds review time to go commit by commit. If you can merge the non objectionable stuff, the reviewee gets to feel a some of forward progress and fewer merge conflicts (eg someone lands a refactor before some simple change of yours vs your simple change handed before and you made it the person refactoring their problem where it belongs)

Re: In Praise of Stacked PRs

#112

Post by morelisp »

arxanas wrote: post #100
morelisp wrote: post #41

Earlier quoted context omitted.

I can't be certain what dcow is referring to but there are a few things that point to confusion, or maybe an overambitious attempt at simplification. For example, Stacking PRs keeps the author unblocked. Authors don’t need to wait on a particular change to be merged before starting to build something on top of those changes. This will fundamentally be up to the author's git skill whether or not they are presenting th…

git-branchless author here. By "branchless", I mean literally using detached HEAD as the primary state of development. If you're using tools like Gerrit or Phabricator, you never have to explicitly make a branch to get your code merged. If using GitHub, then branches are unavoidable, but it can be nice to do branchless development as part of rapid prototyping (see https://github.com/arxanas/git-branchless/wiki/Workfl…

But when you call it "anonymous branching" you lay bare that the only advantage is that you don't need to name your work "branch", and meanwhile you have a workflow that's needlessly incompatible with most other git tooling.

In particular, since this is the one I see usually called out as a benefit of branchless:

Stock Git does not have good ways of rebasing a sequence of branches.

A sequence of branches can be rebased by rebasing (or otherwise rewriting) the longest one (the only one you'll need locally) then pushing the individual commits in the current branch to the remote under any relevant branch names. This doesn't take zero time, but with good git UIs it will take less time than remembering `git move`, and it's not especially hard to do with the stock CLI either.

Re: In Praise of Stacked PRs

#113

Post by loloquwowndueo »

Bzr has a pipelines plug-in which does basically this. Branches can be stacked on top of each other forming a “pipeline”, and propagating changes from one branch to the dependent / subsequent ones is achieved with one command, “bzr pump”.

Bzr has no concept of rebasing, so what you get is a simple merge, but with very little work on your part.

Re: In Praise of Stacked PRs

#114

Post by morelisp »

celtain wrote: post #76
computronus wrote: post #30

Earlier quoted context omitted.

First, think about how difficult, and time-consuming, it will be for others to digest and review 2500 new lines of code that sprung from someone else's mind. So you will end up waiting anyway, for even a small part of your work to be merged. The work of breaking up a big, inspired chunk of work into small pieces helps you learn more about it, and the perspective can reveal improvements that weren't obvious in the ini…

Isn't this exactly why you should use stacked PRs? That's what it looks like when I break a big piece of work into smaller pieces. What's the alternative anyway? If you don't want me writing 2500 lines of code in one area, would you rather I write 10 250 line PRs in 10 different parts of the codebase instead? Is that supposed to be easier to review? Or is the rule just "don't write a lot of code in any short period o…

> don't write a lot of code in any short period of time

Depending on the team this may be a completely reasonable policy.

Re: In Praise of Stacked PRs

#115

Post by isoos »

arxanas wrote: post #103
is0tope wrote: post #8

I've usually kept a rule that you should avoid stacking, and if you must only one level deep. The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Stacking in my personal experience usually leads to merge conflict hell as changes and PR suggestions get merged underneath you.

If you work at asynchronous/remote work company, i.e. your coworkers are in different timezones and can't review immediately, what else are you going to do? Put out exactly one code review per day until your feature is fully merged? Some things like refactoring changes can be reviewed and committed individually, but lots of feature work is fundamentally dependent on the previous work. Stacking PRs is like pipelining…

> what else are you going to do?

Depends on the team and the product. My personal approach is to have 2-3 larger things to work on, so while I wait for reviews on one, I can switch and work on the other. This usually means minimum 1-2 weeks of planned work, sometimes even more, without being blocked on reviews. If everything is blocked, then it is time for some code health cleanup, refactoring and fixing those TODOs that are just lingering around, and also nudging the reviewers...

Re: In Praise of Stacked PRs

#116

Post by somehnacct3757 »

I like to figure out what all my PRs will be during the planning phase, optimizing for reviewer cognitive load and incremental development of shippable features.

I never need to stack PRs cuz I can work on the next one while the first one is under review and rebase once it's merged. I can see if the review phase is long at your company that this wouldn't work. But I prefer it if possible.

One thing I hate about stacked PR delivery is ppl go dark for a month building this whole new world and if you have architecture concerns in the root PR they will resist them because it means rebasing and changing all the downstream PRs as well. Bad incentives all around.

Re: In Praise of Stacked PRs

#117

Post by timhh »

davvid wrote: post #109

There is active work related to teaching "git rebase" to natively support stacked branches in the Git core currently being worked on by Derrick Stolee [1]. If you "stack" your changes across multiple inter-dependent branches it looks like "git rebase" is going to learn how to update related branches using a new "update-ref" command (alongside "squash", "fixup", "exec", etc) that gets activated automatically through a…

That sounds great! I have partly solved this issue in my autorebase tool (https://github.com/Timmmm/autorebase) - it basically rebases every branch, and fixes the commit time so that stacked branches get preserved even after a rebase just because the hashes all match properly.

That obviously doesn't work if you modify or drop any of the commits, so this option is very welcome!

Re: In Praise of Stacked PRs

#118

Post by tunesmith »

How would you align this with Jira stories? Our team tries to maintain a one-PR-per-story flow. Partly for QA purposes, we don't want multiple QA cycles per story.

Of course, we still have giant PRs that touch 50-100 files and take forever to code review.

And yet, for those stories in question, they do make it to prod faster than if they were broken apart into multiple 1-2 point stories.

I imagine as always the answer is "invest in better tooling". More robust automated tests, push-button millisecond-deployments, etc...

Re: In Praise of Stacked PRs

#119

Post by arxanas »

morelisp wrote: post #112
arxanas wrote: post #100

Earlier quoted context omitted.

git-branchless author here. By "branchless", I mean literally using detached HEAD as the primary state of development. If you're using tools like Gerrit or Phabricator, you never have to explicitly make a branch to get your code merged. If using GitHub, then branches are unavoidable, but it can be nice to do branchless development as part of rapid prototyping (see https://github.com/arxanas/git-branchless/wiki/Workfl…

But when you call it "anonymous branching" you lay bare that the only advantage is that you don't need to name your work "branch", and meanwhile you have a workflow that's needlessly incompatible with most other git tooling. In particular, since this is the one I see usually called out as a benefit of branchless: Stock Git does not have good ways of rebasing a sequence of branches. A sequence of branches can be rebas…

> But when you call it "anonymous branching" you lay bare that the only advantage is that you don't need to name your work branch

Sure, I'm only responding to what you were saying about "There is no such thing; considering one special branch as 'not a branch'". There is such a thing in that there is no branch involved in the detached HEAD state. It's not some kind of Git misunderstanding. I think you might be referring to trunk-based development and always building off of the remote main branch instead of having your own local copy, which is unrelated to being "branchless", for the reasons you stated.

For many people (particularly those on Github!), a branchless workflow won't help, so you're free to not use it. In my opinion, it's a workflow that is better compatible than stock Git with code review tools like Gerrit and Phabricator.

I personally argue that anonymous branching is useful even in some branch-based workflows. Mainly, if Git branches are so lightweight to use, why do we also use the command `git stash`, instead of just always creating a new branch for our temporary work? One benefit of anonymous branching is that it consolidates these workflows in a convenient way. Some people don't stash changes or feel that branching in those cases is heavyweight, so anonymous branching doesn't help them at all.

> then pushing the individual commits in the current branch to the remote under any relevant branch names

If I'm understanding correctly, every time you rebase the longest branch, for each commit in the branch, you would manually run e.g. `git push origin:my-branch-name`? That seems like it would take a lot of time to me. Is the tacit assumption here that you don't have a lot of commits in your branch, so this doesn't take a lot of time?

Re: In Praise of Stacked PRs

#120

Post by baq »

dreamdeath wrote: post #40
Supermancho wrote: post #37

Earlier quoted context omitted.

> First, think about how difficult, and time-consuming, it will be for others to digest and review 2500 new lines of code that sprung from someone else's mind. There's a tradeoff to be made. Have a feature sooner or later. Review now quickly and more carefully later, or review carefully now. Part of what development teams do, is risk assessment. Put a feature flag on it, do a demo of the branch. If it looks good, do…

> First, think about how difficult, and time-consuming, it will be for others to digest and review 2500 new lines of code that sprung from someone else's mind. I haven’t ever worked at big corp so maybe this kind of thinking is actually valuable there. But in most startups in my experience this mindset is wrong. You literally won’t have a job tomorrow (because your company will fold) if you don’t ship value-generatin…

If you're the only developer for quite a while... otherwise reviewability is a thing PRs must be optimized for or your super-duper-important feature won't get in.

Either way you should be looking for a new job, because what you're describing is quite unsustainable in a team of more than one person working on the same thing.

Post reply on HN