Proposal: Use squash-and-merge strategy for PRs

[AetherUnbound]

I would like to propose that we as a team adopt a policy to Squash-and-Merge PRs, rather than create merge commits via PRs. The intention of this change is to streamline our git history and make changes easier to track over time.

Pros

1. Git history becomes linear. When using merge commits, the git history can quickly become inscrutable. Changes become hard to track, and points of entry from various branches into the main branch are hard to pin down. A clean git history makes it easy to see when a change was introduced, and what its context was. This can significantly reduce the time it takes to track down when a bug might have been introduced. I've also used it on several occasions to correlate changes in infrastructure performance (e.g. DAG processing time, memory usage, etc) with code changes, sometimes weeks/months after the changes are made.
2. Promotes small changes that affect one aspect. It can be very easy to "just throw in some changes" to a PR, particularly if the individual commits will stick around once the branch is merged. A squash-and-merge police promotes PRs that affect a specific aspect of the code and using commit messages that follow the best practices outlined in our pull request template.
3. Reduces confusion. Merge commits can become confusing to pick apart. This strategy is best paired with frequent rebases before squash-and-merge.
4. Commits represent an approved change. When looking at the git history for a branch merged via merge commit, it's possible that one might see a number of intermediate commits that occurred during the development process. These commits may represent a "work in progress", and do not fully encapsulate the merged changes. Said another way, it's possible that we might end up with a dozen "Fix tests" commits in our git history.

Cons

1. Loss of individual commit history. With squash-and-merge, all history on a given branch is compressed into a single commit. This means that the commits within a branch will disappear once that branch is squashed-and-merged. If the distinction within the branch is important, that information will be lost.
2. Works best with rebasing, which rewrites history. Merge commits ensure the preservation of the original commits history. A commit, once made, is not altered; that history remains forever. The con here affects a specific divide: do we care more about the development process (i.e. not altering commits), or do we care more about code changes that affect production (i.e. rebasing + squash-and-merge).
3. New behavior to get used to. Since merge commits are the default behavior, changing to squash-and-merge means getting familiary with a new merge strategy. This can take time and resources.

All that said, I'm in favor of squash-and-merge 😁 I can't count the number of times a clean git history has saved my hide. What do y'all think? Which strategy would you prefer?

[zackkrida]

I've always been a squash n' merge n' rebase fan, some folks on the team have been against it and shared the following resources against it:

• https://stackoverflow.com/questions/26999930/git-merge-to-squash-or-not-to-squash
• https://felixmoessbauer.com/blog-reader/why-git-squash-merges-are-bad.html
• https://medium.com/bananatag-engineering-blog/squash-merges-are-evil-171f55139c51
• https://myst729.github.io/posts/2019/on-merging-pull-requests/

[AetherUnbound]

Thanks for these resources! The author of article 2 brings up that squash and merge makes "all changes are owned by the member who merges". I believe this is no longer the case thanks to the co-authored-by directive. Here's an example: OrcaCollective/not911.me@df5473a. Article 3 appears to be mostly about how squash merges are bad in Bitbucket - GitHub takes care of a many of the issues they mention. Article 4 mentions that one reason to keep the history is to see implementations that didn't work - I think that information is perfectly suitable for the PR description, which will exist in perpetuity after the branch is squashed.

I think that it really boils down to:

Do we care more about the development process (i.e. not altering commits) or do we care more about code changes that affect production (i.e. rebasing + squash-and-merge)

In my experience, ease in navigating the repo post-deployment has proved far more useful than retaining all the history of every commit. I'm interested to hear others' thoughts!

[zackkrida]

I agree, and appreciate how you've distilled the issue down to:

Do we care more about the development process (i.e. not altering commits) or do we care more about code changes that affect production (i.e. rebasing + squash-and-merge)

Let's wait for others to chime in. Thanks again for raising this

[dhruvkb]

Petty consideration but squash and merge will reduce an individual's number of commits in the commit graph (e.g. https://github.com/WordPress/openverse/graphs/contributors).

[zackkrida]

@WordPress/openverse-developers

[sarayourfriend]

I'm a fan of squash-and-merge. It's also what is used in the other WP repos I've interacted with and keeps the main branch history reading more like a changelog rather than a series of disperate commits intertwined by time (which could include lots of "address PR comments" type commit messages unless we became really annoyingly picky with how contributors decide to organize their own PRs... which seems like something we don't want to do if we want to promote community growth 🙂)

[dhruvkb]

