4 ms·
Sometimes, a collection of related changes has to go in together. E.g. one fix may uncover another latent bug, so that tests no longer pass, or an API change m
by jlokier 5y ago
Sometimes, a collection of related changes has to go in together. E.g. one fix may uncover another latent bug, so that tests no longer pass, or an API change may be the right solution, but that has consequent effects, and should be done in phases.
Where I'm working now, it's the opposite to your place: It's almost a no-go to use separate commits to fix separate issues in a change that is reviewed as a whole. We have been asked to use fewer, larger commits. In practice, what I see is commits with a description (if you're lucky) that describes the main change, but there are other changes mixed in which aren't mentioned in any description.
I prefer to use separate commits to fix individual issues even when making a bugfix PR that is "larger in size and scope". That way, people can see each part and I can present the fix most clearly, and I have to figure out the best order in which each fix makes sense. I usually do this even with my own private work that nobody else sees, as there is some satisfaction and clarity in dividing up related issues. I usually construct the logical set of changes after I've figured out what they are and satisfied myself they work well together, through use of "git add -p", "git rebase -i" and per-commit testing in a "curate your Git history for others" approach.
This comes from the Linux approach, where a proposed patch series must be, in principle, a sequence of commits designed for review, that could be cherry-picked or have some of them left out.
The unit of PR should be larger than the unit of commit, and commits should each be one reasonably self-contained logical change that could in principle be cherry-picked in isolation, or individually rejected.
But lots of places have a policy which requires PRs to be "squash merged", so all the changes are one commit in the end. The individual commit history is actively destroyed. There is no point putting in the effort to lay out carefully curated commits for a PR, if most of the history will be destroyed soon after, especially if reviews don't go deep.
I've been told this should be done because it "helps with rolling back changes" but I think that's incorrect for my style of commits (because they are curated into functioning logical changes already), and it's a crude approach to rollback. However, it makes sense for those people's PRs which are chronological (commits like "try X", "try Y", "aha, fixed it"), and where part way through the PR, the project doesn't even build.
When I roll back changes that have been merged, it's rare to want to roll back an entire group of changes. By the time a problem is found with a merged PR, it's probably too late to undo it all anyway as other things will have been built on top. That's when I'm most likely to find the individual commits of the PR most useful.