Open discussion about gem5's PR Merge Policy: Should we use Merge Squash or Rebase?

[BobbyRBruce]

TL;DR: open discussion about which merge policy to use. I advocate enabling Squash merge for certain cases, particular 1-commit PRs (in this case a squash merge is essentially a rebase), and keeping Merge for cases of multiple commits. I argue that Rebase Merge doesn't offer us anything more.

I intend this to be an open discussion. We've been using the GitHub PR model for just under 3 months now and Ive found the most contentious part is how we merge changes to the main gem5 development branch. I'll log here my observations on the pros and cons of each approach and propose my improvement, though this is just me dumping my thoughts. I'm open to altnerative ideas and feedback.

To remind everyone, we can only have a combination of these three merge policies:

• Merge : A merge commit is created, thus merging the PR commits into the develop branch.
• Squash Merge: The commits in the PR are squashed in a single commit then rebased on top of the develop branch. There is no merge commit.
• Rebase Merge: The PR commits are rebased on top of develop. You can think of it as cherry-picking the commits to the top of develop. There is no merge commit.

Merge (CURRENT)

Right now we are using Merge. Here's are problems I've noticed with using just Merge:

1. Most pull-requests comprise of just a single commit. In this case GitHub will put that commit's header and body as the PR title and description by default. We also have our default Merge commit message setup as the PR title and description. If you think it through this results in the merge commit message being almost identical to that of the single commit it is merging (note: the merge commit does append GitHub's #<pr id> to the end PR title). This results in a git log which looks like a lot of duplicates. For example his commit c0db065c and 3520c836 look identical at first glance, but one is the merge commit and the other the actual commit.
2. The merge commits created by Merge have their authors set as the person who did the merge. Our policy is that Maintainers are responsible for merging and are the only ones who have permission to do so create this merge. This is our way of policing changes. This authorship may be seen as unfair credit in some cases. This is particularly annoying with the "duplicate commit message" issue mentioned in 1. For example, looking at this commit: 8d47cda it appears that I am author. However, this is just the merge commit for the change. If you click through to the PR arch-x86: Fix wrong x86 assembly #251 you can see the actual commit and the real author of the change.
3. There's no automatic enforcement of commit standards for the merge commit. The PR title and description are used as default merge commit header and body. The maintainer who creates the merge commit may alter this, but it's a manual process. GitHub automatically line-wraps the body but the header line length can get long and there's no rejection of merges which don't start with tags. We cannot run our checks on the merge commit because it's only created when the merge occurs.
4. Due to the commit duplication issue mentioned in 1. and 2., identical Change IDs can exist in multiple commits.

There's ultimately nothing we can do about (2.) if we are to use merge. If we create a merge commit someone needs to be the author and there's no option to cede this to anyone else. (1.) and (3.) regard the merge commit message default. We do have options here. The default merge commit messages can either be set to:

• Default Message : Takes the format a header set to "Merge pull request from " and the body set to the pull request title.
• Default to PR title : The PR title is used plus the PR ID. E.g.: arch-x86: Fix wrong x86 assembly (251)
• Default to PR title and description (Current) : The PR title is used and the PR description is used as the merge commit's body.

The "Default Message" would it clear we're dealing with a merge commit and not feel we have so many "duplicates" in the repo (ergo addressing 1.). To me anything that stops the "duplicate commit message" issue from happening is an advantage. "Default message" is also likely to be too long, thereby addressing some of (3.). However, it does mean the contributor's description in the PR is not logged in git. This seems desirable at least for cases where a PR consists of multiple commits and we want the merge commit to explain how these patches work together. The "Default to PR title" doesn't have many advantages and the "Default to PR title and descriptions "is what we're using now and has already been discussed.

While these are just default messages, and Maintainers are free to edit them as they wish prior to merging, I think we have to expect most of us to go with the default that is presented. Ultimately, we can only chose one of these three defaults with Merge. That's it really.

Squash Merge

We tried squash merge for a while and there were some complaints:

1. It can create messy git logs. We had our Squash Merges setup with "PR title and squash commit description" as our default merge commit message. Squash commits descriptions are just messy. The constituent messages concatenated together and it's just ugly to see and can make finding things in the log difficult.
2. If there are multiple commits with different authors in a PR, the authors of the commits become co-authors in the commit body on the squashed commit. This is non-standard and won't mean much to, say, git blame but GitHub does recognize this as authorship. The sole author goes to whoever the first commit in the PR is authored to.
3. You could end up with multiple Gerrit Change IDs in the single commit if your PR contains multiple commits with with Change IDs.

