[EnPissant]

>I loathe GitHub PRs because of this. Working at $dayjob the unit of change is the commit, and every commit is reviewed and signed off by at least 1 peer.

Because this is exactly what a squash merged PR is. There is no meaningful difference unless you say "but commits are done by good people and PRs are done by bad people".

[Dylan16807]

They make it very clear that they are praising small commits, and squash merge commits are usually not small. Squashing is the opposite of what they want.

The preference they have is not exactly a problem with github PRs, but github PRs are much more likely to review a big pile of code at once.

The amount of code being reviewed at once is a meaningful and extremely objective measure, and that's the thing they're concerned with. Not who made it.

[EnPissant]

There is zero chance that any shop that reviews individual commits is not squashing them.

I probably wont be able to respond to any more comments. Dang put a slowban on my account because he interpreted one of my comments as right wing.

[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.

[Dylan16807]

Well then you're calling them a liar, I guess.

But even if they're lying about achieving it, the preference they have for reviewing small commits is a preference that makes sense. It's not some nonsense "us versus them" thing.