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

161–170 of 230 posts

Re: In Praise of Stacked PRs

#161

Post by Aeolun »

morelisp wrote: post #43
djur wrote: post #39

Earlier quoted context omitted.

This isn't "how to use a hammer", though, it's more like "how to evaluate the completion of work", which is definitely something working teams have to agree on. git itself is almost irrelevant to the discussion, other than its native feature set having some influence on the options.

If this was just about team review workflow I wouldn't expect to find "easier to rollback" and "don’t need to wait on a particular change to be merged" to be on the list of upsides, as these are properties of the repository structure and not the review workflow.

> "easier to rollback" and "don’t need to wait on a particular change to be merged" to be on the list of upsides

Can you explain how these are not just facts? We’re not using stacked branches because it has enough downsides, but these are indubitably upsides of stacked branches.

Re: In Praise of Stacked PRs

#162

Post by nightpool »

daniel-grigg wrote: post #155
nightpool wrote: post #121

Earlier quoted context omitted.

> and doesn't necessarily help if there's a need to go away and make significant changes based on the outcomes of discussions. See, I disagree, because this is absolutely the place where it helps the most—you can now go away and keep working on the same task without having to context switch to anything else or remember where you were or what you were working on. So you're never in a state where you're blocked and can…

This sounds like you’re just externalising the costs of integration of your changes on to other people. Generally we want to reduce any accidentally complexity and the way to achieve that is to reduce the latency of code reaching production. So for example you want to minimise PR/branches in flight ideally to 0. So you should be asking how you can reduce them - eg trunk based development, omitting asynchronous code r…

> This sounds like you’re just externalising the costs of integration of your changes on to other people.

Huh? Nothing I said has anything to do with "the cost of integrating changes". I'm saying that it's more efficient for developers to work on one single task at once, instead of trying to "multi-task" among a variety of unrelated changes, and that synchronous code reviews help with this because they minimize the amount of time a developer spends blocked waiting for feedback from the rest of the team. Certainly this also has the benefit that you get fewer merge conflicts and have fewer branches in flight, but I don't know why you're saying that synchronous code reviews are "just externalizing the costs of integration [...] on to other people". That doesn't make any sense.

Re: In Praise of Stacked PRs

#163

Post by avl999 »

arxanas wrote: post #98
avl999 wrote: post #46

I do stacked PRs at work, they work great until someone suggests an invasive change in a PR lower down in the stack. Does anyone have ideas on how to deal with merge conflicts in this type of flow? For example let's say I have the following stack of PRs PR1 -> PR2 -> PR3 -> PR4 The reviewer reviews and suggests a change in PR1, this change causes a merge conflict in PR2 and therefore in PR3 and PR4 as well. And then…

The linked tool git-branchless handles this pretty well ( https://github.com/arxanas/git-branchless , I'm the author). You basically run `git checkout `, `git commit --amend`, and then `git restack`. This will rebase all dependent branches. As long as you're not using merge commits, you won't have to resolve the same conflict more than once. (It will also warn you up front whether or not merge conflicts will need to…

Thanks this looks interesting, I will definitely give it a shot.

Re: In Praise of Stacked PRs

#164

Post by dodobirdlord »

dimal wrote: post #156
TazeTSchnitzel wrote: post #82

Stacked reviews is a very natural pattern if you're using a system like Gerrit, where the unit of review is not a whole branch, but individual commits. Since they are commits, the reviews can have any kind of relationship that commits can have. I think it's a great system, including for some other reasons (it encourages amending and rebasing commits during the review process, which results in a very clean git history…

How is reviewing individual commits a good thing? Maybe I’m misunderstanding something, but my branches have tons of micro commits that ultimately lead to a larger change. Reviewing each individual commit in that type of branch seems like it would be very tedious, and not useful. The reviewer would be forced to follow my entire process, with all the dead ends and refactors along the way. Seems like it would take a lo…

Squash your commits once you've finished prior to sending them for review? Code review of individual commits generally stems from a workflow that expects that each individual commit will contain a change that is complete in and of itself, along with the corresponding tests for that change, so that the project builds and all tests pass at every commit. This makes exploring the change history easier and reverting changes cleaner, and simplifies bisecting the commit history.

Re: In Praise of Stacked PRs

#165

Post by djur »

morelisp wrote: post #41
johnfn wrote: post #35

Earlier quoted context omitted.

I'm not understanding how stacking PRs is inconsistent with knowing git well.

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…

> This will fundamentally be up to the author's git skill whether or not they are presenting the PRs to reviewers / mergers as stacked. If they're skilled git users there's little to no cost presenting them one at a time and keeping the not-yet-PRd branches fresh.

On multiple teams I've worked on, this has been explicitly discouraged because reviewers often want to see the changeset in as much context as is available, including future changesets.

Re: In Praise of Stacked PRs

#166

Post by presentation »

shepherdjerred wrote: post #151
tomasreimers wrote: post #106

I am one of the authors of the tool Graphite ( https://graphite.dev ). We built Graphite because we missed the stack diff workflow we knew from previous job experiences at larger companies, happy to answer any questions (and if you would rather not ask in public, feel free to email me at tomas at graphite.dev).

How much team buy-in does this require? Would everyone on my team need to use Graphite, or could they continue to use GitHubs code review if they prefer?

I tried it, requires no buy in, pretty much everything still properly shows up in GitHub.

Re: In Praise of Stacked PRs

#167

Post by sidlls »

Stacked PRs are an indicator of an incompletely thought out approach to the implementation. Prefer sequential PRs that layer in the implementation: test code, interfaces/API stubs, and then concrete implementations of the stubs (for example). This tends to make PRs less complex and more inviting for colleagues to review, which in my experience tends to make them go much faster. It also leads to generally better code quality.

Re: In Praise of Stacked PRs

#168

Post by pluiz »

isoos wrote: post #115
arxanas wrote: post #103

Earlier quoted context omitted.

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's the approach that I've been doing for a while now as well, I feel way more productive when a switch things that I'm working on and then come back later, and it also helps to keep it up with the time that my team-mates are available to review my changes...

Re: In Praise of Stacked PRs

#169

Post by raydiatian »

Here’s a (perhaps) interesting question: has anybody heard of a tool that, given a diff on some state in your codebase, will automatically produce a well organized chain of commits, and/or PRs? As in, “ok tool, here are my files and everything I’ve changed, commit them so that I still have coworkers that think I care about them.”

My naive sense is that the knowledge should exist to organize arbitrary code changes in to “good”, “readable”, segments.

I mean hell, that’s a whole business right there

Re: In Praise of Stacked PRs

#170

Post by hamandcheese »

sidlls wrote: post #167

Stacked PRs are an indicator of an incompletely thought out approach to the implementation. Prefer sequential PRs that layer in the implementation: test code, interfaces/API stubs, and then concrete implementations of the stubs (for example). This tends to make PRs less complex and more inviting for colleagues to review, which in my experience tends to make them go much faster. It also leads to generally better code…

But how can you write the concrete implementation without stacking?

The only thing stacked PRs indicate, IMO, is that your coworkers are slow to review your code.

And in my experience, when folks send out stacked PRs, it’s because they put much _more_ thought and effort into identifying the right boundaries, not less.

Post reply on HN