PR follow-up commits strategy at odds with comment from rust-highfive bot

[mightyiam]

https://github.com/rust-lang/rustc-dev-guide/blob/6b5c62c6f8946a1a158bb1e84e1dabe87daee820/src/contributing.md

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.

[jyn514]

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]

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]

I think squashing commits right before merging makes the most sense

[mightyiam]

Is it OK if the commit strategy depends on the assigned reviewer?

[camelid]

I think squashing commits right before merging makes the most sense

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]

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]

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]

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. 😄