[Git Workflow] Use GitHub "squash and merge" functionality

[Ivorforce]

Describe the project you are working on

Godot changes requiring PRs.

Describe the problem or limitation you are having in your project

The git PR workflow currently requires PR authors to amend their commits, or squash before commit. I want to challenge this policy.

Before we get into this, while, a lot can be said about squashing in general, I'm not here to challenge the squash-merge. Somebody else can do that, if they care enough. Godot maintainers may still use GitHub's squash-merge button when merging a PR:

This proposal is instead about requiring the authors to perform the squash and / or an amend workflow, as explained in the PR workflow policy.

Arguments

Alright, intro aside, let's list my reasons for this proposed change of policies:

Collaborative PR workflows

The most important argument first: Requiring an amend-workflow prevents multiple authors from collaborating effectively on a PR. It effectively turns a feature branch into a unistate filesystem, nullifying all the the benefits git gives you in the first place, for this branch.

This is especially problematic when multiple authors are contributing to the same branch (such as for uploading a quick hotfix), in the worst case possibly even leading to lost changes due to the requirement of force pushes.

Repeated Reviews

Maintainers re-reviewing a PR cannot easily see what has changed since last they looked at the PR (including diffs and commit messages), since the changelog has been intentionally destroyed.

This can obviously add friction to the review process, and decrease the quality of repeated reviews as potentially important changes could be skimmed.

GitHub Quick Edits

Requiring amend-workflow complicates quick fixes from the GitHub editors, as they cannot be configured to amend-commit:

GitHub Mentions

Git commits mentioning an Issue, such as Closes #xxx, will repeat-mention with amend commits:

Unnecessary repeated CI

When an author makes a subsequent commit instead of an amend-commit, the merge will trigger a new build of the GitHub runner, even though no changes have been made. This wastes time, preventing an early merge, and (though you may not care) releases just a tiny bit more CO2 into the atmosphere.

Merge Delay