(1.) and (3.) may be resolvable. We can use "Use PR title and description" for Squash Merge. This would mean we don't get the squashes of the commit messages like we did before. How authors would feel about all their commits headers and descriptions being erased in favor of the PR description is unknown.

Squash Merge does have a few advantages though. I found it nice that:

• Each commit on develop had a one-to-one relationship to a PR.
• The squashed merge commit message contains a reference to the PR by default which means it's easy backtrack to the review page easier.
• Users could push commits to the PR during the review process without worry. Things such as "fixing type", "addressing review commit X", etc. and not worry about polluting the log too much (though by default these would exist in the in the squashed commit message body). We get around this now by using force pushes.
• In the case the PR consists of just one commit, our GitHub repo is setup to have the PR title and description default set to the commit header and description (only the GitHub PR ID is added). In effect, in the lone-commit case, Squash Merge is just a rebase to the develop branch.

Rebase Merge

Rebase merge has been proposed before. It is the approach most similar to the Gerrit system, but I've been hesitant to use it for a a couple of reasons:

1. If you have a PR consisting of multiple changes, rebasing with conflicts can a nightmare to deal with. On Gerrit we had too many people lose code in rebase failures. Merging means the user only needs to come up with a single patch to fix the whole PR cleanly. Rebasing requires thinking how every commit needs to be individually changed as the rebase iteratively goes through them.
2. You don't get a merge commit with a Rebase Merge so we can't group changes and provide overarching context. At the level of our git log, they are just a series of commits with nothing linking them or explaining why they were all added together.

The advantage is we get a clean git history. The reason it was advocated for in the past is it avoids the creation of commits with identical changeIDs which can happen due to the aforementioned default PR description policy and default merge commit message policy causing a "duplicate commit message" when a PR just has one commit. However, I think we can find ways around that which I explain below.

** My preference: Merge for many, Squash Merge for one. **

I thing the following would be best and solves a lot of the problems we're having:

• Squash Merge, using the PR title and PR description as commit message default, for single commit PRs: Most of the issues regarding Merge are in the case where the PR contains a single commit. In this case we just want to rebase that change on top of develop. Rebasing is only bad in the case we have multiple commits. Creating a separate merge commit a single commit is just unreasonable. As previously mentioned, doing a Squash Merge on a single commit, while enforcing a policy where we copy the PR title and PR description to the commit message is effectively a rebase. It also has a small bonus in that it adds the PR ID to the commit header. Problems with Squash Merge's only occur when the PR has multiple commits.
• Merge, using the PR title and PR description as commit message default, for multiple commit PRs: In the case where a PR consists of multiple commits, Merge makes sense. The merge commit containing the PR description gives more context to what a merge of changes actually means. This is good for explaining big features that span many commits.

Given those two cases I can't think of what enabling Rebase Merge gives us. If it's a single commit, it's superseded by "Squash Merge". In the case of having multiple commits we miss out on having merge commits which can provide greater context as well as a link back to the PR. Rebase Merge also means we may have to deal with messy rebase conflict. So I believe Merge is better in this case.

The issues with Gerrit ID are also lessened here. Those were only an issue in the case where a Merge was used for a PR with a single commit, thus resulting in two patches with the same ChangeID, or when Squash Merge was used on a PR with multiple commits, ergo creating a commit with multiple ChangeIDs.

This doesn't solve all our problems but I think it is a step in a better direction. I would hope if this were setup maintainers would understand which merge is better to use in what circumstance.

[odanrc]

This is a good summary of the possibilities, thanks for bringing this discussion up, Bobby!

What do you mean by ¨On Gerrit we had too many people lose code in rebase failures.¨?

I like the rebase strategy exactly because of what you list as cons:

• Many commits may need to be individually fixed on conflicts, but each commit is a logical change (in theory), so fixing things is typically simple. In contrast, fixing conflicts when there are multiple commits in your mind is much more complicated.
• It doesn´t generate a merge conflict grouping the changes to inform the overall goal of the commits; however, neither do the other strategies: they group the commits together, but the merger is responsible for overriding the automatically generated merge commit to provide the goal of the group of changes. Would it have been easier to direct the merge commit to people that have asked me what were the commits they needed to cherry pick in order to have cache compression in their local repos? Probably slightly; I imagine this is more important for changes that don´t have the author around for long. Will someone take the time to write a meaningful description of the PRs, or will the description of the PR´s goal be as good as nothing?
• All commits stay around individually, so they can be studied (¨How did the author progress to implement this goal? I can replicate this approach to implement my similar goal?¨) and reverted easily.

I am not a fan of merge commits, as, for me, they just clutter the log (specially when each PR is composed of few commits) generally bringing little information to the table (see above), and decentralizes the implementation (the fixes to conflicts applicable to each specific commit are squashed under the - unrelated - merge commit).

