[sfink]

At Mozilla, we review individual commits and do not squash them. This is probably true of anyone using one of the forges capable of handling patch stacks. ("Probably" because the shop may or may not review everything!) Once in a great while, I will keep the commits separate for review and squash for landing, but that's because I intentionally left things in a half-complete state for ease of review.

When you're looking back at why something was done in a certain way, the review view is more useful than either the squashed view or the stream-of-work view. A human put effort into making it understandable, so it's no surprise that it's more understandable.

[EnPissant]

So if someone reviews your changes and you both agree to rename a variable, is that its own commit or do you squash it?

[sfink]

Squash. But note that we don't use github PRs, we use Phabricator with support for stacks, so we're likely talking about somewhat different things.

I'm talking about a forge that allows some sort of persistent reviewable unit that can change over time. Phabricator revisions, jj changes, and at least Gerrit has the same thing. There isn't a single unit of review, there are two: the bug and the individual changes. The bug is associated with a stack of changes. An individual change initially corresponds to a commit, but when you rename a variable, you update the commit for that change. jj and I guess git call that squashing, hg calls it amending.

The author does work, useful work, to break down everything that needs to change for that bug into a series of reviewable changes, preferably not breaking the build or tests in the middle of the series, but that's negotiable.

So we may not be disagreeing on anything. If by "commit" you mean one item in a patch stack, then yes we squash. But we do not squash the different changes within a bug, whether or not they change during review. If there's a change that does some refactoring to prepare, then a change to add a new API, then a change to add users and tests of those users, then we review those separately and do not squash for landing.

It is definitely my preferred way of working. I don't want to see a dozen fixup commits, nor do I want to see a giant change that touches everything at once. It's a happy middle ground where the author decides what the reviewer and later debuggers need to look at and what they can skip.

[EnPissant]

It’s been a while since I have used a phabricator, but gh with squash merge is extremely similar. You review the PR as a whole, even though the branch may consist of any number of commits or merges. GH presents you the diff between the target branch (usually main) and your branch, you never see the individual commits except in the timeline or if you want to. When you merge the PR it just adds a single commit on top of main. When i moved from phab to github I preferred it because a PR is just a normal branch and you never have to destroy history of that branch with rebase or ammend until you merge it.