I'm using mostly this workflow with GitHub, with the main disadvantages being that it's more work on my side, and not obvious to my collaborators. But it does carry the same advantages of allowing reviewers to view diffs with just their feedback incorporated, without breaking `git blame` and `git bisect`.
When I incorporate a reviewer's feedback, I'll commit that with `git commit --fixup <hash of commit that I want to update>`. I'll then push that up and leave a comment reply to the review feedback sharing the fixup commit hash.
Then when the PR is approved and I'm about to merge, I'll do
git rebase --interactive origin/main --autosquash
This will then combine the fixup commits with the correct original commits. I then do a final `git push --force-with-lease` and merge it. (Make sure to note force push before the review is done, because then reviewers lose the ability to see what you added since their last review.)
This relies heavily on autocomplete in my terminal, so that I only have to type `git re<right arrow>` to get to that long command above, for example. And it's a bit clunky, so using a tool that supports and encourages this workflow would be nice.
I was going to say... interactive rebase addresses a lot of the "diff soup" comments that the writer complains about. It's really only done by disciplined engineering teams though (who bother to learn some more advanced features of git)
I realized reading the first part of this article that I often want to set up a sequence of PR's, for very similar reasons as in the begininng of OP. Say, a prefatory refactor, then the main work, then some data cleanup.
When I do this, the problem with rebase is that it kind of breaks the additional "next in sequence" PRs "on top", or at least requires (confusing to me) cleanup in all of them when I rebase the base.
Interactive rebase is fine, I used it probably 50x a day for many years. The problem isn't really with Git here, it's more a bunch of interrelated problems with GitHub's UX, and the fact that many people culturally have never approached the problem in any other way. A lot of places basically just outright copy GitHub's UX, which I think is a good example of this.
For example, using 'git absorb' or autosquash doesn't solve the problem of the review UX just doing badly when you say "What is the difference between the last version of commit Y, and the new version." If those changes include a rebase, it shows you the entire diff including the rebase. That's often not good, but sometimes extremely necessary. (See my other posts in the thread about how Gerrit does this better.) And if you have to do something like rebase, then solve a conflict as a result of the rebase, it is basically unavoidable that you have to do all of them at once if you want the CI to stay clean (which I find important, at least.)
Also, a lot of teams just culturally hate shit like autosquash, or any squash/rebasing at all. Do I think they're misinformed? Yes. Do I think they could use tools that make those flows 1000x better and faster? Yes. But at the end of the day default user interface and design philosophy of one of the most popular software forges in the world does actually matter and have downstream cultural impact. So you have to interrogate that and break it down for people. GitHub does not really encourage, explain, or smooth out workflows like this. People learn it, then hate anything else. (I mean, fucking hell, Git rebase didn't even have --update-refs, a huge QOL improvement for stacked diffs, until like 2022!) So, you often don't have a choice. I've had many places where engineers despised GH force pushes, but only because... GitHub's UX is bad at tracking addressed/reviewed comments on a PR after a force push! They get hung by their own noose, so to speak, and don't even know it.
I do have other problems with Git, but this post is mostly about GitHub/Gitlab style PR review flows. The same problems still happen even with autosquash or tools that accomplish the same thing.
Squashing is nice because it keeps the amount of commits minimal, but as mentioned in the article, it has the problem that now reviewers have to figure out what changed since their last review, which can get hard to do if the tree of changes originally proposed needed updates in dependencies of the commits they reviewed.
I really like the idea that I'm working on a tree (often a stack/list) of changes, and as the review progress I get this cross product of iterations over time like [a-better-way-interdiff-review-aka-git-range-diff](https://gist.github.com/thoughtpolice/9c45287550a56b2047c631...) showcases.
In the end, the time dimension is useless after things get submitted, so the repository only gets the latest state of the change tree committed, which is simple and like the auto-squashed version you mention, but during review reviewers get to see the change tree evolve in an easy way.
Thanks, but I do every now and then also want to rebase without applying the fixup commits yet. Just using shell autocomplete gives me the flexibility to do that with little effort. (And honestly since it already autocompletes if I type `git re`, this nor Git aliases wouldn't even really save me anything anyway.)
Wow, my jaw is on the floor. This is such a good idea! I already had the idea of tracking issues in the same repo as the code -- not that I actually use that idea -- but I didn't have the idea of doing PRs in the repo itself. Love it!
Yes, fixup commits are a good way to approach this, though I don't personally like them, though. I think Sapling's "absorb" command which works on an underlying SCCS weave to automatically absorb changes into the relevant diff is much more elegant. It prompts you with an interactive UI. (For me, it sorta falls into the same space as rebasing a series that has multiple branches on it. You need --update-refs for that. Because otherwise it's like, why am I, the human, doing the work of tracking the graph relationships and manually punching in the commits, and moving the branches, and doing all this bullshit? Computers are good at graphs! Let them handle it!)
There is also a `git absorb`, but it isn't as robust as Sapling's implementation[1].
Really the problem isn't interactive rebase or not. It's mostly a problem of the UX of the review tool itself more than anything, and the kind of "cycle" it promotes. I mentioned it elsewhere here, but fixup commits for example still won't solve the problem of GitHub showing you diffs between baselines, for example, which can absolutely ruin a review if the baseline is large (e.g. you rebased on top of 10 new commits.)
I do have problems with Git's UX beyond this, but the original post is mostly a gripe about GitHub.
100% agree that this is ideal, the way Github does it is completely godawful and it's a tragedy that so many people have it normalized for them.
We did this with Phabricator, although it was a somewhat-manual process, helped along by having some command line macros for updating all the reviews at once. But better still would be an explicit UI for it.
I am the author and used the phrase "Code review is a pretty good idea, in general" in the opening very specifically, because it used be one of the selling points listed on the Phabricator homepage. :) I miss it.
Yes! This is what I imagine in my head as a real code review style, not the stuff Github does. Glad to have a name for it.
I'd add I'd also like my review system to be able to kick patches "out" of the review once they're ready. E.g., the small bugfixes that you make while working on that bigger feature should hopefully be small, isolated patches, ones that are going to find consensus with a reviewer quite quickly. Once that happens … I want to just eject that patch from the entire series & cherry-pick it to main, and rebase the review on the new HEAD. (Or, differently, rebase the latest version of the patch series on main, reordered such that the agreed patch is fist, and then FF main onto that agreed upon patch.)
Essentially, narrow the scope of the review to "the part we're still talking about", but let bug fixes see merging as soon as they're ready.
The argument I'd have against this is "just make that a separate review/PR". But then you get into the hairiness of patchset A depends on patchset B, until B is merged, then it just depends on main.
I keep saying this over and over but, Gerrit basically does that. :) You can see the relationships between any two patches on Gerrit, and more importantly, Gerrit shows you each patch individually. So you can see in a series A -> B -> C that yeah, B is small, let's go ahead and get that in.
Part of this is that UX has some really smart ideas like the "Attention Set". The attention set is basically "Which people need to take the next step?" Like a turn-based game. So, if you just did a review, you're not in the attention set for that patch anymore -- the author is.
That means Gerrit puts it down at the bottom of your queue in the UX. And what's at the top of the queue? Things where you are in the attention set! So it naturally groups things this way.
I didn't get into all the other really annoying papercuts with GitHub's UX, but even the pull request listing is worse than the alternatives. How do you know what state anything is in? You don't, you have to go read the whole thing.
I guess I missed it in your article, and I've never had the opportunity otherwise to use Gerrit. (Since Github is essentially so pervasive. I've only used that, Gitlab, and an internal review system that didn't do interdiff.)
Attention Set <3 - that alone is worth another post. Gerrit really is one of the best kept dev secrets, and if you never had the luck of seeing it in person at a company where you worked at, well...
Makes me wonder what other git or dev-in-general blindspots I have.
Do you know how to create a local branch that tracks a gerrit commit? Usually I just do git commit --amend to update gerrit, but then I lose access to my patch's history (it's still on gerrit, but I want it locally).
That's exactly what Gerrit can do. When you push an x-b-c-d-e chain, these show up stacked in the UI, but you can easily cherry-pick b onto main (see that the CI passes, and the usual review), and rebase everything on top of that. If it is x, the bottom one, you can directly submit it and continue with the others.
All this rebasing sounds like constant pain to pull from Gerrit. Does it actually create new branches v2 v3 after a rebase? Or how do I switch my local checkout to the rebased branch from remote?
It's always exciting to see new approaches to code reviews - GitHub has its strengths, but it’s far from perfect.
For the scenario you’ve outlined, have you thought about splitting the 3 patches into separate, dependent pull requests? While GitHub doesn’t natively support this, the right code review tool (shameless plug - I’m part of a team building one called GitContext) should allow you to keep pull requests small while maintaining dependencies between them. For example, patch 3 can depend on patch 2, which in turn depends on patch 1. The dependency tracking between them - provided by the code review tool - can ensure everything is released in unison if that's required.
Each patch can then be reviewed on its own, making feedback more targeted and easier to respond to. You can even squash commits within a pull request, ensuring a clean commit history with messages that accurately reflect the individual changes. Better still, with the right tool, you can use AI to summarize your pull request and review, streamlining the creation of accurate commit messages without all the manual effort.
A good code review tool also won’t get bogged down by git operations like rebases, merges, or force pushes. Reviewers should always see only the changes since their last review, no matter how many crazy git operations happen behind the scenes. That way, you avoid having to re-review large diffs and can focus on what’s new. The review history stays clean, separate from the commit history.
I'd be curious if this approach to splitting up pull requests and tracking their inter-dependencies would address your needs?
> It's always exciting to see new approaches to code reviews - GitHub has its strengths, but it’s far from perfect.
This is nice sentiment, it's positive reception to an idea and polite to the incumbent.
But it's so thoroughly not a new idea. It's literally the workflow git was designed to support, and is core to many long-standing criticisms about GitHub's approach for as long as GitHub has had pull requests.
And I'm over here wondering why this idea took *checks calendar* over 15 years to graduate from the denigrated mailing list degens and into hip trendy development circles.
I thought we were knowingly choosing shit workflows because we had to support the long-standing refusal by so many software devs to properly learn one of their most-used tools. That's why I chose the tools I chose, and built the workflows I built, when I migrated a company to git. Nobody gets fired for buying IBM after all.
I mean, the answer is simple. Even if email-based flows use range-diff, which is the correct conceptual model, all the actual details of using email are, I would estimate, about 1,000x shittier in 2024 than using GitHub in 2008 when I signed up for the beta as user #3000-something.
Email flows fucking suck ass. Yes I have used them. No, I won't budge on this, and no, I'm not going to go proselytize on LKML or Sourcehut or whatever about it, in Rome I'll just do as the Romans even if I think it sucks. But I've used every strategy you can think of to submit patches, and I can't really blame anyone for not wading through 500 gallons of horrendous bullshit known as the mailing list experience in order to glean the important things from it (like range-diff), even if I'm willing to do it because I have high pain tolerance or am a hired gun for someone's project.
Also, to be fair, Gerrit was released in 2009, and as the creator of ReviewBoard (in this thread!) also noted it supports interdiffs, and supported them for multiple version control backends, was released in 2006! This was not a totally foreign concept, it's just that for reasons, GitHub won, and the defaults chosen by the most popular software forge in history tend to have downstream cultural consequences, both good and bad, as you note.
Nope, none of it was knowingly done, and plenty of teams are almost trivially convertible to the normal workflow, even without inventing a buzzword like TFA did!
Though plenty aren't. I get it. (But one of the magic phrases that really works well is "this is what git, itself, does, and there's a man page installed on your system at this very moment explaining it")
As far as I know, splitting the series into individual PRs only works if you have commit rights to the repository, so you can base one PR on a different branch (in the main repository) than main.
As an outside contributor, with a fork of the repository, your three PRs will incrementally contain change A, A+B, and A+B+C, making the review of the last two PRs harder, because you need to review diffs for code you're already reviewed in another PR.
Not sure about the fork workflow but otherwise it is possible to change the base branch (manually on GH’s web interface) so that you don’t have to see the original branches and review the changes from A to B and from B to C. Maybe this is not possible with fork workflows?
What's the point of keeping track of commits? I honestly never understood people wanting that. Is this for some kind of weird accounting / social-score system where the number of commits decides your yearly bonus?
It's useful to see how the system evolved (because you might want to go back a bit and redo the newer stuff), but it's pointless to see the mistakes made along the way, for example, unless you have some administrative use for that.
Similarly, if a sequence of commits doesn't make sense as committed, but would make better sense if split into a different sequence: then I see no problem doing that. What's the point of keeping history in a bad shape? It's just harder to work with, if it's in a bad shape, but gives no practical advantages.
Why not just do good old mergetrains with pullrequest A points to branch B amd then B points to master, merge B into master and thereafter point A back to master or am I missing the point?
This is called "stacked diffs" and it's a good workflow; the issue is that it's annoying to use on GitHub without tooling. The "point A back to master" bit isn't easy/obvious with pull requests.
EDIT: Also, I'm not sure if this is against the rules, but I also need a new job as of recently. I like working on dev tools and other hard problems. If you liked reading this, want me to make your dev team more productive, or just want to experience and enjoy my excellent (and occasionally eclectic) taste, the email is in my profile.
> or just want to experience and enjoy my excellent (and occasionally eclectic) taste
Thanks for the good laugh :)
Seriously though, your CV is impressive. I hope you'll land well, and quickly. In my (very recent) job hunt experience, the job market is currently mortally ill; the more senior and experienced you are, the more the insane interviewing and HR practices, and the inexplicable rejections, will hurt your soul.
Amazing summary of the mental model of the interdiff review style, and the problems with GitHub's approach to code review. Thank you!
One thing you don't touch upon (yet) is that the diff soup may lead people to prefer the squash merge strategy, to get rid of the "noise" of the fixup commits, which throws away the "good" initial 3 atomic commits as well.
With interdiff review style you're left with the initial 3 commits, and the choice of whether to land them individually or squash them is based entirely on the commits themselves and how atomic they really are.
From my interaction with the free part of GitHub, "diff soup" describes it very well. Does the paid version do anything better? What about GitLab, can this get near Gerrit? And then there are the external services which try to make GitHub less painful (and quite pricey, especially compared to a selfhosted Gerrit), by providing stacked diff support, did you look at these?
No, paying for GH doesn't make the code review experience any better. It's identical across public/cloud/enterprise GH.
I do not know if GitLab does anything different; I've never used it in anger. I'd bet $10 the answer is "no, it's basically just the same as GitHub", though.
If you want a service that adds stacking on top of GitHub, my conclusion after some research is that https://graphite.dev/ is the best option. We did consider it, but it couldn't work in my last job for "reasons" (see my other replies in this thread) but if you've got a shop of Git users and just want to throw money at the problem, I think it's the best choice.
GerritHub is also a possibility, they employ many of the Gerrit devs and know what they're doing, but holy shit the corporate options are expensive out of the gate. It's like $20k/yr minimum regardless of size or number of users.
Honestly, Graphite is cheap as hell considering how much more productive your engineers can be with a good review tool. Gerrit was basically night and day for us. It's not "oh, it pays for itself really quickly in a few days!" You'd probably pay off the monthly cost in less than an hour of actual code review. And you don't even have to opt most of your engineers in; you can trial it where 90% of them use GH and only a subset use graphite and pay.
GitHub does have some stacked diff/"merge train" tools on the deeply paid side (the "Call Us" sorts of Pricing tiers) that I've only seen screenshots and demos of.
On the other side: If you get into the habit of "Reviews" on GitHub, which are in the free part, too, GitHub gives you a quick button for "Review commits since your last Review" under the Commits dropdown in the Files view. That mostly only works if you add commits rather than rebase, hence the complaints about contributing to "diff soup", but it's a reasonably useful workflow and there are workarounds on the "other side" to help deal with "diff soup".
This is why some encourage Squash Merging as the GitHub preferred merge button. Review as a bunch of small commits over time, merge an entire PR to a single final commit.
That said, as an alternative to squash merging, git itself provides some useful tools for dealing with "diff soup" style repositories using real merge commits: `--first-parent`. `git log --first-parent`/`git blame --first-parent`/`git bisect --first-parent` and more give you a "PR top-level view" in your integration branches (such as main branch) without you needing to rebase/squash.
I wish more UIs took a `--first-parent` by default approach, including/especially GitHub's weak commit views (though it is understandable why GitHub pushes you to wanting to use its PRs list instead of commit views by keeping them weak).
What do you think of GitLab set up with Merge Request Dependencies + Squash+Merge merge strategy?
I remember it being pretty easy to have that series of MRs pattern set up. On merging 3x MRs it would be 3x merge commits w/ a single squash commit for each. Regardless of the MR’s commit history.
Tradeoffs are having to merge earlier series branches into later series branches after changes are made during review.
But people can do what they want to the commit history during review. Don’t matter as it just gets squashed.
Been a while since I’ve done this mind, been slumming it with GitHub so I might be looking at it with rose tinted sunglasses.
Maybe it’s better selfhosted, but GitLab is almost unbearably slow. I booted up a Gerrit instance to compare and simply rendering a MR page is maybe 10 seconds vs zero. GitHub is still 10x faster. GitLab manages to be almost that slow for cached pages, making you wait, then realise it’s outdated, and load again, totalling maybe 20 seconds just to “go back to the MR list”. Its awful.
Whatever it is you think you might like about GitLab in theory, it’s much worse when this is your reality. When it takes that long to render a single MR, you do not want to be creating more of them than you have to, and you certainly don’t want to make yourself and the rest of your team navigate between MRs to do code review.
I know this is in jest, but I'll just take the opportunity to respond by posting my favorite poem. The relationship between it and your question -- well, that's for you to decide.
The birds have vanished down the sky.
Now the last cloud drains away.
We sit together, the mountain and me,
until only the mountain remains.
We first built interdiffs in Review Board [https://www.reviewboard.org] way back in 2006 (in fact I think I may have coined the term, or arrived at it independently). It's still my favorite part of the product and my process when doing code reviews. And it's one of the things we hear the most nostalgia for when people move to something like GitHub.
I've never felt that fix-it commits are really a proper alternative, since:
1) They don't tell you what upstream changes have been incorporated into a series of commits.
2) They tend to mess up the commit graph, even if temporarily, and make it more difficult to review. If you've been following along with a review, you may have already read the code being fixed in fix-it commits, but if you're coming in fresh, you may start off with a bad sense of what that code's trying to do or how it's structured.
3) Not everyone uses Git or other multi-commit-capable SCMs. Plenty of people are on Perforce (such as in gamedev) or on specialized SCMs like Keysight SOS (such as chip manufacturers). So fix-it commits aren't even an option there.
A proper interdiff-capable code review systems means one reviewer can start off from the first published review request and follow along with every update, seeing only what's changed, while another can jump in to the latest full change and not have to worry about the series of fix-its that led up to it. And it can do this regardless of the SCM.
If done right, it can also exist alongside multi-commit changes.
Let's say I have a small project I've broken up into multiple commits to help with the review process (say, an API handler, front-end UI, and documentation), and have decided this is suitable for posting as one review request (since the commits are largely interrelated and having these as one change helps lend context to the reviewers — if they aren't, multiple review requests in a dependency chain are probably ideal).
Based on review feedback, I may end up making a series of changes to one or all of those commits. When people go to review my updates, it's nice to be able to see how each piece evolved, without trying to do the mental arithmetic of mapping fix-it commits and their changes to their corresponding changes.
So yes, interdiffs are fantastic! More people should use them, whether they're working with lots of small commits or large commits, single-commit review requests or multi-commit.
That's sick as hell, friend. Actually, I have a second part to this article discussing some of the history and politics of what brought me to these tools.
In about 2013, I migrated the Glasgow Haskell Compiler from "read .patch files on bug reporter" that I joked about, to using Phabricator. For a couple reasons, but at the time one of them was not stacked diffs. It was because GitHub was so bad for review it didn't even have side-by-side diffs! It was a total non-starter for me for those reasons among others.
But that actually wasn't the first time I migrated a team to a code review tool. My first job in 2009 was a very small tight knit team of engineers in a single room in Houston, and I remember thinking it would be really good to get reviews of my code from other people, and to read the things they wrote so I could better understand the codebase. So, the first thing I did in the first few months was pester my manager to set up... ReviewBoard! And we all really liked it.
So I guess this is a way of saying thanks for RB! I still think of it fondly from time to time. And because of it, Code Review has always just been a huge part of my career, almost since day one (and I could still do more of it.)
I love that! Thanks for sharing that with me :) It made my day. Looking forward to the second part! If you're on really any of the current social platforms, I'd love to connect. I'm chipx86 everywhere.
I still develop Review Board full-time with a small team :) The development world has changed a lot, and much of the world has converged on GitHub (it's a hard market to be in right now), so we still look for opportunities to build review capabilities that target problems people have that aren't being addressed elsewhere.
Just as an example, we launched PDF Review and diffing a while back, which we see companies use for things like industrial designs and schematics. It's neat, we actually diff two rendered PDFs without just converting them into text files first, like you usually see. Following that up with full Office Document Review soon.
Dark mode finally shipped earlier this year (I should write about that endeavor sometime). And there's a couple of super-neat code review capabilities we've come up with that nobody's doing, which I'm keeping under wraps for next year. I think they're going to be pretty awesome.
I've generally found that code review first, and rebase-centric systems like Gerrit tend to be much easier to review code in. One of the best parts of this is native support for stacking multiple patches, so people make smaller patches that are easier to review.
Code review in Github feels like a bad afterthought - the space-wasting interface that looks more like a forum thread, the inability to track over rebases, etc.
There is one thing I miss on Gerrit when you push a stack of commits: A central place to talk about the whole of the stack, not just individual commits. This "big picture", but still technical stuff, too often happens in the issue tracker. But where to place it, I have no idea. This stack is just too ephemeral and and can be completely different on the next push.
I've heard people argue for this strategy a few times but I am not convinced. Most projects I work on have feature branches that get squashed into a single commit (erasing the history of the branch). If we even have the case described by the author we would just do the three steps (refactor, new api, update) as 3 commits.
One thing that has been a solid practice for me is to avoid long-lived branches. That seems to be what leads to this kind of multi-stage commit scenario. Someone wants to work for several days (or worse, weeks) on a feature and then drop the whole thing all at once. I strongly prefer to move things into main every day (or multiple times a day). One way to do this is to use feature flags to allow work-in-progress commits to land without causing problems. This also has the advantage (if systems are set up correctly) to enable on dev and staging environments for testing. It is also a hop-and-skip jump away from blue/green rollouts.
I don't want ways to make big commits easier to review. I want to force the team to commit small changes early and often. I understand not everyone agrees.
The people I know who prefer stacked diffs argue that it makes it easier to integrate changes faster, no matter the size. Part of this is because unlike a PR on GitHub, you can land parts of a stack: to use the example from the article, if the "small refactor" diff is good to go, it can be landed without landing the "new api" and "migrate API users" diffs. Centering commits rather than branches has the effect of making for smaller overall commits rather than long-lived branches.
I'm not sure I understand your exact logic, so let me talk through a simplified scenario for arguments sake. Imagine each change takes 1 day. Imagine the team releases code to production every day. On Monday a developer picks up the task, does the refactor, puts up a PR and the refactor is shipped Tuesday morning. Tuesday he works on the api and it ships Wednesday. Wednesday he finishes the migration which ships on Thursday.
In the alternate scenario the entire stacked commits go out together on Thursday after the entire 3 days of work is completed. So I do not see how it would make it easier to integrate changes faster. This problem becomes worse, as you can imagine, if the second task in the stack takes a week vs. a day. I believe this kind of logic can be extended to any timescale.
> have feature branches that get squashed into a single commit (erasing the history of the branch)
Terrible.
It makes git-blame and git-bisect essentially unusable.
If you have a regression, git-bisect can help you narrow it down to a single patch. Because of that, you want to have fifty 160-line patches in the git history, for a particular feature, rather than one 8000 line patch.
If a given line of code looks fishy, you want git-blame (or a series of git-blame commands) to lead you to a 160-line commit, with its own detailed commit message, rather than to a 8000-line commit.
You also want to preserve the order of the original commits. Just reading through the individual commit messages, in order, a few years later, can be super helpful for understanding the original design. (Of course the original patch set has to be constructed in dependency order; it needs to compile at every stage, and so on. That's a separate development step that comes on top of just implementing the functionality. The code must be presented in logical stages as well.)
Readit News
When I incorporate a reviewer's feedback, I'll commit that with `git commit --fixup <hash of commit that I want to update>`. I'll then push that up and leave a comment reply to the review feedback sharing the fixup commit hash.
Then when the PR is approved and I'm about to merge, I'll do
This will then combine the fixup commits with the correct original commits. I then do a final `git push --force-with-lease` and merge it. (Make sure to note force push before the review is done, because then reviewers lose the ability to see what you added since their last review.)This relies heavily on autocomplete in my terminal, so that I only have to type `git re<right arrow>` to get to that long command above, for example. And it's a bit clunky, so using a tool that supports and encourages this workflow would be nice.
But given that I'm stuck with GitHub, it's OK.
When I do this, the problem with rebase is that it kind of breaks the additional "next in sequence" PRs "on top", or at least requires (confusing to me) cleanup in all of them when I rebase the base.
For example, using 'git absorb' or autosquash doesn't solve the problem of the review UX just doing badly when you say "What is the difference between the last version of commit Y, and the new version." If those changes include a rebase, it shows you the entire diff including the rebase. That's often not good, but sometimes extremely necessary. (See my other posts in the thread about how Gerrit does this better.) And if you have to do something like rebase, then solve a conflict as a result of the rebase, it is basically unavoidable that you have to do all of them at once if you want the CI to stay clean (which I find important, at least.)
Also, a lot of teams just culturally hate shit like autosquash, or any squash/rebasing at all. Do I think they're misinformed? Yes. Do I think they could use tools that make those flows 1000x better and faster? Yes. But at the end of the day default user interface and design philosophy of one of the most popular software forges in the world does actually matter and have downstream cultural impact. So you have to interrogate that and break it down for people. GitHub does not really encourage, explain, or smooth out workflows like this. People learn it, then hate anything else. (I mean, fucking hell, Git rebase didn't even have --update-refs, a huge QOL improvement for stacked diffs, until like 2022!) So, you often don't have a choice. I've had many places where engineers despised GH force pushes, but only because... GitHub's UX is bad at tracking addressed/reviewed comments on a PR after a force push! They get hung by their own noose, so to speak, and don't even know it.
I do have other problems with Git, but this post is mostly about GitHub/Gitlab style PR review flows. The same problems still happen even with autosquash or tools that accomplish the same thing.
I really like the idea that I'm working on a tree (often a stack/list) of changes, and as the review progress I get this cross product of iterations over time like [a-better-way-interdiff-review-aka-git-range-diff](https://gist.github.com/thoughtpolice/9c45287550a56b2047c631...) showcases. In the end, the time dimension is useless after things get submitted, so the repository only gets the latest state of the change tree committed, which is simple and like the auto-squashed version you mention, but during review reviewers get to see the change tree evolve in an easy way.
There is also a `git absorb`, but it isn't as robust as Sapling's implementation[1].
Really the problem isn't interactive rebase or not. It's mostly a problem of the UX of the review tool itself more than anything, and the kind of "cycle" it promotes. I mentioned it elsewhere here, but fixup commits for example still won't solve the problem of GitHub showing you diffs between baselines, for example, which can absolutely ruin a review if the baseline is large (e.g. you rebased on top of 10 new commits.)
I do have problems with Git's UX beyond this, but the original post is mostly a gripe about GitHub.
[1] There is an example in this GitHub issue that captures the difference between the two underlying algorithms: https://github.com/martinvonz/jj/issues/170
Note that the workflow I described is indeed mostly a workaround for people, like me, stuck with GitHub.
If you do it everyday for years, sure you remember most of them.
But for weekend hackers or those who specialise in some other areas, it creates a lot of frustration.
I often forget all these flags etc....
My brother made this https://github.com/zerocorebeta/Option-K
This enables me to simply write in thermal, "interactive rebase main, auto squash.
And then I hit option+k and it replaces it with the above command.
We did this with Phabricator, although it was a somewhat-manual process, helped along by having some command line macros for updating all the reviews at once. But better still would be an explicit UI for it.
I swear the Herald selling points also had keeping tabs on the pesky interns, or something to that effect.
Maybe one day I'll be able to use Gerrit (it sounds great), and then I can be only annoyed at Jira.
Deleted Comment
I'd add I'd also like my review system to be able to kick patches "out" of the review once they're ready. E.g., the small bugfixes that you make while working on that bigger feature should hopefully be small, isolated patches, ones that are going to find consensus with a reviewer quite quickly. Once that happens … I want to just eject that patch from the entire series & cherry-pick it to main, and rebase the review on the new HEAD. (Or, differently, rebase the latest version of the patch series on main, reordered such that the agreed patch is fist, and then FF main onto that agreed upon patch.)
Essentially, narrow the scope of the review to "the part we're still talking about", but let bug fixes see merging as soon as they're ready.
The argument I'd have against this is "just make that a separate review/PR". But then you get into the hairiness of patchset A depends on patchset B, until B is merged, then it just depends on main.
Part of this is that UX has some really smart ideas like the "Attention Set". The attention set is basically "Which people need to take the next step?" Like a turn-based game. So, if you just did a review, you're not in the attention set for that patch anymore -- the author is.
That means Gerrit puts it down at the bottom of your queue in the UX. And what's at the top of the queue? Things where you are in the attention set! So it naturally groups things this way.
I didn't get into all the other really annoying papercuts with GitHub's UX, but even the pull request listing is worse than the alternatives. How do you know what state anything is in? You don't, you have to go read the whole thing.
Makes me wonder what other git or dev-in-general blindspots I have.
For the scenario you’ve outlined, have you thought about splitting the 3 patches into separate, dependent pull requests? While GitHub doesn’t natively support this, the right code review tool (shameless plug - I’m part of a team building one called GitContext) should allow you to keep pull requests small while maintaining dependencies between them. For example, patch 3 can depend on patch 2, which in turn depends on patch 1. The dependency tracking between them - provided by the code review tool - can ensure everything is released in unison if that's required.
Each patch can then be reviewed on its own, making feedback more targeted and easier to respond to. You can even squash commits within a pull request, ensuring a clean commit history with messages that accurately reflect the individual changes. Better still, with the right tool, you can use AI to summarize your pull request and review, streamlining the creation of accurate commit messages without all the manual effort.
A good code review tool also won’t get bogged down by git operations like rebases, merges, or force pushes. Reviewers should always see only the changes since their last review, no matter how many crazy git operations happen behind the scenes. That way, you avoid having to re-review large diffs and can focus on what’s new. The review history stays clean, separate from the commit history.
I'd be curious if this approach to splitting up pull requests and tracking their inter-dependencies would address your needs?
This is nice sentiment, it's positive reception to an idea and polite to the incumbent.
But it's so thoroughly not a new idea. It's literally the workflow git was designed to support, and is core to many long-standing criticisms about GitHub's approach for as long as GitHub has had pull requests.
And I'm over here wondering why this idea took *checks calendar* over 15 years to graduate from the denigrated mailing list degens and into hip trendy development circles.
I thought we were knowingly choosing shit workflows because we had to support the long-standing refusal by so many software devs to properly learn one of their most-used tools. That's why I chose the tools I chose, and built the workflows I built, when I migrated a company to git. Nobody gets fired for buying IBM after all.
Email flows fucking suck ass. Yes I have used them. No, I won't budge on this, and no, I'm not going to go proselytize on LKML or Sourcehut or whatever about it, in Rome I'll just do as the Romans even if I think it sucks. But I've used every strategy you can think of to submit patches, and I can't really blame anyone for not wading through 500 gallons of horrendous bullshit known as the mailing list experience in order to glean the important things from it (like range-diff), even if I'm willing to do it because I have high pain tolerance or am a hired gun for someone's project.
Also, to be fair, Gerrit was released in 2009, and as the creator of ReviewBoard (in this thread!) also noted it supports interdiffs, and supported them for multiple version control backends, was released in 2006! This was not a totally foreign concept, it's just that for reasons, GitHub won, and the defaults chosen by the most popular software forge in history tend to have downstream cultural consequences, both good and bad, as you note.
Though plenty aren't. I get it. (But one of the magic phrases that really works well is "this is what git, itself, does, and there's a man page installed on your system at this very moment explaining it")
As an outside contributor, with a fork of the repository, your three PRs will incrementally contain change A, A+B, and A+B+C, making the review of the last two PRs harder, because you need to review diffs for code you're already reviewed in another PR.
- Each commit can be reviewed on its own
- Dependency tracking between commits? Yes...
- They don’t have AI built-in (a good thing)
- You can have an interdiff between commits unlike PRs
Commits are the bread and butter of Git. Just use commits.
As for GitContext, how do you keep track of commits across fixups, rebases, reordering, etc.?
It's useful to see how the system evolved (because you might want to go back a bit and redo the newer stuff), but it's pointless to see the mistakes made along the way, for example, unless you have some administrative use for that.
Similarly, if a sequence of commits doesn't make sense as committed, but would make better sense if split into a different sequence: then I see no problem doing that. What's the point of keeping history in a bad shape? It's just harder to work with, if it's in a bad shape, but gives no practical advantages.
EDIT: Also, I'm not sure if this is against the rules, but I also need a new job as of recently. I like working on dev tools and other hard problems. If you liked reading this, want me to make your dev team more productive, or just want to experience and enjoy my excellent (and occasionally eclectic) taste, the email is in my profile.
Thanks for the good laugh :)
Seriously though, your CV is impressive. I hope you'll land well, and quickly. In my (very recent) job hunt experience, the job market is currently mortally ill; the more senior and experienced you are, the more the insane interviewing and HR practices, and the inexplicable rejections, will hurt your soul.
A friend of mine sent me the following links:
https://danluu.com/hiring-lemons/
https://danluu.com/programmer-moneyball/
https://danluu.com/algorithms-interviews/
Good luck!
One thing you don't touch upon (yet) is that the diff soup may lead people to prefer the squash merge strategy, to get rid of the "noise" of the fixup commits, which throws away the "good" initial 3 atomic commits as well.
With interdiff review style you're left with the initial 3 commits, and the choice of whether to land them individually or squash them is based entirely on the commits themselves and how atomic they really are.
I do not know if GitLab does anything different; I've never used it in anger. I'd bet $10 the answer is "no, it's basically just the same as GitHub", though.
If you want a service that adds stacking on top of GitHub, my conclusion after some research is that https://graphite.dev/ is the best option. We did consider it, but it couldn't work in my last job for "reasons" (see my other replies in this thread) but if you've got a shop of Git users and just want to throw money at the problem, I think it's the best choice.
GerritHub is also a possibility, they employ many of the Gerrit devs and know what they're doing, but holy shit the corporate options are expensive out of the gate. It's like $20k/yr minimum regardless of size or number of users.
Honestly, Graphite is cheap as hell considering how much more productive your engineers can be with a good review tool. Gerrit was basically night and day for us. It's not "oh, it pays for itself really quickly in a few days!" You'd probably pay off the monthly cost in less than an hour of actual code review. And you don't even have to opt most of your engineers in; you can trial it where 90% of them use GH and only a subset use graphite and pay.
On the other side: If you get into the habit of "Reviews" on GitHub, which are in the free part, too, GitHub gives you a quick button for "Review commits since your last Review" under the Commits dropdown in the Files view. That mostly only works if you add commits rather than rebase, hence the complaints about contributing to "diff soup", but it's a reasonably useful workflow and there are workarounds on the "other side" to help deal with "diff soup".
This is why some encourage Squash Merging as the GitHub preferred merge button. Review as a bunch of small commits over time, merge an entire PR to a single final commit.
That said, as an alternative to squash merging, git itself provides some useful tools for dealing with "diff soup" style repositories using real merge commits: `--first-parent`. `git log --first-parent`/`git blame --first-parent`/`git bisect --first-parent` and more give you a "PR top-level view" in your integration branches (such as main branch) without you needing to rebase/squash.
I wish more UIs took a `--first-parent` by default approach, including/especially GitHub's weak commit views (though it is understandable why GitHub pushes you to wanting to use its PRs list instead of commit views by keeping them weak).
I remember it being pretty easy to have that series of MRs pattern set up. On merging 3x MRs it would be 3x merge commits w/ a single squash commit for each. Regardless of the MR’s commit history.
Tradeoffs are having to merge earlier series branches into later series branches after changes are made during review.
But people can do what they want to the commit history during review. Don’t matter as it just gets squashed.
Been a while since I’ve done this mind, been slumming it with GitHub so I might be looking at it with rose tinted sunglasses.
Whatever it is you think you might like about GitLab in theory, it’s much worse when this is your reality. When it takes that long to render a single MR, you do not want to be creating more of them than you have to, and you certainly don’t want to make yourself and the rest of your team navigate between MRs to do code review.
I've never felt that fix-it commits are really a proper alternative, since:
1) They don't tell you what upstream changes have been incorporated into a series of commits.
2) They tend to mess up the commit graph, even if temporarily, and make it more difficult to review. If you've been following along with a review, you may have already read the code being fixed in fix-it commits, but if you're coming in fresh, you may start off with a bad sense of what that code's trying to do or how it's structured.
3) Not everyone uses Git or other multi-commit-capable SCMs. Plenty of people are on Perforce (such as in gamedev) or on specialized SCMs like Keysight SOS (such as chip manufacturers). So fix-it commits aren't even an option there.
A proper interdiff-capable code review systems means one reviewer can start off from the first published review request and follow along with every update, seeing only what's changed, while another can jump in to the latest full change and not have to worry about the series of fix-its that led up to it. And it can do this regardless of the SCM.
If done right, it can also exist alongside multi-commit changes.
Let's say I have a small project I've broken up into multiple commits to help with the review process (say, an API handler, front-end UI, and documentation), and have decided this is suitable for posting as one review request (since the commits are largely interrelated and having these as one change helps lend context to the reviewers — if they aren't, multiple review requests in a dependency chain are probably ideal).
Based on review feedback, I may end up making a series of changes to one or all of those commits. When people go to review my updates, it's nice to be able to see how each piece evolved, without trying to do the mental arithmetic of mapping fix-it commits and their changes to their corresponding changes.
So yes, interdiffs are fantastic! More people should use them, whether they're working with lots of small commits or large commits, single-commit review requests or multi-commit.
In about 2013, I migrated the Glasgow Haskell Compiler from "read .patch files on bug reporter" that I joked about, to using Phabricator. For a couple reasons, but at the time one of them was not stacked diffs. It was because GitHub was so bad for review it didn't even have side-by-side diffs! It was a total non-starter for me for those reasons among others.
But that actually wasn't the first time I migrated a team to a code review tool. My first job in 2009 was a very small tight knit team of engineers in a single room in Houston, and I remember thinking it would be really good to get reviews of my code from other people, and to read the things they wrote so I could better understand the codebase. So, the first thing I did in the first few months was pester my manager to set up... ReviewBoard! And we all really liked it.
So I guess this is a way of saying thanks for RB! I still think of it fondly from time to time. And because of it, Code Review has always just been a huge part of my career, almost since day one (and I could still do more of it.)
I still develop Review Board full-time with a small team :) The development world has changed a lot, and much of the world has converged on GitHub (it's a hard market to be in right now), so we still look for opportunities to build review capabilities that target problems people have that aren't being addressed elsewhere.
Just as an example, we launched PDF Review and diffing a while back, which we see companies use for things like industrial designs and schematics. It's neat, we actually diff two rendered PDFs without just converting them into text files first, like you usually see. Following that up with full Office Document Review soon.
Dark mode finally shipped earlier this year (I should write about that endeavor sometime). And there's a couple of super-neat code review capabilities we've come up with that nobody's doing, which I'm keeping under wraps for next year. I think they're going to be pretty awesome.
Code review in Github feels like a bad afterthought - the space-wasting interface that looks more like a forum thread, the inability to track over rebases, etc.
One thing that has been a solid practice for me is to avoid long-lived branches. That seems to be what leads to this kind of multi-stage commit scenario. Someone wants to work for several days (or worse, weeks) on a feature and then drop the whole thing all at once. I strongly prefer to move things into main every day (or multiple times a day). One way to do this is to use feature flags to allow work-in-progress commits to land without causing problems. This also has the advantage (if systems are set up correctly) to enable on dev and staging environments for testing. It is also a hop-and-skip jump away from blue/green rollouts.
I don't want ways to make big commits easier to review. I want to force the team to commit small changes early and often. I understand not everyone agrees.
In the alternate scenario the entire stacked commits go out together on Thursday after the entire 3 days of work is completed. So I do not see how it would make it easier to integrate changes faster. This problem becomes worse, as you can imagine, if the second task in the stack takes a week vs. a day. I believe this kind of logic can be extended to any timescale.
Terrible.
It makes git-blame and git-bisect essentially unusable.
If you have a regression, git-bisect can help you narrow it down to a single patch. Because of that, you want to have fifty 160-line patches in the git history, for a particular feature, rather than one 8000 line patch.
If a given line of code looks fishy, you want git-blame (or a series of git-blame commands) to lead you to a 160-line commit, with its own detailed commit message, rather than to a 8000-line commit.
You also want to preserve the order of the original commits. Just reading through the individual commit messages, in order, a few years later, can be super helpful for understanding the original design. (Of course the original patch set has to be constructed in dependency order; it needs to compile at every stage, and so on. That's a separate development step that comes on top of just implementing the functionality. The code must be presented in logical stages as well.)