Comment by theshrike79
12 hours ago
Why would the reviewer look at the commit messages instead of the code?
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?
In some cases, reviewing PR diffs commit-by-commit (and with the logs as the narration of the diff-by-diff story) is a substantial improvement over reviewing the entire PR diff. Concrete examples...
* 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.
Because it's a useful abstraction. If you only look at PRs and don't ever care about commits, why are they even being sent to reviewer in the first place? Just send a diff file.
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.
> If you only look at PRs and don't ever care about commits, why are they even being sent to reviewer in the first place? Just send a diff file.
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.
Stacking is good for expressing dependencies, but isn't helpful when you need to make several distinct changes that aren't necessarily needed unless you take them all in. What's the value in having a separate PR that introduces a framework that you later use in another PR when you may not actually want to merge it if the latter one doesn't end up being merged as well?
A PR is a group of commits, just utilize that when you need it.
>Swap to diff and start reading through the changes
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