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

61–70 of 230 posts

Re: In Praise of Stacked PRs

#61

Post by zackmorris »

compsciphd wrote: post #9

One thing I wish git did (maybe it does and I don't know how?) is to be able to say that a new branch is based off an old branch (not a commit that used to be that branches head). so I can branch a single pr in progress to start the next. Then if I change the base pr in progress (say via rebase or via squashing or the like), I can easily rebase my new commits in the new pr on top of the current state of the branch. C…

Came here to say the same thing. I always rebase my own branch onto the trunk and force-push before merging. But that's hard to explain to other developers, and there's no way to enforce it, so the repo inevitably becomes filled with overlapping merges, which is an unforced error IMHO. Does anyone know if there's a way to only allow rebase-and-merge on GitHub and GitLab?

https://docs.github.com/en/pull-requests/collaborating-with-...

https://docs.gitlab.com/ee/topics/git/git_rebase.html#rebase...

Re: In Praise of Stacked PRs

#62

Post by avl999 »

krooj wrote: post #60

I've tried this method of taking a large change and breaking it up into smaller, more easily digestible PRs, and it didn't work. Unless you're working with folks that are directly invested in the change, you'll very quickly discover that people have memories worse than goldfish. So, it's a lose-lose situation: can't really do stacked PRs and large PRs will let bugs and other defects through.

There is a tradeoff... smaller, digestible PRs result in thorougher and faster reviews but it gives up the "1000 feet view" of the larger feature as far as the reviewer is concerned.

Large PRs preserve the larger "1000 feet view" of what you are working on but are likely to be slower to get responses on and most likely less thorough thus a larger chance of things being missed.

Almost everyone I've worked with prefers smaller review so I just accept that trade-off of 1

Re: In Praise of Stacked PRs

#64

Post by culi »

krooj wrote: post #60

I've tried this method of taking a large change and breaking it up into smaller, more easily digestible PRs, and it didn't work. Unless you're working with folks that are directly invested in the change, you'll very quickly discover that people have memories worse than goldfish. So, it's a lose-lose situation: can't really do stacked PRs and large PRs will let bugs and other defects through.

Can't this easily be resolved by just using feature branches? Make your small PRs into the feature branch but then you still have the ability to compare the branch to main as a whole and get that wider context

Re: In Praise of Stacked PRs

#65

Post by avl999 »

nateroling wrote: post #5

I’ve played with this idea a bit. In my experience it felt like a hack to work around the fact that individual PRs took a while to get merged. An alternative solve is to work in a way that allows PRs to be merged more quickly, ie pairing, mobbing, or prioritizing getting reviews done asap.

It is more about being unblocked than however long it takes for the PR to be merged in even if it is really fast. It is not always possible to pair/mod on a change.

Getting folks to quickly review PRs is difficult as doing so breaks the flow state of other people and results in constant thrashing and context switching between the work they are doing are reviewing PRs.

Even if the team is committed to quickly reviewing PRs, you are always going to have times when a bunch of people are in a meeting, while a couple are at lunch and a couple are knee deep in another issue and no one can look at your PRs for hours and you need to keep yourself unblocked.

Re: In Praise of Stacked PRs

#66

Post by nemetroid »

chrisweekly wrote: post #32
dtech wrote: post #26

Earlier quoted context omitted.

You can still have them in 2 commits, and configure your CI to build both of them, and the first 1 should fail. We actually have a rule that there must be 1 commit just introducing a test that fails on CI for bugfix PRs.

That's interesting, explicitly to require that bugs be reproduced in CI. It makes sense in theory, but in praxis (IME) CI systems tend to be overtaxed / underprovisioned - meaning this extra burden might be questionable. /$.02

The same burden is present when splitting the two commits into two PRs.

Re: In Praise of Stacked PRs

#67

Post by computronus »

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…

Well, I'm proceeding from the assumption that thoughtful review, which takes time, is desirable. If the situation is really that dire, then it's even more important that you ship product that works. Code review happens to be one of the, if not the most, effective ways to catch bugs and prevent disasters. It's a good idea to make the review process work for you, even and especially when the pressure is on.

It's not about humongous reviews being "inconvenient". A drop of thousands of lines of new code takes a long time to review thoroughly, whether the company is at risk of folding tomorrow or a stodgy enterprise with decades ahead of it. If you have to constrain review time - or ditch reviews altogether, why not? - you can, but there are consequences regardless.

I haven't worked in early startups, but I haven't only worked at large corporations either. I hope that most of us, most of the time, aren't less than a business day away from unemployment, and so there's usually time for code reviews. (If not, there's a lot of useless material about it!)

Without sarcasm, and potentially getting off-topic, I would like to hear stories about how a startup survived impending doom with heroic, fast shipping of product that set aside a lot of time-consuming processes. What were the most crucial steps to keep? How was the technical debt repaid?

Re: In Praise of Stacked PRs

#68

Post by NateEag »

zeroonetwothree wrote: post #50
pizza234 wrote: post #16

Earlier quoted context omitted.

I understand that (experienced the same "problem" today), but writing "2500 new lines of code" on a Sunday afternoon is (hopefully) not representative of regular workplace conditions.

Some people enjoy their work. I think it’s fine as long as it’s a choice.

It may be fine.

Keep in mind, though, that humans are not perfect and some choices are unwise.

Re: In Praise of Stacked PRs

#69

Post by jackblemming »

There's so many articles on trivial things like this and using pretty syntax and less articles on things that actual matter: architecture and design. It's a bit disheartening to hear software engineers argue of if you should put periods in commit titles or if so and so syntax is more readable.

Re: In Praise of Stacked PRs

#70

Post by tablespoon »

kqr wrote: post #49
ignormies wrote: post #17

Earlier quoted context omitted.

Something I often use stacked diffs for is deprecation -> removal flows. 1. Deprecate old feature + add opt-in support for replacement 2. Make replacement default with opt-out for old pattern 3. Completely remove old feature and the opt-out functionality I can write the entire stack of diffs upfront, have them individually reviewed but still linked, and ensure they're merged in the correct order. The bottleneck for m…

But is it really effective use of your time to make changes to the code that you know won't be relevant for weeks or months? You could spend the same time on other changes that would start earning you money tomorrow, instead.

> But is it really effective use of your time to make changes to the code that you know won't be relevant for weeks or months?

It totally is, because it doesn't wastefully discard the mental context needed to make the follow on changes. Task switching unnecessarily incurs significant costs.

> You could spend the same time on other changes that would start earning you money tomorrow, instead.

Maybe in some bare-bones startup context that can't afford to think beyond next week, but most organizations aren't like that.

Post reply on HN