Live data from Hacker News

In Praise of Stacked PRs

benjamincongdon.me

141–150 of 230 posts

Re: In Praise of Stacked PRs

#141

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…

But if you're expecting another dev to always be available to help you with a review, then they're the ones having to "context-switch", interrupting their own work. The reality is context-switching is something that comes with the territory if you're working as part of a team developing software. Which isn't to say there aren't opportunities to minimise the disruption it causes, but the idea that you can more or less…

I think maybe we're using different definitions of the term "context-switch". Certainly it's an interruption, but I don't really think that sitting down with another engineer to do a focused code review where they've already written out a PR description and thought hard about the problem is comparable to starting a brand new branch or picking it up after a while away and trying to juggle 2, 3, or 4 in-progress tickets.

Re: In Praise of Stacked PRs

#142

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. 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 ab…

This isn't going to get me internet points, but after a few decades, I like to get my experience down.

> Well, I'm proceeding from the assumption that thoughtful review, which takes time, is desirable

Ofc it's desirable. Nobody is arguing this.

> If the situation is really that dire, then it's even more important that you ship product that works.

That is simply incorrect. Broken features (in innumerable shades) are debuted all the time to secure future investment in various ways which are not just financial. Sometimes it's community confidence, sometimes it's leadership clout, etc. Betas are a thing. This fear-mongering about 2500 lines of code is pearl clutching that hobbles large organizations that either fail to deliver or fail to deliver stability, regardless of their processes that drags on development for 4x or 10x, which they have misplaced confidence in.

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

There is lots of useless material about it. The number one rule is "don't interrupt the flow of money" but that rule doesn't extend to every project forever. Large organizations get less and less efficient over time (see rule 1) because perfect is the enemy of good. It's a good tradeoff, when you have an organization that can't keep up with how much money they are making to start piling on additional process because you don't know where your weaknesses are so anything that goes wrong gets an additional check. Most orgs are not in this boat.

If you don't look like you're making your money's worth, even if you can explain how some side-process adds value, someone starts looking for your replacement and eventually they will find one and you're gone. That's true, regardless if you work at some mom and pop shop or Amazon. You're always one day away from unemployment, even if you don't know what day the chain of events started. So code reviews that block features can hurt you and have put a lot of people out of work, I can attest. Ofc we're talking about more and more extreme situations, but the idea is the same. Code reviews are a scale and that scale has a tradeoff.

The gut-check is when a company has a merger/acquisition deal. The team doesn't have a lot of time to vet the other company's codebase. You do your best to have your info meetings, try to run parts of the code with data, make your recommendations, and the escrow eventually closes. 2500? How do you think you'll do when looking at millions of lines? At some point, you will be forced into black box testing and realize that this is what matters foremost. Look at the interface, look at the expectations, look at the data. After that, the rest is gravy that can be addressed later.

Re: In Praise of Stacked PRs

#143
post #128

We have some people in our company that use tools like Graphite to implement this pattern on top of Github and I don't know if I'm a big fan. Maybe just a matter of developer discipline, but in my experience people tend to create large stacks of 3+ PRs that then take a while to resolve. Yeah sure, without these tools the code would also exist somewhere, but at least you don't have your pull request list full with PRs…

> Probably some arcane git magic to (interactively) rebase branch There is not really a command for that yet, short of adding a bunch of `exec` steps to your interactive rebase manually. See https://news.ycombinator.com/item?id=32217204 for an upcoming command. You might enjoy using https://github.com/gitext-rs/git-stack , which specifically tries to let you manage stacked branches locally while not exposing tons of…

Also a big fan of https://gitlab.com/wavexx/git-assembler (and previously topgit). Works with the basic idea that you can rebuild branches by combining, merging or rebasing automatically on top of others. I frequently use this to build a local branch which is an amalgamation of the main branch + required patches so that I can work (and later submit) on a clean branch without being blocked.

Re: In Praise of Stacked PRs

#144

Earlier quoted context omitted.

One advantage of ‘stacking’ is breaking up review into more logical units, eg 1. Introduce new test exhibiting bug 2. Introduce bug fix and update the test If 1 and 2 are reviewed together you have less evidence that the test actually shows the bug being fixed.

But once you merge 1, your test suite is broken, and many/most companies wouldn't allow that.

Yeah there’s two solutions:

1. Merge #2 into #1 then merge the result, but this can obscure your history

2. Don’t make the test fail. Eg expect the incorrect behaviour and leave a comment explaining why it is incorrect and what the correct thing should be.

Re: In Praise of Stacked PRs

#145
post #86

Earlier quoted context omitted.

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

I have a couple anecdotes to cover your question. All of them have something in common: startup needs money so you demo to potential customers or investors. Unlike in a stable corporate environment where deadlines can have flex, you really don't want to cancel or postpone a meeting to sell to a client - so those demos dates are set in stone, and if things aren't ready you will need to pull some heroics. One memorable…

Thanks for the anecdotes!

Re: In Praise of Stacked PRs

#146
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 best way I’ve found is to use atomic commits [0], or put simply have each commit do a single logical thing.

For example don’t mix formatting changes and logic changes in a single commit, or don’t fix two separate things and lump them together in the same commit.

It takes a bit of practice to figure out what constitutes a logical thing - you don’t want them too small otherwise it results in lot of noise, and you don’t want them too large otherwise it defeats the main purpose of using them.

A good rule of thumb is to avoid words like ‘and’ in your commit subject line e.g. if your commit message subject contains wording like ‘Do this thing and that thing’, you’d probably be better off having two commits ‘Do this thing’ + ‘Do that thing’.

This doesn’t fix the problem entirely, but I’ve found that using atomic commits greatly reduces merge conflicts when rebasing, and when there are conflicts they are usually easier to manage and don’t result in as many follow on merge conflicts.

0: https://www.aleksandrhovhannisyan.com/blog/atomic-git-commit...

Re: In Praise of Stacked PRs

#147
post #115

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…

The advantage of using stacks, though, is that you can keep focus on what you're working on, or even still work on 2-3 things simultaneously but work on multiple dependent pieces of each at once. Maybe I'm biased because I've used this workflow my whole career, but I think everyone can benefit from trying out a stacked PR workflow.

Not to mentioned getting reviews for smaller, atomic changes is just SO much easier. Even on a team where everyone is using a stacked workflow, if anyone submits a larger PR (especially more than a few hundred lines), you can see how the smaller PRs submitted in the same time, often in the same stack, get reviewed much faster.

Re: In Praise of Stacked PRs

#148

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…

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

But you can still construct stacked PRs out of the 2500-line change even you write it all at once -- just go back and split the changes into a series of PRs that makes sense. Much easier for your reviewers to get through, so it'll be able to ship faster.

Re: In Praise of Stacked PRs

#149
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…

This would be such a great addition! It doesn’t get stacked diffs to quite native support within Git, but it’d be a very good step in that direction

Re: In Praise of Stacked PRs

#150
post #3

We switched to using https://graphite.dev/ after yet-another huge epic that caused a mess of PRs and one PR that touched 100+ files and included dozens of semi-related changed. It's been a blessing so far.

How is Graphite? Does it solve the problem of stacked diffs well? I loved the CR tool at Amazon where I could create CRs for arbitrary commit ranges, and I really want to find a similar tool.
Post reply on HN