Squash merges complicate reverting to fix bugs (i.e., an all-or-nothing approach) and hides knowledge, which IMHO makes it awful. Your proposed solution does not have the drawbacks I mention of squash merge... because it behaves the same as a rebase merge.

Overall, I prefer rebase merges for the reasons above, but your proposal is better than the current scenario.

[BobbyRBruce]

What do you mean by ¨On Gerrit we had too many people lose code in rebase failures.¨?

Rebasing your changing on top of develop mean you need to resolve conflicts in every commit. For instance you have commits A, B, and C in your PR. If develop changes in a way that interferes with A the rebase will force you to resolve conflicts in A. Of note you are literally changing the code changes in A to do this. Now if changing A interferes with B then you need to resolve the conflicts in B, and ditto for C. I've found numerous times that this can cause confusion and frequently people end losing code in this process. As you are literally changing the commits directly there's sometimes no easy way to get those code-changes back. I.e., you can't "revert" a bad rebase as easily as you can revert a merge.

Merging is considerably easier and much more intuitive. You have commits A, B and C in your PR and there's changes on develop which interfere. All you need to do is create merge commit D which resolves the conflicts. If you don't resolve these conflicts correctly, it's fine, you just revert the merge commit and try again. The changes in A, B, and C are maintained.

• Many commits may need to be individually fixed on conflicts, but each commit is a logical change (in theory), so fixing things is typically simple. In contrast, fixing conflicts when there are multiple commits in your mind is much more complicated.

You can always rebase locally to fix conflicts or do anything you want. Regardless of any PR merge strategy rebasing is still an option for you as a contributor. I just don't think it should be forced or required and I still don't see much benefit in it being a policy. As outlined in my original post, it has no real advantages. Commits still make sense even if you need the inclusion of a merge commit to have them work with the develop branch. If you really need to revert a change you can always revert the merge to get rid of the entire PR. I'll explain more on my thinking about this a couple paragraphs down.

• It doesn´t generate a merge conflict grouping the changes to inform the overall goal of the commits; however, neither do the other strategies: they group the commits together, but the merger is responsible for overriding the automatically generated merge commit to provide the goal of the group of changes. Would it have been easier to direct the merge commit to people that have asked me what were the commits they needed to cherry pick in order to have cache compression in their local repos? Probably slightly; I imagine this is more important for changes that don´t have the author around for long. Will someone take the time to write a meaningful description of the PRs, or will the description of the PR´s goal be as good as nothing?

The content of the merge commit, at present, is defaulted to the PR title and description. In this case the creator of the PR decides on the descriptive "grouping" of the commits. The merger may change this if he wishes and override this default though I've rarely done this as, in cases of multiple commits the PR creator has to specify the PR title and description and therefore is forced to describe the PR contents.

• All commits stay around individually, so they can be studied (¨How did the author progress to implement this goal? I can replicate this approach to implement my similar goal?¨) and reverted easily.

I think this still mostly works with my proposed strategy.

• Case 1: Squash Merge on single commit PRs: This works how you describe. You can see the contents of the commit and revert as needed.
• Case 2: Merge on a multiple commit PR: In this case the commits in this PR are likely very interconnected. At the very least they should be otherwise they should be different PRs. Take this as an example: python,util: Add Python MyPy Stubgen to enable Pylance IntelliSense #307. Here the PR contains 3 commits that all are needed to implement the Stubgen feature. If i want this all gone in it's entirety I'd revert the merge that's created. There, gone, it's like the PR never existed. If we want to revert an individual commit, say 39f0bcd, then we could but in this case, why would we? It'd break feature the PR implements. An issue we had with Gerrit is it was hard to revert a relation-chain. You had to individually revert every commit in the chain. In the case where a merge fixes conflicts will be more annoying to deal with if we wish to revert but it's really no different than trying to revert an old commit which has been altered by another commit later in the branch.

[powerjg]

@BobbyRBruce, could you update your original post with a "tl;dr" at the top that quickly summarizes your proposal. I have to admit that I didn't realize what your proposal was until I read this response (granted, I'm very lazy, so it's on me 😆).

[odanrc]

You and Jason brought up good points, so I am leaning towards your solution, Bobby.

[BobbyRBruce]

I've added a TL;DR on my post

[abmerop]

I have a slight preference for rebase merge simply because this is the most similar to what gerrit was doing, but not really any strong preferences either way. What I have seen so far:

• Merge commits clutter the log, as has already been mentioned
• Merge commits are more difficult to revert than a rebased commit
• I think if git can create a merge commit, it should be able to automatically rebase too, right? If so, rebase merge doesn't sound that difficult.
• Not a huge fan of squash merge, especially when the commits are things like "implement A. Fix comment in A. Etc." and the commit message makes no sense outside of the PR.

