Accepting a messy git history
16 points by qznc
16 points by qznc
I really like stacked PRs, but not for any reason mentioned in the blog post. Personally, I think stacked PRs are significantly easier to get reviews and approvals for, and to get merged.
A single PR with multiple commits is hard to review because the tooling sucks. GitHub is really awful at separating this, and it's not immediately obvious a PR should be reviewed commit-by-commit, or if the commits are all WIP and you should review the resulting PR. God forbid someone comments on a change at the bottom of your commit chain, and you have to git rebase. I've messed this up a few times. I used to amend my commit chain to address PR feedback, but if you mess that up and force-push, you can't undo it because you lost history. You can make more commits, but if your PR has a non-trivial amount of feedback, the commit chain grows too long. Even worse, now people are reviewing commits that are stale because you've addressed things in subsequent commits! Stacked PRs solves reviewing with a reasonable user experience.
I often need reviewers for different parts of the code. I can separate a larger change into a stack of 3-4 smaller changes, and each team can review their smaller section.
People will easily approve a PR if they can review it within 5 minutes, and are willing to push up to 15 minutes. Anything that takes more time than that people will pass over. The review time is not proportional to size of PR. Larger PRs take WAY more time because there’s so much more jumping around and keeping things in mind with bigger PRs.
Some stacks cannot be merged all at once, like a squash merge. For example, a DB migration or some infra changes may need to be merged and deployed before dependent changes can be merged and deployed. As such, you need to merge one PR and then another, but it's very helpful to have your top PR be fully testable so you know everything fits together nicely.
The main flaw I see is that if you have to git revert, you might need to do so on a stack of merge commits instead of a single merge commit. Additionally, I have to be extra diligent at explaining that I'm using a stack of PRs that all tie together into a larger story, but that's just for the history. But the number of times I run into these “history” problems is extremely eclipsed by the number of times I need reviews on a PR.
Yeah same. It’s not about commit history hygiene at all, for me. I’m a stacked pr fanatic and I think all talk of commit history is a huge distraction.
And at the companies where this is standard practice, it’s all about code review there too.
I had always thought of stacked PRs in terms of long-lived branches that aren’t ready to be merged just yet, with more work branching off them; and thus wondered, “how often do you actually need that?”—because I don’t feel like such a thing would have been useful to me very often.
You present it as a way of splitting the review into multiple pieces, when a logical piece of work needs review from multiple teams.
Have I got that correct? Still seems a bit wonky, and really an indictment of GitHub’s PRs rather than praise for stacked PRs as such. But at least I can finally understand a more common purpose of stacked PRs.
Yes, you got that correct. It goes further, though! Even when stacked PRs are reviewed by the same person, I prefer them because they support incremental progress towards full approval.
As a reviewer, I'll often look at a stack of PRs and approve one or two before I need to go to my next meeting; then I'll come back to the stack an hour or two later and can continue where I left off. On traditional PRs, I've never seen the ability to mark a commit within a chain of commits as approved. And in a big PR, I can mark a file as viewed, but it takes more work to pull apart what I've already looked at versus what I should go back to.
For example, I recently deduplicated code that was copy-pasted ~12 times. My first PR moved one instance of the code into a library function and called it directly. Subsequent PRs fixed one place at a time, and slightly adjusted the library function to accommodate any new functionality needed. A reviewer could look at any individual PR and say "this makes sense" within one minute because they only need to look at two files in any given PR.
It spreads out the "dopamine" of approving PRs. Approving a single PR in a stack feels a lot more meaningful in a way that a chain of commits and big PRs don't.
My PR stacks live anywhere from 1 day to 1 week. I don't use them for long-lived projects. I strongly believe in developing in trunk.
For me, stacked PRs make the "curated commit history" option realistically viable, but it's the review part itself that is the core benefit.
I tend to see commit messages in a PR as the story I tell the reviewer to make it easier for them to understand what I'm doing and why.
We use squash and merge to make sure things stay sensible on a repo level. That also means that a rollback/revert is much more likely to work on a single commit.
I'm not sure about you folks, but I can count on a single palm how many times I actually looked at git history to understand the state of things. Usually I live with the code as it changes (doesn't matter if I did these changes or not). If I don't, then it might be too big to catch anyway.
If you want a "clean history" to see what happened, then accept that sometimes things happened parallel, you know, because of the team structure/work schedule. There were typos, missed shots, all contributing to where the source is at.
Plus, how much time back a git history is useful? 3 months, 3 years? Do you walk forward or back? Forward walking needs you to understand approaches maybe long replaced, backward faces you with an already complete product which you need to understand before you can start reading.
I usually let the team choose whatever they want to the point it is not obstructing my way and if it does despite, I'll hand it out to whoever chosen that approach.
I'm constantly looking at it -- usually, there are hints about why something is the way it is, and I need to understand that in order to know if a change I'm making sensible, or whether there's additional context I need to consider. It's very common that the weird behavior was done for a reason I hadn't considered, and I need to revise my changes to take it into account.
I'm not sure about you folks, but I can count on a single palm how many times I actually looked at git history to understand the state of things. Usually I live with the code as it changes (doesn't matter if I did these changes or not). If I don't, then it might be too big to catch anyway.
Okay. Well, I look at git blame at least once a week, and often more than that. It is very disappointing when I see projects wreck their history, especially in the LLM age when maintaining a clean history is easier than ever.
Plus, how much time back a git history is useful? 3 months, 3 years?
Try 30 years?
I tend not to look at the history too much, but other people at $WORK depend on it to a point that I didn’t really understand until I read the comments here.
The thing is, when I build a system, it will be composed of a set of bounded contexts, [each] with a small number of very well defined interfaces, extremely low external coupling, minimal dependencies and relatively simple transformations. A side effect of this approach is that the code is reasonably stable over long periods of time and changes tend to be focussed and limited in scope.
But I have recently inherited a bajillion lines of very tightly coupled spaghetti, and to my surprise, my new team often uses git to consider the “intent” of the developer when they made a change many years ago. Because the software itself is poorly organised, with lots of accidental complexity, git is effectively a database of similarly complex decisions over time.
My point is that the dichotomy presented in this article really helped me to see this for the first time. I thought I was doing git wrong, because I haven’t needed this until now, but that’s maybe not the full story.
I wonder if there is a connection between the complexity of the code base in a developer’s early years, and what they prefer.
(As an aside, I think minimising accidental complexity is a much better use of time than years of trying to deal with it later, but it’s hard to do and even harder to explain)
Edit: added [each] for clarity
I absolutely use it less as a contributor and more as a consumer of software. It's not at the top of my list of debugging steps, but a decent way down after discovering that upgrading a piece of software breaks some kind of workflow is checking out the repo and doing git log last-working-tag..first-broken-tag to see what might have caused the issue.
Even better if you know it's a specific subsystem, using the linux kernel as an example git log v6.17..v6.18 -- fs/ext4 lets me browse through all the changes under that dir. Followed by rebuilding the software with some of those commits backed out almost points at a smoking gun.
As much as it pains me to say it: I agree. One core downside is it’s bad practice to force push so if something slips into your pristine git history like a wip commit it’s basically game over. For example if you or a contributor forgot to do a conventional commit and it’s on the remote … now what?
I do read my commit log and sometimes you want small commits for the occasional audit, rollback, or bisect, but those are so rare.
a wip commit it’s basically game over.
Why? It's fine to slip up occasionally. It doesn't have to be perfect to be valuable.
You can protect your trunk from commits that do not match a certain pattern. You can't fix history, but you can prevent from repeating it.
Now, I have a wip commit in my history. As long as it doesn't blow away all of the history or touch every line in every file, what's the problem?
If a clean history is not enforced very strictly, then the git history will become a mess.
I feel like I might be missing something; if you're using GitHub isn't it very easy to enforce at a repository level by only allow squash merging? This is what we've done in our repo and even with a reasonably high PR rate, the git history is completely clean.
I think it depends what your definition of clean is. GitHub's squash mode does add some sort of minimum bound by automatically "fixing" the egregious cases (dozens of "fix" commits).
By using the PR title/description as the commit message it at least uses something the reviewer probably glanced at (As long as you don't have funny templates or tools that modify the description it probably isn't too messy). It also gets rid of any fixup and squash commits.
However it is still common to have massive commits that would be better split and the review part of the PR UI doesn't emphasize the PR description (and for example there is no good way to comment on it. Best option is start a discussion thread on a random file/code line). There are also various tools and templates that inject garbage into the commit description.
But this is basically the route I take. I tell team members that if they aren't going to manage their history just do squash merges and I will consider it good enough.
Agreed. Before stacked PRs, enforced squashing was unwieldy for rebase-people (borrowing OP's term).
With stacked PRs, the rebase-people can always do 1-commit PRs, as granular as we like. Commit-people are also satisfied. Enforced squashing is clearly a win-win now.
Now that you mention it, "clean" is not a good word here. Squash merges may look clean but it is superficial.
Maybe I should have used "neat" or "tidy"? Like cutlery being sorted properly, not just squashed together.
I would like to add to what kevincox said from my own work experience.
I may be the only on my team using jj so my PRs are usually just one commit. The rest of team, using git, usually push from two to undetermined commits to the same PR; that's why, when merged to main, commits are automatically (i.e. forced) squashed.
So, recently, we did a refactor to remove some legacy code. I took care of the front end, and had to restart the task a couple of times because once I finished, the tests would fail. Then I took another approach. First I removed some code that would not change functionality and see if it test passed; if so, I created a new jj change. By the moment I was ready to merge, I had three commits in which tests passed and the production site would not break. After my PR was merged, they noticed that we had to revert part of what I done. In an ideal world (or work), I could have checked main and reverted the specific commit, but since it was squashed, getting a usable diff was impossible (and also the commit modified hundred of mock test files).
Thanks to some superior deity, my old commits were still in my original feature branch; they cherry picked my commits to create a new integration branch. So I sold my soul to an evil deity and told the AI, "Here you have the main branch and here's a git diff of a commit that was squashed. Untangle it to revert what that commit did". The AI managed to do the task and the PR was merged.
So, the lesson is, never force squashing.
So, the lesson is, never force squashing.
Actually the lesson is that you really want some kind of stacked diff/PR support, isn't it? I recently got a flawless 29-commit stack landed over two weeks under tremendous time pressure. They were modeled in GitHub as 29 separate PRs and landed as 29 separate commits on main.
29 separate PRs
I don't mean to be disrespectful, but you surely are a patient person if you took the time to go to github.com that many times. :)
This article, and some comments, seem to be conflating two different notions of rebase:
git rebase -i to curate a chain of small coherent commitsgit pull --rebase to make sure your commit(s) end up on master without a merge commitThat aside, projects with low risk tolerance will probably err towards curated small commits, so they can be better reviewed, and blast radius is smaller.
A startup with high risk tolerance probably cares more about velocity, and reviews less heavily. (All things equal. A pacemaker startup likely reviews heavily!)
Stacked PRs are a great idea for projects that want smaller commits, though a stack doesn't lend itself well to parallel feature decelopment.jj makes it easier to have parallel commits that depend on a common not-yet-merged commit, I.e. a DAG. DAG PRs is ideal for me!
if a clean history is not enforced very strictly
I would say this should be implicitly enforced in the code review process.
Review for me is the main reason to provide and request a clean / readable ('rebased') commit history in the first place.