Comment by schindlabua

> It is better to leave the PR commits separate and atomic so reviewers can digest them more easily.

So reviewers have to digest all of the twists and turns I took to get to the final result? Why oh why oh why?

Sure, if they've already seen some of it, then there should be an easy way for them to see the updates. (Either via separate commits or if you're fortunate enough to have a good review system, integrated interdiffs so you can choose what view to use.)

In a better world, it would be the code author's responsibility to construct a meaningful series of commits. Unless you do everything perfectly right the first time, that means updating commits or using fixup commits. This doesn't just benefit reviewers, it's also enormously valuable when something goes wrong and you can bisect it down to one small change rather than half a dozen not-even-compiling ones.

But then, you said "atomic", which suggests you're already trying to make clean commits. How do you do that without modifying past commits once you discover another piece that belongs with an earlier step?

> You just squash on merge.

I'd rather not. Or more specifically, optimal review granularity != optimal final granularity. Some things should be reviewed separately then squashed together (eg a refactoring + the change on top). Some things should stay separate (eg making a change to one scary area and then making it to another). And optimal authoring granularity can often be yet another thing.

But I'll admit, git + github tooling kind of forces a subpar workflow.

I do leave PR commits separate. In my teams I don't set up pre-commit hooks altogether, unless others feel strongly otherwise. In projects where they are forced upon me I frequently --no-verify hooks if they are slow, as the linter runs on save and I run tests during development. CI failing unintentionally is usually not a problem for me.