Sam Parker (Nov 04 2021 at 09:50): Sam Parker (Nov 04 2021 at 09:50):Hi,
What are people's thoughts on having PRs squashed into a single commit before being committed? When I was working on LLVM, we'd push the whole patch up for review, even after changes from feedback, and the patch would land as one commit. This makes it very simple to view what was changed for a certain feature, which is a great way to learn and remind oneself, and also made reverts simple. Of course, it can also just clean up the commit log considerably.
Benjamin Bouvier (Nov 04 2021 at 10:38):+1 to that! squashing hides most unimportant details and allows work flows where one can just continously push to their github branch, making it nice to collaborate with other people (vs rebasing, which causes force-push and thus merge conflicts if two people are on the same branch).
It could make bisection a bit harder sometimes, but if a commit resulting from a PR is so large that it makes bisection impossible, maybe it shouldn't have been so large in the first place, and could be split in several PRs.
fitzgen (he/him) (Nov 04 2021 at 15:43):As long as commits stand on their own (eg pass tests so they don't break bisection, are a logical chunk of work, etc) then I think it is fine to keep the history.
But if not, then squashing is the way to go.
fitzgen (he/him) (Nov 04 2021 at 16:09):Multiple commits that stand on their own can be nicer for reviewers, as well, since you aren't faced with a massive wall of code, and have things organized into little chunks of functionality
Chris Fallin (Nov 04 2021 at 16:20):One specific thing I find nice during the review process is a separate "Address review comments" sort of commit; without that, it's difficult to tell what changed
Chris Fallin (Nov 04 2021 at 16:20):Other projects I have definitely seen the rule be that this is then squashed before merge; +1 to that if that's the way we want to go
Sam Parker (Nov 05 2021 at 14:35):Yeah, I agree it's probably easier to review the increments, so having the auto squash before the merge would be the best of both worlds. This is something that can just be setup via the github UI, right?
Sam Parker (Nov 05 2021 at 14:47):As for smaller chunks of functionality, maybe it's worth just breaking the PR up. I'm mainly thinking about removing commits that amend typos, fix cargo fmt and other small things that really have no historical use.
Till Schneidereit (Nov 05 2021 at 15:02):we could set things up to force auto-squashing for all PRs, if that's what we end up agreeing on. If we do decide to squash, then I agree that that's much better than manually squashing, with the review issues that brings
bjorn3 (Nov 05 2021 at 18:35):I personally squash PR's for cg_clif if they are a small change combined with a couple of fixups (fixing ci, review comments, ...) If the commits somewhat stand alone I don't squash.
Sam Parker (Nov 17 2021 at 11:29):So... would anyone have strong objections to squashing pre-merge?
bjorn3 (Nov 17 2021 at 15:19):I slightly prefer not squashing when there aren't any fixup commits.
Last updated: Nov 22 2024 at 17:03 UTC