1. Open PR page in whatever tool you're using
2. Read title + description to see what's up
3. Swap to diff and start reading through the changes
4. Comment and/or approve
I've never heard anyone bothering to read the previous commit messages for a second, why would they care?
* A method or function that has code you realize needs to be shared...the code may need to be moved and also modified to accommodate its shared purpose. Separating the migration from any substantive modifications allows you to review the migration commit with the assistance of git's diff.colorMoved feature. It becomes easier to understand what changes are due to the migration, and what changes were added for more effective sharing.
* PRs sometimes contain mechanical work that is easy to review in isolation. Added or removed arguments, function renames, etc. No big deal if it's two or three instances, but if it's dozens or hundreds of instances, it's easier for the humans to review all of those consistent changes together, rather than having them mixed in with other things one has to reason about.
* Sometimes a flow of commits can help follow a difficult chain of reasoning. PR developer claims that condition X can never occur, but the code is complex enough that it's difficult to verify. However, by transforming the code in targeted ways that are possible to reason about, the complexity might be reducible to the point where the claim becomes obvious. One frequent example I see of this is of function/method arguments that are actually unnecessary, but it wasn't obvious until after some code transformations.
Having atomic commits lets you actually benefit from having them. Suddenly you don't have to perform weird dances with interconnected PRs with dependencies as "PR too big" is not such a problem anymore as long as commits are digestible; you can have things property bisectable; you can preserve shared authorship; you can range-diff and have a better view on what and how changed between review passes, and so on...
The unit of change is commit, and PRs group commits you want someone to pull. If you don't want or need any of that, you're just sending a patch file in a needlessly elaborate way.
This is in fact what hg does with amending changesets and yes it works far better. Keep PRs small and atomic and you never need to worry about what happens intra-pr. If you need bigger units of work that's what stacking is for.
A PR is a group of commits, just utilize that when you need it.
this forces the reviewer to view the entire diff at once, which can greatly increase the cognitive load vs. being able to view diffs of logical units of work
for tiny PRs it may not matter, but for substantial PRs it can matter a lot
I meant "shared place" as an open review request or a shared branch rather than shared underlying infrastructure. Shared by people's minds.
It just feels wrong to force push, destroying stuff that used to be there.
And I don't have the time or energy to bisect through my shitty PR commits and combine them into something clean looking - I can just squash instead.
Things that aren't referenced by anything anymore will eventually get garbage collected and actually destroyed, but you can just keep a reference somewhere to prevent that from happening if you need. Or even disable garbage collection completely.
Looks like people's fears about git come just from not knowing what it does.
it's hard to say fully, but unless a changeset is quite small or otherwise is basically 0% or 100%, there are usually smaller steps.
like kind of contrived but say you have one function that uses a helper. if there's a bug in the function, and it turns out to fix that it makes a lot more sense to change the return type of the helper, you would make commit 1 to change the return type, then commit 2 fix the bug. would these be separate PRs? probably not to me but I guess it depends on your project workflow. keeping them in separate commits even if they're small lets you bisect more easily later on in case there was some unforseen or untested problem that was introduced, leading you to smaller chunks of code to check for the cause.
I've never considered how an engineer approaches a problem. As long as I can understand the fundamental change and it passes preflights/CI I don't care if it was scryed from a crystal ball.
This does mean it is on the onus of the engineer to explain their change in natural language. In their own words of course.
I might have not stated my position correctly. When I mean "squash on merge", I mean the commit history is fully present in the PR for full scrutiny. Sometimes commits may introduce multiple changes and I can view commit ranges for each set of changes. But it takes the summation of the commits to illustrate the change the engineer is proposing. The summation is an atomic change, thus scrutinizing terms post-merge is meaningless. Squashing preserves the summation but rids of the terms.
Versioned releases on main are tagged by these summations, not their component parts.
Then I must be a bad reviewer. In a past job, I had a colleague who meticulously crafted his commits - his PRs were a joy to review because I could go commit by commit in logical chunks, rather than wading through a single 3k line diff. I tried to do the same for him and hope I succeeded.
I feel like what you're arguing is that you should clean up your commits before anyone else sees them. Fair. But you could also clean it up right before merging to main. It's not that different, except the latter is much less annoying, particularly when going back and forth with people.
I know this is a very github centric workflow, but that's where most engineers work now, and it's nice and easy. This wouldn't work for eg: contributing to linux, but that's not what most of us do.
I think in most teams I've worked with, the majority of developers (> 85%) barely undestand what Git is doing or what things mean inside GitHub, have never seen commit history as a graph, have never run something like "git log --oneline --graph --decorate" or "--format", and have never heard of "git range-diff" which is very useful for following commit/PR/unit changes.
Personally I review using "git" itself, so I see the graph structure either way, and there's little difference between stacked PRs, commit chains in a single PR, or even feature branches, from that point of view. Even force-push branch updatea aren't difficult to review, because of the reflog and "git range-diff". The differences are mainly in what kinds of behaviour the web-based tooling promotes in the rest of the team, which does matter, and depends on the team.
I agree with you if you're using Graphite instead of GitHub. Having a place to give feedback and/or approval on the individual "units" (commits in a PR, or PRs in a stack) is useful, grouping dependent but distinct changes is useful, and diff'd commit evolution within each unit PR in response to back-end-forth review feedback is useful in some collaborative settings. Though, if you know "git range-diff" and reflog, that shows diff'd commit evolution quite well.
In GitHub, people are confused by stacked PRs both conceptually and due to the GitHub UX around them. Most times when I've posted a stacked PR to a GitHub project, other people didn't realise it was stacked, and occasinally someone else has merged the tip of a stack made by me, and been surprised to see all the dependent PRs merged automatically as a side effect. Usually before they get to reviewing those other PRs :-)
People understand commit sequences in a PR, though I've rarely seen people treat the individual commits as units for review when using GitHub, unfortunately. In the Linux kernel world where Git was born, the PR flow is completely different from GitHub: Their system tends to result in feedback on individual commits. It also encourages better quality feedback, with less nitpicking, and better quality commits.
Not often, but given that it costs me nothing to have it all in my tree, I'd rather have it than not.
And here's a slightly smaller one which isn't about "miscellaneous fixes": https://lore.kernel.org/netdev/20260408122027.80303-1-xuanzh...
Some of these commits even get reviewed by different maintainers before being merged, which is common when a patchset touches several subsystems at once.