Prajwal S N (Oct 09 2022 at 19:04): Prajwal S N (Oct 09 2022 at 19:04):I was wondering if the community is willing to consider changing our style of commits and merges. The primary issue with the current system is that we cannot use git bisect to isolate the source of a bug, and for a project as large and complex as a compiler, I feel that tool is an invaluable asset. As the codebase grows, it will only get harder to do RCA and fix the issues that crop up. I would like to know your opinions on two suggestions:
Ensuring that all commits in PRs are self-contained and atomic (no fixup/"merge main into branch" commits)
OR
Using the Squash and merge option for PRs and having separate PRs for changes in different parts of the codebase
Brendan Hansknecht (Oct 09 2022 at 23:27):Interesting idea. Not sure what i think about the conventional commit specification, but i definitely agree it is useful to have a clean trunk that is easy to understand and bisect. Though it is also important to make sure we keep it easy for new people to contribute to the Roc source code.
I think it is very useful to make sure every commit in trunk can build and pass tests if possible.
I am not sure the best way to do this with open source software that also isn't too much of a hassle for devs, new users, and reviewers. I like the idea of squashing as long as PRs are smallish. If we just switch to squashing and PRs are large, it actually will likely make bisecting worse due to just getting a gigantic diff that broke things somehow. But also, forcing smaller diffs means more reviews, more chunking of futures, more waiting on CI, and in general more hassle for devs. Not to mention GitHub has a very poor story for creating chains of PRs that are dependent on one another.
Not sure where that leaves us, but i do agree that it is definitely worth investigating more and investing in.
Prajwal S N (Oct 10 2022 at 06:07):I think a good starting point would be to specify in CONTRIBUTING.md that PRs should not have fixup commits. We can also include the git workflow to squash smaller commits into a single self-contained commit, so that new contributors don't find it daunting to make changes to a reviewed PR. If this is agreeable with everyone, I'll open a pull adding this!
Anton (Oct 10 2022 at 09:08):I'd prefer to use a github ci workflow to prevent merging in fixup commits.
Anton (Oct 10 2022 at 09:10):to squash smaller commits into a single self-contained commit, so that new contributors don't find it daunting to make changes to a reviewed PR.
Can you explain why more smaller commits would make it daunting to make changes?
Prajwal S N (Oct 10 2022 at 10:24):Can you explain why more smaller commits would make it daunting to make changes?
I meant that newbies who are not familiar with git might not be aware of using git rebase -i to squash fixup commits with the original commit. So listing out the steps they need to follow will allow them to fix review comments, rather than abandoning the PR out of the fear that they'll mess up the commit history.
I'd prefer to use a github ci workflow to prevent merging in fixup commits.
My bad, by "git workflow" I meant the sequence of git commands to squash multiple commits into one
Anton (Oct 10 2022 at 10:29):I think there's been a slight mixup, my "I'd prefer to use a github ci workflow to prevent merging in fixup commits." was in response to " to specify in CONTRIBUTING.md that PRs should not have fixup commits".
Anton (Oct 10 2022 at 10:30):I meant that newbies who are not familiar with git might not be aware of using git rebase -i to squash fixup commits with the original commit. So listing out the steps they need to follow will allow them to fix review comments, rather than abandoning the PR out of the fear that they'll mess up the commit history.
Perhaps we could "squash fixup commits with the original commit" automatically in a CI step.
Richard Feldman (Oct 10 2022 at 12:14):so as I see it, the upsides and downsides of the conventional commit specification:
Upsides:
Downsides:
Richard Feldman (Oct 10 2022 at 12:15):overall I definitely don't think that sounds like an improvement over the status quo of letting people write commit messages however they like :big_smile:
Richard Feldman (Oct 10 2022 at 12:30):regarding bisecting, I use it all the time on the compiler (and have on other projects) and I haven't encountered merge commits being a problem :thinking:
Richard Feldman (Oct 10 2022 at 12:31):the only problem is when a particular commit doesn't build
Richard Feldman (Oct 10 2022 at 12:33):so given that we only merge PRs which pass CI, using squash and merge should make sure that all commits which make it into main do indeed build
Richard Feldman (Oct 10 2022 at 12:34):but like Brendan noted, this has the downside that now we lose commit granularity on main, which makes it harder to track bugs down
Richard Feldman (Oct 10 2022 at 12:34):I would rather keep the commit granularity and have to tell git bisect to skip a commit once in awhile, so I don't think should adopt squash and merge
Richard Feldman (Oct 10 2022 at 12:35):I do like the idea of giving guidance in CONTRIBUTING.md on making commits be buildable
Richard Feldman (Oct 10 2022 at 12:36):although I don't think we should spend time enforcing it in PR review - just let people know we'd prefer that
Richard Feldman (Oct 10 2022 at 12:38):from an enforcement perspective, it seems like the type of thing where it would be very easy to spend more time telling people they need to follow the rules than the amount of time the rules save us in debugging
Richard Feldman (Oct 10 2022 at 12:39):so I think just letting people know, but not trying to enforce it, seems like the best way to save ourselves the most time overall!
Last updated: Jul 05 2025 at 12:14 UTC