Comment by williamdclt
2 years ago
As a reviewer, —amend means I need to reread the whole commit to see what changed. A tiny commit means it takes me seconds.
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.
> As a reviewer, —amend means I need to reread the whole commit to see what changed. A tiny commit means it takes me seconds.
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.
Yeah, fixup commits are the answer when it comes to responses to reviewers, who can easily see just what changed since the last time.
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."
> As a reviewer, —amend means I need to reread the whole commit to see what changed. A tiny commit means it takes me seconds.
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.
If you use a good tool like Gerrit, then you can see a diff of changes made in a review since an arbitrary previous version of the change.
The idea is that --amend would be used before pushing the commit that it amends, and so before a reviewer ever sees the original commit.
The idea of PR review is that the submission is imperfect and that there will be fixes. Like, "typo fix" fixes.
Yes, but the history presented upthread looks much more like someone working before submitting a PR for review, rather than changes made in response to review. How much to amend the history at each step is several separate questions.