When working on a large change, sometimes I make successive PRs built on top of each other. This type of workflow would break down if the merged branch was squashed and the PRs targeting it broke down.

[AetherUnbound]

I think the only hiccup occurs when you have A <- B <- C, branch B is merged into A and then deleted: that automatically closes the B <- C PR. As long as the branch isn't deleted, even if the A <- B PR is merged then the B <- C will remain open an can be rebased.

[sarayourfriend]

Really? Maybe I did something different but in my experience if you have A <- B <- C if you merge B then github will automatically retarget C against A...

[AetherUnbound]

Ah, maybe it's worth testing! I don't remember this being the case, but it certainly could be

[zackkrida]

I just made branches a, b,and c, then merged b. Merging b also merged a, and retarged c to the main branch!

https://github.com/zackkrida/squish

[AetherUnbound]

Sweet! I may do the same test with WordPress/openverse-catalog#217 and WordPress/openverse-catalog#218 👀

[obulat]

Can we use squash-and-merge for merging small branches into the main or large feature branches?

Let me explain it using an example. Let's say we have one large feature branch, "Design Update". When individual PRs are merged into this feature branch, they are squashed. But when this whole "Design Update" branch is merged into main, the commits are kept as a merge commit? So that the individual PRs in the feature branch are kept in the project history?

[AetherUnbound]

That could work! If we have a long-running feature branch, we could also rebase all of the squashed commits onto main. This would preserve the individual PRs but keep the history linear. I'm fine with either, just mentioning an alternative 🙂

Edit: thinking on this more, I totally get why we might want a merge commit for some of the larger feature branches! I think that's a really solid approach.

[zackkrida]

How do people feel about the following general approach? I don't even think it has to be a rule but best practices would be nice to encourage here, especially as some folks are learning to work this way.

Large feature pull-requests with sub-pull requests

• The sub-features should be squashed before merging.
• The primary feature branch should be rebased to be kept up with the main branch.
• The branch should not be squashed when merged into master

Examples:

# ❌ Bad git log, too much functionality in one PR
# Sub-branches were squashed when added to the primary feature branch
# and the feature branch was also squashed when merged into main
a401d48c Add audio support to Openverse

# ❌ Bad git log, too granular
# Sub-branches were not squashed when added to the primary feature branch
a401d48c Fix typo in audio track translation
a401d48c Add margin-left to play/pause button
a401d48c Fix broken logic in audio state
# ....and so on

# ✅ Good git log, just the right level of detail
# Sub-branches were squashed when added to the primary feature branch
a401d48c Add view for single audio result
a401d48c Add audio files to the 'all' results view
# ....and so on

Small pull requests (Most common)

These are the most common types of pull requests. They implement a single feature that might modify a handful of files.

• The branch should be rebased to be kept up with the main branch.
• The branch should be squashed when merged into master

Examples

# ❌ Bad git log, too granular
5b90ff78 Make 'notification' and 'report-content' modules namespaced
9aa833dc Make 'nav' module namespaced
5b17365f Make 'bug-report' and 'provider' modules namespaced
89100e33 Make 'active-media' and 'attribution' modules namespaced
c0ff2fb5 Make 'abtest' module namespaced
# ....and so on

# ✅ Good git log, just the right level of detail
a401d48c Namespace Vuex modules

I think as @AetherUnbound has suggested in different words, our guiding principle here should be to make time travel through the repository, namely rolling back changes, as easy as possible. In local development this is facilitated by small commits, but in production it's made significantly easier by having features chunked into clean, distinct commits.

[krysal]

Count me in with the use of squash-and-merge! 😄 Lately, I've been already following the second workflow Zack describes, and though it can get messy and should be done carefully paying attention, as this falls in more advanced git knowledge (with rebase, cherry-pick, etc...), I do appreciate the result of having a cleaner git history, among the other good advantages that Madison said.

I have found myself in the situation of having to review an old intermediate commit a few times, and that was very helpful, but those have been very few, so it would be just a small loss.

Seems like major scenarios and concerns are addressed, the authoring problem would have been a big one, I didn't know about it ❗️ Good thing it's solved!

[zackkrida]

As I understand, we have consensus on implementing Squash & Merge 🎉 I have made this the default merge strategy on all repositories. Thank you so much @AetherUnbound for gathering everyone to discuss this topic.

[zackkrida]

I'm going with the approach of actively disabling other merge commits (if a coding standard isn't enforced, it isn't a coding standard, is it?) If we run into issues we can enable them and encourage squashing manually, but I suspect that won't be necessary.