In Praise of Stacked PRs(benjamincongdon.me)
benjamincongdon.me
In Praise of Stacked PRs
https://benjamincongdon.me/blog/2022/07/17/In-Praise-of-Stacked-PRs/
222 comments
That sounds great! I have partly solved this issue in my autorebase tool (https://github.com/Timmmm/autorebase) - it basically rebases every branch, and fixes the commit time so that stacked branches get preserved even after a rebase just because the hashes all match properly.
That obviously doesn't work if you modify or drop any of the commits, so this option is very welcome!
That obviously doesn't work if you modify or drop any of the commits, so this option is very welcome!
That’s very interesting! I use a very simple shell script plus git rerere [1] to keep PR stacks up to date with the bottom element’s base branch.
[1] https://git-scm.com/docs/git-rerere
[1] https://git-scm.com/docs/git-rerere
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
Wow, this is incredible and would make myself and so many people I know super happy! This is awesome work!
Is that likely to help with permanent forks or will that still continue to be an issue?
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.
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 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.
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.
> 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...
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...
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.
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.
I 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.
> 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.
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...
One alternative that I sometimes do is to continue the work on a new branch (just as if I was doing stacked PRs), but not send any new PR until the first PR is merged (or at least approved).
Mostly because stacked PRs are usually not ready for review until the base PR is reviewed, and are probably going to get rebased and require some refactors. I've had cases where the base PR had so many changes that I just started over on a new branch.
I know that GitHub now has draft PRs, but I still think that (unless you really want someone to take a look at the draft while the base PR is not merged), it might be better to not make people waste time looking at code that might suffer heavy changes.
Mostly because stacked PRs are usually not ready for review until the base PR is reviewed, and are probably going to get rebased and require some refactors. I've had cases where the base PR had so many changes that I just started over on a new branch.
I know that GitHub now has draft PRs, but I still think that (unless you really want someone to take a look at the draft while the base PR is not merged), it might be better to not make people waste time looking at code that might suffer heavy changes.
> what else are you going to do
You can also change the definition of done - something I try to do when a task gets too large, spin off new sub-tasks, etc. The first feature doesn't quite work right, but the merge differential is usually smaller for tweaks or bug-fixes than it is for major initial features.
You can spend a lot time planning how best to break up your work, and sometimes you may be able to do that successfully, but often times it you are constrained not by git but by your tracking tool (jira, trac), which impose somewhat artificial sizes on code tasks....
And really that's the whole thing stacked prs or commits are trying to solve, minimize the merge differential.
You can also change the definition of done - something I try to do when a task gets too large, spin off new sub-tasks, etc. The first feature doesn't quite work right, but the merge differential is usually smaller for tweaks or bug-fixes than it is for major initial features.
You can spend a lot time planning how best to break up your work, and sometimes you may be able to do that successfully, but often times it you are constrained not by git but by your tracking tool (jira, trac), which impose somewhat artificial sizes on code tasks....
And really that's the whole thing stacked prs or commits are trying to solve, minimize the merge differential.
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 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.
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.
Ooh, good point. I'm suspicious of stacked diffs generally, but this seems like a good example where we truly don't expect to learn anything after the initial deploy.
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.
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.
Sometimes?
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.
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.
Agreed, sizing is one of the things that can set apart devs, either due to experience, knowledge, or some combination of both.
Having 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.
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.
For 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-o...
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-o...
I have stacked diffs sometimes when the rework is large. I want to make sure that I know the full story sounds the change I’m making because I’m forced to think about that upfront. What refactoring was needed? Was it actually needed? What new path do I carve out in the code or how do features interplay? Broken tests with good coverage tell me if I made a foundational mistake. Even if I decide to throw away the work because it was a dead end (rare) the team will have learned something by me explaining what didn’t work out. More often, I’ll need to go back and clean up. But I save significant reviewer time by doing that before putting up random prs one at a time that are not well understood. With the exception of very simple work, stacked reviews generally save significant time. You get reviews of non objectionable prs. Coworkers can see a bigger picture if that’s helpful to understand the context of the change that’s still coming into shape. It actually reduces merge conflicts because, for example, you can enable a refactor that everyone agrees needs to be done and land that. Then your conflict space is smaller.
Small prs don’t need it of course but complex features benefit from shaking out things earlier. Commit more than 100 lines are really hard to review (lots of anecdotal and empirical research). If you’re not reviewing small commit by small commit, the reviews are easily missing things. A single PR that’s 800 lines adds review time to go commit by commit. If you can merge the non objectionable stuff, the reviewee gets to feel a some of forward progress and fewer merge conflicts (eg someone lands a refactor before some simple change of yours vs your simple change handed before and you made it the person refactoring their problem where it belongs)
Small prs don’t need it of course but complex features benefit from shaking out things earlier. Commit more than 100 lines are really hard to review (lots of anecdotal and empirical research). If you’re not reviewing small commit by small commit, the reviews are easily missing things. A single PR that’s 800 lines adds review time to go commit by commit. If you can merge the non objectionable stuff, the reviewee gets to feel a some of forward progress and fewer merge conflicts (eg someone lands a refactor before some simple change of yours vs your simple change handed before and you made it the person refactoring their problem where it belongs)
It sounds like you're talking about a bunch of different cases, and I'm having trouble untangling them.
If there's a simple refactoring everybody agrees is good whether or not your overall goal ends up making sense, then yes, by all means merge that. But that doesn't require stacking unless your review process is slow. In which case I still think the right solution is to speed up review, not to stack.
For the cases where we don't know the full story, my first thought is that we never know the full story. So there I try to instead find the smallest unit of work that everybody agrees is a step forward.
When that's not possible, where the unit of work still seems pretty large, instead of breaking that up into a bunch of stacked diffs that shouldn't be merged until we really understand something (which to me sounds like a large PR in disguise), I think a better option is a spike, where we intentionally do a quick, throwaway version of some change as a way of learning about the change. Instead of trying to do good code along the way to good understanding, we just go for the understanding. Once we have thrown out that scratch code, we then go back with our new knowledge for a proper PR.
So I'm still not seeing where I would use stacked diffs, except in this case here: https://news.ycombinator.com/item?id=32215346
The premise of stacked diffs seems that we won't learn anything significant from reviewing or deploying code. (If we did learn something valuable, then the things stacked on top could be up for a lot of rework or might be thrown out altogether.) I think that has a lot of bad effects, but one of the biggest for me is that the bigger the inventory of code (whether in one big PR or a stacked set of smaller ones), the more a reviewer will feel obliged to say, "LGTM" and let it go, because they know there's not much point in saying, "Actually, I think this whole thing could be better approached by X."
So like you I'm entirely for small, reviewable lumps that are easily merged. I just think they should be then reviewed quickly, so that stacking or agglomerating is unnecessary.
If there's a simple refactoring everybody agrees is good whether or not your overall goal ends up making sense, then yes, by all means merge that. But that doesn't require stacking unless your review process is slow. In which case I still think the right solution is to speed up review, not to stack.
For the cases where we don't know the full story, my first thought is that we never know the full story. So there I try to instead find the smallest unit of work that everybody agrees is a step forward.
When that's not possible, where the unit of work still seems pretty large, instead of breaking that up into a bunch of stacked diffs that shouldn't be merged until we really understand something (which to me sounds like a large PR in disguise), I think a better option is a spike, where we intentionally do a quick, throwaway version of some change as a way of learning about the change. Instead of trying to do good code along the way to good understanding, we just go for the understanding. Once we have thrown out that scratch code, we then go back with our new knowledge for a proper PR.
So I'm still not seeing where I would use stacked diffs, except in this case here: https://news.ycombinator.com/item?id=32215346
The premise of stacked diffs seems that we won't learn anything significant from reviewing or deploying code. (If we did learn something valuable, then the things stacked on top could be up for a lot of rework or might be thrown out altogether.) I think that has a lot of bad effects, but one of the biggest for me is that the bigger the inventory of code (whether in one big PR or a stacked set of smaller ones), the more a reviewer will feel obliged to say, "LGTM" and let it go, because they know there's not much point in saying, "Actually, I think this whole thing could be better approached by X."
So like you I'm entirely for small, reviewable lumps that are easily merged. I just think they should be then reviewed quickly, so that stacking or agglomerating is unnecessary.
> unless your review process is slow. In which case I still think the right solution is to speed up review, not to stack.
This feels like a critically important point to your position but there's no actionable advice provided on how to achieve that. FWIW I've found stacked PRs do speedup reviews because it pipelines the work. Pipelining something removes bubbles (in this case time spent waiting on review) from forming.
Simpler parts of the work get eyeballs from more junior engineers who feel more comfortable approving smaller / simpler PRs (& other people feel confident in that). Trickier stuff is left to the smaller pool of people who have the appropriate context / skillset. If you're waiting on landing 1 PR at a time, then you're serializing the review flow which means your total time on the PR is "time spent writing + time spent reviewing 100% of the code". If you pipeline your stacked diff, then potentially you could get ~80% of the code reviewed & landed by the time you finish the more complex pieces. Then you're left with "time spent writing + time spent reviewing 20% of the code". Additionally, by putting up the commits early, you're letting other people fit smaller reviews into their schedule more easily vs "here's a PR with 5000 lines of code changes" which is a monstrosity to review (i.e. quickly runs into mental fatigue issues / quality of the reviews can easily degrade, especially if commit hygiene wasn't practiced).
Have you actually tried stack PRs with a proper review process & good commit hygiene? This is one of those "try it before you knock" it things.
This feels like a critically important point to your position but there's no actionable advice provided on how to achieve that. FWIW I've found stacked PRs do speedup reviews because it pipelines the work. Pipelining something removes bubbles (in this case time spent waiting on review) from forming.
Simpler parts of the work get eyeballs from more junior engineers who feel more comfortable approving smaller / simpler PRs (& other people feel confident in that). Trickier stuff is left to the smaller pool of people who have the appropriate context / skillset. If you're waiting on landing 1 PR at a time, then you're serializing the review flow which means your total time on the PR is "time spent writing + time spent reviewing 100% of the code". If you pipeline your stacked diff, then potentially you could get ~80% of the code reviewed & landed by the time you finish the more complex pieces. Then you're left with "time spent writing + time spent reviewing 20% of the code". Additionally, by putting up the commits early, you're letting other people fit smaller reviews into their schedule more easily vs "here's a PR with 5000 lines of code changes" which is a monstrosity to review (i.e. quickly runs into mental fatigue issues / quality of the reviews can easily degrade, especially if commit hygiene wasn't practiced).
Have you actually tried stack PRs with a proper review process & good commit hygiene? This is one of those "try it before you knock" it things.
Again, I think you're tangling together cases. And also tangling arguments. You can't argue from theoretical gains in an imagined optimal case and then say the only argument that matters is the empirical one.
If your point is that in some existing organizations stacked pull requests are better for a specific engineer's experience than doing all the related code in a big blob, I certainly believe you.
Similarly, there are manufacturing shops where reducing inventory in line with Lean approaches doesn't work out of the gate, because there are other organizational problems/constraints that have to be dealt with first. For example, you might need a large buffer of component X at stage Y of a manufacturing process because upstream quality issues mean that a smaller buffer would cause frequent stalls at stage Y. First you have to fix the upstream issue before you can cut stocks there.
So are stacked pull requests the optimal choice for some specific person on some specific occasion? Sure! I'll take your word that's the case for you. What I'm saying is that I think they're an indicator that there is some systemic problem that could be resolved so stacked pull requests and giant pull requests both become unnecessary.
If your point is that in some existing organizations stacked pull requests are better for a specific engineer's experience than doing all the related code in a big blob, I certainly believe you.
Similarly, there are manufacturing shops where reducing inventory in line with Lean approaches doesn't work out of the gate, because there are other organizational problems/constraints that have to be dealt with first. For example, you might need a large buffer of component X at stage Y of a manufacturing process because upstream quality issues mean that a smaller buffer would cause frequent stalls at stage Y. First you have to fix the upstream issue before you can cut stocks there.
So are stacked pull requests the optimal choice for some specific person on some specific occasion? Sure! I'll take your word that's the case for you. What I'm saying is that I think they're an indicator that there is some systemic problem that could be resolved so stacked pull requests and giant pull requests both become unnecessary.
> 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...
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...
It 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.
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.
> 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?
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?
Two top-level scenarios here:
* you're working on the same part of the code
* you aren't working on the same part of the code
The second scenario is common but also trivial here since you can just have parallel branches going, so I'm gonna assume more the first - working on something that's building on top of what you just put up for review.
Let's say the review is done in 2 hours. If you're already done with the followup, IMO you may be erring too far on the side of "small PRs." If you aren't, you just rebase on top of whatever changes had to be made, if any, to the first one, once it lands on the primary branch.
If, on the other hand, the review isn't done for 2 days... then that's a PR turnaround time problem for sure.
But I strongly disagree with the people saying "multiple dependent PRs suggests the work wasn't split up properly" - there's nothing worse than a mega-PR of thousands of lines for the sake of doing a "single feature" all in one shot vs having to possibly pause and rebase periodically after review. It's even more painful when this mega-PR requires fundamental changes that could've been caught earlier, but now will take longer to adjust, and then will stay open for a while and likely result in merge conflicts as a result.
* you're working on the same part of the code
* you aren't working on the same part of the code
The second scenario is common but also trivial here since you can just have parallel branches going, so I'm gonna assume more the first - working on something that's building on top of what you just put up for review.
Let's say the review is done in 2 hours. If you're already done with the followup, IMO you may be erring too far on the side of "small PRs." If you aren't, you just rebase on top of whatever changes had to be made, if any, to the first one, once it lands on the primary branch.
If, on the other hand, the review isn't done for 2 days... then that's a PR turnaround time problem for sure.
But I strongly disagree with the people saying "multiple dependent PRs suggests the work wasn't split up properly" - there's nothing worse than a mega-PR of thousands of lines for the sake of doing a "single feature" all in one shot vs having to possibly pause and rebase periodically after review. It's even more painful when this mega-PR requires fundamental changes that could've been caught earlier, but now will take longer to adjust, and then will stay open for a while and likely result in merge conflicts as a result.
> Let's say the review is done in 2 hours.
One of the projects I work on is a large, well-known open source project. Reviews that take days are pretty common.
> If you're already done with the followup, IMO you may be erring too far on the side of "small PRs."
There have been times when I created a sequence of three or four separate commits within under half an hour.
This was on projects where test coverage is tricky (think hardware interfaces) and keeping changes small was motivated a lot by better bisectability.
Clearly, software development is a pretty broad field with lots of different experiences.
One of the projects I work on is a large, well-known open source project. Reviews that take days are pretty common.
> If you're already done with the followup, IMO you may be erring too far on the side of "small PRs."
There have been times when I created a sequence of three or four separate commits within under half an hour.
This was on projects where test coverage is tricky (think hardware interfaces) and keeping changes small was motivated a lot by better bisectability.
Clearly, software development is a pretty broad field with lots of different experiences.
By pinging people on slack and interrupting them for a review asap? Or context switching to an unrelated part of the code in parallel while you wait? Or reducing the frequency of reviews and make a mega-PR every couple weeks?
I've seen no solutions, only tradeoffs but I'm curious if anyone has a tried and true way to avoid this traffic jam scenario.
I've seen no solutions, only tradeoffs but I'm curious if anyone has a tried and true way to avoid this traffic jam scenario.
Yes, complete your story in a single PR, if the next story is related and requires your changes your work wasn’t split up properly, plus in that scenario there is a good chance other devs have to many dependencies on other devs work which is a recipe for disaster!
This 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
In one of my first jobs, code reviews were done exactly like this, with the the reviewer (my boss) and I sitting together at one computer going through the changes.
It definitely has benefits but it's still important to ensure recommendations/concerns etc. get written down and doesn't necessarily help if there's a need to go away and make significant changes based on the outcomes of discussions.
But while the back & forth discussion that often occurs during review can benefit from being synchronous, it might still be some time before your coworker has available time to participate in such a session, so it's still likely you'll need something else to work on in the mean time.
> 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't work on your ticket, and you don't have anything else to do or have to pick up another ticket and start learning about a whole completely separate problem. (And yes, definitely everything still needs to be written down, it's important to walk away knowing which changes you need to make, and why!)
> it might still be some time before your coworker has available time to participate in such a session, so it's still likely you'll need something else to work on in the mean time.
In teams I've worked on, the expectation is that an engineers highest priority is always unblocking another engineer, so this very rarely happened. Unless they had an interview to go to or some kind of meeting—and in that case, you could always just ping somebody else.
Obviously it's a style of work that relies really heavily on everyone sharing the same timezone and work hours, but it works really well to eliminate time lost to context switching and minimize the amount of time engineers spend blocked waiting on someone else.
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't work on your ticket, and you don't have anything else to do or have to pick up another ticket and start learning about a whole completely separate problem. (And yes, definitely everything still needs to be written down, it's important to walk away knowing which changes you need to make, and why!)
> it might still be some time before your coworker has available time to participate in such a session, so it's still likely you'll need something else to work on in the mean time.
In teams I've worked on, the expectation is that an engineers highest priority is always unblocking another engineer, so this very rarely happened. Unless they had an interview to go to or some kind of meeting—and in that case, you could always just ping somebody else.
Obviously it's a style of work that relies really heavily on everyone sharing the same timezone and work hours, but it works really well to eliminate time lost to context switching and minimize the amount of time engineers spend blocked waiting on someone else.
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 eliminate it seems like wishful thinking. Perhaps there's a case for minimising it for more junior devs that aren't as capable of dealing with it, though it's equally possible younger brains are better at it!
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.
It requires a mental switch in focus. For me anyway, it's not a big difference if it's between working on implementing feature A then switching to working on implementing feature B, and working on feature A then switching to doing a code review for feature B, but I probably take my code reviewing responsibilities more seriously than most (e.g. I always checkout the branch locally, ensure it builds and runs etc.).
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 reviews etc.
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 reviews etc.
> 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.
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.
If your second PR is ready before the first PR is merged, then two of the likliest outcomes are that either PR reviews are taking too long, or the second PR is small enough that it could have just been part of the first. Alternatively, the review is taking a long time because the first PR was bad/controversial, in which case the assumptions of the second PR might need to be reevaluated anyway.
Neither of those cases need to be true for a second PR to be ready before the first has been merged.
For example you do your first PR, mark it ready for review. While doing it you notice there's some refactoring you could do to some tangentially related code. It's very conceivable that the second refactoring PR could be ready pretty quickly.
For example you do your first PR, mark it ready for review. While doing it you notice there's some refactoring you could do to some tangentially related code. It's very conceivable that the second refactoring PR could be ready pretty quickly.
There's a reason I said "two of the likliest outcomes", and not "the only two possibilities".
Yes there are other situations, but in my experience, the ones I mentioned are the most common.
Yes there are other situations, but in my experience, the ones I mentioned are the most common.
You 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
Also, trying to make units of work so that they don’t need to overlap like that can be useful too
No? 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.
What if the next thing depends on the previous thing?
Then your new branch starts at the tip of the previous one.
You can (/will have to) rebase later.
You can (/will have to) rebase later.
I thought that was what stacked PRs are - maybe not?
One way this can work is pair programming. Especially if you also practice frequent pair rotation, you can get plenty of eyeballs on code in a timely fashion. That's my preference.
Another way is that you next work on something different enough that it doesn't need to stack. E.g., reviewing pull requests.
Another way is that you next work on something different enough that it doesn't need to stack. E.g., reviewing pull requests.
I 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.
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.
The 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.
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.
It’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?
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 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.
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.
Isn't this exactly why you should use stacked PRs? That's what it looks like when I break a big piece of work into smaller pieces.
What's the alternative anyway? If you don't want me writing 2500 lines of code in one area, would you rather I write 10 250 line PRs in 10 different parts of the codebase instead? Is that supposed to be easier to review?
Or is the rule just "don't write a lot of code in any short period of time"?
What's the alternative anyway? If you don't want me writing 2500 lines of code in one area, would you rather I write 10 250 line PRs in 10 different parts of the codebase instead? Is that supposed to be easier to review?
Or is the rule just "don't write a lot of code in any short period of time"?
> don't write a lot of code in any short period of time
Depending on the team this may be a completely reasonable policy.
Depending on the team this may be a completely reasonable policy.
(if you are on this team, run away as fast as you can)
No, there’s no reason to run away from a team of overenthusiastic junior developers. Teach them instead and you’ll get a team of enthusiastic skilled developers, you just need to make sure they don’t make too much of a mess in the meantime.
> 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.
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.
> 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?
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?
To offer an alternative perspective: I haven’t ever worked at a startup.
If someone wanted a PR review for 2500 lines, I would tell them no. I absolutely expect a developer to think twice when trying to make a change that large in one go.
When your corporation’s billion lines of code are the engine that helps generate $30,000 a minute, you don’t just say “meh, looks good to me.”
Furthermore, this is multiplied on my team, which is the framework team. A mistake in our auth middleware, http client, etc, could easily turn into a mistake on 50+ other teams.
If someone wanted a PR review for 2500 lines, I would tell them no. I absolutely expect a developer to think twice when trying to make a change that large in one go.
When your corporation’s billion lines of code are the engine that helps generate $30,000 a minute, you don’t just say “meh, looks good to me.”
Furthermore, this is multiplied on my team, which is the framework team. A mistake in our auth middleware, http client, etc, could easily turn into a mistake on 50+ other teams.
Hence 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.
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.
If you're in a go-under-the-next-day scenario who is even reviewing your code? How big is your team, realistically?
Once you have significant runway and can hire, you need to be able to start delivering faster and faster. One person writing 2500 lines of code in bursts once or twice a week isn't going to scale. You need a team, and you need to be measuring total team throughput.
Need a new service prototype? A 2500-line PR is appropriate. But don't be surprised if it needs some big revisions - that's the point of a prototype anyway.
Need a feature? One person periodically "when inspiration and time line up" dropping 2500 lines (this is a LOT of lines in most languages used these days, even in newer, more-concise Java versions) on top of what 10 other people may be working on is not going to help everyone else move quickly at all.
If you're the person who's so busy you rarely have time to code, you need to figure out how to turn your inspirations into ideas others can execute and be the code-level experts on. A team of 10 "1x" developers is still more productive than a single "10x" developer who's in meetings figuring out the company's plan half the day anyway.
Once you have significant runway and can hire, you need to be able to start delivering faster and faster. One person writing 2500 lines of code in bursts once or twice a week isn't going to scale. You need a team, and you need to be measuring total team throughput.
Need a new service prototype? A 2500-line PR is appropriate. But don't be surprised if it needs some big revisions - that's the point of a prototype anyway.
Need a feature? One person periodically "when inspiration and time line up" dropping 2500 lines (this is a LOT of lines in most languages used these days, even in newer, more-concise Java versions) on top of what 10 other people may be working on is not going to help everyone else move quickly at all.
If you're the person who's so busy you rarely have time to code, you need to figure out how to turn your inspirations into ideas others can execute and be the code-level experts on. A team of 10 "1x" developers is still more productive than a single "10x" developer who's in meetings figuring out the company's plan half the day anyway.
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?
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?
[deleted]
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 one was when three of us spent the hours leading up to a demo disabling automation and deploying to production by hand. What that meant was disabling a bunch of tests and code that checks those tests succeed in order to get the code out the door. We spent the next week cleaning that up. Sometimes you need to test in production.
Another was actually a demo for the same client that set up that meeting. This time we had heard from the initial introductions they had a specific, very nasty problem and we could ostensibly solve it (that was true, we could, but the product at that point had no facility to do it). So I spent I think three days hacking together a solution that would solve the problem for the contrived demos we would show. That solution had atrocious performance characteristics so we wound up spending a few story points over the next two sprints to optimize and refactor the guts.
Yet another demo was an integration. We had scoped out the general feature (which was magical when it worked) but it required a large amount of effort to coordinate with the original software vendors to get the data we would need. But those vendors have a tool with a free trial install which had a bunch of the data in XML, so I spent a day reverse engineering the schema and a second day parsing into our internal data model which we demoed on the third day to show how our product could solve that particular problem. We never wound up getting data from the first party company, and the parser got rewritten in a different language, and the backend that does the stuff is planned to be replaced in the next few sprints once it gets priority.
So TL;DR you have a demo scheduled, you hack together a thing that can be presented, then spend your time reimplementing or refactoring the demo code into something maintainable.
It helps to have a stack you can really quickly iterate on. And management that understands demos aren't suitable for production.
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 one was when three of us spent the hours leading up to a demo disabling automation and deploying to production by hand. What that meant was disabling a bunch of tests and code that checks those tests succeed in order to get the code out the door. We spent the next week cleaning that up. Sometimes you need to test in production.
Another was actually a demo for the same client that set up that meeting. This time we had heard from the initial introductions they had a specific, very nasty problem and we could ostensibly solve it (that was true, we could, but the product at that point had no facility to do it). So I spent I think three days hacking together a solution that would solve the problem for the contrived demos we would show. That solution had atrocious performance characteristics so we wound up spending a few story points over the next two sprints to optimize and refactor the guts.
Yet another demo was an integration. We had scoped out the general feature (which was magical when it worked) but it required a large amount of effort to coordinate with the original software vendors to get the data we would need. But those vendors have a tool with a free trial install which had a bunch of the data in XML, so I spent a day reverse engineering the schema and a second day parsing into our internal data model which we demoed on the third day to show how our product could solve that particular problem. We never wound up getting data from the first party company, and the parser got rewritten in a different language, and the backend that does the stuff is planned to be replaced in the next few sprints once it gets priority.
So TL;DR you have a demo scheduled, you hack together a thing that can be presented, then spend your time reimplementing or refactoring the demo code into something maintainable.
It helps to have a stack you can really quickly iterate on. And management that understands demos aren't suitable for production.
What am I gaining from having a stack of PRs there versus just a series of commits that I'm gonna go back and polish later, quite probably breaking into various PRs, but with some additional time and under less pressure?
When I've hacked together stuff like that I'm often making it up on the fly, I rarely have a plan in mind that lines up with how I'd want to present it for code review later. Hell, a lot of times some of the later PRs would simply remove the entirety of a failed earlier attempt!
When I've hacked together stuff like that I'm often making it up on the fly, I rarely have a plan in mind that lines up with how I'd want to present it for code review later. Hell, a lot of times some of the later PRs would simply remove the entirety of a failed earlier attempt!
Why are you doing your demos in a production environment though? How many live customers did you have that could have been seriously negatively affected by what you were deploying (assuming it almost certainly contained bugs)?
Zero, but only one of those demos actually touched production.
Thanks for the anecdotes!
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.
> 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.
If you're the only developer for quite a while... otherwise reviewability is a thing PRs must be optimized for or your super-duper-important feature won't get in.
Either way you should be looking for a new job, because what you're describing is quite unsustainable in a team of more than one person working on the same thing.
Either way you should be looking for a new job, because what you're describing is quite unsustainable in a team of more than one person working on the same thing.
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.
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.
No one forces me to. No one asks. Sometimes I’m just inspired and would rather write this code than eat.
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.
Keep in mind, though, that humans are not perfect and some choices are unwise.
Why would a prototype need a code review?
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.
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.
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.
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
Given 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.
The same burden is present when splitting the two commits into two PRs.
Actually 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"
"rule that there must be 1 commit just introducing a test that fails on CI for bugfix PRs"
Ideally your CI workflow prevents merging something that breaks the tests. Also now if the first review merges but the other doesn't for some reason you've broken everyone else which is bad.
EDIT: at least in rebase workflows which is what I'm used to. I guess in merge workflows this works.
EDIT: at least in rebase workflows which is what I'm used to. I guess in merge workflows this works.
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.
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.
My 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.
I 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.
If 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.
> 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.
Yes. Whenever you inspect a proposed solution, you should hopefully find the problem that it solves.
I’m going to be a little crass for a moment: people don’t know how to use their tools! Why are we even talking about “stacked PRs” and “branchless” workflows?! It’s clear the author is documenting their git learning process and that’s great, really. But I’m just surprised how pedantic people get about forcing others to use a tool in a way that caters to their own limited understanding of it. Or, even worse, caters to limitations in the 3rd party UI being used to interface with it (ye ol’ git vs GitHub problem). Learn your tool and then you can worry about the higher order things that are supported by it.
This comment would be more compelling if it gave any hint about what aspect of the tooling the author didn’t understand or what they should be doing differently.
I’m not talking about the author, specifically or criticizing them directly. I’m lamenting how often I encounter people putting intense amounts of effort into arguing about how git should be used. My hypothesis is that if people understood their tools better before getting all evangelical about forcing their entire eng team to work in their style, others wouldn't have to spend so much time trying to convince them that other styles are exist and are valid.
Compare with a construction site: there are obvious uses for tools and there are less obvious uses and I haven't been on a single one where tools are only used specifically in one way for the entirety of the project. They’re used dynamically by different people with different experiences in order to complete the project. Nobody forces other workers to use a specific grip when holding their hammer…
Compare with a construction site: there are obvious uses for tools and there are less obvious uses and I haven't been on a single one where tools are only used specifically in one way for the entirety of the project. They’re used dynamically by different people with different experiences in order to complete the project. Nobody forces other workers to use a specific grip when holding their hammer…
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.
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.
Stacked branches tend to produce repository structures that make this easy, but so do lots of other development practices. It is not related to "how to evaluate the completion of work", it's entirely about how you use the hammer.
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 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. If they're not skilled git users, they have no hope of managing multiple PRs effectively only be virtue of presenting them all at once.
Or: Since stacking PRs allows you to create a DAG of dependent changes, this natually allows you to manage code changes that need to be submitted in a particular order.
Assuming this is in a single repository, a fast-forward-only commit policy alone ensures this.
Or: stacked PRs use branches, and can have multiple commits in a single atomic change; stacked commits use a single commit as the unit of atomic change.
Stacked PRs usually use branches, but stacked commits also still use at least one branch and could have more. In both the commit is the unit of atomic change, because that's what a commit is.
I'll also add I find the entire language around "branchless" workflows in git confusing, not just from this author. There is no such thing; considering one special branch as "not a branch" or your local and remote and someone else's remote as the "same branch" just because they have the same name is a holdover from older/other VC tools. We don't do any new git users a pedagogical or practical favor by clinging to that view.
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 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. If they're not skilled git users, they have no hope of managing multiple PRs effectively only be virtue of presenting them all at once.
Or: Since stacking PRs allows you to create a DAG of dependent changes, this natually allows you to manage code changes that need to be submitted in a particular order.
Assuming this is in a single repository, a fast-forward-only commit policy alone ensures this.
Or: stacked PRs use branches, and can have multiple commits in a single atomic change; stacked commits use a single commit as the unit of atomic change.
Stacked PRs usually use branches, but stacked commits also still use at least one branch and could have more. In both the commit is the unit of atomic change, because that's what a commit is.
I'll also add I find the entire language around "branchless" workflows in git confusing, not just from this author. There is no such thing; considering one special branch as "not a branch" or your local and remote and someone else's remote as the "same branch" just because they have the same name is a holdover from older/other VC tools. We don't do any new git users a pedagogical or practical favor by clinging to that view.
> 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.
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.
What's better context than presenting all the related atomic commits at one time in a single pull request so you know exactly which related set of commits are being added and for what reason? Slap a merge commit on and you can revert the whole thing! But your commits are a mess, and I want to see clean ones. That is what interactive rebase is for and what attentive project contributors would do in order to clean up before or during the merge process. Why is it fair to ask someone to change syntax formatting to but not cleanup their commits? Because the latter is a "big ask" for someone who doesn't know their tool.
Then we get back to the problem of not having independent review, CI status, and deployability for each individual changeset. There's no amount of git competency that changes the fact that GitHub PRs operate at the branch level, not the commit level. I've actually done exactly what you say many times -- interactive rebase to clean up the commit history -- but the end result of that process is the creation of multiple "stacked" branches, solely so I can submit multiple PRs and conform with the team's expectations.
> There's no amount of git competency that changes the fact that GitHub PRs operate at the branch level, not the commit level.
Most all CI tools support per-commit checks with simple configuration/scripting of their execution DSL/API. In GitHub, push actions already works this way. You can do the same thing on any branch after a PR is opened: on synchronize you can iterate over all the updated refs, run checks, and publish results to the commit status API https://docs.github.com/en/rest/commits/statuses, and https://docs.github.com/en/pull-requests/collaborating-with-....
Most all CI tools support per-commit checks with simple configuration/scripting of their execution DSL/API. In GitHub, push actions already works this way. You can do the same thing on any branch after a PR is opened: on synchronize you can iterate over all the updated refs, run checks, and publish results to the commit status API https://docs.github.com/en/rest/commits/statuses, and https://docs.github.com/en/pull-requests/collaborating-with-....
git-branchless author here. By "branchless", I mean literally using detached HEAD as the primary state of development. If you're using tools like Gerrit or Phabricator, you never have to explicitly make a branch to get your code merged. If using GitHub, then branches are unavoidable, but it can be nice to do branchless development as part of rapid prototyping (see https://github.com/arxanas/git-branchless/wiki/Workflow:-div...).
Another term you could use is "anonymous branching". This is not technically accurate in the above workflows, but it captures the essence pretty well in Git, more so than "branchless".
Another term you could use is "anonymous branching". This is not technically accurate in the above workflows, but it captures the essence pretty well in Git, more so than "branchless".
But when you call it "anonymous branching" you lay bare that the only advantage is that you don't need to name your work "branch", and meanwhile you have a workflow that's needlessly incompatible with most other git tooling.
In particular, since this is the one I see usually called out as a benefit of branchless:
Stock Git does not have good ways of rebasing a sequence of branches.
A sequence of branches can be rebased by rebasing (or otherwise rewriting) the longest one (the only one you'll need locally) then pushing the individual commits in the current branch to the remote under any relevant branch names. This doesn't take zero time, but with good git UIs it will take less time than remembering `git move`, and it's not especially hard to do with the stock CLI either.
In particular, since this is the one I see usually called out as a benefit of branchless:
Stock Git does not have good ways of rebasing a sequence of branches.
A sequence of branches can be rebased by rebasing (or otherwise rewriting) the longest one (the only one you'll need locally) then pushing the individual commits in the current branch to the remote under any relevant branch names. This doesn't take zero time, but with good git UIs it will take less time than remembering `git move`, and it's not especially hard to do with the stock CLI either.
> But when you call it "anonymous branching" you lay bare that the only advantage is that you don't need to name your work branch
Sure, I'm only responding to what you were saying about "There is no such thing; considering one special branch as 'not a branch'". There is such a thing in that there is no branch involved in the detached HEAD state. It's not some kind of Git misunderstanding. I think you might be referring to trunk-based development and always building off of the remote main branch instead of having your own local copy, which is unrelated to being "branchless", for the reasons you stated.
For many people (particularly those on Github!), a branchless workflow won't help, so you're free to not use it. In my opinion, it's a workflow that is better compatible than stock Git with code review tools like Gerrit and Phabricator.
I personally argue that anonymous branching is useful even in some branch-based workflows. Mainly, if Git branches are so lightweight to use, why do we also use the command `git stash`, instead of just always creating a new branch for our temporary work? One benefit of anonymous branching is that it consolidates these workflows in a convenient way. Some people don't stash changes or feel that branching in those cases is heavyweight, so anonymous branching doesn't help them at all.
> then pushing the individual commits in the current branch to the remote under any relevant branch names
If I'm understanding correctly, every time you rebase the longest branch, for each commit in the branch, you would manually run e.g. `git push <commit> origin:my-branch-name`? That seems like it would take a lot of time to me. Is the tacit assumption here that you don't have a lot of commits in your branch, so this doesn't take a lot of time?
Sure, I'm only responding to what you were saying about "There is no such thing; considering one special branch as 'not a branch'". There is such a thing in that there is no branch involved in the detached HEAD state. It's not some kind of Git misunderstanding. I think you might be referring to trunk-based development and always building off of the remote main branch instead of having your own local copy, which is unrelated to being "branchless", for the reasons you stated.
For many people (particularly those on Github!), a branchless workflow won't help, so you're free to not use it. In my opinion, it's a workflow that is better compatible than stock Git with code review tools like Gerrit and Phabricator.
I personally argue that anonymous branching is useful even in some branch-based workflows. Mainly, if Git branches are so lightweight to use, why do we also use the command `git stash`, instead of just always creating a new branch for our temporary work? One benefit of anonymous branching is that it consolidates these workflows in a convenient way. Some people don't stash changes or feel that branching in those cases is heavyweight, so anonymous branching doesn't help them at all.
> then pushing the individual commits in the current branch to the remote under any relevant branch names
If I'm understanding correctly, every time you rebase the longest branch, for each commit in the branch, you would manually run e.g. `git push <commit> origin:my-branch-name`? That seems like it would take a lot of time to me. Is the tacit assumption here that you don't have a lot of commits in your branch, so this doesn't take a lot of time?
> There is such a thing in that there is no branch involved in the detached HEAD state.
But at the very least there's still an existing remote branch, which is the ultimate merge target, for example - perhaps even multiple. Since we're talking about coordination, there's also the actual state at the remote vs. my view of the remote vs. my teammates' views of the remote. But we don't think about this too much, because we can synchronize easily as long as the remote branch has a name. Taking the name off the branch makes it more difficult to do anything with someone else's work other than merge it, e.g. pass a changeset back and forth or hand over a half-complete task.
> why do we also use the command `git stash`, instead of just always creating a new branch for our temporary work?
I have no idea actually, because I don't. I haven't run git stash manually since I learned about autostash, and even before that it wasn't for temporary work but for changes I decided I wanted somewhere else only after writing them. Temporary work mostly does get a branch (usually as a new commit on top of my current local branch).
> If I'm understanding correctly, every time you rebase the longest branch, for each commit in the branch, you would manually run e.g. `git push <commit> origin:my-branch-name`?
Close, except I'd scroll down the list of commits and type `P o RET TAB branch-name` (or something to that effect, it'd be slightly different if I have several remotes). This would take perhaps ~0.5 seconds per branch and not require me to context switch to a terminal.
It does take longer for people using less powerful clients, but virtually all do provide some way to do it even it means a few right-clicks on a menu and clicking some "OK" buttons, and there's value in everyone using analogous operations. And the people using those clients usually can't reliably recover from a large class of "git broke" mistakes, assistance from git-branchless or not, so handing them a CLI and a prayer is out of the question. And even the "long version" is pretty negligible (like, maybe 10 seconds per branch?) compared to everything else you should do to make your changeset approachable for review.
But at the very least there's still an existing remote branch, which is the ultimate merge target, for example - perhaps even multiple. Since we're talking about coordination, there's also the actual state at the remote vs. my view of the remote vs. my teammates' views of the remote. But we don't think about this too much, because we can synchronize easily as long as the remote branch has a name. Taking the name off the branch makes it more difficult to do anything with someone else's work other than merge it, e.g. pass a changeset back and forth or hand over a half-complete task.
> why do we also use the command `git stash`, instead of just always creating a new branch for our temporary work?
I have no idea actually, because I don't. I haven't run git stash manually since I learned about autostash, and even before that it wasn't for temporary work but for changes I decided I wanted somewhere else only after writing them. Temporary work mostly does get a branch (usually as a new commit on top of my current local branch).
> If I'm understanding correctly, every time you rebase the longest branch, for each commit in the branch, you would manually run e.g. `git push <commit> origin:my-branch-name`?
Close, except I'd scroll down the list of commits and type `P o RET TAB branch-name` (or something to that effect, it'd be slightly different if I have several remotes). This would take perhaps ~0.5 seconds per branch and not require me to context switch to a terminal.
It does take longer for people using less powerful clients, but virtually all do provide some way to do it even it means a few right-clicks on a menu and clicking some "OK" buttons, and there's value in everyone using analogous operations. And the people using those clients usually can't reliably recover from a large class of "git broke" mistakes, assistance from git-branchless or not, so handing them a CLI and a prayer is out of the question. And even the "long version" is pretty negligible (like, maybe 10 seconds per branch?) compared to everything else you should do to make your changeset approachable for review.
Which Git client do you use, and also, how do you remember all the remote branch names?
> And even the "long version" is pretty negligible (like, maybe 10 seconds per branch?)
I notice that mainly the differences in opinion with regards to workflow is disagreement about "how long is too long" for various operations :)
> And even the "long version" is pretty negligible (like, maybe 10 seconds per branch?)
I notice that mainly the differences in opinion with regards to workflow is disagreement about "how long is too long" for various operations :)
Magit, and the remote branch names are visible in the log view and easy to enter via tab completion. (We're still talking about stacked PRs right? Median stacked at any given time is probably 2 and p90 is maybe 4? Max I've ever seen is ~10 and that was quite a fucked up situation for plenty of other reasons.)
10 seconds would certainly be too long for me. It is obviously not too long for them, or they'd learn some keyboard shortcuts to get the easy 5 out of the way to begin with. But regardless, it's all pretty much dwarfed by things that need actual brainpower like re-reading my commit messages for grammatical errors / broken links / etc.
10 seconds would certainly be too long for me. It is obviously not too long for them, or they'd learn some keyboard shortcuts to get the easy 5 out of the way to begin with. But regardless, it's all pretty much dwarfed by things that need actual brainpower like re-reading my commit messages for grammatical errors / broken links / etc.
I see, thanks for the information. I often stack 10+ commits, so I don't think your workflow would be reasonable for me. (For example, here, I have 11 finished commits and 3 in progress: https://github.com/arxanas/git-branchless/pull/451 — but I don't actually use stacked PRs on Github, particularly if no one is reviewing my code. Most of my stacking is at work with Phabricator anyways, which handles this better.)
That’s not what I’m saying at all. My comment is a meta comment about why we even need to write grand defenses of these practices in the first place. I’ve been “stacking PRs” forever… when it makes sense to do so. There’s no golden git usage pattern. I’ve seen the industry all the rave about gitflow. And then it wasn’t cool anymore with tags being the preference. Now it’s cool to just forgo branch names entirely? I’ve watched teams fight over whether every PR should be exactly one commit because they don’t realize that you don't always have to squash merge if you want two unrelated changes pulled in at the same time—just keep the commits clean and revert doesn't care. My point is that if you know how to use the tool none of this really matters and all seems quite distracting. I no problem with people sharing what works for them, that’s great (as is this article, abstractly).
I mean, in this case, no one's forcing. More like advocating.
And git is a very flexible tool with a lot of generic functions. Pull requests themselves are a prescriptive style of using those generic functions. A "Branchless" workflow is not the author failing to understand git. It's using those functions differently.
And I fail to see how it's worse that workflow constraints may be placed on a very generic tool like git to work better with more specialized tooling in other places.
"I can put any text in a commit message, so why do I have to start it with this specific text?"
"Because that's how our organization tracks work, and the ability to track historic work in this way is more valuable to us than you being able to write whatever you want at the start of a commit message"
And git is a very flexible tool with a lot of generic functions. Pull requests themselves are a prescriptive style of using those generic functions. A "Branchless" workflow is not the author failing to understand git. It's using those functions differently.
And I fail to see how it's worse that workflow constraints may be placed on a very generic tool like git to work better with more specialized tooling in other places.
"I can put any text in a commit message, so why do I have to start it with this specific text?"
"Because that's how our organization tracks work, and the ability to track historic work in this way is more valuable to us than you being able to write whatever you want at the start of a commit message"
I feel similar about languages and style-guides, but I didn't want to conflate the two in my previous comment. Unless you are a royal newb and just learning to write code (which is fine, everybody has to learn), you’re probably writing code in a way that best expresses your intent or goal given your understanding of the language. Let people do that! If you know the language you can handle encountering different styles and structures. Otherwise you’re just forcing your preferred subset of hoe the language can be used to express things. I just don't like regression to the mean.
This is something I've come to realise as I've matured as a developer. I just don't care what style you write in, if you don't use all the syntactic sugar, etc. I care where you draw your module boundaries, which units depend on which, and if you've handled edge cases, and so on. That's what's going to matter in the end. That's where I'll have to spend a lot of time figuring stuff out. Not syntax.
Exactly. I’ve seen style-perfect lint-free “write once” spaghetti code that drops edge cases and error conditions left and right, and I’ve seen “ugly” but clean code that needs no introduction and works perfectly to boot. I worry that auto-formatters (not to discredit their upsides) do sometimes give people a false sense that they’re writing good code.
I don’t care about styling, and that is exactly why I use (and enforce use of) an autoformatter.
I use auto-formatters so that I don't have to deal with coworkers telling me to add a new line or that I put too many/few levels of indentation.
Formatters do nothing for code quality. Linters can help quality but it's more of a last line of defense for common errors, not a signal that your code is 'good'.
Formatters do nothing for code quality. Linters can help quality but it's more of a last line of defense for common errors, not a signal that your code is 'good'.
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 you have to go in manually resolve the exact same merge conflict all the way through you PR stack in each of the PRs. This gets annoying and hard to work with.
Does anyone have a better way of dealing with this pattern of merge conflict? I've tried using git rerere which in theory sounds helpful but doesn't seem to do anything when I strike this issue.
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 you have to go in manually resolve the exact same merge conflict all the way through you PR stack in each of the PRs. This gets annoying and hard to work with.
Does anyone have a better way of dealing with this pattern of merge conflict? I've tried using git rerere which in theory sounds helpful but doesn't seem to do anything when I strike this issue.
I wish I had a better answer for you but my go-to solution as of late has been the pragmatic "I agree. Do you mind if I fix that in a later PR to avoid spending too much time on it?"
If you've built up some trust with your team, they shouldn't have a problem with it.
If you've built up some trust with your team, they shouldn't have a problem with it.
The linked tool git-branchless handles this pretty well (https://github.com/arxanas/git-branchless, I'm the author). You basically run `git checkout <branch>`, `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 be resolved; you can pass `--merge` to start merge conflict resolution.)
To rebase your commit stack on top of the main branch, use `git sync` instead of `git merge`. Merge commits often make it so that you have to resolve conflicts multiple times.
To rebase your commit stack on top of the main branch, use `git sync` instead of `git merge`. Merge commits often make it so that you have to resolve conflicts multiple times.
Thanks this looks interesting, I will definitely give it a shot.
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...
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...
Mercurial solves this 90% of the way with hg absorb: https://gregoryszorc.com/blog/2018/11/05/absorbing-commit-ch...
Its extremely close to magic. You make all the requested changes on the PR4 and type hg absorb and it'll figure out which commit in your stack each change belongs to.
I've used this to work on lots of stacks 20 commits deep in mercurial.
Its extremely close to magic. You make all the requested changes on the PR4 and type hg absorb and it'll figure out which commit in your stack each change belongs to.
I've used this to work on lots of stacks 20 commits deep in mercurial.
The equivalent to "hg absorb" for Git is https://github.com/torbiak/git-autofixup
huh, shouldn't solving the conflict to pr2 create a merge commit that then also solves the conflict in 3 and 4?
It sounds like the OP might be using a rebase workflow. Such a workflow creates many repetitive conflicts because there is no merge commit to record the resolution point.
Using a rebase workflow with stacked/cascaded PRs is an anti-pattern that trips up people used to other git workflows where the history depth is effectively only one level deep instead of arbitrarily deep as in stacked/cascaded PRs.
Using a rebase workflow with stacked/cascaded PRs is an anti-pattern that trips up people used to other git workflows where the history depth is effectively only one level deep instead of arbitrarily deep as in stacked/cascaded PRs.
You can tell git what the resolution point is using `git rebase --onto`. This can help avoid situations where Git gives extra conflicts form trying to reapply an old version of your change on top of a new version of the same change. You can also use `git rebase --skip` if you recognize that you’ve ended up in this situation.
I think this is something git rerere is supposed to help with
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). After using Gerrit professionally for several years I wonder how I ever lived with GitHub-style pull requests.
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 lot longer. Or is there another workflow that I’m not seeing?
With this sort of system you generally squash your commits together to form one-per-reviewable-thing. Or at least you rewrite things a bit to make it approachable. E.g. I split things apart so one commit is an obviously-correct refactor and the later ones are pure changes.
I understand some of the "no, merges are superior because they retain all history" proponents (not claiming you are one, just aiming to head off that flood), but all those dead ends and refactors along the way are just those - dead ends and refactors. They ruin the ability to bisect, for instance, because they are rarely all functional. Intentional commits, like a stacked workflow supports in a tool like Gerrit, lets you have a wonderfully understandable history that works at every point.
I understand some of the "no, merges are superior because they retain all history" proponents (not claiming you are one, just aiming to head off that flood), but all those dead ends and refactors along the way are just those - dead ends and refactors. They ruin the ability to bisect, for instance, because they are rarely all functional. Intentional commits, like a stacked workflow supports in a tool like Gerrit, lets you have a wonderfully understandable history that works at every point.
“They ruin the ability to bisect” is something I find myself constantly fighting. Just use “—-first-parent” in git log!!
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.
github pull request review process, UI and the whole experience is very much the worst thing about github for me. there's literally one feature that I like (C-g, suggest a one-click committable change in a comment) and everything else is better in gerrit.
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.
Graphite is amazing! Would definitely recommend everyone try it at least :)
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.
Little suspicious of a tool with no pricing information from a company that just raised $20 million. I assume it will not remain free.
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.
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.
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.
Yeah I agree. If you can merge things into a central branch quickly, it’s much better.
In my experience with this sort of process you spend quite a lot of time managing your different branches, especially once you start getting feedback and requests for changes. Then keeping everything in sync. Git helps make this quicker but it’s still effort & cognitive load.
In my experience with this sort of process you spend quite a lot of time managing your different branches, especially once you start getting feedback and requests for changes. Then keeping everything in sync. Git helps make this quicker but it’s still effort & cognitive load.
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.
Okay, I watched this video[0] and I'm sold! I'm going to try it out myself, and it seems like my co-workers wouldn't even have to know that I'm using it. Of course, if it works well I'll definitely advocate for it!
[0]: https://www.youtube.com/watch?v=cE59cfwWL7M
[0]: https://www.youtube.com/watch?v=cE59cfwWL7M
Graphite (https://graphite.dev/) has been a good tool at our startup to manage stacked PRs, I recommend checking them out.
We use GitHub Enterprise where I work now. I do sometimes do stacked PRs but GHE does not make it easy. The rebasing and merge conflict resolution can be a headache although git rebase —onto helps a lot.
I previously worked somewhere that used Phabricator. Its “stacked diffs” worked great. I’d use it all the time when working on complex, multipart changes.
I previously worked somewhere that used Phabricator. Its “stacked diffs” worked great. I’d use it all the time when working on complex, multipart changes.
Try out the linked tool git-branchless (https://github.com/arxanas/git-branchless, I'm the author). It should help you restore Phabricator-like workflows. In particular, check out the `git sync`, `git restack`, and `git move` commands for handling rebasing and conflict resolution.
Unfortunately I cannot install the rust compiler tool chain or other third party binaries on my work laptop :(
You don’t have the ability to install tools on your work computer? That would be a deal breaker for me.
Hope you like merge conflicts and rebase hell as people start to comment and ask for changes
This just shows that people will go to any length to procrastinate PR reviews. The truth is that to be an effective team, reviews need to be highest priority because they are blockers. There is just no way around it.
Any categorical assertion like that feels wrong. Not all projects, all teams, all problems are the same.
Lower work in progress generally results in faster & more predictable delivery.
PR's are an inventory of "almost finished goods". "Stacking" PR's essentially means delaying integration and increasing WIP.
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.
Not at all trivial. If it seems that way to you, then it's likely that you simply haven't worked on a project with the scale and workflows which require this.
Have you considered large successful software projects have been done without any of these modern tools or languages? Or are your projects more complicated than the moon landing?
You seem to be arguing any tools and languages made after the moon landing are trivial unless the project is "more complicated than the moon landing".
Is that the argument intended to make?
Is that the argument intended to make?
Feel the need to observe that we don't need to use PRs at all. Furthermore I see the PR workflow is mostly a cult (which by the way I don't remember anyone asking for -- it seemed to come out of the woodwork either from gitHUB, or from Google). I think it makes some sense in the context of maintenance fixes to a mature release. Perhaps also makes sense in the context of "over the transom" contributions to an OSS project. But for the typical development scenario where most code is green field, and all the developers work in the same organization, it's often just a bad choice of process.
So that's why you get to: stacked PRs and whether or not they're good.
Rewind to the place where you decided to use PR process in the first place and consider if that was a wise choice.
So that's why you get to: stacked PRs and whether or not they're good.
Rewind to the place where you decided to use PR process in the first place and consider if that was a wise choice.
What's an alternative, considering that you still want co-workers to review your code before you ship it?
I like to figure out what all my PRs will be during the planning phase, optimizing for reviewer cognitive load and incremental development of shippable features.
I never need to stack PRs cuz I can work on the next one while the first one is under review and rebase once it's merged. I can see if the review phase is long at your company that this wouldn't work. But I prefer it if possible.
One thing I hate about stacked PR delivery is ppl go dark for a month building this whole new world and if you have architecture concerns in the root PR they will resist them because it means rebasing and changing all the downstream PRs as well. Bad incentives all around.
I never need to stack PRs cuz I can work on the next one while the first one is under review and rebase once it's merged. I can see if the review phase is long at your company that this wouldn't work. But I prefer it if possible.
One thing I hate about stacked PR delivery is ppl go dark for a month building this whole new world and if you have architecture concerns in the root PR they will resist them because it means rebasing and changing all the downstream PRs as well. Bad incentives all around.
You might be interested in https://github.com/gitext-rs/git-stack, which perfectly implements your workflow where you only put up one PR but continue to work locally on the next commits.
> ppl go dark for a month building this whole new world
I think this isn't specific to stacking PRs. You can put 100 commits in a PR and submit it at once, or you can make 100 PRs and submit them all at once, and have the same problems either way. Ideally, you put up your first few commits or PRs early so that they can get reviewed.
> ppl go dark for a month building this whole new world
I think this isn't specific to stacking PRs. You can put 100 commits in a PR and submit it at once, or you can make 100 PRs and submit them all at once, and have the same problems either way. Ideally, you put up your first few commits or PRs early so that they can get reviewed.
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 that aren't yet ready because the upstream PRs haven't been approved yet. You anyways have to (or should) review things stricly serially, so the additional PRs existing are not super useful either. Maybe as context for the reviewer to see the future work.
Also for some reason these tools seem to encourage people to put unrelated changes in the stack that could be based directly on master. Probably because they are anyways working on their stack and integrating some unrelated fix they just did into the current stack is easier than rebasing/changing your current context. But that's just a minor pet peeve.
That said, I would still like a tool that lets me manage stacked branches just for myself. Not exposing them as Github PRs or so, but to organize my work into different branches. Executing a chain of rebases of branch N -> (N-1) can get quite annoying manually. Probably some arcane git magic to (interactively) rebase branch N->(N-1)->...1 in one command exists already.
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 that aren't yet ready because the upstream PRs haven't been approved yet. You anyways have to (or should) review things stricly serially, so the additional PRs existing are not super useful either. Maybe as context for the reviewer to see the future work.
Also for some reason these tools seem to encourage people to put unrelated changes in the stack that could be based directly on master. Probably because they are anyways working on their stack and integrating some unrelated fix they just did into the current stack is easier than rebasing/changing your current context. But that's just a minor pet peeve.
That said, I would still like a tool that lets me manage stacked branches just for myself. Not exposing them as Github PRs or so, but to organize my work into different branches. Executing a chain of rebases of branch N -> (N-1) can get quite annoying manually. Probably some arcane git magic to (interactively) rebase branch N->(N-1)->...1 in one command exists already.
> 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 PRs to your coworkers.
git-branchless itself also lets you manage stacked branches in various ways. For example, you can do `git checkout <branch>`, `git commit --amend`, and then `git restack` to rebase all the descendant branches sensibly. You can use it on the local side of things only and then use Github PRs as normal.
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 PRs to your coworkers.
git-branchless itself also lets you manage stacked branches in various ways. For example, you can do `git checkout <branch>`, `git commit --amend`, and then `git restack` to rebase all the descendant branches sensibly. You can use it on the local side of things only and then use Github PRs as normal.
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.
https://git-ps.sh/
This tool is really nice and related to these types of workflows.
This tool is really nice and related to these types of workflows.
Thanks for the reply and the pointers, very helpful!
https://git-ps.sh/
This git patch stack tool is really nice. You don’t really use branches at all with it. You just build up a stack of atomic commits. At any time, you can request review of a patch, and it will cherry-pick the commit onto a new branch off of master and create a PR for it.
This git patch stack tool is really nice. You don’t really use branches at all with it. You just build up a stack of atomic commits. At any time, you can request review of a patch, and it will cherry-pick the commit onto a new branch off of master and create a PR for it.
Interesting, thanks. Will give it a try!
I'm guilty of doing this in the past, but it seems like an anti-pattern because it attempts to create a local optimum, a big no-no in the Theory of Constraints[1]. Better to find ways to sustainably ease the constraint (code reviewers' time/attention) than to find ways to create more WIP at the constraint.
[1]: https://en.wikipedia.org/wiki/Theory_of_constraints
[1]: https://en.wikipedia.org/wiki/Theory_of_constraints
As someone coming down from a nearly 2 year fight with an engineering manager whose response to every bug was enforcing more code review rules, and every code review backlog was demanding more time allocated for code review, thank you for this comment.
i got a major cognitive dissonance from your comment because stacking PRs is for me the way to get more time and attention from potential reviewers by making their units of work smaller; hopefully small enough to be easily mergable. this is a case of 'more is less' (within reason).
I'm confused by your "major cognitive dissonance" comment.
What you describe appears to reduce variable costs (time spent reviewing) while increasing fixed costs (context-switching). That may increase code reviewer usage, but that doesn't necessarily increase system throughput or reduce WIP.
What you describe appears to reduce variable costs (time spent reviewing) while increasing fixed costs (context-switching). That may increase code reviewer usage, but that doesn't necessarily increase system throughput or reduce WIP.
IME it crucially reduces the time a request spends idle waiting for a reviewer, thus increases total throughput.
Could you suggest some specific alternatives?
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
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
Plugging my own tool: if you like to cultivate a stack of commits, you'll know how awkward git makes it to edit previous commits. With git-prev-next you can run 'git prev 3' and then 'git commit --amend'.
https://github.com/ridiculousfish/git-prev-next
https://github.com/ridiculousfish/git-prev-next
`git rebase -i origin/main` (or HEAD~N or any other base) with the e/edit action on selected commit will do the same with only built in functionality.
Yep, and git-prev-next is built on interactive rebase. It's just a lot faster to run `git prev`, compared to `git rebase -i HEAD^^` and then muck around in a text editor.
The mentioned tool `git-branchless` (I am the author) also supports `git prev 3` followed by `git commit --amend`. See https://github.com/arxanas/git-branchless/wiki/Command:-git-...
It depends on the project being worked on. If the main branch changes often enough that you need to merge master before you merge the branch you’ll spend a lot of time propagating changes.
And then there’s the problem of large codebases. Without a really good cache strategy for builds all of the branch switching and merging will cause a lot of rebuilds. For some languages and large projects that can be 20+ minutes for a fresh build on each switch. That’s enough time to get distracted by emails or hacker news.
I’m a big fan of small commits, often and breaking down changes but it does require a large amount of automation and tooling to support well.
And then there’s the problem of large codebases. Without a really good cache strategy for builds all of the branch switching and merging will cause a lot of rebuilds. For some languages and large projects that can be 20+ minutes for a fresh build on each switch. That’s enough time to get distracted by emails or hacker news.
I’m a big fan of small commits, often and breaking down changes but it does require a large amount of automation and tooling to support well.
One nice feature of git-branchless (I'm the author) is in-memory rebases. For example, the `git sync` command will rebase all local work on top of the main branch without checking out those branches. (It will also not start merge conflict resolution unless you pass `--merge`, which makes it a lot "safer" to frequently rebase your work.) This helps a lot to avoid invalidating builds.
> “Pull Request” has a specific meaning in Git
Aren't "pull requests" a GitHub (and similar systems) thing? As far as I know, Git itself does not have a concept of "pull requests".
Aren't "pull requests" a GitHub (and similar systems) thing? As far as I know, Git itself does not have a concept of "pull requests".
My memory is fuzzy at this point but I'm pretty sure the command has been named `git-request-pull` as long as it's existed. It actually makes less sense on most forges since there's never an explicit pull step to apply them, assuming you use the forge to do approval.
Well this would be impossible to explain and coordinate with the team
Clearly not the case. A very large number of people use these sorts of flows without problems coordinating or explaining.
How would you align this with Jira stories? Our team tries to maintain a one-PR-per-story flow. Partly for QA purposes, we don't want multiple QA cycles per story.
Of course, we still have giant PRs that touch 50-100 files and take forever to code review.
And yet, for those stories in question, they do make it to prod faster than if they were broken apart into multiple 1-2 point stories.
I imagine as always the answer is "invest in better tooling". More robust automated tests, push-button millisecond-deployments, etc...
Of course, we still have giant PRs that touch 50-100 files and take forever to code review.
And yet, for those stories in question, they do make it to prod faster than if they were broken apart into multiple 1-2 point stories.
I imagine as always the answer is "invest in better tooling". More robust automated tests, push-button millisecond-deployments, etc...
Basically: you don't :)
I've only ever found this workflow to work if you can attach multiple commits/code reviews to single higher-level work items in your project management software. There's simply too much overhead if you try to correlate project management items one-to-one with commits/code reviews.
I've only ever found this workflow to work if you can attach multiple commits/code reviews to single higher-level work items in your project management software. There's simply too much overhead if you try to correlate project management items one-to-one with commits/code reviews.
I am the author of the linked tool git-branchless. Happy to answer any questions about stacked PR workflows here or on GitHub discussions https://github.com/arxanas/git-branchless/discussions or Discord https://discord.gg/caYQBJ82A4.
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.
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.
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.
Yup, stacks are wonderful for new features. Typically I run a stack of 5 for new features.
First one is the API/interface with null implementations. Then subsequent ones each implement a method with associated tests
First one is the API/interface with null implementations. Then subsequent ones each implement a method with associated tests
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
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
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
Bzr has a pipelines plug-in which does basically this. Branches can be stacked on top of each other forming a “pipeline”, and propagating changes from one branch to the dependent / subsequent ones is achieved with one command, “bzr pump”.
Bzr has no concept of rebasing, so what you get is a simple merge, but with very little work on your part.
Bzr has no concept of rebasing, so what you get is a simple merge, but with very little work on your part.
Any UI suggestions to make stacked PRs easier? Other than graphite.dev - that needs org access on GitHub.
I believe this goes a bit further and decouples the submission from the review. My team reviews PRs very fast, but why should I wait 15 minutes between submitting and continuing my work. Or what about when I'm working late when no one else is online.
Mentioned tools seem highly underfeatured comparing with stacked git[1].
[1] https://stacked-git.github.io/
[1] https://stacked-git.github.io/
Hi bvrmn, I've been meaning to write a comparison of git-branchless with Stacked Git, as they occupy similar spaces. Can you list some of the features which Stacked Git has which git-branchless might not?
Why not just break up your change into small components, then merge each one into master separately, hiding the change behind a feature flag until the whole thing is done?
Your suggestion comes after the changes are merged. That is, you wouldn't normally merge anything to master without having it reviewed.
Stacked PRs is _how_ you can break up your changes into small components to merge more quickly and block development less.
With stacked PRs the changes are broken into smaller pieces because a later PR can depend on an earlier PR. This means it can be developed while earlier PRs are still under review. It also means each review is smaller and therefore likely to complete more quickly.
Stacked PRs can also work like patch series in the Linux Kernel development where a large change is broken into smaller independent changes done in the logical order, even if their order of discovery was probably reverse. For example, if you discover you need a refactor or bug fix while halfway through developing a feature. In that case you simply insert a PR earlier in the stack/graph which contains just that isolated fix.
In your feature flag example, a common patten would be to have a dozen or so stacked PRs. The early ones do necessary refactors and bug fixes. The middle few add the feature flag and code. The last make the feature flag the default and delete the now obsolete !feature path. Then the prereqequites (which you didn't discover until half-way through) can be reviewed and merged quickly. The meat of the feature is broken up into easily digestible pieces for quick review, but reviewers can get a sense of the entire feature by considering the PR series as a whole; this is especially useful where one PR adds some infrastructure that a later PR uses but both PRs together would be too large to effectively review. As PRs are ready to merge, they can. Often the last couple of PRs which make the feature the default and delete the obsolete code do not merge for quite some time, but they were written when everything was still fresh in the developers head.
Stacked PRs is _how_ you can break up your changes into small components to merge more quickly and block development less.
With stacked PRs the changes are broken into smaller pieces because a later PR can depend on an earlier PR. This means it can be developed while earlier PRs are still under review. It also means each review is smaller and therefore likely to complete more quickly.
Stacked PRs can also work like patch series in the Linux Kernel development where a large change is broken into smaller independent changes done in the logical order, even if their order of discovery was probably reverse. For example, if you discover you need a refactor or bug fix while halfway through developing a feature. In that case you simply insert a PR earlier in the stack/graph which contains just that isolated fix.
In your feature flag example, a common patten would be to have a dozen or so stacked PRs. The early ones do necessary refactors and bug fixes. The middle few add the feature flag and code. The last make the feature flag the default and delete the now obsolete !feature path. Then the prereqequites (which you didn't discover until half-way through) can be reviewed and merged quickly. The meat of the feature is broken up into easily digestible pieces for quick review, but reviewers can get a sense of the entire feature by considering the PR series as a whole; this is especially useful where one PR adds some infrastructure that a later PR uses but both PRs together would be too large to effectively review. As PRs are ready to merge, they can. Often the last couple of PRs which make the feature the default and delete the obsolete code do not merge for quite some time, but they were written when everything was still fresh in the developers head.
I like to do this to address PR feedback, mostly because we use bitbucket, and it's UI for reviewing individual commits sucks.
Another one: git-town
https://www.git-town.com/
https://www.git-town.com/
This article uses the term DAG everywhere, but I couldn't find a definition of it.
Directed Acyclic Graph.
[0]: https://en.wikipedia.org/wiki/Directed_acyclic_graph
[0]: https://en.wikipedia.org/wiki/Directed_acyclic_graph
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.
Currently, one can just do it by never modifying the underlying "branch" (i.e. just adding new commits), but in practice that doesn't always work, as many times you have to rebase against one's master/main branch to pull in changes, which even if no conflicts, will reflow your "base" branch" changing all the commits.
TLDR: Basically, want to be able to state that branch depends on branch (which is currently commit id x) but if I rebase, use whatever the current commit id is for that branch, not whatever it was when I first made the branch.
possible? stupid idea? thoughts?
Currently, one can just do it by never modifying the underlying "branch" (i.e. just adding new commits), but in practice that doesn't always work, as many times you have to rebase against one's master/main branch to pull in changes, which even if no conflicts, will reflow your "base" branch" changing all the commits.
TLDR: Basically, want to be able to state that branch depends on branch (which is currently commit id x) but if I rebase, use whatever the current commit id is for that branch, not whatever it was when I first made the branch.
possible? stupid idea? thoughts?
It feels like roughly the issue is that git thinks in terms of snapshots whereas people often think in terms of patches. Darcs and pijul are version control systems that try to have patches as the fundamental object you operate on instead of snapshots. In particular, there isn’t a rerere operation to change the base of a commit because patches don’t have bases in the same way. However if you’re reviewing code you probably do care about the base because changing the base may change the meaning of the branch and if eg some library function is renamed between when you write/approve a patch and when you merge it, the patch may no-longer be valid.
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...
https://docs.github.com/en/pull-requests/collaborating-with-...
https://docs.gitlab.com/ee/topics/git/git_rebase.html#rebase...
In GitLab this is referred to as "Merge commit with semi-linear history" and it's a checkbox in the general MR settings.
This is one of the big features from git-branchless (https://github.com/arxanas/git-branchless/wiki/Command:-git-...) which is recommended in the article; moving a stack (sub-tree) of commits instead of just one branch.
Might be useful for you if you find the rebase flow to be painful. In particular this can be good for local prototyping where you have a bunch of functional candidates on top of some foundational refactors, and you may be rebasing frequently to refine those foundational commits.
Might be useful for you if you find the rebase flow to be painful. In particular this can be good for local prototyping where you have a bunch of functional candidates on top of some foundational refactors, and you may be rebasing frequently to refine those foundational commits.
Also worth pointing out these commands from git-branchless (I'm the author):
- git sync: rebase all commit stacks on top of the main branch. - git restack: run after making a change to a foundational commit to automatically restack dependent commits.
- git sync: rebase all commit stacks on top of the main branch. - git restack: run after making a change to a foundational commit to automatically restack dependent commits.
> I wish git did 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).
> 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.
You'd still end up fixing all the conflicts your branch made either way, and also breaking the flow of commits if you didn't update the message. So why bother doing all of that instead of stashing your current project, rebase, and force push it? I believe that's supposed to be the standard approach to handle conflict changes in PRs.
You'd still end up fixing all the conflicts your branch made either way, and also breaking the flow of commits if you didn't update the message. So why bother doing all of that instead of stashing your current project, rebase, and force push it? I believe that's supposed to be the standard approach to handle conflict changes in PRs.
Isn't that how rebase already works? It will "apply" commits of the current branch onto commits of the base branch, including new commits that were created in the meantime.
It sounds like you're describing what `git branch --set-upstream-to=$otherBranch` does?
> TLDR: Basically, want to be able to state that branch depends on branch (which is currently commit id x) but if I rebase, use whatever the current commit id is for that branch, not whatever it was when I first made the branch.
As far as I can tell (the “problem” you’re describing is a bit vague) this is already how git works with the sole exception being that git doesn’t default to a particular branch for a rebase?
eg) all you seem to de describing here is a bog-standard:
All it seems like you’re asking for us to drop “feature-branch1” from the rebase?
As far as I can tell (the “problem” you’re describing is a bit vague) this is already how git works with the sole exception being that git doesn’t default to a particular branch for a rebase?
eg) all you seem to de describing here is a bog-standard:
(on feature-branch1)> git checkout -b feature-branch2
(on feature-branch2)> git rebase feature-branch1
All it seems like you’re asking for us to drop “feature-branch1” from the rebase?
think of it this way. When I say
git branch ..., I'm creating a new branch pointer that points to a commit and any future commit will update that branch pointer (not the original).
if I then add a new commit to the original branch, I'll update its branch pointer.
if I do then do a git rebase -i original_branch (on new branch), (I believe) git will look for the common ancestor commit, update the head to original_branch's current id, and then apply individual every commit id from that common ancestor to the head of my what was my new branch.
if I just add a commit to original branch, this is "clean" (i.e. i might have lots of conflicts, but it makes sense, its just effectively inserting a new commit into the middle of the commit stream). However, if I have rebased the original branch, such that common ancestor isn't where it used to be, but much further back in time), it no longer makes sense.
What I was proposing (and what I could do manually), is when i rebase against the proposed type of branch, then we essentially, move the head to the current head of the updated "old branch" and then cherry-pick one by one each commit i made against the new branch (fixing merge errors along the way, just like with rebase).
is this a bit clearer?
git branch ..., I'm creating a new branch pointer that points to a commit and any future commit will update that branch pointer (not the original).
if I then add a new commit to the original branch, I'll update its branch pointer.
if I do then do a git rebase -i original_branch (on new branch), (I believe) git will look for the common ancestor commit, update the head to original_branch's current id, and then apply individual every commit id from that common ancestor to the head of my what was my new branch.
if I just add a commit to original branch, this is "clean" (i.e. i might have lots of conflicts, but it makes sense, its just effectively inserting a new commit into the middle of the commit stream). However, if I have rebased the original branch, such that common ancestor isn't where it used to be, but much further back in time), it no longer makes sense.
What I was proposing (and what I could do manually), is when i rebase against the proposed type of branch, then we essentially, move the head to the current head of the updated "old branch" and then cherry-pick one by one each commit i made against the new branch (fixing merge errors along the way, just like with rebase).
is this a bit clearer?
git-stack [0] tries to do that. I also maintain a list of related tools, some of which do similar [1].
[0] https://github.com/gitext-rs/git-stack
[1] https://github.com/gitext-rs/git-stack/blob/main/docs/compar...
[0] https://github.com/gitext-rs/git-stack
[1] https://github.com/gitext-rs/git-stack/blob/main/docs/compar...
With `git rebase --onto <new_base> <previous_base>` you can achieve this, but it isn't as easy as it could be.
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 "git rebase --update-refs" command-line flag and config option.
(This is from Derrick, not me)
""" This is a feature I've wanted for quite a while. When working on the sparse index topic, I created a long RFC that actually broke into three topics for full review upstream. These topics were sequential, so any feedback on an earlier one required updates to the later ones. I would work on the full feature and use interactive rebase to update the full list of commits. However, I would need to update the branches pointing to those sub-topics.
This series adds a new --update-refs option to 'git rebase' (along with a rebase.updateRefs config option) that adds 'git update-ref' commands into the TODO list. This is powered by the commit decoration machinery.
"""
This is under active development (I don't believe the topic has been merged yet) so it's still open for feedback and refinement on the git development mailing list. Thanks Derrick!
[1] https://public-inbox.org/git/pull.1247.git.1654263472.gitgit...