Repository navigation
PR follow-up commits strategy at odds with comment from rust-highfive bot #900
Description
Is this paragraph
Rust follows a no merge-commit policy, meaning, when you encounter merge conflicts you are expected to always rebase instead of merging. E.g. always use rebase when bringing the latest changes from the master branch to your feature branch. Also, please make sure that fixup commits are squashed into other related commits with meaningful commit messages.
At odds with the bot comment rust-lang/rust#77219 (comment) ?
If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.
Activity
jyn514 commentedon Sep 30, 2020
The problem is that different reviewers have different preferences for how you handle git 😅 For example @Mark-Simulacrum prefers commits to be squashed, but I prefer extra commits so I don't have to worry about isaacs/github#1834.
Maybe we could change it to say 'squash commits before it's merged'? I don't think there's a great universal guideline here.
Mark-Simulacrum commentedon Sep 30, 2020
I prefer the commits to be squashed mainly because I find it much easier to re-review PRs rather than trying to remember past state, usually because I go 1-2 weeks at least between reviews on a given PR and I need to figure out what was going on. It's much easier to do so when things are in a consistent state within each commit rather than fixup commits scattered in git history.
On small PRs, it's also true that I personally will not approve things with stray commits without them being squashed in, so if the author is constantly squashing we can save a cycle at the end.
LeSeulArtichaut commentedon Sep 30, 2020
I think squashing commits right before merging makes the most sense
mightyiam commentedon Sep 30, 2020
Is it OK if the commit strategy depends on the assigned reviewer?
camelid commentedon Dec 11, 2020
Sometimes on larger PRs it's helpful to squash once or twice in the middle. E.g. I had a PR that got up to 60 commits because we kept changing the approach, so it was helpful to squash before the final review.
not-an-aardvark commentedon Feb 17, 2021
Just ran into this in #1058. My two cents is that I don't have any strong opinions on squashing versus pushing new commits, but having two pieces of reference material give opposite guidelines is pretty confusing for new contributors, and that aspect can probably be fixed without needing to resolve the squashing-vs-pushing debate.
It seems like even removing both guidelines could be an improvement over the current state, because as a contributor I wouldn't end up worried that I was violating the "real" guidelines by picking the wrong advice to follow.
rylev commentedon Feb 17, 2021
I agree with @not-an-aardvark. If there is no actual agreed upon policy for what to do, we should not pretend there is.
jhg commentedon Apr 18, 2021
I agree with @not-an-aardvark. And maybe the squashing-vs-pushing debate will be never resolved.
I think could be better try to separate a bit personal preferences in reviews. 😄