That said, I agree with the proposal to enable another submit option for single commit PRs which will help with the log clutter. Squash/rebase merge seem to be identical in this situation to me.

[powerjg]

One small thing from my point of view... when doing a "rebase always" policy with more than 1 commit in the relation chain (PR in github nomenclature) is practically impossible to revert. Often, if you bisect and find a particular commit to be the problem one commit cannot be reverted without breaking other things. Alternative, reverting the entire relation chain is not straightforward, especially if there are commits made afterward.

However, reverting squashed changes is quite easy.

I don't know how easy/difficult it is to revert a merged PR, but my bet is that it's easier than a rebased relation chain.

Note: I agree that for a single commit rebase makes more sense than merge.

Finally, IMO, "clean history" should not be a main goal. It's nice, but in practice not used nearly as often as pulling updates and merging new commits. We should optimize for the common case (i.e., our users), not the uncommon case (e.g., a minority of developers).

[BobbyRBruce]

It's very easy to revert a PR. I just did it here: #313.

Via the github.com UI you can click "revert" on any merged PR and it creates the revert PR for you.

[powerjg]

I wonder how this works for longer relation chains and how it compares between merges and rebases. I assume (and I assert above) that it is going to be easier with merge, but I don't actually have any data 😆

[abmerop]

It's not impossible to revert. I did something similar to #313, but I couldn't just do git revert SHA that's what I mean. It wants to know about the parent merge commit.

[BobbyRBruce]

git revert --merge 1 <sha of merge> will work. Though you can just go to the PR page and revert it.

[abmerop]

Oh, that's neat. Thanks, I am still familiarizing myself with the github web interfaces. Makes sense someone else though about it

[giactra]

Many thanks Bobby for the discussion, I believe it is a very detailed overview of the pros and cons of every approach. I honestly have the feeling that distinguishing between a multi-commit and a single-commit PR is a great idea (and I presume GitHub allows you to treat the differently when accepting the PR).

When you say

As previously mentioned, doing a Squash Merge on a single commit, while enforcing a policy where we copy the PR title and PR description to the commit message is effectively a rebase.

Does it mean that it is possible to configure Squash Merge to do a Squash and Rebase?
It would be nice if in the single commit PR case we could distinguishing on whether there is a merge conflict or not. If there is, then the user should fix it and then do a Squash Merge. If there isn't, there is no reason for creating non-linear history and the maintainer could simply do a Rebase Merge.

So my questions are:

1. Does this make sense to you?
2. Is it possible to configure GitHub in such way?
3. If not, can't we leave all 3 options available to the maintainers and let them act accordingly?

[BobbyRBruce]

A squash merge is a Squash and rebase. All GitHub does for squash merges is squash all the commits in the PR then rebase that commit on top of the branch. The scenario you describe works just fine and is an advantage of re-introducing squash merges.

For example, a contributor has a single commit PR. In this case the PR title and description will be that of the commit by default. They then fix something wrong in the PR by adding a couple new patches to the PR. If the PR has conflicts to resolve they can add more patches yet to resolve it. The contributor then thinks the description of this change isn't good enough, so they change the PR title a bit and the PR description. When it's ready to be merged the Maintainer selects "Squash Merge" and all these are squashed into a single commit with the commit header defaulting to the PR title and commit description being that of the PR. It's then rebased on top of the develop branch.

Right now we force contributors to rebase their changes to keep thier commits clean, but in this single-commit case, we can be more lenient as the squash will keep things clean.

[BobbyRBruce]

I want to note here something I didn't outline in our original post, though @giactra touched upon it.

In issues where the PR is a single commit, reviewers may add "suggestions" to his change (you may have seen this in reviews). The contributor can then click "add suggestion" which will create a new commit in the PR adding those changes. This is a great use-case for Squash Merge.

[BobbyRBruce]

Related to this, these commits, right now, would require Change-IDs to pass our CI tests. I've opened Discussion #324 to discuss dropping the Change-ID requirement.

[BobbyRBruce]

I'd enabled Squash Merges and regular Merge in-line with this policy for https://github.com/gem5/website, https://github.com/gem5/gem5-resources, and https://github.com/gem5/gem5-resources-website. I'll leave these repos in that state for a while to see if that works. There before doing it on the main repo.

[BobbyRBruce]

I believe this conversation has had it's day.

Our merge system is still in-perfect, but I do not believe there is a merge system available which can satisfy all the criteria we have. Squash Merge with Merge has worked since introducing them a few months ago. I'm closing this Discussion for now.