It may happen that at the time of intended merge, the PR is not yet squashed, and the author is not reachable for changes (such as on godot-cpp recently, though this time it didn't hang on the squash specifically). The PR may be forgotten again, and another look is needed later, altogether slowing momentum.

Complication of workflow

Not all potential contributors to Godot are intimately familiar with amends, squashes and force-pushes. Requiring an amend workflow, which is very uncommon in the git world, increases the barrier to entry (as can be seen by the lengthy explainer article), and may sooner or later lead to mistakes or potential contributors giving up on their contributions early.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Godot should embrace normal git workflows for their PRs. If squash-merges are still desired, the GitHub squash-merge function should be used.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

N/A

If this enhancement will not be used often, can it be worked around with a few lines of script?

N/A

Is there a reason why this should be core and not an add-on in the asset library?

N/A

[clayjohn]

We have discussed this many times over the years and have agreed that we prefer our current workflow for 2 primary reasons:

1. A project of our scale requires a clean git history / git blame
2. Merge commits are very valuable for us

The "squashless merge" workflow makes the git history messy and unorganized and makes it nearly impossible to effectively use git bisect. Both are tools that are absolutely invaluable for us to be able to maintain this project. Giving those up should only be done if the benefits of doing so far outweigh the cost. I don't think you have made the case for that.

Collaborative PR workflows The most important argument first: Requiring an amend-workflow prevents multiple authors from collaborating effectively on a PR. It effectively turns a feature branch into a unistate filesystem, nullifying all the the benefits git gives you in the first place, for this branch.

This is especially problematic when multiple authors are contributing to the same branch (such as for uploading a quick hotfix), in the worst case possibly even leading to lost changes due to the requirement of force pushes.

This is a non issue. The squash can be done immediately before merging. In fact we often use a collaborative workflow like this to assist new contributors. Multiple commits are fine in a PR as long as they are squashed before merging.

Repeated Reviews Maintainers re-reviewing a PR cannot easily see what has changed since last they looked at the PR (including diffs and commit messages), since the changelog has been intentionally destroyed.

This can obviously add friction to the review process, and decrease the quality of repeated reviews as potentially important changes could be skimmed.

This is annoying, especially on large PRs. But again, the solution is simple (and something we often do already), make multiple commits during the review phase, then squash before merging. No need to throw out our entire workflow.

GitHub Quick Edits Requiring amend-workflow complicates quick fixes from the GitHub editors, as they cannot be configured to amend-commit:

Quick fixes are not suitable for code changes. Code needs to be tested before making a PR. This is not a workflow we encourage.

GitHub Mentions Git commits mentioning an Issue, such as Closes #xxx, will repeat-mention with amend commits:

We discourage people from mentioning issues, PRs, or commits inside commit messages anyway as it makes a mess in Github. I wouldn't sacrifice a good workflow for a bad one.

Unnecessary repeated CI When an author makes a subsequent commit instead of an amend-commit, the merge will trigger a new build of the GitHub runner, even though no changes have been made. This wastes time, preventing an early merge, and (though you may not care) releases just a tiny bit more CO2 into the atmosphere.

CI runs whether you add a new commit or whether you amend the current commit and force push. There is no difference here.

Merge Delay It may happen that at the time of intended merge, the PR is not yet squashed, and the author is not reachable for changes (such as https://github.com/godotengine/godot-cpp/pull/1566, though this time it didn't hang on the squash specifically). The PR may be forgotten again, and another look is needed later, altogether slowing momentum.

For important PRs, maintainers will typically do the squash + rebase themselves. But I agree, that this does add friction and delay.

Complication of workflow Not all potential contributors to Godot are intimately familiar with amends, squashes and force-pushes. Requiring an amend workflow, which is very uncommon in the git world, increases the barrier to entry (as can be seen by the lengthy explainer article), and may sooner or later lead to mistakes or potential contributors giving up on their contributions early.

In my opinion this is the biggest factor against using the squash workflow. However, its a pure tradeoff between having high coding standards and being accessible for newcomers. While I work very hard to make the engine as accessible to newcomers as possible, we can't sacrifice the quality of the engine to make it easier for them. To ensure Godot's continued success, we have to keep the code and the codebase high quality so that is maintainable in the years to come.

A messy codebase with an unusable git history and no ability to bisect will turn away way more contributors than requiring people to follow our interactive rebase tutorial (which, in my biased opinion, is extremely accessible)

Summary

While you raise some good points, I don't think any of them outweigh the benefit of having a clean git history and the ability to git bisect effectively. There is definitely added friction in the process for new contributors. But that friction is reduced as people contribute more. At the same time, I cannot overstate how important git history and git bisect are for us to maintain the engine long term. Giving those up should only be done if there is an overwhelmingly positive reason for doing so.

[Ivorforce]

Hi, thanks for the response!

I want to re-iterate that I'm not challenging the squash-merge in general. I'm just challenging that the squash be performed by the PR author. Having gone through your response, I think you've missed this fact, as I don't feel like my points have been addressed properly under this background.

We have discussed this many times over the years and have agreed that we prefer our current workflow for 2 primary reasons:

1. A project of our scale requires a clean git history / git blame
2. Merge commits are very valuable for us

The "squashless merge" workflow makes the git history messy and unorganized and makes it nearly impossible to effectively use git bisect. Both are tools that are absolutely invaluable for us to be able to maintain this project. Giving those up should only be done if the benefits of doing so far outweigh the cost. I don't think you have made the case for that.

Again, not challenging the squash-merge in general. Both of your arguments do not pertain to my proposal :)

Collaborative PR workflows The most important argument first: Requiring an amend-workflow prevents multiple authors from collaborating effectively on a PR. It effectively turns a feature branch into a unistate filesystem, nullifying all the the benefits git gives you in the first place, for this branch. This is especially problematic when multiple authors are contributing to the same branch (such as for uploading a quick hotfix), in the worst case possibly even leading to lost changes due to the requirement of force pushes.

This is a non issue. The squash can be done immediately before merging. In fact we often use a collaborative workflow like this to assist new contributors. Multiple commits are fine in a PR as long as they are squashed before merging.

... as could be done by the squash-merge github button.

This also ignores that many PRs are ready first and then ready later, when everything has been addressed and fixed. Plenty of PRs need quick fixups.

Repeated Reviews Maintainers re-reviewing a PR cannot easily see what has changed since last they looked at the PR (including diffs and commit messages), since the changelog has been intentionally destroyed. This can obviously add friction to the review process, and decrease the quality of repeated reviews as potentially important changes could be skimmed.

This is annoying, especially on large PRs. But again, the solution is simple (and something we often do already), make multiple commits during the review phase, then squash before merging. No need to throw out our entire workflow.

And again, easier to do by using the squash-merge button instead of letting the author do it.

GitHub Quick Edits Requiring amend-workflow complicates quick fixes from the GitHub editors, as they cannot be configured to amend-commit:

Quick fixes are not suitable for code changes. Code needs to be tested before making a PR. This is not a workflow we encourage.

Not all changes need to be tested before making a PR. I make plenty of PRs on the GitHub editor, especially those that do not change functionality, such as simple string edits. We have CI for a reason.

GitHub Mentions Git commits mentioning an Issue, such as Closes #xxx, will repeat-mention with amend commits:

We discourage people from mentioning issues, PRs, or commits inside commit messages anyway as it makes a mess in Github. I wouldn't sacrifice a good workflow for a bad one.

Sure, I disagree that this is a good decision, but it's not a hill i'm willing to die on.

Unnecessary repeated CI When an author makes a subsequent commit instead of an amend-commit, the merge will trigger a new build of the GitHub runner, even though no changes have been made. This wastes time, preventing an early merge, and (though you may not care) releases just a tiny bit more CO2 into the atmosphere.

CI runs whether you add a new commit or whether you amend the current commit and force push. There is no difference here.

Not when the final commit is just a squash, as would be the case always if authors use regular git workflows until just before the merge, as you propose.

Complication of workflow Not all potential contributors to Godot are intimately familiar with amends, squashes and force-pushes. Requiring an amend workflow, which is very uncommon in the git world, increases the barrier to entry (as can be seen by the lengthy explainer article), and may sooner or later lead to mistakes or potential contributors giving up on their contributions early.

In my opinion this is the biggest factor against using the squash workflow. However, its a pure tradeoff between having high coding standards and being accessible for newcomers. While I work very hard to make the engine as accessible to newcomers as possible, we can't sacrifice the quality of the engine to make it easier for them. To ensure Godot's continued success, we have to keep the code and the codebase high quality so that is maintainable in the years to come.

A messy codebase with an unusable git history and no ability to bisect will turn away way more contributors than requiring people to follow our interactive rebase tutorial (which, in my biased opinion, is extremely accessible)

Remember that my proposal, again, does not challenge the squash-merge. The end result will look the same in the git history.

[adamscott]

AFAIK, merge maintainers usually do batches when merging PRs, so using the GitHub button prevents that workflow. And that workflow is pretty much essential when working with a repo as big as Godot.

[Ivorforce]

AFAIK, merge maintainers usually do batches when merging PRs, so using the GitHub button prevents that workflow. And that workflow is pretty much essential when working with a repo as big as Godot.

This would be an interesting caveat. Can someone confirm that the GitHub squash-merge button prevents quick subsequent merges?

[adamscott]

AFAIK, merge maintainers usually do batches when merging PRs, so using the GitHub button prevents that workflow. And that workflow is pretty much essential when working with a repo as big as Godot.

This would be an interesting caveat. Can someone confirm that the GitHub squash-merge button prevents quick subsequent merges?

It's more that it prevents doing 15 main branch builds. With a batch-merge, there's only one build: for the latest commit in the branch. As you said, it helps saving resources (and prevents emitting too much CO2).

[clayjohn]

Squash merge isn't a viable alternative yet for two reasons:

1. It squashes the merge commit, so you lose the history of when a PR was actually merged. This can be problematic if a PR that is multiple months old is merged
2. Github just squashes all the commit messages together, so if someone has multiple commits with names like "fix a few bugs found in review" it ends up in the final commit message.

Github could fix both of these workflow problems, but AFAIK they haven't yet, so we still don't use squash on merge.

The end result will look the same in the git history.

That's not the case at all..

This would be an interesting caveat. Can someone confirm that the GitHub squash-merge button prevents quick subsequent merges?

Its a workflow difference, squash merge would run the CI for each merge. While a batch merge only runs the CI for each batch. Its not preventing subsequent merges, its just running the CI 25X more than is necessary.

An alternative proposal could be for maintainers to manually squash and rebase while doing a batch merge, but that puts more work on maintainers which means that there will be further delay in PR merges, and PRs will stagnate longer, which is the biggest complaint we get from new contributors

[KoBeWi]

Maintainers re-reviewing a PR cannot easily see what has changed since last they looked at the PR (including diffs and commit messages), since the changelog has been intentionally destroyed.

GitHub is capable of showing changes between force-pushes. Although it becomes unreadable if you do a rebase in the meantime.

Not all potential contributors to Godot are intimately familiar with amends, squashes and force-pushes.

From what I observed, many newcomer contributors are not even familiar with git. For those people, learning amends and squashing makes little difference if they have to learn git too. And people already familiar with git rarely have problem with this workflow.