I've made sure via repo config that only squash commits are enabled from now on, so this will not happen again. Thanks for the feedback!
The "real" solution is making the developers aware of the issue and cleanup up their history before doing an MR.
For a start, it's easy to review, and easier to disect if something breaks.
When messy git history is provided (i.e. a history of the developer fixing their own code that they missed), squashing can be a reasonable fallback. But it's never the best option, IMHO.
It's a separate project from git [0].
And the benefit is that instead of humans spending time deciding whether or not to squash commits in some branch before merging, or being asked to do so by a maintainer, a computer just does it automatically. It's a lot like automatic code formatting; it's a big win to have tools do things automatically, rather than requiring human time and judgement and communication.
They're not independent, they're related. Having multiple levels of "namespace" - repo -> PR -> commit - makes things easier to navigate, just like having multiple levels of folder in your filesystem.
> And the benefit is that instead of humans spending time deciding whether or not to squash commits in some branch before merging, or being asked to do so by a maintainer, a computer just does it automatically.
If you demand that each PR must be a single commit you just force the human to make that decision earlier and more manually.
> If you demand that each PR must be a single commit you just force the human to make that decision earlier and more manually.
How so? I think you may be misunderstanding the suggestion... The pattern is that people can have whatever commits they want on their own branch, but when that branch gets merged into the trunk, all those commits are automatically squashed into a single one. I don't see how that is "forc[ing] the human to make that decision earlier and more manually".
Right, so that means people have to decide whether something's going to be a separate commit immediately when they're working on it, and start a new branch if so. Or they could mess around splitting their branch into smaller branches at the end when they're ready to PR, but that's fiddlier than rebasing to split/merge commits.
I don't know what you mean by making PRs being "fiddler than rebasing to split/merge commits". What is the fiddliness you see?
That's a horrible way to work because when you squash a branch it breaks the history of any other branch that's based on it. So you'd have to do a fiddly rebase when you merge your first branch in.
> I don't know what you mean by making PRs being "fiddler than rebasing to split/merge commits". What is the fiddliness you see?
Imagine I've done a bunch of work in a branch, and then decide I want the contents of that branch to become three commits on master. With your workflow I have to split my branch into three branches, retroactively. That's fiddlier than just turning the commits on my branch into three commits.
It really isn't. Branches are just as cheap as commits. It's super easy to create three branches pointing at three arbitrary commits.
Figuring out bugs with `git bisect` is not going to be a fun endeavour for people trying to understand incompatible changes.
Branch protection doesn't allow merging without a passing test-suite.
> The squash merge is not going to solve the lack of proper commit messages
Could you expand? You choose a sensible commit message on squash, while the PR's commits become fairly irrelevant at that point.
https://github.com/opentffoundation/opentf/pull/243
EDIT: and just to point out. If you have 1 PR with 19 commits that break the test suite. The last commit fixing it doesn't matter as you will be hitting one of those 19 commits at some point during a bisect.
>Could you expand? You choose a sensible commit message on squash, while the PR's commits become fairly irrelevant at that point.
It's optional. Nothing prevents you from just adopting whatever the PR said initially. Turning it on doesn't automatically make it better.
But not if you merge the PR as a squash-merge, which turns those 19 "development" commits into a single "permanent" commit.
Using PRs as the unit of development, with all intra-PR work squashed into a single atomic test-passing commit, is a well functioning process that many teams use.
Then I'll `git bisect --first-parent`, and either isolate it to your merge, or mark to merge OK. `git bisect` doesn't have to walk your branch.
The test suite did its job by alerting you to make changes before merging your work to the trunk branch, I don't see this as anything anybody could possibly pick a fight over.
And to be clear, I still use mainline terraform because I never really wanted a wrapper around terraform. It's just silly to me to take issue with the terraform wrappers for this non-issue.
Imagine you are a terraform provider developer that is working on making sure their code is working with `opentf`.
Lets imagine opentf does an initial `v1.0.0` release of their code and your provider doesn't work. But you know it worked with the last FOSS release of `terraform`.
What do you do?
You find the common ancestor between these two projects, lets say 8a085b427b74ce3829500a59508b77465f1bbef0 (as that is the last commit `opentf` has from `terraform`).
You will now run `git bisect` on the history between `8a085b427b74ce3829500a59508b77465f1bbef0` and `v1.0.0`.
You will do a binary search on the 100-200 commits here, and everytime the source fails to build, or the test suite doesn't pass for whatever reason, you are making it much harder for the downstream provider to figure out why their code doesn't work.
You can easily just try do this today and see what happens. Does the current untidy git history cause you any problems?
I really recommend adopting this strategy.
Don't forget, a project like Terraform is too big. No one person can know how the whole system works. Trying to look through PRs as a means of debugging is a fools errand. You have to take on a different mindset when working on these large codebases.