Can we enforce squash merge for all commits?

[mjsabby]

Great job on the new repo.

Can we enforce squash merge on ALL commits? Users who have legitimate need to have any other kind of commit can request an admin to do so.

Squash merge produces linear history which makes the project history much more approachable than the non-linear history that is created when using other commit types.

Thoughts?

[mjsabby]

cc @jkotas @stephentoub

[Suchiman]

Or rebase fast-forward-merge which also ensures linear history.

[Gnbrkm41]

CoreCLR already had this documented as part of their workflow docs (https://github.com/dotnet/coreclr/blob/master/Documentation/project-docs/contributing-workflow.md), stating:

Use "Squash and Merge" by default for individual contributions unless requested by the PR author. Do so, even if the PR contains only one commit. It creates a simpler history than "Create a Merge Commit". Reasons that PR authors may request "Merge and Commit" may include (but are not limited to):

• The change is easier to understand as a series of focused commits. Each commit in the series must be buildable so as not to break git bisect.
• Contributor is using an e-mail address other than the primary GitHub address and wants that preserved in the history. Contributor must be willing to squash the commits manually before acceptance.

I usually have seen CoreFX PRs being squashed as well.

[jkotas]

This is how the repo is configured:

We had discussion about the pros and cons and this is the setting we have agreed upon.

[mjsabby]

If you allow merge commits, developers will forget and the history goal (keeping it smaller and cleaner) will be impacted. I'm not sure why merge commits need to be allowed for everyone. Is there a reason why it can't be a select few who will need to change this setting temporarily to allow a merge commit to pass through?

[Suchiman]

Odd that rebase merging is not alllowed, since it produces less commits than merge commit strategy, does not produce a commit history that is interleaved with commits not from that source branch and github shows you nontheless from which PR those commits come. Merge commits make history hard to read (for me, at least, i browse through commits, see a merge, read it, and find it's individual commits scattered across the next commit pages depending on how long the PR went), moving all projects into a mono repository certainly won't help that.

While merge commits are useful to adhere to the standard git porting strategy (commit into the oldest branch you want to support that commit in, then merge in upstream direction), for PR's, squash merging (for unclean history) and rebase merging (for cleanly separated commits) makes more sense IMO.

@jkotas I suppose the pros and cons weren't a public discussion?

[jkotas]

@jeffschwMSFT @jaredpar Could you please summarize the repo v-team discussion on this topic?

[jeffschwMSFT]

Could you please summarize the repo v-team discussion on this topic?

The contributing repos all had squash on merge policies already - though these were enforced by norm and not by disabling the actions. In our previous repos we had active members of the team that would help educate people to ensure to squash on merge - those same people are likely just as passionate in dotnet/runtime.

We discussed a policy where we disallowed all but squash on merge. But legitimate scenarios came up - like cross branch merges (which happen on occasion). The outcome was to leave the details as is and see if disabling was required. We are relying on people doing the right thing.

We disabled rebase on merge was proactively disabled as no concrete scenario came up during the discussion.

[jaredpar]

Can we enforce squash merge on ALL commits? Users who have legitimate need to have any other kind of commit can request an admin to do so.

This is not really feasible given the current set of features provided by GitHub.

As @jeffschwMSFT pointed out there are legitimate reasons for traditional merge to occur. Specifically feature and service branch merges. These happen frequently enough that we need a decent path for doing them.

The only toggle GitHub provides here is to disable merge entirely for the repository. This isn't a setting that can be overriden by an admin / maintainer on an individual pull request. Basically there is no disabled for everyone but admin. Instead they have to reset the policy for the repository, merge the change and then reset the policy back again. That's too much process given the regularity of these merges.

If GitHub ever gave us a way to disable, but allow admin / maintainer override, then I think we'd consider opting into that.

[dagood]

If GitHub ever gave us a way to disable, but allow admin / maintainer override, then I think we'd consider opting into that.

Did we reach a conclusion about whether using git commands to push merge commits was a reasonable alternative?

I seem to remember we didn't know yet if GitHub blocked pushes in this scenario. I just checked, and pushing a merge commit does seem to work even with that type of merge button disabled. (I disabled it on a fork and pushed a merge commit to resolve an active PR. I didn't have any branch protections on, but I imagine that simply limits who can do it depending on how the protections are configured.)

A few thoughts why we might not be ok with manual git pushes:

• It's a hassle for people doing branch merges. (Although less of one than asking an admin to go to the settings and flip the switch for a few minutes.)
• It's a place to potentially mess up.

[jaredpar]

@dagood

It's a hassle for people doing branch merges.

This is why I'm against doing this. We're essentially saying "GitHub doesn't provide the features to do this correctly so lets hack up a solution". Instead of doing this I'd prefer to push on getting GitHub to do the correct feature.

This isn't a case where doing the wrong thing diminishes the quality of the repository (and hence something where more pain would be a trade off worth considering). Instead this is a question of style and preference. I don't want to make the jobs of the everyday engineer, particularly the ones dealing with tricky infrastructure issues, worse just because another engineer might violate a merge style preference.

[dagood]

This isn't a case where doing the wrong thing diminishes the quality of the repository (and hence something where more pain would be a trade off worth considering).

This issue makes an argument that it will over time diminish the quality of the repo history, but I do agree that with norms and properly educating everyone with merge permission, I wouldn't expect to get a significant rate of bad merges (and eventually hopefully we get the GitHub feature).

[mjsabby]

The issue makes this argument based on my experience with the coreclr repo earlier this year when I was bisecting numerous times per week.

I'm not sure this is a hack. This is how most products who want a linear history work. Yes, GitHub could go further but I'm not sure the burden is so high that when you want to merge a commit you uncheck that box.

Would it be possible to start off the repo with only squash merge and see how much pain it causes and then decide if it's too painful? It seems like we've already decided that the pain is going to be high without trying the feature.

[jaredpar]

Would it be possible to start off the repo with only squash merge and see how much pain it causes and then decide if it's too painful? It seems like we've already decided that the pain is going to be high without trying the feature.

At this point no. If GitHub doesn't want to support the ability to selectively override the merge policy on PRs then I don't want to force a non-standard system on the infrastructure engineers.

If you feel strongly about this then I think the best course of action is to push GitHub to make their policy here more flexible and allow admin override at the merge point.

[stephentoub]

Closing as discussed. We can re-evaluate as we get more experience with what goes right and wrong in the repo. Thanks!

[jaredpar]

In the time since we initially discussed this issue the UI options in GitHub have changed. It is now possible to enable linear history but allow for admins to override at the point of the merge. This morning we tested out the option on PR #38313. The result when an admin looks at the merge options is:

When the admin selects the "Create merge commit" option we get the nice red background which is typical GitHub UI for "you're about to do an admin action"

When a non-admin looks at the same PR they can only "Squash and merge"

Given that we're going to enable this in the repo for now and see how it goes. If it ends up causing unwanted friction we'll re-evaluate.

[stephentoub]

Seems reasonable. Thanks.

[danmoseley]

Sounds good to me.

[mjsabby]

Excellent, linear history FTW!

[jaredpar]

Excellent, linear history FTW!

It's not linear history though, it's less non-linear history. There will always be merge commits in our repo because it's necessary to support our workflows. Nothing about our workflow has changed.

The only change here is that we have better enforcement for our desired model: linear history for most code changes but merge commits to support arts of our workflow.