[danny_sf45]

Do people actually do code reviews per commit? I know it's a thing, but in all my years of experience, I haven't met anyone who actually does this. The usual practice is to review all the changes at once (e.g., go to the "Files changed" tab of a PR in GitHub and start reviewing the changes). This, of course, means, that PR are "small". If a PR is too "big" then one politely asks the author to split the PR in many.

[dkubb]

My current team does it, and it is so enjoyable.

We practice atomic commits that change only one thing, and in one way. We separate Fix, Refactor, Change, Add, Move and Remove commits from each other. Our commit summaries begin with those keywords so we can tell what "type" the change is. Each commit must pass CI 100% on it's own.

There are specific characteristics we've noticed. For example, for us, a "pure Refactor" commit either touches code or tests, but not both at the same time. If we touch too much we try to split it, or we call it a Change and it gets even more/different scrutiny.

Reviewing by commit allows me to give my full focus to each without having to keep all state in my head at once. I can step through and understand the series of changes. It's like telling a story, once I've stepped through each commit I am better able to understand the whole picture. If I only look at the summary some of the finer points are missed.

Also we've found we can extract commits more easily and get those merged early while we work on our main branches. If we see a Refactor, we can just do it, extract it and then keep our main PR focused.

[Izkata]

For me, it's useful to peek into the thought process behind the commits if I notice something odd or interesting in the aggregate review. On occasion I end up with a totally different set of comments because I find they'd already tried something I was going to suggest.

More important to me, the "logically split" commits are less useful when hunting down a bug months or years later. They explain the final intent, but not the path to get there, when the path to get there reveals when the bug was actually introduced, and following the thought process using those original commits reveals what the developer was intending instead of just what the end result was.

For example, was the bug introduced in the refactoring/cleanup? This happens a lot, and an earlier state of the code reveals what it should be doing. Was it introduced during initial development? If so, is it just an edge case they didn't think about (because it should have been impossible and a recent change elsewhere made it possible), or is it a remnant of some earlier version of the feature? ..etc

(That remnant-of-an-earlier-version one has actually happened to me and straight removing some code rather than trying to fix it was the correct way to handle it)

[seba_dos1]

Depends on the nature of the particular change, but looking at a merge request commit-by-commit is often much easier to grasp and find potential issues. I usually quickly scan the overall diff first and then proceed to look deeper at each commit.

We're working on FLOSS projects where contributors (internal and external) are asked to split commits logically and that's how they end up in the final history. Most merge requests consist of just a couple of small commits, although sometimes there's a bigger and longer-lived branch where it is easier to just look at the diff as a whole (separate commits still massively improve bisectability though and I can't think of any real downsides of having them retained).

[alecbz]

> Do people actually do code reviews per commit?

This is sort of a meaningless question without an understanding of how often people commit. Some people keep a single commit for a single PR (and just constantly update that one commit), others make tiny little commits for every individual change.

The only real question is: what is the granularity you should review code changes at? The number of git commits that that maps to doesn't really matter.

IMO: the ideal is to try to keep pull requests as small as possible while still having each PR be a coherent, justifiable change on its own. I don't know if it's realistic to treat that as a hard rule, but I think it's the right thing to aim for.

[angrais]

Yes, but the OP asked if people are reading specific commits in a PR.

The answer for me at last is no. Because the history in a PR should not matter but only the final commit. That's why we choose to squash commits in my current role.

[vlovich123]

When I was at FB, there’s no PR as defined at GitHub. Each independent commit was a separate review. So in that sense, all of the 10s of thousands of engineers at Facebook daily review “PRs” at the commit level.

[angrais]

In that case, where you then recommended to only make one commit? Are there guidelines for the average/ideal commit length? Did this add to extra workload in creating "perfect" commits? Likewise, did this mean that pushing "WIP" commits to remote was rare?

[vlovich123]

Felt like less workload because each commit had independent feedback that you could address resolve. Then you could push each commit as it was ready (or whichever part of the stack was).

Wip commits we’re often and frequent for CI purposes but as with PRs you don’t want to waste too much review time on WIP unless your asking for some preliminary feedback on a complex change.

Ideal commit length was unchanged to any other commit length you might be expected to do on any other team with good commit hygiene.

[sime2009]

Not really. PRs have become the new commits. I only pay attention to the changed files tab in GH. I encourage other people to commit early and often and share their progress with the rest of the team.