My opinion is that a PR should be small enough that it’s desirable to have it squashed into a single commit at the end anyway.
My opinion is that a PR should be small enough that it’s desirable to have it squashed into a single commit at the end anyway.
A `git commit --fixup` commit can be made which does that. Then when the review is done `git rebase -i --autosquash`. Like squashing but with fine-grained control.
It's even better if there's some system that ensures authors remember to do the squashing, such as by prohibiting actions that would bring fixup commits into the main development branch.
Personally, I use `--fixup` even before making a PR, particularly if there's some work that I want to split into "refactoring prep" versus "the new feature."
If the change is in response to PR feedback, this is definitely a meaningful concern. The history in the comment I was responding to seemed to imply it was presented for review in the form it was created with lots of little commits along the way; fixing up the history into a small number of meaningful commits before requesting review should be easier on the reviewer, not harder.
Whether to fix up commits before requesting review, during review, and after approval are three separate questions somewhat separate from the question of whether to squash everything at the end.
> My opinion is that a PR should be small enough that it’s desirable to have it squashed into a single commit at the end anyway.
I mostly agree directionally, or for a sufficiently weak "should", but as we get stricter we get into tradeoffs and it gets more subjective and/or context sensitive. If the code base and desired change are in a state such that making the change you need to make is easily split into several conceptual pieces, but they do not make sense on their own, then either you combine them into one commit and wind up with a big PR that's harder to follow, you keep them separate but clean, or you merge them in separate PRs. Some downsides of that last are that it's less clear to the reviewer what motivates the early changes, it may require redundant work to keep everything working (and the code clear) with some of the changes and not others of the changes, and related work moves further apart in the commit history. On the other hand, it should mean smaller merge conflicts as incompatible changes are addressed sooner.
I'd also caveat "one single commit" in that sometimes one commit changing behavior and then a separate commit formatting (or one commit with some preparatory reorganizing and then another changing behavior) can make it clearer what the real changes are, and the intermediate step might not pass linting/format checks if you try to do it in separate PRs.