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…
In Praise of Stacked PRs
141–150 of 230 posts
Re: In Praise of Stacked PRs
#142Earlier 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…
> 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
#143We 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…
Re: In Praise of Stacked PRs
#144Earlier 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.
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
#145Earlier 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…
Re: In Praise of Stacked PRs
#146I 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…
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
#147Earlier 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…
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
#148Earlier 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…
Re: In Praise of Stacked PRs
#149There 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…
Re: In Praise of Stacked PRs
#150We 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.