Comment by dllthomas
2 years ago
That's one alternative. Another alternative is cleaning that up in a rebase into a series of (hopefully) easy to follow individual commits that do one thing, and then a merge commit pulling in the branch with a description of the change as a whole (and a reference to the pr and any relevant tickets). There are other alternatives as well that make various tradeoffs between effort for the author, effort for the reviewer, ease of reading the git history, applicability of various tools, etc.
Yes, I'll never understand the "fix typo" commits when `git commit --amend` is so straightforward.
Better-still is `git commit --fixup {rev}` and `git rebase -i --autosquash`, since then the fix(up) can be placed into the most-appropriate commit, which isn't necessarily the most-recent one.
Of course, that assumes a you're already got a workforce that is able to do resolve minor conflicts when something else near the typo got tweaked in a separate commit, so I'll grant that `--amend` is easier when starting out.
Better still, git-absorb figures out in which recent commit your fixups should go on its own (based on which file it touches)
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.
1 reply →
> 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.
2 replies →
And then you have to go through the sysadmin team to temporarily enable push --force on your branch
I think you start from a different opinion of what a PR looks like. You say “commits that do one thing”, but in my workplace PRs already are supposed to be small and do one thing: I _want_ them to be squashed into a single atomic commit that’s easy to revert if needed
A PR should do one thing at a higher level than a commit should do one thing (both at a different level than a function should do one thing).
I expect we're agreed that most PRs should be a single commit with a small number of changes in a small number of places, but IME it's not rare to have situations where dividing the changes into groups makes things clearer but where elevating those groups to the level of PR would make things less clear.
IMO PRs are supposed to do one thing. But they might end up doing a few more things like refactor, clean up whitespace, or even add a new function in order to facilitate the change. And all of these can be put into their own commits.
Now you can make like five PRs for each of those commits. But that seems similar to making five <issue tracker> issues for those commits. You’re already there in the PR. You might not need the overhead of N external items for N commits.