-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge or Squash+Merge? #373
Comments
Another view of Squash vs Merge is Squash destroyed all the raw data (all the little commits) while Merge preserve them. It's like publishing a nice clean scientific study/report but then destroying all the raw data that the study/report used, not very traceable ! Understandably, preserving a bunch of meaningless little commits like "fix travis-ci" is pointless. There are ways to only squash these (see https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History) instead of squashing everything. It follows that we should also write easily understandable commits if we are going to preserve them. This just boils down to writing commits that are not too big (cognitive size, not number of line change size) and the commit message should include the "why" the commit was needed. The "how" and "what" is basically the diff of the commit so we don't really have to describe these in details again. An example of a well crafted commit is the guideline from the Linux kernel: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes |
Another case where squash merge does damage is when we reference to a specific commit (ex: https://github.com/bird-house/flyingpigeon/blob/c97ebca73792c1fb0c32d7b6079385fcd84a73b0/Makefile#L57) a squash merge will destroy that commit and make our reference invalid. Read comment for further explanations bird-house/flyingpigeon#336 (comment). |
Then recommendation is to avoid squashing. Closing. |
Just for documentation, another case where squash merge will create problem: release PR. If we squash merge a release PR, the commit referenced by the release tag created in the PR will not be part of |
This follows from a discussion between myself, @aulemahal and @tlvu earlier today:
For the past few months, I've set the default setting to use squash and merge to deal with the problem (more often on my side) of dealing with lots of little annoying commits to deal with Travis CI builds, be they for triggering a new set of builds or correcting a small error in a yaml file. I'd rather not pollute the commit history with "travis fix" over and over again.
The issue with squashing commits by default is that we lose the information relating to individual changes within a merged PR. It's difficult to determine the reasoning behind a changed line if the history for that specific change is not preserved. In our current setup, we can return to see a specific change by noting the PR and then returning to GitHub and examining the provenance of changes, but let's say we moved platforms; There would be no way to know why something was written as such.
I'm opening a discussion point on this to come up with some guidelines for us to know when squashing is an allowable practice and maybe for us to re-examine the degree of information we place in our commit messages (something that @tlvu has brought up before).
The text was updated successfully, but these errors were encountered: