Openj9: PROPOSAL: committers use "squash and merge" or "rebase and merge"

Created on 5 Oct 2017 · 10Comments · Source: eclipse/openj9

For a cleaner history, committers can use "squash and merge" https://github.com/blog/2141-squash-your-commits or "rebase and merge" https://github.com/blog/2243-rebase-and-merge-pull-requests to merge all commits. This means OpenJ9 will no longer have merge commits in the history, which reduces clutter and makes it easier to see what has changed between two SHAs.

If agreed, we can modify the repo configuration to disable creation of merge commits.

Source

pshipton

👎1

Most helpful comment

I personally like the merge commits and the information they provide.

Most developers generally squash their scratch commits before opening PRs so if there are multiple commits in a PR those commits break the work up into distinct changes. If you use the squash and merge option you lose this information. I would really like to keep this information.

If we really do not want the merge commits then the rebase and merge option is better than the squash and merge. The only downside to the rebase and merge option is that the SHAs I have in my personal fork will never exist in the actual repo. Most of the time this is not an issue since most developers create a new branch for each work item and then once their PR is merged the branch is deleted. In the case where you have a larger development item that might result in multiple PRs (release early and release often) based off of the same branch pulling the latest code from upstream becomes more difficult and error prone.

My vote would be to continue as is but if not then I would vote for rebase and merge.

charliegracie on 5 Oct 2017

👍6

All 10 comments

I personally like the merge commits and the information they provide.

My vote would be to continue as is but if not then I would vote for rebase and merge.

charliegracie on 5 Oct 2017

👍6

Agree with the points about using rebase and merge. The squash and merge would only be for times when the developer didn't squash but should have.

pshipton on 5 Oct 2017

We can leave all the options available, and the committer could squash or rebase and merge when appropriate, based on their judgement, instead of always creating a merge commit.

pshipton on 5 Oct 2017

Having a merge commit makes it easy to find the PR that the set of commits came from. This makes it easier to revert PRs when necessary. I don't know of another way to map a commit back to the PR that created it.

What's the purpose behind this proposal?

DanHeidinga on 5 Oct 2017

What's the purpose behind this proposal?

To reduce the clutter in the history and make it easier to see what changed between two builds.
When using squash and merge we saw the pull request number is added at the end of the commit message in brackets. However I see this doesn't occur with rebase and merge.

pshipton on 6 Oct 2017

Given experience monitoring OMR's history, I prefer the merge commits. It makes it clear when a set of commits was merged, allows finding the PR, and preserves logical commits.

Any feedback on this from other committers? @mstoodle, others?

DanHeidinga on 9 Oct 2017

The squash and merge would only be for times when the developer didn't squash but should have.

We as committers can make the request for developers to squash before merging. As @charliegracie mentioned most developers already do this anyway so it is not much of an issue.

fjeremic on 10 Oct 2017

I'd like to see committers more often ask that PRs be rebased. I find it more difficult to navigate history when the base of a PR and the merge commit are a week or more apart. That might also reduce the concerns that @pshipton mentioned.

keithc-ca on 10 Oct 2017

I think the review process (i.e. from PR created to PR "merged" into the master branch) can be divided into two basic activities: 1) ensuring that the code changes are proper and complete according to the standards of the project, and 2) ensuring the code is "merged" properly into the master branch.

The first part works best if contributors do not rewrite history (i.e. squash) any commits because it maintains all the review comments as new commits are only added to the source branch. So most of the review activity that we have been doing to date falls under this category.

The second part works best (to my mind) if contributors reorder and squash commits as appropriate to "fold" in the review comments (which, at that point, kills the review comments that are attached to code so some people really don't like it). This part is really about maintaining the cosmetics of our git history and I think is key in a project of this size. I am less concerned about maintaining the review comments tagged on the code, though it's nice if some higher level observation or conclusion is drawn that it's summarized on the PR so it can be seen forevermore.

I also prefer merge, or rebase-and-merge so that the clean commits are connected together according to their PR.