Only allow squash-merges

[wjt]

In our contribution documentation we sit on the fence about whether we require a clean commit history. First we say that commit messages should be well written; “Pull requests should contain one commit per self-contained, logical change.”; and so on. But then we say:

If you are not a Git expert, you may prefer to submit pull requests without a clean commit history. In this case, the maintainers will squash your changes into a single commit when the pull request is merged. The title and description of your pull request should still follow the guidelines above in this case. The Threadbare maintainers will be happy to help if you're not sure how to do this.

Currently the repo is configured to allow both squash-merging and real merging of PRs.

The problem with sitting on the fence is that the maintainer who presses “merge” has to remember to choose the correct option based on the approach that the submitter has used. It is very easy to get this wrong and add many messy commits to our commit history. I have looked into it and there is no way to dynamically adjust the available strategies in a PR, short of doing all merges via some external tool - I don't think the complexity of adding non-standard merg

I think we should standardize on squash-merging and disable the ability to create a merge commit. It would simplify our contribution instructions; reduce the possibility of maintainer error when merging an approved PR; and is more consistent with how we teach learners to use Git. (We don't teach git rebase because we are essentially trying to teach just enough Git for people to collaborate and no more.)

This bothers me as someone who has spent 20 years obsessing about commit messages and factoring out his PRs into a series of carefully-crafted commits. But I know I am an outlier here! Many other projects use squash-merge exclusively. (Godot itself is a weird mix: they require PRs to contain exactly 1 commit, and then they create a merge commit. IMO this is the worst of both worlds because it forces contributors to learn to use git rebase but also doesn't allow them to prepare logically-structured series of changes.)

For when one really wants to do a series of self-contained changes there are tools that can help with this, e.g. git branchless submit can open a stack of PRs, one for each commit in a branch.

[wjt]

@manuq when you return I am curious what you think here!

[wjt]

A related point: currently we have a CI workflow that runs pre-commit and complains about code formatting & order, but does not fix the problems.

It would be kinder/more helpful to have the linter just fix the problems. Unlike the tool we currently use, https://github.com/GDQuest/GDScript-formatter can reorder code to match the GDScript style guide. If we use real merges, having extra CI-generated "fix code style" commits would clutter the history, but if we squash-merge, these would be squashed away...

[manuq]

Yes please. As the maintainer who did it wrong twice, the last time adding 52 commits to the project history for a single file change #1800, I find it error-prone to have to check each time if the correct merge option is selected.

It also goes against my own working culture. I prefer clean separate commits to a level that makes the review hopefully easier, and to remember why the change was made after the fact. But since we are not teaching Git rebase in our programs, I agree to only allow squash merges.

[wjt]

I'm still very conflicted here because in #1860 it really did make more sense to split the change into atomic commits. There are lots of notes in each commit message. But I don't think it makes sense to review this as 4 separate PRs.

I suppose I will need to adapt my style!

[manuq]

I'm still very conflicted here because in #1860 it really did make more sense to split the change into atomic commits. There are lots of notes in each commit message. But I don't think it makes sense to review this as 4 separate PRs.

I think opening a stack of PRs will be confusing. I'm reviewing #1860 now and I really prefer to do it by reading each commit message + code changes. So maybe we can keep our workflow, and still use squash merge? I'm afraid in that case we'll lose the git history, but save the review process. In docs we can basically say:

The maintainers will squash your changes into a single commit when the pull request is merged. So the title and description of your pull request should follow the guidelines above. To help with reviews, ideally you should provide clean separate commits. But if you are not a Git expert don't worry, you can submit pull requests without a clean commit history.

[wjt]

That works. What we will lose is that the mini-essays I wrote in https://github.com/endlessm/threadbare/pull/1860/commits. I only summarized in the PR description in the git log. The workflow change I will need to make is to put such details either in code comments, or in the PR description: one of the reasons I write such long commit messages is that I have a bad memory and like to look at the commit history to understand why I wrote some code in some way.

[wjt]

I have made this change. Only squash-merge is permitted. I will update the documentation.