Proposal: switch to squash merge strategy

[davidhewitt]

At the moment we use the merge commit strategy on the merge queue. We chose this strategy because occasionally we like to preserve multiple git commits in a single PR.

Because we like to keep a meaningful git history, the downside of this strategy is that often we ask contributors to squash pull requests (or sometimes I do this myself). Contributors almost always need to do a squash & force-push to add a newsfragment. The force-pushing can also disrupt PR review a bit, by making it harder to see what got added since the last round of review.

Recently I've been working with the squash-merge strategy on the pydantic repositories, and I'm quite liking it. The result of getting a single commit at merge without needing to go through a lot of force-push ceremony on every PR can save time and makes review a bit easier to.

As a result, I'd quite like to switch to using the squash-merge strategy on PyO3 too. I think optimising for easier PR process is worth it, and on the infrequent occasion where we do want multiple commits, we can always fall back to multiple PRs.

Maybe in the future GitHub even makes it possible to choose the merge queue strategy on each PR so we can choose case-by-case, like we used to before we adopted bors.

Any concerns or objections? Otherwise, I propose to switch this setting next week sometime.

[alex]

Fine with me. Squash merge strategy is what we use on my other repos.

[mejrs]

I have no particular opinion but it sounds nice :)

[adamreichold]

As a result, I'd quite like to switch to using the squash-merge strategy on PyO3 too. I think optimising for easier PR process is worth it, and on the infrequent occasion where we do want multiple commits, we can always fall back to multiple PRs.

I do generally consider the commit history as much part of a change as the code itself and hence do not like squash merging. I also never felt the need to use it since curating commits and hence interactive rebases and force pushes are just part of my workflow.

But, there is not a lot of work in flow for me with the last PR being quite old and in review limbo, so I do not want to block this change. I especially agree with the argument of making it easier for new and/or one-time contributors.

I don't think multiple PR will help though, as that is too much ceremony IMHO and it will be detrimental to a coherent discussion which is basically the most important part of the PR user interface. (We already see this for everything that by its nature spans multiple code changes, how fragmented the discussion becomes.)

So in summary, I am fine with doing this even though I do not like it.

(I do think this is a major blunder of GitHub as a platform though which has done a lot to simplify communication in open source projects but is targeted a bit too much on their paying enterprise customers when it comes to the actual processes.)

[davidhewitt]

(I do think this is a major blunder of GitHub as a platform though which has done a lot to simplify communication in open source projects but is targetted a bit too much on their paying enterprise customers when it comes to the actual processes.)

I have to agree with you on this point, I asked for the choice to be supported on the merge queue feedback discussions thread back when it was introduced. It fell largely on deaf ears as far as I can tell.

Before bors was deprecated it was on my list of aspirational things to do to contribute an optional flag to squash in bors' trigger command.

So I really wish we could have both here and the choice on each PR. But lacking that choice, I've come to the opinion of late that it would make the PR process a touch more efficient.

If we think it's not working out, I will happily revert this change again!

[davidhewitt]

I wonder, maybe as an alternative, we could try this? https://docs.mergify.com/workflow/actions/squash/

Then we can ping a bot to squash PRs directly on the web UI, which would go a long way towards getting the same effect without forcing us to squash everything.

[adamreichold]

I am not a huge fan of bringing in more proprietary SaaS offers.

Could we maybe define an additional manual staging branch where we can push a merge ourselves so the full CI checks it. And then we either merge it manually or just have the CI do the merge itself as a success job (i.e. the job which is currently tracked by the merge queue)? I think the main downside is that main would have to be open for direct pushes (either from us maintainers or at least from the CI job finishing on the manual staging branch)?

[davidhewitt]

Uff, I'm open to such an idea but I almost feel it's more work than the squash-and-force-push dance that I am either repeatedly asking new contributors to do or just doing myself.

So, I think, let's just go with the simple-but-slightly-frustrating setting of squashing everything for now, and keep an open mind to whether we want to come up with something better in the future.

[davidhewitt]

A quick heads-up: based on the above responses I'm going to change the setting now, as there's at least 3 PRs which are ready to merge but worth squashing. Thanks all 👍