23 ms·
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 sugge
by is0tope 4y ago
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.
- ramraj07 4y agoIt’s unavoidable sometimes. I get inspiration and time together rarely, I can’t wait for small chunks of code to be merged before I continue. A lot of times it’s an extremely Productive Sunday afternoon and I have 2500 new lines of code that builds a full new prototype. What am I to do?
- pizza234 4y agoI 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.
- zeroonetwothree 4y agoSome people enjoy their work. I think it’s fine as long as it’s a choice.
- NateEag 4y agoIt may be fine. Keep in mind, though, that humans are not perfect and some choices are unwise.
- ramraj07 4y agoNo one forces me to. No one asks. Sometimes I’m just inspired and would rather write this code than eat.
- computronus 4y agoFirst, 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 initial effort. You might notice those yourself, or reviewers may. The final outcome will end up overall better for it, so spending that time is worthwhile.
- Supermancho 4y ago> 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 a quick once-over to see if it's interactive with any limited resources, merge it in, make a ticket for a re-review later.
- dreamdeath 4y ago> 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-generating product yesterday. But you’re going to worry about how inconvenient it will be for some other developer to review your PR?
- duped 4y agoHence why you don't have massive PRs like that, code that is easy to review is quick to merge and quick to ship. I wouldn't want to work at a startup that doesn't value that in their engineering culture, at least not again. More mature teams (in terms of the staff, not the business) I've been on get this and ship quickly.
- sodapopcan 4y agoWhy would a prototype need a code review?
- chrisseaton 4y ago> The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Unless PRs are merged instantly, I'm always going to be waiting after one PR is opened, before I can work on the next, unless I stack, aren't I? Is your definition of 'fast enough' instantly? If not, how does this work?
- wowokay 4y agoNo? You can create a new branch and start working on the next thing. Why would you be waiting on your PR to complete unless you didn’t split your work correctly.
- chrisseaton 4y agoWhat if the next thing depends on the previous thing?
- fuzzy2 4y agoThen your new branch starts at the tip of the previous one. You can (/will have to) rebase later.
- chrisseaton 4y agoI thought that was what stacked PRs are - maybe not?
- californical 4y agoYou can branch off of a PR, but someone should review and merge your first PR before your second is ready to be up in a PR again. Also, trying to make units of work so that they don’t need to overlap like that can be useful too
- nightpool 4y agoThis is why I think it's really really important that all PR reviews be synchronous, so that there's never any time spent twiddling your thumbs or context switching onto another change. Also it just makes it much easier to review a PR when you can sit down and actually talk about it in real time with the author, rather than having the ping messages back and forth interminably until you reach an agreement
- ignormies 4y agoSomething 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 merging isn't in the review process, but in the deprecation. It wouldn't make sense to land all three of these changes as fast as review/merge would allow; that would skip the deprecation period.
- kqr 4y agoBut 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.
- ignormies 4y agoHaving the change implemented from start-to-finish demonstrates a finished and thorough design. It also allows review to happen with the complete change at the forefront of the reviewers' minds. Kicking the completion of the implementation of a change you've started landing to an unspecified future date indicates poor engineering rigor imo. It's just begging for the change to perpetually be half-migrated and never finished.
- tablespoon 4y ago> 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.
- majormajor 4y agoSometimes? Consider also the stakeholder who gets annoyed whenever the dev team wants to work on something that would take longer than a week to turn around, and limits the things they'll ask for to those estimated at a week. So bigger things can never get done at all, and you'll just be looking for a local maxima instead of having the chance to make more significant changes. Sometimes it's worth it to prep and clean up as you go. Knowing when it's worth it and when it isn't is one thing that makes some devs more valuable long-term than others.
- BerislavLopac 4y agoMy personal rule of thumb is to rebase the working branch as soon as the main has been updated; or at least merge main if the changes are too complex.
- dan-robertson 4y agoOne 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.
- dtech 4y agoYou 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.
- chrisweekly 4y agoThat'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
- dtech 4y agoGiven the cost a typical CI system vs the salary cost of a typical dev, the business case should be fairly easy that devs always should have CI capacity available.
- nemetroid 4y agoThe same burden is present when splitting the two commits into two PRs.
- chrisweekly 4y agoActually no, the burden I referred to was -- unrelated to stacked vs serial PRs -- specifically the GP's practice of requiring that tests fail in CI. "rule that there must be 1 commit just introducing a test that fails on CI for bugfix PRs"
- 4y ago
- mrkeen 4y ago> The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough Yes. Whenever you inspect a proposed solution, you should hopefully find the problem that it solves.
- wpietri 4y agoFor 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 yet delivering value to the people we serve. Plans, design docs, and any unshipped code. There are a lot of reasons to avoid inventory, but a big one is that until something is in use, we haven't closed a feedback loop. Software inventory embodies untested hypotheses. E.g., a product manager thinks users will use X. A designer thinks Y will improve an interface for new users. A developer thinks Z will make for cleaner, faster code. Both large pull requests and stacked pull requests increase inventory. In the case of incorrect hypotheses, they also increase rework. I could believe that for a well-performing team stacked PRs are better than equally-sized single big PRs, in that they could reduce inventory and cycle time. But like you, I think I'd rather just go for frequent, unstacked, small PRs. [1] e.g. https://kanbanize.com/lean-management/value-waste/7-wastes-of-lean https://kanbanize.com/lean-management/value-waste/7-wastes-o...
- bosie 4y ago> 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. I guess this is not true anymore post covid outbreak? Pretty sure a lot of companies would kill to have inventory of their raw materials right now...
- wpietri 4y agoIt is still true. The lean analysis of waste splits things into "necessary waste" and "pure waste". For a particular place and moment in time, there will be some waste that you can't remove without harming production significantly. That's necessary waste. The goal in the long term is to reduce total waste by finding ways to make some bit of necessary waste unnecessary. It's true that pandemic supply chain issues have change the level of necessary waste in a lot of supply chains. But that doesn't make inventory good. Often production halts not due to everything being missing, but a shortfall of just one input. A company might mistakenly react by stock up on everything, but that still won't solve the shortfall of the critical component. So should the just stock up on the critical component? Go get a year's backlog of that? If everybody does that, that will cause a shortfall all on its own, as when everybody did panic buying of toilet paper in 2020. And then when supply chains straighten out, then the stockpile is back to being unnecessary waste. So I don't think there are any simple answers there.
- arxanas 4y agoIf 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 for CPUs. It's efficient under the hypothesis that there aren't too many invalidations/stalls. The linked tooling `git-branchless` (I'm the author) is aimed at reducing the impact of such an invalidation by significantly improving the conflict resolution workflows.
- isoos 4y ago> 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...
- jacobegold 4y agoThe 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.
- rendaw 4y agoI don't think anyone wouldn't agree that being able to work deeply on a single async task would be a great benefit, but your reply completely ignores the issue OP brought up: > Stacking in my personal experience usually leads to merge conflict hell as changes and PR suggestions get merged underneath you. That's been my experience too. And beyond the technical issues, the deeper you work on a single issue the more at risk you are of the simple issue of finding design or requirements issues in the base MR that require going a different direction and discarding the whole stack. So even if you can somehow avoid conflict issues, stacks are still dangerous.
- zmj 4y agoI stack PRs when I'm working on a piece of new code, and in the process discover one or more refactors that simplify the diff for the new code. I wouldn't start at the refactors and then wait to proceed - there's a chance they are dead ends until I know exactly what the new code needs.
- gregmac 4y agoI sometimes stack retroactively. I work on something experimental, then realize it'll be easier to review if I break it down. This often means first a code cleanup that doesn't change any functionality yet, but makes the later changes simpler - eg; removing dead code, removing unnecessary abstractions/interfaces/layers, upgrading external packages. Sometimes I recognize this early, and will specifically make a branch for this. If there's changes from the review I'll rebase my next branch(es) on top. Often I don't see this will be needed until later (or rather: I'm focused on the change itself and not the PR experience) and so I'll interactive rebase to put all the refactor commits first, and make a PR for those. Both cases mean I already have a branch built off an unmerged PR. Sometimes even before the PR is published, actually. I don't see any particular problem with this. My development style and speed is independent of the speed PRs get merged; the ability to rebase makes this a total non-issue.
- jillesvangurp 4y agoThe trick with long lived branches is to merge upstream changes frequently so you don't fall behind and keep your branch in a mergeable state. Git was designed for code bases much larger than what most people deal with where long lived branches are just a reality. Like the Linux kernel for example. You don't get to push half finished work with projects like that. Actually, you don't get to push at all. There is only pull. Some branches exist for many months or even years and there could be entire teams collaborating on them. The one absolute certainty you have on such branches is that there is an absolutely insane amount of upstream changes all the time. The only way to stay on top of that is to merge those changes often. You can think of the Linux kernel git as a decentralized network of stacked branches that have a few central people pulling changes from branches they have reviewed into their own branches with the one that Linus Torvalds maintains as the ultimate branch on which releases get tagged. The vast majority of changes land to his branch via multiple layers of other people, each with their own branches and each adding their own reviews. Effectively all changes Linus Torvalds integrates are stacked. And he doesn't integrate them unless they are stacked properly with nice clean histories. You could say, git was explicitly designed to do stacked branches at scale. So, it's kind of ironic that people are re-discovering this as a thing. It always was intended to be used like this. It's been used like that since the very beginning.
- barrkel 4y agoIf you subscribe to the idea of small PRs (dozen or so diff lines at most) and code ownership then you'll want to split PRs up and have them reviewed by different people. In this mode blocking on code review interrupts flow, and it's not unusual in a bigger company for the owners of some code to be in a different time zone.