Make Reviews Possible Again With This One Simple Trick
5 points by noon
5 points by noon
I’m sort of unconvinced here. What does this approach give you over creating one PR with multiple commits and telling people to review the commits?
It's the same thing. Giihub has just always had broken review tooling and a lot of people are influenced by GitHub
It's the same thing
I don't think that's true. Four linear commits with one branch pointing to each and four linear commits with one branch pointing to the most recent commit are different things at both the git-level (which treats branches just as movable pointers to commits) and the GitHub-level (which treats branches as the unit of merging).
It's the same thing from a review point of view, is what I meant, per the context. It's obviously mechanically different under the covers.
Ah, I don't think that's quite true either! Reviewing two different stacked PRs is a different review experience than reviewing two different commits in the same PR, and I think it's a meaningful difference too.
And much of the time that meaningful difference is more busywork switching between PRs for both the author and reviewer.
GitHub-style PRs make it unwieldy to iterate on PRs that are thousands of lines long, even if you could break it up into a series of well-reviewable patches. You also can't 'rewrite'/update a previous commit without destroying attached review history, so you tend to see lots of 'addon' commits being added 'Fix X in response to review feedback' etc etc.
It makes it hard to keep track of, whereas it would be logically quite a bit nicer to have new 'versions' of commits so that I can review entire logical changes at once — even after it's been tweaked following a round of review. (I saw a good blog post that mentioned 'interdiffs' just a few days ago I think, I regret that I can't find it again to link it here)
At the end, with the commit history being so messy, you also get incentivised to squash merge, whereas it might be nicer to have a small number of clean commits instead.
(You can then see why some people prefer a 'patch series-driven' workflow, which as far as I can tell is essentially what stacked PRs approximate)
Another argument in favour of splitting PRs up: PRs are the unit of merge on GitHub and similar; sometimes a PR is structured such that it would be acceptable to land the first 1, 2, 3 (whatever) commits before the entire suite of work is ready. In cases like those, you can see why you might stack PRs and merge them incrementally.
[In a sense, is the tension here that PRs are both the unit of merge and the unit of review?]
IMHO "destroying" the review on the previous version when updating is a feature. The comments on that version have been addressed by this new version after all.
Not necessarily. You can leave that diff hunk unchanged and rebase your branch (/ rewrite the commit to change it), then the review comments get detached even though they could still in principle be attached.
If you push 'fixup' commits, the review thread doesn't get dropped from the UI, but it may get marked as outdated, but only if they actually touch the code region in question. This feels more correct than dropping them outright just because the commit ID changed.
You can probably work with it if you like, but the team I'm on has decided that it's sufficiently bad UX that we don't allow this. It makes it an extra notch harder to follow a PR's conversation.
Every team I've been on has banned "fixup" commits because they clutter the UI and history. So I guess it's somewhat of a preference thing.
Probably how we 'get around' that is that we typically squash merge.
This has downsides, of course, but I guess it's the logical outcome/tradeoff that you arrive at if you want clean history and want the GitHub reviews to not have the problems I describe.
I think a good solution to this problem are fixup commits--commits that are intended to be squashed into other commits.
The workflow here being:
I think fixup commits could work really well (or at least soothe a lot of these pain points), but for it to feel nice, you'd want GitHub to have support for them, like:
Of course, you can say the same about stacked PRs (many of us have tried using them before GitHub supported them as a built-in feature... Would have been nice if GitHub implemented rich support for fixup commits as well or instead)
applying them at merge time (maybe stubbable with a merge bot like what the Rust project uses)
It's honestly crazy that GitHub doesn't do this! And while I'm on the subject, "rebase and merge" in GitHub still doesn't do a fast-forward merge, it rewrites commits. Maybe there's a good reason for it, but I don't understand how GitHub still drops the ball on what should be core functionality.
You can't approve a specific base commit and ship it before hand which is a negative of this flow.
i didn't expect i'd have to explain the stacked diff theory here; but i did link it in the post - https://gist.github.com/thoughtpolice/9c45287550a56b2047c6311fbadebed2#a-better-way-interdiff-review-aka-git-range-diff
i.e. it's the same as long as there are no changes. but if there are actual review comments and revisions; it's completely different.
Actually, as far as I can tell the idea is EXACTLY that.
It's better than "put it all into one giant commit and review everything at once".
Came here to say this.
Also the moment the review flags something in branch-1, all my branches on top need to be rebased individually?
not a full solution, but git rebase can update some refs automatically, though it can take some getting used to b/c branch pointers may ‘unexpectedly’ move to other commits.
from the git-rebase docs:
--update-refs, --no-update-refs
Automatically force-update any branches that point to commits that are
being rebased. Any branches that are checked out in a worktree are not
updated in this way.
If the configuration variable rebase.updateRefs is set, then this
option can be used to override and disable this setting.
Get a tool that does this for you... jj / sapling / I think there are tools for git that do it.
(update-refs aside -- this is a self-inflicted Git and GitHub issue, not something that is fundamental to source control.)
Discussion about stack branches have been popping up more and more frequently recently, and I really disagree with them! Has anyone got any specific thoughts on the following?
They fundamentally don't address the problem at hand - The feature you are implementing is too wide-reaching/too poorly planned/etc, and you are applying a salve instead of practising good engineering principles.
Once all the PRs in the stack are ready to be merged, you have an arduous process ahead of you of sufficient testing. This feature was already determined to be too large for a reviewer to competently review, but now you will have to test/QA for all integration issues in one large change!
The reviewer has no concept of the overall implementation until all reviews are completed. I frequently review stacked PRs that I end up pushing back because the overall approach cannot be seen from the "base" PR of the stack, and when you get to PR 3 in the stack, you see that the data structures introduced in PR 1 are not the best fit.
These problems have all been long-addressed, by simply planning, and using feature flags/versioning. If your feature is enormous, and you want to split it into stacked PRs, you should have some form of design doc/ADR/similar to document the structure of the feature. This should be reviewed separately and prior to the code implementation. Secondly, you should implement each PR in the "stack" in a sensible, testable manner. For example, adding your data structures/models first, which allows you to be unblocked on 5 additional changes of adding API endpoints. If there is a frontend component, then implement a very thin sliver that allows the new functionality of the feature to be tested end-to-end in each individual PR. Put this behind a feature flag and merge it to your main branch on approval.
I'm sure that there are valid uses of stacked PRs, but by and large, I think they are just being used as a means of accommodating poorly thought out features, often LLM driven.
I think your objection makes sense, but perhaps only if you assume that all PRs in a stack would be merged at the same time, which is not necessarily the case.
A doc/ADR before you start such a large feature is a really good principle that I would like to see being done more often.
i believe i addressed all your concerns in the post :)
Some meta-level stuff:
I had an immediate negative reaction to the title because of the phrase "One Simple Trick," which is commonly associated with clickbait. What is the trick exactly (answer: stacked branches), and why not mention it in the title? That said, it is your blog, and you are free to title your posts however you want. I am not entitled to titles that suit my preferences, so fair enough.
Nearly everything you have submitted at https://lobste.rs/~noon/stories appears to be your own posts. It might be worth easing up on that for a while. When almost every submission is self-promotional, it can leave a poor impression. It raises the question of whether you are participating mainly to grow your audience or whether you genuinely want to share material that the community may find interesting.
One approach that I'm experimenting with for large chunks of work that are resistant to a natural split is to have the AI generate review notes. This is ideal when you have say an RFC or detailed spec that you're implementing, and each chunk of code needs to be justified by the RFC, and each line of the RFC needs to have corresponding code. Making the coding agent generate a markdown doc that interleaves the two things allows your human review to be non-linear across the source code / spec, but still structured in addressing the code.
The "one simple trick" I have is use the tool that is good at spitting out tonnes of code to spit out tonnes of text that simplifies your ability to validate the code, and make it optimize that for your attention.
I.e.
"Run me through this code in the order that makes sense to review it
so that I don't miss anything problematic, show snippets of code and
the relevant information to understand it and validate that it meets
the spec directly together...."
One way of framing this better is that a human-machine centaur works best as a pair when the human is a context engineer for the machine and the machine is an attention engineer for the human.
But the other part of this is that any time you catch something that the AI missed in generating (or reviewing), add that to your list of things that you use the AI to review for (or use deterministic lints if possible to avoid that entirely). This becomes your durable working list of things that matter to you and to all future AI generated code. DRY, but for reviews.